[RFC] regulator: Propagate voltage changes to supply regulators

[RFC] regulator: Propagate voltage changes to supply regulators

[Sascha Hauer]

Until now changing the voltage of a regulator only ever effected the
regulator itself, but never its supplies. It's a common pattern though
to put LDO regulators behind switching regulators. The switching
regulators efficiently drop the input voltage but have a high ripple on
their output. The output is then cleaned up by the LDOs. For higher
energy efficiency the voltage drop at the LDOs should be minimized. This
patch adds support for such a scenario.

A new min_dropout_uv field is added to struct regulator_desc. Regulators
can specify the minimun dropout voltage they need for proper function
here. Now when the voltage is changed on a regulator the regulator core
makes sure that
a) before increasing the voltage on the current regulator the supply
   provides at least the desired voltage plus the minimum dropout
b) after decreasing the voltage on the current regulator the supply
   is optimized to the minimum required voltage within the needs of
   the consumers of the supply.

Calculating the optimum voltage for the supply regulator is a bit tricky
since the simple approach of just adding the desired minimum voltage and
the minimum dropout is not enough. It may happen that the current
regulator does not support the desired minimum voltage, but only a
higher one. This means we have to figure out the lowest voltage
supported by the regulator that is higher than the minimum desired
voltage. The regulator_get_voltage_floor introduced with this series
does
exactly that.

This is a first RFC for this series which probably has some rough edges,
but it was already tested successfully on a Phytec PFLA02 i.MX6 board.

Please review, any input welcome.

Sascha

[PATCH 1/6] Revert "regulator: core: Handle full constraints systems when resolving supplies"

[Sascha Hauer]

This reverts commit 9f7e25edb1575a6d2363dc003f9cc09d840657e2.

When a regulator A is registered and is supplied by regulator B which is
not yet registered then a regulator_get on regulator A will set the As
supply to the dummy regulator. This is not correct, we should return
-EPROBE_DEFER instead as done without this patch.
Of course reverting this patch brings back the issue it fixed, so this
is not a solution, but what is the correct solution?

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/core.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 7849187..bd9db70 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1403,13 +1403,9 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 			return 0;
 		}
 
-		if (have_full_constraints()) {
-			r = dummy_regulator_rdev;
-		} else {
-			dev_err(dev, "Failed to resolve %s-supply for %s\n",
-				rdev->supply_name, rdev->desc->name);
-			return -EPROBE_DEFER;
-		}
+		dev_err(dev, "Failed to resolve %s-supply for %s\n",
+			rdev->supply_name, rdev->desc->name);
+		return -EPROBE_DEFER;
 	}
 
 	/* Recursively resolve the supply of the supply */
-- 
2.5.3

[PATCH 2/6] regulator: core: introduce function to lock regulators and its supplies

[Sascha Hauer]

Each regulator_dev is locked with its own mutex. This is fine as long
as only one regulator_dev is locked, but makes lockdep unhappy when we
have to walk up the supply chain like it can happen in
regulator_get_voltage:

regulator_get_voltage ->
 mutex_lock(&regulator->rdev->mutex) ->
_regulator_get_voltage(regulator->rdev) ->
regulator_get_voltage(rdev->supply) ->
mutex_lock(&regulator->rdev->mutex);

This causes lockdep to issue a possible deadlock warning.

There are at least two ways to work around this:

- We can always lock the whole supply chain using the functions
  introduced with this patch.
- We could store the current voltage in struct regulator_rdev so
  that we do not have to walk up the supply chain for the
  _regulator_get_voltage case.

