[PATCH/RFC,4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power
diff mbox

Message ID 1487622809-25127-5-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
Nothing in the PSCI specification requires the SoC to remain powered and
to support wake-up sources when suspended using SYSTEM_SUSPEND.
If the firmware implements the PSCI SYSTEM_SUSPEND operation by cutting
power to the SoC, the only possibly wake-up sources are thus the ones
connected to the PMIC.

Document and add support for an "arm,psci-system-suspend-is-power-down"
DT property, so Linux uses a different suspend method when other wake-up
sources (e.g. wake on LAN, UART or GPIO) are enabled.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/devicetree/bindings/arm/psci.txt | 11 +++++++++++
 drivers/firmware/psci.c                        | 13 ++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

Comments

Sudeep Holla Feb. 21, 2017, 10:50 a.m. UTC | #1
On 20/02/17 20:33, Geert Uytterhoeven wrote:
> Nothing in the PSCI specification requires the SoC to remain powered and
> to support wake-up sources when suspended using SYSTEM_SUSPEND.
> If the firmware implements the PSCI SYSTEM_SUSPEND operation by cutting
> power to the SoC, the only possibly wake-up sources are thus the ones
> connected to the PMIC.
> 
> Document and add support for an "arm,psci-system-suspend-is-power-down"
> DT property, so Linux uses a different suspend method when other wake-up
> sources (e.g. wake on LAN, UART or GPIO) are enabled.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  Documentation/devicetree/bindings/arm/psci.txt | 11 +++++++++++
>  drivers/firmware/psci.c                        | 13 ++++++++++---
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
> index a2c4f1d524929bb7..16e390ecb7531028 100644
> --- a/Documentation/devicetree/bindings/arm/psci.txt
> +++ b/Documentation/devicetree/bindings/arm/psci.txt
> @@ -68,6 +68,17 @@ state nodes, as per bindings in [1]) must specify the following properties:
>  		Definition: power_state parameter to pass to the PSCI
>  			    suspend call.
>  
> + - arm,psci-system-suspend-is-power-down
> +		Nothing in the PSCI specification requires the SoC to remain
> +		powered and to support wake-up sources when suspended using
> +		SYSTEM_SUSPEND.

Again, yes SoC can be powered down but you give no reasons why this is
useful other than help you to hack around to implement suspend_ops. As
suggested please try using freeze_ops.

After commit a94e502c22b6 ("cpuidle: dt: assign ->enter_freeze to same
as ->enter callback function"), you can enter suspend-to-idle(a.k.a
freeze state) on all platforms using ARM DT cpuidle driver.
Pavel Machek Feb. 21, 2017, 11:07 a.m. UTC | #2
On Mon 2017-02-20 21:33:27, Geert Uytterhoeven wrote:
> Nothing in the PSCI specification requires the SoC to remain powered and
> to support wake-up sources when suspended using SYSTEM_SUSPEND.
> If the firmware implements the PSCI SYSTEM_SUSPEND operation by cutting
> power to the SoC, the only possibly wake-up sources are thus the ones
> connected to the PMIC.
> 
> Document and add support for an "arm,psci-system-suspend-is-power-down"
> DT property, so Linux uses a different suspend method when other wake-up
> sources (e.g. wake on LAN, UART or GPIO) are enabled.

Should we make PSCI return that information? (At least in next
specification version?)

								Pavel
Geert Uytterhoeven Feb. 21, 2017, 4:36 p.m. UTC | #3
Hi Sudeep,

