[PATCH V3 0/7] add a generic cpufreq driver

[PATCH V3 0/7] add a 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.

TODO:
 - Add each core seperate freq/volt support (MSM).

Changes in v3:
 - move adjusting smp loops_per_jiffy to arm common code,
   and also adjust global loops_per_jiffy.
 - remove adjusting loops_per_jiffy in imx and omap cpufreq drivers.
 - check compatible "generic-cpufreq" when module_init
 - change printk to pr_xxx
 - add generic-cpufreq DT binding doc

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 V3 1/7] ARM: add cpufreq transiton notifier to adjust
[PATCH V3 2/7] arm/imx: cpufreq: remove loops_per_jiffy recalculate
[PATCH V3 3/7] cpufreq: OMAP: remove loops_per_jiffy recalculate for
[PATCH V3 4/7] cpufreq: add generic cpufreq driver
[PATCH V3 5/7] dts/imx6q: add cpufreq property
[PATCH V3 6/7] arm/imx6q: register arm_clk as cpu to clkdev
[PATCH V3 7/7] arm/imx6q: select ARCH_HAS_CPUFREQ

[PATCH V3 0/7] add a 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.

TODO:
 - Add each core seperate freq/volt support (MSM).

Changes in v3:
 - move adjusting smp loops_per_jiffy to arm common code,
   and also adjust global loops_per_jiffy.
 - remove adjusting loops_per_jiffy in imx and omap cpufreq drivers.
 - check compatible "generic-cpufreq" when module_init
 - change printk to pr_xxx
 - add generic-cpufreq DT binding doc

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 V3 1/7] ARM: add cpufreq transiton notifier to adjust
[PATCH V3 2/7] arm/imx: cpufreq: remove loops_per_jiffy recalculate
[PATCH V3 3/7] cpufreq: OMAP: remove loops_per_jiffy recalculate for
[PATCH V3 4/7] cpufreq: add generic cpufreq driver
[PATCH V3 5/7] dts/imx6q: add cpufreq property
[PATCH V3 6/7] arm/imx6q: register arm_clk as cpu to clkdev
[PATCH V3 7/7] arm/imx6q: select ARCH_HAS_CPUFREQ

[PATCH V3 1/7] ARM: add cpufreq transiton notifier to adjust loops_per_jiffy for smp

[Richard Zhao]

If CONFIG_SMP, cpufreq skips loops_per_jiffy update, because different
arch has different per-cpu loops_per_jiffy definition.

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

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index ef5640b..ac9cadc 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -25,6 +25,7 @@
 #include <linux/percpu.h>
 #include <linux/clockchips.h>
 #include <linux/completion.h>
+#include <linux/cpufreq.h>
 
 #include <linux/atomic.h>
 #include <asm/cacheflush.h>
@@ -631,3 +632,56 @@ int setup_profiling_timer(unsigned int multiplier)
 {
 	return -EINVAL;
 }
