Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753890AbdGUJNq (ORCPT ); Fri, 21 Jul 2017 05:13:46 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:35674 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753388AbdGUJNp (ORCPT ); Fri, 21 Jul 2017 05:13:45 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 5B2A76081E Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=mgautam@codeaurora.org Subject: Re: [PATCH] usb: dwc3: Support the dwc3 host suspend/resume To: Baolin Wang Cc: Felipe Balbi , Greg KH , USB , LKML , Linaro Kernel Mailman List , Mark Brown References: <2086ce508be1c1d75d47c892a5be9593b12fa6f3.1500619870.git.baolin.wang@linaro.org> <0f1345b4-292c-1881-8bba-d81293b9ace8@codeaurora.org> From: Manu Gautam Message-ID: <50e53fba-1f69-6ba1-cc9d-8cb02a3e0468@codeaurora.org> Date: Fri, 21 Jul 2017 14:43:39 +0530 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6966 Lines: 177 Hi, On 7/21/2017 2:31 PM, Baolin Wang wrote: > On 21 July 2017 at 16:45, Manu Gautam 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 >>> --- >>> 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. > >> 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 >>> +#include >>> >>> #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.