[PATCH 0/3] Add a generic cpufreq-cpu0 driver

[PATCH 0/3] Add a generic cpufreq-cpu0 driver

[Shawn Guo]

This is a continuous effort of Richard's work [1].  But that generic
cpufreq driver has been rewritten with taking omap-cpufreq.c as the
reference to adopt OPP library.

Same as Richard's patch, the driver has no intention to support the
multi-core systems that frequency and voltage can be scaled
independently on different CPUs.  Instead, it aims at uniprocessor (UP)
and those symmetric multiprocessor (SMP) systems which share clock and
voltage across all CPUs.

I have seen patch from Mike changing omap-cpufreq driver to scale
regulator from clk notifier.  We can definitely make the same move
for cpufreq-cpu0 later when the patch gets accepted.

Regards,
Shawn

[1] http://thread.gmane.org/gmane.linux.kernel.cpufreq/7688/focus=7688

---

Richard Zhao (1):
  ARM: add cpufreq transiton notifier to adjust loops_per_jiffy for smp

Shawn Guo (2):
  PM / OPP: Initialize OPP table from device tree
  cpufreq: Add a generic cpufreq-cpu0 driver

 .../devicetree/bindings/cpufreq/cpufreq-cpu0.txt   |   58 +++++
 Documentation/devicetree/bindings/power/opp.txt    |   29 +++
 arch/arm/kernel/smp.c                              |   54 +++++
 drivers/base/power/opp.c                           |   66 ++++++
 drivers/cpufreq/Kconfig                            |   11 +
 drivers/cpufreq/Makefile                           |    2 +
 drivers/cpufreq/cpufreq-cpu0.c                     |  235 ++++++++++++++++++++
 include/linux/opp.h                                |    4 +
 8 files changed, 459 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
 create mode 100644 Documentation/devicetree/bindings/power/opp.txt
 create mode 100644 drivers/cpufreq/cpufreq-cpu0.c

-- 
1.7.5.4

[PATCH 0/3] Add a generic cpufreq-cpu0 driver

[Shawn Guo]

This is a continuous effort of Richard's work [1].  But that generic
cpufreq driver has been rewritten with taking omap-cpufreq.c as the
reference to adopt OPP library.

Same as Richard's patch, the driver has no intention to support the
multi-core systems that frequency and voltage can be scaled
independently on different CPUs.  Instead, it aims at uniprocessor (UP)
and those symmetric multiprocessor (SMP) systems which share clock and
voltage across all CPUs.

I have seen patch from Mike changing omap-cpufreq driver to scale
regulator from clk notifier.  We can definitely make the same move
for cpufreq-cpu0 later when the patch gets accepted.

Regards,
Shawn

[1] http://thread.gmane.org/gmane.linux.kernel.cpufreq/7688/focus=7688

---

Richard Zhao (1):
  ARM: add cpufreq transiton notifier to adjust loops_per_jiffy for smp

Shawn Guo (2):
  PM / OPP: Initialize OPP table from device tree
  cpufreq: Add a generic cpufreq-cpu0 driver

 .../devicetree/bindings/cpufreq/cpufreq-cpu0.txt   |   58 +++++
 Documentation/devicetree/bindings/power/opp.txt    |   29 +++
 arch/arm/kernel/smp.c                              |   54 +++++
 drivers/base/power/opp.c                           |   66 ++++++
 drivers/cpufreq/Kconfig                            |   11 +
 drivers/cpufreq/Makefile                           |    2 +
 drivers/cpufreq/cpufreq-cpu0.c                     |  235 ++++++++++++++++++++
 include/linux/opp.h                                |    4 +
 8 files changed, 459 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
 create mode 100644 Documentation/devicetree/bindings/power/opp.txt
 create mode 100644 drivers/cpufreq/cpufreq-cpu0.c

-- 
1.7.5.4

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

[Shawn Guo]

From: Richard Zhao <richard.zhao@linaro.org>

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>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 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 2c7217d..83412d2 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>
@@ -583,3 +584,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 1/3] ARM: add cpufreq transiton notifier to adjust loops_per_jiffy for smp

[Shawn Guo]

From: Richard Zhao <richard.zhao@linaro.org>

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>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 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 2c7217d..83412d2 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>
@@ -583,3 +584,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 2/3] PM / OPP: Initialize OPP table from device tree

[Shawn Guo]

With a lot of devices booting from device tree nowadays, it requires
that OPP table can be initialized from device tree.  The patch adds
a helper function of_init_opp_table together with a binding doc for
that purpose.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 Documentation/devicetree/bindings/power/opp.txt |   29 ++++++++++
 drivers/base/power/opp.c                        |   66 +++++++++++++++++++++++
 include/linux/opp.h                             |    4 ++
 3 files changed, 99 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/opp.txt

diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
new file mode 100644
index 0000000..1dd0db2
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/opp.txt
@@ -0,0 +1,29 @@
+* Generic OPP Interface
+
+SoCs have a standard set of tuples consisting of frequency and
+voltage pairs that the device will support per voltage domain. These
+are called Operating Performance Points or OPPs.
+
+Properties:
+- operating-points: An array of 3-tuples items, and each item consists
+  of frequency, voltage and enabling like <freq vol en>.
+	freq: clock frequency in kHz
+	vol: voltage in microvolt
+	en: initially enabled (1) or not (0)
+
+Examples:
+
+cpu@0 {
+	compatible = "arm,cortex-a9";
+	reg = <0>;
+	next-level-cache = <&L2>;
+	operating-points = <
+		/* kHz    uV    en */
+		1200000 1275000 0
+		996000  1225000 1
+		792000  1100000 1
+		672000  1100000 0
+		396000  950000  1
+		198000  850000  1
+	>;
+};
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index ac993ea..2d750f9 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -22,6 +22,7 @@
 #include <linux/rculist.h>
 #include <linux/rcupdate.h>
 #include <linux/opp.h>
+#include <linux/of.h>
 
 /*
  * Internal data structure organization with the OPP layer library is as
@@ -674,3 +675,68 @@ struct srcu_notifier_head *opp_get_notifier(struct device *dev)
 
 	return &dev_opp->head;
 }
+
+#ifdef CONFIG_OF
+/**
+ * of_init_opp_table() - Initialize opp table from device tree
+ * @dev:	device pointer used to lookup device OPPs.
+ *
+ * Register the initial OPP table with the OPP library for given device.
+ */
+int of_init_opp_table(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	const char *propname = "operating-points";
+	const struct property *pp;
+	u32 *opp;
+	int ret, i, nr;
+
+	pp = of_find_property(np, propname, NULL);
+	if (!pp) {
+		dev_err(dev, "%s: Unable to find property", __func__);
+		return -ENODEV;
+	}
+
+	opp = kzalloc(pp->length, GFP_KERNEL);
+	if (!opp) {
+		dev_err(dev, "%s: Unable to allocate array\n", __func__);
+		return -ENOMEM;
+	}
+
+	nr = pp->length / sizeof(u32);
+	ret = of_property_read_u32_array(np, propname, opp, nr);
+	if (ret) {
+		dev_err(dev, "%s: Unable to read OPPs\n", __func__);
+		goto out;
+	}
+
+	nr /= 3;
+	for (i = 0; i < nr; i++) {
+		/*
+		 * Each OPP is a set of tuples consisting of frequency,
+		 * voltage and availability like <freq-kHz vol-uV enable>.
+		 */
+		u32 *val = opp + i * 3;
+
+		val[0] *= 1000;
+		ret = opp_add(dev, val[0], val[1]);
+		if (ret) {
+			dev_warn(dev, "%s: Failed to add OPP %d: %d\n",
+				 __func__, val[0], ret);
+			continue;
+		}
+
+		if (!val[2]) {
+			ret = opp_disable(dev, val[0]);
+			if (ret)
+				dev_warn(dev, "%s: Failed to disable OPP %d: %d\n",
+					 __func__, val[0], ret);
+		}
+	}
+
+	ret = 0;
+out:
+	kfree(opp);
+	return ret;
+}
+#endif
diff --git a/include/linux/opp.h b/include/linux/opp.h
index 2a4e5fa..fd165ad 100644
--- a/include/linux/opp.h
+++ b/include/linux/opp.h
@@ -48,6 +48,10 @@ int opp_disable(struct device *dev, unsigned long freq);
 
 struct srcu_notifier_head *opp_get_notifier(struct device *dev);
 
+#ifdef CONFIG_OF
+int of_init_opp_table(struct device *dev);
+#endif
+
 #else
 static inline unsigned long opp_get_voltage(struct opp *opp)
 {
-- 
1.7.5.4

[PATCH 2/3] PM / OPP: Initialize OPP table from device tree

[Shawn Guo]

With a lot of devices booting from device tree nowadays, it requires
that OPP table can be initialized from device tree.  The patch adds
a helper function of_init_opp_table together with a binding doc for
that purpose.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 Documentation/devicetree/bindings/power/opp.txt |   29 ++++++++++
 drivers/base/power/opp.c                        |   66 +++++++++++++++++++++++
 include/linux/opp.h                             |    4 ++
 3 files changed, 99 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/opp.txt

diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
new file mode 100644
index 0000000..1dd0db2
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/opp.txt
@@ -0,0 +1,29 @@
+* Generic OPP Interface
+
+SoCs have a standard set of tuples consisting of frequency and
+voltage pairs that the device will support per voltage domain. These
+are called Operating Performance Points or OPPs.
+
+Properties:
+- operating-points: An array of 3-tuples items, and each item consists
+  of frequency, voltage and enabling like <freq vol en>.
+	freq: clock frequency in kHz
+	vol: voltage in microvolt
+	en: initially enabled (1) or not (0)
+
+Examples:
+
+cpu at 0 {
+	compatible = "arm,cortex-a9";
+	reg = <0>;
+	next-level-cache = <&L2>;
+	operating-points = <
+		/* kHz    uV    en */
+		1200000 1275000 0
+		996000  1225000 1
+		792000  1100000 1
+		672000  1100000 0
+		396000  950000  1
+		198000  850000  1
+	>;
+};
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index ac993ea..2d750f9 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -22,6 +22,7 @@
 #include <linux/rculist.h>
 #include <linux/rcupdate.h>
 #include <linux/opp.h>
+#include <linux/of.h>
 
 /*
  * Internal data structure organization with the OPP layer library is as
@@ -674,3 +675,68 @@ struct srcu_notifier_head *opp_get_notifier(struct device *dev)
 
 	return &dev_opp->head;
 }
+
+#ifdef CONFIG_OF
+/**
+ * of_init_opp_table() - Initialize opp table from device tree
+ * @dev:	device pointer used to lookup device OPPs.
+ *
+ * Register the initial OPP table with the OPP library for given device.
+ */
+int of_init_opp_table(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	const char *propname = "operating-points";
+	const struct property *pp;
+	u32 *opp;
+	int ret, i, nr;
+
+	pp = of_find_property(np, propname, NULL);
+	if (!pp) {
+		dev_err(dev, "%s: Unable to find property", __func__);
+		return -ENODEV;
+	}
+
+	opp = kzalloc(pp->length, GFP_KERNEL);
+	if (!opp) {
+		dev_err(dev, "%s: Unable to allocate array\n", __func__);
+		return -ENOMEM;
+	}
+
+	nr = pp->length / sizeof(u32);
+	ret = of_property_read_u32_array(np, propname, opp, nr);
+	if (ret) {
+		dev_err(dev, "%s: Unable to read OPPs\n", __func__);
+		goto out;
+	}
+
+	nr /= 3;
+	for (i = 0; i < nr; i++) {
+		/*
+		 * Each OPP is a set of tuples consisting of frequency,
+		 * voltage and availability like <freq-kHz vol-uV enable>.
+		 */
+		u32 *val = opp + i * 3;
+
+		val[0] *= 1000;
+		ret = opp_add(dev, val[0], val[1]);
+		if (ret) {
+			dev_warn(dev, "%s: Failed to add OPP %d: %d\n",
+				 __func__, val[0], ret);
+			continue;
+		}
+
+		if (!val[2]) {
+			ret = opp_disable(dev, val[0]);
+			if (ret)
+				dev_warn(dev, "%s: Failed to disable OPP %d: %d\n",
+					 __func__, val[0], ret);
+		}
+	}
+
+	ret = 0;
+out:
+	kfree(opp);
+	return ret;
+}
+#endif
diff --git a/include/linux/opp.h b/include/linux/opp.h
index 2a4e5fa..fd165ad 100644
--- a/include/linux/opp.h
+++ b/include/linux/opp.h
@@ -48,6 +48,10 @@ int opp_disable(struct device *dev, unsigned long freq);
 
 struct srcu_notifier_head *opp_get_notifier(struct device *dev);
 
+#ifdef CONFIG_OF
+int of_init_opp_table(struct device *dev);
+#endif
+
 #else
 static inline unsigned long opp_get_voltage(struct opp *opp)
 {
-- 
1.7.5.4

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

It adds a generic cpufreq driver for CPU0 frequency management based on
clk, regulator, OPP and device tree support.  It can support both
uniprocessor (UP) and those symmetric multiprocessor (SMP) systems which
share clock and voltage across all CPUs.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 .../devicetree/bindings/cpufreq/cpufreq-cpu0.txt   |   58 +++++
 drivers/cpufreq/Kconfig                            |   11 +
 drivers/cpufreq/Makefile                           |    2 +
 drivers/cpufreq/cpufreq-cpu0.c                     |  235 ++++++++++++++++++++
 4 files changed, 306 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
 create mode 100644 drivers/cpufreq/cpufreq-cpu0.c

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
new file mode 100644
index 0000000..94799bc
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
@@ -0,0 +1,58 @@
+Generic cpufreq driver for CPU0
+
+It is a generic cpufreq driver for CPU0 frequency management.  It
+supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
+systems which share clock and voltage across all CPUs.
+
+Both required and optional properties listed below must be defined
+under node /cpus/cpu@0.
+
+Required properties:
+- operating-points: Refer to Documentation/devicetree/bindings/power/opp.txt
+  for details
+
+Optional properties:
+- transition-latency: Specify the possible maximum transition latency,
+  in unit of nanoseconds.
+- voltage-tolerance: Specify the CPU voltage tolerance in percentage.
+
+Examples:
+
+cpus {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	cpu@0 {
+		compatible = "arm,cortex-a9";
+		reg = <0>;
+		next-level-cache = <&L2>;
+		operating-points = <
+			/* kHz    uV    en */
+			1200000 1275000 0
+			996000  1225000 1
+			792000  1100000 1
+			672000  1100000 0
+			396000  950000  1
+			198000  850000  1
+		>;
+		transition-latency = <61036>; /* two CLK32 periods */
+	};
+
+	cpu@1 {
+		compatible = "arm,cortex-a9";
+		reg = <1>;
+		next-level-cache = <&L2>;
+	};
+
+	cpu@2 {
+		compatible = "arm,cortex-a9";
+		reg = <2>;
+		next-level-cache = <&L2>;
+	};
+
+	cpu@3 {
+		compatible = "arm,cortex-a9";
+		reg = <3>;
+		next-level-cache = <&L2>;
+	};
+};
diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index e24a2a1..e77a1e0 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -179,6 +179,17 @@ config CPU_FREQ_GOV_CONSERVATIVE
 
 	  If in doubt, say N.
 
+config GENERIC_CPUFREQ_CPU0
+	bool "Generic cpufreq driver for CPU0"
+	depends on HAVE_CLK && REGULATOR && PM_OPP && OF
+	select CPU_FREQ_TABLE
+	help
+	  This adds a generic cpufreq driver for CPU0 frequency management.
+	  It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
+	  systems which share clock and voltage across all CPUs.
+
+	  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 9531fc2..a378ed2 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_CPU0)	+= cpufreq-cpu0.o
+
 ##################################################################################
 # x86 drivers.
 # Link order matters. K8 is preferred to ACPI because of firmware bugs in early
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
new file mode 100644
index 0000000..f1bb7f37
--- /dev/null
+++ b/drivers/cpufreq/cpufreq-cpu0.c
@@ -0,0 +1,235 @@
+/*
+ * Copyright (C) 2012 Freescale Semiconductor, Inc.
+ *
+ * Reuse some codes from drivers/cpufreq/omap-cpufreq.c
+ *
+ * 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
+ */
+
+#define pr_fmt(fmt)	KBUILD_MODNAME ": " fmt
+
+#include <linux/clk.h>
+#include <linux/cpu.h>
+#include <linux/cpufreq.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/opp.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+/* voltage tolerance in percentage */
+static unsigned int voltage_tolerance;
+
+static struct device *cpu_dev;
+static struct clk *cpu_clk;
+static struct regulator *cpu_reg;
+static struct cpufreq_frequency_table *freq_table;
+
+static int cpu0_verify_speed(struct cpufreq_policy *policy)
+{
+	return cpufreq_frequency_table_verify(policy, freq_table);
+}
+
+static unsigned int cpu0_get_speed(unsigned int cpu)
+{
+	return clk_get_rate(cpu_clk) / 1000;
+}
+
+static int cpu0_set_target(struct cpufreq_policy *policy,
+			   unsigned int target_freq, unsigned int relation)
+{
+	struct cpufreq_freqs freqs;
+	struct opp *opp;
+	unsigned long freq_Hz, volt = 0, volt_old = 0, tol = 0;
+	unsigned int index, cpu;
+	int ret = 0;
+
+	ret = cpufreq_frequency_table_target(policy, freq_table, target_freq,
+					     relation, &index);
+	if (ret) {
+		pr_err("failed to match target freqency %d: %d\n",
+		       target_freq, ret);
+		return ret;
+	}
+
+	freq_Hz = clk_round_rate(cpu_clk, freq_table[index].frequency * 1000);
+	if (freq_Hz < 0)
+		freq_Hz = freq_table[index].frequency * 1000;
+	freqs.new = freq_Hz / 1000;
+	freqs.old = clk_get_rate(cpu_clk) / 1000;
+
+	if (freqs.old == freqs.new)
+		return 0;
+
+	for_each_cpu(cpu, policy->cpus) {
+		freqs.cpu = cpu;
+		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+	}
+
+	if (cpu_reg) {
+		opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
+		if (IS_ERR(opp)) {
+			pr_err("failed to find OPP for %ld\n", freq_Hz);
+			return PTR_ERR(opp);
+		}
+		volt = opp_get_voltage(opp);
+		tol = volt * voltage_tolerance / 100;
+		volt_old = regulator_get_voltage(cpu_reg);
+	}
+
+	pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
+		 freqs.old / 1000, volt_old ? volt_old / 1000 : -1,
+		 freqs.new / 1000, volt ? volt / 1000 : -1);
+
+	/* scaling up?  scale voltage before frequency */
+	if (cpu_reg && freqs.new > freqs.old) {
+		if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
+			pr_warn("Unable to scale voltage up\n");
+			freqs.new = freqs.old;
+			goto done;
+		}
+	}
+
+	ret = clk_set_rate(cpu_clk, freqs.new * 1000);
+
+	/* scaling down?  scale voltage after frequency */
+	if (cpu_reg && (freqs.new < freqs.old)) {
+		if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
+			pr_warn("Unable to scale voltage down\n");
+			ret = clk_set_rate(cpu_clk, freqs.old * 1000);
+			freqs.new = freqs.old;
+			goto done;
+		}
+	}
+
+done:
+	for_each_cpu(cpu, policy->cpus) {
+		freqs.cpu = cpu;
+		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+	}
+
+	return ret;
+}
+
+static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
+{
+	struct device_node *np;
+	int ret;
+
+	/* Initialize resources via CPU0 */
+	if (policy->cpu != 0)
+		return -EINVAL;
+
+	np = of_find_node_by_path("/cpus/cpu@0");
+	if (!np) {
+		pr_err("failed to find cpu0 node\n");
+		return -ENOENT;
+	}
+
+	cpu_dev = get_cpu_device(0);
+	if (!cpu_dev) {
+		pr_err("failed to get cpu0 device\n");
+		ret = -ENODEV;
+		goto out_put_node;
+	}
+
+	cpu_dev->of_node = np;
+	ret = of_init_opp_table(cpu_dev);
+	if (ret) {
+		pr_err("failed to init OPP table\n");
+		goto out_put_node;
+	}
+
+	ret = opp_init_cpufreq_table(cpu_dev, &freq_table);
+	if (ret) {
+		pr_err("failed to init cpufreq table\n");
+		goto out_put_node;
+	}
+
+	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
+	if (ret) {
+		pr_err("invalid frequency table for cpu0\n");
+		goto out_free_table;
+	}
+
+	if (of_property_read_u32(np, "transition-latency",
+				 &policy->cpuinfo.transition_latency))
+		policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
+
+	of_property_read_u32(np, "voltage-tolerance", &voltage_tolerance);
+
+	cpu_clk = clk_get(cpu_dev, NULL);
+	if (IS_ERR(cpu_clk)) {
+		pr_err("failed to get cpu0 clock\n");
+		ret = PTR_ERR(cpu_clk);
+		goto out_free_table;
+	}
+	policy->cur = clk_get_rate(cpu_clk) / 1000;
+
+	cpu_reg = regulator_get(cpu_dev, "cpu0");
+	if (IS_ERR(cpu_reg)) {
+		pr_warn("failed to get cpu0 regulator\n");
+		cpu_reg = NULL;
+	}
+
+	/*
+	 * The driver only supports the SMP configuartion where all processors
+	 * share the clock and voltage and clock.  Use cpufreq affected_cpus
+	 * interface to have all CPUs scaled together.
+	 */
+	policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
+	cpumask_setall(policy->cpus);
+
+	cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
+
+	of_node_put(np);
+	return 0;
+
+out_free_table:
+	opp_free_cpufreq_table(cpu_dev, &freq_table);
+out_put_node:
+	of_node_put(np);
+	return ret;
+}
+
+static int cpu0_cpufreq_exit(struct cpufreq_policy *policy)
+{
+	cpufreq_frequency_table_put_attr(policy->cpu);
+	regulator_put(cpu_reg);
+	clk_put(cpu_clk);
+	opp_free_cpufreq_table(cpu_dev, &freq_table);
+
+	return 0;
+}
+
+static struct freq_attr *cpu0_cpufreq_attr[] = {
+	&cpufreq_freq_attr_scaling_available_freqs,
+	NULL,
+};
+
+static struct cpufreq_driver cpu0_cpufreq_driver = {
+	.flags = CPUFREQ_STICKY,
+	.verify = cpu0_verify_speed,
+	.target = cpu0_set_target,
+	.get = cpu0_get_speed,
+	.init = cpu0_cpufreq_init,
+	.exit = cpu0_cpufreq_exit,
+	.name = "generic_cpu0",
+	.attr = cpu0_cpufreq_attr,
+};
+
+static int __devinit cpu0_cpufreq_driver_init(void)
+{
+	return cpufreq_register_driver(&cpu0_cpufreq_driver);
+}
+late_initcall(cpu0_cpufreq_driver_init);
+
+MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>");
+MODULE_DESCRIPTION("Generic cpufreq driver for CPU0");
+MODULE_LICENSE("GPL");
-- 
1.7.5.4

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