+
+#ifdef CONFIG_CPU_FREQ
+
+static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
+static DEFINE_PER_CPU(unsigned long, l_p_j_ref_freq);
+static unsigned long global_l_p_j_ref;
+static unsigned long global_l_p_j_ref_freq;
+
+static int cpufreq_callback(struct notifier_block *nb,
+					unsigned long val, void *data)
+{
+	struct cpufreq_freqs *freq = data;
+	int cpu = freq->cpu;
+
+	if (freq->flags & CPUFREQ_CONST_LOOPS)
+		return NOTIFY_OK;
+
+	if (!per_cpu(l_p_j_ref, cpu)) {
+		per_cpu(l_p_j_ref, cpu) =
+			per_cpu(cpu_data, cpu).loops_per_jiffy;
+		per_cpu(l_p_j_ref_freq, cpu) = freq->old;
+		if (!global_l_p_j_ref) {
+			global_l_p_j_ref = loops_per_jiffy;
+			global_l_p_j_ref_freq = freq->old;
+		}
+	}
+
+	if ((val == CPUFREQ_PRECHANGE  && freq->old < freq->new) ||
+	    (val == CPUFREQ_POSTCHANGE && freq->old > freq->new) ||
+	    (val == CPUFREQ_RESUMECHANGE || val == CPUFREQ_SUSPENDCHANGE)) {
+		loops_per_jiffy = cpufreq_scale(global_l_p_j_ref,
+						global_l_p_j_ref_freq,
+						freq->new);
+		per_cpu(cpu_data, cpu).loops_per_jiffy =
+			cpufreq_scale(per_cpu(l_p_j_ref, cpu),
+					per_cpu(l_p_j_ref_freq, cpu),
+					freq->new);
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block cpufreq_notifier = {
+	.notifier_call  = cpufreq_callback,
+};
+
+static int __init register_cpufreq_notifier(void)
+{
+	return cpufreq_register_notifier(&cpufreq_notifier,
+						CPUFREQ_TRANSITION_NOTIFIER);
+}
+core_initcall(register_cpufreq_notifier);
+
+#endif
-- 
1.7.5.4

[PATCH V3 1/7] ARM: add cpufreq transiton notifier to adjust loops_per_jiffy for smp

[Richard Zhao]

If CONFIG_SMP, cpufreq skips loops_per_jiffy update, because different
arch has different per-cpu loops_per_jiffy definition.

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

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index ef5640b..ac9cadc 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -25,6 +25,7 @@
 #include <linux/percpu.h>
 #include <linux/clockchips.h>
 #include <linux/completion.h>
+#include <linux/cpufreq.h>
 
 #include <linux/atomic.h>
 #include <asm/cacheflush.h>
@@ -631,3 +632,56 @@ int setup_profiling_timer(unsigned int multiplier)
 {
 	return -EINVAL;
 }
+
+#ifdef CONFIG_CPU_FREQ
+
+static DEFINE_PER_CPU(unsigned long, l_p_j_ref);
+static DEFINE_PER_CPU(unsigned long, l_p_j_ref_freq);
+static unsigned long global_l_p_j_ref;
+static unsigned long global_l_p_j_ref_freq;
+
+static int cpufreq_callback(struct notifier_block *nb,
+					unsigned long val, void *data)
+{
+	struct cpufreq_freqs *freq = data;
+	int cpu = freq->cpu;
+
+	if (freq->flags & CPUFREQ_CONST_LOOPS)
+		return NOTIFY_OK;
+
+	if (!per_cpu(l_p_j_ref, cpu)) {
+		per_cpu(l_p_j_ref, cpu) =
+			per_cpu(cpu_data, cpu).loops_per_jiffy;
+		per_cpu(l_p_j_ref_freq, cpu) = freq->old;
+		if (!global_l_p_j_ref) {
+			global_l_p_j_ref = loops_per_jiffy;
+			global_l_p_j_ref_freq = freq->old;
+		}
+	}
+
+	if ((val == CPUFREQ_PRECHANGE  && freq->old < freq->new) ||
+	    (val == CPUFREQ_POSTCHANGE && freq->old > freq->new) ||
+	    (val == CPUFREQ_RESUMECHANGE || val == CPUFREQ_SUSPENDCHANGE)) {
+		loops_per_jiffy = cpufreq_scale(global_l_p_j_ref,
+						global_l_p_j_ref_freq,
+						freq->new);
+		per_cpu(cpu_data, cpu).loops_per_jiffy =
+			cpufreq_scale(per_cpu(l_p_j_ref, cpu),
+					per_cpu(l_p_j_ref_freq, cpu),
+					freq->new);
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block cpufreq_notifier = {
+	.notifier_call  = cpufreq_callback,
+};
+
+static int __init register_cpufreq_notifier(void)
+{
+	return cpufreq_register_notifier(&cpufreq_notifier,
+						CPUFREQ_TRANSITION_NOTIFIER);
+}
+core_initcall(register_cpufreq_notifier);
+
+#endif
-- 
1.7.5.4

[PATCH V3 2/7] arm/imx: cpufreq: remove loops_per_jiffy recalculate for smp

[Richard Zhao]

arm registered cpufreq transition notifier to recalculate it.

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 arch/arm/plat-mxc/cpufreq.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/arch/arm/plat-mxc/cpufreq.c b/arch/arm/plat-mxc/cpufreq.c
index c937e75..364793a 100644
--- a/arch/arm/plat-mxc/cpufreq.c
+++ b/arch/arm/plat-mxc/cpufreq.c
@@ -99,16 +99,6 @@ static int mxc_set_target(struct cpufreq_policy *policy,
 
 	ret = set_cpu_freq(freq_Hz);
 
-#ifdef CONFIG_SMP
-	/* loops_per_jiffy is not updated by the cpufreq core for SMP systems.
-	 * So update it for all CPUs.
-	 */
-	for_each_possible_cpu(cpu)
-		per_cpu(cpu_data, cpu).loops_per_jiffy =
-		cpufreq_scale(per_cpu(cpu_data, cpu).loops_per_jiffy,
-					freqs.old, freqs.new);
-#endif
-
 	for_each_possible_cpu(cpu) {
 		freqs.cpu = cpu;
 		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
-- 
1.7.5.4

[PATCH V3 2/7] arm/imx: cpufreq: remove loops_per_jiffy recalculate for smp

[Richard Zhao]

arm registered cpufreq transition notifier to recalculate it.

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 arch/arm/plat-mxc/cpufreq.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/arch/arm/plat-mxc/cpufreq.c b/arch/arm/plat-mxc/cpufreq.c
index c937e75..364793a 100644
--- a/arch/arm/plat-mxc/cpufreq.c
+++ b/arch/arm/plat-mxc/cpufreq.c
@@ -99,16 +99,6 @@ static int mxc_set_target(struct cpufreq_policy *policy,
 
 	ret = set_cpu_freq(freq_Hz);
 
-#ifdef CONFIG_SMP
-	/* loops_per_jiffy is not updated by the cpufreq core for SMP systems.
-	 * So update it for all CPUs.
-	 */
-	for_each_possible_cpu(cpu)
-		per_cpu(cpu_data, cpu).loops_per_jiffy =
-		cpufreq_scale(per_cpu(cpu_data, cpu).loops_per_jiffy,
-					freqs.old, freqs.new);
-#endif
-
 	for_each_possible_cpu(cpu) {
 		freqs.cpu = cpu;
 		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
-- 
1.7.5.4

[PATCH V3 3/7] cpufreq: OMAP: remove loops_per_jiffy recalculate for smp

[Richard Zhao]

arm registered cpufreq transition notifier to recalculate it.

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 drivers/cpufreq/omap-cpufreq.c |   36 ------------------------------------
 1 files changed, 0 insertions(+), 36 deletions(-)

diff --git a/drivers/cpufreq/omap-cpufreq.c b/drivers/cpufreq/omap-cpufreq.c
index 5d04c57..17da4c4 100644
--- a/drivers/cpufreq/omap-cpufreq.c
+++ b/drivers/cpufreq/omap-cpufreq.c
@@ -37,16 +37,6 @@
 
 #include <mach/hardware.h>
 
-#ifdef CONFIG_SMP
-struct lpj_info {
-	unsigned long	ref;
-	unsigned int	freq;
-};
-
-static DEFINE_PER_CPU(struct lpj_info, lpj_ref);
-static struct lpj_info global_lpj_ref;
-#endif
-
 static struct cpufreq_frequency_table *freq_table;
 static atomic_t freq_table_users = ATOMIC_INIT(0);
 static struct clk *mpu_clk;
@@ -118,32 +108,6 @@ static int omap_target(struct cpufreq_policy *policy,
 	ret = clk_set_rate(mpu_clk, freqs.new * 1000);
 	freqs.new = omap_getspeed(policy->cpu);
 
-#ifdef CONFIG_SMP
-	/*
-	 * Note that loops_per_jiffy is not updated on SMP systems in
-	 * cpufreq driver. So, update the per-CPU loops_per_jiffy value
-	 * on frequency transition. We need to update all dependent CPUs.
-	 */
-	for_each_cpu(i, policy->cpus) {
-		struct lpj_info *lpj = &per_cpu(lpj_ref, i);
-		if (!lpj->freq) {
-			lpj->ref = per_cpu(cpu_data, i).loops_per_jiffy;
-			lpj->freq = freqs.old;
-		}
-
-		per_cpu(cpu_data, i).loops_per_jiffy =
-			cpufreq_scale(lpj->ref, lpj->freq, freqs.new);
-	}
-
-	/* And don't forget to adjust the global one */
-	if (!global_lpj_ref.freq) {
-		global_lpj_ref.ref = loops_per_jiffy;
-		global_lpj_ref.freq = freqs.old;
-	}
-	loops_per_jiffy = cpufreq_scale(global_lpj_ref.ref, global_lpj_ref.freq,
-					freqs.new);
-#endif
-
 	/* notifiers */
 	for_each_cpu(i, policy->cpus) {
 		freqs.cpu = i;
-- 
1.7.5.4

[PATCH V3 3/7] cpufreq: OMAP: remove loops_per_jiffy recalculate for smp

[Richard Zhao]

arm registered cpufreq transition notifier to recalculate it.

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 drivers/cpufreq/omap-cpufreq.c |   36 ------------------------------------
 1 files changed, 0 insertions(+), 36 deletions(-)

diff --git a/drivers/cpufreq/omap-cpufreq.c b/drivers/cpufreq/omap-cpufreq.c
index 5d04c57..17da4c4 100644
--- a/drivers/cpufreq/omap-cpufreq.c
+++ b/drivers/cpufreq/omap-cpufreq.c
@@ -37,16 +37,6 @@
 
 #include <mach/hardware.h>
 
-#ifdef CONFIG_SMP
-struct lpj_info {
-	unsigned long	ref;
-	unsigned int	freq;
-};
-
-static DEFINE_PER_CPU(struct lpj_info, lpj_ref);
-static struct lpj_info global_lpj_ref;
-#endif
-
 static struct cpufreq_frequency_table *freq_table;
 static atomic_t freq_table_users = ATOMIC_INIT(0);
 static struct clk *mpu_clk;
@@ -118,32 +108,6 @@ static int omap_target(struct cpufreq_policy *policy,
 	ret = clk_set_rate(mpu_clk, freqs.new * 1000);
 	freqs.new = omap_getspeed(policy->cpu);
 
-#ifdef CONFIG_SMP
-	/*
-	 * Note that loops_per_jiffy is not updated on SMP systems in
-	 * cpufreq driver. So, update the per-CPU loops_per_jiffy value
-	 * on frequency transition. We need to update all dependent CPUs.
-	 */
-	for_each_cpu(i, policy->cpus) {
-		struct lpj_info *lpj = &per_cpu(lpj_ref, i);
-		if (!lpj->freq) {
-			lpj->ref = per_cpu(cpu_data, i).loops_per_jiffy;
-			lpj->freq = freqs.old;
-		}
-
-		per_cpu(cpu_data, i).loops_per_jiffy =
-			cpufreq_scale(lpj->ref, lpj->freq, freqs.new);
-	}
-
-	/* And don't forget to adjust the global one */
-	if (!global_lpj_ref.freq) {
-		global_lpj_ref.ref = loops_per_jiffy;
-		global_lpj_ref.freq = freqs.old;
-	}
-	loops_per_jiffy = cpufreq_scale(global_lpj_ref.ref, global_lpj_ref.freq,
-					freqs.new);
-#endif
-
 	/* notifiers */
 	for_each_cpu(i, policy->cpus) {
 		freqs.cpu = i;
-- 
1.7.5.4

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

It support single core and multi-core ARM SoCs. But currently it assume
all cores share the same frequency and voltage.

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 .../devicetree/bindings/cpufreq/generic-cpufreq    |    7 +
 drivers/cpufreq/Kconfig                            |    8 +
 drivers/cpufreq/Makefile                           |    2 +
 drivers/cpufreq/generic-cpufreq.c                  |  251 ++++++++++++++++++++
 4 files changed, 268 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq
 create mode 100644 drivers/cpufreq/generic-cpufreq.c

diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
new file mode 100644
index 0000000..15dd780
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
@@ -0,0 +1,7 @@
+Generic cpufreq driver
+
+Required properties in /cpus/cpu@0:
+- compatible : "generic-cpufreq"
+- cpu-freqs : cpu frequency points it support
+- cpu-volts : cpu voltages required by the frequency point at the same index
+- trans-latency :  transition_latency
diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index e24a2a1..216eecd 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
 
 	  If in doubt, say N.
 
+config GENERIC_CPUFREQ_DRIVER
+	bool "Generic cpufreq driver using clock/regulator/devicetree"
+	help
+	  This adds generic CPUFreq driver. It assumes all
+	  cores of the CPU share the same clock and voltage.
+
+	  If in doubt, say N.
+
 menu "x86 CPU frequency scaling drivers"
 depends on X86
 source "drivers/cpufreq/Kconfig.x86"
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index ce75fcb..2dbdab1 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -13,6 +13,8 @@ obj-$(CONFIG_CPU_FREQ_GOV_CONSERVATIVE)	+= cpufreq_conservative.o
 # CPUfreq cross-arch helpers
 obj-$(CONFIG_CPU_FREQ_TABLE)		+= freq_table.o
 
+obj-$(CONFIG_GENERIC_CPUFREQ_DRIVER)	+= generic-cpufreq.o
+
 ##################################################################################
 # x86 drivers.
 # Link order matters. K8 is preferred to ACPI because of firmware bugs in early
diff --git a/drivers/cpufreq/generic-cpufreq.c b/drivers/cpufreq/generic-cpufreq.c
new file mode 100644
index 0000000..781bb9b
--- /dev/null
+++ b/drivers/cpufreq/generic-cpufreq.c
@@ -0,0 +1,251 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc.
+ */
+
+/*
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/module.h>
+#include <linux/cpufreq.h>
+#include <linux/clk.h>
+#include <linux/regulator/consumer.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+
+static u32 *cpu_freqs; /* HZ */
+static u32 *cpu_volts; /* uV */
+static u32 trans_latency; /* ns */
+static int cpu_op_nr;
+
+static struct clk *cpu_clk;
+static struct regulator *cpu_reg;
+static struct cpufreq_frequency_table *freq_table;
+
+static int set_cpu_freq(unsigned long freq, int index, int higher)
+{
+	int ret = 0;
+
+	if (higher && cpu_reg)
+		regulator_set_voltage(cpu_reg,
+				cpu_volts[index], cpu_volts[index]);
+
+	ret = clk_set_rate(cpu_clk, freq);
+	if (ret != 0) {
+		pr_err("generic-cpufreq: cannot set CPU clock rate\n");
+		return ret;
+	}
+
+	if (!higher && cpu_reg)
+		regulator_set_voltage(cpu_reg,
+				cpu_volts[index], cpu_volts[index]);
+
+	return ret;
+}
+
+static int generic_verify_speed(struct cpufreq_policy *policy)
+{
+	return cpufreq_frequency_table_verify(policy, freq_table);
+}
+
+static unsigned int generic_get_speed(unsigned int cpu)
+{
+	return clk_get_rate(cpu_clk) / 1000;
+}
+
+static int generic_set_target(struct cpufreq_policy *policy,
+			  unsigned int target_freq, unsigned int relation)
+{
+	struct cpufreq_freqs freqs;
+	unsigned long freq_Hz;
+	int cpu;
+	int ret = 0;
+	unsigned int index;
+
+	cpufreq_frequency_table_target(policy, freq_table,
+			target_freq, relation, &index);
+	freq_Hz = clk_round_rate(cpu_clk, cpu_freqs[index]);
+	freq_Hz = freq_Hz ? freq_Hz : cpu_freqs[index];
+	freqs.old = clk_get_rate(cpu_clk) / 1000;
+	freqs.new = freq_Hz / 1000;
+	freqs.flags = 0;
+
+	if (freqs.old == freqs.new)
+		return 0;
+
+	for_each_possible_cpu(cpu) {
+		freqs.cpu = cpu;
+		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+	}
+
+	ret = set_cpu_freq(freq_Hz, index, (freqs.new > freqs.old));
+
+	for_each_possible_cpu(cpu) {
+		freqs.cpu = cpu;
+		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+	}
+
+	return ret;
+}
+
+static int generic_cpufreq_init(struct cpufreq_policy *policy)
+{
+	int ret;
+
+	if (policy->cpu >= num_possible_cpus())
+		return -EINVAL;
+
+	policy->cur = clk_get_rate(cpu_clk) / 1000;
+	policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
+	cpumask_setall(policy->cpus);
+	/* Manual states, that PLL stabilizes in two CLK32 periods */
+	policy->cpuinfo.transition_latency = trans_latency;
+
+	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
+
+	if (ret < 0) {
+		pr_err("%s: invalid frequency table for cpu %d\n",
+		       __func__, policy->cpu);
+		return ret;
+	}
+
+	cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
+	return 0;
+}
+
+static int generic_cpufreq_exit(struct cpufreq_policy *policy)
+{
+	cpufreq_frequency_table_put_attr(policy->cpu);
+	return 0;
+}
+
+static struct cpufreq_driver generic_cpufreq_driver = {
+	.flags = CPUFREQ_STICKY,
+	.verify = generic_verify_speed,
+	.target = generic_set_target,
+	.get = generic_get_speed,
+	.init = generic_cpufreq_init,
+	.exit = generic_cpufreq_exit,
+	.name = "generic",
+};
+
+static int __devinit generic_cpufreq_driver_init(void)
+{
+	struct device_node *cpu0;
+	const struct property *pp;
+	int i, ret;
+
+	pr_info("Generic CPU frequency driver\n");
+
+	cpu0 = of_find_node_by_path("/cpus/cpu@0");
+	if (!cpu0)
+		return -ENODEV;
+
+	if (!of_device_is_compatible(cpu0, "generic-cpufreq"))
+		return -ENODEV;
+
+	pp = of_find_property(cpu0, "cpu-freqs", NULL);
+	if (!pp) {
+		ret = -ENODEV;
+		goto put_node;
+	}
+	cpu_op_nr = pp->length / sizeof(u32);
+	if (!cpu_op_nr) {
+		ret = -ENODEV;
+		goto put_node;
+	}
+	ret = -ENOMEM;
+	cpu_freqs = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr, GFP_KERNEL);
+	if (!cpu_freqs)
+		goto put_node;
+	of_property_read_u32_array(cpu0, "cpu-freqs", cpu_freqs, cpu_op_nr);
+
+	pp = of_find_property(cpu0, "cpu-volts", NULL);
+	if (pp) {
+		if (cpu_op_nr == pp->length / sizeof(u32)) {
+			cpu_volts = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr,
+						GFP_KERNEL);
+			if (!cpu_volts)
+				goto free_cpu_freqs;
+			of_property_read_u32_array(cpu0, "cpu-volts",
+						cpu_volts, cpu_op_nr);
+		} else
+			pr_warn("%s: invalid cpu_volts!\n", __func__);
+	}
+
+	if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
+		trans_latency = CPUFREQ_ETERNAL;
+
+	freq_table = kmalloc(sizeof(struct cpufreq_frequency_table)
+				* (cpu_op_nr + 1), GFP_KERNEL);
+	if (!freq_table)
+		goto free_cpu_volts;
+
+	for (i = 0; i < cpu_op_nr; i++) {
+		freq_table[i].index = i;
+		freq_table[i].frequency = cpu_freqs[i] / 1000;
+	}
+
+	freq_table[i].index = i;
+	freq_table[i].frequency = CPUFREQ_TABLE_END;
+
+	cpu_clk = clk_get(NULL, "cpu");
+	if (IS_ERR(cpu_clk)) {
+		pr_err("%s: failed to get cpu clock\n", __func__);
+		ret = PTR_ERR(cpu_clk);
+		goto free_freq_table;
+	}
+
+	if (cpu_volts) {
+		cpu_reg = regulator_get(NULL, "cpu");
+		if (IS_ERR(cpu_reg)) {
+			pr_warn("%s: regulator cpu get failed.\n", __func__);
+			cpu_reg = NULL;
+		}
+	}
+
+	ret = cpufreq_register_driver(&generic_cpufreq_driver);
+	if (ret)
+		goto reg_put;
+
+	of_node_put(cpu0);
+
+	return 0;
+
+reg_put:
+	if (cpu_reg)
+		regulator_put(cpu_reg);
+	clk_put(cpu_clk);
+free_freq_table:
+	kfree(freq_table);
+free_cpu_volts:
+	kfree(cpu_volts);
+free_cpu_freqs:
+	kfree(cpu_freqs);
+put_node:
+	of_node_put(cpu0);
+
+	return ret;
+}
+
+static void generic_cpufreq_driver_exit(void)
+{
+	cpufreq_unregister_driver(&generic_cpufreq_driver);
+	kfree(cpu_freqs);
+	kfree(cpu_volts);
+	kfree(freq_table);
+	clk_put(cpu_clk);
+}
+
+module_init(generic_cpufreq_driver_init);
+module_exit(generic_cpufreq_driver_exit);
+
+MODULE_AUTHOR("Freescale Semiconductor Inc. Richard Zhao <richard.zhao@freescale.com>");
+MODULE_DESCRIPTION("Generic CPUFreq driver");
+MODULE_LICENSE("GPL");
-- 
1.7.5.4

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

It support single core and multi-core ARM SoCs. But currently it assume
all cores share the same frequency and voltage.

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 .../devicetree/bindings/cpufreq/generic-cpufreq    |    7 +
 drivers/cpufreq/Kconfig                            |    8 +
 drivers/cpufreq/Makefile                           |    2 +
 drivers/cpufreq/generic-cpufreq.c                  |  251 ++++++++++++++++++++
 4 files changed, 268 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq
 create mode 100644 drivers/cpufreq/generic-cpufreq.c

diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
new file mode 100644
index 0000000..15dd780
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
@@ -0,0 +1,7 @@
+Generic cpufreq driver
+
+Required properties in /cpus/cpu at 0:
+- compatible : "generic-cpufreq"
+- cpu-freqs : cpu frequency points it support
+- cpu-volts : cpu voltages required by the frequency point at the same index
+- trans-latency :  transition_latency
diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index e24a2a1..216eecd 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
 
 	  If in doubt, say N.
 
+config GENERIC_CPUFREQ_DRIVER
+	bool "Generic cpufreq driver using clock/regulator/devicetree"
+	help
+	  This adds generic CPUFreq driver. It assumes all
+	  cores of the CPU share the same clock and voltage.
+
+	  If in doubt, say N.
+
 menu "x86 CPU frequency scaling drivers"
 depends on X86
 source "drivers/cpufreq/Kconfig.x86"
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index ce75fcb..2dbdab1 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -13,6 +13,8 @@ obj-$(CONFIG_CPU_FREQ_GOV_CONSERVATIVE)	+= cpufreq_conservative.o
 # CPUfreq cross-arch helpers
 obj-$(CONFIG_CPU_FREQ_TABLE)		+= freq_table.o
 
+obj-$(CONFIG_GENERIC_CPUFREQ_DRIVER)	+= generic-cpufreq.o
+
 ##################################################################################
 # x86 drivers.
 # Link order matters. K8 is preferred to ACPI because of firmware bugs in early
diff --git a/drivers/cpufreq/generic-cpufreq.c b/drivers/cpufreq/generic-cpufreq.c
new file mode 100644
index 0000000..781bb9b
--- /dev/null
+++ b/drivers/cpufreq/generic-cpufreq.c
@@ -0,0 +1,251 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc.
+ */
+
+/*
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/module.h>
+#include <linux/cpufreq.h>
+#include <linux/clk.h>
+#include <linux/regulator/consumer.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+
+static u32 *cpu_freqs; /* HZ */
+static u32 *cpu_volts; /* uV */
+static u32 trans_latency; /* ns */
+static int cpu_op_nr;
+
+static struct clk *cpu_clk;
+static struct regulator *cpu_reg;
+static struct cpufreq_frequency_table *freq_table;
+
+static int set_cpu_freq(unsigned long freq, int index, int higher)
+{
+	int ret = 0;
+
+	if (higher && cpu_reg)
+		regulator_set_voltage(cpu_reg,
+				cpu_volts[index], cpu_volts[index]);
+
+	ret = clk_set_rate(cpu_clk, freq);
+	if (ret != 0) {
+		pr_err("generic-cpufreq: cannot set CPU clock rate\n");
+		return ret;
+	}
+
+	if (!higher && cpu_reg)
+		regulator_set_voltage(cpu_reg,
+				cpu_volts[index], cpu_volts[index]);
+
+	return ret;
+}
+
+static int generic_verify_speed(struct cpufreq_policy *policy)
+{
+	return cpufreq_frequency_table_verify(policy, freq_table);
+}
+
+static unsigned int generic_get_speed(unsigned int cpu)
+{
+	return clk_get_rate(cpu_clk) / 1000;
+}
+
+static int generic_set_target(struct cpufreq_policy *policy,
+			  unsigned int target_freq, unsigned int relation)
+{
+	struct cpufreq_freqs freqs;
+	unsigned long freq_Hz;
+	int cpu;
+	int ret = 0;
+	unsigned int index;
+
+	cpufreq_frequency_table_target(policy, freq_table,
+			target_freq, relation, &index);
+	freq_Hz = clk_round_rate(cpu_clk, cpu_freqs[index]);
+	freq_Hz = freq_Hz ? freq_Hz : cpu_freqs[index];
+	freqs.old = clk_get_rate(cpu_clk) / 1000;
+	freqs.new = freq_Hz / 1000;
+	freqs.flags = 0;
+
+	if (freqs.old == freqs.new)
+		return 0;
+
+	for_each_possible_cpu(cpu) {
+		freqs.cpu = cpu;
+		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+	}
+
+	ret = set_cpu_freq(freq_Hz, index, (freqs.new > freqs.old));
+
+	for_each_possible_cpu(cpu) {
+		freqs.cpu = cpu;
+		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+	}
+
+	return ret;
+}
+
+static int generic_cpufreq_init(struct cpufreq_policy *policy)
+{
+	int ret;
+
+	if (policy->cpu >= num_possible_cpus())
+		return -EINVAL;
+
+	policy->cur = clk_get_rate(cpu_clk) / 1000;
+	policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
+	cpumask_setall(policy->cpus);
+	/* Manual states, that PLL stabilizes in two CLK32 periods */
+	policy->cpuinfo.transition_latency = trans_latency;
+
+	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
+
+	if (ret < 0) {
+		pr_err("%s: invalid frequency table for cpu %d\n",
+		       __func__, policy->cpu);
+		return ret;
+	}
+
+	cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
+	return 0;
+}
+
+static int generic_cpufreq_exit(struct cpufreq_policy *policy)
+{
+	cpufreq_frequency_table_put_attr(policy->cpu);
+	return 0;
+}
+
+static struct cpufreq_driver generic_cpufreq_driver = {
+	.flags = CPUFREQ_STICKY,
+	.verify = generic_verify_speed,
+	.target = generic_set_target,
+	.get = generic_get_speed,
+	.init = generic_cpufreq_init,
+	.exit = generic_cpufreq_exit,
+	.name = "generic",
+};
+
+static int __devinit generic_cpufreq_driver_init(void)
+{
+	struct device_node *cpu0;
+	const struct property *pp;
+	int i, ret;
+
+	pr_info("Generic CPU frequency driver\n");
+
+	cpu0 = of_find_node_by_path("/cpus/cpu at 0");
+	if (!cpu0)
+		return -ENODEV;
+
+	if (!of_device_is_compatible(cpu0, "generic-cpufreq"))
+		return -ENODEV;
+
+	pp = of_find_property(cpu0, "cpu-freqs", NULL);
+	if (!pp) {
+		ret = -ENODEV;
+		goto put_node;
+	}
+	cpu_op_nr = pp->length / sizeof(u32);
+	if (!cpu_op_nr) {
+		ret = -ENODEV;
+		goto put_node;
+	}
+	ret = -ENOMEM;
+	cpu_freqs = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr, GFP_KERNEL);
+	if (!cpu_freqs)
+		goto put_node;
+	of_property_read_u32_array(cpu0, "cpu-freqs", cpu_freqs, cpu_op_nr);
+
+	pp = of_find_property(cpu0, "cpu-volts", NULL);
+	if (pp) {
+		if (cpu_op_nr == pp->length / sizeof(u32)) {
+			cpu_volts = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr,
+						GFP_KERNEL);
+			if (!cpu_volts)
+				goto free_cpu_freqs;
+			of_property_read_u32_array(cpu0, "cpu-volts",
+						cpu_volts, cpu_op_nr);
+		} else
+			pr_warn("%s: invalid cpu_volts!\n", __func__);
+	}
+
+	if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
+		trans_latency = CPUFREQ_ETERNAL;
+
+	freq_table = kmalloc(sizeof(struct cpufreq_frequency_table)
+				* (cpu_op_nr + 1), GFP_KERNEL);
+	if (!freq_table)
+		goto free_cpu_volts;
+
+	for (i = 0; i < cpu_op_nr; i++) {
+		freq_table[i].index = i;
+		freq_table[i].frequency = cpu_freqs[i] / 1000;
+	}
+
+	freq_table[i].index = i;
+	freq_table[i].frequency = CPUFREQ_TABLE_END;
+
+	cpu_clk = clk_get(NULL, "cpu");
+	if (IS_ERR(cpu_clk)) {
+		pr_err("%s: failed to get cpu clock\n", __func__);
+		ret = PTR_ERR(cpu_clk);
+		goto free_freq_table;
+	}
+
+	if (cpu_volts) {
+		cpu_reg = regulator_get(NULL, "cpu");
+		if (IS_ERR(cpu_reg)) {
+			pr_warn("%s: regulator cpu get failed.\n", __func__);
+			cpu_reg = NULL;
+		}
+	}
+
+	ret = cpufreq_register_driver(&generic_cpufreq_driver);
+	if (ret)
+		goto reg_put;
+
+	of_node_put(cpu0);
+
+	return 0;
+
+reg_put:
+	if (cpu_reg)
+		regulator_put(cpu_reg);
+	clk_put(cpu_clk);
+free_freq_table:
+	kfree(freq_table);
+free_cpu_volts:
+	kfree(cpu_volts);
+free_cpu_freqs:
+	kfree(cpu_freqs);
+put_node:
+	of_node_put(cpu0);
+
+	return ret;
+}
+
+static void generic_cpufreq_driver_exit(void)
+{
+	cpufreq_unregister_driver(&generic_cpufreq_driver);
+	kfree(cpu_freqs);
+	kfree(cpu_volts);
+	kfree(freq_table);
+	clk_put(cpu_clk);
+}
+
+module_init(generic_cpufreq_driver_init);
+module_exit(generic_cpufreq_driver_exit);
+
+MODULE_AUTHOR("Freescale Semiconductor Inc. Richard Zhao <richard.zhao@freescale.com>");
+MODULE_DESCRIPTION("Generic CPUFreq driver");
+MODULE_LICENSE("GPL");
-- 
1.7.5.4

[PATCH V3 5/7] dts/imx6q: add cpufreq property

[Richard Zhao]

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

diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 263e8f3..80e47b5 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -26,9 +26,12 @@
 		#size-cells = <0>;
 
 		cpu@0 {
-			compatible = "arm,cortex-a9";
+			compatible = "arm,cortex-a9", "generic-cpufreq";
 			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 V3 5/7] dts/imx6q: add cpufreq property

[Richard Zhao]

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

diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 263e8f3..80e47b5 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -26,9 +26,12 @@
 		#size-cells = <0>;
 
 		cpu at 0 {
-			compatible = "arm,cortex-a9";
+			compatible = "arm,cortex-a9", "generic-cpufreq";
 			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 at 1 {
-- 
1.7.5.4

[PATCH V3 6/7] 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

[PATCH V3 6/7] 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

[PATCH V3 7/7] 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

[PATCH V3 7/7] 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

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Jamie Iles]

Hi Richard,

On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> It support single core and multi-core ARM SoCs. But currently it assume
> all cores share the same frequency and voltage.
> 
> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> ---
>  .../devicetree/bindings/cpufreq/generic-cpufreq    |    7 +
>  drivers/cpufreq/Kconfig                            |    8 +
>  drivers/cpufreq/Makefile                           |    2 +
>  drivers/cpufreq/generic-cpufreq.c                  |  251 ++++++++++++++++++++
>  4 files changed, 268 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq
>  create mode 100644 drivers/cpufreq/generic-cpufreq.c
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> new file mode 100644
> index 0000000..15dd780
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> @@ -0,0 +1,7 @@
> +Generic cpufreq driver
> +
> +Required properties in /cpus/cpu@0:
> +- compatible : "generic-cpufreq"

I'm not convinced this is the best way to do this.  By requiring a 
generic-cpufreq compatible string we're encoding Linux driver 
information into the hardware description.  The only way I can see to 
avoid this is to provide a generic_clk_cpufreq_init() function that 
platforms can call in their machine init code to use the driver.

> +- cpu-freqs : cpu frequency points it support
> +- cpu-volts : cpu voltages required by the frequency point at the same index
> +- trans-latency :  transition_latency
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index e24a2a1..216eecd 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
>  
>  	  If in doubt, say N.
>  
> +config GENERIC_CPUFREQ_DRIVER
> +	bool "Generic cpufreq driver using clock/regulator/devicetree"
> +	help
> +	  This adds generic CPUFreq driver. It assumes all
> +	  cores of the CPU share the same clock and voltage.
> +
> +	  If in doubt, say N.

I think this needs dependencies on HAVE_CLK, OF and REGULATOR.

> +
>  menu "x86 CPU frequency scaling drivers"
>  depends on X86
>  source "drivers/cpufreq/Kconfig.x86"
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index ce75fcb..2dbdab1 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -13,6 +13,8 @@ obj-$(CONFIG_CPU_FREQ_GOV_CONSERVATIVE)	+= cpufreq_conservative.o
>  # CPUfreq cross-arch helpers
>  obj-$(CONFIG_CPU_FREQ_TABLE)		+= freq_table.o
>  
> +obj-$(CONFIG_GENERIC_CPUFREQ_DRIVER)	+= generic-cpufreq.o
> +
>  ##################################################################################
>  # x86 drivers.
>  # Link order matters. K8 is preferred to ACPI because of firmware bugs in early
> diff --git a/drivers/cpufreq/generic-cpufreq.c b/drivers/cpufreq/generic-cpufreq.c
> new file mode 100644
> index 0000000..781bb9b
> --- /dev/null
> +++ b/drivers/cpufreq/generic-cpufreq.c
> @@ -0,0 +1,251 @@
> +/*
> + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> + */
> +
> +/*
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/module.h>
> +#include <linux/cpufreq.h>
> +#include <linux/clk.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +
> +static u32 *cpu_freqs; /* HZ */
> +static u32 *cpu_volts; /* uV */
> +static u32 trans_latency; /* ns */
> +static int cpu_op_nr;
> +
> +static struct clk *cpu_clk;
> +static struct regulator *cpu_reg;
> +static struct cpufreq_frequency_table *freq_table;
> +
> +static int set_cpu_freq(unsigned long freq, int index, int higher)
> +{
> +	int ret = 0;
> +
> +	if (higher && cpu_reg)
> +		regulator_set_voltage(cpu_reg,
> +				cpu_volts[index], cpu_volts[index]);
> +
> +	ret = clk_set_rate(cpu_clk, freq);
> +	if (ret != 0) {
> +		pr_err("generic-cpufreq: cannot set CPU clock rate\n");
> +		return ret;
> +	}
> +
> +	if (!higher && cpu_reg)
> +		regulator_set_voltage(cpu_reg,
> +				cpu_volts[index], cpu_volts[index]);
> +
> +	return ret;
> +}
> +
> +static int generic_verify_speed(struct cpufreq_policy *policy)
> +{
> +	return cpufreq_frequency_table_verify(policy, freq_table);
> +}
> +
> +static unsigned int generic_get_speed(unsigned int cpu)
> +{
> +	return clk_get_rate(cpu_clk) / 1000;
> +}
> +
> +static int generic_set_target(struct cpufreq_policy *policy,
> +			  unsigned int target_freq, unsigned int relation)
> +{
> +	struct cpufreq_freqs freqs;
> +	unsigned long freq_Hz;
> +	int cpu;
> +	int ret = 0;
> +	unsigned int index;
> +
> +	cpufreq_frequency_table_target(policy, freq_table,
> +			target_freq, relation, &index);
> +	freq_Hz = clk_round_rate(cpu_clk, cpu_freqs[index]);
> +	freq_Hz = freq_Hz ? freq_Hz : cpu_freqs[index];
> +	freqs.old = clk_get_rate(cpu_clk) / 1000;
> +	freqs.new = freq_Hz / 1000;
> +	freqs.flags = 0;
> +
> +	if (freqs.old == freqs.new)
> +		return 0;
> +
> +	for_each_possible_cpu(cpu) {
> +		freqs.cpu = cpu;
> +		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> +	}
> +
> +	ret = set_cpu_freq(freq_Hz, index, (freqs.new > freqs.old));

If this fails then we'll still be notifying the transition at the 
requested rate even though it didn't work.  I guess we should really get 
the rate of the clk and put that into freqs for the POSTCHANGE 
notification.

> +
> +	for_each_possible_cpu(cpu) {
> +		freqs.cpu = cpu;
> +		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> +	}
> +
> +	return ret;
> +}
> +
> +static int generic_cpufreq_init(struct cpufreq_policy *policy)
> +{
> +	int ret;
> +
> +	if (policy->cpu >= num_possible_cpus())
> +		return -EINVAL;
> +
> +	policy->cur = clk_get_rate(cpu_clk) / 1000;
> +	policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> +	cpumask_setall(policy->cpus);
> +	/* Manual states, that PLL stabilizes in two CLK32 periods */
> +	policy->cpuinfo.transition_latency = trans_latency;
> +
> +	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> +
> +	if (ret < 0) {
> +		pr_err("%s: invalid frequency table for cpu %d\n",
> +		       __func__, policy->cpu);
> +		return ret;
> +	}
> +
> +	cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
> +	return 0;
> +}
> +
> +static int generic_cpufreq_exit(struct cpufreq_policy *policy)
> +{
> +	cpufreq_frequency_table_put_attr(policy->cpu);
> +	return 0;
> +}
> +
> +static struct cpufreq_driver generic_cpufreq_driver = {
> +	.flags = CPUFREQ_STICKY,
> +	.verify = generic_verify_speed,
> +	.target = generic_set_target,
> +	.get = generic_get_speed,
> +	.init = generic_cpufreq_init,
> +	.exit = generic_cpufreq_exit,
> +	.name = "generic",

This may be a little too generic?  "generic-reg-clk"?

> +};
> +
> +static int __devinit generic_cpufreq_driver_init(void)
> +{
> +	struct device_node *cpu0;
> +	const struct property *pp;
> +	int i, ret;
> +
> +	pr_info("Generic CPU frequency driver\n");
> +
> +	cpu0 = of_find_node_by_path("/cpus/cpu@0");
> +	if (!cpu0)
> +		return -ENODEV;
> +
> +	if (!of_device_is_compatible(cpu0, "generic-cpufreq"))
> +		return -ENODEV;

As above, I'd personally rather not use compatible strings, but if you 
do, then I think return 0 here rather than -ENODEV else I believe you'll 
get a potentially confusing message on the console for platforms that 
don't use this.

> +
> +	pp = of_find_property(cpu0, "cpu-freqs", NULL);
> +	if (!pp) {
> +		ret = -ENODEV;
> +		goto put_node;
> +	}
> +	cpu_op_nr = pp->length / sizeof(u32);
> +	if (!cpu_op_nr) {
> +		ret = -ENODEV;
> +		goto put_node;
> +	}
> +	ret = -ENOMEM;
> +	cpu_freqs = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr, GFP_KERNEL);
> +	if (!cpu_freqs)
> +		goto put_node;
> +	of_property_read_u32_array(cpu0, "cpu-freqs", cpu_freqs, cpu_op_nr);
> +
> +	pp = of_find_property(cpu0, "cpu-volts", NULL);
> +	if (pp) {
> +		if (cpu_op_nr == pp->length / sizeof(u32)) {
> +			cpu_volts = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr,
> +						GFP_KERNEL);
> +			if (!cpu_volts)
> +				goto free_cpu_freqs;
> +			of_property_read_u32_array(cpu0, "cpu-volts",
> +						cpu_volts, cpu_op_nr);
> +		} else
> +			pr_warn("%s: invalid cpu_volts!\n", __func__);
> +	}
> +
> +	if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
> +		trans_latency = CPUFREQ_ETERNAL;
> +
> +	freq_table = kmalloc(sizeof(struct cpufreq_frequency_table)
> +				* (cpu_op_nr + 1), GFP_KERNEL);
> +	if (!freq_table)
> +		goto free_cpu_volts;
> +
> +	for (i = 0; i < cpu_op_nr; i++) {
> +		freq_table[i].index = i;
> +		freq_table[i].frequency = cpu_freqs[i] / 1000;
> +	}
> +
> +	freq_table[i].index = i;
> +	freq_table[i].frequency = CPUFREQ_TABLE_END;
> +
> +	cpu_clk = clk_get(NULL, "cpu");
> +	if (IS_ERR(cpu_clk)) {
> +		pr_err("%s: failed to get cpu clock\n", __func__);
> +		ret = PTR_ERR(cpu_clk);
> +		goto free_freq_table;
> +	}
> +
> +	if (cpu_volts) {
> +		cpu_reg = regulator_get(NULL, "cpu");
> +		if (IS_ERR(cpu_reg)) {
> +			pr_warn("%s: regulator cpu get failed.\n", __func__);
> +			cpu_reg = NULL;
> +		}
> +	}
> +
> +	ret = cpufreq_register_driver(&generic_cpufreq_driver);
> +	if (ret)
> +		goto reg_put;
> +
> +	of_node_put(cpu0);
> +
> +	return 0;
> +
> +reg_put:
> +	if (cpu_reg)
> +		regulator_put(cpu_reg);
> +	clk_put(cpu_clk);
> +free_freq_table:
> +	kfree(freq_table);
> +free_cpu_volts:
> +	kfree(cpu_volts);
> +free_cpu_freqs:
> +	kfree(cpu_freqs);
> +put_node:
> +	of_node_put(cpu0);
> +
> +	return ret;
> +}
> +
> +static void generic_cpufreq_driver_exit(void)
> +{
> +	cpufreq_unregister_driver(&generic_cpufreq_driver);
> +	kfree(cpu_freqs);
> +	kfree(cpu_volts);
> +	kfree(freq_table);
> +	clk_put(cpu_clk);

Should this do something with the regulator too?

Jamie

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Jamie Iles]

Hi Richard,

On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> It support single core and multi-core ARM SoCs. But currently it assume
> all cores share the same frequency and voltage.
> 
> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> ---
>  .../devicetree/bindings/cpufreq/generic-cpufreq    |    7 +
>  drivers/cpufreq/Kconfig                            |    8 +
>  drivers/cpufreq/Makefile                           |    2 +
>  drivers/cpufreq/generic-cpufreq.c                  |  251 ++++++++++++++++++++
>  4 files changed, 268 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq
>  create mode 100644 drivers/cpufreq/generic-cpufreq.c
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> new file mode 100644
> index 0000000..15dd780
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> @@ -0,0 +1,7 @@
> +Generic cpufreq driver
> +
> +Required properties in /cpus/cpu at 0:
> +- compatible : "generic-cpufreq"

I'm not convinced this is the best way to do this.  By requiring a 
generic-cpufreq compatible string we're encoding Linux driver 
information into the hardware description.  The only way I can see to 
avoid this is to provide a generic_clk_cpufreq_init() function that 
platforms can call in their machine init code to use the driver.

> +- cpu-freqs : cpu frequency points it support
> +- cpu-volts : cpu voltages required by the frequency point at the same index
> +- trans-latency :  transition_latency
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index e24a2a1..216eecd 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
>  
>  	  If in doubt, say N.
>  
> +config GENERIC_CPUFREQ_DRIVER
> +	bool "Generic cpufreq driver using clock/regulator/devicetree"
> +	help
> +	  This adds generic CPUFreq driver. It assumes all
> +	  cores of the CPU share the same clock and voltage.
> +
> +	  If in doubt, say N.

I think this needs dependencies on HAVE_CLK, OF and REGULATOR.

> +
>  menu "x86 CPU frequency scaling drivers"
>  depends on X86
>  source "drivers/cpufreq/Kconfig.x86"
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index ce75fcb..2dbdab1 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -13,6 +13,8 @@ obj-$(CONFIG_CPU_FREQ_GOV_CONSERVATIVE)	+= cpufreq_conservative.o
>  # CPUfreq cross-arch helpers
>  obj-$(CONFIG_CPU_FREQ_TABLE)		+= freq_table.o
>  
> +obj-$(CONFIG_GENERIC_CPUFREQ_DRIVER)	+= generic-cpufreq.o
> +
>  ##################################################################################
>  # x86 drivers.
>  # Link order matters. K8 is preferred to ACPI because of firmware bugs in early
> diff --git a/drivers/cpufreq/generic-cpufreq.c b/drivers/cpufreq/generic-cpufreq.c
> new file mode 100644
> index 0000000..781bb9b
> --- /dev/null
> +++ b/drivers/cpufreq/generic-cpufreq.c
> @@ -0,0 +1,251 @@
> +/*
> + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> + */
> +
> +/*
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/module.h>
> +#include <linux/cpufreq.h>
> +#include <linux/clk.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +
> +static u32 *cpu_freqs; /* HZ */
> +static u32 *cpu_volts; /* uV */
> +static u32 trans_latency; /* ns */
> +static int cpu_op_nr;
> +
> +static struct clk *cpu_clk;
> +static struct regulator *cpu_reg;
> +static struct cpufreq_frequency_table *freq_table;
> +
> +static int set_cpu_freq(unsigned long freq, int index, int higher)
> +{
> +	int ret = 0;
> +
> +	if (higher && cpu_reg)
> +		regulator_set_voltage(cpu_reg,
> +				cpu_volts[index], cpu_volts[index]);
> +
> +	ret = clk_set_rate(cpu_clk, freq);
> +	if (ret != 0) {
> +		pr_err("generic-cpufreq: cannot set CPU clock rate\n");
> +		return ret;
> +	}
> +
> +	if (!higher && cpu_reg)
> +		regulator_set_voltage(cpu_reg,
> +				cpu_volts[index], cpu_volts[index]);
> +
> +	return ret;
> +}
> +
> +static int generic_verify_speed(struct cpufreq_policy *policy)
> +{
> +	return cpufreq_frequency_table_verify(policy, freq_table);
> +}
> +
> +static unsigned int generic_get_speed(unsigned int cpu)
> +{
> +	return clk_get_rate(cpu_clk) / 1000;
> +}
> +
> +static int generic_set_target(struct cpufreq_policy *policy,
> +			  unsigned int target_freq, unsigned int relation)
> +{
> +	struct cpufreq_freqs freqs;
> +	unsigned long freq_Hz;
> +	int cpu;
> +	int ret = 0;
> +	unsigned int index;
> +
> +	cpufreq_frequency_table_target(policy, freq_table,
> +			target_freq, relation, &index);
> +	freq_Hz = clk_round_rate(cpu_clk, cpu_freqs[index]);
> +	freq_Hz = freq_Hz ? freq_Hz : cpu_freqs[index];
> +	freqs.old = clk_get_rate(cpu_clk) / 1000;
> +	freqs.new = freq_Hz / 1000;
> +	freqs.flags = 0;
> +
> +	if (freqs.old == freqs.new)
> +		return 0;
> +
> +	for_each_possible_cpu(cpu) {
> +		freqs.cpu = cpu;
> +		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> +	}
> +
> +	ret = set_cpu_freq(freq_Hz, index, (freqs.new > freqs.old));

If this fails then we'll still be notifying the transition at the 
requested rate even though it didn't work.  I guess we should really get 
the rate of the clk and put that into freqs for the POSTCHANGE 
notification.

> +
> +	for_each_possible_cpu(cpu) {
> +		freqs.cpu = cpu;
> +		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> +	}
> +
> +	return ret;
> +}
> +
> +static int generic_cpufreq_init(struct cpufreq_policy *policy)
> +{
> +	int ret;
> +
> +	if (policy->cpu >= num_possible_cpus())
> +		return -EINVAL;
> +
> +	policy->cur = clk_get_rate(cpu_clk) / 1000;
> +	policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> +	cpumask_setall(policy->cpus);
> +	/* Manual states, that PLL stabilizes in two CLK32 periods */
> +	policy->cpuinfo.transition_latency = trans_latency;
> +
> +	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> +
> +	if (ret < 0) {
> +		pr_err("%s: invalid frequency table for cpu %d\n",
> +		       __func__, policy->cpu);
> +		return ret;
> +	}
> +
> +	cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
> +	return 0;
> +}
> +
> +static int generic_cpufreq_exit(struct cpufreq_policy *policy)
> +{
> +	cpufreq_frequency_table_put_attr(policy->cpu);
> +	return 0;
> +}
> +
> +static struct cpufreq_driver generic_cpufreq_driver = {
> +	.flags = CPUFREQ_STICKY,
> +	.verify = generic_verify_speed,
> +	.target = generic_set_target,
> +	.get = generic_get_speed,
> +	.init = generic_cpufreq_init,
> +	.exit = generic_cpufreq_exit,
> +	.name = "generic",

This may be a little too generic?  "generic-reg-clk"?

> +};
> +
> +static int __devinit generic_cpufreq_driver_init(void)
> +{
> +	struct device_node *cpu0;
> +	const struct property *pp;
> +	int i, ret;
> +
> +	pr_info("Generic CPU frequency driver\n");
> +
> +	cpu0 = of_find_node_by_path("/cpus/cpu at 0");
> +	if (!cpu0)
> +		return -ENODEV;
> +
> +	if (!of_device_is_compatible(cpu0, "generic-cpufreq"))
> +		return -ENODEV;

As above, I'd personally rather not use compatible strings, but if you 
do, then I think return 0 here rather than -ENODEV else I believe you'll 
get a potentially confusing message on the console for platforms that 
don't use this.

> +
> +	pp = of_find_property(cpu0, "cpu-freqs", NULL);
> +	if (!pp) {
> +		ret = -ENODEV;
> +		goto put_node;
> +	}
> +	cpu_op_nr = pp->length / sizeof(u32);
> +	if (!cpu_op_nr) {
> +		ret = -ENODEV;
> +		goto put_node;
> +	}
> +	ret = -ENOMEM;
> +	cpu_freqs = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr, GFP_KERNEL);
> +	if (!cpu_freqs)
> +		goto put_node;
> +	of_property_read_u32_array(cpu0, "cpu-freqs", cpu_freqs, cpu_op_nr);
> +
> +	pp = of_find_property(cpu0, "cpu-volts", NULL);
> +	if (pp) {
> +		if (cpu_op_nr == pp->length / sizeof(u32)) {
> +			cpu_volts = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr,
> +						GFP_KERNEL);
> +			if (!cpu_volts)
> +				goto free_cpu_freqs;
> +			of_property_read_u32_array(cpu0, "cpu-volts",
> +						cpu_volts, cpu_op_nr);
> +		} else
> +			pr_warn("%s: invalid cpu_volts!\n", __func__);
> +	}
> +
> +	if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
> +		trans_latency = CPUFREQ_ETERNAL;
> +
> +	freq_table = kmalloc(sizeof(struct cpufreq_frequency_table)
> +				* (cpu_op_nr + 1), GFP_KERNEL);
> +	if (!freq_table)
> +		goto free_cpu_volts;
> +
> +	for (i = 0; i < cpu_op_nr; i++) {
> +		freq_table[i].index = i;
> +		freq_table[i].frequency = cpu_freqs[i] / 1000;
> +	}
> +
> +	freq_table[i].index = i;
> +	freq_table[i].frequency = CPUFREQ_TABLE_END;
> +
> +	cpu_clk = clk_get(NULL, "cpu");
> +	if (IS_ERR(cpu_clk)) {
> +		pr_err("%s: failed to get cpu clock\n", __func__);
> +		ret = PTR_ERR(cpu_clk);
> +		goto free_freq_table;
> +	}
> +
> +	if (cpu_volts) {
> +		cpu_reg = regulator_get(NULL, "cpu");
> +		if (IS_ERR(cpu_reg)) {
> +			pr_warn("%s: regulator cpu get failed.\n", __func__);
> +			cpu_reg = NULL;
> +		}
> +	}
> +
> +	ret = cpufreq_register_driver(&generic_cpufreq_driver);
> +	if (ret)
> +		goto reg_put;
> +
> +	of_node_put(cpu0);
> +
> +	return 0;
> +
> +reg_put:
> +	if (cpu_reg)
> +		regulator_put(cpu_reg);
> +	clk_put(cpu_clk);
> +free_freq_table:
> +	kfree(freq_table);
> +free_cpu_volts:
> +	kfree(cpu_volts);
> +free_cpu_freqs:
> +	kfree(cpu_freqs);
> +put_node:
> +	of_node_put(cpu0);
> +
> +	return ret;
> +}
> +
> +static void generic_cpufreq_driver_exit(void)
> +{
> +	cpufreq_unregister_driver(&generic_cpufreq_driver);
> +	kfree(cpu_freqs);
> +	kfree(cpu_volts);
> +	kfree(freq_table);
> +	clk_put(cpu_clk);

Should this do something with the regulator too?

Jamie

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
> Hi Richard,
> 
> On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> > It support single core and multi-core ARM SoCs. But currently it assume
> > all cores share the same frequency and voltage.
> > 
> > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > ---
> >  .../devicetree/bindings/cpufreq/generic-cpufreq    |    7 +
> >  drivers/cpufreq/Kconfig                            |    8 +
> >  drivers/cpufreq/Makefile                           |    2 +
> >  drivers/cpufreq/generic-cpufreq.c                  |  251 ++++++++++++++++++++
> >  4 files changed, 268 insertions(+), 0 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> >  create mode 100644 drivers/cpufreq/generic-cpufreq.c
> > 
> > diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> > new file mode 100644
> > index 0000000..15dd780
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> > @@ -0,0 +1,7 @@
> > +Generic cpufreq driver
> > +
> > +Required properties in /cpus/cpu@0:
> > +- compatible : "generic-cpufreq"
> 
> I'm not convinced this is the best way to do this.  By requiring a 
> generic-cpufreq compatible string we're encoding Linux driver 
> information into the hardware description.  The only way I can see to 
> avoid this is to provide a generic_clk_cpufreq_init() function that 
> platforms can call in their machine init code to use the driver.
It'll prevent the driver from being a kernel module.

Hi Grant & Rob,

Could you comment?

> 
> > +- cpu-freqs : cpu frequency points it support
> > +- cpu-volts : cpu voltages required by the frequency point at the same index
> > +- trans-latency :  transition_latency
> > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> > index e24a2a1..216eecd 100644
> > --- a/drivers/cpufreq/Kconfig
> > +++ b/drivers/cpufreq/Kconfig
> > @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
> >  
> >  	  If in doubt, say N.
> >  
> > +config GENERIC_CPUFREQ_DRIVER
> > +	bool "Generic cpufreq driver using clock/regulator/devicetree"
> > +	help
> > +	  This adds generic CPUFreq driver. It assumes all
> > +	  cores of the CPU share the same clock and voltage.
> > +
> > +	  If in doubt, say N.
> 
> I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
right, Thanks. I can not check clk before generic clock framework
come in.
Added:
	depends on OF && REGULATOR
	select CPU_FREQ_TABLE
> 
> > +
> >  menu "x86 CPU frequency scaling drivers"
> >  depends on X86
> >  source "drivers/cpufreq/Kconfig.x86"
> > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > index ce75fcb..2dbdab1 100644
> > --- a/drivers/cpufreq/Makefile
> > +++ b/drivers/cpufreq/Makefile
> > @@ -13,6 +13,8 @@ obj-$(CONFIG_CPU_FREQ_GOV_CONSERVATIVE)	+= cpufreq_conservative.o
> >  # CPUfreq cross-arch helpers
> >  obj-$(CONFIG_CPU_FREQ_TABLE)		+= freq_table.o
> >  
> > +obj-$(CONFIG_GENERIC_CPUFREQ_DRIVER)	+= generic-cpufreq.o
> > +
> >  ##################################################################################
> >  # x86 drivers.
> >  # Link order matters. K8 is preferred to ACPI because of firmware bugs in early
> > diff --git a/drivers/cpufreq/generic-cpufreq.c b/drivers/cpufreq/generic-cpufreq.c
> > new file mode 100644
> > index 0000000..781bb9b
> > --- /dev/null
> > +++ b/drivers/cpufreq/generic-cpufreq.c
> > @@ -0,0 +1,251 @@
> > +/*
> > + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> > + */
> > +
> > +/*
> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/cpufreq.h>
> > +#include <linux/clk.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/err.h>
> > +#include <linux/slab.h>
> > +#include <linux/of.h>
> > +
> > +static u32 *cpu_freqs; /* HZ */
> > +static u32 *cpu_volts; /* uV */
> > +static u32 trans_latency; /* ns */
> > +static int cpu_op_nr;
> > +
> > +static struct clk *cpu_clk;
> > +static struct regulator *cpu_reg;
> > +static struct cpufreq_frequency_table *freq_table;
> > +
> > +static int set_cpu_freq(unsigned long freq, int index, int higher)
> > +{
> > +	int ret = 0;
> > +
> > +	if (higher && cpu_reg)
> > +		regulator_set_voltage(cpu_reg,
> > +				cpu_volts[index], cpu_volts[index]);
> > +
> > +	ret = clk_set_rate(cpu_clk, freq);
> > +	if (ret != 0) {
> > +		pr_err("generic-cpufreq: cannot set CPU clock rate\n");
> > +		return ret;
> > +	}
> > +
> > +	if (!higher && cpu_reg)
> > +		regulator_set_voltage(cpu_reg,
> > +				cpu_volts[index], cpu_volts[index]);
> > +
> > +	return ret;
> > +}
> > +
> > +static int generic_verify_speed(struct cpufreq_policy *policy)
> > +{
> > +	return cpufreq_frequency_table_verify(policy, freq_table);
> > +}
> > +
> > +static unsigned int generic_get_speed(unsigned int cpu)
> > +{
> > +	return clk_get_rate(cpu_clk) / 1000;
> > +}
> > +
> > +static int generic_set_target(struct cpufreq_policy *policy,
> > +			  unsigned int target_freq, unsigned int relation)
> > +{
> > +	struct cpufreq_freqs freqs;
> > +	unsigned long freq_Hz;
> > +	int cpu;
> > +	int ret = 0;
> > +	unsigned int index;
> > +
> > +	cpufreq_frequency_table_target(policy, freq_table,
> > +			target_freq, relation, &index);
> > +	freq_Hz = clk_round_rate(cpu_clk, cpu_freqs[index]);
> > +	freq_Hz = freq_Hz ? freq_Hz : cpu_freqs[index];
> > +	freqs.old = clk_get_rate(cpu_clk) / 1000;
> > +	freqs.new = freq_Hz / 1000;
> > +	freqs.flags = 0;
> > +
> > +	if (freqs.old == freqs.new)
> > +		return 0;
> > +
> > +	for_each_possible_cpu(cpu) {
> > +		freqs.cpu = cpu;
> > +		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > +	}
> > +
> > +	ret = set_cpu_freq(freq_Hz, index, (freqs.new > freqs.old));
> 
> If this fails then we'll still be notifying the transition at the 
> requested rate even though it didn't work.  I guess we should really get 
> the rate of the clk and put that into freqs for the POSTCHANGE 
> notification.
right, Thanks.
Added:
	if (ret)
		freq.new = clk_get_rate(cpu_clk) / 1000;

> 
> > +
> > +	for_each_possible_cpu(cpu) {
> > +		freqs.cpu = cpu;
> > +		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int generic_cpufreq_init(struct cpufreq_policy *policy)
> > +{
> > +	int ret;
> > +
> > +	if (policy->cpu >= num_possible_cpus())
> > +		return -EINVAL;
> > +
> > +	policy->cur = clk_get_rate(cpu_clk) / 1000;
> > +	policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> > +	cpumask_setall(policy->cpus);
> > +	/* Manual states, that PLL stabilizes in two CLK32 periods */
> > +	policy->cpuinfo.transition_latency = trans_latency;
> > +
> > +	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> > +
> > +	if (ret < 0) {
> > +		pr_err("%s: invalid frequency table for cpu %d\n",
> > +		       __func__, policy->cpu);
> > +		return ret;
> > +	}
> > +
> > +	cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
> > +	return 0;
> > +}
> > +
> > +static int generic_cpufreq_exit(struct cpufreq_policy *policy)
> > +{
> > +	cpufreq_frequency_table_put_attr(policy->cpu);
> > +	return 0;
> > +}
> > +
> > +static struct cpufreq_driver generic_cpufreq_driver = {
> > +	.flags = CPUFREQ_STICKY,
> > +	.verify = generic_verify_speed,
> > +	.target = generic_set_target,
> > +	.get = generic_get_speed,
> > +	.init = generic_cpufreq_init,
> > +	.exit = generic_cpufreq_exit,
> > +	.name = "generic",
> 
> This may be a little too generic?  "generic-reg-clk"?
I ever thought about it. If it's exact, it'll be "generic-reg-clk-dt".
Is "generic-reg-clk" or "generic-reg-clk-dt" too long for file name?
> 
> > +};
> > +
> > +static int __devinit generic_cpufreq_driver_init(void)
> > +{
> > +	struct device_node *cpu0;
> > +	const struct property *pp;
> > +	int i, ret;
> > +
> > +	pr_info("Generic CPU frequency driver\n");
> > +
> > +	cpu0 = of_find_node_by_path("/cpus/cpu@0");
> > +	if (!cpu0)
> > +		return -ENODEV;
> > +
> > +	if (!of_device_is_compatible(cpu0, "generic-cpufreq"))
> > +		return -ENODEV;
> 
> As above, I'd personally rather not use compatible strings, 
I still think checking compatible is better. So I need device tree
maintainer's comments.
> but if you 
> do, then I think return 0 here rather than -ENODEV else I believe you'll 
> get a potentially confusing message on the console for platforms that 
> don't use this.
I should let it be tristate. If it's not compatible, I don't need
the module any more.
> 
> > +
> > +	pp = of_find_property(cpu0, "cpu-freqs", NULL);
> > +	if (!pp) {
> > +		ret = -ENODEV;
> > +		goto put_node;
> > +	}
> > +	cpu_op_nr = pp->length / sizeof(u32);
> > +	if (!cpu_op_nr) {
> > +		ret = -ENODEV;
> > +		goto put_node;
> > +	}
> > +	ret = -ENOMEM;
> > +	cpu_freqs = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr, GFP_KERNEL);
> > +	if (!cpu_freqs)
> > +		goto put_node;
> > +	of_property_read_u32_array(cpu0, "cpu-freqs", cpu_freqs, cpu_op_nr);
> > +
> > +	pp = of_find_property(cpu0, "cpu-volts", NULL);
> > +	if (pp) {
> > +		if (cpu_op_nr == pp->length / sizeof(u32)) {
> > +			cpu_volts = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr,
> > +						GFP_KERNEL);
> > +			if (!cpu_volts)
> > +				goto free_cpu_freqs;
> > +			of_property_read_u32_array(cpu0, "cpu-volts",
> > +						cpu_volts, cpu_op_nr);
> > +		} else
> > +			pr_warn("%s: invalid cpu_volts!\n", __func__);
> > +	}
> > +
> > +	if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
> > +		trans_latency = CPUFREQ_ETERNAL;
> > +
> > +	freq_table = kmalloc(sizeof(struct cpufreq_frequency_table)
> > +				* (cpu_op_nr + 1), GFP_KERNEL);
> > +	if (!freq_table)
> > +		goto free_cpu_volts;
> > +
> > +	for (i = 0; i < cpu_op_nr; i++) {
> > +		freq_table[i].index = i;
> > +		freq_table[i].frequency = cpu_freqs[i] / 1000;
> > +	}
> > +
> > +	freq_table[i].index = i;
> > +	freq_table[i].frequency = CPUFREQ_TABLE_END;
> > +
> > +	cpu_clk = clk_get(NULL, "cpu");
> > +	if (IS_ERR(cpu_clk)) {
> > +		pr_err("%s: failed to get cpu clock\n", __func__);
> > +		ret = PTR_ERR(cpu_clk);
> > +		goto free_freq_table;
> > +	}
> > +
> > +	if (cpu_volts) {
> > +		cpu_reg = regulator_get(NULL, "cpu");
> > +		if (IS_ERR(cpu_reg)) {
> > +			pr_warn("%s: regulator cpu get failed.\n", __func__);
> > +			cpu_reg = NULL;
> > +		}
> > +	}
> > +
> > +	ret = cpufreq_register_driver(&generic_cpufreq_driver);
> > +	if (ret)
> > +		goto reg_put;
> > +
> > +	of_node_put(cpu0);
> > +
> > +	return 0;
> > +
> > +reg_put:
> > +	if (cpu_reg)
> > +		regulator_put(cpu_reg);
> > +	clk_put(cpu_clk);
> > +free_freq_table:
> > +	kfree(freq_table);
> > +free_cpu_volts:
> > +	kfree(cpu_volts);
> > +free_cpu_freqs:
> > +	kfree(cpu_freqs);
> > +put_node:
> > +	of_node_put(cpu0);
> > +
> > +	return ret;
> > +}
> > +
> > +static void generic_cpufreq_driver_exit(void)
> > +{
> > +	cpufreq_unregister_driver(&generic_cpufreq_driver);
> > +	kfree(cpu_freqs);
> > +	kfree(cpu_volts);
> > +	kfree(freq_table);
> > +	clk_put(cpu_clk);
> 
> Should this do something with the regulator too?
right.
Added:
	if (cpu_reg)
		regulator_put(cpu_reg);

Thanks very much for your review!
Richard
> 
> Jamie

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
> Hi Richard,
> 
> On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> > It support single core and multi-core ARM SoCs. But currently it assume
> > all cores share the same frequency and voltage.
> > 
> > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > ---
> >  .../devicetree/bindings/cpufreq/generic-cpufreq    |    7 +
> >  drivers/cpufreq/Kconfig                            |    8 +
> >  drivers/cpufreq/Makefile                           |    2 +
> >  drivers/cpufreq/generic-cpufreq.c                  |  251 ++++++++++++++++++++
> >  4 files changed, 268 insertions(+), 0 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> >  create mode 100644 drivers/cpufreq/generic-cpufreq.c
> > 
> > diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> > new file mode 100644
> > index 0000000..15dd780
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> > @@ -0,0 +1,7 @@
> > +Generic cpufreq driver
> > +
> > +Required properties in /cpus/cpu at 0:
> > +- compatible : "generic-cpufreq"
> 
> I'm not convinced this is the best way to do this.  By requiring a 
> generic-cpufreq compatible string we're encoding Linux driver 
> information into the hardware description.  The only way I can see to 
> avoid this is to provide a generic_clk_cpufreq_init() function that 
> platforms can call in their machine init code to use the driver.
It'll prevent the driver from being a kernel module.

Hi Grant & Rob,

Could you comment?

> 
> > +- cpu-freqs : cpu frequency points it support
> > +- cpu-volts : cpu voltages required by the frequency point at the same index
> > +- trans-latency :  transition_latency
> > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> > index e24a2a1..216eecd 100644
> > --- a/drivers/cpufreq/Kconfig
> > +++ b/drivers/cpufreq/Kconfig
> > @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
> >  
> >  	  If in doubt, say N.
> >  
> > +config GENERIC_CPUFREQ_DRIVER
> > +	bool "Generic cpufreq driver using clock/regulator/devicetree"
> > +	help
> > +	  This adds generic CPUFreq driver. It assumes all
> > +	  cores of the CPU share the same clock and voltage.
> > +
> > +	  If in doubt, say N.
> 
> I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
right, Thanks. I can not check clk before generic clock framework
come in.
Added:
	depends on OF && REGULATOR
	select CPU_FREQ_TABLE
> 
> > +
> >  menu "x86 CPU frequency scaling drivers"
> >  depends on X86
> >  source "drivers/cpufreq/Kconfig.x86"
> > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > index ce75fcb..2dbdab1 100644
> > --- a/drivers/cpufreq/Makefile
> > +++ b/drivers/cpufreq/Makefile
> > @@ -13,6 +13,8 @@ obj-$(CONFIG_CPU_FREQ_GOV_CONSERVATIVE)	+= cpufreq_conservative.o
> >  # CPUfreq cross-arch helpers
> >  obj-$(CONFIG_CPU_FREQ_TABLE)		+= freq_table.o
> >  
> > +obj-$(CONFIG_GENERIC_CPUFREQ_DRIVER)	+= generic-cpufreq.o
> > +
> >  ##################################################################################
> >  # x86 drivers.
> >  # Link order matters. K8 is preferred to ACPI because of firmware bugs in early
> > diff --git a/drivers/cpufreq/generic-cpufreq.c b/drivers/cpufreq/generic-cpufreq.c
> > new file mode 100644
> > index 0000000..781bb9b
> > --- /dev/null
> > +++ b/drivers/cpufreq/generic-cpufreq.c
> > @@ -0,0 +1,251 @@
> > +/*
> > + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> > + */
> > +
> > +/*
> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/cpufreq.h>
> > +#include <linux/clk.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/err.h>
> > +#include <linux/slab.h>
> > +#include <linux/of.h>
> > +
> > +static u32 *cpu_freqs; /* HZ */
> > +static u32 *cpu_volts; /* uV */
> > +static u32 trans_latency; /* ns */
> > +static int cpu_op_nr;
> > +
> > +static struct clk *cpu_clk;
> > +static struct regulator *cpu_reg;
> > +static struct cpufreq_frequency_table *freq_table;
> > +
> > +static int set_cpu_freq(unsigned long freq, int index, int higher)
> > +{
> > +	int ret = 0;
> > +
> > +	if (higher && cpu_reg)
> > +		regulator_set_voltage(cpu_reg,
> > +				cpu_volts[index], cpu_volts[index]);
> > +
> > +	ret = clk_set_rate(cpu_clk, freq);
> > +	if (ret != 0) {
> > +		pr_err("generic-cpufreq: cannot set CPU clock rate\n");
> > +		return ret;
> > +	}
> > +
> > +	if (!higher && cpu_reg)
> > +		regulator_set_voltage(cpu_reg,
> > +				cpu_volts[index], cpu_volts[index]);
> > +
> > +	return ret;
> > +}
> > +
> > +static int generic_verify_speed(struct cpufreq_policy *policy)
> > +{
> > +	return cpufreq_frequency_table_verify(policy, freq_table);
> > +}
> > +
> > +static unsigned int generic_get_speed(unsigned int cpu)
> > +{
> > +	return clk_get_rate(cpu_clk) / 1000;
> > +}
> > +
> > +static int generic_set_target(struct cpufreq_policy *policy,
> > +			  unsigned int target_freq, unsigned int relation)
> > +{
> > +	struct cpufreq_freqs freqs;
> > +	unsigned long freq_Hz;
> > +	int cpu;
> > +	int ret = 0;
> > +	unsigned int index;
> > +
> > +	cpufreq_frequency_table_target(policy, freq_table,
> > +			target_freq, relation, &index);
> > +	freq_Hz = clk_round_rate(cpu_clk, cpu_freqs[index]);
> > +	freq_Hz = freq_Hz ? freq_Hz : cpu_freqs[index];
> > +	freqs.old = clk_get_rate(cpu_clk) / 1000;
> > +	freqs.new = freq_Hz / 1000;
> > +	freqs.flags = 0;
> > +
> > +	if (freqs.old == freqs.new)
> > +		return 0;
> > +
> > +	for_each_possible_cpu(cpu) {
> > +		freqs.cpu = cpu;
> > +		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > +	}
> > +
> > +	ret = set_cpu_freq(freq_Hz, index, (freqs.new > freqs.old));
> 
> If this fails then we'll still be notifying the transition at the 
> requested rate even though it didn't work.  I guess we should really get 
> the rate of the clk and put that into freqs for the POSTCHANGE 
> notification.
right, Thanks.
Added:
	if (ret)
		freq.new = clk_get_rate(cpu_clk) / 1000;

> 
> > +
> > +	for_each_possible_cpu(cpu) {
> > +		freqs.cpu = cpu;
> > +		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int generic_cpufreq_init(struct cpufreq_policy *policy)
> > +{
> > +	int ret;
> > +
> > +	if (policy->cpu >= num_possible_cpus())
> > +		return -EINVAL;
> > +
> > +	policy->cur = clk_get_rate(cpu_clk) / 1000;
> > +	policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> > +	cpumask_setall(policy->cpus);
> > +	/* Manual states, that PLL stabilizes in two CLK32 periods */
> > +	policy->cpuinfo.transition_latency = trans_latency;
> > +
> > +	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> > +
> > +	if (ret < 0) {
> > +		pr_err("%s: invalid frequency table for cpu %d\n",
> > +		       __func__, policy->cpu);
> > +		return ret;
> > +	}
> > +
> > +	cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
> > +	return 0;
> > +}
> > +
> > +static int generic_cpufreq_exit(struct cpufreq_policy *policy)
> > +{
> > +	cpufreq_frequency_table_put_attr(policy->cpu);
> > +	return 0;
> > +}
> > +
> > +static struct cpufreq_driver generic_cpufreq_driver = {
> > +	.flags = CPUFREQ_STICKY,
> > +	.verify = generic_verify_speed,
> > +	.target = generic_set_target,
> > +	.get = generic_get_speed,
> > +	.init = generic_cpufreq_init,
> > +	.exit = generic_cpufreq_exit,
> > +	.name = "generic",
> 
> This may be a little too generic?  "generic-reg-clk"?
I ever thought about it. If it's exact, it'll be "generic-reg-clk-dt".
Is "generic-reg-clk" or "generic-reg-clk-dt" too long for file name?
> 
> > +};
> > +
> > +static int __devinit generic_cpufreq_driver_init(void)
> > +{
> > +	struct device_node *cpu0;
> > +	const struct property *pp;
> > +	int i, ret;
> > +
> > +	pr_info("Generic CPU frequency driver\n");
> > +
> > +	cpu0 = of_find_node_by_path("/cpus/cpu at 0");
> > +	if (!cpu0)
> > +		return -ENODEV;
> > +
> > +	if (!of_device_is_compatible(cpu0, "generic-cpufreq"))
> > +		return -ENODEV;
> 
> As above, I'd personally rather not use compatible strings, 
I still think checking compatible is better. So I need device tree
maintainer's comments.
> but if you 
> do, then I think return 0 here rather than -ENODEV else I believe you'll 
> get a potentially confusing message on the console for platforms that 
> don't use this.
I should let it be tristate. If it's not compatible, I don't need
the module any more.
> 
> > +
> > +	pp = of_find_property(cpu0, "cpu-freqs", NULL);
> > +	if (!pp) {
> > +		ret = -ENODEV;
> > +		goto put_node;
> > +	}
> > +	cpu_op_nr = pp->length / sizeof(u32);
> > +	if (!cpu_op_nr) {
> > +		ret = -ENODEV;
> > +		goto put_node;
> > +	}
> > +	ret = -ENOMEM;
> > +	cpu_freqs = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr, GFP_KERNEL);
> > +	if (!cpu_freqs)
> > +		goto put_node;
> > +	of_property_read_u32_array(cpu0, "cpu-freqs", cpu_freqs, cpu_op_nr);
> > +
> > +	pp = of_find_property(cpu0, "cpu-volts", NULL);
> > +	if (pp) {
> > +		if (cpu_op_nr == pp->length / sizeof(u32)) {
> > +			cpu_volts = kzalloc(sizeof(*cpu_freqs) * cpu_op_nr,
> > +						GFP_KERNEL);
> > +			if (!cpu_volts)
> > +				goto free_cpu_freqs;
> > +			of_property_read_u32_array(cpu0, "cpu-volts",
> > +						cpu_volts, cpu_op_nr);
> > +		} else
> > +			pr_warn("%s: invalid cpu_volts!\n", __func__);
> > +	}
> > +
> > +	if (of_property_read_u32(cpu0, "trans-latency", &trans_latency))
> > +		trans_latency = CPUFREQ_ETERNAL;
> > +
> > +	freq_table = kmalloc(sizeof(struct cpufreq_frequency_table)
> > +				* (cpu_op_nr + 1), GFP_KERNEL);
> > +	if (!freq_table)
> > +		goto free_cpu_volts;
> > +
> > +	for (i = 0; i < cpu_op_nr; i++) {
> > +		freq_table[i].index = i;
> > +		freq_table[i].frequency = cpu_freqs[i] / 1000;
> > +	}
> > +
> > +	freq_table[i].index = i;
> > +	freq_table[i].frequency = CPUFREQ_TABLE_END;
> > +
> > +	cpu_clk = clk_get(NULL, "cpu");
> > +	if (IS_ERR(cpu_clk)) {
> > +		pr_err("%s: failed to get cpu clock\n", __func__);
> > +		ret = PTR_ERR(cpu_clk);
> > +		goto free_freq_table;
> > +	}
> > +
> > +	if (cpu_volts) {
> > +		cpu_reg = regulator_get(NULL, "cpu");
> > +		if (IS_ERR(cpu_reg)) {
> > +			pr_warn("%s: regulator cpu get failed.\n", __func__);
> > +			cpu_reg = NULL;
> > +		}
> > +	}
> > +
> > +	ret = cpufreq_register_driver(&generic_cpufreq_driver);
> > +	if (ret)
> > +		goto reg_put;
> > +
> > +	of_node_put(cpu0);
> > +
> > +	return 0;
> > +
> > +reg_put:
> > +	if (cpu_reg)
> > +		regulator_put(cpu_reg);
> > +	clk_put(cpu_clk);
> > +free_freq_table:
> > +	kfree(freq_table);
> > +free_cpu_volts:
> > +	kfree(cpu_volts);
> > +free_cpu_freqs:
> > +	kfree(cpu_freqs);
> > +put_node:
> > +	of_node_put(cpu0);
> > +
> > +	return ret;
> > +}
> > +
> > +static void generic_cpufreq_driver_exit(void)
> > +{
> > +	cpufreq_unregister_driver(&generic_cpufreq_driver);
> > +	kfree(cpu_freqs);
> > +	kfree(cpu_volts);
> > +	kfree(freq_table);
> > +	clk_put(cpu_clk);
> 
> Should this do something with the regulator too?
right.
Added:
	if (cpu_reg)
		regulator_put(cpu_reg);

Thanks very much for your review!
Richard
> 
> Jamie

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Jamie Iles]

On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote:
> On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
> > Hi Richard,
> > 
> > On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> > > It support single core and multi-core ARM SoCs. But currently it assume
> > > all cores share the same frequency and voltage.
> > > 
> > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > ---
> > >  .../devicetree/bindings/cpufreq/generic-cpufreq    |    7 +
> > >  drivers/cpufreq/Kconfig                            |    8 +
> > >  drivers/cpufreq/Makefile                           |    2 +
> > >  drivers/cpufreq/generic-cpufreq.c                  |  251 ++++++++++++++++++++
> > >  4 files changed, 268 insertions(+), 0 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> > >  create mode 100644 drivers/cpufreq/generic-cpufreq.c
> > > 
> > > diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> > > new file mode 100644
> > > index 0000000..15dd780
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> > > @@ -0,0 +1,7 @@
> > > +Generic cpufreq driver
> > > +
> > > +Required properties in /cpus/cpu@0:
> > > +- compatible : "generic-cpufreq"
> > 
> > I'm not convinced this is the best way to do this.  By requiring a 
> > generic-cpufreq compatible string we're encoding Linux driver 
> > information into the hardware description.  The only way I can see to 
> > avoid this is to provide a generic_clk_cpufreq_init() function that 
> > platforms can call in their machine init code to use the driver.
> It'll prevent the driver from being a kernel module.

Hmm, that's not very nice either!  I guess you _could_ add an 
of_machine_is_compatible() check against a list of compatible machines 
in the driver but that feels a little gross.  Hopefully Rob or Grant 
have a good alternative!

> Hi Grant & Rob,
> 
> Could you comment?
> 
> > 
> > > +- cpu-freqs : cpu frequency points it support
> > > +- cpu-volts : cpu voltages required by the frequency point at the same index
> > > +- trans-latency :  transition_latency
> > > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> > > index e24a2a1..216eecd 100644
> > > --- a/drivers/cpufreq/Kconfig
> > > +++ b/drivers/cpufreq/Kconfig
> > > @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
> > >  
> > >  	  If in doubt, say N.
> > >  
> > > +config GENERIC_CPUFREQ_DRIVER
> > > +	bool "Generic cpufreq driver using clock/regulator/devicetree"
> > > +	help
> > > +	  This adds generic CPUFreq driver. It assumes all
> > > +	  cores of the CPU share the same clock and voltage.
> > > +
> > > +	  If in doubt, say N.
> > 
> > I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
> right, Thanks. I can not check clk before generic clock framework
> come in.
> Added:
> 	depends on OF && REGULATOR
> 	select CPU_FREQ_TABLE

You can still use HAVE_CLK.  That symbol has been around for ages and 
any platform implementing the clk API should select it so it's fine to 
depend on it even before there is a generic struct clk.

Jamie

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Jamie Iles]

On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote:
> On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
> > Hi Richard,
> > 
> > On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> > > It support single core and multi-core ARM SoCs. But currently it assume
> > > all cores share the same frequency and voltage.
> > > 
> > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > ---
> > >  .../devicetree/bindings/cpufreq/generic-cpufreq    |    7 +
> > >  drivers/cpufreq/Kconfig                            |    8 +
> > >  drivers/cpufreq/Makefile                           |    2 +
> > >  drivers/cpufreq/generic-cpufreq.c                  |  251 ++++++++++++++++++++
> > >  4 files changed, 268 insertions(+), 0 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> > >  create mode 100644 drivers/cpufreq/generic-cpufreq.c
> > > 
> > > diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> > > new file mode 100644
> > > index 0000000..15dd780
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> > > @@ -0,0 +1,7 @@
> > > +Generic cpufreq driver
> > > +
> > > +Required properties in /cpus/cpu at 0:
> > > +- compatible : "generic-cpufreq"
> > 
> > I'm not convinced this is the best way to do this.  By requiring a 
> > generic-cpufreq compatible string we're encoding Linux driver 
> > information into the hardware description.  The only way I can see to 
> > avoid this is to provide a generic_clk_cpufreq_init() function that 
> > platforms can call in their machine init code to use the driver.
> It'll prevent the driver from being a kernel module.

Hmm, that's not very nice either!  I guess you _could_ add an 
of_machine_is_compatible() check against a list of compatible machines 
in the driver but that feels a little gross.  Hopefully Rob or Grant 
have a good alternative!

> Hi Grant & Rob,
> 
> Could you comment?
> 
> > 
> > > +- cpu-freqs : cpu frequency points it support
> > > +- cpu-volts : cpu voltages required by the frequency point at the same index
> > > +- trans-latency :  transition_latency
> > > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> > > index e24a2a1..216eecd 100644
> > > --- a/drivers/cpufreq/Kconfig
> > > +++ b/drivers/cpufreq/Kconfig
> > > @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
> > >  
> > >  	  If in doubt, say N.
> > >  
> > > +config GENERIC_CPUFREQ_DRIVER
> > > +	bool "Generic cpufreq driver using clock/regulator/devicetree"
> > > +	help
> > > +	  This adds generic CPUFreq driver. It assumes all
> > > +	  cores of the CPU share the same clock and voltage.
> > > +
> > > +	  If in doubt, say N.
> > 
> > I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
> right, Thanks. I can not check clk before generic clock framework
> come in.
> Added:
> 	depends on OF && REGULATOR
> 	select CPU_FREQ_TABLE

You can still use HAVE_CLK.  That symbol has been around for ages and 
any platform implementing the clk API should select it so it's fine to 
depend on it even before there is a generic struct clk.

Jamie

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Rob Herring]

On 12/19/2011 08:39 AM, Jamie Iles wrote:
> On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote:
>> On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
>>> Hi Richard,
>>>
>>> On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
>>>> It support single core and multi-core ARM SoCs. But currently it assume
>>>> all cores share the same frequency and voltage.
>>>>
>>>> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
>>>> ---
>>>>  .../devicetree/bindings/cpufreq/generic-cpufreq    |    7 +
>>>>  drivers/cpufreq/Kconfig                            |    8 +
>>>>  drivers/cpufreq/Makefile                           |    2 +
>>>>  drivers/cpufreq/generic-cpufreq.c                  |  251 ++++++++++++++++++++
>>>>  4 files changed, 268 insertions(+), 0 deletions(-)
>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq
>>>>  create mode 100644 drivers/cpufreq/generic-cpufreq.c
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
>>>> new file mode 100644
>>>> index 0000000..15dd780
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
>>>> @@ -0,0 +1,7 @@
>>>> +Generic cpufreq driver
>>>> +
>>>> +Required properties in /cpus/cpu@0:
>>>> +- compatible : "generic-cpufreq"
>>>
>>> I'm not convinced this is the best way to do this.  By requiring a 
>>> generic-cpufreq compatible string we're encoding Linux driver 
>>> information into the hardware description.  The only way I can see to 
>>> avoid this is to provide a generic_clk_cpufreq_init() function that 
>>> platforms can call in their machine init code to use the driver.

Agreed on the compatible string. It's putting Linux specifics into DT.

You could flip this around and have the module make a call into the
kernel to determine whether to initialize or not. Then platforms could
set a flag to indicate this.

>> It'll prevent the driver from being a kernel module.
> 
> Hmm, that's not very nice either!  I guess you _could_ add an 
> of_machine_is_compatible() check against a list of compatible machines 
> in the driver but that feels a little gross.  Hopefully Rob or Grant 
> have a good alternative!
> 

What does cpufreq core do if multiple drivers are registered? Perhaps a
ranking is needed and this would only get enabled if there are no other
drivers and other conditions like having the clock "cpu" present are met.

Rob

>> Hi Grant & Rob,
>>
>> Could you comment?
>>
>>>
>>>> +- cpu-freqs : cpu frequency points it support
>>>> +- cpu-volts : cpu voltages required by the frequency point at the same index
>>>> +- trans-latency :  transition_latency
>>>> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
>>>> index e24a2a1..216eecd 100644
>>>> --- a/drivers/cpufreq/Kconfig
>>>> +++ b/drivers/cpufreq/Kconfig
>>>> @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
>>>>  
>>>>  	  If in doubt, say N.
>>>>  
>>>> +config GENERIC_CPUFREQ_DRIVER
>>>> +	bool "Generic cpufreq driver using clock/regulator/devicetree"
>>>> +	help
>>>> +	  This adds generic CPUFreq driver. It assumes all
>>>> +	  cores of the CPU share the same clock and voltage.
>>>> +
>>>> +	  If in doubt, say N.
>>>
>>> I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
>> right, Thanks. I can not check clk before generic clock framework
>> come in.
>> Added:
>> 	depends on OF && REGULATOR
>> 	select CPU_FREQ_TABLE
> 
> You can still use HAVE_CLK.  That symbol has been around for ages and 
> any platform implementing the clk API should select it so it's fine to 
> depend on it even before there is a generic struct clk.
> 
> Jamie

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Rob Herring]

On 12/19/2011 08:39 AM, Jamie Iles wrote:
> On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote:
>> On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
>>> Hi Richard,
>>>
>>> On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
>>>> It support single core and multi-core ARM SoCs. But currently it assume
>>>> all cores share the same frequency and voltage.
>>>>
>>>> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
>>>> ---
>>>>  .../devicetree/bindings/cpufreq/generic-cpufreq    |    7 +
>>>>  drivers/cpufreq/Kconfig                            |    8 +
>>>>  drivers/cpufreq/Makefile                           |    2 +
>>>>  drivers/cpufreq/generic-cpufreq.c                  |  251 ++++++++++++++++++++
>>>>  4 files changed, 268 insertions(+), 0 deletions(-)
>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq
>>>>  create mode 100644 drivers/cpufreq/generic-cpufreq.c
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
>>>> new file mode 100644
>>>> index 0000000..15dd780
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
>>>> @@ -0,0 +1,7 @@
>>>> +Generic cpufreq driver
>>>> +
>>>> +Required properties in /cpus/cpu at 0:
>>>> +- compatible : "generic-cpufreq"
>>>
>>> I'm not convinced this is the best way to do this.  By requiring a 
>>> generic-cpufreq compatible string we're encoding Linux driver 
>>> information into the hardware description.  The only way I can see to 
>>> avoid this is to provide a generic_clk_cpufreq_init() function that 
>>> platforms can call in their machine init code to use the driver.

Agreed on the compatible string. It's putting Linux specifics into DT.

You could flip this around and have the module make a call into the
kernel to determine whether to initialize or not. Then platforms could
set a flag to indicate this.

>> It'll prevent the driver from being a kernel module.
> 
> Hmm, that's not very nice either!  I guess you _could_ add an 
> of_machine_is_compatible() check against a list of compatible machines 
> in the driver but that feels a little gross.  Hopefully Rob or Grant 
> have a good alternative!
> 

What does cpufreq core do if multiple drivers are registered? Perhaps a
ranking is needed and this would only get enabled if there are no other
drivers and other conditions like having the clock "cpu" present are met.

Rob

>> Hi Grant & Rob,
>>
>> Could you comment?
>>
>>>
>>>> +- cpu-freqs : cpu frequency points it support
>>>> +- cpu-volts : cpu voltages required by the frequency point at the same index
>>>> +- trans-latency :  transition_latency
>>>> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
>>>> index e24a2a1..216eecd 100644
>>>> --- a/drivers/cpufreq/Kconfig
>>>> +++ b/drivers/cpufreq/Kconfig
>>>> @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
>>>>  
>>>>  	  If in doubt, say N.
>>>>  
>>>> +config GENERIC_CPUFREQ_DRIVER
>>>> +	bool "Generic cpufreq driver using clock/regulator/devicetree"
>>>> +	help
>>>> +	  This adds generic CPUFreq driver. It assumes all
>>>> +	  cores of the CPU share the same clock and voltage.
>>>> +
>>>> +	  If in doubt, say N.
>>>
>>> I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
>> right, Thanks. I can not check clk before generic clock framework
>> come in.
>> Added:
>> 	depends on OF && REGULATOR
>> 	select CPU_FREQ_TABLE
> 
> You can still use HAVE_CLK.  That symbol has been around for ages and 
> any platform implementing the clk API should select it so it's fine to 
> depend on it even before there is a generic struct clk.
> 
> Jamie

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

On Mon, Dec 19, 2011 at 09:00:44AM -0600, Rob Herring wrote:
> On 12/19/2011 08:39 AM, Jamie Iles wrote:
> > On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote:
> >> On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
> >>> Hi Richard,
> >>>
> >>> On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> >>>> It support single core and multi-core ARM SoCs. But currently it assume
> >>>> all cores share the same frequency and voltage.
> >>>>
> >>>> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> >>>> ---
> >>>>  .../devicetree/bindings/cpufreq/generic-cpufreq    |    7 +
> >>>>  drivers/cpufreq/Kconfig                            |    8 +
> >>>>  drivers/cpufreq/Makefile                           |    2 +
> >>>>  drivers/cpufreq/generic-cpufreq.c                  |  251 ++++++++++++++++++++
> >>>>  4 files changed, 268 insertions(+), 0 deletions(-)
> >>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> >>>>  create mode 100644 drivers/cpufreq/generic-cpufreq.c
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> >>>> new file mode 100644
> >>>> index 0000000..15dd780
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> >>>> @@ -0,0 +1,7 @@
> >>>> +Generic cpufreq driver
> >>>> +
> >>>> +Required properties in /cpus/cpu@0:
> >>>> +- compatible : "generic-cpufreq"
> >>>
> >>> I'm not convinced this is the best way to do this.  By requiring a 
> >>> generic-cpufreq compatible string we're encoding Linux driver 
> >>> information into the hardware description.  The only way I can see to 
> >>> avoid this is to provide a generic_clk_cpufreq_init() function that 
> >>> platforms can call in their machine init code to use the driver.
> 
> Agreed on the compatible string.
Assume you reject to use compatible string.
> It's putting Linux specifics into DT.
> 
> You could flip this around and have the module make a call into the
> kernel to determine whether to initialize or not. Then platforms could
> set a flag to indicate this.
Could you make it more clear? kernel global variable, macro, or function?

- Following your idea, I think, we can add in driver/cpufreq/cpufreq.c:
int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size);
SoC code set the callback. If it's NULL, driver will exit. We can get rid
of DT. It'll make cpufreq core dirty, but it's the only file built-in.

- Drop module support. SoC call generic_clk_cpufreq_init as Jamie said.

> 
> >> It'll prevent the driver from being a kernel module.
> > 
> > Hmm, that's not very nice either!  I guess you _could_ add an 
> > of_machine_is_compatible() check against a list of compatible machines 
> > in the driver but that feels a little gross.  Hopefully Rob or Grant 
> > have a good alternative!
> > 
> 
> What does cpufreq core do if multiple drivers are registered?
current cpufreq core only support one cpufreq_driver. Others will fail
except the first time.
> Perhaps a
> ranking is needed and this would only get enabled if there are no other
> drivers and other conditions like having the clock "cpu" present are met.
We'd better keep cpufreq core simple. For this driver, register cpufreq_driver
is the last thing after checking all conditions.

> 
> Rob
> 
> >> Hi Grant & Rob,
> >>
> >> Could you comment?
> >>
> >>>
> >>>> +- cpu-freqs : cpu frequency points it support
> >>>> +- cpu-volts : cpu voltages required by the frequency point at the same index
> >>>> +- trans-latency :  transition_latency
> >>>> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> >>>> index e24a2a1..216eecd 100644
> >>>> --- a/drivers/cpufreq/Kconfig
> >>>> +++ b/drivers/cpufreq/Kconfig
> >>>> @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
> >>>>  
> >>>>  	  If in doubt, say N.
> >>>>  
> >>>> +config GENERIC_CPUFREQ_DRIVER
> >>>> +	bool "Generic cpufreq driver using clock/regulator/devicetree"
> >>>> +	help
> >>>> +	  This adds generic CPUFreq driver. It assumes all
> >>>> +	  cores of the CPU share the same clock and voltage.
> >>>> +
> >>>> +	  If in doubt, say N.
> >>>
> >>> I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
> >> right, Thanks. I can not check clk before generic clock framework
> >> come in.
> >> Added:
> >> 	depends on OF && REGULATOR
> >> 	select CPU_FREQ_TABLE
> > 
> > You can still use HAVE_CLK.  That symbol has been around for ages and 
> > any platform implementing the clk API should select it so it's fine to 
> > depend on it even before there is a generic struct clk.
You are right. Thanks.

Richard
> > 
> > Jamie
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

On Mon, Dec 19, 2011 at 09:00:44AM -0600, Rob Herring wrote:
> On 12/19/2011 08:39 AM, Jamie Iles wrote:
> > On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote:
> >> On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
> >>> Hi Richard,
> >>>
> >>> On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> >>>> It support single core and multi-core ARM SoCs. But currently it assume
> >>>> all cores share the same frequency and voltage.
> >>>>
> >>>> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> >>>> ---
> >>>>  .../devicetree/bindings/cpufreq/generic-cpufreq    |    7 +
> >>>>  drivers/cpufreq/Kconfig                            |    8 +
> >>>>  drivers/cpufreq/Makefile                           |    2 +
> >>>>  drivers/cpufreq/generic-cpufreq.c                  |  251 ++++++++++++++++++++
> >>>>  4 files changed, 268 insertions(+), 0 deletions(-)
> >>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> >>>>  create mode 100644 drivers/cpufreq/generic-cpufreq.c
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> >>>> new file mode 100644
> >>>> index 0000000..15dd780
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> >>>> @@ -0,0 +1,7 @@
> >>>> +Generic cpufreq driver
> >>>> +
> >>>> +Required properties in /cpus/cpu at 0:
> >>>> +- compatible : "generic-cpufreq"
> >>>
> >>> I'm not convinced this is the best way to do this.  By requiring a 
> >>> generic-cpufreq compatible string we're encoding Linux driver 
> >>> information into the hardware description.  The only way I can see to 
> >>> avoid this is to provide a generic_clk_cpufreq_init() function that 
> >>> platforms can call in their machine init code to use the driver.
> 
> Agreed on the compatible string.
Assume you reject to use compatible string.
> It's putting Linux specifics into DT.
> 
> You could flip this around and have the module make a call into the
> kernel to determine whether to initialize or not. Then platforms could
> set a flag to indicate this.
Could you make it more clear? kernel global variable, macro, or function?

- Following your idea, I think, we can add in driver/cpufreq/cpufreq.c:
int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size);
SoC code set the callback. If it's NULL, driver will exit. We can get rid
of DT. It'll make cpufreq core dirty, but it's the only file built-in.

- Drop module support. SoC call generic_clk_cpufreq_init as Jamie said.

> 
> >> It'll prevent the driver from being a kernel module.
> > 
> > Hmm, that's not very nice either!  I guess you _could_ add an 
> > of_machine_is_compatible() check against a list of compatible machines 
> > in the driver but that feels a little gross.  Hopefully Rob or Grant 
> > have a good alternative!
> > 
> 
> What does cpufreq core do if multiple drivers are registered?
current cpufreq core only support one cpufreq_driver. Others will fail
except the first time.
> Perhaps a
> ranking is needed and this would only get enabled if there are no other
> drivers and other conditions like having the clock "cpu" present are met.
We'd better keep cpufreq core simple. For this driver, register cpufreq_driver
is the last thing after checking all conditions.

> 
> Rob
> 
> >> Hi Grant & Rob,
> >>
> >> Could you comment?
> >>
> >>>
> >>>> +- cpu-freqs : cpu frequency points it support
> >>>> +- cpu-volts : cpu voltages required by the frequency point at the same index
> >>>> +- trans-latency :  transition_latency
> >>>> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> >>>> index e24a2a1..216eecd 100644
> >>>> --- a/drivers/cpufreq/Kconfig
> >>>> +++ b/drivers/cpufreq/Kconfig
> >>>> @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
> >>>>  
> >>>>  	  If in doubt, say N.
> >>>>  
> >>>> +config GENERIC_CPUFREQ_DRIVER
> >>>> +	bool "Generic cpufreq driver using clock/regulator/devicetree"
> >>>> +	help
> >>>> +	  This adds generic CPUFreq driver. It assumes all
> >>>> +	  cores of the CPU share the same clock and voltage.
> >>>> +
> >>>> +	  If in doubt, say N.
> >>>
> >>> I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
> >> right, Thanks. I can not check clk before generic clock framework
> >> come in.
> >> Added:
> >> 	depends on OF && REGULATOR
> >> 	select CPU_FREQ_TABLE
> > 
> > You can still use HAVE_CLK.  That symbol has been around for ages and 
> > any platform implementing the clk API should select it so it's fine to 
> > depend on it even before there is a generic struct clk.
You are right. Thanks.

Richard
> > 
> > Jamie
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Mark Brown]

On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> It support single core and multi-core ARM SoCs. But currently it assume
> all cores share the same frequency and voltage.

My comments on the previous version of the patch still apply:

 - The voltage ranges being set need to be specified as ranges.
 - Frequencies that can't be supported due to limitations of the
   available supplies shouldn't be exposed to users.
 - The driver needs to handle errors.

> +Required properties in /cpus/cpu@0:
> +- compatible : "generic-cpufreq"
> +- cpu-freqs : cpu frequency points it support
> +- cpu-volts : cpu voltages required by the frequency point at the same index
> +- trans-latency :  transition_latency

You need to define units for all of these, and for the transition
latency you need to be clear about what's being measured (it looks like
the CPU time only, not any voltage ramping).

You also need to define how the core supplies get looked up.

> +	/* Manual states, that PLL stabilizes in two CLK32 periods */
> +	policy->cpuinfo.transition_latency = trans_latency;

I guess this comment is a cut'n'paste error.

> +
> +	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> +
> +	if (ret < 0) {
> +		pr_err("%s: invalid frequency table for cpu %d\n",
> +		       __func__, policy->cpu);

You should define pr_fmt to always include __func__ in the messages
rather than open coding - ensures consistency and is less noisy in the
code.

> +	pr_info("Generic CPU frequency driver\n");

This seems noisy...

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Mark Brown]

On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> It support single core and multi-core ARM SoCs. But currently it assume
> all cores share the same frequency and voltage.

My comments on the previous version of the patch still apply:

 - The voltage ranges being set need to be specified as ranges.
 - Frequencies that can't be supported due to limitations of the
   available supplies shouldn't be exposed to users.
 - The driver needs to handle errors.

> +Required properties in /cpus/cpu at 0:
> +- compatible : "generic-cpufreq"
> +- cpu-freqs : cpu frequency points it support
> +- cpu-volts : cpu voltages required by the frequency point at the same index
> +- trans-latency :  transition_latency

You need to define units for all of these, and for the transition
latency you need to be clear about what's being measured (it looks like
the CPU time only, not any voltage ramping).

