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

["Rafael J. Wysocki" <rjw@sisk.pl>]

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