Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1584889 > unrolled thread

[PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power

Back to article view | Back to linux.kernel

Contents

  [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power Geert Uytterhoeven <geert+renesas@glider.be> - 2017-02-20 21:40 +0100
    [PATCH/RFC 2/6] PM / Wakeup: Add wakeup_source_available() Geert Uytterhoeven <geert+renesas@glider.be> - 2017-02-20 21:40 +0100
    [PATCH/RFC 3/6] drivers: firmware: psci: Implement shallow suspend mode Geert Uytterhoeven <geert+renesas@glider.be> - 2017-02-20 21:40 +0100
      Re: [PATCH/RFC 3/6] drivers: firmware: psci: Implement shallow  suspend mode Sudeep Holla <sudeep.holla@arm.com> - 2017-02-21 11:50 +0100
        Re: [PATCH/RFC 3/6] drivers: firmware: psci: Implement shallow  suspend mode Geert Uytterhoeven <geert@linux-m68k.org> - 2017-02-21 17:30 +0100
          Re: [PATCH/RFC 3/6] drivers: firmware: psci: Implement shallow  suspend mode Sudeep Holla <sudeep.holla@arm.com> - 2017-02-21 18:00 +0100
      Re: [PATCH/RFC 3/6] drivers: firmware: psci: Implement shallow  suspend mode Pavel Machek <pavel@ucw.cz> - 2017-02-21 12:10 +0100
        Re: [PATCH/RFC 3/6] drivers: firmware: psci: Implement shallow  suspend mode Sudeep Holla <sudeep.holla@arm.com> - 2017-02-21 12:20 +0100
          Re: [PATCH/RFC 3/6] drivers: firmware: psci: Implement shallow  suspend mode Geert Uytterhoeven <geert@linux-m68k.org> - 2017-02-21 17:40 +0100
            Re: [PATCH/RFC 3/6] drivers: firmware: psci: Implement shallow  suspend mode Sudeep Holla <sudeep.holla@arm.com> - 2017-02-21 18:30 +0100
              Re: [PATCH/RFC 3/6] drivers: firmware: psci: Implement shallow  suspend mode Geert Uytterhoeven <geert@linux-m68k.org> - 2017-02-22 14:50 +0100
                Re: [PATCH/RFC 3/6] drivers: firmware: psci: Implement shallow  suspend mode Sudeep Holla <sudeep.holla@arm.com> - 2017-02-22 15:40 +0100
            Re: [PATCH/RFC 3/6] drivers: firmware: psci: Implement shallow  suspend mode Mark Rutland <mark.rutland@arm.com> - 2017-02-21 18:30 +0100
              Re: [PATCH/RFC 3/6] drivers: firmware: psci: Implement shallow  suspend mode Geert Uytterhoeven <geert@linux-m68k.org> - 2017-02-21 19:10 +0100
                Re: [PATCH/RFC 3/6] drivers: firmware: psci: Implement shallow  suspend mode Mark Rutland <mark.rutland@arm.com> - 2017-02-21 19:20 +0100
                  Re: [PATCH/RFC 3/6] drivers: firmware: psci: Implement shallow  suspend mode Geert Uytterhoeven <geert@linux-m68k.org> - 2017-02-21 19:30 +0100
    [PATCH/RFC 1/6] alarmtimer: Postpone wake-up source registration until really available Geert Uytterhoeven <geert+renesas@glider.be> - 2017-02-20 21:40 +0100
    [PATCH/RFC 6/6] arm64: dts: r8a7796: Fix non-PMIC wake-up sources Geert Uytterhoeven <geert+renesas@glider.be> - 2017-02-20 21:40 +0100
    [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power Geert Uytterhoeven <geert+renesas@glider.be> - 2017-02-20 21:40 +0100
      Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if  SYSTEM_SUSPEND cuts power Sudeep Holla <sudeep.holla@arm.com> - 2017-02-21 12:00 +0100
        Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if  SYSTEM_SUSPEND cuts power Geert Uytterhoeven <geert@linux-m68k.org> - 2017-02-21 17:40 +0100
          Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if  SYSTEM_SUSPEND cuts power Sudeep Holla <sudeep.holla@arm.com> - 2017-02-21 17:50 +0100
      Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if  SYSTEM_SUSPEND cuts power Pavel Machek <pavel@ucw.cz> - 2017-02-21 12:10 +0100
        Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if  SYSTEM_SUSPEND cuts power Geert Uytterhoeven <geert@linux-m68k.org> - 2017-02-21 17:40 +0100
        Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if  SYSTEM_SUSPEND cuts power Mark Rutland <mark.rutland@arm.com> - 2017-02-21 19:00 +0100
      Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if  SYSTEM_SUSPEND cuts power Mark Rutland <mark.rutland@arm.com> - 2017-02-21 18:50 +0100
        Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if  SYSTEM_SUSPEND cuts power Geert Uytterhoeven <geert@linux-m68k.org> - 2017-02-22 15:10 +0100
          Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if  SYSTEM_SUSPEND cuts power "Rafael J. Wysocki" <rafael@kernel.org> - 2017-02-22 16:00 +0100
    Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts  power Sudeep Holla <sudeep.holla@arm.com> - 2017-02-21 11:40 +0100
      Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power Geert Uytterhoeven <geert@linux-m68k.org> - 2017-02-21 17:30 +0100
        Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts  power Sudeep Holla <sudeep.holla@arm.com> - 2017-02-21 17:50 +0100
          Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power Geert Uytterhoeven <geert@linux-m68k.org> - 2017-02-21 18:40 +0100
            Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts  power Sudeep Holla <sudeep.holla@arm.com> - 2017-02-21 19:00 +0100
              Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts  power Sudeep Holla <sudeep.holla@arm.com> - 2017-02-21 19:40 +0100
                Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts  power Sudeep Holla <sudeep.holla@arm.com> - 2017-02-21 19:50 +0100
                  Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-02-22 02:30 +0100
                    Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts  power Sudeep Holla <sudeep.holla@arm.com> - 2017-02-22 12:10 +0100
                      Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power Geert Uytterhoeven <geert@linux-m68k.org> - 2017-02-22 14:40 +0100
                        Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts  power Sudeep Holla <sudeep.holla@arm.com> - 2017-02-22 15:40 +0100
                          Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power "Rafael J. Wysocki" <rafael@kernel.org> - 2017-02-22 16:00 +0100
                            Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts  power Sudeep Holla <sudeep.holla@arm.com> - 2017-02-22 17:00 +0100
                          Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power Geert Uytterhoeven <geert@linux-m68k.org> - 2017-02-23 16:30 +0100
                            Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power Geert Uytterhoeven <geert@linux-m68k.org> - 2017-02-23 16:40 +0100
                              Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts  power Sudeep Holla <sudeep.holla@arm.com> - 2017-02-23 17:00 +0100
                            Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts  power Sudeep Holla <sudeep.holla@arm.com> - 2017-02-23 17:00 +0100
                    Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power Geert Uytterhoeven <geert@linux-m68k.org> - 2017-02-22 14:20 +0100
                      Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power "Rafael J. Wysocki" <rafael@kernel.org> - 2017-02-22 15:40 +0100

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1585506 — Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power

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

[toc] | [prev] | [next] | [standalone]

#1585525 — Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power

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.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1585219 — Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power

[Multipart message — attachments visible in raw view] — view raw

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

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1585514 — Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power

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

[toc] | [prev] | [next] | [standalone]

#1585572 — Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power

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.

[toc] | [prev] | [next] | [standalone]

#1585564 — Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power

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.

[toc] | [prev] | [next] | [standalone]

#1586177 — Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power

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

[toc] | [prev] | [next] | [standalone]

#1586230 — Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power

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

[toc] | [prev] | [next] | [standalone]

#1585199 — Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power

Hi Geert,

On 20/02/17 20:33, Geert Uytterhoeven wrote:
> 	Hi all,
> 
> This patch series adds support for using non-PMIC wake-up sources on the
> Renesas R-Car Gen3 (H3 or M3-W) Salvator-X development boards.
> 
> 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.
> 

OK, but I don't see any issue with that. That's exactly how it works on
ARM Juno platform. The SoC is powered down.

> To allow other wake-up sources, this patch series documents and adds
> support for an "arm,psci-system-suspend-is-power-down" DT property, so

NACK, you don't need any such properties.

> Linux uses a different suspend method when other wake-up sources (e.g.
> wake on LAN, UART or GPIO) are enabled.  Hence the user no longer has to
> manually restrict "mem" suspend to "s2idle" or "shallow" states using:
> 

Have you explored suspend-to-idle instead ? It looks like thats exactly
what you are doing in this patch set. You also get low latency for free
as it just enters the deepest idle state on all CPUs instead of
hotplugging out all the secondaries.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1585495

Hi Sudeep,

On Tue, Feb 21, 2017 at 11:38 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On 20/02/17 20:33, Geert Uytterhoeven wrote:
>> This patch series adds support for using non-PMIC wake-up sources on the
>> Renesas R-Car Gen3 (H3 or M3-W) Salvator-X development boards.
>>
>> 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.
>
> OK, but I don't see any issue with that. That's exactly how it works on

How do you use other wake-up sources, like wake on LAN, UART or GPIO?

> ARM Juno platform. The SoC is powered down.

Good to hear this is not limited to Renesas platforms, so there's a common
problem to solve.

>> To allow other wake-up sources, this patch series documents and adds
>> support for an "arm,psci-system-suspend-is-power-down" DT property, so
>
> NACK, you don't need any such properties.

If this is true for all PSCI platforms, there's indeed no need for such a
property, and drivers/firmware/psci.c should default to this case.

>> Linux uses a different suspend method when other wake-up sources (e.g.
>> wake on LAN, UART or GPIO) are enabled.  Hence the user no longer has to
>> manually restrict "mem" suspend to "s2idle" or "shallow" states using:
>
> Have you explored suspend-to-idle instead ? It looks like thats exactly
> what you are doing in this patch set. You also get low latency for free
> as it just enters the deepest idle state on all CPUs instead of
> hotplugging out all the secondaries.

Yes, cfr. "s2idle" above.
The user can specify to use "s2idle" manually:

    $ echo s2idle > /sys/power/mem_sleep # or "shallow"

However, how to handle this automatically, e.g. by a distro?
On most other platforms, userspace can just do e.g.

    ethtool -s eth0 wol g

to enable wake-on-LAN, and suspend to the deepest supported state using:

    echo mem > /sys/power/state

On systems where PSCI SYSTEM_SUSPEND powers down the SoC, userspace must
make sure to configure to use "s2idle" (or "shallow) instead, else the
configured wake-up sources won't work.
I want Linux to handle this automatically.

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

[toc] | [prev] | [next] | [standalone]

#1585520 — Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power

Hi Geert,

On 21/02/17 16:21, Geert Uytterhoeven wrote:
> Hi Sudeep,
> 
> On Tue, Feb 21, 2017 at 11:38 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> On 20/02/17 20:33, Geert Uytterhoeven wrote:
>>> This patch series adds support for using non-PMIC wake-up sources on the
>>> Renesas R-Car Gen3 (H3 or M3-W) Salvator-X development boards.
>>>
>>> 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.
>>
>> OK, but I don't see any issue with that. That's exactly how it works on
> 
> How do you use other wake-up sources, like wake on LAN, UART or GPIO?
> 

From wakeup source configuration/management perspective, s2ram and
s2idle are exactly same.

>> ARM Juno platform. The SoC is powered down.
> 
> Good to hear this is not limited to Renesas platforms, so there's a common
> problem to solve.
> 

No, there's no problem to solve. Firmware should enter deepest sleep
state in the system with SYSTEM_SUSPEND from which it can wakeup of course.

>>> To allow other wake-up sources, this patch series documents and adds
>>> support for an "arm,psci-system-suspend-is-power-down" DT property, so
>>
>> NACK, you don't need any such properties.
> 
> If this is true for all PSCI platforms, there's indeed no need for such a
> property, and drivers/firmware/psci.c should default to this case.
> 

Cool.

>>> Linux uses a different suspend method when other wake-up sources (e.g.
>>> wake on LAN, UART or GPIO) are enabled.  Hence the user no longer has to
>>> manually restrict "mem" suspend to "s2idle" or "shallow" states using:
>>
>> Have you explored suspend-to-idle instead ? It looks like thats exactly
>> what you are doing in this patch set. You also get low latency for free
>> as it just enters the deepest idle state on all CPUs instead of
>> hotplugging out all the secondaries.
> 
> Yes, cfr. "s2idle" above.
> The user can specify to use "s2idle" manually:
> 
>     $ echo s2idle > /sys/power/mem_sleep # or "shallow"
> 

This looks like custom file for me.
IIUC, the standard sysfs file for the system PM state is:
	/sys/power/state
1. s2ram:
	$ echo mem > /sys/power/state
2. s2idle
	$ echo freeze > /sys/power/state

> However, how to handle this automatically, e.g. by a distro?

As above

> On most other platforms, userspace can just do e.g.
> 
>     ethtool -s eth0 wol g
> 

That should work.

> to enable wake-on-LAN, and suspend to the deepest supported state using:
> 
>     echo mem > /sys/power/state
> 

This will work only if PSCI SYSTEM_SUPEND is implemented. If the SoC
can't wakeup if it's powered down, then it should not use that state
to implement SYSTEM_SUSPEND in PSCI firmware or just return the
SYSTEM_SUSPEND feature is not implemented in which case "freeze" is the
next available state to enter.

Just botching whatever shallow state you can enter on a particular SoC
into standard "mem" state sounds *horrible* to me.

> On systems where PSCI SYSTEM_SUSPEND powers down the SoC, userspace must
> make sure to configure to use "s2idle" (or "shallow) instead, else the
> configured wake-up sources won't work.

That's perfect. I was worried that user-space is not doing that. So to
summarize, PSCI firmware either:
1. enters a sane and resumable state in SYSTEM_SUSPEND api
or
2. just don't implement SYSTEM_SYSTEM. Use the cpuidle+s2idle framework
   in Linux to enter the deepest idle state.

You literally need no extra work to enter this "freeze" state if the
CPU_SUSPEND in PSCI can enter the deepest idle state you want to enter
in this "s2idle" you are referring so far.
Just start with:

	$ cat /sys/power/state

and you should see "freeze" there, if not that's the first thing to
check provided the platform has cpuidle working.

Hope this helps.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1585561

Hi Sudeep,

On Tue, Feb 21, 2017 at 5:45 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On 21/02/17 16:21, Geert Uytterhoeven wrote:
>> On Tue, Feb 21, 2017 at 11:38 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>> On 20/02/17 20:33, Geert Uytterhoeven wrote:
>>>> This patch series adds support for using non-PMIC wake-up sources on the
>>>> Renesas R-Car Gen3 (H3 or M3-W) Salvator-X development boards.
>>>>
>>>> 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.
>>>
>>> OK, but I don't see any issue with that. That's exactly how it works on
>>
>> How do you use other wake-up sources, like wake on LAN, UART or GPIO?
>
> From wakeup source configuration/management perspective, s2ram and
> s2idle are exactly same.

From the point of view of Linux, that's indeed the case. Linux knows about e.g.
interrupt controllers to keep awake if they're needed for one of the configured
wake-up sources.

PSCI does not know about the wake-up sources configured under Linux.

>>> ARM Juno platform. The SoC is powered down.
>>
>> Good to hear this is not limited to Renesas platforms, so there's a common
>> problem to solve.
>
> No, there's no problem to solve. Firmware should enter deepest sleep
> state in the system with SYSTEM_SUSPEND from which it can wakeup of course.

While SYSTEM_SUSPEND can wake up (e.g. from PMIC), it may not support all
wake-up sources configured from Linux. There's no API to communicate that
information (from Linux to PSCI), or to communicate that limitation (from
PSCI to Linux).

>>>> To allow other wake-up sources, this patch series documents and adds
>>>> support for an "arm,psci-system-suspend-is-power-down" DT property, so
>>>
>>> NACK, you don't need any such properties.
>>
>> If this is true for all PSCI platforms, there's indeed no need for such a
>> property, and drivers/firmware/psci.c should default to this case.
>
> Cool.
>
>>>> Linux uses a different suspend method when other wake-up sources (e.g.
>>>> wake on LAN, UART or GPIO) are enabled.  Hence the user no longer has to
>>>> manually restrict "mem" suspend to "s2idle" or "shallow" states using:
>>>
>>> Have you explored suspend-to-idle instead ? It looks like thats exactly
>>> what you are doing in this patch set. You also get low latency for free
>>> as it just enters the deepest idle state on all CPUs instead of
>>> hotplugging out all the secondaries.
>>
>> Yes, cfr. "s2idle" above.
>> The user can specify to use "s2idle" manually:
>>
>>     $ echo s2idle > /sys/power/mem_sleep # or "shallow"
>
> This looks like custom file for me.

/sys/power/mem_sleep was added in v4.10-rc1, to choose which state to use
for s2ram, cfr. Documentation/power/states.txt.

> IIUC, the standard sysfs file for the system PM state is:
>         /sys/power/state
> 1. s2ram:
>         $ echo mem > /sys/power/state

As of v4.10-rc1, this will use either "s2idle", "shallow", or "deep" mode,
depending on availability and configuration through mem_sleep.
("deep" maps to PSCI SYSTEM_SUSPEND).

> 2. s2idle
>         $ echo freeze > /sys/power/state

Correct.

>> However, how to handle this automatically, e.g. by a distro?
>
> As above

I meant the "mem" one, which should not pick "deep" mode if it cannot wake-up
from that state using the configured wake-up sources.

>> On most other platforms, userspace can just do e.g.
>>
>>     ethtool -s eth0 wol g
>
> That should work.
>
>> to enable wake-on-LAN, and suspend to the deepest supported state using:
>>
>>     echo mem > /sys/power/state
>
> This will work only if PSCI SYSTEM_SUPEND is implemented. If the SoC
> can't wakeup if it's powered down, then it should not use that state
> to implement SYSTEM_SUSPEND in PSCI firmware or just return the
> SYSTEM_SUSPEND feature is not implemented in which case "freeze" is the
> next available state to enter.

The SoC can wake-up. It's just not guaranteed that it can wake-up using
the wakeup-source configured from Linux. Which wakeup-sources are available
depends on the actual PSCI implementation.  It's not specified by the PSCI
specification.

> Just botching whatever shallow state you can enter on a particular SoC
> into standard "mem" state sounds *horrible* to me.

That's more or less what /sys/power/mem_sleep does, though.

>> On systems where PSCI SYSTEM_SUSPEND powers down the SoC, userspace must
>> make sure to configure to use "s2idle" (or "shallow) instead, else the
>> configured wake-up sources won't work.
>
> That's perfect. I was worried that user-space is not doing that. So to
> summarize, PSCI firmware either:
> 1. enters a sane and resumable state in SYSTEM_SUSPEND api

In this case, it may resume using the PMIC only.
And there's no way for userspace (or even the kernel) to find out!
Hence my solution to:
  - add a DT property to indicate that PSCI will power down the SoC,
  - use "shallow" suspend if any Linux wakeup-sources have been configured
    and the property above is present.

> or
> 2. just don't implement SYSTEM_SYSTEM. Use the cpuidle+s2idle framework
>    in Linux to enter the deepest idle state.

In that case, it indeeds falls back to cpuidle/s2idle, which works fine.

> You literally need no extra work to enter this "freeze" state if the
> CPU_SUSPEND in PSCI can enter the deepest idle state you want to enter
> in this "s2idle" you are referring so far.
> Just start with:
>
>         $ cat /sys/power/state
>
> and you should see "freeze" there, if not that's the first thing to
> check provided the platform has cpuidle working.

"freeze" is always available.
"deep" is available if PSCI supports SYSTEM_SUSPEND.
My third patch adds "shallow", but it can be dropped (patch 4 can fall
through to cpu_do_idle() when needed, regardless of the existence of shallow).

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

[toc] | [prev] | [next] | [standalone]

#1585575 — Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power

On 21/02/17 17:34, Geert Uytterhoeven wrote:
> Hi Sudeep,
> 
> On Tue, Feb 21, 2017 at 5:45 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> On 21/02/17 16:21, Geert Uytterhoeven wrote:
>>> On Tue, Feb 21, 2017 at 11:38 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>> On 20/02/17 20:33, Geert Uytterhoeven wrote:
>>>>> This patch series adds support for using non-PMIC wake-up sources on the
>>>>> Renesas R-Car Gen3 (H3 or M3-W) Salvator-X development boards.
>>>>>
>>>>> 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.
>>>>
>>>> OK, but I don't see any issue with that. That's exactly how it works on
>>>
>>> How do you use other wake-up sources, like wake on LAN, UART or GPIO?
>>
>> From wakeup source configuration/management perspective, s2ram and
>> s2idle are exactly same.
> 
> From the point of view of Linux, that's indeed the case. Linux knows about e.g.
> interrupt controllers to keep awake if they're needed for one of the configured
> wake-up sources.
> 

Correct.

> PSCI does not know about the wake-up sources configured under Linux.
>

And the PSCI for a particular platform has to respect those
configurations if they powered down the interrupt controller. They need
to read those wakeups and configure the wakeup controller if required.

Also note we can't mark any device as wakeup source in the device tree
in there's dedicated wakeup controller to enable them and main interrupt
controller is powered down.

>>>> ARM Juno platform. The SoC is powered down.
>>>
>>> Good to hear this is not limited to Renesas platforms, so there's a common
>>> problem to solve.
>>
>> No, there's no problem to solve. Firmware should enter deepest sleep
>> state in the system with SYSTEM_SUSPEND from which it can wakeup of course.
> 
> While SYSTEM_SUSPEND can wake up (e.g. from PMIC), it may not support all
> wake-up sources configured from Linux. There's no API to communicate that
> information (from Linux to PSCI), or to communicate that limitation (from
> PSCI to Linux).
> 

It's platform specific. The SYSTEM_SUSPEND implementation will be aware
of the platform and it's wakeup configuration. As mention above, it
needs to take some action if required.

[...]

>>>>> Linux uses a different suspend method when other wake-up sources (e.g.
>>>>> wake on LAN, UART or GPIO) are enabled.  Hence the user no longer has to
>>>>> manually restrict "mem" suspend to "s2idle" or "shallow" states using:
>>>>
>>>> Have you explored suspend-to-idle instead ? It looks like thats exactly
>>>> what you are doing in this patch set. You also get low latency for free
>>>> as it just enters the deepest idle state on all CPUs instead of
>>>> hotplugging out all the secondaries.
>>>
>>> Yes, cfr. "s2idle" above.
>>> The user can specify to use "s2idle" manually:
>>>
>>>     $ echo s2idle > /sys/power/mem_sleep # or "shallow"
>>
>> This looks like custom file for me.
> 
> /sys/power/mem_sleep was added in v4.10-rc1, to choose which state to use
> for s2ram, cfr. Documentation/power/states.txt.
>

OK, that's new to me. I need to read and understand that then. Sorry for
the noise. I was unaware of that.

>> IIUC, the standard sysfs file for the system PM state is:
>>         /sys/power/state
>> 1. s2ram:
>>         $ echo mem > /sys/power/state
> 
> As of v4.10-rc1, this will use either "s2idle", "shallow", or "deep" mode,
> depending on availability and configuration through mem_sleep.
> ("deep" maps to PSCI SYSTEM_SUSPEND).
> 
>> 2. s2idle
>>         $ echo freeze > /sys/power/state
> 
> Correct.
> 
>>> However, how to handle this automatically, e.g. by a distro?
>>
>> As above
> 
> I meant the "mem" one, which should not pick "deep" mode if it cannot wake-up
> from that state using the configured wake-up sources.
> 
>>> On most other platforms, userspace can just do e.g.
>>>
>>>     ethtool -s eth0 wol g
>>
>> That should work.
>>
>>> to enable wake-on-LAN, and suspend to the deepest supported state using:
>>>
>>>     echo mem > /sys/power/state
>>
>> This will work only if PSCI SYSTEM_SUPEND is implemented. If the SoC
>> can't wakeup if it's powered down, then it should not use that state
>> to implement SYSTEM_SUSPEND in PSCI firmware or just return the
>> SYSTEM_SUSPEND feature is not implemented in which case "freeze" is the
>> next available state to enter.
> 
> The SoC can wake-up. It's just not guaranteed that it can wake-up using
> the wakeup-source configured from Linux. Which wakeup-sources are available
> depends on the actual PSCI implementation.  It's not specified by the PSCI
> specification.
> 
>> Just botching whatever shallow state you can enter on a particular SoC
>> into standard "mem" state sounds *horrible* to me.
> 
> That's more or less what /sys/power/mem_sleep does, though.
> 

OK, I will go through that in detail.

>>> On systems where PSCI SYSTEM_SUSPEND powers down the SoC, userspace must
>>> make sure to configure to use "s2idle" (or "shallow) instead, else the
>>> configured wake-up sources won't work.
>>
>> That's perfect. I was worried that user-space is not doing that. So to
>> summarize, PSCI firmware either:
>> 1. enters a sane and resumable state in SYSTEM_SUSPEND api
> 
> In this case, it may resume using the PMIC only.
> And there's no way for userspace (or even the kernel) to find out!
> Hence my solution to:
>   - add a DT property to indicate that PSCI will power down the SoC,
>   - use "shallow" suspend if any Linux wakeup-sources have been configured
>     and the property above is present.
> 

But what you are doing is *exactly* same as "freeze" state except the
secondaries powered off instead of idling. So my NACK is mainly for that
reason. Had you done something different, then yes, it should be considered.

>> or
>> 2. just don't implement SYSTEM_SYSTEM. Use the cpuidle+s2idle framework
>>    in Linux to enter the deepest idle state.
> 
> In that case, it indeeds falls back to cpuidle/s2idle, which works fine.
> 
>> You literally need no extra work to enter this "freeze" state if the
>> CPU_SUSPEND in PSCI can enter the deepest idle state you want to enter
>> in this "s2idle" you are referring so far.
>> Just start with:
>>
>>         $ cat /sys/power/state
>>
>> and you should see "freeze" there, if not that's the first thing to
>> check provided the platform has cpuidle working.
> 
> "freeze" is always available.
> "deep" is available if PSCI supports SYSTEM_SUSPEND.
> My third patch adds "shallow", but it can be dropped (patch 4 can fall
> through to cpu_do_idle() when needed, regardless of the existence of shallow).
> 

I understand that. But tell me how is that any different from the
"freeze" state. Why do you need that new state at all. Again just
because there's a new feature added doesn't mean we need to use it for
sake of using it. If "freeze" != "shallow", then we can explore this
further. But it's not in your case and hence I see no real need for this.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1585612 — Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power

On 21/02/17 17:51, Sudeep Holla wrote:
> 
> 
> On 21/02/17 17:34, Geert Uytterhoeven wrote:

[...]

>>
>> The SoC can wake-up. It's just not guaranteed that it can wake-up using
>> the wakeup-source configured from Linux. Which wakeup-sources are available
>> depends on the actual PSCI implementation.  It's not specified by the PSCI
>> specification.
>>
>>> Just botching whatever shallow state you can enter on a particular SoC
>>> into standard "mem" state sounds *horrible* to me.
>>
>> That's more or less what /sys/power/mem_sleep does, though.
>>
> 
> OK, I will go through that in detail.
> 

OK, I went through the patch and the main intention is was added.
So I will begin by summarizing my understanding:

A new suspend interface(/sys/power/mem_sleep) is added to allow the
"mem" string in /sys/power/state to represent multiple things that can
be selected.

Before:
A. echo freeze > /sys/power/state ---> Enters s2idle
B. echo mem > /sys/power/state ---> Enters s2r(a.k.a now deep mem sleep)

After:
1. echo freeze > /sys/power/state ---> Enters s2idle still same
2. echo s2idle > /sys/power/mem_sleep
   echo mem > /sys/power/state ---> Also enter s2idle
3. echo deep > /sys/power/mem_sleep
   echo mem > /sys/power/state ---> Also enter s2r(same as [B] above)

Please note I have carefully dropped standby/shallow as we will not
support that state on ARM64 platforms(refer previous discussions for the
same)

Now IIUC, you need 2 above. So, since this new interface allow mem to
mean "s2idle", we need to fix the core to register default suspend_ops
to achieve what you need. And since I now better understand you problem,
you get extra NACK for this series ;)

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1585619 — Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power

On 21/02/17 18:27, Sudeep Holla wrote:
> 
> 
> On 21/02/17 17:51, Sudeep Holla wrote:
>>
>>
>> On 21/02/17 17:34, Geert Uytterhoeven wrote:
> 
> [...]
> 
>>>
>>> The SoC can wake-up. It's just not guaranteed that it can wake-up using
>>> the wakeup-source configured from Linux. Which wakeup-sources are available
>>> depends on the actual PSCI implementation.  It's not specified by the PSCI
>>> specification.
>>>
>>>> Just botching whatever shallow state you can enter on a particular SoC
>>>> into standard "mem" state sounds *horrible* to me.
>>>
>>> That's more or less what /sys/power/mem_sleep does, though.
>>>
>>
>> OK, I will go through that in detail.
>>
> 
> OK, I went through the patch and the main intention is was added.
> So I will begin by summarizing my understanding:
> 
> A new suspend interface(/sys/power/mem_sleep) is added to allow the
> "mem" string in /sys/power/state to represent multiple things that can
> be selected.
> 
> Before:
> A. echo freeze > /sys/power/state ---> Enters s2idle
> B. echo mem > /sys/power/state ---> Enters s2r(a.k.a now deep mem sleep)
> 
> After:
> 1. echo freeze > /sys/power/state ---> Enters s2idle still same
> 2. echo s2idle > /sys/power/mem_sleep
>    echo mem > /sys/power/state ---> Also enter s2idle
> 3. echo deep > /sys/power/mem_sleep
>    echo mem > /sys/power/state ---> Also enter s2r(same as [B] above)
> 
> Please note I have carefully dropped standby/shallow as we will not
> support that state on ARM64 platforms(refer previous discussions for the
> same)
> 
> Now IIUC, you need 2 above. So, since this new interface allow mem to
> mean "s2idle", we need to fix the core to register default suspend_ops
> to achieve what you need. 

I take this back, you have everything you need in place, nothing needs
to be done. I just checked again. If I don't register PSCI suspend_ops,
I still get mem in /sys/power/state with s2idle in /sys/power/mem_sleep
which is exactly what we need. Again we don't support standby/shallow
state on ARM64/PSCI.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1585848

On Tuesday, February 21, 2017 06:45:13 PM Sudeep Holla wrote:
> 
> On 21/02/17 18:27, Sudeep Holla wrote:
> > 
> > 
> > On 21/02/17 17:51, Sudeep Holla wrote:
> >>
> >>
> >> On 21/02/17 17:34, Geert Uytterhoeven wrote:
> > 
> > [...]
> > 
> >>>
> >>> The SoC can wake-up. It's just not guaranteed that it can wake-up using
> >>> the wakeup-source configured from Linux. Which wakeup-sources are available
> >>> depends on the actual PSCI implementation.  It's not specified by the PSCI
> >>> specification.
> >>>
> >>>> Just botching whatever shallow state you can enter on a particular SoC
> >>>> into standard "mem" state sounds *horrible* to me.
> >>>
> >>> That's more or less what /sys/power/mem_sleep does, though.
> >>>
> >>
> >> OK, I will go through that in detail.
> >>
> > 
> > OK, I went through the patch and the main intention is was added.
> > So I will begin by summarizing my understanding:
> > 
> > A new suspend interface(/sys/power/mem_sleep) is added to allow the
> > "mem" string in /sys/power/state to represent multiple things that can
> > be selected.
> > 
> > Before:
> > A. echo freeze > /sys/power/state ---> Enters s2idle
> > B. echo mem > /sys/power/state ---> Enters s2r(a.k.a now deep mem sleep)
> > 
> > After:
> > 1. echo freeze > /sys/power/state ---> Enters s2idle still same
> > 2. echo s2idle > /sys/power/mem_sleep
> >    echo mem > /sys/power/state ---> Also enter s2idle
> > 3. echo deep > /sys/power/mem_sleep
> >    echo mem > /sys/power/state ---> Also enter s2r(same as [B] above)
> > 
> > Please note I have carefully dropped standby/shallow as we will not
> > support that state on ARM64 platforms(refer previous discussions for the
> > same)
> > 
> > Now IIUC, you need 2 above. So, since this new interface allow mem to
> > mean "s2idle", we need to fix the core to register default suspend_ops
> > to achieve what you need. 
> 
> I take this back, you have everything you need in place, nothing needs
> to be done. I just checked again. If I don't register PSCI suspend_ops,
> I still get mem in /sys/power/state with s2idle in /sys/power/mem_sleep
> which is exactly what we need. Again we don't support standby/shallow
> state on ARM64/PSCI.

Except for one thing which may or may not be a concern here.

Suspend to idle should only go into states in which all of the available wakeup
devices work.  If there are devices that cannot wake you up from a given state,
this isn't "idle" any more, is it?

As for the device wakeup disable/enable interface, it is for controlling
whether or not a given device should be allowed to generate wakeup signals at
all.

The information on what states a given device can wake up the system from is
platform-specific and generally would need to be taken into consideration at
the platform level.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1586064 — Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power

On 22/02/17 01:14, Rafael J. Wysocki wrote:
> On Tuesday, February 21, 2017 06:45:13 PM Sudeep Holla wrote:

[...]

>>
>> I take this back, you have everything you need in place, nothing needs
>> to be done. I just checked again. If I don't register PSCI suspend_ops,
>> I still get mem in /sys/power/state with s2idle in /sys/power/mem_sleep
>> which is exactly what we need. Again we don't support standby/shallow
>> state on ARM64/PSCI.
> 
> Except for one thing which may or may not be a concern here.
> 
> Suspend to idle should only go into states in which all of the available wakeup
> devices work.  If there are devices that cannot wake you up from a given state,
> this isn't "idle" any more, is it?
> 

True. In this Renasas platform, since the platform doesn't have PSCI
system suspend, we can only support s2idle and not s2ram. In this case
we don't ask platform to enter some system state whereas we suspend all
the devices(leaving wakeup capable devices active) and ask platform to
enter deepest idle state on all the CPUs. I still don't understand the
issue Geert is facing.

Geert, so far you have failed to explain what's different from the new
state you are adding and the existing s2idle.

> As for the device wakeup disable/enable interface, it is for controlling
> whether or not a given device should be allowed to generate wakeup signals at
> all.
> 
> The information on what states a given device can wake up the system from is
> platform-specific and generally would need to be taken into consideration at
> the platform level.
> 

Exactly, that's what I am trying to convince Geert ;)

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1586149

Hi Sudeep,

On Wed, Feb 22, 2017 at 12:03 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On 22/02/17 01:14, Rafael J. Wysocki wrote:
>> On Tuesday, February 21, 2017 06:45:13 PM Sudeep Holla wrote:
>
> [...]
>
>>> I take this back, you have everything you need in place, nothing needs
>>> to be done. I just checked again. If I don't register PSCI suspend_ops,
>>> I still get mem in /sys/power/state with s2idle in /sys/power/mem_sleep
>>> which is exactly what we need. Again we don't support standby/shallow
>>> state on ARM64/PSCI.
>>
>> Except for one thing which may or may not be a concern here.
>>
>> Suspend to idle should only go into states in which all of the available wakeup
>> devices work.  If there are devices that cannot wake you up from a given state,
>> this isn't "idle" any more, is it?
>>
>
> True. In this Renasas platform, since the platform doesn't have PSCI
> system suspend, we can only support s2idle and not s2ram. In this case

Not correct: this Renesas platform does have PSCI system suspend.
So s2ram "works" (it suspends the system, which can be resumed by a switch)

> we don't ask platform to enter some system state whereas we suspend all
> the devices(leaving wakeup capable devices active) and ask platform to
> enter deepest idle state on all the CPUs. I still don't understand the
> issue Geert is facing.

PSCI system suspend does not support wake-up sources configured from Linux.
Hence I cannot use PSCI system suspend if any wake-up sources have been
configured from Linux, and I expect to be able to use them for wake-up.

> Geert, so far you have failed to explain what's different from the new
> state you are adding and the existing s2idle.

I did explain, cfr.:
  1. The power consumption figures in the cover letter:
      - shallow:    8.4 W   6.2 W   (secondary CPU cores off)
  2. The description for patch 3/6:
        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.

>> As for the device wakeup disable/enable interface, it is for controlling
>> whether or not a given device should be allowed to generate wakeup signals at
>> all.
>>
>> The information on what states a given device can wake up the system from is
>> platform-specific and generally would need to be taken into consideration at
>> the platform level.
>
> Exactly, that's what I am trying to convince Geert ;)

So, how does the communication of wake-up sources happens between PSCI
and Linux?

Perhaps, I didn't make myself clear. Let's summarize:
  1. On Renesas R-Car Gen3 platforms, PSCI SYSTEM_SUSPEND is implemented,
  2. On these platforms, PSCI SYSTEM_SUSPEND powers down the SoC, and supports
     wake-up from PMIC only,
  3. If the user wants to use a different wake-up source, these other
wake-up sources fail
     to wake up the system from PSCI SYSTEM_SUSPEND.,
  4. Patch 3/6 adds a new "shallow" state, as it allows to save more
power (the difference
     may be due to suboptimal cpuidle platform support on R-Car Gen3, though),
  5. Patch 4/6 makes the system use the new "shallow" state iff
       a. the user has configured other wake-up sources, and
       b. DT advertises that PSCI will power down the SoC on SYSTEM_SUSPEND,
     hence other wake-up sources now work as expected.

(If you're so against implementing the "shallow" state, it's a red herring,
 and that patch can be dropped.  Patch 4/6 can be trivially modified, as
 all it does is call cpu_do_idle() instead).

E.g. on non-PSCI platforms with an Ethernet driver that supports
Wake-on-LAN, I can do:

        ethtool -s eth0 wol g
        echo mem > /sys/power/state

and be sure that the system can be woken up by sending a WoL MagicPacket.

On PSCI systems, the above may work, or may not work. And there's no way to
find out (in an automated way) whether it will work or not.

If it doesn't work, the user has to configure his system (manually) to
not use "mem" state.
Since v4.10-rc1, that can be done using e.g.

    echo s2idle > /sys/power/mem_sleep

and my patches make that automatic (for a new "shallow" state instead
of "s2idle", though).

So all of this is a usability issue, and a nightmare for userspace
that wants to treat
different platforms in a generic way (e.g. for distros).

I hope this time it's clear what I want to achieve, and how.

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

[toc] | [prev] | [next] | [standalone]

#1586202 — Re: [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power

On 22/02/17 13:38, Geert Uytterhoeven wrote:
> Hi Sudeep,
> 
> On Wed, Feb 22, 2017 at 12:03 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> On 22/02/17 01:14, Rafael J. Wysocki wrote:
>>> On Tuesday, February 21, 2017 06:45:13 PM Sudeep Holla wrote:
>>
>> [...]
>>
>>>> I take this back, you have everything you need in place, nothing needs
>>>> to be done. I just checked again. If I don't register PSCI suspend_ops,
>>>> I still get mem in /sys/power/state with s2idle in /sys/power/mem_sleep
>>>> which is exactly what we need. Again we don't support standby/shallow
>>>> state on ARM64/PSCI.
>>>
>>> Except for one thing which may or may not be a concern here.
>>>
>>> Suspend to idle should only go into states in which all of the available wakeup
>>> devices work.  If there are devices that cannot wake you up from a given state,
>>> this isn't "idle" any more, is it?
>>>
>>
>> True. In this Renasas platform, since the platform doesn't have PSCI
>> system suspend, we can only support s2idle and not s2ram. In this case
> 
> Not correct: this Renesas platform does have PSCI system suspend.
> So s2ram "works" (it suspends the system, which can be resumed by a switch)
> 

Ah OK. Sorry for misunderstanding the platform support.

>> we don't ask platform to enter some system state whereas we suspend all
>> the devices(leaving wakeup capable devices active) and ask platform to
>> enter deepest idle state on all the CPUs. I still don't understand the
>> issue Geert is facing.
> 
> PSCI system suspend does not support wake-up sources configured from Linux.
> Hence I cannot use PSCI system suspend if any wake-up sources have been
> configured from Linux, and I expect to be able to use them for wake-up.
> 

OK, I thought I had told this before. What do you mean by PSCI system
suspend can't wakeup from the configured wakeup source. You just said
above that you can wake up from the switch.

Just enabling the wakeup sources in Linux doesn't mean you can enter
system suspend anytime. You must enter only the state from which you can
resume. And in your case if you can't wakeup from WLAN or wakeup source
you have configured then simply don't enter system suspend.

>> Geert, so far you have failed to explain what's different from the new
>> state you are adding and the existing s2idle.
> 
> I did explain, cfr.:
>   1. The power consumption figures in the cover letter:
>       - shallow:    8.4 W   6.2 W   (secondary CPU cores off)

That's because your CPU_SUSPEND implementation is incomplete. You can
enter the same state as secondary CPU core off even with idle. It's just
that we can save by not entering and exiting the CPU hotplug state
machine. So this "shallow" state can be achieved if your CPU_SUSPEND
implements that state.

>   2. The description for patch 3/6:
>         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.
> 

Yes I understood that, you need to add an extra idle states to get that
shallow state. We have discussed this in past to depth. On ARM64/PSCI,
we will that support "shallow" system suspend mode which can't be
defined generically. Also we can support this shallow state with s2idle.

Your system probably not supporting all the CPU idle states. E.g.: it
may just support CPU ON/OFF/RET and not cluster ON/OFF/RET. Please add
that state to CPU_SUSPEND implementation in the firmware.

>>> As for the device wakeup disable/enable interface, it is for controlling
>>> whether or not a given device should be allowed to generate wakeup signals at
>>> all.
>>>
>>> The information on what states a given device can wake up the system from is
>>> platform-specific and generally would need to be taken into consideration at
>>> the platform level.
>>
>> Exactly, that's what I am trying to convince Geert ;)
> 
> So, how does the communication of wake-up sources happens between PSCI
> and Linux?
> 
> Perhaps, I didn't make myself clear. Let's summarize:
>   1. On Renesas R-Car Gen3 platforms, PSCI SYSTEM_SUSPEND is implemented,

OK got that.

>   2. On these platforms, PSCI SYSTEM_SUSPEND powers down the SoC, and supports
>      wake-up from PMIC only,

OK

>   3. If the user wants to use a different wake-up source, these other
> wake-up sources fail to wake up the system from PSCI SYSTEM_SUSPEND.

In that case don't enter PSCI SYSTEM_SUSPEND

>   4. Patch 3/6 adds a new "shallow" state, as it allows to save more
> power (the difference may be due to suboptimal cpuidle platform support on R-Car Gen3, though),

Why can't you do that in s2idle mode. Please give me the difference
between your shallow state and s2idle state, not just power numbers
but the actual state of CPUs and the devices in the system.

>   5. Patch 4/6 makes the system use the new "shallow" state iff
>        a. the user has configured other wake-up sources, and
>        b. DT advertises that PSCI will power down the SoC on SYSTEM_SUSPEND,
>      hence other wake-up sources now work as expected.
> 

Linux can just leave the wakeup interrupts enabled in the interrupt
controller and the PSCI implementation can understand that. That's what
we mean when we say platform must deal with that. It should not enter
the state blindly if it can't wake up with those sources. It's not
always necessary to communicate such details via dedicated PSCI APIs.
It's impossible and not scalable in a system with 100s of interrupt
sources. Platform can deal with that in better and simple ways than APIs
to exchange such information.

> (If you're so against implementing the "shallow" state, it's a red herring,
>  and that patch can be dropped.  Patch 4/6 can be trivially modified, as
>  all it does is call cpu_do_idle() instead).
> 

And again that's exactly what s2idle implementation. So NACK for each
patch in this series.

> E.g. on non-PSCI platforms with an Ethernet driver that supports
> Wake-on-LAN, I can do:
> 
>         ethtool -s eth0 wol g
>         echo mem > /sys/power/state
> 
> and be sure that the system can be woken up by sending a WoL MagicPacket.
> 

Still possible with s2idle if CPU_SUSPEND is correctly implemented by
the platform.

> On PSCI systems, the above may work, or may not work. And there's no way to
> find out (in an automated way) whether it will work or not.
> 
> If it doesn't work, the user has to configure his system (manually) to
> not use "mem" state.
> Since v4.10-rc1, that can be done using e.g.
> 
>     echo s2idle > /sys/power/mem_sleep
> 
> and my patches make that automatic (for a new "shallow" state instead
> of "s2idle", though).

How is that ? If "deep" is available as in your case too, why will
shallow become default. IIUC the user still have to write "shallow"
to mem_sleep.

Again to re-iterate, we will not support the not so well defined
"shallow" sleep state unless it's proved that the same can't be done
with cpuidle.

> 
> So all of this is a usability issue, and a nightmare for userspace
> that wants to treat different platforms in a generic way (e.g. for distros).
> 

Yes exactly and that's one reason why "shallow" is useless as it's not
well defined or should I say ill-defined.

> I hope this time it's clear what I want to achieve, and how.
> 

Yes, go and fix the CPU_SUSPEND to implement all the idle states correctly.

Does this platform use generic arm64 DT cpuidle driver ? I don't see so
from the DT.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1586224

On Wed, Feb 22, 2017 at 3:32 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 22/02/17 13:38, Geert Uytterhoeven wrote:
>> Hi Sudeep,
>>
>> On Wed, Feb 22, 2017 at 12:03 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>> On 22/02/17 01:14, Rafael J. Wysocki wrote:
>>>> On Tuesday, February 21, 2017 06:45:13 PM Sudeep Holla wrote:
>>>
>>> [...]
>>>
>>>>> I take this back, you have everything you need in place, nothing needs
>>>>> to be done. I just checked again. If I don't register PSCI suspend_ops,
>>>>> I still get mem in /sys/power/state with s2idle in /sys/power/mem_sleep
>>>>> which is exactly what we need. Again we don't support standby/shallow
>>>>> state on ARM64/PSCI.
>>>>
>>>> Except for one thing which may or may not be a concern here.
>>>>
>>>> Suspend to idle should only go into states in which all of the available wakeup
>>>> devices work.  If there are devices that cannot wake you up from a given state,
>>>> this isn't "idle" any more, is it?
>>>>
>>>
>>> True. In this Renasas platform, since the platform doesn't have PSCI
>>> system suspend, we can only support s2idle and not s2ram. In this case
>>
>> Not correct: this Renesas platform does have PSCI system suspend.
>> So s2ram "works" (it suspends the system, which can be resumed by a switch)
>>
>
> Ah OK. Sorry for misunderstanding the platform support.
>
>>> we don't ask platform to enter some system state whereas we suspend all
>>> the devices(leaving wakeup capable devices active) and ask platform to
>>> enter deepest idle state on all the CPUs. I still don't understand the
>>> issue Geert is facing.
>>
>> PSCI system suspend does not support wake-up sources configured from Linux.
>> Hence I cannot use PSCI system suspend if any wake-up sources have been
>> configured from Linux, and I expect to be able to use them for wake-up.
>>
>
> OK, I thought I had told this before. What do you mean by PSCI system
> suspend can't wakeup from the configured wakeup source. You just said
> above that you can wake up from the switch.
>
> Just enabling the wakeup sources in Linux doesn't mean you can enter
> system suspend anytime. You must enter only the state from which you can
> resume. And in your case if you can't wakeup from WLAN or wakeup source
> you have configured then simply don't enter system suspend.

Well, not quite.

The sysfs wakeup setting for devices only means whether or not to
enable the generation of wakeup signals for them while suspending.  It
allows to *prevent* devices from waking up the system, but it doesn't
guarantee that they will actually wake up if enabled.

Now, the platform doesn't decide on the sleep state it will go to on
the basis of what devices have been enabled to wake up the system.
The states ("shallow", "deep") have to be defined upfront, including
what devices can wake up from the "shallow" and what devices can wake
up from the "deep" states (these lists need not be the same).  [As I
said before, the assumption is that all of them will be able to wake
up the system from suspend-to-idle.]

So, if user space triggers a transition to the "shallow" state, say,
it will be possible to wake up the system from it by devices that (a)
can wake it up from the "shallow" state as defined for the given
platform and (b) have been enabled to wake up the system via sysfs.

Conversely, if you have a system power state such that only a subset
of devices can wake up from it, it needs to be defined as either
"shallow" or "deep" and the list of possible wakeup sources is part of
that definition.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web