Hi, Baolin Wang writes: > Hi Felipe, > > On 18 August 2016 at 15:33, Felipe Balbi wrote: >> >> Hi, >> >> Baolin Wang 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 >>> --- >>> 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 >>> #include >>> +#include >>> +#include >>> >>> #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