Skip to contentSkip to conversation

ARM: imx: Set LDO regulator supply

PATCH Applied Leonard Crestez 4 participants 12 messages Apr 4, 2017 – Apr 19, 2017

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

v2 cover ARM: imx: Set LDO regulator supply

Setting the LDO regulator parent is optional but beneficial. It will cause the PMIC output voltage to be dynamically set to the minimum input for the LDOs, this should be more efficient.

This propagation was introduced by:
commit fc42112c0eaa ("regulator: core: Propagate voltage changes to supply regulators")

Changes since v1:
* Drop patch 1 since it only avoids logging a warning and the gpc driver
is going through more changes.
* Initialize cpufreq->suspend_freq based on policy->max instead.
* Remove reference to ldo-bypass from suspend_freq patch message.

It's not clear if policy->cpuinfo.max would be preferable, at init time they should be identical.

Irina Tirdea (1):
cpufreq: imx6q: Fix handling EPROBE_DEFER from regulator

Leonard Crestez (3):
cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend
ARM: dts: imx6qdl-sabresd: Set LDO regulator supply
ARM: dts: imx6qp-sabresd: Set reg_arm regulator supply

 arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 12 ++++++++++++
 arch/arm/boot/dts/imx6qp-sabresd.dts   |  4 ++--
 drivers/cpufreq/imx6q-cpufreq.c        | 15 ++++++++++++++-
 3 files changed, 28 insertions(+), 3 deletions(-)
Signature · 1 line
2.7.4
#1 lore raw

PATCH v2 1/4 cpufreq: imx6q: Fix handling EPROBE_DEFER from regulator

From: Irina Tirdea <[email protected]>

If there are any errors in getting the cpu0 regulators, the driver returns -ENOENT. In case the regulators are not yet available, the devm_regulator_get calls will return -EPROBE_DEFER, so that the driver can be probed later. If we return -ENOENT, the driver will fail its initialization and will not try to probe again (when the regulators become available).

Return the actual error received from regulator_get in probe. Print a differentiated message in case we need to probe the device later and in case we actually failed. Also add a message to inform when the driver has been successfully registered.

---
 drivers/cpufreq/imx6q-cpufreq.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/cpufreq/imx6q-cpufreq.c +7 -0
index 7719b02..be90ee3 100644
@@ -222,6 +222,13 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 	arm_reg = regulator_get(cpu_dev, "arm");
 	pu_reg = regulator_get_optional(cpu_dev, "pu");
 	soc_reg = regulator_get(cpu_dev, "soc");
