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

[Felipe Balbi <balbi@kernel.org>]

Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>> 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.

IMHO, that's a bug in xhci-plat, not something dwc3 should work around.

-- 
balbi