[PATCH v2] cpufreq: i.MX6: -EPROBE_DEFER fixes

[PATCH v2] cpufreq: i.MX6: -EPROBE_DEFER fixes

[Sascha Hauer]

The i.MX6 cpufreq driver can't cope with -EPROBE_DEFER returned from
regulator_get/clk_get. This series fix that and some other small issues.

Shawn, Could you give me your ack for patch 4/5?

Sascha

Changes since v1:

- Fix Coding style nitpicks
- drop unnecessary platform_device argument from
  imx6q_cpufreq_get_resources
- Add more comments
- Use id_table to distinguish between SoCs
- make PU regulator non optional on SoCs that need it and don't request
  it on SoCs that don't need it

----------------------------------------------------------------
Sascha Hauer (5):
      cpufreq: imx6q: Fix goto wrong error label
      cpufreq: imx6q: Fix wrong device in devm_kzalloc
      cpufreq: imx6q: Fix regulator/clock error handling
      cpufreq: imx6q: Use id_table to distinguish between SoCs
      cpufreq: imx6q: pu regulator is not optional

 arch/arm/mach-imx/mach-imx6sl.c |   2 +-
 arch/arm/mach-imx/mach-imx6sx.c |   2 +-
 arch/arm/mach-imx/mach-imx6ul.c |   2 +-
 drivers/cpufreq/imx6q-cpufreq.c | 213 ++++++++++++++++++++++++++--------------
 4 files changed, 145 insertions(+), 74 deletions(-)

[PATCH 1/5] cpufreq: imx6q: Fix goto wrong error label

[Sascha Hauer]

After the call to dev_pm_opp_init_cpufreq_table() we have to go to
out_free_opp because we have already allocated the opp.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/cpufreq/imx6q-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index ef1fa81..fabd144 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -256,7 +256,7 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
 	if (ret) {
 		dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
-		goto put_reg;
+		goto out_free_opp;
 	}
 
 	/* Make imx6_soc_volt array's size same as arm opp number */
-- 
2.6.1

[PATCH 2/5] cpufreq: imx6q: Fix wrong device in devm_kzalloc

[Sascha Hauer]

devm_kzalloc must be called with the device that is actually probed,
not with cpu_dev which resources are not freed when probe fails.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/cpufreq/imx6q-cpufreq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index fabd144..6fefd19 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -260,7 +260,8 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 	}
 
 	/* Make imx6_soc_volt array's size same as arm opp number */
