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

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

Hi Felipe,

On 18 August 2016 at 15:33, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> 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, and
>> we also should enable the 'XHCI_SLOW_SUSPEND' quirk for extraordinary delay when
>> suspending the xhci.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>>  drivers/usb/dwc3/Kconfig |    7 +++++++
>>  drivers/usb/dwc3/core.c  |   25 ++++++++++++++++++++++++-
>>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>>  drivers/usb/dwc3/host.c  |   32 ++++++++++++++++++++++++++++++++
>>  4 files changed, 78 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>> index a64ce1c..725d2bd 100644
>> --- a/drivers/usb/dwc3/Kconfig
>> +++ b/drivers/usb/dwc3/Kconfig
>> @@ -45,6 +45,13 @@ config USB_DWC3_DUAL_ROLE
>>         This is the default mode of working of DWC3 controller where
>>         both host and gadget features are enabled.
>>
>> +config USB_DWC3_HOST_SUSPEND
>> +     bool "Choose if the host (xhci) can be suspend/resume"
>> +     depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
>> +     help
>> +       We can suspend the host when the slave is detached for power saving,
>> +       and resume the host when one slave is attached.
>> +
>>  endchoice
>>
>>  comment "Platform Glue Driver Support"
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 1485480..5140b4d 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1103,15 +1103,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;
>> @@ -1133,12 +1145,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 45d6de5..0ba203e 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1177,4 +1177,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
>>  { }
>>  #endif
>>
>> +#if IS_ENABLED(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 2e960ed..2ec3eff 100644
>> --- a/drivers/usb/dwc3/host.c
>> +++ b/drivers/usb/dwc3/host.c
>> @@ -17,8 +17,11 @@
>>
>>  #include <linux/platform_device.h>
>>  #include <linux/usb/xhci_pdriver.h>
>> +#include <linux/usb.h>
>> +#include <linux/usb/hcd.h>
>>
>>  #include "core.h"
>> +#include "../host/xhci.h"
>>
>>  int dwc3_host_init(struct dwc3 *dwc)
>>  {
>> @@ -91,6 +94,8 @@ int dwc3_host_init(struct dwc3 *dwc)
>>       memset(&pdata, 0, sizeof(pdata));
>>
>>       pdata.usb3_lpm_capable = dwc->usb3_lpm_capable;
>> +     /* dwc3 controller need an extraordinary delay when suspending xhci. */
>> +     pdata.usb3_slow_suspend = 1;
>>
>>       ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
>>       if (ret) {
>> @@ -128,3 +133,30 @@ void dwc3_host_exit(struct dwc3 *dwc)
>>                         dev_name(&dwc->xhci->dev));
>>       platform_device_unregister(dwc->xhci);
>>  }
>> +
>> +int dwc3_host_suspend(struct dwc3 *dwc)
>> +{
>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>> +     int ret, cnt = 20;
>> +
>> +try_again:
>> +      /* We should wait for xhci bus has been into suspend mode firstly. */
>> +     ret = xhci_suspend(xhci, device_may_wakeup(&dwc->xhci->dev));
>> +     if (ret && --cnt > 0) {
>> +             dev_warn(dwc->dev, "xhci suspend failed %d, try again...\n",
>> +                      ret);
>> +             msleep(200);
>> +             goto try_again;
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +int dwc3_host_resume(struct dwc3 *dwc)
>> +{
>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>> +
>> +     return xhci_resume(xhci, 0);
>> +}
>
> This is wrong. XHCI is a child of dwc3, when dwc3 suspends,
> xhci_supend() has already been called. Why isn't it called for you?

xhci-plat.c did not set the runtime PM callbacks, how can we issued
xhci_supend() by runtime PM from dwc3?

-- 
Baolin.wang
Best Regards