diff mbox series

[v10,09/11] PM / devfreq: Introduce get_freq_range helper

Message ID 6fd2ab4b54f5cd3a02b3f5f8aae5e776485441e3.1572556786.git.leonard.crestez@nxp.com (mailing list archive)
State Accepted
Headers show
Series [v10,01/11] PM / devfreq: Fix devfreq_notifier_call returning errno | expand

Commit Message

Leonard Crestez Oct. 31, 2019, 9:34 p.m. UTC 
Moving handling of min/max freq to a single function and call it from
update_devfreq and for printing min/max freq values in sysfs.

This changes the behavior of out-of-range min_freq/max_freq: clamping
is now done at evaluation time. This means that if an out-of-range
constraint is imposed by sysfs and it later becomes valid then it will
be enforced.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c | 108 +++++++++++++++++++++-----------------
 1 file changed, 60 insertions(+), 48 deletions(-)

Comments

Chanwoo Choi Nov. 11, 2019, 5:56 a.m. UTC | #1 
On 11/1/19 6:34 AM, Leonard Crestez wrote:
> Moving handling of min/max freq to a single function and call it from
> update_devfreq and for printing min/max freq values in sysfs.
> 
> This changes the behavior of out-of-range min_freq/max_freq: clamping
> is now done at evaluation time. This means that if an out-of-range
> constraint is imposed by sysfs and it later becomes valid then it will
> be enforced.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/devfreq.c | 108 +++++++++++++++++++++-----------------
>  1 file changed, 60 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index ab12fd22aa08..ba3b53ee23fd 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -96,10 +96,51 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>  		dev_pm_opp_put(opp);
>  
>  	return max_freq;
>  }
>  
> +/**
> + * get_freq_range() - Get the current freq range
> + * @devfreq:	the devfreq instance
> + * @min_freq:	the min frequency
> + * @max_freq:	the max frequency
> + *
> + * This takes into consideration all constraints.
> + */
> +static void get_freq_range(struct devfreq *devfreq,
> +			   unsigned long *min_freq,
> +			   unsigned long *max_freq)
> +{
> +	unsigned long *freq_table = devfreq->profile->freq_table;
> +
> +	lockdep_assert_held(&devfreq->lock);
> +
> +	/*
> +	 * Initialize minimum/maximum frequency from freq table.
> +	 * The devfreq drivers can initialize this in either ascending or
> +	 * descending order and devfreq core supports both.
> +	 */
> +	if (freq_table[0] < freq_table[devfreq->profile->max_state - 1]) {
> +		*min_freq = freq_table[0];
> +		*max_freq = freq_table[devfreq->profile->max_state - 1];
> +	} else {
> +		*min_freq = freq_table[devfreq->profile->max_state - 1];
> +		*max_freq = freq_table[0];
> +	}
> +
> +	/* Apply constraints from sysfs */
> +	*min_freq = max(*min_freq, devfreq->min_freq);
> +	*max_freq = min(*max_freq, devfreq->max_freq);
> +
> +	/* Apply constraints from OPP interface */
> +	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
> +	*max_freq = min(*max_freq, devfreq->scaling_max_freq);
> +
> +	if (*min_freq > *max_freq)
> +		*min_freq = *max_freq;
> +}
> +
>  /**
>   * devfreq_get_freq_level() - Lookup freq_table for the frequency
>   * @devfreq:	the devfreq instance
>   * @freq:	the target frequency
>   */
> @@ -348,20 +389,11 @@ int update_devfreq(struct devfreq *devfreq)
>  
>  	/* Reevaluate the proper frequency */
>  	err = devfreq->governor->get_target_freq(devfreq, &freq);
>  	if (err)
>  		return err;
> -
> -	/*
> -	 * Adjust the frequency with user freq, QoS and available freq.
> -	 *
> -	 * List from the highest priority
> -	 * max_freq
> -	 * min_freq
> -	 */
> -	max_freq = min(devfreq->scaling_max_freq, devfreq->max_freq);
> -	min_freq = max(devfreq->scaling_min_freq, devfreq->min_freq);
> +	get_freq_range(devfreq, &min_freq, &max_freq);
>  
>  	if (freq < min_freq) {
>  		freq = min_freq;
>  		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
>  	}
> @@ -1292,40 +1324,28 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>  	ret = sscanf(buf, "%lu", &value);
>  	if (ret != 1)
>  		return -EINVAL;
>  
>  	mutex_lock(&df->lock);
> -
> -	if (value) {
> -		if (value > df->max_freq) {
> -			ret = -EINVAL;
> -			goto unlock;
> -		}
> -	} else {
> -		unsigned long *freq_table = df->profile->freq_table;
> -
> -		/* Get minimum frequency according to sorting order */
> -		if (freq_table[0] < freq_table[df->profile->max_state - 1])
> -			value = freq_table[0];
> -		else
> -			value = freq_table[df->profile->max_state - 1];
> -	}
> -
>  	df->min_freq = value;
>  	update_devfreq(df);
> -	ret = count;
> -unlock:
>  	mutex_unlock(&df->lock);
> -	return ret;
> +
> +	return count;
>  }
>  
>  static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
>  			     char *buf)
>  {
>  	struct devfreq *df = to_devfreq(dev);
> +	unsigned long min_freq, max_freq;
>  
> -	return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
> +	mutex_lock(&df->lock);
> +	get_freq_range(df, &min_freq, &max_freq);
> +	mutex_unlock(&df->lock);
> +
> +	return sprintf(buf, "%lu\n", min_freq);
>  }
>  
>  static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>  			      const char *buf, size_t count)
>  {
> @@ -1337,40 +1357,32 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>  	if (ret != 1)
>  		return -EINVAL;
>  
>  	mutex_lock(&df->lock);
>  
> -	if (value) {
> -		if (value < df->min_freq) {
> -			ret = -EINVAL;
> -			goto unlock;
> -		}
> -	} else {
> -		unsigned long *freq_table = df->profile->freq_table;
> -
> -		/* Get maximum frequency according to sorting order */
> -		if (freq_table[0] < freq_table[df->profile->max_state - 1])
> -			value = freq_table[df->profile->max_state - 1];
> -		else
> -			value = freq_table[0];
> -	}
> +	if (!value)
> +		value = ULONG_MAX;
>  
>  	df->max_freq = value;
>  	update_devfreq(df);
> -	ret = count;
> -unlock:
>  	mutex_unlock(&df->lock);
> -	return ret;
> +
> +	return count;
>  }
>  static DEVICE_ATTR_RW(min_freq);
>  
>  static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
>  			     char *buf)
>  {
>  	struct devfreq *df = to_devfreq(dev);
> +	unsigned long min_freq, max_freq;
> +
> +	mutex_lock(&df->lock);
> +	get_freq_range(df, &min_freq, &max_freq);
> +	mutex_unlock(&df->lock);
>  
> -	return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq));
> +	return sprintf(buf, "%lu\n", max_freq);
>  }
>  static DEVICE_ATTR_RW(max_freq);
>  
>  static ssize_t available_frequencies_show(struct device *d,
>  					  struct device_attribute *attr,
> 

Applied it because it doesn't depend on the pm_qos feature.
Leonard Crestez Nov. 11, 2019, 2:37 p.m. UTC | #2 
On Mon, 2019-11-11 at 14:56 +0900, Chanwoo Choi wrote:
> On 11/1/19 6:34 AM, Leonard Crestez wrote:
> > Moving handling of min/max freq to a single function and call it from
> > update_devfreq and for printing min/max freq values in sysfs.
> > 
> > This changes the behavior of out-of-range min_freq/max_freq: clamping
> > is now done at evaluation time. This means that if an out-of-range
> > constraint is imposed by sysfs and it later becomes valid then it will
> > be enforced.
> > 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> > ---
> >  drivers/devfreq/devfreq.c | 108 +++++++++++++++++++++-----------------
> >  1 file changed, 60 insertions(+), 48 deletions(-)
> > 
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index ab12fd22aa08..ba3b53ee23fd 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -96,10 +96,51 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
> >  		dev_pm_opp_put(opp);
> >  
> >  	return max_freq;
> >  }
> >  
> > +/**
> > + * get_freq_range() - Get the current freq range
> > + * @devfreq:	the devfreq instance
> > + * @min_freq:	the min frequency
> > + * @max_freq:	the max frequency
> > + *
> > + * This takes into consideration all constraints.
> > + */
> > +static void get_freq_range(struct devfreq *devfreq,
> > +			   unsigned long *min_freq,
> > +			   unsigned long *max_freq)
> > +{
> > +	unsigned long *freq_table = devfreq->profile->freq_table;
> > +
> > +	lockdep_assert_held(&devfreq->lock);
> > +
> > +	/*
> > +	 * Initialize minimum/maximum frequency from freq table.
> > +	 * The devfreq drivers can initialize this in either ascending or
> > +	 * descending order and devfreq core supports both.
> > +	 */
> > +	if (freq_table[0] < freq_table[devfreq->profile->max_state - 1]) {
> > +		*min_freq = freq_table[0];
> > +		*max_freq = freq_table[devfreq->profile->max_state - 1];
> > +	} else {
> > +		*min_freq = freq_table[devfreq->profile->max_state - 1];
> > +		*max_freq = freq_table[0];
> > +	}
> > +
> > +	/* Apply constraints from sysfs */
> > +	*min_freq = max(*min_freq, devfreq->min_freq);
> > +	*max_freq = min(*max_freq, devfreq->max_freq);
> > +
> > +	/* Apply constraints from OPP interface */
> > +	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
> > +	*max_freq = min(*max_freq, devfreq->scaling_max_freq);
> > +
> > +	if (*min_freq > *max_freq)
> > +		*min_freq = *max_freq;
> > +}
> > +
> >  /**
> >   * devfreq_get_freq_level() - Lookup freq_table for the frequency
> >   * @devfreq:	the devfreq instance
> >   * @freq:	the target frequency
> >   */
> > @@ -348,20 +389,11 @@ int update_devfreq(struct devfreq *devfreq)
> >  
> >  	/* Reevaluate the proper frequency */
> >  	err = devfreq->governor->get_target_freq(devfreq, &freq);
> >  	if (err)
> >  		return err;
> > -
> > -	/*
> > -	 * Adjust the frequency with user freq, QoS and available freq.
> > -	 *
> > -	 * List from the highest priority
> > -	 * max_freq
> > -	 * min_freq
> > -	 */
> > -	max_freq = min(devfreq->scaling_max_freq, devfreq->max_freq);
> > -	min_freq = max(devfreq->scaling_min_freq, devfreq->min_freq);
> > +	get_freq_range(devfreq, &min_freq, &max_freq);
> >  
> >  	if (freq < min_freq) {
> >  		freq = min_freq;
> >  		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
> >  	}
> > @@ -1292,40 +1324,28 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
> >  	ret = sscanf(buf, "%lu", &value);
> >  	if (ret != 1)
> >  		return -EINVAL;
> >  
> >  	mutex_lock(&df->lock);
> > -
> > -	if (value) {
> > -		if (value > df->max_freq) {
> > -			ret = -EINVAL;
> > -			goto unlock;
> > -		}
> > -	} else {
> > -		unsigned long *freq_table = df->profile->freq_table;
> > -
> > -		/* Get minimum frequency according to sorting order */
> > -		if (freq_table[0] < freq_table[df->profile->max_state - 1])
> > -			value = freq_table[0];
> > -		else
> > -			value = freq_table[df->profile->max_state - 1];
> > -	}
> > -
> >  	df->min_freq = value;
> >  	update_devfreq(df);
> > -	ret = count;
> > -unlock:
> >  	mutex_unlock(&df->lock);
> > -	return ret;
> > +
> > +	return count;
> >  }
> >  
> >  static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
> >  			     char *buf)
> >  {
> >  	struct devfreq *df = to_devfreq(dev);
> > +	unsigned long min_freq, max_freq;
> >  
> > -	return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
> > +	mutex_lock(&df->lock);
> > +	get_freq_range(df, &min_freq, &max_freq);
> > +	mutex_unlock(&df->lock);
> > +
> > +	return sprintf(buf, "%lu\n", min_freq);
> >  }
> >  
> >  static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
> >  			      const char *buf, size_t count)
> >  {
> > @@ -1337,40 +1357,32 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
> >  	if (ret != 1)
> >  		return -EINVAL;
> >  
> >  	mutex_lock(&df->lock);
> >  
> > -	if (value) {
> > -		if (value < df->min_freq) {
> > -			ret = -EINVAL;
> > -			goto unlock;
> > -		}
> > -	} else {
> > -		unsigned long *freq_table = df->profile->freq_table;
> > -
> > -		/* Get maximum frequency according to sorting order */
> > -		if (freq_table[0] < freq_table[df->profile->max_state - 1])
> > -			value = freq_table[df->profile->max_state - 1];
> > -		else
> > -			value = freq_table[0];
> > -	}
> > +	if (!value)
> > +		value = ULONG_MAX;
> >  
> >  	df->max_freq = value;
> >  	update_devfreq(df);
> > -	ret = count;
> > -unlock:
> >  	mutex_unlock(&df->lock);
> > -	return ret;
> > +
> > +	return count;
> >  }
> >  static DEVICE_ATTR_RW(min_freq);
> >  
> >  static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
> >  			     char *buf)
> >  {
> >  	struct devfreq *df = to_devfreq(dev);
> > +	unsigned long min_freq, max_freq;
> > +
> > +	mutex_lock(&df->lock);
> > +	get_freq_range(df, &min_freq, &max_freq);
> > +	mutex_unlock(&df->lock);
> >  
> > -	return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq));
> > +	return sprintf(buf, "%lu\n", max_freq);
> >  }
> >  static DEVICE_ATTR_RW(max_freq);
> >  
> >  static ssize_t available_frequencies_show(struct device *d,
> >  					  struct device_attribute *attr,
> > 
> 
> Applied it because it doesn't depend on the pm_qos feature.

Thanks, this will make it easier to move forward.

--
Regards,
Leonard
diff mbox series

Patch

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index ab12fd22aa08..ba3b53ee23fd 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -96,10 +96,51 @@  static unsigned long find_available_max_freq(struct devfreq *devfreq)
 		dev_pm_opp_put(opp);
 
 	return max_freq;
 }
 
