[v7,6/6] PM / devfreq: Use PM QoS for sysfs min/max_freq
diff mbox series

Message ID 00747fe09746282ef4d99ffd2a4e58e592ba4f66.1569272883.git.leonard.crestez@nxp.com
State Superseded
Headers show
Series
  • PM / devfreq: Add dev_pm_qos support
Related show

Commit Message

Leonard Crestez Sept. 23, 2019, 9:10 p.m. UTC
Switch the handling of min_freq and max_freq from sysfs to use the
dev_pm_qos_request interface.

Since PM QoS handles frequencies as kHz this change reduces the
precision of min_freq and max_freq. This shouldn't introduce problems
because frequencies which are not an integer number of kHz are likely
not an integer number of Hz either.

Try to ensure compatibility by rounding min values down and rounding
max values up.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/devfreq/devfreq.c | 49 +++++++++++++++++++++++++--------------
 include/linux/devfreq.h   |  9 +++----
 2 files changed, 36 insertions(+), 22 deletions(-)

Comments

Chanwoo Choi Sept. 24, 2019, 7:26 a.m. UTC | #1
Hi,

On 19. 9. 24. 오전 6:10, Leonard Crestez wrote:
> Switch the handling of min_freq and max_freq from sysfs to use the
> dev_pm_qos_request interface.
> 
> Since PM QoS handles frequencies as kHz this change reduces the
> precision of min_freq and max_freq. This shouldn't introduce problems
> because frequencies which are not an integer number of kHz are likely
> not an integer number of Hz either.
> 
> Try to ensure compatibility by rounding min values down and rounding
> max values up.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/devfreq/devfreq.c | 49 +++++++++++++++++++++++++--------------
>  include/linux/devfreq.h   |  9 +++----
>  2 files changed, 36 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 9887408f23bb..a00737e34d36 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -132,14 +132,10 @@ static void devfreq_get_freq_range(struct devfreq *devfreq,
>  	*min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
>  				devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY));
>  	*max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
>  				devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY));
>  
> -	/* constraints from sysfs */
> -	*min_freq = max(*min_freq, devfreq->min_freq);
> -	*max_freq = min(*max_freq, devfreq->max_freq);
> -
>  	/* constraints from OPP interface */
>  	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
>  	/* scaling_max_freq can be zero on error */
>  	if (devfreq->scaling_max_freq)
>  		*max_freq = min(*max_freq, devfreq->scaling_max_freq);
> @@ -679,10 +675,12 @@ static void devfreq_dev_release(struct device *dev)
>  			DEV_PM_QOS_MIN_FREQUENCY);
>  
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
> +	dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
> +	dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
>  	mutex_destroy(&devfreq->lock);
>  	kfree(devfreq);
>  }
> @@ -747,18 +745,26 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
>  	if (!devfreq->scaling_min_freq) {
>  		err = -EINVAL;
>  		goto err_dev;
>  	}
> -	devfreq->min_freq = devfreq->scaling_min_freq;
>  
>  	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
>  	if (!devfreq->scaling_max_freq) {
>  		err = -EINVAL;
>  		goto err_dev;
>  	}
> -	devfreq->max_freq = devfreq->scaling_max_freq;
> +
> +	/* PM QoS requests for min/max freq from sysfs */

The comment is important. But the devfreq_add_device() has usually
not added the comment for each step. I'm not sure to add the some
comments for only this. How about removing it?

> +	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
> +	if (err < 0)
> +		goto err_dev;
> +	err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
> +				     DEV_PM_QOS_MAX_FREQUENCY, S32_MAX);
> +	if (err < 0)
> +		goto err_dev;
>  
>  	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>  	atomic_set(&devfreq->suspend_count, 0);
>  
>  	devfreq->trans_table = kzalloc(
> @@ -843,10 +849,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  err_dev:
>  	/*
>  	 * Cleanup path for errors that happen before registration.
>  	 * Otherwise we rely on devfreq_dev_release.
>  	 */
> +	if (dev_pm_qos_request_active(&devfreq->user_max_freq_req))
> +		dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
> +	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
> +		dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
>  	kfree(devfreq);
>  err_out:
>  	return ERR_PTR(err);
> @@ -1387,14 +1397,17 @@ 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);
> -	df->min_freq = value;
> -	update_devfreq(df);
> -	mutex_unlock(&df->lock);
> +	/* round down to kHz for dev_pm_qos */

