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@xxxxxxxxxx> > > --- > > .../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 -- To unsubscribe from this list: send the line "unsubscribe cpufreq" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
