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

[Fabrice Gasnier]

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