Sometimes boot loaders set CPU frequency to a value outside of frequency table present with cpufreq core. In such cases CPU might be unstable if it has to run on that frequency for long duration of time and so its better to set it to a frequency which is specified in freq-table. This also makes cpufreq stats inconsistent as cpufreq-stats would fail to register because current frequency of CPU isn't found in freq-table.
Because we don't want this change to effect boot process badly, we go for the next freq which is >= policy->cur ('cur' must be set by now, otherwise we will end up setting freq to lowest of the table as 'cur' is initialized to zero).
In case where CPU is already running on one of the frequencies present in freq-table, this would turn into a dummy call as __cpufreq_driver_target() would return early.
Reported-by: Carlos Hernandez ceh@ti.com Reported-by: Nishanth Menon nm@ti.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- After lots of discussion with Nishanth and others, I feel something like this..
@Nishanth: Please see if this works for you and I hope we don't need any of these patches anymore:
- https://lkml.org/lkml/2013/11/15/569 : cpufreq: cpufreq-cpu0: Use a sane boot frequency when booting with a mismatched bootloader configuration - https://lkml.org/lkml/2013/11/15/503 : cpufreq: stats: Do not populate stats when policy->cur has no exact match - https://lkml.org/lkml/2013/11/19/16 : cpufreq/stats: Add "unknown" frequency field in stats tables
drivers/cpufreq/cpufreq.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 02d534d..d55c843 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1038,6 +1038,32 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, } }
+ /* + * Sometimes boot loaders set CPU frequency to a value outside of + * frequency table present with cpufreq core. In such cases CPU might be + * unstable if it has to run on that frequency for long duration of time + * and so its better to set it to a frequency which is specified in + * freq-table. This also makes cpufreq stats inconsistent as + * cpufreq-stats would fail to register because current frequency of CPU + * isn't found in freq-table. + * + * Because we don't want this change to effect boot process badly, we go + * for the next freq which is >= policy->cur ('cur' must be set by now, + * otherwise we will end up setting freq to lowest of the table as 'cur' + * is initialized to zero). + * + * In case where CPU is already running on one of the frequencies + * present in freq-table, this would turn into a dummy call as + * __cpufreq_driver_target() would return early. + */ + if (has_target()) { + ret = __cpufreq_driver_target(policy, policy->cur, + CPUFREQ_RELATION_L); + if (ret) + pr_err("%s: Unable to set frequency from table: %d\n", + __func__, ret); + } + /* related cpus should atleast have policy->cpus */ cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
On Thursday, November 21, 2013 12:39:02 PM Viresh Kumar wrote:
Sometimes boot loaders set CPU frequency to a value outside of frequency table present with cpufreq core. In such cases CPU might be unstable if it has to run on that frequency for long duration of time and so its better to set it to a frequency which is specified in freq-table. This also makes cpufreq stats inconsistent as cpufreq-stats would fail to register because current frequency of CPU isn't found in freq-table.
Because we don't want this change to effect boot process badly, we go for the next freq which is >= policy->cur ('cur' must be set by now, otherwise we will end up setting freq to lowest of the table as 'cur' is initialized to zero).
In case where CPU is already running on one of the frequencies present in freq-table, this would turn into a dummy call as __cpufreq_driver_target() would return early.
Reported-by: Carlos Hernandez ceh@ti.com Reported-by: Nishanth Menon nm@ti.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
After lots of discussion with Nishanth and others, I feel something like this..
@Nishanth: Please see if this works for you and I hope we don't need any of these patches anymore:
- https://lkml.org/lkml/2013/11/15/569 : cpufreq: cpufreq-cpu0: Use a sane boot
frequency when booting with a mismatched bootloader configuration
- https://lkml.org/lkml/2013/11/15/503 : cpufreq: stats: Do not populate stats when policy->cur has no exact match
- https://lkml.org/lkml/2013/11/19/16 : cpufreq/stats: Add "unknown" frequency field in stats tables
drivers/cpufreq/cpufreq.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 02d534d..d55c843 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1038,6 +1038,32 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, } }
- /*
* Sometimes boot loaders set CPU frequency to a value outside of* frequency table present with cpufreq core. In such cases CPU might be* unstable if it has to run on that frequency for long duration of time* and so its better to set it to a frequency which is specified in* freq-table. This also makes cpufreq stats inconsistent as* cpufreq-stats would fail to register because current frequency of CPU* isn't found in freq-table.** Because we don't want this change to effect boot process badly, we go* for the next freq which is >= policy->cur ('cur' must be set by now,* otherwise we will end up setting freq to lowest of the table as 'cur'* is initialized to zero).** In case where CPU is already running on one of the frequencies* present in freq-table, this would turn into a dummy call as* __cpufreq_driver_target() would return early.*/- if (has_target()) {
ret = __cpufreq_driver_target(policy, policy->cur,CPUFREQ_RELATION_L);if (ret)pr_err("%s: Unable to set frequency from table: %d\n",__func__, ret);
Should we continue in that case?
- }
- /* related cpus should atleast have policy->cpus */ cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
On 21 November 2013 18:41, Rafael J. Wysocki rjw@rjwysocki.net wrote:
On Thursday, November 21, 2013 12:39:02 PM Viresh Kumar wrote:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c @@ -1038,6 +1038,32 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
if (has_target()) {ret = __cpufreq_driver_target(policy, policy->cur,CPUFREQ_RELATION_L);if (ret)pr_err("%s: Unable to set frequency from table: %d\n",__func__, ret);Should we continue in that case?
I wasn't sure. I thought maybe there are platforms which might not be ready for transitions so early and so an error message would be fine, as we will fail soon anyway in case there is a bug.
-- viresh
On 11/21/2013 07:57 AM, Viresh Kumar wrote:
On 21 November 2013 18:41, Rafael J. Wysocki rjw@rjwysocki.net wrote:
On Thursday, November 21, 2013 12:39:02 PM Viresh Kumar wrote:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c @@ -1038,6 +1038,32 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
if (has_target()) {ret = __cpufreq_driver_target(policy, policy->cur,CPUFREQ_RELATION_L);if (ret)pr_err("%s: Unable to set frequency from table: %d\n",__func__, ret);Should we continue in that case?
I wasn't sure. I thought maybe there are platforms which might not be ready for transitions so early and so an error message would be fine, as we will fail soon anyway in case there is a bug.
The scaling driver for the CPU has already loaded and its .init procedure has been called so .target better be callable.
Since the scaling driver is responsible maintaining the set of valid frequencies and setting policy->cur I think it is reasonable to have the scaling driver ensure that policy->cur returned from its .init and the operating frequency are in sync and match one of the values in its frequency table.
--Dirk
-- viresh -- To unsubscribe from this list: send the line "unsubscribe cpufreq" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, November 21, 2013 09:56:32 AM Dirk Brandewie wrote:
On 11/21/2013 07:57 AM, Viresh Kumar wrote:
On 21 November 2013 18:41, Rafael J. Wysocki rjw@rjwysocki.net wrote:
On Thursday, November 21, 2013 12:39:02 PM Viresh Kumar wrote:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c @@ -1038,6 +1038,32 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
if (has_target()) {ret = __cpufreq_driver_target(policy, policy->cur,CPUFREQ_RELATION_L);if (ret)pr_err("%s: Unable to set frequency from table: %d\n",__func__, ret);Should we continue in that case?
I wasn't sure. I thought maybe there are platforms which might not be ready for transitions so early and so an error message would be fine, as we will fail soon anyway in case there is a bug.
The scaling driver for the CPU has already loaded and its .init procedure has been called so .target better be callable.
Since the scaling driver is responsible maintaining the set of valid frequencies and setting policy->cur I think it is reasonable to have the scaling driver ensure that policy->cur returned from its .init and the operating frequency are in sync and match one of the values in its frequency table.
From that I infer that we should not continue on errors here. Which also is my
opinion.
Rafael
On Friday 22 November 2013 03:13 AM, Rafael J. Wysocki wrote:
On Thursday, November 21, 2013 09:56:32 AM Dirk Brandewie wrote:
The scaling driver for the CPU has already loaded and its .init procedure has been called so .target better be callable.
Yeah..
Since the scaling driver is responsible maintaining the set of valid frequencies and setting policy->cur I think it is reasonable to have the scaling driver ensure that policy->cur returned from its .init
I agree..
and the operating frequency are in sync and match one of the values in its frequency table.
Hmmm, that doesn't necessarily lie in driver's domain but maybe at a common place like core. That's why we had this patch..
From that I infer that we should not continue on errors here. Which also is my opinion.
Okay.. Code modified to return error on failure.. Will send V2 as soon as patch is tested by Nishanth..
On Thursday 21 November 2013 12:39 PM, Viresh Kumar wrote:
Sometimes boot loaders set CPU frequency to a value outside of frequency table present with cpufreq core. In such cases CPU might be unstable if it has to run on that frequency for long duration of time and so its better to set it to a frequency which is specified in freq-table. This also makes cpufreq stats inconsistent as cpufreq-stats would fail to register because current frequency of CPU isn't found in freq-table.
Because we don't want this change to effect boot process badly, we go for the next freq which is >= policy->cur ('cur' must be set by now, otherwise we will end up setting freq to lowest of the table as 'cur' is initialized to zero).
In case where CPU is already running on one of the frequencies present in freq-table, this would turn into a dummy call as __cpufreq_driver_target() would return early.
Reported-by: Carlos Hernandez ceh@ti.com Reported-by: Nishanth Menon nm@ti.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Copying another mail from Nishant here to get my cc'list back..
On Friday 22 November 2013 05:12 AM, Nishanth Menon wrote:
I gave this a quick run on my 3.12 kernel: http://pastebin.mozilla.org/3649730 is what I applied and output http://pastebin.mozilla.org/3649729
I need to see what I might have mucked up.. or see if I can test this on 3.13 master on a different board (since OMAP5 wont boot in master without clock dts nodes :()..
This happened because of common sense, which was missing in my patch :)
Try this over existing patch:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 6e8b226..e403388 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1126,13 +1126,19 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, * In case where CPU is already running on one of the frequencies * present in freq-table, this would turn into a dummy call as * __cpufreq_driver_target() would return early. + * + * We are passing target-freq as "policy->cur - 1" otherwise + * __cpufreq_driver_target() would simply fail, as policy->cur will be + * equal to target-freq. */ if (has_target()) { - ret = __cpufreq_driver_target(policy, policy->cur, + ret = __cpufreq_driver_target(policy, policy->cur - 1, CPUFREQ_RELATION_L); - if (ret) + if (ret) { pr_err("%s: Unable to set frequency from table: %d\n", __func__, ret); + goto err_out_unregister; + } }
/* related cpus should atleast have policy->cpus */
On 11/22/2013 01:03 AM, viresh kumar wrote:
On Thursday 21 November 2013 12:39 PM, Viresh Kumar wrote:
[...]
Copying another mail from Nishant here to get my cc'list back..
On Friday 22 November 2013 05:12 AM, Nishanth Menon wrote:
I gave this a quick run on my 3.12 kernel: http://pastebin.mozilla.org/3649730 is what I applied and output http://pastebin.mozilla.org/3649729
I need to see what I might have mucked up.. or see if I can test this on 3.13 master on a different board (since OMAP5 wont boot in master without clock dts nodes :()..
This happened because of common sense, which was missing in my patch :)
Try this over existing patch:
Works like a charm :) thanks.
http://pastebin.mozilla.org/3655494 -> simple Ondemand governor testing. http://pastebin.mozilla.org/3655493 -> simple Userspace governor testing.
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 6e8b226..e403388 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1126,13 +1126,19 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, * In case where CPU is already running on one of the frequencies * present in freq-table, this would turn into a dummy call as * __cpufreq_driver_target() would return early.
** We are passing target-freq as "policy->cur - 1" otherwise* __cpufreq_driver_target() would simply fail, as policy->cur will be* equal to target-freq. */ if (has_target()) {
ret = __cpufreq_driver_target(policy, policy->cur,
ret = __cpufreq_driver_target(policy, policy->cur - 1, CPUFREQ_RELATION_L);
if (ret)
if (ret) { pr_err("%s: Unable to set frequency from table: %d\n", __func__, ret);goto err_out_unregister;} } /* related cpus should atleast have policy->cpus */
linaro-kernel@lists.linaro.org
-
Dirk Brandewie -
Nishanth Menon -
Rafael J. Wysocki -
Viresh Kumar -
viresh kumar