[U-Boot] [RFC PATCH v2 3/6] dm: usb: Add UCLASS_USB_DEV_GENERIC shutdown

[Simon Glass]

Hi Jagan,

On 17 August 2018 at 07:37, Jagan Teki <jagan at amarulasolutions.com> wrote:
> Hi Simon,
>
> On Fri, Aug 17, 2018 at 6:18 PM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Jagan,
>>
>> On 7 August 2018 at 01:03, Jagan Teki <jagan at amarulasolutions.com> wrote:
>>> On Tue, Jul 24, 2018 at 5:18 AM, Simon Glass <sjg at chromium.org> wrote:
>>>> Hi Jagan,
>>>>
>>>> On 20 July 2018 at 01:13, Jagan Teki <jagan at amarulasolutions.com> wrote:
>>>>> Some OTG controllers which operates on Peripheral
>>>>> mode are registered as UCLASS_USB_DEV_GENERIC.
>>>>>
>>>>> So add support to shutdown them as well. shutdown
>>>>> happened during 'usb reset' and U-Boot handoff code
>>>>> for Linux boot.
>>>>>
>>>>> controller restarting in 'usb reset' like probing
>>>>> again is still missing for this type of UCLASS.
>>>>>
>>>>> Cc: Simon Glass <sjg at chromium.org>
>>>>> Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
>>>>> ---
>>>>> Note:
>>>>> I tried of traversing for multiple UCLASS in removal
>>>>> code in usb_stop, but couldn't come with proper solutions.
>>>>> I don't think any other area of code need a requirement like
>>>>> this, or may be I'm missing. any help would appreciate.
>>>>>
>>>>> Changes for v2:
>>>>> - none
>>>>>
>>>>>  drivers/usb/host/usb-uclass.c | 43 +++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 43 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
>>>>> index 611ea97a72..99cf3d2b49 100644
>>>>> --- a/drivers/usb/host/usb-uclass.c
>>>>> +++ b/drivers/usb/host/usb-uclass.c
>>>>> @@ -158,6 +158,45 @@ int usb_get_max_xfer_size(struct usb_device *udev, size_t *size)
>>>>>         return ops->get_max_xfer_size(bus, size);
>>>>>  }
>>>>>
>>>>> +int __usb_stop(void)
>>>>
>>>> Why the __ ? I think it should be something like usb_remove_and_unbind_all()
>>>>
>>>>> +{
>>>>> +       struct udevice *bus;
>>>>> +       struct udevice *rh;
>>>>> +       struct uclass *uc;
>>>>> +       struct usb_uclass_priv *uc_priv;
>>>>> +       int err = 0, ret;
>>>>> +
>>>>> +       /* De-activate any devices that have been activated */
>>>>> +       ret = uclass_get(UCLASS_USB_DEV_GENERIC, &uc);
>>>>> +       if (ret)
>>>>> +               return ret;
>>>>> +
>>>>> +       uc_priv = uc->priv;
>>>>> +
>>>>> +       uclass_foreach_dev(bus, uc) {
>>>>> +               ret = device_remove(bus, DM_REMOVE_NORMAL);
>>>>> +               if (ret && !err)
>>>>> +                       err = ret;
>>>>> +
>>>>> +               /* Locate root hub device */
>>>>> +               device_find_first_child(bus, &rh);
>>>>> +               if (rh) {
>>>>> +                       /*
>>>>> +                        * All USB devices are children of root hub.
>>>>> +                        * Unbinding root hub will unbind all of its children.
>>>>> +                        */
>>>>> +                       ret = device_unbind(rh);
>>>>> +                       if (ret && !err)
>>>>> +                               err = ret;
>>>>> +               }
>>>>> +       }
>>>>> +
>>>>> +       uc_priv->companion_device_count = 0;
>>>>> +       usb_started = 0;
>>>>> +
>>>>> +       return err;
>>>>> +}
>>>>> +
>>>>>  int usb_stop(void)
>>>>>  {
>>>>>         struct udevice *bus;
>>>>> @@ -166,6 +205,10 @@ int usb_stop(void)
>>>>>         struct usb_uclass_priv *uc_priv;
>>>>>         int err = 0, ret;
>>>>>
>>>>> +       ret = __usb_stop();
>>>>> +       if (ret)
>>>>> +               return ret;
>>>>> +
>>>>
>>>> This looks like the same code that appears below here, or very
>>>> similar. Why is this needed?
>>>
>>> Here the usage-case it to remove/unbind UCLASS_USB and
>>> UCLASS_USB_DEV_GENERIC and same code will use to do that, any
>>> suggestions?
>>
>> I'm just wondering why you appear to be duplicating the exact code
>> that is already there? Maybe I am missing something, iwc please can
>> you explain it?
>
> Yes, the code is duplicate. Here, I'm looking for suggestion to
> unbind/remove multiple UCLASS's like UCLASS_USB and
> ULASS_USB_DEV_GENERIC in this use-case. do you have any? or am I
> missing something?

But if you unbind the bus, doesn't this automatically unbind its children?

At the very least, it seems like there is some common code here that
you should factor out.

Regards,
Simon