diff mbox

[RFC,usb-next,v5,1/3] dt-bindings: usb: add the documentation for USB root-hub

Message ID CAFBinCC7disratgMmmaSJMfywb-bPF32L-1BarSsX6zTChHEJg@mail.gmail.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Martin Blumenstingl Oct. 17, 2017, 9 p.m. UTC
Hi Arnd,

On Fri, Oct 13, 2017 at 9:37 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Oct 12, 2017 at 10:56 PM, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>> Hi Arnd,
>>
>> thank you for reviewing this patch!
>>
>> On Mon, Oct 9, 2017 at 12:24 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Sun, Oct 8, 2017 at 11:17 PM, Martin Blumenstingl
>>> <martin.blumenstingl@googlemail.com> wrote:
>>>> A USB root-hub may have several PHYs which need to be configured before
>>>> the root-hub starts working.
>>>> This adds the documentation for such a USB root-hub as well as a hint
>>>> regarding the child-nodes on XHCI controllers which can include the
>>>> roothub.
>>>>
>>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>>> Acked-by: Rob Herring <robh@kernel.org>
>>>
>>> Have you checked that this still works with DT properties on a USB device
>>> that is listed in the DT? A common use-case is to provide the MAC address
>>> of a soldered-down USB-ethernet that lacks its own eeprom, and it seems
>>> you are changing the way the parent devices of that get represented,
>>> so the dev->of_node pointer in the USB device might no longer refer
>>> to the correct device.
>> I haven't tested the described use-case. however, this patch is not
>> supposed to change the binding for actual devices.
>> USB device numbering starts at 1, while 0 is reserved for the root-hub
>> (at least from what I know). before this patch there was no way to
>> describe the root-hub via .dts. this however is required for some
>> platforms that need to set up a PHY for each port on the root-hub
>> (Amlogic Meson GXL platform is one example: there are two ports
>> enabled on dwc3's root-hub with 2x USB2 and 1x USB3 PHYs) - so this
>> patch uses a similar binding as we already have (to describe the
>> devices) to describe the root-hub
>
> My point is that the DT binding does depend on the hierarchy, there
> is no way to find a particular device unless each parent up the
> chain is described correctly in DT as well and has a valid of_node
> pointer.
>
> It's possible that this has never worked on XHCI because of the lack
> of the root-hub in DT. Either way, we should ensure that it does work
> now and you didn't break it, so please at least test it with your patches.
>
> The patch below should be sufficient to see if any device has an
> of_node pointer when you add it to your DT:
I slightly modified your patch (see the attached version) and tried it:
# lsusb -t
/:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/0p, 5000M
/:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/2p, 480M
   |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
       |__ Port 3: Dev 3, If 0, Class=Mass Storage, Driver=usb-storage, 480M

the roothub (bus 1 port 1 dev 1) and a soldered down hub (child of
that root-hub, port 1 dev 2) have an entry in the .dts

# dmesg | grep usb
[    0.174097] usbcore: registered new interface driver usbfs
[    0.174147] usbcore: registered new interface driver hub
[    0.174217] usbcore: registered new device driver usb
[    1.354280] usb usb2: We don't know the algorithms for LPM for this
host, disabling LPM.
[    1.373297] usbcore: registered new interface driver usb-storage
[    1.512840] usbcore: registered new interface driver dvb_usb_rtl28xxu
[    1.550506] usb 1-1: of_node /soc/usb@c9000000/hub@1 parent /soc/usb@c9000000
^ this is the soldered down hub
[    1.552579] usbcore: registered new interface driver bcm203x
[    1.712033] usbcore: registered new interface driver usbhid
[    1.716994] usbhid: USB HID core driver
[    1.738827] usb 1-1: new high-speed USB device number 2 using xhci-hcd
[    2.142392] usb 1-1.3: of_node <no-node> parent /soc/usb@c9000000/hub@1
^ this is the thumb drive plugged into the hub (not specified in the .dts)
[    2.220399] usb 1-1.3: new high-speed USB device number 3 using xhci-hcd
[    2.326144] usb-storage 1-1.3:1.0: USB Mass Storage device detected
[    2.328352] scsi host0: usb-storage 1-1.3:1.0

so for me it seems to be working fine

is there anything else you want me to test?

> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 17681d5638ac..498d0aa0a5c0 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -647,6 +647,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
>                 }
>                 dev->dev.of_node = usb_of_get_child_node(parent->dev.of_node,
>                                 raw_port);
> +               dev_info(&dev->dev, "of_node %p parent %p\n",
> dev->dev.of_node, parent->dev.of_node);
>
>                 /* hub driver sets up TT records */
>         }
>
>
>        Arnd


