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 3 of 3 — ← Prev page 1 2 [3]

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

On 22/02/17 14:50, Rafael J. Wysocki wrote:
> On Wed, Feb 22, 2017 at 3:32 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

[...]

>>
>> 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.

Completely agreed.

And yes the current assumption is that the set of wake-up source in DT
applies to the suspend-to-idle case and may/may not from the deeper
s2ram state.

Sorry if I conveyed something else in my emails. I was mostly referring
to the patch set and the way it's hacking up. I still fail to understand
what Geert is doing different in his "shallow" state that "s2idle" can't
achieve that state. So my comments were more aligned to that when I made
the above comment.

-- 
Regards,
Sudeep

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

#1586957

Hi Sudeep,

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:
>> 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:
>>> 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.

Does that include power areas?

>>   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.

I can find CPU_ON and CPU_OFF in the PSCI specification, but not
CPU_RET?

How is the cluster ON/OFF/RET called exactly? I can't find any CLUSTER_*
calls in the PSCI specification.

From a quick glance in the PSCI sources, there's some support for powering
down clusters.

>> 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

Or prevent the system from doing that...

>>   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.

From the Linux side, there's not much difference, except that the secondary
CPU cores are disabled.  As that is handled by PSCI, the difference may be
in the PSCI implementation.  I will have to check that...

On these SoCs, the individual CPU cores and the SCU/L2 are in separate
(nested) power areas.  Perhaps these power areas are turned off when
disabling the CPU cores, but not when suspending them.

>> 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.

Sure. But not automatic, as it needs fiddling with mem_sleep.

>> 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.

After patch 4, if needed (DT property + extra wake-up sources configured),
psci_system_suspend_enter() will call cpu_do_idle() instead of
psci_system_suspend(). No need to fiddle with mem_sleep manually.

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

I think that task isn't complete yet.

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]

#1586962

Hi Sudeep,

On Thu, Feb 23, 2017 at 4:26 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> 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:
>>> On Wed, Feb 22, 2017 at 12:03 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>   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.
>
> From the Linux side, there's not much difference, except that the secondary
> CPU cores are disabled.  As that is handled by PSCI, the difference may be
> in the PSCI implementation.  I will have to check that...
>
> On these SoCs, the individual CPU cores and the SCU/L2 are in separate
> (nested) power areas.  Perhaps these power areas are turned off when
> disabling the CPU cores, but not when suspending them.

BTW, I don't care much about the extra state.

>>> 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.
>
> Sure. But not automatic, as it needs fiddling with mem_sleep.

I do care about this, as it affects user experience.

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]

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

Hi Geert,

On 23/02/17 15:34, Geert Uytterhoeven wrote:
> Hi Sudeep,
> 
> On Thu, Feb 23, 2017 at 4:26 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> 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:
>>>> On Wed, Feb 22, 2017 at 12:03 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>>   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.
>>
>> From the Linux side, there's not much difference, except that the secondary
>> CPU cores are disabled.  As that is handled by PSCI, the difference may be
>> in the PSCI implementation.  I will have to check that...
>>
>> On these SoCs, the individual CPU cores and the SCU/L2 are in separate
>> (nested) power areas.  Perhaps these power areas are turned off when
>> disabling the CPU cores, but not when suspending them.
> 
> BTW, I don't care much about the extra state.
> 

Then stop caring about extra power usage too ;). Seriously this is not a
valid argument.

>>>> 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.
>>
>> Sure. But not automatic, as it needs fiddling with mem_sleep.
> 
> I do care about this, as it affects user experience.
> 

Again when you have both "deep" and "standby" suspend states as per your
patch set, user has to choose one. No escape from that.

-- 
Regards,
Sudeep

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

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

On 23/02/17 15:26, Geert Uytterhoeven wrote:
> Hi Sudeep,
> 
> 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:
>>> 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:
>>>> 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.
> 
> Does that include power areas?
> 

OK, I first I didn't understand what you are referring here but I see
some reference further in the email.

