[PATCH v3 0/2] platform/chrome: Add support for host sleep event command v1

[PATCH v3 0/2] platform/chrome: Add support for host sleep event command v1

[Evan Green]

The Chrome OS EC has an updated set of parameters for the host
sleep event command. With the new parameters, the host can indicate
a timeout along with suspend messages. Specifically S0ix suspend
messages are supported now, though the host command format isn't
specific to S0ix. When the EC sees an S0ix suspend host sleep event,
it arms a timer for the specified number of milliseconds (or a sane
per-board default baked into the EC). If the EC does not observe
the platform's SLP_S0 line assert within the specified timeout, then
the EC wakes the system.

On resume, the EC reports the number of transitions seen on the SLP_S0
line. The high bit is used to report whether or not a timeout occurred.
The number of transitions can then be used to detect cases of excessive
housekeeping activities, where the system wakes up out of S0ix temporarily
(unbeknownst to Linux), and then (hopefully) goes back to sleep.

This mechanism helps in cases where the system attempted to suspend
via S0ix, but due to driver bugs ended up suspending to a shallower
idle state instead. In concert with additional changes that detect
S0ix entry failures, this mechanism allows the system to quickly
detect and report on incorrect suspend outcomes.

Enric,
Gwendal informed me that his patch was rejected, so I'm sending this
as a standalone series rather than basing on top of his. The corresponding
EC code for this has now landed at:
https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1501512


Changes in v3:
- Consolidated boolean logic for host_sleep_v1 (Guenter)

Changes in v2:
- Made unions anonymous
- Replaced reserved union members with a comment
- Removed unnecessary version assignment (Guenter)
- Changed WARN to WARN_ONCE (Guenter)
- Fixed C code to use anonymous unions
- insize is only bigger for resume events.

Evan Green (2):
  mfd: cros_ec: Add host_sleep_event_v1 command
  platform/chrome: Add support for v1 of host sleep event

 drivers/mfd/cros_ec.c                   | 39 ++++++++++++++---
 drivers/platform/chrome/cros_ec_proto.c |  6 +++
 include/linux/mfd/cros_ec.h             |  2 +
 include/linux/mfd/cros_ec_commands.h    | 57 +++++++++++++++++++++++++
 4 files changed, 99 insertions(+), 5 deletions(-)

-- 
2.20.1

[PATCH v3 1/2] mfd: cros_ec: Add host_sleep_event_v1 command

[Evan Green]

Introduce the command and response structures for the second revision
of the host sleep event. These structures are part of a new EC change
that enables detection of failure to enter S0ix. The EC waits a
kernel-specified timeout (or a default amount of time) for the S0_SLP
pin to change, and wakes the system if that change does not occur in
time.

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

---

Changes in v3: None
Changes in v2:
- Made unions anonymous
- Replaced reserved union members with a comment

 include/linux/mfd/cros_ec_commands.h | 57 ++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index fc91082d4c35..1cdb07c0ece1 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -2729,6 +2729,63 @@ struct ec_params_host_sleep_event {
 	uint8_t sleep_event;
 } __packed;
 