Please remove it.

> +	if (value)
> +		value = value / HZ_PER_KHZ;

It doesn't be necessary.

> +
> +	ret = dev_pm_qos_update_request(&df->user_min_freq_req, value);

Change it as following:
	ret = dev_pm_qos_update_request(&df->user_min_freq_req, (value / HZ_PER_KHZ));

> +	if (ret < 0)
> +		return ret;
>  
>  	return count;
>  }
>  
>  static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
> @@ -1419,19 +1432,19 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>  
>  	ret = sscanf(buf, "%lu", &value);
>  	if (ret != 1)
>  		return -EINVAL;
>  
> -	mutex_lock(&df->lock);
> -
> -	/* Interpret zero as "don't care" */
> -	if (!value)
> -		value = ULONG_MAX;
> +	/* round up to kHz for dev_pm_qos and interpret zero as "don't care" */

Please remove it.

> +	if (value)
> +		value = DIV_ROUND_UP(value, HZ_PER_KHZ);

Why do you use 'DIV_ROUND_UP(value, HZ_PER_KHZ)'
instead of 'value / HZ_PER_KHZ' in min_freq_store()?

> +	else
> +		value = S32_MAX;> > -	df->max_freq = value;
> -	update_devfreq(df);
> -	mutex_unlock(&df->lock);
> +	ret = dev_pm_qos_update_request(&df->user_max_freq_req, value);
> +	if (ret < 0)
> +		return ret;
>  
>  	return count;
>  }
>  static DEVICE_ATTR_RW(min_freq);
>  
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 8b92ccbd1962..3162eb9b0954 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -11,10 +11,11 @@
>  #define __LINUX_DEVFREQ_H__
>  
>  #include <linux/device.h>
>  #include <linux/notifier.h>
>  #include <linux/pm_opp.h>
> +#include <linux/pm_qos.h>
>  
>  #define DEVFREQ_NAME_LEN 16
>  
>  /* DEVFREQ governor name */
>  #define DEVFREQ_GOV_SIMPLE_ONDEMAND	"simple_ondemand"
> @@ -121,12 +122,12 @@ struct devfreq_dev_profile {
>   *		devfreq.nb to the corresponding register notifier call chain.
>   * @work:	delayed work for load monitoring.
>   * @previous_freq:	previously configured frequency value.
>   * @data:	Private data of the governor. The devfreq framework does not
>   *		touch this.
> - * @min_freq:	Limit minimum frequency requested by user (0: none)
> - * @max_freq:	Limit maximum frequency requested by user (0: none)
> + * @user_min_freq_req:	PM QoS min frequency request from user (via sysfs)
> + * @user_max_freq_req:	PM QoS max frequency request from user (via sysfs)

I think that 'user' prefix is not needed. You better to change it as following
	@min_freq_req:	PM QoS minimum frequency request by user via sysfs
	@max_freq_req:	PM QoS maximum frequency request by user via sysfs

>   * @scaling_min_freq:	Limit minimum frequency requested by OPP interface
>   * @scaling_max_freq:	Limit maximum frequency requested by OPP interface
>   * @stop_polling:	 devfreq polling status of a device.
>   * @suspend_freq:	 frequency of a device set during suspend phase.
>   * @resume_freq:	 frequency of a device set in resume phase.
> @@ -161,12 +162,12 @@ struct devfreq {
>  	unsigned long previous_freq;
>  	struct devfreq_dev_status last_status;
>  
>  	void *data; /* private data for governors */
>  
> -	unsigned long min_freq;
> -	unsigned long max_freq;
> +	struct dev_pm_qos_request user_min_freq_req;
> +	struct dev_pm_qos_request user_max_freq_req;

ditto.

>  	unsigned long scaling_min_freq;
>  	unsigned long scaling_max_freq;
>  	bool stop_polling;
>  
>  	unsigned long suspend_freq;
>
Leonard Crestez Sept. 24, 2019, 9:48 a.m. UTC | #2
On 2019-09-24 10:22 AM, Chanwoo Choi wrote:
> Hi,
> 
> On 19. 9. 24. 오전 6:10, Leonard Crestez wrote:
>> Switch the handling of min_freq and max_freq from sysfs to use the
>> dev_pm_qos_request interface.
>>
>> Since PM QoS handles frequencies as kHz this change reduces the
>> precision of min_freq and max_freq. This shouldn't introduce problems
>> because frequencies which are not an integer number of kHz are likely
>> not an integer number of Hz either.
>>
>> Try to ensure compatibility by rounding min values down and rounding
>> max values up.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>   drivers/devfreq/devfreq.c | 49 +++++++++++++++++++++++++--------------
>>   include/linux/devfreq.h   |  9 +++----
>>   2 files changed, 36 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 9887408f23bb..a00737e34d36 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -132,14 +132,10 @@ static void devfreq_get_freq_range(struct devfreq *devfreq,
>>   	*min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
>>   				devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY));
>>   	*max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
>>   				devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY));
>>   
>> -	/* constraints from sysfs */
>> -	*min_freq = max(*min_freq, devfreq->min_freq);
>> -	*max_freq = min(*max_freq, devfreq->max_freq);
>> -
>>   	/* constraints from OPP interface */
>>   	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
>>   	/* scaling_max_freq can be zero on error */
>>   	if (devfreq->scaling_max_freq)
>>   		*max_freq = min(*max_freq, devfreq->scaling_max_freq);
>> @@ -679,10 +675,12 @@ static void devfreq_dev_release(struct device *dev)
>>   			DEV_PM_QOS_MIN_FREQUENCY);
>>   
>>   	if (devfreq->profile->exit)
>>   		devfreq->profile->exit(devfreq->dev.parent);
>>   
>> +	dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
>> +	dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
>>   	kfree(devfreq->time_in_state);
>>   	kfree(devfreq->trans_table);
>>   	mutex_destroy(&devfreq->lock);
>>   	kfree(devfreq);
>>   }
>> @@ -747,18 +745,26 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
>>   	if (!devfreq->scaling_min_freq) {
>>   		err = -EINVAL;
>>   		goto err_dev;
>>   	}
>> -	devfreq->min_freq = devfreq->scaling_min_freq;
>>   
>>   	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
>>   	if (!devfreq->scaling_max_freq) {
>>   		err = -EINVAL;
>>   		goto err_dev;
>>   	}
>> -	devfreq->max_freq = devfreq->scaling_max_freq;
>> +
>> +	/* PM QoS requests for min/max freq from sysfs */
> 
> The comment is important. But the devfreq_add_device() has usually
> not added the comment for each step. I'm not sure to add the some
> comments for only this. How about removing it?