You also need to define how the core supplies get looked up.

> +	/* Manual states, that PLL stabilizes in two CLK32 periods */
> +	policy->cpuinfo.transition_latency = trans_latency;

I guess this comment is a cut'n'paste error.

> +
> +	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> +
> +	if (ret < 0) {
> +		pr_err("%s: invalid frequency table for cpu %d\n",
> +		       __func__, policy->cpu);

You should define pr_fmt to always include __func__ in the messages
rather than open coding - ensures consistency and is less noisy in the
code.

> +	pr_info("Generic CPU frequency driver\n");

This seems noisy...

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Rob Herring]

On 12/19/2011 07:59 PM, Richard Zhao wrote:
> On Mon, Dec 19, 2011 at 09:00:44AM -0600, Rob Herring wrote:
>> On 12/19/2011 08:39 AM, Jamie Iles wrote:
>>> On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote:
>>>> On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
>>>>> Hi Richard,
>>>>>
>>>>> On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
>>>>>> It support single core and multi-core ARM SoCs. But currently it assume
>>>>>> all cores share the same frequency and voltage.
>>>>>>
>>>>>> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
>>>>>> ---
>>>>>>  .../devicetree/bindings/cpufreq/generic-cpufreq    |    7 +
>>>>>>  drivers/cpufreq/Kconfig                            |    8 +
>>>>>>  drivers/cpufreq/Makefile                           |    2 +
>>>>>>  drivers/cpufreq/generic-cpufreq.c                  |  251 ++++++++++++++++++++
>>>>>>  4 files changed, 268 insertions(+), 0 deletions(-)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq
>>>>>>  create mode 100644 drivers/cpufreq/generic-cpufreq.c
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
>>>>>> new file mode 100644
>>>>>> index 0000000..15dd780
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
>>>>>> @@ -0,0 +1,7 @@
>>>>>> +Generic cpufreq driver
>>>>>> +
>>>>>> +Required properties in /cpus/cpu@0:
>>>>>> +- compatible : "generic-cpufreq"
>>>>>
>>>>> I'm not convinced this is the best way to do this.  By requiring a 
>>>>> generic-cpufreq compatible string we're encoding Linux driver 
>>>>> information into the hardware description.  The only way I can see to 
>>>>> avoid this is to provide a generic_clk_cpufreq_init() function that 
>>>>> platforms can call in their machine init code to use the driver.
>>
>> Agreed on the compatible string.
> Assume you reject to use compatible string.
>> It's putting Linux specifics into DT.
>>
>> You could flip this around and have the module make a call into the
>> kernel to determine whether to initialize or not. Then platforms could
>> set a flag to indicate this.
> Could you make it more clear? kernel global variable, macro, or function?