+/*
+ * Use a default timeout value (CONFIG_SLEEP_TIMEOUT_MS) for detecting sleep
+ * transition failures
+ */
+#define EC_HOST_SLEEP_TIMEOUT_DEFAULT 0
+
+/* Disable timeout detection for this sleep transition */
+#define EC_HOST_SLEEP_TIMEOUT_INFINITE 0xFFFF
+
+struct ec_params_host_sleep_event_v1 {
+	/* The type of sleep being entered or exited. */
+	uint8_t sleep_event;
+
+	/* Padding */
+	uint8_t reserved;
+	union {
+		/* Parameters that apply for suspend messages. */
+		struct {
+			/*
+			 * The timeout in milliseconds between when this message
+			 * is received and when the EC will declare sleep
+			 * transition failure if the sleep signal is not
+			 * asserted.
+			 */
+			uint16_t sleep_timeout_ms;
+		} suspend_params;
+
+		/* No parameters for non-suspend messages. */
+	};
+} __packed;
+
+/* A timeout occurred when this bit is set */
+#define EC_HOST_RESUME_SLEEP_TIMEOUT 0x80000000
+
+/*
+ * The mask defining which bits correspond to the number of sleep transitions,
+ * as well as the maximum number of suspend line transitions that will be
+ * reported back to the host.
+ */
+#define EC_HOST_RESUME_SLEEP_TRANSITIONS_MASK 0x7FFFFFFF
+
+struct ec_response_host_sleep_event_v1 {
+	union {
+		/* Response fields that apply for resume messages. */
+		struct {
+			/*
+			 * The number of sleep power signal transitions that
+			 * occurred since the suspend message. The high bit
+			 * indicates a timeout occurred.
+			 */
+			uint32_t sleep_transitions;
+		} resume_response;
+
+		/* No response fields for non-resume messages. */
+	};
+} __packed;
+
 /*****************************************************************************/
 /* Smart battery pass-through */
 
-- 
2.20.1

Re: [PATCH v3 1/2] mfd: cros_ec: Add host_sleep_event_v1 command

[Lee Jones]

On Wed, 03 Apr 2019, Evan Green wrote:

> Introduce the command and response structures for the second revision
> of the host sleep event. These structures are part of a new EC change
> that enables detection of failure to enter S0ix. The EC waits a
> kernel-specified timeout (or a default amount of time) for the S0_SLP
> pin to change, and wakes the system if that change does not occur in
> time.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> 
> ---
> 
> Changes in v3: None
> Changes in v2:
> - Made unions anonymous
> - Replaced reserved union members with a comment
> 
>  include/linux/mfd/cros_ec_commands.h | 57 ++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)

Always good to have some additional Chromium reviewers on patches
touching this file.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 1/2] mfd: cros_ec: Add host_sleep_event_v1 command

[Guenter Roeck]

On Wed, Apr 3, 2019 at 2:34 PM Evan Green <evgreen@chromium.org> wrote:
>
> Introduce the command and response structures for the second revision
> of the host sleep event. These structures are part of a new EC change
> that enables detection of failure to enter S0ix. The EC waits a
> kernel-specified timeout (or a default amount of time) for the S0_SLP
> pin to change, and wakes the system if that change does not occur in
> time.
>
> Signed-off-by: Evan Green <evgreen@chromium.org>
>

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

> ---
>
> Changes in v3: None
> Changes in v2:
> - Made unions anonymous
> - Replaced reserved union members with a comment
>
>  include/linux/mfd/cros_ec_commands.h | 57 ++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>
> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> index fc91082d4c35..1cdb07c0ece1 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h
> @@ -2729,6 +2729,63 @@ struct ec_params_host_sleep_event {
>         uint8_t sleep_event;
>  } __packed;
>
> +/*
> + * Use a default timeout value (CONFIG_SLEEP_TIMEOUT_MS) for detecting sleep
> + * transition failures
> + */
> +#define EC_HOST_SLEEP_TIMEOUT_DEFAULT 0
> +
> +/* Disable timeout detection for this sleep transition */
> +#define EC_HOST_SLEEP_TIMEOUT_INFINITE 0xFFFF
> +
> +struct ec_params_host_sleep_event_v1 {
> +       /* The type of sleep being entered or exited. */
> +       uint8_t sleep_event;
> +
> +       /* Padding */
> +       uint8_t reserved;
> +       union {
> +               /* Parameters that apply for suspend messages. */
> +               struct {
> +                       /*
> +                        * The timeout in milliseconds between when this message
> +                        * is received and when the EC will declare sleep
> +                        * transition failure if the sleep signal is not
> +                        * asserted.
> +                        */
> +                       uint16_t sleep_timeout_ms;
> +               } suspend_params;
> +
> +               /* No parameters for non-suspend messages. */
> +       };
> +} __packed;
> +
> +/* A timeout occurred when this bit is set */
> +#define EC_HOST_RESUME_SLEEP_TIMEOUT 0x80000000
> +
> +/*
> + * The mask defining which bits correspond to the number of sleep transitions,
> + * as well as the maximum number of suspend line transitions that will be
> + * reported back to the host.
> + */
> +#define EC_HOST_RESUME_SLEEP_TRANSITIONS_MASK 0x7FFFFFFF
> +
> +struct ec_response_host_sleep_event_v1 {
> +       union {
> +               /* Response fields that apply for resume messages. */
> +               struct {
> +                       /*
> +                        * The number of sleep power signal transitions that
> +                        * occurred since the suspend message. The high bit
> +                        * indicates a timeout occurred.
> +                        */
> +                       uint32_t sleep_transitions;
> +               } resume_response;
> +
> +               /* No response fields for non-resume messages. */
> +       };
> +} __packed;
> +
>  /*****************************************************************************/
>  /* Smart battery pass-through */
>
> --
> 2.20.1
>

