[PATCH 1/4] clk: imx5: add step and cpu_podf mux

[PATCH 1/4] clk: imx5: add step and cpu_podf mux

[Lucas Stach]

Those two muxes are used when reprogramming PLL1 for
an ARM clock change.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 arch/arm/mach-imx/clk-imx51-imx53.c    | 6 ++++++
 include/dt-bindings/clock/imx5-clock.h | 4 +++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-imx/clk-imx51-imx53.c b/arch/arm/mach-imx/clk-imx51-imx53.c
index 568ef0a4de84..a983b4381331 100644
--- a/arch/arm/mach-imx/clk-imx51-imx53.c
+++ b/arch/arm/mach-imx/clk-imx51-imx53.c
@@ -82,6 +82,8 @@ static const char *mx53_spdif_xtal_sel[] = { "osc", "ckih", "ckih2", "pll4_sw",
 static const char *spdif_sel[] = { "pll1_sw", "pll2_sw", "pll3_sw", "spdif_xtal_sel", };
 static const char *spdif0_com_sel[] = { "spdif0_podf", "ssi1_root_gate", };
 static const char *mx51_spdif1_com_sel[] = { "spdif1_podf", "ssi2_root_gate", };
+static const char *step_sels[] = { "lp_apm", };
+static const char *cpu_podf_sels[] = { "pll1_sw", "step_sel" };
 
 static struct clk *clk[IMX5_CLK_END];
 static struct clk_onecell_data clk_data;
@@ -152,6 +154,10 @@ static void __init mx5_clocks_common_init(unsigned long rate_ckil,
 	clk[IMX5_CLK_USB_PHY_PODF]	= imx_clk_divider("usb_phy_podf", "usb_phy_pred", MXC_CCM_CDCDR, 0, 3);
 	clk[IMX5_CLK_USB_PHY_SEL]	= imx_clk_mux("usb_phy_sel", MXC_CCM_CSCMR1, 26, 1,
 						usb_phy_sel_str, ARRAY_SIZE(usb_phy_sel_str));
+	clk[IMX5_CLK_STEP_SEL]		= imx_clk_mux("step_sel", MXC_CCM_CCSR, 7, 2,
+						step_sels, ARRAY_SIZE(step_sels));
+	clk[IMX5_CLK_CPU_PODF_SEL]	= imx_clk_mux("cpu_podf_sel", MXC_CCM_CCSR, 2, 1,
+						cpu_podf_sels, ARRAY_SIZE(cpu_podf_sels));
 	clk[IMX5_CLK_CPU_PODF]		= imx_clk_divider("cpu_podf", "pll1_sw", MXC_CCM_CACRR, 0, 3);
 	clk[IMX5_CLK_DI_PRED]		= imx_clk_divider("di_pred", "pll3_sw", MXC_CCM_CDCDR, 6, 3);
 	clk[IMX5_CLK_IIM_GATE]		= imx_clk_gate2("iim_gate", "ipg", MXC_CCM_CCGR0, 30);
diff --git a/include/dt-bindings/clock/imx5-clock.h b/include/dt-bindings/clock/imx5-clock.h
index 5f2667ecd98e..1a36ff4ace1e 100644
--- a/include/dt-bindings/clock/imx5-clock.h
+++ b/include/dt-bindings/clock/imx5-clock.h
@@ -198,6 +198,8 @@
 #define IMX5_CLK_OCRAM			186
 #define IMX5_CLK_SAHARA_IPG_GATE	187
 #define IMX5_CLK_SATA_REF		188
-#define IMX5_CLK_END			189
+#define IMX5_CLK_STEP_SEL		189
+#define IMX5_CLK_CPU_PODF_SEL		190
+#define IMX5_CLK_END			191
 
 #endif /* __DT_BINDINGS_CLOCK_IMX5_H */
-- 
2.0.0.rc2

[PATCH 2/4] cpufreq: add i.MX5 cpufreq driver

[Lucas Stach]

SoC specific driver to be able to handle PLL reprogramming
for exact OPP frequencies and additional power saving.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 .../devicetree/bindings/cpufreq/cpufreq-imx5.txt   |  43 +++
 drivers/cpufreq/Kconfig.arm                        |   8 +
 drivers/cpufreq/Makefile                           |   1 +
 drivers/cpufreq/imx5-cpufreq.c                     | 302 +++++++++++++++++++++
 4 files changed, 354 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-imx5.txt
 create mode 100644 drivers/cpufreq/imx5-cpufreq.c

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-imx5.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-imx5.txt
new file mode 100644
index 000000000000..03b14bd2ca59
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-imx5.txt
@@ -0,0 +1,43 @@
+Freescale i.MX5 cpufreq driver
+------------------------------
+
+The imx5-cpufreq driver supports scaling the CPU core clock on Freescale i.MX5
+SoCs by directly changing the CPU PLL frequency. This allows for accurate
+matching of the documented operating points and potentially some additional
+power saving compared to a cpufreq driver using just the CPU frequency
+pre-divider.
+
+Required properties:
+ - operating-points: Refer to Documentation/devicetree/bindings/power/opp.txt
+ - clocks: Must contain an entry for each entry in the clock-names property.
+ - clock-names:
+   - "cpu": the final CPU clock
+   - "pll1": raw, undivided PLL1 output clock
+   - "step": the step clock used as the PLL1 bypass
+   - "lp_apm": the low power clock used as input to the step clock
+ - vddgp-supply: Power supply connected to VddGP aka the CPU core voltage
+ - vddgp-supply-max-microvolt: maximum allowed voltage for the VddGP rail
+ 
+All properties listed above must be defined under node /cpus/cpu@0.
+
+Example:
+--------
+
+cpus {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	cpu0: cpu@0 {
+		clocks = <&clks 24>, <&clks 112>, <&clks 187>, <&clks 89>;
+		clock-names = "cpu", "pll1", "step", "lp_apm";
+		vddgp-supply = <&sw1_reg>;
+		vddgp-supply-max-microvolt = <1400000>;
+		operating-points-range = <
+			/* kHz    uV */
+			 166666  850000
+			 400000  900000
+			 800000 1050000
+			1000000 1200000
+			1200000 1300000
+		>;
+	};
+};
\ No newline at end of file
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 0e9cce82844b..636008e15a51 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -104,6 +104,14 @@ config ARM_HIGHBANK_CPUFREQ
 
 	  If in doubt, say N.
 
+config ARM_IMX5_CPUFREQ
+	tristate "Freescale i.MX5 cpufreq support"
+	depends on SOC_IMX5
+	help
+	  This adds cpufreq driver support for Freescale i.MX5 SOC.
+
+	  If in doubt, say N.
+
 config ARM_IMX6Q_CPUFREQ
 	tristate "Freescale i.MX6 cpufreq support"
 	depends on ARCH_MXC
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 0dbb963c1aef..16d2094fa106 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_ARM_EXYNOS4X12_CPUFREQ)	+= exynos4x12-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS5250_CPUFREQ)	+= exynos5250-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS5440_CPUFREQ)	+= exynos5440-cpufreq.o
 obj-$(CONFIG_ARM_HIGHBANK_CPUFREQ)	+= highbank-cpufreq.o
