[PATCH 0/3] Add imx6q-cpufreq driver support

[PATCH 0/3] Add imx6q-cpufreq driver support

[Shawn Guo]

The generic cpufreq-cpu0 driver works well for imx6q before.  But it
does not work for 1 GHz and 1.2 GHz operating points the series adds
here.  We ended up with having to add an imx6q specific cpufreq driver
to handle those special frequency and voltage scaling requirements for
the new operating points.

I expect patches #1 and #2 go via cpufreq tree and the #3 through
arm-soc tree.

Shawn Guo (3):
  PM / OPP: Export more symbols for module usage
  cpufreq: add imx6q-cpufreq driver
  ARM: imx: enable imx6q-cpufreq support

 arch/arm/boot/dts/imx6q.dtsi    |   19 ++-
 arch/arm/mach-imx/mach-imx6q.c  |   64 ++++++++
 drivers/base/power/opp.c        |    2 +
 drivers/cpufreq/Kconfig.arm     |    9 ++
 drivers/cpufreq/Makefile        |    1 +
 drivers/cpufreq/imx6q-cpufreq.c |  325 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 414 insertions(+), 6 deletions(-)
 create mode 100644 drivers/cpufreq/imx6q-cpufreq.c

-- 
1.7.9.5

[PATCH 1/3] PM / OPP: Export more symbols for module usage

[Shawn Guo]

Export opp_init_cpufreq_table and opp_free_cpufreq_table for module
usage.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/base/power/opp.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 50b2831..d16db9a 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -661,6 +661,7 @@ int opp_init_cpufreq_table(struct device *dev,
 
 	return 0;
 }
