[v7,5/6] PM / devfreq: Add PM QoS support
diff mbox series

Message ID e9868310f9543b4f4a6c7bbe5d4d015da9a0e71d.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
Register notifiers with the PM QoS framework in order to respond to
requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY.

No notifiers are added by this patch but PM QoS constraints can be
imposed externally (for example from other devices).

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/devfreq.c | 76 +++++++++++++++++++++++++++++++++++++++
 include/linux/devfreq.h   |  5 +++
 2 files changed, 81 insertions(+)

Comments

Matthias Kaehlcke Sept. 23, 2019, 9:42 p.m. UTC | #1
On Tue, Sep 24, 2019 at 12:10:33AM +0300, Leonard Crestez wrote:
> Register notifiers with the PM QoS framework in order to respond to
> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY.
> 
> No notifiers are added by this patch but PM QoS constraints can be
> imposed externally (for example from other devices).
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 76 +++++++++++++++++++++++++++++++++++++++
>  include/linux/devfreq.h   |  5 +++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 7f152a582e78..9887408f23bb 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -22,17 +22,20 @@
>  #include <linux/platform_device.h>
>  #include <linux/list.h>
>  #include <linux/printk.h>
>  #include <linux/hrtimer.h>
>  #include <linux/of.h>
> +#include <linux/pm_qos.h>
>  #include "governor.h"
>  
>  #define HZ_PER_KHZ 1000
>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/devfreq.h>
>  
> +#define HZ_PER_KHZ	1000
> +
>  static struct class *devfreq_class;
>  
>  /*
>   * devfreq core provides delayed work based load monitoring helper
>   * functions. Governors can use these or can implement their own
> @@ -123,10 +126,16 @@ static void devfreq_get_freq_range(struct devfreq *devfreq,
>  	} else {
>  		*min_freq = freq_table[devfreq->profile->max_state - 1];
>  		*max_freq = freq_table[0];
>  	}
>  
> +	/* constraints from PM QoS */
> +	*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 */
> @@ -605,10 +614,53 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>  	mutex_unlock(&devfreq->lock);
>  
>  	return ret;
>  }
>  
> +/**
> + * devfreq_qos_notifier_call() - Common handler for QoS constraints.
> + * @devfreq:    the devfreq instance.
> + */
> +static int devfreq_qos_notifier_call(struct devfreq *devfreq)
> +{
> +	int err;
> +
> +	mutex_lock(&devfreq->lock);
> +	err = update_devfreq(devfreq);
> +	mutex_unlock(&devfreq->lock);
> +	if (err)
> +		dev_err(&devfreq->dev, "dvfs for QoS constraints"
> +				" failed with (%d) error\n", err);

nit: DVFS. devfreq_monitor() also uses the lower-case acronym though, so
you can claim this is consistent :)

I'd prefer to spare you another trivial re-spin, but unfortunately
breaking the log message into multiple lines is a coding style
violation:

Documentation/process/coding-style.rst
  2) Breaking long lines and strings

  However, never break user-visible strings such as printk messages,
  because that breaks the ability to grep for them.


With that fixed:

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Chanwoo Choi Sept. 24, 2019, 6:52 a.m. UTC | #2
Hi,

On 19. 9. 24. 오전 6:10, Leonard Crestez wrote:
> Register notifiers with the PM QoS framework in order to respond to
> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY.
> 
> No notifiers are added by this patch but PM QoS constraints can be
> imposed externally (for example from other devices).
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 76 +++++++++++++++++++++++++++++++++++++++
>  include/linux/devfreq.h   |  5 +++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 7f152a582e78..9887408f23bb 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -22,17 +22,20 @@
>  #include <linux/platform_device.h>
>  #include <linux/list.h>
>  #include <linux/printk.h>
>  #include <linux/hrtimer.h>
>  #include <linux/of.h>
> +#include <linux/pm_qos.h>
>  #include "governor.h"
>  
>  #define HZ_PER_KHZ 1000

You have to remove it on patch4.

>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/devfreq.h>
>  
> +#define HZ_PER_KHZ	1000> +
>  static struct class *devfreq_class;
>  
>  /*
>   * devfreq core provides delayed work based load monitoring helper
>   * functions. Governors can use these or can implement their own
> @@ -123,10 +126,16 @@ static void devfreq_get_freq_range(struct devfreq *devfreq,
>  	} else {
>  		*min_freq = freq_table[devfreq->profile->max_state - 1];
>  		*max_freq = freq_table[0];
>  	}
>  
> +	/* constraints from PM QoS */

