Re: [PATCH v4 06/17] platform: chrome: cros_ec: handle MKBP more events flag

[Jonathan Cameron]

On Tue,  5 Nov 2019 14:26:41 -0800
Gwendal Grignou <gwendal@xxxxxxxxxxxx> wrote:

> From: Enrico Granata <egranata@xxxxxxxxxxxx>
> 
> 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 <egranata@xxxxxxxxxxxx>
> Signed-off-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
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 <Jonathan.Cameron@xxxxxxxxxx>

> ---
> 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.
>   *