| Message ID | 20180619135518.8990-2-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Tue, Jun 19, 2018 at 3:55 PM, Geert Uytterhoeven <geert+renesas@glider.be> 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 <rafael@kernel.org> wrote: > On Tue, Jun 19, 2018 at 3:55 PM, Geert Uytterhoeven > <geert+renesas@glider.be> 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? In v1 (https://patchwork.kernel.org/patch/9996567/), I had a driver-specific "backup_mode" sysfs file. In v2 and later of the driver series (https://lkml.org/lkml/2018/4/18/345), I changed that to use the standard "wakeup" file in sysfs, in response to a comment from Mark Brown. > 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
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 <rafael@kernel.org> wrote: > > On Tue, Jun 19, 2018 at 3:55 PM, Geert Uytterhoeven > > <geert+renesas@glider.be> 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? > > In v1 (https://patchwork.kernel.org/patch/9996567/), I had a > driver-specific "backup_mode" sysfs file. > In v2 and later of the driver series (https://lkml.org/lkml/2018/4/18/345), > I changed that to use the standard "wakeup" file in sysfs, in response to > a comment from Mark Brown. Well, I'm not convinced that this is the right approach, though. > > 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? Yes, I do. Thanks, Rafael
On Wed, Jun 20, 2018 at 10:02:41AM +0200, Rafael J. Wysocki wrote: > On Tuesday, June 19, 2018 5:22:06 PM CEST Geert Uytterhoeven wrote: > > On Tue, Jun 19, 2018 at 4:48 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > On Tue, Jun 19, 2018 at 3:55 PM, Geert Uytterhoeven > > In v1 (https://patchwork.kernel.org/patch/9996567/), I had a > > driver-specific "backup_mode" sysfs file. > > In v2 and later of the driver series (https://lkml.org/lkml/2018/4/18/345), > > I changed that to use the standard "wakeup" file in sysfs, in response to > > a comment from Mark Brown. > 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. > > 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.
On Wed, Jun 20, 2018 at 12:35 PM, Mark Brown <broonie@kernel.org> wrote: > On Wed, Jun 20, 2018 at 10:02:41AM +0200, Rafael J. Wysocki wrote: >> On Tuesday, June 19, 2018 5:22:06 PM CEST Geert Uytterhoeven wrote: >> > On Tue, Jun 19, 2018 at 4:48 PM Rafael J. Wysocki <rafael@kernel.org> wrote: >> > > On Tue, Jun 19, 2018 at 3:55 PM, Geert Uytterhoeven > >> > In v1 (https://patchwork.kernel.org/patch/9996567/), I had a >> > driver-specific "backup_mode" sysfs file. >> > In v2 and later of the driver series (https://lkml.org/lkml/2018/4/18/345), >> > I changed that to use the standard "wakeup" file in sysfs, in response to >> > a comment from Mark Brown. > >> 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. > >> > 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. 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.
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 <broonie@kernel.org> wrote: > > 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 <broonie@kernel.org> wrote: > 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 <broonie@kernel.org> wrote: > > > > 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. 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
On Tuesday, June 26, 2018 12:06:16 PM CEST Geert Uytterhoeven wrote: > On Wed, Jun 20, 2018 at 3:25 PM Mark Brown <broonie@kernel.org> wrote: > > 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 <broonie@kernel.org> wrote: > > > > > > 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. > > 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. > > 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 <rjw@rjwysocki.net> wrote: > On Tuesday, June 26, 2018 12:06:16 PM CEST Geert Uytterhoeven wrote: > > On Wed, Jun 20, 2018 at 3:25 PM Mark Brown <broonie@kernel.org> wrote: > > > 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 <broonie@kernel.org> wrote: > > > > > > > > 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. > > > > 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. > > > 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
Hi Geert, On Tue, Jun 26, 2018 at 12:29 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Rafael, > > On Tue, Jun 26, 2018 at 12:17 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> On Tuesday, June 26, 2018 12:06:16 PM CEST Geert Uytterhoeven wrote: >> > On Wed, Jun 20, 2018 at 3:25 PM Mark Brown <broonie@kernel.org> wrote: >> > > 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 <broonie@kernel.org> wrote: >> > > >> > > > > 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. >> > >> > 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. >> > > 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? Under Documentation/ABI/ I suppose.
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c index 5fa1898755a34878..933560d658692fe3 100644 --- a/drivers/base/power/wakeup.c +++ b/drivers/base/power/wakeup.c @@ -255,6 +255,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; } @@ -372,6 +374,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 */
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(+)