I think that it is not necessary. But, if you think it is required,
Please add the detailed comment with consistent comment style.

> +	*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));

If you use the separate variable for getting the value from dev_pm_qos_read_value(),
you can add this line under 80 char. If there are any special reason,
I prefer to keep the line under 80 char.


> +
>  	/* constraints from sysfs */
>  	*min_freq = max(*min_freq, devfreq->min_freq);
>  	*max_freq = min(*max_freq, devfreq->max_freq);
>  
>  	/* constraints from OPP interface */
> @@ -605,10 +614,53 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>  	mutex_unlock(&devfreq->lock);
>  
>  	return ret;
>  }
>  
> +/**
> + * devfreq_qos_notifier_call() - Common handler for QoS constraints.
> + * @devfreq:    the devfreq instance.
> + */
> +static int devfreq_qos_notifier_call(struct devfreq *devfreq)

Also, as I commented on patch4, we better to remove 'devfreq' prefix
for internal function as following:
	devfreq_qos_notifier_call -> qos_notifier_call

> +{
> +	int err;
> +
> +	mutex_lock(&devfreq->lock);
> +	err = update_devfreq(devfreq);
> +	mutex_unlock(&devfreq->lock);
> +	if (err)
> +		dev_err(&devfreq->dev, "dvfs for QoS constraints"
> +				" failed with (%d) error\n", err);

'dvfs' is not full name. Also, the capital letter is more correct.
But, the devfreq used 'failed to ...' comment style on some points.

I suggest the comment as following:

--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -628,8 +628,8 @@ static int devfreq_qos_notifier_call(struct devfreq *devfreq)
        err = update_devfreq(devfreq);
        mutex_unlock(&devfreq->lock);
        if (err)
-               dev_err(&devfreq->dev, "dvfs for QoS constraints"
-                               " failed with (%d) error\n", err);
+               dev_err(&devfreq->dev,
+                       "failed to update frequency with PMQoS (%d)\n", err);
 


> +
> +	/* QoS is best effort - let all notifiers run on error */

Please remove it.

> +	return NOTIFY_OK;
> +}
> +
> +/**
> + * devfreq_qos_min_notifier_call() - Callback for QoS min_freq changes.
> + * @nb:		Should be devfreq->nb_min
> + */
> +static int devfreq_qos_min_notifier_call(struct notifier_block *nb,

ditto.
	devfreq_qos_min_notifier_call -> qos_min_notifier_call

> +					 unsigned long val, void *ptr)
> +{
> +	struct devfreq *devfreq = container_of(nb, struct devfreq, nb_min);
> +

nitpick. You can remove this line.

> +	return devfreq_qos_notifier_call(devfreq);
> +}
> +
> +/**
> + * devfreq_qos_max_notifier_call() - Callback for QoS max_freq changes.
> + * @nb:		Should be devfreq->nb_max
> + */
> +static int devfreq_qos_max_notifier_call(struct notifier_block *nb,

ditto.
	devfreq_qos_max_notifier_call -> qos_max_notifier_call


> +					 unsigned long val, void *ptr)
> +{
> +	struct devfreq *devfreq = container_of(nb, struct devfreq, nb_max);
> +

nitpick. You can remove this line.

> +	return devfreq_qos_notifier_call(devfreq);
> +}
> +
>  /**
>   * devfreq_dev_release() - Callback for struct device to release the device.
>   * @dev:	the devfreq device
>   *
>   * Remove devfreq from the list and release its resources.
> @@ -619,10 +671,15 @@ static void devfreq_dev_release(struct device *dev)
>  
>  	mutex_lock(&devfreq_list_lock);
>  	list_del(&devfreq->node);
>  	mutex_unlock(&devfreq_list_lock);
>  
> +	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
> +			DEV_PM_QOS_MAX_FREQUENCY);
> +	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
> +			DEV_PM_QOS_MIN_FREQUENCY);

Even if devfreq_dev_release() is called at the end of device driver,
dev_pm_remove_notifier() have returned the value. Need to check
the return value for checking the error state.

> +
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
> @@ -732,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	if (err) {
>  		put_device(&devfreq->dev);
>  		goto err_out;
>  	}
>  
> +	/*
> +	 * Register notifiers for updates to min/max_freq after device is
> +	 * initialized (and we can handle notifications) but before the
> +	 * governor is started (which should do an initial enforcement of
> +	 * constraints).> +	 */