Any of those. Of course, direct access to variables across modules is
discouraged, so it would probably be a function that checks a variable.

> - Following your idea, I think, we can add in driver/cpufreq/cpufreq.c:
> int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size);
> SoC code set the callback. If it's NULL, driver will exit. We can get rid
> of DT. It'll make cpufreq core dirty, but it's the only file built-in.

But aren't you getting the operating points from the DT? Then you don't
want to put this code into each platform.

> 
> - Drop module support. SoC call generic_clk_cpufreq_init as Jamie said.
> 
>>
>>>> It'll prevent the driver from being a kernel module.
>>>
>>> Hmm, that's not very nice either!  I guess you _could_ add an 
>>> of_machine_is_compatible() check against a list of compatible machines 
>>> in the driver but that feels a little gross.  Hopefully Rob or Grant 
>>> have a good alternative!
>>>
>>
>> What does cpufreq core do if multiple drivers are registered?
> current cpufreq core only support one cpufreq_driver. Others will fail
> except the first time.

Then whoever gets there first wins. Make your driver register late and
if someone doesn't want to use it they can register a custom driver earlier.

Rob

>> Perhaps a
>> ranking is needed and this would only get enabled if there are no other
>> drivers and other conditions like having the clock "cpu" present are met.
> We'd better keep cpufreq core simple. For this driver, register cpufreq_driver
> is the last thing after checking all conditions.
> 
>>
>> Rob
>>
>>>> Hi Grant & Rob,
>>>>
>>>> Could you comment?
>>>>
>>>>>
>>>>>> +- cpu-freqs : cpu frequency points it support
>>>>>> +- cpu-volts : cpu voltages required by the frequency point at the same index
>>>>>> +- trans-latency :  transition_latency
>>>>>> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
>>>>>> index e24a2a1..216eecd 100644
>>>>>> --- a/drivers/cpufreq/Kconfig
>>>>>> +++ b/drivers/cpufreq/Kconfig
>>>>>> @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
>>>>>>  
>>>>>>  	  If in doubt, say N.
>>>>>>  
>>>>>> +config GENERIC_CPUFREQ_DRIVER
>>>>>> +	bool "Generic cpufreq driver using clock/regulator/devicetree"
>>>>>> +	help
>>>>>> +	  This adds generic CPUFreq driver. It assumes all
>>>>>> +	  cores of the CPU share the same clock and voltage.
>>>>>> +
>>>>>> +	  If in doubt, say N.
>>>>>
>>>>> I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
>>>> right, Thanks. I can not check clk before generic clock framework
>>>> come in.
>>>> Added:
>>>> 	depends on OF && REGULATOR
>>>> 	select CPU_FREQ_TABLE
>>>
>>> You can still use HAVE_CLK.  That symbol has been around for ages and 
>>> any platform implementing the clk API should select it so it's fine to 
>>> depend on it even before there is a generic struct clk.
> You are right. Thanks.
> 
> Richard
>>>
>>> Jamie
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Rob Herring]