Re: [PATCH v3 1/2] mfd: cros_ec: Add host_sleep_event_v1 command

[Rajat Jain]

On Wed, Apr 3, 2019 at 2:34 PM Evan Green <evgreen@chromium.org> wrote:
>
> Introduce the command and response structures for the second revision
> of the host sleep event. These structures are part of a new EC change
> that enables detection of failure to enter S0ix. The EC waits a
> kernel-specified timeout (or a default amount of time) for the S0_SLP
> pin to change, and wakes the system if that change does not occur in
> time.
>
> Signed-off-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Rajat Jain <rajatja@chromium.org>

>
> ---
>
> Changes in v3: None
> Changes in v2:
> - Made unions anonymous
> - Replaced reserved union members with a comment
>
>  include/linux/mfd/cros_ec_commands.h | 57 ++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>
> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> index fc91082d4c35..1cdb07c0ece1 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h
> @@ -2729,6 +2729,63 @@ struct ec_params_host_sleep_event {
>         uint8_t sleep_event;
>  } __packed;
>
> +/*
> + * Use a default timeout value (CONFIG_SLEEP_TIMEOUT_MS) for detecting sleep
> + * transition failures
> + */
> +#define EC_HOST_SLEEP_TIMEOUT_DEFAULT 0
> +
> +/* Disable timeout detection for this sleep transition */
> +#define EC_HOST_SLEEP_TIMEOUT_INFINITE 0xFFFF
> +
> +struct ec_params_host_sleep_event_v1 {
> +       /* The type of sleep being entered or exited. */
> +       uint8_t sleep_event;
> +
> +       /* Padding */
> +       uint8_t reserved;
> +       union {
> +               /* Parameters that apply for suspend messages. */
> +               struct {
> +                       /*
> +                        * The timeout in milliseconds between when this message
> +                        * is received and when the EC will declare sleep
> +                        * transition failure if the sleep signal is not
> +                        * asserted.
> +                        */
> +                       uint16_t sleep_timeout_ms;
> +               } suspend_params;
> +
> +               /* No parameters for non-suspend messages. */
> +       };
> +} __packed;
> +
> +/* A timeout occurred when this bit is set */
> +#define EC_HOST_RESUME_SLEEP_TIMEOUT 0x80000000
> +
> +/*
> + * The mask defining which bits correspond to the number of sleep transitions,
> + * as well as the maximum number of suspend line transitions that will be
> + * reported back to the host.
> + */
> +#define EC_HOST_RESUME_SLEEP_TRANSITIONS_MASK 0x7FFFFFFF
> +
> +struct ec_response_host_sleep_event_v1 {
> +       union {
> +               /* Response fields that apply for resume messages. */
> +               struct {
> +                       /*
> +                        * The number of sleep power signal transitions that
> +                        * occurred since the suspend message. The high bit
> +                        * indicates a timeout occurred.
> +                        */
> +                       uint32_t sleep_transitions;
> +               } resume_response;
> +
> +               /* No response fields for non-resume messages. */
> +       };
> +} __packed;
> +
>  /*****************************************************************************/
>  /* Smart battery pass-through */
>
> --
> 2.20.1
>

