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

[Baolin Wang <baolin.wang@linaro.org>]

Hi,

On 8 December 2016 at 19:02, Felipe Balbi <balbi@kernel.org> wrote:
>
> 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?

Since we have get the runtime count for xHCI device in
xhci_plat_probe(), in case it will suspend automatically if some
controllers do not want xHCI enters runtime suspend automatically. So
the parent device (dwc3 device) need to get/put the xHCI device's
runtime count  to resume/suspend xHCI.

-- 
Baolin.wang
Best Regards