Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190320212030.43682-1-evgreen@chromium.org> <20190320212030.43682-3-evgreen@chromium.org>
In-Reply-To: <20190320212030.43682-3-evgreen@chromium.org>
From:   Guenter Roeck <groeck@google.com>
Date:   Thu, 21 Mar 2019 08:14:50 -0700
Message-ID: <CABXOdTfPfyxWfAvAaphfbhei4wuTxZLCbqS8gr=uCemepJRkkA@mail.gmail.com>
Subject: Re: [PATCH 2/2] platform/chrome: Add support for v1 of host sleep event
To:     Evan Green <evgreen@chromium.org>
Cc:     Benson Leung <bleung@chromium.org>,
        Enric Balletbo i Serra <enric.balletbo@collabora.com>,
        Furquan Shaikh <furquan@chromium.org>,
        Rajat Jain <rajatja@chromium.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Guenter Roeck <groeck@chromium.org>,
        Lee Jones <lee.jones@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Mar 20, 2019 at 2:21 PM Evan Green <evgreen@chromium.org> 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@chromium.org>
>
> ---
>
>  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).
> +       }
>
>         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 ?

> +                    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)));

> +
>         /*
>          * Get host event wake mask, assume all events are wake events
>          * if unavailable.
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 8f2a8918bfa3..b6442201f77f 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -120,6 +120,7 @@ struct cros_ec_command {
>   * @pkt_xfer: Send packet to EC and get response.
>   * @lock: One transaction at a time.
>   * @mkbp_event_supported: True if this EC supports the MKBP event protocol.
> + * @host_sleep_v1: True if this EC supports the sleep v1 command.
>   * @event_notifier: Interrupt event notifier for transport devices.
>   * @event_data: Raw payload transferred with the MKBP event.
>   * @event_size: Size in bytes of the event data.
> @@ -153,6 +154,7 @@ struct cros_ec_device {
>                         struct cros_ec_command *msg);
>         struct mutex lock;
>         bool mkbp_event_supported;
> +       bool host_sleep_v1;
>         struct blocking_notifier_head event_notifier;
>
>         struct ec_response_get_next_event_v1 event_data;
> --
> 2.20.1
>