Re: [PATCH v3 1/2] mfd: cros_ec: Add host_sleep_event_v1 command

[Enric Balletbo Serra]

Missatge de Rajat Jain <rajatja@google.com> del dia dj., 4 d’abr. 2019
a les 20:40:
>
> On Wed, Apr 3, 2019 at 2:34 PM Evan Green <evgreen@chromium.org> wrote:
> >
> > Introduce the command and response structures for the second revision
> > of the host sleep event. These structures are part of a new EC change
> > that enables detection of failure to enter S0ix. The EC waits a
> > kernel-specified timeout (or a default amount of time) for the S0_SLP
> > pin to change, and wakes the system if that change does not occur in
> > time.
> >
> > Signed-off-by: Evan Green <evgreen@chromium.org>
> Reviewed-by: Rajat Jain <rajatja@chromium.org>

Acked-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

>
> >
> > ---
> >
> > Changes in v3: None
> > Changes in v2:
> > - Made unions anonymous
> > - Replaced reserved union members with a comment
> >
> >  include/linux/mfd/cros_ec_commands.h | 57 ++++++++++++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> >
> > diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> > index fc91082d4c35..1cdb07c0ece1 100644
> > --- a/include/linux/mfd/cros_ec_commands.h
> > +++ b/include/linux/mfd/cros_ec_commands.h
> > @@ -2729,6 +2729,63 @@ struct ec_params_host_sleep_event {
> >         uint8_t sleep_event;
> >  } __packed;
> >
> > +/*
> > + * Use a default timeout value (CONFIG_SLEEP_TIMEOUT_MS) for detecting sleep
> > + * transition failures
> > + */
> > +#define EC_HOST_SLEEP_TIMEOUT_DEFAULT 0
> > +
> > +/* Disable timeout detection for this sleep transition */
> > +#define EC_HOST_SLEEP_TIMEOUT_INFINITE 0xFFFF
> > +
> > +struct ec_params_host_sleep_event_v1 {
> > +       /* The type of sleep being entered or exited. */
> > +       uint8_t sleep_event;
> > +
> > +       /* Padding */
> > +       uint8_t reserved;
> > +       union {
> > +               /* Parameters that apply for suspend messages. */
> > +               struct {
> > +                       /*
> > +                        * The timeout in milliseconds between when this message
> > +                        * is received and when the EC will declare sleep
> > +                        * transition failure if the sleep signal is not
> > +                        * asserted.
> > +                        */
> > +                       uint16_t sleep_timeout_ms;
> > +               } suspend_params;
> > +
> > +               /* No parameters for non-suspend messages. */
> > +       };
> > +} __packed;
> > +
> > +/* A timeout occurred when this bit is set */
> > +#define EC_HOST_RESUME_SLEEP_TIMEOUT 0x80000000
> > +
> > +/*
> > + * The mask defining which bits correspond to the number of sleep transitions,
> > + * as well as the maximum number of suspend line transitions that will be
> > + * reported back to the host.
> > + */
> > +#define EC_HOST_RESUME_SLEEP_TRANSITIONS_MASK 0x7FFFFFFF
> > +
> > +struct ec_response_host_sleep_event_v1 {
> > +       union {
> > +               /* Response fields that apply for resume messages. */
> > +               struct {
> > +                       /*
> > +                        * The number of sleep power signal transitions that
> > +                        * occurred since the suspend message. The high bit
> > +                        * indicates a timeout occurred.
> > +                        */
> > +                       uint32_t sleep_transitions;
> > +               } resume_response;
> > +
> > +               /* No response fields for non-resume messages. */
> > +       };
> > +} __packed;
> > +
> >  /*****************************************************************************/
> >  /* Smart battery pass-through */
> >
> > --
> > 2.20.1
> >

