| Message ID | 1324264903-15395-5-git-send-email-richard.zhao@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
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 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. > + > 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)); 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); > + } 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); 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 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 > > > + > > 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)); > > 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); > > + } 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); > > 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 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. 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 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. > > Jamie
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.
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");
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