[PATCH] ARM: dts: sun8i-h2-plus-bananapi-m2-zero: add gpio-poweroff to DT

[PATCH] ARM: dts: sun8i-h2-plus-bananapi-m2-zero: add gpio-poweroff to DT

[Michael Klein]

Add gpio-poweroff node to allow the board to power itself off after
shutdown by disabling the SYSTEM and CPUX regulators (U5 resp. U6).
The RST button can be used to restart the board.

Signed-off-by: Michael Klein <michael@fossekall.de>
---
 arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
index 4c6704e4c57e..76e79e6db733 100644
--- a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
+++ b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
@@ -46,6 +46,11 @@ sw4 {
 		};
 	};
 
+	gpio_poweroff {
+		compatible = "gpio-poweroff";
+		gpios = <&r_pio 0 8 GPIO_ACTIVE_LOW>; /* PL8 */
+	};
+
 	reg_vdd_cpux: vdd-cpux-regulator {
 		compatible = "regulator-gpio";
 		regulator-name = "vdd-cpux";
-- 
2.29.2

Re: [PATCH] ARM: dts: sun8i-h2-plus-bananapi-m2-zero: add gpio-poweroff to DT

[Samuel Holland]

On 11/23/20 10:10 AM, Michael Klein wrote:
> Add gpio-poweroff node to allow the board to power itself off after
> shutdown by disabling the SYSTEM and CPUX regulators (U5 resp. U6).
> The RST button can be used to restart the board.

The PSCI client will override this driver once the PSCI implementation
is upgraded to v0.2 or newer functions. So having this around should
cause no compatibility issues (although it would print an error in dmesg
at that point). This seems like a reasonable thing to do for the other
H2+/H3 boards that use a similar regulator layout.

Reviewed-by: Samuel Holland <samuel@sholland.org>

> Signed-off-by: Michael Klein <michael@fossekall.de>
> ---
>  arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> index 4c6704e4c57e..76e79e6db733 100644
> --- a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> +++ b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> @@ -46,6 +46,11 @@ sw4 {
>  		};
>  	};
>  
> +	gpio_poweroff {
> +		compatible = "gpio-poweroff";
> +		gpios = <&r_pio 0 8 GPIO_ACTIVE_LOW>; /* PL8 */
> +	};
> +
>  	reg_vdd_cpux: vdd-cpux-regulator {
>  		compatible = "regulator-gpio";
>  		regulator-name = "vdd-cpux";
> 

Re: [PATCH] ARM: dts: sun8i-h2-plus-bananapi-m2-zero: add gpio-poweroff to DT

[Chen-Yu Tsai]

On Tue, Nov 24, 2020 at 12:14 PM Samuel Holland <samuel@sholland.org> wrote:
>
> On 11/23/20 10:10 AM, Michael Klein wrote:
> > Add gpio-poweroff node to allow the board to power itself off after
> > shutdown by disabling the SYSTEM and CPUX regulators (U5 resp. U6).
> > The RST button can be used to restart the board.
>
> The PSCI client will override this driver once the PSCI implementation
> is upgraded to v0.2 or newer functions. So having this around should
> cause no compatibility issues (although it would print an error in dmesg
> at that point). This seems like a reasonable thing to do for the other
> H2+/H3 boards that use a similar regulator layout.

I wonder if this (gpio-poweroff) works if those regulators are also in the DT?

> Reviewed-by: Samuel Holland <samuel@sholland.org>
>
> > Signed-off-by: Michael Klein <michael@fossekall.de>
> > ---
> >  arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> > index 4c6704e4c57e..76e79e6db733 100644
> > --- a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> > +++ b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> > @@ -46,6 +46,11 @@ sw4 {
> >               };
> >       };
> >
> > +     gpio_poweroff {
> > +             compatible = "gpio-poweroff";
> > +             gpios = <&r_pio 0 8 GPIO_ACTIVE_LOW>; /* PL8 */
> > +     };
> > +
> >       reg_vdd_cpux: vdd-cpux-regulator {
> >               compatible = "regulator-gpio";
> >               regulator-name = "vdd-cpux";
> >
>

Re: [PATCH] ARM: dts: sun8i-h2-plus-bananapi-m2-zero: add gpio-poweroff to DT

[Maxime Ripard]

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

On Tue, Nov 24, 2020 at 12:41:37PM +0800, Chen-Yu Tsai wrote:
> On Tue, Nov 24, 2020 at 12:14 PM Samuel Holland <samuel@sholland.org> wrote:
> >
> > On 11/23/20 10:10 AM, Michael Klein wrote:
> > > Add gpio-poweroff node to allow the board to power itself off after
> > > shutdown by disabling the SYSTEM and CPUX regulators (U5 resp. U6).
> > > The RST button can be used to restart the board.
> >
> > The PSCI client will override this driver once the PSCI implementation
> > is upgraded to v0.2 or newer functions. So having this around should
> > cause no compatibility issues (although it would print an error in dmesg
> > at that point). This seems like a reasonable thing to do for the other
> > H2+/H3 boards that use a similar regulator layout.
> 
> I wonder if this (gpio-poweroff) works if those regulators are also in the DT?

It's probably not going to probe at all, since both would claim the
exclusive usage of the GPIO?

I guess we should model this properly using the regulator framework, and
regulator_force_disable allows to bypass any usage count

Maxime

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

Re: [PATCH] ARM: dts: sun8i-h2-plus-bananapi-m2-zero: add gpio-poweroff to DT

[Maxime Ripard]

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

On Mon, Nov 23, 2020 at 05:10:41PM +0100, Michael Klein wrote:
> Add gpio-poweroff node to allow the board to power itself off after
> shutdown by disabling the SYSTEM and CPUX regulators (U5 resp. U6).
> The RST button can be used to restart the board.
> 
> Signed-off-by: Michael Klein <michael@fossekall.de>
> ---
>  arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> index 4c6704e4c57e..76e79e6db733 100644
> --- a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> +++ b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> @@ -46,6 +46,11 @@ sw4 {
>  		};
>  	};
>  
> +	gpio_poweroff {

DT nodes are not supposed to have an underscore in their name. The name
should also be the class of the "device", so I guess we can simply use
poweroff here?

Maxime

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

[PATCH v2] ARM: dts: sun8i-h2-plus-bananapi-m2-zero: add poweroff node to DT

[Michael Klein]

Add poweroff node to allow the board to power itself off after shutdown
by disabling the SYSTEM and CPUX regulators (U5 resp. U6).  The RST
button can be used to restart the board.

Signed-off-by: Michael Klein <michael@fossekall.de>
---
 arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
index 4c6704e4c57e..ea2fa48a1647 100644
--- a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
+++ b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
@@ -46,6 +46,11 @@ sw4 {
 		};
 	};
 
