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

From: Felipe Balbi
Date: Thu Aug 18 2016 - 08:26:22 EST


Hi,

Baolin Wang <baolin.wang@xxxxxxxxxx> writes:
> Hi Felipe,
>
> On 18 August 2016 at 15:33, Felipe Balbi <balbi@xxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> Baolin Wang <baolin.wang@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> ---
>>> 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?

wouldn't it be nicer to just initialize PM runtime callbacks from
xhci-plat? It just needs to be added and verified, right?

--
balbi

Attachment: signature.asc
Description: PGP signature