Regards,
Martin

Comments

Arnd Bergmann Oct. 17, 2017, 9:10 p.m. UTC | #1
On Tue, Oct 17, 2017 at 11:00 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> On Fri, Oct 13, 2017 at 9:37 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Thu, Oct 12, 2017 at 10:56 PM, Martin Blumenstingl
>> <martin.blumenstingl@googlemail.com> wrote:
>>
>> It's possible that this has never worked on XHCI because of the lack
>> of the root-hub in DT. Either way, we should ensure that it does work
>> now and you didn't break it, so please at least test it with your patches.
>>
>> The patch below should be sufficient to see if any device has an
>> of_node pointer when you add it to your DT:
> I slightly modified your patch (see the attached version) and tried it:
> # lsusb -t
> /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/0p, 5000M
> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/2p, 480M
>    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
>        |__ Port 3: Dev 3, If 0, Class=Mass Storage, Driver=usb-storage, 480M
>
> the roothub (bus 1 port 1 dev 1) and a soldered down hub (child of
> that root-hub, port 1 dev 2) have an entry in the .dts
>
> # dmesg | grep usb
> [    0.174097] usbcore: registered new interface driver usbfs
> [    0.174147] usbcore: registered new interface driver hub
> [    0.174217] usbcore: registered new device driver usb
> [    1.354280] usb usb2: We don't know the algorithms for LPM for this
> host, disabling LPM.
> [    1.373297] usbcore: registered new interface driver usb-storage
> [    1.512840] usbcore: registered new interface driver dvb_usb_rtl28xxu
> [    1.550506] usb 1-1: of_node /soc/usb@c9000000/hub@1 parent /soc/usb@c9000000
> ^ this is the soldered down hub
> [    1.552579] usbcore: registered new interface driver bcm203x
> [    1.712033] usbcore: registered new interface driver usbhid
> [    1.716994] usbhid: USB HID core driver
> [    1.738827] usb 1-1: new high-speed USB device number 2 using xhci-hcd
> [    2.142392] usb 1-1.3: of_node <no-node> parent /soc/usb@c9000000/hub@1
> ^ this is the thumb drive plugged into the hub (not specified in the .dts)
> [    2.220399] usb 1-1.3: new high-speed USB device number 3 using xhci-hcd
> [    2.326144] usb-storage 1-1.3:1.0: USB Mass Storage device detected
> [    2.328352] scsi host0: usb-storage 1-1.3:1.0
>
> so for me it seems to be working fine

Ok, very good!

> is there anything else you want me to test?

What about the same dtb when run on a kernel without your
patch series? Does that work as well, or are your patches
required to make it work?

       Arnd
Martin Blumenstingl Oct. 17, 2017, 9:19 p.m. UTC | #2
Hi Arnd,

