[PATCH v3 0/3] regulator: bd9571mwv: Add support for toggle power switches

[PATCH v3 0/3] regulator: bd9571mwv: Add support for toggle power switches

[Geert Uytterhoeven]

	Hi all,

The ROHM BD9571MWV PMIC on the Renesas Salvator-X(S) and ULCB
development boards supports DDR Backup Power, which means that the DDR
power rails can be kept powered while the main SoC is powered down.

This patch series extends the support for DDR backup mode (see commit
6eb0bfae6973eb6a ("regulator: bd9571mwv: Add support for backup mode"))
to systems with toggle instead of momentary power switches.

With a toggle power switch (or level signal), the following steps must
be followed exactly:
   1. Configure PMIC for backup mode, which changes the role of the
      power switch to a wake-up switch, 
   2. Switch accessory power switch off, to prepare for system suspend,
      which is a manual step not controlled by software,
   3. Suspend system,
   4. Switch accessory power switch on, to resume.

Unlike on systems with a momentary toggle switch, an additional step 2
must be performed in between step 1 and step 3.  Hence step 1 can no
longer be handled in the PMIC's suspend callback.

This patch series allows performing step 1 when the user writes
"on" to the PMIC's "backup_mode" virtual file in sysfs, e.g.

    echo on > /sys/bus/i2c/drivers/bd9571mwv/*/bd9571mwv-regulator*/backup_mode

Conversely, writing "off" reverts the role of the accessory switch to a
power switch.
Note that unlike with momentary switches, backup mode is not enabled by
default, as enabling it prevents the board from being powered off using
the power switch, which may confuse the user.

Changes compared to v2:
  - Drop 'PM / wakeup: Add callback for wake-up change notification',
  - New patch 'regulator: bd9571mwv: Use "backup_mode" sysfs file
    instead of "wake_up"',
  - Replace use of "wake_up" sysfs file and extra callback for wake-up
    change notification by custom "backup_mode" sysfs file,
  - New patch ;regulator: bd9571mwv: Document "backup_mode" sysfs file'.

Changes compared to v1:
  - Improve patch descriptions,
  - Drop "return;" at end of function.

This has been tested on Salvator-XS.

For testing, this series is also available in the
topic/bd9571-toggle-power-switch-v3 branch of my renesas-drivers git
repository at
git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git.

Thanks!

Geert Uytterhoeven (3):
  regulator: bd9571mwv: Use "backup_mode" sysfs file instead of
    "wake_up"
  regulator: bd9571mwv: Add support for toggle power switches
  regulator: bd9571mwv: Document "backup_mode" sysfs file

 .../testing/sysfs-driver-bd9571mwv-regulator  | 27 ++++++++
 drivers/regulator/bd9571mwv-regulator.c       | 69 +++++++++++++++++--
 2 files changed, 90 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-bd9571mwv-regulator

-- 
2.17.1

[PATCH v3 1/3] regulator: bd9571mwv: Use "backup_mode" sysfs file instead of "wake_up"

[Geert Uytterhoeven]

Currently the BD9571MWV PMIC driver uses the standard "wake_up" sysfs
file to control enablement of DDR Backup Mode.

However, configuring DDR Backup Mode is not really equivalent to
configuring the PMIC as a wake-up source.  To avoid confusion, use a
custom "backup_mode" attribute file in sysfs instead.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
  - New.
---
 drivers/regulator/bd9571mwv-regulator.c | 52 ++++++++++++++++++++++---
 1 file changed, 46 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/bd9571mwv-regulator.c b/drivers/regulator/bd9571mwv-regulator.c
index be574eb444ebda97..7432b605db4dc879 100644
--- a/drivers/regulator/bd9571mwv-regulator.c
+++ b/drivers/regulator/bd9571mwv-regulator.c
@@ -30,6 +30,7 @@ struct bd9571mwv_reg {
 	/* DDR Backup Power */
 	u8 bkup_mode_cnt_keepon;	/* from "rohm,ddr-backup-power" */
 	u8 bkup_mode_cnt_saved;
+	bool bkup_mode_enabled;
 
 	/* Power switch type */
 	bool rstbmode_level;
@@ -171,13 +172,40 @@ static int bd9571mwv_bkup_mode_write(struct bd9571mwv *bd, unsigned int mode)
 	return 0;
 }
 