On 12/19/2011 07:59 PM, Richard Zhao wrote:
> On Mon, Dec 19, 2011 at 09:00:44AM -0600, Rob Herring wrote:
>> On 12/19/2011 08:39 AM, Jamie Iles wrote:
>>> On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote:
>>>> On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
>>>>> Hi Richard,
>>>>>
>>>>> On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
>>>>>> It support single core and multi-core ARM SoCs. But currently it assume
>>>>>> all cores share the same frequency and voltage.
>>>>>>
>>>>>> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
>>>>>> ---
>>>>>>  .../devicetree/bindings/cpufreq/generic-cpufreq    |    7 +
>>>>>>  drivers/cpufreq/Kconfig                            |    8 +
>>>>>>  drivers/cpufreq/Makefile                           |    2 +
>>>>>>  drivers/cpufreq/generic-cpufreq.c                  |  251 ++++++++++++++++++++
>>>>>>  4 files changed, 268 insertions(+), 0 deletions(-)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq
>>>>>>  create mode 100644 drivers/cpufreq/generic-cpufreq.c
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
>>>>>> new file mode 100644
>>>>>> index 0000000..15dd780
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
>>>>>> @@ -0,0 +1,7 @@
>>>>>> +Generic cpufreq driver
>>>>>> +
>>>>>> +Required properties in /cpus/cpu at 0:
>>>>>> +- compatible : "generic-cpufreq"
>>>>>
>>>>> I'm not convinced this is the best way to do this.  By requiring a 
>>>>> generic-cpufreq compatible string we're encoding Linux driver 
>>>>> information into the hardware description.  The only way I can see to 
>>>>> avoid this is to provide a generic_clk_cpufreq_init() function that 
>>>>> platforms can call in their machine init code to use the driver.
>>
>> Agreed on the compatible string.
> Assume you reject to use compatible string.
>> It's putting Linux specifics into DT.
>>
>> You could flip this around and have the module make a call into the
>> kernel to determine whether to initialize or not. Then platforms could
>> set a flag to indicate this.
> Could you make it more clear? kernel global variable, macro, or function?

