Hi,
On 21 July 2017 at 17:13, Manu Gautam mgautam@codeaurora.org wrote:
Hi,
On 7/21/2017 2:31 PM, Baolin Wang wrote:
On 21 July 2017 at 16:45, Manu Gautam mgautam@codeaurora.org wrote:
Hi,
On 7/21/2017 12:28 PM, Baolin Wang wrote:
For some mobile devices with strict power management, we also want to suspend the host when the slave was detached for power saving. Thus adding the host suspend/resume functions to support this requirement.
USB/PM core already takes care of suspending root-HUB/XHCI when no device connected.
Correct, but what I mean is we can power off the dwc3 controller when there are no device connected.
We will issue the pm_suspend_ignore_children() for the dwc3 device, since we will resume the child device (xHCI device) in runtime resume callback (dwc3_host_resume()) of dwc3 device, now the dwc3 device's runtime state is not RPM_ACTIVE, which will block to resume its child device (xHCI device). Add ignore children flag can avoid this situation.
This defeats the basic purpose of runtime PM. Without ignore_children once USB bus is suspended, dwc3 gets suspended and then dwc3 glue device. Only requirement I see from the patch is to resume XHCI/root-hub on dwc3 resume. I am sure there must be other way to deal with that e.g. doing same from glue driver by using a wq or use pm_request_resume()
Ah, I will try pm_request_resume().
Signed-off-by: Baolin Wang baolin.wang@linaro.org
drivers/usb/dwc3/core.c | 26 +++++++++++++++++++++++++- drivers/usb/dwc3/core.h | 7 +++++++ drivers/usb/dwc3/host.c | 15 +++++++++++++++ 3 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 326b302..2be7ddc 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -1193,6 +1193,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); ret = pm_runtime_get_sync(dev); if (ret < 0) goto err1;@@ -1292,15 +1293,27 @@ static int dwc3_remove(struct platform_device *pdev) static int dwc3_suspend_common(struct dwc3 *dwc)
What is the trigger for runtime suspend now that you have ignore_children set.
{ 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);With DRD/OTG, if current mode is device and dwc3->xhci won't be valid.
If current mode is device, then xHCI device is always in suspend state.
dwc->xhci is allocated in dwc3_host_init. And dwc->xhci device is unregistred from dwc3_host_exit that is called from __dwc3_set_mode.
Yes, you are right. I think I need check the dwc->current_dr_role to identify host or device mode.
You can refer to the patch that I pushed to address this.
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;@@ -1322,12 +1335,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 ea910ac..9b5a4eb 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -1193,11 +1193,18 @@ static inline bool dwc3_is_usb31(struct dwc3 *dwc) #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE) int dwc3_host_init(struct dwc3 *dwc); void dwc3_host_exit(struct dwc3 *dwc); +int dwc3_host_suspend(struct dwc3 *dwc); +int dwc3_host_resume(struct dwc3 *dwc); #else static inline int dwc3_host_init(struct dwc3 *dwc) { return 0; } static inline void dwc3_host_exit(struct dwc3 *dwc) { }
+static inline int dwc3_host_suspend(struct dwc3 *dwc) +{ return 0; } +static inline int dwc3_host_resume(struct dwc3 *dwc) +{ return 0; } #endif
#if IS_ENABLED(CONFIG_USB_DWC3_GADGET) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE) diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c index 76f0b0d..3fa4414 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"
@@ -151,3 +152,17 @@ void dwc3_host_exit(struct dwc3 *dwc) dev_name(dwc->dev)); platform_device_unregister(dwc->xhci); }
+int dwc3_host_suspend(struct dwc3 *dwc) +{
struct device *xhci = &dwc->xhci->dev;return pm_runtime_suspend(xhci);If ignore_children is not marked then xhci would indeed be suspended.
+}
+int dwc3_host_resume(struct dwc3 *dwc) +{
struct device *xhci = &dwc->xhci->dev;return pm_runtime_resume(xhci);Other than what I pushed in my patch - ("usb: dwc3: Don't reinitialize core during host bus-suspend/resume") Just performing pm_request_resume or handling same in dwc3 glue driver should be sufficient.
Yes.
Also, what is trigger for runtime_resume for your platform?
In our platform glue layer, we have extcon events to notify there are devices connected, then gule layer will resume dwc3 device.
IMO you can just perform resume of &dwc->xhci->dev instead of resuming dwc3. Since, dwc3 is parent of xhci that will trigger resume of dwc3 as well.
I am not sure this is good enough but it seems can solve some problems, then we do not need resume/suspend host in dwc3 core.