+	if (PTR_ERR(arm_reg) == -EPROBE_DEFER ||
+			PTR_ERR(soc_reg) == -EPROBE_DEFER ||
+			PTR_ERR(pu_reg) == -EPROBE_DEFER) {
+		ret = -EPROBE_DEFER;
+		dev_dbg(cpu_dev, "regulators not ready, defer\n");
+		goto put_reg;
+	}
 	if (IS_ERR(arm_reg) || IS_ERR(soc_reg)) {
 		dev_err(cpu_dev, "failed to get regulators\n");
 		ret = -ENOENT;
Signature · 1 line
2.7.4
#2 lore raw

PATCH v2 2/4 cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend

In reply to Leonard Crestez

If the cpufreq driver tries to modify voltage/freq during suspend/resume it might need to control an external PMIC via I2C or SPI but those devices might be already suspended. This issue is likely to happen whenever the LDOs have their vin-supply set.

To avoid this scenario we just increase cpufreq to the maximum before suspend.

---
 drivers/cpufreq/imx6q-cpufreq.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/cpufreq/imx6q-cpufreq.c +7 -1
index be90ee3..786122e 100644
@@ -161,8 +161,13 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 
 static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
 {
+	int ret;
+
 	policy->clk = arm_clk;
-	return cpufreq_generic_init(policy, freq_table, transition_latency);
+	ret = cpufreq_generic_init(policy, freq_table, transition_latency);
+	policy->suspend_freq = policy->max;
+
+	return ret;
 }
 
 static struct cpufreq_driver imx6q_cpufreq_driver = {
@@ -173,6 +178,7 @@ static struct cpufreq_driver imx6q_cpufreq_driver = {
 	.init = imx6q_cpufreq_init,
 	.name = "imx6q-cpufreq",
 	.attr = cpufreq_generic_attr,
+	.suspend = cpufreq_generic_suspend,
 };
 
 static int imx6q_cpufreq_probe(struct platform_device *pdev)
Signature · 1 line
2.7.4
Reviewed-by on v2 2/4
#3 lore raw

Re: [PATCH v2 2/4] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend

Am Dienstag, den 04.04.2017, 20:04 +0300 schrieb Leonard Crestez:

Quoted 9 lines

If the cpufreq driver tries to modify voltage/freq during suspend/resume it might need to control an external PMIC via I2C or SPI but those devices might be already suspended. This issue is likely to happen whenever the LDOs have their vin-supply set.

To avoid this scenario we just increase cpufreq to the maximum before suspend.

Quoted 31 lines
---
 drivers/cpufreq/imx6q-cpufreq.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/cpufreq/imx6q-cpufreq.c +7 -1
index be90ee3..786122e 100644
@@ -161,8 +161,13 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 
 static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
 {
+	int ret;
+
 	policy->clk = arm_clk;
-	return cpufreq_generic_init(policy, freq_table, transition_latency);
+	ret = cpufreq_generic_init(policy, freq_table, transition_latency);
+	policy->suspend_freq = policy->max;
+
+	return ret;
 }
 
 static struct cpufreq_driver imx6q_cpufreq_driver = {
@@ -173,6 +178,7 @@ static struct cpufreq_driver imx6q_cpufreq_driver = {
 	.init = imx6q_cpufreq_init,
 	.name = "imx6q-cpufreq",
 	.attr = cpufreq_generic_attr,
+	.suspend = cpufreq_generic_suspend,
 };
 
 static int imx6q_cpufreq_probe(struct platform_device *pdev)
maintainer
#4 lore raw

Re: [PATCH v2 2/4] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend

In reply to Leonard Crestez
Quoted 5 lines · Leonard Crestez

If the cpufreq driver tries to modify voltage/freq during suspend/resume it might need to control an external PMIC via I2C or SPI but those devices might be already suspended. This issue is likely to happen whenever the LDOs have their vin-supply set.

Quoted 7 lines

To avoid this scenario we just increase cpufreq to the maximum before suspend.

---
 drivers/cpufreq/imx6q-cpufreq.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

I have already Acked this earlier, why didn't you add it ?

Signature · 1 line
viresh
#5 lore raw

Re: [PATCH v2 2/4] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend

Quoted 9 lines · Viresh Kumar
Quoted 8 lines · Leonard Crestez

If the cpufreq driver tries to modify voltage/freq during suspend/resume it might need to control an external PMIC via I2C or SPI but those devices might be already suspended. This issue is likely to happen whenever the LDOs have their vin-supply set.

To avoid this scenario we just increase cpufreq to the maximum before suspend.

Quoted 6 lines
Quoted 4 lines
---
 drivers/cpufreq/imx6q-cpufreq.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

I have already Acked this earlier, why didn't you add it ?

Because v2 is different based on comments from Lucas. It didn't seem appropriate to keep an old "Acked-by" in this case.

maintainer Acked-by on v2 2/4
#6 lore raw

Re: [PATCH v2 2/4] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend

In reply to Leonard Crestez
Quoted 33 lines · Leonard Crestez

If the cpufreq driver tries to modify voltage/freq during suspend/resume it might need to control an external PMIC via I2C or SPI but those devices might be already suspended. This issue is likely to happen whenever the LDOs have their vin-supply set.

To avoid this scenario we just increase cpufreq to the maximum before suspend.

---
 drivers/cpufreq/imx6q-cpufreq.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/cpufreq/imx6q-cpufreq.c +6 -1
index be90ee3..786122e 100644
@@ -161,8 +161,13 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 
 static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
 {
+	int ret;
+
 	policy->clk = arm_clk;
-	return cpufreq_generic_init(policy, freq_table, transition_latency);
+	ret = cpufreq_generic_init(policy, freq_table, transition_latency);
+	policy->suspend_freq = policy->max;
+
+	return ret;
 }
 
 static struct cpufreq_driver imx6q_cpufreq_driver = {

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

Quoted 7 lines
drivers/cpufreq/imx6q-cpufreq.c +1 -0
@@ -173,6 +178,7 @@ static struct cpufreq_driver imx6q_cpufreq_driver = {
 	.init = imx6q_cpufreq_init,
 	.name = "imx6q-cpufreq",
 	.attr = cpufreq_generic_attr,
+	.suspend = cpufreq_generic_suspend,
 };
 
 static int imx6q_cpufreq_probe(struct platform_device *pdev)
Signature · 1 line
viresh
#7 lore raw

PATCH v2 3/4 ARM: dts: imx6qdl-sabresd: Set LDO regulator supply

In reply to Leonard Crestez

Setting the supply is optional but beneficial, it will cause PMIC voltages to be dynamically changed with cpu frequency.

---
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)
arch/arm/boot/dts/imx6qdl-sabresd.dtsi +12 -0
index 63bf95e..58055ce 100644
@@ -548,6 +548,18 @@
 	status = "okay";
 };
 
+&reg_arm {
+       vin-supply = <&sw1a_reg>;
+};
+
+&reg_pu {
+       vin-supply = <&sw1c_reg>;
+};
+
+&reg_soc {
+       vin-supply = <&sw1c_reg>;
+};
+
 &snvs_poweroff {
 	status = "okay";
 };
Signature · 1 line
2.7.4
#9 lore raw

PATCH v2 4/4 ARM: dts: imx6qp-sabresd: Set reg_arm regulator supply

In reply to Leonard Crestez

On imx6qp-sabresd LDO_ARM is connected to a different PMIC output than the other imx6qdl-sabresd boards.

Setting cpu0 arm-supply to sw2_reg is wrong, this must have mistakenly slipped out of the vendor tree where this is are used for LDO bypass.

---
 arch/arm/boot/dts/imx6qp-sabresd.dts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/arm/boot/dts/imx6qp-sabresd.dts +2 -2
index b234580..a8a5004 100644
@@ -50,8 +50,8 @@
 	compatible = "fsl,imx6qp-sabresd", "fsl,imx6qp";
 };
 
-&cpu0 {
-	arm-supply = <&sw2_reg>;
+&reg_arm {
+	vin-supply = <&sw2_reg>;
 };
 
 &iomuxc {
Signature · 1 line
2.7.4
Applied
#10 lore raw

Re: [PATCH v2 4/4] ARM: dts: imx6qp-sabresd: Set reg_arm regulator supply

Quoted 8 lines · Leonard Crestez

On imx6qp-sabresd LDO_ARM is connected to a different PMIC output than the other imx6qdl-sabresd boards.

Setting cpu0 arm-supply to sw2_reg is wrong, this must have mistakenly slipped out of the vendor tree where this is are used for LDO bypass.

Applied, thanks.

#11 lore raw
In reply to Leonard Crestez
Quoted 8 lines · Leonard Crestez

Setting the LDO regulator parent is optional but beneficial. It will cause the PMIC output voltage to be dynamically set to the minimum input for the LDOs, this should be more efficient.

This propagation was introduced by:
commit fc42112c0eaa ("regulator: core: Propagate voltage changes to supply regulators")

Quoted 5 lines

Changes since v1:
* Drop patch 1 since it only avoids logging a warning and the gpc driver
is going through more changes.
* Initialize cpufreq->suspend_freq based on policy->max instead.
* Remove reference to ldo-bypass from suspend_freq patch message.

Hello,

This is a gentle reminder that this was sent ~2 weeks ago and patches 1 and 2 are still waiting (with no objections). Patches 3/4 were applied.

--
Regards,
Leonard