linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RFC 0/2] ARM: imx: ventana: enable LDO-bypass mode
@ 2014-09-26  5:59 Tim Harvey
  2014-09-26  5:59 ` [PATCH 1/2] regulator: anatop: allow LDO bypass to be set from device-tree Tim Harvey
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Tim Harvey @ 2014-09-26  5:59 UTC (permalink / raw)
  To: linux-arm-kernel

The IMX6 has some internal LDO regulators provided by the anatop regulator
block that can regulate the arm, soc, gpu/vpu core supplies. Alternatively a
design can supply vdd_arm and vdd_soc externally via a PMIC to provide a lower
power draw (switches are more efficient that ldo's).

The first patch allows a device-tree property to put the anatop core regulators
in bypass mode so that an external PMIC regulator can be used. It is
important that the anatop regulators be put in bypass mode because otherwise
the external inputs need to be 125mV above what the datasheets specify for
the various CPU frequency operating points defined in the imx6q.dtsi and
imx6dl.dtsi.

I would like some feedback as to how best to specify or determine if the
anatop regulators should be placed in bypass mode and I see the following
options:
 1) use a device-tree node on the core regulators to put them in bypass mode
    (what I've done for this patchset).
 2) depend on the bootloader to put the LDO's in bypass mode (by setting them
    to fully on) which the anatop-regulator driver will recognized and
    keep them in bypass mode (per Phillip's commit d38018f). The downside of
    this is that it creates a horrible dependence between the kernel and the
    bootloader.
 3) have the imx6q-cpufreq driver (the sole consumer of the arm, soc, gpu/vpu
    regulators) determine that the arm and soc regulators are 'not' the
    anatop regulators and if so set the anatop core regulators in bypass mode
    as this can only mean an external PMIC is being used. I'm not exactly
    sure 'how' to do this (how can a regulator consumer determine the provider 
    of a regulator?) and I'm also not quite sure if this assumption is always
    true. What if external regulators are used on a design but one or both need
    further regulation?

I have tested this patchset with the Gateworks Ventana GW5400 board and
verified the following:
  a) at the various CPU setpoints supported by the CPU on this board
     (Automative IMX6Q at 1GHz) VDD_ARM and VDD_SOC (both the inputs to the
     IMX6 and the outputs of the bypassed LDO) are at the voltage setpoint
     requested for the CPU frequency.
  b) the PMU_REG_CORE REG0/REG1/REG2 get set to full scale (bypassed) and stay
     that way.

Another possible concern with LDO-bypass mode is that in the case of a 1.2GHz
CPU, the datasheet specifies that the internal LDO's must be used at the 1.2GHz
frequency setpoint as they produce less noise as an external switcher. This
means that the imx6q-cpufreq driver would need to be the one making the
decision to enable bypass mode and would do so per operating setpoint.

Note that the only imx6 boards I know of in mainline that appear to have a PMIC
(based on their devicetree) are:
 - dmo-edmqmx6 (pfuze100)
 - imx6sl-evk (pfuze100)
 - gw54xx (pfuze100)
 - gw53xx/gw52xx/gw51xx/gw552x (ltc3676)
 - phytec-pfla02 (da9063)
 - riotboard (pfuze100)
 - sabreauto (pfuze100)
 - sabresd (pfuze100)

I've cc'd the authors of the original commits of the above device-trees as
they likely have an interest in LDO-bypass mode.

Tim Harvey (2):
  regulator: anatop: allow LDO bypass to be set from device-tree
  ARM: imx: ventana: enable LDO bypass mode for GW54xx

 .../bindings/regulator/anatop-regulator.txt        |  1 +
 arch/arm/boot/dts/imx6qdl-gw54xx.dtsi              | 63 ++++++++++++++++------
 drivers/regulator/anatop-regulator.c               |  4 +-
 3 files changed, 51 insertions(+), 17 deletions(-)

-- 
1.8.3.2

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] regulator: anatop: allow LDO bypass to be set from device-tree
  2014-09-26  5:59 [PATCH RFC 0/2] ARM: imx: ventana: enable LDO-bypass mode Tim Harvey
@ 2014-09-26  5:59 ` Tim Harvey
  2014-09-26  8:57   ` Philipp Zabel
  2014-09-26  5:59 ` [PATCH 2/2] ARM: imx: ventana: enable LDO bypass mode for GW54xx Tim Harvey
  2014-09-26  7:20 ` [PATCH RFC 0/2] ARM: imx: ventana: enable LDO-bypass mode Stefan Wahren
  2 siblings, 1 reply; 6+ messages in thread
From: Tim Harvey @ 2014-09-26  5:59 UTC (permalink / raw)
  To: linux-arm-kernel

The core regulators provided by the Anatop regulator (reg_core, reg_soc,
reg_pu) should be set in bypass mode if an extern PMIC regulator is used.
This allows configuring this from device-tree.

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Mark Brown <broonie@linaro.org>
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 Documentation/devicetree/bindings/regulator/anatop-regulator.txt | 1 +
 drivers/regulator/anatop-regulator.c                             | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/regulator/anatop-regulator.txt b/Documentation/devicetree/bindings/regulator/anatop-regulator.txt
index 758eae2..e958c60 100644
--- a/Documentation/devicetree/bindings/regulator/anatop-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/anatop-regulator.txt
@@ -13,6 +13,7 @@ Optional properties:
 - anatop-delay-reg-offset: Anatop MFD step time register offset
 - anatop-delay-bit-shift: Bit shift for the step time register
 - anatop-delay-bit-width: Number of bits used in the step time register
+- anatop-ldo-bypass: Enable LDO bypass mode (for core regulators)
 
 Any property defined as part of the core regulator
 binding, defined in regulator.txt, can also be used.
diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index 4f730af..0a22d3a 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -279,8 +279,10 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 		}
 
 		sreg->sel = (val & rdesc->vsel_mask) >> sreg->vol_bit_shift;
-		if (sreg->sel == LDO_FET_FULL_ON) {
+		if (sreg->sel == LDO_FET_FULL_ON ||
+		    of_property_read_bool(np, "anatop-ldo-bypass")) {
 			sreg->sel = 0;
+			dev_info(dev, "LDO bypass mode\n");
 			sreg->bypass = true;
 		}
 	} else {
-- 
1.8.3.2

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] ARM: imx: ventana: enable LDO bypass mode for GW54xx
  2014-09-26  5:59 [PATCH RFC 0/2] ARM: imx: ventana: enable LDO-bypass mode Tim Harvey
  2014-09-26  5:59 ` [PATCH 1/2] regulator: anatop: allow LDO bypass to be set from device-tree Tim Harvey
