[RFC 0/8] ARM: imx: Upstream fsl,ldo-bypass

[RFC 0/8] ARM: imx: Upstream fsl,ldo-bypass

[Leonard Crestez]

Freescale imx socs have three built-in regulators LDO_ARM LDO_SOC and LDO_PU
used to control internal chip voltages. "ldo-bypass" mode refers to placing
these regulators in bypass mode and controlling voltages from an external power
management chip instead. The intention is to save power (at the expense of an
extra PMIC on the board).

Looking through commit messages and emails it seems that there are other users
of unusual power configurations for imx but there doesn't appear to be any
board dts file using this mode. This is an attempt to upstream this feature
based on how the freescale BSP does it. This series only enables it for the
imx6qdl-sabresd boards for simplicity.

Versions of u-boot shipped by freescale for imx partially handle this in the
bootloader. His patch series tries to detect if the bootloader did this and
takes advantage of it but it is not required. This series works fine with
upstream u-boot. The binding is odd because it's shared with those versions of
uboot.

The LDO regulators have a minimum dropout of 125mv and this is removed when
bypass mode is entered. This results in a brief increase in voltage. The bypass
code sets the minimum frequency before enabling LDO bypass to avoid
overvolting. Some new checks are added in the regulator core to check for this
(patch 4) and the checks would trigger without lowering the frequency.

Issues that need to be discussed:

* I'm not happy with the regulator_allow_bypass API. It claims to "allow the
regulator to go into bypass mode if all other consumers for the regulator also
enable bypass mode" but it doesn't seem to properly handle the case when
additional consumers show up after bypass mode is enabled. It's not even clear
how it should be done, should regulator_get on a bypassed regulator disable
bypass mode?

There are only 2 other users of this and they don't seem to use multiple
consumers. For imx the gpc driver is a secondary consumer for vddpu so this
needs to be handled somehow. There is a hack in patch 7 which forces gpc to
always probe after cpufreq and rely on it to perform the bypass procedure.

Having both gpc and cpufreq call regulator_allow_bypass would sort of work but
it would be ugly. If gpc probes first it would attempt bypass and get an error
(because of the supply voltage check), bypass would later be performed
successfully by cpufreq.

I think it would be better to have an imperative regulator_set_bypass API. I
think it might even be reasonable for it to completely replace the current
regulator_allow_bypass API.

It would also make the code a lot simpler, there would no longer be a need to
check regulator_is_bypass after regulator_allow_bypass.

* There is an anatop patch which dynamically modifies regulator_desc to
zero-out "min_dropout_uV" in bypass mode, despite the fact that documentation
claims that regulator_desc should be read-only. Perhaps the core should handle
this or this should be replaced with a function in regulator_ops?

Irina Tirdea (2):
  cpufreq: imx6q: Fix handling EPROBE_DEFER from regulator
  regulator: anatop: fix min dropout for bypass mode

Leonard Crestez (6):
  ARM: imx: gpc: Do not print error message for EPROBE_DEFER
  cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend
  regulator: core: Check enabling bypass respects constraints
  regulator: core: Add regulator_is_bypass function
  cpufreq: imx6q: Initialize LDO bypass
  ARM: dts: imx6qdl-sabresd: Enable fsl,ldo-bypass

 .../devicetree/bindings/power/fsl,imx-gpc.txt      |   4 +
 arch/arm/boot/dts/imx6dl-sabresd-ldo.dts           |  13 ++
 arch/arm/boot/dts/imx6q-sabresd-ldo.dts            |  13 ++
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi             |  19 +++
 arch/arm/boot/dts/imx6qdl.dtsi                     |   6 +-
 arch/arm/boot/dts/imx6qp-sabresd-ldo.dts           |  13 ++
 arch/arm/boot/dts/imx6qp-sabresd.dts               |   4 +-
 arch/arm/mach-imx/gpc.c                            |  13 +-
 drivers/cpufreq/imx6q-cpufreq.c                    | 180 +++++++++++++++++++++
 drivers/regulator/anatop-regulator.c               |   9 +-
 drivers/regulator/core.c                           |  78 ++++++++-
 include/linux/regulator/consumer.h                 |   1 +
 12 files changed, 344 insertions(+), 9 deletions(-)
 create mode 100644 arch/arm/boot/dts/imx6dl-sabresd-ldo.dts
 create mode 100644 arch/arm/boot/dts/imx6q-sabresd-ldo.dts
 create mode 100644 arch/arm/boot/dts/imx6qp-sabresd-ldo.dts

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 1/8] 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

[RFC 2/8] 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>
---
 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

Re: [RFC 2/8] cpufreq: imx6q: Fix handling EPROBE_DEFER from regulator

[Viresh Kumar]

On 22-03-17, 18:53, Leonard Crestez wrote:
> From: Irina Tirdea <irina.tirdea-3arQi8VN3Tc@public.gmane.org>
> 
> 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-3arQi8VN3Tc@public.gmane.org>
> Signed-off-by: Leonard Crestez <leonard.crestez-3arQi8VN3Tc@public.gmane.org>
> ---
>  drivers/cpufreq/imx6q-cpufreq.c | 7 +++++++
>  1 file changed, 7 insertions(+)

Acked-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 3/8] 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>
---
 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

Re: [RFC 3/8] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend

[Viresh Kumar]

On 22-03-17, 18:53, Leonard Crestez wrote:
> 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>
> ---
>  drivers/cpufreq/imx6q-cpufreq.c | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [RFC 3/8] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend

[Leonard Crestez]

On Thu, 2017-03-23 at 10:04 +0530, Viresh Kumar wrote:
> On 22-03-17, 18:53, Leonard Crestez wrote:
> > 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-3arQi8VN3Tc@public.gmane.org>
> > ---
> >  drivers/cpufreq/imx6q-cpufreq.c | 2 ++
> >  1 file changed, 2 insertions(+)
> 
> Acked-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

The first couple of patches are obvious fixes despite being marked as
RFC. It would be great if you could apply them to your tree separately
from the rest of the series but I'm not sure what the process is here.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 3/8] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend

[Rafael J. Wysocki]

On Tuesday, March 28, 2017 11:03:51 PM Leonard Crestez wrote:
> On Thu, 2017-03-23 at 10:04 +0530, Viresh Kumar wrote:
> > On 22-03-17, 18:53, Leonard Crestez wrote:
> > > 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>
> > > ---
> > >  drivers/cpufreq/imx6q-cpufreq.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > 
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> The first couple of patches are obvious fixes despite being marked as
> RFC. It would be great if you could apply them to your tree separately

Why?

> from the rest of the series but I'm not sure what the process is here.

Well, you have to talk to me.

Thanks,
Rafael

Re: [RFC 3/8] cpufreq: imx6q: Set max suspend_freq to avoid changes during suspend

[Rafael J. Wysocki]

On Tuesday, March 28, 2017 10:50:50 PM Rafael J. Wysocki wrote:
> On Tuesday, March 28, 2017 11:03:51 PM Leonard Crestez wrote:
> > On Thu, 2017-03-23 at 10:04 +0530, Viresh Kumar wrote:
> > > On 22-03-17, 18:53, Leonard Crestez wrote:
> > > > 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>
> > > > ---
> > > >  drivers/cpufreq/imx6q-cpufreq.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > 
> > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > 
> > The first couple of patches are obvious fixes despite being marked as
> > RFC. It would be great if you could apply them to your tree separately
> 
> Why?
> 
> > from the rest of the series but I'm not sure what the process is here.
> 
> Well, you have to talk to me.

OK, so if I understand this correctly, you would like the patches ACKed by
Viresh to be applied regardless of what happens to the rest of the series,
right?

In that case please resend them separately without the [RFC] tag and with
the ACKs from Viresh.

Thanks,
Rafael

[RFC 4/8] regulator: core: Check enabling bypass respects constraints

[Leonard Crestez]

Enabling bypass mode makes a regulator passthrough the supply voltage
directly. It is possible that the supply voltage is set high enough that
it violates machine constraints so let's check for that.

The supply voltage might be higher because of min_dropout_uV or maybe
there is just an unrelated consumer who requested a higher voltage.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/regulator/core.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 53d4fc7..9d893aa 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3453,6 +3453,54 @@ int regulator_set_load(struct regulator *regulator, int uA_load)
 }
 EXPORT_SYMBOL_GPL(regulator_set_load);
 
