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

[PATCH] 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>
---

I originally was going to check for S3 only with !ARCH_X86 but I can't
convince myself that it's any use to limit the check to s0ix in general.
This approach assumes the last resume result is valid if the command is
supported, regardless of whether or not the AP supports s0ix or not.
This seems to be the case at least on ARM, and looking at the EC
convinces me that CONFIG_POWER_SLEEP_FAILURE_DETECTION is only enabled
on x86 devices that have s0ix.

 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..881d3ad09be0 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_SUSPEND ||
+	     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] platform/chrome: cros_ec: Always expose last resume result

[Guenter Roeck]

On Fri, Jun 10, 2022 at 3:37 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 originally was going to check for S3 only with !ARCH_X86 but I can't
> convince myself that it's any use to limit the check to s0ix in general.
> This approach assumes the last resume result is valid if the command is
> supported, regardless of whether or not the AP supports s0ix or not.
> This seems to be the case at least on ARM, and looking at the EC
> convinces me that CONFIG_POWER_SLEEP_FAILURE_DETECTION is only enabled
> on x86 devices that have s0ix.
>
>  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..881d3ad09be0 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_SUSPEND ||
> +            sleep_event == HOST_SLEEP_EVENT_S3_RESUME)) {

I am sure I am missing something, but the description doesn't explain
(to me) the switch from HOST_SLEEP_EVENT_S0IX_RESUME to
HOST_SLEEP_EVENT_S0IX_SUSPEND.

Thanks,
Guenter

>                 ec_dev->last_resume_result =
>                         buf.u.resp1.resume_response.sleep_transitions;
>
>
> base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
> --
> https://chromeos.dev
>

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

[Stephen Boyd]

Quoting Guenter Roeck (2022-06-10 18:40:33)
> On Fri, Jun 10, 2022 at 3:37 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> > index b3e94cdf7d1a..881d3ad09be0 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_SUSPEND ||
> > +            sleep_event == HOST_SLEEP_EVENT_S3_RESUME)) {
>
> I am sure I am missing something, but the description doesn't explain
> (to me) the switch from HOST_SLEEP_EVENT_S0IX_RESUME to
> HOST_SLEEP_EVENT_S0IX_SUSPEND.

You didn't miss anything.