[PATCH/RFC 0/2] regulator: bd9571mwv: Add support for toggle power switches

[PATCH/RFC 0/2] 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[1] 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 proposes a new callback for wake-up change
notification, which allows performing step 1 when the user writes
"enabled" to the PMIC's "wakeup" virtual file in sysfs, e.g.

    echo enabled > /sys/devices/platform/soc/e60b0000.i2c/i2c-7/7-0030/bd9571mwv-regulator.2.auto/power/wakeup

Conversely, writing "disabled" 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.

This has been tested on M3ULCB (thanks Jacopo!), and on Salvator-X(S).

For testing, driver and DTS patches are available in the
topic/bd9571-ddr-backup-mode-driver-v2 and
topic/bd9571-ddr-backup-mode-dt-v2 branches of my renesas-drivers git
repository at
git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git.

Thanks for your comments!

[1] https://lkml.org/lkml/2018/3/14/302

Geert Uytterhoeven (2):
  PM / wakeup: Add callback for wake-up change notification
  regulator: bd9571mwv: Add support for toggle power switches

 drivers/base/power/wakeup.c             |  4 ++++
 drivers/regulator/bd9571mwv-regulator.c | 24 ++++++++++++++++++++++++
 include/linux/pm.h                      |  1 +
 3 files changed, 29 insertions(+)

-- 
2.7.4

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/RFC 1/2] PM / wakeup: Add callback for wake-up change notification

[Geert Uytterhoeven]

Add a callback to inform a device that his wake-up setting has been
changed.  This allows a device to synchronize device configuration with
an external user action.

E.g. on systems using a Rohm BD9571MWV PMIC and a toggle accessory power
switch, the system suspend procedure is:
  1. Configure PMIC for DDR backup mode, which changes the role of the
     accessory power switch from a power to a wake-up switch,
  2. Switch accessory power switch off, to prepare for system suspend,
  3. Suspend system.

Hence step 1 cannot be done in the PMIC's suspend callback, but it can
be done in the new callback, in response to the user writing "enabled"
to the PMIC's wakeup virtual file in sysfs.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Is there a better way to handle this?
---
 drivers/base/power/wakeup.c | 4 ++++
 include/linux/pm.h          | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index ea01621ed769ab26..40bda13d4dcd36f3 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -256,6 +256,8 @@ static int device_wakeup_attach(struct device *dev, struct wakeup_source *ws)
 	if (dev->power.wakeirq)
 		device_wakeup_attach_irq(dev, dev->power.wakeirq);
 	spin_unlock_irq(&dev->power.lock);
+	if (dev->power.wakeup_change_notify)
+		dev->power.wakeup_change_notify(dev, true);
 	return 0;
 }
 
