[PATCH 0/5] ARM: imx: Set LDO regulator supply

[PATCH 0/5] ARM: imx: Set LDO regulator supply

[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")

In this case it is desirable that the parent regulator sets the minimum
voltage for it's consumers. It's not clear if this behavior from the
regulator core is entirely intentional. If it's not then perhaps it should
be made configurable through DT somehow?

This makes probe order more complicated, fix that in patches 1 and 2.

It can also break suspend because cpufreq voltage switches can end up
talking to a PMIC via I2C which is already suspended. This is fixed in
patch 3.

It's good to have these issues fixed upstream because they might affect
other complex configurations.

These changes are required for LDO bypass but they are also useful
standalone. Here's a link to the that other discussion:
https://lkml.org/lkml/2017/3/22/640

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

Leonard Crestez (4):
  ARM: imx: gpc: Do not print error message for EPROBE_DEFER
  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 ++--
 arch/arm/mach-imx/gpc.c                |  6 +++++-
 drivers/cpufreq/imx6q-cpufreq.c        |  9 +++++++++
 4 files changed, 28 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH 1/5] ARM: imx: gpc: Do not print error message for EPROBE_DEFER

[Leonard Crestez]

The pu regulator request will return -EPROBE_DEFER if it has a supply
from PMIC and this supply is not yet registered. This does not represent
an error since the driver will call probe again later, so only print a
warning message in this case.

Signed-off-by: Irina Tirdea <irina.tirdea@nxp.com>
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 arch/arm/mach-imx/gpc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
index 1dc2a34..ce64d11 100644
--- a/arch/arm/mach-imx/gpc.c
+++ b/arch/arm/mach-imx/gpc.c
@@ -466,7 +466,11 @@ static int imx_gpc_probe(struct platform_device *pdev)
 		pu_reg = NULL;
 	if (IS_ERR(pu_reg)) {
 		ret = PTR_ERR(pu_reg);
-		dev_err(&pdev->dev, "failed to get pu regulator: %d\n", ret);
+		if (ret == -EPROBE_DEFER)
+			dev_dbg(&pdev->dev, "pu regulator not ready, retry\n");
+		else
+			dev_err(&pdev->dev, "failed to get pu regulator: %d\n",
+					ret);
 		return ret;
 	}
 
-- 
2.7.4

[PATCH 2/5] cpufreq: imx6q: Fix handling EPROBE_DEFER from regulator

[Leonard Crestez]

From: Irina Tirdea <irina.tirdea@nxp.com>

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.

Signed-off-by: Irina Tirdea <irina.tirdea@nxp.com>
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/imx6q-cpufreq.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 7719b02..be90ee3 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -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;
-- 
2.7.4

[PATCH 3/5] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend

[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.

To avoid this scenario we just increase cpufreq to highest setpoint
before suspend. This issue can easily be triggered by ldo-bypass but in
theory any regulator set_voltage call can end up having to modify
external supply voltages.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/imx6q-cpufreq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index be90ee3..e2c1fbf 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -162,6 +162,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
 {
 	policy->clk = arm_clk;
+	policy->suspend_freq = freq_table[soc_opp_count - 1].frequency;
 	return cpufreq_generic_init(policy, freq_table, transition_latency);
 }
 
@@ -173,6 +174,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)
-- 
2.7.4

[PATCH 4/5] ARM: dts: imx6qdl-sabresd: Set LDO regulator supply

[Leonard Crestez]

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

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index 63bf95e..58055ce 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -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";
 };
-- 
2.7.4

[PATCH 5/5] ARM: dts: imx6qp-sabresd: Set reg_arm regulator supply

[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.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 arch/arm/boot/dts/imx6qp-sabresd.dts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qp-sabresd.dts b/arch/arm/boot/dts/imx6qp-sabresd.dts
index b234580..a8a5004 100644
--- a/arch/arm/boot/dts/imx6qp-sabresd.dts
+++ b/arch/arm/boot/dts/imx6qp-sabresd.dts
@@ -50,8 +50,8 @@
 	compatible = "fsl,imx6qp-sabresd", "fsl,imx6qp";
 };
 
