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 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.
Changes compared to v1:
- Improve patch descriptions,
- Drop "return;" at end of function.
This has been tested on M3ULCB (thanks Jacopo!), and on Salvator-X(S).
For testing, this series is also available in the
topic/bd9571-toggle-power-switch-v2 branch of my renesas-drivers git
repository at
git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git.
Thanks!
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 | 23 +++++++++++++++++++++++
include/linux/pm.h | 1 +
3 files changed, 28 insertions(+)
--
2.17.1
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
Add a callback to inform a device that its 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/resume procedure is:
1. Configure PMIC for DDR backup mode (by software), which changes the
role of the accessory power switch from a power to a wake-up
switch,
2. Switch accessory power switch off (manually), to prepare for system
suspend,
3. Suspend system (by software),
4. Switch accessory power switch on (manually), to wake up the system.
As step 2 involves a manual operation, step 1 cannot be combined
with step 3 and performed in the PMIC's suspend callback (unlike on
systems with a momentary power switch).
Adding the new callback allows to move step 1 to the new callback, to be
performed 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?
Currently step 1 is done in userspace using i2set, which is very
user-unfriendly
(https://elinux.org/R-Car/Boards/Salvator-XS#PSCI_System_Suspend).
v2:
- Improve patch description.
---
drivers/base/power/wakeup.c | 4 ++++
include/linux/pm.h | 1 +
2 files changed, 5 insertions(+)
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 "enabled" is written
to the PMIC's "wakeup" virtual file in sysfs, using the newly introduced
callback for wake-up change notification. Conversely, writing
"disabled" 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>
---
v2:
- Improve patch description,
- Drop "return;" at end of function.
---
drivers/regulator/bd9571mwv-regulator.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2018-06-19 14:47:58
On Tue, Jun 19, 2018 at 3:55 PM, Geert Uytterhoeven
[off-list ref] wrote:
Add a callback to inform a device that its 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/resume procedure is:
1. Configure PMIC for DDR backup mode (by software), which changes the
role of the accessory power switch from a power to a wake-up
switch,
2. Switch accessory power switch off (manually), to prepare for system
suspend,
3. Suspend system (by software),
4. Switch accessory power switch on (manually), to wake up the system.
As step 2 involves a manual operation, step 1 cannot be combined
with step 3 and performed in the PMIC's suspend callback (unlike on
systems with a momentary power switch).
Adding the new callback allows to move step 1 to the new callback, to be
performed in response to the user writing "enabled" to the PMIC's
"wakeup" virtual file in sysfs.
I still don't quite understand this TBH.
In particular, why do you want a write to "wakeup" trigger this
instead of having a special sysfs attr for that exposed by your PMIC
driver?
Writing "enabled" to "wakeup" for the PMIC should enable the PMIC
itself to wake up the system, which isn't quite the case, or is it?
Thanks,
Rafael
Hi Rafael,
On Tue, Jun 19, 2018 at 4:48 PM Rafael J. Wysocki [off-list ref] wrote:
On Tue, Jun 19, 2018 at 3:55 PM, Geert Uytterhoeven
[off-list ref] wrote:
quoted
Add a callback to inform a device that its 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/resume procedure is:
1. Configure PMIC for DDR backup mode (by software), which changes the
role of the accessory power switch from a power to a wake-up
switch,
2. Switch accessory power switch off (manually), to prepare for system
suspend,
3. Suspend system (by software),
4. Switch accessory power switch on (manually), to wake up the system.
As step 2 involves a manual operation, step 1 cannot be combined
with step 3 and performed in the PMIC's suspend callback (unlike on
systems with a momentary power switch).
Adding the new callback allows to move step 1 to the new callback, to be
performed in response to the user writing "enabled" to the PMIC's
"wakeup" virtual file in sysfs.
I still don't quite understand this TBH.
In particular, why do you want a write to "wakeup" trigger this
instead of having a special sysfs attr for that exposed by your PMIC
driver?
Writing "enabled" to "wakeup" for the PMIC should enable the PMIC
itself to wake up the system, which isn't quite the case, or is it?
Actually the PMIC cannot wake up the system if backup mode is not enabled.
When suspending the system (PSCI suspend) without enabling backup mode
first, the system will just crash (which cannot be distinguished from being
suspended, as PSCI doesn't support any other wake-up sources anyway[*] ;-)
So in that sense writing "enabled" to the "wakeup" file does enable the
PMIC to wake up the system.
Do you still prefer a driver-specific sysfs file?
Thanks!
[*] Perhaps you remember my ill-received attempt to prevent the system from
using PSCI system suspend when any other wake-up sources were enabled?
("[PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts
power", https://lkml.org/lkml/2017/2/20/566).
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
From: Rafael J. Wysocki <hidden> Date: 2018-06-20 08:30:56
On Tuesday, June 19, 2018 5:22:06 PM CEST Geert Uytterhoeven wrote:
Hi Rafael,
On Tue, Jun 19, 2018 at 4:48 PM Rafael J. Wysocki [off-list ref] wrote:
quoted
On Tue, Jun 19, 2018 at 3:55 PM, Geert Uytterhoeven
[off-list ref] wrote:
quoted
Add a callback to inform a device that its 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/resume procedure is:
1. Configure PMIC for DDR backup mode (by software), which changes the
role of the accessory power switch from a power to a wake-up
switch,
2. Switch accessory power switch off (manually), to prepare for system
suspend,
3. Suspend system (by software),
4. Switch accessory power switch on (manually), to wake up the system.
As step 2 involves a manual operation, step 1 cannot be combined
with step 3 and performed in the PMIC's suspend callback (unlike on
systems with a momentary power switch).
Adding the new callback allows to move step 1 to the new callback, to be
performed in response to the user writing "enabled" to the PMIC's
"wakeup" virtual file in sysfs.
I still don't quite understand this TBH.
In particular, why do you want a write to "wakeup" trigger this
instead of having a special sysfs attr for that exposed by your PMIC
driver?
Well, I'm not convinced that this is the right approach, though.
quoted
Writing "enabled" to "wakeup" for the PMIC should enable the PMIC
itself to wake up the system, which isn't quite the case, or is it?
Actually the PMIC cannot wake up the system if backup mode is not enabled.
That doesn't really matter. "enabled" in "wakeup" only means that user space
allows the device to wake up the system from sleep, but it still may not be
able to do that for certain reasons (like in this case). By adding this
callback you're changing the meaning of the attribute.
When suspending the system (PSCI suspend) without enabling backup mode
first, the system will just crash (which cannot be distinguished from being
suspended, as PSCI doesn't support any other wake-up sources anyway[*] ;-)
So in that sense writing "enabled" to the "wakeup" file does enable the
PMIC to wake up the system.
Do you still prefer a driver-specific sysfs file?
Well, I'm not convinced that this is the right approach, though.
The original description of the changes made it sound like this
controlled if the power switch would make the device resume from suspend
or not which seems like a wakeup to me. Honestly as things are I've no
idea what the hardware designers were thinking or how to explain what
this stuff is doing.
quoted
Do you still prefer a driver-specific sysfs file?
Yes, I do.
The flip side of that is that either suspend and resume or poweroff are
broken for userspace unless they know about this magic sysfs file which
isn't great either.
Well, I'm not convinced that this is the right approach, though.
The original description of the changes made it sound like this
controlled if the power switch would make the device resume from suspend
or not which seems like a wakeup to me. Honestly as things are I've no
idea what the hardware designers were thinking or how to explain what
this stuff is doing.
quoted
quoted
Do you still prefer a driver-specific sysfs file?
quoted
Yes, I do.
The flip side of that is that either suspend and resume or poweroff are
broken for userspace unless they know about this magic sysfs file which
isn't great either.
But to me that isn't that much different from an RTC wake alarm, say.
Enabling it to wake up the system in general isn't sufficient, you
also need to actually set the alarm using a different interface.
From: Mark Brown <broonie@kernel.org> Date: 2018-06-20 13:25:26
On Wed, Jun 20, 2018 at 02:15:38PM +0200, Rafael J. Wysocki wrote:
On Wed, Jun 20, 2018 at 12:35 PM, Mark Brown [off-list ref] wrote:
quoted
The flip side of that is that either suspend and resume or poweroff are
broken for userspace unless they know about this magic sysfs file which
isn't great either.
But to me that isn't that much different from an RTC wake alarm, say.
Enabling it to wake up the system in general isn't sufficient, you
also need to actually set the alarm using a different interface.
It seems more like hardware breakage we're trying to fix than a feature
- it's not like it's adding something we didn't have already (like
setting a time in an alarm where the alarm is an additional thing), more
just trying to execute on an existing user interface successfully. I
can see that there's a case that it doesn't map very well onto the
standard interfaces so perhaps we have to add something on the side as
the hardware is just too horrible to fit in with the standard interfaces
and we have to do that.
On Wed, Jun 20, 2018 at 3:25 PM Mark Brown [off-list ref] wrote:
On Wed, Jun 20, 2018 at 02:15:38PM +0200, Rafael J. Wysocki wrote:
quoted
On Wed, Jun 20, 2018 at 12:35 PM, Mark Brown [off-list ref] wrote:
quoted
quoted
The flip side of that is that either suspend and resume or poweroff are
broken for userspace unless they know about this magic sysfs file which
isn't great either.
quoted
But to me that isn't that much different from an RTC wake alarm, say.
quoted
Enabling it to wake up the system in general isn't sufficient, you
also need to actually set the alarm using a different interface.
The RTC wake alarm time is indeed different, as it is not a simple boolean flag.
It is also more natural for the user, who expects to need to find some way to
configure the wake-up time.
It seems more like hardware breakage we're trying to fix than a feature
- it's not like it's adding something we didn't have already (like
setting a time in an alarm where the alarm is an additional thing), more
just trying to execute on an existing user interface successfully. I
can see that there's a case that it doesn't map very well onto the
standard interfaces so perhaps we have to add something on the side as
the hardware is just too horrible to fit in with the standard interfaces
and we have to do that.
My main worry is usability: with a separate sysfs file, we need to document the
file, and the user needs to be aware of it.
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
From: Rafael J. Wysocki <hidden> Date: 2018-06-26 10:17:36
On Tuesday, June 26, 2018 12:06:16 PM CEST Geert Uytterhoeven wrote:
On Wed, Jun 20, 2018 at 3:25 PM Mark Brown [off-list ref] wrote:
quoted
On Wed, Jun 20, 2018 at 02:15:38PM +0200, Rafael J. Wysocki wrote:
quoted
On Wed, Jun 20, 2018 at 12:35 PM, Mark Brown [off-list ref] wrote:
quoted
quoted
The flip side of that is that either suspend and resume or poweroff are
broken for userspace unless they know about this magic sysfs file which
isn't great either.
quoted
But to me that isn't that much different from an RTC wake alarm, say.
quoted
Enabling it to wake up the system in general isn't sufficient, you
also need to actually set the alarm using a different interface.
The RTC wake alarm time is indeed different, as it is not a simple boolean flag.
It is also more natural for the user, who expects to need to find some way to
configure the wake-up time.
OK, take Ethernet. You need to configure WoL on that to wake up the system
in addition to setting power/wakeup for it.
Take WiFi: You need to set up WoW on that.
And so on.
quoted
It seems more like hardware breakage we're trying to fix than a feature
- it's not like it's adding something we didn't have already (like
setting a time in an alarm where the alarm is an additional thing), more
just trying to execute on an existing user interface successfully. I
can see that there's a case that it doesn't map very well onto the
standard interfaces so perhaps we have to add something on the side as
the hardware is just too horrible to fit in with the standard interfaces
and we have to do that.
My main worry is usability: with a separate sysfs file, we need to document the
file, and the user needs to be aware of it.
That's right, but it will be very hard to convince me that changing the
meaning of the "wakeup" attribute just in order to work around this issue
(which arguably is a consequence of "unfortunate" hardware design) is a
good idea. :-)
Hi Rafael,
On Tue, Jun 26, 2018 at 12:17 PM Rafael J. Wysocki [off-list ref] wrote:
On Tuesday, June 26, 2018 12:06:16 PM CEST Geert Uytterhoeven wrote:
quoted
On Wed, Jun 20, 2018 at 3:25 PM Mark Brown [off-list ref] wrote:
quoted
On Wed, Jun 20, 2018 at 02:15:38PM +0200, Rafael J. Wysocki wrote:
quoted
On Wed, Jun 20, 2018 at 12:35 PM, Mark Brown [off-list ref] wrote:
quoted
quoted
The flip side of that is that either suspend and resume or poweroff are
broken for userspace unless they know about this magic sysfs file which
isn't great either.
quoted
But to me that isn't that much different from an RTC wake alarm, say.
quoted
Enabling it to wake up the system in general isn't sufficient, you
also need to actually set the alarm using a different interface.
The RTC wake alarm time is indeed different, as it is not a simple boolean flag.
It is also more natural for the user, who expects to need to find some way to
configure the wake-up time.
OK, take Ethernet. You need to configure WoL on that to wake up the system
in addition to setting power/wakeup for it.
Take WiFi: You need to set up WoW on that.
And so on.
I always found it strange that you have both "ethtool wol" and and a
"wakeup" file
in sysfs (does "ethtool wol" predate the wakeup file in sysfs?)
I believe originally WoL supported MagicPacket only (many systems still
support only that), so originally it was boolean.
quoted
quoted
It seems more like hardware breakage we're trying to fix than a feature
- it's not like it's adding something we didn't have already (like
setting a time in an alarm where the alarm is an additional thing), more
just trying to execute on an existing user interface successfully. I
can see that there's a case that it doesn't map very well onto the
standard interfaces so perhaps we have to add something on the side as
the hardware is just too horrible to fit in with the standard interfaces
and we have to do that.
My main worry is usability: with a separate sysfs file, we need to document the
file, and the user needs to be aware of it.
That's right, but it will be very hard to convince me that changing the
meaning of the "wakeup" attribute just in order to work around this issue
(which arguably is a consequence of "unfortunate" hardware design) is a
good idea. :-)
OK.
Next question: where to document device-specific sysfs files for regulators?
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
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2018-06-26 13:56:06
Hi Geert,
On Tue, Jun 26, 2018 at 12:29 PM, Geert Uytterhoeven
[off-list ref] wrote:
Hi Rafael,
On Tue, Jun 26, 2018 at 12:17 PM Rafael J. Wysocki [off-list ref] wrote:
quoted
On Tuesday, June 26, 2018 12:06:16 PM CEST Geert Uytterhoeven wrote:
quoted
On Wed, Jun 20, 2018 at 3:25 PM Mark Brown [off-list ref] wrote:
quoted
On Wed, Jun 20, 2018 at 02:15:38PM +0200, Rafael J. Wysocki wrote:
quoted
On Wed, Jun 20, 2018 at 12:35 PM, Mark Brown [off-list ref] wrote:
quoted
quoted
The flip side of that is that either suspend and resume or poweroff are
broken for userspace unless they know about this magic sysfs file which
isn't great either.
quoted
But to me that isn't that much different from an RTC wake alarm, say.
quoted
Enabling it to wake up the system in general isn't sufficient, you
also need to actually set the alarm using a different interface.
The RTC wake alarm time is indeed different, as it is not a simple boolean flag.
It is also more natural for the user, who expects to need to find some way to
configure the wake-up time.
OK, take Ethernet. You need to configure WoL on that to wake up the system
in addition to setting power/wakeup for it.
Take WiFi: You need to set up WoW on that.
And so on.
I always found it strange that you have both "ethtool wol" and and a
"wakeup" file
in sysfs (does "ethtool wol" predate the wakeup file in sysfs?)
Yes, it does.
I believe originally WoL supported MagicPacket only (many systems still
support only that), so originally it was boolean.
I don't recall the details here. When looked at it first time
multiple options had been there already.
quoted
quoted
quoted
It seems more like hardware breakage we're trying to fix than a feature
- it's not like it's adding something we didn't have already (like
setting a time in an alarm where the alarm is an additional thing), more
just trying to execute on an existing user interface successfully. I
can see that there's a case that it doesn't map very well onto the
standard interfaces so perhaps we have to add something on the side as
the hardware is just too horrible to fit in with the standard interfaces
and we have to do that.
My main worry is usability: with a separate sysfs file, we need to document the
file, and the user needs to be aware of it.
That's right, but it will be very hard to convince me that changing the
meaning of the "wakeup" attribute just in order to work around this issue
(which arguably is a consequence of "unfortunate" hardware design) is a
good idea. :-)
OK.
Next question: where to document device-specific sysfs files for regulators?