[PATCH v2 0/2] regulator: Add support for parallel regulators

[PATCH v2 0/2] regulator: Add support for parallel regulators

[Maxime Ripard]

Hi,

On some boards, devices consuming a lot of power cannot use a single
regulator, but few of them in parallel to spread the consumption.

In such a case, we must ensure that all these regulators are kept in
sync.

Since this is something that is totally board specific, it should
obviously not be handle by each and every customer drivers that might
be driving a device wired this way on a particular board.

Instead, we implemented a regulator driver that just aggregates
several parent regulators and just forwards the regulators calls to
them.

Let me know what you think,
Maxime

Changes from v1:
  - Added documentation
  - Fixed an error in the is_enabled callback returned value
  - Unwind the enable and disable callbacks in case of a failure
  - Added a depency on CONFIG_OF
  - Added the missing module device table

Maxime Ripard (2):
  regulator: Add coupled regulator
  ARM: sunxi: chip: Add Wifi chip

 .../bindings/regulator/coupled-voltage.txt         |  18 ++
 arch/arm/boot/dts/sun5i-r8-chip.dts                |  44 ++-
 drivers/regulator/Kconfig                          |   8 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/coupled-voltage-regulator.c      | 299 +++++++++++++++++++++
 5 files changed, 369 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/coupled-voltage.txt
 create mode 100644 drivers/regulator/coupled-voltage-regulator.c

-- 
2.6.4

[PATCH v2 1/2] regulator: Add coupled regulator

[Maxime Ripard]

Some boards, in order to power devices that have a quite high power
consumption, wire multiple regulators in parallel.

In such a case, the regulators need to be kept in sync, all of them being
enabled or disabled in parallel.

This also requires to expose only the voltages that are common to all the
regulators.

Eventually support for changing the voltage in parallel should be added
too, possibly with delays between each other to avoid having a too brutal
peak consumption.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 .../bindings/regulator/coupled-voltage.txt         |  18 ++
 drivers/regulator/Kconfig                          |   8 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/coupled-voltage-regulator.c      | 299 +++++++++++++++++++++
 4 files changed, 326 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/coupled-voltage.txt
 create mode 100644 drivers/regulator/coupled-voltage-regulator.c

diff --git a/Documentation/devicetree/bindings/regulator/coupled-voltage.txt b/Documentation/devicetree/bindings/regulator/coupled-voltage.txt
new file mode 100644
index 000000000000..f5401aab52f2
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/coupled-voltage.txt
@@ -0,0 +1,18 @@
+Coupled voltage regulators
+
+Required properties:
+- compatible		: Must be "coupled-voltage-regulator".
+
+Optional properties:
+- vinX-supply		: Phandle to the regulators it aggregates
+
+Any property defined as part of the core regulator binding defined in
+regulator.txt can also be used.
+
+Example:
+	vcc_wifi: wifi_reg {
+		compatible = "coupled-voltage-regulator";
+		regulator-name = "vcc-wifi";
+		vin0-supply = <&reg_ldo3>;
+		vin1-supply = <&reg_ldo4>;
+	};
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 7df9da82f592..5d18c9a6169f 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -35,6 +35,14 @@ config REGULATOR_FIXED_VOLTAGE
 	  useful for systems which use a combination of software
 	  managed regulators and simple non-configurable regulators.
 
+config REGULATOR_COUPLED_VOLTAGE
+	tristate "Coupled voltage regulator support"
+	depends on OF
+	help
+	  This driver provides support for regulators that are an
+	  aggregate of other regulators in the system, and need to
+	  keep them all in sync.
+
 config REGULATOR_VIRTUAL_CONSUMER
 	tristate "Virtual regulator consumer support"
 	help
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 980b1943fa81..76c9728113f7 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_REGULATOR_AS3711) += as3711-regulator.o
 obj-$(CONFIG_REGULATOR_AS3722) += as3722-regulator.o
 obj-$(CONFIG_REGULATOR_AXP20X) += axp20x-regulator.o
 obj-$(CONFIG_REGULATOR_BCM590XX) += bcm590xx-regulator.o
+obj-$(CONFIG_REGULATOR_COUPLED_VOLTAGE) += coupled-voltage-regulator.o
 obj-$(CONFIG_REGULATOR_DA903X)	+= da903x.o
 obj-$(CONFIG_REGULATOR_DA9052)	+= da9052-regulator.o
 obj-$(CONFIG_REGULATOR_DA9055)	+= da9055-regulator.o
