runtime PM: common hooks for static and runtime PM

runtime PM: common hooks for static and runtime PM

[Kevin Hilman]

Hello,

I'm implementing runtime PM for the TI OMAP SoCs by overriding the
platform_bus hooks.  All is working well for runtime PM, but it's 
brought up a couple snags for static PM.

Most of our drivers don't really need to distinguish between runtime
PM and static PM as we can hit the same power states when idle as we
can in suspend.  Before switching to runtime PM we've been using the
clock framework to do both runtime PM and static PM.  The driver would
disable its clocks & HW when idle and when going into suspend,
typically using a common 'disable' function.

In converting a test driver to runtime PM, I just converted this
common disable function from a clock disable to a
pm_runtime_put_sync() and the common enable function to do a
pm_runtime_get_sync().  This all worked well for runtime PM, resulting
in my platform_pm_runtime_* hooks being called where I can then
disable/re-enable the clocks/HW etc.  So far so good.

However, I'm not able to use the common function in the static suspend
path because dpm_prepare() does a pm_runtime_get_no_resume() which
prevents any runtime PM transitions during suspend.

I understand the motivation for this is probably to prevent runtime PM
transitions during static suspend, and that makes sense.  However, I'm
wondering if there's some other way to handle my problem without
having to have the driver have different paths for static and runtime
PM.

My current hack is to add an "extra" pm_runtime_put_sync() to the
driver's static suspend hook (and 'get' in the resume hook) to
essentially cancel out the 'get' held by dpm_prepare(), but I'm pretty
sure this is not an acceptable long-term solution.

Any other ideas?  Or maybe I've misunderstood something more basic.
Any pointers would be appreciated.

Thanks,

Kevin

Re: runtime PM: common hooks for static and runtime PM

[Alan Stern]

On Wed, 3 Feb 2010, Kevin Hilman wrote:

> Hello,
> 
> I'm implementing runtime PM for the TI OMAP SoCs by overriding the
> platform_bus hooks.  All is working well for runtime PM, but it's 
> brought up a couple snags for static PM.
> 
> Most of our drivers don't really need to distinguish between runtime
> PM and static PM as we can hit the same power states when idle as we
> can in suspend.  Before switching to runtime PM we've been using the
> clock framework to do both runtime PM and static PM.  The driver would
> disable its clocks & HW when idle and when going into suspend,
> typically using a common 'disable' function.
> 
> In converting a test driver to runtime PM, I just converted this
> common disable function from a clock disable to a
> pm_runtime_put_sync() and the common enable function to do a
> pm_runtime_get_sync().  This all worked well for runtime PM, resulting
> in my platform_pm_runtime_* hooks being called where I can then
> disable/re-enable the clocks/HW etc.  So far so good.
> 
> However, I'm not able to use the common function in the static suspend
> path because dpm_prepare() does a pm_runtime_get_no_resume() which
> prevents any runtime PM transitions during suspend.
> 
> I understand the motivation for this is probably to prevent runtime PM
> transitions during static suspend, and that makes sense.  However, I'm
> wondering if there's some other way to handle my problem without
> having to have the driver have different paths for static and runtime
> PM.

The system PM methods could directly call the runtime_suspend and
runtime_resume methods (which presumably is where you actually disable
or enable the clocks etc.), instead of going indirectly through
pm_runtime_put_sync() and pm_runtime_get_sync().

Or alternatively, both sets of PM methods could call a single pair of
routines to handle the clocks etc.

Alan Stern

Re: runtime PM: common hooks for static and runtime PM

[Mark Brown]

On Thu, Feb 04, 2010 at 10:24:48AM -0500, Alan Stern wrote:
> On Wed, 3 Feb 2010, Kevin Hilman wrote:

> > I understand the motivation for this is probably to prevent runtime PM
> > transitions during static suspend, and that makes sense.  However, I'm
> > wondering if there's some other way to handle my problem without
> > having to have the driver have different paths for static and runtime
> > PM.

> The system PM methods could directly call the runtime_suspend and
> runtime_resume methods (which presumably is where you actually disable
> or enable the clocks etc.), instead of going indirectly through
> pm_runtime_put_sync() and pm_runtime_get_sync().

> Or alternatively, both sets of PM methods could call a single pair of
> routines to handle the clocks etc.

One issue with avoiding the indirection is that they'll need to remember
if the device is already suspended in order to avoid doing things like
double disabling of clocks or regulators.  Using the runtime PM calls
would mean that the core would keep track of that for the driver.

Re: runtime PM: common hooks for static and runtime PM

[Alan Stern]

On Fri, 5 Feb 2010, Mark Brown wrote:

> On Thu, Feb 04, 2010 at 10:24:48AM -0500, Alan Stern wrote:
> > On Wed, 3 Feb 2010, Kevin Hilman wrote:
> 
> > > I understand the motivation for this is probably to prevent runtime PM
> > > transitions during static suspend, and that makes sense.  However, I'm
> > > wondering if there's some other way to handle my problem without
> > > having to have the driver have different paths for static and runtime
> > > PM.
> 
> > The system PM methods could directly call the runtime_suspend and
> > runtime_resume methods (which presumably is where you actually disable
> > or enable the clocks etc.), instead of going indirectly through
> > pm_runtime_put_sync() and pm_runtime_get_sync().
> 
> > Or alternatively, both sets of PM methods could call a single pair of
> > routines to handle the clocks etc.
> 
> One issue with avoiding the indirection is that they'll need to remember
> if the device is already suspended in order to avoid doing things like
> double disabling of clocks or regulators.  Using the runtime PM calls
> would mean that the core would keep track of that for the driver.

As you have found out, that approach doesn't work.  You need to figure
out some way of your own to avoid double disabling.  The PM core can't
do it for you, because there may be devices that enter _different_
power states for runtime suspend and system suspend.  For the sake of 
such devices the core _has_ to do double suspends.

Alan Stern

Re: runtime PM: common hooks for static and runtime PM

[Mark Brown]

On Fri, Feb 05, 2010 at 10:41:40AM -0500, Alan Stern wrote:
> On Fri, 5 Feb 2010, Mark Brown wrote:
> > On Thu, Feb 04, 2010 at 10:24:48AM -0500, Alan Stern wrote:
> > > On Wed, 3 Feb 2010, Kevin Hilman wrote:

> > > > I understand the motivation for this is probably to prevent runtime PM
> > > > transitions during static suspend, and that makes sense.  However, I'm
> > > > wondering if there's some other way to handle my problem without
> > > > having to have the driver have different paths for static and runtime
> > > > PM.

> > > The system PM methods could directly call the runtime_suspend and
> > > runtime_resume methods (which presumably is where you actually disable
> > > or enable the clocks etc.), instead of going indirectly through
> > > pm_runtime_put_sync() and pm_runtime_get_sync().

> > > Or alternatively, both sets of PM methods could call a single pair of
> > > routines to handle the clocks etc.

> > One issue with avoiding the indirection is that they'll need to remember
> > if the device is already suspended in order to avoid doing things like
> > double disabling of clocks or regulators.  Using the runtime PM calls
> > would mean that the core would keep track of that for the driver.

> As you have found out, that approach doesn't work.  You need to figure
> out some way of your own to avoid double disabling.  The PM core can't
> do it for you, because there may be devices that enter _different_
> power states for runtime suspend and system suspend.  For the sake of 
> such devices the core _has_ to do double suspends.