It adds a generic cpufreq driver for CPU0 frequency management based on
clk, regulator, OPP and device tree support.  It can support both
uniprocessor (UP) and those symmetric multiprocessor (SMP) systems which
share clock and voltage across all CPUs.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 .../devicetree/bindings/cpufreq/cpufreq-cpu0.txt   |   58 +++++
 drivers/cpufreq/Kconfig                            |   11 +
 drivers/cpufreq/Makefile                           |    2 +
 drivers/cpufreq/cpufreq-cpu0.c                     |  235 ++++++++++++++++++++
 4 files changed, 306 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
 create mode 100644 drivers/cpufreq/cpufreq-cpu0.c

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
new file mode 100644
index 0000000..94799bc
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
@@ -0,0 +1,58 @@
+Generic cpufreq driver for CPU0
+
+It is a generic cpufreq driver for CPU0 frequency management.  It
+supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
+systems which share clock and voltage across all CPUs.
+
+Both required and optional properties listed below must be defined
+under node /cpus/cpu at 0.
+
+Required properties:
+- operating-points: Refer to Documentation/devicetree/bindings/power/opp.txt
+  for details
+
+Optional properties:
+- transition-latency: Specify the possible maximum transition latency,
+  in unit of nanoseconds.
+- voltage-tolerance: Specify the CPU voltage tolerance in percentage.
+
+Examples:
+
+cpus {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	cpu at 0 {
+		compatible = "arm,cortex-a9";
+		reg = <0>;
+		next-level-cache = <&L2>;
+		operating-points = <
+			/* kHz    uV    en */
+			1200000 1275000 0
+			996000  1225000 1
+			792000  1100000 1
+			672000  1100000 0
+			396000  950000  1
+			198000  850000  1
+		>;
+		transition-latency = <61036>; /* two CLK32 periods */
+	};
+
+	cpu at 1 {
+		compatible = "arm,cortex-a9";
+		reg = <1>;
+		next-level-cache = <&L2>;
+	};
+
+	cpu at 2 {
+		compatible = "arm,cortex-a9";
+		reg = <2>;
+		next-level-cache = <&L2>;
+	};
+
+	cpu at 3 {
+		compatible = "arm,cortex-a9";
+		reg = <3>;
+		next-level-cache = <&L2>;
+	};
+};
diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index e24a2a1..e77a1e0 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -179,6 +179,17 @@ config CPU_FREQ_GOV_CONSERVATIVE
 
 	  If in doubt, say N.
 
+config GENERIC_CPUFREQ_CPU0
+	bool "Generic cpufreq driver for CPU0"
+	depends on HAVE_CLK && REGULATOR && PM_OPP && OF
+	select CPU_FREQ_TABLE
+	help
+	  This adds a generic cpufreq driver for CPU0 frequency management.
+	  It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
+	  systems which share clock and voltage across all CPUs.
+
+	  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 9531fc2..a378ed2 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_CPU0)	+= cpufreq-cpu0.o
+
 ##################################################################################
 # x86 drivers.
 # Link order matters. K8 is preferred to ACPI because of firmware bugs in early
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
new file mode 100644
index 0000000..f1bb7f37
--- /dev/null
+++ b/drivers/cpufreq/cpufreq-cpu0.c
@@ -0,0 +1,235 @@
+/*
+ * Copyright (C) 2012 Freescale Semiconductor, Inc.
+ *
+ * Reuse some codes from drivers/cpufreq/omap-cpufreq.c
+ *
+ * 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
+ */
+
+#define pr_fmt(fmt)	KBUILD_MODNAME ": " fmt
+
+#include <linux/clk.h>
+#include <linux/cpu.h>
+#include <linux/cpufreq.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/opp.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+/* voltage tolerance in percentage */
+static unsigned int voltage_tolerance;
+
+static struct device *cpu_dev;
+static struct clk *cpu_clk;
+static struct regulator *cpu_reg;
+static struct cpufreq_frequency_table *freq_table;
+
+static int cpu0_verify_speed(struct cpufreq_policy *policy)
+{
+	return cpufreq_frequency_table_verify(policy, freq_table);
+}
+
+static unsigned int cpu0_get_speed(unsigned int cpu)
+{
+	return clk_get_rate(cpu_clk) / 1000;
+}
+
+static int cpu0_set_target(struct cpufreq_policy *policy,
+			   unsigned int target_freq, unsigned int relation)
+{
+	struct cpufreq_freqs freqs;
+	struct opp *opp;
+	unsigned long freq_Hz, volt = 0, volt_old = 0, tol = 0;
+	unsigned int index, cpu;
+	int ret = 0;
+
+	ret = cpufreq_frequency_table_target(policy, freq_table, target_freq,
+					     relation, &index);
+	if (ret) {
+		pr_err("failed to match target freqency %d: %d\n",
+		       target_freq, ret);
+		return ret;
+	}
+
+	freq_Hz = clk_round_rate(cpu_clk, freq_table[index].frequency * 1000);
+	if (freq_Hz < 0)
+		freq_Hz = freq_table[index].frequency * 1000;
+	freqs.new = freq_Hz / 1000;
+	freqs.old = clk_get_rate(cpu_clk) / 1000;
+
+	if (freqs.old == freqs.new)
+		return 0;
+
+	for_each_cpu(cpu, policy->cpus) {
+		freqs.cpu = cpu;
+		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+	}
+
+	if (cpu_reg) {
+		opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
+		if (IS_ERR(opp)) {
+			pr_err("failed to find OPP for %ld\n", freq_Hz);
+			return PTR_ERR(opp);
+		}
+		volt = opp_get_voltage(opp);
+		tol = volt * voltage_tolerance / 100;
+		volt_old = regulator_get_voltage(cpu_reg);
+	}
+
+	pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
+		 freqs.old / 1000, volt_old ? volt_old / 1000 : -1,
+		 freqs.new / 1000, volt ? volt / 1000 : -1);
+
+	/* scaling up?  scale voltage before frequency */
+	if (cpu_reg && freqs.new > freqs.old) {
+		if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
+			pr_warn("Unable to scale voltage up\n");
+			freqs.new = freqs.old;
+			goto done;
+		}
+	}
+
+	ret = clk_set_rate(cpu_clk, freqs.new * 1000);
+
+	/* scaling down?  scale voltage after frequency */
+	if (cpu_reg && (freqs.new < freqs.old)) {
+		if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
+			pr_warn("Unable to scale voltage down\n");
+			ret = clk_set_rate(cpu_clk, freqs.old * 1000);
+			freqs.new = freqs.old;
+			goto done;
+		}
+	}
+
+done:
+	for_each_cpu(cpu, policy->cpus) {
+		freqs.cpu = cpu;
+		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+	}
+
+	return ret;
+}
+
+static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
+{
+	struct device_node *np;
+	int ret;
+
+	/* Initialize resources via CPU0 */
+	if (policy->cpu != 0)
+		return -EINVAL;
+
+	np = of_find_node_by_path("/cpus/cpu at 0");
+	if (!np) {
+		pr_err("failed to find cpu0 node\n");
+		return -ENOENT;
+	}
+
+	cpu_dev = get_cpu_device(0);
+	if (!cpu_dev) {
+		pr_err("failed to get cpu0 device\n");
+		ret = -ENODEV;
+		goto out_put_node;
+	}
+
+	cpu_dev->of_node = np;
+	ret = of_init_opp_table(cpu_dev);
+	if (ret) {
+		pr_err("failed to init OPP table\n");
+		goto out_put_node;
+	}
+
+	ret = opp_init_cpufreq_table(cpu_dev, &freq_table);
+	if (ret) {
+		pr_err("failed to init cpufreq table\n");
+		goto out_put_node;
+	}
+
+	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
+	if (ret) {
+		pr_err("invalid frequency table for cpu0\n");
+		goto out_free_table;
+	}
+
+	if (of_property_read_u32(np, "transition-latency",
+				 &policy->cpuinfo.transition_latency))
+		policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
+
+	of_property_read_u32(np, "voltage-tolerance", &voltage_tolerance);
+
+	cpu_clk = clk_get(cpu_dev, NULL);
+	if (IS_ERR(cpu_clk)) {
+		pr_err("failed to get cpu0 clock\n");
+		ret = PTR_ERR(cpu_clk);
+		goto out_free_table;
+	}
+	policy->cur = clk_get_rate(cpu_clk) / 1000;
+
+	cpu_reg = regulator_get(cpu_dev, "cpu0");
+	if (IS_ERR(cpu_reg)) {
+		pr_warn("failed to get cpu0 regulator\n");
+		cpu_reg = NULL;
+	}
+
+	/*
+	 * The driver only supports the SMP configuartion where all processors
+	 * share the clock and voltage and clock.  Use cpufreq affected_cpus
+	 * interface to have all CPUs scaled together.
+	 */
+	policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
+	cpumask_setall(policy->cpus);
+
+	cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
+
+	of_node_put(np);
+	return 0;
+
+out_free_table:
+	opp_free_cpufreq_table(cpu_dev, &freq_table);
+out_put_node:
+	of_node_put(np);
+	return ret;
+}
+
+static int cpu0_cpufreq_exit(struct cpufreq_policy *policy)
+{
+	cpufreq_frequency_table_put_attr(policy->cpu);
+	regulator_put(cpu_reg);
+	clk_put(cpu_clk);
+	opp_free_cpufreq_table(cpu_dev, &freq_table);
+
+	return 0;
+}
+
+static struct freq_attr *cpu0_cpufreq_attr[] = {
+	&cpufreq_freq_attr_scaling_available_freqs,
+	NULL,
+};
+
+static struct cpufreq_driver cpu0_cpufreq_driver = {
+	.flags = CPUFREQ_STICKY,
+	.verify = cpu0_verify_speed,
+	.target = cpu0_set_target,
+	.get = cpu0_get_speed,
+	.init = cpu0_cpufreq_init,
+	.exit = cpu0_cpufreq_exit,
+	.name = "generic_cpu0",
+	.attr = cpu0_cpufreq_attr,
+};
+
+static int __devinit cpu0_cpufreq_driver_init(void)
+{
+	return cpufreq_register_driver(&cpu0_cpufreq_driver);
+}
+late_initcall(cpu0_cpufreq_driver_init);
+
+MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>");
+MODULE_DESCRIPTION("Generic cpufreq driver for CPU0");
+MODULE_LICENSE("GPL");
-- 
1.7.5.4

Re: [PATCH 0/3] Add a generic cpufreq-cpu0 driver

[Rafael J. Wysocki]

Hi,

On Thursday, July 19, 2012, Shawn Guo wrote:
> This is a continuous effort of Richard's work [1].  But that generic
> cpufreq driver has been rewritten with taking omap-cpufreq.c as the
> reference to adopt OPP library.
> 
> Same as Richard's patch, the driver has no intention to support the
> multi-core systems that frequency and voltage can be scaled
> independently on different CPUs.  Instead, it aims at uniprocessor (UP)
> and those symmetric multiprocessor (SMP) systems which share clock and
> voltage across all CPUs.
> 
> I have seen patch from Mike changing omap-cpufreq driver to scale
> regulator from clk notifier.  We can definitely make the same move
> for cpufreq-cpu0 later when the patch gets accepted.
> 
> Regards,
> Shawn
> 
> [1] http://thread.gmane.org/gmane.linux.kernel.cpufreq/7688/focus=7688

It is too late to consider your patches for inclusion into v3.6, so I'm
regarding them as RFC at this point.

Please resubmit after v3.6-rc1 has been released.

Thanks,
Rafael

[PATCH 0/3] Add a generic cpufreq-cpu0 driver

[Rafael J. Wysocki]

Hi,

On Thursday, July 19, 2012, Shawn Guo wrote:
> This is a continuous effort of Richard's work [1].  But that generic
> cpufreq driver has been rewritten with taking omap-cpufreq.c as the
> reference to adopt OPP library.
> 
> Same as Richard's patch, the driver has no intention to support the
> multi-core systems that frequency and voltage can be scaled
> independently on different CPUs.  Instead, it aims at uniprocessor (UP)
> and those symmetric multiprocessor (SMP) systems which share clock and
> voltage across all CPUs.
> 
> I have seen patch from Mike changing omap-cpufreq driver to scale
> regulator from clk notifier.  We can definitely make the same move
> for cpufreq-cpu0 later when the patch gets accepted.
> 
> Regards,
> Shawn
> 
> [1] http://thread.gmane.org/gmane.linux.kernel.cpufreq/7688/focus=7688

It is too late to consider your patches for inclusion into v3.6, so I'm
regarding them as RFC at this point.

Please resubmit after v3.6-rc1 has been released.

Thanks,
Rafael

Re: [PATCH 0/3] Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Thu, Jul 19, 2012 at 08:39:39PM +0200, Rafael J. Wysocki wrote:
> It is too late to consider your patches for inclusion into v3.6, so I'm
> regarding them as RFC at this point.
> 
Yes, it's on the review stage, and 3.7 is the target.

> Please resubmit after v3.6-rc1 has been released.
> 
Will do.

-- 
Regards,
Shawn

[PATCH 0/3] Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Thu, Jul 19, 2012 at 08:39:39PM +0200, Rafael J. Wysocki wrote:
> It is too late to consider your patches for inclusion into v3.6, so I'm
> regarding them as RFC at this point.
> 
Yes, it's on the review stage, and 3.7 is the target.

> Please resubmit after v3.6-rc1 has been released.
> 
Will do.

-- 
Regards,
Shawn

Re: [PATCH 2/3] PM / OPP: Initialize OPP table from device tree

[Menon, Nishanth]

