| Message ID | CAFBinCC7disratgMmmaSJMfywb-bPF32L-1BarSsX6zTChHEJg@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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
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
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
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
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
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
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 --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 */ }