Cpufreq core checks the range of target_freq before calling driver->target() and so we don't need to do it again.
Remove it.
Cc: Sekhar Nori nsekhar@ti.com Cc: Linus Walleij linus.walleij@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- arch/arm/mach-davinci/cpufreq.c | 9 --------- drivers/cpufreq/dbx500-cpufreq.c | 6 ------ 2 files changed, 15 deletions(-)
diff --git a/arch/arm/mach-davinci/cpufreq.c b/arch/arm/mach-davinci/cpufreq.c index 55eb870..8fb0c2a 100644 --- a/arch/arm/mach-davinci/cpufreq.c +++ b/arch/arm/mach-davinci/cpufreq.c @@ -79,15 +79,6 @@ static int davinci_target(struct cpufreq_policy *policy, struct davinci_cpufreq_config *pdata = cpufreq.dev->platform_data; struct clk *armclk = cpufreq.armclk;
- /* - * Ensure desired rate is within allowed range. Some govenors - * (ondemand) will just pass target_freq=0 to get the minimum. - */ - if (target_freq < policy->cpuinfo.min_freq) - target_freq = policy->cpuinfo.min_freq; - if (target_freq > policy->cpuinfo.max_freq) - target_freq = policy->cpuinfo.max_freq; - freqs.old = davinci_getspeed(0); freqs.new = clk_round_rate(armclk, target_freq * 1000) / 1000;
diff --git a/drivers/cpufreq/dbx500-cpufreq.c b/drivers/cpufreq/dbx500-cpufreq.c index 7192a6d..15ed367 100644 --- a/drivers/cpufreq/dbx500-cpufreq.c +++ b/drivers/cpufreq/dbx500-cpufreq.c @@ -37,12 +37,6 @@ static int dbx500_cpufreq_target(struct cpufreq_policy *policy, unsigned int idx; int ret;
- /* scale the target frequency to one of the extremes supported */ - if (target_freq < policy->cpuinfo.min_freq) - target_freq = policy->cpuinfo.min_freq; - if (target_freq > policy->cpuinfo.max_freq) - target_freq = policy->cpuinfo.max_freq; - /* Lookup the next frequency */ if (cpufreq_frequency_table_target(policy, freq_table, target_freq, relation, &idx))
Some assignments of policy-> min/max/cur/cpuinfo.min_freq/cpuinfo.max_freq aren't required as part of it is done by cpufreq driver or cpufreq core.
Remove them.
At some places we merge multiple lines together too.
Cc: Sekhar Nori nsekhar@ti.com Cc: Sascha Hauer kernel@pengutronix.de Cc: Paul Mundt lethal@linux-sh.org Cc: linux-sh@vger.kernel.org Cc: linux-omap@vger.kernel.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- Documentation/cpu-freq/cpu-drivers.txt | 5 +++-- arch/arm/mach-davinci/cpufreq.c | 4 ---- arch/arm/mach-imx/cpufreq.c | 3 --- arch/sh/kernel/cpufreq.c | 9 +++------ drivers/cpufreq/cpufreq-nforce2.c | 6 ++---- drivers/cpufreq/omap-cpufreq.c | 4 +--- 6 files changed, 9 insertions(+), 22 deletions(-)
diff --git a/Documentation/cpu-freq/cpu-drivers.txt b/Documentation/cpu-freq/cpu-drivers.txt index 72f70b1..c94383f 100644 --- a/Documentation/cpu-freq/cpu-drivers.txt +++ b/Documentation/cpu-freq/cpu-drivers.txt @@ -108,8 +108,9 @@ policy->governor must contain the "default policy" for cpufreq_driver.target is called with these values.
-For setting some of these values, the frequency table helpers might be -helpful. See the section 2 for more information on them. +For setting some of these values (cpuinfo.min[max]_freq, policy->min[max]), the +frequency table helpers might be helpful. See the section 2 for more information +on them.
SMP systems normally have same clock source for a group of cpus. For these the .init() would be called only once for the first online cpu. Here the .init() diff --git a/arch/arm/mach-davinci/cpufreq.c b/arch/arm/mach-davinci/cpufreq.c index 8fb0c2a..ff46862 100644 --- a/arch/arm/mach-davinci/cpufreq.c +++ b/arch/arm/mach-davinci/cpufreq.c @@ -149,10 +149,6 @@ static int davinci_cpu_init(struct cpufreq_policy *policy) policy->cpuinfo.max_freq = policy->max; }
- policy->min = policy->cpuinfo.min_freq; - policy->max = policy->cpuinfo.max_freq; - policy->cur = davinci_getspeed(0); - /* * Time measurement across the target() function yields ~1500-1800us * time taken with no drivers on notification list. diff --git a/arch/arm/mach-imx/cpufreq.c b/arch/arm/mach-imx/cpufreq.c index cfce5e3..387dc4c 100644 --- a/arch/arm/mach-imx/cpufreq.c +++ b/arch/arm/mach-imx/cpufreq.c @@ -144,14 +144,11 @@ static int mxc_cpufreq_init(struct cpufreq_policy *policy) imx_freq_table[i].frequency = CPUFREQ_TABLE_END;
policy->cur = clk_get_rate(cpu_clk) / 1000; - policy->min = policy->cpuinfo.min_freq = cpu_freq_khz_min; - policy->max = policy->cpuinfo.max_freq = cpu_freq_khz_max;
/* Manual states, that PLL stabilizes in two CLK32 periods */ policy->cpuinfo.transition_latency = 2 * NANOSECOND / CLK32_FREQ;
ret = cpufreq_frequency_table_cpuinfo(policy, imx_freq_table); - if (ret < 0) { printk(KERN_ERR "%s: failed to register i.MXC CPUfreq with error code %d\n", __func__, ret); diff --git a/arch/sh/kernel/cpufreq.c b/arch/sh/kernel/cpufreq.c index 0fdf64b..88c8fee 100644 --- a/arch/sh/kernel/cpufreq.c +++ b/arch/sh/kernel/cpufreq.c @@ -116,7 +116,7 @@ static int sh_cpufreq_cpu_init(struct cpufreq_policy *policy) return PTR_ERR(cpuclk); }
- policy->cur = policy->min = policy->max = sh_cpufreq_get(cpu); + policy->cur = sh_cpufreq_get(cpu);
freq_table = cpuclk->nr_freqs ? cpuclk->freq_table : NULL; if (freq_table) { @@ -129,15 +129,12 @@ static int sh_cpufreq_cpu_init(struct cpufreq_policy *policy) dev_notice(dev, "no frequency table found, falling back " "to rate rounding.\n");
- policy->cpuinfo.min_freq = + policy->min = policy->cpuinfo.min_freq = (clk_round_rate(cpuclk, 1) + 500) / 1000; - policy->cpuinfo.max_freq = + policy->max = policy->cpuinfo.max_freq = (clk_round_rate(cpuclk, ~0UL) + 500) / 1000; }
- policy->min = policy->cpuinfo.min_freq; - policy->max = policy->cpuinfo.max_freq; - policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
dev_info(dev, "CPU Frequencies - Minimum %u.%03u MHz, " diff --git a/drivers/cpufreq/cpufreq-nforce2.c b/drivers/cpufreq/cpufreq-nforce2.c index 224a478..af1542d 100644 --- a/drivers/cpufreq/cpufreq-nforce2.c +++ b/drivers/cpufreq/cpufreq-nforce2.c @@ -359,12 +359,10 @@ static int nforce2_cpu_init(struct cpufreq_policy *policy) min_fsb = NFORCE2_MIN_FSB;
/* cpuinfo and default policy values */ - policy->cpuinfo.min_freq = min_fsb * fid * 100; - policy->cpuinfo.max_freq = max_fsb * fid * 100; + policy->min = policy->cpuinfo.min_freq = min_fsb * fid * 100; + policy->max = policy->cpuinfo.max_freq = max_fsb * fid * 100; policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL; policy->cur = nforce2_get(policy->cpu); - policy->min = policy->cpuinfo.min_freq; - policy->max = policy->cpuinfo.max_freq;
return 0; } diff --git a/drivers/cpufreq/omap-cpufreq.c b/drivers/cpufreq/omap-cpufreq.c index b610edd..ad7549c 100644 --- a/drivers/cpufreq/omap-cpufreq.c +++ b/drivers/cpufreq/omap-cpufreq.c @@ -177,7 +177,7 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy) goto fail_ck; }
- policy->cur = policy->min = policy->max = omap_getspeed(policy->cpu); + policy->cur = omap_getspeed(policy->cpu);
if (!freq_table) result = opp_init_cpufreq_table(mpu_dev, &freq_table); @@ -196,8 +196,6 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
- policy->min = policy->cpuinfo.min_freq; - policy->max = policy->cpuinfo.max_freq; policy->cur = omap_getspeed(policy->cpu);
/*
On 3/24/2013 8:59 PM, Viresh Kumar wrote:
Some assignments of policy-> min/max/cur/cpuinfo.min_freq/cpuinfo.max_freq aren't required as part of it is done by cpufreq driver or cpufreq core.
Remove them.
At some places we merge multiple lines together too.
Cc: Sekhar Nori nsekhar@ti.com Cc: Sascha Hauer kernel@pengutronix.de Cc: Paul Mundt lethal@linux-sh.org Cc: linux-sh@vger.kernel.org Cc: linux-omap@vger.kernel.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Documentation/cpu-freq/cpu-drivers.txt | 5 +++-- arch/arm/mach-davinci/cpufreq.c | 4 ---- arch/arm/mach-imx/cpufreq.c | 3 --- arch/sh/kernel/cpufreq.c | 9 +++------ drivers/cpufreq/cpufreq-nforce2.c | 6 ++---- drivers/cpufreq/omap-cpufreq.c | 4 +--- 6 files changed, 9 insertions(+), 22 deletions(-)
diff --git a/Documentation/cpu-freq/cpu-drivers.txt b/Documentation/cpu-freq/cpu-drivers.txt index 72f70b1..c94383f 100644 --- a/Documentation/cpu-freq/cpu-drivers.txt +++ b/Documentation/cpu-freq/cpu-drivers.txt @@ -108,8 +108,9 @@ policy->governor must contain the "default policy" for cpufreq_driver.target is called with these values. -For setting some of these values, the frequency table helpers might be -helpful. See the section 2 for more information on them. +For setting some of these values (cpuinfo.min[max]_freq, policy->min[max]), the +frequency table helpers might be helpful. See the section 2 for more information +on them. SMP systems normally have same clock source for a group of cpus. For these the .init() would be called only once for the first online cpu. Here the .init() diff --git a/arch/arm/mach-davinci/cpufreq.c b/arch/arm/mach-davinci/cpufreq.c index 8fb0c2a..ff46862 100644 --- a/arch/arm/mach-davinci/cpufreq.c +++ b/arch/arm/mach-davinci/cpufreq.c @@ -149,10 +149,6 @@ static int davinci_cpu_init(struct cpufreq_policy *policy) policy->cpuinfo.max_freq = policy->max; }
- policy->min = policy->cpuinfo.min_freq;
- policy->max = policy->cpuinfo.max_freq;
- policy->cur = davinci_getspeed(0);
There is a line in the code a little above the ones you deleted that also sets these same variables. I guess you were relying on that line to set policy->cur, but that also sets policy->{min, max} which can be cleaned up.
Thanks, Sekhar
On 25 March 2013 14:06, Sekhar Nori nsekhar@ti.com wrote:
There is a line in the code a little above the ones you deleted that also sets these same variables. I guess you were relying on that line to set policy->cur, but that also sets policy->{min, max} which can be cleaned up.
This code is rather confusing or wrong, this was the state of code before this patch:
policy->cur = policy->min = policy->max = davinci_getspeed(0);
if (freq_table) { result = cpufreq_frequency_table_cpuinfo(policy, freq_table); if (!result) cpufreq_frequency_table_get_attr(freq_table, policy->cpu); } else { policy->cpuinfo.min_freq = policy->min; policy->cpuinfo.max_freq = policy->max; }
policy->min = policy->cpuinfo.min_freq; policy->max = policy->cpuinfo.max_freq; policy->cur = davinci_getspeed(0);
The tricky part is if/else, where if don't return error if cpufreq_frequency_table_cpuinfo() fails. We want to set ->min[max] and cpuinfo.min[max] always. And i can see this code not doing that for some case even with my patch.
Possible scenarios: 1. Valid freq_table: My patch + what you suggested is required. 2. Invalid freq_table: We never set cpuinfo.min[max] with or without my patch 3. No freq_table: Only my patch is required.
If i do what you suggested then 2 and 3 would fail... If you want to return error in case cpufreq_frequency_table_cpuinfo(), then i can fix it properly.
On 3/25/2013 2:15 PM, Viresh Kumar wrote:
On 25 March 2013 14:06, Sekhar Nori nsekhar@ti.com wrote:
There is a line in the code a little above the ones you deleted that also sets these same variables. I guess you were relying on that line to set policy->cur, but that also sets policy->{min, max} which can be cleaned up.
This code is rather confusing or wrong, this was the state of code before this patch:
policy->cur = policy->min = policy->max = davinci_getspeed(0);
if (freq_table) { result = cpufreq_frequency_table_cpuinfo(policy, freq_table); if (!result) cpufreq_frequency_table_get_attr(freq_table, policy->cpu); } else { policy->cpuinfo.min_freq = policy->min; policy->cpuinfo.max_freq = policy->max; }
policy->min = policy->cpuinfo.min_freq; policy->max = policy->cpuinfo.max_freq; policy->cur = davinci_getspeed(0);The tricky part is if/else, where if don't return error if cpufreq_frequency_table_cpuinfo() fails. We want to set ->min[max] and cpuinfo.min[max] always. And i can see this code not doing that for some case even with my patch.
Possible scenarios:
- Valid freq_table: My patch + what you suggested is required.
- Invalid freq_table: We never set cpuinfo.min[max] with or without my patch
- No freq_table: Only my patch is required.
If i do what you suggested then 2 and 3 would fail... If you want to return error in case cpufreq_frequency_table_cpuinfo(), then i can fix it properly.
So down in the cpufreq driver probe below, we bail out if freq_table is not provided. So all this checking for freq_table in the code you pasted above is superfluous. If you can clean that part up and add checking for cpufreq_frequency_table_cpuinfo() as you proposed, I will be glad to test it out ;)
Thanks, Sekhar
On 25 March 2013 15:11, Sekhar Nori nsekhar@ti.com wrote:
So down in the cpufreq driver probe below, we bail out if freq_table is not provided. So all this checking for freq_table in the code you pasted above is superfluous. If you can clean that part up and add checking for cpufreq_frequency_table_cpuinfo() as you proposed, I will be glad to test it out ;)
Attached is the complete patch and following is your fixup for davinci (just to review):
diff --git a/arch/arm/mach-davinci/cpufreq.c b/arch/arm/mach-davinci/cpufreq.c index ff46862..7c2e943 100644 --- a/arch/arm/mach-davinci/cpufreq.c +++ b/arch/arm/mach-davinci/cpufreq.c @@ -137,18 +137,17 @@ static int davinci_cpu_init(struct cpufreq_policy *policy) return result; }
- policy->cur = policy->min = policy->max = davinci_getspeed(0); - - if (freq_table) { - result = cpufreq_frequency_table_cpuinfo(policy, freq_table); - if (!result) - cpufreq_frequency_table_get_attr(freq_table, - policy->cpu); - } else { - policy->cpuinfo.min_freq = policy->min; - policy->cpuinfo.max_freq = policy->max; + policy->cur = davinci_getspeed(0); + + result = cpufreq_frequency_table_cpuinfo(policy, freq_table); + if (result) { + pr_err("%s: cpufreq_frequency_table_cpuinfo() failed", + __func__); + return result; }
+ cpufreq_frequency_table_get_attr(freq_table, policy->cpu); + /* * Time measurement across the target() function yields ~1500-1800us * time taken with no drivers on notification list.
On 3/25/2013 3:54 PM, Viresh Kumar wrote:
On 25 March 2013 15:11, Sekhar Nori nsekhar@ti.com wrote:
So down in the cpufreq driver probe below, we bail out if freq_table is not provided. So all this checking for freq_table in the code you pasted above is superfluous. If you can clean that part up and add checking for cpufreq_frequency_table_cpuinfo() as you proposed, I will be glad to test it out ;)
Attached is the complete patch and following is your fixup for davinci (just to review):
For the attached patch,
[nsekhar@ti.com: tested on da850 evm] Acked-by: Sekhar Nori nsekhar@ti.com
Thanks, sekhar
On Sun, Mar 24, 2013 at 4:29 PM, Viresh Kumar viresh.kumar@linaro.org wrote:
Cpufreq core checks the range of target_freq before calling driver->target() and so we don't need to do it again.
Remove it.
Cc: Sekhar Nori nsekhar@ti.com Cc: Linus Walleij linus.walleij@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Acked-by: Linus Walleij linus.walleij@linaro.org
Yours, Linus Walleij
linaro-kernel@lists.linaro.org
-
Linus Walleij -
Sekhar Nori -
Viresh Kumar