@@ -373,6 +375,8 @@ static struct wakeup_source *device_wakeup_detach(struct device *dev)
 {
 	struct wakeup_source *ws;
 
+	if (dev->power.wakeup_change_notify)
+		dev->power.wakeup_change_notify(dev, false);
 	spin_lock_irq(&dev->power.lock);
 	ws = dev->power.wakeup;
 	dev->power.wakeup = NULL;
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e723b78d835706f2..3dec274bffffc6f8 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -599,6 +599,7 @@ struct dev_pm_info {
 	struct list_head	entry;
 	struct completion	completion;
 	struct wakeup_source	*wakeup;
+	void (*wakeup_change_notify)(struct device *dev, bool enable);
 	bool			wakeup_path:1;
 	bool			syscore:1;
 	bool			no_pm_callbacks:1;	/* Owned by the PM core */
-- 
2.7.4

Re: [PATCH/RFC 1/2] PM / wakeup: Add callback for wake-up change notification

[Sergei Shtylyov]

On 03/14/2018 02:26 PM, Geert Uytterhoeven wrote:

> Add a callback to inform a device that his wake-up setting has been

   s/his/its/.

> changed.  This allows a device to synchronize device configuration with
> an external user action.
> 
> E.g. on systems using a Rohm BD9571MWV PMIC and a toggle accessory power
> switch, the system suspend procedure is:
>   1. Configure PMIC for DDR backup mode, which changes the role of the
>      accessory power switch from a power to a wake-up switch,
>   2. Switch accessory power switch off, to prepare for system suspend,
>   3. Suspend system.
> 
> Hence step 1 cannot be done in the PMIC's suspend callback, but it can
> be done in the new callback, in response to the user writing "enabled"
> to the PMIC's wakeup virtual file in sysfs.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
[...]

MBR, Sergei

Re: [PATCH/RFC 1/2] PM / wakeup: Add callback for wake-up change notification

[Rafael J. Wysocki]

First, sorry for the huge delay.

On Wednesday, March 14, 2018 12:26:24 PM CEST Geert Uytterhoeven wrote:
> Add a callback to inform a device that his wake-up setting has been
> changed.  This allows a device to synchronize device configuration with
> an external user action.
> 
> E.g. on systems using a Rohm BD9571MWV PMIC and a toggle accessory power
> switch, the system suspend procedure is:
>   1. Configure PMIC for DDR backup mode, which changes the role of the
>      accessory power switch from a power to a wake-up switch,
>   2. Switch accessory power switch off, to prepare for system suspend,
>   3. Suspend system.
> 
> Hence step 1 cannot be done in the PMIC's suspend callback,

I don't quite understand this, so can you please explain?

What can't it be done from ->prepare() or even from a suspend notifier?

> but it can be done in the new callback, in response to the user writing "enabled"
> to the PMIC's wakeup virtual file in sysfs.

Re: [PATCH/RFC 1/2] PM / wakeup: Add callback for wake-up change notification

[Geert Uytterhoeven]

Hi Rafael,

On Mon, Apr 23, 2018 at 11:18 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, March 14, 2018 12:26:24 PM CEST Geert Uytterhoeven wrote:
>> Add a callback to inform a device that his wake-up setting has been
>> changed.  This allows a device to synchronize device configuration with
>> an external user action.
>>
>> E.g. on systems using a Rohm BD9571MWV PMIC and a toggle accessory power
>> switch, the system suspend procedure is:
>>   1. Configure PMIC for DDR backup mode, which changes the role of the
>>      accessory power switch from a power to a wake-up switch,
>>   2. Switch accessory power switch off, to prepare for system suspend,
>>   3. Suspend system.
>>
>> Hence step 1 cannot be done in the PMIC's suspend callback,
>
> I don't quite understand this, so can you please explain?
>
> What can't it be done from ->prepare() or even from a suspend notifier?
>
>> but it can be done in the new callback, in response to the user writing "enabled"
>> to the PMIC's wakeup virtual file in sysfs.

Step 2 (flip a switch on the board) is a manual step, to be done by the user.
So it cannot be done from any suspend callback or notifier.

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

Re: [PATCH/RFC 1/2] PM / wakeup: Add callback for wake-up change notification

[Rafael J. Wysocki]

On Mon, Apr 23, 2018 at 11:32 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Rafael,
>
> On Mon, Apr 23, 2018 at 11:18 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Wednesday, March 14, 2018 12:26:24 PM CEST Geert Uytterhoeven wrote:
>>> Add a callback to inform a device that his wake-up setting has been
>>> changed.  This allows a device to synchronize device configuration with
>>> an external user action.
>>>
>>> E.g. on systems using a Rohm BD9571MWV PMIC and a toggle accessory power
>>> switch, the system suspend procedure is:
>>>   1. Configure PMIC for DDR backup mode, which changes the role of the
>>>      accessory power switch from a power to a wake-up switch,
>>>   2. Switch accessory power switch off, to prepare for system suspend,
>>>   3. Suspend system.
>>>
>>> Hence step 1 cannot be done in the PMIC's suspend callback,
>>
>> I don't quite understand this, so can you please explain?
>>
>> What can't it be done from ->prepare() or even from a suspend notifier?
>>
>>> but it can be done in the new callback, in response to the user writing "enabled"
>>> to the PMIC's wakeup virtual file in sysfs.
>
> Step 2 (flip a switch on the board) is a manual step, to be done by the user.
> So it cannot be done from any suspend callback or notifier.

I still find it somewhat difficult to follow you here. :-)

How is this expected to work?  Is the user expected to flip the switch
on the board and then write to "wakeup" or is that the other way
around?

Re: [PATCH/RFC 1/2] PM / wakeup: Add callback for wake-up change notification

[Geert Uytterhoeven]

Hi Rafael,

On Mon, Apr 23, 2018 at 11:37 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Mon, Apr 23, 2018 at 11:32 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Mon, Apr 23, 2018 at 11:18 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> On Wednesday, March 14, 2018 12:26:24 PM CEST Geert Uytterhoeven wrote:
>>>> Add a callback to inform a device that his wake-up setting has been
>>>> changed.  This allows a device to synchronize device configuration with
>>>> an external user action.
>>>>
>>>> E.g. on systems using a Rohm BD9571MWV PMIC and a toggle accessory power
>>>> switch, the system suspend procedure is:
>>>>   1. Configure PMIC for DDR backup mode, which changes the role of the
>>>>      accessory power switch from a power to a wake-up switch,
>>>>   2. Switch accessory power switch off, to prepare for system suspend,
>>>>   3. Suspend system.
>>>>
>>>> Hence step 1 cannot be done in the PMIC's suspend callback,
>>>
>>> I don't quite understand this, so can you please explain?
>>>
>>> What can't it be done from ->prepare() or even from a suspend notifier?
>>>
>>>> but it can be done in the new callback, in response to the user writing "enabled"
>>>> to the PMIC's wakeup virtual file in sysfs.
>>
>> Step 2 (flip a switch on the board) is a manual step, to be done by the user.
>> So it cannot be done from any suspend callback or notifier.
>
> I still find it somewhat difficult to follow you here. :-)
>
> How is this expected to work?  Is the user expected to flip the switch
> on the board and then write to "wakeup" or is that the other way
> around?

