* [PATCH v2 0/2] Add devfreq runtime pm support @ 2013-03-27 13:52 Rajagopal Venkat 2013-03-27 13:52 ` [PATCH v2 1/2] PM / devfreq: Fix compiler warnings Rajagopal Venkat 2013-03-27 13:52 ` [PATCH v2 2/2] PM / devfreq: tie suspend/resume to runtime-pm Rajagopal Venkat 0 siblings, 2 replies; 8+ messages in thread From: Rajagopal Venkat @ 2013-03-27 13:52 UTC (permalink / raw) To: myungjoo.ham, rjw, Kevin Hilman, Alan Stern Cc: patches, linaro-kernel, linux-pm, linux-kernel, Rajagopal Venkat Patch to bind devfreq to runtime pm framework. Instead of explicitly using devfreq_suspend_device() and devfreq_resume_device() apis for devfreq core suspend/resume, let runtime-pm core handle it automatically. Suspend device devfreq core load monitoring with pm_runtime_suspend() and resume back on pm_runtime_resume(). Discussed at http://comments.gmane.org/gmane.linux.linaro.devel/13787 Changes from v1: - improved change log and code comments - added NULL check for devfreq runtime-pm callbacks ----- Rajagopal Venkat (2): PM / devfreq: Fix compiler warnings PM / devfreq: tie suspend/resume to runtime-pm drivers/base/power/runtime.c | 21 ++++++++++++- drivers/devfreq/devfreq.c | 69 +++++++++++++++++++++++++++++++++++++++--- include/linux/devfreq.h | 30 ++++++------------ include/linux/pm.h | 2 ++ 4 files changed, 97 insertions(+), 25 deletions(-) -- 1.7.10.4 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/2] PM / devfreq: Fix compiler warnings 2013-03-27 13:52 [PATCH v2 0/2] Add devfreq runtime pm support Rajagopal Venkat @ 2013-03-27 13:52 ` Rajagopal Venkat 2013-03-28 6:03 ` MyungJoo Ham 2013-03-27 13:52 ` [PATCH v2 2/2] PM / devfreq: tie suspend/resume to runtime-pm Rajagopal Venkat 1 sibling, 1 reply; 8+ messages in thread From: Rajagopal Venkat @ 2013-03-27 13:52 UTC (permalink / raw) To: myungjoo.ham, rjw, Kevin Hilman, Alan Stern Cc: patches, linaro-kernel, linux-pm, linux-kernel, Rajagopal Venkat Fix compiler warnings generated when devfreq is not enabled (CONFIG_PM_DEVFREQ is not set). Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org> --- include/linux/devfreq.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h index 04ad61d..5f1ab92 100644 --- a/include/linux/devfreq.h +++ b/include/linux/devfreq.h @@ -215,7 +215,7 @@ struct devfreq_simple_ondemand_data { #endif #else /* !CONFIG_PM_DEVFREQ */ -static struct devfreq *devfreq_add_device(struct device *dev, +static inline struct devfreq *devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile, const char *governor_name, void *data) @@ -223,34 +223,34 @@ static struct devfreq *devfreq_add_device(struct device *dev, return NULL; } -static int devfreq_remove_device(struct devfreq *devfreq) +static inline int devfreq_remove_device(struct devfreq *devfreq) { return 0; } -static int devfreq_suspend_device(struct devfreq *devfreq) +static inline int devfreq_suspend_device(struct devfreq *devfreq) { return 0; } -static int devfreq_resume_device(struct devfreq *devfreq) +static inline int devfreq_resume_device(struct devfreq *devfreq) { return 0; } -static struct opp *devfreq_recommended_opp(struct device *dev, +static inline struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq, u32 flags) { - return -EINVAL; + return ERR_PTR(-EINVAL); } -static int devfreq_register_opp_notifier(struct device *dev, +static inline int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq) { return -EINVAL; } -static int devfreq_unregister_opp_notifier(struct device *dev, +static inline int devfreq_unregister_opp_notifier(struct device *dev, struct devfreq *devfreq) { return -EINVAL; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] PM / devfreq: Fix compiler warnings 2013-03-27 13:52 ` [PATCH v2 1/2] PM / devfreq: Fix compiler warnings Rajagopal Venkat @ 2013-03-28 6:03 ` MyungJoo Ham 2013-04-02 0:16 ` Rafael J. Wysocki 0 siblings, 1 reply; 8+ messages in thread From: MyungJoo Ham @ 2013-03-28 6:03 UTC (permalink / raw) To: Rajagopal Venkat Cc: rjw, Kevin Hilman, Alan Stern, patches, linaro-kernel, linux-pm, linux-kernel On Wed, Mar 27, 2013 at 10:52 PM, Rajagopal Venkat <rajagopal.venkat@linaro.org> wrote: > Fix compiler warnings generated when devfreq is not enabled > (CONFIG_PM_DEVFREQ is not set). > > Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org> Thanks! Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com> > --- > include/linux/devfreq.h | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > index 04ad61d..5f1ab92 100644 > --- a/include/linux/devfreq.h > +++ b/include/linux/devfreq.h > @@ -215,7 +215,7 @@ struct devfreq_simple_ondemand_data { > #endif > > #else /* !CONFIG_PM_DEVFREQ */ > -static struct devfreq *devfreq_add_device(struct device *dev, > +static inline struct devfreq *devfreq_add_device(struct device *dev, > struct devfreq_dev_profile *profile, > const char *governor_name, > void *data) > @@ -223,34 +223,34 @@ static struct devfreq *devfreq_add_device(struct device *dev, > return NULL; > } > > -static int devfreq_remove_device(struct devfreq *devfreq) > +static inline int devfreq_remove_device(struct devfreq *devfreq) > { > return 0; > } > > -static int devfreq_suspend_device(struct devfreq *devfreq) > +static inline int devfreq_suspend_device(struct devfreq *devfreq) > { > return 0; > } > > -static int devfreq_resume_device(struct devfreq *devfreq) > +static inline int devfreq_resume_device(struct devfreq *devfreq) > { > return 0; > } > > -static struct opp *devfreq_recommended_opp(struct device *dev, > +static inline struct opp *devfreq_recommended_opp(struct device *dev, > unsigned long *freq, u32 flags) > { > - return -EINVAL; > + return ERR_PTR(-EINVAL); > } > > -static int devfreq_register_opp_notifier(struct device *dev, > +static inline int devfreq_register_opp_notifier(struct device *dev, > struct devfreq *devfreq) > { > return -EINVAL; > } > > -static int devfreq_unregister_opp_notifier(struct device *dev, > +static inline int devfreq_unregister_opp_notifier(struct device *dev, > struct devfreq *devfreq) > { > return -EINVAL; > -- > 1.7.10.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- MyungJoo Ham, Ph.D. Mobile Software Platform Lab, DMC Business, Samsung Electronics ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] PM / devfreq: Fix compiler warnings 2013-03-28 6:03 ` MyungJoo Ham @ 2013-04-02 0:16 ` Rafael J. Wysocki 0 siblings, 0 replies; 8+ messages in thread From: Rafael J. Wysocki @ 2013-04-02 0:16 UTC (permalink / raw) To: myungjoo.ham Cc: Rajagopal Venkat, Kevin Hilman, Alan Stern, patches, linaro-kernel, linux-pm, linux-kernel On Thursday, March 28, 2013 03:03:12 PM MyungJoo Ham wrote: > On Wed, Mar 27, 2013 at 10:52 PM, Rajagopal Venkat > <rajagopal.venkat@linaro.org> wrote: > > Fix compiler warnings generated when devfreq is not enabled > > (CONFIG_PM_DEVFREQ is not set). > > > > Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org> > > Thanks! > > Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com> Applied. Thanks, Rafael > > --- > > include/linux/devfreq.h | 16 ++++++++-------- > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > > index 04ad61d..5f1ab92 100644 > > --- a/include/linux/devfreq.h > > +++ b/include/linux/devfreq.h > > @@ -215,7 +215,7 @@ struct devfreq_simple_ondemand_data { > > #endif > > > > #else /* !CONFIG_PM_DEVFREQ */ > > -static struct devfreq *devfreq_add_device(struct device *dev, > > +static inline struct devfreq *devfreq_add_device(struct device *dev, > > struct devfreq_dev_profile *profile, > > const char *governor_name, > > void *data) > > @@ -223,34 +223,34 @@ static struct devfreq *devfreq_add_device(struct device *dev, > > return NULL; > > } > > > > -static int devfreq_remove_device(struct devfreq *devfreq) > > +static inline int devfreq_remove_device(struct devfreq *devfreq) > > { > > return 0; > > } > > > > -static int devfreq_suspend_device(struct devfreq *devfreq) > > +static inline int devfreq_suspend_device(struct devfreq *devfreq) > > { > > return 0; > > } > > > > -static int devfreq_resume_device(struct devfreq *devfreq) > > +static inline int devfreq_resume_device(struct devfreq *devfreq) > > { > > return 0; > > } > > > > -static struct opp *devfreq_recommended_opp(struct device *dev, > > +static inline struct opp *devfreq_recommended_opp(struct device *dev, > > unsigned long *freq, u32 flags) > > { > > - return -EINVAL; > > + return ERR_PTR(-EINVAL); > > } > > > > -static int devfreq_register_opp_notifier(struct device *dev, > > +static inline int devfreq_register_opp_notifier(struct device *dev, > > struct devfreq *devfreq) > > { > > return -EINVAL; > > } > > > > -static int devfreq_unregister_opp_notifier(struct device *dev, > > +static inline int devfreq_unregister_opp_notifier(struct device *dev, > > struct devfreq *devfreq) > > { > > return -EINVAL; > > -- > > 1.7.10.4 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] PM / devfreq: tie suspend/resume to runtime-pm 2013-03-27 13:52 [PATCH v2 0/2] Add devfreq runtime pm support Rajagopal Venkat 2013-03-27 13:52 ` [PATCH v2 1/2] PM / devfreq: Fix compiler warnings Rajagopal Venkat @ 2013-03-27 13:52 ` Rajagopal Venkat 2013-04-03 18:16 ` Kevin Hilman 1 sibling, 1 reply; 8+ messages in thread From: Rajagopal Venkat @ 2013-03-27 13:52 UTC (permalink / raw) To: myungjoo.ham, rjw, Kevin Hilman, Alan Stern Cc: patches, linaro-kernel, linux-pm, linux-kernel, Rajagopal Venkat Devfreq core suspend/resume of a device is explicitly handled by devfreq driver through devfreq_suspend_device() and devfreq_resume_device() apis typically called from runtime suspend/resume callbacks. This patch aims to take away this from devfreq drivers and handle it from runtime-pm core. So that device devfreq core suspend/resume is automatically done with runtime pm suspend/resume. The devfreq drivers shouldn't be concerned on when to suspend/resume the devfreq. This patch is targeted to handle devfreq core load monitoring suspend/resume only. Not the actual hardware itself. All the resources like clocks and regulators must still be handled by devfreq driver using runtime-pm. The sequence of devfreq and device runtime suspend/resume is, pm_runtime_suspend(dev) will first suspend device devfreq (if available) before device is suspended to ensure devfreq load monitoring is stopped and no device resources like clocks are accessed while device suspend is in progress. pm_runtime_resume(dev) will resume device devfreq(if available) after device is resumed to ensure device resources like clocks are ready for use. As devfreq suspend/resume is done automatically from runtime-pm, this patch removes the existing devfreq_suspend_device() and devfreq_resume_device() apis. Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org> --- drivers/base/power/runtime.c | 21 ++++++++++++- drivers/devfreq/devfreq.c | 69 +++++++++++++++++++++++++++++++++++++++--- include/linux/devfreq.h | 18 +++-------- include/linux/pm.h | 2 ++ 4 files changed, 91 insertions(+), 19 deletions(-) diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 3148b10..2438abc 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -11,6 +11,7 @@ #include <linux/export.h> #include <linux/pm_runtime.h> #include <trace/events/rpm.h> +#include <linux/devfreq.h> #include "power.h" static int rpm_resume(struct device *dev, int rpmflags); @@ -406,6 +407,14 @@ static int rpm_suspend(struct device *dev, int rpmflags) __update_runtime_status(dev, RPM_SUSPENDING); + /* + * Try suspending devfreq before device is runtime suspended. + * It wouldn't make sense to continue devfreq load monitoring + * when device is runtime suspended. + */ + if (dev->power.devfreq && dev->power.devfreq->runtime_suspend) + dev->power.devfreq->runtime_suspend(dev); + if (dev->pm_domain) callback = dev->pm_domain->ops.runtime_suspend; else if (dev->type && dev->type->pm) @@ -421,8 +430,12 @@ static int rpm_suspend(struct device *dev, int rpmflags) callback = dev->driver->pm->runtime_suspend; retval = rpm_callback(callback, dev); - if (retval) + if (retval) { + /* Resume back devfreq on device suspend failure */ + if (dev->power.devfreq && dev->power.devfreq->runtime_resume) + dev->power.devfreq->runtime_resume(dev); goto fail; + } no_callback: __update_runtime_status(dev, RPM_SUSPENDED); @@ -661,6 +674,12 @@ static int rpm_resume(struct device *dev, int rpmflags) __update_runtime_status(dev, RPM_ACTIVE); if (parent) atomic_inc(&parent->power.child_count); + /* + * Resume devfreq after runtime resume sequence to ensure + * device resources are available for devfreq load monitoring. + */ + if (dev->power.devfreq && dev->power.devfreq->runtime_resume) + dev->power.devfreq->runtime_resume(dev); } wake_up_all(&dev->power.wait_queue); diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 44c4079..b8dab0d 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -25,6 +25,7 @@ #include <linux/list.h> #include <linux/printk.h> #include <linux/hrtimer.h> +#include <linux/pm_runtime.h> #include "governor.h" static struct class *devfreq_class; @@ -42,6 +43,9 @@ static LIST_HEAD(devfreq_governor_list); static LIST_HEAD(devfreq_list); static DEFINE_MUTEX(devfreq_list_lock); +static void devfreq_runtime_suspend(struct device *dev); +static void devfreq_runtime_resume(struct device *dev); + /** * find_device_devfreq() - find devfreq struct using device pointer * @dev: device pointer used to lookup device devfreq. @@ -387,6 +391,8 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, */ static void _remove_devfreq(struct devfreq *devfreq, bool skip) { + unsigned long flags; + mutex_lock(&devfreq_list_lock); if (IS_ERR(find_device_devfreq(devfreq->dev.parent))) { mutex_unlock(&devfreq_list_lock); @@ -400,6 +406,10 @@ static void _remove_devfreq(struct devfreq *devfreq, bool skip) devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_STOP, NULL); + spin_lock_irqsave(&devfreq->dev.parent->power.lock, flags); + devfreq->dev.parent->power.devfreq = NULL; + spin_unlock_irqrestore(&devfreq->dev.parent->power.lock, flags); + if (devfreq->profile->exit) devfreq->profile->exit(devfreq->dev.parent); @@ -442,6 +452,7 @@ struct devfreq *devfreq_add_device(struct device *dev, { struct devfreq *devfreq; struct devfreq_governor *governor; + unsigned long flags; int err = 0; if (!dev || !profile || !governor_name) { @@ -476,6 +487,12 @@ struct devfreq *devfreq_add_device(struct device *dev, devfreq->previous_freq = profile->initial_freq; devfreq->data = data; devfreq->nb.notifier_call = devfreq_notifier_call; + devfreq->runtime_suspend = devfreq_runtime_suspend; + devfreq->runtime_resume = devfreq_runtime_resume; + spin_lock_irqsave(&dev->power.lock, flags); + dev->power.devfreq = devfreq; + spin_unlock_irqrestore(&dev->power.lock, flags); + devfreq->trans_table = devm_kzalloc(dev, sizeof(unsigned int) * devfreq->profile->max_state * @@ -549,7 +566,7 @@ EXPORT_SYMBOL(devfreq_remove_device); * (e.g., runtime_suspend, suspend) of the device driver that * holds the devfreq. */ -int devfreq_suspend_device(struct devfreq *devfreq) +static int devfreq_suspend_device(struct devfreq *devfreq) { if (!devfreq) return -EINVAL; @@ -560,7 +577,6 @@ int devfreq_suspend_device(struct devfreq *devfreq) return devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_SUSPEND, NULL); } -EXPORT_SYMBOL(devfreq_suspend_device); /** * devfreq_resume_device() - Resume devfreq of a device. @@ -570,7 +586,7 @@ EXPORT_SYMBOL(devfreq_suspend_device); * (e.g., runtime_resume, resume) of the device driver that * holds the devfreq. */ -int devfreq_resume_device(struct devfreq *devfreq) +static int devfreq_resume_device(struct devfreq *devfreq) { if (!devfreq) return -EINVAL; @@ -581,7 +597,52 @@ int devfreq_resume_device(struct devfreq *devfreq) return devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_RESUME, NULL); } -EXPORT_SYMBOL(devfreq_resume_device); + +/** + * devfreq_runtime_suspend() - Suspend devfreq of a device. + * @dev: the device associated with devfreq + * + * This function is intended to be called by the runtime-pm + * core when the device associated with devfreq is + * runtime suspended. + */ +static void devfreq_runtime_suspend(struct device *dev) +{ + struct devfreq *devfreq; + int err; + + mutex_lock(&devfreq_list_lock); + devfreq = find_device_devfreq(dev); + mutex_unlock(&devfreq_list_lock); + + err = devfreq_suspend_device(devfreq); + if (err) + dev_err(dev, "devfreq runtime suspend failed with (%d) error\n", + err); +} + +/** + * devfreq_runtime_resume() - Resume devfreq of a device. + * @dev: the device associated with devfreq + * + * This function is intended to be called by the runtime-pm + * core when the device associated with devfreq is + * runtime resumed. + */ +static void devfreq_runtime_resume(struct device *dev) +{ + struct devfreq *devfreq; + int err; + + mutex_lock(&devfreq_list_lock); + devfreq = find_device_devfreq(dev); + mutex_unlock(&devfreq_list_lock); + + err = devfreq_resume_device(devfreq); + if (err) + dev_err(dev, "devfreq runtime resume failed with (%d) error\n", + err); +} /** * devfreq_add_governor() - Add devfreq governor diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h index 5f1ab92..669368b 100644 --- a/include/linux/devfreq.h +++ b/include/linux/devfreq.h @@ -140,6 +140,8 @@ struct devfreq_governor { * @trans_table: Statistics of devfreq transitions * @time_in_state: Statistics of devfreq states * @last_stat_updated: The last time stat updated + * @runtime_suspend: func to runtime suspend devfreq from runtime core + * @runtime_resume: func to runtime resume devfreq from runtime core * * This structure stores the devfreq information for a give device. * @@ -173,6 +175,8 @@ struct devfreq { unsigned int *trans_table; unsigned long *time_in_state; unsigned long last_stat_updated; + void (*runtime_suspend)(struct device *dev); + void (*runtime_resume)(struct device *dev); }; #if defined(CONFIG_PM_DEVFREQ) @@ -182,10 +186,6 @@ extern struct devfreq *devfreq_add_device(struct device *dev, void *data); extern int devfreq_remove_device(struct devfreq *devfreq); -/* Supposed to be called by PM_SLEEP/PM_RUNTIME callbacks */ -extern int devfreq_suspend_device(struct devfreq *devfreq); -extern int devfreq_resume_device(struct devfreq *devfreq); - /* Helper functions for devfreq user device driver with OPP. */ extern struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq, u32 flags); @@ -228,16 +228,6 @@ static inline int devfreq_remove_device(struct devfreq *devfreq) return 0; } -static inline int devfreq_suspend_device(struct devfreq *devfreq) -{ - return 0; -} - -static inline int devfreq_resume_device(struct devfreq *devfreq) -{ - return 0; -} - static inline struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq, u32 flags) { diff --git a/include/linux/pm.h b/include/linux/pm.h index 03d7bb1..734449c 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -40,6 +40,7 @@ extern void (*pm_power_off_prepare)(void); */ struct device; +struct devfreq; #ifdef CONFIG_PM extern const char power_group_name[]; /* = "power" */ @@ -549,6 +550,7 @@ struct dev_pm_info { #endif struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */ struct dev_pm_qos *qos; + struct devfreq *devfreq; /* device devfreq */ }; extern void update_pm_runtime_accounting(struct device *dev); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] PM / devfreq: tie suspend/resume to runtime-pm 2013-03-27 13:52 ` [PATCH v2 2/2] PM / devfreq: tie suspend/resume to runtime-pm Rajagopal Venkat @ 2013-04-03 18:16 ` Kevin Hilman 2013-04-04 10:41 ` Rajagopal Venkat 2013-04-04 10:48 ` Rajagopal Venkat 0 siblings, 2 replies; 8+ messages in thread From: Kevin Hilman @ 2013-04-03 18:16 UTC (permalink / raw) To: Rajagopal Venkat Cc: myungjoo.ham, rjw, Alan Stern, patches, linaro-kernel, linux-pm, linux-kernel Rajagopal Venkat <rajagopal.venkat@linaro.org> writes: > Devfreq core suspend/resume of a device is explicitly handled > by devfreq driver through devfreq_suspend_device() and > devfreq_resume_device() apis typically called from runtime > suspend/resume callbacks. This patch aims to take away this > from devfreq drivers and handle it from runtime-pm core. So > that device devfreq core suspend/resume is automatically done > with runtime pm suspend/resume. The devfreq drivers shouldn't > be concerned on when to suspend/resume the devfreq. The terminology is a bit confusing here since you're mixing runtime suspend/resume and just suspend/resume. AFAICT, this patch is only handling devfreq in the runtime PM core, correct? Also, how does this work when multiple devices may be using the same devfreq instance? Using this patch, it looks like the devfreq governor will be shutdown as soon as the first device runtime suspends, right? > This patch is targeted to handle devfreq core load monitoring > suspend/resume only. Not the actual hardware itself. All the > resources like clocks and regulators must still be handled by > devfreq driver using runtime-pm. The sequence of devfreq and ^^^^^^^ devfreq driver? Did you mean device driver? Kevin > device runtime suspend/resume is, > > pm_runtime_suspend(dev) will first suspend device devfreq > (if available) before device is suspended to ensure devfreq load > monitoring is stopped and no device resources like clocks are > accessed while device suspend is in progress. > > pm_runtime_resume(dev) will resume device devfreq(if available) > after device is resumed to ensure device resources like clocks > are ready for use. > > As devfreq suspend/resume is done automatically from runtime-pm, > this patch removes the existing devfreq_suspend_device() and > devfreq_resume_device() apis. > > Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org> > --- > drivers/base/power/runtime.c | 21 ++++++++++++- > drivers/devfreq/devfreq.c | 69 +++++++++++++++++++++++++++++++++++++++--- > include/linux/devfreq.h | 18 +++-------- > include/linux/pm.h | 2 ++ > 4 files changed, 91 insertions(+), 19 deletions(-) > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index 3148b10..2438abc 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -11,6 +11,7 @@ > #include <linux/export.h> > #include <linux/pm_runtime.h> > #include <trace/events/rpm.h> > +#include <linux/devfreq.h> > #include "power.h" > > static int rpm_resume(struct device *dev, int rpmflags); > @@ -406,6 +407,14 @@ static int rpm_suspend(struct device *dev, int rpmflags) > > __update_runtime_status(dev, RPM_SUSPENDING); > > + /* > + * Try suspending devfreq before device is runtime suspended. > + * It wouldn't make sense to continue devfreq load monitoring > + * when device is runtime suspended. > + */ > + if (dev->power.devfreq && dev->power.devfreq->runtime_suspend) > + dev->power.devfreq->runtime_suspend(dev); > + > if (dev->pm_domain) > callback = dev->pm_domain->ops.runtime_suspend; > else if (dev->type && dev->type->pm) > @@ -421,8 +430,12 @@ static int rpm_suspend(struct device *dev, int rpmflags) > callback = dev->driver->pm->runtime_suspend; > > retval = rpm_callback(callback, dev); > - if (retval) > + if (retval) { > + /* Resume back devfreq on device suspend failure */ > + if (dev->power.devfreq && dev->power.devfreq->runtime_resume) > + dev->power.devfreq->runtime_resume(dev); > goto fail; > + } > > no_callback: > __update_runtime_status(dev, RPM_SUSPENDED); > @@ -661,6 +674,12 @@ static int rpm_resume(struct device *dev, int rpmflags) > __update_runtime_status(dev, RPM_ACTIVE); > if (parent) > atomic_inc(&parent->power.child_count); > + /* > + * Resume devfreq after runtime resume sequence to ensure > + * device resources are available for devfreq load monitoring. > + */ > + if (dev->power.devfreq && dev->power.devfreq->runtime_resume) > + dev->power.devfreq->runtime_resume(dev); > } > wake_up_all(&dev->power.wait_queue); > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 44c4079..b8dab0d 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -25,6 +25,7 @@ > #include <linux/list.h> > #include <linux/printk.h> > #include <linux/hrtimer.h> > +#include <linux/pm_runtime.h> > #include "governor.h" > > static struct class *devfreq_class; > @@ -42,6 +43,9 @@ static LIST_HEAD(devfreq_governor_list); > static LIST_HEAD(devfreq_list); > static DEFINE_MUTEX(devfreq_list_lock); > > +static void devfreq_runtime_suspend(struct device *dev); > +static void devfreq_runtime_resume(struct device *dev); > + > /** > * find_device_devfreq() - find devfreq struct using device pointer > * @dev: device pointer used to lookup device devfreq. > @@ -387,6 +391,8 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, > */ > static void _remove_devfreq(struct devfreq *devfreq, bool skip) > { > + unsigned long flags; > + > mutex_lock(&devfreq_list_lock); > if (IS_ERR(find_device_devfreq(devfreq->dev.parent))) { > mutex_unlock(&devfreq_list_lock); > @@ -400,6 +406,10 @@ static void _remove_devfreq(struct devfreq *devfreq, bool skip) > devfreq->governor->event_handler(devfreq, > DEVFREQ_GOV_STOP, NULL); > > + spin_lock_irqsave(&devfreq->dev.parent->power.lock, flags); > + devfreq->dev.parent->power.devfreq = NULL; > + spin_unlock_irqrestore(&devfreq->dev.parent->power.lock, flags); > + > if (devfreq->profile->exit) > devfreq->profile->exit(devfreq->dev.parent); > > @@ -442,6 +452,7 @@ struct devfreq *devfreq_add_device(struct device *dev, > { > struct devfreq *devfreq; > struct devfreq_governor *governor; > + unsigned long flags; > int err = 0; > > if (!dev || !profile || !governor_name) { > @@ -476,6 +487,12 @@ struct devfreq *devfreq_add_device(struct device *dev, > devfreq->previous_freq = profile->initial_freq; > devfreq->data = data; > devfreq->nb.notifier_call = devfreq_notifier_call; > + devfreq->runtime_suspend = devfreq_runtime_suspend; > + devfreq->runtime_resume = devfreq_runtime_resume; > + spin_lock_irqsave(&dev->power.lock, flags); > + dev->power.devfreq = devfreq; > + spin_unlock_irqrestore(&dev->power.lock, flags); > + > > devfreq->trans_table = devm_kzalloc(dev, sizeof(unsigned int) * > devfreq->profile->max_state * > @@ -549,7 +566,7 @@ EXPORT_SYMBOL(devfreq_remove_device); > * (e.g., runtime_suspend, suspend) of the device driver that > * holds the devfreq. > */ > -int devfreq_suspend_device(struct devfreq *devfreq) > +static int devfreq_suspend_device(struct devfreq *devfreq) > { > if (!devfreq) > return -EINVAL; > @@ -560,7 +577,6 @@ int devfreq_suspend_device(struct devfreq *devfreq) > return devfreq->governor->event_handler(devfreq, > DEVFREQ_GOV_SUSPEND, NULL); > } > -EXPORT_SYMBOL(devfreq_suspend_device); > > /** > * devfreq_resume_device() - Resume devfreq of a device. > @@ -570,7 +586,7 @@ EXPORT_SYMBOL(devfreq_suspend_device); > * (e.g., runtime_resume, resume) of the device driver that > * holds the devfreq. > */ > -int devfreq_resume_device(struct devfreq *devfreq) > +static int devfreq_resume_device(struct devfreq *devfreq) > { > if (!devfreq) > return -EINVAL; > @@ -581,7 +597,52 @@ int devfreq_resume_device(struct devfreq *devfreq) > return devfreq->governor->event_handler(devfreq, > DEVFREQ_GOV_RESUME, NULL); > } > -EXPORT_SYMBOL(devfreq_resume_device); > + > +/** > + * devfreq_runtime_suspend() - Suspend devfreq of a device. > + * @dev: the device associated with devfreq > + * > + * This function is intended to be called by the runtime-pm > + * core when the device associated with devfreq is > + * runtime suspended. > + */ > +static void devfreq_runtime_suspend(struct device *dev) > +{ > + struct devfreq *devfreq; > + int err; > + > + mutex_lock(&devfreq_list_lock); > + devfreq = find_device_devfreq(dev); > + mutex_unlock(&devfreq_list_lock); > + > + err = devfreq_suspend_device(devfreq); > + if (err) > + dev_err(dev, "devfreq runtime suspend failed with (%d) error\n", > + err); > +} > + > +/** > + * devfreq_runtime_resume() - Resume devfreq of a device. > + * @dev: the device associated with devfreq > + * > + * This function is intended to be called by the runtime-pm > + * core when the device associated with devfreq is > + * runtime resumed. > + */ > +static void devfreq_runtime_resume(struct device *dev) > +{ > + struct devfreq *devfreq; > + int err; > + > + mutex_lock(&devfreq_list_lock); > + devfreq = find_device_devfreq(dev); > + mutex_unlock(&devfreq_list_lock); > + > + err = devfreq_resume_device(devfreq); > + if (err) > + dev_err(dev, "devfreq runtime resume failed with (%d) error\n", > + err); > +} > > /** > * devfreq_add_governor() - Add devfreq governor > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > index 5f1ab92..669368b 100644 > --- a/include/linux/devfreq.h > +++ b/include/linux/devfreq.h > @@ -140,6 +140,8 @@ struct devfreq_governor { > * @trans_table: Statistics of devfreq transitions > * @time_in_state: Statistics of devfreq states > * @last_stat_updated: The last time stat updated > + * @runtime_suspend: func to runtime suspend devfreq from runtime core > + * @runtime_resume: func to runtime resume devfreq from runtime core > * > * This structure stores the devfreq information for a give device. > * > @@ -173,6 +175,8 @@ struct devfreq { > unsigned int *trans_table; > unsigned long *time_in_state; > unsigned long last_stat_updated; > + void (*runtime_suspend)(struct device *dev); > + void (*runtime_resume)(struct device *dev); > }; > > #if defined(CONFIG_PM_DEVFREQ) > @@ -182,10 +186,6 @@ extern struct devfreq *devfreq_add_device(struct device *dev, > void *data); > extern int devfreq_remove_device(struct devfreq *devfreq); > > -/* Supposed to be called by PM_SLEEP/PM_RUNTIME callbacks */ > -extern int devfreq_suspend_device(struct devfreq *devfreq); > -extern int devfreq_resume_device(struct devfreq *devfreq); > - > /* Helper functions for devfreq user device driver with OPP. */ > extern struct opp *devfreq_recommended_opp(struct device *dev, > unsigned long *freq, u32 flags); > @@ -228,16 +228,6 @@ static inline int devfreq_remove_device(struct devfreq *devfreq) > return 0; > } > > -static inline int devfreq_suspend_device(struct devfreq *devfreq) > -{ > - return 0; > -} > - > -static inline int devfreq_resume_device(struct devfreq *devfreq) > -{ > - return 0; > -} > - > static inline struct opp *devfreq_recommended_opp(struct device *dev, > unsigned long *freq, u32 flags) > { > diff --git a/include/linux/pm.h b/include/linux/pm.h > index 03d7bb1..734449c 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -40,6 +40,7 @@ extern void (*pm_power_off_prepare)(void); > */ > > struct device; > +struct devfreq; > > #ifdef CONFIG_PM > extern const char power_group_name[]; /* = "power" */ > @@ -549,6 +550,7 @@ struct dev_pm_info { > #endif > struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */ > struct dev_pm_qos *qos; > + struct devfreq *devfreq; /* device devfreq */ > }; > > extern void update_pm_runtime_accounting(struct device *dev); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] PM / devfreq: tie suspend/resume to runtime-pm 2013-04-03 18:16 ` Kevin Hilman @ 2013-04-04 10:41 ` Rajagopal Venkat 2013-04-04 10:48 ` Rajagopal Venkat 1 sibling, 0 replies; 8+ messages in thread From: Rajagopal Venkat @ 2013-04-04 10:41 UTC (permalink / raw) To: Kevin Hilman Cc: myungjoo.ham, rjw, Alan Stern, patches, linaro-kernel, linux-pm, linux-kernel On 3 April 2013 23:46, Kevin Hilman <khilman@linaro.org> wrote: > Rajagopal Venkat <rajagopal.venkat@linaro.org> writes: > >> Devfreq core suspend/resume of a device is explicitly handled >> by devfreq driver through devfreq_suspend_device() and >> devfreq_resume_device() apis typically called from runtime >> suspend/resume callbacks. This patch aims to take away this >> from devfreq drivers and handle it from runtime-pm core. So >> that device devfreq core suspend/resume is automatically done >> with runtime pm suspend/resume. The devfreq drivers shouldn't >> be concerned on when to suspend/resume the devfreq. > > The terminology is a bit confusing here since you're mixing runtime > suspend/resume and just suspend/resume. AFAICT, this patch is only > handling devfreq in the runtime PM core, correct? Yes correct. The suspend/resume refers to devfreq core suspend/resume of a device. Nothing to do with system wide suspend/resume. May be I am not clear enough. I will update it. > > Also, how does this work when multiple devices may be using the same > devfreq instance? Using this patch, it looks like the devfreq governor > will be shutdown as soon as the first device runtime suspends, right? As per the devfre > >> This patch is targeted to handle devfreq core load monitoring >> suspend/resume only. Not the actual hardware itself. All the >> resources like clocks and regulators must still be handled by >> devfreq driver using runtime-pm. The sequence of devfreq and > ^^^^^^^ > devfreq driver? Did you mean device driver? Yes device driver. > > Kevin > >> device runtime suspend/resume is, >> >> pm_runtime_suspend(dev) will first suspend device devfreq >> (if available) before device is suspended to ensure devfreq load >> monitoring is stopped and no device resources like clocks are >> accessed while device suspend is in progress. >> >> pm_runtime_resume(dev) will resume device devfreq(if available) >> after device is resumed to ensure device resources like clocks >> are ready for use. >> >> As devfreq suspend/resume is done automatically from runtime-pm, >> this patch removes the existing devfreq_suspend_device() and >> devfreq_resume_device() apis. >> >> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org> >> --- >> drivers/base/power/runtime.c | 21 ++++++++++++- >> drivers/devfreq/devfreq.c | 69 +++++++++++++++++++++++++++++++++++++++--- >> include/linux/devfreq.h | 18 +++-------- >> include/linux/pm.h | 2 ++ >> 4 files changed, 91 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c >> index 3148b10..2438abc 100644 >> --- a/drivers/base/power/runtime.c >> +++ b/drivers/base/power/runtime.c >> @@ -11,6 +11,7 @@ >> #include <linux/export.h> >> #include <linux/pm_runtime.h> >> #include <trace/events/rpm.h> >> +#include <linux/devfreq.h> >> #include "power.h" >> >> static int rpm_resume(struct device *dev, int rpmflags); >> @@ -406,6 +407,14 @@ static int rpm_suspend(struct device *dev, int rpmflags) >> >> __update_runtime_status(dev, RPM_SUSPENDING); >> >> + /* >> + * Try suspending devfreq before device is runtime suspended. >> + * It wouldn't make sense to continue devfreq load monitoring >> + * when device is runtime suspended. >> + */ >> + if (dev->power.devfreq && dev->power.devfreq->runtime_suspend) >> + dev->power.devfreq->runtime_suspend(dev); >> + >> if (dev->pm_domain) >> callback = dev->pm_domain->ops.runtime_suspend; >> else if (dev->type && dev->type->pm) >> @@ -421,8 +430,12 @@ static int rpm_suspend(struct device *dev, int rpmflags) >> callback = dev->driver->pm->runtime_suspend; >> >> retval = rpm_callback(callback, dev); >> - if (retval) >> + if (retval) { >> + /* Resume back devfreq on device suspend failure */ >> + if (dev->power.devfreq && dev->power.devfreq->runtime_resume) >> + dev->power.devfreq->runtime_resume(dev); >> goto fail; >> + } >> >> no_callback: >> __update_runtime_status(dev, RPM_SUSPENDED); >> @@ -661,6 +674,12 @@ static int rpm_resume(struct device *dev, int rpmflags) >> __update_runtime_status(dev, RPM_ACTIVE); >> if (parent) >> atomic_inc(&parent->power.child_count); >> + /* >> + * Resume devfreq after runtime resume sequence to ensure >> + * device resources are available for devfreq load monitoring. >> + */ >> + if (dev->power.devfreq && dev->power.devfreq->runtime_resume) >> + dev->power.devfreq->runtime_resume(dev); >> } >> wake_up_all(&dev->power.wait_queue); >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 44c4079..b8dab0d 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -25,6 +25,7 @@ >> #include <linux/list.h> >> #include <linux/printk.h> >> #include <linux/hrtimer.h> >> +#include <linux/pm_runtime.h> >> #include "governor.h" >> >> static struct class *devfreq_class; >> @@ -42,6 +43,9 @@ static LIST_HEAD(devfreq_governor_list); >> static LIST_HEAD(devfreq_list); >> static DEFINE_MUTEX(devfreq_list_lock); >> >> +static void devfreq_runtime_suspend(struct device *dev); >> +static void devfreq_runtime_resume(struct device *dev); >> + >> /** >> * find_device_devfreq() - find devfreq struct using device pointer >> * @dev: device pointer used to lookup device devfreq. >> @@ -387,6 +391,8 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, >> */ >> static void _remove_devfreq(struct devfreq *devfreq, bool skip) >> { >> + unsigned long flags; >> + >> mutex_lock(&devfreq_list_lock); >> if (IS_ERR(find_device_devfreq(devfreq->dev.parent))) { >> mutex_unlock(&devfreq_list_lock); >> @@ -400,6 +406,10 @@ static void _remove_devfreq(struct devfreq *devfreq, bool skip) >> devfreq->governor->event_handler(devfreq, >> DEVFREQ_GOV_STOP, NULL); >> >> + spin_lock_irqsave(&devfreq->dev.parent->power.lock, flags); >> + devfreq->dev.parent->power.devfreq = NULL; >> + spin_unlock_irqrestore(&devfreq->dev.parent->power.lock, flags); >> + >> if (devfreq->profile->exit) >> devfreq->profile->exit(devfreq->dev.parent); >> >> @@ -442,6 +452,7 @@ struct devfreq *devfreq_add_device(struct device *dev, >> { >> struct devfreq *devfreq; >> struct devfreq_governor *governor; >> + unsigned long flags; >> int err = 0; >> >> if (!dev || !profile || !governor_name) { >> @@ -476,6 +487,12 @@ struct devfreq *devfreq_add_device(struct device *dev, >> devfreq->previous_freq = profile->initial_freq; >> devfreq->data = data; >> devfreq->nb.notifier_call = devfreq_notifier_call; >> + devfreq->runtime_suspend = devfreq_runtime_suspend; >> + devfreq->runtime_resume = devfreq_runtime_resume; >> + spin_lock_irqsave(&dev->power.lock, flags); >> + dev->power.devfreq = devfreq; >> + spin_unlock_irqrestore(&dev->power.lock, flags); >> + >> >> devfreq->trans_table = devm_kzalloc(dev, sizeof(unsigned int) * >> devfreq->profile->max_state * >> @@ -549,7 +566,7 @@ EXPORT_SYMBOL(devfreq_remove_device); >> * (e.g., runtime_suspend, suspend) of the device driver that >> * holds the devfreq. >> */ >> -int devfreq_suspend_device(struct devfreq *devfreq) >> +static int devfreq_suspend_device(struct devfreq *devfreq) >> { >> if (!devfreq) >> return -EINVAL; >> @@ -560,7 +577,6 @@ int devfreq_suspend_device(struct devfreq *devfreq) >> return devfreq->governor->event_handler(devfreq, >> DEVFREQ_GOV_SUSPEND, NULL); >> } >> -EXPORT_SYMBOL(devfreq_suspend_device); >> >> /** >> * devfreq_resume_device() - Resume devfreq of a device. >> @@ -570,7 +586,7 @@ EXPORT_SYMBOL(devfreq_suspend_device); >> * (e.g., runtime_resume, resume) of the device driver that >> * holds the devfreq. >> */ >> -int devfreq_resume_device(struct devfreq *devfreq) >> +static int devfreq_resume_device(struct devfreq *devfreq) >> { >> if (!devfreq) >> return -EINVAL; >> @@ -581,7 +597,52 @@ int devfreq_resume_device(struct devfreq *devfreq) >> return devfreq->governor->event_handler(devfreq, >> DEVFREQ_GOV_RESUME, NULL); >> } >> -EXPORT_SYMBOL(devfreq_resume_device); >> + >> +/** >> + * devfreq_runtime_suspend() - Suspend devfreq of a device. >> + * @dev: the device associated with devfreq >> + * >> + * This function is intended to be called by the runtime-pm >> + * core when the device associated with devfreq is >> + * runtime suspended. >> + */ >> +static void devfreq_runtime_suspend(struct device *dev) >> +{ >> + struct devfreq *devfreq; >> + int err; >> + >> + mutex_lock(&devfreq_list_lock); >> + devfreq = find_device_devfreq(dev); >> + mutex_unlock(&devfreq_list_lock); >> + >> + err = devfreq_suspend_device(devfreq); >> + if (err) >> + dev_err(dev, "devfreq runtime suspend failed with (%d) error\n", >> + err); >> +} >> + >> +/** >> + * devfreq_runtime_resume() - Resume devfreq of a device. >> + * @dev: the device associated with devfreq >> + * >> + * This function is intended to be called by the runtime-pm >> + * core when the device associated with devfreq is >> + * runtime resumed. >> + */ >> +static void devfreq_runtime_resume(struct device *dev) >> +{ >> + struct devfreq *devfreq; >> + int err; >> + >> + mutex_lock(&devfreq_list_lock); >> + devfreq = find_device_devfreq(dev); >> + mutex_unlock(&devfreq_list_lock); >> + >> + err = devfreq_resume_device(devfreq); >> + if (err) >> + dev_err(dev, "devfreq runtime resume failed with (%d) error\n", >> + err); >> +} >> >> /** >> * devfreq_add_governor() - Add devfreq governor >> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >> index 5f1ab92..669368b 100644 >> --- a/include/linux/devfreq.h >> +++ b/include/linux/devfreq.h >> @@ -140,6 +140,8 @@ struct devfreq_governor { >> * @trans_table: Statistics of devfreq transitions >> * @time_in_state: Statistics of devfreq states >> * @last_stat_updated: The last time stat updated >> + * @runtime_suspend: func to runtime suspend devfreq from runtime core >> + * @runtime_resume: func to runtime resume devfreq from runtime core >> * >> * This structure stores the devfreq information for a give device. >> * >> @@ -173,6 +175,8 @@ struct devfreq { >> unsigned int *trans_table; >> unsigned long *time_in_state; >> unsigned long last_stat_updated; >> + void (*runtime_suspend)(struct device *dev); >> + void (*runtime_resume)(struct device *dev); >> }; >> >> #if defined(CONFIG_PM_DEVFREQ) >> @@ -182,10 +186,6 @@ extern struct devfreq *devfreq_add_device(struct device *dev, >> void *data); >> extern int devfreq_remove_device(struct devfreq *devfreq); >> >> -/* Supposed to be called by PM_SLEEP/PM_RUNTIME callbacks */ >> -extern int devfreq_suspend_device(struct devfreq *devfreq); >> -extern int devfreq_resume_device(struct devfreq *devfreq); >> - >> /* Helper functions for devfreq user device driver with OPP. */ >> extern struct opp *devfreq_recommended_opp(struct device *dev, >> unsigned long *freq, u32 flags); >> @@ -228,16 +228,6 @@ static inline int devfreq_remove_device(struct devfreq *devfreq) >> return 0; >> } >> >> -static inline int devfreq_suspend_device(struct devfreq *devfreq) >> -{ >> - return 0; >> -} >> - >> -static inline int devfreq_resume_device(struct devfreq *devfreq) >> -{ >> - return 0; >> -} >> - >> static inline struct opp *devfreq_recommended_opp(struct device *dev, >> unsigned long *freq, u32 flags) >> { >> diff --git a/include/linux/pm.h b/include/linux/pm.h >> index 03d7bb1..734449c 100644 >> --- a/include/linux/pm.h >> +++ b/include/linux/pm.h >> @@ -40,6 +40,7 @@ extern void (*pm_power_off_prepare)(void); >> */ >> >> struct device; >> +struct devfreq; >> >> #ifdef CONFIG_PM >> extern const char power_group_name[]; /* = "power" */ >> @@ -549,6 +550,7 @@ struct dev_pm_info { >> #endif >> struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */ >> struct dev_pm_qos *qos; >> + struct devfreq *devfreq; /* device devfreq */ >> }; >> >> extern void update_pm_runtime_accounting(struct device *dev); -- Regards, Rajagopal ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] PM / devfreq: tie suspend/resume to runtime-pm 2013-04-03 18:16 ` Kevin Hilman 2013-04-04 10:41 ` Rajagopal Venkat @ 2013-04-04 10:48 ` Rajagopal Venkat 1 sibling, 0 replies; 8+ messages in thread From: Rajagopal Venkat @ 2013-04-04 10:48 UTC (permalink / raw) To: Kevin Hilman Cc: myungjoo.ham, rjw, Alan Stern, patches, linaro-kernel, linux-pm, linux-kernel On 3 April 2013 23:46, Kevin Hilman <khilman@linaro.org> wrote: > Rajagopal Venkat <rajagopal.venkat@linaro.org> writes: > >> Devfreq core suspend/resume of a device is explicitly handled >> by devfreq driver through devfreq_suspend_device() and >> devfreq_resume_device() apis typically called from runtime >> suspend/resume callbacks. This patch aims to take away this >> from devfreq drivers and handle it from runtime-pm core. So >> that device devfreq core suspend/resume is automatically done >> with runtime pm suspend/resume. The devfreq drivers shouldn't >> be concerned on when to suspend/resume the devfreq. > > The terminology is a bit confusing here since you're mixing runtime > suspend/resume and just suspend/resume. AFAICT, this patch is only > handling devfreq in the runtime PM core, correct? Yes correct. The suspend/resume refers to devfreq core suspend/resume of a device. Nothing to do with system wide suspend/resume. May be I am not clear enough. I will update it. > > Also, how does this work when multiple devices may be using the same As per the devfreq design, this is not allowed. Only one devfreq instance can be associated with a device. > devfreq instance? Using this patch, it looks like the devfreq governor > will be shutdown as soon as the first device runtime suspends, right? > >> This patch is targeted to handle devfreq core load monitoring >> suspend/resume only. Not the actual hardware itself. All the >> resources like clocks and regulators must still be handled by >> devfreq driver using runtime-pm. The sequence of devfreq and > ^^^^^^^ > devfreq driver? Did you mean device driver? Yes. device driver. > > Kevin > >> device runtime suspend/resume is, >> >> pm_runtime_suspend(dev) will first suspend device devfreq >> (if available) before device is suspended to ensure devfreq load >> monitoring is stopped and no device resources like clocks are >> accessed while device suspend is in progress. >> >> pm_runtime_resume(dev) will resume device devfreq(if available) >> after device is resumed to ensure device resources like clocks >> are ready for use. >> >> As devfreq suspend/resume is done automatically from runtime-pm, >> this patch removes the existing devfreq_suspend_device() and >> devfreq_resume_device() apis. >> >> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org> >> --- >> drivers/base/power/runtime.c | 21 ++++++++++++- >> drivers/devfreq/devfreq.c | 69 +++++++++++++++++++++++++++++++++++++++--- >> include/linux/devfreq.h | 18 +++-------- >> include/linux/pm.h | 2 ++ >> 4 files changed, 91 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c >> index 3148b10..2438abc 100644 >> --- a/drivers/base/power/runtime.c >> +++ b/drivers/base/power/runtime.c >> @@ -11,6 +11,7 @@ >> #include <linux/export.h> >> #include <linux/pm_runtime.h> >> #include <trace/events/rpm.h> >> +#include <linux/devfreq.h> >> #include "power.h" >> >> static int rpm_resume(struct device *dev, int rpmflags); >> @@ -406,6 +407,14 @@ static int rpm_suspend(struct device *dev, int rpmflags) >> >> __update_runtime_status(dev, RPM_SUSPENDING); >> >> + /* >> + * Try suspending devfreq before device is runtime suspended. >> + * It wouldn't make sense to continue devfreq load monitoring >> + * when device is runtime suspended. >> + */ >> + if (dev->power.devfreq && dev->power.devfreq->runtime_suspend) >> + dev->power.devfreq->runtime_suspend(dev); >> + >> if (dev->pm_domain) >> callback = dev->pm_domain->ops.runtime_suspend; >> else if (dev->type && dev->type->pm) >> @@ -421,8 +430,12 @@ static int rpm_suspend(struct device *dev, int rpmflags) >> callback = dev->driver->pm->runtime_suspend; >> >> retval = rpm_callback(callback, dev); >> - if (retval) >> + if (retval) { >> + /* Resume back devfreq on device suspend failure */ >> + if (dev->power.devfreq && dev->power.devfreq->runtime_resume) >> + dev->power.devfreq->runtime_resume(dev); >> goto fail; >> + } >> >> no_callback: >> __update_runtime_status(dev, RPM_SUSPENDED); >> @@ -661,6 +674,12 @@ static int rpm_resume(struct device *dev, int rpmflags) >> __update_runtime_status(dev, RPM_ACTIVE); >> if (parent) >> atomic_inc(&parent->power.child_count); >> + /* >> + * Resume devfreq after runtime resume sequence to ensure >> + * device resources are available for devfreq load monitoring. >> + */ >> + if (dev->power.devfreq && dev->power.devfreq->runtime_resume) >> + dev->power.devfreq->runtime_resume(dev); >> } >> wake_up_all(&dev->power.wait_queue); >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 44c4079..b8dab0d 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -25,6 +25,7 @@ >> #include <linux/list.h> >> #include <linux/printk.h> >> #include <linux/hrtimer.h> >> +#include <linux/pm_runtime.h> >> #include "governor.h" >> >> static struct class *devfreq_class; >> @@ -42,6 +43,9 @@ static LIST_HEAD(devfreq_governor_list); >> static LIST_HEAD(devfreq_list); >> static DEFINE_MUTEX(devfreq_list_lock); >> >> +static void devfreq_runtime_suspend(struct device *dev); >> +static void devfreq_runtime_resume(struct device *dev); >> + >> /** >> * find_device_devfreq() - find devfreq struct using device pointer >> * @dev: device pointer used to lookup device devfreq. >> @@ -387,6 +391,8 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, >> */ >> static void _remove_devfreq(struct devfreq *devfreq, bool skip) >> { >> + unsigned long flags; >> + >> mutex_lock(&devfreq_list_lock); >> if (IS_ERR(find_device_devfreq(devfreq->dev.parent))) { >> mutex_unlock(&devfreq_list_lock); >> @@ -400,6 +406,10 @@ static void _remove_devfreq(struct devfreq *devfreq, bool skip) >> devfreq->governor->event_handler(devfreq, >> DEVFREQ_GOV_STOP, NULL); >> >> + spin_lock_irqsave(&devfreq->dev.parent->power.lock, flags); >> + devfreq->dev.parent->power.devfreq = NULL; >> + spin_unlock_irqrestore(&devfreq->dev.parent->power.lock, flags); >> + >> if (devfreq->profile->exit) >> devfreq->profile->exit(devfreq->dev.parent); >> >> @@ -442,6 +452,7 @@ struct devfreq *devfreq_add_device(struct device *dev, >> { >> struct devfreq *devfreq; >> struct devfreq_governor *governor; >> + unsigned long flags; >> int err = 0; >> >> if (!dev || !profile || !governor_name) { >> @@ -476,6 +487,12 @@ struct devfreq *devfreq_add_device(struct device *dev, >> devfreq->previous_freq = profile->initial_freq; >> devfreq->data = data; >> devfreq->nb.notifier_call = devfreq_notifier_call; >> + devfreq->runtime_suspend = devfreq_runtime_suspend; >> + devfreq->runtime_resume = devfreq_runtime_resume; >> + spin_lock_irqsave(&dev->power.lock, flags); >> + dev->power.devfreq = devfreq; >> + spin_unlock_irqrestore(&dev->power.lock, flags); >> + >> >> devfreq->trans_table = devm_kzalloc(dev, sizeof(unsigned int) * >> devfreq->profile->max_state * >> @@ -549,7 +566,7 @@ EXPORT_SYMBOL(devfreq_remove_device); >> * (e.g., runtime_suspend, suspend) of the device driver that >> * holds the devfreq. >> */ >> -int devfreq_suspend_device(struct devfreq *devfreq) >> +static int devfreq_suspend_device(struct devfreq *devfreq) >> { >> if (!devfreq) >> return -EINVAL; >> @@ -560,7 +577,6 @@ int devfreq_suspend_device(struct devfreq *devfreq) >> return devfreq->governor->event_handler(devfreq, >> DEVFREQ_GOV_SUSPEND, NULL); >> } >> -EXPORT_SYMBOL(devfreq_suspend_device); >> >> /** >> * devfreq_resume_device() - Resume devfreq of a device. >> @@ -570,7 +586,7 @@ EXPORT_SYMBOL(devfreq_suspend_device); >> * (e.g., runtime_resume, resume) of the device driver that >> * holds the devfreq. >> */ >> -int devfreq_resume_device(struct devfreq *devfreq) >> +static int devfreq_resume_device(struct devfreq *devfreq) >> { >> if (!devfreq) >> return -EINVAL; >> @@ -581,7 +597,52 @@ int devfreq_resume_device(struct devfreq *devfreq) >> return devfreq->governor->event_handler(devfreq, >> DEVFREQ_GOV_RESUME, NULL); >> } >> -EXPORT_SYMBOL(devfreq_resume_device); >> + >> +/** >> + * devfreq_runtime_suspend() - Suspend devfreq of a device. >> + * @dev: the device associated with devfreq >> + * >> + * This function is intended to be called by the runtime-pm >> + * core when the device associated with devfreq is >> + * runtime suspended. >> + */ >> +static void devfreq_runtime_suspend(struct device *dev) >> +{ >> + struct devfreq *devfreq; >> + int err; >> + >> + mutex_lock(&devfreq_list_lock); >> + devfreq = find_device_devfreq(dev); >> + mutex_unlock(&devfreq_list_lock); >> + >> + err = devfreq_suspend_device(devfreq); >> + if (err) >> + dev_err(dev, "devfreq runtime suspend failed with (%d) error\n", >> + err); >> +} >> + >> +/** >> + * devfreq_runtime_resume() - Resume devfreq of a device. >> + * @dev: the device associated with devfreq >> + * >> + * This function is intended to be called by the runtime-pm >> + * core when the device associated with devfreq is >> + * runtime resumed. >> + */ >> +static void devfreq_runtime_resume(struct device *dev) >> +{ >> + struct devfreq *devfreq; >> + int err; >> + >> + mutex_lock(&devfreq_list_lock); >> + devfreq = find_device_devfreq(dev); >> + mutex_unlock(&devfreq_list_lock); >> + >> + err = devfreq_resume_device(devfreq); >> + if (err) >> + dev_err(dev, "devfreq runtime resume failed with (%d) error\n", >> + err); >> +} >> >> /** >> * devfreq_add_governor() - Add devfreq governor >> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >> index 5f1ab92..669368b 100644 >> --- a/include/linux/devfreq.h >> +++ b/include/linux/devfreq.h >> @@ -140,6 +140,8 @@ struct devfreq_governor { >> * @trans_table: Statistics of devfreq transitions >> * @time_in_state: Statistics of devfreq states >> * @last_stat_updated: The last time stat updated >> + * @runtime_suspend: func to runtime suspend devfreq from runtime core >> + * @runtime_resume: func to runtime resume devfreq from runtime core >> * >> * This structure stores the devfreq information for a give device. >> * >> @@ -173,6 +175,8 @@ struct devfreq { >> unsigned int *trans_table; >> unsigned long *time_in_state; >> unsigned long last_stat_updated; >> + void (*runtime_suspend)(struct device *dev); >> + void (*runtime_resume)(struct device *dev); >> }; >> >> #if defined(CONFIG_PM_DEVFREQ) >> @@ -182,10 +186,6 @@ extern struct devfreq *devfreq_add_device(struct device *dev, >> void *data); >> extern int devfreq_remove_device(struct devfreq *devfreq); >> >> -/* Supposed to be called by PM_SLEEP/PM_RUNTIME callbacks */ >> -extern int devfreq_suspend_device(struct devfreq *devfreq); >> -extern int devfreq_resume_device(struct devfreq *devfreq); >> - >> /* Helper functions for devfreq user device driver with OPP. */ >> extern struct opp *devfreq_recommended_opp(struct device *dev, >> unsigned long *freq, u32 flags); >> @@ -228,16 +228,6 @@ static inline int devfreq_remove_device(struct devfreq *devfreq) >> return 0; >> } >> >> -static inline int devfreq_suspend_device(struct devfreq *devfreq) >> -{ >> - return 0; >> -} >> - >> -static inline int devfreq_resume_device(struct devfreq *devfreq) >> -{ >> - return 0; >> -} >> - >> static inline struct opp *devfreq_recommended_opp(struct device *dev, >> unsigned long *freq, u32 flags) >> { >> diff --git a/include/linux/pm.h b/include/linux/pm.h >> index 03d7bb1..734449c 100644 >> --- a/include/linux/pm.h >> +++ b/include/linux/pm.h >> @@ -40,6 +40,7 @@ extern void (*pm_power_off_prepare)(void); >> */ >> >> struct device; >> +struct devfreq; >> >> #ifdef CONFIG_PM >> extern const char power_group_name[]; /* = "power" */ >> @@ -549,6 +550,7 @@ struct dev_pm_info { >> #endif >> struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */ >> struct dev_pm_qos *qos; >> + struct devfreq *devfreq; /* device devfreq */ >> }; >> >> extern void update_pm_runtime_accounting(struct device *dev); -- Regards, Rajagopal ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-04-04 10:48 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-27 13:52 [PATCH v2 0/2] Add devfreq runtime pm support Rajagopal Venkat 2013-03-27 13:52 ` [PATCH v2 1/2] PM / devfreq: Fix compiler warnings Rajagopal Venkat 2013-03-28 6:03 ` MyungJoo Ham 2013-04-02 0:16 ` Rafael J. Wysocki 2013-03-27 13:52 ` [PATCH v2 2/2] PM / devfreq: tie suspend/resume to runtime-pm Rajagopal Venkat 2013-04-03 18:16 ` Kevin Hilman 2013-04-04 10:41 ` Rajagopal Venkat 2013-04-04 10:48 ` Rajagopal Venkat
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).