+obj-$(CONFIG_ARM_IMX5_CPUFREQ)		+= imx5-cpufreq.o
 obj-$(CONFIG_ARM_IMX6Q_CPUFREQ)		+= imx6q-cpufreq.o
 obj-$(CONFIG_ARM_INTEGRATOR)		+= integrator-cpufreq.o
 obj-$(CONFIG_ARM_KIRKWOOD_CPUFREQ)	+= kirkwood-cpufreq.o
diff --git a/drivers/cpufreq/imx5-cpufreq.c b/drivers/cpufreq/imx5-cpufreq.c
new file mode 100644
index 000000000000..668b395ce2bc
--- /dev/null
+++ b/drivers/cpufreq/imx5-cpufreq.c
@@ -0,0 +1,302 @@
+/*
+ * Copyright (c) 2014 Lucas Stach <l.stach@pengutronix.de>, Pengutronix
+ *
+ * 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/platform_device.h>
+#include <linux/pm_opp.h>
+#include <linux/regulator/consumer.h>
+
+struct imx5_cpufreq_private {
+	struct device *dev, *cpu_dev;
+	struct clk *cpu_clk, *pll1_clk, *step_clk, *lp_apm_clk;
+	struct regulator *vddgp_reg;
+	unsigned int max_volt;
+	struct cpufreq_frequency_table *freq_table;
+	unsigned int latency;
+};
+
+/* this should really go away, when the cpufreq driver interface gets sane */
+static struct imx5_cpufreq_private *global_private;
+
+static int imx5_set_target(struct cpufreq_policy *policy, unsigned int index)
+{
+	struct imx5_cpufreq_private *priv = global_private;
+	struct clk *cpu_clk_sel = clk_get_parent(priv->cpu_clk);
+	struct dev_pm_opp *opp;
+	unsigned long freq_hz, volt, volt_old;
+	unsigned int old_freq, new_freq;
+	int ret;
+
+	new_freq = priv->freq_table[index].frequency;
+	freq_hz = new_freq * 1000;
+	old_freq = clk_get_rate(priv->cpu_clk) / 1000;
+
+	rcu_read_lock();
+	opp = dev_pm_opp_find_freq_ceil(priv->cpu_dev, &freq_hz);
+	if (IS_ERR(opp)) {
+		rcu_read_unlock();
+		dev_err(priv->dev, "failed to find OPP for %ld\n", freq_hz);
+		return PTR_ERR(opp);
+	}
+
+	volt = dev_pm_opp_get_voltage(opp);
+	rcu_read_unlock();
+	volt_old = regulator_get_voltage(priv->vddgp_reg);
+
+	dev_dbg(priv->dev, "%4u MHz, %4ld mV --> %4u MHz, %4ld mV\n",
+		old_freq / 1000, volt_old / 1000,
+		new_freq / 1000, volt / 1000);
+
+	/* scaling up? scale voltage before frequency */
+	if (new_freq > old_freq) {
+		ret = regulator_set_voltage(priv->vddgp_reg,
+					    volt, priv->max_volt);
+		if (ret) {
+			dev_err(priv->dev,
+				"failed to scale vddgp up: %d\n", ret);
+			return ret;
+		}
+	}
+
+	/* switch CPU to lp_apm clock */
+	ret = clk_set_parent(priv->step_clk, priv->lp_apm_clk);
+	if (ret)
+		goto out_revert_regulator;
+	ret = clk_set_parent(cpu_clk_sel, priv->step_clk);
+	if (ret)
+		goto out_revert_regulator;
+
+	/* reprogram PLL */
+	ret = clk_set_rate(priv->pll1_clk, new_freq * 1000);
+	if (ret)
+		goto out_revert_pll;
+
+	/* switch back CPU to PLL clock */
+	clk_set_parent(cpu_clk_sel, priv->pll1_clk);
+
+	/* Ensure the arm clock divider is what we expect */
+	clk_set_rate(priv->cpu_clk, new_freq * 1000);
+
+	/* scaling down? scale voltage after frequency */
+	if (new_freq < old_freq) {
+		ret = regulator_set_voltage(priv->vddgp_reg,
+					    volt, priv->max_volt);
+		if (ret) {
+			dev_warn(priv->dev,
+				 "failed to scale vddgp down: %d\n", ret);
+			ret = 0;
+		}
+	}
+
+	return 0;
+
+out_revert_pll:
+	clk_set_rate(priv->pll1_clk, old_freq);
+	clk_set_parent(cpu_clk_sel, priv->pll1_clk);
+out_revert_regulator:
+	regulator_set_voltage_tol(priv->vddgp_reg, volt_old, 0);
+
+	return ret;
+}
+
+static unsigned int imx5_get_speed(unsigned int cpu)
+{
+	struct imx5_cpufreq_private *priv = global_private;
+
+	return clk_get_rate(priv->cpu_clk) / 1000;
+}
+
+static int imx5_cpufreq_init(struct cpufreq_policy *policy)
+{
+	struct imx5_cpufreq_private *priv = global_private;
+
+	return cpufreq_generic_init(policy, priv->freq_table, priv->latency);
+}
+
+static struct cpufreq_driver imx5_cpufreq_driver = {
+	.verify = cpufreq_generic_frequency_table_verify,
+	.target_index = imx5_set_target,
+	.get = imx5_get_speed,
+	.init = imx5_cpufreq_init,
+	.exit = cpufreq_generic_exit,
+	.name = "imx5-cpufreq",
+	.attr = cpufreq_generic_attr,
+};
+
+static int imx5_cpufreq_probe(struct platform_device *pdev)
+{
+	struct device_node *np;
+	struct dev_pm_opp *opp;
+	struct imx5_cpufreq_private *priv;
+	unsigned long opp_freq = 0;
+	unsigned int opp_volt, min_volt = ~0, max_volt = 0;
+	int num_opp, ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &pdev->dev;
+
+	priv->cpu_dev = get_cpu_device(0);
+	if (!priv->cpu_dev) {
+		pr_err("failed to get cpu0 device\n");
+		return -ENODEV;
+	}
+
+	np = of_node_get(priv->cpu_dev->of_node);
+	if (!np) {
+		dev_err(priv->dev, "failed to find cpu0 node\n");
+		return -ENOENT;
+	}
+
+	priv->cpu_clk = clk_get(priv->cpu_dev, "cpu");
+	priv->pll1_clk = clk_get(priv->cpu_dev, "pll1");
+	priv->step_clk = clk_get(priv->cpu_dev, "step");
+	priv->lp_apm_clk = clk_get(priv->cpu_dev, "lp_apm");
+	if (IS_ERR(priv->cpu_clk) || IS_ERR(priv->step_clk) ||
+	    IS_ERR(priv->lp_apm_clk)) {
+		dev_err(priv->dev, "failed to get clocks\n");
+		ret = -ENOENT;
+		goto out_clk_put;
+	}
+
+	priv->vddgp_reg = regulator_get(priv->cpu_dev, "vddgp");
+	if (IS_ERR(priv->vddgp_reg)) {
+		dev_err(priv->dev, "failed to get regulators\n");
+		ret = PTR_ERR(priv->vddgp_reg);
+		goto out_clk_put;
+	}
+
+	ret = of_init_opp_table(priv->cpu_dev);
+	if (ret) {
+		dev_err(priv->dev, "failed to init OPP table: %d\n", ret);
+		goto out_regulator_put;
+	}
+
+	ret = of_property_read_u32(np, "vddgp-supply-max-microvolt",
+				   &priv->max_volt);
+	if (ret) {
+		dev_err(priv->dev, "no max-voltage found\n");
+		goto out_regulator_put;
+	}
+
+	/*
+	 * Disable any OPPs where the connected regulator isn't able to provide
+	 * the specified voltage.
+	 */
+	while (1){
+		rcu_read_lock();
+		opp = dev_pm_opp_find_freq_ceil(priv->cpu_dev, &opp_freq);
+		if (IS_ERR(opp)) {
+			rcu_read_unlock();
+			break;
+		}
+		opp_volt = (unsigned int)dev_pm_opp_get_voltage(opp);
+		rcu_read_unlock();
+
+		if (regulator_is_supported_voltage(priv->vddgp_reg,
+						   opp_volt, priv->max_volt)) {
+			if (opp_volt < min_volt)
+				min_volt = opp_volt;
+			if (opp_volt > max_volt)
+				max_volt = opp_volt;
+		} else {
+			dev_pm_opp_disable(priv->cpu_dev, opp_freq);
+		}
+
+		opp_freq++;
+	}
+
+	/*
+	 * If the number of enabled OPPs has changed, we need to adjust the
+	 * frequency table. Also if there isn't any usable OPP this driver is of
+	 * no use, so just bail in this case.
+	 */
+	num_opp = dev_pm_opp_get_opp_count(priv->cpu_dev);
+	if (!num_opp) {
+		dev_err(priv->dev, "regulator does not support any OPP mandated voltage\n");
+		goto out_regulator_put;
+	}
+
+	ret = dev_pm_opp_init_cpufreq_table(priv->cpu_dev, &priv->freq_table);
+	if (ret) {
+		dev_err(priv->dev, "failed to init cpufreq table: %d\n", ret);
+		goto out_regulator_put;
+	}
+
+	priv->latency = 61036; /* two CLK32 periods to relock PLL */
+	ret = regulator_set_voltage_time(priv->vddgp_reg, min_volt, max_volt);
+	if (ret > 0)
+		priv->latency += ret * 1000;
+
+	platform_set_drvdata(pdev, priv);
+	global_private = priv;
+
+	ret = cpufreq_register_driver(&imx5_cpufreq_driver);
+	if (ret) {
+		dev_err(priv->dev, "failed register driver: %d\n", ret);
+		goto out_free_freq_table;
+	}
+
+	of_node_put(np);
+
+	return 0;
+
+out_free_freq_table:
+	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &priv->freq_table);
+out_regulator_put:
+	regulator_put(priv->vddgp_reg);
+out_clk_put:
+	if (!IS_ERR(priv->cpu_clk))
+		clk_put(priv->cpu_clk);
+	if (!IS_ERR(priv->pll1_clk))
+		clk_put(priv->pll1_clk);
+	if (!IS_ERR(priv->step_clk))
+		clk_put(priv->step_clk);
+	if (!IS_ERR(priv->lp_apm_clk))
+		clk_put(priv->lp_apm_clk);
+
+	of_node_put(np);
+
+	return ret;
+}
+
+static int imx5_cpufreq_remove(struct platform_device *pdev)
+{
+	struct imx5_cpufreq_private *priv = platform_get_drvdata(pdev);
+
+	cpufreq_unregister_driver(&imx5_cpufreq_driver);
+	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &priv->freq_table);
+	regulator_put(priv->vddgp_reg);
+	clk_put(priv->cpu_clk);
+	clk_put(priv->pll1_clk);
+	clk_put(priv->step_clk);
+	clk_put(priv->lp_apm_clk);
+
+	return 0;
+}
+
+static struct platform_driver imx5_cpufreq_drv = {
+	.driver = {
+		.name	= "imx5-cpufreq",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= imx5_cpufreq_probe,
+	.remove		= imx5_cpufreq_remove,
+};
+module_platform_driver(imx5_cpufreq_drv);
+
+MODULE_AUTHOR("Lucas Stach <l.stach@pengutronix.de>");
+MODULE_DESCRIPTION("Freescale i.MX5 cpufreq driver");
+MODULE_LICENSE("GPL v2");
-- 
2.0.0.rc2

Re: [PATCH 2/4] cpufreq: add i.MX5 cpufreq driver

[Viresh Kumar]

On 26 May 2014 15:45, Lucas Stach <l.stach@pengutronix.de> wrote:
> SoC specific driver to be able to handle PLL reprogramming
> for exact OPP frequencies and additional power saving.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  .../devicetree/bindings/cpufreq/cpufreq-imx5.txt   |  43 +++
>  drivers/cpufreq/Kconfig.arm                        |   8 +
>  drivers/cpufreq/Makefile                           |   1 +
>  drivers/cpufreq/imx5-cpufreq.c                     | 302 +++++++++++++++++++++
>  4 files changed, 354 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-imx5.txt
>  create mode 100644 drivers/cpufreq/imx5-cpufreq.c

What stops you to use cpufreq-cpu0 driver with a CPU clock
driver? Its highly discouraged to add a new driver here if you
can reuse it.. Which is more or less confirmed from a broad look..

Re: [PATCH 2/4] cpufreq: add i.MX5 cpufreq driver

[Lucas Stach]

Am Montag, den 26.05.2014, 16:02 +0530 schrieb Viresh Kumar:
> On 26 May 2014 15:45, Lucas Stach <l.stach@pengutronix.de> wrote:
> > SoC specific driver to be able to handle PLL reprogramming
> > for exact OPP frequencies and additional power saving.
> >
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  .../devicetree/bindings/cpufreq/cpufreq-imx5.txt   |  43 +++
> >  drivers/cpufreq/Kconfig.arm                        |   8 +
> >  drivers/cpufreq/Makefile                           |   1 +
> >  drivers/cpufreq/imx5-cpufreq.c                     | 302 +++++++++++++++++++++
> >  4 files changed, 354 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-imx5.txt
> >  create mode 100644 drivers/cpufreq/imx5-cpufreq.c
> 
> What stops you to use cpufreq-cpu0 driver with a CPU clock
> driver? Its highly discouraged to add a new driver here if you
> can reuse it.. Which is more or less confirmed from a broad look..

This driver handles the i.MX5 specific clock reparenting to be able to
reprogramm the PLL. cpufreq-cpu0 can only change a postdivider of the
PLL, which means we can't reach the exact OPP frequencies and can not
profit from the additional power savings of a slower running PLL.

Also without reprogramming the PLL we could possibly not scale to the
highest OPPs if the bootloader left the PLL at a slower rate. This is
the case for many i.MX53 boards, as the CPU regulator normally does
start up with the voltage required for 800MHz operation. If the
bootloader decides to not touch the regulator and set the PLL to 800MHz
the 1GHz and 1.2GHz operating points won't be useable.

Regards,
Lucas
-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 2/4] cpufreq: add i.MX5 cpufreq driver