OK.

>> +	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
>> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
>> +	if (err < 0)
>> +		goto err_dev;
>> +	err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
>> +				     DEV_PM_QOS_MAX_FREQUENCY, S32_MAX);
>> +	if (err < 0)
>> +		goto err_dev;
>>   
>>   	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>>   	atomic_set(&devfreq->suspend_count, 0);
>>   
>>   	devfreq->trans_table = kzalloc(
>> @@ -843,10 +849,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   err_dev:
>>   	/*
>>   	 * Cleanup path for errors that happen before registration.
>>   	 * Otherwise we rely on devfreq_dev_release.
>>   	 */
>> +	if (dev_pm_qos_request_active(&devfreq->user_max_freq_req))
>> +		dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
>> +	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
>> +		dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
>>   	kfree(devfreq->time_in_state);
>>   	kfree(devfreq->trans_table);
>>   	kfree(devfreq);
>>   err_out:
>>   	return ERR_PTR(err);
>> @@ -1387,14 +1397,17 @@ 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);
>> -	df->min_freq = value;
>> -	update_devfreq(df);
>> -	mutex_unlock(&df->lock);
>> +	/* round down to kHz for dev_pm_qos */
> 
> Please remove it.
> 
>> +	if (value)
>> +		value = value / HZ_PER_KHZ;
> 
> It doesn't be necessary.
> 
>> +
>> +	ret = dev_pm_qos_update_request(&df->user_min_freq_req, value);
> 
> Change it as following:
> 	ret = dev_pm_qos_update_request(&df->user_min_freq_req, (value / HZ_PER_KHZ));

OK

