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

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

From: Richard Zhao <redacted>

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 <redacted>
Acked-by: Russell King <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>
---
 .../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

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

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

On Thu, Jul 19, 2012 at 10:54 AM, Shawn Guo [off-list ref] 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:

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.

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.

#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

On Thu, Jul 19, 2012 at 9:24 PM, Shawn Guo [off-list ref] wrote:

From: Richard Zhao <redacted>

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 <redacted>
Acked-by: Russell King <redacted>
---

Nice. Avoids other driver duplicating this code.
May be you can keep Richard's credit in commit too.

FWIW, Acked-by: Santosh Shilimkar [off-list ref]

Shawn,

On Thu, Jul 19, 2012 at 9:24 PM, Shawn Guo [off-list ref] 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 <redacted>
---

[...]

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.

[...]

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

On Fri, Jul 20, 2012 at 12:06:50PM +0530, Shilimkar, Santosh wrote:

On Thu, Jul 19, 2012 at 9:24 PM, Shawn Guo [off-list ref] wrote:

quoted

From: Richard Zhao <redacted>

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 <redacted>
Acked-by: Russell King <redacted>
---

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 [off-list ref]

Thanks.

-- 
Regards,
Shawn

n Fri, Jul 20, 2012 at 1:26 PM, Shawn Guo [off-list ref] wrote:

On Fri, Jul 20, 2012 at 12:06:50PM +0530, Shilimkar, Santosh wrote:

quoted

On Thu, Jul 19, 2012 at 9:24 PM, Shawn Guo [off-list ref] wrote:

quoted

From: Richard Zhao <redacted>

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 <redacted>
Acked-by: Russell King <redacted>
---

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 :-(

Thanks for reviewing it, Nishanth.

On Fri, Jul 20, 2012 at 01:00:26AM -0500, Menon, Nishanth wrote:

quoted

+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.

quoted

+       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.

quoted

+       >;
+};

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.

quoted

+       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.

quoted

+                       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.

quoted

+               }
+       }
+
+       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

quoted

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

Regards,
Nishanth Menon

On Fri, Jul 20, 2012 at 3:46 AM, Shawn Guo [off-list ref] wrote:

quoted

quoted

+       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

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????????

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

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

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.

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 [off-list ref]");
+MODULE_DESCRIPTION("Generic cpufreq driver for CPU0");
+MODULE_LICENSE("GPL");
-- 
1.7.5.4

On Fri, Jul 27, 2012 at 02:33:35PM +0800, Richard Zhao wrote:

quoted

+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.

quoted

+- 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.

quoted

+	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.

quoted

+	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

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:

quoted

quoted

+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.

quoted

quoted

+- 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.

quoted

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.

quoted

quoted

+	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.

quoted

quoted

+	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.

quoted

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.

quoted

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

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