The other way around. The user must not flip the switch before the PMIC
is configured for DDR backup mode, else the system will be powered off
immediately.

  1. User writes to wakeup, which invokes the new callback, and configures
       the PMIC for DDR backup mode,
  2. User flips the switch,
  3. User writes to /sys/power/state to suspend.

After system suspend:
  4. User flips the switch to resume.

Yes, it is convoluted. I hope I managed to explain it clearly this time.

Currently step 1 is done using i2set from userspace, which is very unfriendly
(cfr. https://elinux.org/R-Car/Boards/Salvator-XS#PSCI_System_Suspend).

Note that on systems with a momentary power switch (e.g. R-Car Starter Kit Pro
or Premier aka [HM]3ULCB), step 2 is not needed.
Hence things are simplified a lot, as PMIC configuration can be done from
the PMIC's suspend callback on these platforms.

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/RFC 2/2] 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.
Hence the PMIC is configured for backup mode when "enabled" is written
to the PMIC's "wakeup" virtual file in sysfs, using the newly introduced
callback for wake-up change notification.
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.
Conversely, writing "disabled" reverts the role of the accessory switch
to a power switch.

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

diff --git a/drivers/regulator/bd9571mwv-regulator.c b/drivers/regulator/bd9571mwv-regulator.c
index be574eb444ebda97..435c8da576b604cf 100644
--- a/drivers/regulator/bd9571mwv-regulator.c
+++ b/drivers/regulator/bd9571mwv-regulator.c
@@ -171,6 +171,24 @@ static int bd9571mwv_bkup_mode_write(struct bd9571mwv *bd, unsigned int mode)
 	return 0;
 }
 
+static void bd9571mwv_wakeup_change_notify(struct device *dev, bool enable)
+{
+	struct bd9571mwv_reg *bdreg = dev_get_drvdata(dev);
+
+	unsigned int mode;
+	int ret;
+
+	ret = bd9571mwv_bkup_mode_read(bdreg->bd, &mode);
+	if (ret)
+		return;
+
+	mode &= ~BD9571MWV_BKUP_MODE_CNT_KEEPON_MASK;
+	if (enable)
+		mode |= bdreg->bkup_mode_cnt_keepon;
+
+	bd9571mwv_bkup_mode_write(bdreg->bd, mode);
+}
+
 static int bd9571mwv_suspend(struct device *dev)
 {
 	struct bd9571mwv_reg *bdreg = dev_get_drvdata(dev);
@@ -277,6 +295,11 @@ static int bd9571mwv_regulator_probe(struct platform_device *pdev)
 		 * explicit user setup in level mode.
 		 */
 		device_set_wakeup_enable(&pdev->dev, bdreg->rstbmode_pulse);
+#ifdef CONFIG_PM_SLEEP
+		if (bdreg->rstbmode_level)
+			pdev->dev.power.wakeup_change_notify =
+				bd9571mwv_wakeup_change_notify;
+#endif /* CONFIG_PM_SLEEP */
 	}
 
 	return 0;
-- 
2.7.4

Re: [PATCH/RFC 0/2] regulator: bd9571mwv: Add support for toggle power switches

[Geert Uytterhoeven]

Hi all,