Right, but my point is that one reason for wanting to call into the
runtime PM API during vanilla suspend is that the indirection buys you
the reference counting.

I wonder if it's worth the PM core providing an off the shelf suspend
and resume via runtime PM implementation so drivers only need to assign
function pointers?  As Kevin says this is going to be *very* common for
embedded drivers.

Re: runtime PM: common hooks for static and runtime PM

[Alan Stern]

On Fri, 5 Feb 2010, Mark Brown wrote:

> > As you have found out, that approach doesn't work.  You need to figure
> > out some way of your own to avoid double disabling.  The PM core can't
> > do it for you, because there may be devices that enter _different_
> > power states for runtime suspend and system suspend.  For the sake of 
> > such devices the core _has_ to do double suspends.
> 
> Right, but my point is that one reason for wanting to call into the
> runtime PM API during vanilla suspend is that the indirection buys you
> the reference counting.

It's not reference counting; it's just a state variable.  (There _is_ a
reference counter involved, but you don't want to use it.)

> I wonder if it's worth the PM core providing an off the shelf suspend
> and resume via runtime PM implementation so drivers only need to assign
> function pointers?  As Kevin says this is going to be *very* common for
> embedded drivers.

What's so hard about doing this?

	int my_suspend(struct device *dev)
	{
		if (dev->power.runtime_status == RPM_SUSPENDED)
			return 0;
		return my_runtime_suspend(dev);
	}

Or if you prefer, stick the "if" statement at the beginning of your 
suspend method and then set both function pointers to the same method.

Alan Stern

Re: runtime PM: common hooks for static and runtime PM

[Mark Brown]

On Fri, Feb 05, 2010 at 04:40:11PM -0500, Alan Stern wrote:
> On Fri, 5 Feb 2010, Mark Brown wrote:

> > I wonder if it's worth the PM core providing an off the shelf suspend
> > and resume via runtime PM implementation so drivers only need to assign
> > function pointers?  As Kevin says this is going to be *very* common for
> > embedded drivers.

> What's so hard about doing this?
> 
> 	int my_suspend(struct device *dev)
> 	{
> 		if (dev->power.runtime_status == RPM_SUSPENDED)
> 			return 0;
> 		return my_runtime_suspend(dev);
> 	}

> Or if you prefer, stick the "if" statement at the beginning of your 
> suspend method and then set both function pointers to the same method.

It's not that it's hard per se, it's that it feels like it's peering
inside the implementation of the API.  Having the PM core provide
something would make it clear that this is the expected approach and
ensure that there aren't any silly mistakes, in much the same way that
having SIMPLE_DEV_PM_OPS makes the handling of drivers that use the same
suspend path for both suspend to disk and suspend to RAM clear.

Re: runtime PM: common hooks for static and runtime PM

[Alan Stern]

On Fri, 5 Feb 2010, Mark Brown wrote:

> On Fri, Feb 05, 2010 at 04:40:11PM -0500, Alan Stern wrote:
> > On Fri, 5 Feb 2010, Mark Brown wrote:
> 
> > > I wonder if it's worth the PM core providing an off the shelf suspend
> > > and resume via runtime PM implementation so drivers only need to assign
> > > function pointers?  As Kevin says this is going to be *very* common for
> > > embedded drivers.
> 
> > What's so hard about doing this?
> > 
> > 	int my_suspend(struct device *dev)
> > 	{
> > 		if (dev->power.runtime_status == RPM_SUSPENDED)
> > 			return 0;
> > 		return my_runtime_suspend(dev);
> > 	}
> 
> > Or if you prefer, stick the "if" statement at the beginning of your 
> > suspend method and then set both function pointers to the same method.
> 
> It's not that it's hard per se, it's that it feels like it's peering
> inside the implementation of the API.  Having the PM core provide
> something would make it clear that this is the expected approach and
> ensure that there aren't any silly mistakes, in much the same way that
> having SIMPLE_DEV_PM_OPS makes the handling of drivers that use the same
> suspend path for both suspend to disk and suspend to RAM clear.

You mean like providing an is_runtime_suspended() test?  Then you could 
write:

	int my_suspend(struct device *dev)
	{
		if (is_runtime_suspended(dev))
			return 0;
		return my_runtime_suspend(dev);
	}

Or maybe you'd prefer to see a convenient pm_use_runtime_suspend() 
function that you could use for your dev_pm_ops.suspend pointer, which 
would do essentially the same as the above?  (Along with a 
corresponding pm_use_runtime_resume() function, of course.)

Alan Stern

Re: runtime PM: common hooks for static and runtime PM

[Mark Brown]

On Fri, Feb 05, 2010 at 09:57:46PM -0500, Alan Stern wrote:
> On Fri, 5 Feb 2010, Mark Brown wrote:

> > It's not that it's hard per se, it's that it feels like it's peering
> > inside the implementation of the API.  Having the PM core provide

> You mean like providing an is_runtime_suspended() test?  Then you could 
> write:

> 	int my_suspend(struct device *dev)
> 	{
> 		if (is_runtime_suspended(dev))
> 			return 0;
> 		return my_runtime_suspend(dev);
> 	}

> Or maybe you'd prefer to see a convenient pm_use_runtime_suspend() 
> function that you could use for your dev_pm_ops.suspend pointer, which 
> would do essentially the same as the above?  (Along with a 
> corresponding pm_use_runtime_resume() function, of course.)

Either (or both, of course - implementing the second would probably
imply the former).  The ops that can be assigned would be the clearest
option but the accessor function should be enough to make it clear that
this is something drivers are supposed to be doing.

Re: runtime PM: common hooks for static and runtime PM

[Alan Stern]

On Sat, 6 Feb 2010, Mark Brown wrote:

> On Fri, Feb 05, 2010 at 09:57:46PM -0500, Alan Stern wrote:
> > On Fri, 5 Feb 2010, Mark Brown wrote:
> 
> > > It's not that it's hard per se, it's that it feels like it's peering
> > > inside the implementation of the API.  Having the PM core provide
> 
> > You mean like providing an is_runtime_suspended() test?  Then you could 
> > write:
> 
> > 	int my_suspend(struct device *dev)
> > 	{
> > 		if (is_runtime_suspended(dev))
> > 			return 0;
> > 		return my_runtime_suspend(dev);
> > 	}
> 
> > Or maybe you'd prefer to see a convenient pm_use_runtime_suspend() 
> > function that you could use for your dev_pm_ops.suspend pointer, which 
> > would do essentially the same as the above?  (Along with a 
> > corresponding pm_use_runtime_resume() function, of course.)
> 
> Either (or both, of course - implementing the second would probably
> imply the former).  The ops that can be assigned would be the clearest
> option but the accessor function should be enough to make it clear that
> this is something drivers are supposed to be doing.

Providing the new ops would be a little awkward because 
CONFIG_PM_SLEEP and CONFIG_PM_RUNTIME are independent: Either can be 
enabled without the other.  This means the code to call the 
runtime_suspend and runtime_resume methods would have to be split out 
into a separate source file that would get built whenever either config 
option is enabled.  It's doable, just somewhat awkward.

I'll give it a try and we'll see how it turns out...

Alan Stern

Re: runtime PM: common hooks for static and runtime PM

[Alan Stern]

On Sat, 6 Feb 2010, Alan Stern wrote:

> On Sat, 6 Feb 2010, Mark Brown wrote:
> 
> > On Fri, Feb 05, 2010 at 09:57:46PM -0500, Alan Stern wrote:
> > > On Fri, 5 Feb 2010, Mark Brown wrote:
> > 
> > > > It's not that it's hard per se, it's that it feels like it's peering
> > > > inside the implementation of the API.  Having the PM core provide
> > 
> > > You mean like providing an is_runtime_suspended() test?  Then you could 
> > > write:
> > 
> > > 	int my_suspend(struct device *dev)
> > > 	{
> > > 		if (is_runtime_suspended(dev))
> > > 			return 0;
> > > 		return my_runtime_suspend(dev);
> > > 	}
> > 
> > > Or maybe you'd prefer to see a convenient pm_use_runtime_suspend() 
> > > function that you could use for your dev_pm_ops.suspend pointer, which 
> > > would do essentially the same as the above?  (Along with a 
> > > corresponding pm_use_runtime_resume() function, of course.)
> > 
> > Either (or both, of course - implementing the second would probably
> > imply the former).  The ops that can be assigned would be the clearest
> > option but the accessor function should be enough to make it clear that
> > this is something drivers are supposed to be doing.
> 
> Providing the new ops would be a little awkward because 
> CONFIG_PM_SLEEP and CONFIG_PM_RUNTIME are independent: Either can be 
> enabled without the other.  This means the code to call the 
> runtime_suspend and runtime_resume methods would have to be split out 
> into a separate source file that would get built whenever either config 
> option is enabled.  It's doable, just somewhat awkward.
> 
> I'll give it a try and we'll see how it turns out...

And here it is.  You can see that the patch isn't ideal.

Anyway, it should do what you want -- I haven't tested it.

Alan Stern


Index: usb-2.6/include/linux/pm.h
===================================================================
--- usb-2.6.orig/include/linux/pm.h
+++ usb-2.6/include/linux/pm.h
@@ -508,6 +508,9 @@ extern void __suspend_report_result(cons
 		__suspend_report_result(__func__, fn, ret);		\
 	} while (0)
 
+extern int dpm_use_runtime_suspend(struct device *dev);
+extern int dpm_use_runtime_resume(struct device *dev);
+
 #else /* !CONFIG_PM_SLEEP */
 
 #define device_pm_lock() do {} while (0)
@@ -520,8 +523,15 @@ static inline int dpm_suspend_start(pm_m
 
 #define suspend_report_result(fn, ret)		do {} while (0)
 
+#define dpm_use_runtime_suspend		NULL
+#define dpm_use_runtime_resume		NULL
+
 #endif /* !CONFIG_PM_SLEEP */
 
+void rpm_invoke_runtime_idle(struct device *dev);
+int rpm_invoke_runtime_suspend(struct device *dev);
+int rpm_invoke_runtime_resume(struct device *dev);
+
 /* How to reorder dpm_list after device_move() */
 enum dpm_order {
 	DPM_ORDER_NONE,
Index: usb-2.6/include/linux/pm_runtime.h
===================================================================
--- usb-2.6.orig/include/linux/pm_runtime.h
+++ usb-2.6/include/linux/pm_runtime.h
@@ -60,6 +60,11 @@ static inline void device_set_run_wake(s
 	dev->power.run_wake = enable;
 }
 
+static inline bool pm_is_runtime_suspended(struct device *dev)
+{
+	return dev->power.runtime_status == RPM_SUSPENDED;
+}
+
 #else /* !CONFIG_PM_RUNTIME */
 
 static inline int pm_runtime_idle(struct device *dev) { return -ENOSYS; }
@@ -86,6 +91,9 @@ static inline void pm_runtime_put_noidle
 static inline bool device_run_wake(struct device *dev) { return false; }
 static inline void device_set_run_wake(struct device *dev, bool enable) {}
 
+static inline bool pm_is_runtime_suspended(struct device *dev)
+		{ return false; }
+
 #endif /* !CONFIG_PM_RUNTIME */
 
 static inline int pm_runtime_get(struct device *dev)
Index: usb-2.6/drivers/base/power/Makefile
===================================================================
--- usb-2.6.orig/drivers/base/power/Makefile
+++ usb-2.6/drivers/base/power/Makefile
@@ -1,6 +1,6 @@
 obj-$(CONFIG_PM)	+= sysfs.o
-obj-$(CONFIG_PM_SLEEP)	+= main.o
-obj-$(CONFIG_PM_RUNTIME)	+= runtime.o
+obj-$(CONFIG_PM_SLEEP)	+= main.o invoke_runtime.o
+obj-$(CONFIG_PM_RUNTIME)	+= runtime.o invoke_runtime.o
 obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
Index: usb-2.6/drivers/base/power/main.c
===================================================================
--- usb-2.6.orig/drivers/base/power/main.c
+++ usb-2.6/drivers/base/power/main.c
@@ -936,3 +936,52 @@ void __suspend_report_result(const char 
 		printk(KERN_ERR "%s(): %pF returns %d\n", function, fn, ret);
 }
 EXPORT_SYMBOL_GPL(__suspend_report_result);
+
+/* Convenience routines for drivers that want to use the same functions
+ * for system suspend/resume and runtime suspend/resume:
+ * Set driver->pm_ops->suspend = dpm_use_runtime_suspend and
+ * driver->pm_ops->resume = dpm_use_runtime_resume.
+ */
+
+/**
+ * dpm_use_runtime_suspend - Perform system suspend via runtime_suspend method
+ * @dev: Device to suspend.
+ *
+ * This convenience routine implements a system suspend by invoking the
+ * runtime_suspend method.  The method is invoked only if @dev isn't already
+ * runtime-suspended.
+ */
+int dpm_use_runtime_suspend(struct device *dev)
+{
+	int retval;
+
+	if (pm_is_runtime_suspended(dev))
+		return 0;
+	retval = rpm_invoke_runtime_suspend(dev);
+	if (retval == -ENOSYS)
+		retval = 0;
+	return retval;
+}
+EXPORT_SYMBOL_GPL(dpm_use_runtime_suspend);
+
+/**
+ * dpm_use_runtime_resume - Perform system resume via runtime_resume method
+ * @dev: Device to resume.
+ *
+ * This convenience routine implements a system resume by invoking the
+ * runtime_resume method.  If the method is successful then the device's
+ * runtime status is set to RPM_ACTIVE.
+ */
+int dpm_use_runtime_resume(struct device *dev)
+{
+	int retval;
+
+	retval = rpm_invoke_runtime_resume(dev);
+	if (retval == 0) {
+		pm_runtime_disable(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+	}
+	return retval;
+}
+EXPORT_SYMBOL_GPL(dpm_use_runtime_resume);
Index: usb-2.6/drivers/base/power/runtime.c
===================================================================
--- usb-2.6.orig/drivers/base/power/runtime.c
+++ usb-2.6/drivers/base/power/runtime.c
@@ -79,26 +79,9 @@ static int __pm_runtime_idle(struct devi
 
 	dev->power.idle_notification = true;
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle) {
-		spin_unlock_irq(&dev->power.lock);
-
-		dev->bus->pm->runtime_idle(dev);
-
-		spin_lock_irq(&dev->power.lock);
-	} else if (dev->type && dev->type->pm && dev->type->pm->runtime_idle) {
-		spin_unlock_irq(&dev->power.lock);
-
-		dev->type->pm->runtime_idle(dev);
-
-		spin_lock_irq(&dev->power.lock);
-	} else if (dev->class && dev->class->pm
-	    && dev->class->pm->runtime_idle) {
-		spin_unlock_irq(&dev->power.lock);
-
-		dev->class->pm->runtime_idle(dev);
-
-		spin_lock_irq(&dev->power.lock);
-	}
+	spin_unlock_irq(&dev->power.lock);
+	rpm_invoke_runtime_idle(dev);
+	spin_lock_irq(&dev->power.lock);
 
 	dev->power.idle_notification = false;
 	wake_up_all(&dev->power.wait_queue);
@@ -200,32 +183,11 @@ int __pm_runtime_suspend(struct device *
 	dev->power.runtime_status = RPM_SUSPENDING;
 	dev->power.deferred_resume = false;
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend) {
-		spin_unlock_irq(&dev->power.lock);
-
-		retval = dev->bus->pm->runtime_suspend(dev);
-
-		spin_lock_irq(&dev->power.lock);
-		dev->power.runtime_error = retval;
-	} else if (dev->type && dev->type->pm
-	    && dev->type->pm->runtime_suspend) {
-		spin_unlock_irq(&dev->power.lock);
-
-		retval = dev->type->pm->runtime_suspend(dev);
-
-		spin_lock_irq(&dev->power.lock);
-		dev->power.runtime_error = retval;
-	} else if (dev->class && dev->class->pm
-	    && dev->class->pm->runtime_suspend) {
-		spin_unlock_irq(&dev->power.lock);
-
-		retval = dev->class->pm->runtime_suspend(dev);
-
-		spin_lock_irq(&dev->power.lock);
+	spin_unlock_irq(&dev->power.lock);
+	retval = rpm_invoke_runtime_suspend(dev);
+	spin_lock_irq(&dev->power.lock);
+	if (retval != -ENOSYS)
 		dev->power.runtime_error = retval;
-	} else {
-		retval = -ENOSYS;
-	}
 
 	if (retval) {
 		dev->power.runtime_status = RPM_ACTIVE;
@@ -381,32 +343,11 @@ int __pm_runtime_resume(struct device *d
 
 	dev->power.runtime_status = RPM_RESUMING;
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume) {
-		spin_unlock_irq(&dev->power.lock);
-
-		retval = dev->bus->pm->runtime_resume(dev);
-
-		spin_lock_irq(&dev->power.lock);
-		dev->power.runtime_error = retval;
-	} else if (dev->type && dev->type->pm
-	    && dev->type->pm->runtime_resume) {
-		spin_unlock_irq(&dev->power.lock);
-
-		retval = dev->type->pm->runtime_resume(dev);
-
-		spin_lock_irq(&dev->power.lock);
-		dev->power.runtime_error = retval;
-	} else if (dev->class && dev->class->pm
-	    && dev->class->pm->runtime_resume) {
-		spin_unlock_irq(&dev->power.lock);
-
-		retval = dev->class->pm->runtime_resume(dev);
-
-		spin_lock_irq(&dev->power.lock);
+	spin_unlock_irq(&dev->power.lock);
+	retval = rpm_invoke_runtime_resume(dev);
+	spin_lock_irq(&dev->power.lock);
+	if (retval != -ENOSYS)
 		dev->power.runtime_error = retval;
-	} else {
-		retval = -ENOSYS;
-	}
 
 	if (retval) {
 		dev->power.runtime_status = RPM_SUSPENDED;
Index: usb-2.6/drivers/base/power/invoke_runtime.c
===================================================================
--- /dev/null
+++ usb-2.6/drivers/base/power/invoke_runtime.c
@@ -0,0 +1,80 @@
+/*
+ * drivers/base/power/runtime.c - Helper functions for device run-time PM
+ *
+ * Copyright (c) 2009 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc.
+ * Copyright (c) 2010 Alan Stern <stern@rowland.harvard.edu>
+ *
+ * This file is released under the GPLv2.
+ */
+
+#include <linux/pm_runtime.h>
+
+/* The following functions are separated out from runtime.c so that they
+ * can be used even when CONFIG_PM_RUNTIME is disabled.
+ */
+
+/**
+ * rpm_invoke_runtime_idle - Invoke a device's runtime PM idle method(s)
+ * @dev: Device to handle.
+ */
+void rpm_invoke_runtime_idle(struct device *dev)
+{
+	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle) {
+		dev->bus->pm->runtime_idle(dev);
+	} else if (dev->type && dev->type->pm && dev->type->pm->runtime_idle) {
+		dev->type->pm->runtime_idle(dev);
+	} else if (dev->class && dev->class->pm
+	    && dev->class->pm->runtime_idle) {
+		dev->class->pm->runtime_idle(dev);
+	}
+}
+
+/**
+ * rpm_invoke_runtime_suspend - Invoke a device's runtime PM suspend method(s)
+ * @dev: Device to handle.
+ */
+int rpm_invoke_runtime_suspend(struct device *dev)
+{
+	int retval;
+
+	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend) {
+		retval = dev->bus->pm->runtime_suspend(dev);
+		dev->power.runtime_error = retval;
+	} else if (dev->type && dev->type->pm
+	    && dev->type->pm->runtime_suspend) {
+		retval = dev->type->pm->runtime_suspend(dev);
+		dev->power.runtime_error = retval;
+	} else if (dev->class && dev->class->pm
+	    && dev->class->pm->runtime_suspend) {
+		retval = dev->class->pm->runtime_suspend(dev);
+		dev->power.runtime_error = retval;
+	} else {
+		retval = -ENOSYS;
+	}
+	return retval;
+}
+
+/**
+ * rpm_invoke_runtime_resume - Invoke a device's runtime PM resume method(s)
+ * @dev: Device to handle.
+ */
+int rpm_invoke_runtime_resume(struct device *dev)
+{
+	int retval;
+
+	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume) {
+		retval = dev->bus->pm->runtime_resume(dev);
+		dev->power.runtime_error = retval;
+	} else if (dev->type && dev->type->pm
+	    && dev->type->pm->runtime_resume) {
+		retval = dev->type->pm->runtime_resume(dev);
+		dev->power.runtime_error = retval;
+	} else if (dev->class && dev->class->pm
+	    && dev->class->pm->runtime_resume) {
+		retval = dev->class->pm->runtime_resume(dev);
+		dev->power.runtime_error = retval;
+	} else {
+		retval = -ENOSYS;
+	}
+	return retval;
+}
Index: usb-2.6/Documentation/power/runtime_pm.txt
===================================================================
--- usb-2.6.orig/Documentation/power/runtime_pm.txt
+++ usb-2.6/Documentation/power/runtime_pm.txt
@@ -329,6 +329,11 @@ drivers/base/power/runtime.c and include
       'power.runtime_error' is set or 'power.disable_depth' is greater than
       zero)
 
+  bool pm_is_runtime_suspended(struct device *dev);
+    - returns 'true' if the device's run-time PM status is 'suspended'; the
+      status is constantly subject to change, hence this function should be
+      used only in contexts such as during a system sleep transition
+
 It is safe to execute the following helper functions from interrupt context:
 
 pm_request_idle()
@@ -342,6 +347,7 @@ pm_suspend_ignore_children()
 pm_runtime_set_active()
 pm_runtime_set_suspended()
 pm_runtime_enable()
+pm_is_runtime_suspended()
 
 5. Run-time PM Initialization, Device Probing and Removal
 

Re: runtime PM: common hooks for static and runtime PM

[Mark Brown]

On Mon, Feb 08, 2010 at 09:54:07AM -0500, Alan Stern wrote:
> On Sat, 6 Feb 2010, Alan Stern wrote:

> > I'll give it a try and we'll see how it turns out...

> And here it is.  You can see that the patch isn't ideal.

> Anyway, it should do what you want -- I haven't tested it.

It's taken a little while to test (and to be frank my testing is paper
thin) but it looks like this does what I'm looking for - thanks!

Re: runtime PM: common hooks for static and runtime PM

[Alan Stern]

On Wed, 24 Feb 2010, Mark Brown wrote:

> On Mon, Feb 08, 2010 at 09:54:07AM -0500, Alan Stern wrote:
> > On Sat, 6 Feb 2010, Alan Stern wrote:
> 
> > > I'll give it a try and we'll see how it turns out...
> 
> > And here it is.  You can see that the patch isn't ideal.
> 
> > Anyway, it should do what you want -- I haven't tested it.
> 
> It's taken a little while to test (and to be frank my testing is paper
> thin) but it looks like this does what I'm looking for - thanks!