+	poweroff {
+		compatible = "gpio-poweroff";
+		gpios = <&r_pio 0 8 GPIO_ACTIVE_LOW>; /* PL8 */
+	};
+
 	reg_vdd_cpux: vdd-cpux-regulator {
 		compatible = "regulator-gpio";
 		regulator-name = "vdd-cpux";
-- 
2.29.2

Re: [PATCH v2] ARM: dts: sun8i-h2-plus-bananapi-m2-zero: add poweroff node to DT

[Maxime Ripard]

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

On Tue, Nov 24, 2020 at 02:36:33PM +0100, Michael Klein wrote:
> Add poweroff node to allow the board to power itself off after shutdown
> by disabling the SYSTEM and CPUX regulators (U5 resp. U6).  The RST
> button can be used to restart the board.
> 
> Signed-off-by: Michael Klein <michael@fossekall.de>
> ---
>  arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts | 5 +++++
>  1 file changed, 5 insertions(+)

You should have a summary of the changes between versions here

> diff --git a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> index 4c6704e4c57e..ea2fa48a1647 100644
> --- a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> +++ b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> @@ -46,6 +46,11 @@ sw4 {
>  		};
>  	};
>  
> +	poweroff {
> +		compatible = "gpio-poweroff";
> +		gpios = <&r_pio 0 8 GPIO_ACTIVE_LOW>; /* PL8 */
> +	};
> +

Like I said in the previous version, this should really be modelled as a
regulator instead of just a GPIO

Maxime

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

Re: [PATCH v2] ARM: dts: sun8i-h2-plus-bananapi-m2-zero: add poweroff node to DT

[Michael Klein]

On Tue, Nov 24, 2020 at 03:26:56PM +0100, Maxime Ripard wrote:
>On Tue, Nov 24, 2020 at 02:36:33PM +0100, Michael Klein wrote:
>> Add poweroff node to allow the board to power itself off after shutdown
>> by disabling the SYSTEM and CPUX regulators (U5 resp. U6).  The RST
>> button can be used to restart the board.
>>
>> Signed-off-by: Michael Klein <michael@fossekall.de>
>> ---
>>  arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts | 5 +++++
>>  1 file changed, 5 insertions(+)
>
>You should have a summary of the changes between versions here
>
>> diff --git a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
>> index 4c6704e4c57e..ea2fa48a1647 100644
>> --- a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
>> +++ b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
>> @@ -46,6 +46,11 @@ sw4 {
>>  		};
>>  	};
>>
>> +	poweroff {
>> +		compatible = "gpio-poweroff";
>> +		gpios = <&r_pio 0 8 GPIO_ACTIVE_LOW>; /* PL8 */
>> +	};
>> +
>
>Like I said in the previous version, this should really be modelled as a
>regulator instead of just a GPIO

Please excuse my ignorance, do you mean something like this?

        reg_vdd_sys: vdd-sys {
                compatible = "regulator-fixed";
                regulator-name = "vdd-sys";
                regulator-min-microvolt = <1200000>;
                regulator-max-microvolt = <1200000>;
                regulator-always-on;
                regulator-boot-on;
                enable-active-high;
                gpio = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */
                vin-supply = <&reg_vcc5v0>;
         };

With this, the board still draws 60mA (cheap USB ampere meter) after 
shutdown, presumably because of "regulator-always-on".  Without this 
property the board powers off shortly after booting up.

Michael

Re: [PATCH v2] ARM: dts: sun8i-h2-plus-bananapi-m2-zero: add poweroff node to DT

[Maxime Ripard]

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

