| Message ID | 1487622809-25127-5-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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.
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
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
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
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.
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.
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.
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
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
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;
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(-)