On Tue, Feb 21, 2017 at 11:50 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On 20/02/17 20:33, Geert Uytterhoeven wrote:
>> Nothing in the PSCI specification requires the SoC to remain powered and
>> to support wake-up sources when suspended using SYSTEM_SUSPEND.
>> If the firmware implements the PSCI SYSTEM_SUSPEND operation by cutting
>> power to the SoC, the only possibly wake-up sources are thus the ones
>> connected to the PMIC.
>>
>> Document and add support for an "arm,psci-system-suspend-is-power-down"
>> DT property, so Linux uses a different suspend method when other wake-up
>> sources (e.g. wake on LAN, UART or GPIO) are enabled.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>>  Documentation/devicetree/bindings/arm/psci.txt | 11 +++++++++++
>>  drivers/firmware/psci.c                        | 13 ++++++++++---
>>  2 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
>> index a2c4f1d524929bb7..16e390ecb7531028 100644
>> --- a/Documentation/devicetree/bindings/arm/psci.txt
>> +++ b/Documentation/devicetree/bindings/arm/psci.txt
>> @@ -68,6 +68,17 @@ state nodes, as per bindings in [1]) must specify the following properties:
>>               Definition: power_state parameter to pass to the PSCI
>>                           suspend call.
>>
>> + - arm,psci-system-suspend-is-power-down
>> +             Nothing in the PSCI specification requires the SoC to remain
>> +             powered and to support wake-up sources when suspended using
>> +             SYSTEM_SUSPEND.
>
> Again, yes SoC can be powered down but you give no reasons why this is
> useful other than help you to hack around to implement suspend_ops. As

This is useful to support other wake-up sources. Linux has a standardized
way to handle wake-up sources, which may be circumvented by calling PSCI
SYSTEM_SUSPEND.

> suggested please try using freeze_ops.

Freezing the system works, but requires manual configurarion.
This should be done automatically.

> After commit a94e502c22b6 ("cpuidle: dt: assign ->enter_freeze to same
> as ->enter callback function"), you can enter suspend-to-idle(a.k.a
> freeze state) on all platforms using ARM DT cpuidle driver.

This should be used automatically if the system can't wake-up from
suspend-to-RAM using the configured wake-up sources.

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:36 p.m. UTC | #4
Hi Pavel,

On Tue, Feb 21, 2017 at 12:07 PM, Pavel Machek <pavel@ucw.cz> wrote:
> On Mon 2017-02-20 21:33:27, Geert Uytterhoeven wrote:
>> Nothing in the PSCI specification requires the SoC to remain powered and
>> to support wake-up sources when suspended using SYSTEM_SUSPEND.
>> If the firmware implements the PSCI SYSTEM_SUSPEND operation by cutting
>> power to the SoC, the only possibly wake-up sources are thus the ones
>> connected to the PMIC.
>>
>> Document and add support for an "arm,psci-system-suspend-is-power-down"
>> DT property, so Linux uses a different suspend method when other wake-up
>> sources (e.g. wake on LAN, UART or GPIO) are enabled.
>
> Should we make PSCI return that information? (At least in next
> specification version?)

That's a possible solution.

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:49 p.m. UTC | #5
Hi Geert,

On 21/02/17 16:36, Geert Uytterhoeven wrote:
> Hi Sudeep,
> 
> On Tue, Feb 21, 2017 at 11:50 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> On 20/02/17 20:33, Geert Uytterhoeven wrote:
>>> Nothing in the PSCI specification requires the SoC to remain powered and
>>> to support wake-up sources when suspended using SYSTEM_SUSPEND.
>>> If the firmware implements the PSCI SYSTEM_SUSPEND operation by cutting
>>> power to the SoC, the only possibly wake-up sources are thus the ones
>>> connected to the PMIC.
>>>
>>> Document and add support for an "arm,psci-system-suspend-is-power-down"
>>> DT property, so Linux uses a different suspend method when other wake-up
>>> sources (e.g. wake on LAN, UART or GPIO) are enabled.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>>  Documentation/devicetree/bindings/arm/psci.txt | 11 +++++++++++
>>>  drivers/firmware/psci.c                        | 13 ++++++++++---
>>>  2 files changed, 21 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
>>> index a2c4f1d524929bb7..16e390ecb7531028 100644
>>> --- a/Documentation/devicetree/bindings/arm/psci.txt
>>> +++ b/Documentation/devicetree/bindings/arm/psci.txt
>>> @@ -68,6 +68,17 @@ state nodes, as per bindings in [1]) must specify the following properties:
>>>               Definition: power_state parameter to pass to the PSCI
>>>                           suspend call.
>>>
>>> + - arm,psci-system-suspend-is-power-down
>>> +             Nothing in the PSCI specification requires the SoC to remain
>>> +             powered and to support wake-up sources when suspended using
>>> +             SYSTEM_SUSPEND.
>>
>> Again, yes SoC can be powered down but you give no reasons why this is
>> useful other than help you to hack around to implement suspend_ops. As
> 
> This is useful to support other wake-up sources. Linux has a standardized
> way to handle wake-up sources, which may be circumvented by calling PSCI
> SYSTEM_SUSPEND.
> 
>> suggested please try using freeze_ops.
> 
> Freezing the system works, but requires manual configurarion.
> This should be done automatically.
> 

