| Message ID | 3eb8a1d1e7d738f7599d54348d6b474196581693.1569319738.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / devfreq: Add dev_pm_qos support | expand |
On 19. 9. 24. 오후 7:11, 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 | 46 ++++++++++++++++++++++++--------------- > include/linux/devfreq.h | 9 ++++---- > 2 files changed, 33 insertions(+), 22 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 784f3e40536a..8bb7efd821ab 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -137,14 +137,10 @@ static void get_freq_range(struct devfreq *devfreq, > qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, > DEV_PM_QOS_MIN_FREQUENCY); > *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); > *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); > > - /* 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); Please check the return value if error happen, just print the err with dev_err() without stopping the release steps. > kfree(devfreq->time_in_state); > kfree(devfreq->trans_table); > mutex_destroy(&devfreq->lock); > kfree(devfreq); > } > @@ -747,18 +745,25 @@ 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; > + > + 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 +848,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); Please check the return value if error happen, just print the err with dev_err() without stopping the release steps. dev_err(... "failed to remove request of DEV_PM_QOS_MAX_FREQUENCY\n"); > + if (dev_pm_qos_request_active(&devfreq->user_min_freq_req)) > + dev_pm_qos_remove_request(&devfreq->user_min_freq_req); dev_err(... "failed to remove request of DEV_PM_QOS_MIN_FREQUENCY\n"); > kfree(devfreq->time_in_state); > kfree(devfreq->trans_table); > kfree(devfreq); > err_out: > return ERR_PTR(err); > @@ -1407,14 +1416,15 @@ 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 PM QoS */ I prefer more detailed description as following: /* * Round down to KHz to decide the proper minimum frequency * which is closed to user request. */ > + 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, > @@ -1439,19 +1449,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 PM QoS and interpret zero as "don't care" */ I think that "don't care" comment style is not good. I referred to the Documentation/ABI/testing/sysfs-class-devfreq file. I prefer more detailed description as following: /* * Round up to KHz to decide the proper maximum frequency * which is closed to user request. If value is zero, * the user does not 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 dac0dffeabb4..7849fe4c666d 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) min -> minimum and then remove parenthesis as following: PM QoS minimum frequency request by user via sysfs > + * @user_max_freq_req: PM QoS max frequency request from user (via sysfs) ditto. max -> maximum 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; > unsigned long scaling_min_freq; > unsigned long scaling_max_freq; > bool stop_polling; > > unsigned long suspend_freq; >
On Wed, Sep 25, 2019 at 11:41:07AM +0900, Chanwoo Choi wrote: > On 19. 9. 24. 오후 7:11, 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 | 46 ++++++++++++++++++++++++--------------- > > include/linux/devfreq.h | 9 ++++---- > > 2 files changed, 33 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > > index 784f3e40536a..8bb7efd821ab 100644 > > --- a/drivers/devfreq/devfreq.c > > +++ b/drivers/devfreq/devfreq.c > > @@ -137,14 +137,10 @@ static void get_freq_range(struct devfreq *devfreq, > > qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, > > DEV_PM_QOS_MIN_FREQUENCY); > > *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); > > *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); > > > > - /* 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); > > Please check the return value if error happen, just print the err with dev_err() > without stopping the release steps. I wonder if dev_warn() would be more appropriate, since the current operation is not aborted. > > kfree(devfreq->time_in_state); > > kfree(devfreq->trans_table); > > mutex_destroy(&devfreq->lock); > > kfree(devfreq); > > } > > @@ -747,18 +745,25 @@ 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; > > + > > + 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 +848,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); > > Please check the return value if error happen, just print the err with dev_err() > without stopping the release steps. > > dev_err(... "failed to remove request of DEV_PM_QOS_MAX_FREQUENCY\n"); dev_warn() for the same reason as above? I think the message would be better with a slight change: "failed to remove DEV_PM_QOS_MAX_FREQUENCY request\n" > > > + if (dev_pm_qos_request_active(&devfreq->user_min_freq_req)) > > + dev_pm_qos_remove_request(&devfreq->user_min_freq_req); > > dev_err(... "failed to remove request of DEV_PM_QOS_MIN_FREQUENCY\n"); ditto > > kfree(devfreq->time_in_state); > > kfree(devfreq->trans_table); > > kfree(devfreq); > > err_out: > > return ERR_PTR(err); > > @@ -1407,14 +1416,15 @@ 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 PM QoS */ > > I prefer more detailed description as following: > > /* > * Round down to KHz to decide the proper minimum frequency it should be kHz, with a lower-case 'k', as in the original comment. > * which is closed to user request. > */ The comment you suggest doesn't provide any information about why the conversion to kHz is done, in this sense the original comment that mentions PM QoS provides more value. With whatever we end up, I suggest to use 'convert' instead of 'round down'. > > + 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, > > @@ -1439,19 +1449,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 PM QoS and interpret zero as "don't care" */ > > I think that "don't care" comment style is not good. > > I referred to the Documentation/ABI/testing/sysfs-class-devfreq file. > I prefer more detailed description as following: > /* > * Round up to KHz to decide the proper maximum frequency kHz > * which is closed to user request. If value is zero, > * the user does not care. "the user does not care" is still very casual you didn't like initially. How about "A value of zero is interpreted as 'no limit'."? As for the min freq, I think PM QoS should be mentioned to make clear why the conversion to kHz is needed. > */ > > > > + 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 dac0dffeabb4..7849fe4c666d 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) > > min -> minimum and then remove parenthesis as following: > PM QoS minimum frequency request by user via sysfs > > > + * @user_max_freq_req: PM QoS max frequency request from user (via sysfs) > > ditto. max -> maximum > 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; > > unsigned long scaling_min_freq; > > unsigned long scaling_max_freq; > > bool stop_polling; > > > > unsigned long suspend_freq; > > > > > -- > Best Regards, > Chanwoo Choi > Samsung Electronics
On 25.09.2019 05:36, Chanwoo Choi wrote: > On 19. 9. 24. 오후 7:11, 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 | 46 ++++++++++++++++++++++++--------------- >> include/linux/devfreq.h | 9 ++++---- >> 2 files changed, 33 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 784f3e40536a..8bb7efd821ab 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -137,14 +137,10 @@ static void get_freq_range(struct devfreq *devfreq, >> qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >> DEV_PM_QOS_MIN_FREQUENCY); >> *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); >> *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); >> >> - /* 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); > > Please check the return value if error happen, just print the err with dev_err() > without stopping the release steps. OK, will print errors >> kfree(devfreq->time_in_state); >> kfree(devfreq->trans_table); >> mutex_destroy(&devfreq->lock); >> kfree(devfreq); >> } >> @@ -747,18 +745,25 @@ 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; >> + >> + 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 +848,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); > > Please check the return value if error happen, just print the err with dev_err() > without stopping the release steps. OK, will print errors > > dev_err(... "failed to remove request of DEV_PM_QOS_MAX_FREQUENCY\n"); > >> + if (dev_pm_qos_request_active(&devfreq->user_min_freq_req)) >> + dev_pm_qos_remove_request(&devfreq->user_min_freq_req); > > dev_err(... "failed to remove request of DEV_PM_QOS_MIN_FREQUENCY\n"); > >> kfree(devfreq->time_in_state); >> kfree(devfreq->trans_table); >> kfree(devfreq); >> err_out: >> return ERR_PTR(err); >> @@ -1407,14 +1416,15 @@ 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 PM QoS */ > > I prefer more detailed description as following: > > /* > * Round down to KHz to decide the proper minimum frequency > * which is closed to user request. > */ > > >> + 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, >> @@ -1439,19 +1449,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 PM QoS and interpret zero as "don't care" */ > > I think that "don't care" comment style is not good. > > I referred to the Documentation/ABI/testing/sysfs-class-devfreq file. > I prefer more detailed description as following: > /* > * Round up to KHz to decide the proper maximum frequency > * which is closed to user request. If value is zero, > * the user does not care. > */ OK, will update this comment >> + 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 dac0dffeabb4..7849fe4c666d 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) > > min -> minimum and then remove parenthesis as following: > PM QoS minimum frequency request by user via sysfs > >> + * @user_max_freq_req: PM QoS max frequency request from user (via sysfs) > > ditto. max -> maximum > PM QoS maximum frequency request by user via sysfs OK >> * @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; >> > >
On 19. 9. 26. 오전 1:45, Matthias Kaehlcke wrote: > On Wed, Sep 25, 2019 at 11:41:07AM +0900, Chanwoo Choi wrote: >> On 19. 9. 24. 오후 7:11, 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 | 46 ++++++++++++++++++++++++--------------- >>> include/linux/devfreq.h | 9 ++++---- >>> 2 files changed, 33 insertions(+), 22 deletions(-) >>> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>> index 784f3e40536a..8bb7efd821ab 100644 >>> --- a/drivers/devfreq/devfreq.c >>> +++ b/drivers/devfreq/devfreq.c >>> @@ -137,14 +137,10 @@ static void get_freq_range(struct devfreq *devfreq, >>> qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>> DEV_PM_QOS_MIN_FREQUENCY); >>> *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); >>> *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); >>> >>> - /* 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); >> >> Please check the return value if error happen, just print the err with dev_err() >> without stopping the release steps. > > I wonder if dev_warn() would be more appropriate, since the current operation > is not aborted. > >>> kfree(devfreq->time_in_state); >>> kfree(devfreq->trans_table); >>> mutex_destroy(&devfreq->lock); >>> kfree(devfreq); >>> } >>> @@ -747,18 +745,25 @@ 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; >>> + >>> + 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 +848,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); >> >> Please check the return value if error happen, just print the err with dev_err() >> without stopping the release steps. >> >> dev_err(... "failed to remove request of DEV_PM_QOS_MAX_FREQUENCY\n"); > > dev_warn() for the same reason as above? Actually, I think that is not critical error but need to print the error So, I think that dev_err() is enough. If this thing is critical, better to use dev_warn. > > I think the message would be better with a slight change: > > "failed to remove DEV_PM_QOS_MAX_FREQUENCY request\n" OK. > >> >>> + if (dev_pm_qos_request_active(&devfreq->user_min_freq_req)) >>> + dev_pm_qos_remove_request(&devfreq->user_min_freq_req); >> >> dev_err(... "failed to remove request of DEV_PM_QOS_MIN_FREQUENCY\n"); > > ditto > >>> kfree(devfreq->time_in_state); >>> kfree(devfreq->trans_table); >>> kfree(devfreq); >>> err_out: >>> return ERR_PTR(err); >>> @@ -1407,14 +1416,15 @@ 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 PM QoS */ >> >> I prefer more detailed description as following: >> >> /* >> * Round down to KHz to decide the proper minimum frequency > > it should be kHz, with a lower-case 'k', as in the original comment. Good. > >> * which is closed to user request. >> */ > > The comment you suggest doesn't provide any information about why the > conversion to kHz is done, in this sense the original comment that > mentions PM QoS provides more value. > > With whatever we end up, I suggest to use 'convert' instead of > 'round down'. I agree to use 'convert' instead of 'round down' if some expression indicates the correct meaning. > >>> + 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, >>> @@ -1439,19 +1449,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 PM QoS and interpret zero as "don't care" */ >> >> I think that "don't care" comment style is not good. >> >> I referred to the Documentation/ABI/testing/sysfs-class-devfreq file. >> I prefer more detailed description as following: >> /* >> * Round up to KHz to decide the proper maximum frequency > > kHz > >> * which is closed to user request. If value is zero, >> * the user does not care. > > "the user does not care" is still very casual you didn't like initially. > How about "A value of zero is interpreted as 'no limit'."? > > As for the min freq, I think PM QoS should be mentioned to make clear why > the conversion to kHz is needed. Agree. I expect that Leonard will mention that. > >> */ >> >> >>> + 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 dac0dffeabb4..7849fe4c666d 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) >> >> min -> minimum and then remove parenthesis as following: >> PM QoS minimum frequency request by user via sysfs >> >>> + * @user_max_freq_req: PM QoS max frequency request from user (via sysfs) >> >> ditto. max -> maximum >> 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; >>> unsigned long scaling_min_freq; >>> unsigned long scaling_max_freq; >>> bool stop_polling; >>> >>> unsigned long suspend_freq; >>> >> >> >> -- >> Best Regards, >> Chanwoo Choi >> Samsung Electronics > >
On 19. 9. 26. 오전 7:11, Leonard Crestez wrote: > On 25.09.2019 05:36, Chanwoo Choi wrote: >> On 19. 9. 24. 오후 7:11, 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 | 46 ++++++++++++++++++++++++--------------- >>> include/linux/devfreq.h | 9 ++++---- >>> 2 files changed, 33 insertions(+), 22 deletions(-) >>> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>> index 784f3e40536a..8bb7efd821ab 100644 >>> --- a/drivers/devfreq/devfreq.c >>> +++ b/drivers/devfreq/devfreq.c >>> @@ -137,14 +137,10 @@ static void get_freq_range(struct devfreq *devfreq, >>> qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>> DEV_PM_QOS_MIN_FREQUENCY); >>> *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); >>> *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); >>> >>> - /* 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); >> >> Please check the return value if error happen, just print the err with dev_err() >> without stopping the release steps. > > OK, will print errors > >>> kfree(devfreq->time_in_state); >>> kfree(devfreq->trans_table); >>> mutex_destroy(&devfreq->lock); >>> kfree(devfreq); >>> } >>> @@ -747,18 +745,25 @@ 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; >>> + >>> + 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 +848,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); >> >> Please check the return value if error happen, just print the err with dev_err() >> without stopping the release steps. > > OK, will print errors > >> >> dev_err(... "failed to remove request of DEV_PM_QOS_MAX_FREQUENCY\n"); >> >>> + if (dev_pm_qos_request_active(&devfreq->user_min_freq_req)) >>> + dev_pm_qos_remove_request(&devfreq->user_min_freq_req); >> >> dev_err(... "failed to remove request of DEV_PM_QOS_MIN_FREQUENCY\n"); >> >>> kfree(devfreq->time_in_state); >>> kfree(devfreq->trans_table); >>> kfree(devfreq); >>> err_out: >>> return ERR_PTR(err); >>> @@ -1407,14 +1416,15 @@ 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 PM QoS */ >> >> I prefer more detailed description as following: >> >> /* >> * Round down to KHz to decide the proper minimum frequency >> * which is closed to user request. >> */ How about this comment? and with Matthias comment on other reply thread. >> >> >>> + 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, >>> @@ -1439,19 +1449,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 PM QoS and interpret zero as "don't care" */ >> >> I think that "don't care" comment style is not good. >> >> I referred to the Documentation/ABI/testing/sysfs-class-devfreq file. >> I prefer more detailed description as following: >> /* >> * Round up to KHz to decide the proper maximum frequency >> * which is closed to user request. If value is zero, >> * the user does not care. >> */ > > OK, will update this comment > >>> + 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 dac0dffeabb4..7849fe4c666d 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) >> >> min -> minimum and then remove parenthesis as following: >> PM QoS minimum frequency request by user via sysfs >> >>> + * @user_max_freq_req: PM QoS max frequency request from user (via sysfs) >> >> ditto. max -> maximum >> PM QoS maximum frequency request by user via sysfs > > OK Thanks. > >>> * @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; >>> >> >> >
On Thu, Sep 26, 2019 at 10:25:41AM +0900, Chanwoo Choi wrote: > On 19. 9. 26. 오전 1:45, Matthias Kaehlcke wrote: > > On Wed, Sep 25, 2019 at 11:41:07AM +0900, Chanwoo Choi wrote: > >> On 19. 9. 24. 오후 7:11, 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 | 46 ++++++++++++++++++++++++--------------- > >>> include/linux/devfreq.h | 9 ++++---- > >>> 2 files changed, 33 insertions(+), 22 deletions(-) > >>> > >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > >>> index 784f3e40536a..8bb7efd821ab 100644 > >>> --- a/drivers/devfreq/devfreq.c > >>> +++ b/drivers/devfreq/devfreq.c > >>> @@ -137,14 +137,10 @@ static void get_freq_range(struct devfreq *devfreq, > >>> qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, > >>> DEV_PM_QOS_MIN_FREQUENCY); > >>> *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); > >>> *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); > >>> > >>> - /* 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); > >> > >> Please check the return value if error happen, just print the err with dev_err() > >> without stopping the release steps. > > > > I wonder if dev_warn() would be more appropriate, since the current operation > > is not aborted. > > > >>> kfree(devfreq->time_in_state); > >>> kfree(devfreq->trans_table); > >>> mutex_destroy(&devfreq->lock); > >>> kfree(devfreq); > >>> } > >>> @@ -747,18 +745,25 @@ 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; > >>> + > >>> + 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 +848,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); > >> > >> Please check the return value if error happen, just print the err with dev_err() > >> without stopping the release steps. > >> > >> dev_err(... "failed to remove request of DEV_PM_QOS_MAX_FREQUENCY\n"); > > > > dev_warn() for the same reason as above? > > Actually, I think that is not critical error but need to print the error > So, I think that dev_err() is enough. If this thing is critical, > better to use dev_warn. We agree that this is not critical, but not when to use dev_err() and dev_warn(). You seem to imply that dev_warn() is more severe than dev_err(), however it's the other way around: #define KERN_ERR KERN_SOH "3" /* error conditions */ #define KERN_WARNING KERN_SOH "4" /* warning conditions */ #define KERN_NOTICE KERN_SOH "5" /* normal but significant condition */ I think in this case you can go either way, it's certainly an error, but the driver continues with the normal execution path. > > > > I think the message would be better with a slight change: > > > > "failed to remove DEV_PM_QOS_MAX_FREQUENCY request\n" > > OK. > > > > >> > >>> + if (dev_pm_qos_request_active(&devfreq->user_min_freq_req)) > >>> + dev_pm_qos_remove_request(&devfreq->user_min_freq_req); > >> > >> dev_err(... "failed to remove request of DEV_PM_QOS_MIN_FREQUENCY\n"); > > > > ditto > > > >>> kfree(devfreq->time_in_state); > >>> kfree(devfreq->trans_table); > >>> kfree(devfreq); > >>> err_out: > >>> return ERR_PTR(err); > >>> @@ -1407,14 +1416,15 @@ 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 PM QoS */ > >> > >> I prefer more detailed description as following: > >> > >> /* > >> * Round down to KHz to decide the proper minimum frequency > > > > it should be kHz, with a lower-case 'k', as in the original comment. > > Good. > > > > >> * which is closed to user request. > >> */ > > > > The comment you suggest doesn't provide any information about why the > > conversion to kHz is done, in this sense the original comment that > > mentions PM QoS provides more value. > > > > With whatever we end up, I suggest to use 'convert' instead of > > 'round down'. > > I agree to use 'convert' instead of 'round down' > if some expression indicates the correct meaning. > > > > >>> + 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, > >>> @@ -1439,19 +1449,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 PM QoS and interpret zero as "don't care" */ > >> > >> I think that "don't care" comment style is not good. > >> > >> I referred to the Documentation/ABI/testing/sysfs-class-devfreq file. > >> I prefer more detailed description as following: > >> /* > >> * Round up to KHz to decide the proper maximum frequency > > > > kHz > > > >> * which is closed to user request. If value is zero, > >> * the user does not care. > > > > "the user does not care" is still very casual you didn't like initially. > > How about "A value of zero is interpreted as 'no limit'."? > > > > As for the min freq, I think PM QoS should be mentioned to make clear why > > the conversion to kHz is needed. > > Agree. I expect that Leonard will mention that. > > > > >> */ > >> > >> > >>> + 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 dac0dffeabb4..7849fe4c666d 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) > >> > >> min -> minimum and then remove parenthesis as following: > >> PM QoS minimum frequency request by user via sysfs > >> > >>> + * @user_max_freq_req: PM QoS max frequency request from user (via sysfs) > >> > >> ditto. max -> maximum > >> 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; > >>> unsigned long scaling_min_freq; > >>> unsigned long scaling_max_freq; > >>> bool stop_polling; > >>> > >>> unsigned long suspend_freq; > >>> > >> > >> > >> -- > >> Best Regards, > >> Chanwoo Choi > >> Samsung Electronics > > > > > > > -- > Best Regards, > Chanwoo Choi > Samsung Electronics
On 19. 9. 27. 오전 1:04, Matthias Kaehlcke wrote: > On Thu, Sep 26, 2019 at 10:25:41AM +0900, Chanwoo Choi wrote: >> On 19. 9. 26. 오전 1:45, Matthias Kaehlcke wrote: >>> On Wed, Sep 25, 2019 at 11:41:07AM +0900, Chanwoo Choi wrote: >>>> On 19. 9. 24. 오후 7:11, 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 | 46 ++++++++++++++++++++++++--------------- >>>>> include/linux/devfreq.h | 9 ++++---- >>>>> 2 files changed, 33 insertions(+), 22 deletions(-) >>>>> >>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>>> index 784f3e40536a..8bb7efd821ab 100644 >>>>> --- a/drivers/devfreq/devfreq.c >>>>> +++ b/drivers/devfreq/devfreq.c >>>>> @@ -137,14 +137,10 @@ static void get_freq_range(struct devfreq *devfreq, >>>>> qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, >>>>> DEV_PM_QOS_MIN_FREQUENCY); >>>>> *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); >>>>> *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); >>>>> >>>>> - /* 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); >>>> >>>> Please check the return value if error happen, just print the err with dev_err() >>>> without stopping the release steps. >>> >>> I wonder if dev_warn() would be more appropriate, since the current operation >>> is not aborted. >>> >>>>> kfree(devfreq->time_in_state); >>>>> kfree(devfreq->trans_table); >>>>> mutex_destroy(&devfreq->lock); >>>>> kfree(devfreq); >>>>> } >>>>> @@ -747,18 +745,25 @@ 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; >>>>> + >>>>> + 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 +848,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); >>>> >>>> Please check the return value if error happen, just print the err with dev_err() >>>> without stopping the release steps. >>>> >>>> dev_err(... "failed to remove request of DEV_PM_QOS_MAX_FREQUENCY\n"); >>> >>> dev_warn() for the same reason as above? >> >> Actually, I think that is not critical error but need to print the error >> So, I think that dev_err() is enough. If this thing is critical, >> better to use dev_warn. > > We agree that this is not critical, but not when to use dev_err() > and dev_warn(). You seem to imply that dev_warn() is more severe > than dev_err(), however it's the other way around: > > #define KERN_ERR KERN_SOH "3" /* error conditions */ > #define KERN_WARNING KERN_SOH "4" /* warning conditions */ > #define KERN_NOTICE KERN_SOH "5" /* normal but significant condition */ > > I think in this case you can go either way, it's certainly an error, > but the driver continues with the normal execution path. I agree to use dev_warn. Thanks for detailed explanation. > >>> >>> I think the message would be better with a slight change: >>> >>> "failed to remove DEV_PM_QOS_MAX_FREQUENCY request\n" >> >> OK. >> >>> >>>> >>>>> + if (dev_pm_qos_request_active(&devfreq->user_min_freq_req)) >>>>> + dev_pm_qos_remove_request(&devfreq->user_min_freq_req); >>>> >>>> dev_err(... "failed to remove request of DEV_PM_QOS_MIN_FREQUENCY\n"); >>> >>> ditto >>> >>>>> kfree(devfreq->time_in_state); >>>>> kfree(devfreq->trans_table); >>>>> kfree(devfreq); >>>>> err_out: >>>>> return ERR_PTR(err); >>>>> @@ -1407,14 +1416,15 @@ 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 PM QoS */ >>>> >>>> I prefer more detailed description as following: >>>> >>>> /* >>>> * Round down to KHz to decide the proper minimum frequency >>> >>> it should be kHz, with a lower-case 'k', as in the original comment. >> >> Good. >> >>> >>>> * which is closed to user request. >>>> */ >>> >>> The comment you suggest doesn't provide any information about why the >>> conversion to kHz is done, in this sense the original comment that >>> mentions PM QoS provides more value. >>> >>> With whatever we end up, I suggest to use 'convert' instead of >>> 'round down'. >> >> I agree to use 'convert' instead of 'round down' >> if some expression indicates the correct meaning. >> >>> >>>>> + 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, >>>>> @@ -1439,19 +1449,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 PM QoS and interpret zero as "don't care" */ >>>> >>>> I think that "don't care" comment style is not good. >>>> >>>> I referred to the Documentation/ABI/testing/sysfs-class-devfreq file. >>>> I prefer more detailed description as following: >>>> /* >>>> * Round up to KHz to decide the proper maximum frequency >>> >>> kHz >>> >>>> * which is closed to user request. If value is zero, >>>> * the user does not care. >>> >>> "the user does not care" is still very casual you didn't like initially. >>> How about "A value of zero is interpreted as 'no limit'."? >>> >>> As for the min freq, I think PM QoS should be mentioned to make clear why >>> the conversion to kHz is needed. >> >> Agree. I expect that Leonard will mention that. >> >>> >>>> */ >>>> >>>> >>>>> + 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 dac0dffeabb4..7849fe4c666d 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) >>>> >>>> min -> minimum and then remove parenthesis as following: >>>> PM QoS minimum frequency request by user via sysfs >>>> >>>>> + * @user_max_freq_req: PM QoS max frequency request from user (via sysfs) >>>> >>>> ditto. max -> maximum >>>> 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; >>>>> unsigned long scaling_min_freq; >>>>> unsigned long scaling_max_freq; >>>>> bool stop_polling; >>>>> >>>>> unsigned long suspend_freq; >>>>> >>>> >>>> >>>> -- >>>> Best Regards, >>>> Chanwoo Choi >>>> Samsung Electronics >>> >>> >> >> >> -- >> Best Regards, >> Chanwoo Choi >> Samsung Electronics > >
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 784f3e40536a..8bb7efd821ab 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -137,14 +137,10 @@ static void get_freq_range(struct devfreq *devfreq, qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY); *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq); *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq); - /* 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,25 @@ 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; + + 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 +848,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); @@ -1407,14 +1416,15 @@ 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 PM QoS */ + 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, @@ -1439,19 +1449,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 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 dac0dffeabb4..7849fe4c666d 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;