[PACTH v6 0/2] usb: xhci: plat: Enable runtime PM

[PACTH v6 0/2] usb: xhci: plat: Enable runtime PM

[robert.foss]

From: Robert Foss <robert.foss@collabora.com>

This series enables runtime PM and asynchronous resume/suspend support for
xhci-plat devices.

Changes since v1:
- Added Signed-off-by: Robert Foss <robert.foss@collabora.com>
- Added proper metadata tags to series

Changes since v2:
- Added missing changelog to cover-letter
- Added error checking to pm_runtime_get_sync() calls

Changes since v3:
- Decrement usage_counter for failed pm_runtime_get*() calls

Changes since v4:
- Added missing brackets

Changes since v5:
- Switched out atomic_dec() calls with pm_runtime_put() calls


Thanks Filipe for having a look at all of these revisions, I appreciate it!


Andrew Bresticker (1):
  usb: xhci: plat: Enable async suspend/resume

Robert Foss (1):
  usb: xhci: plat: Enable runtime PM

 drivers/usb/host/xhci-plat.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

-- 
2.7.4

[PACTH v6 1/2] usb: xhci: plat: Enable runtime PM

[robert.foss]

From: Robert Foss <robert.foss@collabora.com>

Enable runtime PM for the xhci-plat device so that the parent device
may implement runtime PM.

Signed-off-by: Robert Foss <robert.foss@collabora.com>

Tested-by: Robert Foss <robert.foss@collabora.com>
---
 drivers/usb/host/xhci-plat.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index ed56bf9..ba4efe7 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -246,6 +246,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (ret)
 		goto dealloc_usb2_hcd;
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	return 0;
 
 
@@ -274,6 +277,8 @@ static int xhci_plat_remove(struct platform_device *dev)
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 	struct clk *clk = xhci->clk;
 
+	pm_runtime_disable(&dev->dev);
+
 	usb_remove_hcd(xhci->shared_hcd);
 	usb_phy_shutdown(hcd->usb_phy);
 
@@ -292,6 +297,13 @@ static int xhci_plat_suspend(struct device *dev)
 {
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+	int ret;
+
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		pm_runtime_put(dev);
+		return ret;
+	}
 
 	/*
 	 * xhci_suspend() needs `do_wakeup` to know whether host is allowed
@@ -301,15 +313,28 @@ static int xhci_plat_suspend(struct device *dev)
 	 * reconsider this when xhci_plat_suspend enlarges its scope, e.g.,
 	 * also applies to runtime suspend.
 	 */