+static ssize_t backup_mode_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct bd9571mwv_reg *bdreg = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%s\n", bdreg->bkup_mode_enabled ? "on" : "off");
+}
+
+static ssize_t backup_mode_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct bd9571mwv_reg *bdreg = dev_get_drvdata(dev);
+	int ret;
+
+	if (!count)
+		return 0;
+
+	ret = kstrtobool(buf, &bdreg->bkup_mode_enabled);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+DEVICE_ATTR_RW(backup_mode);
+
 static int bd9571mwv_suspend(struct device *dev)
 {
 	struct bd9571mwv_reg *bdreg = dev_get_drvdata(dev);
 	unsigned int mode;
 	int ret;
 
-	if (!device_may_wakeup(dev))
+	if (!bdreg->bkup_mode_enabled)
 		return 0;
 
 	/* Save DDR Backup Mode */
@@ -204,7 +232,7 @@ static int bd9571mwv_resume(struct device *dev)
 {
 	struct bd9571mwv_reg *bdreg = dev_get_drvdata(dev);
 
-	if (!device_may_wakeup(dev))
+	if (!bdreg->bkup_mode_enabled)
 		return 0;
 
 	/* Restore DDR Backup Mode */
@@ -227,7 +255,7 @@ static int bd9571mwv_regulator_probe(struct platform_device *pdev)
 	struct bd9571mwv_reg *bdreg;
 	struct regulator_dev *rdev;
 	unsigned int val;
-	int i;
+	int i, ret;
 
 	bdreg = devm_kzalloc(&pdev->dev, sizeof(*bdreg), GFP_KERNEL);
 	if (!bdreg)
@@ -270,15 +298,26 @@ static int bd9571mwv_regulator_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+#ifdef CONFIG_PM_SLEEP
 	if (bdreg->bkup_mode_cnt_keepon) {
-		device_set_wakeup_capable(&pdev->dev, true);
 		/*
-		 * Wakeup is enabled by default in pulse mode, but needs
+		 * Backup mode is enabled by default in pulse mode, but needs
 		 * explicit user setup in level mode.
 		 */
-		device_set_wakeup_enable(&pdev->dev, bdreg->rstbmode_pulse);
+		bdreg->bkup_mode_enabled = bdreg->rstbmode_pulse;
+
+		ret = device_create_file(&pdev->dev, &dev_attr_backup_mode);
+		if (ret)
+			return ret;
 	}
+#endif /* CONFIG_PM_SLEEP */
+
+	return 0;
+}
 
+static int bd9571mwv_regulator_remove(struct platform_device *pdev)
+{
+	device_remove_file(&pdev->dev, &dev_attr_backup_mode);
 	return 0;
 }
 
@@ -294,6 +333,7 @@ static struct platform_driver bd9571mwv_regulator_driver = {
 		.pm = DEV_PM_OPS,
 	},
 	.probe = bd9571mwv_regulator_probe,
+	.remove = bd9571mwv_regulator_remove,
 	.id_table = bd9571mwv_regulator_id_table,
 };
 module_platform_driver(bd9571mwv_regulator_driver);
-- 
2.17.1

Re: [PATCH v3 1/3] regulator: bd9571mwv: Use "backup_mode" sysfs file instead of "wake_up"

[Geert Uytterhoeven]

On Fri, Jul 13, 2018 at 6:31 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> Currently the BD9571MWV PMIC driver uses the standard "wake_up" sysfs
> file to control enablement of DDR Backup Mode.
>
> However, configuring DDR Backup Mode is not really equivalent to
> configuring the PMIC as a wake-up source.  To avoid confusion, use a
> custom "backup_mode" attribute file in sysfs instead.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v3:
>   - New.
> ---
>  drivers/regulator/bd9571mwv-regulator.c | 52 ++++++++++++++++++++++---
>  1 file changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/regulator/bd9571mwv-regulator.c b/drivers/regulator/bd9571mwv-regulator.c
> index be574eb444ebda97..7432b605db4dc879 100644
> --- a/drivers/regulator/bd9571mwv-regulator.c
> +++ b/drivers/regulator/bd9571mwv-regulator.c

> @@ -270,15 +298,26 @@ static int bd9571mwv_regulator_probe(struct platform_device *pdev)
>                 return -EINVAL;
>         }
>
> +#ifdef CONFIG_PM_SLEEP
>         if (bdreg->bkup_mode_cnt_keepon) {
> -               device_set_wakeup_capable(&pdev->dev, true);
>                 /*
> -                * Wakeup is enabled by default in pulse mode, but needs
> +                * Backup mode is enabled by default in pulse mode, but needs
>                  * explicit user setup in level mode.
>                  */
> -               device_set_wakeup_enable(&pdev->dev, bdreg->rstbmode_pulse);
> +               bdreg->bkup_mode_enabled = bdreg->rstbmode_pulse;
> +
> +               ret = device_create_file(&pdev->dev, &dev_attr_backup_mode);
> +               if (ret)
> +                       return ret;
>         }
> +#endif /* CONFIG_PM_SLEEP */
> +
> +       return 0;
> +}
>
> +static int bd9571mwv_regulator_remove(struct platform_device *pdev)
> +{
> +       device_remove_file(&pdev->dev, &dev_attr_backup_mode);

Woops, this line needs to be protected by CONFIG_PM_SLEEP.
Thank you, 0-day!

>         return 0;
>  }
>
> @@ -294,6 +333,7 @@ static struct platform_driver bd9571mwv_regulator_driver = {
>                 .pm = DEV_PM_OPS,
>         },
>         .probe = bd9571mwv_regulator_probe,
> +       .remove = bd9571mwv_regulator_remove,
>         .id_table = bd9571mwv_regulator_id_table,
>  };
>  module_platform_driver(bd9571mwv_regulator_driver);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v3 2/3] regulator: bd9571mwv: Add support for toggle power switches