diff --git a/drivers/regulator/coupled-voltage-regulator.c b/drivers/regulator/coupled-voltage-regulator.c
new file mode 100644
index 000000000000..c1bf798abfef
--- /dev/null
+++ b/drivers/regulator/coupled-voltage-regulator.c
@@ -0,0 +1,299 @@
+/*
+ * Copyright 2015 Free Electrons
+ * Copyright 2015 NextThing Co.
+ *
+ * Author: Maxime Ripard <maxime.ripard@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <linux/regulator/consumer.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+#define COUPLED_REGULATOR_MAX_SUPPLIES	16
+
+/**
+ * struct coupled_regulator - Private data for the driver
+ * @regulators: array of struct regulator aggregated
+ * @n_regulators: number of regulators aggregated
+ * @voltages: array of voltages common to all the regulators
+ * @n_voltages: number of common voltages
+ */
+struct coupled_regulator {
+	struct regulator	**regulators;
+	int			n_regulators;
+	int			*voltages;
+	int			n_voltages;
+};
+
+static int coupled_regulator_disable(struct regulator_dev *rdev)
+{
+	struct coupled_regulator *creg = rdev_get_drvdata(rdev);
+	int ret, i;
+
+	for (i = 0; i < creg->n_regulators; i++) {
+		ret = regulator_disable(creg->regulators[i]);
+		if (ret) {
+			dev_warn(&rdev->dev, "Couldn't disable our regulator\n");
+			goto err_unwind;
+		}
+	}
+
+	return 0;
+
+err_unwind:
+	/*
+	 * In case a regulator cannot disable, unwind everything to
+	 * make sure we're in a consistent (and probably safe) state
+	 */
+	for (; i >= 0; i--) {
+		int en_ret = regulator_enable(creg->regulators[i]);
+		if (en_ret)
+			dev_warn(&rdev->dev,
+				 "Couldn't unwind disable operation for our regulator\n");
+	}
+
+	return ret;
+}
+
+static int coupled_regulator_enable(struct regulator_dev *rdev)
+{
+	struct coupled_regulator *creg = rdev_get_drvdata(rdev);
+	int ret, i;
+
+	for (i = 0; i < creg->n_regulators; i++) {
+		ret = regulator_enable(creg->regulators[i]);
+		if (ret) {
+			dev_warn(&rdev->dev, "Couldn't enable our regulator\n");
+			goto err_unwind;
+		}
+	}
+
+	return 0;
+
+err_unwind:
+	/*
+	 * In case a regulator cannot enable, unwind everything to
+	 * make sure we're in a consistent (and probably safe) state
+	 */
+	for (; i >= 0; i--) {
+		int en_ret = regulator_disable(creg->regulators[i]);
+		if (en_ret)
+			dev_warn(&rdev->dev,
+				 "Couldn't unwind enable operation for our regulator\n");
+	}
+
+	return ret;
+}
+
+static int coupled_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	struct coupled_regulator *creg = rdev_get_drvdata(rdev);
+	int i;
+
+	for (i = 0; i < creg->n_regulators; i++) {
+		int ret = regulator_is_enabled(creg->regulators[i]);
+
+		/*
+		 * If there's at least one regulator that isn't
+		 * enabled or returns an error, return that,
+		 * otherwise, just go on.
+		 */
+		if (ret <= 0)
+			return ret;
+	}
+
+	return 1;
+}
+
+static int coupled_regulator_list_voltage(struct regulator_dev *rdev,
+					  unsigned int selector)
+{
+	struct coupled_regulator *creg = rdev_get_drvdata(rdev);
+
+	if (selector >= creg->n_voltages)
+		return -EINVAL;
+
+	return creg->voltages[selector];
+}
+
+static struct regulator_ops coupled_regulator_ops = {
+	.enable			= coupled_regulator_enable,
+	.disable		= coupled_regulator_disable,
+	.is_enabled		= coupled_regulator_is_enabled,
+	.list_voltage		= coupled_regulator_list_voltage,
+};
+
+static struct regulator_desc coupled_regulator_desc = {
+	.name		= "coupled-voltage-regulator",
+	.type		= REGULATOR_VOLTAGE,
+	.ops		= &coupled_regulator_ops,
+	.owner		= THIS_MODULE,
+};
+
+static int coupled_regulator_probe(struct platform_device *pdev)
+{
+	const struct regulator_init_data *init_data;
+	struct coupled_regulator *creg;
+	struct regulator_config config = { };
+	struct regulator_dev *regulator;
+	struct regulator_desc *desc;
+	struct device_node *np = pdev->dev.of_node;
+	int max_voltages, i;
+
+	if (!np) {
+		dev_err(&pdev->dev, "Device Tree node missing\n");
+		return -EINVAL;
+	}
+
+	creg = devm_kzalloc(&pdev->dev, sizeof(*creg), GFP_KERNEL);
+	if (!creg)
+		return -ENOMEM;
+
+	init_data = of_get_regulator_init_data(&pdev->dev, np,
+					       &coupled_regulator_desc);
+	if (!init_data)
+		return -ENOMEM;
+
+	config.of_node = np;
+	config.dev = &pdev->dev;
+	config.driver_data = creg;
+	config.init_data = init_data;
+
+	for (i = 0; i < COUPLED_REGULATOR_MAX_SUPPLIES; i++) {
+		char *propname = kasprintf(GFP_KERNEL, "vin%d-supply", i);
+		const void *prop = of_get_property(np, propname, NULL);
+		kfree(propname);
+
+		if (!prop) {
+			creg->n_regulators = i;
+			break;
+		}
+	}
+
+	dev_dbg(&pdev->dev, "Found %d parent regulators\n",
+		creg->n_regulators);
+
+	if (!creg->n_regulators) {
+		dev_err(&pdev->dev, "No parent regulators listed\n");
+		return -EINVAL;
+	}
+
+	creg->regulators = devm_kcalloc(&pdev->dev, creg->n_regulators,
+					sizeof(*creg->regulators), GFP_KERNEL);
+	if (!creg->regulators)
+		return -ENOMEM;
+
+	for (i = 0; i < creg->n_regulators; i++) {
+		char *propname = kasprintf(GFP_KERNEL, "vin%d", i);
+
+		dev_dbg(&pdev->dev, "Trying to get supply %s\n", propname);
+
+		creg->regulators[i] = devm_regulator_get(&pdev->dev, propname);
+		kfree(propname);
+
+		if (IS_ERR(creg->regulators[i])) {
+			dev_err(&pdev->dev, "Couldn't get regulator vin%d\n",
+				i);
+			return PTR_ERR(creg->regulators[i]);
+		}
+
+		/*
+		 * FIXME: We should probably be doing something
+		 * smarter here to avoid having a bunch of regulators
+		 * disabled and a bunch enabled.
+		 */
+		if (regulator_is_enabled(creg->regulators[i])) {
+			int ret = regulator_enable(creg->regulators[i]);
+			if (ret) {
+				regulator_put(creg->regulators[i]);
+				return ret;
+			}
+		}
+	}
+
+	/*
+	 * Since we want only to expose voltages that can be set on
+	 * all the regulators, we won't have more voltages supported
+	 * than the number of voltages supported by the first
+	 * regulator in our list
+	 */
+	max_voltages = regulator_count_voltages(creg->regulators[0]);
+
+	creg->voltages = devm_kcalloc(&pdev->dev, max_voltages, sizeof(int),
+				      GFP_KERNEL);
+
+	/* Build up list of supported voltages */
+	for (i = 0; i < max_voltages; i++) {
+		int voltage = regulator_list_voltage(creg->regulators[0], i);
+		bool usable = true;
+		int j;
+
+		if (voltage <= 0)
+			continue;
+
+		dev_dbg(&pdev->dev, "Checking voltage %d...\n", voltage);
+
+		for (j = 1; j < creg->n_regulators; j++) {
+			if (!regulator_is_supported_voltage(creg->regulators[j],
+							    voltage, voltage)) {
+				usable = false;
+				break;
+			}
+		}
+
+		if (usable) {
+			creg->voltages[creg->n_voltages++] = voltage;
+			dev_dbg(&pdev->dev,
+				 "Adding voltage %d to the list of supported voltages\n",
+				 voltage);
+		}
+	}
+
+	dev_dbg(&pdev->dev, "Supporting %d voltages\n", creg->n_voltages);
+
+	desc = devm_kmemdup(&pdev->dev, &coupled_regulator_desc,
+			    sizeof(coupled_regulator_desc), GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+	desc->n_voltages = creg->n_voltages;
+
+	regulator = devm_regulator_register(&pdev->dev, desc, &config);
+	if (IS_ERR(regulator)) {
+		dev_err(&pdev->dev, "Failed to register regulator %s\n",
+			coupled_regulator_desc.name);
+		return PTR_ERR(regulator);
+	}
+
+	return 0;
+}
+
+static struct of_device_id coupled_regulator_of_match[] = {
+	{ .compatible = "coupled-voltage-regulator" },
+	{ /* Sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, coupled_regulator_of_match);
+
+static struct platform_driver coupled_regulator_driver = {
+	.probe	= coupled_regulator_probe,
+
+	.driver = {
+		.name		= "coupled-voltage-regulator",
+		.of_match_table	= coupled_regulator_of_match,
+	},
+};
+module_platform_driver(coupled_regulator_driver);
+
+MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
+MODULE_DESCRIPTION("Coupled Regulator Driver");
+MODULE_LICENSE("GPL");
-- 
2.6.4

[PATCH v2 2/2] ARM: sunxi: chip: Add Wifi chip

[Maxime Ripard]

The CHIP has a WiFi/BT chip on an SDIO bus. That controller is maintained
in reset through a GPIO and uses two independent regulators to be powered
that must be kept in sync.

Model this using an MMC power sequence and a coupled voltage regulator.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/boot/dts/sun5i-r8-chip.dts | 44 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/sun5i-r8-chip.dts b/arch/arm/boot/dts/sun5i-r8-chip.dts
index 530ab28e9ca2..b5ef4eb5b6b4 100644
--- a/arch/arm/boot/dts/sun5i-r8-chip.dts
+++ b/arch/arm/boot/dts/sun5i-r8-chip.dts
@@ -64,6 +64,24 @@
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
+
+	mmc0_pwrseq: mmc0_pwrseq {
+		compatible = "mmc-pwrseq-simple";
+		pinctrl-names = "default";
+		pinctrl-0 = <&chip_wifi_reg_on_pin>;
+		reset-gpios = <&pio 2 19 GPIO_ACTIVE_LOW>; /* PC19 */
+	};
+
+	/*
+	 * Both LDO3 and LDO4 are used in parallel to power up the
+	 * WiFi/BT Chip.
+	 */
+	vcc_wifi: wifi_reg {
+		compatible = "coupled-voltage-regulator";
+		regulator-name = "vcc-wifi";
+		vin0-supply = <&reg_ldo3>;
+		vin1-supply = <&reg_ldo4>;
+	};
 };
 
 &codec {
@@ -113,10 +131,15 @@
 	};
 };
 