On Tue, Nov 24, 2020 at 11:31:59PM +0100, Michael Klein wrote:
> On Tue, Nov 24, 2020 at 03:26:56PM +0100, Maxime Ripard wrote:
> > On Tue, Nov 24, 2020 at 02:36:33PM +0100, Michael Klein wrote:
> > > Add poweroff node to allow the board to power itself off after shutdown
> > > by disabling the SYSTEM and CPUX regulators (U5 resp. U6).  The RST
> > > button can be used to restart the board.
> > > 
> > > Signed-off-by: Michael Klein <michael@fossekall.de>
> > > ---
> > >  arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > 
> > You should have a summary of the changes between versions here
> > 
> > > diff --git a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> > > index 4c6704e4c57e..ea2fa48a1647 100644
> > > --- a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> > > +++ b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> > > @@ -46,6 +46,11 @@ sw4 {
> > >  		};
> > >  	};
> > > 
> > > +	poweroff {
> > > +		compatible = "gpio-poweroff";
> > > +		gpios = <&r_pio 0 8 GPIO_ACTIVE_LOW>; /* PL8 */
> > > +	};
> > > +
> > 
> > Like I said in the previous version, this should really be modelled as a
> > regulator instead of just a GPIO
> 
> Please excuse my ignorance, do you mean something like this?
> 
>        reg_vdd_sys: vdd-sys {
>                compatible = "regulator-fixed";
>                regulator-name = "vdd-sys";
>                regulator-min-microvolt = <1200000>;
>                regulator-max-microvolt = <1200000>;
>                regulator-always-on;
>                regulator-boot-on;
>                enable-active-high;
>                gpio = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */
>                vin-supply = <&reg_vcc5v0>;
>         };
> 
> With this, the board still draws 60mA (cheap USB ampere meter) after
> shutdown, presumably because of "regulator-always-on".  Without this
> property the board powers off shortly after booting up.

Yes, because you're only describing the regulator itself here, but
you're not telling linux that it needs to shut it down to power-down the
board.

You'd need a driver similar to gpio-poweroff, using a regulator instead,
and calling regulator_force_disable to shut it down

Maxime

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

[PATCH v3 0/3] BPi M2 Zero poweroff support via new regulator-poweroff driver

[Michael Klein]

Changes in v2:
  - rename DT node

Changes in v3:
  - add regulator-poweroff driver
  - use regulator-poweroff driver instead of gpio-poweroff

Michael Klein (3):
  power: reset: new driver regulator-poweroff
  Documentation: DT: binding documentation for regulator-poweroff
  ARM: dts: sun8i-h2-plus-bananapi-m2-zero: add poweroff node

 .../power/reset/regulator-poweroff.yaml       |  53 +++++++++
 .../dts/sun8i-h2-plus-bananapi-m2-zero.dts    |   7 ++
 drivers/power/reset/Kconfig                   |   7 ++
 drivers/power/reset/Makefile                  |   1 +
 drivers/power/reset/regulator-poweroff.c      | 107 ++++++++++++++++++
 5 files changed, 175 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
 create mode 100644 drivers/power/reset/regulator-poweroff.c

-- 
2.29.2

[PATCH v3 1/3] power: reset: new driver regulator-poweroff

[Michael Klein]

This driver registers a pm_power_off function to disable a set of
regulators defined in the devicetree to turn off the board.

Signed-off-by: Michael Klein <michael@fossekall.de>
---
 drivers/power/reset/Kconfig              |   7 ++
 drivers/power/reset/Makefile             |   1 +
 drivers/power/reset/regulator-poweroff.c | 107 +++++++++++++++++++++++
 3 files changed, 115 insertions(+)
 create mode 100644 drivers/power/reset/regulator-poweroff.c

diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index d55b3727e00e..ae6cb7b0bd4d 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -177,6 +177,13 @@ config POWER_RESET_QNAP
 
 	  Say Y if you have a QNAP NAS.
 
+config POWER_RESET_REGULATOR
+	bool "Regulator subsystem power-off driver"
+	depends on OF && REGULATOR
+	help
+	  This driver supports turning off your board by disabling a set
+	  of regulators defined in the devicetree.
+
 config POWER_RESET_RESTART
 	bool "Restart power-off driver"
 	help
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index c51eceba9ea3..9dc49d3a57ff 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_POWER_RESET_OCELOT_RESET) += ocelot-reset.o
 obj-$(CONFIG_POWER_RESET_PIIX4_POWEROFF) += piix4-poweroff.o
 obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
 obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
+obj-$(CONFIG_POWER_RESET_REGULATOR) += regulator-poweroff.o
 obj-$(CONFIG_POWER_RESET_RESTART) += restart-poweroff.o
 obj-$(CONFIG_POWER_RESET_ST) += st-poweroff.o
 obj-$(CONFIG_POWER_RESET_VERSATILE) += arm-versatile-reboot.o
diff --git a/drivers/power/reset/regulator-poweroff.c b/drivers/power/reset/regulator-poweroff.c
new file mode 100644
index 000000000000..df2ca4fdcc49
--- /dev/null
+++ b/drivers/power/reset/regulator-poweroff.c
@@ -0,0 +1,107 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Force-disables a regulator to power down a device
+ *
+ * Michael Klein <michael@fossekall.de>
+ *
+ * Copyright (C) 2020 Michael Klein
+ *
+ * Based on the gpio-poweroff driver.
+ */
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+
+#define DEFAULT_TIMEOUT_MS 3000
+
+/*
+ * Hold configuration here, cannot be more than one instance of the driver
+ * since pm_power_off itself is global.
+ */
+static struct regulator **poweroff_regulators;
+static u32 timeout = DEFAULT_TIMEOUT_MS;
+
+static void regulator_poweroff_do_poweroff(void)
+{
+	struct regulator **it;
+
+	if (poweroff_regulators)
+		for (it = poweroff_regulators; *it; ++it)
+			if (regulator_is_enabled(*it))
+				regulator_force_disable(*it);
+
+	/* give it some time */
+	mdelay(timeout);
+
+	WARN_ON(1);
+}
+
+static int regulator_poweroff_probe(struct platform_device *pdev)
+{
+	int count;
+	const char *name;
+	struct regulator **it;
+	struct property *prop;
+	struct device_node *node = pdev->dev.of_node;
+
+	/* If a pm_power_off function has already been added, leave it alone */
+	if (pm_power_off != NULL) {
+		dev_err(&pdev->dev,
+			"%s: pm_power_off function already registered\n",
+		       __func__);
+		return -EBUSY;
+	}
+
+	count = of_property_count_strings(node, "regulator-names");
+	if (count <= 0)
+		return -ENOENT;
+
+	poweroff_regulators = devm_kcalloc(&pdev->dev, count + 1,
+		sizeof(struct regulator *), GFP_KERNEL);
+
+	it = poweroff_regulators;
+	of_property_for_each_string(node, "regulator-names", prop, name) {
+		*it = devm_regulator_get(&pdev->dev, name);
+		if (IS_ERR(*it))
+			return PTR_ERR(*it);
+		it++;
+	}
+
+	of_property_read_u32(node, "timeout-ms", &timeout);
+
+	pm_power_off = &regulator_poweroff_do_poweroff;
+	return 0;
+}
+
+static int regulator_poweroff_remove(__maybe_unused struct platform_device *pdev)
+{
+	if (pm_power_off == &regulator_poweroff_do_poweroff)
+		pm_power_off = NULL;
+
+	return 0;
+}
+
+static const struct of_device_id of_regulator_poweroff_match[] = {
+	{ .compatible = "regulator-poweroff", },
+	{},
+};
+
+static struct platform_driver regulator_poweroff_driver = {
+	.probe = regulator_poweroff_probe,
+	.remove = regulator_poweroff_remove,
+	.driver = {
+		.name = "poweroff-regulator",
+		.of_match_table = of_regulator_poweroff_match,
+	},
+};
+
+module_platform_driver(regulator_poweroff_driver);
+
+MODULE_AUTHOR("Michael Klein <michael@fossekall.de>");
+MODULE_DESCRIPTION("Regulator poweroff driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:poweroff-regulator");
-- 
2.29.2