Anyway, regulator_lock_supply/regulator_unlock_supply will be needed
once we allow regulator_set_voltage to optimize the supply voltages.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/core.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index bd9db70..e5de3d9 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -132,6 +132,45 @@ static bool have_full_constraints(void)
 }
 
 /**
+ * regulator_lock_supply - lock a regulator and its supplies
+ * @rdev:         regulator source
+ */
+static void regulator_lock_supply(struct regulator_dev *rdev)
+{
+	struct regulator *supply;
+	int i = 0;
+
+	while (1) {
+		mutex_lock_nested(&rdev->mutex, i++);
+		supply = rdev->supply;
+
+		if (!rdev->supply)
+			return;
+
+		rdev = supply->rdev;
+	}
+}
+
+/**
+ * regulator_unlock_supply - unlock a regulator and its supplies
+ * @rdev:         regulator source
+ */
+static void regulator_unlock_supply(struct regulator_dev *rdev)
+{
+	struct regulator *supply;
+
+	while (1) {
+		mutex_unlock(&rdev->mutex);
+		supply = rdev->supply;
+
+		if (!rdev->supply)
+			return;
+
+		rdev = supply->rdev;
+	}
+}
+
+/**
  * of_get_regulator - get a regulator device node based on supply name
  * @dev: Device pointer for the consumer (of regulator) device
  * @supply: regulator supply name
-- 
2.5.3

[PATCH 3/6] regulator: core: create unlocked version of regulator_list_voltage

[Sascha Hauer]

The unlocked version will be needed when we start propagating voltage
changes to the supply regulators.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/core.c | 62 +++++++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 27 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e5de3d9..d148545 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2343,6 +2343,40 @@ static int _regulator_is_enabled(struct regulator_dev *rdev)
 	return rdev->desc->ops->is_enabled(rdev);
 }
 
+static int _regulator_list_voltage(struct regulator *regulator,
+				    unsigned selector, int lock)
+{
+	struct regulator_dev *rdev = regulator->rdev;
+	const struct regulator_ops *ops = rdev->desc->ops;
+	int ret;
+
+	if (rdev->desc->fixed_uV && rdev->desc->n_voltages == 1 && !selector)
+		return rdev->desc->fixed_uV;
+
+	if (ops->list_voltage) {
+		if (selector >= rdev->desc->n_voltages)
+			return -EINVAL;
+		if (lock)
+			mutex_lock(&rdev->mutex);
+		ret = ops->list_voltage(rdev, selector);
+		if (lock)
+			mutex_unlock(&rdev->mutex);
+	} else if (rdev->supply) {
+		ret = _regulator_list_voltage(rdev->supply, selector, lock);
+	} else {
+		return -EINVAL;
+	}
+
+	if (ret > 0) {
+		if (ret < rdev->constraints->min_uV)
+			ret = 0;
+		else if (ret > rdev->constraints->max_uV)
+			ret = 0;
+	}
+
+	return ret;
+}
+
 /**
  * regulator_is_enabled - is the regulator output enabled
  * @regulator: regulator source
@@ -2432,33 +2466,7 @@ EXPORT_SYMBOL_GPL(regulator_count_voltages);
  */
 int regulator_list_voltage(struct regulator *regulator, unsigned selector)
 {
-	struct regulator_dev *rdev = regulator->rdev;
-	const struct regulator_ops *ops = rdev->desc->ops;
-	int ret;
-
-	if (rdev->desc->fixed_uV && rdev->desc->n_voltages == 1 && !selector)
-		return rdev->desc->fixed_uV;
-
-	if (ops->list_voltage) {
-		if (selector >= rdev->desc->n_voltages)
-			return -EINVAL;
-		mutex_lock(&rdev->mutex);
-		ret = ops->list_voltage(rdev, selector);
-		mutex_unlock(&rdev->mutex);
-	} else if (rdev->supply) {
-		ret = regulator_list_voltage(rdev->supply, selector);
-	} else {
-		return -EINVAL;
-	}
-
-	if (ret > 0) {
-		if (ret < rdev->constraints->min_uV)
-			ret = 0;
-		else if (ret > rdev->constraints->max_uV)
-			ret = 0;
-	}
-
-	return ret;
+	return _regulator_list_voltage(regulator, selector, 1);
 }
 EXPORT_SYMBOL_GPL(regulator_list_voltage);
 
-- 
2.5.3

[PATCH 4/6] regulator: core: Propagate voltage changes to supply regulators

[Sascha Hauer]

Until now changing the voltage of a regulator only ever effected the
regulator itself, but never its supplies. It's a common pattern though
to put LDO regulators behind switching regulators. The switching
regulators efficiently drop the input voltage but have a high ripple on
their output. The output is then cleaned up by the LDOs. For higher
energy efficiency the voltage drop at the LDOs should be minimized. This
patch adds support for such a scenario.

A new min_dropout_uv field is added to struct regulator_desc. Regulators
can specify the minimun dropout voltage they need for proper function
here. Now when the voltage is changed on a regulator the regulator core
makes sure that
a) before increasing the voltage on the current regulator the supply
   provides at least the desired voltage plus the minimum dropout