+EXPORT_SYMBOL(opp_init_cpufreq_table);
 
 /**
  * opp_free_cpufreq_table() - free the cpufreq table
@@ -678,6 +679,7 @@ void opp_free_cpufreq_table(struct device *dev,
 	kfree(*table);
 	*table = NULL;
 }
+EXPORT_SYMBOL(opp_free_cpufreq_table);
 #endif		/* CONFIG_CPU_FREQ */
 
 /**
-- 
1.7.9.5

[PATCH 2/3] cpufreq: add imx6q-cpufreq driver

[Shawn Guo]

Add an imx6q-cpufreq for Freescale i.MX6Q SoC to handle the hardware
specific frequency and voltage scaling requirements.

The driver supports module build and is instantiated by the platform
device/driver mechanism, so that it will be instantiated on other
platform, as IMX is built with multiplatform support.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/cpufreq/Kconfig.arm     |    9 ++
 drivers/cpufreq/Makefile        |    1 +
 drivers/cpufreq/imx6q-cpufreq.c |  325 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 335 insertions(+)
 create mode 100644 drivers/cpufreq/imx6q-cpufreq.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index a0b3661..9e628ba 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -77,6 +77,15 @@ config ARM_EXYNOS5250_CPUFREQ
 	  This adds the CPUFreq driver for Samsung EXYNOS5250
 	  SoC.
 
+config ARM_IMX6Q_CPUFREQ
+	tristate "Freescale i.MX6Q cpufreq support"
+	depends on SOC_IMX6Q
+	depends on REGULATOR_ANATOP
+	help
+	  This adds cpufreq driver support for Freescale i.MX6Q SOC.
+
+	  If in doubt, say N.
+
 config ARM_SPEAR_CPUFREQ
 	bool "SPEAr CPUFreq support"
 	depends on PLAT_SPEAR
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 1f254ec0..31699a0 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_ARM_EXYNOS_CPUFREQ)	+= exynos-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS4210_CPUFREQ)	+= exynos4210-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS4X12_CPUFREQ)	+= exynos4x12-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS5250_CPUFREQ)	+= exynos5250-cpufreq.o
+obj-$(CONFIG_ARM_IMX6Q_CPUFREQ)		+= imx6q-cpufreq.o
 obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ)     += omap-cpufreq.o
 obj-$(CONFIG_ARM_SPEAR_CPUFREQ)		+= spear-cpufreq.o
 
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
new file mode 100644
index 0000000..8b3db8c
--- /dev/null
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -0,0 +1,325 @@
+/*
+ * Copyright (C) 2013 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#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/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+#define PU_SOC_VOLTAGE_NORMAL	1250000
+#define PU_SOC_VOLTAGE_HIGH	1275000
+#define FREQ_1P2_GHZ		1200000000
+
+static struct regulator *arm_reg;
+static struct regulator *pu_reg;
+static struct regulator *soc_reg;
+
+static struct clk *arm_clk;
+static struct clk *pll1_sys_clk;
+static struct clk *pll1_sw_clk;
+static struct clk *step_clk;
+static struct clk *pll2_pfd2_396m_clk;
+
+static struct device *cpu_dev;
+static struct cpufreq_frequency_table *freq_table;
+static unsigned int transition_latency;
+
+static int imx6q_verify_speed(struct cpufreq_policy *policy)
+{
+	return cpufreq_frequency_table_verify(policy, freq_table);
+}
+
+static unsigned int imx6q_get_speed(unsigned int cpu)
+{
+	return clk_get_rate(arm_clk) / 1000;
+}
+
+static int imx6q_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, volt_old;
+	unsigned int index, cpu;
+	int ret;
+
+	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 = freq_table[index].frequency * 1000;
+	freqs.new = freq_hz / 1000;
+	freqs.old = clk_get_rate(arm_clk) / 1000;
+
+	if (freqs.old == freqs.new)
+		return 0;
+
+	for_each_online_cpu(cpu) {
+		freqs.cpu = cpu;
+		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+	}
+
+	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);
+	volt_old = regulator_get_voltage(arm_reg);
+
+	pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
+		 freqs.old / 1000, volt_old / 1000,
+		 freqs.new / 1000, volt / 1000);
+
+	/* scaling up?  scale voltage before frequency */
+	if (freqs.new > freqs.old) {
+		ret = regulator_set_voltage_tol(arm_reg, volt, 0);
+		if (ret) {
+			pr_err("failed to scale voltage up: %d\n", ret);
+			freqs.new = freqs.old;
+			return ret;
+		}
+
+		/*
+		 * Need to increase vddpu and vddsoc for safety
+		 * if we are about to run at 1.2 GHz.
+		 */
+		if (freqs.new == FREQ_1P2_GHZ / 1000) {
+			regulator_set_voltage_tol(pu_reg,
+					PU_SOC_VOLTAGE_HIGH, 0);
+			regulator_set_voltage_tol(soc_reg,
+					PU_SOC_VOLTAGE_HIGH, 0);
+		}
+	}
+
+	/*
+	 * The setpoints are selected per PLL/PDF frequencies, so we need to
+	 * reprogram PLL for frequency scaling.  The procedure of reprogramming
+	 * PLL1 is as blow.
+	 *
+	 *  - Enable pll2_pfd2_396m_clk and reparent pll1_sw_clk to it
+	 *  - Disable pll1_sys_clk and reprogram it
+	 *  - Enable pll1_sys_clk and reparent pll1_sw_clk back to it
+	 *  - Disable pll2_pfd2_396m_clk
+	 */
+	clk_prepare_enable(pll2_pfd2_396m_clk);
+	clk_set_parent(step_clk, pll2_pfd2_396m_clk);
+	clk_set_parent(pll1_sw_clk, step_clk);
+	clk_prepare_enable(pll1_sys_clk);
+	if (freq_hz > clk_get_rate(pll2_pfd2_396m_clk)) {
+		clk_disable_unprepare(pll1_sys_clk);
+		clk_set_rate(pll1_sys_clk, freqs.new * 1000);
+		clk_prepare_enable(pll1_sys_clk);
+		clk_set_parent(pll1_sw_clk, pll1_sys_clk);
+		clk_disable_unprepare(pll2_pfd2_396m_clk);
+	} else {
+		/*
+		 * Disable pll1_sys_clk if pll2_pfd2_396m_clk is sufficient
+		 * to provide the frequency.
+		 */
+		clk_disable_unprepare(pll1_sys_clk);
+	}
+
+	/* Ensure the arm clock divider is what we expect */
+	ret = clk_set_rate(arm_clk, freqs.new * 1000);
+	if (ret) {
+		pr_err("failed to set clock rate: %d\n", ret);
+		regulator_set_voltage_tol(arm_reg, volt_old, 0);
+		return ret;
+	}
+
+	/* scaling down?  scale voltage after frequency */
+	if (freqs.new < freqs.old) {
+		ret = regulator_set_voltage_tol(arm_reg, volt, 0);
+		if (ret)
+			pr_warn("failed to scale voltage down: %d\n", ret);
+
+		if (freqs.old == FREQ_1P2_GHZ / 1000) {
+			regulator_set_voltage_tol(pu_reg,
+					PU_SOC_VOLTAGE_NORMAL, 0);
+			regulator_set_voltage_tol(soc_reg,
+					PU_SOC_VOLTAGE_NORMAL, 0);
+		}
+	}
+
+	for_each_online_cpu(cpu) {
+		freqs.cpu = cpu;
+		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+	}
+
+	return 0;
+}
+
+static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
+{
+	int ret;
+
+	if (policy->cpu != 0)
+		return -EINVAL;
+
+	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
+	if (ret) {
+		pr_err("invalid frequency table: %d\n", ret);
+		return ret;
+	}
+
+	policy->cpuinfo.transition_latency = transition_latency;
+	policy->cur = clk_get_rate(arm_clk) / 1000;
+	policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
+	cpumask_setall(policy->cpus);
+	cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
+
+	return 0;
+}
+
+static int imx6q_cpufreq_exit(struct cpufreq_policy *policy)
+{
+	cpufreq_frequency_table_put_attr(policy->cpu);
+	return 0;
+}
+
+static struct freq_attr *imx6q_cpufreq_attr[] = {
+	&cpufreq_freq_attr_scaling_available_freqs,
+	NULL,
+};
+
+static struct cpufreq_driver imx6q_cpufreq_driver = {
+	.verify = imx6q_verify_speed,
+	.target = imx6q_set_target,
+	.get = imx6q_get_speed,
+	.init = imx6q_cpufreq_init,
+	.exit = imx6q_cpufreq_exit,
+	.name = "imx6q-cpufreq",
+	.attr = imx6q_cpufreq_attr,
+};
+
+static int imx6q_cpufreq_probe(struct platform_device *pdev)
+{
+	struct device_node *np;
+	int ret;
+
+	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 put_node;
+	}
+
+	cpu_dev->of_node = np;
+
+	arm_clk = clk_get(cpu_dev, "arm");
+	pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
+	pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
+	step_clk = clk_get(cpu_dev, "step");
+	pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
+	if (IS_ERR(arm_clk) || IS_ERR(pll1_sys_clk) || IS_ERR(pll1_sw_clk) ||
+	    IS_ERR(step_clk) || IS_ERR(pll2_pfd2_396m_clk)) {
+		pr_err("failed to get clocks\n");
+		ret = -ENOENT;
+		goto put_node;
+	}
+
+	arm_reg = regulator_get(cpu_dev, "arm");
+	pu_reg = regulator_get(cpu_dev, "pu");
+	soc_reg = regulator_get(cpu_dev, "soc");
+	if (!arm_reg || !pu_reg || !soc_reg) {
+		pr_err("failed to get regulators\n");
+		ret = -ENOENT;
+		goto put_clk;
+	}
+
+	/* We expect an OPP table supplied by platform */
+	ret = opp_get_opp_count(cpu_dev);
+	if (ret < 0) {
+		pr_err("no OPP table is found: %d\n", ret);
+		goto put_reg;
+	}
+
+	ret = opp_init_cpufreq_table(cpu_dev, &freq_table);
+	if (ret) {
+		pr_err("failed to init cpufreq table: %d\n", ret);
+		goto put_reg;
+	}
+
+	if (of_property_read_u32(np, "clock-latency", &transition_latency))
+		transition_latency = CPUFREQ_ETERNAL;
+
+	ret = cpufreq_register_driver(&imx6q_cpufreq_driver);
+	if (ret) {
+		pr_err("failed register driver: %d\n", ret);
+		goto free_freq_table;
+	}
+
+	of_node_put(np);
+	return 0;
+
+free_freq_table:
+	opp_free_cpufreq_table(cpu_dev, &freq_table);
+put_reg:
+	regulator_put(soc_reg);
+	regulator_put(pu_reg);
+	regulator_put(arm_reg);
+put_clk:
+	clk_put(pll2_pfd2_396m_clk);
+	clk_put(step_clk);
+	clk_put(pll1_sw_clk);
+	clk_put(pll1_sys_clk);
+	clk_put(arm_clk);
+put_node:
+	of_node_put(np);
+	return ret;
+}
+
+static int imx6q_cpufreq_remove(struct platform_device *pdev)
+{
+	cpufreq_unregister_driver(&imx6q_cpufreq_driver);
+	opp_free_cpufreq_table(cpu_dev, &freq_table);
+	regulator_put(soc_reg);
+	regulator_put(pu_reg);
+	regulator_put(arm_reg);
+	clk_put(pll2_pfd2_396m_clk);
+	clk_put(step_clk);
+	clk_put(pll1_sw_clk);
+	clk_put(pll1_sys_clk);
+	clk_put(arm_clk);
+
+	return 0;
+}
+
+static struct platform_driver imx6q_cpufreq_platdrv = {
+	.driver = {
+		.name	= "imx6q_cpufreq",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= imx6q_cpufreq_probe,
+	.remove		= imx6q_cpufreq_remove,
+};
+module_platform_driver(imx6q_cpufreq_platdrv);
+
+MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>");
+MODULE_DESCRIPTION("Freescale i.MX6Q cpufreq driver");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

[PATCH 3/3] ARM: imx: enable imx6q-cpufreq support

[Shawn Guo]

Update operating-points per hardware document and add support for
1 GHz and 1.2 GHz frequencies.

400 MHz, 800 MHz and 1 GHz should be supported by all i.MX6Q chips,
while 1.2 GHz support needs to know from OTP fuse bit.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/boot/dts/imx6q.dtsi   |   19 ++++++++----
 arch/arm/mach-imx/mach-imx6q.c |   64 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 6 deletions(-)

diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index d6265ca..17c5618 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -38,12 +38,18 @@
 			next-level-cache = <&L2>;
 			operating-points = <
 				/* kHz    uV */
-				792000  1100000
+				996000  1250000
+				792000  1150000
 				396000  950000
-				198000  850000
 			>;
 			clock-latency = <61036>; /* two CLK32 periods */
-			cpu0-supply = <&reg_cpu>;
+			clocks = <&clks 104>, <&clks 6>, <&clks 16>,
+				 <&clks 17>, <&clks 170>;
+			clock-names = "arm", "pll2_pfd2_396m", "step",
+				      "pll1_sw", "pll1_sys";
+			arm-supply = <&reg_arm>;
+			pu-supply = <&reg_pu>;
+			soc-supply = <&reg_soc>;
 		};
 
 		cpu@1 {
@@ -471,7 +477,7 @@
 					anatop-max-voltage = <2750000>;
 				};
 
