by Enric Balletbo Serra

[permalink] [raw]

Subject: Re: [RFC,5/5] mfd: cros_ec: add EC host command support using rpmsg.

Hi Peter,

Missatge de Peter Shih <[email protected]> del dia dv., 4 de gen.
2019 a les 8:58:
>
> Thanks for the review.
> I would leave some formatting comment to v2, and reply others first.
>
> On Fri, Jan 4, 2019 at 12:05 AM Enric Balletbo Serra
> <[email protected]> wrote:
> >
> > Hi,
> >
> > Many thanks for sending this. Please, add Guenter and me for next
> > versions, we are interested in it, thanks :)
> >
> > Missatge de Pi-Hsun Shih <[email protected]> del dia dc., 26 de des.
> > 2018 a les 8:57:
> > >
> > > Add EC host command support through rpmsg.
> > >
> > > Signed-off-by: Pi-Hsun Shih <[email protected]>
> > > ---
> > > drivers/mfd/cros_ec_dev.c | 9 ++
> > > drivers/platform/chrome/Kconfig | 8 ++
> > > drivers/platform/chrome/Makefile | 1 +
> > > drivers/platform/chrome/cros_ec_rpmsg.c | 164 ++++++++++++++++++++++++
> > > include/linux/mfd/cros_ec.h | 1 +
> > > include/linux/mfd/cros_ec_commands.h | 2 +
> > > 6 files changed, 185 insertions(+)
> > > create mode 100644 drivers/platform/chrome/cros_ec_rpmsg.c
> > >
> > > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > > index 2d0fee488c5aa8..67983853413d07 100644
> > > --- a/drivers/mfd/cros_ec_dev.c
> > > +++ b/drivers/mfd/cros_ec_dev.c
> > > @@ -414,6 +414,15 @@ static int ec_device_probe(struct platform_device *pdev)
> > > device_initialize(&ec->class_dev);
> > > cdev_init(&ec->cdev, &fops);
> > >
> > > + if (cros_ec_check_features(ec, EC_FEATURE_SCP)) {
> > > + dev_info(dev, "SCP detected.\n");
> > > + /*
> > > + * Help userspace differentiating ECs from SCP,
> > > + * regardless of the probing order.
> > > + */
> > > + ec_platform->ec_name = CROS_EC_DEV_SCP_NAME;
> > > + }
> > > +
> >
> > Why userspace should know that this is an SCP? From the userspace
> > point of view shouldn't be this transparent, we don't do distinctions
> > when the transport layer is i2c, spi or lpc, and I think that the
> > cros_ec_rpmsg driver is a cros-ec transport layer, like these. So, I
> > think that this is not needed.
> >
>
> Since both the EC and the SCP talk in EC host command format here, and they can
> both exist on the same system, if we don't do the distinction, both of them
> would be registered as /dev/cros_ec, and cause an error.
>

Interesting, so this system will have two cros-ec, one connected via
spi or i2c to the soc and another one using the M4 within the M8183?

Actually, on some systems, we have chained EC's (ie cros_ec and
cros_pd). The way we actually handle the name to access the different
ECs is create a mfd cell with their specific platform data, I am
wondering if we can do the same here (see drivers/mfd/cros_ec.c)

