Re: [PATCH v5 1/2] usb: host: plat: Enable xhci plat runtime PM

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

Hi Mathias,

On 6 February 2017 at 13:26, Baolin Wang <baolin.wang@linaro.org> wrote:
> Hi Mathias,
>
> On 31 January 2017 at 21:14, Mathias Nyman
> <mathias.nyman@linux.intel.com> wrote:
>> On 16.01.2017 12:56, Baolin Wang wrote:
>>>
>>> Hi Mathias,
>>
>>
>> Hi
>>
>> Sorry about the long review delay
>> CC Alan in case my pm assumptions need to be corrected
>>
>>
>>>
>>> On 13 December 2016 at 15:49, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>>
>>>> Enable the xhci plat runtime PM for parent device to suspend/resume xhci.
>>>> Also call pm_runtime_get_noresume() in probe() function in case the
>>>> parent
>>>> device doesn't call suspend/resume callback by runtime PM now.
>>>>
>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>> ---
>>>> Changes since v4:
>>>>   - No updates.
>>>>
>>>> Changes since v3:
>>>>   - Fix kbuild error.
>>>>
>>>> Changes since v2:
>>>>   - Add pm_runtime_get_noresume() in probe() function.
>>>>   - Add pm_runtime_set_suspended()/pm_runtime_put_noidle() in remove()
>>>> function.
>>>>
>>>> Changes since v1:
>>>>   - No updates.
>>>> ---
>>>
>>>
>>> Do you have any comments about this patch? Thanks.
>>>
>>>>   drivers/usb/host/xhci-plat.c |   41
>>>> ++++++++++++++++++++++++++++++++++++-----
>>>>   1 file changed, 36 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>>>> index ed56bf9..5805c6a 100644
>>>> --- a/drivers/usb/host/xhci-plat.c
>>>> +++ b/drivers/usb/host/xhci-plat.c
>>>> @@ -246,6 +246,10 @@ static int xhci_plat_probe(struct platform_device
>>>> *pdev)
>>>>          if (ret)
>>>>                  goto dealloc_usb2_hcd;
>>>>
>>>> +       pm_runtime_get_noresume(&pdev->dev);
>>>> +       pm_runtime_set_active(&pdev->dev);
>>>> +       pm_runtime_enable(&pdev->dev);
>>>> +
>>>>          return 0;
>>>>
>>>>
>>
>> To me this looks like we are not really enabling runtime pm for xhci, we
>> increment the
>> usage count in probe, and keep it until remove is called.
>>
>> This would normally prevent parent dwc3 from runtime suspending, but I see
>> that in patch 2/2 adds
>> pm_suspend_ignore_children(dev, true) to dwc3, and, that dwc3 actually
>> controls xhci runtime
>> pm by calling pm_runtime_get/put for xhci in its own dwc3
>> runtime_suspend/resume callbacks.
>>
>> Looks a bit upside down to me, what's the reason for this?
>>
>> what prevents calling pm_runtime_put_* before leaving probe in xhci and let
>> pm code handle
>> the runtime suspend and parent/child relationships?
>
> When dwc3 controller is working on host mode, which will create and
> add the USB hcd for host side. Then if we want to suspend dwc3
> controller as host mode, the USB device (bus) of host side will
> runtime suspend automatically if there are no slave attached (by
> usb_runtime_suspend()--->usb_suspend_both()--->usb_suspend_interface()--->usb_suspend_device()--->generic_suspend()--->hcd_bus_suspend()--->xhci_bus_suspend()),
> but we should not suspend xHCI device (issuing xhci_suspend()) now,
> since some other  controllers did not implement the runtime PM to
> control xHCI device's runtime state, which will cause other
> controllers can not resume xHCI device (issuing xhci_resume()) if the
> xHCI device suspend automatically by its child device. Thus we should
> get the runtime count for xHCI device in xhci_plat_probe() in case
> xHCI device will also suspend automatically by its child device.
> According to that, for xHCI device's parent: dwc3 device, we should
> put the xHCI device's runtime count to suspend xHCI device manually.
>

Any more comments?

-- 
Baolin.wang
Best Regards