+static int _regulator_set_bypass(struct regulator *regulator, bool bypass)
+{
+	struct regulator_dev *rdev = regulator->rdev;
+	int output_voltage;
+	int supply_voltage;
+
+	if (bypass && !rdev->supply) {
+		rdev_err(rdev, "Refuse to set bypass on regulator with no supply!\n");
+		return -EINVAL;
+	}
+
+	/* Check that enabling bypass won't break constraints */
+	if (bypass && _regulator_is_enabled(rdev)) {
+		output_voltage = _regulator_get_voltage(rdev);
+		if (output_voltage < 0) {
+			rdev_err(rdev, "Failed to get old output voltage before"
+					" enabling bypass: %d\n", output_voltage);
+			return output_voltage;
+		}
+		supply_voltage = _regulator_get_voltage(rdev->supply->rdev);
+		if (supply_voltage < 0) {
+			rdev_err(rdev, "Failed to get supply voltage before"
+					" enabling bypass: %d\n", supply_voltage);
+			return supply_voltage;
+		}
+		if (supply_voltage < rdev->constraints->min_uV ||
+				supply_voltage > rdev->constraints->max_uV) {
+			rdev_err(rdev, "Enabling bypass would change voltage"
+					" from %duV to %duV violating"
+					" constraint range %duV to %duV\n",
+				output_voltage,
+				supply_voltage,
+				rdev->constraints->min_uV,
+				rdev->constraints->max_uV);
+			return -EINVAL;
+		}
+		rdev_dbg(rdev, "Enabling bypass would change voltage"
+				" from %duV to %duV respecting"
+				" constraint range %duV to %duV\n",
+			output_voltage,
+			supply_voltage,
+			rdev->constraints->min_uV,
+			rdev->constraints->max_uV);
+	}
+
+	return rdev->desc->ops->set_bypass(rdev, bypass);
+}
+
 /**
  * regulator_allow_bypass - allow the regulator to go into bypass mode
  *
@@ -3481,7 +3529,7 @@ int regulator_allow_bypass(struct regulator *regulator, bool enable)
 		rdev->bypass_count++;
 
 		if (rdev->bypass_count == rdev->open_count) {
-			ret = rdev->desc->ops->set_bypass(rdev, enable);
+			ret = _regulator_set_bypass(regulator, enable);
 			if (ret != 0)
 				rdev->bypass_count--;
 		}
@@ -3490,7 +3538,7 @@ int regulator_allow_bypass(struct regulator *regulator, bool enable)
 		rdev->bypass_count--;
 
 		if (rdev->bypass_count != rdev->open_count) {
-			ret = rdev->desc->ops->set_bypass(rdev, enable);
+			ret = _regulator_set_bypass(regulator, enable);
 			if (ret != 0)
 				rdev->bypass_count++;
 		}
-- 
2.7.4

Re: [RFC 4/8] regulator: core: Check enabling bypass respects constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 913 bytes --]

On Wed, Mar 22, 2017 at 06:53:06PM +0200, Leonard Crestez wrote:

> Enabling bypass mode makes a regulator passthrough the supply voltage
> directly. It is possible that the supply voltage is set high enough that
> it violates machine constraints so let's check for that.
> 
> The supply voltage might be higher because of min_dropout_uV or maybe
> there is just an unrelated consumer who requested a higher voltage.

I would expect that if bypass is enabled then the constraints on the
parent regulator would be set appropriately to support this, I wouldn't
expect that we'd try to apply the operating constraints of the regulator
to the supply.  Usually bypass is used for low power retention modes
with different settings to those used in normal operation that wouldn't
be desired in normal operation, if we were going to have constraints for
this I'd expect a separate set used during bypass.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 4/8] regulator: core: Check enabling bypass respects constraints

[Leonard Crestez]

On Fri, 2017-03-24 at 12:52 +0000, Mark Brown wrote:
> On Wed, Mar 22, 2017 at 06:53:06PM +0200, Leonard Crestez wrote:
> 
> > 
> > Enabling bypass mode makes a regulator passthrough the supply voltage
> > directly. It is possible that the supply voltage is set high enough that
> > it violates machine constraints so let's check for that.
> > 
> > The supply voltage might be higher because of min_dropout_uV or maybe
> > there is just an unrelated consumer who requested a higher voltage.
> I would expect that if bypass is enabled then the constraints on the
> parent regulator would be set appropriately to support this, I wouldn't
> expect that we'd try to apply the operating constraints of the regulator
> to the supply.  Usually bypass is used for low power retention modes
> with different settings to those used in normal operation that wouldn't
> be desired in normal operation, if we were going to have constraints for
> this I'd expect a separate set used during bypass.

In this particular case it's not possible to set constraints on the parent
regulator so that both ldo-enable and ldo-bypass modes work. The maximum allowed
voltage for ldo-bypass is lower than the minimum required to support the chip at
max frequency wit ldo-enable.

It would be possible to also change the constraint values on the PMIC together
with ldo-bypass in the .dts files but that seems awful.

I'm not sure I understand why you are against applying constraints to the parent
when in bypass mode, it seems like the obvious thing to do if you want to
support flexible configuration. The check I introduced is probably not enough to
cover all cases, for example it would still be possible to explicitly change
parent voltage afterwards.

A regulator_dev registers a consumer for the supply. Right now this is being
used to propagate minimum voltages upwards since commit fc42112c0eaa ("Propagate
voltage changes to supply regulators"). It seems to me like it would be
reasonable to also use it to propagate maximum voltage from constraints, right?

Instead of asking for [best_uV + min_dropout_uV, INT_MAX] it could instead ask
for [min_uV + bypass ? min_dropout_uV : 0, min(max_uV, constraints->max_uV)].
The _regulator_do_set_voltage call on the supply can deal with stuff like
mapping it to a selector, just like it does for regulator consumers.

If more elaborate constraints are required instead of this simple behavior it
could be handled by adding an interface for drivers to expose explicit dynamic
min/max constraints.

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC 4/8] regulator: core: Check enabling bypass respects constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1852 bytes --]

On Tue, Mar 28, 2017 at 03:39:41PM +0300, Leonard Crestez wrote:
> On Fri, 2017-03-24 at 12:52 +0000, Mark Brown wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> > to the supply.  Usually bypass is used for low power retention modes
> > with different settings to those used in normal operation that wouldn't
> > be desired in normal operation, if we were going to have constraints for
> > this I'd expect a separate set used during bypass.

> In this particular case it's not possible to set constraints on the parent
> regulator so that both ldo-enable and ldo-bypass modes work. The maximum allowed
> voltage for ldo-bypass is lower than the minimum required to support the chip at
> max frequency wit ldo-enable.

If things are really so sensitive that you can't bypass without lowering
the voltage then it's hard to see how you can safely transition into and
out of bypass mode.  

> I'm not sure I understand why you are against applying constraints to the parent
> when in bypass mode, it seems like the obvious thing to do if you want to
> support flexible configuration. The check I introduced is probably not enough to
> cover all cases, for example it would still be possible to explicitly change
> parent voltage afterwards.

To repeat what I said previously the whole point of bypassing is to not
do regulation and generally the constraints in the unregulated idle case
are substantially more relaxed.  This would break use cases relying on
the existing behaviour which wouldn't expect to affect the parent
voltage at all, either stopping things working or making them less
efficient by needlessly regulating the voltage down which defeats the
main point of bypassing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 4/8] regulator: core: Check enabling bypass respects constraints

[Leonard Crestez]

On Tue, 2017-03-28 at 17:47 +0100, Mark Brown wrote:
> On Tue, Mar 28, 2017 at 03:39:41PM +0300, Leonard Crestez wrote:
> > On Fri, 2017-03-24 at 12:52 +0000, Mark Brown wrote:

> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns.  Doing this makes your messages much
> easier to read and reply to.

Sorry, still messing around with Evolution and Exchange.

> > > to the supply.  Usually bypass is used for low power retention modes
> > > with different settings to those used in normal operation that wouldn't
> > > be desired in normal operation, if we were going to have constraints for
> > > this I'd expect a separate set used during bypass.
> > 
> > In this particular case it's not possible to set constraints on the parent
> > regulator so that both ldo-enable and ldo-bypass modes work. The maximum allowed
> > voltage for ldo-bypass is lower than the minimum required to support the chip at
> > max frequency wit ldo-enable.
> 
> If things are really so sensitive that you can't bypass without lowering
> the voltage then it's hard to see how you can safely transition into and
> out of bypass mode.

The CPU frequency is set to the minimum value so that when bypass mode
is entered and voltage rises (because the dropout goes away) it is
still low enough. Transitioning out of bypass mode is not implemented
but you would presumably have to go to the minimum frequency again,
raise the voltage above what is required and the flip the switch.

> > I'm not sure I understand why you are against applying constraints to the parent
> > when in bypass mode, it seems like the obvious thing to do if you want to
> > support flexible configuration. The check I introduced is probably not enough to
> > cover all cases, for example it would still be possible to explicitly change
> > parent voltage afterwards.
> 
> To repeat what I said previously the whole point of bypassing is to not
> do regulation and generally the constraints in the unregulated idle case
> are substantially more relaxed.  This would break use cases relying on
> the existing behaviour which wouldn't expect to affect the parent
> voltage at all, either stopping things working or making them less
> efficient by needlessly regulating the voltage down which defeats the
> main point of bypassing.

So what you want is to prevent voltage changes unless strictly
required, even lowering? What I want is to get the minimum voltage in
the SOC because that's where power is being consumed.

It's not at all obvious that in bypass mode the output constraints of a
regulator need not be respected by the core. I expected the opposite,
this is something that should be documented.

But if the bypassed regulator has a downstream consumer then it's
requirements should definitely still be met in bypass mode, right? I
could set my maximum voltage directly from cpufreq in that case.

Or should a bypassed regulator ignore all other requests? One of the
behaviors that this patch series relies on is that calling set_voltage
on a bypassed regulator propagates this request to the supply and picks
the minimum voltage there. An alternative implementation would be to
call set_voltage directly on the supply regulator by changing the
"{arm,soc,pu}-supply" references in DT to point to the PMIC instead.
Would that be better?

Both approaches work. Relying on propagation feels like it is the
"right way" to handle this, even if it's harder to get right and the
regulator core does not entirely support it. But it's possible that
this is based on a misunderstanding of what "bypass" is actually
supposed to do.

-- 
Regards,
Leonard

Re: [RFC 4/8] regulator: core: Check enabling bypass respects constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3543 bytes --]

On Tue, Mar 28, 2017 at 10:49:55PM +0300, Leonard Crestez wrote:
> On Tue, 2017-03-28 at 17:47 +0100, Mark Brown wrote:

> > To repeat what I said previously the whole point of bypassing is to not
> > do regulation and generally the constraints in the unregulated idle case
> > are substantially more relaxed.  This would break use cases relying on
> > the existing behaviour which wouldn't expect to affect the parent
> > voltage at all, either stopping things working or making them less
> > efficient by needlessly regulating the voltage down which defeats the
> > main point of bypassing.

> So what you want is to prevent voltage changes unless strictly
> required, even lowering? What I want is to get the minimum voltage in
> the SOC because that's where power is being consumed.

So your end goal here is to bypass a regulator which is forced into your
system design by being integrated into the SoC which isn't able to
regulate down to a low enough voltage for your use case?  I guess one
question is if you need to use the regulator at all?

> It's not at all obvious that in bypass mode the output constraints of a
> regulator need not be respected by the core. I expected the opposite,
> this is something that should be documented.

SubmittingPatches...  Bear in mind that most regulators are fixed
voltage in a given system so bypass would be very difficult to use if it
tried to pass the constraints upstream.

> But if the bypassed regulator has a downstream consumer then it's
> requirements should definitely still be met in bypass mode, right? I
> could set my maximum voltage directly from cpufreq in that case.

What we're interpreting bypass mode as meaning is "stop regulating".
There will still be some limits but we're expecting them to be enforced
in the extremes of the constraints in the parent regulators.

> Or should a bypassed regulator ignore all other requests? One of the
> behaviors that this patch series relies on is that calling set_voltage
> on a bypassed regulator propagates this request to the supply and picks
> the minimum voltage there. An alternative implementation would be to

Yes, the expectation is that if anything is being changed it won't have
any effect until regulation is reenabled but we're not particularly
expecting much activity on bypassed regulators.

> call set_voltage directly on the supply regulator by changing the
> "{arm,soc,pu}-supply" references in DT to point to the PMIC instead.
> Would that be better?

That's more what's expected here.

> Both approaches work. Relying on propagation feels like it is the
> "right way" to handle this, even if it's harder to get right and the
> regulator core does not entirely support it. But it's possible that
> this is based on a misunderstanding of what "bypass" is actually
> supposed to do.

Another option would be to add a regulator configuration which allowed
the regulator to transparently go into bypass mode if the parent could
do things directly, only enabling regulation if the parent couldn't
support.  That would mean you'd loose the supply cleanup function which
is typically part of why there are LDOs in the system but it sounds like
you're OK with that in at least your use case.

We could also perhaps have a way to set the regulator into permanent
bypass mode from the constraints for use cases that just never need it
and then remap the supply relationship at probe time (which avoids any
special runtime casing) for something even simpler.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 4/8] regulator: core: Check enabling bypass respects constraints

[Leonard Crestez]

On Thu, 2017-04-06 at 19:52 +0100, Mark Brown wrote:
> On Tue, Mar 28, 2017 at 10:49:55PM +0300, Leonard Crestez wrote:
> > On Tue, 2017-03-28 at 17:47 +0100, Mark Brown wrote:

> > > To repeat what I said previously the whole point of bypassing is to not
> > > do regulation and generally the constraints in the unregulated idle case
> > > are substantially more relaxed.  This would break use cases relying on
> > > the existing behaviour which wouldn't expect to affect the parent
> > > voltage at all, either stopping things working or making them less
> > > efficient by needlessly regulating the voltage down which defeats the
> > > main point of bypassing.

> > So what you want is to prevent voltage changes unless strictly
> > required, even lowering? What I want is to get the minimum voltage in
> > the SOC because that's where power is being consumed.

> So your end goal here is to bypass a regulator which is forced into your
> system design by being integrated into the SoC which isn't able to
> regulate down to a low enough voltage for your use case?  I guess one
> question is if you need to use the regulator at all?

Both the PMIC and the LDOs can provide the required voltage range. LDO
bypass mode is a design tradeoff: the PMIC provides more efficient
conversion but with more fluctuations.

My question was about how the regulator framework handles constraints
and consumer voltage requests. What I want is for the output to be set
to the minimum value that meets all constraints. Maybe other users
would prefer regulators to keep their output as much as possible?

This is relevant even if LDOs are enabled, it's still preferable to
have PMIC output as low as possible because then less of the voltage
reduction is performed in the LDO.

It currently seems to work how I expect but from your statement it's
not clear if it's entirely intentional.

> > It's not at all obvious that in bypass mode the output constraints of a
> > regulator need not be respected by the core. I expected the opposite,
> > this is something that should be documented.

> SubmittingPatches...  Bear in mind that most regulators are fixed
> voltage in a given system so bypass would be very difficult to use if it
> tried to pass the constraints upstream.

> > But if the bypassed regulator has a downstream consumer then it's
> > requirements should definitely still be met in bypass mode, right? I
> > could set my maximum voltage directly from cpufreq in that case.

> What we're interpreting bypass mode as meaning is "stop regulating".
> There will still be some limits but we're expecting them to be enforced
> in the extremes of the constraints in the parent regulators.

> > Or should a bypassed regulator ignore all other requests? One of the
> > behaviors that this patch series relies on is that calling set_voltage
> > on a bypassed regulator propagates this request to the supply and picks
> > the minimum voltage there. An alternative implementation would be to

> Yes, the expectation is that if anything is being changed it won't have
> any effect until regulation is reenabled but we're not particularly
> expecting much activity on bypassed regulators.

OK, so it is up to consumers to ensure the supply voltage is acceptable
before allowing bypass. If they want to do something clever they should
register as a consumer to the supply as well.

> > 
> > call set_voltage directly on the supply regulator by changing the
> > "{arm,soc,pu}-supply" references in DT to point to the PMIC instead.
> > Would that be better?

> That's more what's expected here.

Ok, I will try that approach instead.

> > Both approaches work. Relying on propagation feels like it is the
> > "right way" to handle this, even if it's harder to get right and the
> > regulator core does not entirely support it. But it's possible that
> > this is based on a misunderstanding of what "bypass" is actually
> > supposed to do.

> Another option would be to add a regulator configuration which allowed
> the regulator to transparently go into bypass mode if the parent could
> do things directly, only enabling regulation if the parent couldn't
> support.  That would mean you'd loose the supply cleanup function which
> is typically part of why there are LDOs in the system but it sounds like
> you're OK with that in at least your use case.

Dynamic enabling of bypass mode in the core seems very complex and not
terribly useful for my case. I pretty much want them always in bypass
or always enabled.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 4/8] regulator: core: Check enabling bypass respects constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1156 bytes --]

On Fri, Apr 07, 2017 at 01:51:52PM +0300, Leonard Crestez wrote:

> It currently seems to work how I expect but from your statement it's
> not clear if it's entirely intentional.

The current behaviour of bypassed regulators is intentional.

> > > Or should a bypassed regulator ignore all other requests? One of the
> > > behaviors that this patch series relies on is that calling set_voltage
> > > on a bypassed regulator propagates this request to the supply and picks
> > > the minimum voltage there. An alternative implementation would be to

> > Yes, the expectation is that if anything is being changed it won't have
> > any effect until regulation is reenabled but we're not particularly
> > expecting much activity on bypassed regulators.

> OK, so it is up to consumers to ensure the supply voltage is acceptable
> before allowing bypass. If they want to do something clever they should
> register as a consumer to the supply as well.

No, that's not expected either - one or the other is fine but both isn't
expected.  What you're looking for here is something logically different
to bypass even if the hardware implementation ends up the same.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 4/8] regulator: core: Check enabling bypass respects constraints

[Leonard Crestez]

On Fri, 2017-04-07 at 12:22 +0100, Mark Brown wrote:
> On Fri, Apr 07, 2017 at 01:51:52PM +0300, Leonard Crestez wrote:

> > It currently seems to work how I expect but from your statement it's
> > not clear if it's entirely intentional.

> The current behaviour of bypassed regulators is intentional.

I did not mean to imply that there is something wrong with bypassed
regulators. I just wanted more information about how regulators (non-
bypassed) pick their voltage when consumers allow a range.

After some more reading through the code it seems that the driver
itself receives the range (either through set_voltage or map_voltage)
and gets to make the choice.

So it seems fine for my concerns, sorry to bother you.

--
Regards,
Leonard
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 5/8] regulator: anatop: fix min dropout for bypass mode

[Leonard Crestez]

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

In bypass mode, the anatop digital regulators do not have any minimum
dropout value (the input voltage is equal to the output voltage according
to documentation).

Having a min dropout value of 125mV will lead to an increased voltage
for PMIC supplies.

Only set minimum dropout value for ldo enabled mode.

Signed-off-by: Irina Tirdea <irina.tirdea@nxp.com>
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/regulator/anatop-regulator.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index b041f27..64554e8 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -38,6 +38,8 @@
 #define LDO_POWER_GATE			0x00
 #define LDO_FET_FULL_ON			0x1f
 
+#define LDO_MIN_DROPOUT_UV		125000
+
 struct anatop_regulator {
 	const char *name;
 	u32 control_reg;
@@ -153,6 +155,10 @@ static int anatop_regmap_set_bypass(struct regulator_dev *reg, bool enable)
 
 	sel = enable ? LDO_FET_FULL_ON : anatop_reg->sel;
 	anatop_reg->bypass = enable;
+	if (anatop_reg->bypass)
+		anatop_reg->rdesc.min_dropout_uV = 0;
+	else
+		anatop_reg->rdesc.min_dropout_uV = LDO_MIN_DROPOUT_UV;
 
 	return regulator_set_voltage_sel_regmap(reg, sel);
 }
@@ -264,7 +270,7 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 	rdesc->vsel_reg = sreg->control_reg;
 	rdesc->vsel_mask = ((1 << sreg->vol_bit_width) - 1) <<
 			   sreg->vol_bit_shift;
-	rdesc->min_dropout_uV = 125000;
+	rdesc->min_dropout_uV = LDO_MIN_DROPOUT_UV;
 
 	config.dev = &pdev->dev;
 	config.init_data = initdata;
@@ -286,6 +292,7 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 		if (sreg->sel == LDO_FET_FULL_ON) {
 			sreg->sel = 0;
 			sreg->bypass = true;
+			rdesc->min_dropout_uV = 0;
 		}
 
 		/*
-- 
2.7.4

Re: [RFC 5/8] regulator: anatop: fix min dropout for bypass mode

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 661 bytes --]

On Wed, Mar 22, 2017 at 06:53:07PM +0200, Leonard Crestez wrote:

> +	if (anatop_reg->bypass)
> +		anatop_reg->rdesc.min_dropout_uV = 0;
> +	else
> +		anatop_reg->rdesc.min_dropout_uV = LDO_MIN_DROPOUT_UV;

No, this is completely broken - you can't expect to randomly change hthe
regulator description at runtime behind the back of the framework and
expect things to work.  If there is a need to do this we need an
interface for getting the current value and a way to notify of changes.

That said I would not expect the dropout voltage to be considered at
all when the regulator is bypassed, since the regulator is not
regulating it doesn't need any headroom.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC 5/8] regulator: anatop: fix min dropout for bypass mode

[Leonard Crestez]

On Fri, 2017-03-24 at 12:54 +0000, Mark Brown wrote:
> On Wed, Mar 22, 2017 at 06:53:07PM +0200, Leonard Crestez wrote:
> > +	if (anatop_reg->bypass)
> > +		anatop_reg->rdesc.min_dropout_uV = 0;
> > +	else
> > +		anatop_reg->rdesc.min_dropout_uV = LDO_MIN_DROPOUT_UV;
> No, this is completely broken - you can't expect to randomly change hthe
> regulator description at runtime behind the back of the framework and
> expect things to work.  If there is a need to do this we need an
> interface for getting the current value and a way to notify of changes.
> 
> That said I would not expect the dropout voltage to be considered at
> all when the regulator is bypassed, since the regulator is not
> regulating it doesn't need any headroom.

It's a more complex solution but this could be handled in the core instead.
Basically the core would treat min_dropout_uV as zero if the regulator is
currently in bypass mode.

In theory a function could be added in regulator_ops to ask a regulator driver
what requirements it has for its supply but this does not seem necessary.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC 6/8] regulator: core: Add regulator_is_bypass function

[Leonard Crestez]

This is a simple kernel API to query the bypass state of a regulator.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/regulator/core.c           | 26 ++++++++++++++++++++++++++
 include/linux/regulator/consumer.h |  1 +
 2 files changed, 27 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9d893aa..7d4f59e 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3554,6 +3554,32 @@ int regulator_allow_bypass(struct regulator *regulator, bool enable)
 EXPORT_SYMBOL_GPL(regulator_allow_bypass);
 
 /**
+ * regulator_is_bypass - Determine if the regulator is in bypass mode
+ *
+ * @regulator: Regulator to examine
+ *
+ * @return 0 or 1 for true/false or errno on failure
+ *
+ * Returns zero on a regulator without bypass support.
+ */
+int regulator_is_bypass(struct regulator *regulator)
+{
+	struct regulator_dev *rdev = regulator->rdev;
+	bool bypass;
+	int ret = 0;
+
+	if (!rdev->desc->ops->get_bypass)
+		return 0;
+
+	mutex_lock(&rdev->mutex);
+	ret = rdev->desc->ops->get_bypass(rdev, &bypass);
+	mutex_unlock(&rdev->mutex);
+
+	return ret ? ret : !!bypass;
+}
+EXPORT_SYMBOL_GPL(regulator_is_bypass);
+
+/**
  * regulator_register_notifier - register regulator event notifier
  * @regulator: regulator source
  * @nb: notifier block
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index ea0fffa..ba78511 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -261,6 +261,7 @@ int regulator_get_error_flags(struct regulator *regulator,
 int regulator_set_load(struct regulator *regulator, int load_uA);
 
 int regulator_allow_bypass(struct regulator *regulator, bool allow);
+int regulator_is_bypass(struct regulator *regulator);
 
 struct regmap *regulator_get_regmap(struct regulator *regulator);
 int regulator_get_hardware_vsel_register(struct regulator *regulator,
-- 
2.7.4

Re: [RFC 6/8] regulator: core: Add regulator_is_bypass function

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 206 bytes --]

On Wed, Mar 22, 2017 at 06:53:08PM +0200, Leonard Crestez wrote:

>  /**
> + * regulator_is_bypass - Determine if the regulator is in bypass mode

Bypass is a verb so this should be regulator_is_bypassed()

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 6/8] regulator: core: Add regulator_is_bypass function

[Leonard Crestez]

On Fri, 2017-03-24 at 12:55 +0000, Mark Brown wrote:
> On Wed, Mar 22, 2017 at 06:53:08PM +0200, Leonard Crestez wrote:
> >  /**
> > + * regulator_is_bypass - Determine if the regulator is in bypass mode
> Bypass is a verb so this should be regulator_is_bypassed()

Very well, I will change this.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC 7/8] cpufreq: imx6q: Initialize LDO bypass

[Leonard Crestez]

Several imx6* socs have three built-in regulators LDO_ARM LDO_SOC and
LDO_PU used to control internal chip voltages. "ldo-bypass" mode refers
to placing these regulators in bypass mode and controlling voltages from
an external power management chip instead. This is intended to save
power at the expense of an extra PMIC on the board.

The voltages for these regulators are controlled from the imxq6 cpufreq
driver so it makes sense to also control LDO bypass from here. The gpc
driver also fetches a reference to LDO_PU and uses it to gate power to
graphics blocks.

The LDO regulator has a minimum dropout voltage of 125mv so enabling
bypass mode will raise the effective voltage by that amount. We need set
the minimum frequency first to avoid overvolting when enabling LDO
bypass.

The binding is an u32 fsl,ldo-bypass in the gpc node because that's how
it was defined in the freescale vendor tree for a long time and
compatibility is desirable. Otherwise it would be a bool.

Some versions of u-boot shipped by freescale check this same property
and set regulators in bypass mode before linux actually starts so we
check for that scenario as well and finish early.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 .../devicetree/bindings/power/fsl,imx-gpc.txt      |   4 +
 arch/arm/mach-imx/gpc.c                            |   7 +
 drivers/cpufreq/imx6q-cpufreq.c                    | 171 +++++++++++++++++++++
 3 files changed, 182 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt b/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
index 65cc034..8a7584b 100644
--- a/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
+++ b/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
@@ -11,6 +11,10 @@ Required properties:
   datasheet
 - interrupts: Should contain GPC interrupt request 1
 - pu-supply: Link to the LDO regulator powering the PU power domain
+- fsl,ldo-bypass: Should be 0 or 1 to enable LDO bypass mode (default 0).
+  This is performed in cooperation with cpufreq. Some versions of uboot will
+  also look at this property and set the arm and soc regulators in bypass mode
+  before linux.
 - clocks: Clock phandles to devices in the PU power domain that need
 	  to be enabled during domain power-up for reset propagation.
 - #power-domain-cells: Should be 1, see below:
diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
index ce64d11..62a2555 100644
--- a/arch/arm/mach-imx/gpc.c
+++ b/arch/arm/mach-imx/gpc.c
@@ -22,6 +22,7 @@
 #include <linux/pm_domain.h>
 #include <linux/regulator/consumer.h>
 #include <linux/irqchip/arm-gic.h>
+#include <linux/cpufreq.h>
 #include "common.h"
 #include "hardware.h"
 
@@ -461,6 +462,12 @@ static int imx_gpc_probe(struct platform_device *pdev)
 	if (!of_property_read_bool(pdev->dev.of_node, "#power-domain-cells"))
 		return 0;
 
+	/* wait for cpufreq to initialize before using pu_reg */
+	if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ) && cpufreq_get_current_driver() == NULL) {
+		dev_dbg(&pdev->dev, "cpufreq driver not ready, retry\n");
+		return -EPROBE_DEFER;
+	}
+
 	pu_reg = devm_regulator_get_optional(&pdev->dev, "pu");
 	if (PTR_ERR(pu_reg) == -ENODEV)
 		pu_reg = NULL;
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index e2c1fbf..a0c11ed 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -159,8 +159,179 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 	return 0;
 }
 
