[PATCH 0/2] Add devfreq runtime pm support

[PATCH 0/2] Add devfreq runtime pm support

[Rajagopal Venkat]

Patch to bind devfreq to runtime pm framework. The device
devfreq is  automatically suspended with pm_runtime_suspend()
and resumed with pm_runtime_resume().

Discussed at http://comments.gmane.org/gmane.linux.linaro.devel/13787

Rajagopal Venkat (2):
  PM / devfreq: Fix compiler warnings
  PM / devfreq: tie suspend/resume to runtime-pm

 drivers/base/power/runtime.c |   18 ++++++++++++++-
 drivers/devfreq/devfreq.c    |   51 ++++++++++++++++++++++++++++++++++++++----
 include/linux/devfreq.h      |   30 +++++++++----------------
 include/linux/pm.h           |    2 ++
 4 files changed, 76 insertions(+), 25 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] PM / devfreq: Fix compiler warnings

[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

[PATCH 2/2] PM / devfreq: tie suspend/resume to runtime-pm

[Rajagopal Venkat]

Allow device devfreq to be suspend/resume automatically with
runtime pm suspend/resume. The devfreq drivers should be least
cared when to suspend/resume the devfreq.

pm_runtime_suspend(dev) will first suspend device devfreq(if available)
before device is suspended from runtime pm core.

pm_runtime_resume(dev) will resume device devfreq(if available) after
device is resumed from runtime pm core.

Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org>
---
 drivers/base/power/runtime.c |   18 ++++++++++++++-
 drivers/devfreq/devfreq.c    |   51 ++++++++++++++++++++++++++++++++++++++----
 include/linux/devfreq.h      |   18 ++++-----------
 include/linux/pm.h           |    2 ++
 4 files changed, 70 insertions(+), 19 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 3148b10..951bf92 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,10 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 
 	__update_runtime_status(dev, RPM_SUSPENDING);
 
+	if (dev->power.devfreq &&
+			dev->power.devfreq->runtime_suspend(dev))
+		goto fail;
+
 	if (dev->pm_domain)
 		callback = dev->pm_domain->ops.runtime_suspend;
 	else if (dev->type && dev->type->pm)
@@ -421,8 +426,11 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 		callback = dev->driver->pm->runtime_suspend;
 
 	retval = rpm_callback(callback, dev);
-	if (retval)
+	if (retval) {
+		if (dev->power.devfreq)
+			dev->power.devfreq->runtime_resume(dev);
 		goto fail;
+	}
 
  no_callback:
 	__update_runtime_status(dev, RPM_SUSPENDED);
@@ -638,6 +646,11 @@ static int rpm_resume(struct device *dev, int rpmflags)
 
 	__update_runtime_status(dev, RPM_RESUMING);
 
+	if (dev->power.devfreq)
+		retval = dev->power.devfreq->runtime_resume(dev);
+		if (retval)
+			goto skip_callback;
+
 	if (dev->pm_domain)
 		callback = dev->pm_domain->ops.runtime_resume;
 	else if (dev->type && dev->type->pm)
@@ -653,9 +666,12 @@ static int rpm_resume(struct device *dev, int rpmflags)
 		callback = dev->driver->pm->runtime_resume;
 
 	retval = rpm_callback(callback, dev);
+skip_callback:
 	if (retval) {
 		__update_runtime_status(dev, RPM_SUSPENDED);
 		pm_runtime_cancel_pending(dev);
+		if (dev->power.devfreq)
+			dev->power.devfreq->runtime_suspend(dev);
 	} else {
  no_callback:
 		__update_runtime_status(dev, RPM_ACTIVE);
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 44c4079..ed6cb88 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 int devfreq_runtime_suspend(struct device *dev);
+static int devfreq_runtime_resume(struct device *dev);
+
 /**
  * find_device_devfreq() - find devfreq struct using device pointer
  * @dev:	device pointer used to lookup device devfreq.
@@ -476,6 +480,9 @@ 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;
+	dev->power.devfreq = devfreq;
 
 	devfreq->trans_table =	devm_kzalloc(dev, sizeof(unsigned int) *
 						devfreq->profile->max_state *
@@ -549,7 +556,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 +567,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 +576,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 +587,44 @@ 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 int devfreq_runtime_suspend(struct device *dev)
+{
+	struct devfreq *devfreq;
+
+	mutex_lock(&devfreq_list_lock);
+	devfreq = find_device_devfreq(dev);
+	mutex_unlock(&devfreq_list_lock);
+
+	return devfreq_suspend_device(devfreq);
+}
+
+/**
+ * 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 int devfreq_runtime_resume(struct device *dev)
+{
+	struct devfreq *devfreq;
+
+	mutex_lock(&devfreq_list_lock);
+	devfreq = find_device_devfreq(dev);
+	mutex_unlock(&devfreq_list_lock);
+
+	return devfreq_resume_device(devfreq);
+}
 
 /**
  * devfreq_add_governor() - Add devfreq governor
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 5f1ab92..f50f46e 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;
+	int (*runtime_suspend)(struct device *dev);
+	int (*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..6097db1 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;
 };
 
 extern void update_pm_runtime_accounting(struct device *dev);
-- 
1.7.10.4

Re: [PATCH 2/2] PM / devfreq: tie suspend/resume to runtime-pm

[Alan Stern]

On Thu, 21 Mar 2013, Rajagopal Venkat wrote:

> Allow device devfreq to be suspend/resume automatically with
> runtime pm suspend/resume. The devfreq drivers should be least
> cared when to suspend/resume the devfreq.
> 
> pm_runtime_suspend(dev) will first suspend device devfreq(if available)
> before device is suspended from runtime pm core.
> 
> pm_runtime_resume(dev) will resume device devfreq(if available) after
> device is resumed from runtime pm core.

Aren't these backward?  The device continues to need its clocks and 
frequency settings until _after_ it has been suspended.  Similarly, it 
needs them to be available _before_ it resumes.

Alan Stern

Re: [PATCH 2/2] PM / devfreq: tie suspend/resume to runtime-pm

[Rajagopal Venkat]

On 21 March 2013 20:32, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 21 Mar 2013, Rajagopal Venkat wrote:
>
>> Allow device devfreq to be suspend/resume automatically with
>> runtime pm suspend/resume. The devfreq drivers should be least
>> cared when to suspend/resume the devfreq.
>>
>> pm_runtime_suspend(dev) will first suspend device devfreq(if available)
>> before device is suspended from runtime pm core.
>>
>> pm_runtime_resume(dev) will resume device devfreq(if available) after
>> device is resumed from runtime pm core.
>
> Aren't these backward?  The device continues to need its clocks and
> frequency settings until _after_ it has been suspended.  Similarly, it
> needs them to be available _before_ it resumes.
>

To clarify, devfreq suspend/resume means suspend/resume of load
monitoring of a device. The clocks and other resources are still taken
care by the devfreq driver runtime-pm callbacks. The devfreq must be
suspended _before_ runtime-pm suspend callback in order to stop load
monitoring while device is suspending. Similarly devfreq resume should
happen _after_ runtime-pm resume callback.

Just realised both devfreq suspend/resume are done _before_ invoking
runtime-pm callbacks. I will fix this up. Thanks.


> Alan Stern
>



-- 
Regards,
Rajagopal

Re: [PATCH 2/2] PM / devfreq: tie suspend/resume to runtime-pm

[Kevin Hilman]

Hi Rajagopal,

Rajagopal Venkat <rajagopal.venkat@linaro.org> writes:

> Allow device devfreq to be suspend/resume automatically with
> runtime pm suspend/resume. The devfreq drivers should be least
> cared when to suspend/resume the devfreq.

That is the "what", but the changelog should also answer the question
"why?".

> pm_runtime_suspend(dev) will first suspend device devfreq(if available)
> before device is suspended from runtime pm core.

And here you should describe what you mentioned in your response to Alan
to clarify that suspending devfreq is suspending/resuming the load
monitoring, not any actual device hardware.  You should also describe
the rationale behind the ordering of devfreq susepnd/resume and device
suspend/resume.

> pm_runtime_resume(dev) will resume device devfreq(if available) after
> device is resumed from runtime pm core.
>
> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org>
> ---
>  drivers/base/power/runtime.c |   18 ++++++++++++++-
>  drivers/devfreq/devfreq.c    |   51 ++++++++++++++++++++++++++++++++++++++----
>  include/linux/devfreq.h      |   18 ++++-----------
>  include/linux/pm.h           |    2 ++
>  4 files changed, 70 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 3148b10..951bf92 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,10 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>  
>  	__update_runtime_status(dev, RPM_SUSPENDING);
>  
> +	if (dev->power.devfreq &&
> +			dev->power.devfreq->runtime_suspend(dev))
> +		goto fail;

It's not obvious to me why you want to abort the entire runtime suspend
path if the devfreq governor fails to runtime suspend.  That should also
be described someplace.

Same comment on skipping the callbacks in rpm_resume below.

>  	if (dev->pm_domain)
>  		callback = dev->pm_domain->ops.runtime_suspend;
>  	else if (dev->type && dev->type->pm)
> @@ -421,8 +426,11 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>  		callback = dev->driver->pm->runtime_suspend;
>  
>  	retval = rpm_callback(callback, dev);
> -	if (retval)
> +	if (retval) {
> +		if (dev->power.devfreq)
> +			dev->power.devfreq->runtime_resume(dev);

What if ->runtime_resume is NULL?

>  		goto fail;
> +	}
>  
>   no_callback:
>  	__update_runtime_status(dev, RPM_SUSPENDED);
> @@ -638,6 +646,11 @@ static int rpm_resume(struct device *dev, int rpmflags)
>  
>  	__update_runtime_status(dev, RPM_RESUMING);
>  
> +	if (dev->power.devfreq)
> +		retval = dev->power.devfreq->runtime_resume(dev);

ditto.

I see that the devfreq core always sets these function pointers in this
patch, but that's not an assumption the PM core should make, IMO.

> +		if (retval)
> +			goto skip_callback;
> +
>  	if (dev->pm_domain)
>  		callback = dev->pm_domain->ops.runtime_resume;
>  	else if (dev->type && dev->type->pm)
> @@ -653,9 +666,12 @@ static int rpm_resume(struct device *dev, int rpmflags)
>  		callback = dev->driver->pm->runtime_resume;
>  
>  	retval = rpm_callback(callback, dev);
> +skip_callback:
>  	if (retval) {
>  		__update_runtime_status(dev, RPM_SUSPENDED);
>  		pm_runtime_cancel_pending(dev);
> +		if (dev->power.devfreq)
> +			dev->power.devfreq->runtime_suspend(dev);
>  	} else {
>   no_callback:
>  		__update_runtime_status(dev, RPM_ACTIVE);
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 44c4079..ed6cb88 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 int devfreq_runtime_suspend(struct device *dev);
> +static int devfreq_runtime_resume(struct device *dev);
> +
>  /**
>   * find_device_devfreq() - find devfreq struct using device pointer
>   * @dev:	device pointer used to lookup device devfreq.
> @@ -476,6 +480,9 @@ 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;
> +	dev->power.devfreq = devfreq;
>  
>  	devfreq->trans_table =	devm_kzalloc(dev, sizeof(unsigned int) *
>  						devfreq->profile->max_state *
> @@ -549,7 +556,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 +567,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 +576,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 +587,44 @@ 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 int devfreq_runtime_suspend(struct device *dev)
> +{
> +	struct devfreq *devfreq;
> +
> +	mutex_lock(&devfreq_list_lock);
> +	devfreq = find_device_devfreq(dev);
> +	mutex_unlock(&devfreq_list_lock);
> +
> +	return devfreq_suspend_device(devfreq);
> +}
> +
> +/**
> + * 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 int devfreq_runtime_resume(struct device *dev)
> +{
> +	struct devfreq *devfreq;
> +
> +	mutex_lock(&devfreq_list_lock);
> +	devfreq = find_device_devfreq(dev);
> +	mutex_unlock(&devfreq_list_lock);
> +
> +	return devfreq_resume_device(devfreq);
> +}
>  
>  /**
>   * devfreq_add_governor() - Add devfreq governor
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 5f1ab92..f50f46e 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;
> +	int (*runtime_suspend)(struct device *dev);
> +	int (*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);

Remvoing these, and removing the EXPORT_SYMBOL()s is an API change that
is not described in the changelog.

Kevin

> -
>  /* 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..6097db1 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;
>  };
>  
>  extern void update_pm_runtime_accounting(struct device *dev);

Re: [PATCH 2/2] PM / devfreq: tie suspend/resume to runtime-pm

[Rajagopal Venkat]

On 22 March 2013 23:04, Kevin Hilman <khilman@linaro.org> wrote:
> Hi Rajagopal,
>
> Rajagopal Venkat <rajagopal.venkat@linaro.org> writes:
>
>> Allow device devfreq to be suspend/resume automatically with
>> runtime pm suspend/resume. The devfreq drivers should be least
>> cared when to suspend/resume the devfreq.
>
> That is the "what", but the changelog should also answer the question
> "why?".
>
>> pm_runtime_suspend(dev) will first suspend device devfreq(if available)
>> before device is suspended from runtime pm core.
>
> And here you should describe what you mentioned in your response to Alan
> to clarify that suspending devfreq is suspending/resuming the load
> monitoring, not any actual device hardware.  You should also describe
> the rationale behind the ordering of devfreq susepnd/resume and device
> suspend/resume.
>

Kevin, Thanks for the review, I am taking in all your comments. I will
improve the changelog and code comments and send new version.

>> pm_runtime_resume(dev) will resume device devfreq(if available) after
>> device is resumed from runtime pm core.
>>
>> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org>
>> ---
>>  drivers/base/power/runtime.c |   18 ++++++++++++++-
>>  drivers/devfreq/devfreq.c    |   51 ++++++++++++++++++++++++++++++++++++++----
>>  include/linux/devfreq.h      |   18 ++++-----------
>>  include/linux/pm.h           |    2 ++
>>  4 files changed, 70 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> index 3148b10..951bf92 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,10 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>>
>>       __update_runtime_status(dev, RPM_SUSPENDING);
>>
>> +     if (dev->power.devfreq &&
>> +                     dev->power.devfreq->runtime_suspend(dev))
>> +             goto fail;
>
> It's not obvious to me why you want to abort the entire runtime suspend
> path if the devfreq governor fails to runtime suspend.  That should also
> be described someplace.
>
> Same comment on skipping the callbacks in rpm_resume below.
>
>>       if (dev->pm_domain)
>>               callback = dev->pm_domain->ops.runtime_suspend;
>>       else if (dev->type && dev->type->pm)
>> @@ -421,8 +426,11 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>>               callback = dev->driver->pm->runtime_suspend;
>>
>>       retval = rpm_callback(callback, dev);
>> -     if (retval)
>> +     if (retval) {
>> +             if (dev->power.devfreq)
>> +                     dev->power.devfreq->runtime_resume(dev);
>
> What if ->runtime_resume is NULL?
>
>>               goto fail;
>> +     }
>>
>>   no_callback:
>>       __update_runtime_status(dev, RPM_SUSPENDED);
>> @@ -638,6 +646,11 @@ static int rpm_resume(struct device *dev, int rpmflags)
>>
>>       __update_runtime_status(dev, RPM_RESUMING);
>>
>> +     if (dev->power.devfreq)
>> +             retval = dev->power.devfreq->runtime_resume(dev);
>
> ditto.
>
> I see that the devfreq core always sets these function pointers in this
> patch, but that's not an assumption the PM core should make, IMO.
>
>> +             if (retval)
>> +                     goto skip_callback;
>> +
>>       if (dev->pm_domain)
>>               callback = dev->pm_domain->ops.runtime_resume;
>>       else if (dev->type && dev->type->pm)
>> @@ -653,9 +666,12 @@ static int rpm_resume(struct device *dev, int rpmflags)
>>               callback = dev->driver->pm->runtime_resume;
>>
>>       retval = rpm_callback(callback, dev);
>> +skip_callback:
>>       if (retval) {
>>               __update_runtime_status(dev, RPM_SUSPENDED);
>>               pm_runtime_cancel_pending(dev);
>> +             if (dev->power.devfreq)
>> +                     dev->power.devfreq->runtime_suspend(dev);
>>       } else {
>>   no_callback:
>>               __update_runtime_status(dev, RPM_ACTIVE);
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 44c4079..ed6cb88 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 int devfreq_runtime_suspend(struct device *dev);
>> +static int devfreq_runtime_resume(struct device *dev);
>> +
>>  /**
>>   * find_device_devfreq() - find devfreq struct using device pointer
>>   * @dev:     device pointer used to lookup device devfreq.
>> @@ -476,6 +480,9 @@ 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;
>> +     dev->power.devfreq = devfreq;
>>
>>       devfreq->trans_table =  devm_kzalloc(dev, sizeof(unsigned int) *
>>                                               devfreq->profile->max_state *
>> @@ -549,7 +556,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 +567,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 +576,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 +587,44 @@ 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 int devfreq_runtime_suspend(struct device *dev)
>> +{
>> +     struct devfreq *devfreq;
>> +
>> +     mutex_lock(&devfreq_list_lock);
>> +     devfreq = find_device_devfreq(dev);
>> +     mutex_unlock(&devfreq_list_lock);
>> +
>> +     return devfreq_suspend_device(devfreq);
>> +}
>> +
>> +/**
>> + * 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 int devfreq_runtime_resume(struct device *dev)
>> +{
>> +     struct devfreq *devfreq;
>> +
>> +     mutex_lock(&devfreq_list_lock);
>> +     devfreq = find_device_devfreq(dev);
>> +     mutex_unlock(&devfreq_list_lock);
>> +
>> +     return devfreq_resume_device(devfreq);
>> +}
>>
>>  /**
>>   * devfreq_add_governor() - Add devfreq governor
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index 5f1ab92..f50f46e 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;
>> +     int (*runtime_suspend)(struct device *dev);
>> +     int (*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);
>
> Remvoing these, and removing the EXPORT_SYMBOL()s is an API change that
> is not described in the changelog.
>
> Kevin
>
>> -
>>  /* 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..6097db1 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;
>>  };
>>
>>  extern void update_pm_runtime_accounting(struct device *dev);



-- 
Regards,
Rajagopal