* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2011-12-16 10:30 ` Richard Zhao
0 siblings, 0 replies; 46+ messages in thread
From: Richard Zhao @ 2011-12-16 10:30 UTC (permalink / raw)
To: linux-arm-kernel
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.zhao@linaro.org>
---
drivers/cpufreq/Kconfig.arm | 8 ++
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/arm-cpufreq.c | 269 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 278 insertions(+), 0 deletions(-)
create mode 100644 drivers/cpufreq/arm-cpufreq.c
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 72a0044..a0f7d35 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -2,6 +2,14 @@
# ARM CPU Frequency scaling drivers
#
+config ARM_GENERIC_CPUFREQ
+ bool "ARM generic"
+ help
+ This adds ARM generic CPUFreq driver. It assumes all
+ cores of the SoC share the same clock and voltage.
+
+ If in doubt, say N.
+
config ARM_S3C64XX_CPUFREQ
bool "Samsung S3C64XX"
depends on CPU_S3C6410
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index ce75fcb..6ccf02d 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_X86_CPUFREQ_NFORCE2) += cpufreq-nforce2.o
##################################################################################
# ARM SoC drivers
+obj-$(CONFIG_ARM_GENERIC_CPUFREQ) += arm-cpufreq.o
obj-$(CONFIG_UX500_SOC_DB8500) += db8500-cpufreq.o
obj-$(CONFIG_ARM_S3C64XX_CPUFREQ) += s3c64xx-cpufreq.o
obj-$(CONFIG_ARM_S5PV210_CPUFREQ) += s5pv210-cpufreq.o
diff --git a/drivers/cpufreq/arm-cpufreq.c b/drivers/cpufreq/arm-cpufreq.c
new file mode 100644
index 0000000..e4d20da
--- /dev/null
+++ b/drivers/cpufreq/arm-cpufreq.c
@@ -0,0 +1,269 @@
+/*
+ * 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>
+#include <asm/cpu.h>
+
+static u32 *cpu_freqs; /* HZ */
+static u32 *cpu_volts; /* uV */
+static u32 trans_latency; /* ns */
+static int cpu_op_nr;
+
+static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
+static unsigned long l_p_j_ref_freq;
+
+static struct clk *cpu_clk;
+static struct regulator *cpu_reg;
+static struct cpufreq_frequency_table *arm_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) {
+ printk(KERN_DEBUG "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 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;
+ unsigned long freq_Hz;
+ int cpu;
+ int ret = 0;
+ unsigned int index;
+
+ cpufreq_frequency_table_target(policy, arm_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 = clk_round_rate(cpu_clk, cpu_freqs[index]);
+ 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));
+
+#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(l_p_j_ref, cpu), l_p_j_ref_freq,
+ 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;
+
+ 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, arm_freq_table);
+
+ if (ret < 0) {
+ printk(KERN_ERR "%s: invalid frequency table for cpu %d\n",
+ __func__, policy->cpu);
+ 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);
+ 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 cpu, i, ret;
+
+ printk(KERN_INFO "ARM SoC generic CPU frequency driver\n");
+
+ cpu0 = of_find_node_by_path("/cpus/cpu at 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_volts, cpu_op_nr);
+ } else
+ printk(KERN_WARNING "cpufreq: invalid cpu_volts!\n");
+ }
+
+ if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
+ trans_latency = CPUFREQ_ETERNAL;
+
+ 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;
+ }
+
+ 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;
+ }
+
+ if (cpu_volts) {
+ cpu_reg = regulator_get(NULL, "cpu");
+ if (IS_ERR(cpu_reg)) {
+ printk(KERN_WARNING
+ "cpufreq: regulator cpu get failed.\n");
+ cpu_reg = NULL;
+ }
+ }
+
+ l_p_j_ref_freq = clk_get_rate(cpu_clk);
+ for_each_possible_cpu(cpu)
+ per_cpu(l_p_j_ref, cpu) =
+ per_cpu(cpu_data, cpu).loops_per_jiffy;
+
+ ret = cpufreq_register_driver(&arm_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(arm_freq_table);
+free_cpu_volts:
+ kfree(cpu_volts);
+free_cpu_freqs:
+ kfree(cpu_freqs);
+put_node:
+ of_node_put(cpu0);
+
+ return ret;
+}
+
+static void arm_cpufreq_driver_exit(void)
+{
+ cpufreq_unregister_driver(&arm_cpufreq_driver);
+ kfree(cpu_freqs);
+ kfree(cpu_volts);
+ kfree(arm_freq_table);
+ clk_put(cpu_clk);
+}
+
+module_init(arm_cpufreq_driver_init);
+module_exit(arm_cpufreq_driver_exit);
+
+MODULE_AUTHOR("Freescale Semiconductor Inc. Richard Zhao <richard.zhao@freescale.com>");
+MODULE_DESCRIPTION("ARM SoC generic CPUFreq driver");
+MODULE_LICENSE("GPL");
--
1.7.5.4
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
2011-12-16 10:30 ` Richard Zhao
@ 2011-12-16 10:52 ` Jamie Iles
-1 siblings, 0 replies; 46+ messages in thread
From: Jamie Iles @ 2011-12-16 10:52 UTC (permalink / raw)
To: Richard Zhao
Cc: linux-arm-kernel, cpufreq, devicetree-discuss, linux,
mark.langsdorf, patches, eric.miao, kernel, davej, linaro-dev,
shawn.guo
Hi Richard,
A couple of questions inline, but otherwise looks nice!
Jamie
On Fri, Dec 16, 2011 at 06:30:59PM +0800, 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.zhao@linaro.org>
> ---
[...]
> diff --git a/drivers/cpufreq/arm-cpufreq.c b/drivers/cpufreq/arm-cpufreq.c
> new file mode 100644
> index 0000000..e4d20da
> --- /dev/null
> +++ b/drivers/cpufreq/arm-cpufreq.c
> @@ -0,0 +1,269 @@
> +/*
> + * 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>
> +#include <asm/cpu.h>
> +
> +static u32 *cpu_freqs; /* HZ */
> +static u32 *cpu_volts; /* uV */
> +static u32 trans_latency; /* ns */
> +static int cpu_op_nr;
> +
> +static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
> +static unsigned long l_p_j_ref_freq;
> +
> +static struct clk *cpu_clk;
This assumes that all CPU's share the same clk and run at the same rate.
Is that a fair/safe assumption? I honestly don't know the answer to
this so it's just a question!!!
> +static struct regulator *cpu_reg;
> +static struct cpufreq_frequency_table *arm_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) {
> + printk(KERN_DEBUG "cannot set CPU clock rate\n");
Perhaps use pr_debug() and friends throughout this driver?
> + return ret;
> + }
> +
> + if (!higher && cpu_reg)
> + regulator_set_voltage(cpu_reg,
> + cpu_volts[index], cpu_volts[index]);
> +
> + 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;
> + unsigned long freq_Hz;
> + int cpu;
> + int ret = 0;
> + unsigned int index;
> +
> + cpufreq_frequency_table_target(policy, arm_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 = clk_round_rate(cpu_clk, cpu_freqs[index]);
> + freqs.new = freq_Hz / 1000;
Why round the rate then overwrite it? Should this be freqs.new /= 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));
> +
> +#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(l_p_j_ref, cpu), l_p_j_ref_freq,
> + 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;
> +
> + 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, arm_freq_table);
> +
> + if (ret < 0) {
> + printk(KERN_ERR "%s: invalid frequency table for cpu %d\n",
> + __func__, policy->cpu);
> + 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);
> + 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",
Is this really just for ARM or can it be a generic-clk driver? I can't
see any ARM specifics here.
> +};
> +
> +static int __devinit arm_cpufreq_driver_init(void)
> +{
> + struct device_node *cpu0;
> + const struct property *pp;
> + int cpu, i, ret;
> +
> + printk(KERN_INFO "ARM SoC generic CPU frequency driver\n");
> +
> + 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_volts, cpu_op_nr);
> + } else
> + printk(KERN_WARNING "cpufreq: invalid cpu_volts!\n");
> + }
> +
> + if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
> + trans_latency = CPUFREQ_ETERNAL;
> +
> + 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;
> + }
> +
> + 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;
> + }
Should there be a clk_prepare() + clk_enable() pair here? I can't see
it would really be needed but maybe for completeness? Again, just a
question!
> +
> + if (cpu_volts) {
> + cpu_reg = regulator_get(NULL, "cpu");
> + if (IS_ERR(cpu_reg)) {
> + printk(KERN_WARNING
> + "cpufreq: regulator cpu get failed.\n");
> + cpu_reg = NULL;
> + }
> + }
> +
> + l_p_j_ref_freq = clk_get_rate(cpu_clk);
> + for_each_possible_cpu(cpu)
> + per_cpu(l_p_j_ref, cpu) =
> + per_cpu(cpu_data, cpu).loops_per_jiffy;
> +
> + ret = cpufreq_register_driver(&arm_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(arm_freq_table);
> +free_cpu_volts:
> + kfree(cpu_volts);
> +free_cpu_freqs:
> + kfree(cpu_freqs);
> +put_node:
> + of_node_put(cpu0);
> +
> + return ret;
> +}
> +
> +static void arm_cpufreq_driver_exit(void)
> +{
> + cpufreq_unregister_driver(&arm_cpufreq_driver);
> + kfree(cpu_freqs);
> + kfree(cpu_volts);
> + kfree(arm_freq_table);
> + clk_put(cpu_clk);
> +}
> +
> +module_init(arm_cpufreq_driver_init);
> +module_exit(arm_cpufreq_driver_exit);
Are there any ARM platforms that wouldn't be able to use this driver?
If there are then should platforms "opt-in" by calling a register
function rather than having it auto registering as when we have multiple
platforms in a single zImage the probe errors might not be too nice.
> +
> +MODULE_AUTHOR("Freescale Semiconductor Inc. Richard Zhao <richard.zhao@freescale.com>");
> +MODULE_DESCRIPTION("ARM SoC generic CPUFreq driver");
> +MODULE_LICENSE("GPL");
> --
> 1.7.5.4
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2011-12-16 10:52 ` Jamie Iles
0 siblings, 0 replies; 46+ messages in thread
From: Jamie Iles @ 2011-12-16 10:52 UTC (permalink / raw)
To: linux-arm-kernel
Hi Richard,
A couple of questions inline, but otherwise looks nice!
Jamie
On Fri, Dec 16, 2011 at 06:30:59PM +0800, 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.zhao@linaro.org>
> ---
[...]
> diff --git a/drivers/cpufreq/arm-cpufreq.c b/drivers/cpufreq/arm-cpufreq.c
> new file mode 100644
> index 0000000..e4d20da
> --- /dev/null
> +++ b/drivers/cpufreq/arm-cpufreq.c
> @@ -0,0 +1,269 @@
> +/*
> + * 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>
> +#include <asm/cpu.h>
> +
> +static u32 *cpu_freqs; /* HZ */
> +static u32 *cpu_volts; /* uV */
> +static u32 trans_latency; /* ns */
> +static int cpu_op_nr;
> +
> +static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
> +static unsigned long l_p_j_ref_freq;
> +
> +static struct clk *cpu_clk;
This assumes that all CPU's share the same clk and run at the same rate.
Is that a fair/safe assumption? I honestly don't know the answer to
this so it's just a question!!!
> +static struct regulator *cpu_reg;
> +static struct cpufreq_frequency_table *arm_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) {
> + printk(KERN_DEBUG "cannot set CPU clock rate\n");
Perhaps use pr_debug() and friends throughout this driver?
> + return ret;
> + }
> +
> + if (!higher && cpu_reg)
> + regulator_set_voltage(cpu_reg,
> + cpu_volts[index], cpu_volts[index]);
> +
> + 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;
> + unsigned long freq_Hz;
> + int cpu;
> + int ret = 0;
> + unsigned int index;
> +
> + cpufreq_frequency_table_target(policy, arm_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 = clk_round_rate(cpu_clk, cpu_freqs[index]);
> + freqs.new = freq_Hz / 1000;
Why round the rate then overwrite it? Should this be freqs.new /= 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));
> +
> +#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(l_p_j_ref, cpu), l_p_j_ref_freq,
> + 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;
> +
> + 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, arm_freq_table);
> +
> + if (ret < 0) {
> + printk(KERN_ERR "%s: invalid frequency table for cpu %d\n",
> + __func__, policy->cpu);
> + 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);
> + 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",
Is this really just for ARM or can it be a generic-clk driver? I can't
see any ARM specifics here.
> +};
> +
> +static int __devinit arm_cpufreq_driver_init(void)
> +{
> + struct device_node *cpu0;
> + const struct property *pp;
> + int cpu, i, ret;
> +
> + printk(KERN_INFO "ARM SoC generic CPU frequency driver\n");
> +
> + cpu0 = of_find_node_by_path("/cpus/cpu at 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_volts, cpu_op_nr);
> + } else
> + printk(KERN_WARNING "cpufreq: invalid cpu_volts!\n");
> + }
> +
> + if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
> + trans_latency = CPUFREQ_ETERNAL;
> +
> + 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;
> + }
> +
> + 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;
> + }
Should there be a clk_prepare() + clk_enable() pair here? I can't see
it would really be needed but maybe for completeness? Again, just a
question!
> +
> + if (cpu_volts) {
> + cpu_reg = regulator_get(NULL, "cpu");
> + if (IS_ERR(cpu_reg)) {
> + printk(KERN_WARNING
> + "cpufreq: regulator cpu get failed.\n");
> + cpu_reg = NULL;
> + }
> + }
> +
> + l_p_j_ref_freq = clk_get_rate(cpu_clk);
> + for_each_possible_cpu(cpu)
> + per_cpu(l_p_j_ref, cpu) =
> + per_cpu(cpu_data, cpu).loops_per_jiffy;
> +
> + ret = cpufreq_register_driver(&arm_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(arm_freq_table);
> +free_cpu_volts:
> + kfree(cpu_volts);
> +free_cpu_freqs:
> + kfree(cpu_freqs);
> +put_node:
> + of_node_put(cpu0);
> +
> + return ret;
> +}
> +
> +static void arm_cpufreq_driver_exit(void)
> +{
> + cpufreq_unregister_driver(&arm_cpufreq_driver);
> + kfree(cpu_freqs);
> + kfree(cpu_volts);
> + kfree(arm_freq_table);
> + clk_put(cpu_clk);
> +}
> +
> +module_init(arm_cpufreq_driver_init);
> +module_exit(arm_cpufreq_driver_exit);
Are there any ARM platforms that wouldn't be able to use this driver?
If there are then should platforms "opt-in" by calling a register
function rather than having it auto registering as when we have multiple
platforms in a single zImage the probe errors might not be too nice.
> +
> +MODULE_AUTHOR("Freescale Semiconductor Inc. Richard Zhao <richard.zhao@freescale.com>");
> +MODULE_DESCRIPTION("ARM SoC generic CPUFreq driver");
> +MODULE_LICENSE("GPL");
> --
> 1.7.5.4
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
2011-12-16 10:52 ` Jamie Iles
@ 2011-12-16 19:59 ` Bryan Huntsman
-1 siblings, 0 replies; 46+ messages in thread
From: Bryan Huntsman @ 2011-12-16 19:59 UTC (permalink / raw)
To: Jamie Iles
Cc: Richard Zhao, linux, patches, devicetree-discuss, cpufreq,
eric.miao, kernel, davej, linaro-dev, linux-arm-kernel,
linux-arm-msm
On 12/16/2011 02:52 AM, Jamie Iles wrote:
>
>> +static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
>> +static unsigned long l_p_j_ref_freq;
>> +
>> +static struct clk *cpu_clk;
>
> This assumes that all CPU's share the same clk and run at the same rate.
> Is that a fair/safe assumption? I honestly don't know the answer to
> this so it's just a question!!!
On MSM, cpus independently scale both frequency and voltage. Our clock
driver isn't upstream yet. David Brown has a preliminary version here:
https://www.codeaurora.org/gitweb/quic/kernel/?p=davidb/linux-msm.git;a=shortlog;h=refs/heads/msm-clock-rfc
Once we get our driver upstream, MSM will be an exception and not select
ARM_GENERIC_CPUFREQ. We'll probably have a separate msm-cpufreq.c
driver under drivers/cpufreq/.
- Bryan
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2011-12-16 19:59 ` Bryan Huntsman
0 siblings, 0 replies; 46+ messages in thread
From: Bryan Huntsman @ 2011-12-16 19:59 UTC (permalink / raw)
To: linux-arm-kernel
On 12/16/2011 02:52 AM, Jamie Iles wrote:
>
>> +static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
>> +static unsigned long l_p_j_ref_freq;
>> +
>> +static struct clk *cpu_clk;
>
> This assumes that all CPU's share the same clk and run at the same rate.
> Is that a fair/safe assumption? I honestly don't know the answer to
> this so it's just a question!!!
On MSM, cpus independently scale both frequency and voltage. Our clock
driver isn't upstream yet. David Brown has a preliminary version here:
https://www.codeaurora.org/gitweb/quic/kernel/?p=davidb/linux-msm.git;a=shortlog;h=refs/heads/msm-clock-rfc
Once we get our driver upstream, MSM will be an exception and not select
ARM_GENERIC_CPUFREQ. We'll probably have a separate msm-cpufreq.c
driver under drivers/cpufreq/.
- Bryan
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
2011-12-16 19:59 ` Bryan Huntsman
@ 2011-12-17 8:39 ` Richard Zhao
-1 siblings, 0 replies; 46+ messages in thread
From: Richard Zhao @ 2011-12-17 8:39 UTC (permalink / raw)
To: Bryan Huntsman
Cc: Jamie Iles, linux, patches, devicetree-discuss, cpufreq,
eric.miao, kernel, davej, linaro-dev, linux-arm-kernel,
linux-arm-msm
On Fri, Dec 16, 2011 at 11:59:02AM -0800, Bryan Huntsman wrote:
> On 12/16/2011 02:52 AM, Jamie Iles wrote:
> >
> >> +static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
> >> +static unsigned long l_p_j_ref_freq;
> >> +
> >> +static struct clk *cpu_clk;
> >
> > This assumes that all CPU's share the same clk and run at the same rate.
> > Is that a fair/safe assumption? I honestly don't know the answer to
> > this so it's just a question!!!
>
> On MSM, cpus independently scale both frequency and voltage. Our clock
> driver isn't upstream yet. David Brown has a preliminary version here:
If
- cpu_clk are per_cpu, and get from cpu0 clk, cpu1 clk, etc.
The same to regulators.
- set CPUFREQ_SHARED_TYPE_NONE
- don't set affect cpus.
Does that help you?
Thanks
Richard
>
> https://www.codeaurora.org/gitweb/quic/kernel/?p=davidb/linux-msm.git;a=shortlog;h=refs/heads/msm-clock-rfc
>
> Once we get our driver upstream, MSM will be an exception and not select
> ARM_GENERIC_CPUFREQ. We'll probably have a separate msm-cpufreq.c
> driver under drivers/cpufreq/.
>
> - Bryan
>
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2011-12-17 8:39 ` Richard Zhao
0 siblings, 0 replies; 46+ messages in thread
From: Richard Zhao @ 2011-12-17 8:39 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Dec 16, 2011 at 11:59:02AM -0800, Bryan Huntsman wrote:
> On 12/16/2011 02:52 AM, Jamie Iles wrote:
> >
> >> +static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
> >> +static unsigned long l_p_j_ref_freq;
> >> +
> >> +static struct clk *cpu_clk;
> >
> > This assumes that all CPU's share the same clk and run at the same rate.
> > Is that a fair/safe assumption? I honestly don't know the answer to
> > this so it's just a question!!!
>
> On MSM, cpus independently scale both frequency and voltage. Our clock
> driver isn't upstream yet. David Brown has a preliminary version here:
If
- cpu_clk are per_cpu, and get from cpu0 clk, cpu1 clk, etc.
The same to regulators.
- set CPUFREQ_SHARED_TYPE_NONE
- don't set affect cpus.
Does that help you?
Thanks
Richard
>
> https://www.codeaurora.org/gitweb/quic/kernel/?p=davidb/linux-msm.git;a=shortlog;h=refs/heads/msm-clock-rfc
>
> Once we get our driver upstream, MSM will be an exception and not select
> ARM_GENERIC_CPUFREQ. We'll probably have a separate msm-cpufreq.c
> driver under drivers/cpufreq/.
>
> - Bryan
>
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
2011-12-16 19:59 ` Bryan Huntsman
@ 2011-12-19 1:03 ` Richard Zhao
-1 siblings, 0 replies; 46+ messages in thread
From: Richard Zhao @ 2011-12-19 1:03 UTC (permalink / raw)
To: Bryan Huntsman
Cc: Jamie Iles, linux, patches, linux-arm-msm, devicetree-discuss,
eric.miao, cpufreq, kernel, davej, linaro-dev, Richard Zhao,
linux-arm-kernel
Hi Bryan,
On Fri, Dec 16, 2011 at 11:59:02AM -0800, Bryan Huntsman wrote:
> On 12/16/2011 02:52 AM, Jamie Iles wrote:
> >
> >> +static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
> >> +static unsigned long l_p_j_ref_freq;
> >> +
> >> +static struct clk *cpu_clk;
> >
> > This assumes that all CPU's share the same clk and run at the same rate.
> > Is that a fair/safe assumption? I honestly don't know the answer to
> > this so it's just a question!!!
>
> On MSM, cpus independently scale both frequency and voltage. Our clock
> driver isn't upstream yet. David Brown has a preliminary version here:
>
> https://www.codeaurora.org/gitweb/quic/kernel/?p=davidb/linux-msm.git;a=shortlog;h=refs/heads/msm-clock-rfc
>
> Once we get our driver upstream, MSM will be an exception and not select
> ARM_GENERIC_CPUFREQ. We'll probably have a separate msm-cpufreq.c
> driver under drivers/cpufreq/.
Do you have to patch to implement per-cpu udelay? In current code, udelay uses
global loops_per_jiffy.
Thanks
Richard
>
> - Bryan
>
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2011-12-19 1:03 ` Richard Zhao
0 siblings, 0 replies; 46+ messages in thread
From: Richard Zhao @ 2011-12-19 1:03 UTC (permalink / raw)
To: linux-arm-kernel
Hi Bryan,
On Fri, Dec 16, 2011 at 11:59:02AM -0800, Bryan Huntsman wrote:
> On 12/16/2011 02:52 AM, Jamie Iles wrote:
> >
> >> +static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
> >> +static unsigned long l_p_j_ref_freq;
> >> +
> >> +static struct clk *cpu_clk;
> >
> > This assumes that all CPU's share the same clk and run at the same rate.
> > Is that a fair/safe assumption? I honestly don't know the answer to
> > this so it's just a question!!!
>
> On MSM, cpus independently scale both frequency and voltage. Our clock
> driver isn't upstream yet. David Brown has a preliminary version here:
>
> https://www.codeaurora.org/gitweb/quic/kernel/?p=davidb/linux-msm.git;a=shortlog;h=refs/heads/msm-clock-rfc
>
> Once we get our driver upstream, MSM will be an exception and not select
> ARM_GENERIC_CPUFREQ. We'll probably have a separate msm-cpufreq.c
> driver under drivers/cpufreq/.
Do you have to patch to implement per-cpu udelay? In current code, udelay uses
global loops_per_jiffy.
Thanks
Richard
>
> - Bryan
>
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 46+ messages in thread
[parent not found: <20111219010357.GW28768-iWYTGMXpHj9ITqJhDdzsOjpauB2SiJktrE5yTffgRl4@public.gmane.org>]
* Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
2011-12-19 1:03 ` Richard Zhao
@ 2011-12-19 17:42 ` Stephen Boyd
-1 siblings, 0 replies; 46+ messages in thread
From: Stephen Boyd @ 2011-12-19 17:42 UTC (permalink / raw)
To: Richard Zhao
Cc: linux-lFZ/pmaqli7XmaaqVzeoHQ, patches-QSEj5FYQhm4dnm+yROfE0A,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
eric.miao-QSEj5FYQhm4dnm+yROfE0A, Bryan Huntsman,
cpufreq-u79uwXL29TY76Z2rM5mHXA, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
davej-H+wXaHxf7aLQT0dZR+AlfA, linaro-dev-cunTk1MwBs8s++Sfvej+rw,
Richard Zhao, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 12/18/11 17:03, Richard Zhao wrote:
>
> Do you have to patch to implement per-cpu udelay? In current code, udelay uses
> global loops_per_jiffy.
>
>
We've been carrying forward the timer based udelay patches. They're in
the patch tracker as 6873/1, 6874/1, and 6875/1.
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2011-12-19 17:42 ` Stephen Boyd
0 siblings, 0 replies; 46+ messages in thread
From: Stephen Boyd @ 2011-12-19 17:42 UTC (permalink / raw)
To: linux-arm-kernel
On 12/18/11 17:03, Richard Zhao wrote:
>
> Do you have to patch to implement per-cpu udelay? In current code, udelay uses
> global loops_per_jiffy.
>
>
We've been carrying forward the timer based udelay patches. They're in
the patch tracker as 6873/1, 6874/1, and 6875/1.
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
2011-12-16 10:52 ` Jamie Iles
@ 2011-12-17 8:29 ` Richard Zhao
-1 siblings, 0 replies; 46+ messages in thread
From: Richard Zhao @ 2011-12-17 8:29 UTC (permalink / raw)
To: Jamie Iles
Cc: linux-arm-kernel, cpufreq, devicetree-discuss, linux,
mark.langsdorf, patches, eric.miao, kernel, davej, linaro-dev,
shawn.guo
On Fri, Dec 16, 2011 at 10:52:29AM +0000, Jamie Iles wrote:
> Hi Richard,
>
> A couple of questions inline, but otherwise looks nice!
Thanks for your review.
>
> Jamie
>
> On Fri, Dec 16, 2011 at 06:30:59PM +0800, 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.zhao@linaro.org>
> > ---
> [...]
> > diff --git a/drivers/cpufreq/arm-cpufreq.c b/drivers/cpufreq/arm-cpufreq.c
> > new file mode 100644
> > index 0000000..e4d20da
> > --- /dev/null
> > +++ b/drivers/cpufreq/arm-cpufreq.c
> > @@ -0,0 +1,269 @@
> > +/*
> > + * 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>
> > +#include <asm/cpu.h>
> > +
> > +static u32 *cpu_freqs; /* HZ */
> > +static u32 *cpu_volts; /* uV */
> > +static u32 trans_latency; /* ns */
> > +static int cpu_op_nr;
> > +
> > +static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
> > +static unsigned long l_p_j_ref_freq;
> > +
> > +static struct clk *cpu_clk;
>
> This assumes that all CPU's share the same clk and run at the same rate.
> Is that a fair/safe assumption? I honestly don't know the answer to
> this so it's just a question!!!
As I know, most share the same clk/volt. From the code:
IMX6: yes
Tegra: Yes, but strange it sets CPUFREQ_SHARED_TYPE_ALL.
MSM is an exception. I can support the case, but I have to make sure it's
handy to use.
>
> > +static struct regulator *cpu_reg;
> > +static struct cpufreq_frequency_table *arm_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) {
> > + printk(KERN_DEBUG "cannot set CPU clock rate\n");
>
> Perhaps use pr_debug() and friends throughout this driver?
ok. Thanks.
>
> > + return ret;
> > + }
> > +
> > + if (!higher && cpu_reg)
> > + regulator_set_voltage(cpu_reg,
> > + cpu_volts[index], cpu_volts[index]);
> > +
> > + 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;
> > + unsigned long freq_Hz;
> > + int cpu;
> > + int ret = 0;
> > + unsigned int index;
> > +
> > + cpufreq_frequency_table_target(policy, arm_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 = clk_round_rate(cpu_clk, cpu_freqs[index]);
I forgot to delete this line.
> > + freqs.new = freq_Hz / 1000;
>
> Why round the rate then overwrite it? Should this be freqs.new /= 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));
> > +
> > +#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(l_p_j_ref, cpu), l_p_j_ref_freq,
> > + 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;
> > +
> > + 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, arm_freq_table);
> > +
> > + if (ret < 0) {
> > + printk(KERN_ERR "%s: invalid frequency table for cpu %d\n",
> > + __func__, policy->cpu);
> > + 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);
> > + 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",
>
> Is this really just for ARM or can it be a generic-clk driver? I can't
> see any ARM specifics here.
If we make recalculating smp loops_per_jiffy portable, this driver will
be portable too.
>
> > +};
> > +
> > +static int __devinit arm_cpufreq_driver_init(void)
> > +{
> > + struct device_node *cpu0;
> > + const struct property *pp;
> > + int cpu, i, ret;
> > +
> > + printk(KERN_INFO "ARM SoC generic CPU frequency driver\n");
> > +
> > + 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_volts, cpu_op_nr);
> > + } else
> > + printk(KERN_WARNING "cpufreq: invalid cpu_volts!\n");
> > + }
> > +
> > + if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
> > + trans_latency = CPUFREQ_ETERNAL;
> > +
> > + 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;
> > + }
> > +
> > + 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;
> > + }
>
> Should there be a clk_prepare() + clk_enable() pair here? I can't see
> it would really be needed but maybe for completeness? Again, just a
> question!
The cpu clock should already be prepared/enabled. The same to the regulator.
IMHO, cpufreq is not used to handle cpu clk/power gating. The gating should be
handled by cpu hotplug, idle and suspend/resume.
Please correct me if I'm wrong.
>
> > +
> > + if (cpu_volts) {
> > + cpu_reg = regulator_get(NULL, "cpu");
> > + if (IS_ERR(cpu_reg)) {
> > + printk(KERN_WARNING
> > + "cpufreq: regulator cpu get failed.\n");
> > + cpu_reg = NULL;
> > + }
> > + }
> > +
> > + l_p_j_ref_freq = clk_get_rate(cpu_clk);
> > + for_each_possible_cpu(cpu)
> > + per_cpu(l_p_j_ref, cpu) =
> > + per_cpu(cpu_data, cpu).loops_per_jiffy;
> > +
> > + ret = cpufreq_register_driver(&arm_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(arm_freq_table);
> > +free_cpu_volts:
> > + kfree(cpu_volts);
> > +free_cpu_freqs:
> > + kfree(cpu_freqs);
> > +put_node:
> > + of_node_put(cpu0);
> > +
> > + return ret;
> > +}
> > +
> > +static void arm_cpufreq_driver_exit(void)
> > +{
> > + cpufreq_unregister_driver(&arm_cpufreq_driver);
> > + kfree(cpu_freqs);
> > + kfree(cpu_volts);
> > + kfree(arm_freq_table);
> > + clk_put(cpu_clk);
> > +}
> > +
> > +module_init(arm_cpufreq_driver_init);
> > +module_exit(arm_cpufreq_driver_exit);
>
> Are there any ARM platforms that wouldn't be able to use this driver?
> If there are then should platforms "opt-in" by calling a register
> function rather than having it auto registering as when we have multiple
> platforms in a single zImage the probe errors might not be too nice.
Good point. But would it be better to check cpu node compatible property?
Thanks
Richard
>
> > +
> > +MODULE_AUTHOR("Freescale Semiconductor Inc. Richard Zhao <richard.zhao@freescale.com>");
> > +MODULE_DESCRIPTION("ARM SoC generic CPUFreq driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 1.7.5.4
> >
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2011-12-17 8:29 ` Richard Zhao
0 siblings, 0 replies; 46+ messages in thread
From: Richard Zhao @ 2011-12-17 8:29 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Dec 16, 2011 at 10:52:29AM +0000, Jamie Iles wrote:
> Hi Richard,
>
> A couple of questions inline, but otherwise looks nice!
Thanks for your review.
>
> Jamie
>
> On Fri, Dec 16, 2011 at 06:30:59PM +0800, 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.zhao@linaro.org>
> > ---
> [...]
> > diff --git a/drivers/cpufreq/arm-cpufreq.c b/drivers/cpufreq/arm-cpufreq.c
> > new file mode 100644
> > index 0000000..e4d20da
> > --- /dev/null
> > +++ b/drivers/cpufreq/arm-cpufreq.c
> > @@ -0,0 +1,269 @@
> > +/*
> > + * 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>
> > +#include <asm/cpu.h>
> > +
> > +static u32 *cpu_freqs; /* HZ */
> > +static u32 *cpu_volts; /* uV */
> > +static u32 trans_latency; /* ns */
> > +static int cpu_op_nr;
> > +
> > +static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
> > +static unsigned long l_p_j_ref_freq;
> > +
> > +static struct clk *cpu_clk;
>
> This assumes that all CPU's share the same clk and run at the same rate.
> Is that a fair/safe assumption? I honestly don't know the answer to
> this so it's just a question!!!
As I know, most share the same clk/volt. From the code:
IMX6: yes
Tegra: Yes, but strange it sets CPUFREQ_SHARED_TYPE_ALL.
MSM is an exception. I can support the case, but I have to make sure it's
handy to use.
>
> > +static struct regulator *cpu_reg;
> > +static struct cpufreq_frequency_table *arm_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) {
> > + printk(KERN_DEBUG "cannot set CPU clock rate\n");
>
> Perhaps use pr_debug() and friends throughout this driver?
ok. Thanks.
>
> > + return ret;
> > + }
> > +
> > + if (!higher && cpu_reg)
> > + regulator_set_voltage(cpu_reg,
> > + cpu_volts[index], cpu_volts[index]);
> > +
> > + 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;
> > + unsigned long freq_Hz;
> > + int cpu;
> > + int ret = 0;
> > + unsigned int index;
> > +
> > + cpufreq_frequency_table_target(policy, arm_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 = clk_round_rate(cpu_clk, cpu_freqs[index]);
I forgot to delete this line.
> > + freqs.new = freq_Hz / 1000;
>
> Why round the rate then overwrite it? Should this be freqs.new /= 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));
> > +
> > +#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(l_p_j_ref, cpu), l_p_j_ref_freq,
> > + 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;
> > +
> > + 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, arm_freq_table);
> > +
> > + if (ret < 0) {
> > + printk(KERN_ERR "%s: invalid frequency table for cpu %d\n",
> > + __func__, policy->cpu);
> > + 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);
> > + 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",
>
> Is this really just for ARM or can it be a generic-clk driver? I can't
> see any ARM specifics here.
If we make recalculating smp loops_per_jiffy portable, this driver will
be portable too.
>
> > +};
> > +
> > +static int __devinit arm_cpufreq_driver_init(void)
> > +{
> > + struct device_node *cpu0;
> > + const struct property *pp;
> > + int cpu, i, ret;
> > +
> > + printk(KERN_INFO "ARM SoC generic CPU frequency driver\n");
> > +
> > + cpu0 = of_find_node_by_path("/cpus/cpu at 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_volts, cpu_op_nr);
> > + } else
> > + printk(KERN_WARNING "cpufreq: invalid cpu_volts!\n");
> > + }
> > +
> > + if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
> > + trans_latency = CPUFREQ_ETERNAL;
> > +
> > + 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;
> > + }
> > +
> > + 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;
> > + }
>
> Should there be a clk_prepare() + clk_enable() pair here? I can't see
> it would really be needed but maybe for completeness? Again, just a
> question!
The cpu clock should already be prepared/enabled. The same to the regulator.
IMHO, cpufreq is not used to handle cpu clk/power gating. The gating should be
handled by cpu hotplug, idle and suspend/resume.
Please correct me if I'm wrong.
>
> > +
> > + if (cpu_volts) {
> > + cpu_reg = regulator_get(NULL, "cpu");
> > + if (IS_ERR(cpu_reg)) {
> > + printk(KERN_WARNING
> > + "cpufreq: regulator cpu get failed.\n");
> > + cpu_reg = NULL;
> > + }
> > + }
> > +
> > + l_p_j_ref_freq = clk_get_rate(cpu_clk);
> > + for_each_possible_cpu(cpu)
> > + per_cpu(l_p_j_ref, cpu) =
> > + per_cpu(cpu_data, cpu).loops_per_jiffy;
> > +
> > + ret = cpufreq_register_driver(&arm_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(arm_freq_table);
> > +free_cpu_volts:
> > + kfree(cpu_volts);
> > +free_cpu_freqs:
> > + kfree(cpu_freqs);
> > +put_node:
> > + of_node_put(cpu0);
> > +
> > + return ret;
> > +}
> > +
> > +static void arm_cpufreq_driver_exit(void)
> > +{
> > + cpufreq_unregister_driver(&arm_cpufreq_driver);
> > + kfree(cpu_freqs);
> > + kfree(cpu_volts);
> > + kfree(arm_freq_table);
> > + clk_put(cpu_clk);
> > +}
> > +
> > +module_init(arm_cpufreq_driver_init);
> > +module_exit(arm_cpufreq_driver_exit);
>
> Are there any ARM platforms that wouldn't be able to use this driver?
> If there are then should platforms "opt-in" by calling a register
> function rather than having it auto registering as when we have multiple
> platforms in a single zImage the probe errors might not be too nice.
Good point. But would it be better to check cpu node compatible property?
Thanks
Richard
>
> > +
> > +MODULE_AUTHOR("Freescale Semiconductor Inc. Richard Zhao <richard.zhao@freescale.com>");
> > +MODULE_DESCRIPTION("ARM SoC generic CPUFreq driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 1.7.5.4
> >
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
2011-12-16 10:30 ` Richard Zhao
@ 2011-12-16 11:26 ` Heiko Stübner
-1 siblings, 0 replies; 46+ messages in thread
From: Heiko Stübner @ 2011-12-16 11:26 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Richard Zhao, cpufreq, devicetree-discuss, linux, mark.langsdorf,
patches, eric.miao, kernel, davej, linaro-dev, shawn.guo
Am Freitag, 16. Dezember 2011, 11:30:59 schrieb Richard Zhao:
> 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.zhao@linaro.org>
> ---
> drivers/cpufreq/Kconfig.arm | 8 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/arm-cpufreq.c | 269
> +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 278
> insertions(+), 0 deletions(-)
> create mode 100644 drivers/cpufreq/arm-cpufreq.c
looks quite cool, but should also provide a description of the devicetree
bindings (i.e. like the rest in Documentation/devicetree/bindings/...) for
future reference.
Heiko
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2011-12-16 11:26 ` Heiko Stübner
0 siblings, 0 replies; 46+ messages in thread
From: Heiko Stübner @ 2011-12-16 11:26 UTC (permalink / raw)
To: linux-arm-kernel
Am Freitag, 16. Dezember 2011, 11:30:59 schrieb Richard Zhao:
> 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.zhao@linaro.org>
> ---
> drivers/cpufreq/Kconfig.arm | 8 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/arm-cpufreq.c | 269
> +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 278
> insertions(+), 0 deletions(-)
> create mode 100644 drivers/cpufreq/arm-cpufreq.c
looks quite cool, but should also provide a description of the devicetree
bindings (i.e. like the rest in Documentation/devicetree/bindings/...) for
future reference.
Heiko
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
2011-12-16 11:26 ` Heiko Stübner
@ 2011-12-17 7:57 ` Richard Zhao
-1 siblings, 0 replies; 46+ messages in thread
From: Richard Zhao @ 2011-12-17 7:57 UTC (permalink / raw)
To: Heiko Stübner
Cc: linux-arm-kernel, cpufreq, devicetree-discuss, linux,
mark.langsdorf, patches, eric.miao, kernel, davej, linaro-dev,
shawn.guo
On Fri, Dec 16, 2011 at 12:26:03PM +0100, Heiko Stübner wrote:
> Am Freitag, 16. Dezember 2011, 11:30:59 schrieb Richard Zhao:
> > 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.zhao@linaro.org>
> > ---
> > drivers/cpufreq/Kconfig.arm | 8 ++
> > drivers/cpufreq/Makefile | 1 +
> > drivers/cpufreq/arm-cpufreq.c | 269
> > +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 278
> > insertions(+), 0 deletions(-)
> > create mode 100644 drivers/cpufreq/arm-cpufreq.c
>
> looks quite cool, but should also provide a description of the devicetree
> bindings (i.e. like the rest in Documentation/devicetree/bindings/...) for
> future reference.
Yes, Thanks for your remind.
>
> Heiko
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2011-12-17 7:57 ` Richard Zhao
0 siblings, 0 replies; 46+ messages in thread
From: Richard Zhao @ 2011-12-17 7:57 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Dec 16, 2011 at 12:26:03PM +0100, Heiko St?bner wrote:
> Am Freitag, 16. Dezember 2011, 11:30:59 schrieb Richard Zhao:
> > 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.zhao@linaro.org>
> > ---
> > drivers/cpufreq/Kconfig.arm | 8 ++
> > drivers/cpufreq/Makefile | 1 +
> > drivers/cpufreq/arm-cpufreq.c | 269
> > +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 278
> > insertions(+), 0 deletions(-)
> > create mode 100644 drivers/cpufreq/arm-cpufreq.c
>
> looks quite cool, but should also provide a description of the devicetree
> bindings (i.e. like the rest in Documentation/devicetree/bindings/...) for
> future reference.
Yes, Thanks for your remind.
>
> Heiko
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
2011-12-16 10:30 ` Richard Zhao
@ 2011-12-16 14:32 ` Rob Herring
-1 siblings, 0 replies; 46+ messages in thread
From: Rob Herring @ 2011-12-16 14:32 UTC (permalink / raw)
To: Richard Zhao
Cc: linux-arm-kernel, cpufreq, devicetree-discuss, linux,
mark.langsdorf, patches, eric.miao, kernel, davej, linaro-dev,
shawn.guo
On 12/16/2011 04:30 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.zhao@linaro.org>
> ---
> drivers/cpufreq/Kconfig.arm | 8 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/arm-cpufreq.c | 269 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 278 insertions(+), 0 deletions(-)
> create mode 100644 drivers/cpufreq/arm-cpufreq.c
>
What makes this specific to ARM and not a generic DT + clk api +
regulator api driver?
Also, you need documentation of the DT bindings.
Rob
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 72a0044..a0f7d35 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -2,6 +2,14 @@
> # ARM CPU Frequency scaling drivers
> #
>
> +config ARM_GENERIC_CPUFREQ
> + bool "ARM generic"
> + help
> + This adds ARM generic CPUFreq driver. It assumes all
> + cores of the SoC share the same clock and voltage.
> +
> + If in doubt, say N.
> +
> config ARM_S3C64XX_CPUFREQ
> bool "Samsung S3C64XX"
> depends on CPU_S3C6410
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index ce75fcb..6ccf02d 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -39,6 +39,7 @@ obj-$(CONFIG_X86_CPUFREQ_NFORCE2) += cpufreq-nforce2.o
>
> ##################################################################################
> # ARM SoC drivers
> +obj-$(CONFIG_ARM_GENERIC_CPUFREQ) += arm-cpufreq.o
> obj-$(CONFIG_UX500_SOC_DB8500) += db8500-cpufreq.o
> obj-$(CONFIG_ARM_S3C64XX_CPUFREQ) += s3c64xx-cpufreq.o
> obj-$(CONFIG_ARM_S5PV210_CPUFREQ) += s5pv210-cpufreq.o
> diff --git a/drivers/cpufreq/arm-cpufreq.c b/drivers/cpufreq/arm-cpufreq.c
> new file mode 100644
> index 0000000..e4d20da
> --- /dev/null
> +++ b/drivers/cpufreq/arm-cpufreq.c
> @@ -0,0 +1,269 @@
> +/*
> + * 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>
> +#include <asm/cpu.h>
> +
> +static u32 *cpu_freqs; /* HZ */
> +static u32 *cpu_volts; /* uV */
> +static u32 trans_latency; /* ns */
> +static int cpu_op_nr;
> +
> +static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
> +static unsigned long l_p_j_ref_freq;
> +
> +static struct clk *cpu_clk;
> +static struct regulator *cpu_reg;
> +static struct cpufreq_frequency_table *arm_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) {
> + printk(KERN_DEBUG "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 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;
> + unsigned long freq_Hz;
> + int cpu;
> + int ret = 0;
> + unsigned int index;
> +
> + cpufreq_frequency_table_target(policy, arm_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 = clk_round_rate(cpu_clk, cpu_freqs[index]);
> + 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));
> +
> +#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(l_p_j_ref, cpu), l_p_j_ref_freq,
> + 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;
> +
> + 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, arm_freq_table);
> +
> + if (ret < 0) {
> + printk(KERN_ERR "%s: invalid frequency table for cpu %d\n",
> + __func__, policy->cpu);
> + 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);
> + 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 cpu, i, ret;
> +
> + printk(KERN_INFO "ARM SoC generic CPU frequency driver\n");
> +
> + 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_volts, cpu_op_nr);
> + } else
> + printk(KERN_WARNING "cpufreq: invalid cpu_volts!\n");
> + }
> +
> + if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
> + trans_latency = CPUFREQ_ETERNAL;
> +
> + 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;
> + }
> +
> + 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;
> + }
> +
> + if (cpu_volts) {
> + cpu_reg = regulator_get(NULL, "cpu");
> + if (IS_ERR(cpu_reg)) {
> + printk(KERN_WARNING
> + "cpufreq: regulator cpu get failed.\n");
> + cpu_reg = NULL;
> + }
> + }
> +
> + l_p_j_ref_freq = clk_get_rate(cpu_clk);
> + for_each_possible_cpu(cpu)
> + per_cpu(l_p_j_ref, cpu) =
> + per_cpu(cpu_data, cpu).loops_per_jiffy;
> +
> + ret = cpufreq_register_driver(&arm_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(arm_freq_table);
> +free_cpu_volts:
> + kfree(cpu_volts);
> +free_cpu_freqs:
> + kfree(cpu_freqs);
> +put_node:
> + of_node_put(cpu0);
> +
> + return ret;
> +}
> +
> +static void arm_cpufreq_driver_exit(void)
> +{
> + cpufreq_unregister_driver(&arm_cpufreq_driver);
> + kfree(cpu_freqs);
> + kfree(cpu_volts);
> + kfree(arm_freq_table);
> + clk_put(cpu_clk);
> +}
> +
> +module_init(arm_cpufreq_driver_init);
> +module_exit(arm_cpufreq_driver_exit);
> +
> +MODULE_AUTHOR("Freescale Semiconductor Inc. Richard Zhao <richard.zhao@freescale.com>");
> +MODULE_DESCRIPTION("ARM SoC generic CPUFreq driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2011-12-16 14:32 ` Rob Herring
0 siblings, 0 replies; 46+ messages in thread
From: Rob Herring @ 2011-12-16 14:32 UTC (permalink / raw)
To: linux-arm-kernel
On 12/16/2011 04:30 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.zhao@linaro.org>
> ---
> drivers/cpufreq/Kconfig.arm | 8 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/arm-cpufreq.c | 269 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 278 insertions(+), 0 deletions(-)
> create mode 100644 drivers/cpufreq/arm-cpufreq.c
>
What makes this specific to ARM and not a generic DT + clk api +
regulator api driver?
Also, you need documentation of the DT bindings.
Rob
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 72a0044..a0f7d35 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -2,6 +2,14 @@
> # ARM CPU Frequency scaling drivers
> #
>
> +config ARM_GENERIC_CPUFREQ
> + bool "ARM generic"
> + help
> + This adds ARM generic CPUFreq driver. It assumes all
> + cores of the SoC share the same clock and voltage.
> +
> + If in doubt, say N.
> +
> config ARM_S3C64XX_CPUFREQ
> bool "Samsung S3C64XX"
> depends on CPU_S3C6410
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index ce75fcb..6ccf02d 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -39,6 +39,7 @@ obj-$(CONFIG_X86_CPUFREQ_NFORCE2) += cpufreq-nforce2.o
>
> ##################################################################################
> # ARM SoC drivers
> +obj-$(CONFIG_ARM_GENERIC_CPUFREQ) += arm-cpufreq.o
> obj-$(CONFIG_UX500_SOC_DB8500) += db8500-cpufreq.o
> obj-$(CONFIG_ARM_S3C64XX_CPUFREQ) += s3c64xx-cpufreq.o
> obj-$(CONFIG_ARM_S5PV210_CPUFREQ) += s5pv210-cpufreq.o
> diff --git a/drivers/cpufreq/arm-cpufreq.c b/drivers/cpufreq/arm-cpufreq.c
> new file mode 100644
> index 0000000..e4d20da
> --- /dev/null
> +++ b/drivers/cpufreq/arm-cpufreq.c
> @@ -0,0 +1,269 @@
> +/*
> + * 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>
> +#include <asm/cpu.h>
> +
> +static u32 *cpu_freqs; /* HZ */
> +static u32 *cpu_volts; /* uV */
> +static u32 trans_latency; /* ns */
> +static int cpu_op_nr;
> +
> +static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
> +static unsigned long l_p_j_ref_freq;
> +
> +static struct clk *cpu_clk;
> +static struct regulator *cpu_reg;
> +static struct cpufreq_frequency_table *arm_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) {
> + printk(KERN_DEBUG "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 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;
> + unsigned long freq_Hz;
> + int cpu;
> + int ret = 0;
> + unsigned int index;
> +
> + cpufreq_frequency_table_target(policy, arm_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 = clk_round_rate(cpu_clk, cpu_freqs[index]);
> + 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));
> +
> +#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(l_p_j_ref, cpu), l_p_j_ref_freq,
> + 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;
> +
> + 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, arm_freq_table);
> +
> + if (ret < 0) {
> + printk(KERN_ERR "%s: invalid frequency table for cpu %d\n",
> + __func__, policy->cpu);
> + 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);
> + 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 cpu, i, ret;
> +
> + printk(KERN_INFO "ARM SoC generic CPU frequency driver\n");
> +
> + cpu0 = of_find_node_by_path("/cpus/cpu at 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_volts, cpu_op_nr);
> + } else
> + printk(KERN_WARNING "cpufreq: invalid cpu_volts!\n");
> + }
> +
> + if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
> + trans_latency = CPUFREQ_ETERNAL;
> +
> + 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;
> + }
> +
> + 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;
> + }
> +
> + if (cpu_volts) {
> + cpu_reg = regulator_get(NULL, "cpu");
> + if (IS_ERR(cpu_reg)) {
> + printk(KERN_WARNING
> + "cpufreq: regulator cpu get failed.\n");
> + cpu_reg = NULL;
> + }
> + }
> +
> + l_p_j_ref_freq = clk_get_rate(cpu_clk);
> + for_each_possible_cpu(cpu)
> + per_cpu(l_p_j_ref, cpu) =
> + per_cpu(cpu_data, cpu).loops_per_jiffy;
> +
> + ret = cpufreq_register_driver(&arm_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(arm_freq_table);
> +free_cpu_volts:
> + kfree(cpu_volts);
> +free_cpu_freqs:
> + kfree(cpu_freqs);
> +put_node:
> + of_node_put(cpu0);
> +
> + return ret;
> +}
> +
> +static void arm_cpufreq_driver_exit(void)
> +{
> + cpufreq_unregister_driver(&arm_cpufreq_driver);
> + kfree(cpu_freqs);
> + kfree(cpu_volts);
> + kfree(arm_freq_table);
> + clk_put(cpu_clk);
> +}
> +
> +module_init(arm_cpufreq_driver_init);
> +module_exit(arm_cpufreq_driver_exit);
> +
> +MODULE_AUTHOR("Freescale Semiconductor Inc. Richard Zhao <richard.zhao@freescale.com>");
> +MODULE_DESCRIPTION("ARM SoC generic CPUFreq driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
2011-12-16 14:32 ` Rob Herring
@ 2011-12-17 8:00 ` Richard Zhao
-1 siblings, 0 replies; 46+ messages in thread
From: Richard Zhao @ 2011-12-17 8:00 UTC (permalink / raw)
To: Rob Herring
Cc: linux-arm-kernel, cpufreq, devicetree-discuss, linux,
mark.langsdorf, patches, eric.miao, kernel, davej, linaro-dev,
shawn.guo
On Fri, Dec 16, 2011 at 08:32:35AM -0600, Rob Herring wrote:
> On 12/16/2011 04:30 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.zhao@linaro.org>
> > ---
> > drivers/cpufreq/Kconfig.arm | 8 ++
> > drivers/cpufreq/Makefile | 1 +
> > drivers/cpufreq/arm-cpufreq.c | 269 +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 278 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/cpufreq/arm-cpufreq.c
> >
>
> What makes this specific to ARM and not a generic DT + clk api +
> regulator api driver?
smp loops_per_jiffy update needs arm header <asm/cpu.h>.
>
> Also, you need documentation of the DT bindings.
Yes, Thanks.
Richard
>
> Rob
>
> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > index 72a0044..a0f7d35 100644
> > --- a/drivers/cpufreq/Kconfig.arm
> > +++ b/drivers/cpufreq/Kconfig.arm
> > @@ -2,6 +2,14 @@
> > # ARM CPU Frequency scaling drivers
> > #
> >
> > +config ARM_GENERIC_CPUFREQ
> > + bool "ARM generic"
> > + help
> > + This adds ARM generic CPUFreq driver. It assumes all
> > + cores of the SoC share the same clock and voltage.
> > +
> > + If in doubt, say N.
> > +
> > config ARM_S3C64XX_CPUFREQ
> > bool "Samsung S3C64XX"
> > depends on CPU_S3C6410
> > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > index ce75fcb..6ccf02d 100644
> > --- a/drivers/cpufreq/Makefile
> > +++ b/drivers/cpufreq/Makefile
> > @@ -39,6 +39,7 @@ obj-$(CONFIG_X86_CPUFREQ_NFORCE2) += cpufreq-nforce2.o
> >
> > ##################################################################################
> > # ARM SoC drivers
> > +obj-$(CONFIG_ARM_GENERIC_CPUFREQ) += arm-cpufreq.o
> > obj-$(CONFIG_UX500_SOC_DB8500) += db8500-cpufreq.o
> > obj-$(CONFIG_ARM_S3C64XX_CPUFREQ) += s3c64xx-cpufreq.o
> > obj-$(CONFIG_ARM_S5PV210_CPUFREQ) += s5pv210-cpufreq.o
> > diff --git a/drivers/cpufreq/arm-cpufreq.c b/drivers/cpufreq/arm-cpufreq.c
> > new file mode 100644
> > index 0000000..e4d20da
> > --- /dev/null
> > +++ b/drivers/cpufreq/arm-cpufreq.c
> > @@ -0,0 +1,269 @@
> > +/*
> > + * 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>
> > +#include <asm/cpu.h>
> > +
> > +static u32 *cpu_freqs; /* HZ */
> > +static u32 *cpu_volts; /* uV */
> > +static u32 trans_latency; /* ns */
> > +static int cpu_op_nr;
> > +
> > +static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
> > +static unsigned long l_p_j_ref_freq;
> > +
> > +static struct clk *cpu_clk;
> > +static struct regulator *cpu_reg;
> > +static struct cpufreq_frequency_table *arm_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) {
> > + printk(KERN_DEBUG "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 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;
> > + unsigned long freq_Hz;
> > + int cpu;
> > + int ret = 0;
> > + unsigned int index;
> > +
> > + cpufreq_frequency_table_target(policy, arm_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 = clk_round_rate(cpu_clk, cpu_freqs[index]);
> > + 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));
> > +
> > +#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(l_p_j_ref, cpu), l_p_j_ref_freq,
> > + 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;
> > +
> > + 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, arm_freq_table);
> > +
> > + if (ret < 0) {
> > + printk(KERN_ERR "%s: invalid frequency table for cpu %d\n",
> > + __func__, policy->cpu);
> > + 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);
> > + 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 cpu, i, ret;
> > +
> > + printk(KERN_INFO "ARM SoC generic CPU frequency driver\n");
> > +
> > + 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_volts, cpu_op_nr);
> > + } else
> > + printk(KERN_WARNING "cpufreq: invalid cpu_volts!\n");
> > + }
> > +
> > + if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
> > + trans_latency = CPUFREQ_ETERNAL;
> > +
> > + 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;
> > + }
> > +
> > + 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;
> > + }
> > +
> > + if (cpu_volts) {
> > + cpu_reg = regulator_get(NULL, "cpu");
> > + if (IS_ERR(cpu_reg)) {
> > + printk(KERN_WARNING
> > + "cpufreq: regulator cpu get failed.\n");
> > + cpu_reg = NULL;
> > + }
> > + }
> > +
> > + l_p_j_ref_freq = clk_get_rate(cpu_clk);
> > + for_each_possible_cpu(cpu)
> > + per_cpu(l_p_j_ref, cpu) =
> > + per_cpu(cpu_data, cpu).loops_per_jiffy;
> > +
> > + ret = cpufreq_register_driver(&arm_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(arm_freq_table);
> > +free_cpu_volts:
> > + kfree(cpu_volts);
> > +free_cpu_freqs:
> > + kfree(cpu_freqs);
> > +put_node:
> > + of_node_put(cpu0);
> > +
> > + return ret;
> > +}
> > +
> > +static void arm_cpufreq_driver_exit(void)
> > +{
> > + cpufreq_unregister_driver(&arm_cpufreq_driver);
> > + kfree(cpu_freqs);
> > + kfree(cpu_volts);
> > + kfree(arm_freq_table);
> > + clk_put(cpu_clk);
> > +}
> > +
> > +module_init(arm_cpufreq_driver_init);
> > +module_exit(arm_cpufreq_driver_exit);
> > +
> > +MODULE_AUTHOR("Freescale Semiconductor Inc. Richard Zhao <richard.zhao@freescale.com>");
> > +MODULE_DESCRIPTION("ARM SoC generic CPUFreq driver");
> > +MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2011-12-17 8:00 ` Richard Zhao
0 siblings, 0 replies; 46+ messages in thread
From: Richard Zhao @ 2011-12-17 8:00 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Dec 16, 2011 at 08:32:35AM -0600, Rob Herring wrote:
> On 12/16/2011 04:30 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.zhao@linaro.org>
> > ---
> > drivers/cpufreq/Kconfig.arm | 8 ++
> > drivers/cpufreq/Makefile | 1 +
> > drivers/cpufreq/arm-cpufreq.c | 269 +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 278 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/cpufreq/arm-cpufreq.c
> >
>
> What makes this specific to ARM and not a generic DT + clk api +
> regulator api driver?
smp loops_per_jiffy update needs arm header <asm/cpu.h>.
>
> Also, you need documentation of the DT bindings.
Yes, Thanks.
Richard
>
> Rob
>
> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > index 72a0044..a0f7d35 100644
> > --- a/drivers/cpufreq/Kconfig.arm
> > +++ b/drivers/cpufreq/Kconfig.arm
> > @@ -2,6 +2,14 @@
> > # ARM CPU Frequency scaling drivers
> > #
> >
> > +config ARM_GENERIC_CPUFREQ
> > + bool "ARM generic"
> > + help
> > + This adds ARM generic CPUFreq driver. It assumes all
> > + cores of the SoC share the same clock and voltage.
> > +
> > + If in doubt, say N.
> > +
> > config ARM_S3C64XX_CPUFREQ
> > bool "Samsung S3C64XX"
> > depends on CPU_S3C6410
> > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > index ce75fcb..6ccf02d 100644
> > --- a/drivers/cpufreq/Makefile
> > +++ b/drivers/cpufreq/Makefile
> > @@ -39,6 +39,7 @@ obj-$(CONFIG_X86_CPUFREQ_NFORCE2) += cpufreq-nforce2.o
> >
> > ##################################################################################
> > # ARM SoC drivers
> > +obj-$(CONFIG_ARM_GENERIC_CPUFREQ) += arm-cpufreq.o
> > obj-$(CONFIG_UX500_SOC_DB8500) += db8500-cpufreq.o
> > obj-$(CONFIG_ARM_S3C64XX_CPUFREQ) += s3c64xx-cpufreq.o
> > obj-$(CONFIG_ARM_S5PV210_CPUFREQ) += s5pv210-cpufreq.o
> > diff --git a/drivers/cpufreq/arm-cpufreq.c b/drivers/cpufreq/arm-cpufreq.c
> > new file mode 100644
> > index 0000000..e4d20da
> > --- /dev/null
> > +++ b/drivers/cpufreq/arm-cpufreq.c
> > @@ -0,0 +1,269 @@
> > +/*
> > + * 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>
> > +#include <asm/cpu.h>
> > +
> > +static u32 *cpu_freqs; /* HZ */
> > +static u32 *cpu_volts; /* uV */
> > +static u32 trans_latency; /* ns */
> > +static int cpu_op_nr;
> > +
> > +static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
> > +static unsigned long l_p_j_ref_freq;
> > +
> > +static struct clk *cpu_clk;
> > +static struct regulator *cpu_reg;
> > +static struct cpufreq_frequency_table *arm_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) {
> > + printk(KERN_DEBUG "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 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;
> > + unsigned long freq_Hz;
> > + int cpu;
> > + int ret = 0;
> > + unsigned int index;
> > +
> > + cpufreq_frequency_table_target(policy, arm_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 = clk_round_rate(cpu_clk, cpu_freqs[index]);
> > + 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));
> > +
> > +#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(l_p_j_ref, cpu), l_p_j_ref_freq,
> > + 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;
> > +
> > + 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, arm_freq_table);
> > +
> > + if (ret < 0) {
> > + printk(KERN_ERR "%s: invalid frequency table for cpu %d\n",
> > + __func__, policy->cpu);
> > + 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);
> > + 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 cpu, i, ret;
> > +
> > + printk(KERN_INFO "ARM SoC generic CPU frequency driver\n");
> > +
> > + cpu0 = of_find_node_by_path("/cpus/cpu at 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_volts, cpu_op_nr);
> > + } else
> > + printk(KERN_WARNING "cpufreq: invalid cpu_volts!\n");
> > + }
> > +
> > + if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
> > + trans_latency = CPUFREQ_ETERNAL;
> > +
> > + 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;
> > + }
> > +
> > + 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;
> > + }
> > +
> > + if (cpu_volts) {
> > + cpu_reg = regulator_get(NULL, "cpu");
> > + if (IS_ERR(cpu_reg)) {
> > + printk(KERN_WARNING
> > + "cpufreq: regulator cpu get failed.\n");
> > + cpu_reg = NULL;
> > + }
> > + }
> > +
> > + l_p_j_ref_freq = clk_get_rate(cpu_clk);
> > + for_each_possible_cpu(cpu)
> > + per_cpu(l_p_j_ref, cpu) =
> > + per_cpu(cpu_data, cpu).loops_per_jiffy;
> > +
> > + ret = cpufreq_register_driver(&arm_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(arm_freq_table);
> > +free_cpu_volts:
> > + kfree(cpu_volts);
> > +free_cpu_freqs:
> > + kfree(cpu_freqs);
> > +put_node:
> > + of_node_put(cpu0);
> > +
> > + return ret;
> > +}
> > +
> > +static void arm_cpufreq_driver_exit(void)
> > +{
> > + cpufreq_unregister_driver(&arm_cpufreq_driver);
> > + kfree(cpu_freqs);
> > + kfree(cpu_volts);
> > + kfree(arm_freq_table);
> > + clk_put(cpu_clk);
> > +}
> > +
> > +module_init(arm_cpufreq_driver_init);
> > +module_exit(arm_cpufreq_driver_exit);
> > +
> > +MODULE_AUTHOR("Freescale Semiconductor Inc. Richard Zhao <richard.zhao@freescale.com>");
> > +MODULE_DESCRIPTION("ARM SoC generic CPUFreq driver");
> > +MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
2011-12-17 8:00 ` Richard Zhao
@ 2011-12-17 9:29 ` Arnd Bergmann
-1 siblings, 0 replies; 46+ messages in thread
From: Arnd Bergmann @ 2011-12-17 9:29 UTC (permalink / raw)
To: linaro-dev
Cc: linux, patches, devicetree-discuss, cpufreq, eric.miao, kernel,
davej, Richard Zhao, linux-arm-kernel
On Saturday 17 December 2011 16:00:03 Richard Zhao wrote:
> On Fri, Dec 16, 2011 at 08:32:35AM -0600, Rob Herring wrote:
> > On 12/16/2011 04:30 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.zhao@linaro.org>
> > > ---
> > > drivers/cpufreq/Kconfig.arm | 8 ++
> > > drivers/cpufreq/Makefile | 1 +
> > > drivers/cpufreq/arm-cpufreq.c | 269 +++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 278 insertions(+), 0 deletions(-)
> > > create mode 100644 drivers/cpufreq/arm-cpufreq.c
> > >
> >
> > What makes this specific to ARM and not a generic DT + clk api +
> > regulator api driver?
>
> smp loops_per_jiffy update needs arm header <asm/cpu.h>.
I would suggest to instead change the definition of adjust_jiffies in the
core so it can be overridden by the architecture, like this
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 987a165..174584d 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -189,6 +189,7 @@ EXPORT_SYMBOL_GPL(cpufreq_cpu_put);
* systems as each CPU might be scaled differently. So, use the arch
* per-CPU loops_per_jiffy value wherever possible.
*/
+#ifndef adjust_jiffies
#ifndef CONFIG_SMP
static unsigned long l_p_j_ref;
static unsigned int l_p_j_ref_freq;
@@ -218,7 +219,8 @@ static inline void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci)
{
return;
}
-#endif
+#endif /* CONFIG_SMP */
+#endif /* adjust_jiffies */
/**
Then ARM (and any others that want the driver) can provide their own
implementation and set
#define adjust_jiffies(val, ci) adjust_jiffies((val), (ci))
to let the core use that instead of the generic UP version.
While we're there, we should probably try to fix drivers that use loops_per_jiffy,
because that is not what they think it is on SMP.
Arnd
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2011-12-17 9:29 ` Arnd Bergmann
0 siblings, 0 replies; 46+ messages in thread
From: Arnd Bergmann @ 2011-12-17 9:29 UTC (permalink / raw)
To: linux-arm-kernel
On Saturday 17 December 2011 16:00:03 Richard Zhao wrote:
> On Fri, Dec 16, 2011 at 08:32:35AM -0600, Rob Herring wrote:
> > On 12/16/2011 04:30 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.zhao@linaro.org>
> > > ---
> > > drivers/cpufreq/Kconfig.arm | 8 ++
> > > drivers/cpufreq/Makefile | 1 +
> > > drivers/cpufreq/arm-cpufreq.c | 269 +++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 278 insertions(+), 0 deletions(-)
> > > create mode 100644 drivers/cpufreq/arm-cpufreq.c
> > >
> >
> > What makes this specific to ARM and not a generic DT + clk api +
> > regulator api driver?
>
> smp loops_per_jiffy update needs arm header <asm/cpu.h>.
I would suggest to instead change the definition of adjust_jiffies in the
core so it can be overridden by the architecture, like this
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 987a165..174584d 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -189,6 +189,7 @@ EXPORT_SYMBOL_GPL(cpufreq_cpu_put);
* systems as each CPU might be scaled differently. So, use the arch
* per-CPU loops_per_jiffy value wherever possible.
*/
+#ifndef adjust_jiffies
#ifndef CONFIG_SMP
static unsigned long l_p_j_ref;
static unsigned int l_p_j_ref_freq;
@@ -218,7 +219,8 @@ static inline void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci)
{
return;
}
-#endif
+#endif /* CONFIG_SMP */
+#endif /* adjust_jiffies */
/**
Then ARM (and any others that want the driver) can provide their own
implementation and set
#define adjust_jiffies(val, ci) adjust_jiffies((val), (ci))
to let the core use that instead of the generic UP version.
While we're there, we should probably try to fix drivers that use loops_per_jiffy,
because that is not what they think it is on SMP.
Arnd
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
2011-12-17 9:29 ` Arnd Bergmann
@ 2011-12-18 12:34 ` Richard Zhao
-1 siblings, 0 replies; 46+ messages in thread
From: Richard Zhao @ 2011-12-18 12:34 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linaro-dev, linux, patches, devicetree-discuss, cpufreq,
eric.miao, kernel, davej, linux-arm-kernel
On Sat, Dec 17, 2011 at 10:29:29AM +0100, Arnd Bergmann wrote:
> On Saturday 17 December 2011 16:00:03 Richard Zhao wrote:
> > On Fri, Dec 16, 2011 at 08:32:35AM -0600, Rob Herring wrote:
> > > On 12/16/2011 04:30 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.zhao@linaro.org>
> > > > ---
> > > > drivers/cpufreq/Kconfig.arm | 8 ++
> > > > drivers/cpufreq/Makefile | 1 +
> > > > drivers/cpufreq/arm-cpufreq.c | 269 +++++++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 278 insertions(+), 0 deletions(-)
> > > > create mode 100644 drivers/cpufreq/arm-cpufreq.c
> > > >
> > >
> > > What makes this specific to ARM and not a generic DT + clk api +
> > > regulator api driver?
> >
> > smp loops_per_jiffy update needs arm header <asm/cpu.h>.
>
> I would suggest to instead change the definition of adjust_jiffies in the
> core so it can be overridden by the architecture, like this
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 987a165..174584d 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -189,6 +189,7 @@ EXPORT_SYMBOL_GPL(cpufreq_cpu_put);
> * systems as each CPU might be scaled differently. So, use the arch
> * per-CPU loops_per_jiffy value wherever possible.
> */
> +#ifndef adjust_jiffies
> #ifndef CONFIG_SMP
> static unsigned long l_p_j_ref;
> static unsigned int l_p_j_ref_freq;
> @@ -218,7 +219,8 @@ static inline void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci)
> {
> return;
> }
> -#endif
> +#endif /* CONFIG_SMP */
> +#endif /* adjust_jiffies */
>
>
> /**
>
>
> Then ARM (and any others that want the driver) can provide their own
> implementation and set
>
> #define adjust_jiffies(val, ci) adjust_jiffies((val), (ci))
>
> to let the core use that instead of the generic UP version.
>
>
> While we're there, we should probably try to fix drivers that use loops_per_jiffy,
> because that is not what they think it is on SMP.
Or let different arch register its different CPUFREQ_TRANSITION_NOTIFIER ?
Thanks
Richard
>
> Arnd
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2011-12-18 12:34 ` Richard Zhao
0 siblings, 0 replies; 46+ messages in thread
From: Richard Zhao @ 2011-12-18 12:34 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Dec 17, 2011 at 10:29:29AM +0100, Arnd Bergmann wrote:
> On Saturday 17 December 2011 16:00:03 Richard Zhao wrote:
> > On Fri, Dec 16, 2011 at 08:32:35AM -0600, Rob Herring wrote:
> > > On 12/16/2011 04:30 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.zhao@linaro.org>
> > > > ---
> > > > drivers/cpufreq/Kconfig.arm | 8 ++
> > > > drivers/cpufreq/Makefile | 1 +
> > > > drivers/cpufreq/arm-cpufreq.c | 269 +++++++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 278 insertions(+), 0 deletions(-)
> > > > create mode 100644 drivers/cpufreq/arm-cpufreq.c
> > > >
> > >
> > > What makes this specific to ARM and not a generic DT + clk api +
> > > regulator api driver?
> >
> > smp loops_per_jiffy update needs arm header <asm/cpu.h>.
>
> I would suggest to instead change the definition of adjust_jiffies in the
> core so it can be overridden by the architecture, like this
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 987a165..174584d 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -189,6 +189,7 @@ EXPORT_SYMBOL_GPL(cpufreq_cpu_put);
> * systems as each CPU might be scaled differently. So, use the arch
> * per-CPU loops_per_jiffy value wherever possible.
> */
> +#ifndef adjust_jiffies
> #ifndef CONFIG_SMP
> static unsigned long l_p_j_ref;
> static unsigned int l_p_j_ref_freq;
> @@ -218,7 +219,8 @@ static inline void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci)
> {
> return;
> }
> -#endif
> +#endif /* CONFIG_SMP */
> +#endif /* adjust_jiffies */
>
>
> /**
>
>
> Then ARM (and any others that want the driver) can provide their own
> implementation and set
>
> #define adjust_jiffies(val, ci) adjust_jiffies((val), (ci))
>
> to let the core use that instead of the generic UP version.
>
>
> While we're there, we should probably try to fix drivers that use loops_per_jiffy,
> because that is not what they think it is on SMP.
Or let different arch register its different CPUFREQ_TRANSITION_NOTIFIER ?
Thanks
Richard
>
> Arnd
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
2011-12-16 10:30 ` Richard Zhao
@ 2011-12-20 14:41 ` Mark Brown
-1 siblings, 0 replies; 46+ messages in thread
From: Mark Brown @ 2011-12-20 14:41 UTC (permalink / raw)
To: Richard Zhao
Cc: linux-arm-kernel, cpufreq, devicetree-discuss, linux,
mark.langsdorf, patches, eric.miao, kernel, davej, linaro-dev,
shawn.guo
On Fri, Dec 16, 2011 at 06:30:59PM +0800, Richard Zhao wrote:
> +
> + if (higher && cpu_reg)
> + regulator_set_voltage(cpu_reg,
> + cpu_volts[index], cpu_volts[index]);
This is really bad, you're only supporting the configuration of a
specific voltage which doesn't reflect what hardware does (things will
be specified as a range of voltages) and will needlessly cause
interoperability problems between chips and regulators if the regulator
can't hit the *exact* voltage requested. There's a good solid reason
why the regulator API specifies everything in terms of voltage ranges.
> + ret = clk_set_rate(cpu_clk, freq);
> + if (ret != 0) {
> + printk(KERN_DEBUG "cannot set CPU clock rate\n");
> + return ret;
> + }
This error checking is really random - you're ignoring some errors and
logging the errors you do detect as debug messages which seems odd.
Similar issues apply throughough.
> + if (cpu_volts) {
> + cpu_reg = regulator_get(NULL, "cpu");
> + if (IS_ERR(cpu_reg)) {
> + printk(KERN_WARNING
> + "cpufreq: regulator cpu get failed.\n");
> + cpu_reg = NULL;
> + }
> + }
You should log what the error was. You're also not doing anything to
check that the voltage ranges required by the frequencies are actually
supported on this system, the driver should double check this so that
governors don't sit there trying to set voltages that are impossible on
a given board.
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2011-12-20 14:41 ` Mark Brown
0 siblings, 0 replies; 46+ messages in thread
From: Mark Brown @ 2011-12-20 14:41 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Dec 16, 2011 at 06:30:59PM +0800, Richard Zhao wrote:
> +
> + if (higher && cpu_reg)
> + regulator_set_voltage(cpu_reg,
> + cpu_volts[index], cpu_volts[index]);
This is really bad, you're only supporting the configuration of a
specific voltage which doesn't reflect what hardware does (things will
be specified as a range of voltages) and will needlessly cause
interoperability problems between chips and regulators if the regulator
can't hit the *exact* voltage requested. There's a good solid reason
why the regulator API specifies everything in terms of voltage ranges.
> + ret = clk_set_rate(cpu_clk, freq);
> + if (ret != 0) {
> + printk(KERN_DEBUG "cannot set CPU clock rate\n");
> + return ret;
> + }
This error checking is really random - you're ignoring some errors and
logging the errors you do detect as debug messages which seems odd.
Similar issues apply throughough.
> + if (cpu_volts) {
> + cpu_reg = regulator_get(NULL, "cpu");
> + if (IS_ERR(cpu_reg)) {
> + printk(KERN_WARNING
> + "cpufreq: regulator cpu get failed.\n");
> + cpu_reg = NULL;
> + }
> + }
You should log what the error was. You're also not doing anything to
check that the voltage ranges required by the frequencies are actually
supported on this system, the driver should double check this so that
governors don't sit there trying to set voltages that are impossible on
a given board.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
2011-12-16 10:30 ` Richard Zhao
@ 2012-01-18 11:39 ` Mark Brown
-1 siblings, 0 replies; 46+ messages in thread
From: Mark Brown @ 2012-01-18 11:39 UTC (permalink / raw)
To: Richard Zhao
Cc: linux-arm-kernel, cpufreq, devicetree-discuss, linux,
mark.langsdorf, patches, eric.miao, kernel, davej, linaro-dev
On Fri, Dec 16, 2011 at 06:30:59PM +0800, Richard Zhao wrote:
> + 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) {
> + printk(KERN_DEBUG "cannot set CPU clock rate\n");
> + return ret;
> + }
> +
> + if (!higher && cpu_reg)
> + regulator_set_voltage(cpu_reg,
> + cpu_volts[index], cpu_volts[index]);
This appears to reintroduce the setting of an exact voltage which I'm
sure was fixed in previous versions of the patch.
> +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",
> +};
This code doesn't actually look terribly ARM specific...
> + printk(KERN_INFO "ARM SoC generic CPU frequency driver\n");
Do we need this?
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2012-01-18 11:39 ` Mark Brown
0 siblings, 0 replies; 46+ messages in thread
From: Mark Brown @ 2012-01-18 11:39 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Dec 16, 2011 at 06:30:59PM +0800, Richard Zhao wrote:
> + 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) {
> + printk(KERN_DEBUG "cannot set CPU clock rate\n");
> + return ret;
> + }
> +
> + if (!higher && cpu_reg)
> + regulator_set_voltage(cpu_reg,
> + cpu_volts[index], cpu_volts[index]);
This appears to reintroduce the setting of an exact voltage which I'm
sure was fixed in previous versions of the patch.
> +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",
> +};
This code doesn't actually look terribly ARM specific...
> + printk(KERN_INFO "ARM SoC generic CPU frequency driver\n");
Do we need this?
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
2012-01-18 11:39 ` Mark Brown
@ 2012-01-18 11:42 ` Mark Brown
-1 siblings, 0 replies; 46+ messages in thread
From: Mark Brown @ 2012-01-18 11:42 UTC (permalink / raw)
To: Richard Zhao
Cc: linux-arm-kernel, cpufreq, devicetree-discuss, linux,
mark.langsdorf, patches, eric.miao, kernel, davej, linaro-dev
On Wed, Jan 18, 2012 at 11:39:50AM +0000, Mark Brown wrote:
> This appears to reintroduce the setting of an exact voltage which I'm
> sure was fixed in previous versions of the patch.
Erk, sorry - it looks like the device tree list has quite a bit of lag
in moderation and sent out some old patches which I didn't notice. All
this has been fixed in subsequent verisons.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2012-01-18 11:42 ` Mark Brown
0 siblings, 0 replies; 46+ messages in thread
From: Mark Brown @ 2012-01-18 11:42 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jan 18, 2012 at 11:39:50AM +0000, Mark Brown wrote:
> This appears to reintroduce the setting of an exact voltage which I'm
> sure was fixed in previous versions of the patch.
Erk, sorry - it looks like the device tree list has quite a bit of lag
in moderation and sent out some old patches which I didn't notice. All
this has been fixed in subsequent verisons.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
2012-01-18 11:42 ` Mark Brown
@ 2012-01-18 20:51 ` Grant Likely
-1 siblings, 0 replies; 46+ messages in thread
From: Grant Likely @ 2012-01-18 20:51 UTC (permalink / raw)
To: Mark Brown
Cc: Richard Zhao, linux, mark.langsdorf, patches, devicetree-discuss,
cpufreq, eric.miao, kernel, davej, linaro-dev, linux-arm-kernel
On Wed, Jan 18, 2012 at 11:42:28AM +0000, Mark Brown wrote:
> On Wed, Jan 18, 2012 at 11:39:50AM +0000, Mark Brown wrote:
>
> > This appears to reintroduce the setting of an exact voltage which I'm
> > sure was fixed in previous versions of the patch.
>
> Erk, sorry - it looks like the device tree list has quite a bit of lag
> in moderation and sent out some old patches which I didn't notice. All
> this has been fixed in subsequent verisons.
Yeah, that's my fault. I'm rather a bursty moderator at times.
g.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
@ 2012-01-18 20:51 ` Grant Likely
0 siblings, 0 replies; 46+ messages in thread
From: Grant Likely @ 2012-01-18 20:51 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jan 18, 2012 at 11:42:28AM +0000, Mark Brown wrote:
> On Wed, Jan 18, 2012 at 11:39:50AM +0000, Mark Brown wrote:
>
> > This appears to reintroduce the setting of an exact voltage which I'm
> > sure was fixed in previous versions of the patch.
>
> Erk, sorry - it looks like the device tree list has quite a bit of lag
> in moderation and sent out some old patches which I didn't notice. All
> this has been fixed in subsequent verisons.
Yeah, that's my fault. I'm rather a bursty moderator at times.
g.
^ permalink raw reply [flat|nested] 46+ messages in thread