In the devfreq_add_device(), each step has not contained
the detailed comment. If possible, in order to keep the existing style,
please remove it. 

> +	devfreq->nb_min.notifier_call = devfreq_qos_min_notifier_call;
> +	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
> +				      DEV_PM_QOS_MIN_FREQUENCY);
> +	if (err)
> +		goto err_devfreq;
> +
> +	devfreq->nb_max.notifier_call = devfreq_qos_max_notifier_call;
> +	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max,
> +				      DEV_PM_QOS_MAX_FREQUENCY);
> +	if (err)
> +		goto err_devfreq;
> +
>  	mutex_lock(&devfreq_list_lock);
>  
>  	governor = try_then_request_governor(devfreq->governor_name);
>  	if (IS_ERR(governor)) {
>  		dev_err(dev, "%s: Unable to find governor for the device\n",
> @@ -759,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  
>  	return devfreq;
>  
>  err_init:
>  	mutex_unlock(&devfreq_list_lock);
> +err_devfreq:
>  	devfreq_remove_device(devfreq);
>  	return ERR_PTR(err);
>  
>  err_dev:
>  	/*
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 2bae9ed3c783..8b92ccbd1962 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -134,10 +134,12 @@ struct devfreq_dev_profile {
>   * @total_trans:	Number of devfreq transitions
>   * @trans_table:	Statistics of devfreq transitions
>   * @time_in_state:	Statistics of devfreq states
>   * @last_stat_updated:	The last time stat updated
>   * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier
> + * @nb_min:		Notifier block for DEV_PM_QOS_MIN_FREQUENCY
> + * @nb_max:		Notifier block for DEV_PM_QOS_MAX_FREQUENCY
>   *
>   * This structure stores the devfreq information for a give device.
>   *
>   * Note that when a governor accesses entries in struct devfreq in its
>   * functions except for the context of callbacks defined in struct
> @@ -176,10 +178,13 @@ struct devfreq {
>  	unsigned int *trans_table;
>  	unsigned long *time_in_state;
>  	unsigned long last_stat_updated;
>  
>  	struct srcu_notifier_head transition_notifier_list;
> +
> +	struct notifier_block nb_min;
> +	struct notifier_block nb_max;
>  };
>  
>  struct devfreq_freqs {
>  	unsigned long old;
>  	unsigned long new;
>
Leonard Crestez Sept. 24, 2019, 9:27 a.m. UTC | #3
On 2019-09-24 9:47 AM, Chanwoo Choi wrote:
> Hi,
> 
> On 19. 9. 24. 오전 6:10, Leonard Crestez wrote:
>> Register notifiers with the PM QoS framework in order to respond to
>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY.
>>
>> No notifiers are added by this patch but PM QoS constraints can be
>> imposed externally (for example from other devices).
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   drivers/devfreq/devfreq.c | 76 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/devfreq.h   |  5 +++
>>   2 files changed, 81 insertions(+)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 7f152a582e78..9887408f23bb 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -22,17 +22,20 @@
>>   #include <linux/platform_device.h>
>>   #include <linux/list.h>
>>   #include <linux/printk.h>
>>   #include <linux/hrtimer.h>
>>   #include <linux/of.h>
>> +#include <linux/pm_qos.h>
>>   #include "governor.h"
>>   
>>   #define HZ_PER_KHZ 1000
> 
> You have to remove it on patch4.

Done. This was added twice, probably messed up during rebasing.

>>   /*
>>    * devfreq core provides delayed work based load monitoring helper
>>    * functions. Governors can use these or can implement their own
>> @@ -123,10 +126,16 @@ static void devfreq_get_freq_range(struct devfreq *devfreq,
>>   	} else {
>>   		*min_freq = freq_table[devfreq->profile->max_state - 1];
>>   		*max_freq = freq_table[0];
>>   	}
>>   
>> +	/* constraints from PM QoS */
> 
> I think that it is not necessary. But, if you think it is required,
> Please add the detailed comment with consistent comment style.

Not sure what comment style you mean, it seems like a reasonable 
single-line comment.
>> +	*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));
> 
> If you use the separate variable for getting the value from dev_pm_qos_read_value(),
> you can add this line under 80 char. If there are any special reason,
> I prefer to keep the line under 80 char.

