| Message ID | 1487622809-25127-4-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Geert, On 20/02/17 20:33, Geert Uytterhoeven wrote: > Enable support for "shallow" suspend mode, also known as "Standby" or > "Power-On Suspend". > > As secondary CPU cores are taken offline, "shallow" suspend mode saves > slightly more power than "s2idle", but less than "deep" suspend mode. > However, unlike "deep" suspend mode, "shallow" suspend mode can be used > regardless of the presence of support for PSCI_SYSTEM_SUSPEND, which is > an optional API in PSCI v1.0. > > List the available system suspend modes: > > $ cat /sys/power/mem_sleep > s2idle shallow [deep] > > Suspend to "shallow" mode: > > $ echo shallow > /sys/power/mem_sleep > $ echo mem > /sys/power/state > I don't have the links to such previous attempts handy, but we have more elegant alternative options(suspend-to-idle) and any such attempts to hack around the PSCI will be NACKed.
Hi! > Enable support for "shallow" suspend mode, also known as "Standby" or > "Power-On Suspend". > > As secondary CPU cores are taken offline, "shallow" suspend mode saves > slightly more power than "s2idle", but less than "deep" suspend mode. > However, unlike "deep" suspend mode, "shallow" suspend mode can be used > regardless of the presence of support for PSCI_SYSTEM_SUSPEND, which is > an optional API in PSCI v1.0. If system supports "shallow" suspend, why does not PSCI implement it? In the past, I was told PSCI will not turn into ACPI-like mess, and that we'll be able to fix PSCI and will not have to work around its problems in kernel :-(. Not your fault, Mark made those promises. Pavel
On 21/02/17 11:07, Pavel Machek wrote: > Hi! > >> Enable support for "shallow" suspend mode, also known as "Standby" or >> "Power-On Suspend". >> >> As secondary CPU cores are taken offline, "shallow" suspend mode saves >> slightly more power than "s2idle", but less than "deep" suspend mode. >> However, unlike "deep" suspend mode, "shallow" suspend mode can be used >> regardless of the presence of support for PSCI_SYSTEM_SUSPEND, which is >> an optional API in PSCI v1.0. > > If system supports "shallow" suspend, why does not PSCI implement it? > Yes it can, and IIUC it already does on this platform with CPU_SUSPEND. All it now needs is just to use existing "freeze" suspend mode in Linux. > In the past, I was told PSCI will not turn into ACPI-like mess, and > that we'll be able to fix PSCI and will not have to work around its > problems in kernel :-(. Can you be more elaborate on the mess you see on this Renesas platform. For me, it looks like this patch is attempting to *re-implement* the existing "suspend-to-idle" functionality. So IMO, this patch set is creating unnecessary mess giving an illusion that PSCI specification is broken.
Hi Sudeep, On Tue, Feb 21, 2017 at 11:42 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 20/02/17 20:33, Geert Uytterhoeven wrote: >> Enable support for "shallow" suspend mode, also known as "Standby" or >> "Power-On Suspend". >> >> As secondary CPU cores are taken offline, "shallow" suspend mode saves >> slightly more power than "s2idle", but less than "deep" suspend mode. >> However, unlike "deep" suspend mode, "shallow" suspend mode can be used >> regardless of the presence of support for PSCI_SYSTEM_SUSPEND, which is >> an optional API in PSCI v1.0. >> >> List the available system suspend modes: >> >> $ cat /sys/power/mem_sleep >> s2idle shallow [deep] >> >> Suspend to "shallow" mode: >> >> $ echo shallow > /sys/power/mem_sleep >> $ echo mem > /sys/power/state >> > > I don't have the links to such previous attempts handy, but we have Don't worry, I did read earlier discussions about implementing shallow mode. > more elegant alternative options(suspend-to-idle) and any such attempts > to hack around the PSCI will be NACKed. "s2idle" does not power down secondary CPU cores, so this is an improvement. "deep" may not support configured wake-up sources, which is a bug. 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
Hi Sudeep, On Tue, Feb 21, 2017 at 12:14 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 21/02/17 11:07, Pavel Machek wrote: >>> Enable support for "shallow" suspend mode, also known as "Standby" or >>> "Power-On Suspend". >>> >>> As secondary CPU cores are taken offline, "shallow" suspend mode saves >>> slightly more power than "s2idle", but less than "deep" suspend mode. >>> However, unlike "deep" suspend mode, "shallow" suspend mode can be used >>> regardless of the presence of support for PSCI_SYSTEM_SUSPEND, which is >>> an optional API in PSCI v1.0. >> >> If system supports "shallow" suspend, why does not PSCI implement it? > > Yes it can, and IIUC it already does on this platform with CPU_SUSPEND. > All it now needs is just to use existing "freeze" suspend mode in Linux. How can Linux know if using "deep" suspend will allow to wake-up the system according to configured wake-up sources, or not? Note that "it will not, ever" is an accepted answer. 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 21/02/17 16:23, Geert Uytterhoeven wrote: > Hi Sudeep, > > On Tue, Feb 21, 2017 at 11:42 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> On 20/02/17 20:33, Geert Uytterhoeven wrote: >>> Enable support for "shallow" suspend mode, also known as "Standby" or >>> "Power-On Suspend". >>> >>> As secondary CPU cores are taken offline, "shallow" suspend mode saves >>> slightly more power than "s2idle", but less than "deep" suspend mode. >>> However, unlike "deep" suspend mode, "shallow" suspend mode can be used >>> regardless of the presence of support for PSCI_SYSTEM_SUSPEND, which is >>> an optional API in PSCI v1.0. >>> >>> List the available system suspend modes: >>> >>> $ cat /sys/power/mem_sleep >>> s2idle shallow [deep] >>> >>> Suspend to "shallow" mode: >>> >>> $ echo shallow > /sys/power/mem_sleep >>> $ echo mem > /sys/power/state >>> >> >> I don't have the links to such previous attempts handy, but we have > > Don't worry, I did read earlier discussions about implementing shallow mode. > In short or just to summarize in one line, just use "freeze"(a.k.a suspend-to-idle suspend mode)
Hi, On Tue, Feb 21, 2017 at 05:32:50PM +0100, Geert Uytterhoeven wrote: > On Tue, Feb 21, 2017 at 12:14 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On 21/02/17 11:07, Pavel Machek wrote: > >>> Enable support for "shallow" suspend mode, also known as "Standby" or > >>> "Power-On Suspend". > >>> > >>> As secondary CPU cores are taken offline, "shallow" suspend mode saves > >>> slightly more power than "s2idle", but less than "deep" suspend mode. > >>> However, unlike "deep" suspend mode, "shallow" suspend mode can be used > >>> regardless of the presence of support for PSCI_SYSTEM_SUSPEND, which is > >>> an optional API in PSCI v1.0. > >> > >> If system supports "shallow" suspend, why does not PSCI implement it? > > > > Yes it can, and IIUC it already does on this platform with CPU_SUSPEND. > > All it now needs is just to use existing "freeze" suspend mode in Linux. > > How can Linux know if using "deep" suspend will allow to wake-up the system > according to configured wake-up sources, or not? My understanding is that if a device can wake the system from PSCI_SYSTEM_SUSPEND, it should be described in the DT as a wakeup source [1]. So we should be able to determine the set of devices which can wake the system from a suspend. We shouldn't assume that other devices can (though I don't precisely what we do currently). Otherwise, where PSCI_CPU_SUSPEND, we'd expect that most devices (barring cpu-local timers) can wake up CPUs, and hence the system, by raising an interrupt. Thanks, Mark. [1] Documentation/devicetree/bindings/power/wakeup-source.txt
On 21/02/17 16:32, Geert Uytterhoeven wrote: > Hi Sudeep, > > On Tue, Feb 21, 2017 at 12:14 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> On 21/02/17 11:07, Pavel Machek wrote: >>>> Enable support for "shallow" suspend mode, also known as "Standby" or >>>> "Power-On Suspend". >>>> >>>> As secondary CPU cores are taken offline, "shallow" suspend mode saves >>>> slightly more power than "s2idle", but less than "deep" suspend mode. >>>> However, unlike "deep" suspend mode, "shallow" suspend mode can be used >>>> regardless of the presence of support for PSCI_SYSTEM_SUSPEND, which is >>>> an optional API in PSCI v1.0. >>> >>> If system supports "shallow" suspend, why does not PSCI implement it? >> >> Yes it can, and IIUC it already does on this platform with CPU_SUSPEND. >> All it now needs is just to use existing "freeze" suspend mode in Linux. > > How can Linux know if using "deep" suspend will allow to wake-up the system > according to configured wake-up sources, or not? > I am not sure if we have such selective configuration of wakeup source implemented in Linux. ACPI specification has some provisions where each device can state if it can specify device state in each system sleeping state that can wake the system. DT has no mechanism today to express this relations. I had brought up this discussion in plumbers(2015). Refer slide 7 in [0] And the way you are trying to do that is not correct IMO especially making it just PSCI specific. > Note that "it will not, ever" is an accepted answer. > IIUC, it's not implemented today. I can't talk about future ;), but your proposal is horrible hack.
Hi Mark, On Tue, Feb 21, 2017 at 6:20 PM, Mark Rutland <mark.rutland@arm.com> wrote: > On Tue, Feb 21, 2017 at 05:32:50PM +0100, Geert Uytterhoeven wrote: >> On Tue, Feb 21, 2017 at 12:14 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> > On 21/02/17 11:07, Pavel Machek wrote: >> >>> Enable support for "shallow" suspend mode, also known as "Standby" or >> >>> "Power-On Suspend". >> >>> >> >>> As secondary CPU cores are taken offline, "shallow" suspend mode saves >> >>> slightly more power than "s2idle", but less than "deep" suspend mode. >> >>> However, unlike "deep" suspend mode, "shallow" suspend mode can be used >> >>> regardless of the presence of support for PSCI_SYSTEM_SUSPEND, which is >> >>> an optional API in PSCI v1.0. >> >> >> >> If system supports "shallow" suspend, why does not PSCI implement it? >> > >> > Yes it can, and IIUC it already does on this platform with CPU_SUSPEND. >> > All it now needs is just to use existing "freeze" suspend mode in Linux. >> >> How can Linux know if using "deep" suspend will allow to wake-up the system >> according to configured wake-up sources, or not? > > My understanding is that if a device can wake the system from > PSCI_SYSTEM_SUSPEND, it should be described in the DT as a wakeup source > [1]. So we should be able to determine the set of devices which can wake > the system from a suspend. We shouldn't assume that other devices can > (though I don't precisely what we do currently). > > Otherwise, where PSCI_CPU_SUSPEND, we'd expect that most devices > (barring cpu-local timers) can wake up CPUs, and hence the system, by > raising an interrupt. > [1] Documentation/devicetree/bindings/power/wakeup-source.txt "wakeup-source" in DT is used as a mix of hardware description and software policy. E.g. some keys on a keyboard may have it, others don't, while there's not always a technical reason for that. Also, it doesn't specify from which suspend state it can wake-up. On top of that, the Linux PM subsystem allows to configure wakeup by writing "enabled" to a device's "wakeup" file in sysfs. Or you can use ethtool for Wake-on-LAN. 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
Hi, On Tue, Feb 21, 2017 at 07:06:04PM +0100, Geert Uytterhoeven wrote: > On Tue, Feb 21, 2017 at 6:20 PM, Mark Rutland <mark.rutland@arm.com> wrote: > > On Tue, Feb 21, 2017 at 05:32:50PM +0100, Geert Uytterhoeven wrote: > >> How can Linux know if using "deep" suspend will allow to wake-up the system > >> according to configured wake-up sources, or not? > > > > My understanding is that if a device can wake the system from > > PSCI_SYSTEM_SUSPEND, it should be described in the DT as a wakeup source > > [1]. So we should be able to determine the set of devices which can wake > > the system from a suspend. We shouldn't assume that other devices can > > (though I don't precisely what we do currently). > > > > Otherwise, where PSCI_CPU_SUSPEND, we'd expect that most devices > > (barring cpu-local timers) can wake up CPUs, and hence the system, by > > raising an interrupt. > > > [1] Documentation/devicetree/bindings/power/wakeup-source.txt > > "wakeup-source" in DT is used as a mix of hardware description and software > policy. E.g. some keys on a keyboard may have it, others don't, while there's > not always a technical reason for that. > > Also, it doesn't specify from which suspend state it can wake-up. Joy. If we need to do something here, we should clarify the semantics of wakeup-source and/or introduce a property which is explicitly for the purpose of expressing HW capability to wake up from a specific power state. > On top of that, the Linux PM subsystem allows to configure wakeup by writing > "enabled" to a device's "wakeup" file in sysfs. Or you can use ethtool for > Wake-on-LAN. Sure; userspace can always do something silly here. As I mentioned in my other reply, we could/should add an interface to allow userspace to determine if it has a guaranteed wakeup, which would allow us to do the right thing. Thanks, Mark.
Hi Mark, On Tue, Feb 21, 2017 at 7:18 PM, Mark Rutland <mark.rutland@arm.com> wrote: >> On top of that, the Linux PM subsystem allows to configure wakeup by writing >> "enabled" to a device's "wakeup" file in sysfs. Or you can use ethtool for >> Wake-on-LAN. > > Sure; userspace can always do something silly here. Not that silly: you can wake up using these sources, but not necessarily from all states. > As I mentioned in my other reply, we could/should add an interface to > allow userspace to determine if it has a guaranteed wakeup, which would > allow us to do the right thing. Right. 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
Hi Sudeep, On Tue, Feb 21, 2017 at 6:22 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 21/02/17 16:32, Geert Uytterhoeven wrote: >> On Tue, Feb 21, 2017 at 12:14 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> On 21/02/17 11:07, Pavel Machek wrote: >>>>> Enable support for "shallow" suspend mode, also known as "Standby" or >>>>> "Power-On Suspend". >>>>> >>>>> As secondary CPU cores are taken offline, "shallow" suspend mode saves >>>>> slightly more power than "s2idle", but less than "deep" suspend mode. >>>>> However, unlike "deep" suspend mode, "shallow" suspend mode can be used >>>>> regardless of the presence of support for PSCI_SYSTEM_SUSPEND, which is >>>>> an optional API in PSCI v1.0. >>>> >>>> If system supports "shallow" suspend, why does not PSCI implement it? >>> >>> Yes it can, and IIUC it already does on this platform with CPU_SUSPEND. >>> All it now needs is just to use existing "freeze" suspend mode in Linux. >> >> How can Linux know if using "deep" suspend will allow to wake-up the system >> according to configured wake-up sources, or not? > > I am not sure if we have such selective configuration of wakeup source > implemented in Linux. > > ACPI specification has some provisions where each device can state if it > can specify device state in each system sleeping state that can wake the > system. > > DT has no mechanism today to express this relations. I had brought up > this discussion in plumbers(2015). Refer slide 7 in [0] > > And the way you are trying to do that is not correct IMO especially > making it just PSCI specific. > >> Note that "it will not, ever" is an accepted answer. > > IIUC, it's not implemented today. I can't talk about future ;), but your Good, so there's no need for the DT property, and drivers/firmware/psci.c should aways call do_cpu_idle() instead of PSCI SYSTEM_SUSPEND if any other wake-up sources are configured? That follows the principle of least surprise: it doesn't leave the user with a system that won't wake up the way he configured it to wake up. > proposal is horrible hack. 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 22/02/17 13:47, Geert Uytterhoeven wrote: > Hi Sudeep, > > On Tue, Feb 21, 2017 at 6:22 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: [...] >> >> IIUC, it's not implemented today. I can't talk about future ;), but your > > Good, so there's no need for the DT property, and drivers/firmware/psci.c > should aways call do_cpu_idle() instead of PSCI SYSTEM_SUSPEND if any > other wake-up sources are configured? > No. > That follows the principle of least surprise: it doesn't leave the user with > a system that won't wake up the way he configured it to wake up. > But he can still wake up with the "switch" so there's no surprise. He just need to better understand his system before playing with it ;)
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c index 493a56a4cfc4a836..13b4d50bb3577384 100644 --- a/drivers/firmware/psci.c +++ b/drivers/firmware/psci.c @@ -85,6 +85,7 @@ static u32 psci_function_id[PSCI_FN_MAX]; PSCI_1_0_EXT_POWER_STATE_TYPE_MASK) static u32 psci_cpu_suspend_feature; +static bool psci_suspend_mem_supported; static inline bool psci_has_ext_power_state(void) { @@ -422,13 +423,36 @@ static int psci_system_suspend(unsigned long unused) __pa_symbol(cpu_resume), 0, 0); } +static int psci_system_suspend_valid(suspend_state_t state) +{ + switch (state) { + case PM_SUSPEND_STANDBY: + return true; + + case PM_SUSPEND_MEM: + return psci_suspend_mem_supported; + + default: + return false; + } +} + static int psci_system_suspend_enter(suspend_state_t state) { - return cpu_suspend(0, psci_system_suspend); + switch (state) { + case PM_SUSPEND_STANDBY: + cpu_do_idle(); + break; + + case PM_SUSPEND_MEM: + return cpu_suspend(0, psci_system_suspend); + } + + return 0; } static const struct platform_suspend_ops psci_suspend_ops = { - .valid = suspend_valid_only_mem, + .valid = psci_system_suspend_valid, .enter = psci_system_suspend_enter, }; @@ -442,7 +466,9 @@ static void __init psci_init_system_suspend(void) ret = psci_features(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND)); if (ret != PSCI_RET_NOT_SUPPORTED) - suspend_set_ops(&psci_suspend_ops); + psci_suspend_mem_supported = true; + + suspend_set_ops(&psci_suspend_ops); } static void __init psci_init_cpu_suspend(void)
Enable support for "shallow" suspend mode, also known as "Standby" or "Power-On Suspend". As secondary CPU cores are taken offline, "shallow" suspend mode saves slightly more power than "s2idle", but less than "deep" suspend mode. However, unlike "deep" suspend mode, "shallow" suspend mode can be used regardless of the presence of support for PSCI_SYSTEM_SUSPEND, which is an optional API in PSCI v1.0. List the available system suspend modes: $ cat /sys/power/mem_sleep s2idle shallow [deep] Suspend to "shallow" mode: $ echo shallow > /sys/power/mem_sleep $ echo mem > /sys/power/state Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/firmware/psci.c | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)