-	return xhci_suspend(xhci, device_may_wakeup(dev));
+	ret = xhci_suspend(xhci, device_may_wakeup(dev));
+	pm_runtime_put(dev);
+
+	return ret;
 }
 
 static int xhci_plat_resume(struct device *dev)
 {
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+	int ret;
 
-	return xhci_resume(xhci, 0);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		pm_runtime_put(dev);
+		return ret;
+	}
+
+	ret = xhci_resume(xhci, 0);
+	pm_runtime_put(dev);
+
+	return ret;
 }
 
 static const struct dev_pm_ops xhci_plat_pm_ops = {
-- 
2.7.4

[PACTH v6 2/2] usb: xhci: plat: Enable async suspend/resume

[robert.foss]

From: Andrew Bresticker <abrestic@chromium.org>

USB host controllers can take a significant amount of time to suspend
and resume, adding several hundred miliseconds to the kernel resume
time. Since the XHCI controller has no outside dependencies (other than
clocks, which are suspended late/resumed early), allow it to suspend and
resume asynchronously.

Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Tested-by: Andrew Bresticker <abrestic@chromium.org>
Tested-by: Robert Foss <robert.foss@collabora.com>
Signed-off-by: Robert Foss <robert.foss@collabora.com>
---
 drivers/usb/host/xhci-plat.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index ba4efe7..c35b7fe 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -248,6 +248,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
+	device_enable_async_suspend(&pdev->dev);
 
 	return 0;
 
-- 
2.7.4

Re: [PACTH,v6,1/2] usb: xhci: plat: Enable runtime PM

[Brian Norris]

+ others

Hi Robert and Felipe,

I have a few questions for one or both of you. I'm not really an expert
on runtime PM, so please take my questions with a grain of salt.

On Wed, Aug 10, 2016 at 04:32:15PM -0400, robert.foss@collabora.com wrote:
> From: Robert Foss <robert.foss@collabora.com>
> 
> Enable runtime PM for the xhci-plat device so that the parent device
> may implement runtime PM.
> 
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> 
> Tested-by: Robert Foss <robert.foss@collabora.com>
> ---
>  drivers/usb/host/xhci-plat.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index ed56bf9..ba4efe7 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -246,6 +246,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto dealloc_usb2_hcd;
>  
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +

How does it help to enable PM runtime like this, if you don't have any
kind of runtime_{suspend,resume}() callbacks?

I suspect that this patch set was derived from the Chromium OS kernel
tree, where we were supporting a Tegra XHCI chipset:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.10/drivers/usb/host/xhci-tegra.c#1920

It looks like the driver was refactored to not use xhci-plat.c before it
was upstreamed (and runtime PM support was dropped along the way).

So, I'm wondering how I might actually use this? Particularly, I'm
looking at trying out runtime suspend for a DWC3 controller in host
mode, and it looks like I'd have to do some layer-violating calls to
xhci_suspend()/xhci_resume() from the parent dwc3 device, or else
rewrite drivers/usb/dwc3/host.c to avoid using xhci-plat.c.

(I also see that Baolin, CC'd here, was interested in dwc3 [1].)

Or possibly an enlightening question for me: if you don't mind, how are
you utilizing runtime PM in conjunction with xhci-plat.c, Robert?
Presumably some other parent device/driver is doing some additional
management of the XHCI core?

Regards,
Brian

[1] [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume
    https://lkml.org/lkml/2016/7/15/181
    https://patchwork.kernel.org/patch/9231417/

>  	return 0;
>  
>  
> @@ -274,6 +277,8 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>  	struct clk *clk = xhci->clk;
>  
> +	pm_runtime_disable(&dev->dev);
> +
>  	usb_remove_hcd(xhci->shared_hcd);
>  	usb_phy_shutdown(hcd->usb_phy);
>  
> @@ -292,6 +297,13 @@ static int xhci_plat_suspend(struct device *dev)
>  {
>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0) {
> +		pm_runtime_put(dev);
> +		return ret;
> +	}
>  
>  	/*
>  	 * xhci_suspend() needs `do_wakeup` to know whether host is allowed
> @@ -301,15 +313,28 @@ static int xhci_plat_suspend(struct device *dev)
>  	 * reconsider this when xhci_plat_suspend enlarges its scope, e.g.,
>  	 * also applies to runtime suspend.
>  	 */
> -	return xhci_suspend(xhci, device_may_wakeup(dev));
> +	ret = xhci_suspend(xhci, device_may_wakeup(dev));
> +	pm_runtime_put(dev);
> +
> +	return ret;
>  }
>  
>  static int xhci_plat_resume(struct device *dev)
>  {
>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> +	int ret;
>  
> -	return xhci_resume(xhci, 0);
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0) {
> +		pm_runtime_put(dev);
> +		return ret;
> +	}
> +
> +	ret = xhci_resume(xhci, 0);
> +	pm_runtime_put(dev);
> +
> +	return ret;
>  }
>  
>  static const struct dev_pm_ops xhci_plat_pm_ops = {

Re: [PACTH,v6,1/2] usb: xhci: plat: Enable runtime PM

[Robert Foss]

On 2016-08-22 11:23 PM, Brian Norris wrote:
> + others
>
> Hi Robert and Felipe,
>
> I have a few questions for one or both of you. I'm not really an expert
> on runtime PM, so please take my questions with a grain of salt.
>
> On Wed, Aug 10, 2016 at 04:32:15PM -0400, robert.foss@collabora.com wrote:
>> From: Robert Foss <robert.foss@collabora.com>
>>
>> Enable runtime PM for the xhci-plat device so that the parent device
>> may implement runtime PM.
>>
>> Signed-off-by: Robert Foss <robert.foss@collabora.com>
>>
>> Tested-by: Robert Foss <robert.foss@collabora.com>
>> ---
>>  drivers/usb/host/xhci-plat.c | 29 +++++++++++++++++++++++++++--
>>  1 file changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index ed56bf9..ba4efe7 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -246,6 +246,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		goto dealloc_usb2_hcd;
>>
>> +	pm_runtime_set_active(&pdev->dev);
>> +	pm_runtime_enable(&pdev->dev);
>> +
>
> How does it help to enable PM runtime like this, if you don't have any
> kind of runtime_{suspend,resume}() callbacks?

Andrew, I think you understand the inner workings of this code better 
than me, maybe you could give a short summary?

>
> I suspect that this patch set was derived from the Chromium OS kernel
> tree, where we were supporting a Tegra XHCI chipset:
>
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.10/drivers/usb/host/xhci-tegra.c#1920
>
> It looks like the driver was refactored to not use xhci-plat.c before it
> was upstreamed (and runtime PM support was dropped along the way).
>
> So, I'm wondering how I might actually use this? Particularly, I'm
> looking at trying out runtime suspend for a DWC3 controller in host
> mode, and it looks like I'd have to do some layer-violating calls to
> xhci_suspend()/xhci_resume() from the parent dwc3 device, or else
> rewrite drivers/usb/dwc3/host.c to avoid using xhci-plat.c.
>
> (I also see that Baolin, CC'd here, was interested in dwc3 [1].)
>
> Or possibly an enlightening question for me: if you don't mind, how are
> you utilizing runtime PM in conjunction with xhci-plat.c, Robert?
> Presumably some other parent device/driver is doing some additional
> management of the XHCI core?
>
> Regards,
> Brian
>
> [1] [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume
>     https://lkml.org/lkml/2016/7/15/181
>     https://patchwork.kernel.org/patch/9231417/
>
>>  	return 0;
>>
>>
>> @@ -274,6 +277,8 @@ static int xhci_plat_remove(struct platform_device *dev)
>>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>>  	struct clk *clk = xhci->clk;
>>
>> +	pm_runtime_disable(&dev->dev);
>> +
>>  	usb_remove_hcd(xhci->shared_hcd);
>>  	usb_phy_shutdown(hcd->usb_phy);
>>
>> @@ -292,6 +297,13 @@ static int xhci_plat_suspend(struct device *dev)
>>  {
>>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>> +	int ret;
>> +
>> +	ret = pm_runtime_get_sync(dev);
>> +	if (ret < 0) {
>> +		pm_runtime_put(dev);
>> +		return ret;
>> +	}
>>
>>  	/*
>>  	 * xhci_suspend() needs `do_wakeup` to know whether host is allowed
>> @@ -301,15 +313,28 @@ static int xhci_plat_suspend(struct device *dev)
>>  	 * reconsider this when xhci_plat_suspend enlarges its scope, e.g.,
>>  	 * also applies to runtime suspend.
>>  	 */
>> -	return xhci_suspend(xhci, device_may_wakeup(dev));
>> +	ret = xhci_suspend(xhci, device_may_wakeup(dev));
>> +	pm_runtime_put(dev);
>> +
>> +	return ret;
>>  }
>>
>>  static int xhci_plat_resume(struct device *dev)
>>  {
>>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>> +	int ret;
>>
>> -	return xhci_resume(xhci, 0);
>> +	ret = pm_runtime_get_sync(dev);
>> +	if (ret < 0) {
>> +		pm_runtime_put(dev);
>> +		return ret;
>> +	}
>> +
>> +	ret = xhci_resume(xhci, 0);
>> +	pm_runtime_put(dev);
>> +
>> +	return ret;
>>  }
>>
>>  static const struct dev_pm_ops xhci_plat_pm_ops = {

Re: [PACTH,v6,1/2] usb: xhci: plat: Enable runtime PM

[Brian Norris]

Hi,

On Wed, Aug 24, 2016 at 04:48:01PM -0400, Robert Foss wrote:
> On 2016-08-22 11:23 PM, Brian Norris wrote:
> >+ others
> >
> >Hi Robert and Felipe,
> >
> >I have a few questions for one or both of you. I'm not really an expert
> >on runtime PM, so please take my questions with a grain of salt.
> >
> >On Wed, Aug 10, 2016 at 04:32:15PM -0400, robert.foss@collabora.com wrote:
> >>From: Robert Foss <robert.foss@collabora.com>
> >>
> >>Enable runtime PM for the xhci-plat device so that the parent device
> >>may implement runtime PM.
> >>
> >>Signed-off-by: Robert Foss <robert.foss@collabora.com>
> >>
> >>Tested-by: Robert Foss <robert.foss@collabora.com>
> >>---
> >> drivers/usb/host/xhci-plat.c | 29 +++++++++++++++++++++++++++--
> >> 1 file changed, 27 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> >>index ed56bf9..ba4efe7 100644
> >>--- a/drivers/usb/host/xhci-plat.c
> >>+++ b/drivers/usb/host/xhci-plat.c
> >>@@ -246,6 +246,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >> 	if (ret)
> >> 		goto dealloc_usb2_hcd;
> >>
> >>+	pm_runtime_set_active(&pdev->dev);
> >>+	pm_runtime_enable(&pdev->dev);
> >>+
> >
> >How does it help to enable PM runtime like this, if you don't have any
> >kind of runtime_{suspend,resume}() callbacks?
> 
> Andrew, I think you understand the inner workings of this code
> better than me, maybe you could give a short summary?

I believe Andrew is fairly busy, and I already talked with him a bit
about this. This is needed (as per your (or his?) commit message) only
if you have a parent device that wants to implement runtime PM. Now
depending on what you want to do with "runtime PM", this might mean
the parent device has to suspend xhci_{suspend,resume}() on behalf of
xhci-plat.c. Not very nice layering IMO, but it has been done before...

So I guess this comes down to "what does a parent device/driver want to
do"? If that's (e.g.) just putting some PHY into a slightly lower power
mode, then maybe it's fine for xhci-plat not to do anything else. But if
you actually want to completely power down the parent bus, reset an
accompanying dual-role/OTG controller, etc., then this really isn't
sufficient, AFAICT. But maybe that's just an indictment of the poor
structure of dwc3's host-mode support, more than it is an indictment of
this patch.

Brian

Re: [PACTH,v6,1/2] usb: xhci: plat: Enable runtime PM

[Brian Norris]

Corrected a bouncing email address (sorry for the noise)

On Fri, Aug 26, 2016 at 03:11:36PM -0700, Brian Norris wrote:
> Hi,
> 
> On Wed, Aug 24, 2016 at 04:48:01PM -0400, Robert Foss wrote:
> > On 2016-08-22 11:23 PM, Brian Norris wrote:
> > >+ others
> > >
> > >Hi Robert and Felipe,
> > >
> > >I have a few questions for one or both of you. I'm not really an expert
> > >on runtime PM, so please take my questions with a grain of salt.
> > >
> > >On Wed, Aug 10, 2016 at 04:32:15PM -0400, robert.foss@collabora.com wrote:
> > >>From: Robert Foss <robert.foss@collabora.com>
> > >>
> > >>Enable runtime PM for the xhci-plat device so that the parent device
> > >>may implement runtime PM.
> > >>
> > >>Signed-off-by: Robert Foss <robert.foss@collabora.com>
> > >>
> > >>Tested-by: Robert Foss <robert.foss@collabora.com>
> > >>---
> > >> drivers/usb/host/xhci-plat.c | 29 +++++++++++++++++++++++++++--
> > >> 1 file changed, 27 insertions(+), 2 deletions(-)
> > >>
> > >>diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > >>index ed56bf9..ba4efe7 100644
> > >>--- a/drivers/usb/host/xhci-plat.c
> > >>+++ b/drivers/usb/host/xhci-plat.c
> > >>@@ -246,6 +246,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
> > >> 	if (ret)
> > >> 		goto dealloc_usb2_hcd;
> > >>
> > >>+	pm_runtime_set_active(&pdev->dev);
> > >>+	pm_runtime_enable(&pdev->dev);
> > >>+
> > >
> > >How does it help to enable PM runtime like this, if you don't have any
> > >kind of runtime_{suspend,resume}() callbacks?
> > 
> > Andrew, I think you understand the inner workings of this code
> > better than me, maybe you could give a short summary?
> 
> I believe Andrew is fairly busy, and I already talked with him a bit
> about this. This is needed (as per your (or his?) commit message) only
> if you have a parent device that wants to implement runtime PM. Now
> depending on what you want to do with "runtime PM", this might mean
> the parent device has to suspend xhci_{suspend,resume}() on behalf of
> xhci-plat.c. Not very nice layering IMO, but it has been done before...
> 
> So I guess this comes down to "what does a parent device/driver want to
> do"? If that's (e.g.) just putting some PHY into a slightly lower power
> mode, then maybe it's fine for xhci-plat not to do anything else. But if
> you actually want to completely power down the parent bus, reset an
> accompanying dual-role/OTG controller, etc., then this really isn't
> sufficient, AFAICT. But maybe that's just an indictment of the poor
> structure of dwc3's host-mode support, more than it is an indictment of
> this patch.
> 
> Brian