From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.6 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6693EC43387 for ; Thu, 3 Jan 2019 16:57:08 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 13790217D9 for ; Thu, 3 Jan 2019 16:57:08 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="i2zNxw37" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732385AbfACQ5G (ORCPT ); Thu, 3 Jan 2019 11:57:06 -0500 Received: from mail-yb1-f193.google.com ([209.85.219.193]:35782 "EHLO mail-yb1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730411AbfACQ5G (ORCPT ); Thu, 3 Jan 2019 11:57:06 -0500 Received: by mail-yb1-f193.google.com with SMTP id f187so3872768ybb.2 for ; Thu, 03 Jan 2019 08:57:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=39B9H4utt9pzx0C8knwlNDpXW0gXTiMla/EYh6c6gck=; b=i2zNxw37PzAgJq/FgJuOf0R4lqNVO3suH0JcB+VpLmQlxxUcUATBb46p/Przz/Pypg BtormNM8Q8c0cUvTSCecNHOTQIBE7O9Z+TYWQbnWgKNcTgA7X+j4LXNGOKAyUvRk9Nvi DIyJVj57Gp2q1bqbfzTzOczVSnZ+3/10VIhVNJFj/mSRAlIiJ466e8G5w2VV3Err7GN2 lPhHv8uyeNAWm6C26fm/Esq52ZGKOKVG3EpT0osun5+QowvmZJLI7drrTpbncoVh5v5o OrnwK5rdSDwF1LYoMiZ9kkjGXABemogbBZOOq89aP8fcHrVQI6Fh8UHxVTJgLQXkfe5V SGug== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=39B9H4utt9pzx0C8knwlNDpXW0gXTiMla/EYh6c6gck=; b=dKLZL1XYd+QEW/DeVj3F7HSy1hPgp5G+cGgDF5c6646hC1fTnlL7h79ZRIx5LZ8w6/ 4q7HlPe8los6p2yWPVLKkz7BUAD5zm5Y+1bpNJM5nAe8fQ6dkO9w7DLq6GwptLzm7Imj X39CPTPnH+a65TWKncFhdAJc1o3JMb9JHXedCYn7p6QfLslNXzHmuyrgmQV8S8isLyp+ 9lvvSrT2xmcsvCu7vOaBBKV7yTaKciYwpq6bkWvggltitRKRW0OHl7fSSzbo3ROcX6Qi E0+o4Mkko37cMW7PmYyXWK14DS4uWysnCDo0zRZGcM5XMPj8O+kQ2cbOXLLiVH1kpgUM zgOw== X-Gm-Message-State: AJcUukdzJ21VQokG5DjwRImKF+ndB6yunTn51vkh5CBWaiA6WQBj/i20 x7STXehYQcHTuwbmSK7IcJ/hXbTkbFSMLL7n8T/qZQ== X-Google-Smtp-Source: ALg8bN45av5a8AZX3pmxI9oACBTuHq5Rl0+GwkNCsvaEGcTFbSkaRZxe/9nczgY8HMGkolK6PtN0JX3Q518wW2DgsAE= X-Received: by 2002:a25:d957:: with SMTP id q84mr46111737ybg.151.1546534624473; Thu, 03 Jan 2019 08:57:04 -0800 (PST) MIME-Version: 1.0 References: <20181226075330.82462-1-pihsun@chromium.org> <20181226075330.82462-6-pihsun@chromium.org> In-Reply-To: From: Guenter Roeck Date: Thu, 3 Jan 2019 08:56:53 -0800 Message-ID: Subject: Re: [RFC,5/5] mfd: cros_ec: add EC host command support using rpmsg. To: Enric Balletbo Serra Cc: Pi-Hsun Shih , Nicolas Boichat , Lee Jones , Benson Leung , Olof Johansson , open list , Guenter Roeck Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 3, 2019 at 8:39 AM Enric Balletbo Serra wrote: > > Hi Guenter, > > Missatge de Guenter Roeck del dia dj., 3 de gen. > 2019 a les 17:08: > > > > On Thu, Jan 3, 2019 at 8:06 AM Enric Balletbo Serra 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 del dia dc., 26 de des. > > > 2018 a les 8:57: > > > > > > > > Add EC host command support through rpmsg. > > > > > > > > Signed-off-by: Pi-Hsun Shih > > > > --- > > > > 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. > > > > > > > /* > > > > * 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 ? > > > > > > > + 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 > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > + > > > > +/** > > > > + * 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? > > > > > > > + > > > > + 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 > > > > > > > +{ > > > > + 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. > > > > > > > +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. > > > > It might be needed for instantiation, ie instantiate only if the > > feature is supported. > > > > Actually, in this code, this is only used to change the EC name (and > as I commented above I think is not really needed). If I understand > correctly the purpose of these patches is to be able to talk with the > SCP via remoteproc. The SCP is a small Cortex M4 within MT8183 > processor that will run the EC codebase. So, the remoteproc message > driver looks more a transport driver (like the cros-ec-spi/i2c/lpc) > than a subdev driver to me. So I'd expect this be instantiated via DT > like the other transport layers, i.e. > > cros-ec@0 { > compatible = "google,cros-ec-spi"; > }; > > or > > cros-ec@ef { > compatible = "google,cros-ec-i2c"; > }; > > or > cros-ec { > compatible = "google,cros-ec-rpmsg"; > }; > > If I am not wrong we don't have an EC_FEATURE_SPI or EC_FEATURE_I2C, > so I think will be better instantiate in the same way? > Yes, you are correct. Thanks for the explanation. Guenter > Thanks, > Enric > > > Guenter > > > > > > }; > > > > > > > > #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32)) > > > > -- > > > > 2.20.1.415.g653613c723-goog > > > > > > > > > > Thanks, > > > Enric