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 -- 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
