[PATCH] cpufreq: Make sure CPU is running on a freq from freq-table

[PATCH] cpufreq: Make sure CPU is running on a freq from freq-table

[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

Re: [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table

[Rafael J. Wysocki]

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.

Re: [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table

[Viresh Kumar]

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

Re: [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table

[Dirk Brandewie]

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
>

Re: [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table

[Rafael J. Wysocki]

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

Re: [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table

[viresh kumar]

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..

Re: [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table

[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 */

Re: [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table

[Nishanth Menon]

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