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

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2980 bytes --]


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>>> On 28 November 2016 at 14:43, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>> 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.
>>>>
>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>> ---
>>>> Changes since v3:
>>>>  - No updates.
>>>>
>>>> Changes since v2:
>>>>  - Remove pm_children_suspended() and other unused macros.
>>>>
>>>>  Changes since v1:
>>>>    - Add pm_runtime.h head file to avoid kbuild error.
>>>> ---
>>>>  drivers/usb/dwc3/Kconfig |    7 +++++++
>>>>  drivers/usb/dwc3/core.c  |   26 +++++++++++++++++++++++++-
>>>>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>>>>  drivers/usb/dwc3/host.c  |   37 +++++++++++++++++++++++++++++++++++++
>>>>  4 files changed, 84 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>>> index a45b4f1..47bb2f3 100644
>>>> --- a/drivers/usb/dwc3/Kconfig
>>>> +++ b/drivers/usb/dwc3/Kconfig
>>>> @@ -47,6 +47,13 @@ config USB_DWC3_DUAL_ROLE
>>>>
>>>>  endchoice
>>>>
>>>> +config USB_DWC3_HOST_SUSPEND
>>>> +       bool "Choose if the DWC3 host (xhci) can be suspend/resume"
>>>> +       depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
>>
>> why do you need another Kconfig for this? Just enable it already :-p
>
> I assume some platforms may do not need this feature. But okay, I can
> remove this kconfig and enable it.

thanks :-)

>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 9a4a5e4..7ad4bc3 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -1091,6 +1091,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);
>>
>> why do you need this?
>
> Since the dwc3 device is the parent deive of xhci device, if we want
> to suspend dwc3 device, we need to suspend child device (xhci device)
> manually by issuing pm_runtime_put_sync() in dwc3_host_suspend(). In
> pm_runtime_put_sync(), it will check if the children (xhci device) of
> dwc3 device have been in suspend state, if not we will suspend dwc3
> device failed.
>
> We get/put the child device manually in parent device's runtime
> callback, we need to ignore the child device's runtime state, or it
> will failed due to the dependency.

I see. Good explanation :-)

There's one thing though, if you want to runtime suspend the gadget and
dwc3 is working on peripheral mode, host side (XHCI) should already be
runtime suspended because there's nothing attached to it. Why isn't it
runtime suspended?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]