>>>   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.
> 
> I can find CPU_ON and CPU_OFF in the PSCI specification, but not
> CPU_RET?
> 

No, they were just examples of idle states that CPU_SUSPEND call might
support based on what h/w can support on a particular platform.

> How is the cluster ON/OFF/RET called exactly? I can't find any CLUSTER_*
> calls in the PSCI specification.
> 

CPU_SUSPEND with different parameters, just look at the details on
suspend parameters in Section 5.4.2 CPU_SUSPEND parameters: power_state


> From a quick glance in the PSCI sources, there's some support for powering
> down clusters.
> 

Yes, the above section should provide some insight on the same.

>>> 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
> 
> Or prevent the system from doing that...
> 

Agreed.

>>>   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.
> 
> From the Linux side, there's not much difference, except that the secondary
> CPU cores are disabled.  As that is handled by PSCI, the difference may be
> in the PSCI implementation.  I will have to check that...
> 

Yes it's better to check. I am afraid that both these states will be
same if PSCI implementation is correct and hence we don't want to
support standby suspend mode.

> On these SoCs, the individual CPU cores and the SCU/L2 are in separate
> (nested) power areas.  Perhaps these power areas are turned off when
> disabling the CPU cores, but not when suspending them.
> 

Yes that's what I suspect and hence I said it's incomplete
implementation of CPU_SUSPEND

>>> 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.
> 
> Sure. But not automatic, as it needs fiddling with mem_sleep.
> 

Yes that's true form any thing other than "deep" state. i.e. s2idle or
standby. If you are OK to choose standby why not s2idle ?

>>> 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.
> 
> After patch 4, if needed (DT property + extra wake-up sources configured),
> psci_system_suspend_enter() will call cpu_do_idle() instead of
> psci_system_suspend(). No need to fiddle with mem_sleep manually.
> 

I understand your intentions and but I have NACKed it with sufficient
reasoning. I don't want to repeat them again here.

>> Does this platform use generic arm64 DT cpuidle driver ? I don't see so
>> from the DT.
> 
> I think that task isn't complete yet.
>

So, all these hacks are just to cope up with that ? Sorry that's
non-sense. Working around a firmware bug is different from working
around the incomplete firmware implementation. We may consider former
but for me latter is just insane.

-- 
Regards,
Sudeep

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

#1586134

Hi Rafael,

On Wed, Feb 22, 2017 at 2:14 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> 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:
>> >>> 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?

Indeed. And I have no problem with handling wake-up sources from Linux,
as Linux knows how to handle them.

> 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.

OK. So it's not guaranteed that it will actually work...

> 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.

So that's PSCI on arm64?
But the PSCI specification doesn't handle that.

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]

#1586211

On Wed, Feb 22, 2017 at 2:14 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Rafael,
>
> On Wed, Feb 22, 2017 at 2:14 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> 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:
>>> >>> 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?
>
> Indeed. And I have no problem with handling wake-up sources from Linux,
> as Linux knows how to handle them.
>
>> 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.
>
> OK. So it's not guaranteed that it will actually work...

No, it is not.

Enabling generation of wakeup signals at a device doesn't guarantee
that the interrupt (or GPIO etc) controller will be functional when
those signals reach it, for example.

There actually is no way to guarantee that in general.  In the ACPI
land, for example, devices may be able to wake up the system from S3,
but not from S4 or S5, and you can't say "I want that device to wake
up the system from S4", because that may be physically impossible to
achieve.

>> 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.
>
> So that's PSCI on arm64?
> But the PSCI specification doesn't handle that.

In theory, that should be some code that knows how the platform is
configured and can set up things to work as expected.

That's why we have all of the platform hooks, syscore operations etc
(of course, all of that is not needed for suspend to idle, because it
is entered via the idle path and wakeup signals for wakeup devices
should be handled then).

Thanks,
Rafael

[toc] | [prev] | [standalone]

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

Back to top | Article view | linux.kernel

csiph-web