> This change is actually independent to the rpmsg change (EC through all
> transport layer can report that they have feature EC_FEATURE_SCP, and would
> then be seen from userspace as /dev/cros_scp), I'll move this to another patch
> in v2.
>
> > > /*
> > > * Add the class device
> > > * Link to the character device for creating the /dev entry
> > > diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> > > index 16b1615958aa2d..b03d68eb732177 100644
> > > --- a/drivers/platform/chrome/Kconfig
> > > +++ b/drivers/platform/chrome/Kconfig
> > > @@ -72,6 +72,14 @@ config CROS_EC_SPI
> > > response time cannot be guaranteed, we support ignoring
> > > 'pre-amble' bytes before the response actually starts.
> > >
> > > +config CROS_EC_RPMSG
> > > + tristate "ChromeOS Embedded Controller (rpmsg)"
> > > + depends on MFD_CROS_EC && RPMSG
> >
> > I think that this driver is DT-only, && OF ?
>
> Would add this in v2.
>
> >
> > > + 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.
> > > +
> > > config CROS_EC_LPC
> > > tristate "ChromeOS Embedded Controller (LPC)"
> > > depends on MFD_CROS_EC && ACPI && (X86 || COMPILE_TEST)
> > > diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> > > index cd591bf872bbe9..3e3190af2b50f4 100644
> > > --- a/drivers/platform/chrome/Makefile
> > > +++ b/drivers/platform/chrome/Makefile
> > > @@ -8,6 +8,7 @@ cros_ec_ctl-objs := cros_ec_sysfs.o cros_ec_lightbar.o \
> > > obj-$(CONFIG_CROS_EC_CTL) += cros_ec_ctl.o
> > > obj-$(CONFIG_CROS_EC_I2C) += cros_ec_i2c.o
> > > obj-$(CONFIG_CROS_EC_SPI) += cros_ec_spi.o
> > > +obj-$(CONFIG_CROS_EC_RPMSG) += cros_ec_rpmsg.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
> > > obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
> > > diff --git a/drivers/platform/chrome/cros_ec_rpmsg.c b/drivers/platform/chrome/cros_ec_rpmsg.c
> > > new file mode 100644
> > > index 00000000000000..f123ca6d1c029c
> > > --- /dev/null
> > > +++ b/drivers/platform/chrome/cros_ec_rpmsg.c
> > > @@ -0,0 +1,164 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +//
> > > +// Copyright 2018 Google LLC.
> > > +
> > > +#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>
> > > +
> > > +/**
> > > + * cros_ec_cmd_xfer_rpmsg - Transfer a message over rpmsg and receive the reply
> > > + *
> > > + * This is only used for old EC proto version, and is not supported for this
> > > + * driver.
> > > + *
> > > + * @ec_dev: ChromeOS EC device
> > > + * @ec_msg: Message to transfer
> > > + */
> > > +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
> > > + */
> > > +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 rpmsg_device *rpdev = ec_dev->priv;
> > > + int len;
> > > + u8 sum;
> > > + int ret;
> > > + int i;
> > > +
> > > + ec_msg->result = 0;
> > > + len = cros_ec_prepare_tx(ec_dev, ec_msg);
> > > + dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);
> > > +
> > > + // TODO: This currently relies on that mtk_rpmsg send actually blocks
> > > + // until ack. Should do the wait here instead.
> >
> > Use standard C style comments.
> >
> > > + ret = rpmsg_send(rpdev->ept, ec_dev->dout, len);
> > > +
> >
> > Remove that empty line.
> >
> > > + if (ret) {
> > > + dev_err(ec_dev->dev, "rpmsg send failed\n");
> > > + return ret;
> > > + }
> > > +
> > > + /* 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);
> >
> > Can you explain why this sleep is needed?
>
> From the comment of EC_CMD_REBOOT_EC: "The EC is unresponsive for a time after
> a reboot command. Add a simple delay to make sure that the bus stays locked."
>
> This is copied from other transport layer drivers, and probably not needed
> since we would reload the firmware for SCP while it's rebooting. I would test
> to see if this is needed when the reboot flow for SCP work as expected.
> (There's still some firmware work need to be done before it can be tested...)
>
> >
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +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);
> > > +
> > > + if (len > ec_dev->din_size) {
> > > + dev_warn(ec_dev->dev,
> > > + "ipi received length %d > din_size, truncating", len);
> > > + len = ec_dev->din_size;
> > > + }
> > > +
> > > + memcpy(ec_dev->din, data, len);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int cros_ec_rpmsg_probe(struct rpmsg_device *rpdev)
> > > +{
> > > + struct device *dev = &rpdev->dev;
> > > +
> > Remove that empty line
> >
> > > + struct cros_ec_device *ec_dev;
> > > + int ret;
> > > +
> > > + ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
> > > + if (!ec_dev)
> > > + return -ENOMEM;
> > > +
> > > + ec_dev->dev = dev;
> > > + ec_dev->priv = rpdev;
> > > + 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);
> > > +
> > > + ret = cros_ec_register(ec_dev);
> > > + if (ret)
> >
> > I'd add an error message here
> >
> > dev_err(dev, "cannot register EC\n"
> >
> > > + return ret;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void cros_ec_rpmsg_remove(struct rpmsg_device *rpdev)
> >
> > This function will not be needed after apply [1]. I would recommend
> > base your patches on top of [2]
> >
> > [1] https://lkml.org/lkml/2018/12/12/672
> > [2] https://lkml.org/lkml/2018/12/12/679
>
> Noted.
>
> >
> > > +{
> > > + struct cros_ec_device *ec_dev = dev_get_drvdata(&rpdev->dev);
> > > +
> > > + cros_ec_remove(ec_dev);
> > > +}
> > > +
> >
> > How this driver is instantiated?
> >
> > I expect something like this here (like the other transport layers)
> >
> > 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);
> >
> > And the DT containing the compatible = "google,cros-ec-rpmsg" like the
> > other cros-ec transport layers.
>
> This is a part that I'm getting quite confused on how to do properly.
> For SPI, a spi_device is created for each node listed under spi node in device
> tree.
> spi0 {
> compatible = "xxx-spi";
> cros_ec@0 {
> compatible = "google,cros-ec-spi";
> };
> }
>
> For rpmsg, the rpmsg_device are dynamically created from the request
> of the SCP, and then a matching rpmsg_driver is used when found.
> Currently without the cros-ec-rpmsg being in the device tree, the cros_ec_rpmsg
> module would need to be manually loaded by modprobe.
>
> To follow what SPI/I2C does, the device tree would look like:
> scp {
> compatible = "mediatek,mt8183-scp";
> mt8183-rpmsg {
> compatible = "mediatek,mt8183-rpmsg";
> cros_ec_rpmsg {
> compatible = "google,cros-ec-rpmsg";
> };
> };
> };
> But the rpmsg driver would not actually create those rpmsg_device on probe, but
> only look at those sub node and load the corresponding rpmsg_driver modules.
> When requested by SCP to create the rpmsg_device, it would find a matching
> rpmsg_driver independent on how the device tree looks.
>
> So my question is, should these dynamically created rpmsg_device be listed on
> device tree?
>

I think that right now that's our main problem, how to properly
instantiate all this stuff. One approach that I like is the one used
in the TI PRU ICSS, they create a pruss_soc_bus driver with the
purpose to allow the child nodes to be bound. I suspect that something
similar would work, but I need to look in more detail. See [1]

[1] https://lkml.org/lkml/2018/11/22/948

Cheers,
Enric

> >
> > > +static const struct rpmsg_device_id cros_ec_rpmsg_device_id[] = {
> > > + { .name = "cros-ec-rpmsg", },
> > > + { /* sentinel */ },
> >
> > I got convinced that the '/* sentinel */' comment doesn't means
> > anything, so use { } only here, remove the comment and the last comma
> > (there is nothing to separate)
> > + { }
> >
> > > +};
> > > +
> > > +static struct rpmsg_driver cros_ec_driver_rpmsg = {
> > > + .drv.name = KBUILD_MODNAME,
> >
> > And something like this here
> > .drv = {
> > .name = "cros-ec-rpmsg",
> > .of_match_table = cros_ec_rpmsg_of_match,
> > },
> >
> > > + .id_table = cros_ec_rpmsg_device_id,
> > > + .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)");
> > > diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> > > index de8b588c8776da..fd297cf8f97295 100644
> > > --- a/include/linux/mfd/cros_ec.h
> > > +++ b/include/linux/mfd/cros_ec.h
> > > @@ -24,6 +24,7 @@
> > >
> > > #define CROS_EC_DEV_NAME "cros_ec"
> > > #define CROS_EC_DEV_PD_NAME "cros_pd"
> > > +#define CROS_EC_DEV_SCP_NAME "cros_scp"
> >
> > I think this definition is not needed.
> >
> > >
> > > /*
> > > * The EC is unresponsive for a time after a reboot command. Add a
> > > diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> > > index fc91082d4c357b..3e5da6e93b2f42 100644
> > > --- a/include/linux/mfd/cros_ec_commands.h
> > > +++ b/include/linux/mfd/cros_ec_commands.h
> > > @@ -856,6 +856,8 @@ enum ec_feature_code {
> > > EC_FEATURE_RTC = 27,
> > > /* EC supports CEC commands */
> > > EC_FEATURE_CEC = 35,
> > > + /* The MCU exposes a SCP */
> > > + EC_FEATURE_SCP = 39,
> >
> > Same here, I think this is not needed.
> > > };
> > >
> > > #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32))
> > > --
> > > 2.20.1.415.g653613c723-goog
> > >
> >
> > Thanks,
> > Enric

2019-01-07 07:13:50

by Pi-Hsun Shih

[permalink] [raw]

Subject: Re: [RFC,5/5] mfd: cros_ec: add EC host command support using rpmsg.

On Fri, Jan 4, 2019 at 7:39 PM Enric Balletbo Serra <[email protected]> wrote:
>
> Hi Peter,
>
> Missatge de Peter Shih <[email protected]> del dia dv., 4 de gen.
> 2019 a les 8:58:
> >
> > Thanks for the review.
> > I would leave some formatting comment to v2, and reply others first.
> >
> > On Fri, Jan 4, 2019 at 12:05 AM Enric Balletbo Serra
> > <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > Many thanks for sending this. Please, add Guenter and me for next
> > > versions, we are interested in it, thanks :)
> > >
> > > Missatge de Pi-Hsun Shih <[email protected]> del dia dc., 26 de des.
> > > 2018 a les 8:57:
> > > >
> > > > Add EC host command support through rpmsg.
> > > >
> > > > Signed-off-by: Pi-Hsun Shih <[email protected]>
> > > > ---
> > > > drivers/mfd/cros_ec_dev.c | 9 ++
> > > > drivers/platform/chrome/Kconfig | 8 ++
> > > > drivers/platform/chrome/Makefile | 1 +
> > > > drivers/platform/chrome/cros_ec_rpmsg.c | 164 ++++++++++++++++++++++++
> > > > include/linux/mfd/cros_ec.h | 1 +
> > > > include/linux/mfd/cros_ec_commands.h | 2 +
> > > > 6 files changed, 185 insertions(+)
> > > > create mode 100644 drivers/platform/chrome/cros_ec_rpmsg.c
> > > >
> > > > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > > > index 2d0fee488c5aa8..67983853413d07 100644
> > > > --- a/drivers/mfd/cros_ec_dev.c
> > > > +++ b/drivers/mfd/cros_ec_dev.c
> > > > @@ -414,6 +414,15 @@ static int ec_device_probe(struct platform_device *pdev)
> > > > device_initialize(&ec->class_dev);
> > > > cdev_init(&ec->cdev, &fops);
> > > >
> > > > + if (cros_ec_check_features(ec, EC_FEATURE_SCP)) {
> > > > + dev_info(dev, "SCP detected.\n");
> > > > + /*
> > > > + * Help userspace differentiating ECs from SCP,
> > > > + * regardless of the probing order.
> > > > + */
> > > > + ec_platform->ec_name = CROS_EC_DEV_SCP_NAME;
> > > > + }
> > > > +
> > >
> > > Why userspace should know that this is an SCP? From the userspace
> > > point of view shouldn't be this transparent, we don't do distinctions
> > > when the transport layer is i2c, spi or lpc, and I think that the
> > > cros_ec_rpmsg driver is a cros-ec transport layer, like these. So, I
> > > think that this is not needed.
> > >
> >
> > Since both the EC and the SCP talk in EC host command format here, and they can
> > both exist on the same system, if we don't do the distinction, both of them
> > would be registered as /dev/cros_ec, and cause an error.
> >
>
> Interesting, so this system will have two cros-ec, one connected via
> spi or i2c to the soc and another one using the M4 within the M8183?
>
> Actually, on some systems, we have chained EC's (ie cros_ec and
> cros_pd). The way we actually handle the name to access the different
> ECs is create a mfd cell with their specific platform data, I am
> wondering if we can do the same here (see drivers/mfd/cros_ec.c)
>

