Date:   Thu, 28 Mar 2019 12:15:56 +0100
From:   Enric Balletbo Serra <eballetbo@...il.com>
To:     Peter Shih <pihsun@...omium.org>
Cc:     Benson Leung <bleung@...omium.org>,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Guenter Roeck <groeck@...omium.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v7 6/7] platform/chrome: cros_ec: add EC host command
 support using rpmsg.

Hi

Thanks for sending this upstream, Some few comments and questions
below. Apart from these LGTM.

Missatge de Peter Shih <pihsun@...omium.org> del dia dc., 27 de març
2019 a les 6:17:
>
> From: Pi-Hsun Shih <pihsun@...omium.org>
>
> Add EC host command support through rpmsg.
>
> Signed-off-by: Pi-Hsun Shih <pihsun@...omium.org>
> ---
> Changes from v6:
>  - Make data for response aligned to 4 bytes.
>
> Changes from v5:
>  - Change commit title.
>  - Add documents for some structs, and fix all warning from
>    scripts/kernel-doc.
>  - Miscellaneous fixes based on feedback.
>
> Changes from v4:
>  - Change from work queue to completion.
>  - Change from matching using rpmsg id to device tree compatible, to
>    support EC subdevices.
>
> Changes from v3:
>  - Add host event support by adding an extra bytes at the start of IPC
>    message to indicate the type of the message (host event or host
>    command), since there's no additional irq that can be used for host
>    event.
>
> Changes from v2:
>  - Wait for ipi ack instead of depends on the behavior in mtk-rpmsg.
>
> Changes from v1:
>  - Code format fix based on feedback for cros_ec_rpmsg.c.
>  - Extract feature detection for SCP into separate patch (Patch 6).
> ---
>  drivers/platform/chrome/Kconfig         |   9 +
>  drivers/platform/chrome/Makefile        |   1 +
>  drivers/platform/chrome/cros_ec_rpmsg.c | 265 ++++++++++++++++++++++++
>  3 files changed, 275 insertions(+)
>  create mode 100644 drivers/platform/chrome/cros_ec_rpmsg.c
>
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index 9186d81a51cc..5c48aa6da2f8 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -59,6 +59,15 @@ config CROS_EC_I2C
>           a checksum. Failing accesses will be retried three times to
>           improve reliability.
>
> +config CROS_EC_RPMSG
> +       tristate "ChromeOS Embedded Controller (rpmsg)"
> +       depends on MFD_CROS_EC && RPMSG && OF
> +       help
> +         If you say Y here, you get support for talking to the ChromeOS EC
> +         through rpmsg. This uses a simple byte-level protocol with a
> +         checksum. Also since there's no addition EC-to-host interrupt, this
> +         use a byte in message to distinguish host event from host command.
> +
>  config CROS_EC_SPI
>         tristate "ChromeOS Embedded Controller (SPI)"
>         depends on MFD_CROS_EC && SPI
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index 1e2f0029b597..4b69d795720d 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -4,6 +4,7 @@ obj-$(CONFIG_CHROMEOS_LAPTOP)           += chromeos_laptop.o
>  obj-$(CONFIG_CHROMEOS_PSTORE)          += chromeos_pstore.o
>  obj-$(CONFIG_CHROMEOS_TBMC)            += chromeos_tbmc.o
>  obj-$(CONFIG_CROS_EC_I2C)              += cros_ec_i2c.o
> +obj-$(CONFIG_CROS_EC_RPMSG)            += cros_ec_rpmsg.o
>  obj-$(CONFIG_CROS_EC_SPI)              += cros_ec_spi.o
>  cros_ec_lpcs-objs                      := cros_ec_lpc.o cros_ec_lpc_reg.o
>  cros_ec_lpcs-$(CONFIG_CROS_EC_LPC_MEC) += cros_ec_lpc_mec.o
> diff --git a/drivers/platform/chrome/cros_ec_rpmsg.c b/drivers/platform/chrome/cros_ec_rpmsg.c
> new file mode 100644
> index 000000000000..2ecae806cfc5
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_ec_rpmsg.c
> @@ -0,0 +1,265 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright 2018 Google LLC.
> +
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mfd/cros_ec.h>
> +#include <linux/mfd/cros_ec_commands.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/rpmsg.h>
> +#include <linux/slab.h>
> +
> +#define EC_MSG_TIMEOUT_MS 200
> +#define HOST_COMMAND_MARK 1
> +#define HOST_EVENT_MARK 2
> +
> +/**
> + * struct cros_ec_rpmsg_response - rpmsg message format from from EC.
> + *
> + * @type:      The type of message, should be either HOST_COMMAND_MARK or
> + *             HOST_EVENT_MARK, representing that the message is a response to
> + *             host command, or a host event.
> + * @data:      ec_host_response for host command.
> + */
> +struct cros_ec_rpmsg_response {
> +       u8 type;
> +       u8 data[] __aligned(4);
> +};
> +
> +/**
> + * struct cros_ec_rpmsg - information about a EC over rpmsg.
> + *
> + * @rpdev:     rpmsg device we are connected to
> + * @xfer_ack:  completion for host command transfer.
> + * @host_event_work:   Work struct for pending host event.
> + */
> +struct cros_ec_rpmsg {
> +       struct rpmsg_device *rpdev;
> +       struct completion xfer_ack;
> +       struct work_struct host_event_work;
> +};
> +
> +/**
> + * cros_ec_cmd_xfer_rpmsg - Transfer a message over rpmsg and receive the reply
> + *
> + * @ec_dev: ChromeOS EC device
> + * @ec_msg: Message to transfer
> + *
> + * This is only used for old EC proto version, and is not supported for this
> + * driver.
> + *
> + * Return: number of bytes of the reply on success or negative error code.
> + */
> +static int cros_ec_cmd_xfer_rpmsg(struct cros_ec_device *ec_dev,
> +                                 struct cros_ec_command *ec_msg)
> +{
> +       return -EINVAL;
> +}
> +
> +/**
> + * cros_ec_pkt_xfer_rpmsg - Transfer a packet over rpmsg and receive the reply
> + *
> + * @ec_dev: ChromeOS EC device
> + * @ec_msg: Message to transfer
> + *
> + * Return: number of bytes of the reply on success or negative error code.
> + */
> +static int cros_ec_pkt_xfer_rpmsg(struct cros_ec_device *ec_dev,
> +                                 struct cros_ec_command *ec_msg)
> +{
> +       struct ec_host_response *response;
> +       struct cros_ec_rpmsg *ec_rpmsg = ec_dev->priv;
> +       struct rpmsg_device *rpdev = ec_rpmsg->rpdev;
> +       int len;
> +       u8 sum;
> +       int ret;
> +       int i;
> +       unsigned long timeout;
> +
> +       ec_msg->result = 0;
> +       len = cros_ec_prepare_tx(ec_dev, ec_msg);
> +       dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);
> +
> +       reinit_completion(&ec_rpmsg->xfer_ack);
> +       ret = rpmsg_send(rpdev->ept, ec_dev->dout, len);
> +       if (ret) {
> +               dev_err(ec_dev->dev, "rpmsg send failed\n");
> +               return ret;
> +       }
> +
> +       timeout = msecs_to_jiffies(EC_MSG_TIMEOUT_MS);
> +       ret = wait_for_completion_timeout(&ec_rpmsg->xfer_ack, timeout);
> +       if (!ret) {
> +               dev_err(ec_dev->dev, "rpmsg send timeout\n");
> +               return -EIO;
> +       }
> +
> +       /* check response error code */
> +       response = (struct ec_host_response *)ec_dev->din;
> +       ec_msg->result = response->result;
> +
> +       ret = cros_ec_check_result(ec_dev, ec_msg);
> +       if (ret)
> +               goto exit;
> +
> +       if (response->data_len > ec_msg->insize) {
> +               dev_err(ec_dev->dev, "packet too long (%d bytes, expected %d)",
> +                       response->data_len, ec_msg->insize);
> +               ret = -EMSGSIZE;
> +               goto exit;
> +       }
> +
> +       /* copy response packet payload and compute checksum */
> +       memcpy(ec_msg->data, ec_dev->din + sizeof(*response),
> +              response->data_len);
> +
> +       sum = 0;
> +       for (i = 0; i < sizeof(*response) + response->data_len; i++)
> +               sum += ec_dev->din[i];
> +
> +       if (sum) {
> +               dev_err(ec_dev->dev, "bad packet checksum, calculated %x\n",
> +                       sum);
> +               ret = -EBADMSG;
> +               goto exit;
> +       }
> +
> +       ret = response->data_len;
> +exit:
> +       if (ec_msg->command == EC_CMD_REBOOT_EC)
> +               msleep(EC_REBOOT_DELAY_MS);
> +
> +       return ret;
> +}
> +
> +static void
> +cros_ec_rpmsg_host_event_function(struct work_struct *host_event_work)
> +{
> +       struct cros_ec_rpmsg *ec_rpmsg = container_of(
> +               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);
> +
> +       if (ret > 0)
> +               blocking_notifier_call_chain(&ec_dev->event_notifier,
> +                                            0, ec_dev);
> +}
> +
> +static int cros_ec_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
> +                                 int len, void *priv, u32 src)
> +{
> +       struct cros_ec_device *ec_dev = dev_get_drvdata(&rpdev->dev);
> +       struct cros_ec_rpmsg *ec_rpmsg = ec_dev->priv;
> +       struct cros_ec_rpmsg_response *resp;
> +
> +       if (!len) {
> +               dev_warn(ec_dev->dev, "rpmsg received empty response");

Is this is unlikely to happen?

> +               return -EINVAL;
> +       }
> +
> +       resp = data;
> +       len -= offsetof(struct cros_ec_rpmsg_response, data);
> +       if (resp->type == HOST_COMMAND_MARK) {
> +               if (len > ec_dev->din_size) {
> +                       dev_warn(
> +                               ec_dev->dev,
> +                               "received length %d > din_size %d, truncating",
> +                               len, ec_dev->din_size);

How often this warning appears? Should be this a dev_dbg?

> +                       len = ec_dev->din_size;
> +               }
> +
> +               memcpy(ec_dev->din, resp->data, len);
> +               complete(&ec_rpmsg->xfer_ack);
> +       } else if (resp->type == HOST_EVENT_MARK) {
> +               schedule_work(&ec_rpmsg->host_event_work);
> +       } else {
> +               dev_warn(ec_dev->dev, "rpmsg received invalid type = %d",
> +                        resp->type);

Is this is unlikely to happen?

> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int cros_ec_rpmsg_probe(struct rpmsg_device *rpdev)
> +{
> +       struct device *dev = &rpdev->dev;
> +       struct cros_ec_device *ec_dev;
> +       struct cros_ec_rpmsg *ec_rpmsg;
> +       int ret;
> +
> +       ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
> +       if (!ec_dev)
> +               return -ENOMEM;
> +
> +       ec_rpmsg = devm_kzalloc(dev, sizeof(*ec_rpmsg), GFP_KERNEL);
> +       if (!ec_rpmsg)
> +               return -ENOMEM;
> +
> +       ec_dev->dev = dev;
> +       ec_dev->priv = ec_rpmsg;
> +       ec_dev->cmd_xfer = cros_ec_cmd_xfer_rpmsg;
> +       ec_dev->pkt_xfer = cros_ec_pkt_xfer_rpmsg;
> +       ec_dev->phys_name = dev_name(&rpdev->dev);
> +       ec_dev->din_size = sizeof(struct ec_host_response) +
> +                          sizeof(struct ec_response_get_protocol_info);
> +       ec_dev->dout_size = sizeof(struct ec_host_request);
> +       dev_set_drvdata(dev, ec_dev);
> +
> +       ec_rpmsg->rpdev = rpdev;
> +       init_completion(&ec_rpmsg->xfer_ack);
> +       INIT_WORK(&ec_rpmsg->host_event_work,
> +                 cros_ec_rpmsg_host_event_function);
> +
> +       ret = cros_ec_register(ec_dev);
> +       if (ret) {
> +               dev_err(dev, "cannot register EC\n");
> +               return ret;
> +       }
> +
> +       return 0;

cros_ec_register returns 0 on success and prints an error message if
something went wrong. Remove the above and just do:

return cros_ec_register(ec_dev);


> +}
> +
> +static void cros_ec_rpmsg_remove(struct rpmsg_device *rpdev)
> +{
> +       struct cros_ec_device *ec_dev = dev_get_drvdata(&rpdev->dev);
> +       struct cros_ec_rpmsg *ec_rpmsg = ec_dev->priv;
> +
> +       cancel_work_sync(&ec_rpmsg->host_event_work);
> +}
> +
> +static const struct of_device_id cros_ec_rpmsg_of_match[] = {
> +       { .compatible = "google,cros-ec-rpmsg", },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, cros_ec_rpmsg_of_match);
> +
> +static struct rpmsg_driver cros_ec_driver_rpmsg = {
> +       .drv = {
> +               .name   = KBUILD_MODNAME,
> +               .of_match_table = cros_ec_rpmsg_of_match,
> +       },
> +       .probe          = cros_ec_rpmsg_probe,
> +       .remove         = cros_ec_rpmsg_remove,
> +       .callback       = cros_ec_rpmsg_callback,
> +};
> +
> +module_rpmsg_driver(cros_ec_driver_rpmsg);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("ChromeOS EC multi function device (rpmsg)");
> --
> 2.21.0.392.gf8f6787159e-goog
>

Powered by blists - more mailing lists