Re: [PATCH v3 1/3] power: reset: new driver regulator-poweroff

[Maxime Ripard]

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

On Mon, Dec 07, 2020 at 03:27:54PM +0100, Michael Klein wrote:
> This driver registers a pm_power_off function to disable a set of
> regulators defined in the devicetree to turn off the board.
> 
> Signed-off-by: Michael Klein <michael@fossekall.de>
> ---
>  drivers/power/reset/Kconfig              |   7 ++
>  drivers/power/reset/Makefile             |   1 +
>  drivers/power/reset/regulator-poweroff.c | 107 +++++++++++++++++++++++
>  3 files changed, 115 insertions(+)
>  create mode 100644 drivers/power/reset/regulator-poweroff.c
> 
> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index d55b3727e00e..ae6cb7b0bd4d 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -177,6 +177,13 @@ config POWER_RESET_QNAP
>  
>  	  Say Y if you have a QNAP NAS.
>  
> +config POWER_RESET_REGULATOR
> +	bool "Regulator subsystem power-off driver"
> +	depends on OF && REGULATOR
> +	help
> +	  This driver supports turning off your board by disabling a set
> +	  of regulators defined in the devicetree.
> +
>  config POWER_RESET_RESTART
>  	bool "Restart power-off driver"
>  	help
> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> index c51eceba9ea3..9dc49d3a57ff 100644
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_POWER_RESET_OCELOT_RESET) += ocelot-reset.o
>  obj-$(CONFIG_POWER_RESET_PIIX4_POWEROFF) += piix4-poweroff.o
>  obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
>  obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
> +obj-$(CONFIG_POWER_RESET_REGULATOR) += regulator-poweroff.o
>  obj-$(CONFIG_POWER_RESET_RESTART) += restart-poweroff.o
>  obj-$(CONFIG_POWER_RESET_ST) += st-poweroff.o
>  obj-$(CONFIG_POWER_RESET_VERSATILE) += arm-versatile-reboot.o
> diff --git a/drivers/power/reset/regulator-poweroff.c b/drivers/power/reset/regulator-poweroff.c
> new file mode 100644
> index 000000000000..df2ca4fdcc49
> --- /dev/null
> +++ b/drivers/power/reset/regulator-poweroff.c
> @@ -0,0 +1,107 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Force-disables a regulator to power down a device
> + *
> + * Michael Klein <michael@fossekall.de>
> + *
> + * Copyright (C) 2020 Michael Klein
> + *
> + * Based on the gpio-poweroff driver.
> + */
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define DEFAULT_TIMEOUT_MS 3000
> +
> +/*
> + * Hold configuration here, cannot be more than one instance of the driver
> + * since pm_power_off itself is global.
> + */
> +static struct regulator **poweroff_regulators;
> +static u32 timeout = DEFAULT_TIMEOUT_MS;
> +
> +static void regulator_poweroff_do_poweroff(void)
> +{
> +	struct regulator **it;
> +
> +	if (poweroff_regulators)
> +		for (it = poweroff_regulators; *it; ++it)
> +			if (regulator_is_enabled(*it))
> +				regulator_force_disable(*it);
> +
> +	/* give it some time */
> +	mdelay(timeout);
> +
> +	WARN_ON(1);
> +}
> +
> +static int regulator_poweroff_probe(struct platform_device *pdev)
> +{
> +	int count;
> +	const char *name;
> +	struct regulator **it;
> +	struct property *prop;
> +	struct device_node *node = pdev->dev.of_node;
> +
> +	/* If a pm_power_off function has already been added, leave it alone */
> +	if (pm_power_off != NULL) {
> +		dev_err(&pdev->dev,
> +			"%s: pm_power_off function already registered\n",
> +		       __func__);
> +		return -EBUSY;
> +	}
> +
> +	count = of_property_count_strings(node, "regulator-names");
> +	if (count <= 0)
> +		return -ENOENT;
> +
> +	poweroff_regulators = devm_kcalloc(&pdev->dev, count + 1,
> +		sizeof(struct regulator *), GFP_KERNEL);
> +
> +	it = poweroff_regulators;
> +	of_property_for_each_string(node, "regulator-names", prop, name) {
> +		*it = devm_regulator_get(&pdev->dev, name);
> +		if (IS_ERR(*it))
> +			return PTR_ERR(*it);
> +		it++;
> +	}
> +
> +	of_property_read_u32(node, "timeout-ms", &timeout);
> +
> +	pm_power_off = &regulator_poweroff_do_poweroff;
> +	return 0;
> +}
> +
> +static int regulator_poweroff_remove(__maybe_unused struct platform_device *pdev)
> +{
> +	if (pm_power_off == &regulator_poweroff_do_poweroff)
> +		pm_power_off = NULL;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id of_regulator_poweroff_match[] = {
> +	{ .compatible = "regulator-poweroff", },
> +	{},
> +};
> +
> +static struct platform_driver regulator_poweroff_driver = {
> +	.probe = regulator_poweroff_probe,
> +	.remove = regulator_poweroff_remove,
> +	.driver = {
> +		.name = "poweroff-regulator",
> +		.of_match_table = of_regulator_poweroff_match,
> +	},
> +};
> +
> +module_platform_driver(regulator_poweroff_driver);

Since this can't be compiled as a module, you can use
module_platform_driver_probe instead.

Maxime

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

Re: [PATCH v3 1/3] power: reset: new driver regulator-poweroff

[Michael Klein]

On Tue, Dec 08, 2020 at 11:10:52AM +0100, Maxime Ripard wrote:
>On Mon, Dec 07, 2020 at 03:27:54PM +0100, Michael Klein wrote:
>> This driver registers a pm_power_off function to disable a set of
>> regulators defined in the devicetree to turn off the board.
>>
>> Signed-off-by: Michael Klein <michael@fossekall.de>
>> ---
>>  drivers/power/reset/Kconfig              |   7 ++
>>  drivers/power/reset/Makefile             |   1 +
>>  drivers/power/reset/regulator-poweroff.c | 107 +++++++++++++++++++++++
>>  3 files changed, 115 insertions(+)
>>  create mode 100644 drivers/power/reset/regulator-poweroff.c
>>
>> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
>> index d55b3727e00e..ae6cb7b0bd4d 100644
>> --- a/drivers/power/reset/Kconfig
>> +++ b/drivers/power/reset/Kconfig
>> @@ -177,6 +177,13 @@ config POWER_RESET_QNAP
>>
>>  	  Say Y if you have a QNAP NAS.
>>
>> +config POWER_RESET_REGULATOR
>> +	bool "Regulator subsystem power-off driver"
>> +	depends on OF && REGULATOR
>> +	help
>> +	  This driver supports turning off your board by disabling a set
>> +	  of regulators defined in the devicetree.
>> +
>>  config POWER_RESET_RESTART
>>  	bool "Restart power-off driver"
>>  	help
>> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
>> index c51eceba9ea3..9dc49d3a57ff 100644
>> --- a/drivers/power/reset/Makefile
>> +++ b/drivers/power/reset/Makefile
>> @@ -19,6 +19,7 @@ obj-$(CONFIG_POWER_RESET_OCELOT_RESET) += ocelot-reset.o
>>  obj-$(CONFIG_POWER_RESET_PIIX4_POWEROFF) += piix4-poweroff.o
>>  obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
>>  obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
>> +obj-$(CONFIG_POWER_RESET_REGULATOR) += regulator-poweroff.o
>>  obj-$(CONFIG_POWER_RESET_RESTART) += restart-poweroff.o
>>  obj-$(CONFIG_POWER_RESET_ST) += st-poweroff.o
>>  obj-$(CONFIG_POWER_RESET_VERSATILE) += arm-versatile-reboot.o
>> diff --git a/drivers/power/reset/regulator-poweroff.c b/drivers/power/reset/regulator-poweroff.c
>> new file mode 100644
>> index 000000000000..df2ca4fdcc49
>> --- /dev/null
>> +++ b/drivers/power/reset/regulator-poweroff.c
>> @@ -0,0 +1,107 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Force-disables a regulator to power down a device
>> + *
>> + * Michael Klein <michael@fossekall.de>
>> + *
>> + * Copyright (C) 2020 Michael Klein
>> + *
>> + * Based on the gpio-poweroff driver.
>> + */
>> +#include <linux/delay.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#define DEFAULT_TIMEOUT_MS 3000
>> +
>> +/*
>> + * Hold configuration here, cannot be more than one instance of the driver
>> + * since pm_power_off itself is global.
>> + */
>> +static struct regulator **poweroff_regulators;
>> +static u32 timeout = DEFAULT_TIMEOUT_MS;
>> +
>> +static void regulator_poweroff_do_poweroff(void)
>> +{
>> +	struct regulator **it;
>> +
>> +	if (poweroff_regulators)
>> +		for (it = poweroff_regulators; *it; ++it)
>> +			if (regulator_is_enabled(*it))
>> +				regulator_force_disable(*it);
>> +
>> +	/* give it some time */
>> +	mdelay(timeout);
>> +
>> +	WARN_ON(1);
>> +}
>> +
>> +static int regulator_poweroff_probe(struct platform_device *pdev)
>> +{
>> +	int count;
>> +	const char *name;
>> +	struct regulator **it;
>> +	struct property *prop;
>> +	struct device_node *node = pdev->dev.of_node;
>> +
>> +	/* If a pm_power_off function has already been added, leave it alone */
>> +	if (pm_power_off != NULL) {
>> +		dev_err(&pdev->dev,
>> +			"%s: pm_power_off function already registered\n",
>> +		       __func__);
>> +		return -EBUSY;
>> +	}
>> +
>> +	count = of_property_count_strings(node, "regulator-names");
>> +	if (count <= 0)
>> +		return -ENOENT;
>> +
>> +	poweroff_regulators = devm_kcalloc(&pdev->dev, count + 1,
>> +		sizeof(struct regulator *), GFP_KERNEL);
>> +
>> +	it = poweroff_regulators;
>> +	of_property_for_each_string(node, "regulator-names", prop, name) {
>> +		*it = devm_regulator_get(&pdev->dev, name);
>> +		if (IS_ERR(*it))
>> +			return PTR_ERR(*it);
>> +		it++;
>> +	}
>> +
>> +	of_property_read_u32(node, "timeout-ms", &timeout);
>> +
>> +	pm_power_off = &regulator_poweroff_do_poweroff;
>> +	return 0;
>> +}
>> +
>> +static int regulator_poweroff_remove(__maybe_unused struct platform_device *pdev)
>> +{
>> +	if (pm_power_off == &regulator_poweroff_do_poweroff)
>> +		pm_power_off = NULL;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id of_regulator_poweroff_match[] = {
>> +	{ .compatible = "regulator-poweroff", },
>> +	{},
>> +};
>> +
>> +static struct platform_driver regulator_poweroff_driver = {
>> +	.probe = regulator_poweroff_probe,
>> +	.remove = regulator_poweroff_remove,
>> +	.driver = {
>> +		.name = "poweroff-regulator",
>> +		.of_match_table = of_regulator_poweroff_match,
>> +	},
>> +};
>> +
>> +module_platform_driver(regulator_poweroff_driver);
>
>Since this can't be compiled as a module, you can use
>module_platform_driver_probe instead.

actually no, as platform_driver_probe() does not support deferred 
probing and the regulator might not be available during 
regulator_poweroff_probe() yet:

# dmesg | grep poweroff
[    0.788135] poweroff-regulator poweroff: probe deferral not supported

-- 
Michael

[PATCH v3 2/3] Documentation: DT: binding documentation for regulator-poweroff

[Michael Klein]

Add devicetree binding documentation for regulator-poweroff driver.

Signed-off-by: Michael Klein <michael@fossekall.de>
---
 .../power/reset/regulator-poweroff.yaml       | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml

diff --git a/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
new file mode 100644
index 000000000000..8c8ce6bb031a
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/reset/regulator-poweroff.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Force-disable power regulators to turn the power off.
+
+maintainers:
+  - Michael Klein <michael@fossekall.de>
+
+description: |
+  When the power-off handler is called, one more regulators are disabled
+  by calling regulator_force_disable(). If the power is still on and the
+  CPU still running after a 3000ms delay, a WARN_ON(1) is emitted.
+
+properties:
+  compatible:
+    const: "regulator-poweroff"
+
+  regulator-names:
+    description:
+      Array of regulator names
+    $ref: /schemas/types.yaml#/definitions/string-array
+
+  REGULATOR-supply:
+    description:
+      For any REGULATOR listed in regulator-names, a phandle
+      to the corresponding regulator node
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+  timeout-ms:
+    description:
+      Time to wait before asserting a WARN_ON(1). If nothing is
+      specified, 3000 ms is used.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+  - compatible
+  - regulator-names
+  - REGULATOR-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    regulator-poweroff {
+        compatible = "regulator-poweroff";
+        regulator-names = "vcc1v2", "vcc-dram";
+        vcc1v2-supply = <&reg_vcc1v2>;
+        vcc-dram-supply = <&reg_vcc_dram>;
+    };
+...
-- 
2.29.2

Re: [PATCH v3 2/3] Documentation: DT: binding documentation for regulator-poweroff

[Maxime Ripard]

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

On Mon, Dec 07, 2020 at 03:27:55PM +0100, Michael Klein wrote:
> Add devicetree binding documentation for regulator-poweroff driver.
> 
> Signed-off-by: Michael Klein <michael@fossekall.de>
> ---
>  .../power/reset/regulator-poweroff.yaml       | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
> new file mode 100644
> index 000000000000..8c8ce6bb031a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/reset/regulator-poweroff.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Force-disable power regulators to turn the power off.
> +
> +maintainers:
> +  - Michael Klein <michael@fossekall.de>
> +
> +description: |
> +  When the power-off handler is called, one more regulators are disabled
> +  by calling regulator_force_disable(). If the power is still on and the
> +  CPU still running after a 3000ms delay, a WARN_ON(1) is emitted.
> +
> +properties:
> +  compatible:
> +    const: "regulator-poweroff"
> +
> +  regulator-names:
> +    description:
> +      Array of regulator names
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +
> +  REGULATOR-supply:

This should be a patternProperties

> +    description:
> +      For any REGULATOR listed in regulator-names, a phandle
> +      to the corresponding regulator node
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +
> +  timeout-ms:
> +    description:
> +      Time to wait before asserting a WARN_ON(1). If nothing is
> +      specified, 3000 ms is used.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +required:
> +  - compatible
> +  - regulator-names
> +  - REGULATOR-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    regulator-poweroff {
> +        compatible = "regulator-poweroff";
> +        regulator-names = "vcc1v2", "vcc-dram";
> +        vcc1v2-supply = <&reg_vcc1v2>;
> +        vcc-dram-supply = <&reg_vcc_dram>;
> +    };

I'm not entirely sure how multiple regulators would work here. I guess
the ordering is board/purpose sensitive. In this particular case, I
assume that vcc1v2 would be shut down before vcc-dram?

If so, I would expect that one regulator_force_disable is run, the CPU
is disabled and you never get the chance to cut vcc-dram.

Similarly, cutting the RAM regulator first would probably be fine if
you're running code from the cache / SRAM, but I don't see anything
making sure it's the case in the driver?

Maxime

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

Re: [PATCH v3 2/3] Documentation: DT: binding documentation for regulator-poweroff

[Michael Klein]

Thanks for reviewing!

On Tue, Dec 08, 2020 at 11:13:58AM +0100, Maxime Ripard wrote:
>On Mon, Dec 07, 2020 at 03:27:55PM +0100, Michael Klein wrote:
>> Add devicetree binding documentation for regulator-poweroff driver.
>>
>> Signed-off-by: Michael Klein <michael@fossekall.de>
>> ---
>>  .../power/reset/regulator-poweroff.yaml       | 53 +++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
>> new file mode 100644
>> index 000000000000..8c8ce6bb031a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
>> @@ -0,0 +1,53 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/power/reset/regulator-poweroff.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Force-disable power regulators to turn the power off.
>> +
>> +maintainers:
>> +  - Michael Klein <michael@fossekall.de>
>> +
>> +description: |
>> +  When the power-off handler is called, one more regulators are disabled
>> +  by calling regulator_force_disable(). If the power is still on and the
>> +  CPU still running after a 3000ms delay, a WARN_ON(1) is emitted.
>> +
>> +properties:
>> +  compatible:
>> +    const: "regulator-poweroff"
>> +
>> +  regulator-names:
>> +    description:
>> +      Array of regulator names
>> +    $ref: /schemas/types.yaml#/definitions/string-array
>> +
>> +  REGULATOR-supply:
>
>This should be a patternProperties
>
>> +    description:
>> +      For any REGULATOR listed in regulator-names, a phandle
>> +      to the corresponding regulator node
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +
>> +  timeout-ms:
>> +    description:
>> +      Time to wait before asserting a WARN_ON(1). If nothing is
>> +      specified, 3000 ms is used.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +required:
>> +  - compatible
>> +  - regulator-names
>> +  - REGULATOR-supply
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    regulator-poweroff {
>> +        compatible = "regulator-poweroff";
>> +        regulator-names = "vcc1v2", "vcc-dram";
>> +        vcc1v2-supply = <&reg_vcc1v2>;
>> +        vcc-dram-supply = <&reg_vcc_dram>;
>> +    };
>
>I'm not entirely sure how multiple regulators would work here. I guess
>the ordering is board/purpose sensitive. In this particular case, I
>assume that vcc1v2 would be shut down before vcc-dram?

yes, the regulators are shut down from left to right.

>If so, I would expect that one regulator_force_disable is run, the CPU
>is disabled and you never get the chance to cut vcc-dram.

I assume that any relevant regulator here has enough capacitance on the 
output that provides enough charge to disable any remaining regulators 
(my board has 3*10µF for vcc1v2 and 1*10µF for vcc-dram). But there is 
of course no guarantee, so I'm shutting down the most relevant (in terms 
of current consumption) regulator first.

In any case, if it's deemed unnecessary to allow more than one regulator 
in the driver I could remove the regulator-names property altogether and 
reduce the DT node to:

   regulator-poweroff {
       compatible = "regulator-poweroff";
       poweroff-supply = <&reg_vcc1v2>;
   };

-- 
Michael

Re: [PATCH v3 2/3] Documentation: DT: binding documentation for regulator-poweroff

[Maxime Ripard]

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

Hi

On Tue, Dec 08, 2020 at 01:52:14PM +0100, Michael Klein wrote:
> Thanks for reviewing!
> 
> On Tue, Dec 08, 2020 at 11:13:58AM +0100, Maxime Ripard wrote:
> > On Mon, Dec 07, 2020 at 03:27:55PM +0100, Michael Klein wrote:
> > > Add devicetree binding documentation for regulator-poweroff driver.
> > > 
> > > Signed-off-by: Michael Klein <michael@fossekall.de>
> > > ---
> > >  .../power/reset/regulator-poweroff.yaml       | 53 +++++++++++++++++++
> > >  1 file changed, 53 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
> > > new file mode 100644
> > > index 000000000000..8c8ce6bb031a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
> > > @@ -0,0 +1,53 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/power/reset/regulator-poweroff.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Force-disable power regulators to turn the power off.
> > > +
> > > +maintainers:
> > > +  - Michael Klein <michael@fossekall.de>
> > > +
> > > +description: |
> > > +  When the power-off handler is called, one more regulators are disabled
> > > +  by calling regulator_force_disable(). If the power is still on and the
> > > +  CPU still running after a 3000ms delay, a WARN_ON(1) is emitted.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: "regulator-poweroff"
> > > +
> > > +  regulator-names:
> > > +    description:
> > > +      Array of regulator names
> > > +    $ref: /schemas/types.yaml#/definitions/string-array
> > > +
> > > +  REGULATOR-supply:
> > 
> > This should be a patternProperties
> > 
> > > +    description:
> > > +      For any REGULATOR listed in regulator-names, a phandle
> > > +      to the corresponding regulator node
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +
> > > +  timeout-ms:
> > > +    description:
> > > +      Time to wait before asserting a WARN_ON(1). If nothing is
> > > +      specified, 3000 ms is used.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +required:
> > > +  - compatible
> > > +  - regulator-names
> > > +  - REGULATOR-supply
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    regulator-poweroff {
> > > +        compatible = "regulator-poweroff";
> > > +        regulator-names = "vcc1v2", "vcc-dram";
> > > +        vcc1v2-supply = <&reg_vcc1v2>;
> > > +        vcc-dram-supply = <&reg_vcc_dram>;
> > > +    };
> > 
> > I'm not entirely sure how multiple regulators would work here. I guess
> > the ordering is board/purpose sensitive. In this particular case, I
> > assume that vcc1v2 would be shut down before vcc-dram?
> 
> yes, the regulators are shut down from left to right.
> 
> > If so, I would expect that one regulator_force_disable is run, the CPU
> > is disabled and you never get the chance to cut vcc-dram.
> 
> I assume that any relevant regulator here has enough capacitance on the
> output that provides enough charge to disable any remaining regulators (my
> board has 3*10µF for vcc1v2 and 1*10µF for vcc-dram). But there is of course
> no guarantee, so I'm shutting down the most relevant (in terms of current
> consumption) regulator first.
>
> In any case, if it's deemed unnecessary to allow more than one regulator in
> the driver I could remove the regulator-names property altogether and reduce
> the DT node to:
> 
>   regulator-poweroff {
>       compatible = "regulator-poweroff";
>       poweroff-supply = <&reg_vcc1v2>;
>   };

