[v2,06/10] usb: xhci: Enable runtime pm in xhci-plat
diff mbox

Message ID 1362230590-20960-7-git-send-email-gautam.vivek@samsung.com
State New, archived
Headers show

Commit Message

Vivek Gautam March 2, 2013, 1:23 p.m. UTC
By enabling runtime pm in this driver allows users of
xhci-plat to enter into runtime pm. This is not full
runtime pm support (AKA xhci-plat doesn't actually power
anything off when in runtime suspend mode) but,
just basic enablement.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
CC: Doug Anderson <dianders@chromium.org>
---
 drivers/usb/host/xhci-plat.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

Comments

Alan Stern March 2, 2013, 3:53 p.m. UTC | #1
On Sat, 2 Mar 2013, Vivek Gautam wrote:

> By enabling runtime pm in this driver allows users of
> xhci-plat to enter into runtime pm. This is not full
> runtime pm support (AKA xhci-plat doesn't actually power
> anything off when in runtime suspend mode) but,
> just basic enablement.
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> CC: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/usb/host/xhci-plat.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index c9c7e13..595cb52 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -12,6 +12,7 @@
>   */
>  
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  
> @@ -149,6 +150,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto put_usb3_hcd;
>  
> +	pm_runtime_enable(&pdev->dev);

This is generally not a good idea.  You shouldn't enable a device for 
runtime PM without first telling the PM core what state it is in.

> @@ -174,6 +177,10 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	struct usb_hcd	*hcd = platform_get_drvdata(dev);
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>  
> +	if (!pm_runtime_suspended(&dev->dev))
> +		pm_runtime_put(&dev->dev);
> +	pm_runtime_disable(&dev->dev);
> +
>  	usb_remove_hcd(xhci->shared_hcd);
>  	usb_put_hcd(xhci->shared_hcd);

This is very strange.  Why have a pm_runtime_put with no balancing 
pm_runtime_get?

And why use an async routine when you're about to unbind the driver?  
Don't you want the callback to occur before the unbinding?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi March 2, 2013, 8:39 p.m. UTC | #2
Hi,

On Sat, Mar 02, 2013 at 10:53:16AM -0500, Alan Stern wrote:
> On Sat, 2 Mar 2013, Vivek Gautam wrote:
> 
> > By enabling runtime pm in this driver allows users of
> > xhci-plat to enter into runtime pm. This is not full
> > runtime pm support (AKA xhci-plat doesn't actually power
> > anything off when in runtime suspend mode) but,
> > just basic enablement.
> > 
> > Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> > CC: Doug Anderson <dianders@chromium.org>
> > ---
> >  drivers/usb/host/xhci-plat.c |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index c9c7e13..595cb52 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -12,6 +12,7 @@
> >   */
> >  
> >  #include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/module.h>
> >  #include <linux/slab.h>
> >  
> > @@ -149,6 +150,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		goto put_usb3_hcd;
> >  
> > +	pm_runtime_enable(&pdev->dev);
> 
> This is generally not a good idea.  You shouldn't enable a device for 
> runtime PM without first telling the PM core what state it is in.
> 
> > @@ -174,6 +177,10 @@ static int xhci_plat_remove(struct platform_device *dev)
> >  	struct usb_hcd	*hcd = platform_get_drvdata(dev);
> >  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> >  
> > +	if (!pm_runtime_suspended(&dev->dev))
> > +		pm_runtime_put(&dev->dev);
> > +	pm_runtime_disable(&dev->dev);
> > +
> >  	usb_remove_hcd(xhci->shared_hcd);
> >  	usb_put_hcd(xhci->shared_hcd);
> 
> This is very strange.  Why have a pm_runtime_put with no balancing 
> pm_runtime_get?

this is good point and, in fact, a doubt I have myself. How are we
supposed to check if device is suspended ? In case it _is_ suspended we
might not be able to read device's registers due to clocks possibly
being gated.

Also, considering that some drivers are used in multiple platforms and
those might behave differently when it comes to clock handling, how do
we do that ? Should we require drivers to explicitly clk_get();
clk_prepare_enable(); pm_runtime_set_active(); pm_runtime_enable() ?

While that's doable, I don't see how that'd be doable for OMAP since
they want to hide clock handling from drivers.