+/**
+ * get_freq_range() - Get the current freq range
+ * @devfreq:	the devfreq instance
+ * @min_freq:	the min frequency
+ * @max_freq:	the max frequency
+ *
+ * This takes into consideration all constraints.
+ */
+static void get_freq_range(struct devfreq *devfreq,
+			   unsigned long *min_freq,
+			   unsigned long *max_freq)
+{
+	unsigned long *freq_table = devfreq->profile->freq_table;
+
+	lockdep_assert_held(&devfreq->lock);
+
+	/*
+	 * Initialize minimum/maximum frequency from freq table.
+	 * The devfreq drivers can initialize this in either ascending or
+	 * descending order and devfreq core supports both.
+	 */
+	if (freq_table[0] < freq_table[devfreq->profile->max_state - 1]) {
+		*min_freq = freq_table[0];
+		*max_freq = freq_table[devfreq->profile->max_state - 1];
+	} else {
+		*min_freq = freq_table[devfreq->profile->max_state - 1];
+		*max_freq = freq_table[0];
+	}
+
+	/* Apply constraints from sysfs */
+	*min_freq = max(*min_freq, devfreq->min_freq);
+	*max_freq = min(*max_freq, devfreq->max_freq);
+
+	/* Apply constraints from OPP interface */
+	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
+	*max_freq = min(*max_freq, devfreq->scaling_max_freq);
+
+	if (*min_freq > *max_freq)
+		*min_freq = *max_freq;
+}
+
 /**
  * devfreq_get_freq_level() - Lookup freq_table for the frequency
  * @devfreq:	the devfreq instance
  * @freq:	the target frequency
  */