-&cpu0 {
-	arm-supply = <&sw2_reg>;
+&reg_arm {
+	vin-supply = <&sw2_reg>;
 };
 
 &iomuxc {
-- 
2.7.4

[PATCH 1/5] ARM: imx: gpc: Do not print error message for EPROBE_DEFER

[Lucas Stach]

Am Freitag, den 31.03.2017, 22:25 +0300 schrieb Leonard Crestez:
> The pu regulator request will return -EPROBE_DEFER if it has a supply
> from PMIC and this supply is not yet registered. This does not represent
> an error since the driver will call probe again later, so only print a
> warning message in this case.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@nxp.com>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  arch/arm/mach-imx/gpc.c | 6 +++++-

The driver moved places, together with a large rewrite, to
drivers/soc/imx/.

This issue isn't present in the new driver, so this patch can just be
dropped.

Regards,
Lucas

>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
> index 1dc2a34..ce64d11 100644
> --- a/arch/arm/mach-imx/gpc.c
> +++ b/arch/arm/mach-imx/gpc.c
> @@ -466,7 +466,11 @@ static int imx_gpc_probe(struct platform_device *pdev)
>  		pu_reg = NULL;
>  	if (IS_ERR(pu_reg)) {
>  		ret = PTR_ERR(pu_reg);
> -		dev_err(&pdev->dev, "failed to get pu regulator: %d\n", ret);
> +		if (ret == -EPROBE_DEFER)
> +			dev_dbg(&pdev->dev, "pu regulator not ready, retry\n");
> +		else
> +			dev_err(&pdev->dev, "failed to get pu regulator: %d\n",
> +					ret);
>  		return ret;
>  	}
>  

[PATCH 2/5] cpufreq: imx6q: Fix handling EPROBE_DEFER from regulator

[Lucas Stach]

Am Freitag, den 31.03.2017, 22:25 +0300 schrieb Leonard Crestez:
> From: Irina Tirdea <irina.tirdea@nxp.com>
> 
> 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.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@nxp.com>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

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

> ---
>  drivers/cpufreq/imx6q-cpufreq.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index 7719b02..be90ee3 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -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;

[PATCH 3/5] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend

[Lucas Stach]

Am Freitag, den 31.03.2017, 22:25 +0300 schrieb 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.
> 
> To avoid this scenario we just increase cpufreq to highest setpoint
> before suspend. This issue can easily be triggered by ldo-bypass but in
> theory any regulator set_voltage call can end up having to modify
> external supply voltages.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/imx6q-cpufreq.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index be90ee3..e2c1fbf 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -162,6 +162,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
>  static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
>  {
>  	policy->clk = arm_clk;
> +	policy->suspend_freq = freq_table[soc_opp_count - 1].frequency;

I think soc_opp_count includes all OPPs from the DT, some of which might
be disabled based on the fuse settings of the SoC. So this should
probably not switch to the highest OPP unconditionally, but rather
switch to the highest _enabled_ OPP.

Regards,
Lucas
 
>  	return cpufreq_generic_init(policy, freq_table, transition_latency);
>  }
>  
> @@ -173,6 +174,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)

[PATCH 4/5] ARM: dts: imx6qdl-sabresd: Set LDO regulator supply

[Lucas Stach]

Am Freitag, den 31.03.2017, 22:25 +0300 schrieb Leonard Crestez:
> Setting the supply is optional but beneficial, it will cause PMIC
> voltages to be dynamically changed with cpu frequency.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

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

> ---
>  arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> index 63bf95e..58055ce 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> @@ -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";
>  };

[PATCH 5/5] ARM: dts: imx6qp-sabresd: Set reg_arm regulator supply

[Lucas Stach]

Am Freitag, den 31.03.2017, 22:25 +0300 schrieb 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.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

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

