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

[Kevin Hilman <khilman@deeprootsystems.com>]

"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