On Thu, Jul 19, 2012 at 10:54 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> With a lot of devices booting from device tree nowadays, it requires
> that OPP table can be initialized from device tree.  The patch adds
> a helper function of_init_opp_table together with a binding doc for
> that purpose.
nice to see this happen, a quick feedback:
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  Documentation/devicetree/bindings/power/opp.txt |   29 ++++++++++
>  drivers/base/power/opp.c                        |   66 +++++++++++++++++++++++
>  include/linux/opp.h                             |    4 ++
>  3 files changed, 99 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/power/opp.txt
>
> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
> new file mode 100644
> index 0000000..1dd0db2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/opp.txt
> @@ -0,0 +1,29 @@
> +* Generic OPP Interface
> +
> +SoCs have a standard set of tuples consisting of frequency and
> +voltage pairs that the device will support per voltage domain. These
> +are called Operating Performance Points or OPPs.
> +
> +Properties:
> +- operating-points: An array of 3-tuples items, and each item consists
> +  of frequency, voltage and enabling like <freq vol en>.
> +       freq: clock frequency in kHz
> +       vol: voltage in microvolt
> +       en: initially enabled (1) or not (0)
> +
> +Examples:
> +
> +cpu@0 {
> +       compatible = "arm,cortex-a9";
I am not sure how this works - would an example of OMAP4430, 60, 70
help? they have completely different OPP entries.

> +       reg = <0>;
> +       next-level-cache = <&L2>;
> +       operating-points = <
> +               /* kHz    uV    en */
> +               1200000 1275000 0
> +               996000  1225000 1
> +               792000  1100000 1
> +               672000  1100000 0
> +               396000  950000  1
> +               198000  850000  1

Just my 2cents, If we change en to be a flag:
0 - add, but disable
1 - add (enabled)
we could extend this further if the definition is a flag, for example:
2 - add and disable IF any of notifier chain return failure
3- add but dont call notifier chain (e.g. OPPs that are present for All SoC)

in addition, SoC might have additional properties associated with each
OPP such a flag
could be split up to mean lower 16 bits as OPP library flag and higher
16 bit to mean SoC custom flag

Example - On certain SoC a specific type of power technique is
supposed to be used per OPP, such a flag
passed on via notifiers to SoC handler might be capable of
centralizing the OPP information into the DT.

> +       >;
> +};
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index ac993ea..2d750f9 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -22,6 +22,7 @@
>  #include <linux/rculist.h>
>  #include <linux/rcupdate.h>
>  #include <linux/opp.h>
> +#include <linux/of.h>
>
>  /*
>   * Internal data structure organization with the OPP layer library is as
> @@ -674,3 +675,68 @@ struct srcu_notifier_head *opp_get_notifier(struct device *dev)
>
>         return &dev_opp->head;
>  }
> +
> +#ifdef CONFIG_OF
> +/**
> + * of_init_opp_table() - Initialize opp table from device tree
> + * @dev:       device pointer used to lookup device OPPs.
> + *
> + * Register the initial OPP table with the OPP library for given device.
> + */
> +int of_init_opp_table(struct device *dev)
> +{
> +       struct device_node *np = dev->of_node;
> +       const char *propname = "operating-points";
> +       const struct property *pp;
> +       u32 *opp;
> +       int ret, i, nr;
> +
> +       pp = of_find_property(np, propname, NULL);
> +       if (!pp) {
> +               dev_err(dev, "%s: Unable to find property", __func__);
> +               return -ENODEV;
> +       }
> +
> +       opp = kzalloc(pp->length, GFP_KERNEL);
> +       if (!opp) {
> +               dev_err(dev, "%s: Unable to allocate array\n", __func__);
> +               return -ENOMEM;
> +       }
> +
> +       nr = pp->length / sizeof(u32);
error warn if the pp->length is not multiple of u32? we also expect
later on to be a multiple of 3(f,v,disable

> +       ret = of_property_read_u32_array(np, propname, opp, nr);
> +       if (ret) {
> +               dev_err(dev, "%s: Unable to read OPPs\n", __func__);
> +               goto out;
> +       }
> +
> +       nr /= 3;
> +       for (i = 0; i < nr; i++) {
> +               /*
> +                * Each OPP is a set of tuples consisting of frequency,
> +                * voltage and availability like <freq-kHz vol-uV enable>.
> +                */
> +               u32 *val = opp + i * 3;
> +
> +               val[0] *= 1000;
> +               ret = opp_add(dev, val[0], val[1]);
> +               if (ret) {
> +                       dev_warn(dev, "%s: Failed to add OPP %d: %d\n",
> +                                __func__, val[0], ret);
> +                       continue;
> +               }
> +
> +               if (!val[2]) {
> +                       ret = opp_disable(dev, val[0]);
Since we are updating the table out of context of the SoC users,
consider what might happen if someone where to operate on the OPP
after opp_add, but before opp_disable?
instead of having the pain of adding an OPP and then disabling it,
since the code will now move to core OPP
library itself, wont it be better to hold dev_opp_list_lock and update
the full table with a proper list walk - yes
the current opp_add and opp_disable apis would need refactoring to be
reusable. It will also save on
synchronize_rcu calls on multiple iterations of the list.


> +                       if (ret)
> +                               dev_warn(dev, "%s: Failed to disable OPP %d: %d\n",
> +                                        __func__, val[0], ret);

umm... but we override the return with 0? OPP add failure might
indicate the table is invalid/corrupted - or no memory.
What is the point in populating a bad table up? having a singular
function with direct access to internal structures
might save us these un-necessary dilemma.


> +               }
> +       }
> +
> +       ret = 0;
> +out:
> +       kfree(opp);
> +       return ret;
> +}
> +#endif
> diff --git a/include/linux/opp.h b/include/linux/opp.h
> index 2a4e5fa..fd165ad 100644
> --- a/include/linux/opp.h
> +++ b/include/linux/opp.h
> @@ -48,6 +48,10 @@ int opp_disable(struct device *dev, unsigned long freq);
>
>  struct srcu_notifier_head *opp_get_notifier(struct device *dev);
>
> +#ifdef CONFIG_OF
> +int of_init_opp_table(struct device *dev);

#else
static inline int of_init_opp_table(struct device *dev) { return -EINVAL; }
?

> +#endif
> +
>  #else
>  static inline unsigned long opp_get_voltage(struct opp *opp)
>  {


Regards,
Nishanth Menon

[PATCH 2/3] PM / OPP: Initialize OPP table from device tree

[Menon, Nishanth]

On Thu, Jul 19, 2012 at 10:54 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> With a lot of devices booting from device tree nowadays, it requires
> that OPP table can be initialized from device tree.  The patch adds
> a helper function of_init_opp_table together with a binding doc for
> that purpose.
nice to see this happen, a quick feedback:
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  Documentation/devicetree/bindings/power/opp.txt |   29 ++++++++++
>  drivers/base/power/opp.c                        |   66 +++++++++++++++++++++++
>  include/linux/opp.h                             |    4 ++
>  3 files changed, 99 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/power/opp.txt
>
> diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt
> new file mode 100644
> index 0000000..1dd0db2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/opp.txt
> @@ -0,0 +1,29 @@
> +* Generic OPP Interface
> +
> +SoCs have a standard set of tuples consisting of frequency and
> +voltage pairs that the device will support per voltage domain. These
> +are called Operating Performance Points or OPPs.
> +
> +Properties:
> +- operating-points: An array of 3-tuples items, and each item consists
> +  of frequency, voltage and enabling like <freq vol en>.
> +       freq: clock frequency in kHz
> +       vol: voltage in microvolt
> +       en: initially enabled (1) or not (0)
> +
> +Examples:
> +
> +cpu at 0 {
> +       compatible = "arm,cortex-a9";
I am not sure how this works - would an example of OMAP4430, 60, 70
help? they have completely different OPP entries.

> +       reg = <0>;
> +       next-level-cache = <&L2>;
> +       operating-points = <
> +               /* kHz    uV    en */
> +               1200000 1275000 0
> +               996000  1225000 1
> +               792000  1100000 1
> +               672000  1100000 0
> +               396000  950000  1
> +               198000  850000  1

Just my 2cents, If we change en to be a flag:
0 - add, but disable
1 - add (enabled)
we could extend this further if the definition is a flag, for example:
2 - add and disable IF any of notifier chain return failure
3- add but dont call notifier chain (e.g. OPPs that are present for All SoC)

in addition, SoC might have additional properties associated with each
OPP such a flag
could be split up to mean lower 16 bits as OPP library flag and higher
16 bit to mean SoC custom flag

Example - On certain SoC a specific type of power technique is
supposed to be used per OPP, such a flag
passed on via notifiers to SoC handler might be capable of
centralizing the OPP information into the DT.

> +       >;
> +};
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index ac993ea..2d750f9 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -22,6 +22,7 @@
>  #include <linux/rculist.h>
>  #include <linux/rcupdate.h>
>  #include <linux/opp.h>
> +#include <linux/of.h>
>
>  /*
>   * Internal data structure organization with the OPP layer library is as
> @@ -674,3 +675,68 @@ struct srcu_notifier_head *opp_get_notifier(struct device *dev)
>
>         return &dev_opp->head;
>  }
> +
> +#ifdef CONFIG_OF
> +/**
> + * of_init_opp_table() - Initialize opp table from device tree
> + * @dev:       device pointer used to lookup device OPPs.
> + *
> + * Register the initial OPP table with the OPP library for given device.
> + */
> +int of_init_opp_table(struct device *dev)
> +{
> +       struct device_node *np = dev->of_node;
> +       const char *propname = "operating-points";
> +       const struct property *pp;
> +       u32 *opp;
> +       int ret, i, nr;
> +
> +       pp = of_find_property(np, propname, NULL);
> +       if (!pp) {
> +               dev_err(dev, "%s: Unable to find property", __func__);
> +               return -ENODEV;
> +       }
> +
> +       opp = kzalloc(pp->length, GFP_KERNEL);
> +       if (!opp) {
> +               dev_err(dev, "%s: Unable to allocate array\n", __func__);
> +               return -ENOMEM;
> +       }
> +
> +       nr = pp->length / sizeof(u32);
error warn if the pp->length is not multiple of u32? we also expect
later on to be a multiple of 3(f,v,disable

> +       ret = of_property_read_u32_array(np, propname, opp, nr);
> +       if (ret) {
> +               dev_err(dev, "%s: Unable to read OPPs\n", __func__);
> +               goto out;
> +       }
> +
> +       nr /= 3;
> +       for (i = 0; i < nr; i++) {
> +               /*
> +                * Each OPP is a set of tuples consisting of frequency,
> +                * voltage and availability like <freq-kHz vol-uV enable>.
> +                */
> +               u32 *val = opp + i * 3;
> +
> +               val[0] *= 1000;
> +               ret = opp_add(dev, val[0], val[1]);
> +               if (ret) {
> +                       dev_warn(dev, "%s: Failed to add OPP %d: %d\n",
> +                                __func__, val[0], ret);
> +                       continue;
> +               }
> +
> +               if (!val[2]) {
> +                       ret = opp_disable(dev, val[0]);
Since we are updating the table out of context of the SoC users,
consider what might happen if someone where to operate on the OPP
after opp_add, but before opp_disable?
instead of having the pain of adding an OPP and then disabling it,
since the code will now move to core OPP
library itself, wont it be better to hold dev_opp_list_lock and update
the full table with a proper list walk - yes
the current opp_add and opp_disable apis would need refactoring to be
reusable. It will also save on
synchronize_rcu calls on multiple iterations of the list.


> +                       if (ret)
> +                               dev_warn(dev, "%s: Failed to disable OPP %d: %d\n",
> +                                        __func__, val[0], ret);

umm... but we override the return with 0? OPP add failure might
indicate the table is invalid/corrupted - or no memory.
What is the point in populating a bad table up? having a singular
function with direct access to internal structures
might save us these un-necessary dilemma.


> +               }
> +       }
> +
> +       ret = 0;
> +out:
> +       kfree(opp);
> +       return ret;
> +}
> +#endif
> diff --git a/include/linux/opp.h b/include/linux/opp.h
> index 2a4e5fa..fd165ad 100644
> --- a/include/linux/opp.h
> +++ b/include/linux/opp.h
> @@ -48,6 +48,10 @@ int opp_disable(struct device *dev, unsigned long freq);
>
>  struct srcu_notifier_head *opp_get_notifier(struct device *dev);
>
> +#ifdef CONFIG_OF
> +int of_init_opp_table(struct device *dev);

#else
static inline int of_init_opp_table(struct device *dev) { return -EINVAL; }
?

> +#endif
> +
>  #else
>  static inline unsigned long opp_get_voltage(struct opp *opp)
>  {


Regards,
Nishanth Menon

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

[Shilimkar, Santosh]

On Thu, Jul 19, 2012 at 9:24 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
>
> From: Richard Zhao <richard.zhao@linaro.org>
>
> 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>
> Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
Nice. Avoids other driver duplicating this code.
May be you can keep Richard's credit in commit too.

FWIW, Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

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

[Shilimkar, Santosh]

On Thu, Jul 19, 2012 at 9:24 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
>
> From: Richard Zhao <richard.zhao@linaro.org>
>
> 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>
> Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
Nice. Avoids other driver duplicating this code.
May be you can keep Richard's credit in commit too.

FWIW, Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shilimkar, Santosh]

Shawn,

On Thu, Jul 19, 2012 at 9:24 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> It adds a generic cpufreq driver for CPU0 frequency management based on
> clk, regulator, OPP and device tree support.  It can support both
> uniprocessor (UP) and those symmetric multiprocessor (SMP) systems which
> share clock and voltage across all CPUs.
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---

[...]

> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index e24a2a1..e77a1e0 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -179,6 +179,17 @@ config CPU_FREQ_GOV_CONSERVATIVE
>
>           If in doubt, say N.
>
> +config GENERIC_CPUFREQ_CPU0
> +       bool "Generic cpufreq driver for CPU0"
> +       depends on HAVE_CLK && REGULATOR && PM_OPP && OF
> +       select CPU_FREQ_TABLE
> +       help
> +         This adds a generic cpufreq driver for CPU0 frequency management.
> +         It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> +         systems which share clock and voltage across all CPUs.
> +
> +         If in doubt, say N.
> +
This *_CPU0 doesn't seems to be appropriate since this infrastructure will be
used on SMP systems where CPUs shares clock/voltage rails. May be you can
get rid of that unless there is a strong need.

[...]

> +++ b/drivers/cpufreq/cpufreq-cpu0.c
> @@ -0,0 +1,235 @@
> +/*
> + * Copyright (C) 2012 Freescale Semiconductor, Inc.
> + *
> + * Reuse some codes from drivers/cpufreq/omap-cpufreq.c
> + *
> + * 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
> + */
> +
> +#define pr_fmt(fmt)    KBUILD_MODNAME ": " fmt
> +
> +#include <linux/clk.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/opp.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +/* voltage tolerance in percentage */
> +static unsigned int voltage_tolerance;
> +
> +static struct device *cpu_dev;
> +static struct clk *cpu_clk;
> +static struct regulator *cpu_reg;
> +static struct cpufreq_frequency_table *freq_table;
> +
> +static int cpu0_verify_speed(struct cpufreq_policy *policy)
> +{
> +       return cpufreq_frequency_table_verify(policy, freq_table);
> +}
> +
> +static unsigned int cpu0_get_speed(unsigned int cpu)
> +{
> +       return clk_get_rate(cpu_clk) / 1000;
> +}
> +
> +static int cpu0_set_target(struct cpufreq_policy *policy,
> +                          unsigned int target_freq, unsigned int relation)
> +{
> +       struct cpufreq_freqs freqs;
> +       struct opp *opp;
> +       unsigned long freq_Hz, volt = 0, volt_old = 0, tol = 0;
> +       unsigned int index, cpu;
> +       int ret = 0;
> +
> +       ret = cpufreq_frequency_table_target(policy, freq_table, target_freq,
> +                                            relation, &index);
> +       if (ret) {
> +               pr_err("failed to match target freqency %d: %d\n",
> +                      target_freq, ret);
> +               return ret;
> +       }
> +
> +       freq_Hz = clk_round_rate(cpu_clk, freq_table[index].frequency * 1000);
> +       if (freq_Hz < 0)
> +               freq_Hz = freq_table[index].frequency * 1000;
> +       freqs.new = freq_Hz / 1000;
> +       freqs.old = clk_get_rate(cpu_clk) / 1000;
> +
> +       if (freqs.old == freqs.new)
> +               return 0;
> +
> +       for_each_cpu(cpu, policy->cpus) {
You need for_each_online_cpu() here o.w you will end up calling the
notifier on CPU which is are hot-plugged out or not online yet.

> +               freqs.cpu = cpu;
> +               cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> +       }
> +
> +       if (cpu_reg) {
> +               opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
> +               if (IS_ERR(opp)) {
> +                       pr_err("failed to find OPP for %ld\n", freq_Hz);
> +                       return PTR_ERR(opp);
> +               }
> +               volt = opp_get_voltage(opp);
> +               tol = volt * voltage_tolerance / 100;
> +               volt_old = regulator_get_voltage(cpu_reg);
> +       }
> +
> +       pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
> +                freqs.old / 1000, volt_old ? volt_old / 1000 : -1,
> +                freqs.new / 1000, volt ? volt / 1000 : -1);
> +
> +       /* scaling up?  scale voltage before frequency */
> +       if (cpu_reg && freqs.new > freqs.old) {
> +               if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> +                       pr_warn("Unable to scale voltage up\n");
> +                       freqs.new = freqs.old;
> +                       goto done;
Do you need a POST notifier in case of the failure ?

> +               }
> +       }
> +
> +       ret = clk_set_rate(cpu_clk, freqs.new * 1000);
> +
> +       /* scaling down?  scale voltage after frequency */
> +       if (cpu_reg && (freqs.new < freqs.old)) {
> +               if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> +                       pr_warn("Unable to scale voltage down\n");
> +                       ret = clk_set_rate(cpu_clk, freqs.old * 1000);
> +                       freqs.new = freqs.old;
> +                       goto done;
Here too.

> +               }
> +       }
> +
> +done:
> +       for_each_cpu(cpu, policy->cpus) {
Same as first comment.

> +               freqs.cpu = cpu;
> +               cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> +       }
> +
> +       return ret;
> +}
> +
> +static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
> +{
> +       struct device_node *np;
> +       int ret;
> +
> +       /* Initialize resources via CPU0 */
> +       if (policy->cpu != 0)
> +               return -EINVAL;
> +
> +       np = of_find_node_by_path("/cpus/cpu@0");
> +       if (!np) {
> +               pr_err("failed to find cpu0 node\n");
> +               return -ENOENT;
> +       }
> +
> +       cpu_dev = get_cpu_device(0);
> +       if (!cpu_dev) {
> +               pr_err("failed to get cpu0 device\n");
> +               ret = -ENODEV;
> +               goto out_put_node;
> +       }
> +
> +       cpu_dev->of_node = np;
> +       ret = of_init_opp_table(cpu_dev);
> +       if (ret) {
> +               pr_err("failed to init OPP table\n");
> +               goto out_put_node;
> +       }
> +
> +       ret = opp_init_cpufreq_table(cpu_dev, &freq_table);
> +       if (ret) {
> +               pr_err("failed to init cpufreq table\n");
> +               goto out_put_node;
> +       }
> +
> +       ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> +       if (ret) {
> +               pr_err("invalid frequency table for cpu0\n");
> +               goto out_free_table;
> +       }
> +
> +       if (of_property_read_u32(np, "transition-latency",
> +                                &policy->cpuinfo.transition_latency))
> +               policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
> +
> +       of_property_read_u32(np, "voltage-tolerance", &voltage_tolerance);
> +
> +       cpu_clk = clk_get(cpu_dev, NULL);
> +       if (IS_ERR(cpu_clk)) {
> +               pr_err("failed to get cpu0 clock\n");
> +               ret = PTR_ERR(cpu_clk);
> +               goto out_free_table;
> +       }
> +       policy->cur = clk_get_rate(cpu_clk) / 1000;
> +
> +       cpu_reg = regulator_get(cpu_dev, "cpu0");
> +       if (IS_ERR(cpu_reg)) {
> +               pr_warn("failed to get cpu0 regulator\n");
> +               cpu_reg = NULL;
> +       }
> +
> +       /*
> +        * The driver only supports the SMP configuartion where all processors
> +        * share the clock and voltage and clock.  Use cpufreq affected_cpus
> +        * interface to have all CPUs scaled together.
> +        */
> +       policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> +       cpumask_setall(policy->cpus);
> +
> +       cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
> +
> +       of_node_put(np);
> +       return 0;
> +
> +out_free_table:
> +       opp_free_cpufreq_table(cpu_dev, &freq_table);
> +out_put_node:
> +       of_node_put(np);
> +       return ret;
> +}
> +
> +static int cpu0_cpufreq_exit(struct cpufreq_policy *policy)
> +{
> +       cpufreq_frequency_table_put_attr(policy->cpu);
> +       regulator_put(cpu_reg);
> +       clk_put(cpu_clk);
> +       opp_free_cpufreq_table(cpu_dev, &freq_table);
> +
> +       return 0;
> +}
> +
> +static struct freq_attr *cpu0_cpufreq_attr[] = {
> +       &cpufreq_freq_attr_scaling_available_freqs,
> +       NULL,
> +};
> +
> +static struct cpufreq_driver cpu0_cpufreq_driver = {
> +       .flags = CPUFREQ_STICKY,
> +       .verify = cpu0_verify_speed,
> +       .target = cpu0_set_target,
> +       .get = cpu0_get_speed,
> +       .init = cpu0_cpufreq_init,
> +       .exit = cpu0_cpufreq_exit,
> +       .name = "generic_cpu0",
> +       .attr = cpu0_cpufreq_attr,
> +};
> +
> +static int __devinit cpu0_cpufreq_driver_init(void)
> +{
> +       return cpufreq_register_driver(&cpu0_cpufreq_driver);
> +}
> +late_initcall(cpu0_cpufreq_driver_init);

Can you also add support to build this as loadable module ?

Regards
santosh

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shilimkar, Santosh]

Shawn,

On Thu, Jul 19, 2012 at 9:24 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> It adds a generic cpufreq driver for CPU0 frequency management based on
> clk, regulator, OPP and device tree support.  It can support both
> uniprocessor (UP) and those symmetric multiprocessor (SMP) systems which
> share clock and voltage across all CPUs.
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---

[...]

> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index e24a2a1..e77a1e0 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -179,6 +179,17 @@ config CPU_FREQ_GOV_CONSERVATIVE
>
>           If in doubt, say N.
>
> +config GENERIC_CPUFREQ_CPU0
> +       bool "Generic cpufreq driver for CPU0"
> +       depends on HAVE_CLK && REGULATOR && PM_OPP && OF
> +       select CPU_FREQ_TABLE
> +       help
> +         This adds a generic cpufreq driver for CPU0 frequency management.
> +         It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> +         systems which share clock and voltage across all CPUs.
> +
> +         If in doubt, say N.
> +
This *_CPU0 doesn't seems to be appropriate since this infrastructure will be
used on SMP systems where CPUs shares clock/voltage rails. May be you can
get rid of that unless there is a strong need.

[...]