>> +	if (ret < 0)
>> +		return ret;
>>   
>>   	return count;
>>   }
>>   
>>   static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
>> @@ -1419,19 +1432,19 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>   
>>   	ret = sscanf(buf, "%lu", &value);
>>   	if (ret != 1)
>>   		return -EINVAL;
>>   
>> -	mutex_lock(&df->lock);
>> -
>> -	/* Interpret zero as "don't care" */
>> -	if (!value)
>> -		value = ULONG_MAX;
>> +	/* round up to kHz for dev_pm_qos and interpret zero as "don't care" */
> 
> Please remove it.
> 
>> +	if (value)
>> +		value = DIV_ROUND_UP(value, HZ_PER_KHZ);
> 
> Why do you use 'DIV_ROUND_UP(value, HZ_PER_KHZ)'
> instead of 'value / HZ_PER_KHZ' in min_freq_store()?

If an user request of max=666666666 Hz is converted to max=666666 kHz 
and then back to 666666000 Hz then the OPP with freq=2/3 gHz might be 
cut out. I deliberately rounded down for min and up for max to make the 
constraint interval "inclusive".

Does this make sense? It seems like the path least likely to cause issues.
>> +	else
>> +		value = S32_MAX; >> -	df->max_freq = value;
>> -	update_devfreq(df);
>> -	mutex_unlock(&df->lock);
>> +	ret = dev_pm_qos_update_request(&df->user_max_freq_req, value);
>> +	if (ret < 0)
>> +		return ret;
>>   
>>   	return count;
>>   }
>>   static DEVICE_ATTR_RW(min_freq);
>>   
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index 8b92ccbd1962..3162eb9b0954 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -11,10 +11,11 @@
>>   #define __LINUX_DEVFREQ_H__
>>   
>>   #include <linux/device.h>
>>   #include <linux/notifier.h>
>>   #include <linux/pm_opp.h>
>> +#include <linux/pm_qos.h>
>>   
>>   #define DEVFREQ_NAME_LEN 16
>>   
>>   /* DEVFREQ governor name */
>>   #define DEVFREQ_GOV_SIMPLE_ONDEMAND	"simple_ondemand"
>> @@ -121,12 +122,12 @@ struct devfreq_dev_profile {
>>    *		devfreq.nb to the corresponding register notifier call chain.
>>    * @work:	delayed work for load monitoring.
>>    * @previous_freq:	previously configured frequency value.
>>    * @data:	Private data of the governor. The devfreq framework does not
>>    *		touch this.
>> - * @min_freq:	Limit minimum frequency requested by user (0: none)
>> - * @max_freq:	Limit maximum frequency requested by user (0: none)
>> + * @user_min_freq_req:	PM QoS min frequency request from user (via sysfs)
>> + * @user_max_freq_req:	PM QoS max frequency request from user (via sysfs)
> 
> I think that 'user' prefix is not needed. You better to change it as following

Was requested here:
     https://patchwork.kernel.org/patch/11149505/#22891887

It makes sense to mention ths request is from userspace via sysfs. Other 
drivers can also make PM QoS frequency requests for their own reasons.

Patch
diff mbox series

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 9887408f23bb..a00737e34d36 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -132,14 +132,10 @@  static void devfreq_get_freq_range(struct devfreq *devfreq,
 	*min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
 				devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY));
 	*max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
 				devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY));
 
-	/* constraints from sysfs */
-	*min_freq = max(*min_freq, devfreq->min_freq);
-	*max_freq = min(*max_freq, devfreq->max_freq);
-
 	/* constraints from OPP interface */
 	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
 	/* scaling_max_freq can be zero on error */
 	if (devfreq->scaling_max_freq)
 		*max_freq = min(*max_freq, devfreq->scaling_max_freq);
@@ -679,10 +675,12 @@  static void devfreq_dev_release(struct device *dev)
 			DEV_PM_QOS_MIN_FREQUENCY);
 
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
+	dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
+	dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
 	kfree(devfreq->time_in_state);
 	kfree(devfreq->trans_table);
 	mutex_destroy(&devfreq->lock);
 	kfree(devfreq);
 }