+&mmc0_pins_a {
+	allwinner,pull = <SUN4I_PINCTRL_PULL_UP>;
+};
+
 &mmc0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&mmc0_pins_a>;
-	vmmc-supply = <&reg_vcc3v3>;
+	vmmc-supply = <&vcc_wifi>;
+	mmc-pwrseq = <&mmc0_pwrseq>;
 	bus-width = <4>;
 	non-removable;
 	status = "okay";
@@ -138,6 +161,13 @@
 		allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
 	};
 
+	chip_wifi_reg_on_pin: chip_wifi_reg_on_pin@0 {
+		allwinner,pins = "PC19";
+		allwinner,function = "gpio_out";
+		allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+		allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+	};
+
 	chip_id_det_pin: chip_id_det_pin@0 {
 		allwinner,pins = "PG2";
 		allwinner,function = "gpio_in";
@@ -171,6 +201,18 @@
 	regulator-always-on;
 };
 
+&reg_ldo3 {
+	regulator-min-microvolt = <3300000>;
+	regulator-max-microvolt = <3300000>;
+	regulator-name = "vcc-wifi-1";
+};
+
+&reg_ldo4 {
+	regulator-min-microvolt = <3300000>;
+	regulator-max-microvolt = <3300000>;
+	regulator-name = "vcc-wifi-2";
+};
+
 &reg_ldo5 {
 	regulator-min-microvolt = <1800000>;
 	regulator-max-microvolt = <1800000>;
-- 
2.6.4

Re: [PATCH v2 1/2] regulator: Add coupled regulator

[Rob Herring]

On Tue, Jan 12, 2016 at 02:37:21PM +0100, Maxime Ripard wrote:
> Some boards, in order to power devices that have a quite high power
> consumption, wire multiple regulators in parallel.
> 
> In such a case, the regulators need to be kept in sync, all of them being
> enabled or disabled in parallel.
> 
> This also requires to expose only the voltages that are common to all the
> regulators.
> 
> Eventually support for changing the voltage in parallel should be added
> too, possibly with delays between each other to avoid having a too brutal
> peak consumption.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  .../bindings/regulator/coupled-voltage.txt         |  18 ++
>  drivers/regulator/Kconfig                          |   8 +
>  drivers/regulator/Makefile                         |   1 +
>  drivers/regulator/coupled-voltage-regulator.c      | 299 +++++++++++++++++++++
>  4 files changed, 326 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/coupled-voltage.txt
>  create mode 100644 drivers/regulator/coupled-voltage-regulator.c
> 
> diff --git a/Documentation/devicetree/bindings/regulator/coupled-voltage.txt b/Documentation/devicetree/bindings/regulator/coupled-voltage.txt
> new file mode 100644
> index 000000000000..f5401aab52f2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/coupled-voltage.txt
> @@ -0,0 +1,18 @@
> +Coupled voltage regulators
> +
> +Required properties:
> +- compatible		: Must be "coupled-voltage-regulator".
> +
> +Optional properties:
> +- vinX-supply		: Phandle to the regulators it aggregates
> +
> +Any property defined as part of the core regulator binding defined in
> +regulator.txt can also be used.
> +
> +Example:
> +	vcc_wifi: wifi_reg {
> +		compatible = "coupled-voltage-regulator";
> +		regulator-name = "vcc-wifi";
> +		vin0-supply = <&reg_ldo3>;
> +		vin1-supply = <&reg_ldo4>;
> +	};

Why not just make ?-supply a list of phandles? That would be simpler 
than a virtual regulator.

Rob

Re: [PATCH v2 1/2] regulator: Add coupled regulator

[Maxime Ripard]

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

Hi Rob,

On Tue, Jan 12, 2016 at 08:31:00AM -0600, Rob Herring wrote:
> On Tue, Jan 12, 2016 at 02:37:21PM +0100, Maxime Ripard wrote:
> > Some boards, in order to power devices that have a quite high power
> > consumption, wire multiple regulators in parallel.
> > 
> > In such a case, the regulators need to be kept in sync, all of them being
> > enabled or disabled in parallel.
> > 
> > This also requires to expose only the voltages that are common to all the
> > regulators.
> > 
> > Eventually support for changing the voltage in parallel should be added
> > too, possibly with delays between each other to avoid having a too brutal
> > peak consumption.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  .../bindings/regulator/coupled-voltage.txt         |  18 ++
> >  drivers/regulator/Kconfig                          |   8 +
> >  drivers/regulator/Makefile                         |   1 +
> >  drivers/regulator/coupled-voltage-regulator.c      | 299 +++++++++++++++++++++
> >  4 files changed, 326 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/regulator/coupled-voltage.txt
> >  create mode 100644 drivers/regulator/coupled-voltage-regulator.c
> > 
> > diff --git a/Documentation/devicetree/bindings/regulator/coupled-voltage.txt b/Documentation/devicetree/bindings/regulator/coupled-voltage.txt
> > new file mode 100644
> > index 000000000000..f5401aab52f2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/coupled-voltage.txt
> > @@ -0,0 +1,18 @@
> > +Coupled voltage regulators
> > +
> > +Required properties:
> > +- compatible		: Must be "coupled-voltage-regulator".
> > +
> > +Optional properties:
> > +- vinX-supply		: Phandle to the regulators it aggregates
> > +
> > +Any property defined as part of the core regulator binding defined in
> > +regulator.txt can also be used.
> > +
> > +Example:
> > +	vcc_wifi: wifi_reg {
> > +		compatible = "coupled-voltage-regulator";
> > +		regulator-name = "vcc-wifi";
> > +		vin0-supply = <&reg_ldo3>;
> > +		vin1-supply = <&reg_ldo4>;
> > +	};
> 
> Why not just make ?-supply a list of phandles? That would be simpler 
> than a virtual regulator.

I'm not sure I get what you're saying. Do you want to remove that
driver entirely, or just allow the -supply properties in the device
tree to take a list?

In the former case, the rationale behind this driver is that the
regulators powering a device also have to be kept in sync, both by
enabling and disabling all of them at once, but also by all having
them at the same voltages.

We could push that code in the consumer drivers, but that has some
significant drawbacks:
  - That would duplicate that code in all the drivers, leading to the
    usual drawbacks of code duplication, especially when it's not
    really trivial to handle (or at least, when there's a few
    gotchas).
  - When you come to consider it from an hardware point of view, the
    device usually have a single pin that powers it. It's the board
    designer that chose to route that pin to multiple regulators, so
    it's really the board that is wired that way, and putting that
    code in the consumer drivers would be an abstraction leak imho.
  - We might not even have a driver for these regulators, or at least
    one that play by the rules. In our case, that's an out-of-tree
    WiFi driver.

In the latter case, I remember Mark saying several times that he was
not in favour of such a change, even recently:
https://lkml.org/lkml/2015/10/14/238

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH v2 1/2] regulator: Add coupled regulator