> +++ b/drivers/cpufreq/cpufreq-cpu0.c
> @@ -0,0 +1,235 @@
> +/*
> + * Copyright (C) 2012 Freescale Semiconductor, Inc.
> + *
> + * Reuse some codes from drivers/cpufreq/omap-cpufreq.c
> + *
> + * 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
> + */
> +
> +#define pr_fmt(fmt)    KBUILD_MODNAME ": " fmt
> +
> +#include <linux/clk.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/opp.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +/* voltage tolerance in percentage */
> +static unsigned int voltage_tolerance;
> +
> +static struct device *cpu_dev;
> +static struct clk *cpu_clk;
> +static struct regulator *cpu_reg;
> +static struct cpufreq_frequency_table *freq_table;
> +
> +static int cpu0_verify_speed(struct cpufreq_policy *policy)
> +{
> +       return cpufreq_frequency_table_verify(policy, freq_table);
> +}
> +
> +static unsigned int cpu0_get_speed(unsigned int cpu)
> +{
> +       return clk_get_rate(cpu_clk) / 1000;
> +}
> +
> +static int cpu0_set_target(struct cpufreq_policy *policy,
> +                          unsigned int target_freq, unsigned int relation)
> +{
> +       struct cpufreq_freqs freqs;
> +       struct opp *opp;
> +       unsigned long freq_Hz, volt = 0, volt_old = 0, tol = 0;
> +       unsigned int index, cpu;
> +       int ret = 0;
> +
> +       ret = cpufreq_frequency_table_target(policy, freq_table, target_freq,
> +                                            relation, &index);
> +       if (ret) {
> +               pr_err("failed to match target freqency %d: %d\n",
> +                      target_freq, ret);
> +               return ret;
> +       }
> +
> +       freq_Hz = clk_round_rate(cpu_clk, freq_table[index].frequency * 1000);
> +       if (freq_Hz < 0)
> +               freq_Hz = freq_table[index].frequency * 1000;
> +       freqs.new = freq_Hz / 1000;
> +       freqs.old = clk_get_rate(cpu_clk) / 1000;
> +
> +       if (freqs.old == freqs.new)
> +               return 0;
> +
> +       for_each_cpu(cpu, policy->cpus) {
You need for_each_online_cpu() here o.w you will end up calling the
notifier on CPU which is are hot-plugged out or not online yet.

> +               freqs.cpu = cpu;
> +               cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> +       }
> +
> +       if (cpu_reg) {
> +               opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
> +               if (IS_ERR(opp)) {
> +                       pr_err("failed to find OPP for %ld\n", freq_Hz);
> +                       return PTR_ERR(opp);
> +               }
> +               volt = opp_get_voltage(opp);
> +               tol = volt * voltage_tolerance / 100;
> +               volt_old = regulator_get_voltage(cpu_reg);
> +       }
> +
> +       pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
> +                freqs.old / 1000, volt_old ? volt_old / 1000 : -1,
> +                freqs.new / 1000, volt ? volt / 1000 : -1);
> +
> +       /* scaling up?  scale voltage before frequency */
> +       if (cpu_reg && freqs.new > freqs.old) {
> +               if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> +                       pr_warn("Unable to scale voltage up\n");
> +                       freqs.new = freqs.old;
> +                       goto done;
Do you need a POST notifier in case of the failure ?

> +               }
> +       }
> +
> +       ret = clk_set_rate(cpu_clk, freqs.new * 1000);
> +
> +       /* scaling down?  scale voltage after frequency */
> +       if (cpu_reg && (freqs.new < freqs.old)) {
> +               if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> +                       pr_warn("Unable to scale voltage down\n");
> +                       ret = clk_set_rate(cpu_clk, freqs.old * 1000);
> +                       freqs.new = freqs.old;
> +                       goto done;
Here too.

> +               }
> +       }
> +
> +done:
> +       for_each_cpu(cpu, policy->cpus) {
Same as first comment.

> +               freqs.cpu = cpu;
> +               cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> +       }
> +
> +       return ret;
> +}
> +
> +static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
> +{
> +       struct device_node *np;
> +       int ret;
> +
> +       /* Initialize resources via CPU0 */
> +       if (policy->cpu != 0)
> +               return -EINVAL;
> +
> +       np = of_find_node_by_path("/cpus/cpu at 0");
> +       if (!np) {
> +               pr_err("failed to find cpu0 node\n");
> +               return -ENOENT;
> +       }
> +
> +       cpu_dev = get_cpu_device(0);
> +       if (!cpu_dev) {
> +               pr_err("failed to get cpu0 device\n");
> +               ret = -ENODEV;
> +               goto out_put_node;
> +       }
> +
> +       cpu_dev->of_node = np;
> +       ret = of_init_opp_table(cpu_dev);
> +       if (ret) {
> +               pr_err("failed to init OPP table\n");
> +               goto out_put_node;
> +       }
> +
> +       ret = opp_init_cpufreq_table(cpu_dev, &freq_table);
> +       if (ret) {
> +               pr_err("failed to init cpufreq table\n");
> +               goto out_put_node;
> +       }
> +
> +       ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> +       if (ret) {
> +               pr_err("invalid frequency table for cpu0\n");
> +               goto out_free_table;
> +       }
> +
> +       if (of_property_read_u32(np, "transition-latency",
> +                                &policy->cpuinfo.transition_latency))
> +               policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
> +
> +       of_property_read_u32(np, "voltage-tolerance", &voltage_tolerance);
> +
> +       cpu_clk = clk_get(cpu_dev, NULL);
> +       if (IS_ERR(cpu_clk)) {
> +               pr_err("failed to get cpu0 clock\n");
> +               ret = PTR_ERR(cpu_clk);
> +               goto out_free_table;
> +       }
> +       policy->cur = clk_get_rate(cpu_clk) / 1000;
> +
> +       cpu_reg = regulator_get(cpu_dev, "cpu0");
> +       if (IS_ERR(cpu_reg)) {
> +               pr_warn("failed to get cpu0 regulator\n");
> +               cpu_reg = NULL;
> +       }
> +
> +       /*
> +        * The driver only supports the SMP configuartion where all processors
> +        * share the clock and voltage and clock.  Use cpufreq affected_cpus
> +        * interface to have all CPUs scaled together.
> +        */
> +       policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> +       cpumask_setall(policy->cpus);
> +
> +       cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
> +
> +       of_node_put(np);
> +       return 0;
> +
> +out_free_table:
> +       opp_free_cpufreq_table(cpu_dev, &freq_table);
> +out_put_node:
> +       of_node_put(np);
> +       return ret;
> +}
> +
> +static int cpu0_cpufreq_exit(struct cpufreq_policy *policy)
> +{
> +       cpufreq_frequency_table_put_attr(policy->cpu);
> +       regulator_put(cpu_reg);
> +       clk_put(cpu_clk);
> +       opp_free_cpufreq_table(cpu_dev, &freq_table);
> +
> +       return 0;
> +}
> +
> +static struct freq_attr *cpu0_cpufreq_attr[] = {
> +       &cpufreq_freq_attr_scaling_available_freqs,
> +       NULL,
> +};
> +
> +static struct cpufreq_driver cpu0_cpufreq_driver = {
> +       .flags = CPUFREQ_STICKY,
> +       .verify = cpu0_verify_speed,
> +       .target = cpu0_set_target,
> +       .get = cpu0_get_speed,
> +       .init = cpu0_cpufreq_init,
> +       .exit = cpu0_cpufreq_exit,
> +       .name = "generic_cpu0",
> +       .attr = cpu0_cpufreq_attr,
> +};
> +
> +static int __devinit cpu0_cpufreq_driver_init(void)
> +{
> +       return cpufreq_register_driver(&cpu0_cpufreq_driver);
> +}
> +late_initcall(cpu0_cpufreq_driver_init);

Can you also add support to build this as loadable module ?

Regards
santosh

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

[Shawn Guo]

On Fri, Jul 20, 2012 at 12:06:50PM +0530, Shilimkar, Santosh wrote:
> On Thu, Jul 19, 2012 at 9:24 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> >
> > From: Richard Zhao <richard.zhao@linaro.org>
> >
> > 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>
> > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> Nice. Avoids other driver duplicating this code.
> May be you can keep Richard's credit in commit too.
> 
It's all Richard's credit in the commit.

> FWIW, Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Thanks.

-- 
Regards,
Shawn

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

[Shawn Guo]

On Fri, Jul 20, 2012 at 12:06:50PM +0530, Shilimkar, Santosh wrote:
> On Thu, Jul 19, 2012 at 9:24 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> >
> > From: Richard Zhao <richard.zhao@linaro.org>
> >
> > 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>
> > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> Nice. Avoids other driver duplicating this code.
> May be you can keep Richard's credit in commit too.
> 
It's all Richard's credit in the commit.

> FWIW, Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Thanks.

-- 
Regards,
Shawn

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

[Shilimkar, Santosh]

