| Message ID | 20230901095201.3874129-1-fabrice.gasnier@foss.st.com |
|---|---|
| State | Accepted |
| Commit | 5b43bc97eeb9f7618ec926c72166ebf879b9d92e |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [v2] usb: check for companion controller in uclass | expand |
On 9/1/23 11:52, Fabrice Gasnier wrote: > EHCI is usually used with companion controller (like OHCI) as companion > controller. This information on the companion is missing currently in > companion drivers. > So, if the usb-uclass isn't aware, it may scan busses in any order: OHCI > first, then EHCI. > This is seen on STM32MP1 where DT probing makes the probe order to occur > by increasing address (OHCI address < EHCI address). > > When a low speed or full-speed device is plugged in, it's not detected as > EHCI should first detect it, and give ownership (handover) to OHCI. > > Current situation on STM32MP1 (with a low speed device plugged-in) > STM32MP> usb start > starting USB... > Bus usb@5800c000: USB OHCI 1.0 > Bus usb@5800d000: USB EHCI 1.00 > scanning bus usb@5800c000 for devices... 1 USB Device(s) found > scanning bus usb@5800d000 for devices... 1 USB Device(s) found > scanning usb for storage devices... 0 Storage Device(s) found > > The "companion" property in the device tree allow to retrieve companion > controller information, from the EHCI node. This allow marking the > companion driver as such. > > With this patch (same low speed device plugged in): > STM32MP> usb start > starting USB... > Bus usb@5800c000: USB OHCI 1.0 > Bus usb@5800d000: USB EHCI 1.00 > scanning bus usb@5800d000 for devices... 1 USB Device(s) found > scanning bus usb@5800c000 for devices... 2 USB Device(s) found > scanning usb for storage devices... 0 Storage Device(s) found > STM32MP> usb tree > USB device tree: > 1 Hub (12 Mb/s, 0mA) > | U-Boot Root Hub > | > +-2 Human Interface (1.5 Mb/s, 100mA) > HP HP USB 1000dpi Laser Mouse > > 1 Hub (480 Mb/s, 0mA) > u-boot EHCI Host Controller > > This also optimize bus scan when a High speed device is plugged in, as > the usb-uclass skips OHCI in this case: > > STM32MP> usb reset > resetting USB... > Bus usb@5800c000: USB OHCI 1.0 > Bus usb@5800d000: USB EHCI 1.00 > scanning bus usb@5800d000 for devices... 2 USB Device(s) found > scanning usb for storage devices... 1 Storage Device(s) found > STM32MP> usb tree > USB device tree: > 1 Hub (480 Mb/s, 0mA) > | u-boot EHCI Host Controller > | > +-2 Mass Storage (480 Mb/s, 200mA) > SanDisk Cruzer Blade 03003432021922011407 > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> > --- > > Changes in v2: > - move companion probing from generic ehci driver to usb-uclass, after > Marek's questions on design choice. > - rename commit title to follow this change > > --- > drivers/usb/host/usb-uclass.c | 36 +++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c > index 02c0138a2065..e238eee8c84d 100644 > --- a/drivers/usb/host/usb-uclass.c > +++ b/drivers/usb/host/usb-uclass.c > @@ -249,6 +249,37 @@ static void remove_inactive_children(struct uclass *uc, struct udevice *bus) > } > } > > +static int usb_probe_companion(struct udevice *bus) > +{ > + struct udevice *companion_dev; > + int ret; > + > + /* > + * Enforce optional companion controller is marked as such in order to > + * 1st scan the primary controller, before the companion controller > + * (ownership is given to companion when low or full speed devices > + * have been detected). > + */ > + ret = uclass_get_device_by_phandle(UCLASS_USB, bus, "companion", &companion_dev); > + if (!ret) { > + struct usb_bus_priv *companion_bus_priv; > + > + debug("%s is the companion of %s\n", companion_dev->name, bus->name); > + companion_bus_priv = dev_get_uclass_priv(companion_dev); > + companion_bus_priv->companion = true; > + } else if (ret && ret != -ENOENT && ret != -ENODEV) { > + /* > + * Treat everything else than no companion or disabled > + * companion as an error. (It may not be enabled on boards > + * that have a High-Speed HUB to handle FS and LS traffic). > + */ > + printf("Failed to get companion (ret=%d)\n", ret); > + return ret; > + } > + > + return 0; > +} > + > int usb_init(void) > { > int controllers_initialized = 0; > @@ -299,6 +330,11 @@ int usb_init(void) > printf("probe failed, error %d\n", ret); > continue; > } > + > + ret = usb_probe_companion(bus); Shouldn't this ^ be in separate uclass_foreach_dev(bus, uc) {} loop before this device_probe() loop , so that when the probe of the controller is called, its ->companion is already set ? > + if (ret) > + continue; > + > controllers_initialized++; > usb_started = true; > } [...]
On 9/1/23 12:48, Marek Vasut wrote: > On 9/1/23 11:52, Fabrice Gasnier wrote: >> EHCI is usually used with companion controller (like OHCI) as companion >> controller. This information on the companion is missing currently in >> companion drivers. >> So, if the usb-uclass isn't aware, it may scan busses in any order: OHCI >> first, then EHCI. >> This is seen on STM32MP1 where DT probing makes the probe order to occur >> by increasing address (OHCI address < EHCI address). >> >> When a low speed or full-speed device is plugged in, it's not detected as >> EHCI should first detect it, and give ownership (handover) to OHCI. >> >> Current situation on STM32MP1 (with a low speed device plugged-in) >> STM32MP> usb start >> starting USB... >> Bus usb@5800c000: USB OHCI 1.0 >> Bus usb@5800d000: USB EHCI 1.00 >> scanning bus usb@5800c000 for devices... 1 USB Device(s) found >> scanning bus usb@5800d000 for devices... 1 USB Device(s) found >> scanning usb for storage devices... 0 Storage Device(s) found >> >> The "companion" property in the device tree allow to retrieve companion >> controller information, from the EHCI node. This allow marking the >> companion driver as such. >> >> With this patch (same low speed device plugged in): >> STM32MP> usb start >> starting USB... >> Bus usb@5800c000: USB OHCI 1.0 >> Bus usb@5800d000: USB EHCI 1.00 >> scanning bus usb@5800d000 for devices... 1 USB Device(s) found >> scanning bus usb@5800c000 for devices... 2 USB Device(s) found >> scanning usb for storage devices... 0 Storage Device(s) found >> STM32MP> usb tree >> USB device tree: >> 1 Hub (12 Mb/s, 0mA) >> | U-Boot Root Hub >> | >> +-2 Human Interface (1.5 Mb/s, 100mA) >> HP HP USB 1000dpi Laser Mouse >> >> 1 Hub (480 Mb/s, 0mA) >> u-boot EHCI Host Controller >> >> This also optimize bus scan when a High speed device is plugged in, as >> the usb-uclass skips OHCI in this case: >> >> STM32MP> usb reset >> resetting USB... >> Bus usb@5800c000: USB OHCI 1.0 >> Bus usb@5800d000: USB EHCI 1.00 >> scanning bus usb@5800d000 for devices... 2 USB Device(s) found >> scanning usb for storage devices... 1 Storage Device(s) found >> STM32MP> usb tree >> USB device tree: >> 1 Hub (480 Mb/s, 0mA) >> | u-boot EHCI Host Controller >> | >> +-2 Mass Storage (480 Mb/s, 200mA) >> SanDisk Cruzer Blade 03003432021922011407 >> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> >> --- >> >> Changes in v2: >> - move companion probing from generic ehci driver to usb-uclass, after >> Marek's questions on design choice. >> - rename commit title to follow this change >> >> --- >> drivers/usb/host/usb-uclass.c | 36 +++++++++++++++++++++++++++++++++++ >> 1 file changed, 36 insertions(+) >> >> diff --git a/drivers/usb/host/usb-uclass.c >> b/drivers/usb/host/usb-uclass.c >> index 02c0138a2065..e238eee8c84d 100644 >> --- a/drivers/usb/host/usb-uclass.c >> +++ b/drivers/usb/host/usb-uclass.c >> @@ -249,6 +249,37 @@ static void remove_inactive_children(struct >> uclass *uc, struct udevice *bus) >> } >> } >> +static int usb_probe_companion(struct udevice *bus) >> +{ >> + struct udevice *companion_dev; >> + int ret; >> + >> + /* >> + * Enforce optional companion controller is marked as such in >> order to >> + * 1st scan the primary controller, before the companion controller >> + * (ownership is given to companion when low or full speed devices >> + * have been detected). >> + */ >> + ret = uclass_get_device_by_phandle(UCLASS_USB, bus, "companion", >> &companion_dev); >> + if (!ret) { >> + struct usb_bus_priv *companion_bus_priv; >> + >> + debug("%s is the companion of %s\n", companion_dev->name, >> bus->name); >> + companion_bus_priv = dev_get_uclass_priv(companion_dev); >> + companion_bus_priv->companion = true; >> + } else if (ret && ret != -ENOENT && ret != -ENODEV) { >> + /* >> + * Treat everything else than no companion or disabled >> + * companion as an error. (It may not be enabled on boards >> + * that have a High-Speed HUB to handle FS and LS traffic). >> + */ >> + printf("Failed to get companion (ret=%d)\n", ret); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> int usb_init(void) >> { >> int controllers_initialized = 0; >> @@ -299,6 +330,11 @@ int usb_init(void) >> printf("probe failed, error %d\n", ret); >> continue; >> } >> + >> + ret = usb_probe_companion(bus); > > Shouldn't this ^ be in separate uclass_foreach_dev(bus, uc) {} loop > before this device_probe() loop , so that when the probe of the > controller is called, its ->companion is already set ? The point is to have the companion flag set before scanning the bus. Generic OHCI or EHCI drivers don't care about this flag. Scanning is performed right after this first loop (low-level), in two subsequent steps: primary controllers (2nd step), then companions if necessary (3rd step). > >> + if (ret) >> + continue; >> + >> controllers_initialized++; >> usb_started = true; >> } > > [...]
On 9/1/23 14:12, Fabrice Gasnier wrote: > On 9/1/23 12:48, Marek Vasut wrote: >> On 9/1/23 11:52, Fabrice Gasnier wrote: >>> EHCI is usually used with companion controller (like OHCI) as companion >>> controller. This information on the companion is missing currently in >>> companion drivers. >>> So, if the usb-uclass isn't aware, it may scan busses in any order: OHCI >>> first, then EHCI. >>> This is seen on STM32MP1 where DT probing makes the probe order to occur >>> by increasing address (OHCI address < EHCI address). >>> >>> When a low speed or full-speed device is plugged in, it's not detected as >>> EHCI should first detect it, and give ownership (handover) to OHCI. >>> >>> Current situation on STM32MP1 (with a low speed device plugged-in) >>> STM32MP> usb start >>> starting USB... >>> Bus usb@5800c000: USB OHCI 1.0 >>> Bus usb@5800d000: USB EHCI 1.00 >>> scanning bus usb@5800c000 for devices... 1 USB Device(s) found >>> scanning bus usb@5800d000 for devices... 1 USB Device(s) found >>> scanning usb for storage devices... 0 Storage Device(s) found >>> >>> The "companion" property in the device tree allow to retrieve companion >>> controller information, from the EHCI node. This allow marking the >>> companion driver as such. >>> >>> With this patch (same low speed device plugged in): >>> STM32MP> usb start >>> starting USB... >>> Bus usb@5800c000: USB OHCI 1.0 >>> Bus usb@5800d000: USB EHCI 1.00 >>> scanning bus usb@5800d000 for devices... 1 USB Device(s) found >>> scanning bus usb@5800c000 for devices... 2 USB Device(s) found >>> scanning usb for storage devices... 0 Storage Device(s) found >>> STM32MP> usb tree >>> USB device tree: >>> 1 Hub (12 Mb/s, 0mA) >>> | U-Boot Root Hub >>> | >>> +-2 Human Interface (1.5 Mb/s, 100mA) >>> HP HP USB 1000dpi Laser Mouse >>> >>> 1 Hub (480 Mb/s, 0mA) >>> u-boot EHCI Host Controller >>> >>> This also optimize bus scan when a High speed device is plugged in, as >>> the usb-uclass skips OHCI in this case: >>> >>> STM32MP> usb reset >>> resetting USB... >>> Bus usb@5800c000: USB OHCI 1.0 >>> Bus usb@5800d000: USB EHCI 1.00 >>> scanning bus usb@5800d000 for devices... 2 USB Device(s) found >>> scanning usb for storage devices... 1 Storage Device(s) found >>> STM32MP> usb tree >>> USB device tree: >>> 1 Hub (480 Mb/s, 0mA) >>> | u-boot EHCI Host Controller >>> | >>> +-2 Mass Storage (480 Mb/s, 200mA) >>> SanDisk Cruzer Blade 03003432021922011407 >>> >>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> >>> --- >>> >>> Changes in v2: >>> - move companion probing from generic ehci driver to usb-uclass, after >>> Marek's questions on design choice. >>> - rename commit title to follow this change >>> >>> --- >>> drivers/usb/host/usb-uclass.c | 36 +++++++++++++++++++++++++++++++++++ >>> 1 file changed, 36 insertions(+) >>> >>> diff --git a/drivers/usb/host/usb-uclass.c >>> b/drivers/usb/host/usb-uclass.c >>> index 02c0138a2065..e238eee8c84d 100644 >>> --- a/drivers/usb/host/usb-uclass.c >>> +++ b/drivers/usb/host/usb-uclass.c >>> @@ -249,6 +249,37 @@ static void remove_inactive_children(struct >>> uclass *uc, struct udevice *bus) >>> } >>> } >>> +static int usb_probe_companion(struct udevice *bus) >>> +{ >>> + struct udevice *companion_dev; >>> + int ret; >>> + >>> + /* >>> + * Enforce optional companion controller is marked as such in >>> order to >>> + * 1st scan the primary controller, before the companion controller >>> + * (ownership is given to companion when low or full speed devices >>> + * have been detected). >>> + */ >>> + ret = uclass_get_device_by_phandle(UCLASS_USB, bus, "companion", >>> &companion_dev); >>> + if (!ret) { >>> + struct usb_bus_priv *companion_bus_priv; >>> + >>> + debug("%s is the companion of %s\n", companion_dev->name, >>> bus->name); >>> + companion_bus_priv = dev_get_uclass_priv(companion_dev); >>> + companion_bus_priv->companion = true; >>> + } else if (ret && ret != -ENOENT && ret != -ENODEV) { >>> + /* >>> + * Treat everything else than no companion or disabled >>> + * companion as an error. (It may not be enabled on boards >>> + * that have a High-Speed HUB to handle FS and LS traffic). >>> + */ >>> + printf("Failed to get companion (ret=%d)\n", ret); >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> int usb_init(void) >>> { >>> int controllers_initialized = 0; >>> @@ -299,6 +330,11 @@ int usb_init(void) >>> printf("probe failed, error %d\n", ret); >>> continue; >>> } >>> + >>> + ret = usb_probe_companion(bus); >> >> Shouldn't this ^ be in separate uclass_foreach_dev(bus, uc) {} loop >> before this device_probe() loop , so that when the probe of the >> controller is called, its ->companion is already set ? > > The point is to have the companion flag set before scanning the bus. > Generic OHCI or EHCI drivers don't care about this flag. > > Scanning is performed right after this first loop (low-level), in two > subsequent steps: primary controllers (2nd step), then companions if > necessary (3rd step). Thanks for clarifying, in that case: Reviewed-by: Marek Vasut <marex@denx.de> Is this a fix for this release (currently rc3) or next release ?
On 9/1/23 18:00, Marek Vasut wrote: > On 9/1/23 14:12, Fabrice Gasnier wrote: >> On 9/1/23 12:48, Marek Vasut wrote: >>> On 9/1/23 11:52, Fabrice Gasnier wrote: >>>> EHCI is usually used with companion controller (like OHCI) as companion >>>> controller. This information on the companion is missing currently in >>>> companion drivers. >>>> So, if the usb-uclass isn't aware, it may scan busses in any order: >>>> OHCI >>>> first, then EHCI. >>>> This is seen on STM32MP1 where DT probing makes the probe order to >>>> occur >>>> by increasing address (OHCI address < EHCI address). >>>> >>>> When a low speed or full-speed device is plugged in, it's not >>>> detected as >>>> EHCI should first detect it, and give ownership (handover) to OHCI. >>>> >>>> Current situation on STM32MP1 (with a low speed device plugged-in) >>>> STM32MP> usb start >>>> starting USB... >>>> Bus usb@5800c000: USB OHCI 1.0 >>>> Bus usb@5800d000: USB EHCI 1.00 >>>> scanning bus usb@5800c000 for devices... 1 USB Device(s) found >>>> scanning bus usb@5800d000 for devices... 1 USB Device(s) found >>>> scanning usb for storage devices... 0 Storage Device(s) found >>>> >>>> The "companion" property in the device tree allow to retrieve companion >>>> controller information, from the EHCI node. This allow marking the >>>> companion driver as such. >>>> >>>> With this patch (same low speed device plugged in): >>>> STM32MP> usb start >>>> starting USB... >>>> Bus usb@5800c000: USB OHCI 1.0 >>>> Bus usb@5800d000: USB EHCI 1.00 >>>> scanning bus usb@5800d000 for devices... 1 USB Device(s) found >>>> scanning bus usb@5800c000 for devices... 2 USB Device(s) found >>>> scanning usb for storage devices... 0 Storage Device(s) found >>>> STM32MP> usb tree >>>> USB device tree: >>>> 1 Hub (12 Mb/s, 0mA) >>>> | U-Boot Root Hub >>>> | >>>> +-2 Human Interface (1.5 Mb/s, 100mA) >>>> HP HP USB 1000dpi Laser Mouse >>>> >>>> 1 Hub (480 Mb/s, 0mA) >>>> u-boot EHCI Host Controller >>>> >>>> This also optimize bus scan when a High speed device is plugged in, as >>>> the usb-uclass skips OHCI in this case: >>>> >>>> STM32MP> usb reset >>>> resetting USB... >>>> Bus usb@5800c000: USB OHCI 1.0 >>>> Bus usb@5800d000: USB EHCI 1.00 >>>> scanning bus usb@5800d000 for devices... 2 USB Device(s) found >>>> scanning usb for storage devices... 1 Storage Device(s) found >>>> STM32MP> usb tree >>>> USB device tree: >>>> 1 Hub (480 Mb/s, 0mA) >>>> | u-boot EHCI Host Controller >>>> | >>>> +-2 Mass Storage (480 Mb/s, 200mA) >>>> SanDisk Cruzer Blade 03003432021922011407 >>>> >>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> >>>> --- >>>> >>>> Changes in v2: >>>> - move companion probing from generic ehci driver to usb-uclass, after >>>> Marek's questions on design choice. >>>> - rename commit title to follow this change >>>> >>>> --- >>>> drivers/usb/host/usb-uclass.c | 36 >>>> +++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 36 insertions(+) >>>> >>>> diff --git a/drivers/usb/host/usb-uclass.c >>>> b/drivers/usb/host/usb-uclass.c >>>> index 02c0138a2065..e238eee8c84d 100644 >>>> --- a/drivers/usb/host/usb-uclass.c >>>> +++ b/drivers/usb/host/usb-uclass.c >>>> @@ -249,6 +249,37 @@ static void remove_inactive_children(struct >>>> uclass *uc, struct udevice *bus) >>>> } >>>> } >>>> +static int usb_probe_companion(struct udevice *bus) >>>> +{ >>>> + struct udevice *companion_dev; >>>> + int ret; >>>> + >>>> + /* >>>> + * Enforce optional companion controller is marked as such in >>>> order to >>>> + * 1st scan the primary controller, before the companion >>>> controller >>>> + * (ownership is given to companion when low or full speed devices >>>> + * have been detected). >>>> + */ >>>> + ret = uclass_get_device_by_phandle(UCLASS_USB, bus, "companion", >>>> &companion_dev); >>>> + if (!ret) { >>>> + struct usb_bus_priv *companion_bus_priv; >>>> + >>>> + debug("%s is the companion of %s\n", companion_dev->name, >>>> bus->name); >>>> + companion_bus_priv = dev_get_uclass_priv(companion_dev); >>>> + companion_bus_priv->companion = true; >>>> + } else if (ret && ret != -ENOENT && ret != -ENODEV) { >>>> + /* >>>> + * Treat everything else than no companion or disabled >>>> + * companion as an error. (It may not be enabled on boards >>>> + * that have a High-Speed HUB to handle FS and LS traffic). >>>> + */ >>>> + printf("Failed to get companion (ret=%d)\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> int usb_init(void) >>>> { >>>> int controllers_initialized = 0; >>>> @@ -299,6 +330,11 @@ int usb_init(void) >>>> printf("probe failed, error %d\n", ret); >>>> continue; >>>> } >>>> + >>>> + ret = usb_probe_companion(bus); >>> >>> Shouldn't this ^ be in separate uclass_foreach_dev(bus, uc) {} loop >>> before this device_probe() loop , so that when the probe of the >>> controller is called, its ->companion is already set ? >> >> The point is to have the companion flag set before scanning the bus. >> Generic OHCI or EHCI drivers don't care about this flag. >> >> Scanning is performed right after this first loop (low-level), in two >> subsequent steps: primary controllers (2nd step), then companions if >> necessary (3rd step). > > Thanks for clarifying, in that case: > > Reviewed-by: Marek Vasut <marex@denx.de> > > Is this a fix for this release (currently rc3) or next release ? Hi Marek, I'd be glad to have it in this release. Thanks, Fabrice
On 9/4/23 14:34, Fabrice Gasnier wrote: > On 9/1/23 18:00, Marek Vasut wrote: >> On 9/1/23 14:12, Fabrice Gasnier wrote: >>> On 9/1/23 12:48, Marek Vasut wrote: >>>> On 9/1/23 11:52, Fabrice Gasnier wrote: >>>>> EHCI is usually used with companion controller (like OHCI) as companion >>>>> controller. This information on the companion is missing currently in >>>>> companion drivers. >>>>> So, if the usb-uclass isn't aware, it may scan busses in any order: >>>>> OHCI >>>>> first, then EHCI. >>>>> This is seen on STM32MP1 where DT probing makes the probe order to >>>>> occur >>>>> by increasing address (OHCI address < EHCI address). >>>>> >>>>> When a low speed or full-speed device is plugged in, it's not >>>>> detected as >>>>> EHCI should first detect it, and give ownership (handover) to OHCI. >>>>> >>>>> Current situation on STM32MP1 (with a low speed device plugged-in) >>>>> STM32MP> usb start >>>>> starting USB... >>>>> Bus usb@5800c000: USB OHCI 1.0 >>>>> Bus usb@5800d000: USB EHCI 1.00 >>>>> scanning bus usb@5800c000 for devices... 1 USB Device(s) found >>>>> scanning bus usb@5800d000 for devices... 1 USB Device(s) found >>>>> scanning usb for storage devices... 0 Storage Device(s) found >>>>> >>>>> The "companion" property in the device tree allow to retrieve companion >>>>> controller information, from the EHCI node. This allow marking the >>>>> companion driver as such. >>>>> >>>>> With this patch (same low speed device plugged in): >>>>> STM32MP> usb start >>>>> starting USB... >>>>> Bus usb@5800c000: USB OHCI 1.0 >>>>> Bus usb@5800d000: USB EHCI 1.00 >>>>> scanning bus usb@5800d000 for devices... 1 USB Device(s) found >>>>> scanning bus usb@5800c000 for devices... 2 USB Device(s) found >>>>> scanning usb for storage devices... 0 Storage Device(s) found >>>>> STM32MP> usb tree >>>>> USB device tree: >>>>> 1 Hub (12 Mb/s, 0mA) >>>>> | U-Boot Root Hub >>>>> | >>>>> +-2 Human Interface (1.5 Mb/s, 100mA) >>>>> HP HP USB 1000dpi Laser Mouse >>>>> >>>>> 1 Hub (480 Mb/s, 0mA) >>>>> u-boot EHCI Host Controller >>>>> >>>>> This also optimize bus scan when a High speed device is plugged in, as >>>>> the usb-uclass skips OHCI in this case: >>>>> >>>>> STM32MP> usb reset >>>>> resetting USB... >>>>> Bus usb@5800c000: USB OHCI 1.0 >>>>> Bus usb@5800d000: USB EHCI 1.00 >>>>> scanning bus usb@5800d000 for devices... 2 USB Device(s) found >>>>> scanning usb for storage devices... 1 Storage Device(s) found >>>>> STM32MP> usb tree >>>>> USB device tree: >>>>> 1 Hub (480 Mb/s, 0mA) >>>>> | u-boot EHCI Host Controller >>>>> | >>>>> +-2 Mass Storage (480 Mb/s, 200mA) >>>>> SanDisk Cruzer Blade 03003432021922011407 >>>>> >>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> >>>>> --- >>>>> >>>>> Changes in v2: >>>>> - move companion probing from generic ehci driver to usb-uclass, after >>>>> Marek's questions on design choice. >>>>> - rename commit title to follow this change >>>>> >>>>> --- >>>>> drivers/usb/host/usb-uclass.c | 36 >>>>> +++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 36 insertions(+) >>>>> >>>>> diff --git a/drivers/usb/host/usb-uclass.c >>>>> b/drivers/usb/host/usb-uclass.c >>>>> index 02c0138a2065..e238eee8c84d 100644 >>>>> --- a/drivers/usb/host/usb-uclass.c >>>>> +++ b/drivers/usb/host/usb-uclass.c >>>>> @@ -249,6 +249,37 @@ static void remove_inactive_children(struct >>>>> uclass *uc, struct udevice *bus) >>>>> } >>>>> } >>>>> +static int usb_probe_companion(struct udevice *bus) >>>>> +{ >>>>> + struct udevice *companion_dev; >>>>> + int ret; >>>>> + >>>>> + /* >>>>> + * Enforce optional companion controller is marked as such in >>>>> order to >>>>> + * 1st scan the primary controller, before the companion >>>>> controller >>>>> + * (ownership is given to companion when low or full speed devices >>>>> + * have been detected). >>>>> + */ >>>>> + ret = uclass_get_device_by_phandle(UCLASS_USB, bus, "companion", >>>>> &companion_dev); >>>>> + if (!ret) { >>>>> + struct usb_bus_priv *companion_bus_priv; >>>>> + >>>>> + debug("%s is the companion of %s\n", companion_dev->name, >>>>> bus->name); >>>>> + companion_bus_priv = dev_get_uclass_priv(companion_dev); >>>>> + companion_bus_priv->companion = true; >>>>> + } else if (ret && ret != -ENOENT && ret != -ENODEV) { >>>>> + /* >>>>> + * Treat everything else than no companion or disabled >>>>> + * companion as an error. (It may not be enabled on boards >>>>> + * that have a High-Speed HUB to handle FS and LS traffic). >>>>> + */ >>>>> + printf("Failed to get companion (ret=%d)\n", ret); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> int usb_init(void) >>>>> { >>>>> int controllers_initialized = 0; >>>>> @@ -299,6 +330,11 @@ int usb_init(void) >>>>> printf("probe failed, error %d\n", ret); >>>>> continue; >>>>> } >>>>> + >>>>> + ret = usb_probe_companion(bus); One more thing, shouldn't this do if (ret) continue; for maximum compatibility ? > I'd be glad to have it in this release. OK
On 9/4/23 15:54, Marek Vasut wrote: > On 9/4/23 14:34, Fabrice Gasnier wrote: >> On 9/1/23 18:00, Marek Vasut wrote: >>> On 9/1/23 14:12, Fabrice Gasnier wrote: >>>> On 9/1/23 12:48, Marek Vasut wrote: >>>>> On 9/1/23 11:52, Fabrice Gasnier wrote: >>>>>> EHCI is usually used with companion controller (like OHCI) as >>>>>> companion >>>>>> controller. This information on the companion is missing currently in >>>>>> companion drivers. >>>>>> So, if the usb-uclass isn't aware, it may scan busses in any order: >>>>>> OHCI >>>>>> first, then EHCI. >>>>>> This is seen on STM32MP1 where DT probing makes the probe order to >>>>>> occur >>>>>> by increasing address (OHCI address < EHCI address). >>>>>> >>>>>> When a low speed or full-speed device is plugged in, it's not >>>>>> detected as >>>>>> EHCI should first detect it, and give ownership (handover) to OHCI. >>>>>> >>>>>> Current situation on STM32MP1 (with a low speed device plugged-in) >>>>>> STM32MP> usb start >>>>>> starting USB... >>>>>> Bus usb@5800c000: USB OHCI 1.0 >>>>>> Bus usb@5800d000: USB EHCI 1.00 >>>>>> scanning bus usb@5800c000 for devices... 1 USB Device(s) found >>>>>> scanning bus usb@5800d000 for devices... 1 USB Device(s) found >>>>>> scanning usb for storage devices... 0 Storage Device(s) found >>>>>> >>>>>> The "companion" property in the device tree allow to retrieve >>>>>> companion >>>>>> controller information, from the EHCI node. This allow marking the >>>>>> companion driver as such. >>>>>> >>>>>> With this patch (same low speed device plugged in): >>>>>> STM32MP> usb start >>>>>> starting USB... >>>>>> Bus usb@5800c000: USB OHCI 1.0 >>>>>> Bus usb@5800d000: USB EHCI 1.00 >>>>>> scanning bus usb@5800d000 for devices... 1 USB Device(s) found >>>>>> scanning bus usb@5800c000 for devices... 2 USB Device(s) found >>>>>> scanning usb for storage devices... 0 Storage Device(s) found >>>>>> STM32MP> usb tree >>>>>> USB device tree: >>>>>> 1 Hub (12 Mb/s, 0mA) >>>>>> | U-Boot Root Hub >>>>>> | >>>>>> +-2 Human Interface (1.5 Mb/s, 100mA) >>>>>> HP HP USB 1000dpi Laser Mouse >>>>>> >>>>>> 1 Hub (480 Mb/s, 0mA) >>>>>> u-boot EHCI Host Controller >>>>>> >>>>>> This also optimize bus scan when a High speed device is plugged >>>>>> in, as >>>>>> the usb-uclass skips OHCI in this case: >>>>>> >>>>>> STM32MP> usb reset >>>>>> resetting USB... >>>>>> Bus usb@5800c000: USB OHCI 1.0 >>>>>> Bus usb@5800d000: USB EHCI 1.00 >>>>>> scanning bus usb@5800d000 for devices... 2 USB Device(s) found >>>>>> scanning usb for storage devices... 1 Storage Device(s) found >>>>>> STM32MP> usb tree >>>>>> USB device tree: >>>>>> 1 Hub (480 Mb/s, 0mA) >>>>>> | u-boot EHCI Host Controller >>>>>> | >>>>>> +-2 Mass Storage (480 Mb/s, 200mA) >>>>>> SanDisk Cruzer Blade 03003432021922011407 >>>>>> >>>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> >>>>>> --- >>>>>> >>>>>> Changes in v2: >>>>>> - move companion probing from generic ehci driver to usb-uclass, >>>>>> after >>>>>> Marek's questions on design choice. >>>>>> - rename commit title to follow this change >>>>>> >>>>>> --- >>>>>> drivers/usb/host/usb-uclass.c | 36 >>>>>> +++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 36 insertions(+) >>>>>> >>>>>> diff --git a/drivers/usb/host/usb-uclass.c >>>>>> b/drivers/usb/host/usb-uclass.c >>>>>> index 02c0138a2065..e238eee8c84d 100644 >>>>>> --- a/drivers/usb/host/usb-uclass.c >>>>>> +++ b/drivers/usb/host/usb-uclass.c >>>>>> @@ -249,6 +249,37 @@ static void remove_inactive_children(struct >>>>>> uclass *uc, struct udevice *bus) >>>>>> } >>>>>> } >>>>>> +static int usb_probe_companion(struct udevice *bus) >>>>>> +{ >>>>>> + struct udevice *companion_dev; >>>>>> + int ret; >>>>>> + >>>>>> + /* >>>>>> + * Enforce optional companion controller is marked as such in >>>>>> order to >>>>>> + * 1st scan the primary controller, before the companion >>>>>> controller >>>>>> + * (ownership is given to companion when low or full speed >>>>>> devices >>>>>> + * have been detected). >>>>>> + */ >>>>>> + ret = uclass_get_device_by_phandle(UCLASS_USB, bus, "companion", >>>>>> &companion_dev); >>>>>> + if (!ret) { >>>>>> + struct usb_bus_priv *companion_bus_priv; >>>>>> + >>>>>> + debug("%s is the companion of %s\n", companion_dev->name, >>>>>> bus->name); >>>>>> + companion_bus_priv = dev_get_uclass_priv(companion_dev); >>>>>> + companion_bus_priv->companion = true; >>>>>> + } else if (ret && ret != -ENOENT && ret != -ENODEV) { >>>>>> + /* >>>>>> + * Treat everything else than no companion or disabled >>>>>> + * companion as an error. (It may not be enabled on boards >>>>>> + * that have a High-Speed HUB to handle FS and LS traffic). >>>>>> + */ >>>>>> + printf("Failed to get companion (ret=%d)\n", ret); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> int usb_init(void) >>>>>> { >>>>>> int controllers_initialized = 0; >>>>>> @@ -299,6 +330,11 @@ int usb_init(void) >>>>>> printf("probe failed, error %d\n", ret); >>>>>> continue; >>>>>> } >>>>>> + >>>>>> + ret = usb_probe_companion(bus); > > One more thing, shouldn't this do > > if (ret) > continue; > > for maximum compatibility ? I think it does :-) Or I miss your question here, could you clarify ? In the original PATCH there is: + + ret = usb_probe_companion(bus); + if (ret) + continue; + > >> I'd be glad to have it in this release. > > OK
On 9/4/23 18:21, Fabrice Gasnier wrote: > On 9/4/23 15:54, Marek Vasut wrote: >> On 9/4/23 14:34, Fabrice Gasnier wrote: >>> On 9/1/23 18:00, Marek Vasut wrote: >>>> On 9/1/23 14:12, Fabrice Gasnier wrote: >>>>> On 9/1/23 12:48, Marek Vasut wrote: >>>>>> On 9/1/23 11:52, Fabrice Gasnier wrote: >>>>>>> EHCI is usually used with companion controller (like OHCI) as >>>>>>> companion >>>>>>> controller. This information on the companion is missing currently in >>>>>>> companion drivers. >>>>>>> So, if the usb-uclass isn't aware, it may scan busses in any order: >>>>>>> OHCI >>>>>>> first, then EHCI. >>>>>>> This is seen on STM32MP1 where DT probing makes the probe order to >>>>>>> occur >>>>>>> by increasing address (OHCI address < EHCI address). >>>>>>> >>>>>>> When a low speed or full-speed device is plugged in, it's not >>>>>>> detected as >>>>>>> EHCI should first detect it, and give ownership (handover) to OHCI. >>>>>>> >>>>>>> Current situation on STM32MP1 (with a low speed device plugged-in) >>>>>>> STM32MP> usb start >>>>>>> starting USB... >>>>>>> Bus usb@5800c000: USB OHCI 1.0 >>>>>>> Bus usb@5800d000: USB EHCI 1.00 >>>>>>> scanning bus usb@5800c000 for devices... 1 USB Device(s) found >>>>>>> scanning bus usb@5800d000 for devices... 1 USB Device(s) found >>>>>>> scanning usb for storage devices... 0 Storage Device(s) found >>>>>>> >>>>>>> The "companion" property in the device tree allow to retrieve >>>>>>> companion >>>>>>> controller information, from the EHCI node. This allow marking the >>>>>>> companion driver as such. >>>>>>> >>>>>>> With this patch (same low speed device plugged in): >>>>>>> STM32MP> usb start >>>>>>> starting USB... >>>>>>> Bus usb@5800c000: USB OHCI 1.0 >>>>>>> Bus usb@5800d000: USB EHCI 1.00 >>>>>>> scanning bus usb@5800d000 for devices... 1 USB Device(s) found >>>>>>> scanning bus usb@5800c000 for devices... 2 USB Device(s) found >>>>>>> scanning usb for storage devices... 0 Storage Device(s) found >>>>>>> STM32MP> usb tree >>>>>>> USB device tree: >>>>>>> 1 Hub (12 Mb/s, 0mA) >>>>>>> | U-Boot Root Hub >>>>>>> | >>>>>>> +-2 Human Interface (1.5 Mb/s, 100mA) >>>>>>> HP HP USB 1000dpi Laser Mouse >>>>>>> >>>>>>> 1 Hub (480 Mb/s, 0mA) >>>>>>> u-boot EHCI Host Controller >>>>>>> >>>>>>> This also optimize bus scan when a High speed device is plugged >>>>>>> in, as >>>>>>> the usb-uclass skips OHCI in this case: >>>>>>> >>>>>>> STM32MP> usb reset >>>>>>> resetting USB... >>>>>>> Bus usb@5800c000: USB OHCI 1.0 >>>>>>> Bus usb@5800d000: USB EHCI 1.00 >>>>>>> scanning bus usb@5800d000 for devices... 2 USB Device(s) found >>>>>>> scanning usb for storage devices... 1 Storage Device(s) found >>>>>>> STM32MP> usb tree >>>>>>> USB device tree: >>>>>>> 1 Hub (480 Mb/s, 0mA) >>>>>>> | u-boot EHCI Host Controller >>>>>>> | >>>>>>> +-2 Mass Storage (480 Mb/s, 200mA) >>>>>>> SanDisk Cruzer Blade 03003432021922011407 >>>>>>> >>>>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> >>>>>>> --- >>>>>>> >>>>>>> Changes in v2: >>>>>>> - move companion probing from generic ehci driver to usb-uclass, >>>>>>> after >>>>>>> Marek's questions on design choice. >>>>>>> - rename commit title to follow this change >>>>>>> >>>>>>> --- >>>>>>> drivers/usb/host/usb-uclass.c | 36 >>>>>>> +++++++++++++++++++++++++++++++++++ >>>>>>> 1 file changed, 36 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/usb/host/usb-uclass.c >>>>>>> b/drivers/usb/host/usb-uclass.c >>>>>>> index 02c0138a2065..e238eee8c84d 100644 >>>>>>> --- a/drivers/usb/host/usb-uclass.c >>>>>>> +++ b/drivers/usb/host/usb-uclass.c >>>>>>> @@ -249,6 +249,37 @@ static void remove_inactive_children(struct >>>>>>> uclass *uc, struct udevice *bus) >>>>>>> } >>>>>>> } >>>>>>> +static int usb_probe_companion(struct udevice *bus) >>>>>>> +{ >>>>>>> + struct udevice *companion_dev; >>>>>>> + int ret; >>>>>>> + >>>>>>> + /* >>>>>>> + * Enforce optional companion controller is marked as such in >>>>>>> order to >>>>>>> + * 1st scan the primary controller, before the companion >>>>>>> controller >>>>>>> + * (ownership is given to companion when low or full speed >>>>>>> devices >>>>>>> + * have been detected). >>>>>>> + */ >>>>>>> + ret = uclass_get_device_by_phandle(UCLASS_USB, bus, "companion", >>>>>>> &companion_dev); >>>>>>> + if (!ret) { >>>>>>> + struct usb_bus_priv *companion_bus_priv; >>>>>>> + >>>>>>> + debug("%s is the companion of %s\n", companion_dev->name, >>>>>>> bus->name); >>>>>>> + companion_bus_priv = dev_get_uclass_priv(companion_dev); >>>>>>> + companion_bus_priv->companion = true; >>>>>>> + } else if (ret && ret != -ENOENT && ret != -ENODEV) { >>>>>>> + /* >>>>>>> + * Treat everything else than no companion or disabled >>>>>>> + * companion as an error. (It may not be enabled on boards >>>>>>> + * that have a High-Speed HUB to handle FS and LS traffic). >>>>>>> + */ >>>>>>> + printf("Failed to get companion (ret=%d)\n", ret); >>>>>>> + return ret; >>>>>>> + } >>>>>>> + >>>>>>> + return 0; >>>>>>> +} >>>>>>> + >>>>>>> int usb_init(void) >>>>>>> { >>>>>>> int controllers_initialized = 0; >>>>>>> @@ -299,6 +330,11 @@ int usb_init(void) >>>>>>> printf("probe failed, error %d\n", ret); >>>>>>> continue; >>>>>>> } >>>>>>> + >>>>>>> + ret = usb_probe_companion(bus); >> >> One more thing, shouldn't this do >> >> if (ret) >> continue; >> >> for maximum compatibility ? > > I think it does :-) Or I miss your question here, could you clarify ? > > In the original PATCH there is: > + > + ret = usb_probe_companion(bus); > + if (ret) > + continue; > + Then all is OK, thanks. The missing context confused me. But since rc4 was just tagged, I'll be putting this into next, we're too close to the release.
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index 02c0138a2065..e238eee8c84d 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -249,6 +249,37 @@ static void remove_inactive_children(struct uclass *uc, struct udevice *bus) } } +static int usb_probe_companion(struct udevice *bus) +{ + struct udevice *companion_dev; + int ret; + + /* + * Enforce optional companion controller is marked as such in order to + * 1st scan the primary controller, before the companion controller + * (ownership is given to companion when low or full speed devices + * have been detected). + */ + ret = uclass_get_device_by_phandle(UCLASS_USB, bus, "companion", &companion_dev); + if (!ret) { + struct usb_bus_priv *companion_bus_priv; + + debug("%s is the companion of %s\n", companion_dev->name, bus->name); + companion_bus_priv = dev_get_uclass_priv(companion_dev); + companion_bus_priv->companion = true; + } else if (ret && ret != -ENOENT && ret != -ENODEV) { + /* + * Treat everything else than no companion or disabled + * companion as an error. (It may not be enabled on boards + * that have a High-Speed HUB to handle FS and LS traffic). + */ + printf("Failed to get companion (ret=%d)\n", ret); + return ret; + } + + return 0; +} + int usb_init(void) { int controllers_initialized = 0; @@ -299,6 +330,11 @@ int usb_init(void) printf("probe failed, error %d\n", ret); continue; } + + ret = usb_probe_companion(bus); + if (ret) + continue; + controllers_initialized++; usb_started = true; }
EHCI is usually used with companion controller (like OHCI) as companion controller. This information on the companion is missing currently in companion drivers. So, if the usb-uclass isn't aware, it may scan busses in any order: OHCI first, then EHCI. This is seen on STM32MP1 where DT probing makes the probe order to occur by increasing address (OHCI address < EHCI address). When a low speed or full-speed device is plugged in, it's not detected as EHCI should first detect it, and give ownership (handover) to OHCI. Current situation on STM32MP1 (with a low speed device plugged-in) STM32MP> usb start starting USB... Bus usb@5800c000: USB OHCI 1.0 Bus usb@5800d000: USB EHCI 1.00 scanning bus usb@5800c000 for devices... 1 USB Device(s) found scanning bus usb@5800d000 for devices... 1 USB Device(s) found scanning usb for storage devices... 0 Storage Device(s) found The "companion" property in the device tree allow to retrieve companion controller information, from the EHCI node. This allow marking the companion driver as such. With this patch (same low speed device plugged in): STM32MP> usb start starting USB... Bus usb@5800c000: USB OHCI 1.0 Bus usb@5800d000: USB EHCI 1.00 scanning bus usb@5800d000 for devices... 1 USB Device(s) found scanning bus usb@5800c000 for devices... 2 USB Device(s) found scanning usb for storage devices... 0 Storage Device(s) found STM32MP> usb tree USB device tree: 1 Hub (12 Mb/s, 0mA) | U-Boot Root Hub | +-2 Human Interface (1.5 Mb/s, 100mA) HP HP USB 1000dpi Laser Mouse 1 Hub (480 Mb/s, 0mA) u-boot EHCI Host Controller This also optimize bus scan when a High speed device is plugged in, as the usb-uclass skips OHCI in this case: STM32MP> usb reset resetting USB... Bus usb@5800c000: USB OHCI 1.0 Bus usb@5800d000: USB EHCI 1.00 scanning bus usb@5800d000 for devices... 2 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found STM32MP> usb tree USB device tree: 1 Hub (480 Mb/s, 0mA) | u-boot EHCI Host Controller | +-2 Mass Storage (480 Mb/s, 200mA) SanDisk Cruzer Blade 03003432021922011407 Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> --- Changes in v2: - move companion probing from generic ehci driver to usb-uclass, after Marek's questions on design choice. - rename commit title to follow this change --- drivers/usb/host/usb-uclass.c | 36 +++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)