cpufreq: i.MX6: -EPROBE_DEFER fixes

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]>

---
 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 */

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..7557d1e 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;

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 +0 -0

index fabd144..7557d1e 100644

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 +48 -0

+{
+	/*
+	 * 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;
+}
+

drivers/cpufreq/imx6q-cpufreq.c static struct cpufreq_driver imx6q_cpufreq_driver = { · line 219

Quoted 5 lines · Sascha Hauer

drivers/cpufreq/imx6q-cpufreq.c +5 -0

+	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");

Yes, current code doesn't care about if this failed, but if that's the intention please add a comment for it now.

Quoted 4 lines

drivers/cpufreq/imx6q-cpufreq.c +3 -0

+	soc_reg = regulator_get(cpu_dev, "soc");
+	if (IS_ERR(soc_reg))
+		return PTR_ERR(soc_reg);

drivers/cpufreq/imx6q-cpufreq.c static struct cpufreq_driver imx6q_cpufreq_driver = { · line 233

Quoted 10 lines

drivers/cpufreq/imx6q-cpufreq.c +10 -0

+
+	return 0;
+}
+
+
+static void imx6q_cpufreq_put_resources(void)
+{
+	if (!IS_ERR_OR_NULL(arm_reg))
+		regulator_put(arm_reg);
+	arm_reg = NULL;

Can you please clarify (maybe again) why this is required to be set to NULL again?

While pu_reg is an optional regulator we still have to check the return value for -EPROBE_DEFER to properly detect the case when we have a pu regulator but it is not yet present.

While at it, set pu_reg to NULL for an erroneous regulator so that we can check for the regulator with if (pu_reg) instead of the slightly less readable IS_ERR().

Signed-off-by Sascha Hauer <[email protected]>

---
 drivers/cpufreq/imx6q-cpufreq.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

drivers/cpufreq/imx6q-cpufreq.c +2 -2

index ce0c6bd..b643614 100644
@@ -71,7 +71,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);

@@ -148,7 +148,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);

drivers/cpufreq/imx6q-cpufreq.c +1 -1

 	soc_reg = regulator_get(cpu_dev, "soc");
 	if (IS_ERR(soc_reg))

@@ -377,7 +382,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;

Am Donnerstag, den 22.10.2015, 10:47 +0200 schrieb Sascha Hauer:

Quoted 38 lines

While pu_reg is an optional regulator we still have to check the return value for -EPROBE_DEFER to properly detect the case when we have a pu regulator but it is not yet present.

While at it, set pu_reg to NULL for an erroneous regulator so that we can check for the regulator with if (pu_reg) instead of the slightly less readable IS_ERR().

Signed-off-by Sascha Hauer <[email protected]>

---
 drivers/cpufreq/imx6q-cpufreq.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

drivers/cpufreq/imx6q-cpufreq.c +2 -2

index ce0c6bd..b643614 100644
@@ -71,7 +71,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);

@@ -148,7 +148,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);

@@ -217,6 +217,11 @@ static int imx6q_cpufreq_get_resources(struct platform_device *pdev)
 		return PTR_ERR(arm_reg);
 

drivers/cpufreq/imx6q-cpufreq.c static int imx6q_cpufreq_get_resources(struct platform_device *pdev) · line 224

Quoted 6 lines

drivers/cpufreq/imx6q-cpufreq.c +5 -0

 	pu_reg = regulator_get_optional(cpu_dev, "pu");
+	if (IS_ERR(pu_reg)) {
+		if (PTR_ERR(pu_reg) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		pu_reg = NULL;
+	}

The PU regulator isn't really optional for MX6 Q/DL (where it is present in hardware) as the datasheet clearly specifies that it needs to be set to the same voltage as SOC. Could you change this to request this regulator as non-optional for those chip derivatives and skip the request otherwise?

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.

This series supersedes
[PATCH 1/2] cpufreq: imx: fix regulator_get error handling [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling from Heiner Kallweit.

Quoted 7 lines · 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.

This series supersedes
[PATCH 1/2] cpufreq: imx: fix regulator_get error handling [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling from Heiner Kallweit.

I'm expecting a new version of this addressing the review comments to be submitted.