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

[Marek Vasut]

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

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.