-	imx6_soc_volt = devm_kzalloc(cpu_dev, sizeof(*imx6_soc_volt) * num, GFP_KERNEL);
+	imx6_soc_volt = devm_kzalloc(&pdev->dev,
+				     sizeof(*imx6_soc_volt) * num, GFP_KERNEL);
 	if (imx6_soc_volt == NULL) {
 		ret = -ENOMEM;
 		goto free_freq_table;
-- 
2.6.1

[PATCH 3/5] cpufreq: imx6q: Fix regulator/clock error handling

[Sascha Hauer]

When one of the regulator_get/clk_get calls fails the driver just
returns -ENOENT. To properly handle the -EPROBE_DEFER case we have
to forward the actual error value instead. This patch fixes that.

Since all clocks/regulators are global static variables the IS_ERR()
check in the error path in probe() may erroneously work on the values
assigned in the previous probe run. This means we have to reinitialize
the variables between two probe runs().

The probe function already is quite overloaded, so this patch moves
the regulator and clock allocation into a separate function and the
dropping of these resourced into a corresponding cleanup function.
This is easier to review for correctness than the current mix of
reverse-cleanup and !IS_ERR checks at the end of probe().

While at it leave a comment that these resource allocations can't use
devm_* to prevent people from trying to convert it.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/cpufreq/imx6q-cpufreq.c | 170 +++++++++++++++++++++++++---------------
 1 file changed, 106 insertions(+), 64 deletions(-)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 6fefd19..f81cf8e 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -176,6 +176,103 @@ static struct cpufreq_driver imx6q_cpufreq_driver = {
 	.attr = cpufreq_generic_attr,
 };
 
+static int imx6q_cpufreq_get_resources(void)
+{
+	/*
+	 * Do not use devm_* here. The resources are bound to the
+	 * cpu_dev and not to this drivers platform_device.
+	 */
+	arm_clk = clk_get(cpu_dev, "arm");
+	if (IS_ERR(arm_clk))
+		return PTR_ERR(arm_clk);
+
+	pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
+	if (IS_ERR(pll1_sys_clk))
+		return PTR_ERR(pll1_sys_clk);
+
+	pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
+	if (IS_ERR(pll1_sw_clk))
+		return PTR_ERR(pll1_sw_clk);
+
+	step_clk = clk_get(cpu_dev, "step");
+	if (IS_ERR(step_clk))
+		return PTR_ERR(step_clk);
+
+	pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
+	if (IS_ERR(pll2_pfd2_396m_clk))
+		return PTR_ERR(pll2_pfd2_396m_clk);
+
+	if (of_machine_is_compatible("fsl,imx6ul")) {
+		pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
+		if (IS_ERR(pll2_bus_clk))
+			return PTR_ERR(pll2_bus_clk);
+
+		secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
+		if (IS_ERR(secondary_sel_clk))
+			return PTR_ERR(secondary_sel_clk);
+	}
+
+	arm_reg = regulator_get(cpu_dev, "arm");
+	if (IS_ERR(arm_reg))
+		return PTR_ERR(arm_reg);
+
+	pu_reg = regulator_get_optional(cpu_dev, "pu");
+
+	soc_reg = regulator_get(cpu_dev, "soc");
+	if (IS_ERR(soc_reg))
+		return PTR_ERR(soc_reg);
+
+	return 0;
+}
+
+
+static void imx6q_cpufreq_put_resources(void)
+{
+	/*
+	 * Set all resources to NULL here so that they are correctly
+	 * initialized next time we enter probe()
+	 */
+	if (!IS_ERR_OR_NULL(arm_reg))
+		regulator_put(arm_reg);
+	arm_reg = NULL;
+
+	if (!IS_ERR_OR_NULL(pu_reg))
+		regulator_put(pu_reg);
+	pu_reg = NULL;
+
+	if (!IS_ERR_OR_NULL(soc_reg))
+		regulator_put(soc_reg);
+	soc_reg = NULL;
+
+	if (!IS_ERR_OR_NULL(arm_clk))
+		clk_put(arm_clk);
+	arm_clk = NULL;
+
+	if (!IS_ERR_OR_NULL(pll1_sys_clk))
+		clk_put(pll1_sys_clk);
+	pll1_sys_clk = NULL;
+
+	if (!IS_ERR_OR_NULL(pll1_sw_clk))
+		clk_put(pll1_sw_clk);
+	pll1_sw_clk = NULL;
+
+	if (!IS_ERR_OR_NULL(step_clk))
+		clk_put(step_clk);
+	step_clk = NULL;
+
+	if (!IS_ERR_OR_NULL(pll2_pfd2_396m_clk))
+		clk_put(pll2_pfd2_396m_clk);
+	pll2_pfd2_396m_clk = NULL;
+
+	if (!IS_ERR_OR_NULL(pll2_bus_clk))
+		clk_put(pll2_bus_clk);
+	pll2_bus_clk = NULL;
+
+	if (!IS_ERR_OR_NULL(secondary_sel_clk))
+		clk_put(secondary_sel_clk);
+	secondary_sel_clk = NULL;
+}
+
 static int imx6q_cpufreq_probe(struct platform_device *pdev)
 {
 	struct device_node *np;
@@ -198,36 +295,9 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
-	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)) {
-		dev_err(cpu_dev, "failed to get clocks\n");
-		ret = -ENOENT;
-		goto put_clk;
-	}
-
-	if (of_machine_is_compatible("fsl,imx6ul")) {
-		pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
-		secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
-		if (IS_ERR(pll2_bus_clk) || IS_ERR(secondary_sel_clk)) {
-			dev_err(cpu_dev, "failed to get clocks specific to imx6ul\n");
-			ret = -ENOENT;
-			goto put_clk;
-		}
-	}
-
-	arm_reg = regulator_get(cpu_dev, "arm");
-	pu_reg = regulator_get_optional(cpu_dev, "pu");
-	soc_reg = regulator_get(cpu_dev, "soc");
-	if (IS_ERR(arm_reg) || IS_ERR(soc_reg)) {
-		dev_err(cpu_dev, "failed to get regulators\n");
-		ret = -ENOENT;
-		goto put_reg;
-	}
+	ret = imx6q_cpufreq_get_resources();
+	if (ret)
+		goto drop_resources;
 
 	/*
 	 * We expect an OPP table supplied by platform.
@@ -239,7 +309,7 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 		ret = dev_pm_opp_of_add_table(cpu_dev);
 		if (ret < 0) {
 			dev_err(cpu_dev, "failed to init OPP table: %d\n", ret);
-			goto put_reg;
+			goto drop_resources;
 		}
 
 		/* Because we have added the OPPs here, we must free them */
@@ -348,28 +418,9 @@ free_freq_table:
 out_free_opp:
 	if (free_opp)
 		dev_pm_opp_of_remove_table(cpu_dev);
