The driver support single core and multi core ARM SoCs. For multi core, it assume all cores share the same clock and voltage.
TODO: - Add each core seperate freq/volt support (MSM).
Changes in v3: - move adjusting smp loops_per_jiffy to arm common code, and also adjust global loops_per_jiffy. - remove adjusting loops_per_jiffy in imx and omap cpufreq drivers. - check compatible "generic-cpufreq" when module_init - change printk to pr_xxx - add generic-cpufreq DT binding doc
Changes in v2: - add volatage change support - change '_' in property name to '-' - use initial value to calculate loops_per_jiffy - fix reading cpu_volts property bug - let cpufreq_frequency_table_cpuinfo routines handle cpu_freq_khz_max/min - don't change freq in arm_cpufreq_exit, because every core share the same freq. - use unsigned long describe frequency as much as possible. Because clk use unsigned long, but cpufreq use unsigned int. [PATCH V3 1/7] ARM: add cpufreq transiton notifier to adjust [PATCH V3 2/7] arm/imx: cpufreq: remove loops_per_jiffy recalculate [PATCH V3 3/7] cpufreq: OMAP: remove loops_per_jiffy recalculate for [PATCH V3 4/7] cpufreq: add generic cpufreq driver [PATCH V3 5/7] dts/imx6q: add cpufreq property [PATCH V3 6/7] arm/imx6q: register arm_clk as cpu to clkdev [PATCH V3 7/7] arm/imx6q: select ARCH_HAS_CPUFREQ
If CONFIG_SMP, cpufreq skips loops_per_jiffy update, because different arch has different per-cpu loops_per_jiffy definition.
Signed-off-by: Richard Zhao richard.zhao@linaro.org --- arch/arm/kernel/smp.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 54 insertions(+), 0 deletions(-)
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c index ef5640b..ac9cadc 100644 --- a/arch/arm/kernel/smp.c +++ b/arch/arm/kernel/smp.c @@ -25,6 +25,7 @@ #include <linux/percpu.h> #include <linux/clockchips.h> #include <linux/completion.h> +#include <linux/cpufreq.h>
#include <linux/atomic.h> #include <asm/cacheflush.h> @@ -631,3 +632,56 @@ int setup_profiling_timer(unsigned int multiplier) { return -EINVAL; } + +#ifdef CONFIG_CPU_FREQ + +static DEFINE_PER_CPU(unsigned long, l_p_j_ref); +static DEFINE_PER_CPU(unsigned long, l_p_j_ref_freq); +static unsigned long global_l_p_j_ref; +static unsigned long global_l_p_j_ref_freq; + +static int cpufreq_callback(struct notifier_block *nb, + unsigned long val, void *data) +{ + struct cpufreq_freqs *freq = data; + int cpu = freq->cpu; + + if (freq->flags & CPUFREQ_CONST_LOOPS) + return NOTIFY_OK; + + if (!per_cpu(l_p_j_ref, cpu)) { + per_cpu(l_p_j_ref, cpu) = + per_cpu(cpu_data, cpu).loops_per_jiffy; + per_cpu(l_p_j_ref_freq, cpu) = freq->old; + if (!global_l_p_j_ref) { + global_l_p_j_ref = loops_per_jiffy; + global_l_p_j_ref_freq = freq->old; + } + } + + if ((val == CPUFREQ_PRECHANGE && freq->old < freq->new) || + (val == CPUFREQ_POSTCHANGE && freq->old > freq->new) || + (val == CPUFREQ_RESUMECHANGE || val == CPUFREQ_SUSPENDCHANGE)) { + loops_per_jiffy = cpufreq_scale(global_l_p_j_ref, + global_l_p_j_ref_freq, + freq->new); + per_cpu(cpu_data, cpu).loops_per_jiffy = + cpufreq_scale(per_cpu(l_p_j_ref, cpu), + per_cpu(l_p_j_ref_freq, cpu), + freq->new); + } + return NOTIFY_OK; +} + +static struct notifier_block cpufreq_notifier = { + .notifier_call = cpufreq_callback, +}; + +static int __init register_cpufreq_notifier(void) +{ + return cpufreq_register_notifier(&cpufreq_notifier, + CPUFREQ_TRANSITION_NOTIFIER); +} +core_initcall(register_cpufreq_notifier); + +#endif
Hi Russel,
Are the patch #1 #2 #3 ok for you?
Thanks Richard
arm registered cpufreq transition notifier to recalculate it.
Signed-off-by: Richard Zhao richard.zhao@linaro.org --- arch/arm/plat-mxc/cpufreq.c | 10 ---------- 1 files changed, 0 insertions(+), 10 deletions(-)
diff --git a/arch/arm/plat-mxc/cpufreq.c b/arch/arm/plat-mxc/cpufreq.c index c937e75..364793a 100644 --- a/arch/arm/plat-mxc/cpufreq.c +++ b/arch/arm/plat-mxc/cpufreq.c @@ -99,16 +99,6 @@ static int mxc_set_target(struct cpufreq_policy *policy,
ret = set_cpu_freq(freq_Hz);
-#ifdef CONFIG_SMP - /* loops_per_jiffy is not updated by the cpufreq core for SMP systems. - * So update it for all CPUs. - */ - for_each_possible_cpu(cpu) - per_cpu(cpu_data, cpu).loops_per_jiffy = - cpufreq_scale(per_cpu(cpu_data, cpu).loops_per_jiffy, - freqs.old, freqs.new); -#endif - for_each_possible_cpu(cpu) { freqs.cpu = cpu; cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
arm registered cpufreq transition notifier to recalculate it.
Signed-off-by: Richard Zhao richard.zhao@linaro.org --- drivers/cpufreq/omap-cpufreq.c | 36 ------------------------------------ 1 files changed, 0 insertions(+), 36 deletions(-)
diff --git a/drivers/cpufreq/omap-cpufreq.c b/drivers/cpufreq/omap-cpufreq.c index 5d04c57..17da4c4 100644 --- a/drivers/cpufreq/omap-cpufreq.c +++ b/drivers/cpufreq/omap-cpufreq.c @@ -37,16 +37,6 @@
#include <mach/hardware.h>
-#ifdef CONFIG_SMP -struct lpj_info { - unsigned long ref; - unsigned int freq; -}; - -static DEFINE_PER_CPU(struct lpj_info, lpj_ref); -static struct lpj_info global_lpj_ref; -#endif - static struct cpufreq_frequency_table *freq_table; static atomic_t freq_table_users = ATOMIC_INIT(0); static struct clk *mpu_clk; @@ -118,32 +108,6 @@ static int omap_target(struct cpufreq_policy *policy, ret = clk_set_rate(mpu_clk, freqs.new * 1000); freqs.new = omap_getspeed(policy->cpu);
-#ifdef CONFIG_SMP - /* - * Note that loops_per_jiffy is not updated on SMP systems in - * cpufreq driver. So, update the per-CPU loops_per_jiffy value - * on frequency transition. We need to update all dependent CPUs. - */ - for_each_cpu(i, policy->cpus) { - struct lpj_info *lpj = &per_cpu(lpj_ref, i); - if (!lpj->freq) { - lpj->ref = per_cpu(cpu_data, i).loops_per_jiffy; - lpj->freq = freqs.old; - } - - per_cpu(cpu_data, i).loops_per_jiffy = - cpufreq_scale(lpj->ref, lpj->freq, freqs.new); - } - - /* And don't forget to adjust the global one */ - if (!global_lpj_ref.freq) { - global_lpj_ref.ref = loops_per_jiffy; - global_lpj_ref.freq = freqs.old; - } - loops_per_jiffy = cpufreq_scale(global_lpj_ref.ref, global_lpj_ref.freq, - freqs.new); -#endif - /* notifiers */ for_each_cpu(i, policy->cpus) { freqs.cpu = i;
It support single core and multi-core ARM SoCs. But currently it assume all cores share the same frequency and voltage.
Signed-off-by: Richard Zhao richard.zhao@linaro.org --- .../devicetree/bindings/cpufreq/generic-cpufreq | 7 + drivers/cpufreq/Kconfig | 8 + drivers/cpufreq/Makefile | 2 + drivers/cpufreq/generic-cpufreq.c | 251 ++++++++++++++++++++ 4 files changed, 268 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq create mode 100644 drivers/cpufreq/generic-cpufreq.c
diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq new file mode 100644 index 0000000..15dd780 --- /dev/null +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq @@ -0,0 +1,7 @@ +Generic cpufreq driver + +Required properties in /cpus/cpu@0: +- compatible : "generic-cpufreq" +- cpu-freqs : cpu frequency points it support +- cpu-volts : cpu voltages required by the frequency point at the same index +- trans-latency : transition_latency diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig index e24a2a1..216eecd 100644 --- a/drivers/cpufreq/Kconfig +++ b/drivers/cpufreq/Kconfig @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
If in doubt, say N.
+config GENERIC_CPUFREQ_DRIVER + bool "Generic cpufreq driver using clock/regulator/devicetree" + help + This adds generic CPUFreq driver. It assumes all + cores of the CPU share the same clock and voltage. + + If in doubt, say N. + menu "x86 CPU frequency scaling drivers" depends on X86 source "drivers/cpufreq/Kconfig.x86" diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile index ce75fcb..2dbdab1 100644 --- a/drivers/cpufreq/Makefile +++ b/drivers/cpufreq/Makefile @@ -13,6 +13,8 @@ obj-$(CONFIG_CPU_FREQ_GOV_CONSERVATIVE) += cpufreq_conservative.o # CPUfreq cross-arch helpers obj-$(CONFIG_CPU_FREQ_TABLE) += freq_table.o
+obj-$(CONFIG_GENERIC_CPUFREQ_DRIVER) += generic-cpufreq.o + ################################################################################## # x86 drivers. # Link order matters. K8 is preferred to ACPI because of firmware bugs in early diff --git a/drivers/cpufreq/generic-cpufreq.c b/drivers/cpufreq/generic-cpufreq.c new file mode 100644 index 0000000..781bb9b --- /dev/null +++ b/drivers/cpufreq/generic-cpufreq.c @@ -0,0 +1,251 @@ +/* + * Copyright (C) 2011 Freescale Semiconductor, Inc. + */ + +/* + * The code contained herein is licensed under the GNU General Public + * License. You may obtain a copy of the GNU General Public License + * Version 2 or later at the following locations: + * + * http://www.opensource.org/licenses/gpl-license.html + * http://www.gnu.org/copyleft/gpl.html + */ + +#include <linux/module.h> +#include <linux/cpufreq.h> +#include <linux/clk.h> +#include <linux/regulator/consumer.h> +#include <linux/err.h> +#include <linux/slab.h> +#include <linux/of.h> + +static u32 *cpu_freqs; /* HZ */ +static u32 *cpu_volts; /* uV */ +static u32 trans_latency; /* ns */ +static int cpu_op_nr; + +static struct clk *cpu_clk; +static struct regulator *cpu_reg; +static struct cpufreq_frequency_table *freq_table; + +static int set_cpu_freq(unsigned long freq, int index, int higher) +{ + int ret = 0; + + if (higher && cpu_reg) + regulator_set_voltage(cpu_reg, + cpu_volts[index], cpu_volts[index]); + + ret = clk_set_rate(cpu_clk, freq); + if (ret != 0) { + pr_err("generic-cpufreq: cannot set CPU clock rate\n"); + return ret; + } + + if (!higher && cpu_reg) + regulator_set_voltage(cpu_reg, + cpu_volts[index], cpu_volts[index]); + + return ret; +} + +static int generic_verify_speed(struct cpufreq_policy *policy) +{ + return cpufreq_frequency_table_verify(policy, freq_table); +} + +static unsigned int generic_get_speed(unsigned int cpu) +{ + return clk_get_rate(cpu_clk) / 1000; +} + +static int generic_set_target(struct cpufreq_policy *policy, + unsigned int target_freq, unsigned int relation) +{ + struct cpufreq_freqs freqs; + unsigned long freq_Hz; + int cpu; + int ret = 0; + unsigned int index; + + cpufreq_frequency_table_target(policy, freq_table, + target_freq, relation, &index); + freq_Hz = clk_round_rate(cpu_clk, cpu_freqs[index]); + freq_Hz = freq_Hz ? freq_Hz : cpu_freqs[index]; + freqs.old = clk_get_rate(cpu_clk) / 1000; + freqs.new = freq_Hz / 1000; + freqs.flags = 0; + + if (freqs.old == freqs.new) + return 0; + + for_each_possible_cpu(cpu) { + freqs.cpu = cpu; + cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE); + } + + ret = set_cpu_freq(freq_Hz, index, (freqs.new > freqs.old)); + + for_each_possible_cpu(cpu) { + freqs.cpu = cpu; + cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE); + } + + return ret; +} + +static int generic_cpufreq_init(struct cpufreq_policy *policy) +{ + int ret; + + if (policy->cpu >= num_possible_cpus()) + return -EINVAL; + + policy->cur = clk_get_rate(cpu_clk) / 1000; + policy->shared_type = CPUFREQ_SHARED_TYPE_ANY; + cpumask_setall(policy->cpus); + /* Manual states, that PLL stabilizes in two CLK32 periods */ + policy->cpuinfo.transition_latency = trans_latency; + + ret = cpufreq_frequency_table_cpuinfo(policy, freq_table); + + if (ret < 0) { + pr_err("%s: invalid frequency table for cpu %d\n", + __func__, policy->cpu); + return ret; + } + + cpufreq_frequency_table_get_attr(freq_table, policy->cpu); + return 0; +} + +static int generic_cpufreq_exit(struct cpufreq_policy *policy) +{ + cpufreq_frequency_table_put_attr(policy->cpu); + return 0; +} + +static struct cpufreq_driver generic_cpufreq_driver = { + .flags = CPUFREQ_STICKY, + .verify = generic_verify_speed, + .target = generic_set_target, + .get = generic_get_speed, + .init = generic_cpufreq_init, + .exit = generic_cpufreq_exit, + .name = "generic", +}; + +static int __devinit generic_cpufreq_driver_init(void) +{ + struct device_node *cpu0; + const struct property *pp; + int i, ret; + + pr_info("Generic CPU frequency driver\n"); + + cpu0 = of_find_node_by_path("/cpus/cpu@0"); + if (!cpu0) + return -ENODEV; + + if (!of_device_is_compatible(cpu0, "generic-cpufreq")) + return -ENODEV; + + pp = of_find_property(cpu0, "cpu-freqs", NULL); + if (!pp) { + ret = -ENODEV; + goto put_node; + } + cpu_op_nr = pp->length / sizeof(u32); + if (!cpu_op_nr) { + ret = -ENODEV; + goto put_node; + } + ret = -ENOMEM; + cpu_freqs = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr, GFP_KERNEL); + if (!cpu_freqs) + goto put_node; + of_property_read_u32_array(cpu0, "cpu-freqs", cpu_freqs, cpu_op_nr); + + pp = of_find_property(cpu0, "cpu-volts", NULL); + if (pp) { + if (cpu_op_nr == pp->length / sizeof(u32)) { + cpu_volts = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr, + GFP_KERNEL); + if (!cpu_volts) + goto free_cpu_freqs; + of_property_read_u32_array(cpu0, "cpu-volts", + cpu_volts, cpu_op_nr); + } else + pr_warn("%s: invalid cpu_volts!\n", __func__); + } + + if (of_property_read_u32(cpu0, "trans-latency", &trans_latency)) + trans_latency = CPUFREQ_ETERNAL; + + freq_table = kmalloc(sizeof(struct cpufreq_frequency_table) + * (cpu_op_nr + 1), GFP_KERNEL); + if (!freq_table) + goto free_cpu_volts; + + for (i = 0; i < cpu_op_nr; i++) { + freq_table[i].index = i; + freq_table[i].frequency = cpu_freqs[i] / 1000; + } + + freq_table[i].index = i; + freq_table[i].frequency = CPUFREQ_TABLE_END; + + cpu_clk = clk_get(NULL, "cpu"); + if (IS_ERR(cpu_clk)) { + pr_err("%s: failed to get cpu clock\n", __func__); + ret = PTR_ERR(cpu_clk); + goto free_freq_table; + } + + if (cpu_volts) { + cpu_reg = regulator_get(NULL, "cpu"); + if (IS_ERR(cpu_reg)) { + pr_warn("%s: regulator cpu get failed.\n", __func__); + cpu_reg = NULL; + } + } + + ret = cpufreq_register_driver(&generic_cpufreq_driver); + if (ret) + goto reg_put; + + of_node_put(cpu0); + + return 0; + +reg_put: + if (cpu_reg) + regulator_put(cpu_reg); + clk_put(cpu_clk); +free_freq_table: + kfree(freq_table); +free_cpu_volts: + kfree(cpu_volts); +free_cpu_freqs: + kfree(cpu_freqs); +put_node: + of_node_put(cpu0); + + return ret; +} + +static void generic_cpufreq_driver_exit(void) +{ + cpufreq_unregister_driver(&generic_cpufreq_driver); + kfree(cpu_freqs); + kfree(cpu_volts); + kfree(freq_table); + clk_put(cpu_clk); +} + +module_init(generic_cpufreq_driver_init); +module_exit(generic_cpufreq_driver_exit); + +MODULE_AUTHOR("Freescale Semiconductor Inc. Richard Zhao richard.zhao@freescale.com"); +MODULE_DESCRIPTION("Generic CPUFreq driver"); +MODULE_LICENSE("GPL");
Hi Richard,
On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
It support single core and multi-core ARM SoCs. But currently it assume all cores share the same frequency and voltage.
Signed-off-by: Richard Zhao richard.zhao@linaro.org
.../devicetree/bindings/cpufreq/generic-cpufreq | 7 + drivers/cpufreq/Kconfig | 8 + drivers/cpufreq/Makefile | 2 + drivers/cpufreq/generic-cpufreq.c | 251 ++++++++++++++++++++ 4 files changed, 268 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq create mode 100644 drivers/cpufreq/generic-cpufreq.c
diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq new file mode 100644 index 0000000..15dd780 --- /dev/null +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq @@ -0,0 +1,7 @@ +Generic cpufreq driver
+Required properties in /cpus/cpu@0: +- compatible : "generic-cpufreq"
I'm not convinced this is the best way to do this. By requiring a generic-cpufreq compatible string we're encoding Linux driver information into the hardware description. The only way I can see to avoid this is to provide a generic_clk_cpufreq_init() function that platforms can call in their machine init code to use the driver.
+- cpu-freqs : cpu frequency points it support +- cpu-volts : cpu voltages required by the frequency point at the same index +- trans-latency : transition_latency diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig index e24a2a1..216eecd 100644 --- a/drivers/cpufreq/Kconfig +++ b/drivers/cpufreq/Kconfig @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE If in doubt, say N. +config GENERIC_CPUFREQ_DRIVER
- bool "Generic cpufreq driver using clock/regulator/devicetree"
- help
This adds generic CPUFreq driver. It assumes allcores of the CPU share the same clock and voltage.If in doubt, say N.
I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
menu "x86 CPU frequency scaling drivers" depends on X86 source "drivers/cpufreq/Kconfig.x86" diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile index ce75fcb..2dbdab1 100644 --- a/drivers/cpufreq/Makefile +++ b/drivers/cpufreq/Makefile @@ -13,6 +13,8 @@ obj-$(CONFIG_CPU_FREQ_GOV_CONSERVATIVE) += cpufreq_conservative.o # CPUfreq cross-arch helpers obj-$(CONFIG_CPU_FREQ_TABLE) += freq_table.o +obj-$(CONFIG_GENERIC_CPUFREQ_DRIVER) += generic-cpufreq.o
################################################################################## # x86 drivers. # Link order matters. K8 is preferred to ACPI because of firmware bugs in early diff --git a/drivers/cpufreq/generic-cpufreq.c b/drivers/cpufreq/generic-cpufreq.c new file mode 100644 index 0000000..781bb9b --- /dev/null +++ b/drivers/cpufreq/generic-cpufreq.c @@ -0,0 +1,251 @@ +/*
- Copyright (C) 2011 Freescale Semiconductor, Inc.
- */
+/*
- The code contained herein is licensed under the GNU General Public
- License. You may obtain a copy of the GNU General Public License
- Version 2 or later at the following locations:
- */
+#include <linux/module.h> +#include <linux/cpufreq.h> +#include <linux/clk.h> +#include <linux/regulator/consumer.h> +#include <linux/err.h> +#include <linux/slab.h> +#include <linux/of.h>
+static u32 *cpu_freqs; /* HZ */ +static u32 *cpu_volts; /* uV */ +static u32 trans_latency; /* ns */ +static int cpu_op_nr;
+static struct clk *cpu_clk; +static struct regulator *cpu_reg; +static struct cpufreq_frequency_table *freq_table;
+static int set_cpu_freq(unsigned long freq, int index, int higher) +{
- int ret = 0;
- if (higher && cpu_reg)
regulator_set_voltage(cpu_reg,cpu_volts[index], cpu_volts[index]);- ret = clk_set_rate(cpu_clk, freq);
- if (ret != 0) {
pr_err("generic-cpufreq: cannot set CPU clock rate\n");return ret;- }
- if (!higher && cpu_reg)
regulator_set_voltage(cpu_reg,cpu_volts[index], cpu_volts[index]);- return ret;
+}
+static int generic_verify_speed(struct cpufreq_policy *policy) +{
- return cpufreq_frequency_table_verify(policy, freq_table);
+}
+static unsigned int generic_get_speed(unsigned int cpu) +{
- return clk_get_rate(cpu_clk) / 1000;
+}
+static int generic_set_target(struct cpufreq_policy *policy,
unsigned int target_freq, unsigned int relation)+{
- struct cpufreq_freqs freqs;
- unsigned long freq_Hz;
- int cpu;
- int ret = 0;
- unsigned int index;
- cpufreq_frequency_table_target(policy, freq_table,
target_freq, relation, &index);- freq_Hz = clk_round_rate(cpu_clk, cpu_freqs[index]);
- freq_Hz = freq_Hz ? freq_Hz : cpu_freqs[index];
- freqs.old = clk_get_rate(cpu_clk) / 1000;
- freqs.new = freq_Hz / 1000;
- freqs.flags = 0;
- if (freqs.old == freqs.new)
return 0;- for_each_possible_cpu(cpu) {
freqs.cpu = cpu;cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);- }
- ret = set_cpu_freq(freq_Hz, index, (freqs.new > freqs.old));
If this fails then we'll still be notifying the transition at the requested rate even though it didn't work. I guess we should really get the rate of the clk and put that into freqs for the POSTCHANGE notification.
- for_each_possible_cpu(cpu) {
freqs.cpu = cpu;cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);- }
- return ret;
+}
+static int generic_cpufreq_init(struct cpufreq_policy *policy) +{
- int ret;
- if (policy->cpu >= num_possible_cpus())
return -EINVAL;- policy->cur = clk_get_rate(cpu_clk) / 1000;
- policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
- cpumask_setall(policy->cpus);
- /* Manual states, that PLL stabilizes in two CLK32 periods */
- policy->cpuinfo.transition_latency = trans_latency;
- ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
- if (ret < 0) {
pr_err("%s: invalid frequency table for cpu %d\n",__func__, policy->cpu);return ret;- }
- cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
- return 0;
+}
+static int generic_cpufreq_exit(struct cpufreq_policy *policy) +{
- cpufreq_frequency_table_put_attr(policy->cpu);
- return 0;
+}
+static struct cpufreq_driver generic_cpufreq_driver = {
- .flags = CPUFREQ_STICKY,
- .verify = generic_verify_speed,
- .target = generic_set_target,
- .get = generic_get_speed,
- .init = generic_cpufreq_init,
- .exit = generic_cpufreq_exit,
- .name = "generic",
This may be a little too generic? "generic-reg-clk"?
+};
+static int __devinit generic_cpufreq_driver_init(void) +{
- struct device_node *cpu0;
- const struct property *pp;
- int i, ret;
- pr_info("Generic CPU frequency driver\n");
- cpu0 = of_find_node_by_path("/cpus/cpu@0");
- if (!cpu0)
return -ENODEV;- if (!of_device_is_compatible(cpu0, "generic-cpufreq"))
return -ENODEV;
As above, I'd personally rather not use compatible strings, but if you do, then I think return 0 here rather than -ENODEV else I believe you'll get a potentially confusing message on the console for platforms that don't use this.
- pp = of_find_property(cpu0, "cpu-freqs", NULL);
- if (!pp) {
ret = -ENODEV;goto put_node;- }
- cpu_op_nr = pp->length / sizeof(u32);
- if (!cpu_op_nr) {
ret = -ENODEV;goto put_node;- }
- ret = -ENOMEM;
- cpu_freqs = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr, GFP_KERNEL);
- if (!cpu_freqs)
goto put_node;- of_property_read_u32_array(cpu0, "cpu-freqs", cpu_freqs, cpu_op_nr);
- pp = of_find_property(cpu0, "cpu-volts", NULL);
- if (pp) {
if (cpu_op_nr == pp->length / sizeof(u32)) {cpu_volts = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr,GFP_KERNEL);if (!cpu_volts)goto free_cpu_freqs;of_property_read_u32_array(cpu0, "cpu-volts",cpu_volts, cpu_op_nr);} elsepr_warn("%s: invalid cpu_volts!\n", __func__);- }
- if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
trans_latency = CPUFREQ_ETERNAL;- freq_table = kmalloc(sizeof(struct cpufreq_frequency_table)
* (cpu_op_nr + 1), GFP_KERNEL);- if (!freq_table)
goto free_cpu_volts;- for (i = 0; i < cpu_op_nr; i++) {
freq_table[i].index = i;freq_table[i].frequency = cpu_freqs[i] / 1000;- }
- freq_table[i].index = i;
- freq_table[i].frequency = CPUFREQ_TABLE_END;
- cpu_clk = clk_get(NULL, "cpu");
- if (IS_ERR(cpu_clk)) {
pr_err("%s: failed to get cpu clock\n", __func__);ret = PTR_ERR(cpu_clk);goto free_freq_table;- }
- if (cpu_volts) {
cpu_reg = regulator_get(NULL, "cpu");if (IS_ERR(cpu_reg)) {pr_warn("%s: regulator cpu get failed.\n", __func__);cpu_reg = NULL;}- }
- ret = cpufreq_register_driver(&generic_cpufreq_driver);
- if (ret)
goto reg_put;- of_node_put(cpu0);
- return 0;
+reg_put:
- if (cpu_reg)
regulator_put(cpu_reg);- clk_put(cpu_clk);
+free_freq_table:
- kfree(freq_table);
+free_cpu_volts:
- kfree(cpu_volts);
+free_cpu_freqs:
- kfree(cpu_freqs);
+put_node:
- of_node_put(cpu0);
- return ret;
+}
+static void generic_cpufreq_driver_exit(void) +{
- cpufreq_unregister_driver(&generic_cpufreq_driver);
- kfree(cpu_freqs);
- kfree(cpu_volts);
- kfree(freq_table);
- clk_put(cpu_clk);
Should this do something with the regulator too?
Jamie
On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
Hi Richard,
On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
It support single core and multi-core ARM SoCs. But currently it assume all cores share the same frequency and voltage.
Signed-off-by: Richard Zhao richard.zhao@linaro.org
.../devicetree/bindings/cpufreq/generic-cpufreq | 7 + drivers/cpufreq/Kconfig | 8 + drivers/cpufreq/Makefile | 2 + drivers/cpufreq/generic-cpufreq.c | 251 ++++++++++++++++++++ 4 files changed, 268 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq create mode 100644 drivers/cpufreq/generic-cpufreq.c
diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq new file mode 100644 index 0000000..15dd780 --- /dev/null +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq @@ -0,0 +1,7 @@ +Generic cpufreq driver
+Required properties in /cpus/cpu@0: +- compatible : "generic-cpufreq"
I'm not convinced this is the best way to do this. By requiring a generic-cpufreq compatible string we're encoding Linux driver information into the hardware description. The only way I can see to avoid this is to provide a generic_clk_cpufreq_init() function that platforms can call in their machine init code to use the driver.
It'll prevent the driver from being a kernel module.
Hi Grant & Rob,
Could you comment?
+- cpu-freqs : cpu frequency points it support +- cpu-volts : cpu voltages required by the frequency point at the same index +- trans-latency : transition_latency diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig index e24a2a1..216eecd 100644 --- a/drivers/cpufreq/Kconfig +++ b/drivers/cpufreq/Kconfig @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE If in doubt, say N. +config GENERIC_CPUFREQ_DRIVER
- bool "Generic cpufreq driver using clock/regulator/devicetree"
- help
This adds generic CPUFreq driver. It assumes allcores of the CPU share the same clock and voltage.If in doubt, say N.I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
right, Thanks. I can not check clk before generic clock framework come in. Added: depends on OF && REGULATOR select CPU_FREQ_TABLE
menu "x86 CPU frequency scaling drivers" depends on X86 source "drivers/cpufreq/Kconfig.x86" diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile index ce75fcb..2dbdab1 100644 --- a/drivers/cpufreq/Makefile +++ b/drivers/cpufreq/Makefile @@ -13,6 +13,8 @@ obj-$(CONFIG_CPU_FREQ_GOV_CONSERVATIVE) += cpufreq_conservative.o # CPUfreq cross-arch helpers obj-$(CONFIG_CPU_FREQ_TABLE) += freq_table.o +obj-$(CONFIG_GENERIC_CPUFREQ_DRIVER) += generic-cpufreq.o
################################################################################## # x86 drivers. # Link order matters. K8 is preferred to ACPI because of firmware bugs in early diff --git a/drivers/cpufreq/generic-cpufreq.c b/drivers/cpufreq/generic-cpufreq.c new file mode 100644 index 0000000..781bb9b --- /dev/null +++ b/drivers/cpufreq/generic-cpufreq.c @@ -0,0 +1,251 @@ +/*
- Copyright (C) 2011 Freescale Semiconductor, Inc.
- */
+/*
- The code contained herein is licensed under the GNU General Public
- License. You may obtain a copy of the GNU General Public License
- Version 2 or later at the following locations:
- */
+#include <linux/module.h> +#include <linux/cpufreq.h> +#include <linux/clk.h> +#include <linux/regulator/consumer.h> +#include <linux/err.h> +#include <linux/slab.h> +#include <linux/of.h>
+static u32 *cpu_freqs; /* HZ */ +static u32 *cpu_volts; /* uV */ +static u32 trans_latency; /* ns */ +static int cpu_op_nr;
+static struct clk *cpu_clk; +static struct regulator *cpu_reg; +static struct cpufreq_frequency_table *freq_table;
+static int set_cpu_freq(unsigned long freq, int index, int higher) +{
- int ret = 0;
- if (higher && cpu_reg)
regulator_set_voltage(cpu_reg,cpu_volts[index], cpu_volts[index]);- ret = clk_set_rate(cpu_clk, freq);
- if (ret != 0) {
pr_err("generic-cpufreq: cannot set CPU clock rate\n");return ret;- }
- if (!higher && cpu_reg)
regulator_set_voltage(cpu_reg,cpu_volts[index], cpu_volts[index]);- return ret;
+}
+static int generic_verify_speed(struct cpufreq_policy *policy) +{
- return cpufreq_frequency_table_verify(policy, freq_table);
+}
+static unsigned int generic_get_speed(unsigned int cpu) +{
- return clk_get_rate(cpu_clk) / 1000;
+}
+static int generic_set_target(struct cpufreq_policy *policy,
unsigned int target_freq, unsigned int relation)+{
- struct cpufreq_freqs freqs;
- unsigned long freq_Hz;
- int cpu;
- int ret = 0;
- unsigned int index;
- cpufreq_frequency_table_target(policy, freq_table,
target_freq, relation, &index);- freq_Hz = clk_round_rate(cpu_clk, cpu_freqs[index]);
- freq_Hz = freq_Hz ? freq_Hz : cpu_freqs[index];
- freqs.old = clk_get_rate(cpu_clk) / 1000;
- freqs.new = freq_Hz / 1000;
- freqs.flags = 0;
- if (freqs.old == freqs.new)
return 0;- for_each_possible_cpu(cpu) {
freqs.cpu = cpu;cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);- }
- ret = set_cpu_freq(freq_Hz, index, (freqs.new > freqs.old));
If this fails then we'll still be notifying the transition at the requested rate even though it didn't work. I guess we should really get the rate of the clk and put that into freqs for the POSTCHANGE notification.
right, Thanks. Added: if (ret) freq.new = clk_get_rate(cpu_clk) / 1000;
- for_each_possible_cpu(cpu) {
freqs.cpu = cpu;cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);- }
- return ret;
+}
+static int generic_cpufreq_init(struct cpufreq_policy *policy) +{
- int ret;
- if (policy->cpu >= num_possible_cpus())
return -EINVAL;- policy->cur = clk_get_rate(cpu_clk) / 1000;
- policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
- cpumask_setall(policy->cpus);
- /* Manual states, that PLL stabilizes in two CLK32 periods */
- policy->cpuinfo.transition_latency = trans_latency;
- ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
- if (ret < 0) {
pr_err("%s: invalid frequency table for cpu %d\n",__func__, policy->cpu);return ret;- }
- cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
- return 0;
+}
+static int generic_cpufreq_exit(struct cpufreq_policy *policy) +{
- cpufreq_frequency_table_put_attr(policy->cpu);
- return 0;
+}
+static struct cpufreq_driver generic_cpufreq_driver = {
- .flags = CPUFREQ_STICKY,
- .verify = generic_verify_speed,
- .target = generic_set_target,
- .get = generic_get_speed,
- .init = generic_cpufreq_init,
- .exit = generic_cpufreq_exit,
- .name = "generic",
This may be a little too generic? "generic-reg-clk"?
I ever thought about it. If it's exact, it'll be "generic-reg-clk-dt". Is "generic-reg-clk" or "generic-reg-clk-dt" too long for file name?
+};
+static int __devinit generic_cpufreq_driver_init(void) +{
- struct device_node *cpu0;
- const struct property *pp;
- int i, ret;
- pr_info("Generic CPU frequency driver\n");
- cpu0 = of_find_node_by_path("/cpus/cpu@0");
- if (!cpu0)
return -ENODEV;- if (!of_device_is_compatible(cpu0, "generic-cpufreq"))
return -ENODEV;As above, I'd personally rather not use compatible strings,
I still think checking compatible is better. So I need device tree maintainer's comments.
but if you do, then I think return 0 here rather than -ENODEV else I believe you'll get a potentially confusing message on the console for platforms that don't use this.
I should let it be tristate. If it's not compatible, I don't need the module any more.
- pp = of_find_property(cpu0, "cpu-freqs", NULL);
- if (!pp) {
ret = -ENODEV;goto put_node;- }
- cpu_op_nr = pp->length / sizeof(u32);
- if (!cpu_op_nr) {
ret = -ENODEV;goto put_node;- }
- ret = -ENOMEM;
- cpu_freqs = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr, GFP_KERNEL);
- if (!cpu_freqs)
goto put_node;- of_property_read_u32_array(cpu0, "cpu-freqs", cpu_freqs, cpu_op_nr);
- pp = of_find_property(cpu0, "cpu-volts", NULL);
- if (pp) {
if (cpu_op_nr == pp->length / sizeof(u32)) {cpu_volts = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr,GFP_KERNEL);if (!cpu_volts)goto free_cpu_freqs;of_property_read_u32_array(cpu0, "cpu-volts",cpu_volts, cpu_op_nr);} elsepr_warn("%s: invalid cpu_volts!\n", __func__);- }
- if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
trans_latency = CPUFREQ_ETERNAL;- freq_table = kmalloc(sizeof(struct cpufreq_frequency_table)
* (cpu_op_nr + 1), GFP_KERNEL);- if (!freq_table)
goto free_cpu_volts;- for (i = 0; i < cpu_op_nr; i++) {
freq_table[i].index = i;freq_table[i].frequency = cpu_freqs[i] / 1000;- }
- freq_table[i].index = i;
- freq_table[i].frequency = CPUFREQ_TABLE_END;
- cpu_clk = clk_get(NULL, "cpu");
- if (IS_ERR(cpu_clk)) {
pr_err("%s: failed to get cpu clock\n", __func__);ret = PTR_ERR(cpu_clk);goto free_freq_table;- }
- if (cpu_volts) {
cpu_reg = regulator_get(NULL, "cpu");if (IS_ERR(cpu_reg)) {pr_warn("%s: regulator cpu get failed.\n", __func__);cpu_reg = NULL;}- }
- ret = cpufreq_register_driver(&generic_cpufreq_driver);
- if (ret)
goto reg_put;- of_node_put(cpu0);
- return 0;
+reg_put:
- if (cpu_reg)
regulator_put(cpu_reg);- clk_put(cpu_clk);
+free_freq_table:
- kfree(freq_table);
+free_cpu_volts:
- kfree(cpu_volts);
+free_cpu_freqs:
- kfree(cpu_freqs);
+put_node:
- of_node_put(cpu0);
- return ret;
+}
+static void generic_cpufreq_driver_exit(void) +{
- cpufreq_unregister_driver(&generic_cpufreq_driver);
- kfree(cpu_freqs);
- kfree(cpu_volts);
- kfree(freq_table);
- clk_put(cpu_clk);
Should this do something with the regulator too?
right. Added: if (cpu_reg) regulator_put(cpu_reg);
Thanks very much for your review! Richard
Jamie
On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote:
On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
Hi Richard,
On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
It support single core and multi-core ARM SoCs. But currently it assume all cores share the same frequency and voltage.
Signed-off-by: Richard Zhao richard.zhao@linaro.org
.../devicetree/bindings/cpufreq/generic-cpufreq | 7 + drivers/cpufreq/Kconfig | 8 + drivers/cpufreq/Makefile | 2 + drivers/cpufreq/generic-cpufreq.c | 251 ++++++++++++++++++++ 4 files changed, 268 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq create mode 100644 drivers/cpufreq/generic-cpufreq.c
diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq new file mode 100644 index 0000000..15dd780 --- /dev/null +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq @@ -0,0 +1,7 @@ +Generic cpufreq driver
+Required properties in /cpus/cpu@0: +- compatible : "generic-cpufreq"
I'm not convinced this is the best way to do this. By requiring a generic-cpufreq compatible string we're encoding Linux driver information into the hardware description. The only way I can see to avoid this is to provide a generic_clk_cpufreq_init() function that platforms can call in their machine init code to use the driver.
It'll prevent the driver from being a kernel module.
Hmm, that's not very nice either! I guess you _could_ add an of_machine_is_compatible() check against a list of compatible machines in the driver but that feels a little gross. Hopefully Rob or Grant have a good alternative!
Hi Grant & Rob,
Could you comment?
+- cpu-freqs : cpu frequency points it support +- cpu-volts : cpu voltages required by the frequency point at the same index +- trans-latency : transition_latency diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig index e24a2a1..216eecd 100644 --- a/drivers/cpufreq/Kconfig +++ b/drivers/cpufreq/Kconfig @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE If in doubt, say N. +config GENERIC_CPUFREQ_DRIVER
- bool "Generic cpufreq driver using clock/regulator/devicetree"
- help
This adds generic CPUFreq driver. It assumes allcores of the CPU share the same clock and voltage.If in doubt, say N.I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
right, Thanks. I can not check clk before generic clock framework come in. Added: depends on OF && REGULATOR select CPU_FREQ_TABLE
You can still use HAVE_CLK. That symbol has been around for ages and any platform implementing the clk API should select it so it's fine to depend on it even before there is a generic struct clk.
Jamie
On 12/19/2011 08:39 AM, Jamie Iles wrote:
On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote:
On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
Hi Richard,
On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
It support single core and multi-core ARM SoCs. But currently it assume all cores share the same frequency and voltage.
Signed-off-by: Richard Zhao richard.zhao@linaro.org
.../devicetree/bindings/cpufreq/generic-cpufreq | 7 + drivers/cpufreq/Kconfig | 8 + drivers/cpufreq/Makefile | 2 + drivers/cpufreq/generic-cpufreq.c | 251 ++++++++++++++++++++ 4 files changed, 268 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq create mode 100644 drivers/cpufreq/generic-cpufreq.c
diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq new file mode 100644 index 0000000..15dd780 --- /dev/null +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq @@ -0,0 +1,7 @@ +Generic cpufreq driver
+Required properties in /cpus/cpu@0: +- compatible : "generic-cpufreq"
I'm not convinced this is the best way to do this. By requiring a generic-cpufreq compatible string we're encoding Linux driver information into the hardware description. The only way I can see to avoid this is to provide a generic_clk_cpufreq_init() function that platforms can call in their machine init code to use the driver.
Agreed on the compatible string. It's putting Linux specifics into DT.
You could flip this around and have the module make a call into the kernel to determine whether to initialize or not. Then platforms could set a flag to indicate this.
It'll prevent the driver from being a kernel module.
Hmm, that's not very nice either! I guess you _could_ add an of_machine_is_compatible() check against a list of compatible machines in the driver but that feels a little gross. Hopefully Rob or Grant have a good alternative!
What does cpufreq core do if multiple drivers are registered? Perhaps a ranking is needed and this would only get enabled if there are no other drivers and other conditions like having the clock "cpu" present are met.
Rob
Hi Grant & Rob,
Could you comment?
+- cpu-freqs : cpu frequency points it support +- cpu-volts : cpu voltages required by the frequency point at the same index +- trans-latency : transition_latency diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig index e24a2a1..216eecd 100644 --- a/drivers/cpufreq/Kconfig +++ b/drivers/cpufreq/Kconfig @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE If in doubt, say N. +config GENERIC_CPUFREQ_DRIVER
- bool "Generic cpufreq driver using clock/regulator/devicetree"
- help
This adds generic CPUFreq driver. It assumes allcores of the CPU share the same clock and voltage.If in doubt, say N.I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
right, Thanks. I can not check clk before generic clock framework come in. Added: depends on OF && REGULATOR select CPU_FREQ_TABLE
You can still use HAVE_CLK. That symbol has been around for ages and any platform implementing the clk API should select it so it's fine to depend on it even before there is a generic struct clk.
Jamie
On Mon, Dec 19, 2011 at 09:00:44AM -0600, Rob Herring wrote:
On 12/19/2011 08:39 AM, Jamie Iles wrote:
On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote:
On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
Hi Richard,
On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
It support single core and multi-core ARM SoCs. But currently it assume all cores share the same frequency and voltage.
Signed-off-by: Richard Zhao richard.zhao@linaro.org
.../devicetree/bindings/cpufreq/generic-cpufreq | 7 + drivers/cpufreq/Kconfig | 8 + drivers/cpufreq/Makefile | 2 + drivers/cpufreq/generic-cpufreq.c | 251 ++++++++++++++++++++ 4 files changed, 268 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq create mode 100644 drivers/cpufreq/generic-cpufreq.c
diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq new file mode 100644 index 0000000..15dd780 --- /dev/null +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq @@ -0,0 +1,7 @@ +Generic cpufreq driver
+Required properties in /cpus/cpu@0: +- compatible : "generic-cpufreq"
I'm not convinced this is the best way to do this. By requiring a generic-cpufreq compatible string we're encoding Linux driver information into the hardware description. The only way I can see to avoid this is to provide a generic_clk_cpufreq_init() function that platforms can call in their machine init code to use the driver.
Agreed on the compatible string.
Assume you reject to use compatible string.
It's putting Linux specifics into DT.
You could flip this around and have the module make a call into the kernel to determine whether to initialize or not. Then platforms could set a flag to indicate this.
Could you make it more clear? kernel global variable, macro, or function?
- Following your idea, I think, we can add in driver/cpufreq/cpufreq.c: int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size); SoC code set the callback. If it's NULL, driver will exit. We can get rid of DT. It'll make cpufreq core dirty, but it's the only file built-in.
- Drop module support. SoC call generic_clk_cpufreq_init as Jamie said.
It'll prevent the driver from being a kernel module.
Hmm, that's not very nice either! I guess you _could_ add an of_machine_is_compatible() check against a list of compatible machines in the driver but that feels a little gross. Hopefully Rob or Grant have a good alternative!
What does cpufreq core do if multiple drivers are registered?
current cpufreq core only support one cpufreq_driver. Others will fail except the first time.
Perhaps a ranking is needed and this would only get enabled if there are no other drivers and other conditions like having the clock "cpu" present are met.
We'd better keep cpufreq core simple. For this driver, register cpufreq_driver is the last thing after checking all conditions.
Rob
Hi Grant & Rob,
Could you comment?
+- cpu-freqs : cpu frequency points it support +- cpu-volts : cpu voltages required by the frequency point at the same index +- trans-latency : transition_latency diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig index e24a2a1..216eecd 100644 --- a/drivers/cpufreq/Kconfig +++ b/drivers/cpufreq/Kconfig @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE If in doubt, say N. +config GENERIC_CPUFREQ_DRIVER
- bool "Generic cpufreq driver using clock/regulator/devicetree"
- help
This adds generic CPUFreq driver. It assumes allcores of the CPU share the same clock and voltage.If in doubt, say N.I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
right, Thanks. I can not check clk before generic clock framework come in. Added: depends on OF && REGULATOR select CPU_FREQ_TABLE
You can still use HAVE_CLK. That symbol has been around for ages and any platform implementing the clk API should select it so it's fine to depend on it even before there is a generic struct clk.
You are right. Thanks.
Richard
Jamie
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 12/19/2011 07:59 PM, Richard Zhao wrote:
On Mon, Dec 19, 2011 at 09:00:44AM -0600, Rob Herring wrote:
On 12/19/2011 08:39 AM, Jamie Iles wrote:
On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote:
On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
Hi Richard,
On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
It support single core and multi-core ARM SoCs. But currently it assume all cores share the same frequency and voltage.
Signed-off-by: Richard Zhao richard.zhao@linaro.org
.../devicetree/bindings/cpufreq/generic-cpufreq | 7 + drivers/cpufreq/Kconfig | 8 + drivers/cpufreq/Makefile | 2 + drivers/cpufreq/generic-cpufreq.c | 251 ++++++++++++++++++++ 4 files changed, 268 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq create mode 100644 drivers/cpufreq/generic-cpufreq.c
diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq new file mode 100644 index 0000000..15dd780 --- /dev/null +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq @@ -0,0 +1,7 @@ +Generic cpufreq driver
+Required properties in /cpus/cpu@0: +- compatible : "generic-cpufreq"
I'm not convinced this is the best way to do this. By requiring a generic-cpufreq compatible string we're encoding Linux driver information into the hardware description. The only way I can see to avoid this is to provide a generic_clk_cpufreq_init() function that platforms can call in their machine init code to use the driver.
Agreed on the compatible string.
Assume you reject to use compatible string.
It's putting Linux specifics into DT.
You could flip this around and have the module make a call into the kernel to determine whether to initialize or not. Then platforms could set a flag to indicate this.
Could you make it more clear? kernel global variable, macro, or function?
Any of those. Of course, direct access to variables across modules is discouraged, so it would probably be a function that checks a variable.
- Following your idea, I think, we can add in driver/cpufreq/cpufreq.c:
int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size); SoC code set the callback. If it's NULL, driver will exit. We can get rid of DT. It'll make cpufreq core dirty, but it's the only file built-in.
But aren't you getting the operating points from the DT? Then you don't want to put this code into each platform.
- Drop module support. SoC call generic_clk_cpufreq_init as Jamie said.
It'll prevent the driver from being a kernel module.
Hmm, that's not very nice either! I guess you _could_ add an of_machine_is_compatible() check against a list of compatible machines in the driver but that feels a little gross. Hopefully Rob or Grant have a good alternative!
What does cpufreq core do if multiple drivers are registered?
current cpufreq core only support one cpufreq_driver. Others will fail except the first time.
Then whoever gets there first wins. Make your driver register late and if someone doesn't want to use it they can register a custom driver earlier.
Rob
Perhaps a ranking is needed and this would only get enabled if there are no other drivers and other conditions like having the clock "cpu" present are met.
We'd better keep cpufreq core simple. For this driver, register cpufreq_driver is the last thing after checking all conditions.
Rob
Hi Grant & Rob,
Could you comment?
+- cpu-freqs : cpu frequency points it support +- cpu-volts : cpu voltages required by the frequency point at the same index +- trans-latency : transition_latency diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig index e24a2a1..216eecd 100644 --- a/drivers/cpufreq/Kconfig +++ b/drivers/cpufreq/Kconfig @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE If in doubt, say N. +config GENERIC_CPUFREQ_DRIVER
- bool "Generic cpufreq driver using clock/regulator/devicetree"
- help
This adds generic CPUFreq driver. It assumes allcores of the CPU share the same clock and voltage.If in doubt, say N.I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
right, Thanks. I can not check clk before generic clock framework come in. Added: depends on OF && REGULATOR select CPU_FREQ_TABLE
You can still use HAVE_CLK. That symbol has been around for ages and any platform implementing the clk API should select it so it's fine to depend on it even before there is a generic struct clk.
You are right. Thanks.
Richard
Jamie
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
在 2011-12-20 下午11:13，"Rob Herring" robherring2@gmail.com写道：
On 12/19/2011 07:59 PM, Richard Zhao wrote:
On Mon, Dec 19, 2011 at 09:00:44AM -0600, Rob Herring wrote:
On 12/19/2011 08:39 AM, Jamie Iles wrote:
On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote:
On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
Hi Richard,
On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote: > It support single core and multi-core ARM SoCs. But currently it
assume
> all cores share the same frequency and voltage. > > Signed-off-by: Richard Zhao richard.zhao@linaro.org > --- > .../devicetree/bindings/cpufreq/generic-cpufreq | 7 + > drivers/cpufreq/Kconfig | 8 + > drivers/cpufreq/Makefile | 2 + > drivers/cpufreq/generic-cpufreq.c | 251
++++++++++++++++++++
> 4 files changed, 268 insertions(+), 0 deletions(-) > create mode 100644
Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> create mode 100644 drivers/cpufreq/generic-cpufreq.c > > diff --git
a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> new file mode 100644 > index 0000000..15dd780 > --- /dev/null > +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq > @@ -0,0 +1,7 @@ > +Generic cpufreq driver > + > +Required properties in /cpus/cpu@0: > +- compatible : "generic-cpufreq"
I'm not convinced this is the best way to do this. By requiring a generic-cpufreq compatible string we're encoding Linux driver information into the hardware description. The only way I can see
to
avoid this is to provide a generic_clk_cpufreq_init() function that platforms can call in their machine init code to use the driver.
Agreed on the compatible string.
Assume you reject to use compatible string.
It's putting Linux specifics into DT.
You could flip this around and have the module make a call into the kernel to determine whether to initialize or not. Then platforms could set a flag to indicate this.
Could you make it more clear? kernel global variable, macro, or
function?
Any of those. Of course, direct access to variables across modules is discouraged, so it would probably be a function that checks a variable.
why not use function pointer? arch that don't use this driver do not have to set it. if use function, everyone should define it.
- Following your idea, I think, we can add in driver/cpufreq/cpufreq.c:
int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size); SoC code set the callback. If it's NULL, driver will exit. We can get
rid
of DT. It'll make cpufreq core dirty, but it's the only file built-in.
But aren't you getting the operating points from the DT? Then you don't want to put this code into each platform.
the variable is mainly used to check whether some platform want to use this driver. getting ride of dt is side affect.
- Drop module support. SoC call generic_clk_cpufreq_init as Jamie said.
It'll prevent the driver from being a kernel module.
Hmm, that's not very nice either! I guess you _could_ add an of_machine_is_compatible() check against a list of compatible machines in the driver but that feels a little gross. Hopefully Rob or Grant have a good alternative!
What does cpufreq core do if multiple drivers are registered?
current cpufreq core only support one cpufreq_driver. Others will fail except the first time.
Then whoever gets there first wins. Make your driver register late and if someone doesn't want to use it they can register a custom driver
earlier. this driver did
Rob
Perhaps a ranking is needed and this would only get enabled if there are no other drivers and other conditions like having the clock "cpu" present are
met.
We'd better keep cpufreq core simple. For this driver, register
cpufreq_driver
is the last thing after checking all conditions.
Rob
Hi Grant & Rob,
Could you comment?
> +- cpu-freqs : cpu frequency points it support > +- cpu-volts : cpu voltages required by the frequency point at the
same index
> +- trans-latency : transition_latency > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig > index e24a2a1..216eecd 100644 > --- a/drivers/cpufreq/Kconfig > +++ b/drivers/cpufreq/Kconfig > @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE > > If in doubt, say N. > > +config GENERIC_CPUFREQ_DRIVER > + bool "Generic cpufreq driver using
clock/regulator/devicetree"
> + help > + This adds generic CPUFreq driver. It assumes all > + cores of the CPU share the same clock and voltage. > + > + If in doubt, say N.
I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
right, Thanks. I can not check clk before generic clock framework come in. Added: depends on OF && REGULATOR select CPU_FREQ_TABLE
You can still use HAVE_CLK. That symbol has been around for ages and any platform implementing the clk API should select it so it's fine to depend on it even before there is a generic struct clk.
You are right. Thanks.
Richard
Jamie
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tuesday 20 December 2011, Richard Zhao wrote:
+Generic cpufreq driver
+Required properties in /cpus/cpu@0: +- compatible : "generic-cpufreq"
I'm not convinced this is the best way to do this. By requiring a generic-cpufreq compatible string we're encoding Linux driver information into the hardware description. The only way I can see to avoid this is to provide a generic_clk_cpufreq_init() function that platforms can call in their machine init code to use the driver.
Agreed on the compatible string.
Assume you reject to use compatible string.
It's putting Linux specifics into DT.
You could flip this around and have the module make a call into the kernel to determine whether to initialize or not. Then platforms could set a flag to indicate this.
Could you make it more clear? kernel global variable, macro, or function?
- Following your idea, I think, we can add in driver/cpufreq/cpufreq.c:
int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size); SoC code set the callback. If it's NULL, driver will exit. We can get rid of DT. It'll make cpufreq core dirty, but it's the only file built-in.
- Drop module support. SoC call generic_clk_cpufreq_init as Jamie said.
I think you don't actually need a "compatible" property here, as long as we ensure that the "cpu-freqs", "cpu-volts" and "trans-latency" properties are present in the cpu node if and only if the machine works with this driver (why else would you put them there?).
CPUs are special in the device trees in a number of ways, so I think we can treat this as a logical extension. This way, you can still make the generic cpufreq driver a loadable module. You don't get module autoloading with this structure, but that is already the case because the cpu nodes in the device tree are not translated into linux devices.
Arnd
在 2011-12-20 下午11:22，"Arnd Bergmann" arnd@arndb.de写道：
On Tuesday 20 December 2011, Richard Zhao wrote:
> +Generic cpufreq driver > + > +Required properties in /cpus/cpu@0: > +- compatible : "generic-cpufreq"
I'm not convinced this is the best way to do this. By requiring a generic-cpufreq compatible string we're encoding Linux driver information into the hardware description. The only way I can
see to
avoid this is to provide a generic_clk_cpufreq_init() function
that
platforms can call in their machine init code to use the driver.
Agreed on the compatible string.
Assume you reject to use compatible string.
It's putting Linux specifics into DT.
You could flip this around and have the module make a call into the kernel to determine whether to initialize or not. Then platforms could set a flag to indicate this.
Could you make it more clear? kernel global variable, macro, or
function?
- Following your idea, I think, we can add in driver/cpufreq/cpufreq.c:
int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size); SoC code set the callback. If it's NULL, driver will exit. We can get
rid
of DT. It'll make cpufreq core dirty, but it's the only file built-in.
- Drop module support. SoC call generic_clk_cpufreq_init as Jamie said.
I think you don't actually need a "compatible" property here, as long as
we
ensure that the "cpu-freqs", "cpu-volts" and "trans-latency" properties
are
present in the cpu node if and only if the machine works with this driver (why else would you put them there?).
It is more easy for me. Rob, agree?
Richard
CPUs are special in the device trees in a number of ways, so I think we can treat this as a logical extension. This way, you can still make the generic cpufreq driver a loadable module. You don't get module autoloading with this structure, but that is already the case because the cpu nodes in the device tree are not translated into linux devices.
Arnd
On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
It support single core and multi-core ARM SoCs. But currently it assume all cores share the same frequency and voltage.
My comments on the previous version of the patch still apply:
- The voltage ranges being set need to be specified as ranges. - Frequencies that can't be supported due to limitations of the available supplies shouldn't be exposed to users. - The driver needs to handle errors.
+Required properties in /cpus/cpu@0: +- compatible : "generic-cpufreq" +- cpu-freqs : cpu frequency points it support +- cpu-volts : cpu voltages required by the frequency point at the same index +- trans-latency : transition_latency
You need to define units for all of these, and for the transition latency you need to be clear about what's being measured (it looks like the CPU time only, not any voltage ramping).
You also need to define how the core supplies get looked up.
- /* Manual states, that PLL stabilizes in two CLK32 periods */
- policy->cpuinfo.transition_latency = trans_latency;
I guess this comment is a cut'n'paste error.
- ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
- if (ret < 0) {
pr_err("%s: invalid frequency table for cpu %d\n",__func__, policy->cpu);
You should define pr_fmt to always include __func__ in the messages rather than open coding - ensures consistency and is less noisy in the code.
- pr_info("Generic CPU frequency driver\n");
This seems noisy...
On Tue, Dec 20, 2011 at 02:59:04PM +0000, Mark Brown wrote:
On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
It support single core and multi-core ARM SoCs. But currently it assume all cores share the same frequency and voltage.
My comments on the previous version of the patch still apply:
- The voltage ranges being set need to be specified as ranges.
cpu normally need strict voltages. and only proved operating opints are allowed to set in dts. If the voltage changes slightly especially for high frequency, it's easy to cause unstable.
- Frequencies that can't be supported due to limitations of the available supplies shouldn't be exposed to users.
As I said, only proved operation points are allowed.
- The driver needs to handle errors.
Yes.
+Required properties in /cpus/cpu@0: +- compatible : "generic-cpufreq" +- cpu-freqs : cpu frequency points it support +- cpu-volts : cpu voltages required by the frequency point at the same index +- trans-latency : transition_latency
You need to define units for all of these, and for the transition latency you need to be clear about what's being measured (it looks like the CPU time only, not any voltage ramping).
right.
You also need to define how the core supplies get looked up.
It's pure software. platform uses this driver have to define "cpu" consumer.
- /* Manual states, that PLL stabilizes in two CLK32 periods */
- policy->cpuinfo.transition_latency = trans_latency;
I guess this comment is a cut'n'paste error.
right, thanks.
- ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
- if (ret < 0) {
pr_err("%s: invalid frequency table for cpu %d\n",__func__, policy->cpu);You should define pr_fmt to always include __func__ in the messages rather than open coding - ensures consistency and is less noisy in the code.
I'll check it.
- pr_info("Generic CPU frequency driver\n");
This seems noisy...
Why? Do you think only errors and warnings can print out?
Thanks Richard
On Wed, Dec 21, 2011 at 07:27:03AM +0800, Richard Zhao wrote:
On Tue, Dec 20, 2011 at 02:59:04PM +0000, Mark Brown wrote:
My comments on the previous version of the patch still apply:
- The voltage ranges being set need to be specified as ranges.
cpu normally need strict voltages. and only proved operating opints are allowed to set in dts. If the voltage changes slightly especially for high frequency, it's easy to cause unstable.
Clearly there will be limits which get more and more restrictive as the frequencies get higher but there will always be at least some play in the numbers as one must at a minimum specify tolerance ranges, and at lower frequencies the ranges specified will typically get compartively large.
Note also that not all hardware specifies things in terms of a fixed set of operating points, sometimes only the minimum voltage specification is varied with frequency or sometimes you see maximum and minimum stepping independently.
Further note that if all hardware really does have as tight a set of requirements as you suggest then the regulator support in the driver needs to be non-optional otherwise a board without software regulator control might drop the frequency without also dropping the voltage.
- Frequencies that can't be supported due to limitations of the available supplies shouldn't be exposed to users.
As I said, only proved operation points are allowed.
This statement appears to be unrelated to the comment you're replying to.
You also need to define how the core supplies get looked up.
It's pure software. platform uses this driver have to define "cpu" consumer.
You still need to define this in the binding.
- pr_info("Generic CPU frequency driver\n");
This seems noisy...
Why? Do you think only errors and warnings can print out?
Yes.
Hi Mark,
On Tue, Dec 20, 2011 at 11:48:45PM +0000, Mark Brown wrote:
On Wed, Dec 21, 2011 at 07:27:03AM +0800, Richard Zhao wrote:
On Tue, Dec 20, 2011 at 02:59:04PM +0000, Mark Brown wrote:
My comments on the previous version of the patch still apply:
- The voltage ranges being set need to be specified as ranges.
cpu normally need strict voltages. and only proved operating opints are allowed to set in dts. If the voltage changes slightly especially for high frequency, it's easy to cause unstable.
Clearly there will be limits which get more and more restrictive as the frequencies get higher but there will always be at least some play in the numbers as one must at a minimum specify tolerance ranges, and at lower frequencies the ranges specified will typically get compartively large.
hmm, reasonable. I'll add it in dts. Thanks.
Note also that not all hardware specifies things in terms of a fixed set of operating points, sometimes only the minimum voltage specification is varied with frequency or sometimes you see maximum and minimum stepping independently.
cpus that don't use freq table is out of scope of this driver.
Further note that if all hardware really does have as tight a set of requirements as you suggest then the regulator support in the driver needs to be non-optional otherwise a board without software regulator control might drop the frequency without also dropping the voltage.
It's ok to only adjuct freq without changes voltage. You can find many arm soc cpufreq drivers don't change voltage. If dts specify voltage but don't have such regulator. I'll assume it always runs on the initial volatage (highest for most cases).
- Frequencies that can't be supported due to limitations of the available supplies shouldn't be exposed to users.
As I said, only proved operation points are allowed.
This statement appears to be unrelated to the comment you're replying to.
I meant the exact voltage can always successfull set. Anyway, I'll add regulator_set_voltage return value checking.
You also need to define how the core supplies get looked up.
It's pure software. platform uses this driver have to define "cpu" consumer.
You still need to define this in the binding.
You mean regulator DT binding? already in ? I'll check it.
- pr_info("Generic CPU frequency driver\n");
This seems noisy...
Why? Do you think only errors and warnings can print out?
Yes.
Maybe I can remove it. But I'd probably add freq table dump. It's more important. Agree?
I checked pr_fmt. Thanks very much. I'll use below and remove __func_. #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Thanks Richard
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:
On Tue, Dec 20, 2011 at 11:48:45PM +0000, Mark Brown wrote:
Note also that not all hardware specifies things in terms of a fixed set of operating points, sometimes only the minimum voltage specification is varied with frequency or sometimes you see maximum and minimum stepping independently.
cpus that don't use freq table is out of scope of this driver.
That's not the point - the point is that you may do something like specify a defined set of frequencies and just drop the minimum supported voltage without altering the maximum supported voltage as the frequency gets lower.
Further note that if all hardware really does have as tight a set of requirements as you suggest then the regulator support in the driver needs to be non-optional otherwise a board without software regulator control might drop the frequency without also dropping the voltage.
It's ok to only adjuct freq without changes voltage. You can find many arm soc cpufreq drivers don't change voltage. If dts specify voltage but don't have such regulator. I'll assume it always runs on the initial volatage (highest for most cases).
My point exactly; such devices are examples of the policy outlined above where only the minimum voltage changes with frequency and the maximum voltage is constant. The cpufreq driver would lower the supported voltage when possible but wouldn't actually care if the voltage changes.
- Frequencies that can't be supported due to limitations of the available supplies shouldn't be exposed to users.
As I said, only proved operation points are allowed.
This statement appears to be unrelated to the comment you're replying to.
I meant the exact voltage can always successfull set. Anyway, I'll add
This is just not the case. Regulators don't offer a continuous range of voltages, they offer steps of varying sizes which may miss setting some voltages, and board designers may choose not to support some of the voltage range.
regulator_set_voltage return value checking.
While this is important the driver should also be enumerating the supported voltages at probe time and eliminating unsupportable settings so that governors aren't constantly trying to set things that can never be supported. The s3c64xx cpufreq driver provides an implementation of this.
Maybe I can remove it. But I'd probably add freq table dump. It's more important. Agree?
That seems like something the core should be doing if
On Wed, Dec 21, 2011 at 01:32:21AM +0000, Mark Brown wrote:
On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:
On Tue, Dec 20, 2011 at 11:48:45PM +0000, Mark Brown wrote:
Note also that not all hardware specifies things in terms of a fixed set of operating points, sometimes only the minimum voltage specification is varied with frequency or sometimes you see maximum and minimum stepping independently.
cpus that don't use freq table is out of scope of this driver.
That's not the point - the point is that you may do something like specify a defined set of frequencies and just drop the minimum supported voltage without altering the maximum supported voltage as the frequency gets lower.
What do you mean by "drop"? cpu-volts = < /* min max */ 1100000 1200000 /* 1G HZ */ 1000000 1200000 /* 800M HZ */ 900000 1200000 /* 600M HZ */ >; The above sample dts could meet your point?
Further note that if all hardware really does have as tight a set of requirements as you suggest then the regulator support in the driver needs to be non-optional otherwise a board without software regulator control might drop the frequency without also dropping the voltage.
It's ok to only adjuct freq without changes voltage. You can find many arm soc cpufreq drivers don't change voltage. If dts specify voltage but don't have such regulator. I'll assume it always runs on the initial volatage (highest for most cases).
My point exactly; such devices are examples of the policy outlined above where only the minimum voltage changes with frequency and the maximum voltage is constant. The cpufreq driver would lower the supported voltage when possible but wouldn't actually care if the voltage changes.
accepted seting voltage range. I guess the above sample dts code meet your point.
- Frequencies that can't be supported due to limitations of the available supplies shouldn't be exposed to users.
As I said, only proved operation points are allowed.
This statement appears to be unrelated to the comment you're replying to.
I meant the exact voltage can always successfull set. Anyway, I'll add
This is just not the case. Regulators don't offer a continuous range of voltages, they offer steps of varying sizes which may miss setting some voltages, and board designers may choose not to support some of the voltage range.
regulator_set_voltage return value checking.
While this is important the driver should also be enumerating the supported voltages at probe time and eliminating unsupportable settings so that governors aren't constantly trying to set things that can never be supported. The s3c64xx cpufreq driver provides an implementation of this.
I'll use regulator_is_supported_voltage pre-check the cpu-volts. For clock, conditions ((clk_round_rate(cpu-freq)/1000) * 1000 == cpu-freq) seems too strict. Because cpu clock is SoC dependent, I'll not add the check.
Maybe I can remove it. But I'd probably add freq table dump. It's more important. Agree?
That seems like something the core should be doing if
hmm, cpu table dumps it with pr_debug.
Thanks Richard
On Wed, Dec 21, 2011 at 10:24:53AM +0800, Richard Zhao wrote:
On Wed, Dec 21, 2011 at 01:32:21AM +0000, Mark Brown wrote:
That's not the point - the point is that you may do something like specify a defined set of frequencies and just drop the minimum supported voltage without altering the maximum supported voltage as the frequency gets lower.
What do you mean by "drop"?
Drop is a synonym for lower in this context.
cpu-volts = < /* min max */ 1100000 1200000 /* 1G HZ */ 1000000 1200000 /* 800M HZ */ 900000 1200000 /* 600M HZ */
;
The above sample dts could meet your point?
Yes.
While this is important the driver should also be enumerating the supported voltages at probe time and eliminating unsupportable settings so that governors aren't constantly trying to set things that can never be supported. The s3c64xx cpufreq driver provides an implementation of this.
I'll use regulator_is_supported_voltage pre-check the cpu-volts. For clock, conditions ((clk_round_rate(cpu-freq)/1000) * 1000 == cpu-freq) seems too strict. Because cpu clock is SoC dependent, I'll not add the check.
The issue that was there for is that there are multiple runtime detectable variant clocking configurations which couldn't be switched between so the driver needs to select only the rates that can be reached from the current configuration. I'd imagine that might be an issue elsewhere.
On Wed, Dec 21, 2011 at 02:33:36AM +0000, Mark Brown wrote:
On Wed, Dec 21, 2011 at 10:24:53AM +0800, Richard Zhao wrote:
On Wed, Dec 21, 2011 at 01:32:21AM +0000, Mark Brown wrote:
That's not the point - the point is that you may do something like specify a defined set of frequencies and just drop the minimum supported voltage without altering the maximum supported voltage as the frequency gets lower.
What do you mean by "drop"?
Drop is a synonym for lower in this context.
cpu-volts = < /* min max */ 1100000 1200000 /* 1G HZ */ 1000000 1200000 /* 800M HZ */ 900000 1200000 /* 600M HZ */
;
The above sample dts could meet your point?
Yes.
While this is important the driver should also be enumerating the supported voltages at probe time and eliminating unsupportable settings so that governors aren't constantly trying to set things that can never be supported. The s3c64xx cpufreq driver provides an implementation of this.
I'll use regulator_is_supported_voltage pre-check the cpu-volts. For clock, conditions ((clk_round_rate(cpu-freq)/1000) * 1000 == cpu-freq) seems too strict. Because cpu clock is SoC dependent, I'll not add the check.
The issue that was there for is that there are multiple runtime detectable variant clocking configurations which couldn't be switched between so the driver needs to select only the rates that can be reached from the current configuration. I'd imagine that might be an issue elsewhere.
one case is that, cpu freq is 800M, clock may only reach 799M. I'd rather let clock code to decide how to handle 800M request. That's why I said the condition check is too strict.
Thanks Richard
On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:
Hi Mark,
On Tue, Dec 20, 2011 at 11:48:45PM +0000, Mark Brown wrote:
On Wed, Dec 21, 2011 at 07:27:03AM +0800, Richard Zhao wrote:
On Tue, Dec 20, 2011 at 02:59:04PM +0000, Mark Brown wrote:
My comments on the previous version of the patch still apply:
- The voltage ranges being set need to be specified as ranges.
cpu normally need strict voltages. and only proved operating opints are allowed to set in dts. If the voltage changes slightly especially for high frequency, it's easy to cause unstable.
Clearly there will be limits which get more and more restrictive as the frequencies get higher but there will always be at least some play in the numbers as one must at a minimum specify tolerance ranges, and at lower frequencies the ranges specified will typically get compartively large.
hmm, reasonable. I'll add it in dts. Thanks.
Note also that not all hardware specifies things in terms of a fixed set of operating points, sometimes only the minimum voltage specification is varied with frequency or sometimes you see maximum and minimum stepping independently.
cpus that don't use freq table is out of scope of this driver.
Further note that if all hardware really does have as tight a set of requirements as you suggest then the regulator support in the driver needs to be non-optional otherwise a board without software regulator control might drop the frequency without also dropping the voltage.
It's ok to only adjuct freq without changes voltage. You can find many arm soc cpufreq drivers don't change voltage. If dts specify voltage but don't have such regulator. I'll assume it always runs on the initial volatage (highest for most cases).
- Frequencies that can't be supported due to limitations of the available supplies shouldn't be exposed to users.
As I said, only proved operation points are allowed.
This statement appears to be unrelated to the comment you're replying to.
I meant the exact voltage can always successfull set. Anyway, I'll add regulator_set_voltage return value checking.
You also need to define how the core supplies get looked up.
It's pure software. platform uses this driver have to define "cpu" consumer.
You still need to define this in the binding.
You mean regulator DT binding? already in ? I'll check it.
Mark, cpu node is not a struct device, sys_device instead. I can not find regulator via device/dt node. Can I still use the string to get regulator after converting to DT?
And regulator binding is not on cpufreq git tree.
Thanks Richard
- pr_info("Generic CPU frequency driver\n");
This seems noisy...
Why? Do you think only errors and warnings can print out?
Yes.
Maybe I can remove it. But I'd probably add freq table dump. It's more important. Agree?
I checked pr_fmt. Thanks very much. I'll use below and remove __func_. #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Thanks Richard
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wednesday 21 December 2011, Richard Zhao wrote:
On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:
You also need to define how the core supplies get looked up.
It's pure software. platform uses this driver have to define "cpu" consumer.
You still need to define this in the binding.
You mean regulator DT binding? already in ? I'll check it.
Mark, cpu node is not a struct device, sys_device instead. I can not find regulator via device/dt node. Can I still use the string to get regulator after converting to DT?
I believe Kay and Greg have the plan to unify "class" and "bus" in sysfs, which implies turning sys_device into a derived class of device instead of kobject. If that understanding is correct, we might as well do that now so we can attach a device_node to a sys_device.
Kay, does this make sense?
Arnd
On Wed, Dec 21, 2011 at 10:43, Arnd Bergmann arnd@arndb.de wrote:
On Wednesday 21 December 2011, Richard Zhao wrote:
On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:
You also need to define how the core supplies get looked up.
It's pure software. platform uses this driver have to define "cpu" consumer.
You still need to define this in the binding.
You mean regulator DT binding? already in ? I'll check it.
Mark, cpu node is not a struct device, sys_device instead. I can not find regulator via device/dt node. Can I still use the string to get regulator after converting to DT?
I believe Kay and Greg have the plan to unify "class" and "bus" in sysfs, which implies turning sys_device into a derived class of device instead of kobject. If that understanding is correct, we might as well do that now so we can attach a device_node to a sys_device.
Kay, does this make sense?
Yes, it's all converted already: http://git.kernel.org/?p=linux/kernel/git/kay/patches.git%3Ba=tree
The sysdev stuff will entirely go away, the devices are just 'struct device' and the sysdev classes are all converted to buses.
We will convert all classes to buses over time time, and have a single type of device and a single type of subsystem.
Kay
On Wed, Dec 21, 2011 at 12:44:57PM +0100, Kay Sievers wrote:
We will convert all classes to buses over time time, and have a single type of device and a single type of subsystem.
Are there any conversions that have been done already that I can look at for reference?
On Wed, Dec 21, 2011 at 13:12, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Wed, Dec 21, 2011 at 12:44:57PM +0100, Kay Sievers wrote:
We will convert all classes to buses over time time, and have a single type of device and a single type of subsystem.
Are there any conversions that have been done already that I can look at for reference?
The first step is the conversion from 'sys_device' to 'device', which is here: http://git.kernel.org/?p=linux/kernel/git/kay/patches.git%3Ba=tree
That should hit the tree soon, if all works according to plan. All sys_devices and sysdev classes will be gone for forever.
The 'class' to 'bus' work is simpler, because the logic in both of them is very similar and both use the same 'struct device' already.
We'll need to add some convenience APIs to bus, and add code to make sure the converted stuff has compat symlinks in /sys/class when needed. Then we can convert-over one 'struct class' to 'struct bus_type' after the other until 'struct class' can be deleted.
This work has not yet started, because we are busy with the sys_device stuff at the moment.
No new stuff should use 'struct class' or 'struct sys_device', they should all start right away with 'struct bus_type'.
Kay
On Wed, Dec 21, 2011 at 01:49:07PM +0100, Kay Sievers wrote:
On Wed, Dec 21, 2011 at 13:12, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Wed, Dec 21, 2011 at 12:44:57PM +0100, Kay Sievers wrote:
We will convert all classes to buses over time time, and have a single type of device and a single type of subsystem.
Are there any conversions that have been done already that I can look at for reference?
The first step is the conversion from 'sys_device' to 'device', which is here: http://git.kernel.org/?p=linux/kernel/git/kay/patches.git%3Ba=tree
That should hit the tree soon, if all works according to plan. All sys_devices and sysdev classes will be gone for forever.
Even cpu node is device, I still need to find a way to get it. I think it's better have another patch to fix the regulator dt binding in cpu node. I'll not include it in this patch series.
Richard
The 'class' to 'bus' work is simpler, because the logic in both of them is very similar and both use the same 'struct device' already.
We'll need to add some convenience APIs to bus, and add code to make sure the converted stuff has compat symlinks in /sys/class when needed. Then we can convert-over one 'struct class' to 'struct bus_type' after the other until 'struct class' can be deleted.
This work has not yet started, because we are busy with the sys_device stuff at the moment.
No new stuff should use 'struct class' or 'struct sys_device', they should all start right away with 'struct bus_type'.
Kay
On Wed, Dec 21, 2011 at 10:19:11PM +0800, Richard Zhao wrote:
Even cpu node is device, I still need to find a way to get it. I think it's better have another patch to fix the regulator dt binding in cpu node. I'll not include it in this patch series.
I'd expect this to be easy if we can find any device tree data for the CPU at all? It's just another piece of data no different to the clock rates or whatever.
On Wed, Dec 21, 2011 at 09:43:34AM +0000, Arnd Bergmann wrote:
On Wednesday 21 December 2011, Richard Zhao wrote:
Mark, cpu node is not a struct device, sys_device instead. I can not find regulator via device/dt node. Can I still use the string to get regulator after converting to DT?
I believe Kay and Greg have the plan to unify "class" and "bus" in sysfs, which implies turning sys_device into a derived class of device instead of kobject. If that understanding is correct, we might as well do that now so we can attach a device_node to a sys_device.
Kay, does this make sense?
I'm noy Kay but I think even if it's not what we end uo doing internally it's a sensible design for the device tree representation.
Signed-off-by: Richard Zhao richard.zhao@linaro.org --- arch/arm/boot/dts/imx6q.dtsi | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi index 263e8f3..80e47b5 100644 --- a/arch/arm/boot/dts/imx6q.dtsi +++ b/arch/arm/boot/dts/imx6q.dtsi @@ -26,9 +26,12 @@ #size-cells = <0>;
cpu@0 { - compatible = "arm,cortex-a9"; + compatible = "arm,cortex-a9", "generic-cpufreq"; reg = <0>; next-level-cache = <&L2>; + cpu-freqs = <996000000 792000000 396000000 198000000>; + cpu-volts = <1225000 1100000 950000 850000>; + trans-latency = <61036>; /* two CLK32 periods */ };
cpu@1 {
cpufreq needs cpu clock to change frequency.
Signed-off-by: Richard Zhao richard.zhao@linaro.org --- arch/arm/mach-imx/clock-imx6q.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-imx/clock-imx6q.c b/arch/arm/mach-imx/clock-imx6q.c index 039a7ab..72acbc2 100644 --- a/arch/arm/mach-imx/clock-imx6q.c +++ b/arch/arm/mach-imx/clock-imx6q.c @@ -1911,6 +1911,7 @@ static struct clk_lookup lookups[] = { _REGISTER_CLOCK(NULL, "gpmi_io_clk", gpmi_io_clk), _REGISTER_CLOCK(NULL, "usboh3_clk", usboh3_clk), _REGISTER_CLOCK(NULL, "sata_clk", sata_clk), + _REGISTER_CLOCK(NULL, "cpu", arm_clk), };
int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode)
Signed-off-by: Richard Zhao richard.zhao@linaro.org --- arch/arm/mach-imx/Kconfig | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig index c44aa97..39cf00a 100644 --- a/arch/arm/mach-imx/Kconfig +++ b/arch/arm/mach-imx/Kconfig @@ -595,6 +595,7 @@ comment "i.MX6 family:"
config SOC_IMX6Q bool "i.MX6 Quad support" + select ARCH_HAS_CPUFREQ select ARM_GIC select CACHE_L2X0 select CPU_V7
-
Arnd Bergmann -
Jamie Iles -
Kay Sievers -
Mark Brown -
Richard Zhao -
Richard Zhao -
Rob Herring