| Message ID | 20171008211713.18696-2-martin.blumenstingl@googlemail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | initialize (multiple) PHYs on the roothub | expand |
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. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 Regards, Martin -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 12 Oct 2017, Martin Blumenstingl wrote: > 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). Actually 1 is reserved for the root hub. External devices are numbered starting from 2. (We can't use 0 for the root hub because, as you said, USB device numbering starts at 1!) Alan Stern > 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 > > > Regards, > Martin > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan, On Thu, Oct 12, 2017 at 11:17 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Thu, 12 Oct 2017, Martin Blumenstingl wrote: > >> 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). > > Actually 1 is reserved for the root hub. External devices are numbered > starting from 2. (We can't use 0 for the root hub because, as you > said, USB device numbering starts at 1!) I just had a look at lsusb again: $ lsusb -t /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M however, for the devicetree bindings the devices are supposed to use 1-31: [0] Alan: just to make sure I understood you correctly: do you agree with this patch? > Alan Stern > >> 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 >> >> >> Regards, >> Martin >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-usb" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> > Regards, Martin [0] http://elixir.free-electrons.com/linux/v4.14-rc4/source/Documentation/devicetree/bindings/usb/usb-device.txt -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 12 Oct 2017, Martin Blumenstingl wrote: > Hi Alan, > > On Thu, Oct 12, 2017 at 11:17 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Thu, 12 Oct 2017, Martin Blumenstingl wrote: > > > >> 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). > > > > Actually 1 is reserved for the root hub. External devices are numbered > > starting from 2. (We can't use 0 for the root hub because, as you > > said, USB device numbering starts at 1!) > I just had a look at lsusb again: > $ lsusb -t > /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M > > however, for the devicetree bindings the devices are supposed to use 1-31: [0] > > Alan: just to make sure I understood you correctly: do you agree with > this patch? I have no idea. I haven't read the patch and I'm not familiar with the details of DeviceTree. I was just trying to clear up your misunderstanding of USB device numbering. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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 */ }
Hi Alan, On Fri, Oct 13, 2017 at 4:15 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Thu, 12 Oct 2017, Martin Blumenstingl wrote: > >> Hi Alan, >> >> On Thu, Oct 12, 2017 at 11:17 PM, Alan Stern <stern@rowland.harvard.edu> wrote: >> > On Thu, 12 Oct 2017, Martin Blumenstingl wrote: >> > >> >> 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). >> > >> > Actually 1 is reserved for the root hub. External devices are numbered >> > starting from 2. (We can't use 0 for the root hub because, as you >> > said, USB device numbering starts at 1!) >> I just had a look at lsusb again: >> $ lsusb -t >> /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M >> >> however, for the devicetree bindings the devices are supposed to use 1-31: [0] >> >> Alan: just to make sure I understood you correctly: do you agree with >> this patch? > > I have no idea. I haven't read the patch and I'm not familiar with the > details of DeviceTree. I was just trying to clear up your > misunderstanding of USB device numbering. thank you for clarifying this. I tested it and the devicetree numbering is "off by one" compared to the lsusb output - this however has nothing to do with my patch. so my previous statements that basically said "0 is used for the roothub" should be "0 is used for the roothub in .dts/.dtsi files" Regards, Martin -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/usb/usb-roothub.txt b/Documentation/devicetree/bindings/usb/usb-roothub.txt new file mode 100644 index 000000000000..fc0797d7cee9 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/usb-roothub.txt @@ -0,0 +1,46 @@ +Generic USB root-hub Properties + +similar to the USB device bindings (documented in usb-device.txt from the +current directory) this provides support for configuring the root-hub. + +Required properties: +- compatible: should be at least one of "usb1d6b,3", "usb1d6b,2" +- reg: must be 0. +- address-cells: must be 1 +- size-cells: must be 0 + +Required sub-nodes: +a sub-node per actual USB port is required. each sub-node supports the +following properties: + Required properties: + - reg: the port number on the root-hub (mandatory) + Optional properties: + - phys: optional, from the *Generic PHY* bindings (mandatory needed + when phy-names is given) + - phy-names: optional, from the *Generic PHY* bindings; supported names + are "usb2-phy" or "usb3-phy" + +Example: + &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"; + }; + + port@2 { + reg = <2>; + phys = <&usb2_phy2>, <&usb3_phy2>; + phy-names = "usb2-phy", "usb3-phy"; + }; + }; + } diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt index ae6e484a8d7c..5b49ba9f2f9a 100644 --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt @@ -30,6 +30,13 @@ Optional properties: - usb3-lpm-capable: determines if platform is USB3 LPM capable - quirk-broken-port-ped: set if the controller has broken port disable mechanism +sub-nodes: +- optionally there can be a node for the root-hub, see usb-roothub.txt in the + current directory +- one or more nodes with reg 1-31 for each port to which a device is connected. + See usb-device.txt in the current directory for more information. + + Example: usb@f0931000 { compatible = "generic-xhci";