-put_reg:
-	if (!IS_ERR(arm_reg))
-		regulator_put(arm_reg);
-	if (!IS_ERR(pu_reg))
-		regulator_put(pu_reg);
-	if (!IS_ERR(soc_reg))
-		regulator_put(soc_reg);
-put_clk:
-	if (!IS_ERR(arm_clk))
-		clk_put(arm_clk);
-	if (!IS_ERR(pll1_sys_clk))
-		clk_put(pll1_sys_clk);
-	if (!IS_ERR(pll1_sw_clk))
-		clk_put(pll1_sw_clk);
-	if (!IS_ERR(step_clk))
-		clk_put(step_clk);
-	if (!IS_ERR(pll2_pfd2_396m_clk))
-		clk_put(pll2_pfd2_396m_clk);
-	if (!IS_ERR(pll2_bus_clk))
-		clk_put(pll2_bus_clk);
-	if (!IS_ERR(secondary_sel_clk))
-		clk_put(secondary_sel_clk);
+drop_resources:
+	imx6q_cpufreq_put_resources();
+
 	of_node_put(np);
 	return ret;
 }
@@ -380,17 +431,8 @@ static int imx6q_cpufreq_remove(struct platform_device *pdev)
 	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
 	if (free_opp)
 		dev_pm_opp_of_remove_table(cpu_dev);
-	regulator_put(arm_reg);
-	if (!IS_ERR(pu_reg))
-		regulator_put(pu_reg);
-	regulator_put(soc_reg);
-	clk_put(arm_clk);
-	clk_put(pll1_sys_clk);
-	clk_put(pll1_sw_clk);
-	clk_put(step_clk);
-	clk_put(pll2_pfd2_396m_clk);
-	clk_put(pll2_bus_clk);
-	clk_put(secondary_sel_clk);
+
+	imx6q_cpufreq_put_resources();
 
 	return 0;
 }
-- 
2.6.1

[PATCH 4/5] cpufreq: imx6q: Use id_table to distinguish between SoCs

[Sascha Hauer]

The i.MX6 cpufreq driver is instantiated from a static device in
architecture code and not from the device tree. These devices offer the
id_table mechanism to distinguish between different types of devices.
Use this mechanism rather than of_machine_is_compatible() as it scales
better with increasing number of SoCs to check against.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/mach-imx/mach-imx6sl.c |  2 +-
 arch/arm/mach-imx/mach-imx6sx.c |  2 +-
 arch/arm/mach-imx/mach-imx6ul.c |  2 +-
 drivers/cpufreq/imx6q-cpufreq.c | 30 +++++++++++++++++++++++++++---
 4 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-imx/mach-imx6sl.c b/arch/arm/mach-imx/mach-imx6sl.c
index 3003263..c93cf69 100644
--- a/arch/arm/mach-imx/mach-imx6sl.c
+++ b/arch/arm/mach-imx/mach-imx6sl.c
@@ -39,7 +39,7 @@ static void __init imx6sl_init_late(void)
 {
 	/* imx6sl reuses imx6q cpufreq driver */
 	if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ))
-		platform_device_register_simple("imx6q-cpufreq", -1, NULL, 0);
+		platform_device_register_simple("imx6sl-cpufreq", -1, NULL, 0);
 
 	imx6sl_cpuidle_init();
 }
diff --git a/arch/arm/mach-imx/mach-imx6sx.c b/arch/arm/mach-imx/mach-imx6sx.c
index 6a0b061..ca86a25 100644
--- a/arch/arm/mach-imx/mach-imx6sx.c
+++ b/arch/arm/mach-imx/mach-imx6sx.c
@@ -94,7 +94,7 @@ static void __init imx6sx_init_late(void)
 	imx6sx_cpuidle_init();
 
 	if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ))
-		platform_device_register_simple("imx6q-cpufreq", -1, NULL, 0);
+		platform_device_register_simple("imx6sx-cpufreq", -1, NULL, 0);
 }
 
 static const char * const imx6sx_dt_compat[] __initconst = {
diff --git a/arch/arm/mach-imx/mach-imx6ul.c b/arch/arm/mach-imx/mach-imx6ul.c
index acaf705..159ec4e 100644
--- a/arch/arm/mach-imx/mach-imx6ul.c
+++ b/arch/arm/mach-imx/mach-imx6ul.c
@@ -81,7 +81,7 @@ static void __init imx6ul_init_irq(void)
 static void __init imx6ul_init_late(void)
 {
 	if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ))
