From: Enrico Granata <[email protected]>
The ChromeOS EC has support for signaling to the host that
a single IRQ can serve multiple MKBP (Matrix KeyBoard Protocol)
events.
Doing this serves an optimization purpose, as it minimizes the
number of round-trips into the interrupt handling machinery, and
it proves beneficial to sensor timestamping as it keeps the desired
synchronization of event times between the two processors.
This patch adds kernel support for this EC feature, allowing the
ec_irq to loop until all events have been served.
Signed-off-by: Enrico Granata <[email protected]>
Signed-off-by: Gwendal Grignou <[email protected]>
---
Changes in v4:
- Check patch with --strict option
Alignement
Changes in v3:
Fix indentation.
Changes in v2:
Process flag inside cros_ec_get_next_event, clean flag from event.
Introduce public function cros_ec_handle_event(), use it in rpmsg and
ishtp transport layer.
Remplace dev_info with dev_dbg, call only once.
drivers/platform/chrome/cros_ec.c | 34 +++++++--
drivers/platform/chrome/cros_ec_ishtp.c | 8 +-
drivers/platform/chrome/cros_ec_lpc.c | 15 +++-
drivers/platform/chrome/cros_ec_proto.c | 81 +++++++++++++--------
drivers/platform/chrome/cros_ec_rpmsg.c | 19 +----
include/linux/platform_data/cros_ec_proto.h | 12 ++-
6 files changed, 107 insertions(+), 62 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index d3dfa27171e6..c9f0b9ebcbc1 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -40,13 +40,23 @@ static irqreturn_t ec_irq_handler(int irq, void *data)
return IRQ_WAKE_THREAD;
}
-static irqreturn_t ec_irq_thread(int irq, void *data)
+/**
+ * cros_ec_handle_event - process and forward pending events on EC
+ * @ec_dev: Device with events to process.
+ *
+ * Call this function in a loop when the kernel is notified that the EC has
+ * pending events.
+ *
+ * Return: true if more events are still pending and this function should be
+ * called again.
+ */
+bool cros_ec_handle_event(struct cros_ec_device *ec_dev)
{
- struct cros_ec_device *ec_dev = data;
- bool wake_event = true;
+ bool wake_event;
+ bool ec_has_more_events;
int ret;
- ret = cros_ec_get_next_event(ec_dev, &wake_event);
+ ret = cros_ec_get_next_event(ec_dev, &wake_event, &ec_has_more_events);
/*
* Signal only if wake host events or any interrupt if
@@ -59,6 +69,20 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
if (ret > 0)
blocking_notifier_call_chain(&ec_dev->event_notifier,
0, ec_dev);
+
+ return ec_has_more_events;
+}
+EXPORT_SYMBOL(cros_ec_handle_event);
+
+static irqreturn_t ec_irq_thread(int irq, void *data)
+{
+ struct cros_ec_device *ec_dev = data;
+ bool ec_has_more_events;
+
+ do {
+ ec_has_more_events = cros_ec_handle_event(ec_dev);
+ } while (ec_has_more_events);
+
return IRQ_HANDLED;
}
@@ -274,7 +298,7 @@ EXPORT_SYMBOL(cros_ec_suspend);
static void cros_ec_report_events_during_suspend(struct cros_ec_device *ec_dev)
{
while (ec_dev->mkbp_event_supported &&
- cros_ec_get_next_event(ec_dev, NULL) > 0)
+ cros_ec_get_next_event(ec_dev, NULL, NULL) > 0)
blocking_notifier_call_chain(&ec_dev->event_notifier,
1, ec_dev);
}
diff --git a/drivers/platform/chrome/cros_ec_ishtp.c b/drivers/platform/chrome/cros_ec_ishtp.c
index 5c848f22b44b..e5996821d08b 100644
--- a/drivers/platform/chrome/cros_ec_ishtp.c
+++ b/drivers/platform/chrome/cros_ec_ishtp.c
@@ -136,11 +136,11 @@ static void ish_evt_handler(struct work_struct *work)
struct ishtp_cl_data *client_data =
container_of(work, struct ishtp_cl_data, work_ec_evt);
struct cros_ec_device *ec_dev = client_data->ec_dev;
+ bool ec_has_more_events;
- if (cros_ec_get_next_event(ec_dev, NULL) > 0) {
- blocking_notifier_call_chain(&ec_dev->event_notifier,
- 0, ec_dev);
- }
+ do {
+ ec_has_more_events = cros_ec_handle_event(ec_dev);
+ } while (ec_has_more_events);
}
/**
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index 3c77496e164d..dccf479c6625 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -312,13 +312,20 @@ static int cros_ec_lpc_readmem(struct cros_ec_device *ec, unsigned int offset,
static void cros_ec_lpc_acpi_notify(acpi_handle device, u32 value, void *data)
{
struct cros_ec_device *ec_dev = data;
+ bool ec_has_more_events;
+ int ret;
ec_dev->last_event_time = cros_ec_get_time_ns();
- if (ec_dev->mkbp_event_supported &&
- cros_ec_get_next_event(ec_dev, NULL) > 0)
- blocking_notifier_call_chain(&ec_dev->event_notifier, 0,
- ec_dev);
+ if (ec_dev->mkbp_event_supported)
+ do {
+ ret = cros_ec_get_next_event(ec_dev, NULL,
+ &ec_has_more_events);
+ if (ret > 0)
+ blocking_notifier_call_chain(
+ &ec_dev->event_notifier, 0,
+ ec_dev);
+ } while (ec_has_more_events);
if (value == ACPI_NOTIFY_DEVICE_WAKE)
pm_system_wakeup();
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index b502933e911b..03173ca66b1b 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -456,7 +456,10 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
if (ret < 0 || ver_mask == 0)
ec_dev->mkbp_event_supported = 0;
else
- ec_dev->mkbp_event_supported = 1;
+ ec_dev->mkbp_event_supported = fls(ver_mask);
+
+ dev_dbg(ec_dev->dev, "MKBP support version %u\n",
+ 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,
@@ -569,6 +572,7 @@ EXPORT_SYMBOL(cros_ec_cmd_xfer_status);
static int get_next_event_xfer(struct cros_ec_device *ec_dev,
struct cros_ec_command *msg,
+ struct ec_response_get_next_event_v1 *event,
int version, uint32_t size)
{
int ret;
@@ -581,7 +585,7 @@ static int get_next_event_xfer(struct cros_ec_device *ec_dev,
ret = cros_ec_cmd_xfer(ec_dev, msg);
if (ret > 0) {
ec_dev->event_size = ret - 1;
- memcpy(&ec_dev->event_data, msg->data, ret);
+ ec_dev->event_data = *event;
}
return ret;
@@ -589,30 +593,26 @@ static int get_next_event_xfer(struct cros_ec_device *ec_dev,
static int get_next_event(struct cros_ec_device *ec_dev)
{
- u8 buffer[sizeof(struct cros_ec_command) + sizeof(ec_dev->event_data)];
- struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
- static int cmd_version = 1;
- int ret;
+ struct {
+ struct cros_ec_command msg;
+ struct ec_response_get_next_event_v1 event;
+ } __packed buf;
+ struct cros_ec_command *msg = &buf.msg;
+ struct ec_response_get_next_event_v1 *event = &buf.event;
+ const int cmd_version = ec_dev->mkbp_event_supported - 1;
+ memset(msg, 0, sizeof(*msg));
if (ec_dev->suspended) {
dev_dbg(ec_dev->dev, "Device suspended.\n");
return -EHOSTDOWN;
}
- if (cmd_version == 1) {
- ret = get_next_event_xfer(ec_dev, msg, cmd_version,
- sizeof(struct ec_response_get_next_event_v1));
- if (ret < 0 || msg->result != EC_RES_INVALID_VERSION)
- return ret;
-
- /* Fallback to version 0 for future send attempts */
- cmd_version = 0;
- }
-
- ret = get_next_event_xfer(ec_dev, msg, cmd_version,
+ if (cmd_version == 0)
+ return get_next_event_xfer(ec_dev, msg, event, 0,
sizeof(struct ec_response_get_next_event));
- return ret;
+ return get_next_event_xfer(ec_dev, msg, event, cmd_version,
+ sizeof(struct ec_response_get_next_event_v1));
}
static int get_keyboard_state_event(struct cros_ec_device *ec_dev)
@@ -639,32 +639,55 @@ static int get_keyboard_state_event(struct cros_ec_device *ec_dev)
* @ec_dev: Device to fetch event from.
* @wake_event: Pointer to a bool set to true upon return if the event might be
* treated as a wake event. Ignored if null.
+ * @has_more_events: Pointer to bool set to true if more than one event is
+ * pending.
+ * Some EC will set this flag to indicate cros_ec_get_next_event()
+ * can be called multiple times in a row.
+ * It is an optimization to prevent issuing a EC command for
+ * nothing or wait for another interrupt from the EC to process
+ * the next message.
+ * Ignored if null.
*
* Return: negative error code on errors; 0 for no data; or else number of
* bytes received (i.e., an event was retrieved successfully). Event types are
* written out to @ec_dev->event_data.event_type on success.
*/
-int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event)
+int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
+ bool *wake_event,
+ bool *has_more_events)
{
u8 event_type;
u32 host_event;
int ret;
- if (!ec_dev->mkbp_event_supported) {
- ret = get_keyboard_state_event(ec_dev);
- if (ret <= 0)
- return ret;
+ /*
+ * Default value for wake_event.
+ * Wake up on keyboard event, wake up for spurious interrupt or link
+ * error to the EC.
+ */
+ if (wake_event)
+ *wake_event = true;
- if (wake_event)
- *wake_event = true;
+ /*
+ * Default value for has_more_events.
+ * EC will raise another interrupt if AP does not process all events
+ * anyway.
+ */
+ if (has_more_events)
+ *has_more_events = false;
- return ret;
- }
+ if (!ec_dev->mkbp_event_supported)
+ return get_keyboard_state_event(ec_dev);
ret = get_next_event(ec_dev);
if (ret <= 0)
return ret;
+ if (has_more_events)
+ *has_more_events = ec_dev->event_data.event_type &
+ EC_MKBP_HAS_MORE_EVENTS;
+ ec_dev->event_data.event_type &= EC_MKBP_EVENT_TYPE_MASK;
+
if (wake_event) {
event_type = ec_dev->event_data.event_type;
host_event = cros_ec_get_host_event(ec_dev);
@@ -679,11 +702,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event)
else if (host_event &&
!(host_event & ec_dev->host_event_wake_mask))
*wake_event = false;
- /* Consider all other events as wake events. */
- else
- *wake_event = true;
}
-
return ret;
}
EXPORT_SYMBOL(cros_ec_get_next_event);
diff --git a/drivers/platform/chrome/cros_ec_rpmsg.c b/drivers/platform/chrome/cros_ec_rpmsg.c
index 0c3738c3244d..bd068afe43b5 100644
--- a/drivers/platform/chrome/cros_ec_rpmsg.c
+++ b/drivers/platform/chrome/cros_ec_rpmsg.c
@@ -143,22 +143,11 @@ cros_ec_rpmsg_host_event_function(struct work_struct *host_event_work)
struct cros_ec_rpmsg,
host_event_work);
struct cros_ec_device *ec_dev = dev_get_drvdata(&ec_rpmsg->rpdev->dev);
- bool wake_event = true;
- int ret;
-
- ret = cros_ec_get_next_event(ec_dev, &wake_event);
-
- /*
- * Signal only if wake host events or any interrupt if
- * cros_ec_get_next_event() returned an error (default value for
- * wake_event is true)
- */
- if (wake_event && device_may_wakeup(ec_dev->dev))
- pm_wakeup_event(ec_dev->dev, 0);
+ bool ec_has_more_events;
- if (ret > 0)
- blocking_notifier_call_chain(&ec_dev->event_notifier,
- 0, ec_dev);
+ do {
+ ec_has_more_events = cros_ec_handle_event(ec_dev);
+ } while (ec_has_more_events);
}
static int cros_ec_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index b183024fef1f..e238930ae967 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -116,7 +116,9 @@ struct cros_ec_command {
* code.
* @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.
+ * @mkbp_event_supported: 0 if MKBP not supported. Otherwise its value is
+ * the maximum supported version of the MKBP host event
+ * command + 1.
* @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.
@@ -156,7 +158,7 @@ struct cros_ec_device {
int (*pkt_xfer)(struct cros_ec_device *ec,
struct cros_ec_command *msg);
struct mutex lock;
- bool mkbp_event_supported;
+ u8 mkbp_event_supported;
bool host_sleep_v1;
struct blocking_notifier_head event_notifier;
@@ -205,7 +207,9 @@ int cros_ec_unregister(struct cros_ec_device *ec_dev);
int cros_ec_query_all(struct cros_ec_device *ec_dev);
-int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event);
+int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
+ bool *wake_event,
+ bool *has_more_events);
u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
@@ -213,6 +217,8 @@ int cros_ec_check_features(struct cros_ec_dev *ec, int feature);
int cros_ec_get_sensor_count(struct cros_ec_dev *ec);
+bool cros_ec_handle_event(struct cros_ec_device *ec_dev);
+
/**
* cros_ec_get_time_ns - Return time in ns.
*
--
2.24.0.rc1.363.gb1bccd3e3d-goog
On Tue, 5 Nov 2019 14:26:41 -0800
Gwendal Grignou <[email protected]> wrote:
> From: Enrico Granata <[email protected]>
>
> The ChromeOS EC has support for signaling to the host that
> a single IRQ can serve multiple MKBP (Matrix KeyBoard Protocol)
> events.
>
> Doing this serves an optimization purpose, as it minimizes the
> number of round-trips into the interrupt handling machinery, and
> it proves beneficial to sensor timestamping as it keeps the desired
> synchronization of event times between the two processors.
>
> This patch adds kernel support for this EC feature, allowing the
> ec_irq to loop until all events have been served.
>
> Signed-off-by: Enrico Granata <[email protected]>
> Signed-off-by: Gwendal Grignou <[email protected]>
A couple of trivial things inline.
I just checked with the script and it doesn't seem to warn on the ()
but the documentation for kernel-doc suggests it should be there...
I guess things are more relaxed than I though.. Fix them if you are
doing another spin perhaps but don't bother otherwise.
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> Changes in v4:
> - Check patch with --strict option
> Alignement
> Changes in v3:
> Fix indentation.
> Changes in v2:
> Process flag inside cros_ec_get_next_event, clean flag from event.
> Introduce public function cros_ec_handle_event(), use it in rpmsg and
> ishtp transport layer.
> Remplace dev_info with dev_dbg, call only once.
>
> drivers/platform/chrome/cros_ec.c | 34 +++++++--
> drivers/platform/chrome/cros_ec_ishtp.c | 8 +-
> drivers/platform/chrome/cros_ec_lpc.c | 15 +++-
> drivers/platform/chrome/cros_ec_proto.c | 81 +++++++++++++--------
> drivers/platform/chrome/cros_ec_rpmsg.c | 19 +----
> include/linux/platform_data/cros_ec_proto.h | 12 ++-
> 6 files changed, 107 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index d3dfa27171e6..c9f0b9ebcbc1 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -40,13 +40,23 @@ static irqreturn_t ec_irq_handler(int irq, void *data)
> return IRQ_WAKE_THREAD;
> }
>
> -static irqreturn_t ec_irq_thread(int irq, void *data)
> +/**
> + * cros_ec_handle_event - process and forward pending events on EC
cros_ec_handle_event() -
> + * @ec_dev: Device with events to process.
> + *
> + * Call this function in a loop when the kernel is notified that the EC has
> + * pending events.
> + *
> + * Return: true if more events are still pending and this function should be
> + * called again.
> + */
> +bool cros_ec_handle_event(struct cros_ec_device *ec_dev)
> {
> - struct cros_ec_device *ec_dev = data;
> - bool wake_event = true;
> + bool wake_event;
> + bool ec_has_more_events;
> int ret;
>
> - ret = cros_ec_get_next_event(ec_dev, &wake_event);
> + ret = cros_ec_get_next_event(ec_dev, &wake_event, &ec_has_more_events);
>
> /*
> * Signal only if wake host events or any interrupt if
> @@ -59,6 +69,20 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
> if (ret > 0)
> blocking_notifier_call_chain(&ec_dev->event_notifier,
> 0, ec_dev);
> +
> + return ec_has_more_events;
> +}
> +EXPORT_SYMBOL(cros_ec_handle_event);
> +
> +static irqreturn_t ec_irq_thread(int irq, void *data)
> +{
> + struct cros_ec_device *ec_dev = data;
> + bool ec_has_more_events;
> +
> + do {
> + ec_has_more_events = cros_ec_handle_event(ec_dev);
> + } while (ec_has_more_events);
> +
> return IRQ_HANDLED;
> }
>
> @@ -274,7 +298,7 @@ EXPORT_SYMBOL(cros_ec_suspend);
> static void cros_ec_report_events_during_suspend(struct cros_ec_device *ec_dev)
> {
> while (ec_dev->mkbp_event_supported &&
> - cros_ec_get_next_event(ec_dev, NULL) > 0)
> + cros_ec_get_next_event(ec_dev, NULL, NULL) > 0)
> blocking_notifier_call_chain(&ec_dev->event_notifier,
> 1, ec_dev);
> }
> diff --git a/drivers/platform/chrome/cros_ec_ishtp.c b/drivers/platform/chrome/cros_ec_ishtp.c
> index 5c848f22b44b..e5996821d08b 100644
> --- a/drivers/platform/chrome/cros_ec_ishtp.c
> +++ b/drivers/platform/chrome/cros_ec_ishtp.c
> @@ -136,11 +136,11 @@ static void ish_evt_handler(struct work_struct *work)
> struct ishtp_cl_data *client_data =
> container_of(work, struct ishtp_cl_data, work_ec_evt);
> struct cros_ec_device *ec_dev = client_data->ec_dev;
> + bool ec_has_more_events;
>
> - if (cros_ec_get_next_event(ec_dev, NULL) > 0) {
> - blocking_notifier_call_chain(&ec_dev->event_notifier,
> - 0, ec_dev);
> - }
> + do {
> + ec_has_more_events = cros_ec_handle_event(ec_dev);
> + } while (ec_has_more_events);
> }
>
> /**
> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> index 3c77496e164d..dccf479c6625 100644
> --- a/drivers/platform/chrome/cros_ec_lpc.c
> +++ b/drivers/platform/chrome/cros_ec_lpc.c
> @@ -312,13 +312,20 @@ static int cros_ec_lpc_readmem(struct cros_ec_device *ec, unsigned int offset,
> static void cros_ec_lpc_acpi_notify(acpi_handle device, u32 value, void *data)
> {
> struct cros_ec_device *ec_dev = data;
> + bool ec_has_more_events;
> + int ret;
>
> ec_dev->last_event_time = cros_ec_get_time_ns();
>
> - if (ec_dev->mkbp_event_supported &&
> - cros_ec_get_next_event(ec_dev, NULL) > 0)
> - blocking_notifier_call_chain(&ec_dev->event_notifier, 0,
> - ec_dev);
> + if (ec_dev->mkbp_event_supported)
> + do {
> + ret = cros_ec_get_next_event(ec_dev, NULL,
> + &ec_has_more_events);
> + if (ret > 0)
> + blocking_notifier_call_chain(
> + &ec_dev->event_notifier, 0,
> + ec_dev);
> + } while (ec_has_more_events);
>
> if (value == ACPI_NOTIFY_DEVICE_WAKE)
> pm_system_wakeup();
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index b502933e911b..03173ca66b1b 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -456,7 +456,10 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> if (ret < 0 || ver_mask == 0)
> ec_dev->mkbp_event_supported = 0;
> else
> - ec_dev->mkbp_event_supported = 1;
> + ec_dev->mkbp_event_supported = fls(ver_mask);
> +
> + dev_dbg(ec_dev->dev, "MKBP support version %u\n",
> + 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,
> @@ -569,6 +572,7 @@ EXPORT_SYMBOL(cros_ec_cmd_xfer_status);
>
> static int get_next_event_xfer(struct cros_ec_device *ec_dev,
> struct cros_ec_command *msg,
> + struct ec_response_get_next_event_v1 *event,
> int version, uint32_t size)
> {
> int ret;
> @@ -581,7 +585,7 @@ static int get_next_event_xfer(struct cros_ec_device *ec_dev,
> ret = cros_ec_cmd_xfer(ec_dev, msg);
> if (ret > 0) {
> ec_dev->event_size = ret - 1;
> - memcpy(&ec_dev->event_data, msg->data, ret);
> + ec_dev->event_data = *event;
> }
>
> return ret;
> @@ -589,30 +593,26 @@ static int get_next_event_xfer(struct cros_ec_device *ec_dev,
>
> static int get_next_event(struct cros_ec_device *ec_dev)
> {
> - u8 buffer[sizeof(struct cros_ec_command) + sizeof(ec_dev->event_data)];
> - struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
> - static int cmd_version = 1;
> - int ret;
> + struct {
> + struct cros_ec_command msg;
> + struct ec_response_get_next_event_v1 event;
> + } __packed buf;
> + struct cros_ec_command *msg = &buf.msg;
> + struct ec_response_get_next_event_v1 *event = &buf.event;
> + const int cmd_version = ec_dev->mkbp_event_supported - 1;
>
> + memset(msg, 0, sizeof(*msg));
> if (ec_dev->suspended) {
> dev_dbg(ec_dev->dev, "Device suspended.\n");
> return -EHOSTDOWN;
> }
>
> - if (cmd_version == 1) {
> - ret = get_next_event_xfer(ec_dev, msg, cmd_version,
> - sizeof(struct ec_response_get_next_event_v1));
> - if (ret < 0 || msg->result != EC_RES_INVALID_VERSION)
> - return ret;
> -
> - /* Fallback to version 0 for future send attempts */
> - cmd_version = 0;
> - }
> -
> - ret = get_next_event_xfer(ec_dev, msg, cmd_version,
> + if (cmd_version == 0)
> + return get_next_event_xfer(ec_dev, msg, event, 0,
> sizeof(struct ec_response_get_next_event));
>
> - return ret;
> + return get_next_event_xfer(ec_dev, msg, event, cmd_version,
> + sizeof(struct ec_response_get_next_event_v1));
> }
>
> static int get_keyboard_state_event(struct cros_ec_device *ec_dev)
> @@ -639,32 +639,55 @@ static int get_keyboard_state_event(struct cros_ec_device *ec_dev)
> * @ec_dev: Device to fetch event from.
> * @wake_event: Pointer to a bool set to true upon return if the event might be
> * treated as a wake event. Ignored if null.
> + * @has_more_events: Pointer to bool set to true if more than one event is
> + * pending.
> + * Some EC will set this flag to indicate cros_ec_get_next_event()
> + * can be called multiple times in a row.
> + * It is an optimization to prevent issuing a EC command for
> + * nothing or wait for another interrupt from the EC to process
> + * the next message.
> + * Ignored if null.
> *
> * Return: negative error code on errors; 0 for no data; or else number of
> * bytes received (i.e., an event was retrieved successfully). Event types are
> * written out to @ec_dev->event_data.event_type on success.
> */
> -int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event)
> +int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
> + bool *wake_event,
> + bool *has_more_events)
> {
> u8 event_type;
> u32 host_event;
> int ret;
>
> - if (!ec_dev->mkbp_event_supported) {
> - ret = get_keyboard_state_event(ec_dev);
> - if (ret <= 0)
> - return ret;
> + /*
> + * Default value for wake_event.
> + * Wake up on keyboard event, wake up for spurious interrupt or link
> + * error to the EC.
> + */
> + if (wake_event)
> + *wake_event = true;
>
> - if (wake_event)
> - *wake_event = true;
> + /*
> + * Default value for has_more_events.
> + * EC will raise another interrupt if AP does not process all events
> + * anyway.
> + */
> + if (has_more_events)
> + *has_more_events = false;
>
> - return ret;
> - }
> + if (!ec_dev->mkbp_event_supported)
> + return get_keyboard_state_event(ec_dev);
>
> ret = get_next_event(ec_dev);
> if (ret <= 0)
> return ret;
>
> + if (has_more_events)
> + *has_more_events = ec_dev->event_data.event_type &
> + EC_MKBP_HAS_MORE_EVENTS;
> + ec_dev->event_data.event_type &= EC_MKBP_EVENT_TYPE_MASK;
> +
> if (wake_event) {
> event_type = ec_dev->event_data.event_type;
> host_event = cros_ec_get_host_event(ec_dev);
> @@ -679,11 +702,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event)
> else if (host_event &&
> !(host_event & ec_dev->host_event_wake_mask))
> *wake_event = false;
> - /* Consider all other events as wake events. */
> - else
> - *wake_event = true;
> }
> -
Nitpick. Unrelated whitespace change ;)
> return ret;
> }
> EXPORT_SYMBOL(cros_ec_get_next_event);
> diff --git a/drivers/platform/chrome/cros_ec_rpmsg.c b/drivers/platform/chrome/cros_ec_rpmsg.c
> index 0c3738c3244d..bd068afe43b5 100644
> --- a/drivers/platform/chrome/cros_ec_rpmsg.c
> +++ b/drivers/platform/chrome/cros_ec_rpmsg.c
> @@ -143,22 +143,11 @@ cros_ec_rpmsg_host_event_function(struct work_struct *host_event_work)
> struct cros_ec_rpmsg,
> host_event_work);
> struct cros_ec_device *ec_dev = dev_get_drvdata(&ec_rpmsg->rpdev->dev);
> - bool wake_event = true;
> - int ret;
> -
> - ret = cros_ec_get_next_event(ec_dev, &wake_event);
> -
> - /*
> - * Signal only if wake host events or any interrupt if
> - * cros_ec_get_next_event() returned an error (default value for
> - * wake_event is true)
> - */
> - if (wake_event && device_may_wakeup(ec_dev->dev))
> - pm_wakeup_event(ec_dev->dev, 0);
> + bool ec_has_more_events;
>
> - if (ret > 0)
> - blocking_notifier_call_chain(&ec_dev->event_notifier,
> - 0, ec_dev);
> + do {
> + ec_has_more_events = cros_ec_handle_event(ec_dev);
> + } while (ec_has_more_events);
> }
>
> static int cros_ec_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
> diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> index b183024fef1f..e238930ae967 100644
> --- a/include/linux/platform_data/cros_ec_proto.h
> +++ b/include/linux/platform_data/cros_ec_proto.h
> @@ -116,7 +116,9 @@ struct cros_ec_command {
> * code.
> * @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.
> + * @mkbp_event_supported: 0 if MKBP not supported. Otherwise its value is
> + * the maximum supported version of the MKBP host event
> + * command + 1.
> * @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.
> @@ -156,7 +158,7 @@ struct cros_ec_device {
> int (*pkt_xfer)(struct cros_ec_device *ec,
> struct cros_ec_command *msg);
> struct mutex lock;
> - bool mkbp_event_supported;
> + u8 mkbp_event_supported;
> bool host_sleep_v1;
> struct blocking_notifier_head event_notifier;
>
> @@ -205,7 +207,9 @@ int cros_ec_unregister(struct cros_ec_device *ec_dev);
>
> int cros_ec_query_all(struct cros_ec_device *ec_dev);
>
> -int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event);
> +int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
> + bool *wake_event,
> + bool *has_more_events);
>
> u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
>
> @@ -213,6 +217,8 @@ int cros_ec_check_features(struct cros_ec_dev *ec, int feature);
>
> int cros_ec_get_sensor_count(struct cros_ec_dev *ec);
>
> +bool cros_ec_handle_event(struct cros_ec_device *ec_dev);
> +
> /**
> * cros_ec_get_time_ns - Return time in ns.
> *
On 10/11/19 13:28, Jonathan Cameron wrote:
> On Tue, 5 Nov 2019 14:26:41 -0800
> Gwendal Grignou <[email protected]> wrote:
>
>> From: Enrico Granata <[email protected]>
>>
>> The ChromeOS EC has support for signaling to the host that
>> a single IRQ can serve multiple MKBP (Matrix KeyBoard Protocol)
>> events.
>>
>> Doing this serves an optimization purpose, as it minimizes the
>> number of round-trips into the interrupt handling machinery, and
>> it proves beneficial to sensor timestamping as it keeps the desired
>> synchronization of event times between the two processors.
>>
>> This patch adds kernel support for this EC feature, allowing the
>> ec_irq to loop until all events have been served.
>>
>> Signed-off-by: Enrico Granata <[email protected]>
>> Signed-off-by: Gwendal Grignou <[email protected]>
> A couple of trivial things inline.
> I just checked with the script and it doesn't seem to warn on the ()
> but the documentation for kernel-doc suggests it should be there...
>
> I guess things are more relaxed than I though.. Fix them if you are
> doing another spin perhaps but don't bother otherwise.
>
> Reviewed-by: Jonathan Cameron <[email protected]>
>
Looks good to me, please fix the above comments and:
Acked-by: Enric Balletbo i Serra <[email protected]>
Thanks,
Enric
>> ---
>> Changes in v4:
>> - Check patch with --strict option
>> Alignement
>> Changes in v3:
>> Fix indentation.
>> Changes in v2:
>> Process flag inside cros_ec_get_next_event, clean flag from event.
>> Introduce public function cros_ec_handle_event(), use it in rpmsg and
>> ishtp transport layer.
>> Remplace dev_info with dev_dbg, call only once.
>>
>> drivers/platform/chrome/cros_ec.c | 34 +++++++--
>> drivers/platform/chrome/cros_ec_ishtp.c | 8 +-
>> drivers/platform/chrome/cros_ec_lpc.c | 15 +++-
>> drivers/platform/chrome/cros_ec_proto.c | 81 +++++++++++++--------
>> drivers/platform/chrome/cros_ec_rpmsg.c | 19 +----
>> include/linux/platform_data/cros_ec_proto.h | 12 ++-
>> 6 files changed, 107 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
>> index d3dfa27171e6..c9f0b9ebcbc1 100644
>> --- a/drivers/platform/chrome/cros_ec.c
>> +++ b/drivers/platform/chrome/cros_ec.c
>> @@ -40,13 +40,23 @@ static irqreturn_t ec_irq_handler(int irq, void *data)
>> return IRQ_WAKE_THREAD;
>> }
>>
>> -static irqreturn_t ec_irq_thread(int irq, void *data)
>> +/**
>> + * cros_ec_handle_event - process and forward pending events on EC
>
> cros_ec_handle_event() -
>
>> + * @ec_dev: Device with events to process.
>> + *
>> + * Call this function in a loop when the kernel is notified that the EC has
>> + * pending events.
>> + *
>> + * Return: true if more events are still pending and this function should be
>> + * called again.
>> + */
>> +bool cros_ec_handle_event(struct cros_ec_device *ec_dev)
>> {
>> - struct cros_ec_device *ec_dev = data;
>> - bool wake_event = true;
>> + bool wake_event;
>> + bool ec_has_more_events;
>> int ret;
>>
>> - ret = cros_ec_get_next_event(ec_dev, &wake_event);
>> + ret = cros_ec_get_next_event(ec_dev, &wake_event, &ec_has_more_events);
>>
>> /*
>> * Signal only if wake host events or any interrupt if
>> @@ -59,6 +69,20 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
>> if (ret > 0)
>> blocking_notifier_call_chain(&ec_dev->event_notifier,
>> 0, ec_dev);
>> +
>> + return ec_has_more_events;
>> +}
>> +EXPORT_SYMBOL(cros_ec_handle_event);
>> +
>> +static irqreturn_t ec_irq_thread(int irq, void *data)
>> +{
>> + struct cros_ec_device *ec_dev = data;
>> + bool ec_has_more_events;
>> +
>> + do {
>> + ec_has_more_events = cros_ec_handle_event(ec_dev);
>> + } while (ec_has_more_events);
>> +
>> return IRQ_HANDLED;
>> }
>>
>> @@ -274,7 +298,7 @@ EXPORT_SYMBOL(cros_ec_suspend);
>> static void cros_ec_report_events_during_suspend(struct cros_ec_device *ec_dev)
>> {
>> while (ec_dev->mkbp_event_supported &&
>> - cros_ec_get_next_event(ec_dev, NULL) > 0)
>> + cros_ec_get_next_event(ec_dev, NULL, NULL) > 0)
>> blocking_notifier_call_chain(&ec_dev->event_notifier,
>> 1, ec_dev);
>> }
>> diff --git a/drivers/platform/chrome/cros_ec_ishtp.c b/drivers/platform/chrome/cros_ec_ishtp.c
>> index 5c848f22b44b..e5996821d08b 100644
>> --- a/drivers/platform/chrome/cros_ec_ishtp.c
>> +++ b/drivers/platform/chrome/cros_ec_ishtp.c
>> @@ -136,11 +136,11 @@ static void ish_evt_handler(struct work_struct *work)
>> struct ishtp_cl_data *client_data =
>> container_of(work, struct ishtp_cl_data, work_ec_evt);
>> struct cros_ec_device *ec_dev = client_data->ec_dev;
>> + bool ec_has_more_events;
>>
>> - if (cros_ec_get_next_event(ec_dev, NULL) > 0) {
>> - blocking_notifier_call_chain(&ec_dev->event_notifier,
>> - 0, ec_dev);
>> - }
>> + do {
>> + ec_has_more_events = cros_ec_handle_event(ec_dev);
>> + } while (ec_has_more_events);
>> }
>>
>> /**
>> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
>> index 3c77496e164d..dccf479c6625 100644
>> --- a/drivers/platform/chrome/cros_ec_lpc.c
>> +++ b/drivers/platform/chrome/cros_ec_lpc.c
>> @@ -312,13 +312,20 @@ static int cros_ec_lpc_readmem(struct cros_ec_device *ec, unsigned int offset,
>> static void cros_ec_lpc_acpi_notify(acpi_handle device, u32 value, void *data)
>> {
>> struct cros_ec_device *ec_dev = data;
>> + bool ec_has_more_events;
>> + int ret;
>>
>> ec_dev->last_event_time = cros_ec_get_time_ns();
>>
>> - if (ec_dev->mkbp_event_supported &&
>> - cros_ec_get_next_event(ec_dev, NULL) > 0)
>> - blocking_notifier_call_chain(&ec_dev->event_notifier, 0,
>> - ec_dev);
>> + if (ec_dev->mkbp_event_supported)
>> + do {
>> + ret = cros_ec_get_next_event(ec_dev, NULL,
>> + &ec_has_more_events);
>> + if (ret > 0)
>> + blocking_notifier_call_chain(
>> + &ec_dev->event_notifier, 0,
>> + ec_dev);
>> + } while (ec_has_more_events);
>>
>> if (value == ACPI_NOTIFY_DEVICE_WAKE)
>> pm_system_wakeup();
>> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
>> index b502933e911b..03173ca66b1b 100644
>> --- a/drivers/platform/chrome/cros_ec_proto.c
>> +++ b/drivers/platform/chrome/cros_ec_proto.c
>> @@ -456,7 +456,10 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
>> if (ret < 0 || ver_mask == 0)
>> ec_dev->mkbp_event_supported = 0;
>> else
>> - ec_dev->mkbp_event_supported = 1;
>> + ec_dev->mkbp_event_supported = fls(ver_mask);
>> +
>> + dev_dbg(ec_dev->dev, "MKBP support version %u\n",
>> + 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,
>> @@ -569,6 +572,7 @@ EXPORT_SYMBOL(cros_ec_cmd_xfer_status);
>>
>> static int get_next_event_xfer(struct cros_ec_device *ec_dev,
>> struct cros_ec_command *msg,
>> + struct ec_response_get_next_event_v1 *event,
>> int version, uint32_t size)
>> {
>> int ret;
>> @@ -581,7 +585,7 @@ static int get_next_event_xfer(struct cros_ec_device *ec_dev,
>> ret = cros_ec_cmd_xfer(ec_dev, msg);
>> if (ret > 0) {
>> ec_dev->event_size = ret - 1;
>> - memcpy(&ec_dev->event_data, msg->data, ret);
>> + ec_dev->event_data = *event;
>> }
>>
>> return ret;
>> @@ -589,30 +593,26 @@ static int get_next_event_xfer(struct cros_ec_device *ec_dev,
>>
>> static int get_next_event(struct cros_ec_device *ec_dev)
>> {
>> - u8 buffer[sizeof(struct cros_ec_command) + sizeof(ec_dev->event_data)];
>> - struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
>> - static int cmd_version = 1;
>> - int ret;
>> + struct {
>> + struct cros_ec_command msg;
>> + struct ec_response_get_next_event_v1 event;
>> + } __packed buf;
>> + struct cros_ec_command *msg = &buf.msg;
>> + struct ec_response_get_next_event_v1 *event = &buf.event;
>> + const int cmd_version = ec_dev->mkbp_event_supported - 1;
>>
>> + memset(msg, 0, sizeof(*msg));
>> if (ec_dev->suspended) {
>> dev_dbg(ec_dev->dev, "Device suspended.\n");
>> return -EHOSTDOWN;
>> }
>>
>> - if (cmd_version == 1) {
>> - ret = get_next_event_xfer(ec_dev, msg, cmd_version,
>> - sizeof(struct ec_response_get_next_event_v1));
>> - if (ret < 0 || msg->result != EC_RES_INVALID_VERSION)
>> - return ret;
>> -
>> - /* Fallback to version 0 for future send attempts */
>> - cmd_version = 0;
>> - }
>> -
>> - ret = get_next_event_xfer(ec_dev, msg, cmd_version,
>> + if (cmd_version == 0)
>> + return get_next_event_xfer(ec_dev, msg, event, 0,
>> sizeof(struct ec_response_get_next_event));
>>
>> - return ret;
>> + return get_next_event_xfer(ec_dev, msg, event, cmd_version,
>> + sizeof(struct ec_response_get_next_event_v1));
>> }
>>
>> static int get_keyboard_state_event(struct cros_ec_device *ec_dev)
>> @@ -639,32 +639,55 @@ static int get_keyboard_state_event(struct cros_ec_device *ec_dev)
>> * @ec_dev: Device to fetch event from.
>> * @wake_event: Pointer to a bool set to true upon return if the event might be
>> * treated as a wake event. Ignored if null.
>> + * @has_more_events: Pointer to bool set to true if more than one event is
>> + * pending.
>> + * Some EC will set this flag to indicate cros_ec_get_next_event()
>> + * can be called multiple times in a row.
>> + * It is an optimization to prevent issuing a EC command for
>> + * nothing or wait for another interrupt from the EC to process
>> + * the next message.
>> + * Ignored if null.
>> *
>> * Return: negative error code on errors; 0 for no data; or else number of
>> * bytes received (i.e., an event was retrieved successfully). Event types are
>> * written out to @ec_dev->event_data.event_type on success.
>> */
>> -int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event)
>> +int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
>> + bool *wake_event,
>> + bool *has_more_events)
>> {
>> u8 event_type;
>> u32 host_event;
>> int ret;
>>
>> - if (!ec_dev->mkbp_event_supported) {
>> - ret = get_keyboard_state_event(ec_dev);
>> - if (ret <= 0)
>> - return ret;
>> + /*
>> + * Default value for wake_event.
>> + * Wake up on keyboard event, wake up for spurious interrupt or link
>> + * error to the EC.
>> + */
>> + if (wake_event)
>> + *wake_event = true;
>>
>> - if (wake_event)
>> - *wake_event = true;
>> + /*
>> + * Default value for has_more_events.
>> + * EC will raise another interrupt if AP does not process all events
>> + * anyway.
>> + */
>> + if (has_more_events)
>> + *has_more_events = false;
>>
>> - return ret;
>> - }
>> + if (!ec_dev->mkbp_event_supported)
>> + return get_keyboard_state_event(ec_dev);
>>
>> ret = get_next_event(ec_dev);
>> if (ret <= 0)
>> return ret;
>>
>> + if (has_more_events)
>> + *has_more_events = ec_dev->event_data.event_type &
>> + EC_MKBP_HAS_MORE_EVENTS;
>> + ec_dev->event_data.event_type &= EC_MKBP_EVENT_TYPE_MASK;
>> +
>> if (wake_event) {
>> event_type = ec_dev->event_data.event_type;
>> host_event = cros_ec_get_host_event(ec_dev);
>> @@ -679,11 +702,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event)
>> else if (host_event &&
>> !(host_event & ec_dev->host_event_wake_mask))
>> *wake_event = false;
>> - /* Consider all other events as wake events. */
>> - else
>> - *wake_event = true;
>> }
>> -
>
> Nitpick. Unrelated whitespace change ;)
>
>> return ret;
>> }
>> EXPORT_SYMBOL(cros_ec_get_next_event);
>> diff --git a/drivers/platform/chrome/cros_ec_rpmsg.c b/drivers/platform/chrome/cros_ec_rpmsg.c
>> index 0c3738c3244d..bd068afe43b5 100644
>> --- a/drivers/platform/chrome/cros_ec_rpmsg.c
>> +++ b/drivers/platform/chrome/cros_ec_rpmsg.c
>> @@ -143,22 +143,11 @@ cros_ec_rpmsg_host_event_function(struct work_struct *host_event_work)
>> struct cros_ec_rpmsg,
>> host_event_work);
>> struct cros_ec_device *ec_dev = dev_get_drvdata(&ec_rpmsg->rpdev->dev);
>> - bool wake_event = true;
>> - int ret;
>> -
>> - ret = cros_ec_get_next_event(ec_dev, &wake_event);
>> -
>> - /*
>> - * Signal only if wake host events or any interrupt if
>> - * cros_ec_get_next_event() returned an error (default value for
>> - * wake_event is true)
>> - */
>> - if (wake_event && device_may_wakeup(ec_dev->dev))
>> - pm_wakeup_event(ec_dev->dev, 0);
>> + bool ec_has_more_events;
>>
>> - if (ret > 0)
>> - blocking_notifier_call_chain(&ec_dev->event_notifier,
>> - 0, ec_dev);
>> + do {
>> + ec_has_more_events = cros_ec_handle_event(ec_dev);
>> + } while (ec_has_more_events);
>> }
>>
>> static int cros_ec_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
>> diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
>> index b183024fef1f..e238930ae967 100644
>> --- a/include/linux/platform_data/cros_ec_proto.h
>> +++ b/include/linux/platform_data/cros_ec_proto.h
>> @@ -116,7 +116,9 @@ struct cros_ec_command {
>> * code.
>> * @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.
>> + * @mkbp_event_supported: 0 if MKBP not supported. Otherwise its value is
>> + * the maximum supported version of the MKBP host event
>> + * command + 1.
>> * @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.
>> @@ -156,7 +158,7 @@ struct cros_ec_device {
>> int (*pkt_xfer)(struct cros_ec_device *ec,
>> struct cros_ec_command *msg);
>> struct mutex lock;
>> - bool mkbp_event_supported;
>> + u8 mkbp_event_supported;
>> bool host_sleep_v1;
>> struct blocking_notifier_head event_notifier;
>>
>> @@ -205,7 +207,9 @@ int cros_ec_unregister(struct cros_ec_device *ec_dev);
>>
>> int cros_ec_query_all(struct cros_ec_device *ec_dev);
>>
>> -int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event);
>> +int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
>> + bool *wake_event,
>> + bool *has_more_events);
>>
>> u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
>>
>> @@ -213,6 +217,8 @@ int cros_ec_check_features(struct cros_ec_dev *ec, int feature);
>>
>> int cros_ec_get_sensor_count(struct cros_ec_dev *ec);
>>
>> +bool cros_ec_handle_event(struct cros_ec_device *ec_dev);
>> +
>> /**
>> * cros_ec_get_time_ns - Return time in ns.
>> *
>