subject:"\[PACTH,v6,1\/2\] usb\: xhci\: plat\: Enable runtime PM"

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

2016-08-26 Thread 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.f...@collabora.com wrote:
> > >>From: Robert Foss 
> > >>
> > >>Enable runtime PM for the xhci-plat device so that the parent device
> > >>may implement runtime PM.
> > >>
> > >>Signed-off-by: Robert Foss 
> > >>
> > >>Tested-by: Robert Foss 
> > >>---
> > >> 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(>dev);
> > >>+ pm_runtime_enable(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-08-26 Thread 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.f...@collabora.com wrote:
> >>From: Robert Foss 
> >>
> >>Enable runtime PM for the xhci-plat device so that the parent device
> >>may implement runtime PM.
> >>
> >>Signed-off-by: Robert Foss 
> >>
> >>Tested-by: Robert Foss 
> >>---
> >> 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(>dev);
> >>+   pm_runtime_enable(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-08-24 Thread 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.f...@collabora.com wrote:

From: Robert Foss 

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

Signed-off-by: Robert Foss 

Tested-by: Robert Foss 
---
 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(>dev);
+   pm_runtime_enable(>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);
+
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 = {

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-08-22 Thread 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.f...@collabora.com wrote:
> From: Robert Foss 
> 
> Enable runtime PM for the xhci-plat device so that the parent device
> may implement runtime PM.
> 
> Signed-off-by: Robert Foss 
> 
> Tested-by: Robert Foss 
> ---
>  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(>dev);
> + pm_runtime_enable(>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);
> +
>   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 = {
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information