[PATCH v2] platform/chrome: cros_ec: Always expose last resume result

[PATCH v2] platform/chrome: cros_ec: Always expose last resume result

[Stephen Boyd]

The last resume result exposing logic in cros_ec_sleep_event()
incorrectly requires S0ix support, which doesn't work on ARM based
systems where S0ix doesn't exist. That's because cros_ec_sleep_event()
only reports the last resume result when the EC indicates the last sleep
event was an S0ix resume. On ARM systems, the last sleep event is always
S3 resume, but the EC can still detect sleep hang events in case some
other part of the AP is blocking sleep.

Always expose the last resume result if the EC supports it so that this
works on all devices regardless of S0ix support. This fixes sleep hang
detection on ARM based chromebooks like Trogdor.

Cc: Evan Green <evgreen@chromium.org>
Cc: Rajat Jain <rajatja@chromium.org>
Cc: Matthias Kaehlcke <mka@chromium.org>
Cc: Hsin-Yi Wang <hsinyi@chromium.org>
Fixes: 7235560ac77a ("platform/chrome: Add support for v1 of host sleep event")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

Changes from v1 (https://lore.kernel.org/r/20220610223703.3387691-1-swboyd@chromium.org):
 * Fixed typo

 drivers/platform/chrome/cros_ec.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index b3e94cdf7d1a..3abef9747482 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -135,10 +135,10 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
 	buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
 
 	ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
-
-	/* For now, report failure to transition to S0ix with a warning. */
+	/* Report failure to transition to system wide suspend with a warning. */
 	if (ret >= 0 && ec_dev->host_sleep_v1 &&
-	    (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME)) {
+	    (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME ||
+	     sleep_event == HOST_SLEEP_EVENT_S3_RESUME)) {
 		ec_dev->last_resume_result =
 			buf.u.resp1.resume_response.sleep_transitions;
 

base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
-- 
https://chromeos.dev

Re: [PATCH v2] platform/chrome: cros_ec: Always expose last resume result

[Guenter Roeck]

On Fri, Jun 10, 2022 at 11:18 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> The last resume result exposing logic in cros_ec_sleep_event()
> incorrectly requires S0ix support, which doesn't work on ARM based
> systems where S0ix doesn't exist. That's because cros_ec_sleep_event()
> only reports the last resume result when the EC indicates the last sleep
> event was an S0ix resume. On ARM systems, the last sleep event is always
> S3 resume, but the EC can still detect sleep hang events in case some
> other part of the AP is blocking sleep.
>
> Always expose the last resume result if the EC supports it so that this
> works on all devices regardless of S0ix support. This fixes sleep hang
> detection on ARM based chromebooks like Trogdor.
>
> Cc: Evan Green <evgreen@chromium.org>
> Cc: Rajat Jain <rajatja@chromium.org>
> Cc: Matthias Kaehlcke <mka@chromium.org>
> Cc: Hsin-Yi Wang <hsinyi@chromium.org>
> Fixes: 7235560ac77a ("platform/chrome: Add support for v1 of host sleep event")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
>
> Changes from v1 (https://lore.kernel.org/r/20220610223703.3387691-1-swboyd@chromium.org):
>  * Fixed typo
>
>  drivers/platform/chrome/cros_ec.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index b3e94cdf7d1a..3abef9747482 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -135,10 +135,10 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
>         buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
>
>         ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> -
> -       /* For now, report failure to transition to S0ix with a warning. */
> +       /* Report failure to transition to system wide suspend with a warning. */
>         if (ret >= 0 && ec_dev->host_sleep_v1 &&
> -           (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME)) {
> +           (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME ||
> +            sleep_event == HOST_SLEEP_EVENT_S3_RESUME)) {
>                 ec_dev->last_resume_result =
>                         buf.u.resp1.resume_response.sleep_transitions;
>
>
> base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
> --
> https://chromeos.dev
>

Re: [PATCH v2] platform/chrome: cros_ec: Always expose last resume result

[Evan Green]

On Fri, Jun 10, 2022 at 11:18 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> The last resume result exposing logic in cros_ec_sleep_event()
> incorrectly requires S0ix support, which doesn't work on ARM based
> systems where S0ix doesn't exist. That's because cros_ec_sleep_event()
> only reports the last resume result when the EC indicates the last sleep
> event was an S0ix resume. On ARM systems, the last sleep event is always
> S3 resume, but the EC can still detect sleep hang events in case some
> other part of the AP is blocking sleep.
>
> Always expose the last resume result if the EC supports it so that this
> works on all devices regardless of S0ix support. This fixes sleep hang
> detection on ARM based chromebooks like Trogdor.
>
> Cc: Evan Green <evgreen@chromium.org>
> Cc: Rajat Jain <rajatja@chromium.org>
> Cc: Matthias Kaehlcke <mka@chromium.org>
> Cc: Hsin-Yi Wang <hsinyi@chromium.org>
> Fixes: 7235560ac77a ("platform/chrome: Add support for v1 of host sleep event")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

I looked at the EC code for firmware-hatch-12669.B, which is I think
the oldest board shipping the suspend_transitions support, and saw
that it correctly initializes resume_response.sleep_transitions for
both S0IX_RESUME and S3_RESUME. I think that mitigates my only concern
that we might be interpreting garbage sleep_transitions from the EC on
some older platforms. So,

Reviewed-by: Evan Green <evgreen@chromium.org>

> ---
>
> Changes from v1 (https://lore.kernel.org/r/20220610223703.3387691-1-swboyd@chromium.org):
>  * Fixed typo
>
>  drivers/platform/chrome/cros_ec.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index b3e94cdf7d1a..3abef9747482 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -135,10 +135,10 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
>         buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
>
>         ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> -
> -       /* For now, report failure to transition to S0ix with a warning. */
> +       /* Report failure to transition to system wide suspend with a warning. */
>         if (ret >= 0 && ec_dev->host_sleep_v1 &&
> -           (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME)) {
> +           (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME ||
> +            sleep_event == HOST_SLEEP_EVENT_S3_RESUME)) {
>                 ec_dev->last_resume_result =
>                         buf.u.resp1.resume_response.sleep_transitions;
>
>
> base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
> --
> https://chromeos.dev
>

Re: [PATCH v2] platform/chrome: cros_ec: Always expose last resume result

[Tzung-Bi Shih]

On Fri, Jun 10, 2022 at 11:18:25PM -0700, Stephen Boyd wrote:
> @@ -135,10 +135,10 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
>  	buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
>  
>  	ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> -
> -	/* For now, report failure to transition to S0ix with a warning. */
> +	/* Report failure to transition to system wide suspend with a warning. */
>  	if (ret >= 0 && ec_dev->host_sleep_v1 &&
> -	    (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME)) {
> +	    (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME ||
> +	     sleep_event == HOST_SLEEP_EVENT_S3_RESUME)) {
>  		ec_dev->last_resume_result =
>  			buf.u.resp1.resume_response.sleep_transitions;

The warning in the if-block:
WARN_ONCE(buf.u.resp1.resume_response.sleep_transitions &
          EC_HOST_RESUME_SLEEP_TIMEOUT,
          "EC detected sleep transition timeout. Total slp_s0 transitions: %d",
          buf.u.resp1.resume_response.sleep_transitions &
          EC_HOST_RESUME_SLEEP_TRANSITIONS_MASK);

Not sure and I wonder if the term "slp_s0" is still generic enough to the case.
And also [1].

[1]: https://crrev.com/a311a7657ca0a488f507fa78fd9c2d825cbf083e/power_manager/powerd/system/suspend_configurator.cc#98

At least we should fix [2] once the patch landed.

[2]: https://crrev.com/a311a7657ca0a488f507fa78fd9c2d825cbf083e/power_manager/tools/suspend_stress_test#369

Re: [PATCH v2] platform/chrome: cros_ec: Always expose last resume result

[Stephen Boyd]

Quoting Tzung-Bi Shih (2022-06-13 22:30:18)
> On Fri, Jun 10, 2022 at 11:18:25PM -0700, Stephen Boyd wrote:
> > @@ -135,10 +135,10 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
> >       buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
> >
> >       ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> > -
> > -     /* For now, report failure to transition to S0ix with a warning. */
> > +     /* Report failure to transition to system wide suspend with a warning. */
> >       if (ret >= 0 && ec_dev->host_sleep_v1 &&
> > -         (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME)) {
> > +         (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME ||
> > +          sleep_event == HOST_SLEEP_EVENT_S3_RESUME)) {
> >               ec_dev->last_resume_result =
> >                       buf.u.resp1.resume_response.sleep_transitions;
>
> The warning in the if-block:
> WARN_ONCE(buf.u.resp1.resume_response.sleep_transitions &
>           EC_HOST_RESUME_SLEEP_TIMEOUT,
>           "EC detected sleep transition timeout. Total slp_s0 transitions: %d",
>           buf.u.resp1.resume_response.sleep_transitions &
>           EC_HOST_RESUME_SLEEP_TRANSITIONS_MASK);
>
> Not sure and I wonder if the term "slp_s0" is still generic enough to the case.
> And also [1].
>
> [1]: https://crrev.com/a311a7657ca0a488f507fa78fd9c2d825cbf083e/power_manager/powerd/system/suspend_configurator.cc#98

I don't know what "slp_s0" means. I guess it's some intelism? It could
change to "sleep" and work for me.

>
> At least we should fix [2] once the patch landed.
>
> [2]: https://crrev.com/a311a7657ca0a488f507fa78fd9c2d825cbf083e/power_manager/tools/suspend_stress_test#369

And by fix you mean change "S0iX" to "suspend"?

Re: [PATCH v2] platform/chrome: cros_ec: Always expose last resume result

[Tzung-Bi Shih]

On Mon, Jun 13, 2022 at 11:17:50PM -0700, Stephen Boyd wrote:
> Quoting Tzung-Bi Shih (2022-06-13 22:30:18)
> > On Fri, Jun 10, 2022 at 11:18:25PM -0700, Stephen Boyd wrote:
> > > @@ -135,10 +135,10 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
> > >       buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
> > >
> > >       ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> > > -
> > > -     /* For now, report failure to transition to S0ix with a warning. */
> > > +     /* Report failure to transition to system wide suspend with a warning. */
> > >       if (ret >= 0 && ec_dev->host_sleep_v1 &&
> > > -         (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME)) {
> > > +         (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME ||
> > > +          sleep_event == HOST_SLEEP_EVENT_S3_RESUME)) {
> > >               ec_dev->last_resume_result =
> > >                       buf.u.resp1.resume_response.sleep_transitions;
> >
> > The warning in the if-block:
> > WARN_ONCE(buf.u.resp1.resume_response.sleep_transitions &
> >           EC_HOST_RESUME_SLEEP_TIMEOUT,
> >           "EC detected sleep transition timeout. Total slp_s0 transitions: %d",
> >           buf.u.resp1.resume_response.sleep_transitions &
> >           EC_HOST_RESUME_SLEEP_TRANSITIONS_MASK);
> >
> > Not sure and I wonder if the term "slp_s0" is still generic enough to the case.
> > And also [1].
> >
> > [1]: https://crrev.com/a311a7657ca0a488f507fa78fd9c2d825cbf083e/power_manager/powerd/system/suspend_configurator.cc#98
> 
> I don't know what "slp_s0" means. I guess it's some intelism? It could
> change to "sleep" and work for me.
> 
> >
> > At least we should fix [2] once the patch landed.
> >
> > [2]: https://crrev.com/a311a7657ca0a488f507fa78fd9c2d825cbf083e/power_manager/tools/suspend_stress_test#369
> 
> And by fix you mean change "S0iX" to "suspend"?

Yes.