Re: [PATCH v3 1/2] mfd: cros_ec: Add host_sleep_event_v1 command

[Lee Jones]

On Wed, 03 Apr 2019, Evan Green wrote:

> Introduce the command and response structures for the second revision
> of the host sleep event. These structures are part of a new EC change
> that enables detection of failure to enter S0ix. The EC waits a
> kernel-specified timeout (or a default amount of time) for the S0_SLP
> pin to change, and wakes the system if that change does not occur in
> time.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> 
> ---
> 
> Changes in v3: None
> Changes in v2:
> - Made unions anonymous
> - Replaced reserved union members with a comment
> 
>  include/linux/mfd/cros_ec_commands.h | 57 ++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Evan Green]

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_ONCE(). 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>

---

Changes in v3:
- Consolidated boolean logic for host_sleep_v1 (Guenter)

Changes in v2:
- Removed unnecessary version assignment (Guenter)
- Changed WARN to WARN_ONCE (Guenter)
- Fixed C code to use anonymous unions
- insize is only bigger for resume events.

 drivers/mfd/cros_ec.c                   | 39 +++++++++++++++++++++----
 drivers/platform/chrome/cros_ec_proto.c |  6 ++++
 include/linux/mfd/cros_ec.h             |  2 ++
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index 6acfe036d522..bd2bcdd4718b 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -75,20 +75,49 @@ 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.suspend_params.sleep_timeout_ms =
+				EC_HOST_SLEEP_TIMEOUT_DEFAULT;
+
+		buf.msg.outsize = sizeof(buf.u.req1);
+		if ((sleep_event == HOST_SLEEP_EVENT_S3_RESUME) ||
+		    (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME))
+			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.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_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);
+
+	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..52ca564a64e7 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -414,6 +414,12 @@ 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);
+	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

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

[Lee Jones]

On Wed, 03 Apr 2019, Evan Green 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_ONCE(). 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>
> 
> ---
> 
> Changes in v3:
> - Consolidated boolean logic for host_sleep_v1 (Guenter)
> 
> Changes in v2:
> - Removed unnecessary version assignment (Guenter)
> - Changed WARN to WARN_ONCE (Guenter)
> - Fixed C code to use anonymous unions
> - insize is only bigger for resume events.
> 
>  drivers/mfd/cros_ec.c                   | 39 +++++++++++++++++++++----
>  drivers/platform/chrome/cros_ec_proto.c |  6 ++++
>  include/linux/mfd/cros_ec.h             |  2 ++
>  3 files changed, 42 insertions(+), 5 deletions(-)

Looks okay in principle.

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Guenter Roeck]

On Wed, Apr 3, 2019 at 2:34 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_ONCE(). 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>
>
Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
>
> Changes in v3:
> - Consolidated boolean logic for host_sleep_v1 (Guenter)
>
> Changes in v2:
> - Removed unnecessary version assignment (Guenter)
> - Changed WARN to WARN_ONCE (Guenter)
> - Fixed C code to use anonymous unions
> - insize is only bigger for resume events.
>
>  drivers/mfd/cros_ec.c                   | 39 +++++++++++++++++++++----
>  drivers/platform/chrome/cros_ec_proto.c |  6 ++++
>  include/linux/mfd/cros_ec.h             |  2 ++
>  3 files changed, 42 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index 6acfe036d522..bd2bcdd4718b 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -75,20 +75,49 @@ 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.suspend_params.sleep_timeout_ms =
> +                               EC_HOST_SLEEP_TIMEOUT_DEFAULT;
> +
> +               buf.msg.outsize = sizeof(buf.u.req1);
> +               if ((sleep_event == HOST_SLEEP_EVENT_S3_RESUME) ||
> +                   (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME))
> +                       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.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_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);
> +
> +       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..52ca564a64e7 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -414,6 +414,12 @@ 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);
> +       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
>

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