@@ -348,20 +389,11 @@  int update_devfreq(struct devfreq *devfreq)
 
 	/* Reevaluate the proper frequency */
 	err = devfreq->governor->get_target_freq(devfreq, &freq);
 	if (err)
 		return err;
-
-	/*
-	 * Adjust the frequency with user freq, QoS and available freq.
-	 *
-	 * List from the highest priority
-	 * max_freq
-	 * min_freq
-	 */
-	max_freq = min(devfreq->scaling_max_freq, devfreq->max_freq);
-	min_freq = max(devfreq->scaling_min_freq, devfreq->min_freq);
+	get_freq_range(devfreq, &min_freq, &max_freq);
 
 	if (freq < min_freq) {
 		freq = min_freq;
 		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
 	}
@@ -1292,40 +1324,28 @@  static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
 	ret = sscanf(buf, "%lu", &value);
 	if (ret != 1)
 		return -EINVAL;
 
 	mutex_lock(&df->lock);
-
-	if (value) {
-		if (value > df->max_freq) {
-			ret = -EINVAL;
-			goto unlock;
-		}
-	} else {
-		unsigned long *freq_table = df->profile->freq_table;
-
-		/* Get minimum frequency according to sorting order */
-		if (freq_table[0] < freq_table[df->profile->max_state - 1])
-			value = freq_table[0];
-		else
-			value = freq_table[df->profile->max_state - 1];
-	}
-
 	df->min_freq = value;
 	update_devfreq(df);