-		platform_device_register_simple("imx6q-cpufreq", -1, NULL, 0);
+		platform_device_register_simple("imx6ul-cpufreq", -1, NULL, 0);
 }
 
 static const char *imx6ul_dt_compat[] __initconst = {
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index f81cf8e..f3729f9 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -42,6 +42,27 @@ static unsigned int transition_latency;
 static u32 *imx6_soc_volt;
 static u32 soc_opp_count;
 
+static unsigned long imx6_cpufreq_flags;
+
+#define IMX6_CPUFREQ_NEED_PU_REG		(1 << 0)
+#define IMX6_CPUFREQ_NEED_SECONDARY_SEL		(1 << 1)
+
+static const struct platform_device_id imx6_cpufreq_devtype[] = {
+	{
+		.name = "imx6q-cpufreq",
+		.driver_data = IMX6_CPUFREQ_NEED_PU_REG,
+	}, {
+		.name = "imx6ul-cpufreq",
+		.driver_data = IMX6_CPUFREQ_NEED_SECONDARY_SEL,
+	}, {
+		.name = "imx6sl-cpufreq",
+		.driver_data = IMX6_CPUFREQ_NEED_PU_REG,
+	}, {
+		.name = "imx6sx-cpufreq",
+		.driver_data = IMX6_CPUFREQ_NEED_PU_REG,
+	},
+};
+
 static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 {
 	struct dev_pm_opp *opp;
@@ -102,7 +123,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 	 *  - Reprogram pll1_sys_clk and reparent pll1_sw_clk back to it
 	 *  - Disable pll2_pfd2_396m_clk
 	 */
-	if (of_machine_is_compatible("fsl,imx6ul")) {
+	if (imx6_cpufreq_flags & IMX6_CPUFREQ_NEED_SECONDARY_SEL) {
 		/*
 		 * When changing pll1_sw_clk's parent to pll1_sys_clk,
 		 * CPU may run at higher than 528MHz, this will lead to
@@ -202,7 +223,7 @@ static int imx6q_cpufreq_get_resources(void)
 	if (IS_ERR(pll2_pfd2_396m_clk))
 		return PTR_ERR(pll2_pfd2_396m_clk);
 
-	if (of_machine_is_compatible("fsl,imx6ul")) {
+	if (imx6_cpufreq_flags & IMX6_CPUFREQ_NEED_SECONDARY_SEL) {
 		pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
 		if (IS_ERR(pll2_bus_clk))
 			return PTR_ERR(pll2_bus_clk);
@@ -283,6 +304,8 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 	const __be32 *val;
 	u32 nr, i, j;
 
+	imx6_cpufreq_flags = platform_get_device_id(pdev)->driver_data;
+
 	cpu_dev = get_cpu_device(0);
 	if (!cpu_dev) {
 		pr_err("failed to get cpu0 device\n");
@@ -439,8 +462,9 @@ static int imx6q_cpufreq_remove(struct platform_device *pdev)
 
 static struct platform_driver imx6q_cpufreq_platdrv = {
 	.driver = {
-		.name	= "imx6q-cpufreq",
+		.name	= "imx6-cpufreq",
 	},
+	.id_table	= imx6_cpufreq_devtype,
 	.probe		= imx6q_cpufreq_probe,
 	.remove		= imx6q_cpufreq_remove,
 };
-- 
2.6.1

[PATCH 5/5] cpufreq: imx6q: pu regulator is not optional

[Sascha Hauer]

Some SoC types have a PU regulator and others don't. On the SoCs which
have a PU regulator it's not optional, so regulator_get_optional is
not appropriate here.

Use the IMX6_CPUFREQ_NEED_PU_REG introduced with the last patch to
check whether we need a PU regulator and if we do, request it using
regulator_get() rather than regulator_get_optional.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/cpufreq/imx6q-cpufreq.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index f3729f9..605897b 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -92,7 +92,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 
 	/* scaling up?  scale voltage before frequency */
 	if (new_freq > old_freq) {
-		if (!IS_ERR(pu_reg)) {
+		if (pu_reg) {
 			ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
 			if (ret) {
 				dev_err(cpu_dev, "failed to scale vddpu up: %d\n", ret);
@@ -169,7 +169,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 			dev_warn(cpu_dev, "failed to scale vddsoc down: %d\n", ret);
 			ret = 0;
 		}
-		if (!IS_ERR(pu_reg)) {
+		if (pu_reg) {
 			ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
 			if (ret) {
 				dev_warn(cpu_dev, "failed to scale vddpu down: %d\n", ret);
@@ -237,7 +237,11 @@ static int imx6q_cpufreq_get_resources(void)
 	if (IS_ERR(arm_reg))
 		return PTR_ERR(arm_reg);
 
-	pu_reg = regulator_get_optional(cpu_dev, "pu");
+	if (imx6_cpufreq_flags & IMX6_CPUFREQ_NEED_PU_REG) {
+		pu_reg = regulator_get(cpu_dev, "pu");
+		if (IS_ERR(pu_reg))
+			return -EPROBE_DEFER;
+	}
 
 	soc_reg = regulator_get(cpu_dev, "soc");
 	if (IS_ERR(soc_reg))
@@ -404,7 +408,7 @@ soc_opp_out:
 	ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
 	if (ret > 0)
 		transition_latency += ret * 1000;
-	if (!IS_ERR(pu_reg)) {
+	if (pu_reg) {
 		ret = regulator_set_voltage_time(pu_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
 		if (ret > 0)
 			transition_latency += ret * 1000;
-- 
2.6.1

Re: [PATCH 2/5] cpufreq: imx6q: Fix wrong device in devm_kzalloc

[Lucas Stach]

Am Mittwoch, den 28.10.2015, 08:59 +0100 schrieb Sascha Hauer:
> devm_kzalloc must be called with the device that is actually probed,
> not with cpu_dev which resources are not freed when probe fails.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

I think I've said this already on the last round,

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/cpufreq/imx6q-cpufreq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index fabd144..6fefd19 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -260,7 +260,8 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Make imx6_soc_volt array's size same as arm opp number */
> -	imx6_soc_volt = devm_kzalloc(cpu_dev, sizeof(*imx6_soc_volt) * num, GFP_KERNEL);
> +	imx6_soc_volt = devm_kzalloc(&pdev->dev,
> +				     sizeof(*imx6_soc_volt) * num, GFP_KERNEL);
>  	if (imx6_soc_volt == NULL) {
>  		ret = -ENOMEM;
>  		goto free_freq_table;

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

Re: [PATCH 3/5] cpufreq: imx6q: Fix regulator/clock error handling

[Lucas Stach]

Am Mittwoch, den 28.10.2015, 08:59 +0100 schrieb Sascha Hauer:
> When one of the regulator_get/clk_get calls fails the driver just
> returns -ENOENT. To properly handle the -EPROBE_DEFER case we have
> to forward the actual error value instead. This patch fixes that.
> 
> Since all clocks/regulators are global static variables the IS_ERR()
> check in the error path in probe() may erroneously work on the values
> assigned in the previous probe run. This means we have to reinitialize
> the variables between two probe runs().
> 
> The probe function already is quite overloaded, so this patch moves
> the regulator and clock allocation into a separate function and the
> dropping of these resourced into a corresponding cleanup function.
> This is easier to review for correctness than the current mix of
> reverse-cleanup and !IS_ERR checks at the end of probe().
> 
> While at it leave a comment that these resource allocations can't use
> devm_* to prevent people from trying to convert it.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Looks good to me now,

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/cpufreq/imx6q-cpufreq.c | 170 +++++++++++++++++++++++++---------------
>  1 file changed, 106 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index 6fefd19..f81cf8e 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -176,6 +176,103 @@ static struct cpufreq_driver imx6q_cpufreq_driver = {
>  	.attr = cpufreq_generic_attr,
>  };
>  
> +static int imx6q_cpufreq_get_resources(void)
> +{
> +	/*
> +	 * Do not use devm_* here. The resources are bound to the
> +	 * cpu_dev and not to this drivers platform_device.
> +	 */
> +	arm_clk = clk_get(cpu_dev, "arm");
> +	if (IS_ERR(arm_clk))
> +		return PTR_ERR(arm_clk);
> +
> +	pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
> +	if (IS_ERR(pll1_sys_clk))
> +		return PTR_ERR(pll1_sys_clk);
> +
> +	pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
> +	if (IS_ERR(pll1_sw_clk))
> +		return PTR_ERR(pll1_sw_clk);
> +
> +	step_clk = clk_get(cpu_dev, "step");
> +	if (IS_ERR(step_clk))
> +		return PTR_ERR(step_clk);
> +
> +	pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
> +	if (IS_ERR(pll2_pfd2_396m_clk))
> +		return PTR_ERR(pll2_pfd2_396m_clk);
> +
> +	if (of_machine_is_compatible("fsl,imx6ul")) {
> +		pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
> +		if (IS_ERR(pll2_bus_clk))
> +			return PTR_ERR(pll2_bus_clk);
> +
> +		secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
> +		if (IS_ERR(secondary_sel_clk))
> +			return PTR_ERR(secondary_sel_clk);
> +	}
> +
> +	arm_reg = regulator_get(cpu_dev, "arm");
> +	if (IS_ERR(arm_reg))
> +		return PTR_ERR(arm_reg);
> +
> +	pu_reg = regulator_get_optional(cpu_dev, "pu");
> +
> +	soc_reg = regulator_get(cpu_dev, "soc");
> +	if (IS_ERR(soc_reg))
> +		return PTR_ERR(soc_reg);
> +
> +	return 0;
> +}
> +
> +
> +static void imx6q_cpufreq_put_resources(void)
> +{
> +	/*
> +	 * Set all resources to NULL here so that they are correctly
> +	 * initialized next time we enter probe()
> +	 */
> +	if (!IS_ERR_OR_NULL(arm_reg))
> +		regulator_put(arm_reg);
> +	arm_reg = NULL;
> +
> +	if (!IS_ERR_OR_NULL(pu_reg))
> +		regulator_put(pu_reg);
> +	pu_reg = NULL;
> +
> +	if (!IS_ERR_OR_NULL(soc_reg))
> +		regulator_put(soc_reg);
> +	soc_reg = NULL;
> +
> +	if (!IS_ERR_OR_NULL(arm_clk))
> +		clk_put(arm_clk);
> +	arm_clk = NULL;
> +
> +	if (!IS_ERR_OR_NULL(pll1_sys_clk))
> +		clk_put(pll1_sys_clk);
> +	pll1_sys_clk = NULL;
> +
> +	if (!IS_ERR_OR_NULL(pll1_sw_clk))
> +		clk_put(pll1_sw_clk);
> +	pll1_sw_clk = NULL;
> +
> +	if (!IS_ERR_OR_NULL(step_clk))
> +		clk_put(step_clk);
> +	step_clk = NULL;
> +
> +	if (!IS_ERR_OR_NULL(pll2_pfd2_396m_clk))
> +		clk_put(pll2_pfd2_396m_clk);
> +	pll2_pfd2_396m_clk = NULL;
> +
> +	if (!IS_ERR_OR_NULL(pll2_bus_clk))
> +		clk_put(pll2_bus_clk);
> +	pll2_bus_clk = NULL;
> +
> +	if (!IS_ERR_OR_NULL(secondary_sel_clk))
> +		clk_put(secondary_sel_clk);
> +	secondary_sel_clk = NULL;
> +}
> +
>  static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np;
> @@ -198,36 +295,9 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  		return -ENOENT;
>  	}
>  
> -	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)) {
> -		dev_err(cpu_dev, "failed to get clocks\n");
> -		ret = -ENOENT;
> -		goto put_clk;
> -	}
> -
> -	if (of_machine_is_compatible("fsl,imx6ul")) {
> -		pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
> -		secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
> -		if (IS_ERR(pll2_bus_clk) || IS_ERR(secondary_sel_clk)) {
> -			dev_err(cpu_dev, "failed to get clocks specific to imx6ul\n");
> -			ret = -ENOENT;
> -			goto put_clk;
> -		}
> -	}
> -
> -	arm_reg = regulator_get(cpu_dev, "arm");
> -	pu_reg = regulator_get_optional(cpu_dev, "pu");
> -	soc_reg = regulator_get(cpu_dev, "soc");
> -	if (IS_ERR(arm_reg) || IS_ERR(soc_reg)) {
> -		dev_err(cpu_dev, "failed to get regulators\n");
> -		ret = -ENOENT;
> -		goto put_reg;
> -	}
> +	ret = imx6q_cpufreq_get_resources();
> +	if (ret)
> +		goto drop_resources;
>  
>  	/*
>  	 * We expect an OPP table supplied by platform.
> @@ -239,7 +309,7 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  		ret = dev_pm_opp_of_add_table(cpu_dev);
>  		if (ret < 0) {
>  			dev_err(cpu_dev, "failed to init OPP table: %d\n", ret);
> -			goto put_reg;
> +			goto drop_resources;
>  		}
>  
>  		/* Because we have added the OPPs here, we must free them */
> @@ -348,28 +418,9 @@ free_freq_table:
>  out_free_opp:
>  	if (free_opp)
>  		dev_pm_opp_of_remove_table(cpu_dev);
> -put_reg:
> -	if (!IS_ERR(arm_reg))
> -		regulator_put(arm_reg);
> -	if (!IS_ERR(pu_reg))
> -		regulator_put(pu_reg);
> -	if (!IS_ERR(soc_reg))
> -		regulator_put(soc_reg);
> -put_clk:
> -	if (!IS_ERR(arm_clk))
> -		clk_put(arm_clk);
> -	if (!IS_ERR(pll1_sys_clk))
> -		clk_put(pll1_sys_clk);
> -	if (!IS_ERR(pll1_sw_clk))
> -		clk_put(pll1_sw_clk);
> -	if (!IS_ERR(step_clk))
> -		clk_put(step_clk);
> -	if (!IS_ERR(pll2_pfd2_396m_clk))
> -		clk_put(pll2_pfd2_396m_clk);
> -	if (!IS_ERR(pll2_bus_clk))
> -		clk_put(pll2_bus_clk);
> -	if (!IS_ERR(secondary_sel_clk))
> -		clk_put(secondary_sel_clk);
> +drop_resources:
> +	imx6q_cpufreq_put_resources();
> +
>  	of_node_put(np);
>  	return ret;
>  }
> @@ -380,17 +431,8 @@ static int imx6q_cpufreq_remove(struct platform_device *pdev)
>  	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
>  	if (free_opp)
>  		dev_pm_opp_of_remove_table(cpu_dev);
> -	regulator_put(arm_reg);
> -	if (!IS_ERR(pu_reg))
> -		regulator_put(pu_reg);
> -	regulator_put(soc_reg);
> -	clk_put(arm_clk);
> -	clk_put(pll1_sys_clk);
> -	clk_put(pll1_sw_clk);
> -	clk_put(step_clk);
> -	clk_put(pll2_pfd2_396m_clk);
> -	clk_put(pll2_bus_clk);
> -	clk_put(secondary_sel_clk);
> +
> +	imx6q_cpufreq_put_resources();
>  
>  	return 0;
>  }

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

Re: [PATCH 4/5] cpufreq: imx6q: Use id_table to distinguish between SoCs

[Lucas Stach]

Am Mittwoch, den 28.10.2015, 08:59 +0100 schrieb Sascha Hauer:
> The i.MX6 cpufreq driver is instantiated from a static device in
> architecture code and not from the device tree. These devices offer the
> id_table mechanism to distinguish between different types of devices.
> Use this mechanism rather than of_machine_is_compatible() as it scales
> better with increasing number of SoCs to check against.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  arch/arm/mach-imx/mach-imx6sl.c |  2 +-
>  arch/arm/mach-imx/mach-imx6sx.c |  2 +-
>  arch/arm/mach-imx/mach-imx6ul.c |  2 +-
>  drivers/cpufreq/imx6q-cpufreq.c | 30 +++++++++++++++++++++++++++---
>  4 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/mach-imx6sl.c b/arch/arm/mach-imx/mach-imx6sl.c
> index 3003263..c93cf69 100644
> --- a/arch/arm/mach-imx/mach-imx6sl.c
> +++ b/arch/arm/mach-imx/mach-imx6sl.c
> @@ -39,7 +39,7 @@ static void __init imx6sl_init_late(void)
>  {
>  	/* imx6sl reuses imx6q cpufreq driver */

This comment seems a little confusing now.

>  	if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ))
> -		platform_device_register_simple("imx6q-cpufreq", -1, NULL, 0);
> +		platform_device_register_simple("imx6sl-cpufreq", -1, NULL, 0);
>  
>  	imx6sl_cpuidle_init();
>  }
> diff --git a/arch/arm/mach-imx/mach-imx6sx.c b/arch/arm/mach-imx/mach-imx6sx.c
> index 6a0b061..ca86a25 100644
> --- a/arch/arm/mach-imx/mach-imx6sx.c
> +++ b/arch/arm/mach-imx/mach-imx6sx.c
> @@ -94,7 +94,7 @@ static void __init imx6sx_init_late(void)
>  	imx6sx_cpuidle_init();
>  
>  	if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ))
> -		platform_device_register_simple("imx6q-cpufreq", -1, NULL, 0);
> +		platform_device_register_simple("imx6sx-cpufreq", -1, NULL, 0);
>  }
>  
>  static const char * const imx6sx_dt_compat[] __initconst = {
> diff --git a/arch/arm/mach-imx/mach-imx6ul.c b/arch/arm/mach-imx/mach-imx6ul.c
> index acaf705..159ec4e 100644
> --- a/arch/arm/mach-imx/mach-imx6ul.c
> +++ b/arch/arm/mach-imx/mach-imx6ul.c
> @@ -81,7 +81,7 @@ static void __init imx6ul_init_irq(void)
>  static void __init imx6ul_init_late(void)
>  {
>  	if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ))
> -		platform_device_register_simple("imx6q-cpufreq", -1, NULL, 0);
> +		platform_device_register_simple("imx6ul-cpufreq", -1, NULL, 0);
>  }
>  
>  static const char *imx6ul_dt_compat[] __initconst = {
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index f81cf8e..f3729f9 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -42,6 +42,27 @@ static unsigned int transition_latency;
>  static u32 *imx6_soc_volt;
>  static u32 soc_opp_count;
>  
> +static unsigned long imx6_cpufreq_flags;
> +
> +#define IMX6_CPUFREQ_NEED_PU_REG		(1 << 0)
> +#define IMX6_CPUFREQ_NEED_SECONDARY_SEL		(1 << 1)
> +
> +static const struct platform_device_id imx6_cpufreq_devtype[] = {
> +	{
> +		.name = "imx6q-cpufreq",
> +		.driver_data = IMX6_CPUFREQ_NEED_PU_REG,
> +	}, {
> +		.name = "imx6ul-cpufreq",
> +		.driver_data = IMX6_CPUFREQ_NEED_SECONDARY_SEL,
> +	}, {
> +		.name = "imx6sl-cpufreq",
> +		.driver_data = IMX6_CPUFREQ_NEED_PU_REG,
> +	}, {
> +		.name = "imx6sx-cpufreq",
> +		.driver_data = IMX6_CPUFREQ_NEED_PU_REG,

This is wrong. SX doesn't have a PU LDO. Also I would have expected this
flag to be added in the next patch.

> +	},
> +};
> +
>  static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
>  {
>  	struct dev_pm_opp *opp;
> @@ -102,7 +123,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
>  	 *  - Reprogram pll1_sys_clk and reparent pll1_sw_clk back to it
>  	 *  - Disable pll2_pfd2_396m_clk
>  	 */
> -	if (of_machine_is_compatible("fsl,imx6ul")) {
> +	if (imx6_cpufreq_flags & IMX6_CPUFREQ_NEED_SECONDARY_SEL) {
>  		/*
>  		 * When changing pll1_sw_clk's parent to pll1_sys_clk,
>  		 * CPU may run at higher than 528MHz, this will lead to
> @@ -202,7 +223,7 @@ static int imx6q_cpufreq_get_resources(void)
>  	if (IS_ERR(pll2_pfd2_396m_clk))
>  		return PTR_ERR(pll2_pfd2_396m_clk);
>  
> -	if (of_machine_is_compatible("fsl,imx6ul")) {
> +	if (imx6_cpufreq_flags & IMX6_CPUFREQ_NEED_SECONDARY_SEL) {
>  		pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
>  		if (IS_ERR(pll2_bus_clk))
>  			return PTR_ERR(pll2_bus_clk);
> @@ -283,6 +304,8 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  	const __be32 *val;
>  	u32 nr, i, j;
>  
> +	imx6_cpufreq_flags = platform_get_device_id(pdev)->driver_data;
> +
>  	cpu_dev = get_cpu_device(0);
>  	if (!cpu_dev) {
>  		pr_err("failed to get cpu0 device\n");
> @@ -439,8 +462,9 @@ static int imx6q_cpufreq_remove(struct platform_device *pdev)
>  
>  static struct platform_driver imx6q_cpufreq_platdrv = {
>  	.driver = {
> -		.name	= "imx6q-cpufreq",
> +		.name	= "imx6-cpufreq",
>  	},
> +	.id_table	= imx6_cpufreq_devtype,
>  	.probe		= imx6q_cpufreq_probe,
>  	.remove		= imx6q_cpufreq_remove,
>  };

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

Re: [PATCH v2] cpufreq: i.MX6: -EPROBE_DEFER fixes

[Lucas Stach]

Am Mittwoch, den 28.10.2015, 08:59 +0100 schrieb Sascha Hauer:
> The i.MX6 cpufreq driver can't cope with -EPROBE_DEFER returned from
> regulator_get/clk_get. This series fix that and some other small
> issues.
> 
> Shawn, Could you give me your ack for patch 4/5?
> 
> Sascha
> 
What's the status of this series? I think I only had some minor
comments on the last round. Can you please fix those and resend the
series? It would be a shame if this misses 4.5, as it includes some
really nice cleanups and fixes.

Regards,
Lucas

> Changes since v1:
> 
> - Fix Coding style nitpicks
> - drop unnecessary platform_device argument from
>   imx6q_cpufreq_get_resources
> - Add more comments
> - Use id_table to distinguish between SoCs
> - make PU regulator non optional on SoCs that need it and don't
> request
>   it on SoCs that don't need it
> 
> ----------------------------------------------------------------
> Sascha Hauer (5):
>       cpufreq: imx6q: Fix goto wrong error label
>       cpufreq: imx6q: Fix wrong device in devm_kzalloc
>       cpufreq: imx6q: Fix regulator/clock error handling
>       cpufreq: imx6q: Use id_table to distinguish between SoCs
>       cpufreq: imx6q: pu regulator is not optional
> 
>  arch/arm/mach-imx/mach-imx6sl.c |   2 +-
>  arch/arm/mach-imx/mach-imx6sx.c |   2 +-
>  arch/arm/mach-imx/mach-imx6ul.c |   2 +-
>  drivers/cpufreq/imx6q-cpufreq.c | 213 ++++++++++++++++++++++++++--
> ------------
>  4 files changed, 145 insertions(+), 74 deletions(-)
> --
> 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 4/5] cpufreq: imx6q: Use id_table to distinguish between SoCs

[Shawn Guo]

On Wed, Oct 28, 2015 at 08:59:13AM +0100, Sascha Hauer wrote:
> The i.MX6 cpufreq driver is instantiated from a static device in
> architecture code and not from the device tree. These devices offer the
> id_table mechanism to distinguish between different types of devices.
> Use this mechanism rather than of_machine_is_compatible() as it scales
> better with increasing number of SoCs to check against.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Acked-by: Shawn Guo <shawnguo@kernel.org>

Sorry for my oversight on that Sascha was asking my ACK on this patch in
the cover letter.  Only noticed that with Lucas' ping.

Shawn