Patchwork [2/2] ARM: dts: USB for Tegra114 Dalmore

login
register
mail settings
Submitter Tuomas Tynkkynen
Date July 31, 2013, 5:42 p.m.
Message ID <1375292543-7896-3-git-send-email-ttynkkynen@nvidia.com>
Download mbox | patch
Permalink /patch/263777/
State Superseded, archived
Headers show

Comments

Tuomas Tynkkynen - July 31, 2013, 5:42 p.m.
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.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 arch/arm/boot/dts/tegra114-dalmore.dts |  9 +++++
 arch/arm/boot/dts/tegra114.dtsi        | 62 ++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)
Sergei Shtylyov - July 31, 2013, 6:18 p.m.
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
Tuomas Tynkkynen - July 31, 2013, 7:31 p.m.
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
Sergei Shtylyov - July 31, 2013, 7:53 p.m.
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
Stephen Warren - July 31, 2013, 10:06 p.m.
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
Sergei Shtylyov - July 31, 2013, 10:20 p.m.
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
Stephen Warren - July 31, 2013, 11:29 p.m.
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
Sergei Shtylyov - Aug. 1, 2013, 12:37 p.m.
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
Thierry Reding - Aug. 15, 2013, 10:54 a.m.
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

Patch

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>;