[Rob Herring]

On Fri, Jan 15, 2016 at 09:57:34AM +0100, Maxime Ripard wrote:
> Hi Rob,
> 
> On Tue, Jan 12, 2016 at 08:31:00AM -0600, Rob Herring wrote:
> > On Tue, Jan 12, 2016 at 02:37:21PM +0100, Maxime Ripard wrote:
> > > Some boards, in order to power devices that have a quite high power
> > > consumption, wire multiple regulators in parallel.
> > > 
> > > In such a case, the regulators need to be kept in sync, all of them being
> > > enabled or disabled in parallel.
> > > 
> > > This also requires to expose only the voltages that are common to all the
> > > regulators.

[...]

> > > +Coupled voltage regulators
> > > +
> > > +Required properties:
> > > +- compatible		: Must be "coupled-voltage-regulator".
> > > +
> > > +Optional properties:
> > > +- vinX-supply		: Phandle to the regulators it aggregates
> > > +
> > > +Any property defined as part of the core regulator binding defined in
> > > +regulator.txt can also be used.
> > > +
> > > +Example:
> > > +	vcc_wifi: wifi_reg {
> > > +		compatible = "coupled-voltage-regulator";
> > > +		regulator-name = "vcc-wifi";
> > > +		vin0-supply = <&reg_ldo3>;
> > > +		vin1-supply = <&reg_ldo4>;
> > > +	};
> > 
> > Why not just make ?-supply a list of phandles? That would be simpler 
> > than a virtual regulator.
> 
> I'm not sure I get what you're saying. Do you want to remove that
> driver entirely, or just allow the -supply properties in the device
> tree to take a list?