Any of those. Of course, direct access to variables across modules is
discouraged, so it would probably be a function that checks a variable.

> - Following your idea, I think, we can add in driver/cpufreq/cpufreq.c:
> int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size);
> SoC code set the callback. If it's NULL, driver will exit. We can get rid
> of DT. It'll make cpufreq core dirty, but it's the only file built-in.

But aren't you getting the operating points from the DT? Then you don't
want to put this code into each platform.

> 
> - Drop module support. SoC call generic_clk_cpufreq_init as Jamie said.
> 
>>
>>>> It'll prevent the driver from being a kernel module.
>>>
>>> Hmm, that's not very nice either!  I guess you _could_ add an 
>>> of_machine_is_compatible() check against a list of compatible machines 
>>> in the driver but that feels a little gross.  Hopefully Rob or Grant 
>>> have a good alternative!
>>>
>>
>> What does cpufreq core do if multiple drivers are registered?
> current cpufreq core only support one cpufreq_driver. Others will fail
> except the first time.

Then whoever gets there first wins. Make your driver register late and
if someone doesn't want to use it they can register a custom driver earlier.

Rob

>> Perhaps a
>> ranking is needed and this would only get enabled if there are no other
>> drivers and other conditions like having the clock "cpu" present are met.
> We'd better keep cpufreq core simple. For this driver, register cpufreq_driver
> is the last thing after checking all conditions.
> 
>>
>> Rob
>>
>>>> Hi Grant & Rob,
>>>>
>>>> Could you comment?
>>>>
>>>>>
>>>>>> +- cpu-freqs : cpu frequency points it support
>>>>>> +- cpu-volts : cpu voltages required by the frequency point at the same index
>>>>>> +- trans-latency :  transition_latency
>>>>>> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
>>>>>> index e24a2a1..216eecd 100644
>>>>>> --- a/drivers/cpufreq/Kconfig
>>>>>> +++ b/drivers/cpufreq/Kconfig
>>>>>> @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
>>>>>>  
>>>>>>  	  If in doubt, say N.
>>>>>>  
>>>>>> +config GENERIC_CPUFREQ_DRIVER
>>>>>> +	bool "Generic cpufreq driver using clock/regulator/devicetree"
>>>>>> +	help
>>>>>> +	  This adds generic CPUFreq driver. It assumes all
>>>>>> +	  cores of the CPU share the same clock and voltage.
>>>>>> +
>>>>>> +	  If in doubt, say N.
>>>>>
>>>>> I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
>>>> right, Thanks. I can not check clk before generic clock framework
>>>> come in.
>>>> Added:
>>>> 	depends on OF && REGULATOR
>>>> 	select CPU_FREQ_TABLE
>>>
>>> You can still use HAVE_CLK.  That symbol has been around for ages and 
>>> any platform implementing the clk API should select it so it's fine to 
>>> depend on it even before there is a generic struct clk.
> You are right. Thanks.
> 
> Richard
>>>
>>> Jamie
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Arnd Bergmann]

On Tuesday 20 December 2011, Richard Zhao wrote:
> > >>>> +Generic cpufreq driver
> > >>>> +
> > >>>> +Required properties in /cpus/cpu@0:
> > >>>> +- compatible : "generic-cpufreq"
> > >>>
> > >>> I'm not convinced this is the best way to do this.  By requiring a 
> > >>> generic-cpufreq compatible string we're encoding Linux driver 
> > >>> information into the hardware description.  The only way I can see to 
> > >>> avoid this is to provide a generic_clk_cpufreq_init() function that 
> > >>> platforms can call in their machine init code to use the driver.
> > 
> > Agreed on the compatible string.
> Assume you reject to use compatible string.
> > It's putting Linux specifics into DT.
> > 
> > You could flip this around and have the module make a call into the
> > kernel to determine whether to initialize or not. Then platforms could
> > set a flag to indicate this.
> Could you make it more clear? kernel global variable, macro, or function?
> 
> - Following your idea, I think, we can add in driver/cpufreq/cpufreq.c:
> int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size);
> SoC code set the callback. If it's NULL, driver will exit. We can get rid
> of DT. It'll make cpufreq core dirty, but it's the only file built-in.
> 
> - Drop module support. SoC call generic_clk_cpufreq_init as Jamie said.

I think you don't actually need a "compatible" property here, as long as we
ensure that the "cpu-freqs", "cpu-volts" and "trans-latency" properties are
present in the cpu node if and only if the machine works with this driver
(why else would you put them there?).

CPUs are special in the device trees in a number of ways, so I think we
can treat this as a logical extension. This way, you can still make the
generic cpufreq driver a loadable module. You don't get module autoloading
with this structure, but that is already the case because the cpu nodes
in the device tree are not translated into linux devices.

	Arnd

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Arnd Bergmann]

On Tuesday 20 December 2011, Richard Zhao wrote:
> > >>>> +Generic cpufreq driver
> > >>>> +
> > >>>> +Required properties in /cpus/cpu at 0:
> > >>>> +- compatible : "generic-cpufreq"
> > >>>
> > >>> I'm not convinced this is the best way to do this.  By requiring a 
> > >>> generic-cpufreq compatible string we're encoding Linux driver 
> > >>> information into the hardware description.  The only way I can see to 
> > >>> avoid this is to provide a generic_clk_cpufreq_init() function that 
> > >>> platforms can call in their machine init code to use the driver.
> > 
> > Agreed on the compatible string.
> Assume you reject to use compatible string.
> > It's putting Linux specifics into DT.
> > 
> > You could flip this around and have the module make a call into the
> > kernel to determine whether to initialize or not. Then platforms could
> > set a flag to indicate this.
> Could you make it more clear? kernel global variable, macro, or function?
> 
> - Following your idea, I think, we can add in driver/cpufreq/cpufreq.c:
> int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size);
> SoC code set the callback. If it's NULL, driver will exit. We can get rid
> of DT. It'll make cpufreq core dirty, but it's the only file built-in.
> 
> - Drop module support. SoC call generic_clk_cpufreq_init as Jamie said.

I think you don't actually need a "compatible" property here, as long as we
ensure that the "cpu-freqs", "cpu-volts" and "trans-latency" properties are
present in the cpu node if and only if the machine works with this driver
(why else would you put them there?).

CPUs are special in the device trees in a number of ways, so I think we
can treat this as a logical extension. This way, you can still make the
generic cpufreq driver a loadable module. You don't get module autoloading
with this structure, but that is already the case because the cpu nodes
in the device tree are not translated into linux devices.

	Arnd

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

[-- Attachment #1.1: Type: text/plain, Size: 2199 bytes --]

在 2011-12-20 下午11:22，"Arnd Bergmann" <arnd@arndb.de>写道：
>
> On Tuesday 20 December 2011, Richard Zhao wrote:
> > > >>>> +Generic cpufreq driver
> > > >>>> +
> > > >>>> +Required properties in /cpus/cpu@0:
> > > >>>> +- compatible : "generic-cpufreq"
> > > >>>
> > > >>> I'm not convinced this is the best way to do this.  By requiring a
> > > >>> generic-cpufreq compatible string we're encoding Linux driver
> > > >>> information into the hardware description.  The only way I can
see to
> > > >>> avoid this is to provide a generic_clk_cpufreq_init() function
that
> > > >>> platforms can call in their machine init code to use the driver.
> > >
> > > Agreed on the compatible string.
> > Assume you reject to use compatible string.
> > > It's putting Linux specifics into DT.
> > >
> > > You could flip this around and have the module make a call into the
> > > kernel to determine whether to initialize or not. Then platforms could
> > > set a flag to indicate this.
> > Could you make it more clear? kernel global variable, macro, or
function?
> >
> > - Following your idea, I think, we can add in driver/cpufreq/cpufreq.c:
> > int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size);
> > SoC code set the callback. If it's NULL, driver will exit. We can get
rid
> > of DT. It'll make cpufreq core dirty, but it's the only file built-in.
> >
> > - Drop module support. SoC call generic_clk_cpufreq_init as Jamie said.
>
> I think you don't actually need a "compatible" property here, as long as
we
> ensure that the "cpu-freqs", "cpu-volts" and "trans-latency" properties
are
> present in the cpu node if and only if the machine works with this driver
> (why else would you put them there?).
It is more easy for me.
Rob, agree?

Richard
>
> CPUs are special in the device trees in a number of ways, so I think we
> can treat this as a logical extension. This way, you can still make the
> generic cpufreq driver a loadable module. You don't get module autoloading
> with this structure, but that is already the case because the cpu nodes
> in the device tree are not translated into linux devices.
>
>        Arnd

