Hi Rafael,
This series fixes all cpufreq drivers that provide a 'target_index' callback or in other words, which provide a freq-table to cpufreq core, to make sure they *only* use the 'index' argument to ->target_index() with the policy->freq_table.
This change allows us to remove the (duplicate) sorted-freq-table, which was added by following series:
[PATCH V2 0/2] cpufreq: Use sorted frequency tables
The final code looks like this: - drivers provide a freq table to the cpufreq core - core makes a copy of that and sort that in ascending order of frequencies. This is what we get from policy->freq_table. - drivers can now free the freq-table they provided earlier. - ->target_index() contains the 'index' to this sorted policy->freq_table.
This is based of the two series I have posted until now:
[PATCH V2 0/6] cpufreq: cleanups and reorganization [PATCH V2 0/2] cpufreq: Use sorted frequency tables
All the patches are pushed here for testing in case anyone wants to try:
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git cpufreq/sorted-freq-table
The 3 series combined makes freq-table traversing to find a match really fast and efficient. Which will also allow us to use it for new governors like schedutil.
-- viresh
Viresh Kumar (11): ARM: davinci: Sort frequency table cpufreq: davinci: Reuse cpufreq_generic_frequency_table_verify() cpufreq: Use policy->freq_table in ->target_index() cpufreq: blackfin: Use 'index' only to index into policy->freq_table cpufreq: elanfreq: Use 'index' only to index into policy->freq_table cpufreq: exynos: Use 'index' only to index into policy->freq_table cpufreq: ia64: Use 'index' only to index into policy->freq_table cpufreq: imx: Use 'index' only to index into policy->freq_table cpufreq: maple: Use 'index' only to index into policy->freq_table cpufreq: Keep a single (sorted) freq_table cpufreq: drivers: Free frequency tables after being used
arch/arm/mach-davinci/da850.c | 16 ++++++++------- drivers/cpufreq/acpi-cpufreq.c | 7 +++---- drivers/cpufreq/arm_big_little.c | 2 +- drivers/cpufreq/at32ap-cpufreq.c | 8 ++++---- drivers/cpufreq/blackfin-cpufreq.c | 17 +++++++++++----- drivers/cpufreq/cpufreq-dt.c | 9 ++++----- drivers/cpufreq/cpufreq.c | 6 +----- drivers/cpufreq/cris-artpec3-cpufreq.c | 2 +- drivers/cpufreq/cris-etraxfs-cpufreq.c | 2 +- drivers/cpufreq/davinci-cpufreq.c | 22 +-------------------- drivers/cpufreq/dbx500-cpufreq.c | 3 ++- drivers/cpufreq/e_powersaver.c | 26 +++++++++++++----------- drivers/cpufreq/elanfreq.c | 8 +++++++- drivers/cpufreq/exynos5440-cpufreq.c | 13 ++++++++---- drivers/cpufreq/freq_table.c | 36 +++++++++++++--------------------- drivers/cpufreq/ia64-acpi-cpufreq.c | 16 ++++++++++----- drivers/cpufreq/imx6q-cpufreq.c | 13 +++++++++--- drivers/cpufreq/kirkwood-cpufreq.c | 2 +- drivers/cpufreq/loongson1-cpufreq.c | 10 +--------- drivers/cpufreq/loongson2_cpufreq.c | 5 ++--- drivers/cpufreq/maple-cpufreq.c | 6 ++++++ include/linux/cpufreq.h | 9 ++------- 22 files changed, 117 insertions(+), 121 deletions(-)
This is required for some of the changes in cpufreq core. There was only one function dependent on the order of the table, that is fixed as well.
Cc: Sekhar Nori nsekhar@ti.com Cc: Kevin Hilman khilman@kernel.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- arch/arm/mach-davinci/da850.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c index 239886299968..f683c119cfed 100644 --- a/arch/arm/mach-davinci/da850.c +++ b/arch/arm/mach-davinci/da850.c @@ -1004,13 +1004,14 @@ static const struct da850_opp da850_opp_96 = { .frequency = freq * 1000, \ }
+/* Table sorted in ascending order of frequencies */ static struct cpufreq_frequency_table da850_freq_table[] = { - OPP(456), - OPP(408), - OPP(372), - OPP(300), - OPP(200), OPP(96), + OPP(200), + OPP(300), + OPP(372), + OPP(408), + OPP(456), { .driver_data = 0, .frequency = CPUFREQ_TABLE_END, @@ -1076,8 +1077,9 @@ int da850_register_cpufreq(char *async_clk) clk_add_alias("async", da850_cpufreq_device.name, async_clk, NULL); for (i = 0; i < ARRAY_SIZE(da850_freq_table); i++) { - if (da850_freq_table[i].frequency <= da850_max_speed) { - cpufreq_info.freq_table = &da850_freq_table[i]; + if (da850_freq_table[i].frequency > da850_max_speed) { + &da850_freq_table[i].driver_data = 0; + &da850_freq_table[i].frequency = CPUFREQ_TABLE_END; break; } }
policy->freq_table will always be valid for this platform, otherwise driver's probe() would fail. And so this routine will *always* return after calling cpufreq_frequency_table_verify().
This can be done using the generic callback provided by core, lets use it instead.
Cc: Sekhar Nori nsekhar@ti.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/davinci-cpufreq.c | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/cpufreq/davinci-cpufreq.c b/drivers/cpufreq/davinci-cpufreq.c index 7e336d20c184..b95a872800ec 100644 --- a/drivers/cpufreq/davinci-cpufreq.c +++ b/drivers/cpufreq/davinci-cpufreq.c @@ -38,26 +38,6 @@ struct davinci_cpufreq { }; static struct davinci_cpufreq cpufreq;
-static int davinci_verify_speed(struct cpufreq_policy *policy) -{ - struct davinci_cpufreq_config *pdata = cpufreq.dev->platform_data; - struct cpufreq_frequency_table *freq_table = pdata->freq_table; - struct clk *armclk = cpufreq.armclk; - - if (freq_table) - return cpufreq_frequency_table_verify(policy, freq_table); - - if (policy->cpu) - return -EINVAL; - - cpufreq_verify_within_cpu_limits(policy); - policy->min = clk_round_rate(armclk, policy->min * 1000) / 1000; - policy->max = clk_round_rate(armclk, policy->max * 1000) / 1000; - cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq, - policy->cpuinfo.max_freq); - return 0; -} - static int davinci_target(struct cpufreq_policy *policy, unsigned int idx) { struct davinci_cpufreq_config *pdata = cpufreq.dev->platform_data; @@ -121,7 +101,7 @@ static int davinci_cpu_init(struct cpufreq_policy *policy)
static struct cpufreq_driver davinci_driver = { .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK, - .verify = davinci_verify_speed, + .verify = cpufreq_generic_frequency_table_verify, .target_index = davinci_target, .get = cpufreq_generic_get, .init = davinci_cpu_init,
The 'policy' already contains a pointer to the freq table, use that instead of using driver specific tables name.
This is done in order to make sure that the 'index' passed to the ->target_index() callback is used *only* to index into the policy->freq_table and nothing else.
Later patches would make changes in cpufreq core, after which policy->freq_table may be reordered by cpufreq core and it wouldn't be safe anymore to use 'index' for any other local array.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/arm_big_little.c | 2 +- drivers/cpufreq/at32ap-cpufreq.c | 2 +- drivers/cpufreq/blackfin-cpufreq.c | 2 +- drivers/cpufreq/cris-artpec3-cpufreq.c | 2 +- drivers/cpufreq/cris-etraxfs-cpufreq.c | 2 +- drivers/cpufreq/dbx500-cpufreq.c | 3 ++- drivers/cpufreq/e_powersaver.c | 2 +- drivers/cpufreq/exynos5440-cpufreq.c | 3 +-- drivers/cpufreq/imx6q-cpufreq.c | 2 +- drivers/cpufreq/kirkwood-cpufreq.c | 2 +- drivers/cpufreq/loongson2_cpufreq.c | 5 ++--- 11 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c index 418042201e6d..fc9e863e39d6 100644 --- a/drivers/cpufreq/arm_big_little.c +++ b/drivers/cpufreq/arm_big_little.c @@ -217,7 +217,7 @@ static int bL_cpufreq_set_target(struct cpufreq_policy *policy, cur_cluster = cpu_to_cluster(cpu); new_cluster = actual_cluster = per_cpu(physical_cluster, cpu);
- freqs_new = freq_table[cur_cluster][index].frequency; + freqs_new = policy->freq_table[index].frequency;
if (is_bL_switching_enabled()) { if ((actual_cluster == A15_CLUSTER) && diff --git a/drivers/cpufreq/at32ap-cpufreq.c b/drivers/cpufreq/at32ap-cpufreq.c index 7b612c8bb09e..9231b1efb70d 100644 --- a/drivers/cpufreq/at32ap-cpufreq.c +++ b/drivers/cpufreq/at32ap-cpufreq.c @@ -31,7 +31,7 @@ static int at32_set_target(struct cpufreq_policy *policy, unsigned int index) unsigned int old_freq, new_freq;
old_freq = policy->cur; - new_freq = freq_table[index].frequency; + new_freq = policy->freq_table[index].frequency;
if (!ref_freq) { ref_freq = old_freq; diff --git a/drivers/cpufreq/blackfin-cpufreq.c b/drivers/cpufreq/blackfin-cpufreq.c index 12e97d8a9db0..1650c213f465 100644 --- a/drivers/cpufreq/blackfin-cpufreq.c +++ b/drivers/cpufreq/blackfin-cpufreq.c @@ -142,7 +142,7 @@ static int bfin_target(struct cpufreq_policy *policy, unsigned int index) #endif
old_freq = bfin_getfreq_khz(0); - new_freq = bfin_freq_table[index].frequency; + new_freq = policy->freq_table[index].frequency;
#ifndef CONFIG_BF60x plldiv = (bfin_read_PLL_DIV() & SSEL) | dpm_state_table[index].csel; diff --git a/drivers/cpufreq/cris-artpec3-cpufreq.c b/drivers/cpufreq/cris-artpec3-cpufreq.c index 601b88c490cf..4e7dc8f1e619 100644 --- a/drivers/cpufreq/cris-artpec3-cpufreq.c +++ b/drivers/cpufreq/cris-artpec3-cpufreq.c @@ -36,7 +36,7 @@ static int cris_freq_target(struct cpufreq_policy *policy, unsigned int state)
/* Even though we may be SMP they will share the same clock * so all settings are made on CPU0. */ - if (cris_freq_table[state].frequency == 200000) + if (policy->freq_table[state].frequency == 200000) clk_ctrl.pll = 1; else clk_ctrl.pll = 0; diff --git a/drivers/cpufreq/cris-etraxfs-cpufreq.c b/drivers/cpufreq/cris-etraxfs-cpufreq.c index 22b2cdde74d9..6ee434a54cae 100644 --- a/drivers/cpufreq/cris-etraxfs-cpufreq.c +++ b/drivers/cpufreq/cris-etraxfs-cpufreq.c @@ -36,7 +36,7 @@ static int cris_freq_target(struct cpufreq_policy *policy, unsigned int state)
/* Even though we may be SMP they will share the same clock * so all settings are made on CPU0. */ - if (cris_freq_table[state].frequency == 200000) + if (policy->freq_table[state].frequency == 200000) clk_ctrl.pll = 1; else clk_ctrl.pll = 0; diff --git a/drivers/cpufreq/dbx500-cpufreq.c b/drivers/cpufreq/dbx500-cpufreq.c index 5c3ec1dd4921..84968889ab29 100644 --- a/drivers/cpufreq/dbx500-cpufreq.c +++ b/drivers/cpufreq/dbx500-cpufreq.c @@ -23,7 +23,8 @@ static int dbx500_cpufreq_target(struct cpufreq_policy *policy, unsigned int index) { /* update armss clk frequency */ - return clk_set_rate(armss_clk, freq_table[index].frequency * 1000); + return clk_set_rate(armss_clk, + policy->freq_table[index].frequency * 1000); }
static int dbx500_cpufreq_init(struct cpufreq_policy *policy) diff --git a/drivers/cpufreq/e_powersaver.c b/drivers/cpufreq/e_powersaver.c index cdf097b29862..a284bddfb067 100644 --- a/drivers/cpufreq/e_powersaver.c +++ b/drivers/cpufreq/e_powersaver.c @@ -163,7 +163,7 @@ static int eps_target(struct cpufreq_policy *policy, unsigned int index) centaur = eps_cpu[cpu];
/* Make frequency transition */ - dest_state = centaur->freq_table[index].driver_data & 0xffff; + dest_state = policy->freq_table[index].driver_data & 0xffff; ret = eps_set_state(centaur, policy, dest_state); if (ret) pr_err("Timeout!\n"); diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c index c0f3373706f4..085f07d31ef0 100644 --- a/drivers/cpufreq/exynos5440-cpufreq.c +++ b/drivers/cpufreq/exynos5440-cpufreq.c @@ -212,12 +212,11 @@ static int exynos_target(struct cpufreq_policy *policy, unsigned int index) { unsigned int tmp; int i; - struct cpufreq_frequency_table *freq_table = dvfs_info->freq_table;
mutex_lock(&cpufreq_lock);
freqs.old = policy->cur; - freqs.new = freq_table[index].frequency; + freqs.new = policy->freq_table[index].frequency;
cpufreq_freq_transition_begin(policy, &freqs);
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c index ef1fa8145419..3858dc7e617b 100644 --- a/drivers/cpufreq/imx6q-cpufreq.c +++ b/drivers/cpufreq/imx6q-cpufreq.c @@ -49,7 +49,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index) unsigned int old_freq, new_freq; int ret;
- new_freq = freq_table[index].frequency; + new_freq = policy->freq_table[index].frequency; freq_hz = new_freq * 1000; old_freq = clk_get_rate(arm_clk) / 1000;
diff --git a/drivers/cpufreq/kirkwood-cpufreq.c b/drivers/cpufreq/kirkwood-cpufreq.c index be42f103db60..f63bf4fcb6fe 100644 --- a/drivers/cpufreq/kirkwood-cpufreq.c +++ b/drivers/cpufreq/kirkwood-cpufreq.c @@ -54,7 +54,7 @@ static unsigned int kirkwood_cpufreq_get_cpu_frequency(unsigned int cpu) static int kirkwood_cpufreq_target(struct cpufreq_policy *policy, unsigned int index) { - unsigned int state = kirkwood_freq_table[index].driver_data; + unsigned int state = policy->freq_table[index].driver_data; unsigned long reg;
local_irq_disable(); diff --git a/drivers/cpufreq/loongson2_cpufreq.c b/drivers/cpufreq/loongson2_cpufreq.c index 6bbdac1065ff..e96c98f9245a 100644 --- a/drivers/cpufreq/loongson2_cpufreq.c +++ b/drivers/cpufreq/loongson2_cpufreq.c @@ -58,9 +58,8 @@ static int loongson2_cpufreq_target(struct cpufreq_policy *policy, cpus_allowed = current->cpus_allowed; set_cpus_allowed_ptr(current, cpumask_of(cpu));
- freq = - ((cpu_clock_freq / 1000) * - loongson2_clockmod_table[index].driver_data) / 8; + freq = (cpu_clock_freq / 1000) * policy->freq_table[index].driver_data; + freq /= 8;
set_cpus_allowed_ptr(current, &cpus_allowed);
Later patches would make changes in cpufreq core, after which policy->freq_table may be reordered by cpufreq core and it wouldn't be safe anymore to use 'index' for any other local arrays.
To prepare for that, use policy->freq_table[index].driver_data for other driver specific usage of 'index'. The 'driver_data' fields are already set properly by the driver.
Cc: Steven Miao realmz6@gmail.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/blackfin-cpufreq.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/cpufreq/blackfin-cpufreq.c b/drivers/cpufreq/blackfin-cpufreq.c index 1650c213f465..101daa851c81 100644 --- a/drivers/cpufreq/blackfin-cpufreq.c +++ b/drivers/cpufreq/blackfin-cpufreq.c @@ -135,6 +135,7 @@ static int bfin_target(struct cpufreq_policy *policy, unsigned int index) static unsigned long lpj_ref; static unsigned int lpj_ref_freq; unsigned int old_freq, new_freq; + int dpm_index; int ret = 0;
#if defined(CONFIG_CYCLES_CLOCKSOURCE) @@ -144,8 +145,14 @@ static int bfin_target(struct cpufreq_policy *policy, unsigned int index) old_freq = bfin_getfreq_khz(0); new_freq = policy->freq_table[index].frequency;
+ /* + * policy->freq_table may be sorted differently, get the index value we + * are concerned about. + */ + dpm_index = policy->freq_table[index].driver_data; + #ifndef CONFIG_BF60x - plldiv = (bfin_read_PLL_DIV() & SSEL) | dpm_state_table[index].csel; + plldiv = (bfin_read_PLL_DIV() & SSEL) | dpm_state_table[dpm_index].csel; bfin_write_PLL_DIV(plldiv); #else ret = cpu_set_cclk(policy->cpu, new_freq * 1000); @@ -154,13 +161,13 @@ static int bfin_target(struct cpufreq_policy *policy, unsigned int index) return ret; } #endif - on_each_cpu(bfin_adjust_core_timer, &index, 1); + on_each_cpu(bfin_adjust_core_timer, &dpm_index, 1); #if defined(CONFIG_CYCLES_CLOCKSOURCE) cycles = get_cycles(); SSYNC(); cycles += 10; /* ~10 cycles we lose after get_cycles() */ - __bfin_cycles_off += (cycles << __bfin_cycles_mod) - (cycles << index); - __bfin_cycles_mod = index; + __bfin_cycles_off += (cycles << __bfin_cycles_mod) - (cycles << dpm_index); + __bfin_cycles_mod = dpm_index; #endif if (!lpj_ref_freq) { lpj_ref = loops_per_jiffy;
Later patches would make changes in cpufreq core, after which policy->freq_table may be reordered by cpufreq core and it wouldn't be safe anymore to use 'index' for any other local arrays.
To prepare for that, use policy->freq_table[index].driver_data for other driver specific usage of 'index'. The 'driver_data' fields are already set properly by the driver.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/elanfreq.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/elanfreq.c b/drivers/cpufreq/elanfreq.c index bfce11cba1df..bd4a51091986 100644 --- a/drivers/cpufreq/elanfreq.c +++ b/drivers/cpufreq/elanfreq.c @@ -108,9 +108,15 @@ static unsigned int elanfreq_get_cpu_frequency(unsigned int cpu)
static int elanfreq_target(struct cpufreq_policy *policy, - unsigned int state) + unsigned int index) { /* + * policy->freq_table may be sorted differently, get the index value we + * are concerned about. + */ + unsigned int state = policy->freq_table[index].driver_data; + + /* * Access to the Elan's internal registers is indexed via * 0x22: Chip Setup & Control Register Index Register (CSCI) * 0x23: Chip Setup & Control Register Data Register (CSCD)
Later patches would make changes in cpufreq core, after which policy->freq_table may be reordered by cpufreq core and it wouldn't be safe anymore to use 'index' for any other local arrays.
To prepare for that, use policy->freq_table[index].driver_data for other driver specific usage of 'index'. The 'driver_data' fields are already set properly by the driver.
Cc: Kukjin Kim kgene@kernel.org Cc: Krzysztof Kozlowski k.kozlowski@samsung.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/exynos5440-cpufreq.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c index 085f07d31ef0..6138cbb7594e 100644 --- a/drivers/cpufreq/exynos5440-cpufreq.c +++ b/drivers/cpufreq/exynos5440-cpufreq.c @@ -210,7 +210,7 @@ static void exynos_enable_dvfs(unsigned int cur_frequency)
static int exynos_target(struct cpufreq_policy *policy, unsigned int index) { - unsigned int tmp; + unsigned int tmp, rindex; int i;
mutex_lock(&cpufreq_lock); @@ -218,13 +218,19 @@ static int exynos_target(struct cpufreq_policy *policy, unsigned int index) freqs.old = policy->cur; freqs.new = policy->freq_table[index].frequency;
+ /* + * policy->freq_table may be sorted differently, get the index value we + * are concerned about. + */ + rindex = policy->freq_table[index].driver_data; + cpufreq_freq_transition_begin(policy, &freqs);
/* Set the target frequency in all C0_3_PSTATE register */ for_each_cpu(i, policy->cpus) { tmp = __raw_readl(dvfs_info->base + XMU_C0_3_PSTATE + i * 4); tmp &= ~(P_VALUE_MASK << C0_3_PSTATE_NEW_SHIFT); - tmp |= (index << C0_3_PSTATE_NEW_SHIFT); + tmp |= (rindex << C0_3_PSTATE_NEW_SHIFT);
__raw_writel(tmp, dvfs_info->base + XMU_C0_3_PSTATE + i * 4); }
Later patches would make changes in cpufreq core, after which policy->freq_table may be reordered by cpufreq core and it wouldn't be safe anymore to use 'index' for any other local arrays.
To prepare for that, use policy->freq_table[index].driver_data for other driver specific usage of 'index'. The 'driver_data' fields are set properly by the driver now.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/ia64-acpi-cpufreq.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/ia64-acpi-cpufreq.c b/drivers/cpufreq/ia64-acpi-cpufreq.c index 759612da4fdc..cc8bb1e5ac50 100644 --- a/drivers/cpufreq/ia64-acpi-cpufreq.c +++ b/drivers/cpufreq/ia64-acpi-cpufreq.c @@ -210,7 +210,12 @@ acpi_cpufreq_target ( struct cpufreq_policy *policy, unsigned int index) { - return processor_set_freq(acpi_io_data[policy->cpu], policy, index); + /* + * policy->freq_table may be sorted differently, get the index value we + * are concerned about. + */ + return processor_set_freq(acpi_io_data[policy->cpu], policy, + policy->freq_table[index].driver_data); }
static int @@ -282,6 +287,8 @@ acpi_cpufreq_cpu_init ( } else { freq_table[i].frequency = CPUFREQ_TABLE_END; } + + freq_table[i].driver_data = i; }
result = cpufreq_table_validate_and_show(policy, freq_table);
Later patches would make changes in cpufreq core, after which policy->freq_table may be reordered by cpufreq core and it wouldn't be safe anymore to use 'index' for any other local arrays.
To prepare for that, use policy->freq_table[index].driver_data for other driver specific usage of 'index'. The 'driver_data' fields are already set properly by the driver.
Cc: Shawn Guo shawn.guo@freescale.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/imx6q-cpufreq.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c index 3858dc7e617b..e7da85890e8c 100644 --- a/drivers/cpufreq/imx6q-cpufreq.c +++ b/drivers/cpufreq/imx6q-cpufreq.c @@ -42,14 +42,21 @@ static unsigned int transition_latency; static u32 *imx6_soc_volt; static u32 soc_opp_count;
-static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index) +static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int pindex) { struct dev_pm_opp *opp; unsigned long freq_hz, volt, volt_old; - unsigned int old_freq, new_freq; + unsigned int old_freq, new_freq, index; int ret;
- new_freq = policy->freq_table[index].frequency; + new_freq = policy->freq_table[pindex].frequency; + + /* + * policy->freq_table may be sorted differently, get the index value we + * are concerned about. + */ + index = policy->freq_table[pindex].driver_data; + freq_hz = new_freq * 1000; old_freq = clk_get_rate(arm_clk) / 1000;
Later patches would make changes in cpufreq core, after which policy->freq_table may be reordered by cpufreq core and it wouldn't be safe anymore to use 'index' for any other local arrays.
To prepare for that, use policy->freq_table[index].driver_data for other driver specific usage of 'index'. The 'driver_data' fields are already set properly by the driver.
Cc: Dmitry Eremin-Solenikov dbaryshkov@gmail.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/maple-cpufreq.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/cpufreq/maple-cpufreq.c b/drivers/cpufreq/maple-cpufreq.c index d9df89392b84..8ce92ee7dd8d 100644 --- a/drivers/cpufreq/maple-cpufreq.c +++ b/drivers/cpufreq/maple-cpufreq.c @@ -133,6 +133,12 @@ static int maple_scom_query_freq(void) static int maple_cpufreq_target(struct cpufreq_policy *policy, unsigned int index) { + /* + * policy->freq_table may be sorted differently, get the index value we + * are concerned about. + */ + index = policy->freq_table[index].driver_data; + return maple_scom_switch_freq(index); }
Now that all drivers providing ->target_index() callback are updated to use 'index' only for indexing into policy->freq_table, we can safely avoid keeping two separate freq-tables.
Which also means that cpufreq core doesn't use the freq_table passed to cpufreq_table_validate_and_show(), once that routine has returned.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 6 +----- drivers/cpufreq/freq_table.c | 36 ++++++++++++++---------------------- include/linux/cpufreq.h | 9 ++------- 3 files changed, 17 insertions(+), 34 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 10c5f7abc205..47983cb0601d 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1143,7 +1143,7 @@ static void cpufreq_policy_exit(struct cpufreq_policy *policy) return;
cpufreq_driver->exit(policy); - free_sorted_freq_table(policy); + kfree(policy->freq_table); policy->freq_table = NULL; }
@@ -1188,10 +1188,6 @@ static int cpufreq_online(unsigned int cpu) goto out_free_policy; }
- ret = create_sorted_freq_table(policy); - if (ret) - goto out_exit_policy; - down_write(&policy->rwsem);
if (new_policy) { diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c index 15c4a2462c68..7afe2c017267 100644 --- a/drivers/cpufreq/freq_table.c +++ b/drivers/cpufreq/freq_table.c @@ -350,20 +350,11 @@ static int next_larger(struct cpufreq_policy *policy, unsigned int freq, return index; }
-void free_sorted_freq_table(struct cpufreq_policy *policy) -{ - kfree(policy->sorted_freq_table); - policy->sorted_freq_table = NULL; -} - -int create_sorted_freq_table(struct cpufreq_policy *policy) +static int create_sorted_freq_table(struct cpufreq_policy *policy, + struct cpufreq_frequency_table *table) { struct cpufreq_frequency_table *pos, *new_table; unsigned int freq, index, i, count = 0; - struct cpufreq_frequency_table *table = policy->freq_table; - - if (!table) - return 0;
cpufreq_for_each_valid_entry(pos, table) count++; @@ -380,31 +371,32 @@ int create_sorted_freq_table(struct cpufreq_policy *policy) if (index == -EINVAL) break;
- /* - * driver_data of the sorted table points to the index of the - * unsorted table. - */ - new_table[i].driver_data = index; new_table[i].frequency = table[index].frequency; + new_table[i].driver_data = table[index].driver_data; + new_table[i].flags = table[index].flags;
freq = table[index].frequency; }
new_table[i].frequency = CPUFREQ_TABLE_END; - policy->sorted_freq_table = new_table; + policy->freq_table = new_table;
return 0; }
int cpufreq_table_validate_and_show(struct cpufreq_policy *policy, - struct cpufreq_frequency_table *table) + struct cpufreq_frequency_table *table) { - int ret = cpufreq_frequency_table_cpuinfo(policy, table); + int ret; + + if (!table) + return -EINVAL;
- if (!ret) - policy->freq_table = table; + ret = cpufreq_frequency_table_cpuinfo(policy, table); + if (ret) + return ret;
- return ret; + return create_sorted_freq_table(policy, table); } EXPORT_SYMBOL_GPL(cpufreq_table_validate_and_show);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 5aabec611e87..9df7c569cfbb 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -87,7 +87,6 @@ struct cpufreq_policy {
struct cpufreq_user_policy user_policy; struct cpufreq_frequency_table *freq_table; - struct cpufreq_frequency_table *sorted_freq_table;
struct list_head policy_list; struct kobject kobj; @@ -593,8 +592,6 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy, struct cpufreq_frequency_table *table); -int create_sorted_freq_table(struct cpufreq_policy *policy); -void free_sorted_freq_table(struct cpufreq_policy *policy);
int cpufreq_frequency_table_verify(struct cpufreq_policy *policy, struct cpufreq_frequency_table *table); @@ -616,10 +613,8 @@ static inline int cpufreq_frequency_table_target(struct cpufreq_policy *policy, unsigned int target_freq, unsigned int relation) { - int index = cpufreq_find_target_index(policy, policy->sorted_freq_table, - target_freq, relation); - - return policy->sorted_freq_table[index].driver_data; + return cpufreq_find_target_index(policy, policy->freq_table, + target_freq, relation); }
#ifdef CONFIG_CPU_FREQ
The cpufreq core doesn't use these tables anymore after cpufreq_table_validate_and_show() has returned. And so these can be freed early.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/acpi-cpufreq.c | 7 +++---- drivers/cpufreq/at32ap-cpufreq.c | 6 +++--- drivers/cpufreq/cpufreq-dt.c | 9 ++++----- drivers/cpufreq/e_powersaver.c | 24 ++++++++++++++---------- drivers/cpufreq/ia64-acpi-cpufreq.c | 7 +++---- drivers/cpufreq/loongson1-cpufreq.c | 10 +--------- 6 files changed, 28 insertions(+), 35 deletions(-)
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index 4f9f7504a17c..047425bcaa84 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -814,8 +814,10 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy) perf->state = 0;
result = cpufreq_table_validate_and_show(policy, freq_table); + kfree(freq_table); + if (result) - goto err_freqfree; + goto err_unreg;
if (perf->states[0].core_frequency * 1000 != policy->cpuinfo.max_freq) pr_warn(FW_WARN "P-state 0 is not max freq\n"); @@ -859,8 +861,6 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
return result;
-err_freqfree: - kfree(freq_table); err_unreg: acpi_processor_unregister_performance(cpu); err_free_mask: @@ -882,7 +882,6 @@ static int acpi_cpufreq_cpu_exit(struct cpufreq_policy *policy) policy->driver_data = NULL; acpi_processor_unregister_performance(data->acpi_perf_cpu); free_cpumask_var(data->freqdomain_cpus); - kfree(policy->freq_table); kfree(data);
return 0; diff --git a/drivers/cpufreq/at32ap-cpufreq.c b/drivers/cpufreq/at32ap-cpufreq.c index 9231b1efb70d..c9751572ac8b 100644 --- a/drivers/cpufreq/at32ap-cpufreq.c +++ b/drivers/cpufreq/at32ap-cpufreq.c @@ -21,8 +21,6 @@ #include <linux/export.h> #include <linux/slab.h>
-static struct cpufreq_frequency_table *freq_table; - static unsigned int ref_freq; static unsigned long loops_per_jiffy_ref;
@@ -51,6 +49,7 @@ static int at32_set_target(struct cpufreq_policy *policy, unsigned int index)
static int at32_cpufreq_driver_init(struct cpufreq_policy *policy) { + struct cpufreq_frequency_table *freq_table; unsigned int frequency, rate, min_freq; struct clk *cpuclk; int retval, steps, i; @@ -99,12 +98,13 @@ static int at32_cpufreq_driver_init(struct cpufreq_policy *policy) freq_table[steps - 1].frequency = CPUFREQ_TABLE_END;
retval = cpufreq_table_validate_and_show(policy, freq_table); + kfree(freq_table); + if (!retval) { printk("cpufreq: AT32AP CPU frequency driver\n"); return 0; }
- kfree(freq_table); out_err_put_clk: clk_put(cpuclk); out_err: diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 3957de801ae8..d46741b69c59 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -253,10 +253,12 @@ static int cpufreq_init(struct cpufreq_policy *policy) rcu_read_unlock();
ret = cpufreq_table_validate_and_show(policy, freq_table); + dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table); + if (ret) { dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__, ret); - goto out_free_cpufreq_table; + goto out_free_priv; }
/* Support turbo/boost mode */ @@ -264,7 +266,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) /* This gets disabled by core on driver unregister */ ret = cpufreq_enable_boost_support(); if (ret) - goto out_free_cpufreq_table; + goto out_free_priv; cpufreq_dt_attr[1] = &cpufreq_freq_attr_scaling_boost_freqs; }
@@ -276,8 +278,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
return 0;
-out_free_cpufreq_table: - dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table); out_free_priv: kfree(priv); out_free_opp: @@ -295,7 +295,6 @@ static int cpufreq_exit(struct cpufreq_policy *policy) struct private_data *priv = policy->driver_data;
cpufreq_cooling_unregister(priv->cdev); - dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); dev_pm_opp_of_cpumask_remove_table(policy->related_cpus); if (priv->reg_name) dev_pm_opp_put_regulator(priv->cpu_dev); diff --git a/drivers/cpufreq/e_powersaver.c b/drivers/cpufreq/e_powersaver.c index a284bddfb067..6c6090492889 100644 --- a/drivers/cpufreq/e_powersaver.c +++ b/drivers/cpufreq/e_powersaver.c @@ -38,7 +38,6 @@ struct eps_cpu_data { #if IS_ENABLED(CONFIG_ACPI_PROCESSOR) u32 bios_limit; #endif - struct cpufreq_frequency_table freq_table[]; };
static struct eps_cpu_data *eps_cpu[NR_CPUS]; @@ -324,11 +323,17 @@ static int eps_cpu_init(struct cpufreq_policy *policy) states = 2;
/* Allocate private data and frequency table for current cpu */ - centaur = kzalloc(sizeof(*centaur) - + (states + 1) * sizeof(struct cpufreq_frequency_table), - GFP_KERNEL); + centaur = kzalloc(sizeof(*centaur), GFP_KERNEL); if (!centaur) return -ENOMEM; + + f_table = kzalloc((states + 1) * sizeof(struct cpufreq_frequency_table), + GFP_KERNEL); + if (!f_table) { + kfree(centaur); + return -ENOMEM; + } + eps_cpu[0] = centaur;
/* Copy basic values */ @@ -338,7 +343,6 @@ static int eps_cpu_init(struct cpufreq_policy *policy) #endif
/* Fill frequency and MSR value table */ - f_table = ¢aur->freq_table[0]; if (brand != EPS_BRAND_C7M) { f_table[0].frequency = fsb * min_multiplier; f_table[0].driver_data = (min_multiplier << 8) | min_voltage; @@ -360,13 +364,13 @@ static int eps_cpu_init(struct cpufreq_policy *policy)
policy->cpuinfo.transition_latency = 140000; /* 844mV -> 700mV in ns */
- ret = cpufreq_table_validate_and_show(policy, ¢aur->freq_table[0]); - if (ret) { + ret = cpufreq_table_validate_and_show(policy, f_table); + if (ret) kfree(centaur); - return ret; - }
- return 0; + kfree(f_table); + + return ret; }
static int eps_cpu_exit(struct cpufreq_policy *policy) diff --git a/drivers/cpufreq/ia64-acpi-cpufreq.c b/drivers/cpufreq/ia64-acpi-cpufreq.c index cc8bb1e5ac50..10e3bfac84d5 100644 --- a/drivers/cpufreq/ia64-acpi-cpufreq.c +++ b/drivers/cpufreq/ia64-acpi-cpufreq.c @@ -292,8 +292,10 @@ acpi_cpufreq_cpu_init ( }
result = cpufreq_table_validate_and_show(policy, freq_table); + kfree(freq_table); + if (result) { - goto err_freqfree; + goto err_unreg; }
/* notify BIOS that we exist */ @@ -317,8 +319,6 @@ acpi_cpufreq_cpu_init (
return (result);
- err_freqfree: - kfree(freq_table); err_unreg: acpi_processor_unregister_performance(cpu); err_free: @@ -340,7 +340,6 @@ acpi_cpufreq_cpu_exit ( if (data) { acpi_io_data[policy->cpu] = NULL; acpi_processor_unregister_performance(policy->cpu); - kfree(policy->freq_table); kfree(data); }
diff --git a/drivers/cpufreq/loongson1-cpufreq.c b/drivers/cpufreq/loongson1-cpufreq.c index be89416e2358..2d35d3cc2ad8 100644 --- a/drivers/cpufreq/loongson1-cpufreq.c +++ b/drivers/cpufreq/loongson1-cpufreq.c @@ -103,18 +103,11 @@ static int ls1x_cpufreq_init(struct cpufreq_policy *policy)
policy->clk = cpufreq->clk; ret = cpufreq_generic_init(policy, freq_tbl, 0); - if (ret) - kfree(freq_tbl); + kfree(freq_tbl);
return ret; }
-static int ls1x_cpufreq_exit(struct cpufreq_policy *policy) -{ - kfree(policy->freq_table); - return 0; -} - static struct cpufreq_driver ls1x_cpufreq_driver = { .name = "cpufreq-ls1x", .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK, @@ -122,7 +115,6 @@ static struct cpufreq_driver ls1x_cpufreq_driver = { .target_index = ls1x_cpufreq_target, .get = cpufreq_generic_get, .init = ls1x_cpufreq_init, - .exit = ls1x_cpufreq_exit, .attr = cpufreq_generic_attr, };
On Thu, Jun 2, 2016 at 4:19 PM, Viresh Kumar viresh.kumar@linaro.org wrote:
Hi Rafael,
This series fixes all cpufreq drivers that provide a 'target_index' callback or in other words, which provide a freq-table to cpufreq core, to make sure they *only* use the 'index' argument to ->target_index() with the policy->freq_table.
This change allows us to remove the (duplicate) sorted-freq-table, which was added by following series:
[PATCH V2 0/2] cpufreq: Use sorted frequency tables
Which I'm not going to apply.
If this series depended on that one, please rework it.
Thanks, Rafael
On 2 June 2016 at 20:38, Rafael J. Wysocki rafael@kernel.org wrote:
On Thu, Jun 2, 2016 at 4:19 PM, Viresh Kumar viresh.kumar@linaro.org wrote:
Hi Rafael,
This series fixes all cpufreq drivers that provide a 'target_index' callback or in other words, which provide a freq-table to cpufreq core, to make sure they *only* use the 'index' argument to ->target_index() with the policy->freq_table.
This change allows us to remove the (duplicate) sorted-freq-table, which was added by following series:
[PATCH V2 0/2] cpufreq: Use sorted frequency tables
Which I'm not going to apply.
If this series depended on that one, please rework it.
Hi Rafael,
The first 9 patches can be applied without any dependency on the earlier series, but not the last two.
The target of this series is to make freq-table lookup faster and so most of its patches would make sense only if we are trying to solve that problem. Otherwise they may not be required at all.
I hope that you will be replying to the earlier series [1] as well, to convey the concerns you have with it.
Thanks
-- viresh
[1] [PATCH V2 0/2] cpufreq: Use sorted frequency tables
On Thu, Jun 2, 2016 at 5:42 PM, Viresh Kumar viresh.kumar@linaro.org wrote:
On 2 June 2016 at 20:38, Rafael J. Wysocki rafael@kernel.org wrote:
On Thu, Jun 2, 2016 at 4:19 PM, Viresh Kumar viresh.kumar@linaro.org wrote:
Hi Rafael,
This series fixes all cpufreq drivers that provide a 'target_index' callback or in other words, which provide a freq-table to cpufreq core, to make sure they *only* use the 'index' argument to ->target_index() with the policy->freq_table.
This change allows us to remove the (duplicate) sorted-freq-table, which was added by following series:
[PATCH V2 0/2] cpufreq: Use sorted frequency tables
Which I'm not going to apply.
If this series depended on that one, please rework it.
Hi Rafael,
The first 9 patches can be applied without any dependency on the earlier series, but not the last two.
The target of this series is to make freq-table lookup faster and so most of its patches would make sense only if we are trying to solve that problem. Otherwise they may not be required at all.
Well, in that case I'm not sure about them.
I hope that you will be replying to the earlier series [1] as well, to convey the concerns you have with it.
Oh, that's very simple: I don't see a reason to apply that series.
Quoting from this very cover letter "This change allows us to remove the (duplicate) sorted-freq-table, which was added by following series:", so why to add it in the first place?
Besides, there already is a number of tables (per policy which in some important cases pretty much means per CPU) in cpufreq that contain more-or-less the same information. For example, if acpi-cpufreq is in use, the ACPI layer has a table coming from _PSS, the driver creates freq_table to pass to the core and there is an additional one for the stats. And your series adds one more just so it is ordered. Come on.
If you want to clean that up, fine, but please don't do that in a hurry. Let's talk about it a bit more without sending any more patches in that area for the time being.
At this point, I'm lost in the patches you've been sending for the last couple of days. I don't know what depends on what, what will be updated and so on and quite frankly I don't really have the time to figure that out. Overall, the most recent burst of activity regarding the sorting of frequency tables hasn't been very helpful from my perspective.
If you have cleanups that look obvious and don't interfere with anything already posted by Steve, please send them *again* in one ordered series and let everybody know whom you would like to apply them etc.
And now I'm going to look at the Steve's patches again and start over from there.
Thanks, Rafael
On 02-06-16, 22:35, Rafael J. Wysocki wrote:
Quoting from this very cover letter "This change allows us to remove the (duplicate) sorted-freq-table, which was added by following series:", so why to add it in the first place?
Okay, that's fine.
Besides, there already is a number of tables (per policy which in some important cases pretty much means per CPU) in cpufreq that contain more-or-less the same information. For example, if acpi-cpufreq is in use, the ACPI layer has a table coming from _PSS, the driver creates freq_table to pass to the core and there is an additional one for the stats. And your series adds one more just so it is ordered. Come on.
Of course.
If you want to clean that up, fine, but please don't do that in a hurry. Let's talk about it a bit more without sending any more patches in that area for the time being.
Okay, I will send all the fixes that you can apply cleanly now in a separate set.
So, yeah, I get your overall concern. What about this: - A single patchset to make sure the current policy->freq_table is always sorted in Ascending order of frequencies. - And this sorting will be done per policy only when the policy is first created. - Which would eventually mean merging this series with the [v2 0/2] one.
Will that work ?
On Friday, June 03, 2016 05:31:34 AM Viresh Kumar wrote:
On 02-06-16, 22:35, Rafael J. Wysocki wrote:
Quoting from this very cover letter "This change allows us to remove the (duplicate) sorted-freq-table, which was added by following series:", so why to add it in the first place?
Okay, that's fine.
Besides, there already is a number of tables (per policy which in some important cases pretty much means per CPU) in cpufreq that contain more-or-less the same information. For example, if acpi-cpufreq is in use, the ACPI layer has a table coming from _PSS, the driver creates freq_table to pass to the core and there is an additional one for the stats. And your series adds one more just so it is ordered. Come on.
Of course.
If you want to clean that up, fine, but please don't do that in a hurry. Let's talk about it a bit more without sending any more patches in that area for the time being.
Okay, I will send all the fixes that you can apply cleanly now in a separate set.
Thanks!
So, yeah, I get your overall concern. What about this:
- A single patchset to make sure the current policy->freq_table is always sorted in Ascending order of frequencies.
Be careful here. acpi-cpufreq sorts the table in the descending order and at least acpi_cpufreq_fast_switch() assumes that.
- And this sorting will be done per policy only when the policy is first created.
- Which would eventually mean merging this series with the [v2 0/2] one.
Will that work ?
Well, it may. :-)
I would like you to talk to Steve and agree on the approach, including which changes to make first, though. You are both from Linaro after all ...
Thanks, Rafael
On 03-06-16, 03:43, Rafael J. Wysocki wrote:
On Friday, June 03, 2016 05:31:34 AM Viresh Kumar wrote:
So, yeah, I get your overall concern. What about this:
- A single patchset to make sure the current policy->freq_table is always sorted in Ascending order of frequencies.
Be careful here. acpi-cpufreq sorts the table in the descending order and at least acpi_cpufreq_fast_switch() assumes that.
Yeah, it was already fixed in [V2 0/2] series. Thanks.
- And this sorting will be done per policy only when the policy is first created.
- Which would eventually mean merging this series with the [v2 0/2] one.
Will that work ?
Well, it may. :-)
I would like you to talk to Steve and agree on the approach, including which changes to make first, though. You are both from Linaro after all ...
Sure, we can get that done and I am not particular here on whose patches should get in first. The outcome should be same whatever order we follow :)
Thanks.
linaro-kernel@lists.linaro.org
-
Rafael J. Wysocki -
Rafael J. Wysocki -
Viresh Kumar