| Message ID | 20220611061825.4119270-1-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] platform/chrome: cros_ec: Always expose last resume result | expand |
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 >
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 >
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
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"?
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.
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;
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(-) base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56