[RFC,0/5] Add support for mt8183 SCP.

[RFC,0/5] Add support for mt8183 SCP.

[Pi-Hsun Shih]

Add support for controlling and communicating with mt8183's system
control processor (SCP), using the remoteproc & rpmsg framework.
And also add a cros_ec driver for CrOS EC host command over rpmsg.

The overall structure of the series is:
* remoteproc/mtk_scp.c: Control the start / stop of SCP (Patch 2).
* remoteproc/mtk_scp_ipi.c: Communicates to SCP using inter-processor
  interrupt (IPI) and shared memory (Patch 2, 3).
* rpmsg/mtk_rpmsg.c: Wrapper to wrap the IPI communication into a rpmsg
  device. Supports name service for SCP firmware to
  announce channels (Patch 4).
* platform/chrome/cros_ec_rpmsg.c: Communicates with the SCP over the
  rpmsg framework (like what platform/chrome/cros_ec_{i2c,spi}.c does)
  (Patch 5).

Since I'm not familiar with the remoteproc / rpmsg framework, and there
are not much other custom rpmsg driver for reference, would like some
review / comments on the overall structure of the driver.


Erin Lo (2):
  dt-bindings: Add a binding for Mediatek SCP
  remoteproc/mediatek: add SCP support for mt8183

Pi-Hsun Shih (3):
  remoteproc: move IPI interface into separate file.
  rpmsg: add rpmsg support for mt8183 SCP.
  mfd: cros_ec: add EC host command support using rpmsg.

 .../bindings/remoteproc/mtk,scp.txt           |  10 +
 drivers/mfd/cros_ec_dev.c                     |  10 +
 drivers/platform/chrome/Kconfig               |   8 +
 drivers/platform/chrome/Makefile              |   1 +
 drivers/platform/chrome/cros_ec_rpmsg.c       | 164 +++++++
 drivers/remoteproc/Kconfig                    |   9 +
 drivers/remoteproc/Makefile                   |   1 +
 drivers/remoteproc/mtk_common.h               |  76 +++
 drivers/remoteproc/mtk_scp.c                  | 441 ++++++++++++++++++
 drivers/remoteproc/mtk_scp_ipi.c              | 109 +++++
 drivers/rpmsg/Kconfig                         |   5 +
 drivers/rpmsg/Makefile                        |   1 +
 drivers/rpmsg/mtk_rpmsg.c                     | 341 ++++++++++++++
 include/linux/mfd/cros_ec.h                   |   1 +
 include/linux/mfd/cros_ec_commands.h          |   2 +
 include/linux/platform_data/mtk_scp.h         | 143 ++++++
 include/linux/rpmsg/mtk_rpmsg.h               |  34 ++
 17 files changed, 1356 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/mtk,scp.txt
 create mode 100644 drivers/platform/chrome/cros_ec_rpmsg.c
 create mode 100644 drivers/remoteproc/mtk_common.h
 create mode 100644 drivers/remoteproc/mtk_scp.c
 create mode 100644 drivers/remoteproc/mtk_scp_ipi.c
 create mode 100644 drivers/rpmsg/mtk_rpmsg.c
 create mode 100644 include/linux/platform_data/mtk_scp.h
 create mode 100644 include/linux/rpmsg/mtk_rpmsg.h

-- 
2.20.1.415.g653613c723-goog

[RFC,1/5] dt-bindings: Add a binding for Mediatek SCP

[Pi-Hsun Shih]

From: Erin Lo <erin.lo@mediatek.com>

Add a DT binding documentation of SCP for the
MT8183 SoC from Mediatek.

Signed-off-by: Erin Lo <erin.lo@mediatek.com>
---
 .../devicetree/bindings/remoteproc/mtk,scp.txt         | 10 ++++++++++
 1 file changed, 10 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/mtk,scp.txt

diff --git a/Documentation/devicetree/bindings/remoteproc/mtk,scp.txt b/Documentation/devicetree/bindings/remoteproc/mtk,scp.txt
new file mode 100644
index 0000000000000..b07e5c4ca9af1
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.txt
@@ -0,0 +1,9 @@
+Mediatek SCP Bindings
+----------------------------------------
+
+This binding provides support for ARM Cortex M4 Co-processor found on some
+Mediatek SoCs.
+
+Required properties:
+- compatible		Should be "mediatek,mt8183-scp"
+- clocks		Clock for co-processor (See: ../clock/clock-bindings.txt)
-- 
2.20.1.415.g653613c723-goog

[RFC,2/5] remoteproc/mediatek: add SCP support for mt8183

[Pi-Hsun Shih]

From: Erin Lo <erin.lo@mediatek.com>

Provide a basic driver to control Cortex M4 co-processor

Signed-off-by: Erin Lo <erin.lo@mediatek.com>
---
 drivers/remoteproc/Kconfig            |   9 +
 drivers/remoteproc/Makefile           |   1 +
 drivers/remoteproc/mtk_scp.c          | 568 ++++++++++++++++++++++++++
 include/linux/platform_data/mtk_scp.h | 136 ++++++
 4 files changed, 714 insertions(+)
 create mode 100644 drivers/remoteproc/mtk_scp.c
 create mode 100644 include/linux/platform_data/mtk_scp.h

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index f0abd26080447..ee0bda2376893 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -22,6 +22,15 @@ config IMX_REMOTEPROC
 
 	  It's safe to say N here.
 
+config MTK_SCP
+        tristate "Mediatek SCP support"
+        depends on ARCH_MEDIATEK
+        help
+          Say y here to support Mediatek's SCP (Cortex M4
+          on MT8183) via the remote processor framework.
+
+          It's safe to say N here.
+
 config OMAP_REMOTEPROC
 	tristate "OMAP remoteproc support"
 	depends on ARCH_OMAP4 || SOC_OMAP5
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index ce5d061e92be5..98e3498dbbe0e 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -10,6 +10,7 @@ remoteproc-y				+= remoteproc_sysfs.o
 remoteproc-y				+= remoteproc_virtio.o
 remoteproc-y				+= remoteproc_elf_loader.o
 obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
+obj-$(CONFIG_MTK_SCP)			+= mtk_scp.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
 obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
 obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
