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.gasn...@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 <ma...@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