Re: [PATCH 2/2] platform/chrome: Add support for v1 of host sleep event

[Evan Green]

On Thu, Mar 21, 2019 at 8:15 AM Guenter Roeck <groeck@xxxxxxxxxx> wrote:
>
> On Wed, Mar 20, 2019 at 2:21 PM Evan Green <evgreen@xxxxxxxxxxxx> wrote:
> >
> > Add support in code for the new forms of the host sleep event.
> > Detects the presence of this version of the command at runtime,
> > and use whichever form the EC supports. At this time, always
> > request the default timeout, and only report the failing response
> > via a WARN(). Future versions could accept the sleep parameter
> > from outside the driver, and return the response information to
> > usermode or elsewhere.
> >
> > Signed-off-by: Evan Green <evgreen@xxxxxxxxxxxx>
> >
> > ---
> >
> >  drivers/mfd/cros_ec.c                   | 37 +++++++++++++++++++++----
> >  drivers/platform/chrome/cros_ec_proto.c |  9 ++++++
> >  include/linux/mfd/cros_ec.h             |  2 ++
> >  3 files changed, 43 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> > index 6acfe036d522..5305ea706aa9 100644
> > --- a/drivers/mfd/cros_ec.c
> > +++ b/drivers/mfd/cros_ec.c
> > @@ -75,20 +75,47 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
> >
> >  static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
> >  {
> > +       int ret;
> >         struct {
> >                 struct cros_ec_command msg;
> > -               struct ec_params_host_sleep_event req;
> > +               union {
> > +                       struct ec_params_host_sleep_event req0;
> > +                       struct ec_params_host_sleep_event_v1 req1;
> > +                       struct ec_response_host_sleep_event_v1 resp1;
> > +               } u;
> >         } __packed buf;
> >
> >         memset(&buf, 0, sizeof(buf));
> >
> > -       buf.req.sleep_event = sleep_event;
> > +       if (ec_dev->host_sleep_v1) {
> > +               buf.u.req1.sleep_event = sleep_event;
> > +               buf.u.req1.u.suspend_params.sleep_timeout_ms =
> > +                               EC_HOST_SLEEP_TIMEOUT_DEFAULT;
> > +
> > +               buf.msg.outsize = sizeof(buf.u.req1);
> > +               buf.msg.insize = sizeof(buf.u.resp1);
> > +               buf.msg.version = 1;
> > +
> > +       } else {
> > +               buf.u.req0.sleep_event = sleep_event;
> > +               buf.msg.outsize = sizeof(buf.u.req0);
> > +               buf.msg.version = 0;
>
> Unnecessary assignment (already 0).

Ok.

> > +       }
> >
> >         buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
> > -       buf.msg.version = 0;
> > -       buf.msg.outsize = sizeof(buf.req);
> >
> > -       return cros_ec_cmd_xfer(ec_dev, &buf.msg);
> > +       ret = cros_ec_cmd_xfer(ec_dev, &buf.msg);
> > +
> > +       /* For now, report failure to transition to S0ix with a warning. */
> > +       if (ret >= 0 && ec_dev->host_sleep_v1 &&
> > +           (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME))
> > +               WARN(buf.u.resp1.u.resume_response.sleep_transitions &
>
> Is WARN really appropriate here, or would dev_warn() do ?

I think we want to be pretty loud here. This represents a failure to
enter suspend as detected by the EC.

>
> > +                    EC_HOST_RESUME_SLEEP_TIMEOUT,
> > +                    "EC detected sleep transition timeout. Total slp_s0 transitions: %d",
> > +                    buf.u.resp1.u.resume_response.sleep_transitions &
> > +                    EC_HOST_RESUME_SLEEP_TRANSITIONS_MASK);
> > +
> > +       return ret;
> >  }
> >
> >  int cros_ec_register(struct cros_ec_device *ec_dev)
> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > index 97a068dff192..78ceab659a36 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -414,6 +414,15 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> >         else
> >                 ec_dev->mkbp_event_supported = 1;
> >
> > +       /* Probe if host sleep v1 is supported for S0ix failure detection. */
> > +       ret = cros_ec_get_host_command_version_mask(ec_dev,
> > +                                                   EC_CMD_HOST_SLEEP_EVENT,
> > +                                                   &ver_mask);
> > +       if (ret < 0 || !(ver_mask & EC_VER_MASK(1)))
> > +               ec_dev->host_sleep_v1 = 0;
> > +       else
> > +               ec_dev->host_sleep_v1 = 1;
>
> This is a boolean.
>         ec_dev->host_sleep_v1 = (ret >= 0 && (ver_mask & EC_VER_MASK(1)));

Ok.