[Geert Uytterhoeven]

Extend the existing support for backup mode to toggle power switches.
With a toggle power switch (or level signal), the following steps must
be followed exactly:
   1. Configure PMIC for backup mode,
   2. Switch accessory power switch off, to prepare for system suspend,
      which is a manual step not controlled by software,
   3. Suspend system,
   4. Switch accessory power switch on to wake up system.

Hence the PMIC is configured for backup mode when "on" or "1" is written
to the PMIC's "backup_mode" virtual file in sysfs.  Conversely, writing
"off" or "0" reverts the role of the accessory switch to a power
switch.

Unlike with momentary switches, backup mode is not enabled by default,
as enabling it prevents the board from being powered off using the power
switch, which may confuse the user.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
  - Replace use of "wake_up" sysfs file and extra callback for wake-up
    change notification by custom "backup_mode" sysfs file,

v2:
  - Improve patch description,
  - Drop "return;" at end of function.
---
 drivers/regulator/bd9571mwv-regulator.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/regulator/bd9571mwv-regulator.c b/drivers/regulator/bd9571mwv-regulator.c
index 7432b605db4dc879..9b5df51751624d8b 100644
--- a/drivers/regulator/bd9571mwv-regulator.c
+++ b/drivers/regulator/bd9571mwv-regulator.c
@@ -185,6 +185,7 @@ static ssize_t backup_mode_store(struct device *dev,
 				 const char *buf, size_t count)
 {
 	struct bd9571mwv_reg *bdreg = dev_get_drvdata(dev);
+	unsigned int mode;
 	int ret;
 
 	if (!count)
@@ -194,6 +195,22 @@ static ssize_t backup_mode_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (!bdreg->rstbmode_level)
+		return count;
+
+	/* Configure DDR Backup Mode */
+	ret = bd9571mwv_bkup_mode_read(bdreg->bd, &mode);
+	if (ret)
+		return ret;
+
+	mode &= ~BD9571MWV_BKUP_MODE_CNT_KEEPON_MASK;
+	if (bdreg->bkup_mode_enabled)
+		mode |= bdreg->bkup_mode_cnt_keepon;
+
+	ret = bd9571mwv_bkup_mode_write(bdreg->bd, mode);
+	if (ret)
+		return ret;
+
 	return count;
 }
 
-- 
2.17.1

[PATCH v3 3/3] regulator: bd9571mwv: Document "backup_mode" sysfs file

[Geert Uytterhoeven]

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
  - New.
---
 .../testing/sysfs-driver-bd9571mwv-regulator  | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-bd9571mwv-regulator

diff --git a/Documentation/ABI/testing/sysfs-driver-bd9571mwv-regulator b/Documentation/ABI/testing/sysfs-driver-bd9571mwv-regulator
new file mode 100644
index 0000000000000000..f2baec6d70312591
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-bd9571mwv-regulator
@@ -0,0 +1,27 @@
+What:		/sys/bus/i2c/devices/.../bd9571mwv-regulator.*.auto/backup_mode
+Date:		Jul 2018
+KernelVersion:	4.19
+Contact:	Geert Uytterhoeven <geert+renesas@glider.be>
+Description:	Read/write the current state of DDR Backup Mode, which controls
+		if DDR power rails will be kept powered during system suspend.
+		("on"/"1" = enabled, "off"/"0" = disabled).
+		Two types of switches (or signals) can be used:
+		  A. With a momentary power switch (or pulse signal), DDR
+		     Backup Mode is enabled by default when available, as the
+		     PMIC will be configured only during system suspend.
+		  B. With a toggle power switch (or level signal), the
+		     following steps must be followed exactly:
+		       1. Configure PMIC for backup mode, to change the role of
+			  the accessory power switch from a power switch to a
+			  wake-up switch,
+		       2. Switch accessory power switch off, to prepare for
+			  system suspend, which is a manual step not controlled
+			  by software,
+		       3. Suspend system,
+		       4. Switch accessory power switch on, to resume the
+			  system.
+		     DDR Backup Mode must be explicitly enabled by the user,
+		     to invoke step 1.
+		See also Documentation/devicetree/bindings/mfd/bd9571mwv.txt.
+Users:		User space applications for embedded boards equipped with a
+		BD9571MWV PMIC.
-- 
2.17.1