[Viresh Kumar]

On 26 May 2014 16:15, Lucas Stach <l.stach@pengutronix.de> wrote:
> This driver handles the i.MX5 specific clock reparenting to be able to
> reprogramm the PLL. cpufreq-cpu0 can only change a postdivider of the
> PLL, which means we can't reach the exact OPP frequencies and can not
> profit from the additional power savings of a slower running PLL.
>
> Also without reprogramming the PLL we could possibly not scale to the
> highest OPPs if the bootloader left the PLL at a slower rate. This is
> the case for many i.MX53 boards, as the CPU regulator normally does
> start up with the voltage required for 800MHz operation. If the
> bootloader decides to not touch the regulator and set the PLL to 800MHz
> the 1GHz and 1.2GHz operating points won't be useable.

Almost all users of cpufreq-cpu0 has such situations and they have
written a clock-driver for "cpu clock" to get this fixed. And they handle
all this PLL stuff in clk_set_rate() for this new clock..

I still couldn't find anything so special here that can't be fixed this way.
There is no point adding and maintaining a cpufreq driver if we can
use cpufreq-cpu0.

Re: [PATCH 2/4] cpufreq: add i.MX5 cpufreq driver

[Lucas Stach]

Am Montag, den 26.05.2014, 16:36 +0530 schrieb Viresh Kumar:
> On 26 May 2014 16:15, Lucas Stach <l.stach@pengutronix.de> wrote:
> > This driver handles the i.MX5 specific clock reparenting to be able to
> > reprogramm the PLL. cpufreq-cpu0 can only change a postdivider of the
> > PLL, which means we can't reach the exact OPP frequencies and can not
> > profit from the additional power savings of a slower running PLL.
> >
> > Also without reprogramming the PLL we could possibly not scale to the
> > highest OPPs if the bootloader left the PLL at a slower rate. This is
> > the case for many i.MX53 boards, as the CPU regulator normally does
> > start up with the voltage required for 800MHz operation. If the
> > bootloader decides to not touch the regulator and set the PLL to 800MHz
> > the 1GHz and 1.2GHz operating points won't be useable.
> 
> Almost all users of cpufreq-cpu0 has such situations and they have
> written a clock-driver for "cpu clock" to get this fixed. And they handle
> all this PLL stuff in clk_set_rate() for this new clock..
> 
> I still couldn't find anything so special here that can't be fixed this way.
> There is no point adding and maintaining a cpufreq driver if we can
> use cpufreq-cpu0.

Ok I can push down the clock handling into the imx5 clk driver.

This leaves two more differences between imx5-cpufreq and cpufreq-cpu0:

1. Disabling of OPPs that could not be supported by the connected
regulator. I think this part can easily be made generic and pushed into
cpu0, as it should be beneficial to other SoCs too.

2. Usage of a fixed max voltage, rather than a voltage-tolerance. i.MX5
has a fixed maximum voltage, which is valid across all operating points.
I'm really opposed to using a tolerance value, where we instead could
have a well defined range. So for merging imx5-cpufreq into cpufreq-cpu0
this driver really need to handle this case.
Does adding a separate property for this, while keeping the
voltage-tolerance for the existing users, sound like a change you would
accept?

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 2/4] cpufreq: add i.MX5 cpufreq driver

[Viresh Kumar]

On 26 May 2014 18:05, Lucas Stach <l.stach@pengutronix.de> wrote:
> Ok I can push down the clock handling into the imx5 clk driver.

Great !!

> This leaves two more differences between imx5-cpufreq and cpufreq-cpu0:
>
> 1. Disabling of OPPs that could not be supported by the connected
> regulator. I think this part can easily be made generic and pushed into
> cpu0, as it should be beneficial to other SoCs too.

Sure.

> 2. Usage of a fixed max voltage, rather than a voltage-tolerance. i.MX5
> has a fixed maximum voltage, which is valid across all operating points.
> I'm really opposed to using a tolerance value, where we instead could
> have a well defined range. So for merging imx5-cpufreq into cpufreq-cpu0
> this driver really need to handle this case.
> Does adding a separate property for this, while keeping the
> voltage-tolerance for the existing users, sound like a change you would
> accept?

It looks you can just send voltage-tolerance to "zero" and this should work
for you.. No need of another field.

Thanks a lot for making our (/me & Rafael) life easier :)

Re: [PATCH 2/4] cpufreq: add i.MX5 cpufreq driver

[Lucas Stach]

Am Montag, den 26.05.2014, 18:26 +0530 schrieb Viresh Kumar:
> On 26 May 2014 18:05, Lucas Stach <l.stach@pengutronix.de> wrote:
[...]
> 
> > 2. Usage of a fixed max voltage, rather than a voltage-tolerance. i.MX5
> > has a fixed maximum voltage, which is valid across all operating points.
> > I'm really opposed to using a tolerance value, where we instead could
> > have a well defined range. So for merging imx5-cpufreq into cpufreq-cpu0
> > this driver really need to handle this case.
> > Does adding a separate property for this, while keeping the
> > voltage-tolerance for the existing users, sound like a change you would
> > accept?
> 
> It looks you can just send voltage-tolerance to "zero" and this should work
> for you.. No need of another field.
> 
No, setting voltage-tolerance to zero means I don't accept any tolerance
at all. Though in practice I have a variable tolerance as I just have a
fixed maximum voltage for the chip.

For example the i.MX53 has a fixed maximum voltage of 1.4V on the cpu
rail, so for the slowest OPP the acceptable voltage range is 0.8V to
1.4V, for the fastest OPP the range is 1.3V to 1.4V.

So if someone connects a fixed 1.3V regulator, I want to still be able
to use all OPPs, even though the slowest OPP wants 0.8V ideally. If I
were to use a tolerance I would have to set is to some relatively strict
value, in order to not exceed the maximum voltage at the highest OPP,
but this would mean I could not use the slowest OPP because it's out of
the tolerance range.

Regards,
Lucas
-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 2/4] cpufreq: add i.MX5 cpufreq driver

[Viresh Kumar]

On 26 May 2014 18:41, Lucas Stach <l.stach@pengutronix.de> wrote:

Just to mention that I am trying to help you and am not opposing any
change. We can add this field if its really required and I am just trying
to understand your problem and if we can solve it with current code.

> No, setting voltage-tolerance to zero means I don't accept any tolerance
> at all. Though in practice I have a variable tolerance as I just have a
> fixed maximum voltage for the chip.
>
> For example the i.MX53 has a fixed maximum voltage of 1.4V on the cpu
> rail, so for the slowest OPP the acceptable voltage range is 0.8V to
> 1.4V, for the fastest OPP the range is 1.3V to 1.4V.
>
> So if someone connects a fixed 1.3V regulator, I want to still be able
> to use all OPPs, even though the slowest OPP wants 0.8V ideally. If I
> were to use a tolerance I would have to set is to some relatively strict
> value, in order to not exceed the maximum voltage at the highest OPP,
> but this would mean I could not use the slowest OPP because it's out of
> the tolerance range.