+/*
+ * Enable ldo-bypass mode if configured and not already performed by u-boot
+ */
+static int imx6q_cpufreq_init_ldo_bypass(void)
+{
+	struct device_node *gpc_node;
+	unsigned long old_freq_hz;
+	int i, old_freq_index;
+	u32 bypass = 0;
+	int ret = 0, ret2;
+
+	/* Read ldo-bypass property */
+	gpc_node = of_find_compatible_node(NULL, NULL, "fsl,imx6q-gpc");
+	if (!gpc_node)
+		return 0;
+	ret = of_property_read_u32(gpc_node, "fsl,ldo-bypass", &bypass);
+	if (ret && ret != -EINVAL)
+		dev_warn(cpu_dev, "failed reading fsl,ldo-bypass property: %d\n", ret);
+	if (!bypass)
+		goto out;
+
+	/*
+	 * Freescale u-boot handles ldo-bypass by setting
+	 * arm/soc in bypass and vddpu disabled.
+	 *
+	 * If this is the case we don't need any special freq lowering.
+	 */
+	if (regulator_is_bypass(arm_reg) == 1 &&
+		regulator_is_bypass(soc_reg) == 1)
+	{
+		dev_info(cpu_dev, "vddarm and vddsoc already in bypass\n");
+		if (IS_ERR(pu_reg)) {
+			ret = 0;
+			goto out;
+		} else if (regulator_is_enabled(pu_reg) == 0) {
+			ret = regulator_allow_bypass(pu_reg, true);
+			if (ret) {
+				dev_err(cpu_dev, "failed to enable vddpu bypass: %d\n", ret);
+				goto out;
+			}
+			ret = regulator_is_bypass(pu_reg);
+			if (ret != 1) {
+				ret = -EINVAL;
+				dev_err(cpu_dev, "failed bypass check for vddpu: %d\n", ret);
+				goto out;
+			}
+			ret = 0;
+			goto out;
+		} else if (regulator_is_bypass(pu_reg) == 1) {
+			dev_info(cpu_dev, "vddpu also already in bypass\n");
+			ret = 0;
+			goto out;
+		} else
+			dev_info(cpu_dev, "Need frequency lowering to set vddpu in bypass\n");
+	}
+
+	/*
+	 * Enable LDO bypass from kernel.
+	 *
+	 * The LDO regulator has a minimum dropout voltage of 125mv so enabling
+	 * bypass mode will raise the effective voltage by that amount.
+	 *
+	 * We set the minimum frequency first to avoid overvolting.
+	 */
+
+	/* Find current freq so we can restore it. */
+	old_freq_hz = clk_get_rate(arm_clk);
+	if (!old_freq_hz) {
+		dev_err(cpu_dev, "failed to determine current arm freq\n");
+		goto out;
+	}
+	old_freq_index = 0;
+	for (i = 1; i < soc_opp_count; ++i) {
+		if (abs(freq_table[old_freq_index].frequency - old_freq_hz) >
+			abs(freq_table[i].frequency - old_freq_hz)) {
+			old_freq_index = i;
+		}
+	}
+	dev_dbg(cpu_dev, "current freq %lu Mhz index %d\n",
+			old_freq_hz / 1000000, old_freq_index);
+
+	dev_info(cpu_dev, "enabling ldo_bypass\n");
+	ret = imx6q_set_target(NULL, 0);
+	if (ret) {
+		dev_warn(cpu_dev, "Failed to lower frequency: %d\n", ret);
+		goto out;
+	}
+
+	ret = regulator_allow_bypass(arm_reg, true);
+	if (ret) {
+		dev_err(cpu_dev, "failed to enable bypass for vddarm: %d\n", ret);
+		goto out_restore_cpufreq;
+	}
+	ret = regulator_allow_bypass(soc_reg, true);
+	if (ret) {
+		dev_err(cpu_dev, "failed to enable bypass for vddsoc: %d\n", ret);
+		goto out_restore_arm_reg;
+	}
+	if (!IS_ERR(pu_reg)) {
+		ret = regulator_allow_bypass(pu_reg, true);
+		if (ret) {
+			dev_err(cpu_dev, "failed to enable bypass for vddsoc: %d\n", ret);
+			goto out_restore_soc_reg;
+		}
+	}
+
+	/*
+	 * We need to do get_bypass afterwards because allow_bypass does not
+	 * actually guarantee bypass mode was entered if it returns 0. In
+	 * theory there might be another used.
+	 */
+	ret = regulator_is_bypass(arm_reg);
+	if (ret != 1) {
+		ret = -EINVAL;
+		dev_err(cpu_dev, "failed bypass check for vddarm: %d\n", ret);
+		goto out_restore_pu_reg;
+	}
+	ret = regulator_is_bypass(soc_reg);
+	if (ret != 1) {
+		ret = -EINVAL;
+		dev_err(cpu_dev, "failed bypass check for vddsoc: %d\n", ret);
+		goto out_restore_pu_reg;
+	}
+	if (!IS_ERR(pu_reg)) {
+		ret = regulator_is_bypass(pu_reg);
+		if (ret != 1) {
+			ret = -EINVAL;
+			dev_err(cpu_dev, "failed bypass check for vddpu: %d\n", ret);
+			goto out_restore_pu_reg;
+		}
+	}
+
+	ret = imx6q_set_target(NULL, old_freq_index);
+	if (ret)
+		dev_warn(cpu_dev, "Failed to restore frequency: %d\n", ret);
+	/* Success! */
+	ret = 0;
+	goto out;
+
+out_restore_pu_reg:
+	if (!IS_ERR(pu_reg)) {
+		ret2 = regulator_allow_bypass(pu_reg, false);
+		if (ret2)
+			dev_err(cpu_dev, "failed to restore vddpu: %d\n", ret2);
+	}
+out_restore_arm_reg:
+	ret2 = regulator_allow_bypass(arm_reg, false);
+	if (ret2)
+		dev_err(cpu_dev, "failed to restore vddarm: %d\n", ret2);
+out_restore_soc_reg:
+	ret2 = regulator_allow_bypass(soc_reg, false);
+	if (ret2)
+		dev_err(cpu_dev, "failed to restore vddsoc: %d\n", ret2);
+out_restore_cpufreq:
+	ret2 = imx6q_set_target(NULL, old_freq_index);
+	if (ret2)
+		dev_warn(cpu_dev, "Failed to restore frequency: %d\n", ret2);
+
+out:
+	of_node_put(gpc_node);
+	return ret;
+}
+
 static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
 {
+	int ret;
+
+	ret = imx6q_cpufreq_init_ldo_bypass();
+	if (ret) {
+		dev_err(cpu_dev, "failed to enable ldo_bypass: %d\n", ret);
+		return ret;
+	}
+
 	policy->clk = arm_clk;
 	policy->suspend_freq = freq_table[soc_opp_count - 1].frequency;
 	return cpufreq_generic_init(policy, freq_table, transition_latency);
-- 
2.7.4

Re: [RFC 7/8] cpufreq: imx6q: Initialize LDO bypass

[Lucas Stach]

Am Mittwoch, den 22.03.2017, 18:53 +0200 schrieb Leonard Crestez:
> Several imx6* socs have three built-in regulators LDO_ARM LDO_SOC and
> LDO_PU used to control internal chip voltages. "ldo-bypass" mode refers
> to placing these regulators in bypass mode and controlling voltages from
> an external power management chip instead. This is intended to save
> power at the expense of an extra PMIC on the board.
> 
> The voltages for these regulators are controlled from the imxq6 cpufreq
> driver so it makes sense to also control LDO bypass from here. The gpc
> driver also fetches a reference to LDO_PU and uses it to gate power to
> graphics blocks.
> 
> The LDO regulator has a minimum dropout voltage of 125mv so enabling
> bypass mode will raise the effective voltage by that amount. We need set
> the minimum frequency first to avoid overvolting when enabling LDO
> bypass.
> 
> The binding is an u32 fsl,ldo-bypass in the gpc node because that's how
> it was defined in the freescale vendor tree for a long time and
> compatibility is desirable. Otherwise it would be a bool.
> 
> Some versions of u-boot shipped by freescale check this same property
> and set regulators in bypass mode before linux actually starts so we
> check for that scenario as well and finish early.

I've not looked at the patch at all, but this feels like the wrong
location to implement this. Using bypass mode or not should really be a
internal decision of the regulator driver, influenced by a DT property
to allow bypass mode.

The regulator driver can also implement the correct sequencing of first
lowering external voltage to min + dropout, then going into bypass mode,
then lower the external voltage by the amount of the dropout. I don't
see why we need a frequency switch for this at all.

Implementing this in the consumers seems like the wrong spot.

Regards,
Lucas

> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  .../devicetree/bindings/power/fsl,imx-gpc.txt      |   4 +
>  arch/arm/mach-imx/gpc.c                            |   7 +
>  drivers/cpufreq/imx6q-cpufreq.c                    | 171 +++++++++++++++++++++
>  3 files changed, 182 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt b/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
> index 65cc034..8a7584b 100644
> --- a/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
> +++ b/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
> @@ -11,6 +11,10 @@ Required properties:
>    datasheet
>  - interrupts: Should contain GPC interrupt request 1
>  - pu-supply: Link to the LDO regulator powering the PU power domain
> +- fsl,ldo-bypass: Should be 0 or 1 to enable LDO bypass mode (default 0).
> +  This is performed in cooperation with cpufreq. Some versions of uboot will
> +  also look at this property and set the arm and soc regulators in bypass mode
> +  before linux.
>  - clocks: Clock phandles to devices in the PU power domain that need
>  	  to be enabled during domain power-up for reset propagation.
>  - #power-domain-cells: Should be 1, see below:
> diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
> index ce64d11..62a2555 100644
> --- a/arch/arm/mach-imx/gpc.c
> +++ b/arch/arm/mach-imx/gpc.c
> @@ -22,6 +22,7 @@
>  #include <linux/pm_domain.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/irqchip/arm-gic.h>
> +#include <linux/cpufreq.h>
>  #include "common.h"
>  #include "hardware.h"
>  
> @@ -461,6 +462,12 @@ static int imx_gpc_probe(struct platform_device *pdev)
>  	if (!of_property_read_bool(pdev->dev.of_node, "#power-domain-cells"))
>  		return 0;
>  
> +	/* wait for cpufreq to initialize before using pu_reg */
> +	if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ) && cpufreq_get_current_driver() == NULL) {
> +		dev_dbg(&pdev->dev, "cpufreq driver not ready, retry\n");
> +		return -EPROBE_DEFER;
> +	}
> +
>  	pu_reg = devm_regulator_get_optional(&pdev->dev, "pu");
>  	if (PTR_ERR(pu_reg) == -ENODEV)
>  		pu_reg = NULL;
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index e2c1fbf..a0c11ed 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -159,8 +159,179 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
>  	return 0;
>  }
>  
> +/*
> + * Enable ldo-bypass mode if configured and not already performed by u-boot
> + */
> +static int imx6q_cpufreq_init_ldo_bypass(void)
> +{
> +	struct device_node *gpc_node;
> +	unsigned long old_freq_hz;
> +	int i, old_freq_index;
> +	u32 bypass = 0;
> +	int ret = 0, ret2;
> +
> +	/* Read ldo-bypass property */
> +	gpc_node = of_find_compatible_node(NULL, NULL, "fsl,imx6q-gpc");
> +	if (!gpc_node)
> +		return 0;
> +	ret = of_property_read_u32(gpc_node, "fsl,ldo-bypass", &bypass);
> +	if (ret && ret != -EINVAL)
> +		dev_warn(cpu_dev, "failed reading fsl,ldo-bypass property: %d\n", ret);
> +	if (!bypass)
> +		goto out;
> +
> +	/*
> +	 * Freescale u-boot handles ldo-bypass by setting
> +	 * arm/soc in bypass and vddpu disabled.
> +	 *
> +	 * If this is the case we don't need any special freq lowering.
> +	 */
> +	if (regulator_is_bypass(arm_reg) == 1 &&
> +		regulator_is_bypass(soc_reg) == 1)
> +	{
> +		dev_info(cpu_dev, "vddarm and vddsoc already in bypass\n");
> +		if (IS_ERR(pu_reg)) {
> +			ret = 0;
> +			goto out;
> +		} else if (regulator_is_enabled(pu_reg) == 0) {
> +			ret = regulator_allow_bypass(pu_reg, true);
> +			if (ret) {
> +				dev_err(cpu_dev, "failed to enable vddpu bypass: %d\n", ret);
> +				goto out;
> +			}
> +			ret = regulator_is_bypass(pu_reg);
> +			if (ret != 1) {
> +				ret = -EINVAL;
> +				dev_err(cpu_dev, "failed bypass check for vddpu: %d\n", ret);
> +				goto out;
> +			}
> +			ret = 0;
> +			goto out;
> +		} else if (regulator_is_bypass(pu_reg) == 1) {
> +			dev_info(cpu_dev, "vddpu also already in bypass\n");
> +			ret = 0;
> +			goto out;
> +		} else
> +			dev_info(cpu_dev, "Need frequency lowering to set vddpu in bypass\n");
> +	}
> +
> +	/*
> +	 * Enable LDO bypass from kernel.
> +	 *
> +	 * The LDO regulator has a minimum dropout voltage of 125mv so enabling
> +	 * bypass mode will raise the effective voltage by that amount.
> +	 *
> +	 * We set the minimum frequency first to avoid overvolting.
> +	 */
> +
> +	/* Find current freq so we can restore it. */
> +	old_freq_hz = clk_get_rate(arm_clk);
> +	if (!old_freq_hz) {
> +		dev_err(cpu_dev, "failed to determine current arm freq\n");
> +		goto out;
> +	}
> +	old_freq_index = 0;
> +	for (i = 1; i < soc_opp_count; ++i) {
> +		if (abs(freq_table[old_freq_index].frequency - old_freq_hz) >
> +			abs(freq_table[i].frequency - old_freq_hz)) {
> +			old_freq_index = i;
> +		}
> +	}
> +	dev_dbg(cpu_dev, "current freq %lu Mhz index %d\n",
> +			old_freq_hz / 1000000, old_freq_index);
> +
> +	dev_info(cpu_dev, "enabling ldo_bypass\n");
> +	ret = imx6q_set_target(NULL, 0);
> +	if (ret) {
> +		dev_warn(cpu_dev, "Failed to lower frequency: %d\n", ret);
> +		goto out;
> +	}
> +
> +	ret = regulator_allow_bypass(arm_reg, true);
> +	if (ret) {
> +		dev_err(cpu_dev, "failed to enable bypass for vddarm: %d\n", ret);
> +		goto out_restore_cpufreq;
> +	}
> +	ret = regulator_allow_bypass(soc_reg, true);
> +	if (ret) {
> +		dev_err(cpu_dev, "failed to enable bypass for vddsoc: %d\n", ret);
> +		goto out_restore_arm_reg;
> +	}
> +	if (!IS_ERR(pu_reg)) {
> +		ret = regulator_allow_bypass(pu_reg, true);
> +		if (ret) {
> +			dev_err(cpu_dev, "failed to enable bypass for vddsoc: %d\n", ret);
> +			goto out_restore_soc_reg;
> +		}
> +	}
> +
> +	/*
> +	 * We need to do get_bypass afterwards because allow_bypass does not
> +	 * actually guarantee bypass mode was entered if it returns 0. In
> +	 * theory there might be another used.
> +	 */
> +	ret = regulator_is_bypass(arm_reg);
> +	if (ret != 1) {
> +		ret = -EINVAL;
> +		dev_err(cpu_dev, "failed bypass check for vddarm: %d\n", ret);
> +		goto out_restore_pu_reg;
> +	}
> +	ret = regulator_is_bypass(soc_reg);
> +	if (ret != 1) {
> +		ret = -EINVAL;
> +		dev_err(cpu_dev, "failed bypass check for vddsoc: %d\n", ret);
> +		goto out_restore_pu_reg;
> +	}
> +	if (!IS_ERR(pu_reg)) {
> +		ret = regulator_is_bypass(pu_reg);
> +		if (ret != 1) {
> +			ret = -EINVAL;
> +			dev_err(cpu_dev, "failed bypass check for vddpu: %d\n", ret);
> +			goto out_restore_pu_reg;
> +		}
> +	}
> +
> +	ret = imx6q_set_target(NULL, old_freq_index);
> +	if (ret)
> +		dev_warn(cpu_dev, "Failed to restore frequency: %d\n", ret);
> +	/* Success! */
> +	ret = 0;
> +	goto out;
> +
> +out_restore_pu_reg:
> +	if (!IS_ERR(pu_reg)) {
> +		ret2 = regulator_allow_bypass(pu_reg, false);
> +		if (ret2)
> +			dev_err(cpu_dev, "failed to restore vddpu: %d\n", ret2);
> +	}
> +out_restore_arm_reg:
> +	ret2 = regulator_allow_bypass(arm_reg, false);
> +	if (ret2)
> +		dev_err(cpu_dev, "failed to restore vddarm: %d\n", ret2);
> +out_restore_soc_reg:
> +	ret2 = regulator_allow_bypass(soc_reg, false);
> +	if (ret2)
> +		dev_err(cpu_dev, "failed to restore vddsoc: %d\n", ret2);
> +out_restore_cpufreq:
> +	ret2 = imx6q_set_target(NULL, old_freq_index);
> +	if (ret2)
> +		dev_warn(cpu_dev, "Failed to restore frequency: %d\n", ret2);
> +
> +out:
> +	of_node_put(gpc_node);
> +	return ret;
> +}
> +
>  static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
>  {
> +	int ret;
> +
> +	ret = imx6q_cpufreq_init_ldo_bypass();
> +	if (ret) {
> +		dev_err(cpu_dev, "failed to enable ldo_bypass: %d\n", ret);
> +		return ret;
> +	}
> +
>  	policy->clk = arm_clk;
>  	policy->suspend_freq = freq_table[soc_opp_count - 1].frequency;
>  	return cpufreq_generic_init(policy, freq_table, transition_latency);

Re: [RFC 7/8] cpufreq: imx6q: Initialize LDO bypass

[Leonard Crestez]

On Wed, 2017-03-22 at 18:09 +0100, Lucas Stach wrote:
> Am Mittwoch, den 22.03.2017, 18:53 +0200 schrieb Leonard Crestez:
> > 
> > Several imx6* socs have three built-in regulators LDO_ARM LDO_SOC and
> > LDO_PU used to control internal chip voltages. "ldo-bypass" mode refers
> > to placing these regulators in bypass mode and controlling voltages from
> > an external power management chip instead. This is intended to save
> > power at the expense of an extra PMIC on the board.
> > 
> > The voltages for these regulators are controlled from the imxq6 cpufreq
> > driver so it makes sense to also control LDO bypass from here. The gpc
> > driver also fetches a reference to LDO_PU and uses it to gate power to
> > graphics blocks.
> > 
> > The LDO regulator has a minimum dropout voltage of 125mv so enabling
> > bypass mode will raise the effective voltage by that amount. We need set
> > the minimum frequency first to avoid overvolting when enabling LDO
> > bypass.
> > 
> > The binding is an u32 fsl,ldo-bypass in the gpc node because that's how
> > it was defined in the freescale vendor tree for a long time and
> > compatibility is desirable. Otherwise it would be a bool.
> > 
> > Some versions of u-boot shipped by freescale check this same property
> > and set regulators in bypass mode before linux actually starts so we
> > check for that scenario as well and finish early.
> I've not looked at the patch at all, but this feels like the wrong
> location to implement this. Using bypass mode or not should really be a
> internal decision of the regulator driver, influenced by a DT property
> to allow bypass mode.
> 
> The regulator driver can also implement the correct sequencing of first
> lowering external voltage to min + dropout, then going into bypass mode,
> then lower the external voltage by the amount of the dropout. I don't
> see why we need a frequency switch for this at all.

Because minimum voltages are dictated by core frequency. At high frequency
the (minimum voltage for frequency + dropout) is too high and would go beyond
the maximum of 1300 mv when bypass is enabled. It doesn't actually instantly
break, this is based on the "operating ranges" from this document:

http://www.nxp.com/assets/documents/data/en/data-sheets/IMX6DQCEC.pdf

> Implementing this in the consumers seems like the wrong spot.

It doesn't belong in drivers for individual regulators either, it's a piece
of board-level global configuration.

--
Regards,
Leonard
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 7/8] cpufreq: imx6q: Initialize LDO bypass