OK

>>   	/* constraints from sysfs */
>>   	*min_freq = max(*min_freq, devfreq->min_freq);
>>   	*max_freq = min(*max_freq, devfreq->max_freq);
>>   
>>   	/* constraints from OPP interface */
>> @@ -605,10 +614,53 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>>   	mutex_unlock(&devfreq->lock);
>>   
>>   	return ret;
>>   }
>>   
>> +/**
>> + * devfreq_qos_notifier_call() - Common handler for QoS constraints.
>> + * @devfreq:    the devfreq instance.
>> + */
>> +static int devfreq_qos_notifier_call(struct devfreq *devfreq)
> 
> Also, as I commented on patch4, we better to remove 'devfreq' prefix
> for internal function as following:
> 	devfreq_qos_notifier_call -> qos_notifier_call

OK

>> +{
>> +	int err;
>> +
>> +	mutex_lock(&devfreq->lock);
>> +	err = update_devfreq(devfreq);
>> +	mutex_unlock(&devfreq->lock);
>> +	if (err)
>> +		dev_err(&devfreq->dev, "dvfs for QoS constraints"
>> +				" failed with (%d) error\n", err);
> 
> 'dvfs' is not full name. Also, the capital letter is more correct.
> But, the devfreq used 'failed to ...' comment style on some points.
> 
> I suggest the comment as following:
> 
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -628,8 +628,8 @@ static int devfreq_qos_notifier_call(struct devfreq *devfreq)
>          err = update_devfreq(devfreq);
>          mutex_unlock(&devfreq->lock);
>          if (err)
> -               dev_err(&devfreq->dev, "dvfs for QoS constraints"
> -                               " failed with (%d) error\n", err);
> +               dev_err(&devfreq->dev,
> +                       "failed to update frequency with PMQoS (%d)\n", err);

I tried to mirror comment in monitor_devfreq, I'll adjust. The dev_name 
of devfreq->dev is usually something unhelpful like "devfreq1" so maybe 
&devfreq->dev.parent should be passed instead.
> 
>> +
>> +	/* QoS is best effort - let all notifiers run on error */
> 
> Please remove it.

OK but an explanation as to why errors are not propagated seems worth 
keeping.

>> +	return NOTIFY_OK;
>> +}
>> +
>> +/**
>> + * devfreq_qos_min_notifier_call() - Callback for QoS min_freq changes.
>> + * @nb:		Should be devfreq->nb_min
>> + */
>> +static int devfreq_qos_min_notifier_call(struct notifier_block *nb,
> 
> ditto.
> 	devfreq_qos_min_notifier_call -> qos_min_notifier_call
> 
>> +					 unsigned long val, void *ptr)
>> +{
>> +	struct devfreq *devfreq = container_of(nb, struct devfreq, nb_min);
>> +
> 
> nitpick. You can remove this line.

OK, if the devfreq_ prefix is removed this can be under 80 chars.

>> @@ -619,10 +671,15 @@ static void devfreq_dev_release(struct device *dev)
>>   
>>   	mutex_lock(&devfreq_list_lock);
>>   	list_del(&devfreq->node);
>>   	mutex_unlock(&devfreq_list_lock);
>>   
>> +	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
>> +			DEV_PM_QOS_MAX_FREQUENCY);
>> +	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
>> +			DEV_PM_QOS_MIN_FREQUENCY);
> 
> Even if devfreq_dev_release() is called at the end of device driver,
> dev_pm_remove_notifier() have returned the value. Need to check
> the return value for checking the error state.

And if dev_pm_qos_remove_notifier returns an error what next?

In cleanup functions there is not much you can do on error other than to 
continue cleaning up. This is why they usually return void.

>>   	if (devfreq->profile->exit)
>>   		devfreq->profile->exit(devfreq->dev.parent);
>>   
>>   	kfree(devfreq->time_in_state);
>>   	kfree(devfreq->trans_table);
>> @@ -732,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   	if (err) {
>>   		put_device(&devfreq->dev);
>>   		goto err_out;
>>   	}
>>   
>> +	/*
>> +	 * Register notifiers for updates to min/max_freq after device is
>> +	 * initialized (and we can handle notifications) but before the
>> +	 * governor is started (which should do an initial enforcement of
>> +	 * constraints).> +	 */
> 
> In the devfreq_add_device(), each step has not contained
> the detailed comment. If possible, in order to keep the existing style,
> please remove it.