Any tips ?
Alan Stern March 2, 2013, 10:02 p.m. UTC | #3
On Sat, 2 Mar 2013, Felipe Balbi wrote:

> > > @@ -174,6 +177,10 @@ static int xhci_plat_remove(struct platform_device *dev)
> > >  	struct usb_hcd	*hcd = platform_get_drvdata(dev);
> > >  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> > >  
> > > +	if (!pm_runtime_suspended(&dev->dev))
> > > +		pm_runtime_put(&dev->dev);
> > > +	pm_runtime_disable(&dev->dev);
> > > +
> > >  	usb_remove_hcd(xhci->shared_hcd);
> > >  	usb_put_hcd(xhci->shared_hcd);
> > 
> > This is very strange.  Why have a pm_runtime_put with no balancing 
> > pm_runtime_get?
> 
> this is good point and, in fact, a doubt I have myself. How are we
> supposed to check if device is suspended ? In case it _is_ suspended we
> might not be able to read device's registers due to clocks possibly
> being gated.

That's really a separate question.  It has a simple answer, though: If 
you want to access a device's registers, call pm_runtime_get_sync() 
beforehand and pm_runtime_put() (or _put_sync()) afterward.  Then it 
won't matter if the device was suspended originally.

If you actually do want to tell whether or not a device is suspended
and nothing more, call pm_runtime_status_suspended().  Of course, this
is racy -- the power state might change right after you make the call.

> Also, considering that some drivers are used in multiple platforms and
> those might behave differently when it comes to clock handling, how do
> we do that ? Should we require drivers to explicitly clk_get();
> clk_prepare_enable(); pm_runtime_set_active(); pm_runtime_enable() ?

I don't know much about clock handling.  In general, the
pm_runtime_set_active() and pm_runtime_enable() parts should be done by
the subsystem, not the driver, whenever possible.

> While that's doable, I don't see how that'd be doable for OMAP since
> they want to hide clock handling from drivers.
> 
> Any tips ?

Whichever piece of code is responsible for associating a clock with a
device should also be responsible for making sure that neither is
suspended when the driver's probe routine runs.  I'm not sure how 
different platforms do this; using a PM domain could be one answer.

All this is somewhat off the point of my original comment, however.  
Drivers must be sure to balance their pm_runtime_get() and _put()  
calls.  Having an unbalanced _put() in the remove routine is almost
certainly a mistake -- especially if it is conditional on the device's
power state, because a device can remain unsuspended even after the
driver does a pm_runtime_put().  For example, this will happen if the
user wrote "on"  to /sys/.../power/control.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi March 2, 2013, 11:21 p.m. UTC | #4
Hi,

On Sat, Mar 02, 2013 at 05:02:13PM -0500, Alan Stern wrote:
> On Sat, 2 Mar 2013, Felipe Balbi wrote:
> 
> > > > @@ -174,6 +177,10 @@ static int xhci_plat_remove(struct platform_device *dev)
> > > >  	struct usb_hcd	*hcd = platform_get_drvdata(dev);
> > > >  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> > > >  
> > > > +	if (!pm_runtime_suspended(&dev->dev))
> > > > +		pm_runtime_put(&dev->dev);
> > > > +	pm_runtime_disable(&dev->dev);
> > > > +
> > > >  	usb_remove_hcd(xhci->shared_hcd);
> > > >  	usb_put_hcd(xhci->shared_hcd);
> > > 
> > > This is very strange.  Why have a pm_runtime_put with no balancing 
> > > pm_runtime_get?
> > 
> > this is good point and, in fact, a doubt I have myself. How are we
> > supposed to check if device is suspended ? In case it _is_ suspended we
> > might not be able to read device's registers due to clocks possibly
> > being gated.
> 
> That's really a separate question.  It has a simple answer, though: If 
> you want to access a device's registers, call pm_runtime_get_sync() 
> beforehand and pm_runtime_put() (or _put_sync()) afterward.  Then it 
> won't matter if the device was suspended originally.

that's alright, but how do you want me to check if my device is enabled
or not before pm_runtime_enable() ?

> If you actually do want to tell whether or not a device is suspended
> and nothing more, call pm_runtime_status_suspended().  Of course, this
> is racy -- the power state might change right after you make the call.

right.

