[PATCH v2 0/2] Add imx6q-cpufreq driver support

[PATCH v2 0/2] Add imx6q-cpufreq driver support

[Shawn Guo]

Changes since v1:
 - Drop patch "PM / OPP: Export more symbols for module usage", as
   there is already one "[PATCH 5/6 v9] power: export opp cpufreq
   functions" from Mark Langsdorf.
 - Instead of having cpu_dev be the struct device retrieved from
   get_cpu_device(), have it be &pdev->dev, so that we can use managed
   functions to simplified the cleanup path.
 - Use dev_* rather than pr_* for message output
 - Fix Typos and others commented by Viresh and Sascha. 

Once the first patch gets accepted, I will apply the second one for
going through arm-soc tree.

Shawn Guo (2):
  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  |   65 +++++++++
 drivers/cpufreq/Kconfig.arm     |    9 ++
 drivers/cpufreq/Makefile        |    1 +
 drivers/cpufreq/imx6q-cpufreq.c |  296 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 384 insertions(+), 6 deletions(-)
 create mode 100644 drivers/cpufreq/imx6q-cpufreq.c

-- 
1.7.9.5

[PATCH v2 1/2] 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 |  296 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 306 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..218d8f1
--- /dev/null
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -0,0 +1,296 @@
+/*
+ * 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.
+ */
+
+#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) {
+		dev_err(cpu_dev, "failed to match target frequency %d: %d\n",
+			target_freq, ret);
+		return ret;
+	}
+
+	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)) {
+		dev_err(cpu_dev, "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);
+
+	dev_dbg(cpu_dev, "%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) {
+			dev_err(cpu_dev, "failed to scale voltage up: %d\n", ret);
+			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 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
+	 */
+	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) {
+		dev_err(cpu_dev, "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)
+			dev_warn(cpu_dev, "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;
+
+	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
+	if (ret) {
+		dev_err(cpu_dev, "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;
+
+	cpu_dev = &pdev->dev;
+
+	np = of_find_node_by_path("/cpus/cpu at 0");
+	if (!np) {
+		dev_err(cpu_dev, "failed to find cpu0 node\n");
+		return -ENOENT;
+	}
+
+	cpu_dev->of_node = np;
+
+	arm_clk = devm_clk_get(cpu_dev, "arm");
+	pll1_sys_clk = devm_clk_get(cpu_dev, "pll1_sys");
+	pll1_sw_clk = devm_clk_get(cpu_dev, "pll1_sw");
+	step_clk = devm_clk_get(cpu_dev, "step");
+	pll2_pfd2_396m_clk = devm_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)) {
+		dev_err(cpu_dev, "failed to get clocks\n");
+		ret = -ENOENT;
+		goto put_node;
+	}
+
+	arm_reg = devm_regulator_get(cpu_dev, "arm");
+	pu_reg = devm_regulator_get(cpu_dev, "pu");
+	soc_reg = devm_regulator_get(cpu_dev, "soc");
+	if (!arm_reg || !pu_reg || !soc_reg) {
+		dev_err(cpu_dev, "failed to get regulators\n");
+		ret = -ENOENT;
+		goto put_node;
+	}
+
+	/* We expect an OPP table supplied by platform */
+	ret = opp_get_opp_count(cpu_dev);
+	if (ret < 0) {
+		dev_err(cpu_dev, "no OPP table is found: %d\n", ret);
+		goto put_node;
+	}
+
+	ret = opp_init_cpufreq_table(cpu_dev, &freq_table);
+	if (ret) {
+		dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
+		goto put_node;
+	}
+
+	if (of_property_read_u32(np, "clock-latency", &transition_latency))
+		transition_latency = CPUFREQ_ETERNAL;
+
+	ret = cpufreq_register_driver(&imx6q_cpufreq_driver);
+	if (ret) {
+		dev_err(cpu_dev, "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_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);
+
+	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 v2 1/2] cpufreq: add imx6q-cpufreq driver

[Viresh Kumar]

On 10 January 2013 14:04, Shawn Guo <shawn.guo@linaro.org> wrote:
> Add an imx6q-cpufreq for Freescale i.MX6Q SoC to handle the hardware

                                       ^
                                   driver

> 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 |  296 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 306 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..218d8f1
> --- /dev/null
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -0,0 +1,296 @@
> +/*
> + * 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.
> + */
> +
> +#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);

@Rafael: Why is this function used in our target routines? Isn't the caller
supposed to send a valid freq from freq_table? All governors do it.

> +       if (ret) {
> +               dev_err(cpu_dev, "failed to match target frequency %d: %d\n",
> +                       target_freq, ret);
> +               return ret;
> +       }
> +
> +       freqs.new = freq_table[index].frequency;
> +       freq_hz = freqs.new * 1000;
> +       freqs.old = clk_get_rate(arm_clk) / 1000;

clk_get_rate() can fail.

> +
> +       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)) {
> +               dev_err(cpu_dev, "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);
> +
> +       dev_dbg(cpu_dev, "%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) {
> +                       dev_err(cpu_dev, "failed to scale voltage up: %d\n", ret);
> +                       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 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
> +        */
> +       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);

all these fns can fail too.. don't want to check return values?

> +       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) {
> +               dev_err(cpu_dev, "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)
> +                       dev_warn(cpu_dev, "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;
> +

So you finally removed the ugly check :)

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

[PATCH v2 1/2] cpufreq: add imx6q-cpufreq driver

[Shawn Guo]

On Thu, Jan 10, 2013 at 02:15:44PM +0530, Viresh Kumar wrote:
...
> > +       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);
> 
> all these fns can fail too.. don't want to check return values?
> 
I initially had every single clk and regulator call return checked
and error handled properly.  But the code becomes very messy with
the error messages and recovering logic all over the places.  I think
it's a bit over engineering, and made a compromise to only check limited
ones to ensure the correctness at subsystem level.

> > +static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
> > +{
> > +       int ret;
> > +
> 
> So you finally removed the ugly check :)
> 
I just found though it's less optimal, removing the check does not
really cause any problem for me.

> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

Thanks.

Shawn

[PATCH v2 2/2] 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 |   65 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 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 at 1 {
@@ -471,7 +477,7 @@
 					anatop-max-voltage = <2750000>;
 				};
 
-				reg_cpu: regulator-vddcore at 140 {
+				reg_arm: regulator-vddcore at 140 {
 					compatible = "fsl,anatop-regulator";
 					regulator-name = "cpu";
 					regulator-min-microvolt = <725000>;
@@ -485,7 +491,7 @@
 					anatop-max-voltage = <1450000>;
 				};
 
-				regulator-vddpu at 140 {
+				reg_pu: regulator-vddpu at 140 {
 					compatible = "fsl,anatop-regulator";
 					regulator-name = "vddpu";
 					regulator-min-microvolt = <725000>;
@@ -499,7 +505,7 @@
 					anatop-max-voltage = <1450000>;
 				};
 
-				regulator-vddsoc at 140 {
+				reg_soc: regulator-vddsoc at 140 {
 					compatible = "fsl,anatop-regulator";
 					regulator-name = "vddsoc";
 					regulator-min-microvolt = <725000>;
@@ -965,6 +971,7 @@
 			};
 
 			ocotp at 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..16f9a13 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,72 @@ 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");
+	if (!np) {
+		pr_warn("failed to find ocotp node\n");
+		return;
+	}
+
+	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(struct device *cpu_dev)
+{
+	struct device_node *np;
+
+	np = of_find_node_by_path("/cpus/cpu at 0");
+	if (!np) {
+		pr_warn("failed to find cpu0 node\n");
+		return;
+	}
+
+	cpu_dev->of_node = np;
+	if (of_init_opp_table(cpu_dev)) {
+		pr_warn("failed to init OPP table\n");
+		goto put_node;
+	}
+
+	imx6q_opp_check_1p2ghz(cpu_dev);
+
+put_node:
+	of_node_put(np);
+}
+
+struct platform_device imx6q_cpufreq_pdev = {
+	.name = "imx6q-cpufreq",
+};
+
 static void __init imx6q_init_late(void)
 {
 	imx_cpuidle_init(&imx6q_cpuidle_driver);
+
+	if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ)) {
+		imx6q_opp_init(&imx6q_cpufreq_pdev.dev);
+		platform_device_register(&imx6q_cpufreq_pdev);
+	}
 }
 
 static void __init imx6q_map_io(void)
-- 
1.7.9.5

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

[Bedia, Vaibhav]

Hi,

On Thu, Jan 10, 2013 at 14:04:23, Shawn Guo wrote:
> 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 |   65 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 78 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
>  			>;

[...]

>  
> +#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");
> +	if (!np) {
> +		pr_warn("failed to find ocotp node\n");
> +		return;
> +	}
> +
> +	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");
> +

I just happened to be thinking about the problem of enabling additional OPPs based on
some Si rev info for TI's AM335x. 

The other approach could be to register additional OPPs and mark then as disabled to begin with.
A platform specific hook could then be used to selectively enable the OPPs for based on the Si rev
info.

If this approach was already discussed sorry for the noise.

Regards,
Vaibhav 

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

[Shawn Guo]

On Thu, Jan 10, 2013 at 10:50:25AM +0000, Bedia, Vaibhav wrote:
...
> > +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");
> > +	if (!np) {
> > +		pr_warn("failed to find ocotp node\n");
> > +		return;
> > +	}
> > +
> > +	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");
> > +
> 
> I just happened to be thinking about the problem of enabling additional OPPs based on
> some Si rev info for TI's AM335x. 
> 
> The other approach could be to register additional OPPs and mark then as disabled to begin with.
> A platform specific hook could then be used to selectively enable the OPPs for based on the Si rev
> info.

May I ask the advantage of this approach?  I hate to create
platform_data merely for passing a platform specific hook to driver.

Shawn

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

[Bedia, Vaibhav]

On Thu, Jan 10, 2013 at 16:37:45, Shawn Guo wrote:
> On Thu, Jan 10, 2013 at 10:50:25AM +0000, Bedia, Vaibhav wrote:
> ...
> > > +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");
> > > +	if (!np) {
> > > +		pr_warn("failed to find ocotp node\n");
> > > +		return;
> > > +	}
> > > +
> > > +	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");
> > > +
> > 
> > I just happened to be thinking about the problem of enabling additional OPPs based on
> > some Si rev info for TI's AM335x. 
> > 
> > The other approach could be to register additional OPPs and mark then as disabled to begin with.
> > A platform specific hook could then be used to selectively enable the OPPs for based on the Si rev
> > info.
> 
> May I ask the advantage of this approach?  I hate to create
> platform_data merely for passing a platform specific hook to driver.
> 

In the current approach the OPP data is split across DT and kernel code. If you take the
other approach all OPP entries can reside in DT and for someone just looking at that file
there's no confusion about what the kernel could potentially support. Whether a particular
an OPP should be supported is best decided at runtime.

Also, IMHO letting platforms invoke opp_add() is open to abuse and platforms should be
restricted to invoking opp_enable/disable to indicate what's possible on a particular Si rev. 

Regards,
Vaibhav

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

[Shawn Guo]

On Thu, Jan 10, 2013 at 01:02:30PM +0000, Bedia, Vaibhav wrote:
> In the current approach the OPP data is split across DT and kernel code. If you take the
> other approach all OPP entries can reside in DT and for someone just looking at that file
> there's no confusion about what the kernel could potentially support. Whether a particular
> an OPP should be supported is best decided at runtime.
> 
Listing the OPP that some Si rev can not support in DT is also
a confusion to people who is just looking at DTS.  To me, the approach
is not really doing anything better on this aspect.

> Also, IMHO letting platforms invoke opp_add() is open to abuse and platforms should be
> restricted to invoking opp_enable/disable to indicate what's possible on a particular Si rev. 
> 
I do not see anything wrong with platform adding the OPP it can support.
Isn't omap_init_opp_table() being called by platform code?  I do not see
omap-cpufreq driver is calling a platform hook to do that.

Sorry, I'm not really a fan of platform hook, and I have spent past
couple years to minimize the use of it when converting IMX family to
device tree.

Shawn

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

[Bedia, Vaibhav]

On Thu, Jan 10, 2013 at 18:50:55, Shawn Guo wrote:
> On Thu, Jan 10, 2013 at 01:02:30PM +0000, Bedia, Vaibhav wrote:
> > In the current approach the OPP data is split across DT and kernel code. If you take the
> > other approach all OPP entries can reside in DT and for someone just looking at that file
> > there's no confusion about what the kernel could potentially support. Whether a particular
> > an OPP should be supported is best decided at runtime.
> > 
> Listing the OPP that some Si rev can not support in DT is also
> a confusion to people who is just looking at DTS.  To me, the approach
> is not really doing anything better on this aspect.
> 

I still think putting the OPP data in a single place is better.

> > Also, IMHO letting platforms invoke opp_add() is open to abuse and platforms should be
> > restricted to invoking opp_enable/disable to indicate what's possible on a particular Si rev. 
> > 
> I do not see anything wrong with platform adding the OPP it can support.
> Isn't omap_init_opp_table() being called by platform code?  I do not see
> omap-cpufreq driver is calling a platform hook to do that.
> 
> Sorry, I'm not really a fan of platform hook, and I have spent past
> couple years to minimize the use of it when converting IMX family to
> device tree.
> 

I don't know what the OMAP plans are but with DT + generic driver adoption I would expect most
of it to go away. And adding the platform hook with the right DT data is just preparing
things for the future with 2 potential users right now.

If OPP data across Si rev starts looking very different all entries can be placed in a single
place and the platform code enables what are relevant. Unless there's a Si rev specific blob a bunch
of opp_add() calls ignoring what was passed defeats the purpose of moving the information out of the
kernel.
 
Regards,
Vaibhav

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

[Shawn Guo]

On Thu, Jan 10, 2013 at 02:02:53PM +0000, Bedia, Vaibhav wrote:
> On Thu, Jan 10, 2013 at 18:50:55, Shawn Guo wrote:
> > On Thu, Jan 10, 2013 at 01:02:30PM +0000, Bedia, Vaibhav wrote:
> > > In the current approach the OPP data is split across DT and kernel code. If you take the
> > > other approach all OPP entries can reside in DT and for someone just looking at that file
> > > there's no confusion about what the kernel could potentially support. Whether a particular
> > > an OPP should be supported is best decided at runtime.
> > > 
> > Listing the OPP that some Si rev can not support in DT is also
> > a confusion to people who is just looking at DTS.  To me, the approach
> > is not really doing anything better on this aspect.
> > 
> 
> I still think putting the OPP data in a single place is better.
> 
Okay, I agree with you on that and plan to commit the following changes.
But no, still no platform hook.

Shawn

diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 17c5618..231a32c 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -38,6 +38,7 @@
                        next-level-cache = <&L2>;
                        operating-points = <
                                /* kHz    uV */
+                               1200000 1275000
                                996000  1250000
                                792000  1150000
                                396000  950000
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 16f9a13..2f974f5 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -235,9 +235,9 @@ static void __init imx6q_opp_check_1p2ghz(struct device *cpu_dev)

        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");
+       if ((val & 0x3) != OCOTP_CFG3_SPEED_1P2GHZ)
+               if (opp_disable(cpu_dev, 1200000000))
+                       pr_warn("failed to disable 1.2 GHz OPP\n");

 put_node:
        of_node_put(np);

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

[Bedia, Vaibhav]

On Thu, Jan 10, 2013 at 20:11:25, Shawn Guo wrote:
> On Thu, Jan 10, 2013 at 02:02:53PM +0000, Bedia, Vaibhav wrote:
> > On Thu, Jan 10, 2013 at 18:50:55, Shawn Guo wrote:
> > > On Thu, Jan 10, 2013 at 01:02:30PM +0000, Bedia, Vaibhav wrote:
> > > > In the current approach the OPP data is split across DT and kernel code. If you take the
> > > > other approach all OPP entries can reside in DT and for someone just looking at that file
> > > > there's no confusion about what the kernel could potentially support. Whether a particular
> > > > an OPP should be supported is best decided at runtime.
> > > > 
> > > Listing the OPP that some Si rev can not support in DT is also
> > > a confusion to people who is just looking at DTS.  To me, the approach
> > > is not really doing anything better on this aspect.
> > > 
> > 
> > I still think putting the OPP data in a single place is better.
> > 
> Okay, I agree with you on that and plan to commit the following changes.
> But no, still no platform hook.
> 

Fair enough.

Regards,
Vaibhav

[PATCH v2 0/2] Add imx6q-cpufreq driver support

[Rafael J. Wysocki]

On Thursday, January 10, 2013 04:34:21 PM Shawn Guo wrote:
> Changes since v1:
>  - Drop patch "PM / OPP: Export more symbols for module usage", as
>    there is already one "[PATCH 5/6 v9] power: export opp cpufreq
>    functions" from Mark Langsdorf.
>  - Instead of having cpu_dev be the struct device retrieved from
>    get_cpu_device(), have it be &pdev->dev, so that we can use managed
>    functions to simplified the cleanup path.
>  - Use dev_* rather than pr_* for message output
>  - Fix Typos and others commented by Viresh and Sascha. 
> 
> Once the first patch gets accepted, I will apply the second one for
> going through arm-soc tree.
> 
> Shawn Guo (2):
>   cpufreq: add imx6q-cpufreq driver
>   ARM: imx: enable imx6q-cpufreq support

I would prefer these two to go through the arm-soc tree if that's not a problem.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.