From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752000AbdBTCrL (ORCPT ); Sun, 19 Feb 2017 21:47:11 -0500 Received: from mail-oi0-f47.google.com ([209.85.218.47]:36196 "EHLO mail-oi0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751789AbdBTCrJ (ORCPT ); Sun, 19 Feb 2017 21:47:09 -0500 MIME-Version: 1.0 In-Reply-To: References: <58908DBC.5010208@linux.intel.com> From: Baolin Wang Date: Mon, 20 Feb 2017 10:47:08 +0800 Message-ID: Subject: Re: [PATCH v5 1/2] usb: host: plat: Enable xhci plat runtime PM To: Mathias Nyman Cc: Mathias Nyman , Felipe Balbi , Greg KH , Mark Brown , USB , LKML , Alan Stern Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Mathias, On 6 February 2017 at 13:26, Baolin Wang wrote: > Hi Mathias, > > On 31 January 2017 at 21:14, Mathias Nyman > 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 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 >>>> --- >>>> 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