-				reg_cpu: regulator-vddcore@140 {
+				reg_arm: regulator-vddcore@140 {
 					compatible = "fsl,anatop-regulator";
 					regulator-name = "cpu";
 					regulator-min-microvolt = <725000>;
@@ -485,7 +491,7 @@
 					anatop-max-voltage = <1450000>;
 				};
 
-				regulator-vddpu@140 {
+				reg_pu: regulator-vddpu@140 {
 					compatible = "fsl,anatop-regulator";
 					regulator-name = "vddpu";
 					regulator-min-microvolt = <725000>;
@@ -499,7 +505,7 @@
 					anatop-max-voltage = <1450000>;
 				};
 
-				regulator-vddsoc@140 {
+				reg_soc: regulator-vddsoc@140 {
 					compatible = "fsl,anatop-regulator";
 					regulator-name = "vddsoc";
 					regulator-min-microvolt = <725000>;
@@ -965,6 +971,7 @@
 			};
 
 			ocotp@021bc000 {
+				compatible = "fsl,imx6q-ocotp";
 				reg = <0x021bc000 0x4000>;
 			};
 
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 4eb1b3a..26f63f28 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -12,6 +12,7 @@
 
 #include <linux/clk.h>
 #include <linux/clkdev.h>
+#include <linux/cpu.h>
 #include <linux/cpuidle.h>
 #include <linux/delay.h>
 #include <linux/export.h>
@@ -22,6 +23,7 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/opp.h>
 #include <linux/phy.h>
 #include <linux/regmap.h>
 #include <linux/micrel_phy.h>
@@ -209,9 +211,71 @@ static struct cpuidle_driver imx6q_cpuidle_driver = {
 	.state_count		= 1,
 };
 
+#define OCOTP_CFG3			0x440
+#define OCOTP_CFG3_SPEED_SHIFT		16
+#define OCOTP_CFG3_SPEED_1P2GHZ		0x3
+
+static void __init imx6q_opp_check_1p2ghz(struct device *cpu_dev)
+{
+	struct device_node *np;
+	void __iomem *base;
+	u32 val;
+
+	np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ocotp");
+	base = of_iomap(np, 0);
+	if (!base) {
+		pr_warn("failed to map ocotp\n");
+		goto put_node;
+	}
+
+	val = readl_relaxed(base + OCOTP_CFG3);
+	val >>= OCOTP_CFG3_SPEED_SHIFT;
+	if ((val & 0x3) == OCOTP_CFG3_SPEED_1P2GHZ)
+		if (opp_add(cpu_dev, 1200000000, 1275000))
+			pr_warn("failed to add 1.2 GHz operating point\n");
+
+put_node:
+	of_node_put(np);
+}
+
+static void __init imx6q_opp_init(void)
+{
+	struct device *cpu_dev;
+	struct device_node *np;
+
+	np = of_find_node_by_path("/cpus/cpu@0");
+	if (!np) {
+		pr_warn("failed to find cpu0 node\n");
+		return;
+	}
+
+	cpu_dev = get_cpu_device(0);
+	if (!cpu_dev) {
+		pr_warn("failed to get cpu0 device\n");
+		goto put_node;
+	}
+
+	cpu_dev->of_node = np;
+	if (of_init_opp_table(cpu_dev))
+		pr_warn("failed to init OPP table\n");
+
+	imx6q_opp_check_1p2ghz(cpu_dev);
+
+put_node:
+	of_node_put(np);
+}
+
 static void __init imx6q_init_late(void)
 {
 	imx_cpuidle_init(&imx6q_cpuidle_driver);
+
+	if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ)) {
+		struct platform_device_info pdevinfo = {
+			.name = "imx6q_cpufreq",
+		};
+		imx6q_opp_init();
+		platform_device_register_full(&pdevinfo);
+	}
 }
 
 static void __init imx6q_map_io(void)
-- 
1.7.9.5

Re: [PATCH 2/3] cpufreq: add imx6q-cpufreq driver

[Sascha Hauer]

On Tue, Jan 08, 2013 at 02:54:02PM +0800, Shawn Guo wrote:
> Add an imx6q-cpufreq for Freescale i.MX6Q SoC to handle the hardware
> specific frequency and voltage scaling requirements.
> 
> The driver supports module build and is instantiated by the platform
> device/driver mechanism, so that it will be instantiated on other
> platform, as IMX is built with multiplatform support.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/cpufreq/Kconfig.arm     |    9 ++
>  drivers/cpufreq/Makefile        |    1 +
>  drivers/cpufreq/imx6q-cpufreq.c |  325 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 335 insertions(+)
>  create mode 100644 drivers/cpufreq/imx6q-cpufreq.c
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index a0b3661..9e628ba 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -77,6 +77,15 @@ config ARM_EXYNOS5250_CPUFREQ
>  	  This adds the CPUFreq driver for Samsung EXYNOS5250
>  	  SoC.
>  
> +config ARM_IMX6Q_CPUFREQ
> +	tristate "Freescale i.MX6Q cpufreq support"
> +	depends on SOC_IMX6Q
> +	depends on REGULATOR_ANATOP
> +	help
> +	  This adds cpufreq driver support for Freescale i.MX6Q SOC.
> +
> +	  If in doubt, say N.
> +
>  config ARM_SPEAR_CPUFREQ
>  	bool "SPEAr CPUFreq support"
>  	depends on PLAT_SPEAR
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 1f254ec0..31699a0 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -49,6 +49,7 @@ obj-$(CONFIG_ARM_EXYNOS_CPUFREQ)	+= exynos-cpufreq.o
>  obj-$(CONFIG_ARM_EXYNOS4210_CPUFREQ)	+= exynos4210-cpufreq.o
>  obj-$(CONFIG_ARM_EXYNOS4X12_CPUFREQ)	+= exynos4x12-cpufreq.o
>  obj-$(CONFIG_ARM_EXYNOS5250_CPUFREQ)	+= exynos5250-cpufreq.o
> +obj-$(CONFIG_ARM_IMX6Q_CPUFREQ)		+= imx6q-cpufreq.o
>  obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ)     += omap-cpufreq.o
>  obj-$(CONFIG_ARM_SPEAR_CPUFREQ)		+= spear-cpufreq.o
>  
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> new file mode 100644
> index 0000000..8b3db8c
> --- /dev/null
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -0,0 +1,325 @@
> +/*
> + * Copyright (C) 2013 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#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/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +#define PU_SOC_VOLTAGE_NORMAL	1250000
> +#define PU_SOC_VOLTAGE_HIGH	1275000
> +#define FREQ_1P2_GHZ		1200000000
> +
> +static struct regulator *arm_reg;
> +static struct regulator *pu_reg;
> +static struct regulator *soc_reg;
> +
> +static struct clk *arm_clk;
> +static struct clk *pll1_sys_clk;
> +static struct clk *pll1_sw_clk;
> +static struct clk *step_clk;
> +static struct clk *pll2_pfd2_396m_clk;
> +
> +static struct device *cpu_dev;
> +static struct cpufreq_frequency_table *freq_table;
> +static unsigned int transition_latency;
> +
> +static int imx6q_verify_speed(struct cpufreq_policy *policy)
> +{
> +	return cpufreq_frequency_table_verify(policy, freq_table);
> +}
> +
> +static unsigned int imx6q_get_speed(unsigned int cpu)
> +{
> +	return clk_get_rate(arm_clk) / 1000;
> +}
> +
> +static int imx6q_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, volt_old;
> +	unsigned int index, cpu;
> +	int ret;
> +
> +	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 = freq_table[index].frequency * 1000;
> +	freqs.new = freq_hz / 1000;
> +	freqs.old = clk_get_rate(arm_clk) / 1000;
> +
> +	if (freqs.old == freqs.new)
> +		return 0;
> +
> +	for_each_online_cpu(cpu) {
> +		freqs.cpu = cpu;
> +		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> +	}
> +
> +	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);
> +	volt_old = regulator_get_voltage(arm_reg);
> +
> +	pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
> +		 freqs.old / 1000, volt_old / 1000,
> +		 freqs.new / 1000, volt / 1000);
> +
> +	/* scaling up?  scale voltage before frequency */
> +	if (freqs.new > freqs.old) {
> +		ret = regulator_set_voltage_tol(arm_reg, volt, 0);
> +		if (ret) {
> +			pr_err("failed to scale voltage up: %d\n", ret);
> +			freqs.new = freqs.old;
> +			return ret;
> +		}

So this regulator_set_voltage_tol can fail, ...

> +
> +		/*
> +		 * Need to increase vddpu and vddsoc for safety
> +		 * if we are about to run at 1.2 GHz.
> +		 */
> +		if (freqs.new == FREQ_1P2_GHZ / 1000) {
> +			regulator_set_voltage_tol(pu_reg,
> +					PU_SOC_VOLTAGE_HIGH, 0);
> +			regulator_set_voltage_tol(soc_reg,
> +					PU_SOC_VOLTAGE_HIGH, 0);

... but these can't?

> +		}
> +	}
> +
> +	/*
> +	 * The setpoints are selected per PLL/PDF frequencies, so we need to
> +	 * reprogram PLL for frequency scaling.  The procedure of reprogramming
> +	 * PLL1 is as blow.

s/blow/below/

> +	 *
> +	 *  - Enable pll2_pfd2_396m_clk and reparent pll1_sw_clk to it
> +	 *  - Disable pll1_sys_clk and reprogram it
> +	 *  - Enable pll1_sys_clk and reparent pll1_sw_clk back to it
> +	 *  - Disable pll2_pfd2_396m_clk
> +	 */

