Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1459865 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PACTH v6 1/2] usb: xhci: plat: Enable runtime PM robert.foss@collabora.com - 2016-08-10 22:40 +0200
    Re: [PACTH,v6,1/2] usb: xhci: plat: Enable runtime PM Brian Norris <briannorris@chromium.org> - 2016-08-23 05:30 +0200
      Re: [PACTH,v6,1/2] usb: xhci: plat: Enable runtime PM Robert Foss <robert.foss@collabora.com> - 2016-08-24 23:00 +0200

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

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

[toc] | [next] | [standalone]

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

+ 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 = {

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web