[-- Attachment #1.2: Type: text/html, Size: 2923 bytes --]

[-- Attachment #2: Type: text/plain, Size: 175 bytes --]

_______________________________________________
linaro-dev mailing list
linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

[-- Attachment #1.1: Type: text/plain, Size: 6264 bytes --]

在 2011-12-20 下午11:13，"Rob Herring" <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>写道：
>
> On 12/19/2011 07:59 PM, Richard Zhao wrote:
> > On Mon, Dec 19, 2011 at 09:00:44AM -0600, Rob Herring wrote:
> >> On 12/19/2011 08:39 AM, Jamie Iles wrote:
> >>> On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote:
> >>>> On Mon, Dec 19, 2011 at 10:05:12AM +0000, Jamie Iles wrote:
> >>>>> Hi Richard,
> >>>>>
> >>>>> On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> >>>>>> It support single core and multi-core ARM SoCs. But currently it
assume
> >>>>>> all cores share the same frequency and voltage.
> >>>>>>
> >>>>>> Signed-off-by: Richard Zhao <richard.zhao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>>>>> ---
> >>>>>>  .../devicetree/bindings/cpufreq/generic-cpufreq    |    7 +
> >>>>>>  drivers/cpufreq/Kconfig                            |    8 +
> >>>>>>  drivers/cpufreq/Makefile                           |    2 +
> >>>>>>  drivers/cpufreq/generic-cpufreq.c                  |  251
++++++++++++++++++++
> >>>>>>  4 files changed, 268 insertions(+), 0 deletions(-)
> >>>>>>  create mode 100644
Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> >>>>>>  create mode 100644 drivers/cpufreq/generic-cpufreq.c
> >>>>>>
> >>>>>> diff --git
a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> >>>>>> new file mode 100644
> >>>>>> index 0000000..15dd780
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> >>>>>> @@ -0,0 +1,7 @@
> >>>>>> +Generic cpufreq driver
> >>>>>> +
> >>>>>> +Required properties in /cpus/cpu@0:
> >>>>>> +- compatible : "generic-cpufreq"
> >>>>>
> >>>>> I'm not convinced this is the best way to do this.  By requiring a
> >>>>> generic-cpufreq compatible string we're encoding Linux driver
> >>>>> information into the hardware description.  The only way I can see
to
> >>>>> avoid this is to provide a generic_clk_cpufreq_init() function that
> >>>>> platforms can call in their machine init code to use the driver.
> >>
> >> Agreed on the compatible string.
> > Assume you reject to use compatible string.
> >> It's putting Linux specifics into DT.
> >>
> >> You could flip this around and have the module make a call into the
> >> kernel to determine whether to initialize or not. Then platforms could
> >> set a flag to indicate this.
> > Could you make it more clear? kernel global variable, macro, or
function?
>
> Any of those. Of course, direct access to variables across modules is
> discouraged, so it would probably be a function that checks a variable.
why not use function pointer? arch that don't use this driver do not have
to set it. if use function, everyone should define it.
>
> > - Following your idea, I think, we can add in driver/cpufreq/cpufreq.c:
> > int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size);
> > SoC code set the callback. If it's NULL, driver will exit. We can get
rid
> > of DT. It'll make cpufreq core dirty, but it's the only file built-in.
>
> But aren't you getting the operating points from the DT? Then you don't
> want to put this code into each platform.
the variable is mainly used to check whether some platform want  to use
this driver. getting ride of dt is side affect.
>
> >
> > - Drop module support. SoC call generic_clk_cpufreq_init as Jamie said.
> >
> >>
> >>>> It'll prevent the driver from being a kernel module.
> >>>
> >>> Hmm, that's not very nice either!  I guess you _could_ add an
> >>> of_machine_is_compatible() check against a list of compatible machines
> >>> in the driver but that feels a little gross.  Hopefully Rob or Grant
> >>> have a good alternative!
> >>>
> >>
> >> What does cpufreq core do if multiple drivers are registered?
> > current cpufreq core only support one cpufreq_driver. Others will fail
> > except the first time.
>
> Then whoever gets there first wins. Make your driver register late and
> if someone doesn't want to use it they can register a custom driver
earlier.
this driver did
>
> Rob
>
> >> Perhaps a
> >> ranking is needed and this would only get enabled if there are no other
> >> drivers and other conditions like having the clock "cpu" present are
met.
> > We'd better keep cpufreq core simple. For this driver, register
cpufreq_driver
> > is the last thing after checking all conditions.
> >
> >>
> >> Rob
> >>
> >>>> Hi Grant & Rob,
> >>>>
> >>>> Could you comment?
> >>>>
> >>>>>
> >>>>>> +- cpu-freqs : cpu frequency points it support
> >>>>>> +- cpu-volts : cpu voltages required by the frequency point at the
same index
> >>>>>> +- trans-latency :  transition_latency
> >>>>>> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> >>>>>> index e24a2a1..216eecd 100644
> >>>>>> --- a/drivers/cpufreq/Kconfig
> >>>>>> +++ b/drivers/cpufreq/Kconfig
> >>>>>> @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
> >>>>>>
> >>>>>>            If in doubt, say N.
> >>>>>>
> >>>>>> +config GENERIC_CPUFREQ_DRIVER
> >>>>>> +        bool "Generic cpufreq driver using
clock/regulator/devicetree"
> >>>>>> +        help
> >>>>>> +          This adds generic CPUFreq driver. It assumes all
> >>>>>> +          cores of the CPU share the same clock and voltage.
> >>>>>> +
> >>>>>> +          If in doubt, say N.
> >>>>>
> >>>>> I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
> >>>> right, Thanks. I can not check clk before generic clock framework
> >>>> come in.
> >>>> Added:
> >>>>    depends on OF && REGULATOR
> >>>>    select CPU_FREQ_TABLE
> >>>
> >>> You can still use HAVE_CLK.  That symbol has been around for ages and
> >>> any platform implementing the clk API should select it so it's fine to
> >>> depend on it even before there is a generic struct clk.
> > You are right. Thanks.
> >
> > Richard
> >>>
> >>> Jamie
> >>
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>
> >
>

[-- Attachment #1.2: Type: text/html, Size: 9118 bytes --]

[-- Attachment #2: Type: text/plain, Size: 175 bytes --]

_______________________________________________
linaro-dev mailing list
linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

On Tue, Dec 20, 2011 at 02:59:04PM +0000, Mark Brown wrote:
> On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> > It support single core and multi-core ARM SoCs. But currently it assume
> > all cores share the same frequency and voltage.
> 
> My comments on the previous version of the patch still apply:
> 
>  - The voltage ranges being set need to be specified as ranges.
cpu normally need strict voltages. and only proved operating opints
are allowed to set in dts. If the voltage changes slightly especially
for high frequency, it's easy to cause unstable.
>  - Frequencies that can't be supported due to limitations of the
>    available supplies shouldn't be exposed to users.
As I said, only proved operation points are allowed.
>  - The driver needs to handle errors.
Yes.
> 
> > +Required properties in /cpus/cpu@0:
> > +- compatible : "generic-cpufreq"
> > +- cpu-freqs : cpu frequency points it support
> > +- cpu-volts : cpu voltages required by the frequency point at the same index
> > +- trans-latency :  transition_latency
> 
> You need to define units for all of these, and for the transition
> latency you need to be clear about what's being measured (it looks like
> the CPU time only, not any voltage ramping).
right.
> 
> You also need to define how the core supplies get looked up.
It's pure software. platform uses this driver have to define "cpu" consumer.
> 
> > +	/* Manual states, that PLL stabilizes in two CLK32 periods */
> > +	policy->cpuinfo.transition_latency = trans_latency;
> 
> I guess this comment is a cut'n'paste error.
right, thanks.
> 
> > +
> > +	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> > +
> > +	if (ret < 0) {
> > +		pr_err("%s: invalid frequency table for cpu %d\n",
> > +		       __func__, policy->cpu);
> 
> You should define pr_fmt to always include __func__ in the messages
> rather than open coding - ensures consistency and is less noisy in the
> code.
I'll check it.
> 
> > +	pr_info("Generic CPU frequency driver\n");
> 
> This seems noisy...
Why? Do you think only errors and warnings can print out?

Thanks
Richard

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

On Tue, Dec 20, 2011 at 02:59:04PM +0000, Mark Brown wrote:
> On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> > It support single core and multi-core ARM SoCs. But currently it assume
> > all cores share the same frequency and voltage.
> 
> My comments on the previous version of the patch still apply:
> 
>  - The voltage ranges being set need to be specified as ranges.
cpu normally need strict voltages. and only proved operating opints
are allowed to set in dts. If the voltage changes slightly especially
for high frequency, it's easy to cause unstable.
>  - Frequencies that can't be supported due to limitations of the
>    available supplies shouldn't be exposed to users.
As I said, only proved operation points are allowed.
>  - The driver needs to handle errors.
Yes.
> 
> > +Required properties in /cpus/cpu at 0:
> > +- compatible : "generic-cpufreq"
> > +- cpu-freqs : cpu frequency points it support
> > +- cpu-volts : cpu voltages required by the frequency point at the same index
> > +- trans-latency :  transition_latency
> 
> You need to define units for all of these, and for the transition
> latency you need to be clear about what's being measured (it looks like
> the CPU time only, not any voltage ramping).
right.
> 
> You also need to define how the core supplies get looked up.
It's pure software. platform uses this driver have to define "cpu" consumer.
> 
> > +	/* Manual states, that PLL stabilizes in two CLK32 periods */
> > +	policy->cpuinfo.transition_latency = trans_latency;
> 
> I guess this comment is a cut'n'paste error.
right, thanks.
> 
> > +
> > +	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> > +
> > +	if (ret < 0) {
> > +		pr_err("%s: invalid frequency table for cpu %d\n",
> > +		       __func__, policy->cpu);
> 
> You should define pr_fmt to always include __func__ in the messages
> rather than open coding - ensures consistency and is less noisy in the
> code.
I'll check it.
> 
> > +	pr_info("Generic CPU frequency driver\n");
> 
> This seems noisy...
Why? Do you think only errors and warnings can print out?

Thanks
Richard

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Mark Brown]

On Wed, Dec 21, 2011 at 07:27:03AM +0800, Richard Zhao wrote:
> On Tue, Dec 20, 2011 at 02:59:04PM +0000, Mark Brown wrote:

> > My comments on the previous version of the patch still apply:

> >  - The voltage ranges being set need to be specified as ranges.

> cpu normally need strict voltages. and only proved operating opints
> are allowed to set in dts. If the voltage changes slightly especially
> for high frequency, it's easy to cause unstable.

Clearly there will be limits which get more and more restrictive as the
frequencies get higher but there will always be at least some play in
the numbers as one must at a minimum specify tolerance ranges, and at
lower frequencies the ranges specified will typically get compartively
large.

Note also that not all hardware specifies things in terms of a fixed set
of operating points, sometimes only the minimum voltage specification is
varied with frequency or sometimes you see maximum and minimum stepping
independently.

Further note that if all hardware really does have as tight a set of
requirements as you suggest then the regulator support in the driver
needs to be non-optional otherwise a board without software regulator
control might drop the frequency without also dropping the voltage.

> >  - Frequencies that can't be supported due to limitations of the
> >    available supplies shouldn't be exposed to users.

> As I said, only proved operation points are allowed.

This statement appears to be unrelated to the comment you're replying
to.

> > You also need to define how the core supplies get looked up.

> It's pure software. platform uses this driver have to define "cpu" consumer.

You still need to define this in the binding.

> > > +	pr_info("Generic CPU frequency driver\n");

> > This seems noisy...

> Why? Do you think only errors and warnings can print out?

Yes.

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Mark Brown]

On Wed, Dec 21, 2011 at 07:27:03AM +0800, Richard Zhao wrote:
> On Tue, Dec 20, 2011 at 02:59:04PM +0000, Mark Brown wrote:

> > My comments on the previous version of the patch still apply:

> >  - The voltage ranges being set need to be specified as ranges.

> cpu normally need strict voltages. and only proved operating opints
> are allowed to set in dts. If the voltage changes slightly especially
> for high frequency, it's easy to cause unstable.

Clearly there will be limits which get more and more restrictive as the
frequencies get higher but there will always be at least some play in
the numbers as one must at a minimum specify tolerance ranges, and at
lower frequencies the ranges specified will typically get compartively
large.

Note also that not all hardware specifies things in terms of a fixed set
of operating points, sometimes only the minimum voltage specification is
varied with frequency or sometimes you see maximum and minimum stepping
independently.

Further note that if all hardware really does have as tight a set of
requirements as you suggest then the regulator support in the driver
needs to be non-optional otherwise a board without software regulator
control might drop the frequency without also dropping the voltage.

> >  - Frequencies that can't be supported due to limitations of the
> >    available supplies shouldn't be exposed to users.

> As I said, only proved operation points are allowed.

This statement appears to be unrelated to the comment you're replying
to.

> > You also need to define how the core supplies get looked up.

> It's pure software. platform uses this driver have to define "cpu" consumer.

You still need to define this in the binding.

> > > +	pr_info("Generic CPU frequency driver\n");

> > This seems noisy...

> Why? Do you think only errors and warnings can print out?

Yes.

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

Hi Mark,

On Tue, Dec 20, 2011 at 11:48:45PM +0000, Mark Brown wrote:
> On Wed, Dec 21, 2011 at 07:27:03AM +0800, Richard Zhao wrote:
> > On Tue, Dec 20, 2011 at 02:59:04PM +0000, Mark Brown wrote:
> 
> > > My comments on the previous version of the patch still apply:
> 
> > >  - The voltage ranges being set need to be specified as ranges.
> 
> > cpu normally need strict voltages. and only proved operating opints
> > are allowed to set in dts. If the voltage changes slightly especially
> > for high frequency, it's easy to cause unstable.
> 
> Clearly there will be limits which get more and more restrictive as the
> frequencies get higher but there will always be at least some play in
> the numbers as one must at a minimum specify tolerance ranges, and at
> lower frequencies the ranges specified will typically get compartively
> large.
hmm, reasonable. I'll add it in dts. Thanks.
> 
> Note also that not all hardware specifies things in terms of a fixed set
> of operating points, sometimes only the minimum voltage specification is
> varied with frequency or sometimes you see maximum and minimum stepping
> independently.
cpus that don't use freq table is out of scope of this driver.
> 
> Further note that if all hardware really does have as tight a set of
> requirements as you suggest then the regulator support in the driver
> needs to be non-optional otherwise a board without software regulator
> control might drop the frequency without also dropping the voltage.
It's ok to only adjuct freq without changes voltage. You can find many
arm soc cpufreq drivers don't change voltage.
If dts specify voltage but don't have such regulator. I'll assume it
always runs on the initial volatage (highest for most cases).
> 
> > >  - Frequencies that can't be supported due to limitations of the
> > >    available supplies shouldn't be exposed to users.
> 
> > As I said, only proved operation points are allowed.
> 
> This statement appears to be unrelated to the comment you're replying
> to.
I meant the exact voltage can always successfull set. Anyway, I'll add
regulator_set_voltage return value checking.
> 
> > > You also need to define how the core supplies get looked up.
> 
> > It's pure software. platform uses this driver have to define "cpu" consumer.
> 
> You still need to define this in the binding.
You mean regulator DT binding? already in ? I'll check it.
> 
> > > > +	pr_info("Generic CPU frequency driver\n");
> 
> > > This seems noisy...
> 
> > Why? Do you think only errors and warnings can print out?
> 
> Yes.

Maybe I can remove it. But I'd probably add freq table dump. It's more important.
Agree?

I checked pr_fmt. Thanks very much. I'll use below and remove __func_.
#define pr_fmt(fmt)             KBUILD_MODNAME ": " fmt


Thanks
Richard
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

Hi Mark,

On Tue, Dec 20, 2011 at 11:48:45PM +0000, Mark Brown wrote:
> On Wed, Dec 21, 2011 at 07:27:03AM +0800, Richard Zhao wrote:
> > On Tue, Dec 20, 2011 at 02:59:04PM +0000, Mark Brown wrote:
> 
> > > My comments on the previous version of the patch still apply:
> 
> > >  - The voltage ranges being set need to be specified as ranges.
> 
> > cpu normally need strict voltages. and only proved operating opints
> > are allowed to set in dts. If the voltage changes slightly especially
> > for high frequency, it's easy to cause unstable.
> 
> Clearly there will be limits which get more and more restrictive as the
> frequencies get higher but there will always be at least some play in
> the numbers as one must at a minimum specify tolerance ranges, and at
> lower frequencies the ranges specified will typically get compartively
> large.
hmm, reasonable. I'll add it in dts. Thanks.
> 
> Note also that not all hardware specifies things in terms of a fixed set
> of operating points, sometimes only the minimum voltage specification is
> varied with frequency or sometimes you see maximum and minimum stepping
> independently.
cpus that don't use freq table is out of scope of this driver.
> 
> Further note that if all hardware really does have as tight a set of
> requirements as you suggest then the regulator support in the driver
> needs to be non-optional otherwise a board without software regulator
> control might drop the frequency without also dropping the voltage.
It's ok to only adjuct freq without changes voltage. You can find many
arm soc cpufreq drivers don't change voltage.
If dts specify voltage but don't have such regulator. I'll assume it
always runs on the initial volatage (highest for most cases).
> 
> > >  - Frequencies that can't be supported due to limitations of the
> > >    available supplies shouldn't be exposed to users.
> 
> > As I said, only proved operation points are allowed.
> 
> This statement appears to be unrelated to the comment you're replying
> to.
I meant the exact voltage can always successfull set. Anyway, I'll add
regulator_set_voltage return value checking.
> 
> > > You also need to define how the core supplies get looked up.
> 
> > It's pure software. platform uses this driver have to define "cpu" consumer.
> 
> You still need to define this in the binding.
You mean regulator DT binding? already in ? I'll check it.
> 
> > > > +	pr_info("Generic CPU frequency driver\n");
> 
> > > This seems noisy...
> 
> > Why? Do you think only errors and warnings can print out?
> 
> Yes.

Maybe I can remove it. But I'd probably add freq table dump. It's more important.
Agree?

I checked pr_fmt. Thanks very much. I'll use below and remove __func_.
#define pr_fmt(fmt)             KBUILD_MODNAME ": " fmt


Thanks
Richard
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Mark Brown]

On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:
> On Tue, Dec 20, 2011 at 11:48:45PM +0000, Mark Brown wrote:

> > Note also that not all hardware specifies things in terms of a fixed set
> > of operating points, sometimes only the minimum voltage specification is
> > varied with frequency or sometimes you see maximum and minimum stepping
> > independently.

> cpus that don't use freq table is out of scope of this driver.

That's not the point - the point is that you may do something like
specify a defined set of frequencies and just drop the minimum supported
voltage without altering the maximum supported voltage as the frequency
gets lower.

> > Further note that if all hardware really does have as tight a set of
> > requirements as you suggest then the regulator support in the driver
> > needs to be non-optional otherwise a board without software regulator
> > control might drop the frequency without also dropping the voltage.

> It's ok to only adjuct freq without changes voltage. You can find many
> arm soc cpufreq drivers don't change voltage.
> If dts specify voltage but don't have such regulator. I'll assume it
> always runs on the initial volatage (highest for most cases).

My point exactly; such devices are examples of the policy outlined above
where only the minimum voltage changes with frequency and the maximum
voltage is constant.  The cpufreq driver would lower the supported
voltage when possible but wouldn't actually care if the voltage changes.

> > > >  - Frequencies that can't be supported due to limitations of the
> > > >    available supplies shouldn't be exposed to users.

> > > As I said, only proved operation points are allowed.

> > This statement appears to be unrelated to the comment you're replying
> > to.

> I meant the exact voltage can always successfull set. Anyway, I'll add

This is just not the case.  Regulators don't offer a continuous range of
voltages, they offer steps of varying sizes which may miss setting some
voltages, and board designers may choose not to support some of the
voltage range.

> regulator_set_voltage return value checking.

While this is important the driver should also be enumerating the
supported voltages at probe time and eliminating unsupportable settings
so that governors aren't constantly trying to set things that can never
be supported.  The s3c64xx cpufreq driver provides an implementation of
this.

> Maybe I can remove it. But I'd probably add freq table dump. It's more important.
> Agree?

That seems like something the core should be doing if

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Mark Brown]

On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:
> On Tue, Dec 20, 2011 at 11:48:45PM +0000, Mark Brown wrote:

> > Note also that not all hardware specifies things in terms of a fixed set
> > of operating points, sometimes only the minimum voltage specification is
> > varied with frequency or sometimes you see maximum and minimum stepping
> > independently.

> cpus that don't use freq table is out of scope of this driver.

That's not the point - the point is that you may do something like
specify a defined set of frequencies and just drop the minimum supported
voltage without altering the maximum supported voltage as the frequency
gets lower.

> > Further note that if all hardware really does have as tight a set of
> > requirements as you suggest then the regulator support in the driver
> > needs to be non-optional otherwise a board without software regulator
> > control might drop the frequency without also dropping the voltage.

> It's ok to only adjuct freq without changes voltage. You can find many
> arm soc cpufreq drivers don't change voltage.
> If dts specify voltage but don't have such regulator. I'll assume it
> always runs on the initial volatage (highest for most cases).

My point exactly; such devices are examples of the policy outlined above
where only the minimum voltage changes with frequency and the maximum
voltage is constant.  The cpufreq driver would lower the supported
voltage when possible but wouldn't actually care if the voltage changes.

> > > >  - Frequencies that can't be supported due to limitations of the
> > > >    available supplies shouldn't be exposed to users.

> > > As I said, only proved operation points are allowed.

> > This statement appears to be unrelated to the comment you're replying
> > to.

> I meant the exact voltage can always successfull set. Anyway, I'll add

This is just not the case.  Regulators don't offer a continuous range of
voltages, they offer steps of varying sizes which may miss setting some
voltages, and board designers may choose not to support some of the
voltage range.

> regulator_set_voltage return value checking.

While this is important the driver should also be enumerating the
supported voltages at probe time and eliminating unsupportable settings
so that governors aren't constantly trying to set things that can never
be supported.  The s3c64xx cpufreq driver provides an implementation of
this.

> Maybe I can remove it. But I'd probably add freq table dump. It's more important.
> Agree?

That seems like something the core should be doing if

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

On Wed, Dec 21, 2011 at 01:32:21AM +0000, Mark Brown wrote:
> On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:
> > On Tue, Dec 20, 2011 at 11:48:45PM +0000, Mark Brown wrote:
> 
> > > Note also that not all hardware specifies things in terms of a fixed set
> > > of operating points, sometimes only the minimum voltage specification is
> > > varied with frequency or sometimes you see maximum and minimum stepping
> > > independently.
> 
> > cpus that don't use freq table is out of scope of this driver.
> 
> That's not the point - the point is that you may do something like
> specify a defined set of frequencies and just drop the minimum supported
> voltage without altering the maximum supported voltage as the frequency
> gets lower.
What do you mean by "drop"?
cpu-volts = <
	/* min  max */
	1100000 1200000 /* 1G HZ */
	1000000 1200000 /* 800M HZ */
	900000  1200000 /* 600M HZ */
	>;
The above sample dts could meet your point?
	
> 
> > > Further note that if all hardware really does have as tight a set of
> > > requirements as you suggest then the regulator support in the driver
> > > needs to be non-optional otherwise a board without software regulator
> > > control might drop the frequency without also dropping the voltage.
> 
> > It's ok to only adjuct freq without changes voltage. You can find many
> > arm soc cpufreq drivers don't change voltage.
> > If dts specify voltage but don't have such regulator. I'll assume it
> > always runs on the initial volatage (highest for most cases).
> 
> My point exactly; such devices are examples of the policy outlined above
> where only the minimum voltage changes with frequency and the maximum
> voltage is constant.  The cpufreq driver would lower the supported
> voltage when possible but wouldn't actually care if the voltage changes.
accepted seting voltage range. I guess the above sample dts code meet your
point.
> 
> > > > >  - Frequencies that can't be supported due to limitations of the
> > > > >    available supplies shouldn't be exposed to users.
> 
> > > > As I said, only proved operation points are allowed.
> 
> > > This statement appears to be unrelated to the comment you're replying
> > > to.
> 
> > I meant the exact voltage can always successfull set. Anyway, I'll add
> 
> This is just not the case.  Regulators don't offer a continuous range of
> voltages, they offer steps of varying sizes which may miss setting some
> voltages, and board designers may choose not to support some of the
> voltage range.
> 
> > regulator_set_voltage return value checking.
> 
> While this is important the driver should also be enumerating the
> supported voltages at probe time and eliminating unsupportable settings
> so that governors aren't constantly trying to set things that can never
> be supported.  The s3c64xx cpufreq driver provides an implementation of
> this.
I'll use regulator_is_supported_voltage pre-check the cpu-volts.
For clock, conditions ((clk_round_rate(cpu-freq)/1000) * 1000 == cpu-freq)
seems too strict. Because cpu clock is SoC dependent, I'll not add the check.
> 
> > Maybe I can remove it. But I'd probably add freq table dump. It's more important.
> > Agree?
> 
> That seems like something the core should be doing if
hmm, cpu table dumps it with pr_debug.

Thanks
Richard
> 

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

On Wed, Dec 21, 2011 at 01:32:21AM +0000, Mark Brown wrote:
> On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:
> > On Tue, Dec 20, 2011 at 11:48:45PM +0000, Mark Brown wrote:
> 
> > > Note also that not all hardware specifies things in terms of a fixed set
> > > of operating points, sometimes only the minimum voltage specification is
> > > varied with frequency or sometimes you see maximum and minimum stepping
> > > independently.
> 
> > cpus that don't use freq table is out of scope of this driver.
> 
> That's not the point - the point is that you may do something like
> specify a defined set of frequencies and just drop the minimum supported
> voltage without altering the maximum supported voltage as the frequency
> gets lower.
What do you mean by "drop"?
cpu-volts = <
	/* min  max */
	1100000 1200000 /* 1G HZ */
	1000000 1200000 /* 800M HZ */
	900000  1200000 /* 600M HZ */
	>;
The above sample dts could meet your point?
	
> 
> > > Further note that if all hardware really does have as tight a set of
> > > requirements as you suggest then the regulator support in the driver
> > > needs to be non-optional otherwise a board without software regulator
> > > control might drop the frequency without also dropping the voltage.
> 
> > It's ok to only adjuct freq without changes voltage. You can find many
> > arm soc cpufreq drivers don't change voltage.
> > If dts specify voltage but don't have such regulator. I'll assume it
> > always runs on the initial volatage (highest for most cases).
> 
> My point exactly; such devices are examples of the policy outlined above
> where only the minimum voltage changes with frequency and the maximum
> voltage is constant.  The cpufreq driver would lower the supported
> voltage when possible but wouldn't actually care if the voltage changes.
accepted seting voltage range. I guess the above sample dts code meet your
point.
> 
> > > > >  - Frequencies that can't be supported due to limitations of the
> > > > >    available supplies shouldn't be exposed to users.
> 
> > > > As I said, only proved operation points are allowed.
> 
> > > This statement appears to be unrelated to the comment you're replying
> > > to.
> 
> > I meant the exact voltage can always successfull set. Anyway, I'll add
> 
> This is just not the case.  Regulators don't offer a continuous range of
> voltages, they offer steps of varying sizes which may miss setting some
> voltages, and board designers may choose not to support some of the
> voltage range.
> 
> > regulator_set_voltage return value checking.
> 
> While this is important the driver should also be enumerating the
> supported voltages at probe time and eliminating unsupportable settings
> so that governors aren't constantly trying to set things that can never
> be supported.  The s3c64xx cpufreq driver provides an implementation of
> this.
I'll use regulator_is_supported_voltage pre-check the cpu-volts.
For clock, conditions ((clk_round_rate(cpu-freq)/1000) * 1000 == cpu-freq)
seems too strict. Because cpu clock is SoC dependent, I'll not add the check.
> 
> > Maybe I can remove it. But I'd probably add freq table dump. It's more important.
> > Agree?
> 
> That seems like something the core should be doing if
hmm, cpu table dumps it with pr_debug.

Thanks
Richard
> 

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Mark Brown]

On Wed, Dec 21, 2011 at 10:24:53AM +0800, Richard Zhao wrote:
> On Wed, Dec 21, 2011 at 01:32:21AM +0000, Mark Brown wrote:

> > That's not the point - the point is that you may do something like
> > specify a defined set of frequencies and just drop the minimum supported
> > voltage without altering the maximum supported voltage as the frequency
> > gets lower.

> What do you mean by "drop"?

Drop is a synonym for lower in this context.

> cpu-volts = <
> 	/* min  max */
> 	1100000 1200000 /* 1G HZ */
> 	1000000 1200000 /* 800M HZ */
> 	900000  1200000 /* 600M HZ */
> 	>;
> The above sample dts could meet your point?

Yes.

> > While this is important the driver should also be enumerating the
> > supported voltages at probe time and eliminating unsupportable settings
> > so that governors aren't constantly trying to set things that can never
> > be supported.  The s3c64xx cpufreq driver provides an implementation of
> > this.

> I'll use regulator_is_supported_voltage pre-check the cpu-volts.
> For clock, conditions ((clk_round_rate(cpu-freq)/1000) * 1000 == cpu-freq)
> seems too strict. Because cpu clock is SoC dependent, I'll not add the check.

The issue that was there for is that there are multiple runtime
detectable variant clocking configurations which couldn't be switched
between so the driver needs to select only the rates that can be reached
from the current configuration.  I'd imagine that might be an issue
elsewhere.

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Mark Brown]

On Wed, Dec 21, 2011 at 10:24:53AM +0800, Richard Zhao wrote:
> On Wed, Dec 21, 2011 at 01:32:21AM +0000, Mark Brown wrote:

> > That's not the point - the point is that you may do something like
> > specify a defined set of frequencies and just drop the minimum supported
> > voltage without altering the maximum supported voltage as the frequency
> > gets lower.

> What do you mean by "drop"?

Drop is a synonym for lower in this context.

> cpu-volts = <
> 	/* min  max */
> 	1100000 1200000 /* 1G HZ */
> 	1000000 1200000 /* 800M HZ */
> 	900000  1200000 /* 600M HZ */
> 	>;
> The above sample dts could meet your point?

Yes.

> > While this is important the driver should also be enumerating the
> > supported voltages at probe time and eliminating unsupportable settings
> > so that governors aren't constantly trying to set things that can never
> > be supported.  The s3c64xx cpufreq driver provides an implementation of
> > this.