On Wed, Mar 14, 2018 at 12:26 PM, Geert Uytterhoeven
<geert+renesas@glider.be> 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[1] 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 proposes a new callback for wake-up change
> notification, which allows performing step 1 when the user writes
> "enabled" to the PMIC's "wakeup" virtual file in sysfs, e.g.
>
>     echo enabled > /sys/devices/platform/soc/e60b0000.i2c/i2c-7/7-0030/bd9571mwv-regulator.2.auto/power/wakeup
>
> Conversely, writing "disabled" 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.
>
> This has been tested on M3ULCB (thanks Jacopo!), and on Salvator-X(S).
>
> For testing, driver and DTS patches are available in the
> topic/bd9571-ddr-backup-mode-driver-v2 and
> topic/bd9571-ddr-backup-mode-dt-v2 branches of my renesas-drivers git
> repository at
> git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git.
>
> Thanks for your comments!
>
> [1] https://lkml.org/lkml/2018/3/14/302
>
> Geert Uytterhoeven (2):
>   PM / wakeup: Add callback for wake-up change notification
>   regulator: bd9571mwv: Add support for toggle power switches
>
>  drivers/base/power/wakeup.c             |  4 ++++
>  drivers/regulator/bd9571mwv-regulator.c | 24 ++++++++++++++++++++++++
>  include/linux/pm.h                      |  1 +
>  3 files changed, 29 insertions(+)

Any comments/suggestions?

In case you lost the patches from the thread:
https://www.spinics.net/lists/linux-renesas-soc/msg24955.html

Thanks!

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

Re: [PATCH/RFC 0/2] regulator: bd9571mwv: Add support for toggle power switches

[Mark Brown]

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

On Wed, Apr 18, 2018 at 03:29:36PM +0200, Geert Uytterhoeven wrote:

> Any comments/suggestions?

> In case you lost the patches from the thread:
> https://www.spinics.net/lists/linux-renesas-soc/msg24955.html

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, though there are some other maintainers who like them - if in
doubt look at how patches for the subsystem are normally handled.

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

Re: [PATCH/RFC 0/2] regulator: bd9571mwv: Add support for toggle power switches

[Pavel Machek]

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

On Wed 2018-04-18 15:00:41, Mark Brown wrote:
> On Wed, Apr 18, 2018 at 03:29:36PM +0200, Geert Uytterhoeven wrote:
> 
> > Any comments/suggestions?
> 
> > In case you lost the patches from the thread:
> > https://www.spinics.net/lists/linux-renesas-soc/msg24955.html
> 
> Please don't send content free pings and please allow a reasonable time
> for review.  People get busy, go on holiday, attend conferences and so 
> on so unless there is some reason for urgency (like critical bug fixes)
> please allow at least a couple of weeks for review.  If there have been
> review comments then people may be waiting for those to be

If I follow the logs right, there was one month before ping. That
seems pretty reasonable time.

> Sending content free pings adds to the mail volume (if they are seen at
> all) which is often the problem and since they can't be reviewed

Yep, and sending content free complains about pings also adds to the
main volume :-(.

Anyway, last time I sent you a patch... you _had_ time to complain
that I'm pinging too often, but you apparently did not have time to
look at the patch. That patch would have been in time for v4.16-rc1
IIRC.

									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/RFC 0/2] regulator: bd9571mwv: Add support for toggle power switches

[Mark Brown]

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

On Thu, Apr 19, 2018 at 10:13:56PM +0200, Pavel Machek wrote:
> On Wed 2018-04-18 15:00:41, Mark Brown wrote:

> > Please don't send content free pings and please allow a reasonable time
> > for review.  People get busy, go on holiday, attend conferences and so 

> If I follow the logs right, there was one month before ping. That
> seems pretty reasonable time.

Right, but the content free bit still applies (as does the bit about
resending which is the main actionable bit for people).  The two go hand
in hand so often that I just wrote the one thing for both.

> > Sending content free pings adds to the mail volume (if they are seen at
> > all) which is often the problem and since they can't be reviewed

> Yep, and sending content free complains about pings also adds to the
> main volume :-(.

They're not content free.  They're telling people that if it looks like
their patch has fallen through the cracks then they need to resend their
patch and why, without that people (especially newer contributors) might
not be clear about what to do.

> Anyway, last time I sent you a patch... you _had_ time to complain
> that I'm pinging too often, but you apparently did not have time to
> look at the patch. That patch would have been in time for v4.16-rc1
> IIRC.

If that's the tlv320dac33 patch you sent a followup in the middle of the
thread with what you said was a current version or something but never
actually sent that version as a regular patch submission.  You'd also
managed to not have the ASoC on the front of the patch which pushes it
to the bottom of the review queue, it won't turn up when I look in in my
inbox for ASoC patches.

Now, I for whatever reason didn't explicitly tell you I was expecting a
resend so you didn't explicitly know that this was what was going on
which is a good example of why letting people know what's going on is a
good idea.

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