b) after decreasing the voltage on the current regulator the supply
   is optimized to the minimum required voltage within the needs of
   the consumers of the supply.

Calculating the optimum voltage for the supply regulator is a bit tricky
since the simple approach of just adding the desired minimum voltage and
the minimum dropout is not enough. It may happen that the current
regulator does not support the desired minimum voltage, but only a
higher one. This means we have to figure out the lowest voltage
supported by the regulator that is higher than the minimum desired
voltage. The regulator_get_voltage_floor introduced with this patch does
exactly that.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/core.c         | 136 ++++++++++++++++++++++++++++++++-------
 include/linux/regulator/driver.h |   2 +
 2 files changed, 116 insertions(+), 22 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d148545..36da924 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2756,32 +2756,54 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 	return ret;
 }
 
-/**
- * regulator_set_voltage - set regulator output voltage
- * @regulator: regulator source
- * @min_uV: Minimum required voltage in uV
- * @max_uV: Maximum acceptable voltage in uV
- *
- * Sets a voltage regulator to the desired output voltage. This can be set
- * during any regulator state. IOW, regulator can be disabled or enabled.
- *
- * If the regulator is enabled then the voltage will change to the new value
- * immediately otherwise if the regulator is disabled the regulator will
- * output at the new voltage when enabled.
- *
- * NOTE: If the regulator is shared between several devices then the lowest
- * request voltage that meets the system constraints will be used.
- * Regulator system constraints must be set for this regulator before
- * calling this function otherwise this call will fail.
+/*
+ * Return the minimum voltage supported by a regulator that is higher or equal
+ * to a given voltage.
  */
-int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
+static int regulator_get_voltage_floor(struct regulator *regulator, int min_uV)
+{
+	struct regulator_dev *rdev = regulator->rdev;
+	int num_voltages;
+	int best = INT_MAX;
+	int max_uV = INT_MAX;
+	int i, now, ret;
+
+	/* constraints check */
+	ret = regulator_check_voltage(rdev, &min_uV, &max_uV);
+	if (ret < 0)
+		return ret;
+
+	ret = regulator_check_consumers(rdev, &min_uV, &max_uV);
+	if (ret < 0)
+		return ret;
+
+	num_voltages = regulator_count_voltages(regulator);
+	if (num_voltages < 0)
+		return num_voltages;
+
+	for (i = 0; i < num_voltages; i++) {
+		now = _regulator_list_voltage(regulator, i, 0);
+		if (now < 0)
+			continue;
+		if (now < best && now >= min_uV)
+			best = now;
+	}
+
+	if (best > max_uV)
+		return -EINVAL;
+
+	return best;
+}
+
+static int regulator_set_voltage_unlocked(struct regulator *regulator,
+					  int min_uV, int max_uV)
 {
 	struct regulator_dev *rdev = regulator->rdev;
 	int ret = 0;
 	int old_min_uV, old_max_uV;
 	int current_uV;
-
-	mutex_lock(&rdev->mutex);
+	int best_supply_uV = 0;
+	int supply_change_uV = 0;
 
 	/* If we're setting the same range as last time the change
 	 * should be a noop (some cpufreq implementations use the same
@@ -2825,17 +2847,87 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
 	if (ret < 0)
 		goto out2;
 
+	if (rdev->supply) {
+		int current_supply_uV;
+
+		best_supply_uV = regulator_get_voltage_floor(regulator, min_uV);
+		if (best_supply_uV < 0) {
+			ret = best_supply_uV;
+			goto out2;
+		}
+
+		best_supply_uV += rdev->desc->min_dropout_uv;
+
+		current_supply_uV = _regulator_get_voltage(rdev->supply->rdev);
+		if (current_supply_uV < 0) {
+			ret = current_supply_uV;
+			goto out2;
+		}
+
+		supply_change_uV = best_supply_uV - current_supply_uV;
+	}
+
+	if (supply_change_uV > 0) {
+		ret = regulator_set_voltage_unlocked(rdev->supply,
+				best_supply_uV, INT_MAX);
+		if (ret) {
+			dev_err(&rdev->dev, "Failed to increase supply voltage: %d\n",
+					ret);
+			goto out2;
+		}
+	}
+
 	ret = _regulator_do_set_voltage(rdev, min_uV, max_uV);
 	if (ret < 0)
 		goto out2;
 
+	if (supply_change_uV < 0) {
+		ret = regulator_set_voltage_unlocked(rdev->supply,
+				best_supply_uV, INT_MAX);
+		if (ret)
+			dev_warn(&rdev->dev, "Failed to decrease supply voltage: %d\n",
+					ret);
+		/* No need to fail here */
+		ret = 0;
+	}
+
 out:
