| Message ID | 1375292543-7896-3-git-send-email-ttynkkynen@nvidia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hello. On 07/31/2013 09:42 PM, Tuomas Tynkkynen wrote: > From: Mikko Perttunen <mperttunen@nvidia.com> > Device tree entries for the three EHCI controllers on Tegra114. > Enables the the third controller (USB host) on Dalmore. I would have done the board patch separately from the SoC one. > Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> [...] > diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi > index abf6c40..2905145 100644 > --- a/arch/arm/boot/dts/tegra114.dtsi > +++ b/arch/arm/boot/dts/tegra114.dtsi > @@ -430,6 +430,68 @@ > status = "disable"; > }; > > + usb@7d000000 { > + compatible = "nvidia,tegra30-ehci", "usb-ehci"; > + reg = <0x7d000000 0x4000>; > + interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>; > + phy_type = "utmi"; > + clocks = <&tegra_car TEGRA114_CLK_USBD>; > + nvidia,phy = <&phy1>; > + status = "disabled"; > + }; > + > + phy1: usb-phy@7d000000 { At the same address as the previous node? > + compatible = "nvidia,tegra30-usb-phy"; > + reg = <0x7d000000 0x4000 0x7d000000 0x4000>; Hm, there must be some mistake: two similar register ranges. > + usb@7d008000 { > + compatible = "nvidia,tegra30-ehci", "usb-ehci"; > + reg = <0x7d008000 0x4000>; > + interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>; > + phy_type = "utmi"; > + clocks = <&tegra_car TEGRA114_CLK_USB3>; > + nvidia,phy = <&phy3>; > + status = "disabled"; > + }; > + > + phy3: usb-phy@7d008000 { Again at the same address as previous node? > + compatible = "nvidia,tegra30-usb-phy"; > + reg = <0x7d008000 0x4000 0x7d000000 0x4000>; Second range conflicts with previous nodes. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On 31/07/13 21:18, Sergei Shtylyov wrote: > Hello. > > On 07/31/2013 09:42 PM, Tuomas Tynkkynen wrote: > >> From: Mikko Perttunen <mperttunen@nvidia.com> > >> Device tree entries for the three EHCI controllers on Tegra114. >> Enables the the third controller (USB host) on Dalmore. > > I would have done the board patch separately from the SoC one. > >> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> > [...] > >> diff --git a/arch/arm/boot/dts/tegra114.dtsi >> b/arch/arm/boot/dts/tegra114.dtsi >> index abf6c40..2905145 100644 >> --- a/arch/arm/boot/dts/tegra114.dtsi >> +++ b/arch/arm/boot/dts/tegra114.dtsi >> @@ -430,6 +430,68 @@ >> status = "disable"; >> }; >> >> + usb@7d000000 { >> + compatible = "nvidia,tegra30-ehci", "usb-ehci"; >> + reg = <0x7d000000 0x4000>; >> + interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>; >> + phy_type = "utmi"; >> + clocks = <&tegra_car TEGRA114_CLK_USBD>; >> + nvidia,phy = <&phy1>; >> + status = "disabled"; >> + }; >> + >> + phy1: usb-phy@7d000000 { > > At the same address as the previous node? Yes. The first node is for the EHCI driver and the second for the PHY driver. There is some overlap in the exact registers used, so both drives map the whole USB controller block. > >> + compatible = "nvidia,tegra30-usb-phy"; >> + reg = <0x7d000000 0x4000 0x7d000000 0x4000>; > > Hm, there must be some mistake: two similar register ranges. The second range is used to configure the UTMI pad registers. All the UTMI pad registers are located in the first USB controller's range. >> + usb@7d008000 { >> + compatible = "nvidia,tegra30-ehci", "usb-ehci"; >> + reg = <0x7d008000 0x4000>; >> + interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>; >> + phy_type = "utmi"; >> + clocks = <&tegra_car TEGRA114_CLK_USB3>; >> + nvidia,phy = <&phy3>; >> + status = "disabled"; >> + }; >> + >> + phy3: usb-phy@7d008000 { > > Again at the same address as previous node? > >> + compatible = "nvidia,tegra30-usb-phy"; >> + reg = <0x7d008000 0x4000 0x7d000000 0x4000>; > > Second range conflicts with previous nodes. > > WBR, Sergei All these entries mapping the same address are expected. See arch/arm/boot/dts/tegra20.dtsi for an existing example of Tegra USB bindings. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 07/31/2013 11:31 PM, Tuomas Tynkkynen wrote: >>> From: Mikko Perttunen <mperttunen@nvidia.com> >>> Device tree entries for the three EHCI controllers on Tegra114. >>> Enables the the third controller (USB host) on Dalmore. >> I would have done the board patch separately from the SoC one. >>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> >> [...] >> >>> diff --git a/arch/arm/boot/dts/tegra114.dtsi >>> b/arch/arm/boot/dts/tegra114.dtsi >>> index abf6c40..2905145 100644 >>> --- a/arch/arm/boot/dts/tegra114.dtsi >>> +++ b/arch/arm/boot/dts/tegra114.dtsi >>> @@ -430,6 +430,68 @@ >>> status = "disable"; >>> }; >>> >>> + usb@7d000000 { >>> + compatible = "nvidia,tegra30-ehci", "usb-ehci"; >>> + reg = <0x7d000000 0x4000>; >>> + interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>; >>> + phy_type = "utmi"; >>> + clocks = <&tegra_car TEGRA114_CLK_USBD>; >>> + nvidia,phy = <&phy1>; >>> + status = "disabled"; >>> + }; >>> + >>> + phy1: usb-phy@7d000000 { >> At the same address as the previous node? > Yes. The first node is for the EHCI driver and the second for the PHY driver. > There is some overlap in the exact registers used, so both drives map the > whole USB controller block. That's really horrible design. >>> + compatible = "nvidia,tegra30-usb-phy"; >>> + reg = <0x7d000000 0x4000 0x7d000000 0x4000>; >> Hm, there must be some mistake: two similar register ranges. > The second range is used to configure the UTMI pad registers. All the UTMI pad > registers are located in the first USB controller's range. Which second range? This is one and the same range. >>> + usb@7d008000 { >>> + compatible = "nvidia,tegra30-ehci", "usb-ehci"; >>> + reg = <0x7d008000 0x4000>; >>> + interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>; >>> + phy_type = "utmi"; >>> + clocks = <&tegra_car TEGRA114_CLK_USB3>; >>> + nvidia,phy = <&phy3>; >>> + status = "disabled"; >>> + }; >>> + >>> + phy3: usb-phy@7d008000 { >> Again at the same address as previous node? >>> + compatible = "nvidia,tegra30-usb-phy"; >>> + reg = <0x7d008000 0x4000 0x7d000000 0x4000>; >> Second range conflicts with previous nodes. Are you sure it's valid? Are you sure it shouldn't be 0x7d008000? >> WBR, Sergei > All these entries mapping the same address are expected. See > arch/arm/boot/dts/tegra20.dtsi for an existing example of Tegra USB bindings. Don't they cause numerous resource conflicts while device nodes being instantiated as the platform devices? WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/31/2013 01:53 PM, Sergei Shtylyov wrote: > Hello. > > On 07/31/2013 11:31 PM, Tuomas Tynkkynen wrote: >>>> Device tree entries for the three EHCI controllers on Tegra114. >>>> Enables the the third controller (USB host) on Dalmore. >>>> diff --git a/arch/arm/boot/dts/tegra114.dtsi >>>> b/arch/arm/boot/dts/tegra114.dtsi >>>> index abf6c40..2905145 100644 >>>> --- a/arch/arm/boot/dts/tegra114.dtsi >>>> +++ b/arch/arm/boot/dts/tegra114.dtsi >>>> @@ -430,6 +430,68 @@ >>>> status = "disable"; >>>> }; >>>> >>>> + usb@7d000000 { >>>> + compatible = "nvidia,tegra30-ehci", "usb-ehci"; >>>> + reg = <0x7d000000 0x4000>; >>>> + interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>; >>>> + phy_type = "utmi"; >>>> + clocks = <&tegra_car TEGRA114_CLK_USBD>; >>>> + nvidia,phy = <&phy1>; >>>> + status = "disabled"; >>>> + }; >>>> + >>>> + phy1: usb-phy@7d000000 { > >>> At the same address as the previous node? > >> Yes. The first node is for the EHCI driver and the second for the PHY >> driver. >> There is some overlap in the exact registers used, so both drives map the >> whole USB controller block. > > That's really horrible design. Yup. Both USB PHY and EHCI controller registers really are interleaved in one range. >>>> + compatible = "nvidia,tegra30-usb-phy"; >>>> + reg = <0x7d000000 0x4000 0x7d000000 0x4000>; > >>> Hm, there must be some mistake: two similar register ranges. > >> The second range is used to configure the UTMI pad registers. All the >> UTMI pad >> registers are located in the first USB controller's range. > > Which second range? This is one and the same range. Some registers in the USB1 register range actually are "global" and relevant to all USB controllers. So: There are two 2-cell entries in that reg property. The first entry defines the registers for this USB PHY, and hence is unique for each USB PHY node. The second defines the registers for whichever USB PHY contains various shared registers across multiple PHYs, so is expected to be identical across all USB PHY DT nodes. Yes, the HW design really is this screwy. > Don't they cause numerous resource conflicts while device nodes being > instantiated as the platform devices? No; the driver knows that the HW is screwy and there's lots of register-range sharing going on, so it simply maps the registers, rather than reserving the physical address range and mapping it. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/01/2013 02:06 AM, Stephen Warren wrote: >>>>> Device tree entries for the three EHCI controllers on Tegra114. >>>>> Enables the the third controller (USB host) on Dalmore. >>>>> diff --git a/arch/arm/boot/dts/tegra114.dtsi >>>>> b/arch/arm/boot/dts/tegra114.dtsi >>>>> index abf6c40..2905145 100644 >>>>> --- a/arch/arm/boot/dts/tegra114.dtsi >>>>> +++ b/arch/arm/boot/dts/tegra114.dtsi >>>>> @@ -430,6 +430,68 @@ >>>>> status = "disable"; >>>>> }; >>>>> >>>>> + usb@7d000000 { >>>>> + compatible = "nvidia,tegra30-ehci", "usb-ehci"; >>>>> + reg = <0x7d000000 0x4000>; >>>>> + interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>; >>>>> + phy_type = "utmi"; >>>>> + clocks = <&tegra_car TEGRA114_CLK_USBD>; >>>>> + nvidia,phy = <&phy1>; >>>>> + status = "disabled"; >>>>> + }; >>>>> + >>>>> + phy1: usb-phy@7d000000 { >>>> At the same address as the previous node? >>> Yes. The first node is for the EHCI driver and the second for the PHY >>> driver. >>> There is some overlap in the exact registers used, so both drives map the >>> whole USB controller block. >> That's really horrible design. > Yup. Both USB PHY and EHCI controller registers really are interleaved > in one range. But the standard EHCI register space has no holes IIRC, so they can't be really that much interleaved as you're describing (unless you have some non-standard registers of course)... We just had a case of misinterpreting the EHCI/PHY register spaces as interleaved (while in fact they weren't) on Renesas R-Car which I had to resolve for 3.11. >>>>> + compatible = "nvidia,tegra30-usb-phy"; >>>>> + reg = <0x7d000000 0x4000 0x7d000000 0x4000>; >>>> Hm, there must be some mistake: two similar register ranges. >>> The second range is used to configure the UTMI pad registers. All the >>> UTMI pad >>> registers are located in the first USB controller's range. >> Which second range? This is one and the same range. > Some registers in the USB1 register range actually are "global" and > relevant to all USB controllers. So: > There are two 2-cell entries in that reg property. The first entry > defines the registers for this USB PHY, and hence is unique for each USB > PHY node. The second defines the registers for whichever USB PHY > contains various shared registers across multiple PHYs, so is expected > to be identical across all USB PHY DT nodes. Hm, couldn't you have those shared registers as a separate device? > Yes, the HW design really is this screwy. Ugh... >> Don't they cause numerous resource conflicts while device nodes being >> instantiated as the platform devices? > No; the driver knows that the HW is screwy and there's lots of > register-range sharing going on, so it simply maps the registers, rather > than reserving the physical address range and mapping it. Yes, it's clear that the driver should take special measures, I was asking about the platform device creation phase. What do you see in /proc/iomem? WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/31/2013 04:20 PM, Sergei Shtylyov wrote: > On 08/01/2013 02:06 AM, Stephen Warren wrote: ... >>> That's really horrible design. >> >> Yup. Both USB PHY and EHCI controller registers really are interleaved >> in one range. > > But the standard EHCI register space has no holes IIRC, so they can't > be really that much interleaved as you're describing (unless you have > some non-standard registers of course)... Yes, there are certainly non-standard registers. ... >>> Don't they cause numerous resource conflicts while device nodes >>> being >>> instantiated as the platform devices? > >> No; the driver knows that the HW is screwy and there's lots of >> register-range sharing going on, so it simply maps the registers, rather >> than reserving the physical address range and mapping it. > > Yes, it's clear that the driver should take special measures, I was > asking about the platform device creation phase. What do you see in > /proc/iomem? The drivers don't request the memory region since doing so would cause conflicts. Hence, the regions don't show up in /proc/iomem. This actually isn't that uncommon for DT-based drivers anyway; many use e.g. of_iomap() which IIRC just looks up the resource and maps it without registering the usage. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 01-08-2013 3:29, Stephen Warren wrote: >>>> Don't they cause numerous resource conflicts while device nodes >>>> being >>>> instantiated as the platform devices? >> >>> No; the driver knows that the HW is screwy and there's lots of >>> register-range sharing going on, so it simply maps the registers, rather >>> than reserving the physical address range and mapping it. >> >> Yes, it's clear that the driver should take special measures, I was >> asking about the platform device creation phase. What do you see in >> /proc/iomem? > The drivers don't request the memory region since doing so would cause > conflicts. Hence, the regions don't show up in /proc/iomem. Regions should show up after the platform device registration. They just show up as subregions of those when you request them. That's why I asked for your /proc/iomem. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 31, 2013 at 05:29:40PM -0600, Stephen Warren wrote: > On 07/31/2013 04:20 PM, Sergei Shtylyov wrote: > > On 08/01/2013 02:06 AM, Stephen Warren wrote: > ... > >>> That's really horrible design. > >> > >> Yup. Both USB PHY and EHCI controller registers really are interleaved > >> in one range. > > > > But the standard EHCI register space has no holes IIRC, so they can't > > be really that much interleaved as you're describing (unless you have > > some non-standard registers of course)... > > Yes, there are certainly non-standard registers. > > ... > >>> Don't they cause numerous resource conflicts while device nodes > >>> being > >>> instantiated as the platform devices? > > > >> No; the driver knows that the HW is screwy and there's lots of > >> register-range sharing going on, so it simply maps the registers, rather > >> than reserving the physical address range and mapping it. > > > > Yes, it's clear that the driver should take special measures, I was > > asking about the platform device creation phase. What do you see in > > /proc/iomem? > > The drivers don't request the memory region since doing so would cause > conflicts. Hence, the regions don't show up in /proc/iomem. > > This actually isn't that uncommon for DT-based drivers anyway; many use > e.g. of_iomap() which IIRC just looks up the resource and maps it > without registering the usage. Not being uncommon isn't a good argument. The problem with doing this is that it sets a bad example and makes it easier for others to do the same thing. I can see that for some drivers providing a proper abstraction or encapsulation might be more complicated than necessary. But I've also seen this kind of shortcut taken quite often lately and especially often in DT-based drivers. Am I the only one concerned about this development? Thierry
diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts index cb640eb..f3fc2b0 100644 --- a/arch/arm/boot/dts/tegra114-dalmore.dts +++ b/arch/arm/boot/dts/tegra114-dalmore.dts @@ -870,6 +870,15 @@ non-removable; }; + usb@7d008000 { + status = "okay"; + }; + + usb-phy@7d008000 { + status = "okay"; + vbus-supply = <&usb3_vbus_reg>; + }; + clocks { compatible = "simple-bus"; #address-cells = <1>; diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi index abf6c40..2905145 100644 --- a/arch/arm/boot/dts/tegra114.dtsi +++ b/arch/arm/boot/dts/tegra114.dtsi @@ -430,6 +430,68 @@ status = "disable"; }; + usb@7d000000 { + compatible = "nvidia,tegra30-ehci", "usb-ehci"; + reg = <0x7d000000 0x4000>; + interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>; + phy_type = "utmi"; + clocks = <&tegra_car TEGRA114_CLK_USBD>; + nvidia,phy = <&phy1>; + status = "disabled"; + }; + + phy1: usb-phy@7d000000 { + compatible = "nvidia,tegra30-usb-phy"; + reg = <0x7d000000 0x4000 0x7d000000 0x4000>; + phy_type = "utmi"; + clocks = <&tegra_car TEGRA114_CLK_USBD>, + <&tegra_car TEGRA114_CLK_PLL_U>, + <&tegra_car TEGRA114_CLK_USBD>; + clock-names = "reg", "pll_u", "utmi-pads"; + nvidia,hssync-start-delay = <0>; + nvidia,idle-wait-delay = <17>; + nvidia,elastic-limit = <16>; + nvidia,term-range-adj = <6>; + nvidia,xcvr-setup = <9>; + nvidia,xcvr-lsfslew = <0>; + nvidia,xcvr-lsrslew = <3>; + nvidia,hssquelch-level = <2>; + nvidia,hsdiscon-level = <5>; + nvidia,xcvr-hsslew = <12>; + status = "disabled"; + }; + + usb@7d008000 { + compatible = "nvidia,tegra30-ehci", "usb-ehci"; + reg = <0x7d008000 0x4000>; + interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>; + phy_type = "utmi"; + clocks = <&tegra_car TEGRA114_CLK_USB3>; + nvidia,phy = <&phy3>; + status = "disabled"; + }; + + phy3: usb-phy@7d008000 { + compatible = "nvidia,tegra30-usb-phy"; + reg = <0x7d008000 0x4000 0x7d000000 0x4000>; + phy_type = "utmi"; + clocks = <&tegra_car TEGRA114_CLK_USB3>, + <&tegra_car TEGRA114_CLK_PLL_U>, + <&tegra_car TEGRA114_CLK_USBD>; + clock-names = "reg", "pll_u", "utmi-pads"; + nvidia,hssync-start-delay = <0>; + nvidia,idle-wait-delay = <17>; + nvidia,elastic-limit = <16>; + nvidia,term-range-adj = <6>; + nvidia,xcvr-setup = <9>; + nvidia,xcvr-lsfslew = <0>; + nvidia,xcvr-lsrslew = <3>; + nvidia,hssquelch-level = <2>; + nvidia,hsdiscon-level = <5>; + nvidia,xcvr-hsslew = <12>; + status = "disabled"; + }; + cpus { #address-cells = <1>; #size-cells = <0>;