Yes there's two cros-ec as described (one throught spi / i2c to a EC, one
through rpmsg to the M4 within M8183).

I think that what transport layer used (rpmsg / spi / i2c) is independent to
what the cros-ec actually is (a normal EC, or a SCP), so we probably still need
some feature detection to check what the cros-ec is. It seems to be hard to do
that on cros_ec_register in drivers/mfd/cros_ec.c using different mfd cell,
since it knows nothing about the EC features.

Or should I just don't do feature detection, but write the information in the
device tree instead? (Via some "dev-name" property probably?)

> > This change is actually independent to the rpmsg change (EC through all
> > transport layer can report that they have feature EC_FEATURE_SCP, and would
> > then be seen from userspace as /dev/cros_scp), I'll move this to another patch
> > in v2.
> >
> > > > /*
> > > > * Add the class device
> > > > * Link to the character device for creating the /dev entry
> > > > diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> > > > index 16b1615958aa2d..b03d68eb732177 100644
> > > > --- a/drivers/platform/chrome/Kconfig
> > > > +++ b/drivers/platform/chrome/Kconfig
> > > > @@ -72,6 +72,14 @@ config CROS_EC_SPI
> > > > response time cannot be guaranteed, we support ignoring
> > > > 'pre-amble' bytes before the response actually starts.
> > > >
> > > > +config CROS_EC_RPMSG
> > > > + tristate "ChromeOS Embedded Controller (rpmsg)"
> > > > + depends on MFD_CROS_EC && RPMSG
> > >
> > > I think that this driver is DT-only, && OF ?
> >
> > Would add this in v2.
> >
> > >
> > > > + 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.
> > > > +
> > > > config CROS_EC_LPC
> > > > tristate "ChromeOS Embedded Controller (LPC)"
> > > > depends on MFD_CROS_EC && ACPI && (X86 || COMPILE_TEST)
> > > > diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> > > > index cd591bf872bbe9..3e3190af2b50f4 100644
> > > > --- a/drivers/platform/chrome/Makefile
> > > > +++ b/drivers/platform/chrome/Makefile
> > > > @@ -8,6 +8,7 @@ cros_ec_ctl-objs := cros_ec_sysfs.o cros_ec_lightbar.o \
> > > > obj-$(CONFIG_CROS_EC_CTL) += cros_ec_ctl.o
> > > > obj-$(CONFIG_CROS_EC_I2C) += cros_ec_i2c.o
> > > > obj-$(CONFIG_CROS_EC_SPI) += cros_ec_spi.o
> > > > +obj-$(CONFIG_CROS_EC_RPMSG) += cros_ec_rpmsg.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
> > > > obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
> > > > diff --git a/drivers/platform/chrome/cros_ec_rpmsg.c b/drivers/platform/chrome/cros_ec_rpmsg.c
> > > > new file mode 100644
> > > > index 00000000000000..f123ca6d1c029c
> > > > --- /dev/null
> > > > +++ b/drivers/platform/chrome/cros_ec_rpmsg.c
> > > > @@ -0,0 +1,164 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +//
> > > > +// Copyright 2018 Google LLC.
> > > > +
> > > > +#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>
> > > > +
> > > > +/**
> > > > + * cros_ec_cmd_xfer_rpmsg - Transfer a message over rpmsg and receive the reply
> > > > + *
> > > > + * This is only used for old EC proto version, and is not supported for this
> > > > + * driver.
> > > > + *
> > > > + * @ec_dev: ChromeOS EC device
> > > > + * @ec_msg: Message to transfer
> > > > + */
> > > > +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
> > > > + */
> > > > +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 rpmsg_device *rpdev = ec_dev->priv;
> > > > + int len;
> > > > + u8 sum;
> > > > + int ret;
> > > > + int i;
> > > > +
> > > > + ec_msg->result = 0;
> > > > + len = cros_ec_prepare_tx(ec_dev, ec_msg);
> > > > + dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);
> > > > +
> > > > + // TODO: This currently relies on that mtk_rpmsg send actually blocks
> > > > + // until ack. Should do the wait here instead.
> > >
> > > Use standard C style comments.
> > >
> > > > + ret = rpmsg_send(rpdev->ept, ec_dev->dout, len);
> > > > +
> > >
> > > Remove that empty line.
> > >
> > > > + if (ret) {
> > > > + dev_err(ec_dev->dev, "rpmsg send failed\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + /* 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);
> > >
> > > Can you explain why this sleep is needed?
> >
> > From the comment of EC_CMD_REBOOT_EC: "The EC is unresponsive for a time after
> > a reboot command. Add a simple delay to make sure that the bus stays locked."
> >
> > This is copied from other transport layer drivers, and probably not needed
> > since we would reload the firmware for SCP while it's rebooting. I would test
> > to see if this is needed when the reboot flow for SCP work as expected.
> > (There's still some firmware work need to be done before it can be tested...)
> >
> > >
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +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);
> > > > +
> > > > + if (len > ec_dev->din_size) {
> > > > + dev_warn(ec_dev->dev,
> > > > + "ipi received length %d > din_size, truncating", len);
> > > > + len = ec_dev->din_size;
> > > > + }
> > > > +
> > > > + memcpy(ec_dev->din, data, len);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int cros_ec_rpmsg_probe(struct rpmsg_device *rpdev)
> > > > +{
> > > > + struct device *dev = &rpdev->dev;
> > > > +
> > > Remove that empty line
> > >
> > > > + struct cros_ec_device *ec_dev;
> > > > + int ret;
> > > > +
> > > > + ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
> > > > + if (!ec_dev)
> > > > + return -ENOMEM;
> > > > +
> > > > + ec_dev->dev = dev;
> > > > + ec_dev->priv = rpdev;
> > > > + 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);
> > > > +
> > > > + ret = cros_ec_register(ec_dev);
> > > > + if (ret)
> > >
> > > I'd add an error message here
> > >
> > > dev_err(dev, "cannot register EC\n"
> > >
> > > > + return ret;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void cros_ec_rpmsg_remove(struct rpmsg_device *rpdev)
> > >
> > > This function will not be needed after apply [1]. I would recommend
> > > base your patches on top of [2]
> > >
> > > [1] https://lkml.org/lkml/2018/12/12/672
> > > [2] https://lkml.org/lkml/2018/12/12/679
> >
> > Noted.
> >
> > >
> > > > +{
> > > > + struct cros_ec_device *ec_dev = dev_get_drvdata(&rpdev->dev);
> > > > +
> > > > + cros_ec_remove(ec_dev);
> > > > +}
> > > > +
> > >
> > > How this driver is instantiated?
> > >
> > > I expect something like this here (like the other transport layers)
> > >
> > > 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);
> > >
> > > And the DT containing the compatible = "google,cros-ec-rpmsg" like the
> > > other cros-ec transport layers.
> >
> > This is a part that I'm getting quite confused on how to do properly.
> > For SPI, a spi_device is created for each node listed under spi node in device
> > tree.
> > spi0 {
> > compatible = "xxx-spi";
> > cros_ec@0 {
> > compatible = "google,cros-ec-spi";
> > };
> > }
> >
> > For rpmsg, the rpmsg_device are dynamically created from the request
> > of the SCP, and then a matching rpmsg_driver is used when found.
> > Currently without the cros-ec-rpmsg being in the device tree, the cros_ec_rpmsg
> > module would need to be manually loaded by modprobe.
> >
> > To follow what SPI/I2C does, the device tree would look like:
> > scp {
> > compatible = "mediatek,mt8183-scp";
> > mt8183-rpmsg {
> > compatible = "mediatek,mt8183-rpmsg";
> > cros_ec_rpmsg {
> > compatible = "google,cros-ec-rpmsg";
> > };
> > };
> > };
> > But the rpmsg driver would not actually create those rpmsg_device on probe, but
> > only look at those sub node and load the corresponding rpmsg_driver modules.
> > When requested by SCP to create the rpmsg_device, it would find a matching
> > rpmsg_driver independent on how the device tree looks.
> >
> > So my question is, should these dynamically created rpmsg_device be listed on
> > device tree?
> >
>
> I think that right now that's our main problem, how to properly
> instantiate all this stuff. One approach that I like is the one used
> in the TI PRU ICSS, they create a pruss_soc_bus driver with the
> purpose to allow the child nodes to be bound. I suspect that something
> similar would work, but I need to look in more detail. See [1]
>
> [1] https://lkml.org/lkml/2018/11/22/948
>

Ok I'll take a look.

> Cheers,
> Enric
>
> > >
> > > > +static const struct rpmsg_device_id cros_ec_rpmsg_device_id[] = {
> > > > + { .name = "cros-ec-rpmsg", },
> > > > + { /* sentinel */ },
> > >
> > > I got convinced that the '/* sentinel */' comment doesn't means
> > > anything, so use { } only here, remove the comment and the last comma
> > > (there is nothing to separate)
> > > + { }
> > >
> > > > +};
> > > > +
> > > > +static struct rpmsg_driver cros_ec_driver_rpmsg = {
> > > > + .drv.name = KBUILD_MODNAME,
> > >
> > > And something like this here
> > > .drv = {
> > > .name = "cros-ec-rpmsg",
> > > .of_match_table = cros_ec_rpmsg_of_match,
> > > },
> > >
> > > > + .id_table = cros_ec_rpmsg_device_id,
> > > > + .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)");
> > > > diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> > > > index de8b588c8776da..fd297cf8f97295 100644
> > > > --- a/include/linux/mfd/cros_ec.h
> > > > +++ b/include/linux/mfd/cros_ec.h
> > > > @@ -24,6 +24,7 @@
> > > >
> > > > #define CROS_EC_DEV_NAME "cros_ec"
> > > > #define CROS_EC_DEV_PD_NAME "cros_pd"
> > > > +#define CROS_EC_DEV_SCP_NAME "cros_scp"
> > >
> > > I think this definition is not needed.
> > >
> > > >
> > > > /*
> > > > * The EC is unresponsive for a time after a reboot command. Add a
> > > > diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> > > > index fc91082d4c357b..3e5da6e93b2f42 100644
> > > > --- a/include/linux/mfd/cros_ec_commands.h
> > > > +++ b/include/linux/mfd/cros_ec_commands.h
> > > > @@ -856,6 +856,8 @@ enum ec_feature_code {
> > > > EC_FEATURE_RTC = 27,
> > > > /* EC supports CEC commands */
> > > > EC_FEATURE_CEC = 35,
> > > > + /* The MCU exposes a SCP */
> > > > + EC_FEATURE_SCP = 39,
> > >
> > > Same here, I think this is not needed.
> > > > };
> > > >
> > > > #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32))
> > > > --
> > > > 2.20.1.415.g653613c723-goog
> > > >
> > >
> > > Thanks,
> > > Enric