n Fri, Jul 20, 2012 at 1:26 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Fri, Jul 20, 2012 at 12:06:50PM +0530, Shilimkar, Santosh wrote:
>> On Thu, Jul 19, 2012 at 9:24 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
>> >
>> > From: Richard Zhao <richard.zhao@linaro.org>
>> >
>> > 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>
>> > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
>> > ---
>> Nice. Avoids other driver duplicating this code.
>> May be you can keep Richard's credit in commit too.
>>
> It's all Richard's credit in the commit.
>
Sorry.... I missed that :-(

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

[Shilimkar, Santosh]

n Fri, Jul 20, 2012 at 1:26 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Fri, Jul 20, 2012 at 12:06:50PM +0530, Shilimkar, Santosh wrote:
>> On Thu, Jul 19, 2012 at 9:24 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
>> >
>> > From: Richard Zhao <richard.zhao@linaro.org>
>> >
>> > 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>
>> > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
>> > ---
>> Nice. Avoids other driver duplicating this code.
>> May be you can keep Richard's credit in commit too.
>>
> It's all Richard's credit in the commit.
>
Sorry.... I missed that :-(

Re: [PATCH 2/3] PM / OPP: Initialize OPP table from device tree

[Shawn Guo]

Thanks for reviewing it, Nishanth.

On Fri, Jul 20, 2012 at 01:00:26AM -0500, Menon, Nishanth wrote:
> > +cpu@0 {
> > +       compatible = "arm,cortex-a9";
> I am not sure how this works - would an example of OMAP4430, 60, 70
> help? they have completely different OPP entries.
> 
Basically, patch #3 is a user of this and shows how this works.

> > +       reg = <0>;
> > +       next-level-cache = <&L2>;
> > +       operating-points = <
> > +               /* kHz    uV    en */
> > +               1200000 1275000 0
> > +               996000  1225000 1
> > +               792000  1100000 1
> > +               672000  1100000 0
> > +               396000  950000  1
> > +               198000  850000  1
> 
> Just my 2cents, If we change en to be a flag:
> 0 - add, but disable
> 1 - add (enabled)
> we could extend this further if the definition is a flag, for example:
> 2 - add and disable IF any of notifier chain return failure
> 3- add but dont call notifier chain (e.g. OPPs that are present for All SoC)
> 
> in addition, SoC might have additional properties associated with each
> OPP such a flag
> could be split up to mean lower 16 bits as OPP library flag and higher
> 16 bit to mean SoC custom flag
> 
> Example - On certain SoC a specific type of power technique is
> supposed to be used per OPP, such a flag
> passed on via notifiers to SoC handler might be capable of
> centralizing the OPP information into the DT.
> 
I do not follow on the idea of having the third tuple being a flag.
The binding is only meant to represent the aspects of operating-points,
while operating-points are all about frequency and voltage, nothing
else.  No Linux implementation details should be encoded here.  I'm
even a little unhappy about having enabling/availability in the third
tuple.  If anyone complains about that,  I will remove it from the
binding without a hesitation.

> > +       >;
> > +};
> > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> > index ac993ea..2d750f9 100644
> > --- a/drivers/base/power/opp.c
> > +++ b/drivers/base/power/opp.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/rculist.h>
> >  #include <linux/rcupdate.h>
> >  #include <linux/opp.h>
> > +#include <linux/of.h>
> >
> >  /*
> >   * Internal data structure organization with the OPP layer library is as
> > @@ -674,3 +675,68 @@ struct srcu_notifier_head *opp_get_notifier(struct device *dev)
> >
> >         return &dev_opp->head;
> >  }
> > +
> > +#ifdef CONFIG_OF
> > +/**
> > + * of_init_opp_table() - Initialize opp table from device tree
> > + * @dev:       device pointer used to lookup device OPPs.
> > + *
> > + * Register the initial OPP table with the OPP library for given device.
> > + */
> > +int of_init_opp_table(struct device *dev)
> > +{
> > +       struct device_node *np = dev->of_node;
> > +       const char *propname = "operating-points";
> > +       const struct property *pp;
> > +       u32 *opp;
> > +       int ret, i, nr;
> > +
> > +       pp = of_find_property(np, propname, NULL);
> > +       if (!pp) {
> > +               dev_err(dev, "%s: Unable to find property", __func__);
> > +               return -ENODEV;
> > +       }
> > +
> > +       opp = kzalloc(pp->length, GFP_KERNEL);
> > +       if (!opp) {
> > +               dev_err(dev, "%s: Unable to allocate array\n", __func__);
> > +               return -ENOMEM;
> > +       }
> > +
> > +       nr = pp->length / sizeof(u32);
> error warn if the pp->length is not multiple of u32?

The DT core ensures that any integer encoded there will an u32.

> we also expect
> later on to be a multiple of 3(f,v,disable
> 
Yes, I thought about checking that, but I chose to simply ignore those
suspicious tuples at the end of the list.  I will add the check for
that, since you commented here.

> > +       ret = of_property_read_u32_array(np, propname, opp, nr);
> > +       if (ret) {
> > +               dev_err(dev, "%s: Unable to read OPPs\n", __func__);
> > +               goto out;
> > +       }
> > +
> > +       nr /= 3;
> > +       for (i = 0; i < nr; i++) {
> > +               /*
> > +                * Each OPP is a set of tuples consisting of frequency,
> > +                * voltage and availability like <freq-kHz vol-uV enable>.
> > +                */
> > +               u32 *val = opp + i * 3;
> > +
> > +               val[0] *= 1000;
> > +               ret = opp_add(dev, val[0], val[1]);
> > +               if (ret) {
> > +                       dev_warn(dev, "%s: Failed to add OPP %d: %d\n",
> > +                                __func__, val[0], ret);
> > +                       continue;
> > +               }
> > +
> > +               if (!val[2]) {
> > +                       ret = opp_disable(dev, val[0]);
> Since we are updating the table out of context of the SoC users,
> consider what might happen if someone where to operate on the OPP
> after opp_add, but before opp_disable?

I would take this as another comment reminding me to remove the
enabling/availability tuple from the binding.  Will do it in the
next version.

> instead of having the pain of adding an OPP and then disabling it,
> since the code will now move to core OPP
> library itself, wont it be better to hold dev_opp_list_lock and update
> the full table with a proper list walk - yes
> the current opp_add and opp_disable apis would need refactoring to be
> reusable. It will also save on
> synchronize_rcu calls on multiple iterations of the list.
> 
> 
> > +                       if (ret)
> > +                               dev_warn(dev, "%s: Failed to disable OPP %d: %d\n",
> > +                                        __func__, val[0], ret);
> 
> umm... but we override the return with 0? OPP add failure might
> indicate the table is invalid/corrupted - or no memory.
> What is the point in populating a bad table up? having a singular
> function with direct access to internal structures
> might save us these un-necessary dilemma.
> 
The return overriding only happens when opp_add or opp_disable call
fails on particular entry.  That does not necessarily mean the whole
OPP table is completely invalid/corrupted.  A warn message is enough,
IMO.

> 
> > +               }
> > +       }
> > +
> > +       ret = 0;
> > +out:
> > +       kfree(opp);
> > +       return ret;
> > +}
> > +#endif
> > diff --git a/include/linux/opp.h b/include/linux/opp.h
> > index 2a4e5fa..fd165ad 100644
> > --- a/include/linux/opp.h
> > +++ b/include/linux/opp.h
> > @@ -48,6 +48,10 @@ int opp_disable(struct device *dev, unsigned long freq);
> >
> >  struct srcu_notifier_head *opp_get_notifier(struct device *dev);
> >
> > +#ifdef CONFIG_OF
> > +int of_init_opp_table(struct device *dev);
> 
> #else
> static inline int of_init_opp_table(struct device *dev) { return -EINVAL; }
> ?
> 
Sounds good.

Regards,
Shawn

> > +#endif
> > +
> >  #else
> >  static inline unsigned long opp_get_voltage(struct opp *opp)
> >  {
> 
> 
> Regards,
> Nishanth Menon

[PATCH 2/3] PM / OPP: Initialize OPP table from device tree

[Shawn Guo]

Thanks for reviewing it, Nishanth.

On Fri, Jul 20, 2012 at 01:00:26AM -0500, Menon, Nishanth wrote:
> > +cpu at 0 {
> > +       compatible = "arm,cortex-a9";
> I am not sure how this works - would an example of OMAP4430, 60, 70
> help? they have completely different OPP entries.
> 
Basically, patch #3 is a user of this and shows how this works.

> > +       reg = <0>;
> > +       next-level-cache = <&L2>;
> > +       operating-points = <
> > +               /* kHz    uV    en */
> > +               1200000 1275000 0
> > +               996000  1225000 1
> > +               792000  1100000 1
> > +               672000  1100000 0
> > +               396000  950000  1
> > +               198000  850000  1
> 
> Just my 2cents, If we change en to be a flag:
> 0 - add, but disable
> 1 - add (enabled)
> we could extend this further if the definition is a flag, for example:
> 2 - add and disable IF any of notifier chain return failure
> 3- add but dont call notifier chain (e.g. OPPs that are present for All SoC)
> 
> in addition, SoC might have additional properties associated with each
> OPP such a flag
> could be split up to mean lower 16 bits as OPP library flag and higher
> 16 bit to mean SoC custom flag
> 
> Example - On certain SoC a specific type of power technique is
> supposed to be used per OPP, such a flag
> passed on via notifiers to SoC handler might be capable of
> centralizing the OPP information into the DT.
> 
I do not follow on the idea of having the third tuple being a flag.
The binding is only meant to represent the aspects of operating-points,
while operating-points are all about frequency and voltage, nothing
else.  No Linux implementation details should be encoded here.  I'm
even a little unhappy about having enabling/availability in the third
tuple.  If anyone complains about that,  I will remove it from the
binding without a hesitation.

> > +       >;
> > +};
> > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> > index ac993ea..2d750f9 100644
> > --- a/drivers/base/power/opp.c
> > +++ b/drivers/base/power/opp.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/rculist.h>
> >  #include <linux/rcupdate.h>
> >  #include <linux/opp.h>
> > +#include <linux/of.h>
> >
> >  /*
> >   * Internal data structure organization with the OPP layer library is as
> > @@ -674,3 +675,68 @@ struct srcu_notifier_head *opp_get_notifier(struct device *dev)
> >
> >         return &dev_opp->head;
> >  }
> > +
> > +#ifdef CONFIG_OF
> > +/**
> > + * of_init_opp_table() - Initialize opp table from device tree
> > + * @dev:       device pointer used to lookup device OPPs.
> > + *
> > + * Register the initial OPP table with the OPP library for given device.
> > + */
> > +int of_init_opp_table(struct device *dev)
> > +{
> > +       struct device_node *np = dev->of_node;
> > +       const char *propname = "operating-points";
> > +       const struct property *pp;
> > +       u32 *opp;
> > +       int ret, i, nr;
> > +
> > +       pp = of_find_property(np, propname, NULL);
> > +       if (!pp) {
> > +               dev_err(dev, "%s: Unable to find property", __func__);
> > +               return -ENODEV;
> > +       }
> > +
> > +       opp = kzalloc(pp->length, GFP_KERNEL);
> > +       if (!opp) {
> > +               dev_err(dev, "%s: Unable to allocate array\n", __func__);
> > +               return -ENOMEM;
> > +       }
> > +
> > +       nr = pp->length / sizeof(u32);
> error warn if the pp->length is not multiple of u32?

The DT core ensures that any integer encoded there will an u32.

> we also expect
> later on to be a multiple of 3(f,v,disable
> 
Yes, I thought about checking that, but I chose to simply ignore those
suspicious tuples at the end of the list.  I will add the check for
that, since you commented here.

> > +       ret = of_property_read_u32_array(np, propname, opp, nr);
> > +       if (ret) {
> > +               dev_err(dev, "%s: Unable to read OPPs\n", __func__);
> > +               goto out;
> > +       }
> > +
> > +       nr /= 3;
> > +       for (i = 0; i < nr; i++) {
> > +               /*
> > +                * Each OPP is a set of tuples consisting of frequency,
> > +                * voltage and availability like <freq-kHz vol-uV enable>.
> > +                */
> > +               u32 *val = opp + i * 3;
> > +
> > +               val[0] *= 1000;
> > +               ret = opp_add(dev, val[0], val[1]);
> > +               if (ret) {
> > +                       dev_warn(dev, "%s: Failed to add OPP %d: %d\n",
> > +                                __func__, val[0], ret);
> > +                       continue;
> > +               }
> > +
> > +               if (!val[2]) {
> > +                       ret = opp_disable(dev, val[0]);
> Since we are updating the table out of context of the SoC users,
> consider what might happen if someone where to operate on the OPP
> after opp_add, but before opp_disable?

I would take this as another comment reminding me to remove the
enabling/availability tuple from the binding.  Will do it in the
next version.

> instead of having the pain of adding an OPP and then disabling it,
> since the code will now move to core OPP
> library itself, wont it be better to hold dev_opp_list_lock and update
> the full table with a proper list walk - yes
> the current opp_add and opp_disable apis would need refactoring to be
> reusable. It will also save on
> synchronize_rcu calls on multiple iterations of the list.
> 
> 
> > +                       if (ret)
> > +                               dev_warn(dev, "%s: Failed to disable OPP %d: %d\n",
> > +                                        __func__, val[0], ret);
> 
> umm... but we override the return with 0? OPP add failure might
> indicate the table is invalid/corrupted - or no memory.
> What is the point in populating a bad table up? having a singular
> function with direct access to internal structures
> might save us these un-necessary dilemma.
> 
The return overriding only happens when opp_add or opp_disable call
fails on particular entry.  That does not necessarily mean the whole
OPP table is completely invalid/corrupted.  A warn message is enough,
IMO.

> 
> > +               }
> > +       }
> > +
> > +       ret = 0;
> > +out:
> > +       kfree(opp);
> > +       return ret;
> > +}
> > +#endif
> > diff --git a/include/linux/opp.h b/include/linux/opp.h
> > index 2a4e5fa..fd165ad 100644
> > --- a/include/linux/opp.h
> > +++ b/include/linux/opp.h
> > @@ -48,6 +48,10 @@ int opp_disable(struct device *dev, unsigned long freq);
> >
> >  struct srcu_notifier_head *opp_get_notifier(struct device *dev);
> >
> > +#ifdef CONFIG_OF
> > +int of_init_opp_table(struct device *dev);
> 
> #else
> static inline int of_init_opp_table(struct device *dev) { return -EINVAL; }
> ?
> 
Sounds good.

Regards,
Shawn

> > +#endif
> > +
> >  #else
> >  static inline unsigned long opp_get_voltage(struct opp *opp)
> >  {
> 
> 
> Regards,
> Nishanth Menon

Re: [PATCH 2/3] PM / OPP: Initialize OPP table from device tree

[Menon, Nishanth]

On Fri, Jul 20, 2012 at 3:46 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
>
>> > +       ret = of_property_read_u32_array(np, propname, opp, nr);
>> > +       if (ret) {
>> > +               dev_err(dev, "%s: Unable to read OPPs\n", __func__);
>> > +               goto out;
>> > +       }
>> > +
>> > +       nr /= 3;
>> > +       for (i = 0; i < nr; i++) {
>> > +               /*
>> > +                * Each OPP is a set of tuples consisting of frequency,
>> > +                * voltage and availability like <freq-kHz vol-uV enable>.
>> > +                */
>> > +               u32 *val = opp + i * 3;
>> > +
>> > +               val[0] *= 1000;
>> > +               ret = opp_add(dev, val[0], val[1]);
>> > +               if (ret) {
>> > +                       dev_warn(dev, "%s: Failed to add OPP %d: %d\n",
>> > +                                __func__, val[0], ret);
>> > +                       continue;
>> > +               }
>> > +
>> > +               if (!val[2]) {
>> > +                       ret = opp_disable(dev, val[0]);
>> Since we are updating the table out of context of the SoC users,
>> consider what might happen if someone where to operate on the OPP
>> after opp_add, but before opp_disable?
>
> I would take this as another comment reminding me to remove the
> enabling/availability tuple from the binding.  Will do it in the
> next version.
I am not completely convinced that dropping the flag would be the best approach
on a long run, but might be a good starting point while we meet current reqs and
as a need arises, could increase the scope. Thanks once again for doing this.

Regards,
Nishanth Menon

[PATCH 2/3] PM / OPP: Initialize OPP table from device tree

[Menon, Nishanth]

On Fri, Jul 20, 2012 at 3:46 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
>
>> > +       ret = of_property_read_u32_array(np, propname, opp, nr);
>> > +       if (ret) {
>> > +               dev_err(dev, "%s: Unable to read OPPs\n", __func__);
>> > +               goto out;
>> > +       }
>> > +
>> > +       nr /= 3;
>> > +       for (i = 0; i < nr; i++) {
>> > +               /*
>> > +                * Each OPP is a set of tuples consisting of frequency,
>> > +                * voltage and availability like <freq-kHz vol-uV enable>.
>> > +                */
>> > +               u32 *val = opp + i * 3;
>> > +
>> > +               val[0] *= 1000;
>> > +               ret = opp_add(dev, val[0], val[1]);
>> > +               if (ret) {
>> > +                       dev_warn(dev, "%s: Failed to add OPP %d: %d\n",
>> > +                                __func__, val[0], ret);
>> > +                       continue;
>> > +               }
>> > +
>> > +               if (!val[2]) {
>> > +                       ret = opp_disable(dev, val[0]);
>> Since we are updating the table out of context of the SoC users,
>> consider what might happen if someone where to operate on the OPP
>> after opp_add, but before opp_disable?
>
> I would take this as another comment reminding me to remove the
> enabling/availability tuple from the binding.  Will do it in the
> next version.
I am not completely convinced that dropping the flag would be the best approach
on a long run, but might be a good starting point while we meet current reqs and
as a need arises, could increase the scope. Thanks once again for doing this.

Regards,
Nishanth Menon

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

Thanks for the reviewing, Santosh.

On Fri, Jul 20, 2012 at 12:22:14PM +0530, Shilimkar, Santosh wrote:
> > +config GENERIC_CPUFREQ_CPU0
> > +       bool "Generic cpufreq driver for CPU0"
> > +       depends on HAVE_CLK && REGULATOR && PM_OPP && OF
> > +       select CPU_FREQ_TABLE
> > +       help
> > +         This adds a generic cpufreq driver for CPU0 frequency management.
> > +         It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> > +         systems which share clock and voltage across all CPUs.
> > +
> > +         If in doubt, say N.
> > +
> This *_CPU0 doesn't seems to be appropriate since this infrastructure will be
> used on SMP systems where CPUs shares clock/voltage rails. May be you can
> get rid of that unless there is a strong need.
> 
All the resource handlers, clk, regulator, opp, DT node, are retrieved
from CPU0 device even for SMP.  I hope the naming of the driver could
tell:

- The driver supports UP
- The driver supports SMP where CPUs shares clock/voltage rails by
  managing CPU0 (resource)
- The driver does not support SMP where individual CPU can scale
  clock/voltage independently.

I also thought about naming the driver cpufreq-coupled, but the name
misses the implication of UP support somehow, so I chose cpufreq-cpu0.

> > +static int cpu0_set_target(struct cpufreq_policy *policy,
> > +                          unsigned int target_freq, unsigned int relation)
> > +{
> > +       struct cpufreq_freqs freqs;
> > +       struct opp *opp;
> > +       unsigned long freq_Hz, volt = 0, volt_old = 0, tol = 0;
> > +       unsigned int index, cpu;
> > +       int ret = 0;
> > +
> > +       ret = cpufreq_frequency_table_target(policy, freq_table, target_freq,
> > +                                            relation, &index);
> > +       if (ret) {
> > +               pr_err("failed to match target freqency %d: %d\n",
> > +                      target_freq, ret);
> > +               return ret;
> > +       }
> > +
> > +       freq_Hz = clk_round_rate(cpu_clk, freq_table[index].frequency * 1000);
> > +       if (freq_Hz < 0)
> > +               freq_Hz = freq_table[index].frequency * 1000;
> > +       freqs.new = freq_Hz / 1000;
> > +       freqs.old = clk_get_rate(cpu_clk) / 1000;
> > +
> > +       if (freqs.old == freqs.new)
> > +               return 0;
> > +
> > +       for_each_cpu(cpu, policy->cpus) {
> You need for_each_online_cpu() here o.w you will end up calling the
> notifier on CPU which is are hot-plugged out or not online yet.
> 
Yes, that's sensible.  Since we have all the CPUs set in policy->cpus
in this driver, we do not have to enumerate policy->cpus, and checking
online CPUs should be better for the cases you mentioned.

> > +               freqs.cpu = cpu;
> > +               cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > +       }
> > +
> > +       if (cpu_reg) {
> > +               opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
> > +               if (IS_ERR(opp)) {
> > +                       pr_err("failed to find OPP for %ld\n", freq_Hz);
> > +                       return PTR_ERR(opp);
> > +               }
> > +               volt = opp_get_voltage(opp);
> > +               tol = volt * voltage_tolerance / 100;
> > +               volt_old = regulator_get_voltage(cpu_reg);
> > +       }
> > +
> > +       pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
> > +                freqs.old / 1000, volt_old ? volt_old / 1000 : -1,
> > +                freqs.new / 1000, volt ? volt / 1000 : -1);
> > +
> > +       /* scaling up?  scale voltage before frequency */
> > +       if (cpu_reg && freqs.new > freqs.old) {
> > +               if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> > +                       pr_warn("Unable to scale voltage up\n");
> > +                       freqs.new = freqs.old;
> > +                       goto done;
> Do you need a POST notifier in case of the failure ?
> 
Honestly, I'm not quite sure about that.  This is a piece of code
reused from omap-cpufreq driver.  Looking at cpufreq_notify_transition,
it seems to me that the POST notifier is not really necessary for
failure case.

> > +static struct cpufreq_driver cpu0_cpufreq_driver = {
> > +       .flags = CPUFREQ_STICKY,
> > +       .verify = cpu0_verify_speed,
> > +       .target = cpu0_set_target,
> > +       .get = cpu0_get_speed,
> > +       .init = cpu0_cpufreq_init,
> > +       .exit = cpu0_cpufreq_exit,
> > +       .name = "generic_cpu0",
> > +       .attr = cpu0_cpufreq_attr,
> > +};
> > +
> > +static int __devinit cpu0_cpufreq_driver_init(void)
> > +{
> > +       return cpufreq_register_driver(&cpu0_cpufreq_driver);
> > +}
> > +late_initcall(cpu0_cpufreq_driver_init);
> 
> Can you also add support to build this as loadable module ?
> 
I'm just following the common pattern of ARM cpufreq drivers to have
CPUFREQ_STICKY set in cpufreq_driver.flag and use "bool" for the driver
Kconfig.  I'm not sure about the necessity of building this as a module.

-- 
Regards,
Shawn

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

Thanks for the reviewing, Santosh.

On Fri, Jul 20, 2012 at 12:22:14PM +0530, Shilimkar, Santosh wrote:
> > +config GENERIC_CPUFREQ_CPU0
> > +       bool "Generic cpufreq driver for CPU0"
> > +       depends on HAVE_CLK && REGULATOR && PM_OPP && OF
> > +       select CPU_FREQ_TABLE
> > +       help
> > +         This adds a generic cpufreq driver for CPU0 frequency management.
> > +         It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> > +         systems which share clock and voltage across all CPUs.
> > +
> > +         If in doubt, say N.
> > +
> This *_CPU0 doesn't seems to be appropriate since this infrastructure will be
> used on SMP systems where CPUs shares clock/voltage rails. May be you can
> get rid of that unless there is a strong need.
> 
All the resource handlers, clk, regulator, opp, DT node, are retrieved
from CPU0 device even for SMP.  I hope the naming of the driver could
tell:

- The driver supports UP
- The driver supports SMP where CPUs shares clock/voltage rails by
  managing CPU0 (resource)
- The driver does not support SMP where individual CPU can scale
  clock/voltage independently.

I also thought about naming the driver cpufreq-coupled, but the name
misses the implication of UP support somehow, so I chose cpufreq-cpu0.

> > +static int cpu0_set_target(struct cpufreq_policy *policy,
> > +                          unsigned int target_freq, unsigned int relation)
> > +{
> > +       struct cpufreq_freqs freqs;
> > +       struct opp *opp;
> > +       unsigned long freq_Hz, volt = 0, volt_old = 0, tol = 0;
> > +       unsigned int index, cpu;
> > +       int ret = 0;
> > +
> > +       ret = cpufreq_frequency_table_target(policy, freq_table, target_freq,
> > +                                            relation, &index);
> > +       if (ret) {
> > +               pr_err("failed to match target freqency %d: %d\n",
> > +                      target_freq, ret);
> > +               return ret;
> > +       }
> > +
> > +       freq_Hz = clk_round_rate(cpu_clk, freq_table[index].frequency * 1000);
> > +       if (freq_Hz < 0)
> > +               freq_Hz = freq_table[index].frequency * 1000;
> > +       freqs.new = freq_Hz / 1000;
> > +       freqs.old = clk_get_rate(cpu_clk) / 1000;
> > +
> > +       if (freqs.old == freqs.new)
> > +               return 0;
> > +
> > +       for_each_cpu(cpu, policy->cpus) {
> You need for_each_online_cpu() here o.w you will end up calling the
> notifier on CPU which is are hot-plugged out or not online yet.
> 
Yes, that's sensible.  Since we have all the CPUs set in policy->cpus
in this driver, we do not have to enumerate policy->cpus, and checking
online CPUs should be better for the cases you mentioned.

> > +               freqs.cpu = cpu;
> > +               cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > +       }
> > +
> > +       if (cpu_reg) {
> > +               opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
> > +               if (IS_ERR(opp)) {
> > +                       pr_err("failed to find OPP for %ld\n", freq_Hz);
> > +                       return PTR_ERR(opp);
> > +               }
> > +               volt = opp_get_voltage(opp);
> > +               tol = volt * voltage_tolerance / 100;
> > +               volt_old = regulator_get_voltage(cpu_reg);
> > +       }
> > +
> > +       pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
> > +                freqs.old / 1000, volt_old ? volt_old / 1000 : -1,
> > +                freqs.new / 1000, volt ? volt / 1000 : -1);
> > +
> > +       /* scaling up?  scale voltage before frequency */
> > +       if (cpu_reg && freqs.new > freqs.old) {
> > +               if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> > +                       pr_warn("Unable to scale voltage up\n");
> > +                       freqs.new = freqs.old;
> > +                       goto done;
> Do you need a POST notifier in case of the failure ?
> 
Honestly, I'm not quite sure about that.  This is a piece of code
reused from omap-cpufreq driver.  Looking at cpufreq_notify_transition,
it seems to me that the POST notifier is not really necessary for
failure case.

> > +static struct cpufreq_driver cpu0_cpufreq_driver = {
> > +       .flags = CPUFREQ_STICKY,
> > +       .verify = cpu0_verify_speed,
> > +       .target = cpu0_set_target,
> > +       .get = cpu0_get_speed,
> > +       .init = cpu0_cpufreq_init,
> > +       .exit = cpu0_cpufreq_exit,
> > +       .name = "generic_cpu0",
> > +       .attr = cpu0_cpufreq_attr,
> > +};
> > +
> > +static int __devinit cpu0_cpufreq_driver_init(void)
> > +{
> > +       return cpufreq_register_driver(&cpu0_cpufreq_driver);
> > +}
> > +late_initcall(cpu0_cpufreq_driver_init);
> 
> Can you also add support to build this as loadable module ?
> 
I'm just following the common pattern of ARM cpufreq drivers to have
CPUFREQ_STICKY set in cpufreq_driver.flag and use "bool" for the driver
Kconfig.  I'm not sure about the necessity of building this as a module.

-- 
Regards,
Shawn

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Richard Zhao]

Hi, Shawn,

I find you create a new driver rather than work on my generic cpufreq driver based on clk and regulator.
Is there any reason besides adding opp support?

Thanks
Richard
-- 
抱歉暂时无法详细说明。这份邮件是使用安装有K-9 Mail的Android移动设备发送的。

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Richard Zhao]

Hi, Shawn,

I find you create a new driver rather than work on my generic cpufreq driver based on clk and regulator.
Is there any reason besides adding opp support?

Thanks
Richard
-- 
?????????????????????K-9 Mail?Android????????

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Fri, Jul 20, 2012 at 08:51:17PM +0800, Richard Zhao wrote:
> Hi, Shawn,
> 
> I find you create a new driver rather than work on my generic cpufreq driver based on clk and regulator.
> Is there any reason besides adding opp support?
> 
Adopting OPP support forces me to look at omap-cpufreq driver, hence
makes me create a new driver by referencing to omap-cpufreq.

-- 
Regards,
Shawn

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Fri, Jul 20, 2012 at 08:51:17PM +0800, Richard Zhao wrote:
> Hi, Shawn,
> 
> I find you create a new driver rather than work on my generic cpufreq driver based on clk and regulator.
> Is there any reason besides adding opp support?
> 
Adopting OPP support forces me to look at omap-cpufreq driver, hence
makes me create a new driver by referencing to omap-cpufreq.

-- 
Regards,
Shawn

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Turquette, Mike]

On Fri, Jul 20, 2012 at 5:33 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Fri, Jul 20, 2012 at 12:22:14PM +0530, Shilimkar, Santosh wrote:
>> This *_CPU0 doesn't seems to be appropriate since this infrastructure will be
>> used on SMP systems where CPUs shares clock/voltage rails. May be you can
>> get rid of that unless there is a strong need.
>>
> All the resource handlers, clk, regulator, opp, DT node, are retrieved
> from CPU0 device even for SMP.  I hope the naming of the driver could
> tell:
>
> - The driver supports UP
> - The driver supports SMP where CPUs shares clock/voltage rails by
>   managing CPU0 (resource)
> - The driver does not support SMP where individual CPU can scale
>   clock/voltage independently.
>

How about cpufreq-single-thread.c and CONFIG_CPUFREQ_SINGLE_THREAD?
That makes sense for both UP and SMP.

Regards,
Mike

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Turquette, Mike]

On Fri, Jul 20, 2012 at 5:33 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Fri, Jul 20, 2012 at 12:22:14PM +0530, Shilimkar, Santosh wrote:
>> This *_CPU0 doesn't seems to be appropriate since this infrastructure will be
>> used on SMP systems where CPUs shares clock/voltage rails. May be you can
>> get rid of that unless there is a strong need.
>>
> All the resource handlers, clk, regulator, opp, DT node, are retrieved
> from CPU0 device even for SMP.  I hope the naming of the driver could
> tell:
>
> - The driver supports UP
> - The driver supports SMP where CPUs shares clock/voltage rails by
>   managing CPU0 (resource)
> - The driver does not support SMP where individual CPU can scale
>   clock/voltage independently.
>

How about cpufreq-single-thread.c and CONFIG_CPUFREQ_SINGLE_THREAD?
That makes sense for both UP and SMP.

Regards,
Mike

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shilimkar, Santosh]

On Fri, Jul 20, 2012 at 9:20 PM, Turquette, Mike <mturquette@ti.com> wrote:
> On Fri, Jul 20, 2012 at 5:33 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
>> On Fri, Jul 20, 2012 at 12:22:14PM +0530, Shilimkar, Santosh wrote:
>>> This *_CPU0 doesn't seems to be appropriate since this infrastructure will be
>>> used on SMP systems where CPUs shares clock/voltage rails. May be you can
>>> get rid of that unless there is a strong need.
>>>
>> All the resource handlers, clk, regulator, opp, DT node, are retrieved
>> from CPU0 device even for SMP.  I hope the naming of the driver could
>> tell:
>>
>> - The driver supports UP
>> - The driver supports SMP where CPUs shares clock/voltage rails by
>>   managing CPU0 (resource)
>> - The driver does not support SMP where individual CPU can scale
>>   clock/voltage independently.
>>
>
> How about cpufreq-single-thread.c and CONFIG_CPUFREQ_SINGLE_THREAD?
> That makes sense for both UP and SMP.
>
Indeed. This sounds more appropriate and also reflects what actually happens
with a UP or shared clock SMP case.

Regards
Santosh

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shilimkar, Santosh]

On Fri, Jul 20, 2012 at 9:20 PM, Turquette, Mike <mturquette@ti.com> wrote:
> On Fri, Jul 20, 2012 at 5:33 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
>> On Fri, Jul 20, 2012 at 12:22:14PM +0530, Shilimkar, Santosh wrote:
>>> This *_CPU0 doesn't seems to be appropriate since this infrastructure will be
>>> used on SMP systems where CPUs shares clock/voltage rails. May be you can
>>> get rid of that unless there is a strong need.
>>>
>> All the resource handlers, clk, regulator, opp, DT node, are retrieved
>> from CPU0 device even for SMP.  I hope the naming of the driver could
>> tell:
>>
>> - The driver supports UP
>> - The driver supports SMP where CPUs shares clock/voltage rails by
>>   managing CPU0 (resource)
>> - The driver does not support SMP where individual CPU can scale
>>   clock/voltage independently.
>>
>
> How about cpufreq-single-thread.c and CONFIG_CPUFREQ_SINGLE_THREAD?
> That makes sense for both UP and SMP.
>
Indeed. This sounds more appropriate and also reflects what actually happens
with a UP or shared clock SMP case.

Regards
Santosh

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Sat, Jul 21, 2012 at 10:34:29AM +0530, Shilimkar, Santosh wrote:
> On Fri, Jul 20, 2012 at 9:20 PM, Turquette, Mike <mturquette@ti.com> wrote:
> > How about cpufreq-single-thread.c and CONFIG_CPUFREQ_SINGLE_THREAD?
> > That makes sense for both UP and SMP.
> >
> Indeed. This sounds more appropriate and also reflects what actually happens
> with a UP or shared clock SMP case.
> 
While I agree with this observation, the suggested naming does not
reflect the rationale of CPU0 though, which is really important for
driver to work, and is exactly the thing I like about *_CPU0 naming.

The driver needs the following stuff around CPU0 to be functional.

- Device tree node /cpus/cpu@0
- clk lookup with dev_id being "cpu0"
- regulator with id being "cpu0"

I think the *_CPU0 naming does a better job on emphasising those in the
first place.

-- 
Regards,
Shawn

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Sat, Jul 21, 2012 at 10:34:29AM +0530, Shilimkar, Santosh wrote:
> On Fri, Jul 20, 2012 at 9:20 PM, Turquette, Mike <mturquette@ti.com> wrote:
> > How about cpufreq-single-thread.c and CONFIG_CPUFREQ_SINGLE_THREAD?
> > That makes sense for both UP and SMP.
> >
> Indeed. This sounds more appropriate and also reflects what actually happens
> with a UP or shared clock SMP case.
> 
While I agree with this observation, the suggested naming does not
reflect the rationale of CPU0 though, which is really important for
driver to work, and is exactly the thing I like about *_CPU0 naming.

The driver needs the following stuff around CPU0 to be functional.

- Device tree node /cpus/cpu at 0
- clk lookup with dev_id being "cpu0"
- regulator with id being "cpu0"

I think the *_CPU0 naming does a better job on emphasising those in the
first place.

-- 
Regards,
Shawn

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Mark Brown]

On Thu, Jul 19, 2012 at 11:54:41PM +0800, Shawn Guo wrote:

> +Optional properties:
> +- transition-latency: Specify the possible maximum transition latency,
> +  in unit of nanoseconds.

This should make it clear that the transition latency being documented
here is just that for the core clock change itself, there may be other
sources of latency like the regulator ramp time or reprogramming PLLs.

> +	if (cpu_reg) {
> +		opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
> +		if (IS_ERR(opp)) {
> +			pr_err("failed to find OPP for %ld\n", freq_Hz);
> +			return PTR_ERR(opp);
> +		}
> +		volt = opp_get_voltage(opp);
> +		tol = volt * voltage_tolerance / 100;
> +		volt_old = regulator_get_voltage(cpu_reg);

regulator_get_voltage() might be a bit expensive, many regulators will
query the hardware every time.  Since it's just for diagnostic purposes
it'd be better to not bother and let people read the historical
information from the logs.

> +	/* scaling up?  scale voltage before frequency */
> +	if (cpu_reg && freqs.new > freqs.old) {
> +		if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {

This is a totally sane and sensible use case for specifying a voltage
range, we should move it into the regulator core for other users so you
can just specify the voltage and the tolerance directly.

It is a little sad here as it means that we loose the ability to do
frequency only scaling if there's no regulator which is nice.

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Mark Brown]

On Thu, Jul 19, 2012 at 11:54:41PM +0800, Shawn Guo wrote:

> +Optional properties:
> +- transition-latency: Specify the possible maximum transition latency,
> +  in unit of nanoseconds.

This should make it clear that the transition latency being documented
here is just that for the core clock change itself, there may be other
sources of latency like the regulator ramp time or reprogramming PLLs.

> +	if (cpu_reg) {
> +		opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
> +		if (IS_ERR(opp)) {
> +			pr_err("failed to find OPP for %ld\n", freq_Hz);
> +			return PTR_ERR(opp);
> +		}
> +		volt = opp_get_voltage(opp);
> +		tol = volt * voltage_tolerance / 100;
> +		volt_old = regulator_get_voltage(cpu_reg);

regulator_get_voltage() might be a bit expensive, many regulators will
query the hardware every time.  Since it's just for diagnostic purposes
it'd be better to not bother and let people read the historical
information from the logs.

> +	/* scaling up?  scale voltage before frequency */
> +	if (cpu_reg && freqs.new > freqs.old) {
> +		if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {

This is a totally sane and sensible use case for specifying a voltage
range, we should move it into the regulator core for other users so you
can just specify the voltage and the tolerance directly.

It is a little sad here as it means that we loose the ability to do
frequency only scaling if there's no regulator which is nice.

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Richard Zhao]

On Sat, Jul 21, 2012 at 02:38:13PM +0800, Shawn Guo wrote:
> On Sat, Jul 21, 2012 at 10:34:29AM +0530, Shilimkar, Santosh wrote:
> > On Fri, Jul 20, 2012 at 9:20 PM, Turquette, Mike <mturquette@ti.com> wrote:
> > > How about cpufreq-single-thread.c and CONFIG_CPUFREQ_SINGLE_THREAD?
> > > That makes sense for both UP and SMP.
> > >
> > Indeed. This sounds more appropriate and also reflects what actually happens
> > with a UP or shared clock SMP case.
> > 
> While I agree with this observation, the suggested naming does not
> reflect the rationale of CPU0 though, which is really important for
> driver to work, and is exactly the thing I like about *_CPU0 naming.
> 
> The driver needs the following stuff around CPU0 to be functional.
> 
> - Device tree node /cpus/cpu@0
> - clk lookup with dev_id being "cpu0"
> - regulator with id being "cpu0"
> 
> I think the *_CPU0 naming does a better job on emphasising those in the
> first place.
Are we going to support the case in the future that cores has
independent freq? If yes, maybe we should have a more common name.

Thanks
Richard
> 
> -- 
> Regards,
> Shawn

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Richard Zhao]

On Sat, Jul 21, 2012 at 02:38:13PM +0800, Shawn Guo wrote:
> On Sat, Jul 21, 2012 at 10:34:29AM +0530, Shilimkar, Santosh wrote:
> > On Fri, Jul 20, 2012 at 9:20 PM, Turquette, Mike <mturquette@ti.com> wrote:
> > > How about cpufreq-single-thread.c and CONFIG_CPUFREQ_SINGLE_THREAD?
> > > That makes sense for both UP and SMP.
> > >
> > Indeed. This sounds more appropriate and also reflects what actually happens
> > with a UP or shared clock SMP case.
> > 
> While I agree with this observation, the suggested naming does not
> reflect the rationale of CPU0 though, which is really important for
> driver to work, and is exactly the thing I like about *_CPU0 naming.
> 
> The driver needs the following stuff around CPU0 to be functional.
> 
> - Device tree node /cpus/cpu at 0
> - clk lookup with dev_id being "cpu0"
> - regulator with id being "cpu0"
> 
> I think the *_CPU0 naming does a better job on emphasising those in the
> first place.
Are we going to support the case in the future that cores has
independent freq? If yes, maybe we should have a more common name.

Thanks
Richard
> 
> -- 
> Regards,
> Shawn

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Richard Zhao]

On Thu, Jul 26, 2012 at 02:11:21PM +0100, Mark Brown wrote:
> On Thu, Jul 19, 2012 at 11:54:41PM +0800, Shawn Guo wrote:
> 
> > +Optional properties:
> > +- transition-latency: Specify the possible maximum transition latency,
> > +  in unit of nanoseconds.
> 
> This should make it clear that the transition latency being documented
> here is just that for the core clock change itself, there may be other
> sources of latency like the regulator ramp time or reprogramming PLLs.
I think it's the total time and board dts can over-write it if it
needs. Different transitions between different operating points may
differ, and regulator may be able to indicate the transition time but
clk don't have such api, and probably not worth to have.
 
Thanks
Richard

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Richard Zhao]

On Thu, Jul 26, 2012 at 02:11:21PM +0100, Mark Brown wrote:
> On Thu, Jul 19, 2012 at 11:54:41PM +0800, Shawn Guo wrote:
> 
> > +Optional properties:
> > +- transition-latency: Specify the possible maximum transition latency,
> > +  in unit of nanoseconds.
> 
> This should make it clear that the transition latency being documented
> here is just that for the core clock change itself, there may be other
> sources of latency like the regulator ramp time or reprogramming PLLs.
I think it's the total time and board dts can over-write it if it
needs. Different transitions between different operating points may
differ, and regulator may be able to indicate the transition time but
clk don't have such api, and probably not worth to have.
 
Thanks
Richard

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Richard Zhao]

On Thu, Jul 19, 2012 at 11:54:41PM +0800, Shawn Guo wrote:
> It adds a generic cpufreq driver for CPU0 frequency management based on
> clk, regulator, OPP and device tree support.  It can support both
> uniprocessor (UP) and those symmetric multiprocessor (SMP) systems which
> share clock and voltage across all CPUs.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  .../devicetree/bindings/cpufreq/cpufreq-cpu0.txt   |   58 +++++
>  drivers/cpufreq/Kconfig                            |   11 +
>  drivers/cpufreq/Makefile                           |    2 +
>  drivers/cpufreq/cpufreq-cpu0.c                     |  235 ++++++++++++++++++++
>  4 files changed, 306 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
>  create mode 100644 drivers/cpufreq/cpufreq-cpu0.c
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
> new file mode 100644
> index 0000000..94799bc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
> @@ -0,0 +1,58 @@
> +Generic cpufreq driver for CPU0
It's going to have generic name if it will become more generic.

> +
> +It is a generic cpufreq driver for CPU0 frequency management.  It
> +supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> +systems which share clock and voltage across all CPUs.
> +
> +Both required and optional properties listed below must be defined
> +under node /cpus/cpu@0.
> +
> +Required properties:
> +- operating-points: Refer to Documentation/devicetree/bindings/power/opp.txt
> +  for details
> +
> +Optional properties:
> +- transition-latency: Specify the possible maximum transition latency,
> +  in unit of nanoseconds.
> +- voltage-tolerance: Specify the CPU voltage tolerance in percentage.
Why do we have the same tolerance for all points? I think you can
either remove tolerance or add it to opps.
> +
> +Examples:
> +
> +cpus {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	cpu@0 {
> +		compatible = "arm,cortex-a9";
> +		reg = <0>;
> +		next-level-cache = <&L2>;
> +		operating-points = <
> +			/* kHz    uV    en */
> +			1200000 1275000 0
> +			996000  1225000 1
> +			792000  1100000 1
> +			672000  1100000 0
> +			396000  950000  1
> +			198000  850000  1
> +		>;
> +		transition-latency = <61036>; /* two CLK32 periods */
> +	};
> +
> +	cpu@1 {
> +		compatible = "arm,cortex-a9";
> +		reg = <1>;
> +		next-level-cache = <&L2>;
> +	};
> +
> +	cpu@2 {
> +		compatible = "arm,cortex-a9";
> +		reg = <2>;
> +		next-level-cache = <&L2>;
> +	};
> +
> +	cpu@3 {
> +		compatible = "arm,cortex-a9";
> +		reg = <3>;
> +		next-level-cache = <&L2>;
> +	};
> +};
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index e24a2a1..e77a1e0 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -179,6 +179,17 @@ config CPU_FREQ_GOV_CONSERVATIVE
>  
>  	  If in doubt, say N.
>  
> +config GENERIC_CPUFREQ_CPU0
> +	bool "Generic cpufreq driver for CPU0"
> +	depends on HAVE_CLK && REGULATOR && PM_OPP && OF
> +	select CPU_FREQ_TABLE
> +	help
> +	  This adds a generic cpufreq driver for CPU0 frequency management.
> +	  It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> +	  systems which share clock and voltage across all CPUs.
> +
> +	  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 9531fc2..a378ed2 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_CPU0)	+= cpufreq-cpu0.o
> +
>  ##################################################################################
>  # x86 drivers.
>  # Link order matters. K8 is preferred to ACPI because of firmware bugs in early
> diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
> new file mode 100644
> index 0000000..f1bb7f37
> --- /dev/null
> +++ b/drivers/cpufreq/cpufreq-cpu0.c
> @@ -0,0 +1,235 @@
> +/*
> + * Copyright (C) 2012 Freescale Semiconductor, Inc.
> + *
> + * Reuse some codes from drivers/cpufreq/omap-cpufreq.c
> + *
> + * 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
> + */
> +
> +#define pr_fmt(fmt)	KBUILD_MODNAME ": " fmt
> +
> +#include <linux/clk.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/opp.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +/* voltage tolerance in percentage */
> +static unsigned int voltage_tolerance;
> +
> +static struct device *cpu_dev;
> +static struct clk *cpu_clk;
> +static struct regulator *cpu_reg;
> +static struct cpufreq_frequency_table *freq_table;
> +
> +static int cpu0_verify_speed(struct cpufreq_policy *policy)
> +{
> +	return cpufreq_frequency_table_verify(policy, freq_table);
> +}
> +
> +static unsigned int cpu0_get_speed(unsigned int cpu)
> +{
> +	return clk_get_rate(cpu_clk) / 1000;
> +}
> +
> +static int cpu0_set_target(struct cpufreq_policy *policy,
> +			   unsigned int target_freq, unsigned int relation)
> +{
> +	struct cpufreq_freqs freqs;
> +	struct opp *opp;
> +	unsigned long freq_Hz, volt = 0, volt_old = 0, tol = 0;
> +	unsigned int index, cpu;
> +	int ret = 0;
> +
> +	ret = cpufreq_frequency_table_target(policy, freq_table, target_freq,
> +					     relation, &index);
> +	if (ret) {
> +		pr_err("failed to match target freqency %d: %d\n",
> +		       target_freq, ret);
> +		return ret;
> +	}
> +
> +	freq_Hz = clk_round_rate(cpu_clk, freq_table[index].frequency * 1000);
> +	if (freq_Hz < 0)
> +		freq_Hz = freq_table[index].frequency * 1000;
> +	freqs.new = freq_Hz / 1000;
> +	freqs.old = clk_get_rate(cpu_clk) / 1000;
> +
> +	if (freqs.old == freqs.new)
> +		return 0;
> +
> +	for_each_cpu(cpu, policy->cpus) {
> +		freqs.cpu = cpu;
> +		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> +	}
> +
> +	if (cpu_reg) {
> +		opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
> +		if (IS_ERR(opp)) {
> +			pr_err("failed to find OPP for %ld\n", freq_Hz);
> +			return PTR_ERR(opp);
> +		}
> +		volt = opp_get_voltage(opp);
> +		tol = volt * voltage_tolerance / 100;
> +		volt_old = regulator_get_voltage(cpu_reg);
> +	}
> +
> +	pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
> +		 freqs.old / 1000, volt_old ? volt_old / 1000 : -1,
> +		 freqs.new / 1000, volt ? volt / 1000 : -1);
> +
> +	/* scaling up?  scale voltage before frequency */
> +	if (cpu_reg && freqs.new > freqs.old) {
> +		if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> +			pr_warn("Unable to scale voltage up\n");
> +			freqs.new = freqs.old;
> +			goto done;
> +		}
> +	}
> +
> +	ret = clk_set_rate(cpu_clk, freqs.new * 1000);
Check return value and fall back to previous point if it needs?
> +
> +	/* scaling down?  scale voltage after frequency */
> +	if (cpu_reg && (freqs.new < freqs.old)) {
> +		if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> +			pr_warn("Unable to scale voltage down\n");
> +			ret = clk_set_rate(cpu_clk, freqs.old * 1000);
> +			freqs.new = freqs.old;
> +			goto done;
> +		}
> +	}
> +
> +done:
> +	for_each_cpu(cpu, policy->cpus) {
> +		freqs.cpu = cpu;
> +		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> +	}
> +
> +	return ret;
> +}
> +
> +static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
> +{
> +	struct device_node *np;
> +	int ret;
> +
> +	/* Initialize resources via CPU0 */
> +	if (policy->cpu != 0)
> +		return -EINVAL;
> +
> +	np = of_find_node_by_path("/cpus/cpu@0");
> +	if (!np) {
> +		pr_err("failed to find cpu0 node\n");
> +		return -ENOENT;
> +	}
> +
> +	cpu_dev = get_cpu_device(0);
> +	if (!cpu_dev) {
> +		pr_err("failed to get cpu0 device\n");
> +		ret = -ENODEV;
> +		goto out_put_node;
> +	}
> +
> +	cpu_dev->of_node = np;
hmm.. sys dev can not set of_node when populate it?
And why not do it in module init? 
> +	ret = of_init_opp_table(cpu_dev);
> +	if (ret) {
> +		pr_err("failed to init OPP table\n");
> +		goto out_put_node;
> +	}
> +
> +	ret = opp_init_cpufreq_table(cpu_dev, &freq_table);
> +	if (ret) {
> +		pr_err("failed to init cpufreq table\n");
> +		goto out_put_node;
> +	}
> +
> +	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> +	if (ret) {
> +		pr_err("invalid frequency table for cpu0\n");
> +		goto out_free_table;
> +	}
> +
> +	if (of_property_read_u32(np, "transition-latency",
> +				 &policy->cpuinfo.transition_latency))
> +		policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
> +
> +	of_property_read_u32(np, "voltage-tolerance", &voltage_tolerance);
> +
> +	cpu_clk = clk_get(cpu_dev, NULL);
> +	if (IS_ERR(cpu_clk)) {
> +		pr_err("failed to get cpu0 clock\n");
> +		ret = PTR_ERR(cpu_clk);
> +		goto out_free_table;
> +	}
> +	policy->cur = clk_get_rate(cpu_clk) / 1000;
> +
> +	cpu_reg = regulator_get(cpu_dev, "cpu0");
> +	if (IS_ERR(cpu_reg)) {
> +		pr_warn("failed to get cpu0 regulator\n");
> +		cpu_reg = NULL;
> +	}
> +
> +	/*
> +	 * The driver only supports the SMP configuartion where all processors
> +	 * share the clock and voltage and clock.  Use cpufreq affected_cpus
> +	 * interface to have all CPUs scaled together.
> +	 */
> +	policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> +	cpumask_setall(policy->cpus);
> +
> +	cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
> +
> +	of_node_put(np);
> +	return 0;
> +
> +out_free_table:
> +	opp_free_cpufreq_table(cpu_dev, &freq_table);
> +out_put_node:
> +	of_node_put(np);
> +	return ret;
> +}
> +
> +static int cpu0_cpufreq_exit(struct cpufreq_policy *policy)
> +{
> +	cpufreq_frequency_table_put_attr(policy->cpu);
> +	regulator_put(cpu_reg);
> +	clk_put(cpu_clk);
> +	opp_free_cpufreq_table(cpu_dev, &freq_table);
> +
> +	return 0;
> +}
> +
> +static struct freq_attr *cpu0_cpufreq_attr[] = {
> +	&cpufreq_freq_attr_scaling_available_freqs,
> +	NULL,
> +};
> +
> +static struct cpufreq_driver cpu0_cpufreq_driver = {
> +	.flags = CPUFREQ_STICKY,
> +	.verify = cpu0_verify_speed,
> +	.target = cpu0_set_target,
> +	.get = cpu0_get_speed,
> +	.init = cpu0_cpufreq_init,
> +	.exit = cpu0_cpufreq_exit,
> +	.name = "generic_cpu0",
> +	.attr = cpu0_cpufreq_attr,
> +};
static u32 max_freq = UINT_MAX / 1000; /* kHz */
module_param(max_freq, uint, 0);
MODULE_PARM_DESC(max_freq, "max cpu frequency in unit of kHz");

It's for debug. Make sense?

Thanks
Richard
> +
> +static int __devinit cpu0_cpufreq_driver_init(void)
> +{
> +	return cpufreq_register_driver(&cpu0_cpufreq_driver);
> +}
> +late_initcall(cpu0_cpufreq_driver_init);
> +
> +MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>");
> +MODULE_DESCRIPTION("Generic cpufreq driver for CPU0");
> +MODULE_LICENSE("GPL");
> -- 
> 1.7.5.4
> 

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Richard Zhao]

