| Message ID | 1362230590-20960-7-git-send-email-gautam.vivek@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ?
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-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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.
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
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-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)