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

[Marek Vasut]

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 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);

One more thing, shouldn't this do

if (ret)
   continue;

for maximum compatibility ?

> I'd be glad to have it in this release.

OK