Both actually, I think. If a power supply input for a device has 2 
supplies connected then list both of them for that device rather than 
create a virtual device in DT. Of course, you could keep a virtual 
regulator independent of DT. That is more of an implementation choice.

> In the former case, the rationale behind this driver is that the
> regulators powering a device also have to be kept in sync, both by
> enabling and disabling all of them at once, but also by all having
> them at the same voltages.

None of this has anything to do with the binding other than you can 
write a regulator driver and already have the mechanism to instantiate 
it with DT.

> We could push that code in the consumer drivers, but that has some
> significant drawbacks:
>   - That would duplicate that code in all the drivers, leading to the
>     usual drawbacks of code duplication, especially when it's not
>     really trivial to handle (or at least, when there's a few
>     gotchas).

Either you could keep the driver and the consumer driver is responsible 
for instantiating the regulator. It could also be implemented as a helper 
library in the regulator core. 

>   - When you come to consider it from an hardware point of view, the
>     device usually have a single pin that powers it. It's the board
>     designer that chose to route that pin to multiple regulators, so
>     it's really the board that is wired that way, and putting that
>     code in the consumer drivers would be an abstraction leak imho.

That's a good point. Perhaps the regulator core needs to be able to 
parse the list and return the single ptr to the virtual regulator.

>   - We might not even have a driver for these regulators, or at least
>     one that play by the rules. In our case, that's an out-of-tree
>     WiFi driver.

