[PATCH V2 0/4] add arm soc generic cpufreq driver

[PATCH V2 0/4] add arm soc generic cpufreq driver

[Richard Zhao]

The driver support single core and multi core ARM SoCs. For multi core,
it assume all cores share the same clock and voltage.

Changes in v2:
 - add volatage change support
 - change '_' in property name to '-'
 - use initial value to calculate loops_per_jiffy
 - fix reading cpu_volts property bug 
 - let cpufreq_frequency_table_cpuinfo routines handle cpu_freq_khz_max/min
 - don't change freq in arm_cpufreq_exit, because every core share the same freq.
 - use unsigned long describe frequency as much as possible. Because clk use
   unsigned long, but cpufreq use unsigned int.

[PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
[PATCH V2 2/4] dts/imx6q: add cpufreq property
[PATCH V2 3/4] arm/imx6q: register arm_clk as cpu to clkdev
[PATCH V2 4/4] arm/imx6q: select ARCH_HAS_CPUFREQ

Thanks
Richard

[PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[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

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");
-- 
1.7.5.4

Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[Jamie Iles]

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

Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[Bryan Huntsman]

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.

Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[Richard Zhao]

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.

Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[Richard Zhao]

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
> 

Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[Stephen Boyd]

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.

Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[Richard Zhao]

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

Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[Heiko Stübner]

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

Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[Richard Zhao]

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
> 

Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[Rob Herring]

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");

Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[Richard Zhao]

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");
> 

Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[Arnd Bergmann]

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

Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[Richard Zhao]

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

Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[Mark Brown]

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.

Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[Mark Brown]

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?

Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[Mark Brown]

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.

Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver

[Grant Likely]

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.

[PATCH V2 2/4] dts/imx6q: add cpufreq property

[Richard Zhao]

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 arch/arm/boot/dts/imx6q.dtsi |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 263e8f3..f2e3eaf 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -29,6 +29,9 @@
 			compatible = "arm,cortex-a9";
 			reg = <0>;
 			next-level-cache = <&L2>;
+			cpu-freqs = <996000000 792000000 396000000 198000000>;
+			cpu-volts = <1225000 1100000 950000 850000>;
+			trans-latency = <61036>; /* two CLK32 periods */
 		};
 
 		cpu@1 {
-- 
1.7.5.4

[PATCH V2 3/4] arm/imx6q: register arm_clk as cpu to clkdev

[Richard Zhao]

cpufreq needs cpu clock to change frequency.

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 arch/arm/mach-imx/clock-imx6q.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-imx/clock-imx6q.c b/arch/arm/mach-imx/clock-imx6q.c
index 039a7ab..72acbc2 100644
--- a/arch/arm/mach-imx/clock-imx6q.c
+++ b/arch/arm/mach-imx/clock-imx6q.c
@@ -1911,6 +1911,7 @@ static struct clk_lookup lookups[] = {
 	_REGISTER_CLOCK(NULL, "gpmi_io_clk", gpmi_io_clk),
 	_REGISTER_CLOCK(NULL, "usboh3_clk", usboh3_clk),
 	_REGISTER_CLOCK(NULL, "sata_clk", sata_clk),
+	_REGISTER_CLOCK(NULL, "cpu", arm_clk),
 };
 
 int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode)
-- 
1.7.5.4

RE: [PATCH V2 3/4] arm/imx6q: register arm_clk as cpu to clkdev

[Mark Langsdorf]

Is there a portable/generic approach for other drivers that may
want to use arm-cpufreq.c? arm_clk is not normally defined for
my SoC and I don't see an easy way to pull it in.

--Mark Langsdorf
Calxeda, Inc.

________________________________________
From: Richard Zhao [richard.zhao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org]
Sent: Friday, December 16, 2011 4:31 AM
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org; cpufreq-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
Cc: linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org; davej-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org; shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; eric.miao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; Mark Langsdorf; linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org; patches-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; Richard Zhao
Subject: [PATCH V2 3/4] arm/imx6q: register arm_clk as cpu to clkdev

cpufreq needs cpu clock to change frequency.

Signed-off-by: Richard Zhao <richard.zhao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/mach-imx/clock-imx6q.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-imx/clock-imx6q.c b/arch/arm/mach-imx/clock-imx6q.c
index 039a7ab..72acbc2 100644
--- a/arch/arm/mach-imx/clock-imx6q.c
+++ b/arch/arm/mach-imx/clock-imx6q.c
@@ -1911,6 +1911,7 @@ static struct clk_lookup lookups[] = {
        _REGISTER_CLOCK(NULL, "gpmi_io_clk", gpmi_io_clk),
        _REGISTER_CLOCK(NULL, "usboh3_clk", usboh3_clk),
        _REGISTER_CLOCK(NULL, "sata_clk", sata_clk),
+       _REGISTER_CLOCK(NULL, "cpu", arm_clk),
 };

 int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode)
--
1.7.5.4

Re: [PATCH V2 3/4] arm/imx6q: register arm_clk as cpu to clkdev

[Richard Zhao]

On Fri, Dec 16, 2011 at 11:35:39AM -0500, Mark Langsdorf wrote:
> Is there a portable/generic approach for other drivers that may
> want to use arm-cpufreq.c? arm_clk is not normally defined for
> my SoC and I don't see an easy way to pull it in.
Could you tell me the details? Is your board arch/arm/mach-highbank/ Rob
maintained?
clk API is the most generic way for arm as far as I find out.

Thanks
Richard
> 
> --Mark Langsdorf
> Calxeda, Inc.
> 
> ________________________________________
> From: Richard Zhao [richard.zhao@linaro.org]
> Sent: Friday, December 16, 2011 4:31 AM
> To: linux-arm-kernel@lists.infradead.org; cpufreq@vger.kernel.org; devicetree-discuss@lists.ozlabs.org
> Cc: linux@arm.linux.org.uk; davej@redhat.com; kernel@pengutronix.de; shawn.guo@linaro.org; eric.miao@linaro.org; Mark Langsdorf; linaro-dev@lists.linaro.org; patches@linaro.org; Richard Zhao
> Subject: [PATCH V2 3/4] arm/imx6q: register arm_clk as cpu to clkdev
> 
> cpufreq needs cpu clock to change frequency.
> 
> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> ---
>  arch/arm/mach-imx/clock-imx6q.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/clock-imx6q.c b/arch/arm/mach-imx/clock-imx6q.c
> index 039a7ab..72acbc2 100644
> --- a/arch/arm/mach-imx/clock-imx6q.c
> +++ b/arch/arm/mach-imx/clock-imx6q.c
> @@ -1911,6 +1911,7 @@ static struct clk_lookup lookups[] = {
>         _REGISTER_CLOCK(NULL, "gpmi_io_clk", gpmi_io_clk),
>         _REGISTER_CLOCK(NULL, "usboh3_clk", usboh3_clk),
>         _REGISTER_CLOCK(NULL, "sata_clk", sata_clk),
> +       _REGISTER_CLOCK(NULL, "cpu", arm_clk),
>  };
> 
>  int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode)
> --
> 1.7.5.4

[PATCH V2 4/4] arm/imx6q: select ARCH_HAS_CPUFREQ

[Richard Zhao]

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 arch/arm/mach-imx/Kconfig |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index c44aa97..39cf00a 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -595,6 +595,7 @@ comment "i.MX6 family:"
 
 config SOC_IMX6Q
 	bool "i.MX6 Quad support"
+	select ARCH_HAS_CPUFREQ
 	select ARM_GIC
 	select CACHE_L2X0
 	select CPU_V7
-- 
1.7.5.4