But should that patch be submitted for inclusion in the kernel, 
warts and all?

Alan Stern

Re: runtime PM: common hooks for static and runtime PM

[Mark Brown]

On 24 Feb 2010, at 18:56, Alan Stern <stern@rowland.harvard.edu> wrote:

> On Wed, 24 Feb 2010, Mark Brown wrote:
>
>> On Mon, Feb 08, 2010 at 09:54:07AM -0500, Alan Stern wrote:
>>> On Sat, 6 Feb 2010, Alan Stern wrote:
>>
>>>> I'll give it a try and we'll see how it turns out...
>>
>>> And here it is.  You can see that the patch isn't ideal.
>>
>>> Anyway, it should do what you want -- I haven't tested it.
>>
>> It's taken a little while to test (and to be frank my testing is  
>> paper
>> thin) but it looks like this does what I'm looking for - thanks!
>
> But should that patch be submitted for inclusion in the kernel,
> warts and all?

My feeling is that it solves a real problem so yes and if it turns out  
there are issues we can always bug fix it later - if it's not merged  
then cross tree issues would mean that there'd be a substantial lag  
before it could be used in drivers.

Re: runtime PM: common hooks for static and runtime PM

[Kevin Hilman]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Wed, 24 Feb 2010, Mark Brown wrote:
>
>> On Mon, Feb 08, 2010 at 09:54:07AM -0500, Alan Stern wrote:
>> > On Sat, 6 Feb 2010, Alan Stern wrote:
>> 
>> > > I'll give it a try and we'll see how it turns out...
>> 
>> > And here it is.  You can see that the patch isn't ideal.
>> 
>> > Anyway, it should do what you want -- I haven't tested it.
>> 
>> It's taken a little while to test (and to be frank my testing is paper
>> thin) but it looks like this does what I'm looking for - thanks!
>
> But should that patch be submitted for inclusion in the kernel, 
> warts and all?