-	mutex_unlock(&rdev->mutex);
 	return ret;
 out2:
 	regulator->min_uV = old_min_uV;
 	regulator->max_uV = old_max_uV;
-	mutex_unlock(&rdev->mutex);
+
+	return ret;
+}
+
+/**
+ * regulator_set_voltage - set regulator output voltage
+ * @regulator: regulator source
+ * @min_uV: Minimum required voltage in uV
+ * @max_uV: Maximum acceptable voltage in uV
+ *
+ * Sets a voltage regulator to the desired output voltage. This can be set
+ * during any regulator state. IOW, regulator can be disabled or enabled.
+ *
+ * If the regulator is enabled then the voltage will change to the new value
+ * immediately otherwise if the regulator is disabled the regulator will
+ * output at the new voltage when enabled.
+ *
+ * NOTE: If the regulator is shared between several devices then the lowest
+ * request voltage that meets the system constraints will be used.
+ * Regulator system constraints must be set for this regulator before
+ * calling this function otherwise this call will fail.
+ */
+int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
+{
+	int ret = 0;
+
+	regulator_lock_supply(regulator->rdev);
+
+	ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV);
+
+	regulator_unlock_supply(regulator->rdev);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_set_voltage);
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 4593222..a3815bd 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -245,6 +245,7 @@ enum regulator_type {
  * @linear_min_sel: Minimal selector for starting linear mapping
  * @fixed_uV: Fixed voltage of rails.
  * @ramp_delay: Time to settle down after voltage change (unit: uV/us)
+ * @min_dropout_uv: The minimum dropout voltage this regulator can handle
  * @linear_ranges: A constant table of possible voltage ranges.
  * @n_linear_ranges: Number of entries in the @linear_ranges table.
  * @volt_table: Voltage mapping table (if table based mapping)
@@ -292,6 +293,7 @@ struct regulator_desc {
 	unsigned int linear_min_sel;
 	int fixed_uV;
 	unsigned int ramp_delay;
+	int min_dropout_uv;
 
 	const struct regulator_linear_range *linear_ranges;
 	int n_linear_ranges;
-- 
2.5.3

[PATCH 5/6] regulator: i.MX anatop: Allow supply regulator

[Sascha Hauer]

The anatop regulators are SoC internal LDO regulators usually supplied
by an external PMIC. This patch adds support for specifying the supply
from the device tree using the vin-supply property.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 Documentation/devicetree/bindings/regulator/anatop-regulator.txt | 1 +
 drivers/regulator/anatop-regulator.c                             | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/anatop-regulator.txt b/Documentation/devicetree/bindings/regulator/anatop-regulator.txt
index 758eae2..37c4ea0 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
+- vin-supply: The supply for this regulator
 
 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 52ea605..ba78870 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -30,6 +30,7 @@
 #include <linux/regmap.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/of_regulator.h>
+#include <linux/regulator/machine.h>
 
 #define LDO_RAMP_UP_UNIT_IN_CYCLES      64 /* 64 cycles per step */
 #define LDO_RAMP_UP_FREQ_IN_MHZ         24 /* cycle based on 24M OSC */
@@ -199,6 +200,7 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 	rdesc->owner = THIS_MODULE;
 
 	initdata = of_get_regulator_init_data(dev, np, rdesc);
+	initdata->supply_regulator = "vin";
 	sreg->initdata = initdata;
 
 	anatop_np = of_get_parent(np);
@@ -262,6 +264,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;
 
 	config.dev = &pdev->dev;
 	config.init_data = initdata;
-- 
2.5.3

[PATCH 6/6] ARM: i.MX6 Phytec PFLA02: Add supplies for the SoC internal regulators

[Sascha Hauer]

The SoC internal regulators for the CPU and the SoC come from the
DA9063 vdd_core and vdd_soc. Add this relationship to the device tree
so that the voltage drop on the SoC internal LDO regulators can be
minimized.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/boot/dts/imx6qdl-phytec-pfla02.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-phytec-pfla02.dtsi b/arch/arm/boot/dts/imx6qdl-phytec-pfla02.dtsi
index 9e6ecd9..f19c680 100644
--- a/arch/arm/boot/dts/imx6qdl-phytec-pfla02.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-phytec-pfla02.dtsi
@@ -379,6 +379,18 @@
 	status = "disabled";
 };
 
+&reg_arm {
+	vin-supply = <&vddcore_reg>;
+};
+
+&reg_pu {
+	vin-supply = <&vddsoc_reg>;
+};
+
+&reg_soc {
+	vin-supply = <&vddsoc_reg>;
+};
+
 &uart3 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_uart3>;
-- 
2.5.3

Re: [PATCH 2/6] regulator: core: introduce function to lock regulators and its supplies

[kbuild test robot]

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

Hi Sascha,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: x86_64-randconfig-i0-201539 (attached as .config)
reproduce:
  git checkout 70f6f03239f72ac3a34fad792af5200c613f4dbc
  # save the attached .config to linux build tree
  make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/regulator/core.c: In function 'regulator_lock_supply':
>> drivers/regulator/core.c:141:6: warning: unused variable 'i' [-Wunused-variable]
     int i = 0;
         ^
   drivers/regulator/core.c: At top level:
   drivers/regulator/core.c:138:13: warning: 'regulator_lock_supply' defined but not used [-Wunused-function]
    static void regulator_lock_supply(struct regulator_dev *rdev)
                ^
   drivers/regulator/core.c:158:13: warning: 'regulator_unlock_supply' defined but not used [-Wunused-function]
    static void regulator_unlock_supply(struct regulator_dev *rdev)
                ^

vim +/i +141 drivers/regulator/core.c

   125		else
   126			return "";
   127	}
   128	
   129	static bool have_full_constraints(void)
   130	{
   131		return has_full_constraints || of_have_populated_dt();
   132	}
   133	
   134	/**
   135	 * regulator_lock_supply - lock a regulator and its supplies
   136	 * @rdev:         regulator source
   137	 */
   138	static void regulator_lock_supply(struct regulator_dev *rdev)
   139	{
   140		struct regulator *supply;
 > 141		int i = 0;
   142	
   143		while (1) {
   144			mutex_lock_nested(&rdev->mutex, i++);
   145			supply = rdev->supply;
   146	
   147			if (!rdev->supply)
   148				return;
   149	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 25033 bytes --]

Re: [PATCH 1/6] Revert "regulator: core: Handle full constraints systems when resolving supplies"

[Mark Brown]

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

On Wed, Sep 30, 2015 at 04:05:41PM +0200, Sascha Hauer wrote:
> This reverts commit 9f7e25edb1575a6d2363dc003f9cc09d840657e2.
> 
> When a regulator A is registered and is supplied by regulator B which is
> not yet registered then a regulator_get on regulator A will set the As
> supply to the dummy regulator. This is not correct, we should return
> -EPROBE_DEFER instead as done without this patch.

What makes you say this is not correct?  In a system with fully
specified supplies if we fail to resolve the supply we know that no
supply will ever appear and so substitute in the dummy on the assumption
that there is a supply with no software control.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 3/6] regulator: core: create unlocked version of regulator_list_voltage