@ 2014-09-26  5:59 ` Tim Harvey
  2014-09-26  7:20 ` [PATCH RFC 0/2] ARM: imx: ventana: enable LDO-bypass mode Stefan Wahren
  2 siblings, 0 replies; 6+ messages in thread
From: Tim Harvey @ 2014-09-26  5:59 UTC (permalink / raw)
  To: linux-arm-kernel

The GW54xx baseboard has a PFUZE100 PMIC capable of regulating the
core voltages (VDD_ARM, VDD_SOC) externally such that the internal IMX6
anatop LDO regulators are not needed. This provides a power reduction
(as the PMIC is more efficient than the LDO's) as well as moves some
of the power/thermal burden from the IMX to the PMIC.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 arch/arm/boot/dts/imx6qdl-gw54xx.dtsi | 63 ++++++++++++++++++++++++++---------
 1 file changed, 47 insertions(+), 16 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi
index 7b8bd61..e31356d 100644
--- a/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi
@@ -142,6 +142,11 @@
 	status = "okay";
 };
 
+&cpu0 {
+        arm-supply = <&reg_sw1a>;
+        soc-supply = <&reg_sw1c>;
+};
+
 &fec {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_enet>;
@@ -215,7 +220,8 @@
 		reg = <0x08>;
 
 		regulators {
-			sw1a_reg: sw1ab {
+			/* VDD_ARM */
+			reg_sw1a: sw1ab {
 				regulator-min-microvolt = <300000>;
 				regulator-max-microvolt = <1875000>;
 				regulator-boot-on;
@@ -223,7 +229,8 @@
 				regulator-ramp-delay = <6250>;
 			};
 
-			sw1c_reg: sw1c {
+			/* VDD_SOC */
+			reg_sw1c: sw1c {
 				regulator-min-microvolt = <300000>;
 				regulator-max-microvolt = <1875000>;
 				regulator-boot-on;
@@ -231,77 +238,89 @@
 				regulator-ramp-delay = <6250>;
 			};
 
-			sw2_reg: sw2 {
+			/* VDD_HIGH */
+			reg_sw2: sw2 {
 				regulator-min-microvolt = <800000>;
 				regulator-max-microvolt = <3950000>;
 				regulator-boot-on;
 				regulator-always-on;
 			};
 
-			sw3a_reg: sw3a {
+			/* VDD_DDR */
+			reg_sw3a: sw3a {
 				regulator-min-microvolt = <400000>;
 				regulator-max-microvolt = <1975000>;
 				regulator-boot-on;
 				regulator-always-on;
 			};
 
-			sw3b_reg: sw3b {
+			/* VDD_DDR */
+			reg_sw3b: sw3b {
 				regulator-min-microvolt = <400000>;
 				regulator-max-microvolt = <1975000>;
 				regulator-boot-on;
 				regulator-always-on;
 			};
 
-			sw4_reg: sw4 {
+			/* VDD_1P8 */
+			reg_sw4: sw4 {
 				regulator-min-microvolt = <800000>;
 				regulator-max-microvolt = <3300000>;
 			};
 
-			swbst_reg: swbst {
+			/* VDD_5P0 */
+			reg_swbst: swbst {
 				regulator-min-microvolt = <5000000>;
 				regulator-max-microvolt = <5150000>;
 			};
 
-			snvs_reg: vsnvs {
+			reg_snvs: vsnvs {
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <3000000>;
 				regulator-boot-on;
 				regulator-always-on;
 			};
 
-			vref_reg: vrefddr {
+			/* VDD_VREF */
+			reg_vref: vrefddr {
 				regulator-boot-on;
 				regulator-always-on;
 			};
 
-			vgen1_reg: vgen1 {
+			/* VDD_PCIE-A_1P5 */
+			reg_vgen1: vgen1 {
 				regulator-min-microvolt = <800000>;
 				regulator-max-microvolt = <1550000>;
 			};
 
-			vgen2_reg: vgen2 {
+			/* VDD_PCIE-B_1P5 */
+			reg_vgen2: vgen2 {
 				regulator-min-microvolt = <800000>;
 				regulator-max-microvolt = <1550000>;
 			};
 
-			vgen3_reg: vgen3 {
+			/* unused */
+			reg_vgen3: vgen3 {
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <3300000>;
 			};
 
-			vgen4_reg: vgen4 {
+			/* VDD_AUD_1P8 */
+			reg_vgen4: vgen4 {
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <3300000>;
 				regulator-always-on;
 			};
 
-			vgen5_reg: vgen5 {
+			/* VDD_2P5 */
+			reg_vgen5: vgen5 {
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <3300000>;
 				regulator-always-on;
 			};
 
-			vgen6_reg: vgen6 {
+			/* unused */
+			reg_vgen6: vgen6 {
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <3300000>;
 				regulator-always-on;
@@ -325,7 +344,7 @@
 		compatible = "fsl,sgtl5000";
 		reg = <0x0a>;
 		clocks = <&clks 201>;
-		VDDA-supply = <&sw4_reg>;
+		VDDA-supply = <&reg_sw4>;
 		VDDIO-supply = <&reg_3p3v>;
 	};
 
@@ -380,6 +399,18 @@
 	status = "okay";
 };
 
+&reg_arm {
+        anatop-ldo-bypass;
+};
+
+&reg_soc {
+        anatop-ldo-bypass;
+};
+
+&reg_pu {
+        anatop-ldo-bypass;
+};
+
 &ssi1 {
 	fsl,mode = "i2s-slave";
 	status = "okay";
-- 
1.8.3.2

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH RFC 0/2] ARM: imx: ventana: enable LDO-bypass mode
  2014-09-26  5:59 [PATCH RFC 0/2] ARM: imx: ventana: enable LDO-bypass mode Tim Harvey
  2014-09-26  5:59 ` [PATCH 1/2] regulator: anatop: allow LDO bypass to be set from device-tree Tim Harvey
  2014-09-26  5:59 ` [PATCH 2/2] ARM: imx: ventana: enable LDO bypass mode for GW54xx Tim Harvey
@ 2014-09-26  7:20 ` Stefan Wahren
  2 siblings, 0 replies; 6+ messages in thread
From: Stefan Wahren @ 2014-09-26  7:20 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Tim,

Am 26.09.2014 um 07:59 schrieb Tim Harvey:
> The IMX6 has some internal LDO regulators provided by the anatop regulator
> block that can regulate the arm, soc, gpu/vpu core supplies. Alternatively a
> design can supply vdd_arm and vdd_soc externally via a PMIC to provide a lower
> power draw (switches are more efficient that ldo's).
>
> [...]
>
> I've cc'd the authors of the original commits of the above device-trees as
> they likely have an interest in LDO-bypass mode.

please cc devicetree at vger.kernel.org for your patches.

>
> Tim Harvey (2):
>   regulator: anatop: allow LDO bypass to be set from device-tree
>   ARM: imx: ventana: enable LDO bypass mode for GW54xx
>
>  .../bindings/regulator/anatop-regulator.txt        |  1 +
>  arch/arm/boot/dts/imx6qdl-gw54xx.dtsi              | 63 ++++++++++++++++------
>  drivers/regulator/anatop-regulator.c               |  4 +-
>  3 files changed, 51 insertions(+), 17 deletions(-)
>

Best regards
Stefan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] regulator: anatop: allow LDO bypass to be set from device-tree
  2014-09-26  5:59 ` [PATCH 1/2] regulator: anatop: allow LDO bypass to be set from device-tree Tim Harvey
@ 2014-09-26  8:57   ` Philipp Zabel
  2014-09-29  5:47     ` Tim Harvey
  0 siblings, 1 reply; 6+ messages in thread
From: Philipp Zabel @ 2014-09-26  8:57 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Tim,

Am Donnerstag, den 25.09.2014, 22:59 -0700 schrieb Tim Harvey:
> The core regulators provided by the Anatop regulator (reg_core, reg_soc,
> reg_pu) should be set in bypass mode if an extern PMIC regulator is used.

Agreed, but ...

> This allows configuring this from device-tree.

... this should not be configured from the device tree.

If the PMIC driver is not loaded or cpufreq is disabled for other
reasons, the full LDO input voltage will be passed through to the core
all the time. The bypass should only be enabled if there is actually a
driver to reduce the voltage of the PMIC regulators.

I have a patch to imx6q-cpufreq (see below) that uses the regulator API
to enable the bypass on the core LDOs if additional PMIC regulators are
provided in the device tree and regulator-allow-bypass is set on the LDO
regulators. But it has its own problems.
First, it adds even more regulators. I'd rather move the voltage
regulation details out of imx6-cpufreq instead, because ideally that
should only have to care about the ARM voltage.
Second, regulator_allow_bypass does nothing if there are other consumers
that have not allowed bypass yet, but immediately enables bypass once
the last registered consumer allows it. In our case the PU power domain
driver is, or rather will be, a second consumer of the VDDSOC regulator,
even though it does not care about the voltage level. This makes the
allow-bypass ordering a bit of a hassle.

regards
Philipp

--8<--
From: Philipp Zabel <p.zabel@pengutronix.de>
Subject: [PATCH] cpufreq: imx6q: Add support for bypass modes

The internal LDOs can be set to bypass if separate external regulators
are available for VDDARM and VDDSOC inputs.

To use external switching regulators, they have to be added to the cpu0
device tree node using the vddarm-supply and vddsoc-supply properties.
Further, the reg_arm, reg_pu, and reg_soc regulators need to have the
regulator-allow-bypass property set.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/cpufreq/imx6q-cpufreq.c | 152 +++++++++++++++++++++++++++++++++-------
 1 file changed, 126 insertions(+), 26 deletions(-)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index c2d3076..c0b3558 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -23,6 +23,8 @@
 static struct regulator *arm_reg;
 static struct regulator *pu_reg;
 static struct regulator *soc_reg;
+static struct regulator *vddarm_reg;
+static struct regulator *vddsoc_reg;
 
 static struct clk *arm_clk;
 static struct clk *pll1_sys_clk;
@@ -40,6 +42,7 @@ static u32 soc_opp_count;
 static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 {
 	struct dev_pm_opp *opp;
+	struct regulator *reg;
 	unsigned long freq_hz, volt, volt_old;
 	unsigned int old_freq, new_freq;
 	int ret;
@@ -64,21 +67,31 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 		old_freq / 1000, volt_old / 1000,
 		new_freq / 1000, volt / 1000);
 
+	reg = vddarm_reg ? vddarm_reg : arm_reg;
+
 	/* scaling up?  scale voltage before frequency */
 	if (new_freq > old_freq) {
-		if (!IS_ERR(pu_reg)) {
-			ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
+		if (!IS_ERR(vddsoc_reg)) {
+			ret = regulator_set_voltage_tol(vddsoc_reg, imx6_soc_volt[index], 0);
 			if (ret) {
-				dev_err(cpu_dev, "failed to scale vddpu up: %d\n", ret);
+				dev_err(cpu_dev, "failed to scale vddsoc up: %d\n", ret);
+				return ret;
+			}
+		} else {
+			if (!IS_ERR(pu_reg)) {
+				ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
+				if (ret) {
+					dev_err(cpu_dev, "failed to scale vddpu up: %d\n", ret);
+					return ret;
+				}
+			}
+			ret = regulator_set_voltage_tol(soc_reg, imx6_soc_volt[index], 0);
+			if (ret) {
+				dev_err(cpu_dev, "failed to scale vddsoc up: %d\n", ret);
 				return ret;
 			}
 		}
-		ret = regulator_set_voltage_tol(soc_reg, imx6_soc_volt[index], 0);
-		if (ret) {
-			dev_err(cpu_dev, "failed to scale vddsoc up: %d\n", ret);
-			return ret;
-		}
-		ret = regulator_set_voltage_tol(arm_reg, volt, 0);
+		ret = regulator_set_voltage_tol(reg, volt, 0);
 		if (ret) {
 			dev_err(cpu_dev,
 				"failed to scale vddarm up: %d\n", ret);
@@ -106,29 +119,37 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 	ret = clk_set_rate(arm_clk, new_freq * 1000);
 	if (ret) {
 		dev_err(cpu_dev, "failed to set clock rate: %d\n", ret);
-		regulator_set_voltage_tol(arm_reg, volt_old, 0);
+		regulator_set_voltage_tol(reg, volt_old, 0);
 		return ret;
 	}
 
 	/* scaling down?  scale voltage after frequency */
 	if (new_freq < old_freq) {
-		ret = regulator_set_voltage_tol(arm_reg, volt, 0);
+		ret = regulator_set_voltage_tol(reg, volt, 0);
 		if (ret) {
 			dev_warn(cpu_dev,
 				 "failed to scale vddarm down: %d\n", ret);
 			ret = 0;
 		}
-		ret = regulator_set_voltage_tol(soc_reg, imx6_soc_volt[index], 0);
-		if (ret) {
-			dev_warn(cpu_dev, "failed to scale vddsoc down: %d\n", ret);
-			ret = 0;
-		}
-		if (!IS_ERR(pu_reg)) {
-			ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
+		if (!IS_ERR(vddsoc_reg)) {
+			ret = regulator_set_voltage_tol(vddsoc_reg, imx6_soc_volt[index], 0);
+			if (ret) {
+				dev_warn(cpu_dev, "failed to scale vddsoc down: %d\n", ret);
+				ret = 0;
+			}
+		} else {
+			ret = regulator_set_voltage_tol(soc_reg, imx6_soc_volt[index], 0);
 			if (ret) {
-				dev_warn(cpu_dev, "failed to scale vddpu down: %d\n", ret);
+				dev_warn(cpu_dev, "failed to scale vddsoc down: %d\n", ret);
 				ret = 0;
 			}
+			if (!IS_ERR(pu_reg)) {
+				ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
+				if (ret) {
+					dev_warn(cpu_dev, "failed to scale vddpu down: %d\n", ret);
+					ret = 0;
+				}
+			}
 		}
 	}
 
@@ -155,7 +176,10 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 {
 	struct device_node *np;
 	struct dev_pm_opp *opp;
+	struct regulator *reg;
 	unsigned long min_volt, max_volt;
+	unsigned long vddarm_old = 0;
+	unsigned long vddsoc_old = 0;
 	int num, ret;
 	const struct property *prop;
 	const __be32 *val;
@@ -185,15 +209,31 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 		goto put_clk;
 	}
 
+	vddarm_reg = regulator_get_optional(cpu_dev, "vddarm");
+	vddsoc_reg = regulator_get_optional(cpu_dev, "vddsoc");
 	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(vddarm_reg) == -EPROBE_DEFER ||
+	    PTR_ERR(vddsoc_reg) == -EPROBE_DEFER ||
+	    PTR_ERR(arm_reg) == -EPROBE_DEFER ||
+	    PTR_ERR(pu_reg) == -EPROBE_DEFER ||
+	    PTR_ERR(soc_reg) == -EPROBE_DEFER) {
+		ret = -EPROBE_DEFER;
+		goto put_reg;
+	}
 	if (IS_ERR(arm_reg) || IS_ERR(soc_reg)) {
 		dev_err(cpu_dev, "failed to get regulators\n");
 		ret = -ENOENT;
 		goto put_reg;
 	}
 
+	if (!IS_ERR(vddarm_reg) && vddarm_reg == vddsoc_reg) {
+		dev_warn(cpu_dev, "arm and pu/soc supplied from the same regulator\n");
+		ret = -EINVAL;
+		goto put_reg;
+	}
+
 	/*
 	 * We expect an OPP table supplied by platform.
 	 * Just, incase the platform did not supply the OPP
@@ -269,13 +309,19 @@ soc_opp_out:
 	 * Calculate the ramp time for max voltage change in the
 	 * VDDSOC and VDDPU regulators.
 	 */
-	ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
-	if (ret > 0)
-		transition_latency += ret * 1000;
-	if (!IS_ERR(pu_reg)) {
-		ret = regulator_set_voltage_time(pu_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
+	if (!IS_ERR(vddsoc_reg)) {
+		ret = regulator_set_voltage_time(vddsoc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
+		if (ret > 0)
+			transition_latency += ret * 1000;
+	} else {
+		ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
 		if (ret > 0)
 			transition_latency += ret * 1000;
+		if (!IS_ERR(pu_reg)) {
+			ret = regulator_set_voltage_time(pu_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
+			if (ret > 0)
+				transition_latency += ret * 1000;
+		}
 	}
 
 	/*
@@ -291,22 +337,72 @@ soc_opp_out:
 				  freq_table[--num].frequency * 1000, true);
 	max_volt = dev_pm_opp_get_voltage(opp);
 	rcu_read_unlock();
-	ret = regulator_set_voltage_time(arm_reg, min_volt, max_volt);
+
+	reg = vddarm_reg ? vddarm_reg : arm_reg;
+	ret = regulator_set_voltage_time(reg, min_volt, max_volt);
 	if (ret > 0)
 		transition_latency += ret * 1000;
 
+	/* Bypass internal LDOs if external supplies are given */
+	if (vddarm_reg) {
+		vddarm_old = regulator_get_voltage(vddarm_reg);
+		regulator_set_voltage_tol(vddarm_reg, max_volt, 0);
+		ret = regulator_allow_bypass(arm_reg, true);
+		if (ret) {
+			dev_err(cpu_dev, "failed to bypass arm supply: %d\n",
+				ret);
+			goto restore_vddarm;
+		}
+	}
+	if (vddsoc_reg) {
+		vddsoc_old = regulator_get_voltage(vddsoc_reg);
+		if (freq_table[num].frequency == FREQ_1P2_GHZ / 1000)
+			regulator_set_voltage_tol(vddsoc_reg,
+						  PU_SOC_VOLTAGE_HIGH, 0);
+		else
+			regulator_set_voltage_tol(vddsoc_reg,
+						  PU_SOC_VOLTAGE_NORMAL, 0);
+		ret = regulator_allow_bypass(pu_reg, true);
+		if (ret) {
+			dev_err(cpu_dev, "failed to bypass pu supply: %d\n",
+				ret);
+			goto restore_vddsoc;
+		}
+		ret = regulator_allow_bypass(soc_reg, true);
+		if (ret) {
+			dev_err(cpu_dev, "failed to bypass soc supply: %d\n",
+				ret);
+			goto restore_vddsoc;
+		}
+	}
+
 	ret = cpufreq_register_driver(&imx6q_cpufreq_driver);
 	if (ret) {
 		dev_err(cpu_dev, "failed register driver: %d\n", ret);
-		goto free_freq_table;
+		goto restore_vddsoc;
 	}
 
 	of_node_put(np);
 	return 0;
 
+restore_vddsoc:
+	if (vddsoc_reg) {
+		regulator_allow_bypass(pu_reg, false);
+		regulator_allow_bypass(soc_reg, false);
+		regulator_set_voltage_tol(vddsoc_reg, vddsoc_old, 0);
+	}
+restore_vddarm:
+	if (vddarm_reg) {
+		regulator_allow_bypass(arm_reg, false);
+		regulator_set_voltage_tol(vddarm_reg, vddarm_old, 0);
+	}
 free_freq_table:
 	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
 put_reg:
+	if (!IS_ERR_OR_NULL(vddarm_reg))
+		regulator_put(vddarm_reg);
+	if (!IS_ERR_OR_NULL(vddsoc_reg))
+		regulator_put(vddsoc_reg);
 	if (!IS_ERR(arm_reg))
 		regulator_put(arm_reg);
 	if (!IS_ERR(pu_reg))
@@ -332,6 +428,10 @@ static int imx6q_cpufreq_remove(struct platform_device *pdev)
 {
 	cpufreq_unregister_driver(&imx6q_cpufreq_driver);
 	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
+	if (vddarm_reg)
+		regulator_put(vddarm_reg);
+	if (vddsoc_reg)
+		regulator_put(vddsoc_reg);
 	regulator_put(arm_reg);
 	if (!IS_ERR(pu_reg))
 		regulator_put(pu_reg);
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 1/2] regulator: anatop: allow LDO bypass to be set from device-tree
  2014-09-26  8:57   ` Philipp Zabel
@ 2014-09-29  5:47     ` Tim Harvey
  0 siblings, 0 replies; 6+ messages in thread
From: Tim Harvey @ 2014-09-29  5:47 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Sep 26, 2014 at 1:57 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Tim,
>
> Am Donnerstag, den 25.09.2014, 22:59 -0700 schrieb Tim Harvey:
>> The core regulators provided by the Anatop regulator (reg_core, reg_soc,
>> reg_pu) should be set in bypass mode if an extern PMIC regulator is used.
>
> Agreed, but ...
>
>> This allows configuring this from device-tree.
>
> ... this should not be configured from the device tree.
>
> If the PMIC driver is not loaded or cpufreq is disabled for other
> reasons, the full LDO input voltage will be passed through to the core
> all the time. The bypass should only be enabled if there is actually a
> driver to reduce the voltage of the PMIC regulators.

Yes, I agree

>
> I have a patch to imx6q-cpufreq (see below) that uses the regulator API
> to enable the bypass on the core LDOs if additional PMIC regulators are
> provided in the device tree and regulator-allow-bypass is set on the LDO
> regulators. But it has its own problems.

So you too the third approach I mention in my cover-letter where you
determine if there are external regulators. You use new props in the
cpu0 node to point to them which makes sense (this is the part that I
couldn't figure out how to do properly). I think this approach is the
best approach, but I think it needs some more work to deal with the
1.2GHz case.

> First, it adds even more regulators. I'd rather move the voltage
> regulation details out of imx6-cpufreq instead, because ideally that
> should only have to care about the ARM voltage.

imx6-cpufreq also needs to care about the SOC voltage.

I don't think we can move the regulation details out of imx6-cpufreq
because of the special handling of the 1.2GHz case.

> Second, regulator_allow_bypass does nothing if there are other consumers
> that have not allowed bypass yet, but immediately enables bypass once
> the last registered consumer allows it. In our case the PU power domain
> driver is, or rather will be, a second consumer of the VDDSOC regulator,
> even though it does not care about the voltage level. This makes the
> allow-bypass ordering a bit of a hassle.
>
> regards
> Philipp
>
> --8<--
> From: Philipp Zabel <p.zabel@pengutronix.de>
> Subject: [PATCH] cpufreq: imx6q: Add support for bypass modes
>
> The internal LDOs can be set to bypass if separate external regulators
> are available for VDDARM and VDDSOC inputs.
>
> To use external switching regulators, they have to be added to the cpu0
> device tree node using the vddarm-supply and vddsoc-supply properties.
> Further, the reg_arm, reg_pu, and reg_soc regulators need to have the
> regulator-allow-bypass property set.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/cpufreq/imx6q-cpufreq.c | 152 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 126 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index c2d3076..c0b3558 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -23,6 +23,8 @@
>  static struct regulator *arm_reg;
>  static struct regulator *pu_reg;
>  static struct regulator *soc_reg;
> +static struct regulator *vddarm_reg;
> +static struct regulator *vddsoc_reg;

maybe a comment should be here that arm_reg/pu_reg/soc_reg are the
internal IMX6 anatop regulators and vddarm_reg/vddsoc_reg are external
PMIC regulators.

>
>  static struct clk *arm_clk;
>  static struct clk *pll1_sys_clk;
> @@ -40,6 +42,7 @@ static u32 soc_opp_count;
>  static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
>  {
>         struct dev_pm_opp *opp;
> +       struct regulator *reg;
>         unsigned long freq_hz, volt, volt_old;
>         unsigned int old_freq, new_freq;
>         int ret;
> @@ -64,21 +67,31 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
>                 old_freq / 1000, volt_old / 1000,
>                 new_freq / 1000, volt / 1000);
>
> +       reg = vddarm_reg ? vddarm_reg : arm_reg;
> +
>         /* scaling up?  scale voltage before frequency */
>         if (new_freq > old_freq) {
> -               if (!IS_ERR(pu_reg)) {
> -                       ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
> +               if (!IS_ERR(vddsoc_reg)) {
> +                       ret = regulator_set_voltage_tol(vddsoc_reg, imx6_soc_volt[index], 0);
>                         if (ret) {
> -                               dev_err(cpu_dev, "failed to scale vddpu up: %d\n", ret);
> +                               dev_err(cpu_dev, "failed to scale vddsoc up: %d\n", ret);
> +                               return ret;
> +                       }
> +               } else {
> +                       if (!IS_ERR(pu_reg)) {
> +                               ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
> +                               if (ret) {
> +                                       dev_err(cpu_dev, "failed to scale vddpu up: %d\n", ret);
> +                                       return ret;
> +                               }
> +                       }
> +                       ret = regulator_set_voltage_tol(soc_reg, imx6_soc_volt[index], 0);
> +                       if (ret) {
> +                               dev_err(cpu_dev, "failed to scale vddsoc up: %d\n", ret);
>                                 return ret;
>                         }
>                 }
> -               ret = regulator_set_voltage_tol(soc_reg, imx6_soc_volt[index], 0);
> -               if (ret) {
> -                       dev_err(cpu_dev, "failed to scale vddsoc up: %d\n", ret);
> -                       return ret;
> -               }
> -               ret = regulator_set_voltage_tol(arm_reg, volt, 0);
> +               ret = regulator_set_voltage_tol(reg, volt, 0);
>                 if (ret) {
>                         dev_err(cpu_dev,
>                                 "failed to scale vddarm up: %d\n", ret);
> @@ -106,29 +119,37 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
>         ret = clk_set_rate(arm_clk, new_freq * 1000);
>         if (ret) {
>                 dev_err(cpu_dev, "failed to set clock rate: %d\n", ret);
> -               regulator_set_voltage_tol(arm_reg, volt_old, 0);
> +               regulator_set_voltage_tol(reg, volt_old, 0);
>                 return ret;
>         }
>
>         /* scaling down?  scale voltage after frequency */
>         if (new_freq < old_freq) {
> -               ret = regulator_set_voltage_tol(arm_reg, volt, 0);
> +               ret = regulator_set_voltage_tol(reg, volt, 0);
>                 if (ret) {
>                         dev_warn(cpu_dev,
>                                  "failed to scale vddarm down: %d\n", ret);
>                         ret = 0;
>                 }
> -               ret = regulator_set_voltage_tol(soc_reg, imx6_soc_volt[index], 0);
> -               if (ret) {
> -                       dev_warn(cpu_dev, "failed to scale vddsoc down: %d\n", ret);
> -                       ret = 0;
> -               }
> -               if (!IS_ERR(pu_reg)) {
> -                       ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
> +               if (!IS_ERR(vddsoc_reg)) {
> +                       ret = regulator_set_voltage_tol(vddsoc_reg, imx6_soc_volt[index], 0);
> +                       if (ret) {
> +                               dev_warn(cpu_dev, "failed to scale vddsoc down: %d\n", ret);
> +                               ret = 0;
> +                       }
> +               } else {
> +                       ret = regulator_set_voltage_tol(soc_reg, imx6_soc_volt[index], 0);
>                         if (ret) {
> -                               dev_warn(cpu_dev, "failed to scale vddpu down: %d\n", ret);
> +                               dev_warn(cpu_dev, "failed to scale vddsoc down: %d\n", ret);
>                                 ret = 0;
>                         }
> +                       if (!IS_ERR(pu_reg)) {
> +                               ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
> +                               if (ret) {
> +                                       dev_warn(cpu_dev, "failed to scale vddpu down: %d\n", ret);
> +                                       ret = 0;
> +                               }
> +                       }
>                 }
>         }
>
> @@ -155,7 +176,10 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  {
>         struct device_node *np;
>         struct dev_pm_opp *opp;
> +       struct regulator *reg;
>         unsigned long min_volt, max_volt;
> +       unsigned long vddarm_old = 0;
> +       unsigned long vddsoc_old = 0;
>         int num, ret;
>         const struct property *prop;
>         const __be32 *val;
> @@ -185,15 +209,31 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>                 goto put_clk;
>         }
>
> +       vddarm_reg = regulator_get_optional(cpu_dev, "vddarm");
> +       vddsoc_reg = regulator_get_optional(cpu_dev, "vddsoc");

ok, so you've added these regs to the cpu0 device-tree node.

>         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(vddarm_reg) == -EPROBE_DEFER ||
> +           PTR_ERR(vddsoc_reg) == -EPROBE_DEFER ||
> +           PTR_ERR(arm_reg) == -EPROBE_DEFER ||
> +           PTR_ERR(pu_reg) == -EPROBE_DEFER ||
> +           PTR_ERR(soc_reg) == -EPROBE_DEFER) {
> +               ret = -EPROBE_DEFER;
> +               goto put_reg;
> +       }

So you are also making these new regulators required in the cpu0 node
correct? We will also need a patch that updates the device-tree
bindings.

>         if (IS_ERR(arm_reg) || IS_ERR(soc_reg)) {
>                 dev_err(cpu_dev, "failed to get regulators\n");
>                 ret = -ENOENT;
>                 goto put_reg;
>         }
>
> +       if (!IS_ERR(vddarm_reg) && vddarm_reg == vddsoc_reg) {
> +               dev_warn(cpu_dev, "arm and pu/soc supplied from the same regulator\n");
> +               ret = -EINVAL;
> +               goto put_reg;
> +       }
> +
>         /*
>          * We expect an OPP table supplied by platform.
>          * Just, incase the platform did not supply the OPP
> @@ -269,13 +309,19 @@ soc_opp_out:
>          * Calculate the ramp time for max voltage change in the
>          * VDDSOC and VDDPU regulators.
>          */
> -       ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
> -       if (ret > 0)
> -               transition_latency += ret * 1000;
> -       if (!IS_ERR(pu_reg)) {
> -               ret = regulator_set_voltage_time(pu_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
> +       if (!IS_ERR(vddsoc_reg)) {
> +               ret = regulator_set_voltage_time(vddsoc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
> +               if (ret > 0)
> +                       transition_latency += ret * 1000;
> +       } else {
> +               ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
>                 if (ret > 0)
>                         transition_latency += ret * 1000;
> +               if (!IS_ERR(pu_reg)) {
> +                       ret = regulator_set_voltage_time(pu_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
> +                       if (ret > 0)
> +                               transition_latency += ret * 1000;
> +               }
>         }
>
>         /*
> @@ -291,22 +337,72 @@ soc_opp_out:
>                                   freq_table[--num].frequency * 1000, true);
>         max_volt = dev_pm_opp_get_voltage(opp);
>         rcu_read_unlock();
> -       ret = regulator_set_voltage_time(arm_reg, min_volt, max_volt);
> +
> +       reg = vddarm_reg ? vddarm_reg : arm_reg;
> +       ret = regulator_set_voltage_time(reg, min_volt, max_volt);
>         if (ret > 0)
>                 transition_latency += ret * 1000;
>
> +       /* Bypass internal LDOs if external supplies are given */
> +       if (vddarm_reg) {
> +               vddarm_old = regulator_get_voltage(vddarm_reg);
> +               regulator_set_voltage_tol(vddarm_reg, max_volt, 0);
> +               ret = regulator_allow_bypass(arm_reg, true);
> +               if (ret) {
> +                       dev_err(cpu_dev, "failed to bypass arm supply: %d\n",
> +                               ret);
> +                       goto restore_vddarm;
> +               }

here we are setting vddarm_reg (the external pmic regulator) to the
max volt needed for the highest opp then attempting to put the anatop
regulator in bypass mode.

Is this where you are saying there is an issue? If another consumer
exists for vddarm_reg that may not allow bypass this will fail (but at
least you handle that).

> +       }
> +       if (vddsoc_reg) {
> +               vddsoc_old = regulator_get_voltage(vddsoc_reg);
> +               if (freq_table[num].frequency == FREQ_1P2_GHZ / 1000)
> +                       regulator_set_voltage_tol(vddsoc_reg,
> +                                                 PU_SOC_VOLTAGE_HIGH, 0);
> +               else
> +                       regulator_set_voltage_tol(vddsoc_reg,
> +                                                 PU_SOC_VOLTAGE_NORMAL, 0);
> +               ret = regulator_allow_bypass(pu_reg, true);
> +               if (ret) {
> +                       dev_err(cpu_dev, "failed to bypass pu supply: %d\n",
> +                               ret);
> +                       goto restore_vddsoc;
> +               }
> +               ret = regulator_allow_bypass(soc_reg, true);
> +               if (ret) {
> +                       dev_err(cpu_dev, "failed to bypass soc supply: %d\n",
> +                               ret);
> +                       goto restore_vddsoc;
> +               }
> +       }
> +

shouldn't we not even allow ldo bypass for 1.2GHz? I'm having trouble
finding where Freescale specifies this but I have seen it mentioned in
their pfuze code.

>         ret = cpufreq_register_driver(&imx6q_cpufreq_driver);
>         if (ret) {
>                 dev_err(cpu_dev, "failed register driver: %d\n", ret);
> -               goto free_freq_table;
> +               goto restore_vddsoc;
>         }
>
>         of_node_put(np);
>         return 0;
>
> +restore_vddsoc:
> +       if (vddsoc_reg) {
> +               regulator_allow_bypass(pu_reg, false);
> +               regulator_allow_bypass(soc_reg, false);
> +               regulator_set_voltage_tol(vddsoc_reg, vddsoc_old, 0);
> +       }
> +restore_vddarm:
> +       if (vddarm_reg) {
> +               regulator_allow_bypass(arm_reg, false);
> +               regulator_set_voltage_tol(vddarm_reg, vddarm_old, 0);
> +       }
>  free_freq_table:
>         dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
>  put_reg:
> +       if (!IS_ERR_OR_NULL(vddarm_reg))
> +               regulator_put(vddarm_reg);
> +       if (!IS_ERR_OR_NULL(vddsoc_reg))
> +               regulator_put(vddsoc_reg);
>         if (!IS_ERR(arm_reg))
>                 regulator_put(arm_reg);
>         if (!IS_ERR(pu_reg))
> @@ -332,6 +428,10 @@ static int imx6q_cpufreq_remove(struct platform_device *pdev)
>  {
>         cpufreq_unregister_driver(&imx6q_cpufreq_driver);
>         dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
> +       if (vddarm_reg)
> +               regulator_put(vddarm_reg);
> +       if (vddsoc_reg)
> +               regulator_put(vddsoc_reg);
>         regulator_put(arm_reg);
>         if (!IS_ERR(pu_reg))
>                 regulator_put(pu_reg);
> --
> 2.1.0
>
>

Additionally however, we can't forget about the case where a 1.2GHz
capable CPU with external PMIC is shifting to 1.2GHz in which case LDO
bypass should be disabled and re-enabled when shifting back. This also
makes me think that whatever is doing this shifting (the cpufreq
driver) needs to manage this and needs to bump the PMIC regulators by
125mV in the case of enabling the LDO for 1.2GHz.

Also, what about the case when external PMIC regulators are used and
yet LDO's are still enabled (not bypassed) - in this case the cpufreq
driver should be setting the voltages to 125mV over the operating
points. This would be occurring now with boards that have external
PMIC's and do not use a bootloader that sets them in LDO bypass mode.

Regards,

Tim

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2014-09-29  5:47 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-26  5:59 [PATCH RFC 0/2] ARM: imx: ventana: enable LDO-bypass mode Tim Harvey
2014-09-26  5:59 ` [PATCH 1/2] regulator: anatop: allow LDO bypass to be set from device-tree Tim Harvey
2014-09-26  8:57   ` Philipp Zabel
2014-09-29  5:47     ` Tim Harvey
2014-09-26  5:59 ` [PATCH 2/2] ARM: imx: ventana: enable LDO bypass mode for GW54xx Tim Harvey
2014-09-26  7:20 ` [PATCH RFC 0/2] ARM: imx: ventana: enable LDO-bypass mode Stefan Wahren

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).