[Lucas Stach]

Am Mittwoch, den 22.03.2017, 19:48 +0200 schrieb Leonard Crestez:
> On Wed, 2017-03-22 at 18:09 +0100, Lucas Stach wrote:
> > Am Mittwoch, den 22.03.2017, 18:53 +0200 schrieb Leonard Crestez:
> > > 
> > > Several imx6* socs have three built-in regulators LDO_ARM LDO_SOC and
> > > LDO_PU used to control internal chip voltages. "ldo-bypass" mode refers
> > > to placing these regulators in bypass mode and controlling voltages from
> > > an external power management chip instead. This is intended to save
> > > power at the expense of an extra PMIC on the board.
> > > 
> > > The voltages for these regulators are controlled from the imxq6 cpufreq
> > > driver so it makes sense to also control LDO bypass from here. The gpc
> > > driver also fetches a reference to LDO_PU and uses it to gate power to
> > > graphics blocks.
> > > 
> > > The LDO regulator has a minimum dropout voltage of 125mv so enabling
> > > bypass mode will raise the effective voltage by that amount. We need set
> > > the minimum frequency first to avoid overvolting when enabling LDO
> > > bypass.
> > > 
> > > The binding is an u32 fsl,ldo-bypass in the gpc node because that's how
> > > it was defined in the freescale vendor tree for a long time and
> > > compatibility is desirable. Otherwise it would be a bool.
> > > 
> > > Some versions of u-boot shipped by freescale check this same property
> > > and set regulators in bypass mode before linux actually starts so we
> > > check for that scenario as well and finish early.
> > I've not looked at the patch at all, but this feels like the wrong
> > location to implement this. Using bypass mode or not should really be a
> > internal decision of the regulator driver, influenced by a DT property
> > to allow bypass mode.
> > 
> > The regulator driver can also implement the correct sequencing of first
> > lowering external voltage to min + dropout, then going into bypass mode,
> > then lower the external voltage by the amount of the dropout. I don't
> > see why we need a frequency switch for this at all.
> 
> Because minimum voltages are dictated by core frequency. At high frequency
> the (minimum voltage for frequency + dropout) is too high and would go beyond
> the maximum of 1300 mv when bypass is enabled. It doesn't actually instantly
> break, this is based on the "operating ranges" from this document:
> 
> http://www.nxp.com/assets/documents/data/en/data-sheets/IMX6DQCEC.pdf