> > Also, considering that some drivers are used in multiple platforms and
> > those might behave differently when it comes to clock handling, how do
> > we do that ? Should we require drivers to explicitly clk_get();
> > clk_prepare_enable(); pm_runtime_set_active(); pm_runtime_enable() ?
> 
> I don't know much about clock handling.  In general, the
> pm_runtime_set_active() and pm_runtime_enable() parts should be done by
> the subsystem, not the driver, whenever possible.

good to know :-) Though I disagree with calling pm_runtime_enable() at
the subsystem level.

This means we can add pm_runtime_set_active() 

> > While that's doable, I don't see how that'd be doable for OMAP since
> > they want to hide clock handling from drivers.
> > 
> > Any tips ?
> 
> Whichever piece of code is responsible for associating a clock with a
> device should also be responsible for making sure that neither is
> suspended when the driver's probe routine runs.  I'm not sure how 
> different platforms do this; using a PM domain could be one answer.

that's alright, but it generates a different set of problems. That same
piece of code which associates a device with its clock, doesn't really
know if the driver is even available which means we could be enabling
clocks for no reason and just wasting precious battery juice ;-)

> All this is somewhat off the point of my original comment, however.  
> Drivers must be sure to balance their pm_runtime_get() and _put()  
> calls.  Having an unbalanced _put() in the remove routine is almost
> certainly a mistake -- especially if it is conditional on the device's
> power state, because a device can remain unsuspended even after the
> driver does a pm_runtime_put().  For example, this will happen if the
> user wrote "on"  to /sys/.../power/control.

indeed... Makes sense. I'll consider mailing linux-pm for the rest of
the discussion, cheers.
Felipe Balbi March 2, 2013, 11:24 p.m. UTC | #5
Hi,

On Sun, Mar 03, 2013 at 01:21:32AM +0200, Felipe Balbi wrote:
> > I don't know much about clock handling.  In general, the
> > pm_runtime_set_active() and pm_runtime_enable() parts should be done by
> > the subsystem, not the driver, whenever possible.
> 
> good to know :-) Though I disagree with calling pm_runtime_enable() at
> the subsystem level.
> 
> This means we can add pm_runtime_set_active() 

ignore this last line, forgot to delete it.
Vivek Gautam March 4, 2013, 8:08 a.m. UTC | #6
Hi,


