| Message ID | 1493281427-8671-1-git-send-email-lukasz.luba@arm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
> This patch adds functionality to make sure when a call to set a new > max or min frequency is possible. > If storing the new value was not possible, restore previous one > and forward the error value. > > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > --- > It is based on v4.11-rc8. > > drivers/devfreq/devfreq.c | 36 ++++++++++++++++++++++++++++++++---- > 1 file changed, 32 insertions(+), 4 deletions(-) ... > + ret = devfreq_get_freq_level(df, value); > + if (ret < 0) { > + dev_warn(dev, "Storing min freq failed with (%d) error\n", ret); > + goto unlock; > + } > + This is not good. For a device that supports [ 100, 400, 800, 1000 ] MHz, saying "min = 200 Mhz" or "max = 600 MHz" shouldn't be prohibited. Those functions are to express lower bound and upper bound, not to designate the exact operating frequencies. Cheers, MyungJoo
Hi MyungJoo, On 27/04/17 09:30, MyungJoo Ham wrote: >> This patch adds functionality to make sure when a call to set a new >> max or min frequency is possible. >> If storing the new value was not possible, restore previous one >> and forward the error value. >> >> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> >> --- >> It is based on v4.11-rc8. >> >> drivers/devfreq/devfreq.c | 36 ++++++++++++++++++++++++++++++++---- >> 1 file changed, 32 insertions(+), 4 deletions(-) > > ... > >> + ret = devfreq_get_freq_level(df, value); >> + if (ret < 0) { >> + dev_warn(dev, "Storing min freq failed with (%d) error\n", ret); >> + goto unlock; >> + } >> + > > This is not good. > > For a device that supports [ 100, 400, 800, 1000 ] MHz, > saying "min = 200 Mhz" or "max = 600 MHz" shouldn't be prohibited. > > Those functions are to express lower bound and upper bound, not to > designate the exact operating frequencies. Would it be possible to convince you to store a 'posible/valid frequency' (from OPP) in that value? Governors (like simpleondemand) and drivers use it with the flags. It would be useful for thermal subsystem. It could have the max/min for the devfreq device. I could prepare a patch which sets the freq from OPP respecting rounding up/down based on 'devfreq_recommended_opp()'. Regards, Lukasz
> > > > ... > > > >> + ret = devfreq_get_freq_level(df, value); > >> + if (ret < 0) { > >> + dev_warn(dev, "Storing min freq failed with (%d) error\n", ret); > >> + goto unlock; > >> + } > >> + > > > > This is not good. > > > > For a device that supports [ 100, 400, 800, 1000 ] MHz, > > saying "min = 200 Mhz" or "max = 600 MHz" shouldn't be prohibited. > > > > Those functions are to express lower bound and upper bound, not to > > designate the exact operating frequencies. > > Would it be possible to convince you to store a > 'posible/valid frequency' (from OPP) in that value? > Governors (like simpleondemand) and drivers use it with the flags. > It would be useful for thermal subsystem. It could have the max/min for > the devfreq device. > > I could prepare a patch which sets the freq from OPP respecting > rounding up/down based on 'devfreq_recommended_opp()'. > > Regards, > Lukasz Before talking about implementation detail, what's the benefit of what you are suggesting? What's the scenario that you cannot do with the current system while you can do with what you suggest? Why do you need rounded (OPP-enabled) max/min values? Why unrounded (lower/upper limits) max/min values cannot do what you want? Cheers, MyungJoo ps. I'll be away from the network for about 10 days after few hours.
On 28/04/17 00:41, MyungJoo Ham wrote: >>> >>> ... >>> >>>> + ret = devfreq_get_freq_level(df, value); >>>> + if (ret < 0) { >>>> + dev_warn(dev, "Storing min freq failed with (%d) error\n", ret); >>>> + goto unlock; >>>> + } >>>> + >>> >>> This is not good. >>> >>> For a device that supports [ 100, 400, 800, 1000 ] MHz, >>> saying "min = 200 Mhz" or "max = 600 MHz" shouldn't be prohibited. >>> >>> Those functions are to express lower bound and upper bound, not to >>> designate the exact operating frequencies. >> >> Would it be possible to convince you to store a >> 'posible/valid frequency' (from OPP) in that value? >> Governors (like simpleondemand) and drivers use it with the flags. >> It would be useful for thermal subsystem. It could have the max/min for >> the devfreq device. >> >> I could prepare a patch which sets the freq from OPP respecting >> rounding up/down based on 'devfreq_recommended_opp()'. >> >> Regards, >> Lukasz > > Before talking about implementation detail, > what's the benefit of what you are suggesting? > > What's the scenario that you cannot do with the current system > while you can do with what you suggest? > > Why do you need rounded (OPP-enabled) max/min values? > Why unrounded (lower/upper limits) max/min values cannot do what you want? > > > Cheers, > MyungJoo > > > ps. I'll be away from the network for about 10 days after few hours. > i.e. when you look at the function: devfreq_cooling_get_max_state() in drivers/thermal/devfreq_cooling you will see that it is not aware of any changes (there are some other issues as well). I am going to modify devfreq_cooling a bit. I thought it would be good to reuse the df->max_freq as much as possible. Regards, Lukasz Luba
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index dea0487..8586035 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -1059,6 +1059,7 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, unsigned long value; int ret; unsigned long max; + unsigned long old_min; ret = sscanf(buf, "%lu", &value); if (ret != 1) @@ -1071,9 +1072,22 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, goto unlock; } + ret = devfreq_get_freq_level(df, value); + if (ret < 0) { + dev_warn(dev, "Storing min freq failed with (%d) error\n", ret); + goto unlock; + } + + old_min = df->min_freq; df->min_freq = value; - update_devfreq(df); - ret = count; + ret = update_devfreq(df); + if (ret) { + df->min_freq = old_min; + dev_warn(dev, "Storing min freq failed with (%d) error\n", ret); + } else { + ret = count; + } + unlock: mutex_unlock(&df->lock); return ret; @@ -1086,6 +1100,7 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, unsigned long value; int ret; unsigned long min; + unsigned long old_max; ret = sscanf(buf, "%lu", &value); if (ret != 1) @@ -1098,9 +1113,22 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, goto unlock; } + ret = devfreq_get_freq_level(df, value); + if (ret < 0) { + dev_warn(dev, "Storing max freq failed with (%d) error\n", ret); + goto unlock; + } + + old_max = df->max_freq; df->max_freq = value; - update_devfreq(df); - ret = count; + ret = update_devfreq(df); + if (ret) { + df->max_freq = old_max; + dev_warn(dev, "Storing max freq failed with (%d) error\n", ret); + } else { + ret = count; + } + unlock: mutex_unlock(&df->lock); return ret;
This patch adds functionality to make sure when a call to set a new max or min frequency is possible. If storing the new value was not possible, restore previous one and forward the error value. Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> --- It is based on v4.11-rc8. drivers/devfreq/devfreq.c | 36 ++++++++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-)