* [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table
@ 2013-11-21 7:09 Viresh Kumar
2013-11-21 13:11 ` Rafael J. Wysocki
2013-11-22 7:03 ` viresh kumar
0 siblings, 2 replies; 8+ messages in thread
From: Viresh Kumar @ 2013-11-21 7:09 UTC (permalink / raw)
To: rjw, nm
Cc: linaro-kernel, patches, cpufreq, linux-pm, linux-kernel,
shawn.guo, ceh, Viresh Kumar
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);
--
1.7.12.rc2.18.g61b472e
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table
2013-11-21 7:09 [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table Viresh Kumar
@ 2013-11-21 13:11 ` Rafael J. Wysocki
2013-11-21 15:57 ` Viresh Kumar
2013-11-22 7:03 ` viresh kumar
1 sibling, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2013-11-21 13:11 UTC (permalink / raw)
To: Viresh Kumar
Cc: nm, linaro-kernel, patches, cpufreq, linux-pm, linux-kernel,
shawn.guo, ceh
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);
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table
2013-11-21 13:11 ` Rafael J. Wysocki
@ 2013-11-21 15:57 ` Viresh Kumar
2013-11-21 17:56 ` Dirk Brandewie
0 siblings, 1 reply; 8+ messages in thread
From: Viresh Kumar @ 2013-11-21 15:57 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Nishanth Menon, Lists linaro-kernel, Patch Tracking,
cpufreq@vger.kernel.org, linux-pm@vger.kernel.org,
Linux Kernel Mailing List, Shawn Guo, Sripa Bagadia
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table
2013-11-21 15:57 ` Viresh Kumar
@ 2013-11-21 17:56 ` Dirk Brandewie
2013-11-21 21:43 ` Rafael J. Wysocki
0 siblings, 1 reply; 8+ messages in thread
From: Dirk Brandewie @ 2013-11-21 17:56 UTC (permalink / raw)
To: Viresh Kumar, Rafael J. Wysocki
Cc: Nishanth Menon, Lists linaro-kernel, Patch Tracking,
cpufreq@vger.kernel.org, linux-pm@vger.kernel.org,
Linux Kernel Mailing List, Shawn Guo, Sripa Bagadia
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table
2013-11-21 17:56 ` Dirk Brandewie
@ 2013-11-21 21:43 ` Rafael J. Wysocki
2013-11-22 7:06 ` viresh kumar
0 siblings, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2013-11-21 21:43 UTC (permalink / raw)
To: Dirk Brandewie
Cc: Viresh Kumar, Nishanth Menon, Lists linaro-kernel, Patch Tracking,
cpufreq@vger.kernel.org, linux-pm@vger.kernel.org,
Linux Kernel Mailing List, Shawn Guo, Sripa Bagadia
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table
2013-11-21 7:09 [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table Viresh Kumar
2013-11-21 13:11 ` Rafael J. Wysocki
@ 2013-11-22 7:03 ` viresh kumar
2013-11-22 21:43 ` Nishanth Menon
1 sibling, 1 reply; 8+ messages in thread
From: viresh kumar @ 2013-11-22 7:03 UTC (permalink / raw)
To: nm
Cc: rjw, linaro-kernel, patches, cpufreq, linux-pm, linux-kernel,
shawn.guo, ceh, Viresh Kumar
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 */
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table
2013-11-21 21:43 ` Rafael J. Wysocki
@ 2013-11-22 7:06 ` viresh kumar
0 siblings, 0 replies; 8+ messages in thread
From: viresh kumar @ 2013-11-22 7:06 UTC (permalink / raw)
To: Rafael J. Wysocki, Dirk Brandewie
Cc: Nishanth Menon, Lists linaro-kernel, Patch Tracking,
cpufreq@vger.kernel.org, linux-pm@vger.kernel.org,
Linux Kernel Mailing List, Shawn Guo, Sripa Bagadia
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..
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table
2013-11-22 7:03 ` viresh kumar
@ 2013-11-22 21:43 ` Nishanth Menon
0 siblings, 0 replies; 8+ messages in thread
From: Nishanth Menon @ 2013-11-22 21:43 UTC (permalink / raw)
To: viresh kumar
Cc: rjw, linaro-kernel, patches, cpufreq, linux-pm, linux-kernel,
shawn.guo, ceh
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 */
>
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-11-22 21:43 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-21 7:09 [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table Viresh Kumar
2013-11-21 13:11 ` Rafael J. Wysocki
2013-11-21 15:57 ` Viresh Kumar
2013-11-21 17:56 ` Dirk Brandewie
2013-11-21 21:43 ` Rafael J. Wysocki
2013-11-22 7:06 ` viresh kumar
2013-11-22 7:03 ` viresh kumar
2013-11-22 21:43 ` Nishanth Menon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).