On Thu, Jul 19, 2012 at 11:54:41PM +0800, Shawn Guo wrote:
> It adds a generic cpufreq driver for CPU0 frequency management based on
> clk, regulator, OPP and device tree support.  It can support both
> uniprocessor (UP) and those symmetric multiprocessor (SMP) systems which
> share clock and voltage across all CPUs.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  .../devicetree/bindings/cpufreq/cpufreq-cpu0.txt   |   58 +++++
>  drivers/cpufreq/Kconfig                            |   11 +
>  drivers/cpufreq/Makefile                           |    2 +
>  drivers/cpufreq/cpufreq-cpu0.c                     |  235 ++++++++++++++++++++
>  4 files changed, 306 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
>  create mode 100644 drivers/cpufreq/cpufreq-cpu0.c
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
> new file mode 100644
> index 0000000..94799bc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
> @@ -0,0 +1,58 @@
> +Generic cpufreq driver for CPU0
It's going to have generic name if it will become more generic.

> +
> +It is a generic cpufreq driver for CPU0 frequency management.  It
> +supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> +systems which share clock and voltage across all CPUs.
> +
> +Both required and optional properties listed below must be defined
> +under node /cpus/cpu at 0.
> +
> +Required properties:
> +- operating-points: Refer to Documentation/devicetree/bindings/power/opp.txt
> +  for details
> +
> +Optional properties:
> +- transition-latency: Specify the possible maximum transition latency,
> +  in unit of nanoseconds.
> +- voltage-tolerance: Specify the CPU voltage tolerance in percentage.
Why do we have the same tolerance for all points? I think you can
either remove tolerance or add it to opps.
> +
> +Examples:
> +
> +cpus {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	cpu at 0 {
> +		compatible = "arm,cortex-a9";
> +		reg = <0>;
> +		next-level-cache = <&L2>;
> +		operating-points = <
> +			/* kHz    uV    en */
> +			1200000 1275000 0
> +			996000  1225000 1
> +			792000  1100000 1
> +			672000  1100000 0
> +			396000  950000  1
> +			198000  850000  1
> +		>;
> +		transition-latency = <61036>; /* two CLK32 periods */
> +	};
> +
> +	cpu at 1 {
> +		compatible = "arm,cortex-a9";
> +		reg = <1>;
> +		next-level-cache = <&L2>;
> +	};
> +
> +	cpu at 2 {
> +		compatible = "arm,cortex-a9";
> +		reg = <2>;
> +		next-level-cache = <&L2>;
> +	};
> +
> +	cpu at 3 {
> +		compatible = "arm,cortex-a9";
> +		reg = <3>;
> +		next-level-cache = <&L2>;
> +	};
> +};
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index e24a2a1..e77a1e0 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -179,6 +179,17 @@ config CPU_FREQ_GOV_CONSERVATIVE
>  
>  	  If in doubt, say N.
>  
> +config GENERIC_CPUFREQ_CPU0
> +	bool "Generic cpufreq driver for CPU0"
> +	depends on HAVE_CLK && REGULATOR && PM_OPP && OF
> +	select CPU_FREQ_TABLE
> +	help
> +	  This adds a generic cpufreq driver for CPU0 frequency management.
> +	  It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> +	  systems which share clock and voltage across all CPUs.
> +
> +	  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 9531fc2..a378ed2 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_CPU0)	+= cpufreq-cpu0.o
> +
>  ##################################################################################
>  # x86 drivers.
>  # Link order matters. K8 is preferred to ACPI because of firmware bugs in early
> diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
> new file mode 100644
> index 0000000..f1bb7f37
> --- /dev/null
> +++ b/drivers/cpufreq/cpufreq-cpu0.c
> @@ -0,0 +1,235 @@
> +/*
> + * Copyright (C) 2012 Freescale Semiconductor, Inc.
> + *
> + * Reuse some codes from drivers/cpufreq/omap-cpufreq.c
> + *
> + * 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
> + */
> +
> +#define pr_fmt(fmt)	KBUILD_MODNAME ": " fmt
> +
> +#include <linux/clk.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/opp.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +/* voltage tolerance in percentage */
> +static unsigned int voltage_tolerance;
> +
> +static struct device *cpu_dev;
> +static struct clk *cpu_clk;
> +static struct regulator *cpu_reg;
> +static struct cpufreq_frequency_table *freq_table;
> +
> +static int cpu0_verify_speed(struct cpufreq_policy *policy)
> +{
> +	return cpufreq_frequency_table_verify(policy, freq_table);
> +}
> +
> +static unsigned int cpu0_get_speed(unsigned int cpu)
> +{
> +	return clk_get_rate(cpu_clk) / 1000;
> +}
> +
> +static int cpu0_set_target(struct cpufreq_policy *policy,
> +			   unsigned int target_freq, unsigned int relation)
> +{
> +	struct cpufreq_freqs freqs;
> +	struct opp *opp;
> +	unsigned long freq_Hz, volt = 0, volt_old = 0, tol = 0;
> +	unsigned int index, cpu;
> +	int ret = 0;
> +
> +	ret = cpufreq_frequency_table_target(policy, freq_table, target_freq,
> +					     relation, &index);
> +	if (ret) {
> +		pr_err("failed to match target freqency %d: %d\n",
> +		       target_freq, ret);
> +		return ret;
> +	}
> +
> +	freq_Hz = clk_round_rate(cpu_clk, freq_table[index].frequency * 1000);
> +	if (freq_Hz < 0)
> +		freq_Hz = freq_table[index].frequency * 1000;
> +	freqs.new = freq_Hz / 1000;
> +	freqs.old = clk_get_rate(cpu_clk) / 1000;
> +
> +	if (freqs.old == freqs.new)
> +		return 0;
> +
> +	for_each_cpu(cpu, policy->cpus) {
> +		freqs.cpu = cpu;
> +		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> +	}
> +
> +	if (cpu_reg) {
> +		opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
> +		if (IS_ERR(opp)) {
> +			pr_err("failed to find OPP for %ld\n", freq_Hz);
> +			return PTR_ERR(opp);
> +		}
> +		volt = opp_get_voltage(opp);
> +		tol = volt * voltage_tolerance / 100;
> +		volt_old = regulator_get_voltage(cpu_reg);
> +	}
> +
> +	pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
> +		 freqs.old / 1000, volt_old ? volt_old / 1000 : -1,
> +		 freqs.new / 1000, volt ? volt / 1000 : -1);
> +
> +	/* scaling up?  scale voltage before frequency */
> +	if (cpu_reg && freqs.new > freqs.old) {
> +		if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> +			pr_warn("Unable to scale voltage up\n");
> +			freqs.new = freqs.old;
> +			goto done;
> +		}
> +	}
> +
> +	ret = clk_set_rate(cpu_clk, freqs.new * 1000);
Check return value and fall back to previous point if it needs?
> +
> +	/* scaling down?  scale voltage after frequency */
> +	if (cpu_reg && (freqs.new < freqs.old)) {
> +		if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> +			pr_warn("Unable to scale voltage down\n");
> +			ret = clk_set_rate(cpu_clk, freqs.old * 1000);
> +			freqs.new = freqs.old;
> +			goto done;
> +		}
> +	}
> +
> +done:
> +	for_each_cpu(cpu, policy->cpus) {
> +		freqs.cpu = cpu;
> +		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> +	}
> +
> +	return ret;
> +}
> +
> +static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
> +{
> +	struct device_node *np;
> +	int ret;
> +
> +	/* Initialize resources via CPU0 */
> +	if (policy->cpu != 0)
> +		return -EINVAL;
> +
> +	np = of_find_node_by_path("/cpus/cpu at 0");
> +	if (!np) {
> +		pr_err("failed to find cpu0 node\n");
> +		return -ENOENT;
> +	}
> +
> +	cpu_dev = get_cpu_device(0);
> +	if (!cpu_dev) {
> +		pr_err("failed to get cpu0 device\n");
> +		ret = -ENODEV;
> +		goto out_put_node;
> +	}
> +
> +	cpu_dev->of_node = np;
hmm.. sys dev can not set of_node when populate it?
And why not do it in module init? 
> +	ret = of_init_opp_table(cpu_dev);
> +	if (ret) {
> +		pr_err("failed to init OPP table\n");
> +		goto out_put_node;
> +	}
> +
> +	ret = opp_init_cpufreq_table(cpu_dev, &freq_table);
> +	if (ret) {
> +		pr_err("failed to init cpufreq table\n");
> +		goto out_put_node;
> +	}
> +
> +	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> +	if (ret) {
> +		pr_err("invalid frequency table for cpu0\n");
> +		goto out_free_table;
> +	}
> +
> +	if (of_property_read_u32(np, "transition-latency",
> +				 &policy->cpuinfo.transition_latency))
> +		policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
> +
> +	of_property_read_u32(np, "voltage-tolerance", &voltage_tolerance);
> +
> +	cpu_clk = clk_get(cpu_dev, NULL);
> +	if (IS_ERR(cpu_clk)) {
> +		pr_err("failed to get cpu0 clock\n");
> +		ret = PTR_ERR(cpu_clk);
> +		goto out_free_table;
> +	}
> +	policy->cur = clk_get_rate(cpu_clk) / 1000;
> +
> +	cpu_reg = regulator_get(cpu_dev, "cpu0");
> +	if (IS_ERR(cpu_reg)) {
> +		pr_warn("failed to get cpu0 regulator\n");
> +		cpu_reg = NULL;
> +	}
> +
> +	/*
> +	 * The driver only supports the SMP configuartion where all processors
> +	 * share the clock and voltage and clock.  Use cpufreq affected_cpus
> +	 * interface to have all CPUs scaled together.
> +	 */
> +	policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> +	cpumask_setall(policy->cpus);
> +
> +	cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
> +
> +	of_node_put(np);
> +	return 0;
> +
> +out_free_table:
> +	opp_free_cpufreq_table(cpu_dev, &freq_table);
> +out_put_node:
> +	of_node_put(np);
> +	return ret;
> +}
> +
> +static int cpu0_cpufreq_exit(struct cpufreq_policy *policy)
> +{
> +	cpufreq_frequency_table_put_attr(policy->cpu);
> +	regulator_put(cpu_reg);
> +	clk_put(cpu_clk);
> +	opp_free_cpufreq_table(cpu_dev, &freq_table);
> +
> +	return 0;
> +}
> +
> +static struct freq_attr *cpu0_cpufreq_attr[] = {
> +	&cpufreq_freq_attr_scaling_available_freqs,
> +	NULL,
> +};
> +
> +static struct cpufreq_driver cpu0_cpufreq_driver = {
> +	.flags = CPUFREQ_STICKY,
> +	.verify = cpu0_verify_speed,
> +	.target = cpu0_set_target,
> +	.get = cpu0_get_speed,
> +	.init = cpu0_cpufreq_init,
> +	.exit = cpu0_cpufreq_exit,
> +	.name = "generic_cpu0",
> +	.attr = cpu0_cpufreq_attr,
> +};
static u32 max_freq = UINT_MAX / 1000; /* kHz */
module_param(max_freq, uint, 0);
MODULE_PARM_DESC(max_freq, "max cpu frequency in unit of kHz");