[...]

> +
> +static struct cpufreq_driver imx6q_cpufreq_driver = {
> +	.verify = imx6q_verify_speed,
> +	.target = imx6q_set_target,
> +	.get = imx6q_get_speed,
> +	.init = imx6q_cpufreq_init,
> +	.exit = imx6q_cpufreq_exit,
> +	.name = "imx6q-cpufreq",
> +	.attr = imx6q_cpufreq_attr,
> +};
> +
> +static int imx6q_cpufreq_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np;
> +	int ret;
> +
> +	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");

Since you have a struct device * you should use it throughout the driver
to give the messages a bit more context.

> +		ret = -ENODEV;
> +		goto put_node;
> +	}
> +
> +	cpu_dev->of_node = np;
> +
> +	arm_clk = clk_get(cpu_dev, "arm");
> +	pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
> +	pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
> +	step_clk = clk_get(cpu_dev, "step");
> +	pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");

devm_*?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/3] cpufreq: add imx6q-cpufreq driver

[Viresh Kumar]

Hi Shawn,

Apart from Sascha's comments, please see few comment from my side:

On Tue, Jan 8, 2013 at 12:24 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> +static struct regulator *arm_reg;
> +static struct regulator *pu_reg;
> +static struct regulator *soc_reg;

Can move to a single line, if you like.

> +static struct clk *arm_clk;
> +static struct clk *pll1_sys_clk;
> +static struct clk *pll1_sw_clk;
> +static struct clk *step_clk;
> +static struct clk *pll2_pfd2_396m_clk;

same.

> +static int imx6q_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, volt_old;
> +       unsigned int index, cpu;
> +       int ret;
> +
> +       ret = cpufreq_frequency_table_target(policy, freq_table, target_freq,
> +                                            relation, &index);
> +       if (ret) {
> +               pr_err("failed to match target freqency %d: %d\n",

s/freqency/frequency

> +                      target_freq, ret);
> +               return ret;
> +       }
> +
> +       freq_hz = freq_table[index].frequency * 1000;
> +       freqs.new = freq_hz / 1000;

could be written as:

       freqs.new = freq_table[index].frequency;
       freq_hz = freqs.new * 1000;