I vote yes.

Kevin

Re: runtime PM: common hooks for static and runtime PM

[Kevin Hilman]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Wed, 3 Feb 2010, Kevin Hilman wrote:
>
>> Hello,
>> 
>> I'm implementing runtime PM for the TI OMAP SoCs by overriding the
>> platform_bus hooks.  All is working well for runtime PM, but it's 
>> brought up a couple snags for static PM.
>> 
>> Most of our drivers don't really need to distinguish between runtime
>> PM and static PM as we can hit the same power states when idle as we
>> can in suspend.  Before switching to runtime PM we've been using the
>> clock framework to do both runtime PM and static PM.  The driver would
>> disable its clocks & HW when idle and when going into suspend,
>> typically using a common 'disable' function.
>> 
>> In converting a test driver to runtime PM, I just converted this
>> common disable function from a clock disable to a
>> pm_runtime_put_sync() and the common enable function to do a
>> pm_runtime_get_sync().  This all worked well for runtime PM, resulting
>> in my platform_pm_runtime_* hooks being called where I can then
>> disable/re-enable the clocks/HW etc.  So far so good.
>> 
>> However, I'm not able to use the common function in the static suspend
>> path because dpm_prepare() does a pm_runtime_get_no_resume() which
>> prevents any runtime PM transitions during suspend.
>> 
>> I understand the motivation for this is probably to prevent runtime PM
>> transitions during static suspend, and that makes sense.  However, I'm
>> wondering if there's some other way to handle my problem without
>> having to have the driver have different paths for static and runtime
>> PM.
>
> The system PM methods could directly call the runtime_suspend and
> runtime_resume methods (which presumably is where you actually disable
> or enable the clocks etc.), instead of going indirectly through
> pm_runtime_put_sync() and pm_runtime_get_sync().
>