new file mode 100644
index 0000000000000..7a1a8fe53b54a
--- /dev/null
+++ b/drivers/remoteproc/mtk_scp.c
@@ -0,0 +1,568 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2018 MediaTek Inc.
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_data/mtk_scp.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc.h>
+
+#include "remoteproc_internal.h"
+
+#define MT8183_SW_RSTN			0x0
+#define MT8183_SW_RSTN_BIT		BIT(0)
+#define MT8183_SCP_TO_HOST		0x1C
+#define MT8183_SCP_IPC_INT_BIT		BIT(0)
+#define MT8183_SCP_WDT_INT_BIT		BIT(8)
+#define MT8183_HOST_TO_SCP		0x28
+#define MT8183_HOST_IPC_INT_BIT		BIT(0)
+#define MT8183_SCP_SRAM_PDN		0x402C
+
+#define INIT_TIMEOUT_MS		2000
+#define IPI_TIMEOUT_MS		2000
+#define SCP_FW_VER_LEN		32
+
+#define MAX_CODE_SIZE 0x500000
+
+struct scp_run {
+	u32 signaled;
+	s8 fw_ver[SCP_FW_VER_LEN];
+	u32 dec_capability;
+	u32 enc_capability;
+	wait_queue_head_t wq;
+};
+
+struct scp_ipi_desc {
+	ipi_handler_t handler;
+	const char *name;
+	void *priv;
+};
+
+struct mtk_scp {
+	struct device *dev;
+	struct rproc *rproc;
+	struct clk *clk;
+	void __iomem *reg_base;
+	void __iomem *sram_base;
+	size_t sram_size;
+
+	struct share_obj *recv_buf;
+	struct share_obj *send_buf;
+	struct scp_run run;
+	struct mutex scp_mutex; /* for protecting mtk_scp data structure */
+	struct scp_ipi_desc ipi_desc[IPI_MAX];
+	bool ipi_id_ack[IPI_MAX];
+	wait_queue_head_t ack_wq;
+
+	void __iomem *cpu_addr;
+	phys_addr_t phys_addr;
+	size_t dram_size;
+};
+
+/**
+ * struct share_obj - SRAM buffer shared with
+ *		      AP and SCP
+ *
+ * @id:		IPI id
+ * @len:	share buffer length
+ * @share_buf:	share buffer data
+ */
+struct share_obj {
+	s32 id;
+	u32 len;
+	u8 share_buf[288];
+};
+
+int scp_ipi_register(struct platform_device *pdev,
+		     enum ipi_id id,
+		     ipi_handler_t handler,
+		     const char *name,
+		     void *priv)
+{
+	struct mtk_scp *scp = platform_get_drvdata(pdev);
+	struct scp_ipi_desc *ipi_desc;
+
+	if (!scp) {
+		dev_err(&pdev->dev, "scp device is not ready\n");
+		return -EPROBE_DEFER;
+	}
+
+	if (WARN(id < 0 ||  id >= IPI_MAX || handler == NULL,
+	    "register scp ipi id %d with invalid arguments\n", id))
+		return -EINVAL;
+
+	ipi_desc = scp->ipi_desc;
+	ipi_desc[id].name = name;
+	ipi_desc[id].handler = handler;
+	ipi_desc[id].priv = priv;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(scp_ipi_register);
+
+int scp_ipi_send(struct platform_device *pdev,
+		 enum ipi_id id, void *buf,
+		 unsigned int len,
+		 unsigned int wait)
+{
+	struct mtk_scp *scp = platform_get_drvdata(pdev);
+	struct share_obj *send_obj = scp->send_buf;
+	unsigned long timeout;
+	int ret;
+
+	if (WARN(id <= IPI_SCP_INIT || id >= IPI_MAX ||
+	    len > sizeof(send_obj->share_buf) || !buf,
+	    "failed to send ipi message\n"))
+		return -EINVAL;
+
+	ret = clk_prepare_enable(scp->clk);
+	if (ret) {
+		dev_err(scp->dev, "failed to enable clock\n");
+		return ret;
+	}
+
+	mutex_lock(&scp->scp_mutex);
+
+	 /* Wait until SCP receives the last command */
+	timeout = jiffies + msecs_to_jiffies(IPI_TIMEOUT_MS);
+	do {
+		if (time_after(jiffies, timeout)) {
+			dev_err(scp->dev, "scp_ipi_send: IPI timeout!\n");
+			ret = -EIO;
+			mutex_unlock(&scp->scp_mutex);
+			goto clock_disable;
+		}
+	} while (readl(scp->reg_base + MT8183_HOST_TO_SCP));
+
+	memcpy(send_obj->share_buf, buf, len);
+	send_obj->len = len;
+	send_obj->id = id;
+
+	scp->ipi_id_ack[id] = false;
+	/* send the command to SCP */
+	writel(MT8183_HOST_IPC_INT_BIT, scp->reg_base + MT8183_HOST_TO_SCP);
+
+	mutex_unlock(&scp->scp_mutex);
+
+	if (wait) {
+		/* wait for SCP's ACK */
+		timeout = msecs_to_jiffies(wait);
+		ret = wait_event_timeout(scp->ack_wq,
+					 scp->ipi_id_ack[id],
+					 timeout);
+		scp->ipi_id_ack[id] = false;
+		if (WARN(!ret,
+			 "scp ipi %d ack time out !", id))
+			ret = -EIO;
+		else
+			ret = 0;
+	}
+
+clock_disable:
+	clk_disable_unprepare(scp->clk);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(scp_ipi_send);
+
+struct platform_device *scp_get_plat_device(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *scp_node;
+	struct platform_device *scp_pdev;
+
+	scp_node = of_parse_phandle(dev->of_node, "mediatek,scp", 0);
+	if (!scp_node) {
+		dev_err(dev, "can't get scp node\n");
+		return NULL;
+	}
+
+	scp_pdev = of_find_device_by_node(scp_node);
+	if (WARN_ON(!scp_pdev)) {
+		dev_err(dev, "scp pdev failed\n");
+		of_node_put(scp_node);
+		return NULL;
+	}
+
+	return scp_pdev;
+}
+EXPORT_SYMBOL_GPL(scp_get_plat_device);
+
+static void scp_wdt_handler(struct mtk_scp *scp)
+{
+	rproc_report_crash(scp->rproc, RPROC_WATCHDOG);
+}
+
+static void scp_init_ipi_handler(void *data, unsigned int len, void *priv)
+{
+	struct mtk_scp *scp = (struct mtk_scp *)priv;
+	struct scp_run *run = (struct scp_run *)data;
+
+	scp->run.signaled = run->signaled;
+	strncpy(scp->run.fw_ver, run->fw_ver, SCP_FW_VER_LEN);
+	scp->run.dec_capability = run->dec_capability;
+	scp->run.enc_capability = run->enc_capability;
+	wake_up_interruptible(&scp->run.wq);
+}
+
+static void scp_ipi_handler(struct mtk_scp *scp)
+{
+	struct share_obj *rcv_obj = scp->recv_buf;
+	struct scp_ipi_desc *ipi_desc = scp->ipi_desc;
+	u8 tmp_data[288];
+
+	if (rcv_obj->id < IPI_MAX && ipi_desc[rcv_obj->id].handler) {
+		memcpy_fromio(tmp_data, &rcv_obj->share_buf, rcv_obj->len);
+		ipi_desc[rcv_obj->id].handler(&tmp_data[0],
+					      rcv_obj->len,
+					      ipi_desc[rcv_obj->id].priv);
+		scp->ipi_id_ack[rcv_obj->id] = true;
+		wake_up(&scp->ack_wq);
+	} else {
+		dev_err(scp->dev, "No such ipi id = %d\n", rcv_obj->id);
+	}
+}
+
+static int scp_ipi_init(struct mtk_scp *scp)
+{
+	size_t send_offset = 0x800 - sizeof(struct share_obj);
+	size_t recv_offset = send_offset - sizeof(struct share_obj);
+
+	/* Disable SCP to host interrupt */
+	writel(MT8183_SCP_IPC_INT_BIT, scp->reg_base + MT8183_SCP_TO_HOST);
+
+	/* shared buffer initialization */
+	scp->recv_buf = (__force struct share_obj *)(scp->sram_base +
+						recv_offset);
+	scp->send_buf = (__force struct share_obj *)(scp->sram_base +
+						send_offset);
+	memset_io(scp->recv_buf, 0, sizeof(scp->recv_buf));
+	memset_io(scp->send_buf, 0, sizeof(scp->send_buf));
+
+	return 0;
+}
+
+static irqreturn_t scp_irq_handler(int irq, void *priv)
+{
+	struct mtk_scp *scp = priv;
+	u32 scp_to_host;
+
+	scp_to_host = readl(scp->reg_base + MT8183_SCP_TO_HOST);
+	if (scp_to_host & MT8183_SCP_IPC_INT_BIT) {
+		scp_ipi_handler(scp);
+	} else {
+		dev_err(scp->dev, "scp watchdog timeout! 0x%x", scp_to_host);
+		scp_wdt_handler(scp);
+	}
+
+	/* SCP won't send another interrupt until we set SCP_TO_HOST to 0. */
+	writel(MT8183_SCP_IPC_INT_BIT | MT8183_SCP_WDT_INT_BIT,
+	       scp->reg_base + MT8183_SCP_TO_HOST);
+
+	return IRQ_HANDLED;
+}
+
+static int mtk_scp_load(struct rproc *rproc, const struct firmware *fw)
+{
+	const struct mtk_scp *scp = rproc->priv;
+	struct device *dev = scp->dev;
+	int ret;
+
+	ret = clk_prepare_enable(scp->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clocks\n");
+		return ret;
+	}
+
+	writel(0x0, scp->reg_base + MT8183_SCP_SRAM_PDN);
+
+	memcpy(scp->sram_base, fw->data, fw->size);
+	return ret;
+}
+
+static int mtk_scp_start(struct rproc *rproc)
+{
+	struct mtk_scp *scp = (struct mtk_scp *)rproc->priv;
+	struct device *dev = scp->dev;
+	struct scp_run *run;
+	u32 val;
+	int ret;
+
+	ret = clk_prepare_enable(scp->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clocks\n");
+		return ret;
+	}
+
+	val = readl(scp->reg_base + MT8183_SW_RSTN);
+	val |= MT8183_SW_RSTN_BIT;
+	writel(val, scp->reg_base + MT8183_SW_RSTN);
+
+	run = &scp->run;
+
+	ret = wait_event_interruptible_timeout(
+					run->wq,
+					run->signaled,
+					msecs_to_jiffies(INIT_TIMEOUT_MS));
+
+	if (ret == 0) {
+		dev_err(dev, "wait scp initialization timeout!\n");
+		return -ETIME;
+	}
+	if (ret == -ERESTARTSYS) {
+		dev_err(dev, "wait scp interrupted by a signal!\n");
+		return -ERESTARTSYS;
+	}
+	dev_info(dev, "scp is ready. Fw version %s\n", run->fw_ver);
+
+	return 0;
+}
+
+static void *mtk_scp_da_to_va(struct rproc *rproc, u64 da, int len)
+{
+	struct mtk_scp *scp = (struct mtk_scp *)rproc->priv;
+	int offset;
+
+	if (da < scp->sram_size) {
+		offset = da;
+		if (offset >= 0 && ((offset + len) < scp->sram_size)) {
+			return (__force void *)(scp->sram_base + offset);
+		}
+	} else if (da >= scp->sram_size && da < (scp->sram_size + MAX_CODE_SIZE)) {
+		offset = da - scp->sram_size;
+		if (offset >= 0 && (offset + len) < MAX_CODE_SIZE) {
+			return scp->cpu_addr + offset;
+		}
+	} else {
+		offset = da - scp->phys_addr;
+		if (offset >= 0 && (offset + len) < (scp->dram_size - MAX_CODE_SIZE)) {
+			return scp->cpu_addr + offset;
+		}
+	}
+
+	return NULL;
+}
+
+static int mtk_scp_stop(struct rproc *rproc)
+{
+	struct mtk_scp *scp = (struct mtk_scp *)rproc->priv;
+	u32 val;
+
+	val = readl(scp->reg_base + MT8183_SW_RSTN);
+	val &= ~MT8183_SW_RSTN_BIT;
+	writel(val, scp->reg_base + MT8183_SW_RSTN);
+
+	clk_disable_unprepare(scp->clk);
+
+	return 0;
+}
+
+static const struct rproc_ops mtk_scp_ops = {
+	.start		= mtk_scp_start,
+	.stop		= mtk_scp_stop,
+	.load		= mtk_scp_load,
+	.da_to_va	= mtk_scp_da_to_va,
+};
+
+unsigned int scp_get_vdec_hw_capa(struct platform_device *pdev)
+{
+	struct mtk_scp *scp = platform_get_drvdata(pdev);
+
+	return scp->run.dec_capability;
+}
+EXPORT_SYMBOL_GPL(scp_get_vdec_hw_capa);
+
+unsigned int scp_get_venc_hw_capa(struct platform_device *pdev)
+{
+	struct mtk_scp *scp = platform_get_drvdata(pdev);
+
+	return scp->run.enc_capability;
+}
+EXPORT_SYMBOL_GPL(scp_get_venc_hw_capa);
+
+void *scp_mapping_dm_addr(struct platform_device *pdev, u32 mem_addr)
+{
+	struct mtk_scp *scp = platform_get_drvdata(pdev);
+	void *ptr = NULL;
+
+	ptr = mtk_scp_da_to_va(scp->rproc, mem_addr, 0);
+
+	if (ptr)
+		return ptr;
+	else
+		return ERR_PTR(-EINVAL);
+}
+EXPORT_SYMBOL_GPL(scp_mapping_dm_addr);
+
+static int scp_map_memory_region(struct mtk_scp *scp)
+{
+	struct device_node *node;
+	struct resource r;
+	int ret;
+
+	node = of_parse_phandle(scp->dev->of_node, "memory-region", 0);
+	if (!node) {
+		dev_err(scp->dev, "no memory-region specified\n");
+		return -EINVAL;
+	}
+
+	ret = of_address_to_resource(node, 0, &r);
+	if (ret)
+		return ret;
+
+	scp->phys_addr = r.start;
+	scp->dram_size = resource_size(&r);
+	scp->cpu_addr = devm_ioremap_wc(scp->dev, scp->phys_addr, scp->dram_size);
+
+	if (!scp->cpu_addr) {
+		dev_err(scp->dev, "unable to map memory region: %pa+%zx\n",
+			&r.start, scp->dram_size);
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
+static int mtk_scp_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct mtk_scp *scp;
+	struct rproc *rproc;
+	struct resource *res;
+	char *fw_name = "scp.img";
+	int ret;
+
+	rproc = rproc_alloc(dev,
+			    np->name,
+			    &mtk_scp_ops,
+			    fw_name,
+			    sizeof(*scp));
+	if (!rproc) {
+		dev_err(dev, "unable to allocate remoteproc\n");
+		return -ENOMEM;
+	}
+
+	scp = (struct mtk_scp *)rproc->priv;
+	scp->rproc = rproc;
+	scp->dev = dev;
+	platform_set_drvdata(pdev, scp);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram");
+	scp->sram_base = devm_ioremap_resource(dev, res);
+	scp->sram_size = resource_size(res);
+	if (IS_ERR((__force void *)scp->sram_base)) {
+		dev_err(dev, "Failed to parse and map sram memory\n");
+		ret = PTR_ERR((__force void *)scp->sram_base);
+		goto free_rproc;
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
+	scp->reg_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR((__force void *)scp->reg_base)) {
+		dev_err(dev, "Failed to parse and map cfg memory\n");
+		ret = PTR_ERR((__force void *)scp->reg_base);
+		goto free_rproc;
+	}
+
+	ret = scp_map_memory_region(scp);
+	if (ret)
+		goto free_rproc;
+
+	scp->clk = devm_clk_get(dev, "main");
+	if (IS_ERR(scp->clk)) {
+		dev_err(dev, "Failed to get clock\n");
+		ret = PTR_ERR(scp->clk);
+		goto free_rproc;
+	}
+
+	ret = clk_prepare_enable(scp->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clocks\n");
+		goto free_rproc;
+	}
+
+	ret = scp_ipi_init(scp);
+	clk_disable_unprepare(scp->clk);
+	if (ret) {
+		dev_err(dev, "Failed to init ipi\n");
+		goto free_rproc;
+	}
+
+	/* register scp initialization IPI */
+	ret = scp_ipi_register(pdev,
+			       IPI_SCP_INIT,
+			       scp_init_ipi_handler,
+			       "scp_init",
+			       scp);
+	if (ret) {
+		dev_err(dev, "Failed to register IPI_SCP_INIT\n");
+		goto free_rproc;
+	}
+
+	ret = devm_request_irq(dev,
+			       platform_get_irq(pdev, 0),
+			       scp_irq_handler,
+			       0,
+			       pdev->name,
+			       scp);
+
+	if (ret) {
+		dev_err(dev, "failed to request irq\n");
+		goto free_rproc;
+	}
+
+	mutex_init(&scp->scp_mutex);
+
+	init_waitqueue_head(&scp->run.wq);
+	init_waitqueue_head(&scp->ack_wq);
+
+	ret = rproc_add(rproc);
+	if (ret)
+		goto destroy_mutex;
+
+	return ret;
+
+destroy_mutex:
+	mutex_destroy(&scp->scp_mutex);
+free_rproc:
+	rproc_free(rproc);
+
+	return ret;
+}
+
+static int mtk_scp_remove(struct platform_device *pdev)
+{
+	struct mtk_scp *scp = platform_get_drvdata(pdev);
+
+	rproc_del(scp->rproc);
+	rproc_free(scp->rproc);
+
+	return 0;
+}
+
+static const struct of_device_id mtk_scp_of_match[] = {
+	{ .compatible = "mediatek,mt8183-scp"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, mtk_scp_of_match);
+
+static struct platform_driver mtk_scp_driver = {
+	.probe = mtk_scp_probe,
+	.remove = mtk_scp_remove,
+	.driver = {
+		.name = "mtk-scp",
+		.of_match_table = of_match_ptr(mtk_scp_of_match),
+	},
+};
+
+module_platform_driver(mtk_scp_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MediaTek scp control driver");
diff --git a/include/linux/platform_data/mtk_scp.h b/include/linux/platform_data/mtk_scp.h
new file mode 100644
index 0000000000000..118838c91564f
--- /dev/null
+++ b/include/linux/platform_data/mtk_scp.h
@@ -0,0 +1,136 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2018 MediaTek Inc.
+ */
+
+#ifndef _MTK_SCP_H
+#define _MTK_SCP_H
+
+#include <linux/platform_device.h>
+
+typedef void (*ipi_handler_t) (void *data,
+			       unsigned int len,
+			       void *priv);
+
+/**
+ * enum ipi_id - the id of inter-processor interrupt
+ *
+ * @IPI_SCP_INIT:	 The interrupt from scp is to notfiy kernel
+ *			 SCP initialization completed.
+ *			 IPI_SCP_INIT is sent from SCP when firmware is
+ *			 loaded. AP doesn't need to send IPI_SCP_INIT
+ *			 command to SCP.
+ *			 For other IPI below, AP should send the request
+ *			 to SCP to trigger the interrupt.
+ * @IPI_MAX:		 The maximum IPI number
+ */
+
+enum ipi_id {
+	IPI_SCP_INIT = 0,
+	IPI_MDP,
+	IPI_MAX,
+};
+
+/**
+ * enum rst_id - reset id to register reset function for SCP watchdog timeout
+ *
+ * @SCP_RST_ENC: encoder reset id
+ * @SCP_RST_DEC: decoder reset id
+ * @SCP_RST_MDP: MDP (Media Data Path) reset id
+ * @SCP_RST_MAX: maximum reset id
+ */
+enum rst_id {
+	SCP_RST_ENC,
+	SCP_RST_DEC,
+	SCP_RST_MDP,
+	SCP_RST_MAX,
+};
+
+/**
+ * scp_ipi_register - register an ipi function
+ *
+ * @pdev:	SCP platform device
+ * @id:		IPI ID
+ * @handler:	IPI handler
+ * @name:	IPI name
+ * @priv:	private data for IPI handler
+ *
+ * Register an ipi function to receive ipi interrupt from SCP.
+ *
+ * Return: Return 0 if ipi registers successfully, otherwise it is failed.
+ */
+int scp_ipi_register(struct platform_device *pdev,
+		     enum ipi_id id,
+		     ipi_handler_t handler,
+		     const char *name,
+		     void *priv);
+
+/**
+ * scp_ipi_send - send data from AP to scp.
+ *
+ * @pdev:	SCP platform device
+ * @id:		IPI ID
+ * @buf:	the data buffer
+ * @len:	the data buffer length
+ * @wait:	1: need ack
+ *
+ * This function is thread-safe. When this function returns,
+ * SCP has received the data and starts the processing.
+ * When the processing completes, IPI handler registered
+ * by scp_ipi_register will be called in interrupt context.
+ *
+ * Return: Return 0 if sending data successfully, otherwise it is failed.
+ **/
+int scp_ipi_send(struct platform_device *pdev,
+		 enum ipi_id id,
+		 void *buf,
+		 unsigned int len,
+		 unsigned int wait);
+
+/**
+ * scp_get_plat_device - get SCP's platform device
+ *
+ * @pdev:	the platform device of the module requesting SCP platform
+ *		device for using SCP API.
+ *
+ * Return: Return NULL if it is failed.
+ * otherwise it is SCP's platform device
+ **/
+struct platform_device *scp_get_plat_device(struct platform_device *pdev);
+
+/**
+ * scp_get_vdec_hw_capa - get video decoder hardware capability
+ *
+ * @pdev:	SCP platform device
+ *
+ * Return: video decoder hardware capability
+ **/
+unsigned int scp_get_vdec_hw_capa(struct platform_device *pdev);
+
+/**
+ * scp_get_venc_hw_capa - get video encoder hardware capability
+ *
+ * @pdev:	SCP platform device
+ *
+ * Return: video encoder hardware capability
+ **/
+unsigned int scp_get_venc_hw_capa(struct platform_device *pdev);
+
+/**
+ * scp_mapping_dm_addr - Mapping SRAM/DRAM to kernel virtual address
+ *
+ * @pdev:	SCP platform device
+ * @mem_addr:	SCP views memory address
+ *
+ * Mapping the SCP's SRAM address /
+ * DMEM (Data Extended Memory) memory address /
+ * Working buffer memory address to
+ * kernel virtual address.
+ *
+ * Return: Return ERR_PTR(-EINVAL) if mapping failed,
+ * otherwise the mapped kernel virtual address
+ **/
+void *scp_mapping_dm_addr(struct platform_device *pdev,
+			  u32 mem_addr);
+
+#endif /* _MTK_SCP_H */
-- 
2.20.1.415.g653613c723-goog

[RFC,3/5] remoteproc: move IPI interface into separate file.

[Pi-Hsun Shih]

Move the IPI interface into a separate file mtk_scp_ipi.c, so the things
that use the interface only can depend on the module only.

Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
---
 drivers/remoteproc/Makefile      |   2 +-
 drivers/remoteproc/mtk_common.h  |  73 +++++++++++++++
 drivers/remoteproc/mtk_scp.c     | 153 +------------------------------
 drivers/remoteproc/mtk_scp_ipi.c | 109 ++++++++++++++++++++++
 4 files changed, 184 insertions(+), 153 deletions(-)
 create mode 100644 drivers/remoteproc/mtk_common.h
 create mode 100644 drivers/remoteproc/mtk_scp_ipi.c

diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 98e3498dbbe0e..16b3e5e7a81c8 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -10,7 +10,7 @@ remoteproc-y				+= remoteproc_sysfs.o
 remoteproc-y				+= remoteproc_virtio.o
 remoteproc-y				+= remoteproc_elf_loader.o
 obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
-obj-$(CONFIG_MTK_SCP)			+= mtk_scp.o
+obj-$(CONFIG_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
 obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
 obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h
new file mode 100644
index 0000000000000..ae55fca2ce2d7
--- /dev/null
+++ b/drivers/remoteproc/mtk_common.h
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2018 MediaTek Inc.
+
+#ifndef __RPROC_MTK_COMMON_H
+#define __RPROC_MTK_COMMON_H
+
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc.h>
+
+#define MT8183_SW_RSTN			0x0
+#define MT8183_SW_RSTN_BIT		BIT(0)
+#define MT8183_SCP_TO_HOST		0x1C
+#define MT8183_SCP_IPC_INT_BIT		BIT(0)
+#define MT8183_SCP_WDT_INT_BIT		BIT(8)
+#define MT8183_HOST_TO_SCP		0x28
+#define MT8183_HOST_IPC_INT_BIT		BIT(0)
+#define MT8183_SCP_SRAM_PDN		0x402C
+
+#define SCP_FW_VER_LEN		32
+
+struct scp_run {
+	u32 signaled;
+	s8 fw_ver[SCP_FW_VER_LEN];
+	u32 dec_capability;
+	u32 enc_capability;
+	wait_queue_head_t wq;
+};
+
+struct scp_ipi_desc {
+	ipi_handler_t handler;
+	const char *name;
+	void *priv;
+};
+
+struct mtk_scp {
+	struct device *dev;
+	struct rproc *rproc;
+	struct clk *clk;
+	void __iomem *reg_base;
+	void __iomem *sram_base;
+	size_t sram_size;
+
+	struct share_obj *recv_buf;
+	struct share_obj *send_buf;
+	struct scp_run run;
+	struct mutex scp_mutex; /* for protecting mtk_scp data structure */
+	struct scp_ipi_desc ipi_desc[IPI_MAX];
+	bool ipi_id_ack[IPI_MAX];
+	wait_queue_head_t ack_wq;
+
+	void __iomem *cpu_addr;
+	phys_addr_t phys_addr;
+	size_t dram_size;
+};
+
+/**
+ * struct share_obj - SRAM buffer shared with
+ *		      AP and SCP
+ *
+ * @id:		IPI id
+ * @len:	share buffer length
+ * @share_buf:	share buffer data
+ */
+struct share_obj {
+	s32 id;
+	u32 len;
+	u8 share_buf[288];
+};
+
+#endif
diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
index 7a1a8fe53b54a..aaebe96bcfd66 100644
--- a/drivers/remoteproc/mtk_scp.c
+++ b/drivers/remoteproc/mtk_scp.c
@@ -14,163 +14,12 @@
 #include <linux/remoteproc.h>
 
 #include "remoteproc_internal.h"
-
-#define MT8183_SW_RSTN			0x0
-#define MT8183_SW_RSTN_BIT		BIT(0)
-#define MT8183_SCP_TO_HOST		0x1C
-#define MT8183_SCP_IPC_INT_BIT		BIT(0)
-#define MT8183_SCP_WDT_INT_BIT		BIT(8)
-#define MT8183_HOST_TO_SCP		0x28
-#define MT8183_HOST_IPC_INT_BIT		BIT(0)
-#define MT8183_SCP_SRAM_PDN		0x402C
+#include "mtk_common.h"
 
 #define INIT_TIMEOUT_MS		2000
-#define IPI_TIMEOUT_MS		2000
-#define SCP_FW_VER_LEN		32
 
 #define MAX_CODE_SIZE 0x500000
 
-struct scp_run {
-	u32 signaled;
-	s8 fw_ver[SCP_FW_VER_LEN];
-	u32 dec_capability;
-	u32 enc_capability;
-	wait_queue_head_t wq;
-};
-
-struct scp_ipi_desc {
-	ipi_handler_t handler;
-	const char *name;
-	void *priv;
-};
-
-struct mtk_scp {
-	struct device *dev;
-	struct rproc *rproc;
-	struct clk *clk;
-	void __iomem *reg_base;
-	void __iomem *sram_base;
-	size_t sram_size;
-
-	struct share_obj *recv_buf;
-	struct share_obj *send_buf;
-	struct scp_run run;
-	struct mutex scp_mutex; /* for protecting mtk_scp data structure */
-	struct scp_ipi_desc ipi_desc[IPI_MAX];
-	bool ipi_id_ack[IPI_MAX];
-	wait_queue_head_t ack_wq;
-
-	void __iomem *cpu_addr;
-	phys_addr_t phys_addr;
-	size_t dram_size;
-};
-
-/**
- * struct share_obj - SRAM buffer shared with
- *		      AP and SCP
- *
- * @id:		IPI id
- * @len:	share buffer length
- * @share_buf:	share buffer data
- */
-struct share_obj {
-	s32 id;
-	u32 len;
-	u8 share_buf[288];
-};
-
-int scp_ipi_register(struct platform_device *pdev,
-		     enum ipi_id id,
-		     ipi_handler_t handler,
-		     const char *name,
-		     void *priv)
-{
-	struct mtk_scp *scp = platform_get_drvdata(pdev);
-	struct scp_ipi_desc *ipi_desc;
-
-	if (!scp) {
-		dev_err(&pdev->dev, "scp device is not ready\n");
-		return -EPROBE_DEFER;
-	}
-
-	if (WARN(id < 0 ||  id >= IPI_MAX || handler == NULL,
-	    "register scp ipi id %d with invalid arguments\n", id))
-		return -EINVAL;
-
-	ipi_desc = scp->ipi_desc;
-	ipi_desc[id].name = name;
-	ipi_desc[id].handler = handler;
-	ipi_desc[id].priv = priv;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(scp_ipi_register);
-
-int scp_ipi_send(struct platform_device *pdev,
-		 enum ipi_id id, void *buf,
-		 unsigned int len,
-		 unsigned int wait)
-{
-	struct mtk_scp *scp = platform_get_drvdata(pdev);
-	struct share_obj *send_obj = scp->send_buf;
-	unsigned long timeout;
-	int ret;
-
-	if (WARN(id <= IPI_SCP_INIT || id >= IPI_MAX ||
-	    len > sizeof(send_obj->share_buf) || !buf,
-	    "failed to send ipi message\n"))
-		return -EINVAL;
-
-	ret = clk_prepare_enable(scp->clk);
-	if (ret) {
-		dev_err(scp->dev, "failed to enable clock\n");
-		return ret;
-	}
-
-	mutex_lock(&scp->scp_mutex);
-
-	 /* Wait until SCP receives the last command */
-	timeout = jiffies + msecs_to_jiffies(IPI_TIMEOUT_MS);
-	do {
-		if (time_after(jiffies, timeout)) {
-			dev_err(scp->dev, "scp_ipi_send: IPI timeout!\n");
-			ret = -EIO;
-			mutex_unlock(&scp->scp_mutex);
-			goto clock_disable;
-		}
-	} while (readl(scp->reg_base + MT8183_HOST_TO_SCP));
-
-	memcpy(send_obj->share_buf, buf, len);
-	send_obj->len = len;
-	send_obj->id = id;
-
-	scp->ipi_id_ack[id] = false;
-	/* send the command to SCP */
-	writel(MT8183_HOST_IPC_INT_BIT, scp->reg_base + MT8183_HOST_TO_SCP);
-
-	mutex_unlock(&scp->scp_mutex);
-
-	if (wait) {
-		/* wait for SCP's ACK */
-		timeout = msecs_to_jiffies(wait);
-		ret = wait_event_timeout(scp->ack_wq,
-					 scp->ipi_id_ack[id],
-					 timeout);
-		scp->ipi_id_ack[id] = false;
-		if (WARN(!ret,
-			 "scp ipi %d ack time out !", id))
-			ret = -EIO;
-		else
-			ret = 0;
-	}
-
-clock_disable:
-	clk_disable_unprepare(scp->clk);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(scp_ipi_send);
-
 struct platform_device *scp_get_plat_device(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
diff --git a/drivers/remoteproc/mtk_scp_ipi.c b/drivers/remoteproc/mtk_scp_ipi.c
new file mode 100644
index 0000000000000..6923066869874
--- /dev/null
+++ b/drivers/remoteproc/mtk_scp_ipi.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2018 MediaTek Inc.
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_data/mtk_scp.h>
+#include <linux/platform_device.h>
+
+#include "mtk_common.h"
+
+#define IPI_TIMEOUT_MS		2000
+
+int scp_ipi_register(struct platform_device *pdev,
+		     enum ipi_id id,
+		     ipi_handler_t handler,
+		     const char *name,
+		     void *priv)
+{
+	struct mtk_scp *scp = platform_get_drvdata(pdev);
+	struct scp_ipi_desc *ipi_desc;
+
+	if (!scp) {
+		dev_err(&pdev->dev, "scp device is not ready\n");
+		return -EPROBE_DEFER;
+	}
+
+	if (WARN(id < 0 || id >= IPI_MAX || handler == NULL,
+	    "register scp ipi id %d with invalid arguments\n", id))
+		return -EINVAL;
+
+	ipi_desc = scp->ipi_desc;
+	ipi_desc[id].name = name;
+	ipi_desc[id].handler = handler;
+	ipi_desc[id].priv = priv;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(scp_ipi_register);
+
+int scp_ipi_send(struct platform_device *pdev,
+		 enum ipi_id id, void *buf,
+		 unsigned int len,
+		 unsigned int wait)
+{
+	struct mtk_scp *scp = platform_get_drvdata(pdev);
+	struct share_obj *send_obj = scp->send_buf;
+	unsigned long timeout;
+	int ret;
+
+	if (WARN(id <= IPI_SCP_INIT || id >= IPI_MAX ||
+	    len > sizeof(send_obj->share_buf) || !buf,
+	    "failed to send ipi message\n"))
+		return -EINVAL;
+
+	ret = clk_prepare_enable(scp->clk);
+	if (ret) {
+		dev_err(scp->dev, "failed to enable clock\n");
+		return ret;
+	}
+
+	mutex_lock(&scp->scp_mutex);
+
+	 /* Wait until SCP receives the last command */
+	timeout = jiffies + msecs_to_jiffies(IPI_TIMEOUT_MS);
+	do {
+		if (time_after(jiffies, timeout)) {
+			dev_err(scp->dev, "scp_ipi_send: IPI timeout!\n");
+			ret = -EIO;
+			mutex_unlock(&scp->scp_mutex);
+			goto clock_disable;
+		}
+	} while (readl(scp->reg_base + MT8183_HOST_TO_SCP));
+
+	memcpy(send_obj->share_buf, buf, len);
+	send_obj->len = len;
+	send_obj->id = id;
+
+	scp->ipi_id_ack[id] = false;
+	/* send the command to SCP */
+	writel(MT8183_HOST_IPC_INT_BIT, scp->reg_base + MT8183_HOST_TO_SCP);
+
+	mutex_unlock(&scp->scp_mutex);
+
+	if (wait) {
+		/* wait for SCP's ACK */
+		timeout = msecs_to_jiffies(wait);
+		ret = wait_event_timeout(scp->ack_wq,
+					 scp->ipi_id_ack[id],
+					 timeout);
+		scp->ipi_id_ack[id] = false;
+		if (WARN(!ret,
+			 "scp ipi %d ack time out !", id))
+			ret = -EIO;
+		else
+			ret = 0;
+	}
+
+clock_disable:
+	clk_disable_unprepare(scp->clk);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(scp_ipi_send);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MediaTek scp IPI interface");
-- 
2.20.1.415.g653613c723-goog

[RFC,4/5] rpmsg: add rpmsg support for mt8183 SCP.

[Pi-Hsun Shih]

Add a simple rpmsg support for mt8183 SCP, that use IPI / IPC directly.

Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
---
There are lots of TODO comments on things that are not done yet / I have
doubt on whether it's the best way to do it.

 drivers/remoteproc/mtk_common.h       |   3 +
 drivers/remoteproc/mtk_scp.c          |  30 ++-
 drivers/remoteproc/mtk_scp_ipi.c      |   2 +-
 drivers/rpmsg/Kconfig                 |   5 +
 drivers/rpmsg/Makefile                |   1 +
 drivers/rpmsg/mtk_rpmsg.c             | 341 ++++++++++++++++++++++++++
 include/linux/platform_data/mtk_scp.h |   7 +
 include/linux/rpmsg/mtk_rpmsg.h       |  34 +++
 8 files changed, 419 insertions(+), 4 deletions(-)
 create mode 100644 drivers/rpmsg/mtk_rpmsg.c
 create mode 100644 include/linux/rpmsg/mtk_rpmsg.h

diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h
index ae55fca2ce2d7..cdd4919a402a8 100644
--- a/drivers/remoteproc/mtk_common.h
+++ b/drivers/remoteproc/mtk_common.h
@@ -54,6 +54,9 @@ struct mtk_scp {
 	void __iomem *cpu_addr;
 	phys_addr_t phys_addr;
 	size_t dram_size;
+
+	struct platform_device *pdev;
+	struct rproc_subdev *rpmsg_subdev;
 };
 
 /**
diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
index aaebe96bcfd66..0ed7904809dce 100644
--- a/drivers/remoteproc/mtk_scp.c
+++ b/drivers/remoteproc/mtk_scp.c
@@ -12,6 +12,7 @@
 #include <linux/platform_data/mtk_scp.h>
 #include <linux/platform_device.h>
 #include <linux/remoteproc.h>
+#include <linux/rpmsg/mtk_rpmsg.h>
 
 #include "remoteproc_internal.h"
 #include "mtk_common.h"
@@ -278,6 +279,24 @@ static int scp_map_memory_region(struct mtk_scp *scp)
 	return 0;
 }
 
+static void scp_add_rpmsg_subdev(struct mtk_scp *scp)
+{
+	scp->rpmsg_subdev =
+		mtk_rpmsg_create_rproc_subdev(scp->pdev, scp->rproc);
+	if (scp->rpmsg_subdev) {
+		rproc_add_subdev(scp->rproc, scp->rpmsg_subdev);
+	}
+}
+
+static void scp_remove_rpmsg_subdev(struct mtk_scp *scp)
+{
+	if (scp->rpmsg_subdev) {
+		rproc_remove_subdev(scp->rproc, scp->rpmsg_subdev);
+		mtk_rpmsg_destroy_rproc_subdev(scp->rpmsg_subdev);
+		scp->rpmsg_subdev = NULL;
+	}
+}
+
 static int mtk_scp_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -301,6 +320,7 @@ static int mtk_scp_probe(struct platform_device *pdev)
 	scp = (struct mtk_scp *)rproc->priv;
 	scp->rproc = rproc;
 	scp->dev = dev;
+	scp->pdev = pdev;
 	platform_set_drvdata(pdev, scp);
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram");
@@ -372,13 +392,16 @@ static int mtk_scp_probe(struct platform_device *pdev)
 	init_waitqueue_head(&scp->run.wq);
 	init_waitqueue_head(&scp->ack_wq);
 
+	scp_add_rpmsg_subdev(scp);
+
 	ret = rproc_add(rproc);
 	if (ret)
-		goto destroy_mutex;
+		goto remove_subdev;
 
-	return ret;
+	return 0;
 
-destroy_mutex:
+remove_subdev:
+	scp_remove_rpmsg_subdev(scp);
 	mutex_destroy(&scp->scp_mutex);
 free_rproc:
 	rproc_free(rproc);
@@ -390,6 +413,7 @@ static int mtk_scp_remove(struct platform_device *pdev)
 {
 	struct mtk_scp *scp = platform_get_drvdata(pdev);
 
+	scp_remove_rpmsg_subdev(scp);
 	rproc_del(scp->rproc);
 	rproc_free(scp->rproc);
 
diff --git a/drivers/remoteproc/mtk_scp_ipi.c b/drivers/remoteproc/mtk_scp_ipi.c
index 6923066869874..1b72c5cc72d36 100644
--- a/drivers/remoteproc/mtk_scp_ipi.c
+++ b/drivers/remoteproc/mtk_scp_ipi.c
@@ -50,7 +50,7 @@ int scp_ipi_send(struct platform_device *pdev,
 	unsigned long timeout;
 	int ret;
 
-	if (WARN(id <= IPI_SCP_INIT || id >= IPI_MAX ||
+	if (WARN(id <= IPI_NS_SERVICE || id >= IPI_MAX ||
 	    len > sizeof(send_obj->share_buf) || !buf,
 	    "failed to send ipi message\n"))
 		return -EINVAL;
diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index d0322b41eca54..b8364a397bb60 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -55,4 +55,9 @@ config RPMSG_VIRTIO
 	select RPMSG
 	select VIRTIO
 
+config RPMSG_MTK_SCP
+	tristate "MediaTek SCP"
+	depends on MTK_SCP
+	select RPMSG
+
 endmenu
diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
index 9aa859502d275..a0c1dcefa36ee 100644
--- a/drivers/rpmsg/Makefile
+++ b/drivers/rpmsg/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_RPMSG_QCOM_GLINK_NATIVE) += qcom_glink_native.o
 obj-$(CONFIG_RPMSG_QCOM_GLINK_SMEM) += qcom_glink_smem.o
 obj-$(CONFIG_RPMSG_QCOM_SMD)	+= qcom_smd.o
 obj-$(CONFIG_RPMSG_VIRTIO)	+= virtio_rpmsg_bus.o
+obj-$(CONFIG_RPMSG_MTK_SCP)	+= mtk_rpmsg.o
diff --git a/drivers/rpmsg/mtk_rpmsg.c b/drivers/rpmsg/mtk_rpmsg.c
new file mode 100644
index 0000000000000..66b39eaae51f2
--- /dev/null
+++ b/drivers/rpmsg/mtk_rpmsg.c
@@ -0,0 +1,341 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright 2018 Google LLC.
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_data/mtk_scp.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc.h>
+#include <linux/rpmsg/mtk_rpmsg.h>
+#include <linux/workqueue.h>
+
+#include "rpmsg_internal.h"
+
+// TODO: This is built on top of scp_ipi_register / scp_ipi_send in mtk_scp.h.
+// It's probably better to move the implementation of register / send to here
+// instead of in remoteproc/mtk_scp_ipi.c
+// TODO: Do we need some sort of vring for performance? We may be able to get
+// rid of this file if so, but that would require SCP firmware support too.
+
+struct mtk_rpmsg_device {
+	struct rpmsg_device rpdev;
+
+	struct platform_device *scp_pdev;
+};
+
+struct mtk_rpmsg_endpoint {
+	struct rpmsg_endpoint ept;
+	u32 ipi_id;
+	struct platform_device *scp_pdev;
+};
+
+// TODO: Naming is hard...
+struct mtk_rpmsg_rproc_subdev {
+	struct rproc *scp_rproc;
+	struct platform_device *scp_pdev;
+	struct rpmsg_endpoint *ns_ept;
+	struct rproc_subdev subdev;
+};
+
+#define to_mtk_subdev(d) container_of(d, struct mtk_rpmsg_rproc_subdev, subdev);
+
+struct mtk_register_device_work {
+	struct work_struct register_work;
+	struct platform_device *scp_pdev;
+	char name[RPMSG_NAME_SIZE];
+	u32 addr;
+};
+
+/**
+ * TODO: This is copied from virtio_rpmsg_bus.
+ * struct rpmsg_ns_msg - dynamic name service announcement message
+ * @name: name of remote service that is published
+ * @addr: address of remote service that is published
+ *
+ * This message is sent across to publish a new service, or announce
+ * about its removal. When we receive these messages, an appropriate
+ * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
+ * or ->remove() handler of the appropriate rpmsg driver will be invoked
+ * (if/as-soon-as one is registered).
+ */
+struct rpmsg_ns_msg {
+	char name[RPMSG_NAME_SIZE];
+	u32 addr;
+} __packed;
+
+#define to_scp_device(r)	container_of(r, struct mtk_rpmsg_device, rpdev)
+#define to_scp_endpoint(r)	container_of(r, struct mtk_rpmsg_endpoint, ept)
+
+static const struct rpmsg_endpoint_ops mtk_rpmsg_endpoint_ops;
+
+static void __ept_release(struct kref *kref)
+{
+	struct rpmsg_endpoint *ept = container_of(kref, struct rpmsg_endpoint,
+						  refcount);
+	kfree(to_scp_endpoint(ept));
+}
+
+static void mtk_rpmsg_ipi_handler(void *data, unsigned int len, void *priv)
+{
+	struct mtk_rpmsg_endpoint *mept = priv;
+	struct rpmsg_endpoint *ept = &mept->ept;
+
+	// TODO: What if the cb() returns error?
+	(*ept->cb)(ept->rpdev, data, len, ept->priv, mept->ipi_id);
+}
+
+static struct rpmsg_endpoint *
+__rpmsg_create_ept(struct platform_device *scp_pdev, struct rpmsg_device *rpdev,
+		   rpmsg_rx_cb_t cb, void *priv,
+		   u32 id, const char *name)
+{
+	struct mtk_rpmsg_endpoint *mept;
+	struct rpmsg_endpoint *ept;
+
+	int ret;
+
+	mept = kzalloc(sizeof(*mept), GFP_KERNEL);
+	if (!mept)
+		return NULL;
+	mept->ipi_id = id;
+	mept->scp_pdev = scp_pdev;
+
+	ept = &mept->ept;
+	kref_init(&ept->refcount);
+
+	ept->rpdev = rpdev;
+	ept->cb = cb;
+	ept->priv = priv;
+	ept->ops = &mtk_rpmsg_endpoint_ops;
+
+	ret = scp_ipi_register(scp_pdev, id, mtk_rpmsg_ipi_handler, name, mept);
+	if (ret) {
+		dev_err(&scp_pdev->dev, "ipi register failed, id = %d", id);
+		kref_put(&ept->refcount, __ept_release);
+		return NULL;
+	}
+
+	return ept;
+}
+
+static struct rpmsg_endpoint *
+mtk_rpmsg_create_ept(struct rpmsg_device *rpdev, rpmsg_rx_cb_t cb, void *priv,
+		     struct rpmsg_channel_info chinfo)
+{
+	struct platform_device *scp_pdev = to_scp_device(rpdev)->scp_pdev;
+	// TODO: Is using src as IPI id "correct"?
+	return __rpmsg_create_ept(scp_pdev, rpdev, cb, priv, chinfo.src,
+				  chinfo.name);
+}
+
+static const struct rpmsg_device_ops mtk_rpmsg_device_ops = {
+	.create_ept = mtk_rpmsg_create_ept,
+};
+
+static void mtk_rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
+{
+	kref_put(&ept->refcount, __ept_release);
+}
+
+static int __mtk_rpmsg_send(struct mtk_rpmsg_endpoint *mept, void *data,
+			    int len, bool wait)
+{
+	// TODO: The "wait" is not same as what scp_ipi_send's "wait" means, so
+	// this is not correct.
+	// (first wait for there's space of tx buffer, second wait for the ack
+	// from scp.)
+	return scp_ipi_send(mept->scp_pdev, mept->ipi_id, data, len,
+			    wait ? 200 : 0);
+}
+
+static int mtk_rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len)
+{
+	struct mtk_rpmsg_endpoint *mept = to_scp_endpoint(ept);
+
+	return __mtk_rpmsg_send(mept, data, len, true);
+}
+
+static int mtk_rpmsg_trysend(struct rpmsg_endpoint *ept, void *data, int len)
+{
+	struct mtk_rpmsg_endpoint *mept = to_scp_endpoint(ept);
+
+	return __mtk_rpmsg_send(mept, data, len, false);
+}
+
+static const struct rpmsg_endpoint_ops mtk_rpmsg_endpoint_ops = {
+	.destroy_ept = mtk_rpmsg_destroy_ept,
+	.send = mtk_rpmsg_send,
+	.trysend = mtk_rpmsg_trysend,
+};
+
+static void mtk_rpmsg_release_device(struct device *dev)
+{
+	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+	struct mtk_rpmsg_device *mdev = to_scp_device(rpdev);
+
+	kfree(mdev);
+}
+
+static void mtk_register_device_work_function(struct work_struct *register_work)
+{
+	struct mtk_rpmsg_device *mdev;
+	struct rpmsg_device *rpdev;
+
+	struct mtk_register_device_work *work = container_of(
+		register_work, struct mtk_register_device_work, register_work);
+	struct platform_device *scp_pdev = work->scp_pdev;
+
+	int ret;
+
+	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
+	if (!mdev) {
+		goto free_work;
+	}
+
+	mdev->scp_pdev = scp_pdev;
+
+	rpdev = &mdev->rpdev;
+	rpdev->ops = &mtk_rpmsg_device_ops;
+	rpdev->src = work->addr;
+	rpdev->dst = RPMSG_ADDR_ANY;
+	strncpy(rpdev->id.name, work->name, RPMSG_NAME_SIZE);
+
+	rpdev->dev.parent = &scp_pdev->dev;
+	rpdev->dev.release = mtk_rpmsg_release_device;
+
+	ret = rpmsg_register_device(rpdev);
+
+	if (ret) {
+		dev_err(&scp_pdev->dev, "rpmsg register device failed\n");
+		goto free_work;
+	}
+	return;
+
+free_work:
+	// TODO: Is it reasonable to free the work itself in the handler?
+	kfree(register_work);
+}
+
+static int mtk_rpmsg_create_device(struct platform_device *scp_pdev,
+				    char *name, u32 addr)
+{
+	struct mtk_register_device_work *work;
+
+	// This is called in interrupt context from name service callback.
+	work = kzalloc(sizeof(*work), GFP_ATOMIC);
+	if (!work)
+		return -ENOMEM;
+
+	INIT_WORK(&work->register_work, mtk_register_device_work_function);
+	work->scp_pdev = scp_pdev;
+	strncpy(work->name, name, RPMSG_NAME_SIZE);
+	work->addr = addr;
+	// rpmsg_register_device can't be called in interrupt context, so need
+	// to schedule a work for it.
+	// TODO: Or should we change mtk_scp to call the callback in
+	// non-interrupt context?
+	schedule_work(&work->register_work);
+	return 0;
+}
+
+static int mtk_rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
+			   void *priv, u32 src)
+{
+	struct rpmsg_ns_msg *msg = data;
+	struct platform_device *scp_pdev = priv;
+	struct device *dev = &scp_pdev->dev;
+
+	int ret;
+
+	if (len != sizeof(*msg)) {
+		dev_err(dev, "malformed ns msg (%d)\n", len);
+		return -EINVAL;
+	}
+
+	/*
+	 * the name service ept does _not_ belong to a real rpmsg channel,
+	 * and is handled by the rpmsg bus itself.
+	 * for sanity reasons, make sure a valid rpdev has _not_ sneaked
+	 * in somehow.
+	 */
+	if (rpdev) {
+		dev_err(dev, "anomaly: ns ept has an rpdev handle\n");
+		return -EINVAL;
+	}
+
+	/* don't trust the remote processor for null terminating the name */
+	msg->name[RPMSG_NAME_SIZE - 1] = '\0';
+
+	dev_info(dev, "creating channel %s addr 0x%x\n", msg->name, msg->addr);
+
+	ret = mtk_rpmsg_create_device(scp_pdev, msg->name, msg->addr);
+	if (ret) {
+		dev_err(dev, "create rpmsg device failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+int mtk_rpmsg_prepare(struct rproc_subdev *subdev)
+{
+	struct mtk_rpmsg_rproc_subdev *mtk_subdev;
+	struct platform_device *scp_pdev;
+
+	mtk_subdev = to_mtk_subdev(subdev);
+	scp_pdev = mtk_subdev->scp_pdev;
+
+	/* a dedicated endpoint handles the name service msgs */
+	mtk_subdev->ns_ept =
+		__rpmsg_create_ept(scp_pdev, NULL, mtk_rpmsg_ns_cb, scp_pdev,
+				   IPI_NS_SERVICE, "name-service");
+	if (!mtk_subdev->ns_ept) {
+		dev_err(&scp_pdev->dev,
+			"failed to create name service endpoint\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+void mtk_rpmsg_unprepare(struct rproc_subdev *subdev)
+{
+	struct mtk_rpmsg_rproc_subdev *mtk_subdev = to_mtk_subdev(subdev);
+
+	kref_put(&mtk_subdev->ns_ept->refcount, __ept_release);
+	// TODO: unregister all rpmsg devices created by name service here.
+}
+
+struct rproc_subdev *
+mtk_rpmsg_create_rproc_subdev(struct platform_device *scp_pdev,
+			      struct rproc *scp_rproc)
+{
+	struct mtk_rpmsg_rproc_subdev *mtk_subdev;
+	struct device *dev = &scp_pdev->dev;
+
+	mtk_subdev = kzalloc(sizeof(*mtk_subdev), GFP_KERNEL);
+	if (!mtk_subdev) {
+		dev_err(dev, "allocate rproc subdevice failed.\n");
+		return NULL;
+	}
+	mtk_subdev->scp_pdev = scp_pdev;
+	mtk_subdev->scp_rproc = scp_rproc;
+	mtk_subdev->subdev.prepare = mtk_rpmsg_prepare;
+	mtk_subdev->subdev.unprepare = mtk_rpmsg_unprepare;
+
+	return &mtk_subdev->subdev;
+}
+EXPORT_SYMBOL_GPL(mtk_rpmsg_create_rproc_subdev);
+
+void mtk_rpmsg_destroy_rproc_subdev(struct rproc_subdev *subdev)
+{
+	struct mtk_rpmsg_rproc_subdev *mtk_subdev = to_mtk_subdev(subdev);
+
+	kfree(mtk_subdev);
+}
+EXPORT_SYMBOL_GPL(mtk_rpmsg_destroy_rproc_subdev);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MediaTek scp rpmsg driver");
diff --git a/include/linux/platform_data/mtk_scp.h b/include/linux/platform_data/mtk_scp.h
index 118838c91564f..4b3cf4f63fbe9 100644
--- a/include/linux/platform_data/mtk_scp.h
+++ b/include/linux/platform_data/mtk_scp.h
@@ -27,7 +27,14 @@ typedef void (*ipi_handler_t) (void *data,
 
 enum ipi_id {
 	IPI_SCP_INIT = 0,
+	// TODO: Should this just be part of the response of IPI_SCP_INIT
+	// instead? e.g. the initial IPI_SCP_INIT from SCP sends a list of
+	// channels.
+	IPI_NS_SERVICE,
 	IPI_MDP,
+	// TODO: Since we'll not be having all IPI numbers here after the name
+	// service & rpmsg, this IPI_MAX need to be manually specified and sync
+	// with the firmware.
 	IPI_MAX,
 };
 
diff --git a/include/linux/rpmsg/mtk_rpmsg.h b/include/linux/rpmsg/mtk_rpmsg.h
new file mode 100644
index 0000000000000..c55dbd658dde1
--- /dev/null
+++ b/include/linux/rpmsg/mtk_rpmsg.h
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright 2018 Google LLC.
+
+#ifndef __LINUX_RPMSG_MTK_RPMSG_H
+#define __LINUX_RPMSG_MTK_RPMSG_H
+
+#include <linux/device.h>
+#include <linux/remoteproc.h>
+
+#if IS_ENABLED(CONFIG_RPMSG_MTK_SCP)
+
+struct rproc_subdev *
+mtk_rpmsg_create_rproc_subdev(struct platform_device *scp_pdev,
+			      struct rproc *scp_rproc);
+
+void mtk_rpmsg_destroy_rproc_subdev(struct rproc_subdev *subdev);
+
+#else
+
+static inline struct rproc_subdev *
+mtk_rpmsg_create_rproc_subdev(struct platform_device *scp_pdev,
+			      struct rproc *scp_rproc)
+{
+	return NULL;
+}
+
+static inline void mtk_rpmsg_destroy_rproc_subdev(struct rproc_subdev *subdev)
+{
+}
+
+#endif
+
+#endif
-- 
2.20.1.415.g653613c723-goog

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

[Pi-Hsun Shih]

Add EC host command support through rpmsg.

Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
---
 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;
+	}
+
 	/*
 	 * 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
+	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.
+	ret = rpmsg_send(rpdev->ept, ec_dev->dout, len);
+
+	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);
+
+	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;
+
+	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)
+		return ret;
+
+	return 0;
+}
+
+static void cros_ec_rpmsg_remove(struct rpmsg_device *rpdev)
+{
+	struct cros_ec_device *ec_dev = dev_get_drvdata(&rpdev->dev);
+
+	cros_ec_remove(ec_dev);
+}
+
+static const struct rpmsg_device_id cros_ec_rpmsg_device_id[] = {
+	{ .name = "cros-ec-rpmsg", },
+	{ /* sentinel */ },
+};
+
+static struct rpmsg_driver cros_ec_driver_rpmsg = {
+	.drv.name       = KBUILD_MODNAME,
+	.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"
 
 /*
  * 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,
 };
 
 #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32))
-- 
2.20.1.415.g653613c723-goog

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

[Enric Balletbo Serra]

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 <pihsun@chromium.org> del dia dc., 26 de des.
2018 a les 8:57:
>
> Add EC host command support through rpmsg.
>
> Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
> ---
>  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 <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?

> +
> +       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.
>  };
>
>  #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32))
> --
> 2.20.1.415.g653613c723-goog
>

Thanks,
  Enric

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

[Guenter Roeck]

On Thu, Jan 3, 2019 at 8:06 AM Enric Balletbo Serra <eballetbo@gmail.com> 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 <pihsun@chromium.org> del dia dc., 26 de des.
> 2018 a les 8:57:
> >
> > Add EC host command support through rpmsg.
> >
> > Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
> > ---
> >  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 <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?
>
> > +
> > +       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.

Guenter

> >  };
> >
> >  #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32))
> > --
> > 2.20.1.415.g653613c723-goog
> >
>
> Thanks,
>   Enric

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

[Enric Balletbo Serra]

Hi Guenter,

Missatge de Guenter Roeck <groeck@google.com> del dia dj., 3 de gen.
2019 a les 17:08:
>
> On Thu, Jan 3, 2019 at 8:06 AM Enric Balletbo Serra <eballetbo@gmail.com> 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 <pihsun@chromium.org> del dia dc., 26 de des.
> > 2018 a les 8:57:
> > >
> > > Add EC host command support through rpmsg.
> > >
> > > Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
> > > ---
> > >  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 <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?
> >
> > > +
> > > +       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?

Thanks,
 Enric

> Guenter
>
> > >  };
> > >
> > >  #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32))
> > > --
> > > 2.20.1.415.g653613c723-goog
> > >
> >
> > Thanks,
> >   Enric

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

[Guenter Roeck]

On Thu, Jan 3, 2019 at 8:39 AM Enric Balletbo Serra <eballetbo@gmail.com> wrote:
>
> Hi Guenter,
>
> Missatge de Guenter Roeck <groeck@google.com> del dia dj., 3 de gen.
> 2019 a les 17:08:
> >
> > On Thu, Jan 3, 2019 at 8:06 AM Enric Balletbo Serra <eballetbo@gmail.com> 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 <pihsun@chromium.org> del dia dc., 26 de des.
> > > 2018 a les 8:57:
> > > >
> > > > Add EC host command support through rpmsg.
> > > >
> > > > Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
> > > > ---
> > > >  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 <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?
> > >
> > > > +
> > > > +       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

Re: [RFC,1/5] dt-bindings: Add a binding for Mediatek SCP

[Rob Herring]

On Wed, Dec 26, 2018 at 03:53:09PM +0800, Pi-Hsun Shih wrote:
> From: Erin Lo <erin.lo@mediatek.com>
> 
> Add a DT binding documentation of SCP for the
> MT8183 SoC from Mediatek.
> 
> Signed-off-by: Erin Lo <erin.lo@mediatek.com>
> ---
>  .../devicetree/bindings/remoteproc/mtk,scp.txt         | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/mtk,scp.txt
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/mtk,scp.txt b/Documentation/devicetree/bindings/remoteproc/mtk,scp.txt
> new file mode 100644
> index 0000000000000..b07e5c4ca9af1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.txt
> @@ -0,0 +1,9 @@
> +Mediatek SCP Bindings
> +----------------------------------------
> +
> +This binding provides support for ARM Cortex M4 Co-processor found on some
> +Mediatek SoCs.
> +
> +Required properties:
> +- compatible		Should be "mediatek,mt8183-scp"
> +- clocks		Clock for co-processor (See: ../clock/clock-bindings.txt)

Seems like this is going to be incomplete considering this used for the 
CrOS EC.

Rob

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

[Peter Shih]

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
<eballetbo@gmail.com> 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 <pihsun@chromium.org> del dia dc., 26 de des.
> 2018 a les 8:57:
> >
> > Add EC host command support through rpmsg.
> >
> > Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
> > ---
> >  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.

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?

>
> > +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

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

[Enric Balletbo Serra]

Hi Peter,

Missatge de Peter Shih <pihsun@chromium.org> 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
> <eballetbo@gmail.com> 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 <pihsun@chromium.org> del dia dc., 26 de des.
> > 2018 a les 8:57:
> > >
> > > Add EC host command support through rpmsg.
> > >
> > > Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
> > > ---
> > >  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

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

[Peter Shih]

On Fri, Jan 4, 2019 at 7:39 PM Enric Balletbo Serra <eballetbo@gmail.com> wrote:
>
> Hi Peter,
>
> Missatge de Peter Shih <pihsun@chromium.org> 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
> > <eballetbo@gmail.com> 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 <pihsun@chromium.org> del dia dc., 26 de des.
> > > 2018 a les 8:57:
> > > >
> > > > Add EC host command support through rpmsg.
> > > >
> > > > Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
> > > > ---
> > > >  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