On Tue, Oct 17, 2017 at 11:10 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Oct 17, 2017 at 11:00 PM, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>> On Fri, Oct 13, 2017 at 9:37 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Thu, Oct 12, 2017 at 10:56 PM, Martin Blumenstingl
>>> <martin.blumenstingl@googlemail.com> wrote:
>>>
>>> It's possible that this has never worked on XHCI because of the lack
>>> of the root-hub in DT. Either way, we should ensure that it does work
>>> now and you didn't break it, so please at least test it with your patches.
>>>
>>> The patch below should be sufficient to see if any device has an
>>> of_node pointer when you add it to your DT:
>> I slightly modified your patch (see the attached version) and tried it:
>> # lsusb -t
>> /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/0p, 5000M
>> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/2p, 480M
>>    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
>>        |__ Port 3: Dev 3, If 0, Class=Mass Storage, Driver=usb-storage, 480M
>>
>> the roothub (bus 1 port 1 dev 1) and a soldered down hub (child of
>> that root-hub, port 1 dev 2) have an entry in the .dts
>>
>> # dmesg | grep usb
>> [    0.174097] usbcore: registered new interface driver usbfs
>> [    0.174147] usbcore: registered new interface driver hub
>> [    0.174217] usbcore: registered new device driver usb
>> [    1.354280] usb usb2: We don't know the algorithms for LPM for this
>> host, disabling LPM.
>> [    1.373297] usbcore: registered new interface driver usb-storage
>> [    1.512840] usbcore: registered new interface driver dvb_usb_rtl28xxu
>> [    1.550506] usb 1-1: of_node /soc/usb@c9000000/hub@1 parent /soc/usb@c9000000
>> ^ this is the soldered down hub
>> [    1.552579] usbcore: registered new interface driver bcm203x
>> [    1.712033] usbcore: registered new interface driver usbhid
>> [    1.716994] usbhid: USB HID core driver
>> [    1.738827] usb 1-1: new high-speed USB device number 2 using xhci-hcd
>> [    2.142392] usb 1-1.3: of_node <no-node> parent /soc/usb@c9000000/hub@1
>> ^ this is the thumb drive plugged into the hub (not specified in the .dts)
>> [    2.220399] usb 1-1.3: new high-speed USB device number 3 using xhci-hcd
>> [    2.326144] usb-storage 1-1.3:1.0: USB Mass Storage device detected
>> [    2.328352] scsi host0: usb-storage 1-1.3:1.0
>>
>> so for me it seems to be working fine
>
> Ok, very good!
>
>> is there anything else you want me to test?
>
> What about the same dtb when run on a kernel without your
> patch series? Does that work as well, or are your patches
> required to make it work?
this is the only device I have which uses devicetree and a xHCI
controller. I can test it with a dwc2 based device though if you want


Regards,
Martin
Arnd Bergmann Oct. 18, 2017, 9:05 a.m. UTC | #3
On Tue, Oct 17, 2017 at 11:19 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>> Ok, very good!
>>
>>> is there anything else you want me to test?
>>
>> What about the same dtb when run on a kernel without your
>> patch series? Does that work as well, or are your patches
>> required to make it work?
>
> this is the only device I have which uses devicetree and a xHCI
> controller. I can test it with a dwc2 based device though if you want

Does dwc2 also use separate nodes for the roothub? From your
description it sounds like it would not be affected by your patch.

To rephrase what I'm interested in, can you check that with your
patch series, we correctly associate a device node in DT with the
struct device in the kernel both with and without the optional
roothub node in the dtb?

Since you used a dtb that already listed an endpoint device below
an xhci, that would answer my earlier question of whether it worked
before your patch series, and you tested that it still works with your
patches applied and the roothub node added in the dtb.  Now we
just need to make sure we don't break existing dtb files that don't
have the roothub node but do have endpoint device nodes.

       Arnd
Neil Armstrong Oct. 19, 2017, 9:54 a.m. UTC | #4
On 18/10/2017 11:05, Arnd Bergmann wrote:
> On Tue, Oct 17, 2017 at 11:19 PM, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>>> Ok, very good!
>>>
>>>> is there anything else you want me to test?
>>>
>>> What about the same dtb when run on a kernel without your
>>> patch series? Does that work as well, or are your patches
>>> required to make it work?
>>
>> this is the only device I have which uses devicetree and a xHCI
>> controller. I can test it with a dwc2 based device though if you want
> 
> Does dwc2 also use separate nodes for the roothub? From your
> description it sounds like it would not be affected by your patch.
> 
> To rephrase what I'm interested in, can you check that with your
> patch series, we correctly associate a device node in DT with the
> struct device in the kernel both with and without the optional
> roothub node in the dtb?
> 
> Since you used a dtb that already listed an endpoint device below
> an xhci, that would answer my earlier question of whether it worked
> before your patch series, and you tested that it still works with your
> patches applied and the roothub node added in the dtb.  Now we
> just need to make sure we don't break existing dtb files that don't
> have the roothub node but do have endpoint device nodes.
> 
>        Arnd
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic
> 

Hi Arnd,

I tested in the same conditions :
- On Odroid-C2 using DWC2, but not using the roothub functionnality
- patch v6 applied
- node for on-board hub
- printk in usb core