Support of out of tree things has never been a winning argument for 
upstream.

With an out of tree binding as well?

> In the latter case, I remember Mark saying several times that he was
> not in favour of such a change, even recently:
> https://lkml.org/lkml/2015/10/14/238

I think this case is somewhat different. What I'm suggesting is closer 
to the hardware which is what Mark also argued for, but we should let 
him speak. If the input supply name on a device is VCC and you have REG1 
and REG2 attached, then having "vcc-supply = <&REG1>, <&REG2>;" makes 
perfect sense in terms of matching the h/w.

Also, simplefb is not a good thing to compare with. 

Rob

Re: [PATCH v2 1/2] regulator: Add coupled regulator

[Mark Brown]

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

On Sat, Jan 16, 2016 at 06:04:34PM -0600, Rob Herring wrote:
> On Fri, Jan 15, 2016 at 09:57:34AM +0100, Maxime Ripard wrote:

> > We could push that code in the consumer drivers, but that has some
> > significant drawbacks:
> >   - That would duplicate that code in all the drivers, leading to the
> >     usual drawbacks of code duplication, especially when it's not
> >     really trivial to handle (or at least, when there's a few
> >     gotchas).

> Either you could keep the driver and the consumer driver is responsible 
> for instantiating the regulator. It could also be implemented as a helper 
> library in the regulator core. 

No, anything that is visible to consumer drivers is completely silly.
The whole point with both the DT bindings and the API is to hide details
of how the supply is implemented from the consumers, any implementation
must be transparent to consumers otherwise it's unusable.

> >   - When you come to consider it from an hardware point of view, the
> >     device usually have a single pin that powers it. It's the board
> >     designer that chose to route that pin to multiple regulators, so
> >     it's really the board that is wired that way, and putting that
> >     code in the consumer drivers would be an abstraction leak imho.

