linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] PM / devfreq: add relation of recommended frequency.
@ 2012-02-10  7:19 MyungJoo Ham
  2012-02-29 22:59 ` Rafael J. Wysocki
  2012-03-07 17:45 ` [PATCH] " Turquette, Mike
  0 siblings, 2 replies; 13+ messages in thread
From: MyungJoo Ham @ 2012-02-10  7:19 UTC (permalink / raw)
  To: linux-kernel, linux-pm
  Cc: Kyungmin Park, Rafael J. Wysocki, Kevin Hilman, Mike Turquette,
	myungjoo.ham

The semantics of "target frequency" given to devfreq driver from
devfreq framework has always been interpretted as "at least" or GLB
(greatest lower bound). However, the framework might want the
device driver to limit its max frequency (LUB: least upper bound),
especially if it is given by thermal framework (it's too hot).

Thus, the target fuction should have another parameter to express
whether the framework wants GLB or LUB. And, the additional parameter,
"u32 options", does it.

With the update, devfreq_recommended_opp() is also updated.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/devfreq/devfreq.c     |   44 ++++++++++++++++++++++++++++++----------
 drivers/devfreq/exynos4_bus.c |   16 +++++++++++---
 include/linux/devfreq.h       |   18 ++++++++++++++--
 3 files changed, 60 insertions(+), 18 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index a33fc6c..83392a6 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -84,6 +84,7 @@ int update_devfreq(struct devfreq *devfreq)
 {
 	unsigned long freq;
 	int err = 0;
+	u32 options = 0;
 
 	if (!mutex_is_locked(&devfreq->lock)) {
 		WARN(true, "devfreq->lock must be locked by the caller.\n");
@@ -104,18 +105,23 @@ int update_devfreq(struct devfreq *devfreq)
 	 * qos_min_freq
 	 */
 
-	if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq)
+	if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq) {
 		freq = devfreq->qos_min_freq;
-	if (devfreq->max_freq && freq > devfreq->max_freq)
+		options &= ~(1 << 0);
+		options |= DEVFREQ_OPTION_FREQ_LUB;
+	}
+	if (devfreq->max_freq && freq > devfreq->max_freq) {
 		freq = devfreq->max_freq;
-	if (devfreq->min_freq && freq < devfreq->min_freq)
+		options &= ~(1 << 0);
+		options |= DEVFREQ_OPTION_FREQ_GLB;
+	}
+	if (devfreq->min_freq && freq < devfreq->min_freq) {
 		freq = devfreq->min_freq;
+		options &= ~(1 << 0);
+		options |= DEVFREQ_OPTION_FREQ_LUB;
+	}
 
-	/*
-	 * TODO in the devfreq-next:
-	 * add relation or use rance (freq_min, freq_max)
-	 */
-	err = devfreq->profile->target(devfreq->dev.parent, &freq);
+	err = devfreq->profile->target(devfreq->dev.parent, &freq, options);
 	if (err)
 		return err;
 
@@ -771,14 +777,30 @@ module_exit(devfreq_exit);
  *			     freq value given to target callback.
  * @dev		The devfreq user device. (parent of devfreq)
  * @freq	The frequency given to target function
+ * @floor	false: find LUB first and use GLB if LUB not available.
+ *		true:  find GLB first and use LUB if GLB not available.
+ *
+ * LUB: least upper bound (at least this freq or above, but the least)
+ * GLB: greatest lower bound (at most this freq or below, but the most)
  *
  */
-struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq)
+struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
+				    bool floor)
 {
-	struct opp *opp = opp_find_freq_ceil(dev, freq);
+	struct opp *opp;
 
-	if (opp == ERR_PTR(-ENODEV))
+	if (floor) {
 		opp = opp_find_freq_floor(dev, freq);
+
+		if (opp == ERR_PTR(-ENODEV))
+			opp = opp_find_freq_ceil(dev, freq);
+	} else {
+		opp = opp_find_freq_ceil(dev, freq);
+
+		if (opp == ERR_PTR(-ENODEV))
+			opp = opp_find_freq_floor(dev, freq);
+	}
+
 	return opp;
 }
 
diff --git a/drivers/devfreq/exynos4_bus.c b/drivers/devfreq/exynos4_bus.c
index 590d686..c0a78da 100644
--- a/drivers/devfreq/exynos4_bus.c
+++ b/drivers/devfreq/exynos4_bus.c
@@ -619,13 +619,21 @@ static int exynos4_bus_setvolt(struct busfreq_data *data, struct opp *opp,
 	return err;
 }
 
-static int exynos4_bus_target(struct device *dev, unsigned long *_freq)
+static int exynos4_bus_target(struct device *dev, unsigned long *_freq,
+			      u32 options)
 {
 	int err = 0;
-	struct busfreq_data *data = dev_get_drvdata(dev);
-	struct opp *opp = devfreq_recommended_opp(dev, _freq);
-	unsigned long old_freq = opp_get_freq(data->curr_opp);
+	unsigned long def = *_freq;
+	struct platform_device *pdev = container_of(dev, struct platform_device,
+						    dev);
+	struct busfreq_data *data = platform_get_drvdata(pdev);
+	struct opp *opp = devfreq_recommended_opp(dev, _freq, options &
+						  DEVFREQ_OPTION_FREQ_GLB);
 	unsigned long freq = opp_get_freq(opp);
+	unsigned long old_freq = opp_get_freq(data->curr_opp);
+
+	if (IS_ERR(opp))
+		return PTR_ERR(opp);
 
 	if (old_freq == freq)
 		return 0;
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index b853379..1aff012 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -59,6 +59,16 @@ struct devfreq_pm_qos_table {
 	s32 qos_value;
 };
 
+/*
+ * target callback, which is to provide additional information to the
+ * devfreq driver.
+ */
+
+/* The resulting frequency should be at least this. (least upper bound) */
+#define DEVFREQ_OPTION_FREQ_LUB	0x0
+/* The resulting frequency should be at most this. (greatest lower bound) */
+#define DEVFREQ_OPTION_FREQ_GLB 0x1
+
 /**
  * struct devfreq_dev_profile - Devfreq's user device profile
  * @initial_freq	The operating frequency when devfreq_add_device() is
@@ -76,6 +86,8 @@ struct devfreq_pm_qos_table {
  *			higher than any operable frequency, set maximum.
  *			Before returning, target function should set
  *			freq at the current frequency.
+ *			The "option" parameter's possible values are
+ *			explained above with "DEVFREQ_OPTION_*" macros.
  * @get_dev_status	The device should provide the current performance
  *			status to devfreq, which is used by governors.
  * @exit		An optional callback that is called when devfreq
@@ -95,7 +107,7 @@ struct devfreq_dev_profile {
 	bool qos_use_max;
 	struct devfreq_pm_qos_table *qos_list;
 
-	int (*target)(struct device *dev, unsigned long *freq);
+	int (*target)(struct device *dev, unsigned long *freq, u32 options);
 	int (*get_dev_status)(struct device *dev,
 			      struct devfreq_dev_status *stat);
 	void (*exit)(struct device *dev);
@@ -198,7 +210,7 @@ extern int devfreq_remove_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);
+					   unsigned long *freq, bool floor);
 extern int devfreq_register_opp_notifier(struct device *dev,
 					 struct devfreq *devfreq);
 extern int devfreq_unregister_opp_notifier(struct device *dev,
@@ -253,7 +265,7 @@ static int devfreq_remove_device(struct devfreq *devfreq)
 }
 
 static struct opp *devfreq_recommended_opp(struct device *dev,
-					   unsigned long *freq)
+					   unsigned long *freq, bool floor)
 {
 	return -EINVAL;
 }
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / devfreq: add relation of recommended frequency.
  2012-02-10  7:19 [PATCH] PM / devfreq: add relation of recommended frequency MyungJoo Ham
@ 2012-02-29 22:59 ` Rafael J. Wysocki
  2012-03-05  7:44   ` MyungJoo Ham
  2012-03-07  7:40   ` MyungJoo Ham
  2012-03-07 17:45 ` [PATCH] " Turquette, Mike
  1 sibling, 2 replies; 13+ messages in thread
From: Rafael J. Wysocki @ 2012-02-29 22:59 UTC (permalink / raw)
  To: MyungJoo Ham
  Cc: linux-kernel, linux-pm, Kyungmin Park, Kevin Hilman,
	Mike Turquette, myungjoo.ham

Hi,

Sorry, I seem to have overlooked this patch.

On Friday, February 10, 2012, MyungJoo Ham wrote:
> The semantics of "target frequency" given to devfreq driver from
> devfreq framework has always been interpretted as "at least" or GLB
> (greatest lower bound). However, the framework might want the
> device driver to limit its max frequency (LUB: least upper bound),
> especially if it is given by thermal framework (it's too hot).
> 
> Thus, the target fuction should have another parameter to express
> whether the framework wants GLB or LUB. And, the additional parameter,
> "u32 options", does it.

I'd call that "flags" (which usually is the case in the kernel for things
handled with the help of bitwise operators).

> With the update, devfreq_recommended_opp() is also updated.
> 
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>
> ---
>  drivers/devfreq/devfreq.c     |   44 ++++++++++++++++++++++++++++++----------
>  drivers/devfreq/exynos4_bus.c |   16 +++++++++++---
>  include/linux/devfreq.h       |   18 ++++++++++++++--
>  3 files changed, 60 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index a33fc6c..83392a6 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -84,6 +84,7 @@ int update_devfreq(struct devfreq *devfreq)
>  {
>  	unsigned long freq;
>  	int err = 0;
> +	u32 options = 0;
>  
>  	if (!mutex_is_locked(&devfreq->lock)) {
>  		WARN(true, "devfreq->lock must be locked by the caller.\n");
> @@ -104,18 +105,23 @@ int update_devfreq(struct devfreq *devfreq)
>  	 * qos_min_freq
>  	 */
>  
> -	if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq)
> +	if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq) {
>  		freq = devfreq->qos_min_freq;
> -	if (devfreq->max_freq && freq > devfreq->max_freq)
> +		options &= ~(1 << 0);

What exactly is the (1 << 0) for?

Whatever the case is I'd just use one flag name, e.g.
DEVFREQ_LEAST_UPPER_BOUND (equal to (1U << 0), and assume that the other
option is to be chosen when this flag is not set.


> +		options |= DEVFREQ_OPTION_FREQ_LUB;

So the above would become:

+       if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq) {
                freq = devfreq->qos_min_freq;
-       if (devfreq->max_freq && freq > devfreq->max_freq)
+               options |= DEVFREQ_LEAST_UPPER_BOUND;
+       }

> +	}
> +	if (devfreq->max_freq && freq > devfreq->max_freq) {
>  		freq = devfreq->max_freq;
> -	if (devfreq->min_freq && freq < devfreq->min_freq)
> +		options &= ~(1 << 0);
> +		options |= DEVFREQ_OPTION_FREQ_GLB;
> +	}

and this one would become:

+       if (devfreq->max_freq && freq > devfreq->max_freq) {
                freq = devfreq->max_freq;
-       if (devfreq->min_freq && freq < devfreq->min_freq)
+               options &= ~DEVFREQ_LEAST_UPPER_BOUND;
+       }

Then, the code would be much easier to follow.

> +	if (devfreq->min_freq && freq < devfreq->min_freq) {
>  		freq = devfreq->min_freq;
> +		options &= ~(1 << 0);
> +		options |= DEVFREQ_OPTION_FREQ_LUB;
> +	}
>  
> -	/*
> -	 * TODO in the devfreq-next:
> -	 * add relation or use rance (freq_min, freq_max)
> -	 */
> -	err = devfreq->profile->target(devfreq->dev.parent, &freq);
> +	err = devfreq->profile->target(devfreq->dev.parent, &freq, options);
>  	if (err)
>  		return err;
>  
> @@ -771,14 +777,30 @@ module_exit(devfreq_exit);
>   *			     freq value given to target callback.
>   * @dev		The devfreq user device. (parent of devfreq)
>   * @freq	The frequency given to target function
> + * @floor	false: find LUB first and use GLB if LUB not available.
> + *		true:  find GLB first and use LUB if GLB not available.
> + *
> + * LUB: least upper bound (at least this freq or above, but the least)
> + * GLB: greatest lower bound (at most this freq or below, but the most)
>   *
>   */
> -struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq)
> +struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
> +				    bool floor)