I understood the problem now. But I feel there is nothing imx specific here.
It should be a general concern and I want to know how are people
working around it currently.

IOW can we say that you want tolerance to work only on the positive
side? But no tolerance on the -ve side ?

Also, your example dts says that you do have different voltage levels
for each frequency but actually the regulator may only support a
single voltage. i.e. 1.4 volts.

--
viresh

Re: [PATCH 2/4] cpufreq: add i.MX5 cpufreq driver

[Lucas Stach]

Am Montag, den 26.05.2014, 19:14 +0530 schrieb Viresh Kumar:
> On 26 May 2014 18:41, Lucas Stach <l.stach@pengutronix.de> wrote:
> 
> Just to mention that I am trying to help you and am not opposing any
> change. We can add this field if its really required and I am just trying
> to understand your problem and if we can solve it with current code.
> 
I understand this and I think we are having a productive conversation
here. I just wanted to make it clear that the currently existing
voltage-tolerance property isn't sufficient for the use-cases I have in
mind.

> > No, setting voltage-tolerance to zero means I don't accept any tolerance
> > at all. Though in practice I have a variable tolerance as I just have a
> > fixed maximum voltage for the chip.
> >
> > For example the i.MX53 has a fixed maximum voltage of 1.4V on the cpu
> > rail, so for the slowest OPP the acceptable voltage range is 0.8V to
> > 1.4V, for the fastest OPP the range is 1.3V to 1.4V.
> >
> > So if someone connects a fixed 1.3V regulator, I want to still be able
> > to use all OPPs, even though the slowest OPP wants 0.8V ideally. If I
> > were to use a tolerance I would have to set is to some relatively strict
> > value, in order to not exceed the maximum voltage at the highest OPP,
> > but this would mean I could not use the slowest OPP because it's out of
> > the tolerance range.
> 
> I understood the problem now. But I feel there is nothing imx specific here.
> It should be a general concern and I want to know how are people
> working around it currently.
> 
I think the voltage-tolerance property is ok for SoCs where we have a
strictly defined voltage level for one OPP and where the tolerance is
used to account for the fact that most regulators can only closely, but
not exactly, match the OPP defined voltage.

> IOW can we say that you want tolerance to work only on the positive
> side? But no tolerance on the -ve side ?
> 
Right, the OPP defined voltages are minimum voltages for the OPP
frequency, so we should not undercut them.

> Also, your example dts says that you do have different voltage levels
> for each frequency but actually the regulator may only support a
> single voltage. i.e. 1.4 volts.
> 
Right, the OPP in my example define the minimum required voltage for
each frequency. This is in accordance to the OPP binding. But for this
chip the datasheet explicitly says that it is ok to power the cpu rail
with up to 1.4V, regardless of the current operating frequency. So this
1.4V is really the upper bound I would like to pass to the regulator
framework. This ensures that the driver still works properly even if the
external regulator can't scale down to the minimum (or in other words
optimal) voltage.

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 2/4] cpufreq: add i.MX5 cpufreq driver

[Viresh Kumar]

On 26 May 2014 19:28, Lucas Stach <l.stach@pengutronix.de> wrote:
> Right, the OPP in my example define the minimum required voltage for
> each frequency. This is in accordance to the OPP binding. But for this
> chip the datasheet explicitly says that it is ok to power the cpu rail
> with up to 1.4V, regardless of the current operating frequency. So this
> 1.4V is really the upper bound I would like to pass to the regulator
> framework. This ensures that the driver still works properly even if the
> external regulator can't scale down to the minimum (or in other words
> optimal) voltage.

One more query. You have mentioned earlier and I just wanted to confirm
this. In your platform/board regulator only supports one voltage, i.e. 1.4 uV,
and it we can't get lower values out of regulator ?

If that's the case, why are you looking to use regulator at all?

Re: [PATCH 2/4] cpufreq: add i.MX5 cpufreq driver

[Lucas Stach]

Am Montag, den 26.05.2014, 20:52 +0530 schrieb Viresh Kumar:
> On 26 May 2014 19:28, Lucas Stach <l.stach@pengutronix.de> wrote:
> > Right, the OPP in my example define the minimum required voltage for
> > each frequency. This is in accordance to the OPP binding. But for this
> > chip the datasheet explicitly says that it is ok to power the cpu rail
> > with up to 1.4V, regardless of the current operating frequency. So this
> > 1.4V is really the upper bound I would like to pass to the regulator
> > framework. This ensures that the driver still works properly even if the
> > external regulator can't scale down to the minimum (or in other words
> > optimal) voltage.
> 
> One more query. You have mentioned earlier and I just wanted to confirm
> this. In your platform/board regulator only supports one voltage, i.e. 1.4 uV,
> and it we can't get lower values out of regulator ?
> 
> If that's the case, why are you looking to use regulator at all?