Urgh, yes, this is unfortunate.

> > Implementing this in the consumers seems like the wrong spot.
> 
> It doesn't belong in drivers for individual regulators either, it's a piece
> of board-level global configuration.

The configuration weather to use the bypass or not is board level, I
agree. But the decision to bypass the regulator should be done either at
the regulator driver level or even in the regulator framework. That is
the point where you are able to match up the constraints of the
consumers with the ability to get the correct voltage from the supplying
regulator.

Regards,
Lucas

[RFC 8/8] ARM: dts: imx6qdl-sabresd: Enable fsl,ldo-bypass

[Leonard Crestez]

This enables LDO bypass by default on the imx6qdl-sabresd boards. New
dts files with -ldo suffix are added for users who want to run with LDOs
enabled on these boards anyway.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 arch/arm/boot/dts/imx6dl-sabresd-ldo.dts | 13 +++++++++++++
 arch/arm/boot/dts/imx6q-sabresd-ldo.dts  | 13 +++++++++++++
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi   | 19 +++++++++++++++++++
 arch/arm/boot/dts/imx6qdl.dtsi           |  6 +++---
 arch/arm/boot/dts/imx6qp-sabresd-ldo.dts | 13 +++++++++++++
 arch/arm/boot/dts/imx6qp-sabresd.dts     |  4 ++--
 6 files changed, 63 insertions(+), 5 deletions(-)
 create mode 100644 arch/arm/boot/dts/imx6dl-sabresd-ldo.dts
 create mode 100644 arch/arm/boot/dts/imx6q-sabresd-ldo.dts
 create mode 100644 arch/arm/boot/dts/imx6qp-sabresd-ldo.dts

