| Message ID | e9868310f9543b4f4a6c7bbe5d4d015da9a0e71d.1569272883.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / devfreq: Add dev_pm_qos support | expand |
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>
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; >
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; >> > >
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;
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(+)