[PATCH v2] usb: check for companion controller in uclass

[Fabrice Gasnier]

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 at 5800c000: USB OHCI 1.0
>>>> Bus usb at 5800d000: USB EHCI 1.00
>>>> scanning bus usb at 5800c000 for devices... 1 USB Device(s) found
>>>> scanning bus usb at 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 at 5800c000: USB OHCI 1.0
>>>> Bus usb at 5800d000: USB EHCI 1.00
>>>> scanning bus usb at 5800d000 for devices... 1 USB Device(s) found
>>>> scanning bus usb at 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 at 5800c000: USB OHCI 1.0
>>>> Bus usb at 5800d000: USB EHCI 1.00
>>>> scanning bus usb at 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 at 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 at 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