It's for debug. Make sense?

Thanks
Richard
> +
> +static int __devinit cpu0_cpufreq_driver_init(void)
> +{
> +	return cpufreq_register_driver(&cpu0_cpufreq_driver);
> +}
> +late_initcall(cpu0_cpufreq_driver_init);
> +
> +MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>");
> +MODULE_DESCRIPTION("Generic cpufreq driver for CPU0");
> +MODULE_LICENSE("GPL");
> -- 
> 1.7.5.4
> 

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1466 bytes --]

On Fri, Jul 27, 2012 at 10:13:04AM +0800, Richard Zhao wrote:
> On Thu, Jul 26, 2012 at 02:11:21PM +0100, Mark Brown wrote:
> > On Thu, Jul 19, 2012 at 11:54:41PM +0800, Shawn Guo wrote:

> > > +Optional properties:
> > > +- transition-latency: Specify the possible maximum transition latency,
> > > +  in unit of nanoseconds.

> > This should make it clear that the transition latency being documented
> > here is just that for the core clock change itself, there may be other
> > sources of latency like the regulator ramp time or reprogramming PLLs.

> I think it's the total time and board dts can over-write it if it
> needs. Different transitions between different operating points may
> differ, and regulator may be able to indicate the transition time but
> clk don't have such api, and probably not worth to have.

That's going to be awfully manual if every board has to tweak values
(though obviously the main effect is just going to be bad decisions
rather than breakage).  I've seen several systems where the clock could
provide useful timing input here - the usual pattern is that you've got
a PLL which you can use as well as some dividers.  Transitions that only
need a divider update are extremely quick but transitions that change
the PLL setup can take much longer.  It seems better to just allow the
board maintainer to plug everything together rather than having to work
out their specific latencies to squeeze the performance out of the
system.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Mark Brown]

On Fri, Jul 27, 2012 at 10:13:04AM +0800, Richard Zhao wrote:
> On Thu, Jul 26, 2012 at 02:11:21PM +0100, Mark Brown wrote:
> > On Thu, Jul 19, 2012 at 11:54:41PM +0800, Shawn Guo wrote:

> > > +Optional properties:
> > > +- transition-latency: Specify the possible maximum transition latency,
> > > +  in unit of nanoseconds.

> > This should make it clear that the transition latency being documented
> > here is just that for the core clock change itself, there may be other
> > sources of latency like the regulator ramp time or reprogramming PLLs.

> I think it's the total time and board dts can over-write it if it
> needs. Different transitions between different operating points may
> differ, and regulator may be able to indicate the transition time but
> clk don't have such api, and probably not worth to have.

That's going to be awfully manual if every board has to tweak values
(though obviously the main effect is just going to be bad decisions
rather than breakage).  I've seen several systems where the clock could
provide useful timing input here - the usual pattern is that you've got
a PLL which you can use as well as some dividers.  Transitions that only
need a divider update are extremely quick but transitions that change
the PLL setup can take much longer.  It seems better to just allow the
board maintainer to plug everything together rather than having to work
out their specific latencies to squeeze the performance out of the
system.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120727/673349c5/attachment.sig>

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Fri, Jul 27, 2012 at 10:04:34AM +0800, Richard Zhao wrote:
> Are we going to support the case in the future that cores has
> independent freq? If yes, maybe we should have a more common name.
> 
I do not think we should support that with this driver.  That's one of
the reasons I named the driver this way.

-- 
Regards,
Shawn

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Fri, Jul 27, 2012 at 10:04:34AM +0800, Richard Zhao wrote:
> Are we going to support the case in the future that cores has
> independent freq? If yes, maybe we should have a more common name.
> 
I do not think we should support that with this driver.  That's one of
the reasons I named the driver this way.

-- 
Regards,
Shawn

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Thu, Jul 26, 2012 at 02:11:21PM +0100, Mark Brown wrote:
> On Thu, Jul 19, 2012 at 11:54:41PM +0800, Shawn Guo wrote:
> 
> > +Optional properties:
> > +- transition-latency: Specify the possible maximum transition latency,
> > +  in unit of nanoseconds.
> 
> This should make it clear that the transition latency being documented
> here is just that for the core clock change itself, there may be other
> sources of latency like the regulator ramp time or reprogramming PLLs.
> 
I intended to make it be the total latency from whatever sources that
should be counted on particular system.  Rather than adding complexity
for the driver to figure out these latencies from every single source,
I would expect that people know the possible maximum latency in total
for their systems and specify it in device tree.