> +       freqs.old = clk_get_rate(arm_clk) / 1000;
> +
> +       if (freqs.old == freqs.new)
> +               return 0;
> +
> +       for_each_online_cpu(cpu) {
> +               freqs.cpu = cpu;
> +               cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> +       }
> +
> +       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);
> +       volt_old = regulator_get_voltage(arm_reg);
> +
> +       pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
> +                freqs.old / 1000, volt_old / 1000,
> +                freqs.new / 1000, volt / 1000);
> +
> +       /* scaling up?  scale voltage before frequency */
> +       if (freqs.new > freqs.old) {
> +               ret = regulator_set_voltage_tol(arm_reg, volt, 0);
> +               if (ret) {
> +                       pr_err("failed to scale voltage up: %d\n", ret);
> +                       freqs.new = freqs.old;

is this useful?

> +                       return ret;
> +               }
> +
> +               /*
> +                * Need to increase vddpu and vddsoc for safety
> +                * if we are about to run at 1.2 GHz.
> +                */
> +               if (freqs.new == FREQ_1P2_GHZ / 1000) {
> +                       regulator_set_voltage_tol(pu_reg,
> +                                       PU_SOC_VOLTAGE_HIGH, 0);
> +                       regulator_set_voltage_tol(soc_reg,
> +                                       PU_SOC_VOLTAGE_HIGH, 0);
> +               }
> +       }
> +
> +       /*
> +        * The setpoints are selected per PLL/PDF frequencies, so we need to
> +        * reprogram PLL for frequency scaling.  The procedure of reprogramming
> +        * PLL1 is as blow.
> +        *
> +        *  - Enable pll2_pfd2_396m_clk and reparent pll1_sw_clk to it
> +        *  - Disable pll1_sys_clk and reprogram it
> +        *  - Enable pll1_sys_clk and reparent pll1_sw_clk back to it
> +        *  - Disable pll2_pfd2_396m_clk
> +        */
> +       clk_prepare_enable(pll2_pfd2_396m_clk);
> +       clk_set_parent(step_clk, pll2_pfd2_396m_clk);
> +       clk_set_parent(pll1_sw_clk, step_clk);
> +       clk_prepare_enable(pll1_sys_clk);
> +       if (freq_hz > clk_get_rate(pll2_pfd2_396m_clk)) {
> +               clk_disable_unprepare(pll1_sys_clk);
> +               clk_set_rate(pll1_sys_clk, freqs.new * 1000);
> +               clk_prepare_enable(pll1_sys_clk);
> +               clk_set_parent(pll1_sw_clk, pll1_sys_clk);
> +               clk_disable_unprepare(pll2_pfd2_396m_clk);
> +       } else {
> +               /*
> +                * Disable pll1_sys_clk if pll2_pfd2_396m_clk is sufficient
> +                * to provide the frequency.
> +                */
> +               clk_disable_unprepare(pll1_sys_clk);

Are you doing anything in  clk_[un]prepare ? If not, do clk_[un]prepare
at init and exit and do enable/disable here.

> +       }
> +
> +       /* Ensure the arm clock divider is what we expect */
> +       ret = clk_set_rate(arm_clk, freqs.new * 1000);
> +       if (ret) {
> +               pr_err("failed to set clock rate: %d\n", ret);
> +               regulator_set_voltage_tol(arm_reg, volt_old, 0);
> +               return ret;
> +       }
> +
> +       /* scaling down?  scale voltage after frequency */
> +       if (freqs.new < freqs.old) {
> +               ret = regulator_set_voltage_tol(arm_reg, volt, 0);
> +               if (ret)
> +                       pr_warn("failed to scale voltage down: %d\n", ret);
> +
> +               if (freqs.old == FREQ_1P2_GHZ / 1000) {
> +                       regulator_set_voltage_tol(pu_reg,
> +                                       PU_SOC_VOLTAGE_NORMAL, 0);
> +                       regulator_set_voltage_tol(soc_reg,
> +                                       PU_SOC_VOLTAGE_NORMAL, 0);
> +               }
> +       }
> +
> +       for_each_online_cpu(cpu) {
> +               freqs.cpu = cpu;
> +               cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> +       }
> +
> +       return 0;
> +}
> +
> +static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
> +{
> +       int ret;
> +
> +       if (policy->cpu != 0)
> +               return -EINVAL;

Why? The problem here is when you hot-unplug cpu0, init would be called for
cpu1 and it would fail.

> +
> +       ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> +       if (ret) {
> +               pr_err("invalid frequency table: %d\n", ret);
> +               return ret;
> +       }
> +
> +       policy->cpuinfo.transition_latency = transition_latency;
> +       policy->cur = clk_get_rate(arm_clk) / 1000;
> +       policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> +       cpumask_setall(policy->cpus);
> +       cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
> +
> +       return 0;
> +}
> +
> +static int imx6q_cpufreq_exit(struct cpufreq_policy *policy)
> +{
> +       cpufreq_frequency_table_put_attr(policy->cpu);
> +       return 0;
> +}
> +
> +static struct freq_attr *imx6q_cpufreq_attr[] = {
> +       &cpufreq_freq_attr_scaling_available_freqs,
> +       NULL,
> +};
> +
> +static struct cpufreq_driver imx6q_cpufreq_driver = {
> +       .verify = imx6q_verify_speed,
> +       .target = imx6q_set_target,
> +       .get = imx6q_get_speed,
> +       .init = imx6q_cpufreq_init,
> +       .exit = imx6q_cpufreq_exit,
> +       .name = "imx6q-cpufreq",
> +       .attr = imx6q_cpufreq_attr,
> +};
> +
> +static int imx6q_cpufreq_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np;
> +       int ret;
> +
> +       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 put_node;
> +       }
> +
> +       cpu_dev->of_node = np;
> +
> +       arm_clk = clk_get(cpu_dev, "arm");
> +       pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
> +       pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
> +       step_clk = clk_get(cpu_dev, "step");
> +       pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
> +       if (IS_ERR(arm_clk) || IS_ERR(pll1_sys_clk) || IS_ERR(pll1_sw_clk) ||
> +           IS_ERR(step_clk) || IS_ERR(pll2_pfd2_396m_clk)) {
> +               pr_err("failed to get clocks\n");
> +               ret = -ENOENT;
> +               goto put_node;

That looks to be wrong. If clk failed for the last clk_get, you must
free all other
clks too.

> +       }
> +
> +       arm_reg = regulator_get(cpu_dev, "arm");
> +       pu_reg = regulator_get(cpu_dev, "pu");
> +       soc_reg = regulator_get(cpu_dev, "soc");
> +       if (!arm_reg || !pu_reg || !soc_reg) {
> +               pr_err("failed to get regulators\n");
> +               ret = -ENOENT;
> +               goto put_clk;

ditto

> +       }
> +
> +       /* We expect an OPP table supplied by platform */
> +       ret = opp_get_opp_count(cpu_dev);
> +       if (ret < 0) {
> +               pr_err("no OPP table is found: %d\n", ret);
> +               goto put_reg;
> +       }
> +
> +       ret = opp_init_cpufreq_table(cpu_dev, &freq_table);
> +       if (ret) {
> +               pr_err("failed to init cpufreq table: %d\n", ret);
> +               goto put_reg;
> +       }
> +
> +       if (of_property_read_u32(np, "clock-latency", &transition_latency))
> +               transition_latency = CPUFREQ_ETERNAL;
> +
> +       ret = cpufreq_register_driver(&imx6q_cpufreq_driver);
> +       if (ret) {
> +               pr_err("failed register driver: %d\n", ret);
> +               goto free_freq_table;
> +       }
> +
> +       of_node_put(np);
> +       return 0;
> +
> +free_freq_table:
> +       opp_free_cpufreq_table(cpu_dev, &freq_table);
> +put_reg:
> +       regulator_put(soc_reg);
> +       regulator_put(pu_reg);
> +       regulator_put(arm_reg);
> +put_clk:
> +       clk_put(pll2_pfd2_396m_clk);
> +       clk_put(step_clk);
> +       clk_put(pll1_sw_clk);
> +       clk_put(pll1_sys_clk);
> +       clk_put(arm_clk);
> +put_node:
> +       of_node_put(np);
> +       return ret;
> +}
> +
> +static int imx6q_cpufreq_remove(struct platform_device *pdev)
> +{
> +       cpufreq_unregister_driver(&imx6q_cpufreq_driver);
> +       opp_free_cpufreq_table(cpu_dev, &freq_table);
> +       regulator_put(soc_reg);
> +       regulator_put(pu_reg);
> +       regulator_put(arm_reg);
> +       clk_put(pll2_pfd2_396m_clk);
> +       clk_put(step_clk);
> +       clk_put(pll1_sw_clk);
> +       clk_put(pll1_sys_clk);
> +       clk_put(arm_clk);
> +
> +       return 0;
> +}
> +
> +static struct platform_driver imx6q_cpufreq_platdrv = {
> +       .driver = {
> +               .name   = "imx6q_cpufreq",
> +               .owner  = THIS_MODULE,
> +       },
> +       .probe          = imx6q_cpufreq_probe,
> +       .remove         = imx6q_cpufreq_remove,
> +};
> +module_platform_driver(imx6q_cpufreq_platdrv);
> +
> +MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>");
> +MODULE_DESCRIPTION("Freescale i.MX6Q cpufreq driver");
> +MODULE_LICENSE("GPL");
> --
> 1.7.9.5
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] cpufreq: add imx6q-cpufreq driver

