Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume

[Baolin Wang <baolin.wang@linaro.org>]

Hi,

On 8 December 2016 at 17:40, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> Hi Felipe,
>>
>> On 28 November 2016 at 14:43, Baolin Wang <baolin.wang@linaro.org> wrote:
>>> For some mobile devices with strict power management, we also want to suspend
>>> the host when the slave is detached for power saving. Thus we add the host
>>> suspend/resume functions to support this requirement.
>>>
>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>> ---
>>> Changes since v3:
>>>  - No updates.
>>>
>>> Changes since v2:
>>>  - Remove pm_children_suspended() and other unused macros.
>>>
>>>  Changes since v1:
>>>    - Add pm_runtime.h head file to avoid kbuild error.
>>> ---
>>>  drivers/usb/dwc3/Kconfig |    7 +++++++
>>>  drivers/usb/dwc3/core.c  |   26 +++++++++++++++++++++++++-
>>>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>>>  drivers/usb/dwc3/host.c  |   37 +++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 84 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>> index a45b4f1..47bb2f3 100644
>>> --- a/drivers/usb/dwc3/Kconfig
>>> +++ b/drivers/usb/dwc3/Kconfig
>>> @@ -47,6 +47,13 @@ config USB_DWC3_DUAL_ROLE
>>>
>>>  endchoice
>>>
>>> +config USB_DWC3_HOST_SUSPEND
>>> +       bool "Choose if the DWC3 host (xhci) can be suspend/resume"
>>> +       depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
>
> why do you need another Kconfig for this? Just enable it already :-p

I assume some platforms may do not need this feature. But okay, I can
remove this kconfig and enable it.

>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 9a4a5e4..7ad4bc3 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1091,6 +1091,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>>         pm_runtime_use_autosuspend(dev);
>>>         pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);
>>>         pm_runtime_enable(dev);
>>> +       pm_suspend_ignore_children(dev, true);
>
> why do you need this?

Since the dwc3 device is the parent deive of xhci device, if we want
to suspend dwc3 device, we need to suspend child device (xhci device)
manually by issuing pm_runtime_put_sync() in dwc3_host_suspend(). In
pm_runtime_put_sync(), it will check if the children (xhci device) of
dwc3 device have been in suspend state, if not we will suspend dwc3
device failed.

We get/put the child device manually in parent device's runtime
callback, we need to ignore the child device's runtime state, or it
will failed due to the dependency.