> That's a good point. Perhaps the regulator core needs to be able to 
> parse the list and return the single ptr to the virtual regulator.

Exactly, if we don't want to represent the combination directly.  For
most uses it's probably OK but I can see us in a situation where we
might want to do things like only use one of the regulators in low load
situations where we might want to attach properties to the merge of the
two regulators rather than just referencing them both.  I'm not sure
that's realistic though or that we wouldn't just be working that use
case out dynamically at runtime.

I'm ambivalent on which way is better, it does complicate the
implementation to support doing this as lists and while it makes the DT
more elegant I'm not clear that it's worth the effort especially when it
comes to constraint combining.  But perhaps the implementation turns out
to be simpler than I would anticiapte.

> >   - We might not even have a driver for these regulators, or at least
> >     one that play by the rules. In our case, that's an out-of-tree
> >     WiFi driver.

> Support of out of tree things has never been a winning argument for 
> upstream.

> With an out of tree binding as well?

Since the consumer driver shouldn't be worrying about implementation
details of the supply it doesn't matter if the device on this particular
board is out of tree.

> > In the latter case, I remember Mark saying several times that he was
> > not in favour of such a change, even recently:
> > https://lkml.org/lkml/2015/10/14/238

> I think this case is somewhat different. What I'm suggesting is closer 

Yes, that's a different case - that's the case where all the supplies
in the device are combined into a single list rather than the case where
we have one supply with multiple regulators providing it.

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

Re: [PATCH v2 1/2] regulator: Add coupled regulator

[Maxime Ripard]

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

Hi,

On Mon, Jan 18, 2016 at 04:25:38PM +0000, Mark Brown wrote:
> > >   - When you come to consider it from an hardware point of view, the
> > >     device usually have a single pin that powers it. It's the board
> > >     designer that chose to route that pin to multiple regulators, so
> > >     it's really the board that is wired that way, and putting that
> > >     code in the consumer drivers would be an abstraction leak imho.
> 
> > That's a good point. Perhaps the regulator core needs to be able to 
> > parse the list and return the single ptr to the virtual regulator.
> 
> Exactly, if we don't want to represent the combination directly.  For
> most uses it's probably OK but I can see us in a situation where we
> might want to do things like only use one of the regulators in low load
> situations where we might want to attach properties to the merge of the
> two regulators rather than just referencing them both.  I'm not sure
> that's realistic though or that we wouldn't just be working that use
> case out dynamically at runtime.
> 
> I'm ambivalent on which way is better, it does complicate the
> implementation to support doing this as lists and while it makes the DT
> more elegant I'm not clear that it's worth the effort especially when it
> comes to constraint combining.  But perhaps the implementation turns out
> to be simpler than I would anticiapte.

I guess a separate driver would make it easier to deal with cases like
the one you suggested (shutting down when the load is going to be
lower). I don't see how we could have a good DT representation of that
if we're going to use lists.

Anyway, I'm fine with both approaches, just let me know what you
prefer.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH v2 1/2] regulator: Add coupled regulator

[Mark Brown]

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

On Thu, Jan 21, 2016 at 04:46:49PM +0100, Maxime Ripard wrote:

> I guess a separate driver would make it easier to deal with cases like
> the one you suggested (shutting down when the load is going to be
> lower). I don't see how we could have a good DT representation of that
> if we're going to use lists.

We can have a driver regardless of what the DT looks like, it's a
question of how things get instantiated.

> Anyway, I'm fine with both approaches, just let me know what you
> prefer.

Without seeing an implementation of the lists it's hard to say.

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

Re: [PATCH v2 1/2] regulator: Add coupled regulator

[Maxime Ripard]

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

Hi Mark,

On Thu, Jan 21, 2016 at 04:28:02PM +0000, Mark Brown wrote:
> On Thu, Jan 21, 2016 at 04:46:49PM +0100, Maxime Ripard wrote:
> 
> > I guess a separate driver would make it easier to deal with cases like
> > the one you suggested (shutting down when the load is going to be
> > lower). I don't see how we could have a good DT representation of that
> > if we're going to use lists.
> 
> We can have a driver regardless of what the DT looks like, it's a
> question of how things get instantiated.
> 
> > Anyway, I'm fine with both approaches, just let me know what you
> > prefer.
> 
> Without seeing an implementation of the lists it's hard to say.

Just to make sure we're on the same page: you want to keep the
regulator, but instead of giving the parent through vinX-supplies
properties, you want to have a single *-supply property, with a list
of regulators, right?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH v2 1/2] regulator: Add coupled regulator

[Mark Brown]

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