[Revisiting this thread again... with a slightly different problem]

In my case, the driver's runtime_suspend and runtime_resume hooks are
not where the clocks are managed.  The actual hardware enable/disable
is done in the bus-level runtime PM hooks, in this case platform_bus.
So having the system PM methods directly call the drivers runtime PM
methods doesn't help.  In fact, because we handle the hardware at the
bus level, most drivers can live without any runtime PM methods, and
simply use get/put.

I've worked around this temporarily by calling the
bus->pm->runtime_suspend() and ->runtime_resume() methods from the
system PM methods, but am curious if that is an acceptable solution.

Kevin

Re: runtime PM: common hooks for static and runtime PM

[Alan Stern]

On Tue, 16 Mar 2010, Kevin Hilman wrote:

> Alan Stern <stern@rowland.harvard.edu> writes:
> 
> > On Wed, 3 Feb 2010, Kevin Hilman wrote:
> >
> >> Hello,
> >> 
> >> I'm implementing runtime PM for the TI OMAP SoCs by overriding the
> >> platform_bus hooks.  All is working well for runtime PM, but it's 
> >> brought up a couple snags for static PM.
> >> 
> >> Most of our drivers don't really need to distinguish between runtime
> >> PM and static PM as we can hit the same power states when idle as we
> >> can in suspend.  Before switching to runtime PM we've been using the
> >> clock framework to do both runtime PM and static PM.  The driver would
> >> disable its clocks & HW when idle and when going into suspend,
> >> typically using a common 'disable' function.
> >> 
> >> In converting a test driver to runtime PM, I just converted this
> >> common disable function from a clock disable to a
> >> pm_runtime_put_sync() and the common enable function to do a
> >> pm_runtime_get_sync().  This all worked well for runtime PM, resulting
> >> in my platform_pm_runtime_* hooks being called where I can then
> >> disable/re-enable the clocks/HW etc.  So far so good.
> >> 
> >> However, I'm not able to use the common function in the static suspend
> >> path because dpm_prepare() does a pm_runtime_get_no_resume() which
> >> prevents any runtime PM transitions during suspend.
> >> 
> >> I understand the motivation for this is probably to prevent runtime PM
> >> transitions during static suspend, and that makes sense.  However, I'm
> >> wondering if there's some other way to handle my problem without
> >> having to have the driver have different paths for static and runtime
> >> PM.
> >
> > The system PM methods could directly call the runtime_suspend and
> > runtime_resume methods (which presumably is where you actually disable
> > or enable the clocks etc.), instead of going indirectly through
> > pm_runtime_put_sync() and pm_runtime_get_sync().
> >
> 
> [Revisiting this thread again... with a slightly different problem]
> 
> In my case, the driver's runtime_suspend and runtime_resume hooks are
> not where the clocks are managed.  The actual hardware enable/disable
> is done in the bus-level runtime PM hooks, in this case platform_bus.
> So having the system PM methods directly call the drivers runtime PM
> methods doesn't help.  In fact, because we handle the hardware at the
> bus level, most drivers can live without any runtime PM methods, and
> simply use get/put.
> 
> I've worked around this temporarily by calling the
> bus->pm->runtime_suspend() and ->runtime_resume() methods from the
> system PM methods, but am curious if that is an acceptable solution.

