Skip to contentSkip to conversation

cpufreq: i.MX6: -EPROBE_DEFER fixes

PATCH New Sascha Hauer 3 participants 11 messages Oct 28, 2015 – Dec 14, 2015

Subsystems (4)
Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape CPU frequency scaling framework
lore
Share & tools
lore

Sascha Hauer

author maintainer
#0 lore raw

PATCH v2 1/1 cpufreq: i.MX6: -EPROBE_DEFER fixes

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(-)
author maintainer
#1 lore raw

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

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 <[email protected]>
Reviewed-by Lucas Stach <[email protected]>
---
 drivers/cpufreq/imx6q-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/cpufreq/imx6q-cpufreq.c +1 -1
index ef1fa81..fabd144 100644
@@ -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 */
Signature · 1 line
2.6.1
author maintainer
#2 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/cpufreq/imx6q-cpufreq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/cpufreq/imx6q-cpufreq.c +2 -1
index fabd144..6fefd19 100644
@@ -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;
Signature · 1 line
2.6.1
#3 lore raw

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

Am Mittwoch, den 28.10.2015, 08:59 +0100 schrieb Sascha Hauer:

Quoted 4 lines

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 <[email protected]>

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

Reviewed-by Lucas Stach <[email protected]>
Quoted 18 lines
---
 drivers/cpufreq/imx6q-cpufreq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/cpufreq/imx6q-cpufreq.c +2 -1
index fabd144..6fefd19 100644
@@ -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;
Signature · 2 lines
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |
author maintainer
#4 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/cpufreq/imx6q-cpufreq.c | 170 +++++++++++++++++++++++++---------------
 1 file changed, 106 insertions(+), 64 deletions(-)
drivers/cpufreq/imx6q-cpufreq.c +106 -64
index 6fefd19..f81cf8e 100644
@@ -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;
 }
Signature · 1 line
2.6.1
#5 lore raw

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

Am Mittwoch, den 28.10.2015, 08:59 +0100 schrieb Sascha Hauer:

Quoted 15 lines

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().

Quoted 4 lines

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 <[email protected]>

Looks good to me now,

Reviewed-by Lucas Stach <[email protected]>
Quoted 213 lines
---
 drivers/cpufreq/imx6q-cpufreq.c | 170 +++++++++++++++++++++++++---------------
 1 file changed, 106 insertions(+), 64 deletions(-)
drivers/cpufreq/imx6q-cpufreq.c +106 -64
index 6fefd19..f81cf8e 100644
@@ -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;
 }
Signature · 2 lines
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |
author maintainer
#6 lore raw

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

In reply to 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 <[email protected]>
---
 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(-)
arch/arm/mach-imx/mach-imx6sl.c +1 -1
index 3003263..c93cf69 100644
@@ -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();
 }
arch/arm/mach-imx/mach-imx6sx.c +1 -1
index 6a0b061..ca86a25 100644
@@ -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 = {
arch/arm/mach-imx/mach-imx6ul.c +1 -1
index acaf705..159ec4e 100644
@@ -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 = {
drivers/cpufreq/imx6q-cpufreq.c +27 -3
index f81cf8e..f3729f9 100644
@@ -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@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,
 };
Signature · 1 line
2.6.1
#7 lore raw

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

Am Mittwoch, den 28.10.2015, 08:59 +0100 schrieb Sascha Hauer:

Quoted 19 lines

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 <[email protected]>
---
 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(-)
arch/arm/mach-imx/mach-imx6sl.c +0 -0
index 3003263..c93cf69 100644

arch/arm/mach-imx/mach-imx6sl.c static void __init imx6sl_init_late(void) · line 40

Quoted 2 lines
arch/arm/mach-imx/mach-imx6sl.c +0 -0
@@ -39,7 +39,7 @@ static void __init imx6sl_init_late(void)
 {
 	/* imx6sl reuses imx6q cpufreq driver */

This comment seems a little confusing now.

Quoted 48 lines
arch/arm/mach-imx/mach-imx6sl.c +1 -1
 	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();
 }
arch/arm/mach-imx/mach-imx6sx.c +1 -1
index 6a0b061..ca86a25 100644
@@ -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 = {
arch/arm/mach-imx/mach-imx6ul.c +1 -1
index acaf705..159ec4e 100644
@@ -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 = {
drivers/cpufreq/imx6q-cpufreq.c +8 -0
index f81cf8e..f3729f9 100644
@@ -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",

drivers/cpufreq/imx6q-cpufreq.c static unsigned int transition_latency; · line 62

Quoted 10 lines
drivers/cpufreq/imx6q-cpufreq.c +10 -0
+		.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.

Quoted 44 lines
drivers/cpufreq/imx6q-cpufreq.c +9 -3
+	},
+};
+
 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,
 };
Signature · 2 lines
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |
#8 lore raw

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

In reply to Sascha Hauer
Quoted 7 lines · 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 <[email protected]>
Acked-by Shawn Guo <[email protected]>

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

author maintainer
#9 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/cpufreq/imx6q-cpufreq.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/cpufreq/imx6q-cpufreq.c +8 -4
index f3729f9..605897b 100644
@@ -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;
Signature · 1 line
2.6.1
#10 lore raw

PATCH v2 1/1 cpufreq: i.MX6: -EPROBE_DEFER fixes

In reply to Sascha Hauer

Am Mittwoch, den 28.10.2015, 08:59 +0100 schrieb Sascha Hauer:

Quoted 8 lines

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

Quoted 30 lines

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 at vger.kernel.org More majordomo info at??http://vger.kernel.org/majordomo-info.html