On Fri, Feb 05, 2016 at 03:33:28PM +0100, Maxime Ripard wrote:
> On Thu, Jan 21, 2016 at 04:28:02PM +0000, Mark Brown wrote:
> > On Thu, Jan 21, 2016 at 04:46:49PM +0100, Maxime Ripard wrote:

> > > Anyway, I'm fine with both approaches, just let me know what you
> > > prefer.

> > Without seeing an implementation of the lists it's hard to say.

> Just to make sure we're on the same page: you want to keep the
> regulator, but instead of giving the parent through vinX-supplies
> properties, you want to have a single *-supply property, with a list
> of regulators, right?

Either that or an explicit regulator describing the merge.  Rob wants
the list I think but I really don't care.

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

Re: [PATCH v2 1/2] regulator: Add coupled regulator

[Maxime Ripard]

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

Hi Mark,

On Fri, Feb 05, 2016 at 03:32:58PM +0000, Mark Brown wrote:
> On Fri, Feb 05, 2016 at 03:33:28PM +0100, Maxime Ripard wrote:
> > On Thu, Jan 21, 2016 at 04:28:02PM +0000, Mark Brown wrote:
> > > On Thu, Jan 21, 2016 at 04:46:49PM +0100, Maxime Ripard wrote:
> 
> > > > Anyway, I'm fine with both approaches, just let me know what you
> > > > prefer.
> 
> > > Without seeing an implementation of the lists it's hard to say.
> 
> > Just to make sure we're on the same page: you want to keep the
> > regulator, but instead of giving the parent through vinX-supplies
> > properties, you want to have a single *-supply property, with a list
> > of regulators, right?
> 
> Either that or an explicit regulator describing the merge.  Rob wants
> the list I think but I really don't care.

So, I'm reviving this old thread after speaking to you about it at
ELCE and trying to code something up, and getting lost..

To put a bit of context, I'm still trying to tackle the issue of
devices that have two regulators powering them on the same pin for
example when each regulator cannot provide enough current alone to
power the device (all the setups like this one I've seen so far were
for WiFi chips, but it might be different).

I guess we already agreed on the fact that the DT binding should just
be to allow a *-supply property to take multiple regulators, and mark
them as "coupled" (or whatever name we see fit) in such a case.

Since regulator_get returns a struct regulator pointer, it felt
logical to try to add the list of parent regulators to it, especially
as this structure is per-consumer, and different consumers might have
different combinations of regulators.

However, this structure embeds a pointer to a struct regulator_dev,
which seems to model the regulator itself, but will also contain
pointer to the struct regulator, probably to model its parent? I guess
my first question would be do we care about nesting? or having a
regulator with multiple parents?

It also contains the constraints on each regulator, which might or
might not be different for each of the coupled regulators, but I'm
guessing the couple might have contraints of its own too I guess. Is
it something that might happen? Should we care about it?

And finally, my real question is, do we want to aggregate them in
struct regulator, at the consumer level, which might make the more
sense, or do we want to create an intermediate regulator internally?
What is your take on this?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v2 1/2] regulator: Add coupled regulator

[Mark Brown]

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

On Mon, Nov 07, 2016 at 04:47:38PM +0100, Maxime Ripard wrote:

> Since regulator_get returns a struct regulator pointer, it felt
> logical to try to add the list of parent regulators to it, especially
> as this structure is per-consumer, and different consumers might have
> different combinations of regulators.

> However, this structure embeds a pointer to a struct regulator_dev,
> which seems to model the regulator itself, but will also contain
> pointer to the struct regulator, probably to model its parent? I guess

It'd be a lot easier to follow this if you named the fields...  The rdev
in the struct regulator is indeed the physical device.  The struct
regulator called supply in struct regulator_dev is indeed the parent
regulator.

> my first question would be do we care about nesting? or having a
> regulator with multiple parents?

Well, it seems that your use case here is multiple parents so I guess we
do care about it.  :)

> It also contains the constraints on each regulator, which might or
> might not be different for each of the coupled regulators, but I'm
> guessing the couple might have contraints of its own too I guess. Is
> it something that might happen? Should we care about it?

I can't see how one could physically have constraints that didn't apply
to both parents.

> And finally, my real question is, do we want to aggregate them in
> struct regulator, at the consumer level, which might make the more
> sense, or do we want to create an intermediate regulator internally?
> What is your take on this?

My initial thought without having tried to implement this is that doing
things in an intermediate regulator might do a better job of
encapsulating things it if it works out but I've got a feeling that it's
not going to work out well and that therefore doing it in the consumer
with multiple rdevs will be better.  But really either approach is fine
if it doesn't look horrible.

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