And it works :
[    8.767964] usb 1-1: of_node /soc/usb@c9100000/hub@1 parent /soc/usb@c9100000
[    8.955901] usb 1-1: new high-speed USB device number 2 using dwc2
[    9.165641] hub 1-1:1.0: USB hub found
[    9.165939] hub 1-1:1.0: 4 ports detected

Neil
Martin Blumenstingl Oct. 19, 2017, 9:25 p.m. UTC | #5
Hi Arnd,

On Wed, Oct 18, 2017 at 11:05 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Oct 17, 2017 at 11:19 PM, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>>> Ok, very good!
>>>
>>>> is there anything else you want me to test?
>>>
>>> What about the same dtb when run on a kernel without your
>>> patch series? Does that work as well, or are your patches
>>> required to make it work?
>>
>> this is the only device I have which uses devicetree and a xHCI
>> controller. I can test it with a dwc2 based device though if you want
>
> Does dwc2 also use separate nodes for the roothub? From your
> description it sounds like it would not be affected by your patch.
currently it doesn't use separate notes for the roothub - however,
with this patch it could (although I haven't explicitly tested this)

> To rephrase what I'm interested in, can you check that with your
> patch series, we correctly associate a device node in DT with the
> struct device in the kernel both with and without the optional
> roothub node in the dtb?
I can do the same tests that Neil did with SoCs that use a dwc2
controller (Amlogic Meson8m2 and Meson8b).
the only SoC I have which uses a dwc3 controller is an Amlogic Meson
GXL - however this won't see any devices (apart from the root-hub)
without this patch

> Since you used a dtb that already listed an endpoint device below
> an xhci, that would answer my earlier question of whether it worked
> before your patch series, and you tested that it still works with your
> patches applied and the roothub node added in the dtb.  Now we
> just need to make sure we don't break existing dtb files that don't
> have the roothub node but do have endpoint device nodes.
the endpoint you're seeing is the root-hub - you can see the full .dts here: [0]

maybe my patch description or documentation is not clear - could you
please explain what makes you think that specifying the root-hub
didn't work before (so I can update the comments where needed)?
to sum up what this series does: find the node with reg = <0>; (= the
root-hub) and get all PHY instances from the child-nodes
this should not change any existing behavior except if someone had a
node with reg = <0>; below any USB controller node (which was
undocumented behavior before my patches)


Regards,
Martin


[0] https://github.com/xdarklight/linux/blob/meson-gxl-usb-v6/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi#L55
Arnd Bergmann Oct. 19, 2017, 10:10 p.m. UTC | #6
On Thu, Oct 19, 2017 at 11:25 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>> Does dwc2 also use separate nodes for the roothub? From your
>> description it sounds like it would not be affected by your patch.
> currently it doesn't use separate notes for the roothub - however,
> with this patch it could (although I haven't explicitly tested this)

Ok.

>> Since you used a dtb that already listed an endpoint device below
>> an xhci, that would answer my earlier question of whether it worked
>> before your patch series, and you tested that it still works with your
>> patches applied and the roothub node added in the dtb.  Now we
>> just need to make sure we don't break existing dtb files that don't
>> have the roothub node but do have endpoint device nodes.
> the endpoint you're seeing is the root-hub - you can see the full .dts here: [0]
>
> maybe my patch description or documentation is not clear - could you
> please explain what makes you think that specifying the root-hub
> didn't work before (so I can update the comments where needed)?
> to sum up what this series does: find the node with reg = <0>; (= the
> root-hub) and get all PHY instances from the child-nodes
> this should not change any existing behavior except if someone had a
> node with reg = <0>; below any USB controller node (which was
> undocumented behavior before my patches)

Maybe I misunderstand what the actual change to the hierarchy
is. Quoting from your example for the new code

+       &usb1 {
+               #address-cells = <1>;
+               #size-cells = <0>;
+
+               roothub@0 {
+                       compatible = "usb1d6b,3", "usb1d6b,2";
+                       #address-cells = <1>;
+                       #size-cells = <0>;
+                       reg = <0>;
+
+                       port@1 {
+                               reg = <1>;
+                               phys = <&usb2_phy1>, <&usb3_phy1>;
+                               phy-names = "usb2-phy", "usb3-phy";
+                       };

The way I understand it, an endpoing device would now be
located in

 &usb1/roothub@0/port@1/hub@2/device@1

where previously it was in

 &usb1/port@1/hub@2/device@1

Is that correct?

I don't see any code that can deal with both cases and still
assign the correct of_node pointer to the device device@1 in
the end (maybe it's there and you just need to point me
to the right patch).

The reason why we have to be careful here is that the
Linux device hierarchy is not just derived from DT here, but
it gets created from the physical devices under &usb1
and the 'struct device' hierarchy has to match the DT hierarchy
exactly.

      Arnd
Martin Blumenstingl Oct. 23, 2017, 9:36 p.m. UTC | #7
Hi Arnd,

thank you for taking 10 minutes to discuss this in person with me!

On Fri, Oct 20, 2017 at 12:10 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Oct 19, 2017 at 11:25 PM, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>>> Does dwc2 also use separate nodes for the roothub? From your
>>> description it sounds like it would not be affected by your patch.
>> currently it doesn't use separate notes for the roothub - however,
>> with this patch it could (although I haven't explicitly tested this)
>
> Ok.
>
>>> Since you used a dtb that already listed an endpoint device below
>>> an xhci, that would answer my earlier question of whether it worked
>>> before your patch series, and you tested that it still works with your
>>> patches applied and the roothub node added in the dtb.  Now we
>>> just need to make sure we don't break existing dtb files that don't
>>> have the roothub node but do have endpoint device nodes.
>> the endpoint you're seeing is the root-hub - you can see the full .dts here: [0]
>>
>> maybe my patch description or documentation is not clear - could you
>> please explain what makes you think that specifying the root-hub
>> didn't work before (so I can update the comments where needed)?
>> to sum up what this series does: find the node with reg = <0>; (= the
>> root-hub) and get all PHY instances from the child-nodes
>> this should not change any existing behavior except if someone had a
>> node with reg = <0>; below any USB controller node (which was
>> undocumented behavior before my patches)
>
> Maybe I misunderstand what the actual change to the hierarchy
> is. Quoting from your example for the new code
>
> +       &usb1 {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               roothub@0 {
> +                       compatible = "usb1d6b,3", "usb1d6b,2";
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       reg = <0>;
> +
> +                       port@1 {
> +                               reg = <1>;
> +                               phys = <&usb2_phy1>, <&usb3_phy1>;
> +                               phy-names = "usb2-phy", "usb3-phy";
> +                       };
>
> The way I understand it, an endpoing device would now be
> located in
>
>  &usb1/roothub@0/port@1/hub@2/device@1
>
> where previously it was in
>
>  &usb1/port@1/hub@2/device@1
>
> Is that correct?
this is not how it's currently implemented - let's find out if this is
a misunderstanding on my side
the second example is still valid with this series, while the first
example won't work with the code as it is

> I don't see any code that can deal with both cases and still
> assign the correct of_node pointer to the device device@1 in
> the end (maybe it's there and you just need to point me
> to the right patch).
it's not you - that code is (currently) not there

> The reason why we have to be careful here is that the
> Linux device hierarchy is not just derived from DT here, but
> it gets created from the physical devices under &usb1
> and the 'struct device' hierarchy has to match the DT hierarchy
> exactly.
yes, I fully agree with you here
I will post an updated version of this series which includes an
updated usb-xhci dt-binding documentation to show how the end result
(after this series) can look like. I'll include Rob in the loop again
so we don't introduce a broken binding.


Regards,
Martin
diff mbox

Patch

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
index cfbfbfeff85d..7677ce77d122 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
@@ -81,6 +81,11 @@ 
 					phy-names = "usb2-phy";
 				};
 			};
+
+			hub@1 {
+				compatible = "usb1a40,801";
+				reg = <1>;
+			};
 		};
 	};
 };
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 17681d5638ac..893f4e8b0733 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -647,6 +647,7 @@  struct usb_device *usb_alloc_dev(struct usb_device *parent,
 		}
 		dev->dev.of_node = usb_of_get_child_node(parent->dev.of_node,
 				raw_port);
+		dev_info(&dev->dev, "of_node %s parent %s\n", of_node_full_name(dev->dev.of_node), of_node_full_name(parent->dev.of_node));
 
 		/* hub driver sets up TT records */
 	}