All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mark Rutland <mark.rutland@arm.com>
To: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Lina Iyer <lina.iyer@linaro.org>,
	John Stultz <john.stultz@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>,
	Rob Herring <robh+dt@kernel.org>,
	Magnus Damm <magnus.damm@gmail.com>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-renesas-soc@vger.kernel.org, linux-pm@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power
Date: Tue, 21 Feb 2017 17:48:45 +0000	[thread overview]
Message-ID: <20170221174844.GE8605@leverpostej> (raw)
In-Reply-To: <1487622809-25127-5-git-send-email-geert+renesas@glider.be>

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.

WARNING: multiple messages have this Message-ID
From: mark.rutland@arm.com (Mark Rutland)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power
Date: Tue, 21 Feb 2017 17:48:45 +0000	[thread overview]
Message-ID: <20170221174844.GE8605@leverpostej> (raw)
In-Reply-To: <1487622809-25127-5-git-send-email-geert+renesas@glider.be>

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.

  parent reply	other threads:[~2017-02-21 17:49 UTC|newest]

Thread overview: 145+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-20 20:33 [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power Geert Uytterhoeven
2017-02-20 20:33 ` Geert Uytterhoeven
2017-02-20 20:33 ` [PATCH/RFC 1/6] alarmtimer: Postpone wake-up source registration until really available Geert Uytterhoeven
2017-02-20 20:33   ` Geert Uytterhoeven
2017-02-20 20:33 ` [PATCH/RFC 2/6] PM / Wakeup: Add wakeup_source_available() Geert Uytterhoeven
2017-02-20 20:33   ` Geert Uytterhoeven
2017-02-20 20:33 ` [PATCH/RFC 3/6] drivers: firmware: psci: Implement shallow suspend mode Geert Uytterhoeven
2017-02-20 20:33   ` Geert Uytterhoeven
2017-02-21 10:42   ` Sudeep Holla
2017-02-21 10:42     ` Sudeep Holla
2017-02-21 16:23     ` Geert Uytterhoeven
2017-02-21 16:23       ` Geert Uytterhoeven
2017-02-21 16:23       ` Geert Uytterhoeven
2017-02-21 16:51       ` Sudeep Holla
2017-02-21 16:51         ` Sudeep Holla
2017-02-21 16:51         ` Sudeep Holla
2017-02-21 11:07   ` Pavel Machek
2017-02-21 11:07     ` Pavel Machek
2017-02-21 11:07     ` Pavel Machek
2017-02-21 11:14     ` Sudeep Holla
2017-02-21 11:14       ` Sudeep Holla
2017-02-21 11:14       ` Sudeep Holla
2017-02-21 16:32       ` Geert Uytterhoeven
2017-02-21 16:32         ` Geert Uytterhoeven
2017-02-21 16:32         ` Geert Uytterhoeven
2017-02-21 17:20         ` Mark Rutland
2017-02-21 17:20           ` Mark Rutland
2017-02-21 17:20           ` Mark Rutland
2017-02-21 17:20           ` Mark Rutland
2017-02-21 18:06           ` Geert Uytterhoeven
2017-02-21 18:06             ` Geert Uytterhoeven
2017-02-21 18:06             ` Geert Uytterhoeven
2017-02-21 18:06             ` Geert Uytterhoeven
2017-02-21 18:18             ` Mark Rutland
2017-02-21 18:18               ` Mark Rutland
2017-02-21 18:18               ` Mark Rutland
2017-02-21 18:23               ` Geert Uytterhoeven
2017-02-21 18:23                 ` Geert Uytterhoeven
2017-02-21 18:23                 ` Geert Uytterhoeven
2017-02-21 18:23                 ` Geert Uytterhoeven
2017-02-21 17:22         ` Sudeep Holla
2017-02-21 17:22           ` Sudeep Holla
2017-02-21 17:22           ` Sudeep Holla
2017-02-21 17:22           ` Sudeep Holla
2017-02-22 13:47           ` Geert Uytterhoeven
2017-02-22 13:47             ` Geert Uytterhoeven
2017-02-22 13:47             ` Geert Uytterhoeven
2017-02-22 14:35             ` Sudeep Holla
2017-02-22 14:35               ` Sudeep Holla
2017-02-22 14:35               ` Sudeep Holla
2017-02-20 20:33 ` [PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power Geert Uytterhoeven
2017-02-20 20:33   ` Geert Uytterhoeven
2017-02-21 10:50   ` Sudeep Holla
2017-02-21 10:50     ` Sudeep Holla
2017-02-21 16:36     ` Geert Uytterhoeven
2017-02-21 16:36       ` Geert Uytterhoeven
2017-02-21 16:36       ` Geert Uytterhoeven
2017-02-21 16:49       ` Sudeep Holla
2017-02-21 16:49         ` Sudeep Holla
2017-02-21 16:49         ` Sudeep Holla
2017-02-21 11:07   ` Pavel Machek
2017-02-21 11:07     ` Pavel Machek
2017-02-21 16:36     ` Geert Uytterhoeven
2017-02-21 16:36       ` Geert Uytterhoeven
2017-02-21 16:36       ` Geert Uytterhoeven
2017-02-21 17:54     ` Mark Rutland
2017-02-21 17:54       ` Mark Rutland
2017-02-21 17:48   ` Mark Rutland [this message]
2017-02-21 17:48     ` Mark Rutland
2017-02-22 14:05     ` Geert Uytterhoeven
2017-02-22 14:05       ` Geert Uytterhoeven
2017-02-22 14:05       ` Geert Uytterhoeven
2017-02-22 14:57       ` Rafael J. Wysocki
2017-02-22 14:57         ` Rafael J. Wysocki
2017-02-22 14:57         ` Rafael J. Wysocki
2017-02-22 14:57         ` Rafael J. Wysocki
2017-02-20 20:33 ` [PATCH/RFC 5/6] arm64: dts: r8a7795: Fix non-PMIC wake-up sources Geert Uytterhoeven
2017-02-20 20:33   ` Geert Uytterhoeven
2017-02-20 20:33   ` Geert Uytterhoeven
2017-02-20 20:33 ` [PATCH/RFC 6/6] arm64: dts: r8a7796: " Geert Uytterhoeven
2017-02-20 20:33   ` Geert Uytterhoeven
2017-02-21 10:38 ` [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts power Sudeep Holla
2017-02-21 10:38   ` Sudeep Holla
2017-02-21 16:21   ` Geert Uytterhoeven
2017-02-21 16:21     ` Geert Uytterhoeven
2017-02-21 16:21     ` Geert Uytterhoeven
2017-02-21 16:45     ` Sudeep Holla
2017-02-21 16:45       ` Sudeep Holla
2017-02-21 16:45       ` Sudeep Holla
2017-02-21 16:45       ` Sudeep Holla
2017-02-21 17:34       ` Geert Uytterhoeven
2017-02-21 17:34         ` Geert Uytterhoeven
2017-02-21 17:34         ` Geert Uytterhoeven
2017-02-21 17:51         ` Sudeep Holla
2017-02-21 17:51           ` Sudeep Holla
2017-02-21 17:51           ` Sudeep Holla
2017-02-21 18:27           ` Sudeep Holla
2017-02-21 18:27             ` Sudeep Holla
2017-02-21 18:27             ` Sudeep Holla
2017-02-21 18:27             ` Sudeep Holla
2017-02-21 18:45             ` Sudeep Holla
2017-02-21 18:45               ` Sudeep Holla
2017-02-21 18:45               ` Sudeep Holla
2017-02-21 18:45               ` Sudeep Holla
2017-02-22  1:14               ` Rafael J. Wysocki
2017-02-22  1:14                 ` Rafael J. Wysocki
2017-02-22  1:14                 ` Rafael J. Wysocki
2017-02-22 11:03                 ` Sudeep Holla
2017-02-22 11:03                   ` Sudeep Holla
2017-02-22 11:03                   ` Sudeep Holla
2017-02-22 13:38                   ` Geert Uytterhoeven
2017-02-22 13:38                     ` Geert Uytterhoeven
2017-02-22 13:38                     ` Geert Uytterhoeven
2017-02-22 14:32                     ` Sudeep Holla
2017-02-22 14:32                       ` Sudeep Holla
2017-02-22 14:32                       ` Sudeep Holla
2017-02-22 14:32                       ` Sudeep Holla
2017-02-22 14:50                       ` Rafael J. Wysocki
2017-02-22 14:50                         ` Rafael J. Wysocki
2017-02-22 14:50                         ` Rafael J. Wysocki
2017-02-22 14:50                         ` Rafael J. Wysocki
2017-02-22 15:24                         ` Sudeep Holla
2017-02-22 15:24                           ` Sudeep Holla
2017-02-22 15:24                           ` Sudeep Holla
2017-02-22 15:24                           ` Sudeep Holla
2017-02-23 15:26                       ` Geert Uytterhoeven
2017-02-23 15:26                         ` Geert Uytterhoeven
2017-02-23 15:26                         ` Geert Uytterhoeven
2017-02-23 15:34                         ` Geert Uytterhoeven
2017-02-23 15:34                           ` Geert Uytterhoeven
2017-02-23 15:34                           ` Geert Uytterhoeven
2017-02-23 15:58                           ` Sudeep Holla
2017-02-23 15:58                             ` Sudeep Holla
2017-02-23 15:58                             ` Sudeep Holla
2017-02-23 15:53                         ` Sudeep Holla
2017-02-23 15:53                           ` Sudeep Holla
2017-02-23 15:53                           ` Sudeep Holla
2017-02-23 15:53                           ` Sudeep Holla
2017-02-22 13:14                 ` Geert Uytterhoeven
2017-02-22 13:14                   ` Geert Uytterhoeven
2017-02-22 13:14                   ` Geert Uytterhoeven
2017-02-22 14:31                   ` Rafael J. Wysocki
2017-02-22 14:31                     ` Rafael J. Wysocki
2017-02-22 14:31                     ` Rafael J. Wysocki
2017-02-22 14:31                     ` Rafael J. Wysocki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170221174844.GE8605@leverpostej \
    --to=mark.rutland@arm.com \
    --cc=devicetree@vger.kernel.org \
    --cc=geert+renesas@glider.be \
    --cc=john.stultz@linaro.org \
    --cc=len.brown@intel.com \
    --cc=lina.iyer@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=magnus.damm@gmail.com \
    --cc=pavel@ucw.cz \
    --cc=rjw@rjwysocki.net \
    --cc=robh+dt@kernel.org \
    --cc=sudeep.holla@arm.com \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.