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

[Baolin Wang]

On 20 February 2017 at 23:10, Mathias Nyman
<mathias.nyman@xxxxxxxxxxxxxxx> wrote:
> On 20.02.2017 04:47, Baolin Wang wrote:
>>
>> Hi Mathias,
>>
>> On 6 February 2017 at 13:26, Baolin Wang <baolin.wang@xxxxxxxxxx> wrote:
>>>
>>> Hi Mathias,
>>>
>>> On 31 January 2017 at 21:14, Mathias Nyman
>>> <mathias.nyman@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>>>>> 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@xxxxxxxxxx>
>>>>>> ---
>>>>>> 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?
>>
>
> I think I at least partially understand your point. You don't want to enable
> runtime suspend for all xhci platform devices by default, but only for the
> ones
> that are part of dwc3.
>
> The implementation you suggest is that xhci platform driver always increase
> the xhci platform
> device usage counter during probe with pm_runtime_get_noresume(), and never
> decrement it,
> preventing xhci platform devices from runtime suspending by themselves.
>
> You then control xhci runtime suspend from dwc3 driver runtime suspend,
> allowing
> xhci platfrom controller to runtime suspend only when dwc3 runtime suspends
> by decrementing xhci
> platform device usage in dwc3 runtime suspend callback.
> As xhci is a child of dwc3 in this case,  dwc3 would never runtime suspend
> as long as xhci is running, so
> dwc3 needed to be detached from xhci by setting dwc3 to ignore its children
> to get it to work.

Yes, that's my point.

>
> I don't yet understand why we can't just keep runtime pm disabled as a
> default for xhci platform devices.
> It could be enabled by whatever creates the platform device by setting some
> device property
> (or equivalent), which would be checked in xhci_plat_probe() before enabling
> runtime pm. It
> could then optionally be set in sysfs via power/control entry.

I think runtime pm is not one hardware property, is it suitable if we
introduce one device property to enable/disable runtime pm?

>
> We would keep the usage counting intact, and only enable xhci platform
> device runtime pm
> if the platform supports it, and dwc3 and xhci would keep their parent-child
> relationship
> intact and make sure dwc3 can't runtime suspend before xhci.

Yes, that is the benefits.

>
> To the concerns about xhci runtime suspending too early, the codepath you
> describet only applies
> for roothub devices, which sohuld be called only after other devices have
> suspended.
>
> To make sure xhci platform device does not runtime suspend during probe
> after first
> hcd is added (without children), but before second one, make sure to
> increase the usage count before creating and adding
> both hcd's, and only decrease it after both are  ready.
>
> This way it will only runtime suspend after both buses are created.
>
> xhci_plat_probe(pdev)
>   /* prevent platform device runtime suspend between adding main and
> shared_hcd */
>   pm_runtime_get_noresume(&pdev->dev)
>   usb_create_hcd()
>   usb_create_shared_hcd()
>   usb_add_hcd(hcd)
>   usb_add_hcd(shared_hcd)
>   /* both hcs's added, allow platform device to runtime suspend */
>   pm_runtime_put_noidle(&dev->dev);
>

Make sense.

-- 
Baolin.wang
Best Regards