The platform I'm working on supports cpu rail voltage regulation just
fine.
I just want to make sure that the driver works properly on platforms
where this isn't the case. As the datasheet explicitly allows this case
I want to make sure to not introduce a software only restriction, where
there isn't any in hardware.

Regards,
Lucas
-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 2/4] cpufreq: add i.MX5 cpufreq driver

[Viresh Kumar]

On 26 May 2014 20:58, Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> The platform I'm working on supports cpu rail voltage regulation just
> fine.

So, the regulator can be scaled to support all values from your dts
and we can work without MAX-volt parameter here..

> I just want to make sure that the driver works properly on platforms
> where this isn't the case.

Can we just take care of this simply by not adding any regulators at
all? In that case cpufreq-cpu0 will try working without a regulator..
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] cpufreq: add i.MX5 cpufreq driver

[Lucas Stach]

Am Montag, den 26.05.2014, 21:05 +0530 schrieb Viresh Kumar:
> On 26 May 2014 20:58, Lucas Stach <l.stach@pengutronix.de> wrote:
> > The platform I'm working on supports cpu rail voltage regulation just
> > fine.
> 
> So, the regulator can be scaled to support all values from your dts
> and we can work without MAX-volt parameter here..

The max-volt parameter is still useful to determine the acceptable
tolerance. It defines that max voltage one would pass to the regulator
framework, without the need to "compute" the tolerance in any way, as we
already have a datasheet defined max value.

> 
> > I just want to make sure that the driver works properly on platforms
> > where this isn't the case.
> 
> Can we just take care of this simply by not adding any regulators at
> all? In that case cpufreq-cpu0 will try working without a regulator..

No, we need the regulator to determine which OPPs are actually useable.
If the regulator is only supplying 1.1V we can only scale to 800MHz max,
which is the case for some of the industrial i.MX53 versions.

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 2/4] cpufreq: add i.MX5 cpufreq driver

[Viresh Kumar]

On 26 May 2014 21:27, Lucas Stach <l.stach@pengutronix.de> wrote:
> No, we need the regulator to determine which OPPs are actually useable.
> If the regulator is only supplying 1.1V we can only scale to 800MHz max,
> which is the case for some of the industrial i.MX53 versions.

Go and add another binding for this max-volt.. And lets see how others
respond.

You can give a link to this discussion as others might find this very helpful.

[PATCH 3/4] ARM: imx53: instanciate cpufreq device

[Lucas Stach]

Instanciate the platform device for the i.MX5 specific
cpufreq driver.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 arch/arm/mach-imx/mach-imx53.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/mach-imx/mach-imx53.c b/arch/arm/mach-imx/mach-imx53.c
index 65850908a4b4..ca90967c10b7 100644
--- a/arch/arm/mach-imx/mach-imx53.c
+++ b/arch/arm/mach-imx/mach-imx53.c
@@ -26,9 +26,12 @@
 
 static void __init imx53_dt_init(void)
 {
+	struct platform_device_info devinfo = { .name = "imx5-cpufreq", };
+
 	mxc_arch_reset_init_dt();
 
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
+	platform_device_register_full(&devinfo);
 }
 
 static const char *imx53_dt_board_compat[] __initconst = {
-- 
2.0.0.rc2

[PATCH 4/4] ARM: imx53: add basic cpufreq properties to dtsi

[Lucas Stach]

Only thing missing is the vddgp regulator, which is board
specific.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 arch/arm/boot/dts/imx53.dtsi | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
index b57ab57740f6..8f005919cf6c 100644
--- a/arch/arm/boot/dts/imx53.dtsi
+++ b/arch/arm/boot/dts/imx53.dtsi
@@ -45,10 +45,24 @@
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
-		cpu@0 {
+		cpu0: cpu@0 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a8";
 			reg = <0x0>;
+			clocks = <&clks IMX5_CLK_CPU_PODF>,
+			         <&clks IMX5_CLK_PLL1_SW>,
+			         <&clks IMX5_CLK_STEP_SEL>,
+			         <&clks IMX5_CLK_LP_APM>;
+			clock-names = "cpu", "pll1", "step", "lp_apm";
+			vddgp-supply-max-microvolt = <1400000>;
+			operating-points = <
+				/* kHz     uV */
+				 166666  850000
+				 400000  900000
+				 800000 1050000
+				1000000 1200000
+				1200000 1300000
+			>;
 		};
 	};
 
-- 
2.0.0.rc2