I can remove it but the comment explains why this is the right step at 
which notifiers should be registered so it seems useful.

>> +	devfreq->nb_min.notifier_call = devfreq_qos_min_notifier_call;
>> +	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
>> +				      DEV_PM_QOS_MIN_FREQUENCY);
>> +	if (err)
>> +		goto err_devfreq;
>> +
>> +	devfreq->nb_max.notifier_call = devfreq_qos_max_notifier_call;
>> +	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max,
>> +				      DEV_PM_QOS_MAX_FREQUENCY);
>> +	if (err)
>> +		goto err_devfreq;
>> +
>>   	mutex_lock(&devfreq_list_lock);
>>   
>>   	governor = try_then_request_governor(devfreq->governor_name);
>>   	if (IS_ERR(governor)) {
>>   		dev_err(dev, "%s: Unable to find governor for the device\n",
>> @@ -759,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   
>>   	return devfreq;
>>   
>>   err_init:
>>   	mutex_unlock(&devfreq_list_lock);
>> +err_devfreq:
>>   	devfreq_remove_device(devfreq);
>>   	return ERR_PTR(err);
>>   
>>   err_dev:
>>   	/*
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index 2bae9ed3c783..8b92ccbd1962 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -134,10 +134,12 @@ struct devfreq_dev_profile {
>>    * @total_trans:	Number of devfreq transitions
>>    * @trans_table:	Statistics of devfreq transitions
>>    * @time_in_state:	Statistics of devfreq states
>>    * @last_stat_updated:	The last time stat updated
>>    * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier
>> + * @nb_min:		Notifier block for DEV_PM_QOS_MIN_FREQUENCY
>> + * @nb_max:		Notifier block for DEV_PM_QOS_MAX_FREQUENCY
>>    *
>>    * This structure stores the devfreq information for a give device.
>>    *
>>    * Note that when a governor accesses entries in struct devfreq in its
>>    * functions except for the context of callbacks defined in struct
>> @@ -176,10 +178,13 @@ struct devfreq {
>>   	unsigned int *trans_table;
>>   	unsigned long *time_in_state;
>>   	unsigned long last_stat_updated;
>>   
>>   	struct srcu_notifier_head transition_notifier_list;
>> +
>> +	struct notifier_block nb_min;
>> +	struct notifier_block nb_max;
>>   };
>>   
>>   struct devfreq_freqs {
>>   	unsigned long old;
>>   	unsigned long new;
>>
> 
>

Patch
diff mbox series

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 7f152a582e78..9887408f23bb 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -22,17 +22,20 @@ 
 #include <linux/platform_device.h>
 #include <linux/list.h>
 #include <linux/printk.h>
 #include <linux/hrtimer.h>
 #include <linux/of.h>
+#include <linux/pm_qos.h>
 #include "governor.h"
 
 #define HZ_PER_KHZ 1000
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/devfreq.h>
 
+#define HZ_PER_KHZ	1000
+
 static struct class *devfreq_class;
 
 /*
  * devfreq core provides delayed work based load monitoring helper
  * functions. Governors can use these or can implement their own
@@ -123,10 +126,16 @@  static void devfreq_get_freq_range(struct devfreq *devfreq,
 	} else {
 		*min_freq = freq_table[devfreq->profile->max_state - 1];
 		*max_freq = freq_table[0];
 	}
 
+	/* constraints from PM QoS */
+	*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 */
@@ -605,10 +614,53 @@  static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
 	mutex_unlock(&devfreq->lock);
 
 	return ret;
 }
 
+/**
+ * devfreq_qos_notifier_call() - Common handler for QoS constraints.
+ * @devfreq:    the devfreq instance.
+ */
+static int devfreq_qos_notifier_call(struct devfreq *devfreq)
+{
+	int err;
+
+	mutex_lock(&devfreq->lock);
+	err = update_devfreq(devfreq);
+	mutex_unlock(&devfreq->lock);
+	if (err)
+		dev_err(&devfreq->dev, "dvfs for QoS constraints"
+				" failed with (%d) error\n", err);
+
+	/* QoS is best effort - let all notifiers run on error */
+	return NOTIFY_OK;
+}
+
+/**
+ * devfreq_qos_min_notifier_call() - Callback for QoS min_freq changes.
+ * @nb:		Should be devfreq->nb_min
+ */
+static int devfreq_qos_min_notifier_call(struct notifier_block *nb,
+					 unsigned long val, void *ptr)
+{
+	struct devfreq *devfreq = container_of(nb, struct devfreq, nb_min);
+
+	return devfreq_qos_notifier_call(devfreq);
+}
+
+/**
+ * devfreq_qos_max_notifier_call() - Callback for QoS max_freq changes.
+ * @nb:		Should be devfreq->nb_max
+ */
+static int devfreq_qos_max_notifier_call(struct notifier_block *nb,
+					 unsigned long val, void *ptr)
+{
+	struct devfreq *devfreq = container_of(nb, struct devfreq, nb_max);
+
+	return devfreq_qos_notifier_call(devfreq);
+}
+
 /**
  * devfreq_dev_release() - Callback for struct device to release the device.
  * @dev:	the devfreq device
  *
  * Remove devfreq from the list and release its resources.
@@ -619,10 +671,15 @@  static void devfreq_dev_release(struct device *dev)
 
 	mutex_lock(&devfreq_list_lock);
 	list_del(&devfreq->node);
 	mutex_unlock(&devfreq_list_lock);
 
+	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
+			DEV_PM_QOS_MAX_FREQUENCY);
+	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
+			DEV_PM_QOS_MIN_FREQUENCY);
+
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
 	kfree(devfreq->time_in_state);
 	kfree(devfreq->trans_table);
@@ -732,10 +789,28 @@  struct devfreq *devfreq_add_device(struct device *dev,
 	if (err) {
 		put_device(&devfreq->dev);
 		goto err_out;
 	}
 
+	/*
+	 * Register notifiers for updates to min/max_freq after device is
+	 * initialized (and we can handle notifications) but before the
+	 * governor is started (which should do an initial enforcement of
+	 * constraints).
+	 */
+	devfreq->nb_min.notifier_call = devfreq_qos_min_notifier_call;
+	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
+				      DEV_PM_QOS_MIN_FREQUENCY);
+	if (err)
+		goto err_devfreq;
+
+	devfreq->nb_max.notifier_call = devfreq_qos_max_notifier_call;
+	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max,
+				      DEV_PM_QOS_MAX_FREQUENCY);
+	if (err)
+		goto err_devfreq;
+
 	mutex_lock(&devfreq_list_lock);
 
 	governor = try_then_request_governor(devfreq->governor_name);
 	if (IS_ERR(governor)) {
 		dev_err(dev, "%s: Unable to find governor for the device\n",
@@ -759,10 +834,11 @@  struct devfreq *devfreq_add_device(struct device *dev,
 
 	return devfreq;
 
 err_init:
 	mutex_unlock(&devfreq_list_lock);
+err_devfreq:
 	devfreq_remove_device(devfreq);
 	return ERR_PTR(err);
 
 err_dev:
 	/*
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 2bae9ed3c783..8b92ccbd1962 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -134,10 +134,12 @@  struct devfreq_dev_profile {
  * @total_trans:	Number of devfreq transitions
  * @trans_table:	Statistics of devfreq transitions
  * @time_in_state:	Statistics of devfreq states
  * @last_stat_updated:	The last time stat updated
  * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier
+ * @nb_min:		Notifier block for DEV_PM_QOS_MIN_FREQUENCY
+ * @nb_max:		Notifier block for DEV_PM_QOS_MAX_FREQUENCY
  *
  * This structure stores the devfreq information for a give device.
  *
  * Note that when a governor accesses entries in struct devfreq in its
  * functions except for the context of callbacks defined in struct
@@ -176,10 +178,13 @@  struct devfreq {
 	unsigned int *trans_table;
 	unsigned long *time_in_state;
 	unsigned long last_stat_updated;
 
 	struct srcu_notifier_head transition_notifier_list;
+
+	struct notifier_block nb_min;
+	struct notifier_block nb_max;
 };
 
 struct devfreq_freqs {
 	unsigned long old;
 	unsigned long new;