[Rajat Jain]

On Wed, Apr 3, 2019 at 2:34 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_ONCE(). 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>
Reviewed-by: Rajat Jain <rajatja@chromium.org>

>
> ---
>
> Changes in v3:
> - Consolidated boolean logic for host_sleep_v1 (Guenter)
>
> Changes in v2:
> - Removed unnecessary version assignment (Guenter)
> - Changed WARN to WARN_ONCE (Guenter)
> - Fixed C code to use anonymous unions
> - insize is only bigger for resume events.
>
>  drivers/mfd/cros_ec.c                   | 39 +++++++++++++++++++++----
>  drivers/platform/chrome/cros_ec_proto.c |  6 ++++
>  include/linux/mfd/cros_ec.h             |  2 ++
>  3 files changed, 42 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index 6acfe036d522..bd2bcdd4718b 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -75,20 +75,49 @@ 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.suspend_params.sleep_timeout_ms =
> +                               EC_HOST_SLEEP_TIMEOUT_DEFAULT;
> +
> +               buf.msg.outsize = sizeof(buf.u.req1);
> +               if ((sleep_event == HOST_SLEEP_EVENT_S3_RESUME) ||
> +                   (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME))
> +                       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.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_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);
> +
> +       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..52ca564a64e7 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -414,6 +414,12 @@ 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);
> +       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
>

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

[Enric Balletbo Serra]

Missatge de Rajat Jain <rajatja@google.com> del dia dj., 4 d’abr. 2019
a les 20:41:
>
> On Wed, Apr 3, 2019 at 2:34 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_ONCE(). 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>
> Reviewed-by: Rajat Jain <rajatja@chromium.org>

Acked-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

>
> >
> > ---
> >
> > Changes in v3:
> > - Consolidated boolean logic for host_sleep_v1 (Guenter)
> >
> > Changes in v2:
> > - Removed unnecessary version assignment (Guenter)
> > - Changed WARN to WARN_ONCE (Guenter)
> > - Fixed C code to use anonymous unions
> > - insize is only bigger for resume events.
> >
> >  drivers/mfd/cros_ec.c                   | 39 +++++++++++++++++++++----
> >  drivers/platform/chrome/cros_ec_proto.c |  6 ++++

Lee, how do you want to manage this, do you want to create an
immutable branch? Or do you want I do?

Thanks,
 - Enric

> >  include/linux/mfd/cros_ec.h             |  2 ++
> >  3 files changed, 42 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> > index 6acfe036d522..bd2bcdd4718b 100644
> > --- a/drivers/mfd/cros_ec.c
> > +++ b/drivers/mfd/cros_ec.c
> > @@ -75,20 +75,49 @@ 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.suspend_params.sleep_timeout_ms =
> > +                               EC_HOST_SLEEP_TIMEOUT_DEFAULT;
> > +
> > +               buf.msg.outsize = sizeof(buf.u.req1);
> > +               if ((sleep_event == HOST_SLEEP_EVENT_S3_RESUME) ||
> > +                   (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME))
> > +                       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.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_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);
> > +
> > +       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..52ca564a64e7 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -414,6 +414,12 @@ 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);
> > +       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
> >

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

[Lee Jones]

On Wed, 03 Apr 2019, Evan Green 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_ONCE(). 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>
> 
> ---
> 
> Changes in v3:
> - Consolidated boolean logic for host_sleep_v1 (Guenter)
> 
> Changes in v2:
> - Removed unnecessary version assignment (Guenter)
> - Changed WARN to WARN_ONCE (Guenter)
> - Fixed C code to use anonymous unions
> - insize is only bigger for resume events.
> 
>  drivers/mfd/cros_ec.c                   | 39 +++++++++++++++++++++----
>  drivers/platform/chrome/cros_ec_proto.c |  6 ++++
>  include/linux/mfd/cros_ec.h             |  2 ++
>  3 files changed, 42 insertions(+), 5 deletions(-)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog