From: Jonathan Cameron <jic23@kernel.org>
To: Gwendal Grignou <gwendal@chromium.org>
Cc: knaack.h@gmx.de, lars@metafoo.de, pmeerw@pmeerw.net,
lee.jones@linaro.org, bleung@chromium.org,
enric.balletbo@collabora.com, dianders@chromium.org,
groeck@chromium.org, fabien.lahoudere@collabora.com,
linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
Enrico Granata <egranata@chromium.org>
Subject: Re: [PATCH 06/13] platform: chrome: cros_ec: handle MKBP more events flag
Date: Sat, 5 Oct 2019 16:52:29 +0100 [thread overview]
Message-ID: <20191005165229.5f1cbd7e@archlinux> (raw)
In-Reply-To: <20190922175021.53449-7-gwendal@chromium.org>
On Sun, 22 Sep 2019 10:50:14 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:
> From: Enrico Granata <egranata@chromium.org>
>
> The ChromeOS EC has support for signaling to the host that
> a single IRQ can serve multiple MKBP events.
Be nice to those of us who don't know the system that well.
What are MKBP 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@chromium.org>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Minor comments inline.
> ---
> drivers/platform/chrome/cros_ec.c | 33 +++++++++----
> drivers/platform/chrome/cros_ec_proto.c | 51 ++++++++++++---------
> include/linux/platform_data/cros_ec_proto.h | 7 ++-
> 3 files changed, 57 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index 9c8dc7cdb2b7..4adc007c357c 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -46,25 +46,38 @@ static irqreturn_t ec_irq_handler(int irq, void *data)
> return IRQ_WAKE_THREAD;
> }
>
> -static irqreturn_t ec_irq_thread(int irq, void *data)
> +static bool ec_handle_event(struct cros_ec_device *ec_dev)
> {
> - struct cros_ec_device *ec_dev = data;
> bool wake_event = true;
> - int ret;
> + bool ec_has_more_events = false;
> + int ret = cros_ec_get_next_event(ec_dev, &wake_event);
>
> - ret = cros_ec_get_next_event(ec_dev, &wake_event);
Moving definition of int to this line just added noise to the
patch. I would leave that as it was to make reviewers life
ever so slightly simpler ;)
> + if (ec_dev->mkbp_event_supported) {
> + ec_has_more_events = (ret > 0) &&
> + (ec_dev->event_data.event_type &
> + EC_MKBP_HAS_MORE_EVENTS);
> + }
>
> - /*
> - * 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))
> + if (device_may_wakeup(ec_dev->dev) && wake_event)
> pm_wakeup_event(ec_dev->dev, 0);
>
> if (ret > 0)
> blocking_notifier_call_chain(&ec_dev->event_notifier,
> 0, ec_dev);
> +
> + return ec_has_more_events;
> +
Drop this blank line.
> +}
> +
> +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 = ec_handle_event(ec_dev);
> + } while (ec_has_more_events);
> +
> return IRQ_HANDLED;
> }
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index f659f96bda12..70e6d6c93b8d 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -425,10 +425,14 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> ret = cros_ec_get_host_command_version_mask(ec_dev,
> EC_CMD_GET_NEXT_EVENT,
> &ver_mask);
> - if (ret < 0 || ver_mask == 0)
> + if (ret < 0 || ver_mask == 0) {
> ec_dev->mkbp_event_supported = 0;
> - else
> - ec_dev->mkbp_event_supported = 1;
> + dev_info(ec_dev->dev, "MKBP not supported\n");
> + } else {
> + ec_dev->mkbp_event_supported = fls(ver_mask);
> + dev_info(ec_dev->dev, "MKBP support version %u\n",
> + ec_dev->mkbp_event_supported - 1);
dev_dbg perhaps?
> + }
>
> /* Probe if host sleep v1 is supported for S0ix failure detection. */
> ret = cros_ec_get_host_command_version_mask(ec_dev,
> @@ -519,6 +523,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;
> @@ -531,7 +536,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;
> @@ -539,30 +544,29 @@ 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;
Nice tidy up, but I don't think directly relevant to what this patch is doing..
I think that should be a precursor patch.
> + 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;
> +
> + BUILD_BUG_ON(sizeof(union ec_response_get_next_data_v1) != 16);
> +
> + memset(&buf, 0, sizeof(buf));
>
> 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)
> @@ -606,7 +610,8 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event)
> return ret;
>
> if (wake_event) {
> - event_type = ec_dev->event_data.event_type;
> + event_type =
> + ec_dev->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK;
> host_event = cros_ec_get_host_event(ec_dev);
>
> /*
> @@ -631,10 +636,12 @@ EXPORT_SYMBOL(cros_ec_get_next_event);
> u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev)
> {
> u32 host_event;
> + const u8 event_type =
> + ec_dev->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK;
>
> BUG_ON(!ec_dev->mkbp_event_supported);
>
> - if (ec_dev->event_data.event_type != EC_MKBP_EVENT_HOST_EVENT)
> + if (event_type != EC_MKBP_EVENT_HOST_EVENT)
> return 0;
>
> if (ec_dev->event_size != sizeof(host_event)) {
> diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> index ab12e28f2107..63b5597294e7 100644
> --- a/include/linux/platform_data/cros_ec_proto.h
> +++ b/include/linux/platform_data/cros_ec_proto.h
> @@ -115,7 +115,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.
> @@ -155,7 +157,8 @@ struct cros_ec_device {
> int (*pkt_xfer)(struct cros_ec_device *ec,
> struct cros_ec_command *msg);
> struct mutex lock;
> - bool mkbp_event_supported;
> + /* 0 == not supported, otherwise it supports version x - 1 */
> + u8 mkbp_event_supported;
> bool host_sleep_v1;
> struct blocking_notifier_head event_notifier;
>
next prev parent reply other threads:[~2019-10-05 15:52 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-22 17:50 [PATCH 00/13] cros_ec: Add sensorhub driver and FIFO processing Gwendal Grignou
2019-09-22 17:50 ` [PATCH 01/13] mfd: cros_ec: Add sensor_count and make check_features public Gwendal Grignou
2019-09-30 13:15 ` Enric Balletbo i Serra
2019-09-30 16:24 ` Gwendal Grignou
2019-10-05 15:26 ` Jonathan Cameron
2019-09-22 17:50 ` [PATCH 02/13] platform: cros_ec: Add cros_ec_sensor_hub driver Gwendal Grignou
2019-10-01 10:31 ` Enric Balletbo i Serra
2019-10-05 15:35 ` Jonathan Cameron
2019-10-05 15:36 ` Jonathan Cameron
2019-09-22 17:50 ` [PATCH 03/13] platform/mfd:iio: cros_ec: Register sensor through sensorhub Gwendal Grignou
2019-10-05 15:41 ` Jonathan Cameron
2019-09-22 17:50 ` [PATCH 04/13] platform: chrome: cros-ec: record event timestamp in the hard irq Gwendal Grignou
2019-10-01 10:32 ` Enric Balletbo i Serra
2019-10-05 15:44 ` Jonathan Cameron
2019-09-22 17:50 ` [PATCH 05/13] platform: chrome: cros_ec: Do not attempt to register a non-positive IRQ number Gwendal Grignou
2019-10-01 10:32 ` Enric Balletbo i Serra
2019-09-22 17:50 ` [PATCH 06/13] platform: chrome: cros_ec: handle MKBP more events flag Gwendal Grignou
2019-10-01 10:32 ` Enric Balletbo i Serra
2019-10-05 15:52 ` Jonathan Cameron [this message]
2019-09-22 17:50 ` [PATCH 07/13] platform: chrome: sensorhub: Add FIFO support Gwendal Grignou
2019-10-05 16:08 ` Jonathan Cameron
2019-10-05 16:14 ` Jonathan Cameron
2019-09-22 17:50 ` [PATCH 08/13] platform: chrome: sensorhub: Add code to spread timestmap Gwendal Grignou
2019-10-05 16:16 ` Jonathan Cameron
2019-09-22 17:50 ` [PATCH 09/13] platform: chrome: sensorhub: Add median filter Gwendal Grignou
2019-10-05 16:24 ` Jonathan Cameron
2019-09-22 17:50 ` [PATCH 10/13] iio: cros_ec: Use triggered buffer only when EC does not support FIFO Gwendal Grignou
2019-10-05 16:30 ` Jonathan Cameron
2019-09-22 17:50 ` [PATCH 11/13] iio: cros_ec: Expose hwfifo_timeout Gwendal Grignou
2019-10-05 16:35 ` Jonathan Cameron
2019-09-22 17:50 ` [PATCH 12/13] iio: cros_ec: Report hwfifo_watermark_max Gwendal Grignou
2019-10-05 16:37 ` Jonathan Cameron
2019-09-22 17:50 ` [PATCH 13/13] iio: cros_ec: Use Hertz as unit for sampling frequency Gwendal Grignou
2019-10-05 16:39 ` Jonathan Cameron
2019-10-05 15:39 ` [PATCH 00/13] cros_ec: Add sensorhub driver and FIFO processing Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191005165229.5f1cbd7e@archlinux \
--to=jic23@kernel.org \
--cc=bleung@chromium.org \
--cc=dianders@chromium.org \
--cc=egranata@chromium.org \
--cc=enric.balletbo@collabora.com \
--cc=fabien.lahoudere@collabora.com \
--cc=groeck@chromium.org \
--cc=gwendal@chromium.org \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=lee.jones@linaro.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pmeerw@pmeerw.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).