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? -- Baolin.wang Best Regards -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