On Sat, Mar 2, 2013 at 9:23 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, 2 Mar 2013, Vivek Gautam wrote:
>
>> By enabling runtime pm in this driver allows users of
>> xhci-plat to enter into runtime pm. This is not full
>> runtime pm support (AKA xhci-plat doesn't actually power
>> anything off when in runtime suspend mode) but,
>> just basic enablement.
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> CC: Doug Anderson <dianders@chromium.org>
>> ---
>>  drivers/usb/host/xhci-plat.c |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index c9c7e13..595cb52 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -12,6 +12,7 @@
>>   */
>>
>>  #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>>  #include <linux/module.h>
>>  #include <linux/slab.h>
>>
>> @@ -149,6 +150,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>       if (ret)
>>               goto put_usb3_hcd;
>>
>> +     pm_runtime_enable(&pdev->dev);
>
> This is generally not a good idea.  You shouldn't enable a device for
> runtime PM without first telling the PM core what state it is in.
>
Right, but i am not completely sure on any fixed path to follow for
enabling runtime
power management on a device. :-(
Does it become necessary to "pm_runtime_set_active" or
"pm_runtime_set_suspended" a device
before "pm_runtime_enable" ? pm_runtime_enable would just try to
decrement the disable_depth
of the device.

>> @@ -174,6 +177,10 @@ static int xhci_plat_remove(struct platform_device *dev)
>>       struct usb_hcd  *hcd = platform_get_drvdata(dev);
>>       struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>
>> +     if (!pm_runtime_suspended(&dev->dev))
>> +             pm_runtime_put(&dev->dev);
>> +     pm_runtime_disable(&dev->dev);
>> +
>>       usb_remove_hcd(xhci->shared_hcd);
>>       usb_put_hcd(xhci->shared_hcd);
>
> This is very strange.  Why have a pm_runtime_put with no balancing
> pm_runtime_get?
>
> And why use an async routine when you're about to unbind the driver?
> Don't you want the callback to occur before the unbinding?
>
> Alan Stern
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern March 4, 2013, 3:29 p.m. UTC | #7
On Mon, 4 Mar 2013, Vivek Gautam wrote:

> >> @@ -149,6 +150,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >>       if (ret)
> >>               goto put_usb3_hcd;
> >>
> >> +     pm_runtime_enable(&pdev->dev);
> >
> > This is generally not a good idea.  You shouldn't enable a device for
> > runtime PM without first telling the PM core what state it is in.
> >
> Right, but i am not completely sure on any fixed path to follow for
> enabling runtime
> power management on a device. :-(
> Does it become necessary to "pm_runtime_set_active" or
> "pm_runtime_set_suspended" a device
> before "pm_runtime_enable" ?

Yes, it usually is necesary.  And especially here, because the runtime
PM core sets the initial status of every device to RPM_SUSPENDED.

>  pm_runtime_enable would just try to
> decrement the disable_depth
> of the device.

That's right.  And once that happens, the PM core would think the 
device was suspended even though it was really at full power.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern March 4, 2013, 3:57 p.m. UTC | #8
On Sun, 3 Mar 2013, Felipe Balbi wrote:

> > > this is good point and, in fact, a doubt I have myself. How are we
> > > supposed to check if device is suspended ? In case it _is_ suspended we
> > > might not be able to read device's registers due to clocks possibly
> > > being gated.
> > 
> > That's really a separate question.  It has a simple answer, though: If 
> > you want to access a device's registers, call pm_runtime_get_sync() 
> > beforehand and pm_runtime_put() (or _put_sync()) afterward.  Then it 
> > won't matter if the device was suspended originally.
> 
> that's alright, but how do you want me to check if my device is enabled
> or not before pm_runtime_enable() ?

You're not supposed to check.  Just make sure your own 
pm_runtime_enable() and _disable() calls are done correctly, and let 
the PM core worry about the rest.  :-)

More to the point, the question of what code enables a device for
runtime PM is decided by the subsystem.  Many subsystems will do it
automatically so that their drivers don't have to worry about it.  
Other subsystems will leave it entirely up to the drivers.  You have to
know what the subsystem wants.

In this case, it's appropriate to do the enable here in the probe
routine because the platform core doesn't do it for you.  This also
means the driver should disable the device in the remove routine.

> > I don't know much about clock handling.  In general, the
> > pm_runtime_set_active() and pm_runtime_enable() parts should be done by
> > the subsystem, not the driver, whenever possible.
> 
> good to know :-) Though I disagree with calling pm_runtime_enable() at
> the subsystem level.

It depends on the subsystem.  For some (like the USB host subsystem),
it is appropriate.

> > Whichever piece of code is responsible for associating a clock with a
> > device should also be responsible for making sure that neither is
> > suspended when the driver's probe routine runs.  I'm not sure how 
> > different platforms do this; using a PM domain could be one answer.
> 
> that's alright, but it generates a different set of problems. That same
> piece of code which associates a device with its clock, doesn't really
> know if the driver is even available which means we could be enabling
> clocks for no reason and just wasting precious battery juice ;-)

Better than wasting our precious bodily fluids...  :-)

I guess the best answer is to set up the association but then leave the
device and clocks in a runtime-suspended status.  Then do a
pm_runtime_get_sync() just before calling the driver's probe routine
and a pm_runtime_put_sync() just after calling the remove routine.  
That should leave the device and the clocks in the state the driver
expects.  (But be careful that these two calls don't invoke the
driver's runtime-PM callbacks!)

Probably nobody has thought these problems through very carefully for 
the platform subsystem.  Nevertheless, that's where the decisions need 
to be made.

Alan Stern

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

Patch
diff mbox

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index c9c7e13..595cb52 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -12,6 +12,7 @@ 
  */
 
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 
@@ -149,6 +150,8 @@  static int xhci_plat_probe(struct platform_device *pdev)
 	if (ret)
 		goto put_usb3_hcd;
 
+	pm_runtime_enable(&pdev->dev);
+
 	return 0;
 
 put_usb3_hcd:
@@ -174,6 +177,10 @@  static int xhci_plat_remove(struct platform_device *dev)
 	struct usb_hcd	*hcd = platform_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 
+	if (!pm_runtime_suspended(&dev->dev))
+		pm_runtime_put(&dev->dev);
+	pm_runtime_disable(&dev->dev);
+
 	usb_remove_hcd(xhci->shared_hcd);
 	usb_put_hcd(xhci->shared_hcd);