> ---
>  arch/arm/boot/dts/imx6qp-sabresd.dts | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6qp-sabresd.dts b/arch/arm/boot/dts/imx6qp-sabresd.dts
> index b234580..a8a5004 100644
> --- a/arch/arm/boot/dts/imx6qp-sabresd.dts
> +++ b/arch/arm/boot/dts/imx6qp-sabresd.dts
> @@ -50,8 +50,8 @@
>  	compatible = "fsl,imx6qp-sabresd", "fsl,imx6qp";
>  };
>  
> -&cpu0 {
> -	arm-supply = <&sw2_reg>;
> +&reg_arm {
> +	vin-supply = <&sw2_reg>;
>  };
>  
>  &iomuxc {

[PATCH 1/5] ARM: imx: gpc: Do not print error message for EPROBE_DEFER

[Leonard Crestez]

On Tue, 2017-04-04 at 11:46 +0200, Lucas Stach wrote:
> Am Freitag, den 31.03.2017, 22:25 +0300 schrieb Leonard Crestez:
> > 
> > The pu regulator request will return -EPROBE_DEFER if it has a supply
> > from PMIC and this supply is not yet registered. This does not represent
> > an error since the driver will call probe again later, so only print a
> > warning message in this case.
> > 
> > Signed-off-by: Irina Tirdea <irina.tirdea@nxp.com>
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > ---
> > ?arch/arm/mach-imx/gpc.c | 6 +++++-
> The driver moved places, together with a large rewrite, to
> drivers/soc/imx/.
> 
> This issue isn't present in the new driver, so this patch can just be
> dropped.

Wait, I'm confused. In what tree did that happen? This patch is against
linus's tree as of last week and there is nothing in drivers/soc/imx.

I saw patches for a new gpcv2 driver but that seems to be only for
imx7. This patch is for imx6. Will that driver also support imx6?

Link:?https://lkml.org/lkml/2017/3/28/834

--
Regards,
Leonard

[PATCH 1/5] ARM: imx: gpc: Do not print error message for EPROBE_DEFER

[Lucas Stach]

Am Dienstag, den 04.04.2017, 13:28 +0300 schrieb Leonard Crestez:
> On Tue, 2017-04-04 at 11:46 +0200, Lucas Stach wrote:
> > Am Freitag, den 31.03.2017, 22:25 +0300 schrieb Leonard Crestez:
> > > 
> > > The pu regulator request will return -EPROBE_DEFER if it has a supply
> > > from PMIC and this supply is not yet registered. This does not represent
> > > an error since the driver will call probe again later, so only print a
> > > warning message in this case.
> > > 
> > > Signed-off-by: Irina Tirdea <irina.tirdea@nxp.com>
> > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > > ---
> > >  arch/arm/mach-imx/gpc.c | 6 +++++-
> > The driver moved places, together with a large rewrite, to
> > drivers/soc/imx/.
> > 
> > This issue isn't present in the new driver, so this patch can just be
> > dropped.
> 
> Wait, I'm confused. In what tree did that happen? This patch is against
> linus's tree as of last week and there is nothing in drivers/soc/imx.
> 
> I saw patches for a new gpcv2 driver but that seems to be only for
> imx7. This patch is for imx6. Will that driver also support imx6?

Shawns maintainer tree:

https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/log/?h=imx/drivers

This is what is staged for the next merge window.

Regards,
Lucas

[PATCH 3/5] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend

[Leonard Crestez]

On Tue, 2017-04-04 at 11:51 +0200, Lucas Stach wrote:
> Am Freitag, den 31.03.2017, 22:25 +0300 schrieb 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.
> > 
> > To avoid this scenario we just increase cpufreq to highest setpoint
> > before suspend. This issue can easily be triggered by ldo-bypass but in
> > theory any regulator set_voltage call can end up having to modify
> > external supply voltages.
> > 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > ?drivers/cpufreq/imx6q-cpufreq.c | 2 ++
> > ?1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> > index be90ee3..e2c1fbf 100644
> > --- a/drivers/cpufreq/imx6q-cpufreq.c
> > +++ b/drivers/cpufreq/imx6q-cpufreq.c
> > @@ -162,6 +162,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
> > ?static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
> > ?{
> > ?	policy->clk = arm_clk;
> > +	policy->suspend_freq = freq_table[soc_opp_count - 1].frequency;
> I think soc_opp_count includes all OPPs from the DT, some of which might
> be disabled based on the fuse settings of the SoC. So this should
> probably not switch to the highest OPP unconditionally, but rather
> switch to the highest _enabled_ OPP.

You're right, this does not appear to be correct. Looking at
soc_opp_count it should probably be a local variable in the code
initializing imx6_soc_volt. And the imx6_soc_volt array itself could
now be replaced with opp's support for multiple supply voltages.

I'll post v2.