Re: [PATCH v3 3/3] regulator: bd9571mwv: Document "backup_mode" sysfs file

[Pavel Machek]

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

On Fri 2018-07-13 17:47:20, Geert Uytterhoeven wrote:
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>


> +		     following steps must be followed exactly:
> +		       1. Configure PMIC for backup mode, to change the role of
> +			  the accessory power switch from a power switch to a
> +			  wake-up switch,
> +		       2. Switch accessory power switch off, to prepare for
> +			  system suspend, which is a manual step not controlled
> +			  by software,
> +		       3. Suspend system,
> +		       4. Switch accessory power switch on, to resume the
> +			  system.
> +		     DDR Backup Mode must be explicitly enabled by the user,
> +		     to invoke step 1.

I'd add "please don't design hardware like that" :-). Anyway,

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

Re: [PATCH v3 0/3] regulator: bd9571mwv: Add support for toggle power switches

[Pavel Machek]

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

Hi!

> The ROHM BD9571MWV PMIC on the Renesas Salvator-X(S) and ULCB
> development boards supports DDR Backup Power, which means that the DDR
> power rails can be kept powered while the main SoC is powered down.
> 
> This patch series extends the support for DDR backup mode (see commit
> 6eb0bfae6973eb6a ("regulator: bd9571mwv: Add support for backup mode"))
> to systems with toggle instead of momentary power switches.
> 
> With a toggle power switch (or level signal), the following steps must
> be followed exactly:
>    1. Configure PMIC for backup mode, which changes the role of the
>       power switch to a wake-up switch, 
>    2. Switch accessory power switch off, to prepare for system suspend,
>       which is a manual step not controlled by software,
>    3. Suspend system,
>    4. Switch accessory power switch on, to resume.
> 
> Unlike on systems with a momentary toggle switch, an additional step 2
> must be performed in between step 1 and step 3.  Hence step 1 can no
> longer be handled in the PMIC's suspend callback.
> 
> This patch series allows performing step 1 when the user writes
> "on" to the PMIC's "backup_mode" virtual file in sysfs, e.g.
> 
>     echo on > /sys/bus/i2c/drivers/bd9571mwv/*/bd9571mwv-regulator*/backup_mode

Do you expect more boards to have similar design?

If so, we may want to have standard place in /sys/ not depending on
i2c paths and driver names, but I believe such design is so... awkward
that it is not going to appear anywhere else...?

								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

Re: [PATCH v3 0/3] regulator: bd9571mwv: Add support for toggle power switches

[Geert Uytterhoeven]

Hi Pavel,

On Sun, Jul 15, 2018 at 6:57 PM Pavel Machek <pavel@ucw.cz> wrote:
> > The ROHM BD9571MWV PMIC on the Renesas Salvator-X(S) and ULCB
> > development boards supports DDR Backup Power, which means that the DDR
> > power rails can be kept powered while the main SoC is powered down.
> >
> > This patch series extends the support for DDR backup mode (see commit
> > 6eb0bfae6973eb6a ("regulator: bd9571mwv: Add support for backup mode"))
> > to systems with toggle instead of momentary power switches.
> >
> > With a toggle power switch (or level signal), the following steps must
> > be followed exactly:
> >    1. Configure PMIC for backup mode, which changes the role of the
> >       power switch to a wake-up switch,
> >    2. Switch accessory power switch off, to prepare for system suspend,
> >       which is a manual step not controlled by software,
> >    3. Suspend system,
> >    4. Switch accessory power switch on, to resume.
> >
> > Unlike on systems with a momentary toggle switch, an additional step 2
> > must be performed in between step 1 and step 3.  Hence step 1 can no
> > longer be handled in the PMIC's suspend callback.
> >
> > This patch series allows performing step 1 when the user writes
> > "on" to the PMIC's "backup_mode" virtual file in sysfs, e.g.
> >
> >     echo on > /sys/bus/i2c/drivers/bd9571mwv/*/bd9571mwv-regulator*/backup_mode
>
> Do you expect more boards to have similar design?
>
> If so, we may want to have standard place in /sys/ not depending on
> i2c paths and driver names, but I believe such design is so... awkward
> that it is not going to appear anywhere else...?

I have no idea. The BD9571MWV PMIC supports two power switch wirings.
Only one of them is problematic for usability reasons.
The newer Renesas ULCB boards use the good wiring.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds