On Thu, Dec 15, 2011 at 12:50:07PM -0600, Mark Langsdorf wrote: > Comments below. I tested this on the Calxeda Highbank SoC using > QEMU. I found one definite error and a few things I would change. Thanks for your test. > > On 12/15/2011 05:16 AM, Richard Zhao wrote: > >It support single core and multi-core ARM SoCs. But it assume > >all cores share the same frequency and voltage. > > > >Signed-off-by: Richard Zhao<richard.z...@linaro.org> > >--- > > >diff --git a/drivers/cpufreq/arm-cpufreq.c b/drivers/cpufreq/arm-cpufreq.c > >new file mode 100644 > >index 0000000..fd9759f > >--- /dev/null > >+++ b/drivers/cpufreq/arm-cpufreq.c > >@@ -0,0 +1,260 @@ > >+/* > >+ * Copyright (C) 2010 Freescale Semiconductor, Inc. All Rights Reserved. > >+ */ > >+ > >+/* > >+ * 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/err.h> > >+#include<linux/slab.h> > >+#include<linux/of.h> > >+#include<asm/cpu.h> > >+#include<mach/hardware.h> > >+#include<mach/clock.h> > > These should probably be replaced by <linux/of_clk.h>. See > below for details. I'll remove <mach/*>. But are you sure clk DT binding has went to -next or -rc? If yes, I'm glad to use it. If no, I don't want to pend on it. > > >+ > >+static u32 *cpu_freqs; > >+static u32 *cpu_volts; > >+static u32 trans_latency; > >+static int cpu_op_nr; > >+ > >+static int cpu_freq_khz_min; > >+static int cpu_freq_khz_max; > >+ > >+static struct clk *cpu_clk; > >+static struct cpufreq_frequency_table *arm_freq_table; > >+ > >+static int set_cpu_freq(int freq) > >+{ > >+ int ret = 0; > >+ int org_cpu_rate; > >+ > >+ org_cpu_rate = clk_get_rate(cpu_clk); > >+ if (org_cpu_rate == freq) > >+ return ret; > >+ > >+ ret = clk_set_rate(cpu_clk, freq); > >+ if (ret != 0) { > >+ printk(KERN_DEBUG "cannot set CPU clock rate\n"); > >+ return ret; > >+ } > >+ > >+ return ret; > >+} > >+ > >+static int arm_verify_speed(struct cpufreq_policy *policy) > >+{ > >+ return cpufreq_frequency_table_verify(policy, arm_freq_table); > >+} > >+ > >+static unsigned int arm_get_speed(unsigned int cpu) > >+{ > >+ return clk_get_rate(cpu_clk) / 1000; > >+} > >+ > >+static int arm_set_target(struct cpufreq_policy *policy, > >+ unsigned int target_freq, unsigned int relation) > >+{ > >+ struct cpufreq_freqs freqs; > >+ int freq_Hz, cpu; > >+ int ret = 0; > >+ unsigned int index; > >+ > >+ cpufreq_frequency_table_target(policy, arm_freq_table, > >+ target_freq, relation,&index); > >+ freq_Hz = arm_freq_table[index].frequency * 1000; > >+ > >+ freqs.old = clk_get_rate(cpu_clk) / 1000; > >+ freqs.new = clk_round_rate(cpu_clk, freq_Hz); > >+ freqs.new = (freqs.new ? 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); > >+ > >+#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); > >+ } > >+ > >+ return ret; > >+} > >+ > >+static int arm_cpufreq_init(struct cpufreq_policy *policy) > >+{ > >+ int ret; > >+ > >+ printk(KERN_INFO "ARM CPU frequency driver\n"); > >+ > >+ if (policy->cpu>= num_possible_cpus()) > >+ return -EINVAL; > >+ > >+ policy->cur = clk_get_rate(cpu_clk) / 1000; > >+ policy->min = policy->cpuinfo.min_freq = cpu_freq_khz_min; > >+ policy->max = policy->cpuinfo.max_freq = cpu_freq_khz_max; > >+ 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, arm_freq_table); > >+ > >+ if (ret< 0) { > >+ printk(KERN_ERR "%s: failed to register i.MXC CPUfreq with > >error code %d\n", > >+ __func__, ret); > >+ return ret; > >+ } > >+ > >+ cpufreq_frequency_table_get_attr(arm_freq_table, policy->cpu); > >+ return 0; > >+} > >+ > >+static int arm_cpufreq_exit(struct cpufreq_policy *policy) > >+{ > >+ cpufreq_frequency_table_put_attr(policy->cpu); > >+ > >+ set_cpu_freq(cpu_freq_khz_max * 1000); > >+ return 0; > >+} > >+ > >+static struct cpufreq_driver arm_cpufreq_driver = { > >+ .flags = CPUFREQ_STICKY, > >+ .verify = arm_verify_speed, > >+ .target = arm_set_target, > >+ .get = arm_get_speed, > >+ .init = arm_cpufreq_init, > >+ .exit = arm_cpufreq_exit, > >+ .name = "arm", > >+}; > >+ > >+static int __devinit arm_cpufreq_driver_init(void) > >+{ > >+ struct device_node *cpu0; > >+ const struct property *pp; > >+ int i, ret; > >+ > >+ cpu0 = of_find_node_by_path("/cpus/cpu@0"); > >+ if (!cpu0) > >+ 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_freqs, cpu_op_nr); > > cpu_freqs should clearly be cpu_volts in this instance. Thanks. > > >+ } else > >+ printk(KERN_WARNING "cpufreq: invalid cpu_volts!\n"); > >+ } > >+ > >+ if (of_property_read_u32(cpu0, "trans_latency",&trans_latency)) > >+ trans_latency = CPUFREQ_ETERNAL; > > I'm not sure this is an appropriate default value. I suspect it will do > very strange things on actual hardware that fails to define > trans_latency in the device tree. CPUFREQ_ETERNAL breaks governor ondemand and conservative. But it's the recommended default vaule in cpufreq doc. > >+ > >+ cpu_freq_khz_min = cpu_freqs[0] / 1000; > >+ cpu_freq_khz_max = cpu_freqs[0] / 1000; > >+ > >+ arm_freq_table = kmalloc(sizeof(struct cpufreq_frequency_table) > >+ * (cpu_op_nr + 1), GFP_KERNEL); > >+ if (!arm_freq_table) > >+ goto free_cpu_volts; > >+ > >+ for (i = 0; i< cpu_op_nr; i++) { > >+ arm_freq_table[i].index = i; > >+ arm_freq_table[i].frequency = cpu_freqs[i] / 1000; > >+ if ((cpu_freqs[i] / 1000)< cpu_freq_khz_min) > >+ cpu_freq_khz_min = cpu_freqs[i] / 1000; > >+ if ((cpu_freqs[i] / 1000)> cpu_freq_khz_max) > >+ cpu_freq_khz_max = cpu_freqs[i] / 1000; > >+ } > >+ > >+ arm_freq_table[i].index = i; > >+ arm_freq_table[i].frequency = CPUFREQ_TABLE_END; > >+ > >+ cpu_clk = clk_get(NULL, "cpu"); > >+ if (IS_ERR(cpu_clk)) { > >+ printk(KERN_ERR "%s: failed to get cpu clock\n", __func__); > >+ ret = PTR_ERR(cpu_clk); > >+ goto free_freq_table; > >+ } > > I'd prefer to see clk_get90 replaced with of_clk_get() ditto > and > get_this_cpu_node() from the clk-cpufreq driver by Jamie Iles that > I resubmitted yesterday. This driver only have one instance, because all cpu cores shares the same clk and voltage. You can see cpumask_setall(policy->cpus). And get_this_cpu_node() adds the dependency that cpufreq_driver.init must run on the cpu the policy indicate, which is not correct.
Thanks for your comments Richard > The of_clk_get() doesn't require defining > an arm_clk structure, so it's slightly more portable for mach- > definitions. Also, the of_clk_get() method doesn't require > mach/clock.h and mach/hardware.h, which is good because most > of the mach- definitions don't include them. > > --Mark Langsdorf > Calxeda, Inc. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > _______________________________________________ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
