[PATCH/RFC,3/6] drivers: firmware: psci: Implement shallow suspend mode
diff mbox

Message ID 1487622809-25127-4-git-send-email-geert+renesas@glider.be
State Under Review
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Geert Uytterhoeven Feb. 20, 2017, 8:33 p.m. UTC
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(-)

Comments

Sudeep Holla Feb. 21, 2017, 10:42 a.m. UTC | #1
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.
Pavel Machek Feb. 21, 2017, 11:07 a.m. UTC | #2
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
Sudeep Holla Feb. 21, 2017, 11:14 a.m. UTC | #3
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.
Geert Uytterhoeven Feb. 21, 2017, 4:23 p.m. UTC | #4
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
Geert Uytterhoeven Feb. 21, 2017, 4:32 p.m. UTC | #5
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
Sudeep Holla Feb. 21, 2017, 4:51 p.m. UTC | #6
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)
Mark Rutland Feb. 21, 2017, 5:20 p.m. UTC | #7
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
Sudeep Holla Feb. 21, 2017, 5:22 p.m. UTC | #8
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.
Geert Uytterhoeven Feb. 21, 2017, 6:06 p.m. UTC | #9
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
Mark Rutland Feb. 21, 2017, 6:18 p.m. UTC | #10
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.
Geert Uytterhoeven Feb. 21, 2017, 6:23 p.m. UTC | #11
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
Geert Uytterhoeven Feb. 22, 2017, 1:47 p.m. UTC | #12
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
Sudeep Holla Feb. 22, 2017, 2:35 p.m. UTC | #13
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 ;)

Patch
diff mbox

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)