If the platform bus manages the clocks from within its runtime-PM 
routines, then it ought to provide a similar service from within its 
system-PM routines.  You could do it by calling the bus's runtime-PM 
routines indirectly through the method pointers (as you do now), or by 
calling the runtime-PM routines directly, or by making the runtime-PM 
routines and the system-PM routines both call a separate common 
function responsible for managing the clocks.

Each of these approaches is acceptable, assuming it doesn't mess up any 
of the other platform drivers in your system.

Alan Stern

Re: runtime PM: common hooks for static and runtime PM

[Kevin Hilman]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Tue, 16 Mar 2010, Kevin Hilman wrote:
>
>> Alan Stern <stern@rowland.harvard.edu> writes:
>> 
>> > On Wed, 3 Feb 2010, Kevin Hilman wrote:
>> >
>> >> Hello,
>> >> 
>> >> I'm implementing runtime PM for the TI OMAP SoCs by overriding the
>> >> platform_bus hooks.  All is working well for runtime PM, but it's 
>> >> brought up a couple snags for static PM.
>> >> 
>> >> Most of our drivers don't really need to distinguish between runtime
>> >> PM and static PM as we can hit the same power states when idle as we
>> >> can in suspend.  Before switching to runtime PM we've been using the
>> >> clock framework to do both runtime PM and static PM.  The driver would
>> >> disable its clocks & HW when idle and when going into suspend,
>> >> typically using a common 'disable' function.
>> >> 
>> >> In converting a test driver to runtime PM, I just converted this
>> >> common disable function from a clock disable to a
>> >> pm_runtime_put_sync() and the common enable function to do a
>> >> pm_runtime_get_sync().  This all worked well for runtime PM, resulting
>> >> in my platform_pm_runtime_* hooks being called where I can then
>> >> disable/re-enable the clocks/HW etc.  So far so good.
>> >> 
>> >> However, I'm not able to use the common function in the static suspend
>> >> path because dpm_prepare() does a pm_runtime_get_no_resume() which
>> >> prevents any runtime PM transitions during suspend.
>> >> 
>> >> I understand the motivation for this is probably to prevent runtime PM
>> >> transitions during static suspend, and that makes sense.  However, I'm
>> >> wondering if there's some other way to handle my problem without
>> >> having to have the driver have different paths for static and runtime
>> >> PM.
>> >
>> > The system PM methods could directly call the runtime_suspend and
>> > runtime_resume methods (which presumably is where you actually disable
>> > or enable the clocks etc.), instead of going indirectly through
>> > pm_runtime_put_sync() and pm_runtime_get_sync().
>> >
>> 
>> [Revisiting this thread again... with a slightly different problem]
>> 
>> In my case, the driver's runtime_suspend and runtime_resume hooks are
>> not where the clocks are managed.  The actual hardware enable/disable
>> is done in the bus-level runtime PM hooks, in this case platform_bus.
>> So having the system PM methods directly call the drivers runtime PM
>> methods doesn't help.  In fact, because we handle the hardware at the
>> bus level, most drivers can live without any runtime PM methods, and
>> simply use get/put.
>> 
>> I've worked around this temporarily by calling the
>> bus->pm->runtime_suspend() and ->runtime_resume() methods from the
>> system PM methods, but am curious if that is an acceptable solution.
>
> If the platform bus manages the clocks from within its runtime-PM 
> routines, then it ought to provide a similar service from within its 
> system-PM routines.  

Hmm, good point.  Currently the platform bus code allows overriding
the runtime PM methods via weak functions (drivers/base/platform.c)
but not the system PM methods.  Below is a patch that allows platforms
to extend the system PM methods of the platform bus as well.

> You could do it by calling the bus's runtime-PM 
> routines indirectly through the method pointers (as you do now), or by 
> calling the runtime-PM routines directly, or by making the runtime-PM 
> routines and the system-PM routines both call a separate common 
> function responsible for managing the clocks.

Using the patch below, I am able to add custom system PM hooks and then
use common code to manage the clocks for runtime PM and system PM.

Comments?

Kevin


commit ca2173923bae3ba631e12698401ef0b59ec0433c
Author: Kevin Hilman <khilman@deeprootsystems.com>
Date:   Wed Mar 17 09:36:10 2010 -0700

    platform_bus: allow custom extensions to system PM methods
    
    When runtime PM for platform_bus was added, it allowed for platforms
    to customize the runtime PM methods since they are defined as weak
    symbols.
    
    This patch allows platforms to extend the system PM methods with
    custom hooks as well so runtime PM and system PM extensions can be
    managed together.
    
    Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 1ba9d61..a30f850 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -729,6 +729,26 @@ static void platform_pm_complete(struct device *dev)
 
 #ifdef CONFIG_SUSPEND
 
+int __weak platform_pm_suspend_hook(struct device *dev)
+{
+	return 0;
+}
+
+int __weak platform_pm_suspend_noirq_hook(struct device *dev)
+{
+	return 0;
+}
+
+int __weak platform_pm_resume_hook(struct device *dev)
+{
+	return 0;
+}
+
+int __weak platform_pm_resume_noirq_hook(struct device *dev)
+{
+	return 0;
+}
+
 static int platform_pm_suspend(struct device *dev)
 {
 	struct device_driver *drv = dev->driver;
@@ -744,6 +764,8 @@ static int platform_pm_suspend(struct device *dev)
 		ret = platform_legacy_suspend(dev, PMSG_SUSPEND);
 	}
 
+	platform_pm_suspend_hook(dev);
+
 	return ret;
 }
 
@@ -760,6 +782,8 @@ static int platform_pm_suspend_noirq(struct device *dev)
 			ret = drv->pm->suspend_noirq(dev);
 	}
 
+	platform_pm_suspend_noirq_hook(dev);
+
 	return ret;
 }
 
@@ -768,6 +792,8 @@ static int platform_pm_resume(struct device *dev)
 	struct device_driver *drv = dev->driver;
 	int ret = 0;
 
+	platform_pm_resume_hook(dev);
+
 	if (!drv)
 		return 0;
 
@@ -786,6 +812,8 @@ static int platform_pm_resume_noirq(struct device *dev)
 	struct device_driver *drv = dev->driver;
 	int ret = 0;
 
+	platform_pm_resume_noirq_hook(dev);
+
 	if (!drv)
 		return 0;
 

Re: runtime PM: common hooks for static and runtime PM

[Alan Stern]

On Wed, 17 Mar 2010, Kevin Hilman wrote:

> >> In my case, the driver's runtime_suspend and runtime_resume hooks are
> >> not where the clocks are managed.  The actual hardware enable/disable
> >> is done in the bus-level runtime PM hooks, in this case platform_bus.
> >> So having the system PM methods directly call the drivers runtime PM
> >> methods doesn't help.  In fact, because we handle the hardware at the
> >> bus level, most drivers can live without any runtime PM methods, and
> >> simply use get/put.
> >> 
> >> I've worked around this temporarily by calling the
> >> bus->pm->runtime_suspend() and ->runtime_resume() methods from the
> >> system PM methods, but am curious if that is an acceptable solution.
> >
> > If the platform bus manages the clocks from within its runtime-PM 
> > routines, then it ought to provide a similar service from within its 
> > system-PM routines.  
> 
> Hmm, good point.  Currently the platform bus code allows overriding
> the runtime PM methods via weak functions (drivers/base/platform.c)
> but not the system PM methods.  Below is a patch that allows platforms
> to extend the system PM methods of the platform bus as well.
> 
> > You could do it by calling the bus's runtime-PM 
> > routines indirectly through the method pointers (as you do now), or by 
> > calling the runtime-PM routines directly, or by making the runtime-PM 
> > routines and the system-PM routines both call a separate common 
> > function responsible for managing the clocks.
> 
> Using the patch below, I am able to add custom system PM hooks and then
> use common code to manage the clocks for runtime PM and system PM.
> 
> Comments?
> 
> Kevin
> 
> 
> commit ca2173923bae3ba631e12698401ef0b59ec0433c
> Author: Kevin Hilman <khilman@deeprootsystems.com>
> Date:   Wed Mar 17 09:36:10 2010 -0700
> 
>     platform_bus: allow custom extensions to system PM methods
>     
>     When runtime PM for platform_bus was added, it allowed for platforms
>     to customize the runtime PM methods since they are defined as weak
>     symbols.
>     
>     This patch allows platforms to extend the system PM methods with
>     custom hooks as well so runtime PM and system PM extensions can be
>     managed together.
>     
>     Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 1ba9d61..a30f850 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -729,6 +729,26 @@ static void platform_pm_complete(struct device *dev)
>  
>  #ifdef CONFIG_SUSPEND

This probably should be CONFIG_SLEEP.

>  
> +int __weak platform_pm_suspend_hook(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +int __weak platform_pm_suspend_noirq_hook(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +int __weak platform_pm_resume_hook(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +int __weak platform_pm_resume_noirq_hook(struct device *dev)
> +{
> +	return 0;
> +}
> +
>  static int platform_pm_suspend(struct device *dev)
>  {
>  	struct device_driver *drv = dev->driver;
> @@ -744,6 +764,8 @@ static int platform_pm_suspend(struct device *dev)
>  		ret = platform_legacy_suspend(dev, PMSG_SUSPEND);
>  	}
>  
> +	platform_pm_suspend_hook(dev);
> +
>  	return ret;
>  }
>  
> @@ -760,6 +782,8 @@ static int platform_pm_suspend_noirq(struct device *dev)
>  			ret = drv->pm->suspend_noirq(dev);
>  	}
>  
> +	platform_pm_suspend_noirq_hook(dev);
> +
>  	return ret;
>  }
>  
> @@ -768,6 +792,8 @@ static int platform_pm_resume(struct device *dev)
>  	struct device_driver *drv = dev->driver;
>  	int ret = 0;
>  
> +	platform_pm_resume_hook(dev);
> +
>  	if (!drv)
>  		return 0;
>  
> @@ -786,6 +812,8 @@ static int platform_pm_resume_noirq(struct device *dev)
>  	struct device_driver *drv = dev->driver;
>  	int ret = 0;
>  
> +	platform_pm_resume_noirq_hook(dev);
> +
>  	if (!drv)
>  		return 0;
>  

It looks reasonable to me, but I'm not actively involved in PM for the 
platform bus.  Magnus Damm might have some suggestions.

Alan Stern

Re: runtime PM: common hooks for static and runtime PM

[Rafael J. Wysocki]

On Wednesday 17 March 2010, Alan Stern wrote:
> On Wed, 17 Mar 2010, Kevin Hilman wrote:
> 
> > >> In my case, the driver's runtime_suspend and runtime_resume hooks are
> > >> not where the clocks are managed.  The actual hardware enable/disable
> > >> is done in the bus-level runtime PM hooks, in this case platform_bus.
> > >> So having the system PM methods directly call the drivers runtime PM
> > >> methods doesn't help.  In fact, because we handle the hardware at the
> > >> bus level, most drivers can live without any runtime PM methods, and
> > >> simply use get/put.
> > >> 
> > >> I've worked around this temporarily by calling the
> > >> bus->pm->runtime_suspend() and ->runtime_resume() methods from the
> > >> system PM methods, but am curious if that is an acceptable solution.
> > >
> > > If the platform bus manages the clocks from within its runtime-PM 
> > > routines, then it ought to provide a similar service from within its 
> > > system-PM routines.  
> > 
> > Hmm, good point.  Currently the platform bus code allows overriding
> > the runtime PM methods via weak functions (drivers/base/platform.c)
> > but not the system PM methods.  Below is a patch that allows platforms
> > to extend the system PM methods of the platform bus as well.
> > 
> > > You could do it by calling the bus's runtime-PM 
> > > routines indirectly through the method pointers (as you do now), or by 
> > > calling the runtime-PM routines directly, or by making the runtime-PM 
> > > routines and the system-PM routines both call a separate common 
> > > function responsible for managing the clocks.
> > 
> > Using the patch below, I am able to add custom system PM hooks and then
> > use common code to manage the clocks for runtime PM and system PM.
> > 
> > Comments?
> > 
> > Kevin
> > 
> > 
> > commit ca2173923bae3ba631e12698401ef0b59ec0433c
> > Author: Kevin Hilman <khilman@deeprootsystems.com>
> > Date:   Wed Mar 17 09:36:10 2010 -0700
> > 
> >     platform_bus: allow custom extensions to system PM methods
> >     
> >     When runtime PM for platform_bus was added, it allowed for platforms
> >     to customize the runtime PM methods since they are defined as weak
> >     symbols.
> >     
> >     This patch allows platforms to extend the system PM methods with
> >     custom hooks as well so runtime PM and system PM extensions can be
> >     managed together.
> >     
> >     Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> > 
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 1ba9d61..a30f850 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -729,6 +729,26 @@ static void platform_pm_complete(struct device *dev)
> >  
> >  #ifdef CONFIG_SUSPEND
> 
> This probably should be CONFIG_SLEEP.

CONFIG_PM_SLEEP to be precise.

> > +int __weak platform_pm_suspend_hook(struct device *dev)
> > +{
> > +	return 0;
> > +}
> > +
> > +int __weak platform_pm_suspend_noirq_hook(struct device *dev)
> > +{
> > +	return 0;
> > +}
> > +
> > +int __weak platform_pm_resume_hook(struct device *dev)
> > +{
> > +	return 0;
> > +}
> > +
> > +int __weak platform_pm_resume_noirq_hook(struct device *dev)
> > +{
> > +	return 0;
> > +}
> > +
> >  static int platform_pm_suspend(struct device *dev)
> >  {
> >  	struct device_driver *drv = dev->driver;
> > @@ -744,6 +764,8 @@ static int platform_pm_suspend(struct device *dev)
> >  		ret = platform_legacy_suspend(dev, PMSG_SUSPEND);
> >  	}
> >  
> > +	platform_pm_suspend_hook(dev);
> > +
> >  	return ret;
> >  }
> >  
> > @@ -760,6 +782,8 @@ static int platform_pm_suspend_noirq(struct device *dev)
> >  			ret = drv->pm->suspend_noirq(dev);
> >  	}
> >  
> > +	platform_pm_suspend_noirq_hook(dev);
> > +
> >  	return ret;
> >  }
> >  
> > @@ -768,6 +792,8 @@ static int platform_pm_resume(struct device *dev)
> >  	struct device_driver *drv = dev->driver;
> >  	int ret = 0;
> >  
> > +	platform_pm_resume_hook(dev);
> > +
> >  	if (!drv)
> >  		return 0;
> >  
> > @@ -786,6 +812,8 @@ static int platform_pm_resume_noirq(struct device *dev)
> >  	struct device_driver *drv = dev->driver;
> >  	int ret = 0;
> >  
> > +	platform_pm_resume_noirq_hook(dev);
> > +
> >  	if (!drv)
> >  		return 0;
> >  
> 
> It looks reasonable to me, but I'm not actively involved in PM for the 
> platform bus.  Magnus Damm might have some suggestions.