[Mark Brown]

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

On Wed, Sep 30, 2015 at 04:05:43PM +0200, Sascha Hauer wrote:
> The unlocked version will be needed when we start propagating voltage
> changes to the supply regulators.

I'm now wondering why we have a locked _list_voltage() in the first
place...  it *should* be a pure function that doesn't need any locking.
I'll check tomorrow if there are any users who care but if there aren't
the change here is probably to remove the locking entirely.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 3/6] regulator: core: create unlocked version of regulator_list_voltage

[Mark Brown]

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

On Wed, Sep 30, 2015 at 11:36:03PM +0100, Mark Brown wrote:
> On Wed, Sep 30, 2015 at 04:05:43PM +0200, Sascha Hauer wrote:
> > The unlocked version will be needed when we start propagating voltage
> > changes to the supply regulators.
> 
> I'm now wondering why we have a locked _list_voltage() in the first
> place...  it *should* be a pure function that doesn't need any locking.
> I'll check tomorrow if there are any users who care but if there aren't
> the change here is probably to remove the locking entirely.

Hrm, looks like there's one that *might* need it - let's go with this
for now, we can always fix up later.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 4/6] regulator: core: Propagate voltage changes to supply regulators

[Mark Brown]

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

On Wed, Sep 30, 2015 at 04:05:44PM +0200, Sascha Hauer wrote:

> A new min_dropout_uv field is added to struct regulator_desc. Regulators
> can specify the minimun dropout voltage they need for proper function
> here. Now when the voltage is changed on a regulator the regulator core
> makes sure that

Can we have this interface addition as a separate patch please?  Makes
it easier to add to other devices.  I could've sworn I'd already done
that bit but obviously never got the patch mailed out.

One change I think we need here is only doing the propagation if either
the device lacks a set_voltage() operation (in which case it's just a
switch passing through the parent voltage) or we have a dropout voltage,
otherwise we'll try to set the exact voltage the regulator is supposed
to output and that'll most likely end in tears.  Otherwise this is
making sense to me.

> Calculating the optimum voltage for the supply regulator is a bit tricky
> since the simple approach of just adding the desired minimum voltage and
> the minimum dropout is not enough. It may happen that the current
> regulator does not support the desired minimum voltage, but only a
> higher one. This means we have to figure out the lowest voltage
> supported by the regulator that is higher than the minimum desired
> voltage. The regulator_get_voltage_floor introduced with this patch does
> exactly that.

Yeah, split out the calcuate and the set.

Thanks for working on this!

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 6/6] ARM: i.MX6 Phytec PFLA02: Add supplies for the SoC internal regulators

[Mark Brown]

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

On Wed, Sep 30, 2015 at 04:05:46PM +0200, Sascha Hauer wrote:
> The SoC internal regulators for the CPU and the SoC come from the
> DA9063 vdd_core and vdd_soc. Add this relationship to the device tree
> so that the voltage drop on the SoC internal LDO regulators can be
> minimized.

Acked-by: Mark Brown <broonie@kernel.org>

(the binding is fine so I think we can merge this already.)

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 4/6] regulator: core: Propagate voltage changes to supply regulators

[Sascha Hauer]

Hi Mark,

On Fri, Oct 02, 2015 at 06:32:56PM +0100, Mark Brown wrote:
> On Wed, Sep 30, 2015 at 04:05:44PM +0200, Sascha Hauer wrote:
> 
> > A new min_dropout_uv field is added to struct regulator_desc. Regulators
> > can specify the minimun dropout voltage they need for proper function
> > here. Now when the voltage is changed on a regulator the regulator core
> > makes sure that
> 
> Can we have this interface addition as a separate patch please?  Makes
> it easier to add to other devices.  I could've sworn I'd already done
> that bit but obviously never got the patch mailed out.
> 
> One change I think we need here is only doing the propagation if either
> the device lacks a set_voltage() operation (in which case it's just a
> switch passing through the parent voltage)

Does the lack of a set_voltage() operation automatically mean it's a
switch passing through the parent voltage? What if the regulator is a
fixed regulator and the output can't be controlled because there is only
one voltage?

Currently we bail out int regulator_set_voltage() when we do not have a
set_voltage() or set_voltage_sel() operation. Instead of propagating the
voltage change up I would keep the current behaviour and implement voltage
propagation for switches when we need it. Then we could also introduce a
REGULATOR_IS_SWITCH flag indicating that this is a switch and not a
fixed voltage regulator.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 4/6] regulator: core: Propagate voltage changes to supply regulators

[Mark Brown]

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

On Mon, Oct 12, 2015 at 01:42:59PM +0200, Sascha Hauer wrote:
> On Fri, Oct 02, 2015 at 06:32:56PM +0100, Mark Brown wrote:

> > One change I think we need here is only doing the propagation if either
> > the device lacks a set_voltage() operation (in which case it's just a
> > switch passing through the parent voltage)

> Does the lack of a set_voltage() operation automatically mean it's a
> switch passing through the parent voltage? What if the regulator is a
> fixed regulator and the output can't be controlled because there is only
> one voltage?

Sorry, that was a typo for get_voltage().

> Currently we bail out int regulator_set_voltage() when we do not have a
> set_voltage() or set_voltage_sel() operation. Instead of propagating the
> voltage change up I would keep the current behaviour and implement voltage
> propagation for switches when we need it. Then we could also introduce a
> REGULATOR_IS_SWITCH flag indicating that this is a switch and not a
> fixed voltage regulator.