It's mostly about semantics at this point but there's value in shutting
down the RAM as well if we're taking some precautions I mentionned.
Maybe we can name that regulator cpu-supply, so that we can easily add
the DRAM one if needed, and the semantics are clear?

Maxime

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

Re: [PATCH v3 2/3] Documentation: DT: binding documentation for regulator-poweroff

[Rob Herring]

On Mon, 07 Dec 2020 15:27:55 +0100, Michael Klein wrote:
> Add devicetree binding documentation for regulator-poweroff driver.
> 
> Signed-off-by: Michael Klein <michael@fossekall.de>
> ---
>  .../power/reset/regulator-poweroff.yaml       | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/reset/regulator-poweroff.example.dt.yaml: regulator-poweroff: 'REGULATOR-supply' is a required property
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/reset/regulator-poweroff.example.dt.yaml: regulator-poweroff: 'vcc-dram-supply', 'vcc1v2-supply' do not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml


See https://patchwork.ozlabs.org/patch/1412084

The base for the patch is generally the last rc1. Any dependencies
should be noted.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v3 2/3] Documentation: DT: binding documentation for regulator-poweroff

[Rob Herring]

On Mon, Dec 07, 2020 at 03:27:55PM +0100, Michael Klein wrote:
> Add devicetree binding documentation for regulator-poweroff driver.
> 
> Signed-off-by: Michael Klein <michael@fossekall.de>
> ---
>  .../power/reset/regulator-poweroff.yaml       | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
> new file mode 100644
> index 000000000000..8c8ce6bb031a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/reset/regulator-poweroff.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Force-disable power regulators to turn the power off.
> +
> +maintainers:
> +  - Michael Klein <michael@fossekall.de>
> +
> +description: |
> +  When the power-off handler is called, one more regulators are disabled
> +  by calling regulator_force_disable(). If the power is still on and the
> +  CPU still running after a 3000ms delay, a WARN_ON(1) is emitted.