diff --git a/arch/arm/boot/dts/imx6dl-sabresd-ldo.dts b/arch/arm/boot/dts/imx6dl-sabresd-ldo.dts
new file mode 100644
index 0000000..1b224d6
--- /dev/null
+++ b/arch/arm/boot/dts/imx6dl-sabresd-ldo.dts
@@ -0,0 +1,13 @@
+/*
+ * Copyright 2017 NXP
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include "imx6dl-sabresd.dts"
+
+&gpc {
+	fsl,ldo-bypass = <0>;
+};
diff --git a/arch/arm/boot/dts/imx6q-sabresd-ldo.dts b/arch/arm/boot/dts/imx6q-sabresd-ldo.dts
new file mode 100644
index 0000000..4d2e8cc
--- /dev/null
+++ b/arch/arm/boot/dts/imx6q-sabresd-ldo.dts
@@ -0,0 +1,13 @@
+/*
+ * Copyright 2017 NXP
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include "imx6q-sabresd.dts"
+
+&gpc {
+	fsl,ldo-bypass = <0>;
+};
diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index 63bf95e..5a73d9d 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -182,6 +182,10 @@
 	status = "okay";
 };
 
+&gpc {
+	fsl,ldo-bypass = <1>;
+};
+
 &hdmi {
 	ddc-i2c-bus = <&i2c2>;
 	status = "okay";
@@ -548,6 +552,21 @@
 	status = "okay";
 };
 
+&reg_arm {
+       vin-supply = <&sw1a_reg>;
+       regulator-allow-bypass;
+};
+
+&reg_pu {
+       vin-supply = <&sw1c_reg>;
+       regulator-allow-bypass;
+};
+
+&reg_soc {
+       vin-supply = <&sw1c_reg>;
+       regulator-allow-bypass;
+};
+
 &snvs_poweroff {
 	status = "okay";
 };
diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index 6d7bf64..54fe769 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -677,7 +677,7 @@
 					compatible = "fsl,anatop-regulator";
 					regulator-name = "vddarm";
 					regulator-min-microvolt = <725000>;
-					regulator-max-microvolt = <1450000>;
+					regulator-max-microvolt = <1300000>;
 					regulator-always-on;
 					anatop-reg-offset = <0x140>;
 					anatop-vol-bit-shift = <0>;
@@ -694,7 +694,7 @@
 					compatible = "fsl,anatop-regulator";
 					regulator-name = "vddpu";
 					regulator-min-microvolt = <725000>;
-					regulator-max-microvolt = <1450000>;
+					regulator-max-microvolt = <1300000>;
 					regulator-enable-ramp-delay = <150>;
 					anatop-reg-offset = <0x140>;
 					anatop-vol-bit-shift = <9>;
@@ -711,7 +711,7 @@
 					compatible = "fsl,anatop-regulator";
 					regulator-name = "vddsoc";
 					regulator-min-microvolt = <725000>;
-					regulator-max-microvolt = <1450000>;
+					regulator-max-microvolt = <1300000>;
 					regulator-always-on;
 					anatop-reg-offset = <0x140>;
 					anatop-vol-bit-shift = <18>;
diff --git a/arch/arm/boot/dts/imx6qp-sabresd-ldo.dts b/arch/arm/boot/dts/imx6qp-sabresd-ldo.dts
new file mode 100644
index 0000000..c659533
--- /dev/null
+++ b/arch/arm/boot/dts/imx6qp-sabresd-ldo.dts
@@ -0,0 +1,13 @@
+/*
+ * Copyright 2017 NXP
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include "imx6qp-sabresd.dts"
+
+&gpc {
+	fsl,ldo-bypass = <0>;
+};
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

Re: [RFC 8/8] ARM: dts: imx6qdl-sabresd: Enable fsl,ldo-bypass

[Lucas Stach]

Am Mittwoch, den 22.03.2017, 18:53 +0200 schrieb Leonard Crestez:
> This enables LDO bypass by default on the imx6qdl-sabresd boards. New
> dts files with -ldo suffix are added for users who want to run with LDOs
> enabled on these boards anyway.

Given that using LDO bypass affects the device lifetime negatively (see
AN4724), I think the default should still be to use LDO enabled mode and
have new DTs for people that desire to shorten the lifetime of the SoC
for a minimal drop in power consumption.

Regards,
Lucas
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  arch/arm/boot/dts/imx6dl-sabresd-ldo.dts | 13 +++++++++++++
>  arch/arm/boot/dts/imx6q-sabresd-ldo.dts  | 13 +++++++++++++
>  arch/arm/boot/dts/imx6qdl-sabresd.dtsi   | 19 +++++++++++++++++++
>  arch/arm/boot/dts/imx6qdl.dtsi           |  6 +++---
>  arch/arm/boot/dts/imx6qp-sabresd-ldo.dts | 13 +++++++++++++
>  arch/arm/boot/dts/imx6qp-sabresd.dts     |  4 ++--
>  6 files changed, 63 insertions(+), 5 deletions(-)
>  create mode 100644 arch/arm/boot/dts/imx6dl-sabresd-ldo.dts
>  create mode 100644 arch/arm/boot/dts/imx6q-sabresd-ldo.dts
>  create mode 100644 arch/arm/boot/dts/imx6qp-sabresd-ldo.dts
> 
> diff --git a/arch/arm/boot/dts/imx6dl-sabresd-ldo.dts b/arch/arm/boot/dts/imx6dl-sabresd-ldo.dts
> new file mode 100644
> index 0000000..1b224d6
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx6dl-sabresd-ldo.dts
> @@ -0,0 +1,13 @@
> +/*
> + * Copyright 2017 NXP
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include "imx6dl-sabresd.dts"
> +
> +&gpc {
> +	fsl,ldo-bypass = <0>;
> +};
> diff --git a/arch/arm/boot/dts/imx6q-sabresd-ldo.dts b/arch/arm/boot/dts/imx6q-sabresd-ldo.dts
> new file mode 100644
> index 0000000..4d2e8cc
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx6q-sabresd-ldo.dts
> @@ -0,0 +1,13 @@
> +/*
> + * Copyright 2017 NXP
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include "imx6q-sabresd.dts"
> +
> +&gpc {
> +	fsl,ldo-bypass = <0>;
> +};
> diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> index 63bf95e..5a73d9d 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> @@ -182,6 +182,10 @@
>  	status = "okay";
>  };
>  
> +&gpc {
> +	fsl,ldo-bypass = <1>;
> +};
> +
>  &hdmi {
>  	ddc-i2c-bus = <&i2c2>;
>  	status = "okay";
> @@ -548,6 +552,21 @@
>  	status = "okay";
>  };
>  
> +&reg_arm {
> +       vin-supply = <&sw1a_reg>;
> +       regulator-allow-bypass;
> +};
> +
> +&reg_pu {
> +       vin-supply = <&sw1c_reg>;
> +       regulator-allow-bypass;
> +};
> +
> +&reg_soc {
> +       vin-supply = <&sw1c_reg>;
> +       regulator-allow-bypass;
> +};
> +
>  &snvs_poweroff {
>  	status = "okay";
>  };
> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index 6d7bf64..54fe769 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -677,7 +677,7 @@
>  					compatible = "fsl,anatop-regulator";
>  					regulator-name = "vddarm";
>  					regulator-min-microvolt = <725000>;
> -					regulator-max-microvolt = <1450000>;
> +					regulator-max-microvolt = <1300000>;
>  					regulator-always-on;
>  					anatop-reg-offset = <0x140>;
>  					anatop-vol-bit-shift = <0>;
> @@ -694,7 +694,7 @@
>  					compatible = "fsl,anatop-regulator";
>  					regulator-name = "vddpu";
>  					regulator-min-microvolt = <725000>;
> -					regulator-max-microvolt = <1450000>;
> +					regulator-max-microvolt = <1300000>;
>  					regulator-enable-ramp-delay = <150>;
>  					anatop-reg-offset = <0x140>;
>  					anatop-vol-bit-shift = <9>;
> @@ -711,7 +711,7 @@
>  					compatible = "fsl,anatop-regulator";
>  					regulator-name = "vddsoc";
>  					regulator-min-microvolt = <725000>;
> -					regulator-max-microvolt = <1450000>;
> +					regulator-max-microvolt = <1300000>;
>  					regulator-always-on;
>  					anatop-reg-offset = <0x140>;
>  					anatop-vol-bit-shift = <18>;
> diff --git a/arch/arm/boot/dts/imx6qp-sabresd-ldo.dts b/arch/arm/boot/dts/imx6qp-sabresd-ldo.dts
> new file mode 100644
> index 0000000..c659533
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx6qp-sabresd-ldo.dts
> @@ -0,0 +1,13 @@
> +/*
> + * Copyright 2017 NXP
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include "imx6qp-sabresd.dts"
> +
> +&gpc {
> +	fsl,ldo-bypass = <0>;
> +};
> 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 {

Re: [RFC 8/8] ARM: dts: imx6qdl-sabresd: Enable fsl,ldo-bypass

[Leonard Crestez]

On Wed, 2017-03-22 at 18:13 +0100, Lucas Stach wrote:
> Am Mittwoch, den 22.03.2017, 18:53 +0200 schrieb Leonard Crestez:

> > This enables LDO bypass by default on the imx6qdl-sabresd boards. New
> > dts files with -ldo suffix are added for users who want to run with LDOs
> > enabled on these boards anyway.

> Given that using LDO bypass affects the device lifetime negatively (see
> AN4724), I think the default should still be to use LDO enabled mode and
> have new DTs for people that desire to shorten the lifetime of the SoC
> for a minimal drop in power consumption.

This was based on what the Freescale BSP does, I don't particularly
object to upstream having a different default. It would be nice for
testing if this ldo-bypass path was enabled by default for some boards
but it's not a very good reason.

I will switch the default.

-- 
Regards,
Leonard
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html