Why don't you use "u32 flags" here too?

>  {
> -	struct opp *opp = opp_find_freq_ceil(dev, freq);
> +	struct opp *opp;
>  
> -	if (opp == ERR_PTR(-ENODEV))
> +	if (floor) {

That would become

+	if (options & DEVFREQ_LEAST_UPPER_BOUND) {

which is a bit more meaningful IMO.

>  		opp = opp_find_freq_floor(dev, freq);
> +
> +		if (opp == ERR_PTR(-ENODEV))
> +			opp = opp_find_freq_ceil(dev, freq);
> +	} else {
> +		opp = opp_find_freq_ceil(dev, freq);
> +
> +		if (opp == ERR_PTR(-ENODEV))
> +			opp = opp_find_freq_floor(dev, freq);
> +	}
> +
>  	return opp;
>  }
>  
> diff --git a/drivers/devfreq/exynos4_bus.c b/drivers/devfreq/exynos4_bus.c
> index 590d686..c0a78da 100644
> --- a/drivers/devfreq/exynos4_bus.c
> +++ b/drivers/devfreq/exynos4_bus.c
> @@ -619,13 +619,21 @@ static int exynos4_bus_setvolt(struct busfreq_data *data, struct opp *opp,
>  	return err;
>  }
>  
> -static int exynos4_bus_target(struct device *dev, unsigned long *_freq)
> +static int exynos4_bus_target(struct device *dev, unsigned long *_freq,
> +			      u32 options)
>  {
>  	int err = 0;
> -	struct busfreq_data *data = dev_get_drvdata(dev);
> -	struct opp *opp = devfreq_recommended_opp(dev, _freq);
> -	unsigned long old_freq = opp_get_freq(data->curr_opp);
> +	unsigned long def = *_freq;
> +	struct platform_device *pdev = container_of(dev, struct platform_device,
> +						    dev);
> +	struct busfreq_data *data = platform_get_drvdata(pdev);
> +	struct opp *opp = devfreq_recommended_opp(dev, _freq, options &
> +						  DEVFREQ_OPTION_FREQ_GLB);
>  	unsigned long freq = opp_get_freq(opp);
> +	unsigned long old_freq = opp_get_freq(data->curr_opp);
> +
> +	if (IS_ERR(opp))
> +		return PTR_ERR(opp);
>  
>  	if (old_freq == freq)
>  		return 0;
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index b853379..1aff012 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -59,6 +59,16 @@ struct devfreq_pm_qos_table {
>  	s32 qos_value;
>  };
>  
> +/*
> + * target callback, which is to provide additional information to the
> + * devfreq driver.
> + */
> +
> +/* The resulting frequency should be at least this. (least upper bound) */
> +#define DEVFREQ_OPTION_FREQ_LUB	0x0
> +/* The resulting frequency should be at most this. (greatest lower bound) */
> +#define DEVFREQ_OPTION_FREQ_GLB 0x1

As I said, I'd use one option name and call it DEVFREQ_LEAST_UPPER_BOUND.
It is not necessary to give a name to the alternative. :-)

> +
>  /**
>   * struct devfreq_dev_profile - Devfreq's user device profile
>   * @initial_freq	The operating frequency when devfreq_add_device() is
> @@ -76,6 +86,8 @@ struct devfreq_pm_qos_table {
>   *			higher than any operable frequency, set maximum.
>   *			Before returning, target function should set
>   *			freq at the current frequency.
> + *			The "option" parameter's possible values are
> + *			explained above with "DEVFREQ_OPTION_*" macros.
>   * @get_dev_status	The device should provide the current performance
>   *			status to devfreq, which is used by governors.
>   * @exit		An optional callback that is called when devfreq
> @@ -95,7 +107,7 @@ struct devfreq_dev_profile {
>  	bool qos_use_max;
>  	struct devfreq_pm_qos_table *qos_list;
>  
> -	int (*target)(struct device *dev, unsigned long *freq);
> +	int (*target)(struct device *dev, unsigned long *freq, u32 options);
>  	int (*get_dev_status)(struct device *dev,
>  			      struct devfreq_dev_status *stat);
>  	void (*exit)(struct device *dev);
> @@ -198,7 +210,7 @@ extern int devfreq_remove_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);
> +					   unsigned long *freq, bool floor);
>  extern int devfreq_register_opp_notifier(struct device *dev,
>  					 struct devfreq *devfreq);
>  extern int devfreq_unregister_opp_notifier(struct device *dev,
> @@ -253,7 +265,7 @@ static int devfreq_remove_device(struct devfreq *devfreq)
>  }
>  
>  static struct opp *devfreq_recommended_opp(struct device *dev,
> -					   unsigned long *freq)
> +					   unsigned long *freq, bool floor)
>  {
>  	return -EINVAL;
>  }
> 

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / devfreq: add relation of recommended frequency.
  2012-02-29 22:59 ` Rafael J. Wysocki
@ 2012-03-05  7:44   ` MyungJoo Ham
  2012-03-07  7:40   ` MyungJoo Ham
  1 sibling, 0 replies; 13+ messages in thread
From: MyungJoo Ham @ 2012-03-05  7:44 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-kernel, linux-pm, Kyungmin Park, Kevin Hilman, Mike Turquette

2012/3/1 Rafael J. Wysocki <rjw@sisk.pl>:
> Hi,
>
> Sorry, I seem to have overlooked this patch.
>
> On Friday, February 10, 2012, MyungJoo Ham wrote:
>> The semantics of "target frequency" given to devfreq driver from
>> devfreq framework has always been interpretted as "at least" or GLB
>> (greatest lower bound). However, the framework might want the
>> device driver to limit its max frequency (LUB: least upper bound),
>> especially if it is given by thermal framework (it's too hot).
>>
>> Thus, the target fuction should have another parameter to express
>> whether the framework wants GLB or LUB. And, the additional parameter,
>> "u32 options", does it.
>
> I'd call that "flags" (which usually is the case in the kernel for things
> handled with the help of bitwise operators).

Hmm.. I agree. "Flag" looks better.

[]
>> -     if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq)
>> +     if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq) {
>>               freq = devfreq->qos_min_freq;
>> -     if (devfreq->max_freq && freq > devfreq->max_freq)
>> +             options &= ~(1 << 0);
>
> What exactly is the (1 << 0) for?
>
> Whatever the case is I'd just use one flag name, e.g.
> DEVFREQ_LEAST_UPPER_BOUND (equal to (1U << 0), and assume that the other
> option is to be chosen when this flag is not set.
>
>
>> +             options |= DEVFREQ_OPTION_FREQ_LUB;
>
> So the above would become:
>
> +       if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq) {
>                freq = devfreq->qos_min_freq;
> -       if (devfreq->max_freq && freq > devfreq->max_freq)
> +               options |= DEVFREQ_LEAST_UPPER_BOUND;
> +       }
>
>> +     }
>> +     if (devfreq->max_freq && freq > devfreq->max_freq) {
>>               freq = devfreq->max_freq;
>> -     if (devfreq->min_freq && freq < devfreq->min_freq)
>> +             options &= ~(1 << 0);
>> +             options |= DEVFREQ_OPTION_FREQ_GLB;
>> +     }
>
> and this one would become:
>
> +       if (devfreq->max_freq && freq > devfreq->max_freq) {
>                freq = devfreq->max_freq;
> -       if (devfreq->min_freq && freq < devfreq->min_freq)
> +               options &= ~DEVFREQ_LEAST_UPPER_BOUND;
> +       }
>
> Then, the code would be much easier to follow.
>

Ok, I'll remove one of the two. It appears pretty without either one of the two.

>> @@ -771,14 +777,30 @@ module_exit(devfreq_exit);
>>   *                        freq value given to target callback.
>>   * @dev              The devfreq user device. (parent of devfreq)
>>   * @freq     The frequency given to target function
>> + * @floor    false: find LUB first and use GLB if LUB not available.
>> + *           true:  find GLB first and use LUB if GLB not available.
>> + *
>> + * LUB: least upper bound (at least this freq or above, but the least)
>> + * GLB: greatest lower bound (at most this freq or below, but the most)
>>   *
>>   */
>> -struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq)
>> +struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
>> +                                 bool floor)
>
> Why don't you use "u32 flags" here too?
>

Reading this again, it doesn't matter. That'd use u32 flags, too.

>>  {
>> -     struct opp *opp = opp_find_freq_ceil(dev, freq);
>> +     struct opp *opp;
>>
>> -     if (opp == ERR_PTR(-ENODEV))
>> +     if (floor) {
>
> That would become
>
> +       if (options & DEVFREQ_LEAST_UPPER_BOUND) {
>
> which is a bit more meaningful IMO.
>

Yes.. I agree.

[]
>>
>> +/*
>> + * target callback, which is to provide additional information to the
>> + * devfreq driver.
>> + */
>> +
>> +/* The resulting frequency should be at least this. (least upper bound) */
>> +#define DEVFREQ_OPTION_FREQ_LUB      0x0
>> +/* The resulting frequency should be at most this. (greatest lower bound) */
>> +#define DEVFREQ_OPTION_FREQ_GLB 0x1
>
> As I said, I'd use one option name and call it DEVFREQ_LEAST_UPPER_BOUND.
> It is not necessary to give a name to the alternative. :-)

Yes. ;)



Thank you.



Cheers!
MyungJoo.


-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab, DMC Business, Samsung Electronics

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] PM / devfreq: add relation of recommended frequency.
  2012-02-29 22:59 ` Rafael J. Wysocki
  2012-03-05  7:44   ` MyungJoo Ham