> I'll use regulator_is_supported_voltage pre-check the cpu-volts.
> For clock, conditions ((clk_round_rate(cpu-freq)/1000) * 1000 == cpu-freq)
> seems too strict. Because cpu clock is SoC dependent, I'll not add the check.

The issue that was there for is that there are multiple runtime
detectable variant clocking configurations which couldn't be switched
between so the driver needs to select only the rates that can be reached
from the current configuration.  I'd imagine that might be an issue
elsewhere.

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

On Wed, Dec 21, 2011 at 02:33:36AM +0000, Mark Brown wrote:
> On Wed, Dec 21, 2011 at 10:24:53AM +0800, Richard Zhao wrote:
> > On Wed, Dec 21, 2011 at 01:32:21AM +0000, Mark Brown wrote:
> 
> > > That's not the point - the point is that you may do something like
> > > specify a defined set of frequencies and just drop the minimum supported
> > > voltage without altering the maximum supported voltage as the frequency
> > > gets lower.
> 
> > What do you mean by "drop"?
> 
> Drop is a synonym for lower in this context.
> 
> > cpu-volts = <
> > 	/* min  max */
> > 	1100000 1200000 /* 1G HZ */
> > 	1000000 1200000 /* 800M HZ */
> > 	900000  1200000 /* 600M HZ */
> > 	>;
> > The above sample dts could meet your point?
> 
> Yes.
> 
> > > While this is important the driver should also be enumerating the
> > > supported voltages at probe time and eliminating unsupportable settings
> > > so that governors aren't constantly trying to set things that can never
> > > be supported.  The s3c64xx cpufreq driver provides an implementation of
> > > this.
> 
> > I'll use regulator_is_supported_voltage pre-check the cpu-volts.
> > For clock, conditions ((clk_round_rate(cpu-freq)/1000) * 1000 == cpu-freq)
> > seems too strict. Because cpu clock is SoC dependent, I'll not add the check.
> 
> The issue that was there for is that there are multiple runtime
> detectable variant clocking configurations which couldn't be switched
> between so the driver needs to select only the rates that can be reached
> from the current configuration.  I'd imagine that might be an issue
> elsewhere.
one case is that, cpu freq is 800M, clock may only reach 799M. I'd rather let
clock code to decide how to handle 800M request. That's why I said
the condition check is too strict.

Thanks
Richard
> 

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

On Wed, Dec 21, 2011 at 02:33:36AM +0000, Mark Brown wrote:
> On Wed, Dec 21, 2011 at 10:24:53AM +0800, Richard Zhao wrote:
> > On Wed, Dec 21, 2011 at 01:32:21AM +0000, Mark Brown wrote:
> 
> > > That's not the point - the point is that you may do something like
> > > specify a defined set of frequencies and just drop the minimum supported
> > > voltage without altering the maximum supported voltage as the frequency
> > > gets lower.
> 
> > What do you mean by "drop"?
> 
> Drop is a synonym for lower in this context.
> 
> > cpu-volts = <
> > 	/* min  max */
> > 	1100000 1200000 /* 1G HZ */
> > 	1000000 1200000 /* 800M HZ */
> > 	900000  1200000 /* 600M HZ */
> > 	>;
> > The above sample dts could meet your point?
> 
> Yes.
> 
> > > While this is important the driver should also be enumerating the
> > > supported voltages at probe time and eliminating unsupportable settings
> > > so that governors aren't constantly trying to set things that can never
> > > be supported.  The s3c64xx cpufreq driver provides an implementation of
> > > this.
> 
> > I'll use regulator_is_supported_voltage pre-check the cpu-volts.
> > For clock, conditions ((clk_round_rate(cpu-freq)/1000) * 1000 == cpu-freq)
> > seems too strict. Because cpu clock is SoC dependent, I'll not add the check.
> 
> The issue that was there for is that there are multiple runtime
> detectable variant clocking configurations which couldn't be switched
> between so the driver needs to select only the rates that can be reached
> from the current configuration.  I'd imagine that might be an issue
> elsewhere.
one case is that, cpu freq is 800M, clock may only reach 799M. I'd rather let
clock code to decide how to handle 800M request. That's why I said
the condition check is too strict.

Thanks
Richard
> 

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:
> Hi Mark,
> 
> On Tue, Dec 20, 2011 at 11:48:45PM +0000, Mark Brown wrote:
> > On Wed, Dec 21, 2011 at 07:27:03AM +0800, Richard Zhao wrote:
> > > On Tue, Dec 20, 2011 at 02:59:04PM +0000, Mark Brown wrote:
> > 
> > > > My comments on the previous version of the patch still apply:
> > 
> > > >  - The voltage ranges being set need to be specified as ranges.
> > 
> > > cpu normally need strict voltages. and only proved operating opints
> > > are allowed to set in dts. If the voltage changes slightly especially
> > > for high frequency, it's easy to cause unstable.
> > 
> > Clearly there will be limits which get more and more restrictive as the
> > frequencies get higher but there will always be at least some play in
> > the numbers as one must at a minimum specify tolerance ranges, and at
> > lower frequencies the ranges specified will typically get compartively
> > large.
> hmm, reasonable. I'll add it in dts. Thanks.
> > 
> > Note also that not all hardware specifies things in terms of a fixed set
> > of operating points, sometimes only the minimum voltage specification is
> > varied with frequency or sometimes you see maximum and minimum stepping
> > independently.
> cpus that don't use freq table is out of scope of this driver.
> > 
> > Further note that if all hardware really does have as tight a set of
> > requirements as you suggest then the regulator support in the driver
> > needs to be non-optional otherwise a board without software regulator
> > control might drop the frequency without also dropping the voltage.
> It's ok to only adjuct freq without changes voltage. You can find many
> arm soc cpufreq drivers don't change voltage.
> If dts specify voltage but don't have such regulator. I'll assume it
> always runs on the initial volatage (highest for most cases).
> > 
> > > >  - Frequencies that can't be supported due to limitations of the
> > > >    available supplies shouldn't be exposed to users.
> > 
> > > As I said, only proved operation points are allowed.
> > 
> > This statement appears to be unrelated to the comment you're replying
> > to.
> I meant the exact voltage can always successfull set. Anyway, I'll add
> regulator_set_voltage return value checking.
> > 
> > > > You also need to define how the core supplies get looked up.
> > 
> > > It's pure software. platform uses this driver have to define "cpu" consumer.
> > 
> > You still need to define this in the binding.
> You mean regulator DT binding? already in ? I'll check it.
Mark, cpu node is not a struct device, sys_device instead. I can not find
regulator via device/dt node. Can I still use the string to get regulator
after converting to DT?

And regulator binding is not on cpufreq git tree.

Thanks
Richard
> > 
> > > > > +	pr_info("Generic CPU frequency driver\n");
> > 
> > > > This seems noisy...
> > 
> > > Why? Do you think only errors and warnings can print out?
> > 
> > Yes.
> 
> Maybe I can remove it. But I'd probably add freq table dump. It's more important.
> Agree?
> 
> I checked pr_fmt. Thanks very much. I'll use below and remove __func_.
> #define pr_fmt(fmt)             KBUILD_MODNAME ": " fmt
> 
> 
> Thanks
> Richard
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:
> Hi Mark,
> 
> On Tue, Dec 20, 2011 at 11:48:45PM +0000, Mark Brown wrote:
> > On Wed, Dec 21, 2011 at 07:27:03AM +0800, Richard Zhao wrote:
> > > On Tue, Dec 20, 2011 at 02:59:04PM +0000, Mark Brown wrote:
> > 
> > > > My comments on the previous version of the patch still apply:
> > 
> > > >  - The voltage ranges being set need to be specified as ranges.
> > 
> > > cpu normally need strict voltages. and only proved operating opints
> > > are allowed to set in dts. If the voltage changes slightly especially
> > > for high frequency, it's easy to cause unstable.
> > 
> > Clearly there will be limits which get more and more restrictive as the
> > frequencies get higher but there will always be at least some play in
> > the numbers as one must at a minimum specify tolerance ranges, and at
> > lower frequencies the ranges specified will typically get compartively
> > large.
> hmm, reasonable. I'll add it in dts. Thanks.
> > 
> > Note also that not all hardware specifies things in terms of a fixed set
> > of operating points, sometimes only the minimum voltage specification is
> > varied with frequency or sometimes you see maximum and minimum stepping
> > independently.
> cpus that don't use freq table is out of scope of this driver.
> > 
> > Further note that if all hardware really does have as tight a set of
> > requirements as you suggest then the regulator support in the driver
> > needs to be non-optional otherwise a board without software regulator
> > control might drop the frequency without also dropping the voltage.
> It's ok to only adjuct freq without changes voltage. You can find many
> arm soc cpufreq drivers don't change voltage.
> If dts specify voltage but don't have such regulator. I'll assume it
> always runs on the initial volatage (highest for most cases).
> > 
> > > >  - Frequencies that can't be supported due to limitations of the
> > > >    available supplies shouldn't be exposed to users.
> > 
> > > As I said, only proved operation points are allowed.
> > 
> > This statement appears to be unrelated to the comment you're replying
> > to.
> I meant the exact voltage can always successfull set. Anyway, I'll add
> regulator_set_voltage return value checking.
> > 
> > > > You also need to define how the core supplies get looked up.
> > 
> > > It's pure software. platform uses this driver have to define "cpu" consumer.
> > 
> > You still need to define this in the binding.
> You mean regulator DT binding? already in ? I'll check it.
Mark, cpu node is not a struct device, sys_device instead. I can not find
regulator via device/dt node. Can I still use the string to get regulator
after converting to DT?

And regulator binding is not on cpufreq git tree.

Thanks
Richard
> > 
> > > > > +	pr_info("Generic CPU frequency driver\n");
> > 
> > > > This seems noisy...
> > 
> > > Why? Do you think only errors and warnings can print out?
> > 
> > Yes.
> 
> Maybe I can remove it. But I'd probably add freq table dump. It's more important.
> Agree?
> 
> I checked pr_fmt. Thanks very much. I'll use below and remove __func_.
> #define pr_fmt(fmt)             KBUILD_MODNAME ": " fmt
> 
> 
> Thanks
> Richard
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Arnd Bergmann]

On Wednesday 21 December 2011, Richard Zhao wrote:
> On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:

> > > > > You also need to define how the core supplies get looked up.
> > > 
> > > > It's pure software. platform uses this driver have to define "cpu" consumer.
> > > 
> > > You still need to define this in the binding.
> > You mean regulator DT binding? already in ? I'll check it.
> Mark, cpu node is not a struct device, sys_device instead. I can not find
> regulator via device/dt node. Can I still use the string to get regulator
> after converting to DT?

I believe Kay and Greg have the plan to unify "class" and "bus" in sysfs, which
implies turning sys_device into a derived class of device instead of kobject.
If that understanding is correct, we might as well do that now so we can
attach a device_node to a sys_device.

Kay, does this make sense?

	Arnd

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Arnd Bergmann]

On Wednesday 21 December 2011, Richard Zhao wrote:
> On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:

> > > > > You also need to define how the core supplies get looked up.
> > > 
> > > > It's pure software. platform uses this driver have to define "cpu" consumer.
> > > 
> > > You still need to define this in the binding.
> > You mean regulator DT binding? already in ? I'll check it.
> Mark, cpu node is not a struct device, sys_device instead. I can not find
> regulator via device/dt node. Can I still use the string to get regulator
> after converting to DT?

I believe Kay and Greg have the plan to unify "class" and "bus" in sysfs, which
implies turning sys_device into a derived class of device instead of kobject.
If that understanding is correct, we might as well do that now so we can
attach a device_node to a sys_device.

Kay, does this make sense?

	Arnd

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Kay Sievers]

On Wed, Dec 21, 2011 at 10:43, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 21 December 2011, Richard Zhao wrote:
>> On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:
>
>> > > > > You also need to define how the core supplies get looked up.
>> > >
>> > > > It's pure software. platform uses this driver have to define "cpu" consumer.
>> > >
>> > > You still need to define this in the binding.
>> > You mean regulator DT binding? already in ? I'll check it.
>> Mark, cpu node is not a struct device, sys_device instead. I can not find
>> regulator via device/dt node. Can I still use the string to get regulator
>> after converting to DT?
>
> I believe Kay and Greg have the plan to unify "class" and "bus" in sysfs, which
> implies turning sys_device into a derived class of device instead of kobject.
> If that understanding is correct, we might as well do that now so we can
> attach a device_node to a sys_device.
>
> Kay, does this make sense?

Yes, it's all converted already:
  http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=tree

The sysdev stuff will entirely go away, the devices are just 'struct
device' and the sysdev classes are all converted to buses.

We will convert all classes to buses over time time, and have a single
type of device and a single type of subsystem.

Kay

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Kay Sievers]

On Wed, Dec 21, 2011 at 10:43, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 21 December 2011, Richard Zhao wrote:
>> On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:
>
>> > > > > You also need to define how the core supplies get looked up.
>> > >
>> > > > It's pure software. platform uses this driver have to define "cpu" consumer.
>> > >
>> > > You still need to define this in the binding.
>> > You mean regulator DT binding? already in ? I'll check it.
>> Mark, cpu node is not a struct device, sys_device instead. I can not find
>> regulator via device/dt node. Can I still use the string to get regulator
>> after converting to DT?
>
> I believe Kay and Greg have the plan to unify "class" and "bus" in sysfs, which
> implies turning sys_device into a derived class of device instead of kobject.
> If that understanding is correct, we might as well do that now so we can
> attach a device_node to a sys_device.
>
> Kay, does this make sense?

Yes, it's all converted already:
  http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=tree

The sysdev stuff will entirely go away, the devices are just 'struct
device' and the sysdev classes are all converted to buses.

We will convert all classes to buses over time time, and have a single
type of device and a single type of subsystem.

Kay

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Mark Brown]

On Wed, Dec 21, 2011 at 09:43:34AM +0000, Arnd Bergmann wrote:
> On Wednesday 21 December 2011, Richard Zhao wrote:

> > Mark, cpu node is not a struct device, sys_device instead. I can not find
> > regulator via device/dt node. Can I still use the string to get regulator
> > after converting to DT?

> I believe Kay and Greg have the plan to unify "class" and "bus" in sysfs, which
> implies turning sys_device into a derived class of device instead of kobject.
> If that understanding is correct, we might as well do that now so we can
> attach a device_node to a sys_device.

> Kay, does this make sense?

I'm noy Kay but I think even if it's not what we end uo doing internally
it's a sensible design for the device tree representation.

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Mark Brown]

On Wed, Dec 21, 2011 at 09:43:34AM +0000, Arnd Bergmann wrote:
> On Wednesday 21 December 2011, Richard Zhao wrote:

> > Mark, cpu node is not a struct device, sys_device instead. I can not find
> > regulator via device/dt node. Can I still use the string to get regulator
> > after converting to DT?

> I believe Kay and Greg have the plan to unify "class" and "bus" in sysfs, which
> implies turning sys_device into a derived class of device instead of kobject.
> If that understanding is correct, we might as well do that now so we can
> attach a device_node to a sys_device.

> Kay, does this make sense?

I'm noy Kay but I think even if it's not what we end uo doing internally
it's a sensible design for the device tree representation.

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Mark Brown]

On Wed, Dec 21, 2011 at 12:44:57PM +0100, Kay Sievers wrote:

> We will convert all classes to buses over time time, and have a single
> type of device and a single type of subsystem.

Are there any conversions that have been done already that I can look at
for reference?

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Mark Brown]

On Wed, Dec 21, 2011 at 12:44:57PM +0100, Kay Sievers wrote:

> We will convert all classes to buses over time time, and have a single
> type of device and a single type of subsystem.

Are there any conversions that have been done already that I can look at
for reference?

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Kay Sievers]

On Wed, Dec 21, 2011 at 13:12, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Dec 21, 2011 at 12:44:57PM +0100, Kay Sievers wrote:
>
>> We will convert all classes to buses over time time, and have a single
>> type of device and a single type of subsystem.
>
> Are there any conversions that have been done already that I can look at
> for reference?

The first step is the conversion from 'sys_device' to 'device', which is here:
  http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=tree

That should hit the tree soon, if all works according to plan. All
sys_devices and sysdev classes will be gone for forever.

The 'class' to 'bus' work is simpler, because the logic in both of
them is very similar and both use the same 'struct device' already.

We'll need to add some convenience APIs to bus, and add code to make
sure the converted stuff has compat symlinks in /sys/class when
needed. Then we can convert-over one 'struct class' to 'struct
bus_type' after the other until 'struct class' can be deleted.

This work has not yet started, because we are busy with the sys_device
stuff at the moment.

No new stuff should use 'struct class' or 'struct sys_device', they
should all start right away with 'struct bus_type'.

Kay

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Kay Sievers]

On Wed, Dec 21, 2011 at 13:12, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Dec 21, 2011 at 12:44:57PM +0100, Kay Sievers wrote:
>
>> We will convert all classes to buses over time time, and have a single
>> type of device and a single type of subsystem.
>
> Are there any conversions that have been done already that I can look at
> for reference?

The first step is the conversion from 'sys_device' to 'device', which is here:
  http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=tree

That should hit the tree soon, if all works according to plan. All
sys_devices and sysdev classes will be gone for forever.

The 'class' to 'bus' work is simpler, because the logic in both of
them is very similar and both use the same 'struct device' already.

We'll need to add some convenience APIs to bus, and add code to make
sure the converted stuff has compat symlinks in /sys/class when
needed. Then we can convert-over one 'struct class' to 'struct
bus_type' after the other until 'struct class' can be deleted.

This work has not yet started, because we are busy with the sys_device
stuff at the moment.

No new stuff should use 'struct class' or 'struct sys_device', they
should all start right away with 'struct bus_type'.

Kay

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

On Wed, Dec 21, 2011 at 01:49:07PM +0100, Kay Sievers wrote:
> On Wed, Dec 21, 2011 at 13:12, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
> > On Wed, Dec 21, 2011 at 12:44:57PM +0100, Kay Sievers wrote:
> >
> >> We will convert all classes to buses over time time, and have a single
> >> type of device and a single type of subsystem.
> >
> > Are there any conversions that have been done already that I can look at
> > for reference?
> 
> The first step is the conversion from 'sys_device' to 'device', which is here:
>   http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=tree
> 
> That should hit the tree soon, if all works according to plan. All
> sys_devices and sysdev classes will be gone for forever.
Even cpu node is device, I still need to find a way to get it.  I think it's
better have another patch to fix the regulator dt binding in cpu node. I'll
not include it in this patch series.

Richard
> 
> The 'class' to 'bus' work is simpler, because the logic in both of
> them is very similar and both use the same 'struct device' already.
> 
> We'll need to add some convenience APIs to bus, and add code to make
> sure the converted stuff has compat symlinks in /sys/class when
> needed. Then we can convert-over one 'struct class' to 'struct
> bus_type' after the other until 'struct class' can be deleted.
> 
> This work has not yet started, because we are busy with the sys_device
> stuff at the moment.
> 
> No new stuff should use 'struct class' or 'struct sys_device', they
> should all start right away with 'struct bus_type'.
> 
> Kay

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao]

On Wed, Dec 21, 2011 at 01:49:07PM +0100, Kay Sievers wrote:
> On Wed, Dec 21, 2011 at 13:12, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
> > On Wed, Dec 21, 2011 at 12:44:57PM +0100, Kay Sievers wrote:
> >
> >> We will convert all classes to buses over time time, and have a single
> >> type of device and a single type of subsystem.
> >
> > Are there any conversions that have been done already that I can look at
> > for reference?
> 
> The first step is the conversion from 'sys_device' to 'device', which is here:
>   http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=tree
> 
> That should hit the tree soon, if all works according to plan. All
> sys_devices and sysdev classes will be gone for forever.
Even cpu node is device, I still need to find a way to get it.  I think it's
better have another patch to fix the regulator dt binding in cpu node. I'll
not include it in this patch series.

Richard
> 
> The 'class' to 'bus' work is simpler, because the logic in both of
> them is very similar and both use the same 'struct device' already.
> 
> We'll need to add some convenience APIs to bus, and add code to make
> sure the converted stuff has compat symlinks in /sys/class when
> needed. Then we can convert-over one 'struct class' to 'struct
> bus_type' after the other until 'struct class' can be deleted.
> 
> This work has not yet started, because we are busy with the sys_device
> stuff at the moment.
> 
> No new stuff should use 'struct class' or 'struct sys_device', they
> should all start right away with 'struct bus_type'.
> 
> Kay

Re: [PATCH V3 1/7] ARM: add cpufreq transiton notifier to adjust loops_per_jiffy for smp

[Richard Zhao]

Hi Russel,

Are the patch #1 #2 #3 ok for you?

Thanks
Richard

[PATCH V3 1/7] ARM: add cpufreq transiton notifier to adjust loops_per_jiffy for smp

[Richard Zhao]

Hi Russel,

Are the patch #1 #2 #3 ok for you?

Thanks
Richard

Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Mark Brown]

On Wed, Dec 21, 2011 at 10:19:11PM +0800, Richard Zhao wrote:

> Even cpu node is device, I still need to find a way to get it.  I think it's
> better have another patch to fix the regulator dt binding in cpu node. I'll
> not include it in this patch series.

I'd expect this to be easy if we can find any device tree data for the
CPU at all?  It's just another piece of data no different to the clock
rates or whatever.

[PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Mark Brown]

On Wed, Dec 21, 2011 at 10:19:11PM +0800, Richard Zhao wrote:

> Even cpu node is device, I still need to find a way to get it.  I think it's
> better have another patch to fix the regulator dt binding in cpu node. I'll
> not include it in this patch series.

I'd expect this to be easy if we can find any device tree data for the
CPU at all?  It's just another piece of data no different to the clock
rates or whatever.