We already have people who'd like it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 4/6] regulator: core: Propagate voltage changes to supply regulators

[Sascha Hauer]

On Mon, Oct 12, 2015 at 03:07:37PM +0100, Mark Brown wrote:
> On Mon, Oct 12, 2015 at 01:42:59PM +0200, Sascha Hauer wrote:
> > On Fri, Oct 02, 2015 at 06:32:56PM +0100, Mark Brown wrote:
> 
> > > One change I think we need here is only doing the propagation if either
> > > the device lacks a set_voltage() operation (in which case it's just a
> > > switch passing through the parent voltage)
> 
> > Does the lack of a set_voltage() operation automatically mean it's a
> > switch passing through the parent voltage? What if the regulator is a
> > fixed regulator and the output can't be controlled because there is only
> > one voltage?
> 
> Sorry, that was a typo for get_voltage().

Ah, that makes it clear.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC] regulator: Propagate voltage changes to supply regulators

[Sascha Hauer]

Please do not answer to this series, it contains a wrong address for the
linux-arm-kernel list. I just resent this with the correct addres. Sorry
for the noise.

Sascha

On Wed, Sep 30, 2015 at 03:57:48PM +0200, Sascha Hauer wrote:
> Until now changing the voltage of a regulator only ever effected the
> regulator itself, but never its supplies. It's a common pattern though
> to put LDO regulators behind switching regulators. The switching
> regulators efficiently drop the input voltage but have a high ripple on
> their output. The output is then cleaned up by the LDOs. For higher
> energy efficiency the voltage drop at the LDOs should be minimized. This
> patch adds support for such a scenario.
> 
> A new min_dropout_uv field is added to struct regulator_desc. Regulators
> can specify the minimun dropout voltage they need for proper function
> here. Now when the voltage is changed on a regulator the regulator core
> makes sure that
> a) before increasing the voltage on the current regulator the supply
>    provides at least the desired voltage plus the minimum dropout
> b) after decreasing the voltage on the current regulator the supply
>    is optimized to the minimum required voltage within the needs of
>    the consumers of the supply.
> 
> Calculating the optimum voltage for the supply regulator is a bit tricky
> since the simple approach of just adding the desired minimum voltage and
> the minimum dropout is not enough. It may happen that the current
> regulator does not support the desired minimum voltage, but only a
> higher one. This means we have to figure out the lowest voltage
> supported by the regulator that is higher than the minimum desired
> voltage. The regulator_get_voltage_floor introduced with this series
> does
> exactly that.
> 
> This is a first RFC for this series which probably has some rough edges,
> but it was already tested successfully on a Phytec PFLA02 i.MX6 board.
> 
> Please review, any input welcome.
> 
> Sascha
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC] regulator: Propagate voltage changes to supply regulators

[Sascha Hauer]

Until now changing the voltage of a regulator only ever effected the
regulator itself, but never its supplies. It's a common pattern though
to put LDO regulators behind switching regulators. The switching
regulators efficiently drop the input voltage but have a high ripple on
their output. The output is then cleaned up by the LDOs. For higher
energy efficiency the voltage drop at the LDOs should be minimized. This
patch adds support for such a scenario.

A new min_dropout_uv field is added to struct regulator_desc. Regulators
can specify the minimun dropout voltage they need for proper function
here. Now when the voltage is changed on a regulator the regulator core
makes sure that
a) before increasing the voltage on the current regulator the supply
   provides at least the desired voltage plus the minimum dropout
b) after decreasing the voltage on the current regulator the supply
   is optimized to the minimum required voltage within the needs of
   the consumers of the supply.

Calculating the optimum voltage for the supply regulator is a bit tricky
since the simple approach of just adding the desired minimum voltage and
the minimum dropout is not enough. It may happen that the current
regulator does not support the desired minimum voltage, but only a
higher one. This means we have to figure out the lowest voltage
supported by the regulator that is higher than the minimum desired
voltage. The regulator_get_voltage_floor introduced with this series
does
exactly that.

This is a first RFC for this series which probably has some rough edges,
but it was already tested successfully on a Phytec PFLA02 i.MX6 board.

Please review, any input welcome.

Sascha