@ 2012-03-07  7:40   ` MyungJoo Ham
  2012-03-10 21:51     ` Rafael J. Wysocki
  2012-03-15  0:25     ` Rafael J. Wysocki
  1 sibling, 2 replies; 13+ messages in thread
From: MyungJoo Ham @ 2012-03-07  7:40 UTC (permalink / raw)
  To: linux-pm
  Cc: linux-kernel, Rafael J. Wysocki, Kyungmin Park, Kevin Hilman,
	Mike Turquette, myungjoo.ham

The semantics of "target frequency" given to devfreq driver from
devfreq framework has always been interpretted as "at least" or GLB
(greatest lower bound). However, the framework might want the
device driver to limit its max frequency (LUB: least upper bound),
especially if it is given by thermal framework (it's too hot).

Thus, the target fuction should have another parameter to express
whether the framework wants GLB or LUB. And, the additional parameter,
"u32 options", does it.

With the update, devfreq_recommended_opp() is also updated.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

---
Changes from v1
- Style update.
---
 drivers/devfreq/devfreq.c     |   41 ++++++++++++++++++++++++++++++-----------
 drivers/devfreq/exynos4_bus.c |   14 ++++++++++----
 include/linux/devfreq.h       |   16 +++++++++++++---
 3 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 0b49f59..13794d1 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -84,6 +84,7 @@ int update_devfreq(struct devfreq *devfreq)
 {
 	unsigned long freq;
 	int err = 0;
+	u32 flags = 0;
 
 	if (!mutex_is_locked(&devfreq->lock)) {
 		WARN(true, "devfreq->lock must be locked by the caller.\n");
@@ -104,18 +105,20 @@ int update_devfreq(struct devfreq *devfreq)
 	 * qos_min_freq
 	 */
 
-	if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq)
+	if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq) {
 		freq = devfreq->qos_min_freq;
-	if (devfreq->max_freq && freq > devfreq->max_freq)
+		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
+	}
+	if (devfreq->max_freq && freq > devfreq->max_freq) {
 		freq = devfreq->max_freq;
-	if (devfreq->min_freq && freq < devfreq->min_freq)
+		flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
+	}
+	if (devfreq->min_freq && freq < devfreq->min_freq) {
 		freq = devfreq->min_freq;
+		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
+	}
 
-	/*
-	 * TODO in the devfreq-next:
-	 * add relation or use rance (freq_min, freq_max)
-	 */
-	err = devfreq->profile->target(devfreq->dev.parent, &freq);
+	err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
 	if (err)
 		return err;
 
@@ -773,14 +776,30 @@ module_exit(devfreq_exit);
  *			     freq value given to target callback.
  * @dev		The devfreq user device. (parent of devfreq)
  * @freq	The frequency given to target function
+ * @flags	Flags handed from devfreq framework.
  *
  */
-struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq)
+struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
+				    u32 flags)
 {
-	struct opp *opp = opp_find_freq_ceil(dev, freq);
+	struct opp *opp;
 
-	if (opp == ERR_PTR(-ENODEV))
+	if (flags & DEVFREQ_FLAG_LEAST_UPPER_BOUND) {
+		/* The freq is an upper bound. opp should be lower */
 		opp = opp_find_freq_floor(dev, freq);
+
+		/* If not available, use the closest opp */
+		if (opp == ERR_PTR(-ENODEV))
+			opp = opp_find_freq_ceil(dev, freq);
+	} else {
+		/* The freq is an lower bound. opp should be higher */
+		opp = opp_find_freq_ceil(dev, freq);
+
+		/* If not available, use the closest opp */
+		if (opp == ERR_PTR(-ENODEV))
+			opp = opp_find_freq_floor(dev, freq);
+	}
+
 	return opp;
 }
 
diff --git a/drivers/devfreq/exynos4_bus.c b/drivers/devfreq/exynos4_bus.c
index 590d686..1a361e9 100644
--- a/drivers/devfreq/exynos4_bus.c
+++ b/drivers/devfreq/exynos4_bus.c
@@ -619,13 +619,19 @@ static int exynos4_bus_setvolt(struct busfreq_data *data, struct opp *opp,
 	return err;
 }
 
-static int exynos4_bus_target(struct device *dev, unsigned long *_freq)
+static int exynos4_bus_target(struct device *dev, unsigned long *_freq,
+			      u32 flags)
 {
 	int err = 0;
-	struct busfreq_data *data = dev_get_drvdata(dev);
-	struct opp *opp = devfreq_recommended_opp(dev, _freq);
-	unsigned long old_freq = opp_get_freq(data->curr_opp);
+	struct platform_device *pdev = container_of(dev, struct platform_device,
+						    dev);
+	struct busfreq_data *data = platform_get_drvdata(pdev);
+	struct opp *opp = devfreq_recommended_opp(dev, _freq, flags);
 	unsigned long freq = opp_get_freq(opp);
+	unsigned long old_freq = opp_get_freq(data->curr_opp);
+
+	if (IS_ERR(opp))
+		return PTR_ERR(opp);
 
 	if (old_freq == freq)
 		return 0;
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index f27d511..85d4b0f 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -59,6 +59,14 @@ struct devfreq_pm_qos_table {
 	s32 qos_value;
 };
 
+/*
+ * The resulting frequency should be at most this. (this bound is the
+ * least upper bound; thus, the resulting freq should be lower or same)
+ * If the flag is not set, the resulting frequency should be at most the
+ * bound (greatest lower bound)
+ */
+#define DEVFREQ_FLAG_LEAST_UPPER_BOUND		0x1
+
 /**
  * struct devfreq_dev_profile - Devfreq's user device profile
  * @initial_freq	The operating frequency when devfreq_add_device() is
@@ -74,6 +82,8 @@ struct devfreq_pm_qos_table {
  *			higher than any operable frequency, set maximum.
  *			Before returning, target function should set
  *			freq at the current frequency.
+ *			The "flags" parameter's possible values are
+ *			explained above with "DEVFREQ_FLAG_*" macros.
  * @get_dev_status	The device should provide the current performance
  *			status to devfreq, which is used by governors.
  * @exit		An optional callback that is called when devfreq
@@ -92,7 +102,7 @@ struct devfreq_dev_profile {
 	int qos_type;
 	struct devfreq_pm_qos_table *qos_list;
 
-	int (*target)(struct device *dev, unsigned long *freq);
+	int (*target)(struct device *dev, unsigned long *freq, u32 flags);
 	int (*get_dev_status)(struct device *dev,
 			      struct devfreq_dev_status *stat);
 	void (*exit)(struct device *dev);
@@ -198,7 +208,7 @@ extern int devfreq_remove_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);
+					   unsigned long *freq, u32 flags);
 extern int devfreq_register_opp_notifier(struct device *dev,
 					 struct devfreq *devfreq);
 extern int devfreq_unregister_opp_notifier(struct device *dev,
@@ -253,7 +263,7 @@ static int devfreq_remove_device(struct devfreq *devfreq)
 }
 
 static struct opp *devfreq_recommended_opp(struct device *dev,
-					   unsigned long *freq)
+					   unsigned long *freq, u32 flags)
 {
 	return -EINVAL;
 }
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / devfreq: add relation of recommended frequency.
  2012-02-10  7:19 [PATCH] PM / devfreq: add relation of recommended frequency MyungJoo Ham
  2012-02-29 22:59 ` Rafael J. Wysocki
@ 2012-03-07 17:45 ` Turquette, Mike
  2012-03-09  7:09   ` MyungJoo Ham
  1 sibling, 1 reply; 13+ messages in thread
From: Turquette, Mike @ 2012-03-07 17:45 UTC (permalink / raw)
  To: MyungJoo Ham
  Cc: linux-kernel, linux-pm, Kyungmin Park, Rafael J. Wysocki,
	Kevin Hilman, myungjoo.ham

On Thu, Feb 9, 2012 at 11:19 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> The semantics of "target frequency" given to devfreq driver from
> devfreq framework has always been interpretted as "at least" or GLB
> (greatest lower bound). However, the framework might want the
> device driver to limit its max frequency (LUB: least upper bound),
> especially if it is given by thermal framework (it's too hot).
>
> Thus, the target fuction should have another parameter to express
> whether the framework wants GLB or LUB. And, the additional parameter,
> "u32 options", does it.

Nitpick: changelog still refers to "options" but code reads "flags".

Regards,
Mike

>
> With the update, devfreq_recommended_opp() is also updated.
>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/devfreq/devfreq.c     |   44 ++++++++++++++++++++++++++++++----------
>  drivers/devfreq/exynos4_bus.c |   16 +++++++++++---
>  include/linux/devfreq.h       |   18 ++++++++++++++--
>  3 files changed, 60 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index a33fc6c..83392a6 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -84,6 +84,7 @@ int update_devfreq(struct devfreq *devfreq)
>  {
>        unsigned long freq;
>        int err = 0;
> +       u32 options = 0;
>
>        if (!mutex_is_locked(&devfreq->lock)) {
>                WARN(true, "devfreq->lock must be locked by the caller.\n");
> @@ -104,18 +105,23 @@ int update_devfreq(struct devfreq *devfreq)
>         * qos_min_freq
>         */
>
> -       if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq)
> +       if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq) {
>                freq = devfreq->qos_min_freq;
> -       if (devfreq->max_freq && freq > devfreq->max_freq)
> +               options &= ~(1 << 0);
> +               options |= DEVFREQ_OPTION_FREQ_LUB;
> +       }
> +       if (devfreq->max_freq && freq > devfreq->max_freq) {
>                freq = devfreq->max_freq;
> -       if (devfreq->min_freq && freq < devfreq->min_freq)
> +               options &= ~(1 << 0);
> +               options |= DEVFREQ_OPTION_FREQ_GLB;
> +       }
> +       if (devfreq->min_freq && freq < devfreq->min_freq) {
>                freq = devfreq->min_freq;
> +               options &= ~(1 << 0);
> +               options |= DEVFREQ_OPTION_FREQ_LUB;
> +       }
>
> -       /*
> -        * TODO in the devfreq-next:
> -        * add relation or use rance (freq_min, freq_max)
> -        */
> -       err = devfreq->profile->target(devfreq->dev.parent, &freq);
> +       err = devfreq->profile->target(devfreq->dev.parent, &freq, options);
>        if (err)
>                return err;
>
> @@ -771,14 +777,30 @@ module_exit(devfreq_exit);
>  *                          freq value given to target callback.
>  * @dev                The devfreq user device. (parent of devfreq)
>  * @freq       The frequency given to target function
> + * @floor      false: find LUB first and use GLB if LUB not available.
> + *             true:  find GLB first and use LUB if GLB not available.
> + *
> + * LUB: least upper bound (at least this freq or above, but the least)
> + * GLB: greatest lower bound (at most this freq or below, but the most)
>  *
>  */
> -struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq)
> +struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
> +                                   bool floor)
>  {
> -       struct opp *opp = opp_find_freq_ceil(dev, freq);
> +       struct opp *opp;
>
> -       if (opp == ERR_PTR(-ENODEV))
> +       if (floor) {
>                opp = opp_find_freq_floor(dev, freq);
> +
> +               if (opp == ERR_PTR(-ENODEV))
> +                       opp = opp_find_freq_ceil(dev, freq);
> +       } else {
> +               opp = opp_find_freq_ceil(dev, freq);
> +
> +               if (opp == ERR_PTR(-ENODEV))
> +                       opp = opp_find_freq_floor(dev, freq);
> +       }
> +
>        return opp;
>  }
>
> diff --git a/drivers/devfreq/exynos4_bus.c b/drivers/devfreq/exynos4_bus.c
> index 590d686..c0a78da 100644
> --- a/drivers/devfreq/exynos4_bus.c
> +++ b/drivers/devfreq/exynos4_bus.c
> @@ -619,13 +619,21 @@ static int exynos4_bus_setvolt(struct busfreq_data *data, struct opp *opp,
>        return err;
>  }
>
> -static int exynos4_bus_target(struct device *dev, unsigned long *_freq)
> +static int exynos4_bus_target(struct device *dev, unsigned long *_freq,
> +                             u32 options)
>  {
>        int err = 0;
> -       struct busfreq_data *data = dev_get_drvdata(dev);
> -       struct opp *opp = devfreq_recommended_opp(dev, _freq);
> -       unsigned long old_freq = opp_get_freq(data->curr_opp);
> +       unsigned long def = *_freq;
> +       struct platform_device *pdev = container_of(dev, struct platform_device,
> +                                                   dev);
> +       struct busfreq_data *data = platform_get_drvdata(pdev);
> +       struct opp *opp = devfreq_recommended_opp(dev, _freq, options &
> +                                                 DEVFREQ_OPTION_FREQ_GLB);
>        unsigned long freq = opp_get_freq(opp);
> +       unsigned long old_freq = opp_get_freq(data->curr_opp);
> +
> +       if (IS_ERR(opp))
> +               return PTR_ERR(opp);
>
>        if (old_freq == freq)
>                return 0;
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index b853379..1aff012 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -59,6 +59,16 @@ struct devfreq_pm_qos_table {
>        s32 qos_value;
>  };
>
> +/*
> + * target callback, which is to provide additional information to the
> + * devfreq driver.
> + */
> +
> +/* The resulting frequency should be at least this. (least upper bound) */
> +#define DEVFREQ_OPTION_FREQ_LUB        0x0
> +/* The resulting frequency should be at most this. (greatest lower bound) */
> +#define DEVFREQ_OPTION_FREQ_GLB 0x1
> +
>  /**
>  * struct devfreq_dev_profile - Devfreq's user device profile
>  * @initial_freq       The operating frequency when devfreq_add_device() is
> @@ -76,6 +86,8 @@ struct devfreq_pm_qos_table {
>  *                     higher than any operable frequency, set maximum.
>  *                     Before returning, target function should set
>  *                     freq at the current frequency.
> + *                     The "option" parameter's possible values are
> + *                     explained above with "DEVFREQ_OPTION_*" macros.
>  * @get_dev_status     The device should provide the current performance
>  *                     status to devfreq, which is used by governors.
>  * @exit               An optional callback that is called when devfreq
> @@ -95,7 +107,7 @@ struct devfreq_dev_profile {
>        bool qos_use_max;
>        struct devfreq_pm_qos_table *qos_list;
>
> -       int (*target)(struct device *dev, unsigned long *freq);
> +       int (*target)(struct device *dev, unsigned long *freq, u32 options);
>        int (*get_dev_status)(struct device *dev,
>                              struct devfreq_dev_status *stat);
>        void (*exit)(struct device *dev);
> @@ -198,7 +210,7 @@ extern int devfreq_remove_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);
> +                                          unsigned long *freq, bool floor);
>  extern int devfreq_register_opp_notifier(struct device *dev,
>                                         struct devfreq *devfreq);
>  extern int devfreq_unregister_opp_notifier(struct device *dev,
> @@ -253,7 +265,7 @@ static int devfreq_remove_device(struct devfreq *devfreq)
>  }
>
>  static struct opp *devfreq_recommended_opp(struct device *dev,
> -                                          unsigned long *freq)
> +                                          unsigned long *freq, bool floor)
>  {
>        return -EINVAL;
>  }
> --
> 1.7.4.1
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / devfreq: add relation of recommended frequency.
  2012-03-07 17:45 ` [PATCH] " Turquette, Mike
@ 2012-03-09  7:09   ` MyungJoo Ham
  0 siblings, 0 replies; 13+ messages in thread
From: MyungJoo Ham @ 2012-03-09  7:09 UTC (permalink / raw)
  To: Turquette, Mike
  Cc: linux-kernel, linux-pm, Kyungmin Park, Rafael J. Wysocki, Kevin Hilman

On Thu, Mar 8, 2012 at 2:45 AM, Turquette, Mike <mturquette@ti.com> wrote:
> On Thu, Feb 9, 2012 at 11:19 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>> The semantics of "target frequency" given to devfreq driver from
>> devfreq framework has always been interpretted as "at least" or GLB
>> (greatest lower bound). However, the framework might want the
>> device driver to limit its max frequency (LUB: least upper bound),
>> especially if it is given by thermal framework (it's too hot).
>>
>> Thus, the target fuction should have another parameter to express
>> whether the framework wants GLB or LUB. And, the additional parameter,
>> "u32 options", does it.
>
> Nitpick: changelog still refers to "options" but code reads "flags".
>
> Regards,
> Mike
>

Thanks. That is updated in the git.infradead.org repository to be
requested to pull.
http://git.infradead.org/users/kmpark/linux-samsung/shortlog/refs/heads/devfreq-for-next

As soon as infradead server gets synched, you can see the commit
message updated.


Cheers!
MyungJoo.

>>
>> With the update, devfreq_recommended_opp() is also updated.
>>
>> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  drivers/devfreq/devfreq.c     |   44 ++++++++++++++++++++++++++++++----------
>>  drivers/devfreq/exynos4_bus.c |   16 +++++++++++---
>>  include/linux/devfreq.h       |   18 ++++++++++++++--
>>  3 files changed, 60 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index a33fc6c..83392a6 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -84,6 +84,7 @@ int update_devfreq(struct devfreq *devfreq)
>>  {
>>        unsigned long freq;
>>        int err = 0;
>> +       u32 options = 0;
>>
>>        if (!mutex_is_locked(&devfreq->lock)) {
>>                WARN(true, "devfreq->lock must be locked by the caller.\n");
>> @@ -104,18 +105,23 @@ int update_devfreq(struct devfreq *devfreq)
>>         * qos_min_freq
>>         */
>>
>> -       if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq)
>> +       if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq) {
>>                freq = devfreq->qos_min_freq;
>> -       if (devfreq->max_freq && freq > devfreq->max_freq)
>> +               options &= ~(1 << 0);
>> +               options |= DEVFREQ_OPTION_FREQ_LUB;
>> +       }
>> +       if (devfreq->max_freq && freq > devfreq->max_freq) {
>>                freq = devfreq->max_freq;
>> -       if (devfreq->min_freq && freq < devfreq->min_freq)
>> +               options &= ~(1 << 0);
>> +               options |= DEVFREQ_OPTION_FREQ_GLB;
>> +       }
>> +       if (devfreq->min_freq && freq < devfreq->min_freq) {
>>                freq = devfreq->min_freq;
>> +               options &= ~(1 << 0);
>> +               options |= DEVFREQ_OPTION_FREQ_LUB;
>> +       }
>>
>> -       /*
>> -        * TODO in the devfreq-next:
>> -        * add relation or use rance (freq_min, freq_max)
>> -        */
>> -       err = devfreq->profile->target(devfreq->dev.parent, &freq);
>> +       err = devfreq->profile->target(devfreq->dev.parent, &freq, options);
>>        if (err)
>>                return err;
>>
>> @@ -771,14 +777,30 @@ module_exit(devfreq_exit);
>>  *                          freq value given to target callback.
>>  * @dev                The devfreq user device. (parent of devfreq)
>>  * @freq       The frequency given to target function
>> + * @floor      false: find LUB first and use GLB if LUB not available.
>> + *             true:  find GLB first and use LUB if GLB not available.
>> + *
>> + * LUB: least upper bound (at least this freq or above, but the least)
>> + * GLB: greatest lower bound (at most this freq or below, but the most)
>>  *
>>  */
>> -struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq)
>> +struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
>> +                                   bool floor)
>>  {
>> -       struct opp *opp = opp_find_freq_ceil(dev, freq);
>> +       struct opp *opp;
>>
>> -       if (opp == ERR_PTR(-ENODEV))
>> +       if (floor) {
>>                opp = opp_find_freq_floor(dev, freq);
>> +
>> +               if (opp == ERR_PTR(-ENODEV))
>> +                       opp = opp_find_freq_ceil(dev, freq);
>> +       } else {
>> +               opp = opp_find_freq_ceil(dev, freq);
>> +
>> +               if (opp == ERR_PTR(-ENODEV))
>> +                       opp = opp_find_freq_floor(dev, freq);
>> +       }
>> +
>>        return opp;
>>  }
>>
>> diff --git a/drivers/devfreq/exynos4_bus.c b/drivers/devfreq/exynos4_bus.c
>> index 590d686..c0a78da 100644
>> --- a/drivers/devfreq/exynos4_bus.c
>> +++ b/drivers/devfreq/exynos4_bus.c
>> @@ -619,13 +619,21 @@ static int exynos4_bus_setvolt(struct busfreq_data *data, struct opp *opp,
>>        return err;
>>  }
>>
>> -static int exynos4_bus_target(struct device *dev, unsigned long *_freq)
>> +static int exynos4_bus_target(struct device *dev, unsigned long *_freq,
>> +                             u32 options)
>>  {
>>        int err = 0;
>> -       struct busfreq_data *data = dev_get_drvdata(dev);
>> -       struct opp *opp = devfreq_recommended_opp(dev, _freq);
>> -       unsigned long old_freq = opp_get_freq(data->curr_opp);
>> +       unsigned long def = *_freq;
>> +       struct platform_device *pdev = container_of(dev, struct platform_device,
>> +                                                   dev);
>> +       struct busfreq_data *data = platform_get_drvdata(pdev);
>> +       struct opp *opp = devfreq_recommended_opp(dev, _freq, options &
>> +                                                 DEVFREQ_OPTION_FREQ_GLB);
>>        unsigned long freq = opp_get_freq(opp);
>> +       unsigned long old_freq = opp_get_freq(data->curr_opp);
>> +
>> +       if (IS_ERR(opp))
>> +               return PTR_ERR(opp);
>>
>>        if (old_freq == freq)
>>                return 0;
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index b853379..1aff012 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -59,6 +59,16 @@ struct devfreq_pm_qos_table {
>>        s32 qos_value;
>>  };
>>
>> +/*
>> + * target callback, which is to provide additional information to the
>> + * devfreq driver.
>> + */
>> +
>> +/* The resulting frequency should be at least this. (least upper bound) */
>> +#define DEVFREQ_OPTION_FREQ_LUB        0x0
>> +/* The resulting frequency should be at most this. (greatest lower bound) */
>> +#define DEVFREQ_OPTION_FREQ_GLB 0x1
>> +
>>  /**
>>  * struct devfreq_dev_profile - Devfreq's user device profile
>>  * @initial_freq       The operating frequency when devfreq_add_device() is
>> @@ -76,6 +86,8 @@ struct devfreq_pm_qos_table {
>>  *                     higher than any operable frequency, set maximum.
>>  *                     Before returning, target function should set
>>  *                     freq at the current frequency.
>> + *                     The "option" parameter's possible values are
>> + *                     explained above with "DEVFREQ_OPTION_*" macros.
>>  * @get_dev_status     The device should provide the current performance
>>  *                     status to devfreq, which is used by governors.
>>  * @exit               An optional callback that is called when devfreq
>> @@ -95,7 +107,7 @@ struct devfreq_dev_profile {
>>        bool qos_use_max;
>>        struct devfreq_pm_qos_table *qos_list;
>>
>> -       int (*target)(struct device *dev, unsigned long *freq);
>> +       int (*target)(struct device *dev, unsigned long *freq, u32 options);
>>        int (*get_dev_status)(struct device *dev,
>>                              struct devfreq_dev_status *stat);
>>        void (*exit)(struct device *dev);
>> @@ -198,7 +210,7 @@ extern int devfreq_remove_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);
>> +                                          unsigned long *freq, bool floor);
>>  extern int devfreq_register_opp_notifier(struct device *dev,
>>                                         struct devfreq *devfreq);
>>  extern int devfreq_unregister_opp_notifier(struct device *dev,
>> @@ -253,7 +265,7 @@ static int devfreq_remove_device(struct devfreq *devfreq)
>>  }
>>
>>  static struct opp *devfreq_recommended_opp(struct device *dev,
>> -                                          unsigned long *freq)
>> +                                          unsigned long *freq, bool floor)
>>  {
>>        return -EINVAL;
>>  }
>> --
>> 1.7.4.1
>>



-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab, DMC Business, Samsung Electronics

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / devfreq: add relation of recommended frequency.
  2012-03-07  7:40   ` MyungJoo Ham
@ 2012-03-10 21:51     ` Rafael J. Wysocki
  2012-03-12  2:12       ` MyungJoo Ham
  2012-03-15  0:25     ` Rafael J. Wysocki
  1 sibling, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2012-03-10 21:51 UTC (permalink / raw)
  To: MyungJoo Ham
  Cc: linux-pm, linux-kernel, Kyungmin Park, Kevin Hilman,
	Mike Turquette, myungjoo.ham

On Wednesday, March 07, 2012, MyungJoo Ham wrote:
> The semantics of "target frequency" given to devfreq driver from
> devfreq framework has always been interpretted as "at least" or GLB
> (greatest lower bound). However, the framework might want the
> device driver to limit its max frequency (LUB: least upper bound),
> especially if it is given by thermal framework (it's too hot).
> 
> Thus, the target fuction should have another parameter to express
> whether the framework wants GLB or LUB. And, the additional parameter,
> "u32 options", does it.
> 
> With the update, devfreq_recommended_opp() is also updated.
> 
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

This one looks good.

Do you want me to take it as a patch, or to pull it from your tree?

Rafael


> ---
> Changes from v1
> - Style update.
> ---
>  drivers/devfreq/devfreq.c     |   41 ++++++++++++++++++++++++++++++-----------
>  drivers/devfreq/exynos4_bus.c |   14 ++++++++++----
>  include/linux/devfreq.h       |   16 +++++++++++++---
>  3 files changed, 53 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 0b49f59..13794d1 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -84,6 +84,7 @@ int update_devfreq(struct devfreq *devfreq)
>  {
>  	unsigned long freq;
>  	int err = 0;
> +	u32 flags = 0;
>  
>  	if (!mutex_is_locked(&devfreq->lock)) {
>  		WARN(true, "devfreq->lock must be locked by the caller.\n");
> @@ -104,18 +105,20 @@ int update_devfreq(struct devfreq *devfreq)
>  	 * qos_min_freq
>  	 */
>  
> -	if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq)
> +	if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq) {
>  		freq = devfreq->qos_min_freq;
> -	if (devfreq->max_freq && freq > devfreq->max_freq)
> +		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
> +	}
> +	if (devfreq->max_freq && freq > devfreq->max_freq) {
>  		freq = devfreq->max_freq;
> -	if (devfreq->min_freq && freq < devfreq->min_freq)
> +		flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
> +	}
> +	if (devfreq->min_freq && freq < devfreq->min_freq) {
>  		freq = devfreq->min_freq;
> +		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
> +	}
>  
> -	/*
> -	 * TODO in the devfreq-next:
> -	 * add relation or use rance (freq_min, freq_max)
> -	 */
> -	err = devfreq->profile->target(devfreq->dev.parent, &freq);
> +	err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
>  	if (err)
>  		return err;
>  
> @@ -773,14 +776,30 @@ module_exit(devfreq_exit);
>   *			     freq value given to target callback.
>   * @dev		The devfreq user device. (parent of devfreq)
>   * @freq	The frequency given to target function
> + * @flags	Flags handed from devfreq framework.
>   *
>   */
> -struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq)
> +struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
> +				    u32 flags)
>  {
> -	struct opp *opp = opp_find_freq_ceil(dev, freq);
> +	struct opp *opp;
>  
> -	if (opp == ERR_PTR(-ENODEV))
> +	if (flags & DEVFREQ_FLAG_LEAST_UPPER_BOUND) {
> +		/* The freq is an upper bound. opp should be lower */
>  		opp = opp_find_freq_floor(dev, freq);
> +
> +		/* If not available, use the closest opp */
> +		if (opp == ERR_PTR(-ENODEV))
> +			opp = opp_find_freq_ceil(dev, freq);
> +	} else {
> +		/* The freq is an lower bound. opp should be higher */
> +		opp = opp_find_freq_ceil(dev, freq);
> +
> +		/* If not available, use the closest opp */
> +		if (opp == ERR_PTR(-ENODEV))
> +			opp = opp_find_freq_floor(dev, freq);
> +	}
> +
>  	return opp;
>  }
>  
> diff --git a/drivers/devfreq/exynos4_bus.c b/drivers/devfreq/exynos4_bus.c
> index 590d686..1a361e9 100644
> --- a/drivers/devfreq/exynos4_bus.c
> +++ b/drivers/devfreq/exynos4_bus.c
> @@ -619,13 +619,19 @@ static int exynos4_bus_setvolt(struct busfreq_data *data, struct opp *opp,
>  	return err;
>  }
>  
> -static int exynos4_bus_target(struct device *dev, unsigned long *_freq)
> +static int exynos4_bus_target(struct device *dev, unsigned long *_freq,
> +			      u32 flags)
>  {
>  	int err = 0;
> -	struct busfreq_data *data = dev_get_drvdata(dev);
> -	struct opp *opp = devfreq_recommended_opp(dev, _freq);
> -	unsigned long old_freq = opp_get_freq(data->curr_opp);
> +	struct platform_device *pdev = container_of(dev, struct platform_device,
> +						    dev);
> +	struct busfreq_data *data = platform_get_drvdata(pdev);
> +	struct opp *opp = devfreq_recommended_opp(dev, _freq, flags);
>  	unsigned long freq = opp_get_freq(opp);
> +	unsigned long old_freq = opp_get_freq(data->curr_opp);
> +
> +	if (IS_ERR(opp))
> +		return PTR_ERR(opp);
>  
>  	if (old_freq == freq)
>  		return 0;
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index f27d511..85d4b0f 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -59,6 +59,14 @@ struct devfreq_pm_qos_table {
>  	s32 qos_value;
>  };
>  
> +/*
> + * The resulting frequency should be at most this. (this bound is the
> + * least upper bound; thus, the resulting freq should be lower or same)
> + * If the flag is not set, the resulting frequency should be at most the
> + * bound (greatest lower bound)
> + */
> +#define DEVFREQ_FLAG_LEAST_UPPER_BOUND		0x1
> +
>  /**
>   * struct devfreq_dev_profile - Devfreq's user device profile
>   * @initial_freq	The operating frequency when devfreq_add_device() is
> @@ -74,6 +82,8 @@ struct devfreq_pm_qos_table {
>   *			higher than any operable frequency, set maximum.
>   *			Before returning, target function should set
>   *			freq at the current frequency.
> + *			The "flags" parameter's possible values are
> + *			explained above with "DEVFREQ_FLAG_*" macros.
>   * @get_dev_status	The device should provide the current performance
>   *			status to devfreq, which is used by governors.
>   * @exit		An optional callback that is called when devfreq
> @@ -92,7 +102,7 @@ struct devfreq_dev_profile {
>  	int qos_type;
>  	struct devfreq_pm_qos_table *qos_list;
>  
> -	int (*target)(struct device *dev, unsigned long *freq);
> +	int (*target)(struct device *dev, unsigned long *freq, u32 flags);
>  	int (*get_dev_status)(struct device *dev,
>  			      struct devfreq_dev_status *stat);
>  	void (*exit)(struct device *dev);
> @@ -198,7 +208,7 @@ extern int devfreq_remove_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);
> +					   unsigned long *freq, u32 flags);
>  extern int devfreq_register_opp_notifier(struct device *dev,
>  					 struct devfreq *devfreq);
>  extern int devfreq_unregister_opp_notifier(struct device *dev,
> @@ -253,7 +263,7 @@ static int devfreq_remove_device(struct devfreq *devfreq)
>  }
>  
>  static struct opp *devfreq_recommended_opp(struct device *dev,
> -					   unsigned long *freq)
> +					   unsigned long *freq, u32 flags)
>  {
>  	return -EINVAL;
>  }
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / devfreq: add relation of recommended frequency.
  2012-03-10 21:51     ` Rafael J. Wysocki
@ 2012-03-12  2:12       ` MyungJoo Ham
  2012-03-12 23:46         ` Rafael J. Wysocki
  0 siblings, 1 reply; 13+ messages in thread
From: MyungJoo Ham @ 2012-03-12  2:12 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-pm, linux-kernel, Kyungmin Park, Kevin Hilman, Mike Turquette

2012/3/11 Rafael J. Wysocki <rjw@sisk.pl>:
> On Wednesday, March 07, 2012, MyungJoo Ham wrote:
>> The semantics of "target frequency" given to devfreq driver from
>> devfreq framework has always been interpretted as "at least" or GLB
>> (greatest lower bound). However, the framework might want the
>> device driver to limit its max frequency (LUB: least upper bound),
>> especially if it is given by thermal framework (it's too hot).
>>
>> Thus, the target fuction should have another parameter to express
>> whether the framework wants GLB or LUB. And, the additional parameter,
>> "u32 options", does it.
>>
>> With the update, devfreq_recommended_opp() is also updated.
>>
>> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>
> This one looks good.
>
> Do you want me to take it as a patch, or to pull it from your tree?
>
> Rafael

Hello Rafael,


Please pull it from the tree
(http://git.infradead.org/users/kmpark/linux-samsung/shortlog/refs/heads/devfreq-for-next)
as the change log has been corrected.

git://git.infradead.org/users/kmpark/linux-samsung devfreq-for-next



Thank you.


Cheers!
MyungJoo.

>
>
>> ---
>> Changes from v1
>> - Style update.
>> ---
>>  drivers/devfreq/devfreq.c     |   41 ++++++++++++++++++++++++++++++-----------
>>  drivers/devfreq/exynos4_bus.c |   14 ++++++++++----
>>  include/linux/devfreq.h       |   16 +++++++++++++---
>>  3 files changed, 53 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 0b49f59..13794d1 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -84,6 +84,7 @@ int update_devfreq(struct devfreq *devfreq)
>>  {
>>       unsigned long freq;
>>       int err = 0;
>> +     u32 flags = 0;
>>
>>       if (!mutex_is_locked(&devfreq->lock)) {
>>               WARN(true, "devfreq->lock must be locked by the caller.\n");
>> @@ -104,18 +105,20 @@ int update_devfreq(struct devfreq *devfreq)
>>        * qos_min_freq
>>        */
>>
>> -     if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq)
>> +     if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq) {
>>               freq = devfreq->qos_min_freq;
>> -     if (devfreq->max_freq && freq > devfreq->max_freq)
>> +             flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
>> +     }
>> +     if (devfreq->max_freq && freq > devfreq->max_freq) {
>>               freq = devfreq->max_freq;
>> -     if (devfreq->min_freq && freq < devfreq->min_freq)
>> +             flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
>> +     }
>> +     if (devfreq->min_freq && freq < devfreq->min_freq) {
>>               freq = devfreq->min_freq;
>> +             flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
>> +     }
>>
>> -     /*
>> -      * TODO in the devfreq-next:
>> -      * add relation or use rance (freq_min, freq_max)
>> -      */
>> -     err = devfreq->profile->target(devfreq->dev.parent, &freq);
>> +     err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
>>       if (err)
>>               return err;
>>
>> @@ -773,14 +776,30 @@ module_exit(devfreq_exit);
>>   *                        freq value given to target callback.
>>   * @dev              The devfreq user device. (parent of devfreq)
>>   * @freq     The frequency given to target function
>> + * @flags    Flags handed from devfreq framework.
>>   *
>>   */
>> -struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq)
>> +struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
>> +                                 u32 flags)
>>  {
>> -     struct opp *opp = opp_find_freq_ceil(dev, freq);
>> +     struct opp *opp;
>>
>> -     if (opp == ERR_PTR(-ENODEV))
>> +     if (flags & DEVFREQ_FLAG_LEAST_UPPER_BOUND) {
>> +             /* The freq is an upper bound. opp should be lower */
>>               opp = opp_find_freq_floor(dev, freq);
>> +
>> +             /* If not available, use the closest opp */
>> +             if (opp == ERR_PTR(-ENODEV))
>> +                     opp = opp_find_freq_ceil(dev, freq);
>> +     } else {
>> +             /* The freq is an lower bound. opp should be higher */
>> +             opp = opp_find_freq_ceil(dev, freq);
>> +
>> +             /* If not available, use the closest opp */
>> +             if (opp == ERR_PTR(-ENODEV))
>> +                     opp = opp_find_freq_floor(dev, freq);
>> +     }
>> +
>>       return opp;
>>  }
>>
>> diff --git a/drivers/devfreq/exynos4_bus.c b/drivers/devfreq/exynos4_bus.c
>> index 590d686..1a361e9 100644
>> --- a/drivers/devfreq/exynos4_bus.c
>> +++ b/drivers/devfreq/exynos4_bus.c
>> @@ -619,13 +619,19 @@ static int exynos4_bus_setvolt(struct busfreq_data *data, struct opp *opp,
>>       return err;
>>  }
>>
>> -static int exynos4_bus_target(struct device *dev, unsigned long *_freq)
>> +static int exynos4_bus_target(struct device *dev, unsigned long *_freq,
>> +                           u32 flags)
>>  {
>>       int err = 0;
>> -     struct busfreq_data *data = dev_get_drvdata(dev);
>> -     struct opp *opp = devfreq_recommended_opp(dev, _freq);
>> -     unsigned long old_freq = opp_get_freq(data->curr_opp);
>> +     struct platform_device *pdev = container_of(dev, struct platform_device,
>> +                                                 dev);
>> +     struct busfreq_data *data = platform_get_drvdata(pdev);
>> +     struct opp *opp = devfreq_recommended_opp(dev, _freq, flags);
>>       unsigned long freq = opp_get_freq(opp);
>> +     unsigned long old_freq = opp_get_freq(data->curr_opp);
>> +
>> +     if (IS_ERR(opp))
>> +             return PTR_ERR(opp);
>>
>>       if (old_freq == freq)
>>               return 0;
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index f27d511..85d4b0f 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -59,6 +59,14 @@ struct devfreq_pm_qos_table {
>>       s32 qos_value;
>>  };
>>
>> +/*
>> + * The resulting frequency should be at most this. (this bound is the
>> + * least upper bound; thus, the resulting freq should be lower or same)
>> + * If the flag is not set, the resulting frequency should be at most the
>> + * bound (greatest lower bound)
>> + */
>> +#define DEVFREQ_FLAG_LEAST_UPPER_BOUND               0x1
>> +
>>  /**
>>   * struct devfreq_dev_profile - Devfreq's user device profile
>>   * @initial_freq     The operating frequency when devfreq_add_device() is
>> @@ -74,6 +82,8 @@ struct devfreq_pm_qos_table {
>>   *                   higher than any operable frequency, set maximum.
>>   *                   Before returning, target function should set
>>   *                   freq at the current frequency.
>> + *                   The "flags" parameter's possible values are
>> + *                   explained above with "DEVFREQ_FLAG_*" macros.
>>   * @get_dev_status   The device should provide the current performance
>>   *                   status to devfreq, which is used by governors.
>>   * @exit             An optional callback that is called when devfreq
>> @@ -92,7 +102,7 @@ struct devfreq_dev_profile {
>>       int qos_type;
>>       struct devfreq_pm_qos_table *qos_list;
>>
>> -     int (*target)(struct device *dev, unsigned long *freq);
>> +     int (*target)(struct device *dev, unsigned long *freq, u32 flags);
>>       int (*get_dev_status)(struct device *dev,
>>                             struct devfreq_dev_status *stat);
>>       void (*exit)(struct device *dev);
>> @@ -198,7 +208,7 @@ extern int devfreq_remove_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);
>> +                                        unsigned long *freq, u32 flags);
>>  extern int devfreq_register_opp_notifier(struct device *dev,
>>                                        struct devfreq *devfreq);
>>  extern int devfreq_unregister_opp_notifier(struct device *dev,
>> @@ -253,7 +263,7 @@ static int devfreq_remove_device(struct devfreq *devfreq)
>>  }
>>
>>  static struct opp *devfreq_recommended_opp(struct device *dev,
>> -                                        unsigned long *freq)
>> +                                        unsigned long *freq, u32 flags)
>>  {
>>       return -EINVAL;
>>  }
>>
>



-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab, DMC Business, Samsung Electronics

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / devfreq: add relation of recommended frequency.
  2012-03-12  2:12       ` MyungJoo Ham
@ 2012-03-12 23:46         ` Rafael J. Wysocki
  0 siblings, 0 replies; 13+ messages in thread
From: Rafael J. Wysocki @ 2012-03-12 23:46 UTC (permalink / raw)
  To: myungjoo.ham
  Cc: linux-pm, linux-kernel, Kyungmin Park, Kevin Hilman, Mike Turquette

On Monday, March 12, 2012, MyungJoo Ham wrote:
> 2012/3/11 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Wednesday, March 07, 2012, MyungJoo Ham wrote:
> >> The semantics of "target frequency" given to devfreq driver from
> >> devfreq framework has always been interpretted as "at least" or GLB
> >> (greatest lower bound). However, the framework might want the
> >> device driver to limit its max frequency (LUB: least upper bound),
> >> especially if it is given by thermal framework (it's too hot).
> >>
> >> Thus, the target fuction should have another parameter to express
> >> whether the framework wants GLB or LUB. And, the additional parameter,
> >> "u32 options", does it.
> >>
> >> With the update, devfreq_recommended_opp() is also updated.
> >>
> >> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >
> > This one looks good.
> >
> > Do you want me to take it as a patch, or to pull it from your tree?
> >
> > Rafael
> 
> Hello Rafael,
> 
> 
> Please pull it from the tree
> (http://git.infradead.org/users/kmpark/linux-samsung/shortlog/refs/heads/devfreq-for-next)
> as the change log has been corrected.
> 
> git://git.infradead.org/users/kmpark/linux-samsung devfreq-for-next

I'm not happy with the "PM / devfreq: add PM QoS support" commit, however.

I believe I explained the reason why in a separate message sent a couple of
days ago.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PM / devfreq: add relation of recommended frequency.
  2012-03-07  7:40   ` MyungJoo Ham
  2012-03-10 21:51     ` Rafael J. Wysocki
@ 2012-03-15  0:25     ` Rafael J. Wysocki
  2012-03-16  6:02       ` [PATCH v3] " MyungJoo Ham
  1 sibling, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2012-03-15  0:25 UTC (permalink / raw)
  To: MyungJoo Ham
  Cc: linux-pm, linux-kernel, Kyungmin Park, Kevin Hilman,
	Mike Turquette, myungjoo.ham

On Wednesday, March 07, 2012, MyungJoo Ham wrote:
> The semantics of "target frequency" given to devfreq driver from
> devfreq framework has always been interpretted as "at least" or GLB
> (greatest lower bound). However, the framework might want the
> device driver to limit its max frequency (LUB: least upper bound),
> especially if it is given by thermal framework (it's too hot).
> 
> Thus, the target fuction should have another parameter to express
> whether the framework wants GLB or LUB. And, the additional parameter,
> "u32 options", does it.
> 
> With the update, devfreq_recommended_opp() is also updated.
> 
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Can you please prepare a version of this patch that applies cleanly on top
of the pm-qos branch of the linux-pm tree?

I'd like to take it for v3.4 if possible.

Thanks,
Rafael


> ---
> Changes from v1
> - Style update.
> ---
>  drivers/devfreq/devfreq.c     |   41 ++++++++++++++++++++++++++++++-----------
>  drivers/devfreq/exynos4_bus.c |   14 ++++++++++----
>  include/linux/devfreq.h       |   16 +++++++++++++---
>  3 files changed, 53 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 0b49f59..13794d1 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -84,6 +84,7 @@ int update_devfreq(struct devfreq *devfreq)
>  {
>  	unsigned long freq;
>  	int err = 0;
> +	u32 flags = 0;
>  
>  	if (!mutex_is_locked(&devfreq->lock)) {
>  		WARN(true, "devfreq->lock must be locked by the caller.\n");
> @@ -104,18 +105,20 @@ int update_devfreq(struct devfreq *devfreq)
>  	 * qos_min_freq
>  	 */
>  
> -	if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq)
> +	if (devfreq->qos_min_freq && freq < devfreq->qos_min_freq) {
>  		freq = devfreq->qos_min_freq;
> -	if (devfreq->max_freq && freq > devfreq->max_freq)
> +		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
> +	}
> +	if (devfreq->max_freq && freq > devfreq->max_freq) {
>  		freq = devfreq->max_freq;
> -	if (devfreq->min_freq && freq < devfreq->min_freq)
> +		flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
> +	}
> +	if (devfreq->min_freq && freq < devfreq->min_freq) {
>  		freq = devfreq->min_freq;
> +		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
> +	}
>  
> -	/*
> -	 * TODO in the devfreq-next:
> -	 * add relation or use rance (freq_min, freq_max)
> -	 */
> -	err = devfreq->profile->target(devfreq->dev.parent, &freq);
> +	err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
>  	if (err)
>  		return err;
>  
> @@ -773,14 +776,30 @@ module_exit(devfreq_exit);
>   *			     freq value given to target callback.
>   * @dev		The devfreq user device. (parent of devfreq)
>   * @freq	The frequency given to target function
> + * @flags	Flags handed from devfreq framework.
>   *
>   */
> -struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq)
> +struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
> +				    u32 flags)
>  {
> -	struct opp *opp = opp_find_freq_ceil(dev, freq);
> +	struct opp *opp;
>  
> -	if (opp == ERR_PTR(-ENODEV))
> +	if (flags & DEVFREQ_FLAG_LEAST_UPPER_BOUND) {
> +		/* The freq is an upper bound. opp should be lower */
>  		opp = opp_find_freq_floor(dev, freq);
> +
> +		/* If not available, use the closest opp */
> +		if (opp == ERR_PTR(-ENODEV))
> +			opp = opp_find_freq_ceil(dev, freq);
> +	} else {
> +		/* The freq is an lower bound. opp should be higher */
> +		opp = opp_find_freq_ceil(dev, freq);
> +
> +		/* If not available, use the closest opp */
> +		if (opp == ERR_PTR(-ENODEV))
> +			opp = opp_find_freq_floor(dev, freq);
> +	}
> +
>  	return opp;
>  }
>  
> diff --git a/drivers/devfreq/exynos4_bus.c b/drivers/devfreq/exynos4_bus.c
> index 590d686..1a361e9 100644
> --- a/drivers/devfreq/exynos4_bus.c
> +++ b/drivers/devfreq/exynos4_bus.c
> @@ -619,13 +619,19 @@ static int exynos4_bus_setvolt(struct busfreq_data *data, struct opp *opp,
>  	return err;
>  }
>  
> -static int exynos4_bus_target(struct device *dev, unsigned long *_freq)
> +static int exynos4_bus_target(struct device *dev, unsigned long *_freq,
> +			      u32 flags)
>  {
>  	int err = 0;
> -	struct busfreq_data *data = dev_get_drvdata(dev);
> -	struct opp *opp = devfreq_recommended_opp(dev, _freq);
> -	unsigned long old_freq = opp_get_freq(data->curr_opp);
> +	struct platform_device *pdev = container_of(dev, struct platform_device,
> +						    dev);
> +	struct busfreq_data *data = platform_get_drvdata(pdev);
> +	struct opp *opp = devfreq_recommended_opp(dev, _freq, flags);
>  	unsigned long freq = opp_get_freq(opp);
> +	unsigned long old_freq = opp_get_freq(data->curr_opp);
> +
> +	if (IS_ERR(opp))
> +		return PTR_ERR(opp);
>  
>  	if (old_freq == freq)
>  		return 0;
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index f27d511..85d4b0f 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -59,6 +59,14 @@ struct devfreq_pm_qos_table {
>  	s32 qos_value;
>  };
>  
> +/*
> + * The resulting frequency should be at most this. (this bound is the
> + * least upper bound; thus, the resulting freq should be lower or same)
> + * If the flag is not set, the resulting frequency should be at most the
> + * bound (greatest lower bound)
> + */
> +#define DEVFREQ_FLAG_LEAST_UPPER_BOUND		0x1
> +
>  /**
>   * struct devfreq_dev_profile - Devfreq's user device profile
>   * @initial_freq	The operating frequency when devfreq_add_device() is
> @@ -74,6 +82,8 @@ struct devfreq_pm_qos_table {
>   *			higher than any operable frequency, set maximum.
>   *			Before returning, target function should set
>   *			freq at the current frequency.
> + *			The "flags" parameter's possible values are
> + *			explained above with "DEVFREQ_FLAG_*" macros.
>   * @get_dev_status	The device should provide the current performance
>   *			status to devfreq, which is used by governors.
>   * @exit		An optional callback that is called when devfreq
> @@ -92,7 +102,7 @@ struct devfreq_dev_profile {
>  	int qos_type;
>  	struct devfreq_pm_qos_table *qos_list;
>  
> -	int (*target)(struct device *dev, unsigned long *freq);
> +	int (*target)(struct device *dev, unsigned long *freq, u32 flags);
>  	int (*get_dev_status)(struct device *dev,
>  			      struct devfreq_dev_status *stat);
>  	void (*exit)(struct device *dev);
> @@ -198,7 +208,7 @@ extern int devfreq_remove_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);
> +					   unsigned long *freq, u32 flags);
>  extern int devfreq_register_opp_notifier(struct device *dev,
>  					 struct devfreq *devfreq);
>  extern int devfreq_unregister_opp_notifier(struct device *dev,
> @@ -253,7 +263,7 @@ static int devfreq_remove_device(struct devfreq *devfreq)
>  }
>  
>  static struct opp *devfreq_recommended_opp(struct device *dev,
> -					   unsigned long *freq)
> +					   unsigned long *freq, u32 flags)
>  {
>  	return -EINVAL;
>  }
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3] PM / devfreq: add relation of recommended frequency.
  2012-03-15  0:25     ` Rafael J. Wysocki
@ 2012-03-16  6:02       ` MyungJoo Ham
  2012-03-16  6:14         ` Turquette, Mike
  0 siblings, 1 reply; 13+ messages in thread
From: MyungJoo Ham @ 2012-03-16  6:02 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-pm, linux-kernel, Kyungmin Park, Kevin Hilman,
	Mike Turquette, myungjoo.ham

The semantics of "target frequency" given to devfreq driver from
devfreq framework has always been interpretted as "at least" or GLB
(greatest lower bound). However, the framework might want the
device driver to limit its max frequency (LUB: least upper bound),
especially if it is given by thermal framework (it's too hot).

Thus, the target fuction should have another parameter to express
whether the framework wants GLB or LUB. And, the additional parameter,
"u32 flags", does it.

With the update, devfreq_recommended_opp() is also updated.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes from v2
- Updated comments
- Rebased.

Changes from v1
- Style update.

 drivers/devfreq/devfreq.c     |   42 +++++++++++++++++++++++++++++++++++++---
 drivers/devfreq/exynos4_bus.c |   14 +++++++++---
 include/linux/devfreq.h       |   16 ++++++++++++--
 3 files changed, 61 insertions(+), 11 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index a129a7b..70c31d4 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -83,6 +83,7 @@ int update_devfreq(struct devfreq *devfreq)
 {
 	unsigned long freq;
 	int err = 0;
+	u32 flags = 0;
 
 	if (!mutex_is_locked(&devfreq->lock)) {
 		WARN(true, "devfreq->lock must be locked by the caller.\n");
@@ -94,7 +95,24 @@ int update_devfreq(struct devfreq *devfreq)
 	if (err)
 		return err;
 
-	err = devfreq->profile->target(devfreq->dev.parent, &freq);
+	/*
+	 * Adjust the freuqency with user freq and QoS.
+	 *
+	 * List from the highest proiority
+	 * max_freq (probably called by thermal when it's too hot)
+	 * min_freq
+	 */
+
+	if (devfreq->min_freq && freq < devfreq->min_freq) {
+		freq = devfreq->min_freq;
+		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
+	}
+	if (devfreq->max_freq && freq > devfreq->max_freq) {
+		freq = devfreq->max_freq;
+		flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
+	}
+
+	err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
 	if (err)
 		return err;
 
@@ -625,14 +643,30 @@ module_exit(devfreq_exit);
  *			     freq value given to target callback.
  * @dev		The devfreq user device. (parent of devfreq)
  * @freq	The frequency given to target function
+ * @flags	Flags handed from devfreq framework.
  *
  */
-struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq)
+struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
+				    u32 flags)
 {
-	struct opp *opp = opp_find_freq_ceil(dev, freq);
+	struct opp *opp;
 
-	if (opp == ERR_PTR(-ENODEV))
+	if (flags & DEVFREQ_FLAG_LEAST_UPPER_BOUND) {
+		/* The freq is an upper bound. opp should be lower */
 		opp = opp_find_freq_floor(dev, freq);
+
+		/* If not available, use the closest opp */
+		if (opp == ERR_PTR(-ENODEV))
+			opp = opp_find_freq_ceil(dev, freq);
+	} else {
+		/* The freq is an lower bound. opp should be higher */
+		opp = opp_find_freq_ceil(dev, freq);
+
+		/* If not available, use the closest opp */
+		if (opp == ERR_PTR(-ENODEV))
+			opp = opp_find_freq_floor(dev, freq);
+	}
+
 	return opp;
 }
 