> > +	if (cpu_reg) {
> > +		opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
> > +		if (IS_ERR(opp)) {
> > +			pr_err("failed to find OPP for %ld\n", freq_Hz);
> > +			return PTR_ERR(opp);
> > +		}
> > +		volt = opp_get_voltage(opp);
> > +		tol = volt * voltage_tolerance / 100;
> > +		volt_old = regulator_get_voltage(cpu_reg);
> 
> regulator_get_voltage() might be a bit expensive, many regulators will
> query the hardware every time.  Since it's just for diagnostic purposes
> it'd be better to not bother and let people read the historical
> information from the logs.
> 
Ok, makes sense to me.

> > +	/* scaling up?  scale voltage before frequency */
> > +	if (cpu_reg && freqs.new > freqs.old) {
> > +		if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> 
> This is a totally sane and sensible use case for specifying a voltage
> range, we should move it into the regulator core for other users so you
> can just specify the voltage and the tolerance directly.
> 
Are you asking for a change on regulator_set_voltage API?  While I agree
with your comment, it's not a thing we need to necessarily do in this
series.  The change on the API requires a touch on all the existing
users.  I do not think it's nice to carry such a patch in this series.

> It is a little sad here as it means that we loose the ability to do
> frequency only scaling if there's no regulator which is nice.

I do not understand it.  The regulator is optional for the driver, and
we can still scale frequency even if there is no regulator.

-- 
Regards,
Shawn

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Thu, Jul 26, 2012 at 02:11:21PM +0100, Mark Brown wrote:
> On Thu, Jul 19, 2012 at 11:54:41PM +0800, Shawn Guo wrote:
> 
> > +Optional properties:
> > +- transition-latency: Specify the possible maximum transition latency,
> > +  in unit of nanoseconds.
> 
> This should make it clear that the transition latency being documented
> here is just that for the core clock change itself, there may be other
> sources of latency like the regulator ramp time or reprogramming PLLs.
> 
I intended to make it be the total latency from whatever sources that
should be counted on particular system.  Rather than adding complexity
for the driver to figure out these latencies from every single source,
I would expect that people know the possible maximum latency in total
for their systems and specify it in device tree.

> > +	if (cpu_reg) {
> > +		opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
> > +		if (IS_ERR(opp)) {
> > +			pr_err("failed to find OPP for %ld\n", freq_Hz);
> > +			return PTR_ERR(opp);
> > +		}
> > +		volt = opp_get_voltage(opp);
> > +		tol = volt * voltage_tolerance / 100;
> > +		volt_old = regulator_get_voltage(cpu_reg);
> 
> regulator_get_voltage() might be a bit expensive, many regulators will
> query the hardware every time.  Since it's just for diagnostic purposes
> it'd be better to not bother and let people read the historical
> information from the logs.
> 
Ok, makes sense to me.

> > +	/* scaling up?  scale voltage before frequency */
> > +	if (cpu_reg && freqs.new > freqs.old) {
> > +		if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> 
> This is a totally sane and sensible use case for specifying a voltage
> range, we should move it into the regulator core for other users so you
> can just specify the voltage and the tolerance directly.
> 
Are you asking for a change on regulator_set_voltage API?  While I agree
with your comment, it's not a thing we need to necessarily do in this
series.  The change on the API requires a touch on all the existing
users.  I do not think it's nice to carry such a patch in this series.

> It is a little sad here as it means that we loose the ability to do
> frequency only scaling if there's no regulator which is nice.

I do not understand it.  The regulator is optional for the driver, and
we can still scale frequency even if there is no regulator.

-- 
Regards,
Shawn

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Fri, Jul 27, 2012 at 02:33:35PM +0800, Richard Zhao wrote:
> > +Generic cpufreq driver for CPU0
> It's going to have generic name if it will become more generic.
> 
I'm not in the position to say that it will become even more generic.

> > +- voltage-tolerance: Specify the CPU voltage tolerance in percentage.
> Why do we have the same tolerance for all points?

Because I haven't seen any case that needs different tolerance for
different operating points.

> I think you can
> either remove tolerance or add it to opps.

I'm not going to remove it, because I have seen potential cpufreq-cpu0
candidates, e.g. omap-cpufreq, need it.  It's also improper to encode
it in operating-points, since OPP library does not have it.

> > +	ret = clk_set_rate(cpu_clk, freqs.new * 1000);
> Check return value and fall back to previous point if it needs?

Right, the voltage should be reverted back if clk_set_rate fails.

> > +	cpu_dev->of_node = np;
> hmm.. sys dev can not set of_node when populate it?

Since the sys dev is not populated from device tree, the of_node is
not set, and we have to do it here on our own.

> And why not do it in module init? 

What's the advantage of doing it in module init over here?

> static u32 max_freq = UINT_MAX / 1000; /* kHz */
> module_param(max_freq, uint, 0);
> MODULE_PARM_DESC(max_freq, "max cpu frequency in unit of kHz");
> 
> It's for debug. Make sense?
> 
It does not look so useful to me, as it never came to me when I was
debugging the driver.

-- 
Regards,
Shawn

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Fri, Jul 27, 2012 at 02:33:35PM +0800, Richard Zhao wrote:
> > +Generic cpufreq driver for CPU0
> It's going to have generic name if it will become more generic.
> 
I'm not in the position to say that it will become even more generic.

> > +- voltage-tolerance: Specify the CPU voltage tolerance in percentage.
> Why do we have the same tolerance for all points?

Because I haven't seen any case that needs different tolerance for
different operating points.

> I think you can
> either remove tolerance or add it to opps.

I'm not going to remove it, because I have seen potential cpufreq-cpu0
candidates, e.g. omap-cpufreq, need it.  It's also improper to encode
it in operating-points, since OPP library does not have it.

> > +	ret = clk_set_rate(cpu_clk, freqs.new * 1000);
> Check return value and fall back to previous point if it needs?

Right, the voltage should be reverted back if clk_set_rate fails.

> > +	cpu_dev->of_node = np;
> hmm.. sys dev can not set of_node when populate it?

Since the sys dev is not populated from device tree, the of_node is
not set, and we have to do it here on our own.

> And why not do it in module init? 

What's the advantage of doing it in module init over here?

> static u32 max_freq = UINT_MAX / 1000; /* kHz */
> module_param(max_freq, uint, 0);
> MODULE_PARM_DESC(max_freq, "max cpu frequency in unit of kHz");
> 
> It's for debug. Make sense?
> 
It does not look so useful to me, as it never came to me when I was
debugging the driver.

-- 
Regards,
Shawn

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Mon, Jul 30, 2012 at 02:52:20PM +0800, Shawn Guo wrote:
> On Thu, Jul 26, 2012 at 02:11:21PM +0100, Mark Brown wrote:
> > regulator_get_voltage() might be a bit expensive, many regulators will
> > query the hardware every time.  Since it's just for diagnostic purposes
> > it'd be better to not bother and let people read the historical
> > information from the logs.
> > 
> Ok, makes sense to me.
> 
In the reply to one of Richard's comment, the volt_old will be used
not only for diagnostic purpose but also for reverting voltage back
when clk_set_rate fails.

-- 
Regards,
Shawn

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Mon, Jul 30, 2012 at 02:52:20PM +0800, Shawn Guo wrote:
> On Thu, Jul 26, 2012 at 02:11:21PM +0100, Mark Brown wrote:
> > regulator_get_voltage() might be a bit expensive, many regulators will
> > query the hardware every time.  Since it's just for diagnostic purposes
> > it'd be better to not bother and let people read the historical
> > information from the logs.
> > 
> Ok, makes sense to me.
> 
In the reply to one of Richard's comment, the volt_old will be used
not only for diagnostic purpose but also for reverting voltage back
when clk_set_rate fails.

-- 
Regards,
Shawn

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Richard Zhao]

On Mon, Jul 30, 2012 at 04:17:53PM +0800, Shawn Guo wrote:
> On Fri, Jul 27, 2012 at 02:33:35PM +0800, Richard Zhao wrote:
> > > +Generic cpufreq driver for CPU0
> > It's going to have generic name if it will become more generic.
> > 
> I'm not in the position to say that it will become even more generic.
> 
> > > +- voltage-tolerance: Specify the CPU voltage tolerance in percentage.
> > Why do we have the same tolerance for all points?
> 
> Because I haven't seen any case that needs different tolerance for
> different operating points.
> 
> > I think you can
> > either remove tolerance or add it to opps.
> 
> I'm not going to remove it, because I have seen potential cpufreq-cpu0
> candidates, e.g. omap-cpufreq, need it.  It's also improper to encode
> it in operating-points, since OPP library does not have it.
I think the real problem here is OPP only provide exact voltage but
regulator api needs a range.

> 
> > > +	ret = clk_set_rate(cpu_clk, freqs.new * 1000);
> > Check return value and fall back to previous point if it needs?
> 
> Right, the voltage should be reverted back if clk_set_rate fails.
> 
> > > +	cpu_dev->of_node = np;
> > hmm.. sys dev can not set of_node when populate it?
> 
> Since the sys dev is not populated from device tree, the of_node is
> not set, and we have to do it here on our own.
It sounds strange. But I think it's ok.
> 
> > And why not do it in module init? 
> 
> What's the advantage of doing it in module init over here?
IIRC, cpufreq driver init is called every time a new cpu get online.
So things that only needs to be done once can be put in module init.
> 
> > static u32 max_freq = UINT_MAX / 1000; /* kHz */
> > module_param(max_freq, uint, 0);
> > MODULE_PARM_DESC(max_freq, "max cpu frequency in unit of kHz");
> > 
> > It's for debug. Make sense?
> > 
> It does not look so useful to me, as it never came to me when I was
> debugging the driver.
Alright.

Thanks
Richard
> 
> -- 
> Regards,
> Shawn

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Richard Zhao]

On Mon, Jul 30, 2012 at 04:17:53PM +0800, Shawn Guo wrote:
> On Fri, Jul 27, 2012 at 02:33:35PM +0800, Richard Zhao wrote:
> > > +Generic cpufreq driver for CPU0
> > It's going to have generic name if it will become more generic.
> > 
> I'm not in the position to say that it will become even more generic.
> 
> > > +- voltage-tolerance: Specify the CPU voltage tolerance in percentage.
> > Why do we have the same tolerance for all points?
> 
> Because I haven't seen any case that needs different tolerance for
> different operating points.
> 
> > I think you can
> > either remove tolerance or add it to opps.
> 
> I'm not going to remove it, because I have seen potential cpufreq-cpu0
> candidates, e.g. omap-cpufreq, need it.  It's also improper to encode
> it in operating-points, since OPP library does not have it.
I think the real problem here is OPP only provide exact voltage but
regulator api needs a range.

> 
> > > +	ret = clk_set_rate(cpu_clk, freqs.new * 1000);
> > Check return value and fall back to previous point if it needs?
> 
> Right, the voltage should be reverted back if clk_set_rate fails.
> 
> > > +	cpu_dev->of_node = np;
> > hmm.. sys dev can not set of_node when populate it?
> 
> Since the sys dev is not populated from device tree, the of_node is
> not set, and we have to do it here on our own.
It sounds strange. But I think it's ok.
> 
> > And why not do it in module init? 
> 
> What's the advantage of doing it in module init over here?
IIRC, cpufreq driver init is called every time a new cpu get online.
So things that only needs to be done once can be put in module init.
> 
> > static u32 max_freq = UINT_MAX / 1000; /* kHz */
> > module_param(max_freq, uint, 0);
> > MODULE_PARM_DESC(max_freq, "max cpu frequency in unit of kHz");
> > 
> > It's for debug. Make sense?
> > 
> It does not look so useful to me, as it never came to me when I was
> debugging the driver.
Alright.

Thanks
Richard
> 
> -- 
> Regards,
> Shawn

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Mon, Jul 30, 2012 at 04:50:41PM +0800, Richard Zhao wrote:
> I think the real problem here is OPP only provide exact voltage but
> regulator api needs a range.
> 
Maybe OPP will get improved on that when the real use cases occur.

> IIRC, cpufreq driver init is called every time a new cpu get online.
> So things that only needs to be done once can be put in module init.
> 
Ah, right.  Something should be moved to module init then.

-- 
Regards,
Shawn

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Mon, Jul 30, 2012 at 04:50:41PM +0800, Richard Zhao wrote:
> I think the real problem here is OPP only provide exact voltage but
> regulator api needs a range.
> 
Maybe OPP will get improved on that when the real use cases occur.

> IIRC, cpufreq driver init is called every time a new cpu get online.
> So things that only needs to be done once can be put in module init.
> 
Ah, right.  Something should be moved to module init then.

-- 
Regards,
Shawn

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Mark Brown]

On Mon, Jul 30, 2012 at 02:52:20PM +0800, Shawn Guo wrote:
> On Thu, Jul 26, 2012 at 02:11:21PM +0100, Mark Brown wrote:

> > This should make it clear that the transition latency being documented
> > here is just that for the core clock change itself, there may be other
> > sources of latency like the regulator ramp time or reprogramming PLLs.

> I intended to make it be the total latency from whatever sources that
> should be counted on particular system.  Rather than adding complexity
> for the driver to figure out these latencies from every single source,
> I would expect that people know the possible maximum latency in total
> for their systems and specify it in device tree.

Quite honestly this seems totally unrealistic for the majority of users,
especially given the very poor documentation for this stuff which SoC
vendors typically provide.  It's a reasonable amount of work to go back
and figure this stuff out (especially given that it should be varying
depending on the transition in question), and it's going to give us a
bunch of magic numbers in people's bindings.

> > > +	if (cpu_reg && freqs.new > freqs.old) {
> > > +		if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {

> > This is a totally sane and sensible use case for specifying a voltage
> > range, we should move it into the regulator core for other users so you
> > can just specify the voltage and the tolerance directly.

> Are you asking for a change on regulator_set_voltage API?  While I agree
> with your comment, it's not a thing we need to necessarily do in this
> series.  The change on the API requires a touch on all the existing
> users.  I do not think it's nice to carry such a patch in this series.

No, add a new API.

> > It is a little sad here as it means that we loose the ability to do
> > frequency only scaling if there's no regulator which is nice.

> I do not understand it.  The regulator is optional for the driver, and
> we can still scale frequency even if there is no regulator.

As soon as a regulator is available your code will insist that we're
able to set the voltage specified to set the frequency, and since it's
using tolerances not ranges to whatever the chip maximum is that'll mean
it'll stop doing frequency scaling at all.

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Mark Brown]

On Mon, Jul 30, 2012 at 02:52:20PM +0800, Shawn Guo wrote:
> On Thu, Jul 26, 2012 at 02:11:21PM +0100, Mark Brown wrote:

> > This should make it clear that the transition latency being documented
> > here is just that for the core clock change itself, there may be other
> > sources of latency like the regulator ramp time or reprogramming PLLs.

> I intended to make it be the total latency from whatever sources that
> should be counted on particular system.  Rather than adding complexity
> for the driver to figure out these latencies from every single source,
> I would expect that people know the possible maximum latency in total
> for their systems and specify it in device tree.

Quite honestly this seems totally unrealistic for the majority of users,
especially given the very poor documentation for this stuff which SoC
vendors typically provide.  It's a reasonable amount of work to go back
and figure this stuff out (especially given that it should be varying
depending on the transition in question), and it's going to give us a
bunch of magic numbers in people's bindings.

> > > +	if (cpu_reg && freqs.new > freqs.old) {
> > > +		if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {

> > This is a totally sane and sensible use case for specifying a voltage
> > range, we should move it into the regulator core for other users so you
> > can just specify the voltage and the tolerance directly.

> Are you asking for a change on regulator_set_voltage API?  While I agree
> with your comment, it's not a thing we need to necessarily do in this
> series.  The change on the API requires a touch on all the existing
> users.  I do not think it's nice to carry such a patch in this series.

No, add a new API.

> > It is a little sad here as it means that we loose the ability to do
> > frequency only scaling if there's no regulator which is nice.

> I do not understand it.  The regulator is optional for the driver, and
> we can still scale frequency even if there is no regulator.

As soon as a regulator is available your code will insist that we're
able to set the voltage specified to set the frequency, and since it's
using tolerances not ranges to whatever the chip maximum is that'll mean
it'll stop doing frequency scaling at all.

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Mark Brown]

On Mon, Jul 30, 2012 at 04:20:54PM +0800, Shawn Guo wrote:
> On Mon, Jul 30, 2012 at 02:52:20PM +0800, Shawn Guo wrote:

> > Ok, makes sense to me.

> In the reply to one of Richard's comment, the volt_old will be used
> not only for diagnostic purpose but also for reverting voltage back
> when clk_set_rate fails.

We should be able to figure out what operating point we set without
having to read it back from the hardware like this.

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Mark Brown]

On Mon, Jul 30, 2012 at 04:20:54PM +0800, Shawn Guo wrote:
> On Mon, Jul 30, 2012 at 02:52:20PM +0800, Shawn Guo wrote:

> > Ok, makes sense to me.

> In the reply to one of Richard's comment, the volt_old will be used
> not only for diagnostic purpose but also for reverting voltage back
> when clk_set_rate fails.

We should be able to figure out what operating point we set without
having to read it back from the hardware like this.

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Mon, Jul 30, 2012 at 07:53:45PM +0100, Mark Brown wrote:
> Quite honestly this seems totally unrealistic for the majority of users,

I doubt that.  Searching drivers/cpufreq folder, I can see there are
several cpufreq drivers scaling voltage with regulator API, but none
of them is calling regulator_set_voltage_time to find voltage latency.
That said, all these users are specify transition latency on their own.

> especially given the very poor documentation for this stuff which SoC
> vendors typically provide.  It's a reasonable amount of work to go back
> and figure this stuff out (especially given that it should be varying
> depending on the transition in question), and it's going to give us a
> bunch of magic numbers in people's bindings.
> 
There will be only one magic number, and it can easily become "magic"
with some comments put there.

> No, add a new API.
> 
Is the following patch what you are ordering here?

diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index da339fd..bfd3cfb 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -352,4 +352,11 @@ static inline void regulator_set_drvdata(struct regulator *regulator,

 #endif

+static inline int regulator_set_voltage_tolerance(struct regulator *regulator,
+                                                 int new_uV, int tol_uV)
+{
+       return regulator_set_voltage(regulator,
+                                    new_uV - tol_uV, new_uV + tol_uV);
+}
+
 #endif

-- 
Regards,
Shawn

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Shawn Guo]

On Mon, Jul 30, 2012 at 07:53:45PM +0100, Mark Brown wrote:
> Quite honestly this seems totally unrealistic for the majority of users,

I doubt that.  Searching drivers/cpufreq folder, I can see there are
several cpufreq drivers scaling voltage with regulator API, but none
of them is calling regulator_set_voltage_time to find voltage latency.
That said, all these users are specify transition latency on their own.

> especially given the very poor documentation for this stuff which SoC
> vendors typically provide.  It's a reasonable amount of work to go back
> and figure this stuff out (especially given that it should be varying
> depending on the transition in question), and it's going to give us a
> bunch of magic numbers in people's bindings.
> 
There will be only one magic number, and it can easily become "magic"
with some comments put there.

> No, add a new API.
> 
Is the following patch what you are ordering here?

diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index da339fd..bfd3cfb 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -352,4 +352,11 @@ static inline void regulator_set_drvdata(struct regulator *regulator,

 #endif

+static inline int regulator_set_voltage_tolerance(struct regulator *regulator,
+                                                 int new_uV, int tol_uV)
+{
+       return regulator_set_voltage(regulator,
+                                    new_uV - tol_uV, new_uV + tol_uV);
+}
+
 #endif

-- 
Regards,
Shawn

Re: [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Mark Brown]

On Tue, Jul 31, 2012 at 12:20:52PM +0800, Shawn Guo wrote:
> On Mon, Jul 30, 2012 at 07:53:45PM +0100, Mark Brown wrote:

> > Quite honestly this seems totally unrealistic for the majority of users,

> I doubt that.  Searching drivers/cpufreq folder, I can see there are
> several cpufreq drivers scaling voltage with regulator API, but none
> of them is calling regulator_set_voltage_time to find voltage latency.
> That said, all these users are specify transition latency on their own.

This is more a sign of poor quality of implementation than anything
else, and obviously the fact that DT is supposed to be an ABI makes it
much more important to get things right.

There's also a bunch of problems in ondemand, it really doesn't cope
terribly well if you actually tell it what the latencies are for most
systems as it becomes absurdly conservative about not ramping.

> > especially given the very poor documentation for this stuff which SoC
> > vendors typically provide.  It's a reasonable amount of work to go back
> > and figure this stuff out (especially given that it should be varying
> > depending on the transition in question), and it's going to give us a
> > bunch of magic numbers in people's bindings.

> There will be only one magic number, and it can easily become "magic"
> with some comments put there.

This doesn't sound like a recipie for success..

> > No, add a new API.

> Is the following patch what you are ordering here?

Yes.  I'd go with _tol rather than _tolerance probably but something
like that, yes.

[PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

[Mark Brown]

On Tue, Jul 31, 2012 at 12:20:52PM +0800, Shawn Guo wrote:
> On Mon, Jul 30, 2012 at 07:53:45PM +0100, Mark Brown wrote:

> > Quite honestly this seems totally unrealistic for the majority of users,

> I doubt that.  Searching drivers/cpufreq folder, I can see there are
> several cpufreq drivers scaling voltage with regulator API, but none
> of them is calling regulator_set_voltage_time to find voltage latency.
> That said, all these users are specify transition latency on their own.

This is more a sign of poor quality of implementation than anything
else, and obviously the fact that DT is supposed to be an ABI makes it
much more important to get things right.

There's also a bunch of problems in ondemand, it really doesn't cope
terribly well if you actually tell it what the latencies are for most
systems as it becomes absurdly conservative about not ramping.

> > especially given the very poor documentation for this stuff which SoC
> > vendors typically provide.  It's a reasonable amount of work to go back
> > and figure this stuff out (especially given that it should be varying
> > depending on the transition in question), and it's going to give us a
> > bunch of magic numbers in people's bindings.

> There will be only one magic number, and it can easily become "magic"
> with some comments put there.

This doesn't sound like a recipie for success..

> > No, add a new API.

> Is the following patch what you are ordering here?

Yes.  I'd go with _tol rather than _tolerance probably but something
like that, yes.