[Shawn Guo]

On Tue, Jan 08, 2013 at 09:57:56AM +0100, Sascha Hauer wrote:
...
> > +	/* scaling up?  scale voltage before frequency */
> > +	if (freqs.new > freqs.old) {
> > +		ret = regulator_set_voltage_tol(arm_reg, volt, 0);
> > +		if (ret) {
> > +			pr_err("failed to scale voltage up: %d\n", ret);
> > +			freqs.new = freqs.old;
> > +			return ret;
> > +		}
> 
> So this regulator_set_voltage_tol can fail, ...
> 
> > +
> > +		/*
> > +		 * Need to increase vddpu and vddsoc for safety
> > +		 * if we are about to run at 1.2 GHz.
> > +		 */
> > +		if (freqs.new == FREQ_1P2_GHZ / 1000) {
> > +			regulator_set_voltage_tol(pu_reg,
> > +					PU_SOC_VOLTAGE_HIGH, 0);
> > +			regulator_set_voltage_tol(soc_reg,
> > +					PU_SOC_VOLTAGE_HIGH, 0);
> 
> ... but these can't?
> 
I initially have return of every single clk and regulator API call
checked and try to recover everything for error cases.  The driver
becomes messy and hard to read with error message and recovering
code all over the places.  And I think it's a little bit over
engineering, so end up choosing to check on selected and critical
part to make sure clk and regulator subsystem work good.

> > +		}
> > +	}
> > +
> > +	/*
> > +	 * The setpoints are selected per PLL/PDF frequencies, so we need to
> > +	 * reprogram PLL for frequency scaling.  The procedure of reprogramming
> > +	 * PLL1 is as blow.
> 
> s/blow/below/
> 
Thanks.

> > +	 *
> > +	 *  - Enable pll2_pfd2_396m_clk and reparent pll1_sw_clk to it
> > +	 *  - Disable pll1_sys_clk and reprogram it
> > +	 *  - Enable pll1_sys_clk and reparent pll1_sw_clk back to it
> > +	 *  - Disable pll2_pfd2_396m_clk
> > +	 */
> 
> [...]
> 
> > +
> > +static struct cpufreq_driver imx6q_cpufreq_driver = {
> > +	.verify = imx6q_verify_speed,
> > +	.target = imx6q_set_target,
> > +	.get = imx6q_get_speed,
> > +	.init = imx6q_cpufreq_init,
> > +	.exit = imx6q_cpufreq_exit,
> > +	.name = "imx6q-cpufreq",
> > +	.attr = imx6q_cpufreq_attr,
> > +};
> > +
> > +static int imx6q_cpufreq_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *np;
> > +	int ret;
> > +
> > +	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");
> 
> Since you have a struct device * you should use it throughout the driver
> to give the messages a bit more context.
> 
In that case, the context will be "cpu cpu0: ...", while we have the
context be "imx6q-cpufreq: ..." right now, since we have the following
line at the very beginning of the code.

#define pr_fmt(fmt)     KBUILD_MODNAME ": " fmt

I think we have a better context with pr_* than dev_* in this case.

> > +		ret = -ENODEV;
> > +		goto put_node;
> > +	}
> > +
> > +	cpu_dev->of_node = np;
> > +
> > +	arm_clk = clk_get(cpu_dev, "arm");
> > +	pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
> > +	pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
> > +	step_clk = clk_get(cpu_dev, "step");
> > +	pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
> 
> devm_*?
> 
That what I tried to do at the first time.  But the dev of cpu is
special, and managed functions do not work as we expect with it.

Shawn

Re: [PATCH 2/3] cpufreq: add imx6q-cpufreq driver

[Shawn Guo]

On Tue, Jan 08, 2013 at 07:28:59PM +0800, Shawn Guo wrote:
> > > +	cpu_dev->of_node = np;
> > > +
> > > +	arm_clk = clk_get(cpu_dev, "arm");
> > > +	pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
> > > +	pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
> > > +	step_clk = clk_get(cpu_dev, "step");
> > > +	pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
> > 
> > devm_*?
> > 
> That what I tried to do at the first time.  But the dev of cpu is
> special, and managed functions do not work as we expect with it.
> 
Inspired by the comment, I think I should attach /cpus/cpu@0 node to
pdev->dev instead of the one retrieved from get_cpu_device(0), so that
both managed functions and dev_* print can work.

Shawn

Re: [PATCH 2/3] cpufreq: add imx6q-cpufreq driver

[Shawn Guo]

On Tue, Jan 08, 2013 at 02:46:31PM +0530, Viresh Kumar wrote:
> Hi Shawn,
> 
> Apart from Sascha's comments, please see few comment from my side:
> 
> On Tue, Jan 8, 2013 at 12:24 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> > diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> > +static struct regulator *arm_reg;
> > +static struct regulator *pu_reg;
> > +static struct regulator *soc_reg;
> 
> Can move to a single line, if you like.
> 
> > +static struct clk *arm_clk;
> > +static struct clk *pll1_sys_clk;
> > +static struct clk *pll1_sw_clk;
> > +static struct clk *step_clk;
> > +static struct clk *pll2_pfd2_396m_clk;
> 
> same.
> 
My personal taste is to have them on separate lines, so that it's
a little easier for reading and editing.