diff --git a/drivers/devfreq/exynos4_bus.c b/drivers/devfreq/exynos4_bus.c
index 590d686..1a361e9 100644
--- a/drivers/devfreq/exynos4_bus.c
+++ b/drivers/devfreq/exynos4_bus.c
@@ -619,13 +619,19 @@ static int exynos4_bus_setvolt(struct busfreq_data *data, struct opp *opp,
 	return err;
 }
 
-static int exynos4_bus_target(struct device *dev, unsigned long *_freq)
+static int exynos4_bus_target(struct device *dev, unsigned long *_freq,
+			      u32 flags)
 {
 	int err = 0;
-	struct busfreq_data *data = dev_get_drvdata(dev);
-	struct opp *opp = devfreq_recommended_opp(dev, _freq);
-	unsigned long old_freq = opp_get_freq(data->curr_opp);
+	struct platform_device *pdev = container_of(dev, struct platform_device,
+						    dev);
+	struct busfreq_data *data = platform_get_drvdata(pdev);
+	struct opp *opp = devfreq_recommended_opp(dev, _freq, flags);
 	unsigned long freq = opp_get_freq(opp);
+	unsigned long old_freq = opp_get_freq(data->curr_opp);
+
+	if (IS_ERR(opp))
+		return PTR_ERR(opp);
 
 	if (old_freq == freq)
 		return 0;
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 5862475..281c72a 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -44,6 +44,14 @@ struct devfreq_dev_status {
 	void *private_data;
 };
 
+/*
+ * The resulting frequency should be at most this. (this bound is the
+ * least upper bound; thus, the resulting freq should be lower or same)
+ * If the flag is not set, the resulting frequency should be at most the
+ * bound (greatest lower bound)
+ */
+#define DEVFREQ_FLAG_LEAST_UPPER_BOUND		0x1
+
 /**
  * struct devfreq_dev_profile - Devfreq's user device profile
  * @initial_freq	The operating frequency when devfreq_add_device() is
@@ -54,6 +62,8 @@ struct devfreq_dev_status {
  *			higher than any operable frequency, set maximum.
  *			Before returning, target function should set
  *			freq at the current frequency.
+ *			The "flags" parameter's possible values are
+ *			explained above with "DEVFREQ_FLAG_*" macros.
  * @get_dev_status	The device should provide the current performance
  *			status to devfreq, which is used by governors.
  * @exit		An optional callback that is called when devfreq
@@ -66,7 +76,7 @@ struct devfreq_dev_profile {
 	unsigned long initial_freq;
 	unsigned int polling_ms;
 
-	int (*target)(struct device *dev, unsigned long *freq);
+	int (*target)(struct device *dev, unsigned long *freq, u32 flags);
 	int (*get_dev_status)(struct device *dev,
 			      struct devfreq_dev_status *stat);
 	void (*exit)(struct device *dev);
@@ -165,7 +175,7 @@ extern int devfreq_remove_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);
+					   unsigned long *freq, u32 flags);
 extern int devfreq_register_opp_notifier(struct device *dev,
 					 struct devfreq *devfreq);
 extern int devfreq_unregister_opp_notifier(struct device *dev,
@@ -216,7 +226,7 @@ static int devfreq_remove_device(struct devfreq *devfreq)
 }
 
 static struct opp *devfreq_recommended_opp(struct device *dev,
-					   unsigned long *freq)
+					   unsigned long *freq, u32 flags)
 {
 	return -EINVAL;
 }
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v3] PM / devfreq: add relation of recommended frequency.
  2012-03-16  6:02       ` [PATCH v3] " MyungJoo Ham
@ 2012-03-16  6:14         ` Turquette, Mike
  2012-03-16 21:07           ` Rafael J. Wysocki
  0 siblings, 1 reply; 13+ messages in thread
From: Turquette, Mike @ 2012-03-16  6:14 UTC (permalink / raw)
  To: MyungJoo Ham
  Cc: Rafael J. Wysocki, linux-pm, linux-kernel, Kyungmin Park,
	Kevin Hilman, myungjoo.ham

On Thu, Mar 15, 2012 at 11:02 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> The semantics of "target frequency" given to devfreq driver from
> devfreq framework has always been interpretted as "at least" or GLB
> (greatest lower bound). However, the framework might want the
> device driver to limit its max frequency (LUB: least upper bound),
> especially if it is given by thermal framework (it's too hot).
>
> Thus, the target fuction should have another parameter to express
> whether the framework wants GLB or LUB. And, the additional parameter,
> "u32 flags", does it.
>
> With the update, devfreq_recommended_opp() is also updated.
>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>

I know Rafael already said he'd take it but I might as well add:

Reviewed-by: Mike Turquette <mturquette@ti.com>

> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> Changes from v2
> - Updated comments
> - Rebased.
>
> Changes from v1
> - Style update.
>
>  drivers/devfreq/devfreq.c     |   42 +++++++++++++++++++++++++++++++++++++---
>  drivers/devfreq/exynos4_bus.c |   14 +++++++++---
>  include/linux/devfreq.h       |   16 ++++++++++++--
>  3 files changed, 61 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index a129a7b..70c31d4 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -83,6 +83,7 @@ int update_devfreq(struct devfreq *devfreq)
>  {
>        unsigned long freq;
>        int err = 0;
> +       u32 flags = 0;
>
>        if (!mutex_is_locked(&devfreq->lock)) {
>                WARN(true, "devfreq->lock must be locked by the caller.\n");
> @@ -94,7 +95,24 @@ int update_devfreq(struct devfreq *devfreq)
>        if (err)
>                return err;
>
> -       err = devfreq->profile->target(devfreq->dev.parent, &freq);
> +       /*
> +        * Adjust the freuqency with user freq and QoS.
> +        *
> +        * List from the highest proiority
> +        * max_freq (probably called by thermal when it's too hot)
> +        * min_freq
> +        */
> +
> +       if (devfreq->min_freq && freq < devfreq->min_freq) {
> +               freq = devfreq->min_freq;
> +               flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
> +       }
> +       if (devfreq->max_freq && freq > devfreq->max_freq) {
> +               freq = devfreq->max_freq;
> +               flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
> +       }
> +
> +       err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
>        if (err)
>                return err;
>
> @@ -625,14 +643,30 @@ module_exit(devfreq_exit);
>  *                          freq value given to target callback.
>  * @dev                The devfreq user device. (parent of devfreq)
>  * @freq       The frequency given to target function
> + * @flags      Flags handed from devfreq framework.
>  *
>  */
> -struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq)
> +struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
> +                                   u32 flags)
>  {
> -       struct opp *opp = opp_find_freq_ceil(dev, freq);
> +       struct opp *opp;
>
> -       if (opp == ERR_PTR(-ENODEV))
> +       if (flags & DEVFREQ_FLAG_LEAST_UPPER_BOUND) {
> +               /* The freq is an upper bound. opp should be lower */
>                opp = opp_find_freq_floor(dev, freq);
> +
> +               /* If not available, use the closest opp */
> +               if (opp == ERR_PTR(-ENODEV))
> +                       opp = opp_find_freq_ceil(dev, freq);
> +       } else {
> +               /* The freq is an lower bound. opp should be higher */
> +               opp = opp_find_freq_ceil(dev, freq);
> +
> +               /* If not available, use the closest opp */
> +               if (opp == ERR_PTR(-ENODEV))
> +                       opp = opp_find_freq_floor(dev, freq);
> +       }
> +
>        return opp;
>  }
>
> diff --git a/drivers/devfreq/exynos4_bus.c b/drivers/devfreq/exynos4_bus.c
> index 590d686..1a361e9 100644
> --- a/drivers/devfreq/exynos4_bus.c
> +++ b/drivers/devfreq/exynos4_bus.c
> @@ -619,13 +619,19 @@ static int exynos4_bus_setvolt(struct busfreq_data *data, struct opp *opp,
>        return err;
>  }
>
> -static int exynos4_bus_target(struct device *dev, unsigned long *_freq)
> +static int exynos4_bus_target(struct device *dev, unsigned long *_freq,
> +                             u32 flags)
>  {
>        int err = 0;
> -       struct busfreq_data *data = dev_get_drvdata(dev);
> -       struct opp *opp = devfreq_recommended_opp(dev, _freq);
> -       unsigned long old_freq = opp_get_freq(data->curr_opp);
> +       struct platform_device *pdev = container_of(dev, struct platform_device,
> +                                                   dev);
> +       struct busfreq_data *data = platform_get_drvdata(pdev);
> +       struct opp *opp = devfreq_recommended_opp(dev, _freq, flags);
>        unsigned long freq = opp_get_freq(opp);
> +       unsigned long old_freq = opp_get_freq(data->curr_opp);
> +
> +       if (IS_ERR(opp))
> +               return PTR_ERR(opp);
>
>        if (old_freq == freq)
>                return 0;
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 5862475..281c72a 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -44,6 +44,14 @@ struct devfreq_dev_status {
>        void *private_data;
>  };
>
> +/*
> + * The resulting frequency should be at most this. (this bound is the
> + * least upper bound; thus, the resulting freq should be lower or same)
> + * If the flag is not set, the resulting frequency should be at most the
> + * bound (greatest lower bound)
> + */
> +#define DEVFREQ_FLAG_LEAST_UPPER_BOUND         0x1
> +
>  /**
>  * struct devfreq_dev_profile - Devfreq's user device profile
>  * @initial_freq       The operating frequency when devfreq_add_device() is
> @@ -54,6 +62,8 @@ struct devfreq_dev_status {
>  *                     higher than any operable frequency, set maximum.
>  *                     Before returning, target function should set
>  *                     freq at the current frequency.
> + *                     The "flags" parameter's possible values are
> + *                     explained above with "DEVFREQ_FLAG_*" macros.
>  * @get_dev_status     The device should provide the current performance
>  *                     status to devfreq, which is used by governors.
>  * @exit               An optional callback that is called when devfreq
> @@ -66,7 +76,7 @@ struct devfreq_dev_profile {
>        unsigned long initial_freq;
>        unsigned int polling_ms;
>
> -       int (*target)(struct device *dev, unsigned long *freq);
> +       int (*target)(struct device *dev, unsigned long *freq, u32 flags);
>        int (*get_dev_status)(struct device *dev,
>                              struct devfreq_dev_status *stat);
>        void (*exit)(struct device *dev);
> @@ -165,7 +175,7 @@ extern int devfreq_remove_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);
> +                                          unsigned long *freq, u32 flags);
>  extern int devfreq_register_opp_notifier(struct device *dev,
>                                         struct devfreq *devfreq);
>  extern int devfreq_unregister_opp_notifier(struct device *dev,
> @@ -216,7 +226,7 @@ static int devfreq_remove_device(struct devfreq *devfreq)
>  }
>
>  static struct opp *devfreq_recommended_opp(struct device *dev,
> -                                          unsigned long *freq)
> +                                          unsigned long *freq, u32 flags)
>  {
>        return -EINVAL;
>  }
> --
> 1.7.4.1
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3] PM / devfreq: add relation of recommended frequency.
  2012-03-16  6:14         ` Turquette, Mike
@ 2012-03-16 21:07           ` Rafael J. Wysocki
  0 siblings, 0 replies; 13+ messages in thread
From: Rafael J. Wysocki @ 2012-03-16 21:07 UTC (permalink / raw)
  To: Turquette, Mike
  Cc: MyungJoo Ham, linux-pm, linux-kernel, Kyungmin Park,
	Kevin Hilman, myungjoo.ham

On Friday, March 16, 2012, Turquette, Mike wrote:
> On Thu, Mar 15, 2012 at 11:02 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> > The semantics of "target frequency" given to devfreq driver from
> > devfreq framework has always been interpretted as "at least" or GLB
> > (greatest lower bound). However, the framework might want the
> > device driver to limit its max frequency (LUB: least upper bound),
> > especially if it is given by thermal framework (it's too hot).
> >
> > Thus, the target fuction should have another parameter to express
> > whether the framework wants GLB or LUB. And, the additional parameter,
> > "u32 flags", does it.
> >
> > With the update, devfreq_recommended_opp() is also updated.
> >
> > Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> 
> I know Rafael already said he'd take it but I might as well add:
> 
> Reviewed-by: Mike Turquette <mturquette@ti.com>
> 
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Applied to linux-pm/linux-next.

Thanks,
Rafael


> > ---
> > Changes from v2
> > - Updated comments
> > - Rebased.
> >
> > Changes from v1
> > - Style update.
> >
> >  drivers/devfreq/devfreq.c     |   42 +++++++++++++++++++++++++++++++++++++---
> >  drivers/devfreq/exynos4_bus.c |   14 +++++++++---
> >  include/linux/devfreq.h       |   16 ++++++++++++--
> >  3 files changed, 61 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index a129a7b..70c31d4 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -83,6 +83,7 @@ int update_devfreq(struct devfreq *devfreq)
> >  {
> >        unsigned long freq;
> >        int err = 0;
> > +       u32 flags = 0;
> >
> >        if (!mutex_is_locked(&devfreq->lock)) {
> >                WARN(true, "devfreq->lock must be locked by the caller.\n");
> > @@ -94,7 +95,24 @@ int update_devfreq(struct devfreq *devfreq)
> >        if (err)
> >                return err;
> >
> > -       err = devfreq->profile->target(devfreq->dev.parent, &freq);
> > +       /*
> > +        * Adjust the freuqency with user freq and QoS.
> > +        *
> > +        * List from the highest proiority
> > +        * max_freq (probably called by thermal when it's too hot)
> > +        * min_freq
> > +        */
> > +
> > +       if (devfreq->min_freq && freq < devfreq->min_freq) {
> > +               freq = devfreq->min_freq;
> > +               flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
> > +       }
> > +       if (devfreq->max_freq && freq > devfreq->max_freq) {
> > +               freq = devfreq->max_freq;
> > +               flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
> > +       }
> > +
> > +       err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
> >        if (err)
> >                return err;
> >
> > @@ -625,14 +643,30 @@ module_exit(devfreq_exit);
> >  *                          freq value given to target callback.
> >  * @dev                The devfreq user device. (parent of devfreq)
> >  * @freq       The frequency given to target function
> > + * @flags      Flags handed from devfreq framework.
> >  *
> >  */
> > -struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq)
> > +struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
> > +                                   u32 flags)
> >  {
> > -       struct opp *opp = opp_find_freq_ceil(dev, freq);
> > +       struct opp *opp;
> >
> > -       if (opp == ERR_PTR(-ENODEV))
> > +       if (flags & DEVFREQ_FLAG_LEAST_UPPER_BOUND) {
> > +               /* The freq is an upper bound. opp should be lower */
> >                opp = opp_find_freq_floor(dev, freq);
> > +
> > +               /* If not available, use the closest opp */
> > +               if (opp == ERR_PTR(-ENODEV))
> > +                       opp = opp_find_freq_ceil(dev, freq);
> > +       } else {
> > +               /* The freq is an lower bound. opp should be higher */
> > +               opp = opp_find_freq_ceil(dev, freq);
> > +
> > +               /* If not available, use the closest opp */
> > +               if (opp == ERR_PTR(-ENODEV))
> > +                       opp = opp_find_freq_floor(dev, freq);
> > +       }
> > +
> >        return opp;
> >  }
> >
> > diff --git a/drivers/devfreq/exynos4_bus.c b/drivers/devfreq/exynos4_bus.c
> > index 590d686..1a361e9 100644
> > --- a/drivers/devfreq/exynos4_bus.c
> > +++ b/drivers/devfreq/exynos4_bus.c
> > @@ -619,13 +619,19 @@ static int exynos4_bus_setvolt(struct busfreq_data *data, struct opp *opp,
> >        return err;
> >  }
> >
> > -static int exynos4_bus_target(struct device *dev, unsigned long *_freq)
> > +static int exynos4_bus_target(struct device *dev, unsigned long *_freq,
> > +                             u32 flags)
> >  {
> >        int err = 0;
> > -       struct busfreq_data *data = dev_get_drvdata(dev);
> > -       struct opp *opp = devfreq_recommended_opp(dev, _freq);
> > -       unsigned long old_freq = opp_get_freq(data->curr_opp);
> > +       struct platform_device *pdev = container_of(dev, struct platform_device,
> > +                                                   dev);
> > +       struct busfreq_data *data = platform_get_drvdata(pdev);
> > +       struct opp *opp = devfreq_recommended_opp(dev, _freq, flags);
> >        unsigned long freq = opp_get_freq(opp);
> > +       unsigned long old_freq = opp_get_freq(data->curr_opp);
> > +
> > +       if (IS_ERR(opp))
> > +               return PTR_ERR(opp);
> >
> >        if (old_freq == freq)
> >                return 0;
> > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> > index 5862475..281c72a 100644
> > --- a/include/linux/devfreq.h
> > +++ b/include/linux/devfreq.h
> > @@ -44,6 +44,14 @@ struct devfreq_dev_status {
> >        void *private_data;
> >  };
> >
> > +/*
> > + * The resulting frequency should be at most this. (this bound is the
> > + * least upper bound; thus, the resulting freq should be lower or same)
> > + * If the flag is not set, the resulting frequency should be at most the
> > + * bound (greatest lower bound)
> > + */
> > +#define DEVFREQ_FLAG_LEAST_UPPER_BOUND         0x1
> > +
> >  /**
> >  * struct devfreq_dev_profile - Devfreq's user device profile
> >  * @initial_freq       The operating frequency when devfreq_add_device() is
> > @@ -54,6 +62,8 @@ struct devfreq_dev_status {
> >  *                     higher than any operable frequency, set maximum.
> >  *                     Before returning, target function should set
> >  *                     freq at the current frequency.
> > + *                     The "flags" parameter's possible values are
> > + *                     explained above with "DEVFREQ_FLAG_*" macros.
> >  * @get_dev_status     The device should provide the current performance
> >  *                     status to devfreq, which is used by governors.
> >  * @exit               An optional callback that is called when devfreq
> > @@ -66,7 +76,7 @@ struct devfreq_dev_profile {
> >        unsigned long initial_freq;
> >        unsigned int polling_ms;
> >
> > -       int (*target)(struct device *dev, unsigned long *freq);
> > +       int (*target)(struct device *dev, unsigned long *freq, u32 flags);
> >        int (*get_dev_status)(struct device *dev,
> >                              struct devfreq_dev_status *stat);
> >        void (*exit)(struct device *dev);
> > @@ -165,7 +175,7 @@ extern int devfreq_remove_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);
> > +                                          unsigned long *freq, u32 flags);
> >  extern int devfreq_register_opp_notifier(struct device *dev,
> >                                         struct devfreq *devfreq);
> >  extern int devfreq_unregister_opp_notifier(struct device *dev,
> > @@ -216,7 +226,7 @@ static int devfreq_remove_device(struct devfreq *devfreq)
> >  }
> >
> >  static struct opp *devfreq_recommended_opp(struct device *dev,
> > -                                          unsigned long *freq)
> > +                                          unsigned long *freq, u32 flags)
> >  {
> >        return -EINVAL;
> >  }
> > --
> > 1.7.4.1
> >
> 
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2012-03-16 21:03 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-10  7:19 [PATCH] PM / devfreq: add relation of recommended frequency MyungJoo Ham
2012-02-29 22:59 ` Rafael J. Wysocki
2012-03-05  7:44   ` MyungJoo Ham
2012-03-07  7:40   ` MyungJoo Ham
2012-03-10 21:51     ` Rafael J. Wysocki
2012-03-12  2:12       ` MyungJoo Ham
2012-03-12 23:46         ` Rafael J. Wysocki
2012-03-15  0:25     ` Rafael J. Wysocki
2012-03-16  6:02       ` [PATCH v3] " MyungJoo Ham
2012-03-16  6:14         ` Turquette, Mike
2012-03-16 21:07           ` Rafael J. Wysocki
2012-03-07 17:45 ` [PATCH] " Turquette, Mike
2012-03-09  7:09   ` MyungJoo Ham

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).