What sort of manual configuration ? I am just asking to understand you
setup better.

>> After commit a94e502c22b6 ("cpuidle: dt: assign ->enter_freeze to same
>> as ->enter callback function"), you can enter suspend-to-idle(a.k.a
>> freeze state) on all platforms using ARM DT cpuidle driver.
> 
> This should be used automatically if the system can't wake-up from
> suspend-to-RAM using the configured wake-up sources.
> 

Yes.
Mark Rutland Feb. 21, 2017, 5:48 p.m. UTC | #6
Hi,

On Mon, Feb 20, 2017 at 09:33:27PM +0100, Geert Uytterhoeven wrote:
> + - arm,psci-system-suspend-is-power-down
> +		Nothing in the PSCI specification requires the SoC to remain
> +		powered and to support wake-up sources when suspended using
> +		SYSTEM_SUSPEND.
> +		If your firmware implements the PSCI SYSTEM_SUSPEND operation
> +		by cutting power to the SoC, the only possibly wake-up sources
> +		are thus the ones connected to the PMIC.  In such case you
> +		should specify this property, so the operating system is aware
> +		it should use a different suspend method when other wake-up
> +		sources (e.g. wake on LAN, UART or GPIO) are enabled.
> +

My understanding is that we already have sufficient information here,
encoded in the wakeup-source property on devices. If DTs have
insufficient information today, we should add those as necessary rather
than modifying the PSCI node.

As Sudeep mentioned, we already have systems which fall into this
category, and those do not require us to do anything special.

As such, I do not believe this is the correct way to describe the
situation.

[...]