Yes, I think Magnus is the right person to ask for comments.

Rafael

Re: runtime PM: common hooks for static and runtime PM

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> On Wednesday 17 March 2010, Alan Stern wrote:
>> On Wed, 17 Mar 2010, Kevin Hilman wrote:
>> 
>> > >> In my case, the driver's runtime_suspend and runtime_resume hooks are
>> > >> not where the clocks are managed.  The actual hardware enable/disable
>> > >> is done in the bus-level runtime PM hooks, in this case platform_bus.
>> > >> So having the system PM methods directly call the drivers runtime PM
>> > >> methods doesn't help.  In fact, because we handle the hardware at the
>> > >> bus level, most drivers can live without any runtime PM methods, and
>> > >> simply use get/put.
>> > >> 
>> > >> I've worked around this temporarily by calling the
>> > >> bus->pm->runtime_suspend() and ->runtime_resume() methods from the
>> > >> system PM methods, but am curious if that is an acceptable solution.
>> > >
>> > > If the platform bus manages the clocks from within its runtime-PM 
>> > > routines, then it ought to provide a similar service from within its 
>> > > system-PM routines.  
>> > 
>> > Hmm, good point.  Currently the platform bus code allows overriding
>> > the runtime PM methods via weak functions (drivers/base/platform.c)
>> > but not the system PM methods.  Below is a patch that allows platforms
>> > to extend the system PM methods of the platform bus as well.
>> > 
>> > > You could do it by calling the bus's runtime-PM 
>> > > routines indirectly through the method pointers (as you do now), or by 
>> > > calling the runtime-PM routines directly, or by making the runtime-PM 
>> > > routines and the system-PM routines both call a separate common 
>> > > function responsible for managing the clocks.
>> > 
>> > Using the patch below, I am able to add custom system PM hooks and then
>> > use common code to manage the clocks for runtime PM and system PM.
>> > 
>> > Comments?
>> > 
>> > Kevin
>> > 
>> > 
>> > commit ca2173923bae3ba631e12698401ef0b59ec0433c
>> > Author: Kevin Hilman <khilman@deeprootsystems.com>
>> > Date:   Wed Mar 17 09:36:10 2010 -0700
>> > 
>> >     platform_bus: allow custom extensions to system PM methods
>> >     
>> >     When runtime PM for platform_bus was added, it allowed for platforms
>> >     to customize the runtime PM methods since they are defined as weak
>> >     symbols.
>> >     
>> >     This patch allows platforms to extend the system PM methods with
>> >     custom hooks as well so runtime PM and system PM extensions can be
>> >     managed together.
>> >     
>> >     Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>> > 
>> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> > index 1ba9d61..a30f850 100644
>> > --- a/drivers/base/platform.c
>> > +++ b/drivers/base/platform.c
>> > @@ -729,6 +729,26 @@ static void platform_pm_complete(struct device *dev)
>> >  
>> >  #ifdef CONFIG_SUSPEND
>> 
>> This probably should be CONFIG_SLEEP.
>
> CONFIG_PM_SLEEP to be precise.

That #ifdef was not part of my patch, it was just part of the diff
context.  I'm just adding these new hooks inside the same ifdef that
the platform_bus system PM hooks are defined.

Are you suggesting I also fixup the #ifdef in that code?

>> > +int __weak platform_pm_suspend_hook(struct device *dev)
>> > +{
>> > +	return 0;
>> > +}
>> > +
>> > +int __weak platform_pm_suspend_noirq_hook(struct device *dev)
>> > +{
>> > +	return 0;
>> > +}
>> > +
>> > +int __weak platform_pm_resume_hook(struct device *dev)
>> > +{
>> > +	return 0;
>> > +}
>> > +
>> > +int __weak platform_pm_resume_noirq_hook(struct device *dev)
>> > +{
>> > +	return 0;
>> > +}
>> > +
>> >  static int platform_pm_suspend(struct device *dev)
>> >  {
>> >  	struct device_driver *drv = dev->driver;
>> > @@ -744,6 +764,8 @@ static int platform_pm_suspend(struct device *dev)
>> >  		ret = platform_legacy_suspend(dev, PMSG_SUSPEND);
>> >  	}
>> >  
>> > +	platform_pm_suspend_hook(dev);
>> > +
>> >  	return ret;
>> >  }
>> >  
>> > @@ -760,6 +782,8 @@ static int platform_pm_suspend_noirq(struct device *dev)
>> >  			ret = drv->pm->suspend_noirq(dev);
>> >  	}
>> >  
>> > +	platform_pm_suspend_noirq_hook(dev);
>> > +
>> >  	return ret;
>> >  }
>> >  
>> > @@ -768,6 +792,8 @@ static int platform_pm_resume(struct device *dev)
>> >  	struct device_driver *drv = dev->driver;
>> >  	int ret = 0;
>> >  
>> > +	platform_pm_resume_hook(dev);
>> > +
>> >  	if (!drv)
>> >  		return 0;
>> >  
>> > @@ -786,6 +812,8 @@ static int platform_pm_resume_noirq(struct device *dev)
>> >  	struct device_driver *drv = dev->driver;
>> >  	int ret = 0;
>> >  
>> > +	platform_pm_resume_noirq_hook(dev);
>> > +
>> >  	if (!drv)
>> >  		return 0;
>> >  
>> 
>> It looks reasonable to me, but I'm not actively involved in PM for the 
>> platform bus.  Magnus Damm might have some suggestions.
>
> Yes, I think Magnus is the right person to ask for comments.

OK, will post a slighly updated version to a broader audience.

Kevin

Re: runtime PM: common hooks for static and runtime PM

[Alan Stern]

On Wed, 17 Mar 2010, Kevin Hilman wrote:

> >> > @@ -729,6 +729,26 @@ static void platform_pm_complete(struct device *dev)
> >> >  
> >> >  #ifdef CONFIG_SUSPEND
> >> 
> >> This probably should be CONFIG_SLEEP.
> >
> > CONFIG_PM_SLEEP to be precise.
> 
> That #ifdef was not part of my patch, it was just part of the diff
> context.  I'm just adding these new hooks inside the same ifdef that
> the platform_bus system PM hooks are defined.
> 
> Are you suggesting I also fixup the #ifdef in that code?

That would be a good idea.  It can be done in a separate patch.

The point is that CONFIG_SUSPEND is defined only when you enable
standby or suspend-to-RAM, and CONFIG_HIBERNATION is defined only when
you enable suspend-to-disk, whereas CONFIG_SLEEP is defined when either
of the others is.

Alan Stern