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

[Kevin Hilman <khilman@deeprootsystems.com>]

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;