>
>>> @@ -1215,15 +1216,27 @@ static int dwc3_remove(struct platform_device *pdev)
>>>  static int dwc3_suspend_common(struct dwc3 *dwc)
>>>  {
>>>         unsigned long   flags;
>>> +       int             ret;
>>>
>>>         switch (dwc->dr_mode) {
>>>         case USB_DR_MODE_PERIPHERAL:
>>> +               spin_lock_irqsave(&dwc->lock, flags);
>>> +               dwc3_gadget_suspend(dwc);
>>> +               spin_unlock_irqrestore(&dwc->lock, flags);
>>> +               break;
>>>         case USB_DR_MODE_OTG:
>>> +               ret = dwc3_host_suspend(dwc);
>>> +               if (ret)
>>> +                       return ret;
>>> +
>>>                 spin_lock_irqsave(&dwc->lock, flags);
>>>                 dwc3_gadget_suspend(dwc);
>>>                 spin_unlock_irqrestore(&dwc->lock, flags);
>>>                 break;
>>>         case USB_DR_MODE_HOST:
>>> +               ret = dwc3_host_suspend(dwc);
>>> +               if (ret)
>>> +                       return ret;
>>>         default:
>>>                 /* do nothing */
>>>                 break;
>>> @@ -1245,12 +1258,23 @@ static int dwc3_resume_common(struct dwc3 *dwc)
>>>
>>>         switch (dwc->dr_mode) {
>>>         case USB_DR_MODE_PERIPHERAL:
>>> +               spin_lock_irqsave(&dwc->lock, flags);
>>> +               dwc3_gadget_resume(dwc);
>>> +               spin_unlock_irqrestore(&dwc->lock, flags);
>>> +               break;
>>>         case USB_DR_MODE_OTG:
>>> +               ret = dwc3_host_resume(dwc);
>>> +               if (ret)
>>> +                       return ret;
>>> +
>>>                 spin_lock_irqsave(&dwc->lock, flags);
>>>                 dwc3_gadget_resume(dwc);
>>>                 spin_unlock_irqrestore(&dwc->lock, flags);
>>> -               /* FALLTHROUGH */
>>> +               break;
>>>         case USB_DR_MODE_HOST:
>>> +               ret = dwc3_host_resume(dwc);
>>> +               if (ret)
>>> +                       return ret;
>>>         default:
>>>                 /* do nothing */
>>>                 break;
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index b585a30..db41908 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -1226,4 +1226,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
>>>  { }
>>>  #endif
>>>
>>> +#if IS_ENABLED(CONFIG_USB_DWC3_HOST_SUSPEND)
>>> +int dwc3_host_suspend(struct dwc3 *dwc);
>>> +int dwc3_host_resume(struct dwc3 *dwc);
>>> +#else
>>> +static inline int dwc3_host_suspend(struct dwc3 *dwc)
>>> +{
>>> +       return 0;
>>> +}
>>> +
>>> +static inline int dwc3_host_resume(struct dwc3 *dwc)
>>> +{
>>> +       return 0;
>>> +}
>>> +#endif
>>> +
>>>  #endif /* __DRIVERS_USB_DWC3_CORE_H */
>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>>> index ed82464..8e5309d6 100644
>>> --- a/drivers/usb/dwc3/host.c
>>> +++ b/drivers/usb/dwc3/host.c
>>> @@ -16,6 +16,7 @@
>>>   */
>>>
>>>  #include <linux/platform_device.h>
>>> +#include <linux/pm_runtime.h>
>>>
>>>  #include "core.h"
>>>
>>> @@ -130,3 +131,39 @@ void dwc3_host_exit(struct dwc3 *dwc)
>>>                           dev_name(&dwc->xhci->dev));
>>>         platform_device_unregister(dwc->xhci);
>>>  }
>>> +
>>> +#ifdef CONFIG_USB_DWC3_HOST_SUSPEND
>>> +int dwc3_host_suspend(struct dwc3 *dwc)
>>> +{
>>> +       struct device *xhci = &dwc->xhci->dev;
>>> +       int ret;
>>> +
>>> +       /*
>>> +        * Note: if we get the -EBUSY, which means the xHCI children devices are
>>> +        * not in suspend state yet, the glue layer need to wait for a while and
>>> +        * try to suspend xHCI device again.
>>> +        */
>>> +       ret = pm_runtime_put_sync(xhci);
>>> +       if (ret) {
>>> +               dev_err(xhci, "failed to suspend xHCI device\n");
>>> +               return ret;
>>> +       }
>
> return pm_runtime_put_sync() is probably enough here.

OK.

>
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +int dwc3_host_resume(struct dwc3 *dwc)
>>> +{
>>> +       struct device *xhci = &dwc->xhci->dev;
>>> +       int ret;
>>> +
>>> +       /* Resume the xHCI device synchronously. */
>>> +       ret = pm_runtime_get_sync(xhci);
>
> likewise.

OK. I will fix these in next version. Thanks.

>
>>> +       if (ret) {
>>> +               dev_err(xhci, "failed to resume xHCI device\n");
>>> +               return ret;
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +#endif
>>> --
>>> 1.7.9.5
>>>
>>
>> How do you think this patch and do you have any suggestion? Thanks.
>>
>> --
>> Baolin.wang
>> Best Regards
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
> balbi



-- 
Baolin.wang
Best Regards