-	ret = count;
-unlock:
 	mutex_unlock(&df->lock);
-	return ret;
+
+	return count;
 }
 
 static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
 	struct devfreq *df = to_devfreq(dev);
+	unsigned long min_freq, max_freq;
 
-	return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
+	mutex_lock(&df->lock);
+	get_freq_range(df, &min_freq, &max_freq);
+	mutex_unlock(&df->lock);
+
+	return sprintf(buf, "%lu\n", min_freq);
 }
 
 static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 			      const char *buf, size_t count)
 {
@@ -1337,40 +1357,32 @@  static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 	if (ret != 1)
 		return -EINVAL;
 
 	mutex_lock(&df->lock);
 
-	if (value) {
-		if (value < df->min_freq) {
-			ret = -EINVAL;
-			goto unlock;
-		}
-	} else {
-		unsigned long *freq_table = df->profile->freq_table;
-
-		/* Get maximum frequency according to sorting order */
-		if (freq_table[0] < freq_table[df->profile->max_state - 1])
-			value = freq_table[df->profile->max_state - 1];
-		else
-			value = freq_table[0];
-	}
+	if (!value)
+		value = ULONG_MAX;
 
 	df->max_freq = value;
 	update_devfreq(df);
-	ret = count;
-unlock:
 	mutex_unlock(&df->lock);
-	return ret;
+
+	return count;
 }
 static DEVICE_ATTR_RW(min_freq);
 
 static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
 	struct devfreq *df = to_devfreq(dev);
+	unsigned long min_freq, max_freq;
+
+	mutex_lock(&df->lock);
+	get_freq_range(df, &min_freq, &max_freq);
+	mutex_unlock(&df->lock);
 
-	return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq));
+	return sprintf(buf, "%lu\n", max_freq);
 }
 static DEVICE_ATTR_RW(max_freq);
 
 static ssize_t available_frequencies_show(struct device *d,
 					  struct device_attribute *attr,