> > +static int imx6q_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, volt_old;
> > +       unsigned int index, cpu;
> > +       int ret;
> > +
> > +       ret = cpufreq_frequency_table_target(policy, freq_table, target_freq,
> > +                                            relation, &index);
> > +       if (ret) {
> > +               pr_err("failed to match target freqency %d: %d\n",
> 
> s/freqency/frequency
> 
Thanks.

> > +                      target_freq, ret);
> > +               return ret;
> > +       }
> > +
> > +       freq_hz = freq_table[index].frequency * 1000;
> > +       freqs.new = freq_hz / 1000;
> 
> could be written as:
> 
>        freqs.new = freq_table[index].frequency;
>        freq_hz = freqs.new * 1000;
> 
Yeah, it looks better.

> 
> > +       freqs.old = clk_get_rate(arm_clk) / 1000;
> > +
> > +       if (freqs.old == freqs.new)
> > +               return 0;
> > +
> > +       for_each_online_cpu(cpu) {
> > +               freqs.cpu = cpu;
> > +               cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > +       }
> > +
> > +       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);
> > +       volt_old = regulator_get_voltage(arm_reg);
> > +
> > +       pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
> > +                freqs.old / 1000, volt_old / 1000,
> > +                freqs.new / 1000, volt / 1000);
> > +
> > +       /* scaling up?  scale voltage before frequency */
> > +       if (freqs.new > freqs.old) {
> > +               ret = regulator_set_voltage_tol(arm_reg, volt, 0);
> > +               if (ret) {
> > +                       pr_err("failed to scale voltage up: %d\n", ret);
> > +                       freqs.new = freqs.old;
> 
> is this useful?
> 
No, it's not.  Will remove it.

> > +                       return ret;
> > +               }
> > +
> > +               /*
> > +                * Need to increase vddpu and vddsoc for safety
> > +                * if we are about to run at 1.2 GHz.
> > +                */
> > +               if (freqs.new == FREQ_1P2_GHZ / 1000) {
> > +                       regulator_set_voltage_tol(pu_reg,
> > +                                       PU_SOC_VOLTAGE_HIGH, 0);
> > +                       regulator_set_voltage_tol(soc_reg,
> > +                                       PU_SOC_VOLTAGE_HIGH, 0);
> > +               }
> > +       }
> > +
> > +       /*
> > +        * The setpoints are selected per PLL/PDF frequencies, so we need to
> > +        * reprogram PLL for frequency scaling.  The procedure of reprogramming
> > +        * PLL1 is as blow.
> > +        *
> > +        *  - Enable pll2_pfd2_396m_clk and reparent pll1_sw_clk to it
> > +        *  - Disable pll1_sys_clk and reprogram it
> > +        *  - Enable pll1_sys_clk and reparent pll1_sw_clk back to it
> > +        *  - Disable pll2_pfd2_396m_clk
> > +        */
> > +       clk_prepare_enable(pll2_pfd2_396m_clk);
> > +       clk_set_parent(step_clk, pll2_pfd2_396m_clk);
> > +       clk_set_parent(pll1_sw_clk, step_clk);
> > +       clk_prepare_enable(pll1_sys_clk);
> > +       if (freq_hz > clk_get_rate(pll2_pfd2_396m_clk)) {
> > +               clk_disable_unprepare(pll1_sys_clk);
> > +               clk_set_rate(pll1_sys_clk, freqs.new * 1000);
> > +               clk_prepare_enable(pll1_sys_clk);
> > +               clk_set_parent(pll1_sw_clk, pll1_sys_clk);
> > +               clk_disable_unprepare(pll2_pfd2_396m_clk);
> > +       } else {
> > +               /*
> > +                * Disable pll1_sys_clk if pll2_pfd2_396m_clk is sufficient
> > +                * to provide the frequency.
> > +                */
> > +               clk_disable_unprepare(pll1_sys_clk);
> 
> Are you doing anything in  clk_[un]prepare ? If not, do clk_[un]prepare
> at init and exit and do enable/disable here.
> 
Yes, we do power on/off PLL in clk_[un]prepare.

> > +       }
> > +
> > +       /* Ensure the arm clock divider is what we expect */
> > +       ret = clk_set_rate(arm_clk, freqs.new * 1000);
> > +       if (ret) {
> > +               pr_err("failed to set clock rate: %d\n", ret);
> > +               regulator_set_voltage_tol(arm_reg, volt_old, 0);
> > +               return ret;
> > +       }
> > +
> > +       /* scaling down?  scale voltage after frequency */
> > +       if (freqs.new < freqs.old) {
> > +               ret = regulator_set_voltage_tol(arm_reg, volt, 0);
> > +               if (ret)
> > +                       pr_warn("failed to scale voltage down: %d\n", ret);
> > +
> > +               if (freqs.old == FREQ_1P2_GHZ / 1000) {
> > +                       regulator_set_voltage_tol(pu_reg,
> > +                                       PU_SOC_VOLTAGE_NORMAL, 0);
> > +                       regulator_set_voltage_tol(soc_reg,
> > +                                       PU_SOC_VOLTAGE_NORMAL, 0);
> > +               }
> > +       }
> > +
> > +       for_each_online_cpu(cpu) {
> > +               freqs.cpu = cpu;
> > +               cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
> > +{
> > +       int ret;
> > +
> > +       if (policy->cpu != 0)
> > +               return -EINVAL;
> 
> Why? The problem here is when you hot-unplug cpu0, init would be called for
> cpu1 and it would fail.
> 

On imx6q, we can never hot-unplug cpu0.  And all the cores of imx6q
share the clock and voltage, so have to scale together.  That's why
we only need to do this init for cpu0.

> > +
> > +       ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> > +       if (ret) {
> > +               pr_err("invalid frequency table: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       policy->cpuinfo.transition_latency = transition_latency;
> > +       policy->cur = clk_get_rate(arm_clk) / 1000;
> > +       policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> > +       cpumask_setall(policy->cpus);
> > +       cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
> > +
> > +       return 0;
> > +}
> > +
> > +static int imx6q_cpufreq_exit(struct cpufreq_policy *policy)
> > +{
> > +       cpufreq_frequency_table_put_attr(policy->cpu);
> > +       return 0;
> > +}
> > +
> > +static struct freq_attr *imx6q_cpufreq_attr[] = {
> > +       &cpufreq_freq_attr_scaling_available_freqs,
> > +       NULL,
> > +};
> > +
> > +static struct cpufreq_driver imx6q_cpufreq_driver = {
> > +       .verify = imx6q_verify_speed,
> > +       .target = imx6q_set_target,
> > +       .get = imx6q_get_speed,
> > +       .init = imx6q_cpufreq_init,
> > +       .exit = imx6q_cpufreq_exit,
> > +       .name = "imx6q-cpufreq",
> > +       .attr = imx6q_cpufreq_attr,
> > +};
> > +
> > +static int imx6q_cpufreq_probe(struct platform_device *pdev)
> > +{
> > +       struct device_node *np;
> > +       int ret;
> > +
> > +       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 put_node;
> > +       }
> > +
> > +       cpu_dev->of_node = np;
> > +
> > +       arm_clk = clk_get(cpu_dev, "arm");
> > +       pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
> > +       pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
> > +       step_clk = clk_get(cpu_dev, "step");
> > +       pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
> > +       if (IS_ERR(arm_clk) || IS_ERR(pll1_sys_clk) || IS_ERR(pll1_sw_clk) ||
> > +           IS_ERR(step_clk) || IS_ERR(pll2_pfd2_396m_clk)) {
> > +               pr_err("failed to get clocks\n");
> > +               ret = -ENOENT;
> > +               goto put_node;
> 
> That looks to be wrong. If clk failed for the last clk_get, you must
> free all other
> clks too.

Yes, you are right.  I just figured out a way to use managed functions,
so it will not be a problem any more.

Shawn

> 
> > +       }
> > +
> > +       arm_reg = regulator_get(cpu_dev, "arm");
> > +       pu_reg = regulator_get(cpu_dev, "pu");
> > +       soc_reg = regulator_get(cpu_dev, "soc");
> > +       if (!arm_reg || !pu_reg || !soc_reg) {
> > +               pr_err("failed to get regulators\n");
> > +               ret = -ENOENT;
> > +               goto put_clk;
> 
> ditto

Re: [PATCH 2/3] cpufreq: add imx6q-cpufreq driver

[Viresh Kumar]

On 8 January 2013 20:05, Shawn Guo <shawn.guo@linaro.org> wrote:
>> > +static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
>> > +{
>> > +       int ret;
>> > +
>> > +       if (policy->cpu != 0)
>> > +               return -EINVAL;
>>
>> Why? The problem here is when you hot-unplug cpu0, init would be called for
>> cpu1 and it would fail.
>>
>
> On imx6q, we can never hot-unplug cpu0.  And all the cores of imx6q
> share the clock and voltage, so have to scale together.  That's why
> we only need to do this init for cpu0.

Ok, in that case too you don't need this code. init() wouldn't be
called for any other
cpu if policy->cpus is set correctly. So, that turns out to be a junk code.

Re: [PATCH 1/3] PM / OPP: Export more symbols for module usage

[Nishanth Menon]

On 14:54-20130108, Shawn Guo wrote:
> Export opp_init_cpufreq_table and opp_free_cpufreq_table for module
> usage.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/base/power/opp.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 50b2831..d16db9a 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -661,6 +661,7 @@ int opp_init_cpufreq_table(struct device *dev,
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(opp_init_cpufreq_table);
>  
>  /**
>   * opp_free_cpufreq_table() - free the cpufreq table
> @@ -678,6 +679,7 @@ void opp_free_cpufreq_table(struct device *dev,
>  	kfree(*table);
>  	*table = NULL;
>  }
> +EXPORT_SYMBOL(opp_free_cpufreq_table);
>  #endif		/* CONFIG_CPU_FREQ */

Is'nt this already covered in 
https://patchwork.kernel.org/patch/1847261/
?
-- 
Regards,
Nishanth Menon

Re: [PATCH 2/3] cpufreq: add imx6q-cpufreq driver

[Shawn Guo]

On Tue, Jan 08, 2013 at 08:11:37PM +0530, Viresh Kumar wrote:
> On 8 January 2013 20:05, Shawn Guo <shawn.guo@linaro.org> wrote:
> >> > +static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
> >> > +{
> >> > +       int ret;
> >> > +
> >> > +       if (policy->cpu != 0)
> >> > +               return -EINVAL;
> >>
> >> Why? The problem here is when you hot-unplug cpu0, init would be called for
> >> cpu1 and it would fail.
> >>
> >
> > On imx6q, we can never hot-unplug cpu0.  And all the cores of imx6q
> > share the clock and voltage, so have to scale together.  That's why
> > we only need to do this init for cpu0.
> 
> Ok, in that case too you don't need this code. init() wouldn't be
> called for any other
> cpu if policy->cpus is set correctly. So, that turns out to be a junk code.

I'm seeing that the .init() will be called on cpu1 when removing the
module.

Shawn

Re: [PATCH 1/3] PM / OPP: Export more symbols for module usage

[Shawn Guo]

On Tue, Jan 08, 2013 at 08:47:21AM -0600, Nishanth Menon wrote:
> On 14:54-20130108, Shawn Guo wrote:
> > Export opp_init_cpufreq_table and opp_free_cpufreq_table for module
> > usage.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  drivers/base/power/opp.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> > index 50b2831..d16db9a 100644
> > --- a/drivers/base/power/opp.c
> > +++ b/drivers/base/power/opp.c
> > @@ -661,6 +661,7 @@ int opp_init_cpufreq_table(struct device *dev,
> >  
> >  	return 0;
> >  }
> > +EXPORT_SYMBOL(opp_init_cpufreq_table);
> >  
> >  /**
> >   * opp_free_cpufreq_table() - free the cpufreq table
> > @@ -678,6 +679,7 @@ void opp_free_cpufreq_table(struct device *dev,
> >  	kfree(*table);
> >  	*table = NULL;
> >  }
> > +EXPORT_SYMBOL(opp_free_cpufreq_table);
> >  #endif		/* CONFIG_CPU_FREQ */
> 
> Is'nt this already covered in 
> https://patchwork.kernel.org/patch/1847261/
> ?

Sorry, I'm working against v3.8-rc and missed that.  Will drop it.

Shawn

Re: [PATCH 2/3] cpufreq: add imx6q-cpufreq driver

[Viresh Kumar]

On 8 January 2013 20:34, Shawn Guo <shawn.guo@linaro.org> wrote:
> I'm seeing that the .init() will be called on cpu1 when removing the
> module.

Hmm... Can you give me sometime to think about how to fix that?
You can go ahead with keeping this change in v2.

Re: [PATCH 2/3] cpufreq: add imx6q-cpufreq driver

[Viresh Kumar]

On 8 January 2013 20:49, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 8 January 2013 20:34, Shawn Guo <shawn.guo@linaro.org> wrote:
>> I'm seeing that the .init() will be called on cpu1 when removing the
>> module.
>
> Hmm... Can you give me sometime to think about how to fix that?
> You can go ahead with keeping this change in v2.

Hi Shawn,

I have sent a fix for this and cc'd you. Can you please give it a try?

cpufreq: Simplify __cpufreq_remove_dev()

patches are present here.

http://git.linaro.org/gitweb?p=arm/big.LITTLE/mp.git;a=shortlog;h=refs/heads/cpufreq-fixes-v2