> @@ -440,12 +442,14 @@ static int psci_system_suspend_valid(suspend_state_t state)
>  static int psci_system_suspend_enter(suspend_state_t state)
>  {
>  	switch (state) {
> +	case PM_SUSPEND_MEM:
> +		if (!psci_system_suspend_is_power_down ||
> +		    !wakeup_source_available())
> +			return cpu_suspend(0, psci_system_suspend);
> +		/* fall through */

I don't believe that this is the correct place to handle this.

The wakeup_source_available() check *might* be ok, though even with that
I'd rather we rejected the request rather than trying to fall back to a
PSCI_CPU_SUSPEND. Otherwise we have a potential silent power regression.

I can imagine that there are cases where the wakeup source is completely
external and invisible to Linux (e.g. a power button that has no
programming interface, and isn't desscribed at all). In that case, even
the wakeup_source_available() check might be too much. :/

What we could/should do is expose to userspace which suspend cases a
device can wake up the system from and/or whether a wakeup source is
suitable configured for a state currently. That way userspace can
determine whether it is gauranteed to be woken.

Thanks,
Mark.
Mark Rutland Feb. 21, 2017, 5:54 p.m. UTC | #7
On Tue, Feb 21, 2017 at 12:07:30PM +0100, Pavel Machek wrote:
> On Mon 2017-02-20 21:33:27, Geert Uytterhoeven wrote:
> > Nothing in the PSCI specification requires the SoC to remain powered and
> > to support wake-up sources when suspended using SYSTEM_SUSPEND.
> > If the firmware implements the PSCI SYSTEM_SUSPEND operation by cutting
> > power to the SoC, the only possibly wake-up sources are thus the ones
> > connected to the PMIC.
> > 
> > Document and add support for an "arm,psci-system-suspend-is-power-down"
> > DT property, so Linux uses a different suspend method when other wake-up
> > sources (e.g. wake on LAN, UART or GPIO) are enabled.
> 
> Should we make PSCI return that information? (At least in next
> specification version?)

Largely, this is somewhat ill-defined, so it's not something that can be
easily (or correctly) described by a limited firmware call interface.

I believe that the correct way to describe this is to describe the
wakeup capabilities on devices. Both ACPI and DT have mechanisms for
that today, though it seems that there is confusion as to precisely how
to use them.

Thanks,
Mark.
Geert Uytterhoeven Feb. 22, 2017, 2:05 p.m. UTC | #8
Hi Mark,

On Tue, Feb 21, 2017 at 6:48 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Feb 20, 2017 at 09:33:27PM +0100, Geert Uytterhoeven wrote:
>> @@ -440,12 +442,14 @@ static int psci_system_suspend_valid(suspend_state_t state)
>>  static int psci_system_suspend_enter(suspend_state_t state)
>>  {
>>       switch (state) {
>> +     case PM_SUSPEND_MEM:
>> +             if (!psci_system_suspend_is_power_down ||
>> +                 !wakeup_source_available())
>> +                     return cpu_suspend(0, psci_system_suspend);
>> +             /* fall through */
>
> I don't believe that this is the correct place to handle this.
>
> The wakeup_source_available() check *might* be ok, though even with that
> I'd rather we rejected the request rather than trying to fall back to a
> PSCI_CPU_SUSPEND. Otherwise we have a potential silent power regression.

If we reject the request here, I think the PM core has to be modified to
try again using a shallower state. Note that it would be better to reject
the state in the .valid() callback instead of in .enter().

You also have to consider this is dynamic not static.
I.e. the availability of other wake-up sources may change at runtime (cfr.
the "wakeup" files in sysfs). Currently pm_sleep_states[] (which controls
which states are available) is initialized from suspend_set_ops(), and not
changed later.

Perhaps pm_sleep_states[] should be updated every time the wakeup_sources
list is changed?

> I can imagine that there are cases where the wakeup source is completely
> external and invisible to Linux (e.g. a power button that has no
> programming interface, and isn't desscribed at all). In that case, even
> the wakeup_source_available() check might be too much. :/

You mean the wakeup source that actually wakes up the system from PSCI
SYSTEM_SUSPEND? On Renesas boards, that's a switch wired to the PMIC,
and currently not described in DT.  Describing it in DT could indeed interfere
with wakeup_source_available() :-(

> What we could/should do is expose to userspace which suspend cases a
> device can wake up the system from and/or whether a wakeup source is
> suitable configured for a state currently. That way userspace can
> determine whether it is gauranteed to be woken.

Right.  And that information should come from DT?

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
Rafael J. Wysocki Feb. 22, 2017, 2:57 p.m. UTC | #9
On Wed, Feb 22, 2017 at 3:05 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Mark,
>
> On Tue, Feb 21, 2017 at 6:48 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> On Mon, Feb 20, 2017 at 09:33:27PM +0100, Geert Uytterhoeven wrote:
>>> @@ -440,12 +442,14 @@ static int psci_system_suspend_valid(suspend_state_t state)
>>>  static int psci_system_suspend_enter(suspend_state_t state)
>>>  {
>>>       switch (state) {
>>> +     case PM_SUSPEND_MEM:
>>> +             if (!psci_system_suspend_is_power_down ||
>>> +                 !wakeup_source_available())
>>> +                     return cpu_suspend(0, psci_system_suspend);
>>> +             /* fall through */
>>
>> I don't believe that this is the correct place to handle this.
>>
>> The wakeup_source_available() check *might* be ok, though even with that
>> I'd rather we rejected the request rather than trying to fall back to a
>> PSCI_CPU_SUSPEND. Otherwise we have a potential silent power regression.
>
> If we reject the request here, I think the PM core has to be modified to
> try again using a shallower state. Note that it would be better to reject
> the state in the .valid() callback instead of in .enter().
>
> You also have to consider this is dynamic not static.
> I.e. the availability of other wake-up sources may change at runtime (cfr.
> the "wakeup" files in sysfs). Currently pm_sleep_states[] (which controls
> which states are available) is initialized from suspend_set_ops(), and not
> changed later.
>
> Perhaps pm_sleep_states[] should be updated every time the wakeup_sources
> list is changed?

No, the definitions of sleep states *are* static.  They have to be, or
user space won't know what sleep state it is asking for.

And, as I said in my last reply to Sudeep, the list of possible wakeup
devices for the given state is part of that definition.  The sysfs
"wakeup" interface is on top of that, not the other way around (which
seems seems to be what you would want).

Thanks,
Rafael

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
index a2c4f1d524929bb7..16e390ecb7531028 100644
--- a/Documentation/devicetree/bindings/arm/psci.txt
+++ b/Documentation/devicetree/bindings/arm/psci.txt
@@ -68,6 +68,17 @@  state nodes, as per bindings in [1]) must specify the following properties:
 		Definition: power_state parameter to pass to the PSCI
 			    suspend call.
 
+ - arm,psci-system-suspend-is-power-down
+		Nothing in the PSCI specification requires the SoC to remain
+		powered and to support wake-up sources when suspended using
+		SYSTEM_SUSPEND.
+		If your firmware implements the PSCI SYSTEM_SUSPEND operation
+		by cutting power to the SoC, the only possibly wake-up sources
+		are thus the ones connected to the PMIC.  In such case you
+		should specify this property, so the operating system is aware
+		it should use a different suspend method when other wake-up
+		sources (e.g. wake on LAN, UART or GPIO) are enabled.
+
 Example:
 
 Case 1: PSCI v0.1 only.
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index 13b4d50bb3577384..0a74c23fd5fe043e 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -20,6 +20,7 @@ 
 #include <linux/linkage.h>
 #include <linux/of.h>
 #include <linux/pm.h>
+#include <linux/pm_wakeup.h>
 #include <linux/printk.h>
 #include <linux/psci.h>
 #include <linux/reboot.h>
@@ -86,6 +87,7 @@  static u32 psci_function_id[PSCI_FN_MAX];
 
 static u32 psci_cpu_suspend_feature;
 static bool psci_suspend_mem_supported;
+static bool psci_system_suspend_is_power_down;
 
 static inline bool psci_has_ext_power_state(void)
 {
@@ -440,12 +442,14 @@  static int psci_system_suspend_valid(suspend_state_t state)
 static int psci_system_suspend_enter(suspend_state_t state)
 {
 	switch (state) {
+	case PM_SUSPEND_MEM:
+		if (!psci_system_suspend_is_power_down ||
+		    !wakeup_source_available())
+			return cpu_suspend(0, psci_system_suspend);
+		/* fall through */
 	case PM_SUSPEND_STANDBY:
 		cpu_do_idle();
 		break;
-
-	case PM_SUSPEND_MEM:
-		return cpu_suspend(0, psci_system_suspend);
 	}
 
 	return 0;
@@ -596,6 +600,9 @@  static int __init psci_0_2_init(struct device_node *np)
 	 */
 	err = psci_probe();
 
+	psci_system_suspend_is_power_down = of_property_read_bool(np,
+		"arm,psci-system-suspend-is-power-down");
+
 out_put_node:
 	of_node_put(np);
 	return err;