@@ -747,18 +745,26 @@  struct devfreq *devfreq_add_device(struct device *dev,
 	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
 	if (!devfreq->scaling_min_freq) {
 		err = -EINVAL;
 		goto err_dev;
 	}
-	devfreq->min_freq = devfreq->scaling_min_freq;
 
 	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
 	if (!devfreq->scaling_max_freq) {
 		err = -EINVAL;
 		goto err_dev;
 	}
-	devfreq->max_freq = devfreq->scaling_max_freq;
+
+	/* PM QoS requests for min/max freq from sysfs */
+	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
+				     DEV_PM_QOS_MIN_FREQUENCY, 0);
+	if (err < 0)
+		goto err_dev;
+	err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
+				     DEV_PM_QOS_MAX_FREQUENCY, S32_MAX);
+	if (err < 0)
+		goto err_dev;
 
 	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
 	atomic_set(&devfreq->suspend_count, 0);
 
 	devfreq->trans_table = kzalloc(
@@ -843,10 +849,14 @@  struct devfreq *devfreq_add_device(struct device *dev,
 err_dev:
 	/*
 	 * Cleanup path for errors that happen before registration.
 	 * Otherwise we rely on devfreq_dev_release.
 	 */
+	if (dev_pm_qos_request_active(&devfreq->user_max_freq_req))
+		dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
+	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
+		dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
 	kfree(devfreq->time_in_state);
 	kfree(devfreq->trans_table);
 	kfree(devfreq);
 err_out:
 	return ERR_PTR(err);
@@ -1387,14 +1397,17 @@  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);
-	df->min_freq = value;
-	update_devfreq(df);
-	mutex_unlock(&df->lock);
+	/* round down to kHz for dev_pm_qos */
+	if (value)
+		value = value / HZ_PER_KHZ;
+
+	ret = dev_pm_qos_update_request(&df->user_min_freq_req, value);
+	if (ret < 0)
+		return ret;
 
 	return count;
 }
 
 static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
@@ -1419,19 +1432,19 @@  static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 
 	ret = sscanf(buf, "%lu", &value);
 	if (ret != 1)
 		return -EINVAL;
 
-	mutex_lock(&df->lock);
-
-	/* Interpret zero as "don't care" */
-	if (!value)
-		value = ULONG_MAX;
+	/* round up to kHz for dev_pm_qos and interpret zero as "don't care" */
+	if (value)
+		value = DIV_ROUND_UP(value, HZ_PER_KHZ);
+	else
+		value = S32_MAX;
 
-	df->max_freq = value;
-	update_devfreq(df);
-	mutex_unlock(&df->lock);
+	ret = dev_pm_qos_update_request(&df->user_max_freq_req, value);
+	if (ret < 0)
+		return ret;
 
 	return count;
 }
 static DEVICE_ATTR_RW(min_freq);
 
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 8b92ccbd1962..3162eb9b0954 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -11,10 +11,11 @@ 
 #define __LINUX_DEVFREQ_H__
 
 #include <linux/device.h>
 #include <linux/notifier.h>
 #include <linux/pm_opp.h>
+#include <linux/pm_qos.h>
 
 #define DEVFREQ_NAME_LEN 16
 
 /* DEVFREQ governor name */
 #define DEVFREQ_GOV_SIMPLE_ONDEMAND	"simple_ondemand"
@@ -121,12 +122,12 @@  struct devfreq_dev_profile {
  *		devfreq.nb to the corresponding register notifier call chain.
  * @work:	delayed work for load monitoring.
  * @previous_freq:	previously configured frequency value.
  * @data:	Private data of the governor. The devfreq framework does not
  *		touch this.
- * @min_freq:	Limit minimum frequency requested by user (0: none)
- * @max_freq:	Limit maximum frequency requested by user (0: none)
+ * @user_min_freq_req:	PM QoS min frequency request from user (via sysfs)
+ * @user_max_freq_req:	PM QoS max frequency request from user (via sysfs)
  * @scaling_min_freq:	Limit minimum frequency requested by OPP interface
  * @scaling_max_freq:	Limit maximum frequency requested by OPP interface
  * @stop_polling:	 devfreq polling status of a device.
  * @suspend_freq:	 frequency of a device set during suspend phase.
  * @resume_freq:	 frequency of a device set in resume phase.
@@ -161,12 +162,12 @@  struct devfreq {
 	unsigned long previous_freq;
 	struct devfreq_dev_status last_status;
 
 	void *data; /* private data for governors */
 
-	unsigned long min_freq;
-	unsigned long max_freq;
+	struct dev_pm_qos_request user_min_freq_req;
+	struct dev_pm_qos_request user_max_freq_req;
 	unsigned long scaling_min_freq;
 	unsigned long scaling_max_freq;
 	bool stop_polling;
 
 	unsigned long suspend_freq;