WARN_ON is a Linux thing. Bindings are independent from Linux.

> +
> +properties:
> +  compatible:
> +    const: "regulator-poweroff"
> +
> +  regulator-names:

We already have 'regulator-name' which is something different, and 
*-names already has a defined usage as a companion to other properties 
('foo-names' goes with 'foos'). More on this below...

> +    description:
> +      Array of regulator names
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +
> +  REGULATOR-supply:
> +    description:
> +      For any REGULATOR listed in regulator-names, a phandle
> +      to the corresponding regulator node
> +    $ref: /schemas/types.yaml#/definitions/phandle

*-supply already has a type.

> +
> +  timeout-ms:
> +    description:
> +      Time to wait before asserting a WARN_ON(1). If nothing is
> +      specified, 3000 ms is used.
> +    $ref: /schemas/types.yaml#/definitions/uint32

Do we really need to tune the timeout just for an error message?

> +
> +required:
> +  - compatible
> +  - regulator-names
> +  - REGULATOR-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    regulator-poweroff {
> +        compatible = "regulator-poweroff";
> +        regulator-names = "vcc1v2", "vcc-dram";
> +        vcc1v2-supply = <&reg_vcc1v2>;
> +        vcc-dram-supply = <&reg_vcc_dram>;

-supply names are supposed to be named based on the consumer names (e.g. 
LDO1 regulator supplies vcc-supply). To avoid 'regulator-names' and 
simplifier the driver, I'd just define fixed, known names. Something 
like:

power1-supply
power2-supply
...

Rob

[PATCH v3 3/3] ARM: dts: sun8i-h2-plus-bananapi-m2-zero: add poweroff node

[Michael Klein]

Add add devicetree information for the regulator-poweroff driver.

Signed-off-by: Michael Klein <michael@fossekall.de>
---
This patch depends on the regulator names added in
https://lore.kernel.org/linux-arm-kernel/20201130183841.136708-1-michael@fossekall.de/

 arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
index b43028f9e6df..408c6e581700 100644
--- a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
+++ b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
@@ -86,6 +86,13 @@ reg_vcc1v2: vcc1v2 {
 		vin-supply = <&reg_vcc5v0>;
 	};
 
+	poweroff {
+		compatible = "regulator-poweroff";
+		regulator-names = "vcc1v2", "vcc-dram";
+		vcc1v2-supply = <&reg_vcc1v2>;
+		vcc-dram-supply = <&reg_vcc_dram>;
+	};
+
 	wifi_pwrseq: wifi_pwrseq {
 		compatible = "mmc-pwrseq-simple";
 		reset-gpios = <&r_pio 0 7 GPIO_ACTIVE_LOW>; /* PL7 */
-- 
2.29.2