[PATCH v4 1/6] dt-bindings: Add a binding for Mediatek SCP

[PATCH v4 1/6] 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes from v3:
 - No change.

Changes from v2:
 - No change. I realized that for this patch series, there's no need to
   add anything under the mt8183-scp node (neither the mt8183-rpmsg or
   the cros-ec-rpmsg) for them to work, since mt8183-rpmsg is added
   directly as a rproc_subdev by code, and cros-ec-rpmsg is dynamically
   created by SCP name service.

Changes from v1:
 - No change.
---
 .../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 00000000000000..b07e5c4ca9af1d
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.txt
@@ -0,0 +1,10 @@
+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.611.gfbb209baf1-goog

[PATCH v4 2/6] 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>
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
---
Changes from v3:
 - Fix some issue found by checkpatch.
 - Make writes aligned in scp_ipi_send.

Changes from v2:
 - Squash patch 3 from v2 (separate the ipi interface) into this patch.
 - Remove unused name argument from scp_ipi_register.
 - Add scp_ipi_unregister for proper cleanup.
 - Move IPI ids in sync with firmware.
 - Add mb() in proper place, and correctly clear the run->signaled.

Changes from v1:
 - Extract functions and rename variables in mtk_scp.c.
---
 drivers/remoteproc/Kconfig            |   9 +
 drivers/remoteproc/Makefile           |   1 +
 drivers/remoteproc/mtk_common.h       |  73 +++++
 drivers/remoteproc/mtk_scp.c          | 441 ++++++++++++++++++++++++++
 drivers/remoteproc/mtk_scp_ipi.c      | 157 +++++++++
 include/linux/platform_data/mtk_scp.h | 135 ++++++++
 6 files changed, 816 insertions(+)
 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 include/linux/platform_data/mtk_scp.h

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index f0abd260804473..ee0bda23768938 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 ce5d061e92be52..16b3e5e7a81c8e 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 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 00000000000000..162798c44ad7f1
--- /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 {
+	scp_ipi_handler_t handler;
+	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[SCP_IPI_MAX];
+	bool ipi_id_ack[SCP_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
new file mode 100644
index 00000000000000..920c81c3525c2a
--- /dev/null
+++ b/drivers/remoteproc/mtk_scp.c
@@ -0,0 +1,441 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2018 MediaTek Inc.
+
+#include <asm/barrier.h>
+#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 "mtk_common.h"
+#include "remoteproc_internal.h"
+
+#define MAX_CODE_SIZE 0x500000
+
+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 < SCP_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 void mtk_scp_reset_assert(const struct mtk_scp *scp)
+{
+	u32 val;
+
+	val = readl(scp->reg_base + MT8183_SW_RSTN);
+	val &= ~MT8183_SW_RSTN_BIT;
+	writel(val, scp->reg_base + MT8183_SW_RSTN);
+}
+
+static void mtk_scp_reset_deassert(const struct mtk_scp *scp)
+{
+	u32 val;
+
+	val = readl(scp->reg_base + MT8183_SW_RSTN);
+	val |= MT8183_SW_RSTN_BIT;
+	writel(val, scp->reg_base + MT8183_SW_RSTN);
+}
+
+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);
+	}
+
+	/*
+	 * Ensure that all write to SRAM are committed before another
+	 * interrupt.
+	 */
+	mb();
+	/* 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;
+
+	/* Hold SCP in reset while loading FW. */
+	mtk_scp_reset_assert(scp);
+
+	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;
+	int ret;
+
+	ret = clk_prepare_enable(scp->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clocks\n");
+		return ret;
+	}
+
+	run = &scp->run;
+	run->signaled = false;
+
+	mtk_scp_reset_deassert(scp);
+
+	ret = wait_event_interruptible_timeout(
+					run->wq,
+					run->signaled,
+					msecs_to_jiffies(2000));
+
+	if (ret == 0) {
+		dev_err(dev, "wait scp initialization timeout!\n");
+		ret = -ETIME;
+		goto stop;
+	}
+	if (ret == -ERESTARTSYS) {
+		dev_err(dev, "wait scp interrupted by a signal!\n");
+		goto stop;
+	}
+	dev_info(dev, "scp is ready. Fw version %s\n", run->fw_ver);
+
+	return 0;
+
+stop:
+	mtk_scp_reset_assert(scp);
+	clk_disable_unprepare(scp->clk);
+	return ret;
+}
+
+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;
+
+	mtk_scp_reset_assert(scp);
+	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,
+			       SCP_IPI_INIT,
+			       scp_init_ipi_handler,
+			       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/drivers/remoteproc/mtk_scp_ipi.c b/drivers/remoteproc/mtk_scp_ipi.c
new file mode 100644
index 00000000000000..51b8449a692970
--- /dev/null
+++ b/drivers/remoteproc/mtk_scp_ipi.c
@@ -0,0 +1,157 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2018 MediaTek Inc.
+
+#include <asm/barrier.h>
+#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"
+
+int scp_ipi_register(struct platform_device *pdev,
+		     enum scp_ipi_id id,
+		     scp_ipi_handler_t handler,
+		     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 >= SCP_IPI_MAX || handler == NULL,
+	    "register scp ipi id %d with invalid arguments\n", id))
+		return -EINVAL;
+
+	ipi_desc = scp->ipi_desc;
+	ipi_desc[id].handler = handler;
+	ipi_desc[id].priv = priv;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(scp_ipi_register);
+
+void scp_ipi_unregister(struct platform_device *pdev, enum scp_ipi_id id)
+{
+	struct mtk_scp *scp = platform_get_drvdata(pdev);
+	struct scp_ipi_desc *ipi_desc;
+
+	if (!scp)
+		return;
+
+	if (WARN(id < 0 || id >= SCP_IPI_MAX,
+		 "unregister scp ipi id %d with invalid arguments\n", id))
+		return;
+
+	ipi_desc = scp->ipi_desc;
+	ipi_desc[id].handler = NULL;
+	ipi_desc[id].priv = NULL;
+}
+EXPORT_SYMBOL_GPL(scp_ipi_unregister);
+
+static void memcpy_aligned(void *dst, const void *src, unsigned int len)
+{
+	void *ptr;
+	u32 val;
+	unsigned int i = 0;
+
+	if (!IS_ALIGNED((unsigned long)dst, 4)) {
+		ptr = (void *)ALIGN_DOWN((unsigned long)dst, 4);
+		i = 4 - (dst - ptr);
+		val = readl_relaxed(ptr);
+		memcpy((u8 *)&val + (4 - i), src, i);
+		writel_relaxed(val, ptr);
+	}
+
+	while (i + 4 <= len) {
+		val = *((u32 *)(src + i));
+		writel_relaxed(val, dst + i);
+		i += 4;
+	}
+	if (i < len) {
+		val = readl_relaxed(dst + i);
+		memcpy(&val, src + i, len - i);
+		writel_relaxed(val, dst + i);
+	}
+}
+
+int scp_ipi_send(struct platform_device *pdev,
+		 enum scp_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 <= SCP_IPI_INIT || id >= SCP_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(2000);
+	do {
+		if (time_after(jiffies, timeout)) {
+			dev_err(scp->dev, "%s: IPI timeout!\n", __func__);
+			ret = -EIO;
+			mutex_unlock(&scp->scp_mutex);
+			goto clock_disable;
+		}
+	} while (readl(scp->reg_base + MT8183_HOST_TO_SCP));
+
+	memcpy_aligned(send_obj->share_buf, buf, len);
+
+	send_obj->len = len;
+	send_obj->id = id;
+
+	scp->ipi_id_ack[id] = false;
+	/*
+	 * Ensure that all write to SRAM are committed before sending the
+	 * interrupt to SCP.
+	 */
+	mb();
+	/* 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");
diff --git a/include/linux/platform_data/mtk_scp.h b/include/linux/platform_data/mtk_scp.h
new file mode 100644
index 00000000000000..0e999ec319446b
--- /dev/null
+++ b/include/linux/platform_data/mtk_scp.h
@@ -0,0 +1,135 @@
+/* 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 (*scp_ipi_handler_t) (void *data,
+				   unsigned int len,
+				   void *priv);
+
+/**
+ * enum ipi_id - the id of inter-processor interrupt
+ *
+ * @SCP_IPI_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 scp_ipi_id {
+	SCP_IPI_INIT = 0,
+	SCP_IPI_VDEC_H264,
+	SCP_IPI_VDEC_VP8,
+	SCP_IPI_VDEC_VP9,
+	SCP_IPI_VENC_H264,
+	SCP_IPI_VENC_VP8,
+	SCP_IPI_MDP,
+	SCP_IPI_CROS_HOST_CMD,
+	SCP_IPI_MAX,
+};
+
+/**
+ * scp_ipi_register - register an ipi function
+ *
+ * @pdev:	SCP platform device
+ * @id:		IPI ID
+ * @handler:	IPI handler
+ * @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 scp_ipi_id id,
+		     scp_ipi_handler_t handler,
+		     void *priv);
+
+/**
+ * scp_ipi_unregister - unregister an ipi function
+ *
+ * @pdev:	SCP platform device
+ * @id:		IPI ID
+ *
+ * Unregister an ipi function to receive ipi interrupt from SCP.
+ */
+void scp_ipi_unregister(struct platform_device *pdev, enum scp_ipi_id id);
+
+/**
+ * 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 scp_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.611.gfbb209baf1-goog

Re: [PATCH v4 2/6] remoteproc/mediatek: add SCP support for mt8183

[Matthias Kaehlcke]

Hi Pi-Hsun Shih,

[excuses if you receive this mail twice, mutt garbled up the headers
and it didn't make it to all recipients/lists]

a few comments inline.

It's the first time I dabble into remoteproc, I don't claim to have a
complete understanding of the driver at this point ;-)

On Thu, Jan 31, 2019 at 05:31:27PM +0800, Pi-Hsun Shih wrote:
> 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>
> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> ---
> Changes from v3:
>  - Fix some issue found by checkpatch.
>  - Make writes aligned in scp_ipi_send.
> 
> Changes from v2:
>  - Squash patch 3 from v2 (separate the ipi interface) into this patch.
>  - Remove unused name argument from scp_ipi_register.
>  - Add scp_ipi_unregister for proper cleanup.
>  - Move IPI ids in sync with firmware.
>  - Add mb() in proper place, and correctly clear the run->signaled.
> 
> Changes from v1:
>  - Extract functions and rename variables in mtk_scp.c.
> ---
>  drivers/remoteproc/Kconfig            |   9 +
>  drivers/remoteproc/Makefile           |   1 +
>  drivers/remoteproc/mtk_common.h       |  73 +++++
>  drivers/remoteproc/mtk_scp.c          | 441 ++++++++++++++++++++++++++
>  drivers/remoteproc/mtk_scp_ipi.c      | 157 +++++++++
>  include/linux/platform_data/mtk_scp.h | 135 ++++++++
>  6 files changed, 816 insertions(+)
>  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 include/linux/platform_data/mtk_scp.h
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index f0abd260804473..ee0bda23768938 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 would be good to spell out SCP somewhere, e.g.

"... support Mediatek's system companion processor (SCP) via ..."

the example is less important IMO, though it's prefectly fine if you
can still squeeze it in ;-)

> +
> +          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 ce5d061e92be52..16b3e5e7a81c8e 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 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 00000000000000..162798c44ad7f1
> --- /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 {
> +	scp_ipi_handler_t handler;
> +	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 */

nit: the scp_ prefix in the name is a bit redundant, maybe just call
it 'lock'?

> +	struct scp_ipi_desc ipi_desc[SCP_IPI_MAX];
> +	bool ipi_id_ack[SCP_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
> new file mode 100644
> index 00000000000000..920c81c3525c2a
> --- /dev/null
> +++ b/drivers/remoteproc/mtk_scp.c
> @@ -0,0 +1,441 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (c) 2018 MediaTek Inc.
> +
> +#include <asm/barrier.h>
> +#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 "mtk_common.h"
> +#include "remoteproc_internal.h"
> +
> +#define MAX_CODE_SIZE 0x500000
> +
> +struct platform_device *scp_get_plat_device(struct platform_device *pdev)

nit: _get_pdev()?

plat_device is a custom abbreviation, pdev is commonly understood.

> +{
> +	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");

nit: use uppercase for acronyms (SCP, IPI, ...) in messages and comments?

> +		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 < SCP_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);
> +	}

You could consider to change this to:

        if (rcv_obj->id >= SCP_IPI_MAX || !ipi_desc[rcv_obj->id].handler) {
                dev_err(scp->dev, "No such ipi id = %d\n", rcv_obj->id);
                return;
        }

        memcpy_fromio(tmp_data, &rcv_obj->share_buf, rcv_obj->len);
        ...

Feel free to leave as is if you prefer.

> +}
> +
> +static int scp_ipi_init(struct mtk_scp *scp)
> +{
> +	size_t send_offset = 0x800 - sizeof(struct share_obj);

should there be a define for 0x800?

> +	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 void mtk_scp_reset_assert(const struct mtk_scp *scp)
> +{
> +	u32 val;
> +
> +	val = readl(scp->reg_base + MT8183_SW_RSTN);
> +	val &= ~MT8183_SW_RSTN_BIT;
> +	writel(val, scp->reg_base + MT8183_SW_RSTN);
> +}
> +
> +static void mtk_scp_reset_deassert(const struct mtk_scp *scp)
> +{
> +	u32 val;
> +
> +	val = readl(scp->reg_base + MT8183_SW_RSTN);
> +	val |= MT8183_SW_RSTN_BIT;
> +	writel(val, scp->reg_base + MT8183_SW_RSTN);
> +}
> +
> +static irqreturn_t scp_irq_handler(int irq, void *priv)

nit: some function use the prefix 'scp_' others 'mtk_scp_', I couldn't
identify a clear pattern. Should we just stick to a single prefix? I
don't have a strong opinion on which, 'scp_' should be good for static
functions, you might want to add 'mtk_' to global ones to avoid
possible clashes in the future.

> +{
> +	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);
> +	}
> +
> +	/*
> +	 * Ensure that all write to SRAM are committed before another

s/write/writes/

> +	 * interrupt.
> +	 */
> +	mb();
> +	/* 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;
> +
> +	/* Hold SCP in reset while loading FW. */
> +	mtk_scp_reset_assert(scp);
> +
> +	ret = clk_prepare_enable(scp->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clocks\n");

should this deassert the reset to leave things as they were?

> +		return ret;
> +	}
> +
> +	writel(0x0, scp->reg_base + MT8183_SCP_SRAM_PDN);

what is the purpose of this write?

> +
> +	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;
> +	int ret;
> +
> +	ret = clk_prepare_enable(scp->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clocks\n");
> +		return ret;
> +	}
> +
> +	run = &scp->run;
> +	run->signaled = false;
> +
> +	mtk_scp_reset_deassert(scp);
> +
> +	ret = wait_event_interruptible_timeout(
> +					run->wq,
> +					run->signaled,
> +					msecs_to_jiffies(2000));
> +
> +	if (ret == 0) {
> +		dev_err(dev, "wait scp initialization timeout!\n");
> +		ret = -ETIME;
> +		goto stop;
> +	}
> +	if (ret == -ERESTARTSYS) {
> +		dev_err(dev, "wait scp interrupted by a signal!\n");
> +		goto stop;
> +	}
> +	dev_info(dev, "scp is ready. Fw version %s\n", run->fw_ver);

s/Fw/FW/

> +
> +	return 0;
> +
> +stop:
> +	mtk_scp_reset_assert(scp);
> +	clk_disable_unprepare(scp->clk);
> +	return ret;
> +}
> +
> +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;
> +
> +	mtk_scp_reset_assert(scp);
> +	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;

initialization is unnecessary

> +
> +	ptr = mtk_scp_da_to_va(scp->rproc, mem_addr, 0);
> +
> +	if (ptr)
> +		return ptr;
> +	else
> +		return ERR_PTR(-EINVAL);

possible readability tweak:

        if (!ptr)
                return ERR_PTR(-EINVAL);

        return ptr;

> +}
> +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");

I assume you don't check 'res', since devm_ioremap_resource() will fail
if it is NULL.

> +	scp->sram_base = devm_ioremap_resource(dev, res);
> +	scp->sram_size = resource_size(res);

However resource_size() will result in a crash if 'res' is NULL. So you
either add a check for 'res' or do resource_size() after the check of
'scp->sram_base' below.

> +	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,
> +			       SCP_IPI_INIT,
> +			       scp_init_ipi_handler,
> +			       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);

shouldn't the interrupt be requested after initializing the mutex and
the waitqueues?

> +
> +	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/drivers/remoteproc/mtk_scp_ipi.c b/drivers/remoteproc/mtk_scp_ipi.c
> new file mode 100644
> index 00000000000000..51b8449a692970
> --- /dev/null
> +++ b/drivers/remoteproc/mtk_scp_ipi.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (c) 2018 MediaTek Inc.
> +
> +#include <asm/barrier.h>
> +#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"
> +
> +int scp_ipi_register(struct platform_device *pdev,
> +		     enum scp_ipi_id id,
> +		     scp_ipi_handler_t handler,
> +		     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 >= SCP_IPI_MAX || handler == NULL,
> +	    "register scp ipi id %d with invalid arguments\n", id))

I think you'd get more useful information with this:

        if (WARN_ON(id < 0) || WARN_ON(id >= SCP_IPI_MAX) || WARN_ON(!handler))
                return -EINVAL;

> +		return -EINVAL;
> +
> +	ipi_desc = scp->ipi_desc;
> +	ipi_desc[id].handler = handler;
> +	ipi_desc[id].priv = priv;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(scp_ipi_register);
> +
> +void scp_ipi_unregister(struct platform_device *pdev, enum scp_ipi_id id)
> +{
> +	struct mtk_scp *scp = platform_get_drvdata(pdev);
> +	struct scp_ipi_desc *ipi_desc;
> +
> +	if (!scp)
> +		return;
> +
> +	if (WARN(id < 0 || id >= SCP_IPI_MAX,
> +		 "unregister scp ipi id %d with invalid arguments\n", id))
> +		return;
> +
> +	ipi_desc = scp->ipi_desc;
> +	ipi_desc[id].handler = NULL;
> +	ipi_desc[id].priv = NULL;
> +}
> +EXPORT_SYMBOL_GPL(scp_ipi_unregister);
> +
> +static void memcpy_aligned(void *dst, const void *src, unsigned int len)
> +{
> +	void *ptr;
> +	u32 val;
> +	unsigned int i = 0;
> +
> +	if (!IS_ALIGNED((unsigned long)dst, 4)) {
> +		ptr = (void *)ALIGN_DOWN((unsigned long)dst, 4);
> +		i = 4 - (dst - ptr);
> +		val = readl_relaxed(ptr);
> +		memcpy((u8 *)&val + (4 - i), src, i);
> +		writel_relaxed(val, ptr);
> +	}
> +
> +	while (i + 4 <= len) {
> +		val = *((u32 *)(src + i));
> +		writel_relaxed(val, dst + i);
> +		i += 4;
> +	}
> +	if (i < len) {
> +		val = readl_relaxed(dst + i);
> +		memcpy(&val, src + i, len - i);
> +		writel_relaxed(val, dst + i);
> +	}
> +}
> +
> +int scp_ipi_send(struct platform_device *pdev,
> +		 enum scp_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 <= SCP_IPI_INIT || id >= SCP_IPI_MAX ||
> +	    len > sizeof(send_obj->share_buf) || !buf,

similar as above, consider a combination of WARN_ONs

> +	    "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;
> +	}

With the clk_enable/disable() outside of the lock the following could happen:

proc 1> clk_prepare_enable()
proc 1> mutex_lock()
proc 1> send data
proc 1> mutex_unlock()
proc 2> clk_prepare_enable()
proc 1> clk_unprepare_disable()
proc 2> mutex_lock()
proc 2> send data => BOOM

> +	mutex_lock(&scp->scp_mutex);
> +
> +	 /* Wait until SCP receives the last command */
> +	timeout = jiffies + msecs_to_jiffies(2000);
> +	do {
> +		if (time_after(jiffies, timeout)) {
> +			dev_err(scp->dev, "%s: IPI timeout!\n", __func__);
> +			ret = -EIO;
> +			mutex_unlock(&scp->scp_mutex);
> +			goto clock_disable;
> +		}
> +	} while (readl(scp->reg_base + MT8183_HOST_TO_SCP));
> +
> +	memcpy_aligned(send_obj->share_buf, buf, len);
> +
> +	send_obj->len = len;
> +	send_obj->id = id;
> +
> +	scp->ipi_id_ack[id] = false;
> +	/*
> +	 * Ensure that all write to SRAM are committed before sending the
> +	 * interrupt to SCP.
> +	 */
> +	mb();
> +	/* 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");
> diff --git a/include/linux/platform_data/mtk_scp.h b/include/linux/platform_data/mtk_scp.h
> new file mode 100644
> index 00000000000000..0e999ec319446b
> --- /dev/null
> +++ b/include/linux/platform_data/mtk_scp.h
> @@ -0,0 +1,135 @@
> +/* 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 (*scp_ipi_handler_t) (void *data,
> +				   unsigned int len,
> +				   void *priv);
> +
> +/**
> + * enum ipi_id - the id of inter-processor interrupt
> + *
> + * @SCP_IPI_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 scp_ipi_id {
> +	SCP_IPI_INIT = 0,
> +	SCP_IPI_VDEC_H264,
> +	SCP_IPI_VDEC_VP8,
> +	SCP_IPI_VDEC_VP9,
> +	SCP_IPI_VENC_H264,
> +	SCP_IPI_VENC_VP8,
> +	SCP_IPI_MDP,
> +	SCP_IPI_CROS_HOST_CMD,
> +	SCP_IPI_MAX,
> +};
> +
> +/**
> + * scp_ipi_register - register an ipi function
> + *
> + * @pdev:	SCP platform device
> + * @id:		IPI ID
> + * @handler:	IPI handler
> + * @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 scp_ipi_id id,
> +		     scp_ipi_handler_t handler,
> +		     void *priv);
> +
> +/**
> + * scp_ipi_unregister - unregister an ipi function
> + *
> + * @pdev:	SCP platform device
> + * @id:		IPI ID
> + *
> + * Unregister an ipi function to receive ipi interrupt from SCP.
> + */
> +void scp_ipi_unregister(struct platform_device *pdev, enum scp_ipi_id id);
> +
> +/**
> + * 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 scp_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 */

Cheers

Matthias

Re: [PATCH v4 2/6] remoteproc/mediatek: add SCP support for mt8183

[Matthias Brugger]

On 31/01/2019 10:31, Pi-Hsun Shih wrote:
> From: Erin Lo <erin.lo@mediatek.com>
[...]
> +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);

IHMO this should be reflected in the binding description

Regards,
Matthias

Re: [PATCH v4 2/6] remoteproc/mediatek: add SCP support for mt8183

[Matthias Brugger]

On 02/02/2019 01:17, Matthias Kaehlcke wrote:
> Hi Pi-Hsun,
> 
> a few comments inline.
> 
> It's the first time I dabble into remoteproc, I don't claim to have a
> complete understanding of the driver at this point ;-)
> 
> On Thu, Jan 31, 2019 at 05:31:27PM +0800, Pi-Hsun Shih wrote:
>> 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>
>> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
>> ---
>> Changes from v3:
>>  - Fix some issue found by checkpatch.
>>  - Make writes aligned in scp_ipi_send.
>>
>> Changes from v2:
>>  - Squash patch 3 from v2 (separate the ipi interface) into this patch.
>>  - Remove unused name argument from scp_ipi_register.
>>  - Add scp_ipi_unregister for proper cleanup.
>>  - Move IPI ids in sync with firmware.
>>  - Add mb() in proper place, and correctly clear the run->signaled.
>>
>> Changes from v1:
>>  - Extract functions and rename variables in mtk_scp.c.
>> ---
>>  drivers/remoteproc/Kconfig            |   9 +
>>  drivers/remoteproc/Makefile           |   1 +
>>  drivers/remoteproc/mtk_common.h       |  73 +++++
>>  drivers/remoteproc/mtk_scp.c          | 441 ++++++++++++++++++++++++++
>>  drivers/remoteproc/mtk_scp_ipi.c      | 157 +++++++++
>>  include/linux/platform_data/mtk_scp.h | 135 ++++++++
>>  6 files changed, 816 insertions(+)
>>  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 include/linux/platform_data/mtk_scp.h
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index f0abd260804473..ee0bda23768938 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 would be good to spell out SCP somewhere, e.g.
> 
> "... support Mediatek's system control processor (SCP) via ..."
> 
> the example is less important IMO, though it's prefectly fine if you
> can still squeeze it in ;-)
> 

In which way is this different from
drivers/soc/mediatek/mtk-scpsys.c ?

Regards,
Matthias (the other one ;)

Re: [PATCH v4 2/6] remoteproc/mediatek: add SCP support for mt8183

[Pi-Hsun Shih]

On Tue, Feb 5, 2019 at 2:02 AM Matthias Brugger <matthias.bgg@gmail.com> wrote:
>
>
>
> On 02/02/2019 01:17, Matthias Kaehlcke wrote:
> > Hi Pi-Hsun,
> >
> > a few comments inline.
> >
> > It's the first time I dabble into remoteproc, I don't claim to have a
> > complete understanding of the driver at this point ;-)
> >
> > On Thu, Jan 31, 2019 at 05:31:27PM +0800, Pi-Hsun Shih wrote:
> >> 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>
> >> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> >> ---
> >> Changes from v3:
> >>  - Fix some issue found by checkpatch.
> >>  - Make writes aligned in scp_ipi_send.
> >>
> >> Changes from v2:
> >>  - Squash patch 3 from v2 (separate the ipi interface) into this patch.
> >>  - Remove unused name argument from scp_ipi_register.
> >>  - Add scp_ipi_unregister for proper cleanup.
> >>  - Move IPI ids in sync with firmware.
> >>  - Add mb() in proper place, and correctly clear the run->signaled.
> >>
> >> Changes from v1:
> >>  - Extract functions and rename variables in mtk_scp.c.
> >> ---
> >>  drivers/remoteproc/Kconfig            |   9 +
> >>  drivers/remoteproc/Makefile           |   1 +
> >>  drivers/remoteproc/mtk_common.h       |  73 +++++
> >>  drivers/remoteproc/mtk_scp.c          | 441 ++++++++++++++++++++++++++
> >>  drivers/remoteproc/mtk_scp_ipi.c      | 157 +++++++++
> >>  include/linux/platform_data/mtk_scp.h | 135 ++++++++
> >>  6 files changed, 816 insertions(+)
> >>  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 include/linux/platform_data/mtk_scp.h
> >>
> >> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> >> index f0abd260804473..ee0bda23768938 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 would be good to spell out SCP somewhere, e.g.
> >
> > "... support Mediatek's system control processor (SCP) via ..."
> >
> > the example is less important IMO, though it's prefectly fine if you
> > can still squeeze it in ;-)
> >
>
> In which way is this different from
> drivers/soc/mediatek/mtk-scpsys.c ?
>
> Regards,
> Matthias (the other one ;)

drivers/remoteprocs/mtk_scp.c is for SCP (System Companion Processor)
on MTK's platform, and drivers/soc/mediatek/mtk-scpsys.c is for SCPSYS
(System Control Processor System). Despite the similar name, they're
not relevant.

Re: [PATCH v4 2/6] remoteproc/mediatek: add SCP support for mt8183

[Pi-Hsun Shih]

Hi,

Thanks for the review, would address most comments in next version of patch.

On Sat, Feb 2, 2019 at 8:17 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Hi Pi-Hsun,
>
> a few comments inline.
>
> It's the first time I dabble into remoteproc, I don't claim to have a
> complete understanding of the driver at this point ;-)
>
> On Thu, Jan 31, 2019 at 05:31:27PM +0800, Pi-Hsun Shih wrote:
> > diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> > new file mode 100644
> > index 00000000000000..920c81c3525c2a
> > --- /dev/null
> > +++ b/drivers/remoteproc/mtk_scp.c
> > +             return ret;
> > +     }
> > +
> > +     writel(0x0, scp->reg_base + MT8183_SCP_SRAM_PDN);
>
> what is the purpose of this write?
>

Answer from Erin:
The code needs to execute on scp’s SRAM.
We need to turn on the power of SRAM before using it.

Re: [PATCH v4 2/6] remoteproc/mediatek: add SCP support for mt8183

[Matthias Kaehlcke]

On Wed, Feb 13, 2019 at 11:20:18AM +0800, Pi-Hsun Shih wrote:
> Hi,
> 
> Thanks for the review, would address most comments in next version of patch.
> 
> On Sat, Feb 2, 2019 at 8:17 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > Hi Pi-Hsun,
> >
> > a few comments inline.
> >
> > It's the first time I dabble into remoteproc, I don't claim to have a
> > complete understanding of the driver at this point ;-)
> >
> > On Thu, Jan 31, 2019 at 05:31:27PM +0800, Pi-Hsun Shih wrote:
> > > diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> > > new file mode 100644
> > > index 00000000000000..920c81c3525c2a
> > > --- /dev/null
> > > +++ b/drivers/remoteproc/mtk_scp.c
> > > +             return ret;
> > > +     }
> > > +
> > > +     writel(0x0, scp->reg_base + MT8183_SCP_SRAM_PDN);
> >
> > what is the purpose of this write?
> >
> 
> Answer from Erin:
> The code needs to execute on scp’s SRAM.
> We need to turn on the power of SRAM before using it.

Please add a brief comment since this is not evident from the code.

Thanks

Matthias

[PATCH v4 3/6] 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>
---
Changes from v3:
 - Change from unprepare to stop, to stop the rpmsg driver before the
   rproc is stopped, avoiding problem that some rpmsg would fail after
   rproc is stopped.
 - Add missing spin_lock_init, and use destroy_ept instead of kref_put.

Changes from v2:
 - Unregiser IPI handler on unprepare.
 - Lock the channel list on operations.
 - Move SCP_IPI_NS_SERVICE to 0xFF.

Changes from v1:
 - Do cleanup properly in mtk_rpmsg.c, which also removes the problem of
   short-lived work items.
 - Fix several issues checkpatch found.
---
 drivers/remoteproc/mtk_common.h       |   3 +
 drivers/remoteproc/mtk_scp.c          |  29 +-
 drivers/remoteproc/mtk_scp_ipi.c      |   5 +-
 drivers/rpmsg/Kconfig                 |  10 +
 drivers/rpmsg/Makefile                |   1 +
 drivers/rpmsg/mtk_rpmsg.c             | 384 ++++++++++++++++++++++++++
 include/linux/platform_data/mtk_scp.h |   6 +-
 include/linux/rpmsg/mtk_rpmsg.h       |  35 +++
 8 files changed, 466 insertions(+), 7 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 162798c44ad7f1..7efe2e7374e5ec 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 920c81c3525c2a..c0375119c53289 100644
--- a/drivers/remoteproc/mtk_scp.c
+++ b/drivers/remoteproc/mtk_scp.c
@@ -13,6 +13,7 @@
 #include <linux/platform_data/mtk_scp.h>
 #include <linux/platform_device.h>
 #include <linux/remoteproc.h>
+#include <linux/rpmsg/mtk_rpmsg.h>
 
 #include "mtk_common.h"
 #include "remoteproc_internal.h"
@@ -303,6 +304,23 @@ 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;
@@ -326,6 +344,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");
@@ -396,13 +415,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);
@@ -414,6 +436,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 51b8449a692970..1026089335a5c3 100644
--- a/drivers/remoteproc/mtk_scp_ipi.c
+++ b/drivers/remoteproc/mtk_scp_ipi.c
@@ -93,8 +93,9 @@ int scp_ipi_send(struct platform_device *pdev,
 	int ret;
 
 	if (WARN(id <= SCP_IPI_INIT || id >= SCP_IPI_MAX ||
-	    len > sizeof(send_obj->share_buf) || !buf,
-	    "failed to send ipi message\n"))
+			 id == SCP_IPI_NS_SERVICE ||
+			 len > sizeof(send_obj->share_buf) || !buf,
+		 "failed to send ipi message\n"))
 		return -EINVAL;
 
 	ret = clk_prepare_enable(scp->clk);
diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index d0322b41eca54c..270eca663fb8c0 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -15,6 +15,16 @@ config RPMSG_CHAR
 	  in /dev. They make it possible for user-space programs to send and
 	  receive rpmsg packets.
 
+config RPMSG_MTK_SCP
+	tristate "MediaTek SCP"
+	depends on MTK_SCP
+	select RPMSG
+	help
+	  Say y here to enable support providing communication channels to
+	  remote processors in MediaTek platforms.
+	  This use IPI and IPC to communicate with remote processors, with a
+	  send and receive buffer of size 1.
+
 config RPMSG_QCOM_GLINK_NATIVE
 	tristate
 	select RPMSG
diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
index 9aa859502d2752..ae92a7fb08f623 100644
--- a/drivers/rpmsg/Makefile
+++ b/drivers/rpmsg/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_RPMSG)		+= rpmsg_core.o
 obj-$(CONFIG_RPMSG_CHAR)	+= rpmsg_char.o
+obj-$(CONFIG_RPMSG_MTK_SCP)	+= mtk_rpmsg.o
 obj-$(CONFIG_RPMSG_QCOM_GLINK_RPM) += qcom_glink_rpm.o
 obj-$(CONFIG_RPMSG_QCOM_GLINK_NATIVE) += qcom_glink_native.o
 obj-$(CONFIG_RPMSG_QCOM_GLINK_SMEM) += qcom_glink_smem.o
diff --git a/drivers/rpmsg/mtk_rpmsg.c b/drivers/rpmsg/mtk_rpmsg.c
new file mode 100644
index 00000000000000..55498ff0a3497d
--- /dev/null
+++ b/drivers/rpmsg/mtk_rpmsg.c
@@ -0,0 +1,384 @@
+// 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"
+
+struct mtk_rpmsg_device {
+	struct rpmsg_device rpdev;
+	struct platform_device *scp_pdev;
+};
+
+struct mtk_rpmsg_endpoint {
+	struct rpmsg_endpoint ept;
+	struct platform_device *scp_pdev;
+};
+
+struct mtk_rpmsg_rproc_subdev {
+	struct rproc *scp_rproc;
+	struct platform_device *scp_pdev;
+	struct rpmsg_endpoint *ns_ept;
+	struct rproc_subdev subdev;
+
+	struct work_struct register_work;
+	struct list_head channels;
+	spinlock_t channels_lock;
+};
+
+#define to_mtk_subdev(d) container_of(d, struct mtk_rpmsg_rproc_subdev, subdev)
+
+struct mtk_rpmsg_channel_info {
+	struct rpmsg_channel_info info;
+	bool registered;
+	struct list_head list;
+};
+
+/**
+ * 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. When we receive these
+ * messages, an appropriate rpmsg channel (i.e device) is created. 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;
+	int ret;
+
+	ret = (*ept->cb)(ept->rpdev, data, len, ept->priv, ept->addr);
+	if (ret)
+		dev_warn(&ept->rpdev->dev, "rpmsg handler return error = %d",
+			 ret);
+}
+
+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)
+{
+	struct mtk_rpmsg_endpoint *mept;
+	struct rpmsg_endpoint *ept;
+
+	int ret;
+
+	mept = kzalloc(sizeof(*mept), GFP_KERNEL);
+	if (!mept)
+		return NULL;
+	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;
+	ept->addr = id;
+
+	ret = scp_ipi_register(scp_pdev, id, mtk_rpmsg_ipi_handler, 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;
+
+	return __rpmsg_create_ept(scp_pdev, rpdev, cb, priv, chinfo.src);
+}
+
+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)
+{
+	struct mtk_rpmsg_endpoint *mept = to_scp_endpoint(ept);
+
+	scp_ipi_unregister(mept->scp_pdev, ept->addr);
+	kref_put(&ept->refcount, __ept_release);
+}
+
+static int __mtk_rpmsg_send(struct mtk_rpmsg_endpoint *mept, void *data,
+			    int len, bool wait)
+{
+	/*
+	 * TODO: This currently ignore the "wait" argument, and always wait
+	 * until SCP receive the last command.
+	 */
+	return scp_ipi_send(mept->scp_pdev, mept->ept.addr, data, len, 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 int mtk_rpmsg_register_device(struct platform_device *scp_pdev,
+				     struct rpmsg_channel_info *info)
+{
+	struct rpmsg_device *rpdev;
+	struct mtk_rpmsg_device *mdev;
+	int ret;
+
+	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
+	if (!mdev)
+		return -ENOMEM;
+
+	mdev->scp_pdev = scp_pdev;
+
+	rpdev = &mdev->rpdev;
+	rpdev->ops = &mtk_rpmsg_device_ops;
+	rpdev->src = info->src;
+	rpdev->dst = info->dst;
+	strncpy(rpdev->id.name, info->name, RPMSG_NAME_SIZE);
+
+	rpdev->dev.parent = &scp_pdev->dev;
+	rpdev->dev.release = mtk_rpmsg_release_device;
+
+	ret = rpmsg_register_device(rpdev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void mtk_register_device_work_function(struct work_struct *register_work)
+{
+	struct mtk_rpmsg_rproc_subdev *mtk_subdev = container_of(
+		register_work, struct mtk_rpmsg_rproc_subdev, register_work);
+	struct platform_device *scp_pdev = mtk_subdev->scp_pdev;
+	struct mtk_rpmsg_channel_info *info;
+
+	int ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&mtk_subdev->channels_lock, flags);
+	list_for_each_entry(info, &mtk_subdev->channels, list) {
+		if (info->registered)
+			continue;
+
+		spin_unlock_irqrestore(&mtk_subdev->channels_lock, flags);
+		ret = mtk_rpmsg_register_device(scp_pdev, &info->info);
+		spin_lock_irqsave(&mtk_subdev->channels_lock, flags);
+
+		if (ret) {
+			dev_err(&scp_pdev->dev, "Can't create rpmsg_device\n");
+			continue;
+		}
+
+		info->registered = true;
+	}
+	spin_unlock_irqrestore(&mtk_subdev->channels_lock, flags);
+}
+
+static int mtk_rpmsg_create_device(struct mtk_rpmsg_rproc_subdev *mtk_subdev,
+				   char *name, u32 addr)
+{
+	struct mtk_rpmsg_channel_info *info;
+	unsigned long flags;
+
+	/* This is called in interrupt context from name service callback. */
+	info = kzalloc(sizeof(*info), GFP_ATOMIC);
+	if (!info)
+		return -ENOMEM;
+
+	strncpy(info->info.name, name, RPMSG_NAME_SIZE);
+	info->info.src = addr;
+	info->info.dst = RPMSG_ADDR_ANY;
+	spin_lock_irqsave(&mtk_subdev->channels_lock, flags);
+	list_add(&info->list, &mtk_subdev->channels);
+	spin_unlock_irqrestore(&mtk_subdev->channels_lock, flags);
+
+	schedule_work(&mtk_subdev->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 mtk_rpmsg_rproc_subdev *mtk_subdev = priv;
+	struct device *dev = &mtk_subdev->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(mtk_subdev, 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, mtk_subdev,
+				   SCP_IPI_NS_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_stop(struct rproc_subdev *subdev, bool crashed)
+{
+	struct mtk_rpmsg_channel_info *info, *next;
+	struct device *dev;
+	struct mtk_rpmsg_rproc_subdev *mtk_subdev = to_mtk_subdev(subdev);
+	unsigned long flags;
+
+	dev = &mtk_subdev->scp_pdev->dev;
+
+	mtk_rpmsg_destroy_ept(mtk_subdev->ns_ept);
+	cancel_work_sync(&mtk_subdev->register_work);
+
+	spin_lock_irqsave(&mtk_subdev->channels_lock, flags);
+	list_for_each_entry(info, &mtk_subdev->channels, list) {
+		if (!info->registered)
+			continue;
+		spin_unlock_irqrestore(&mtk_subdev->channels_lock, flags);
+		if (rpmsg_unregister_device(dev, &info->info)) {
+			dev_warn(
+				dev,
+				"rpmsg_unregister_device failed for %s.%d.%d\n",
+				info->info.name, info->info.src,
+				info->info.dst);
+		}
+		spin_lock_irqsave(&mtk_subdev->channels_lock, flags);
+	}
+
+	list_for_each_entry_safe(info, next,
+				 &mtk_subdev->channels, list) {
+		list_del(&info->list);
+		kfree(info);
+	}
+	spin_unlock_irqrestore(&mtk_subdev->channels_lock, flags);
+}
+
+struct rproc_subdev *
+mtk_rpmsg_create_rproc_subdev(struct platform_device *scp_pdev,
+			      struct rproc *scp_rproc)
+{
+	struct mtk_rpmsg_rproc_subdev *mtk_subdev;
+
+	mtk_subdev = kzalloc(sizeof(*mtk_subdev), GFP_KERNEL);
+	if (!mtk_subdev)
+		return NULL;
+
+	mtk_subdev->scp_pdev = scp_pdev;
+	mtk_subdev->scp_rproc = scp_rproc;
+	mtk_subdev->subdev.prepare = mtk_rpmsg_prepare;
+	mtk_subdev->subdev.stop = mtk_rpmsg_stop;
+	INIT_LIST_HEAD(&mtk_subdev->channels);
+	INIT_WORK(&mtk_subdev->register_work,
+		  mtk_register_device_work_function);
+	spin_lock_init(&mtk_subdev->channels_lock);
+
+	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 0e999ec319446b..643ef305a9e88d 100644
--- a/include/linux/platform_data/mtk_scp.h
+++ b/include/linux/platform_data/mtk_scp.h
@@ -22,7 +22,7 @@ typedef void (*scp_ipi_handler_t) (void *data,
  *			 command to SCP.
  *			 For other IPI below, AP should send the request
  *			 to SCP to trigger the interrupt.
- * @IPI_MAX:		 The maximum IPI number
+ * @SCP_IPI_MAX:	 The maximum IPI number
  */
 
 enum scp_ipi_id {
@@ -34,9 +34,11 @@ enum scp_ipi_id {
 	SCP_IPI_VENC_VP8,
 	SCP_IPI_MDP,
 	SCP_IPI_CROS_HOST_CMD,
-	SCP_IPI_MAX,
 };
 
+#define SCP_IPI_NS_SERVICE 0xFF
+#define SCP_IPI_MAX 0x100
+
 /**
  * scp_ipi_register - register an ipi function
  *
diff --git a/include/linux/rpmsg/mtk_rpmsg.h b/include/linux/rpmsg/mtk_rpmsg.h
new file mode 100644
index 00000000000000..8d21cd3d35b012
--- /dev/null
+++ b/include/linux/rpmsg/mtk_rpmsg.h
@@ -0,0 +1,35 @@
+/* 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.611.gfbb209baf1-goog

Re: [PATCH v4 3/6] rpmsg: add rpmsg support for mt8183 SCP.

[Matthias Kaehlcke]

Hi Pi-Hsun,

On Thu, Jan 31, 2019 at 05:31:28PM +0800, Pi-Hsun Shih wrote:
> Add a simple rpmsg support for mt8183 SCP, that use IPI / IPC directly.
> 
> Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
> ---
> Changes from v3:
>  - Change from unprepare to stop, to stop the rpmsg driver before the
>    rproc is stopped, avoiding problem that some rpmsg would fail after
>    rproc is stopped.
>  - Add missing spin_lock_init, and use destroy_ept instead of kref_put.
> 
> Changes from v2:
>  - Unregiser IPI handler on unprepare.
>  - Lock the channel list on operations.
>  - Move SCP_IPI_NS_SERVICE to 0xFF.
> 
> Changes from v1:
>  - Do cleanup properly in mtk_rpmsg.c, which also removes the problem of
>    short-lived work items.
>  - Fix several issues checkpatch found.
> ---
>  drivers/remoteproc/mtk_common.h       |   3 +
>  drivers/remoteproc/mtk_scp.c          |  29 +-
>  drivers/remoteproc/mtk_scp_ipi.c      |   5 +-
>  drivers/rpmsg/Kconfig                 |  10 +
>  drivers/rpmsg/Makefile                |   1 +
>  drivers/rpmsg/mtk_rpmsg.c             | 384 ++++++++++++++++++++++++++
>  include/linux/platform_data/mtk_scp.h |   6 +-
>  include/linux/rpmsg/mtk_rpmsg.h       |  35 +++
>  8 files changed, 466 insertions(+), 7 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 162798c44ad7f1..7efe2e7374e5ec 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 920c81c3525c2a..c0375119c53289 100644
> --- a/drivers/remoteproc/mtk_scp.c
> +++ b/drivers/remoteproc/mtk_scp.c
> @@ -13,6 +13,7 @@
>  #include <linux/platform_data/mtk_scp.h>
>  #include <linux/platform_device.h>
>  #include <linux/remoteproc.h>
> +#include <linux/rpmsg/mtk_rpmsg.h>
>  
>  #include "mtk_common.h"
>  #include "remoteproc_internal.h"
> @@ -303,6 +304,23 @@ 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;
> @@ -326,6 +344,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;

The field shouldn't be needed since you already have ->dev.
to_platform_device(scp->dev) could be used in scp_add_rpmsg_subdev().

>  	platform_set_drvdata(pdev, scp);
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram");
> @@ -396,13 +415,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);
> @@ -414,6 +436,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 51b8449a692970..1026089335a5c3 100644
> --- a/drivers/remoteproc/mtk_scp_ipi.c
> +++ b/drivers/remoteproc/mtk_scp_ipi.c
> @@ -93,8 +93,9 @@ int scp_ipi_send(struct platform_device *pdev,
>  	int ret;
>  
>  	if (WARN(id <= SCP_IPI_INIT || id >= SCP_IPI_MAX ||
> -	    len > sizeof(send_obj->share_buf) || !buf,
> -	    "failed to send ipi message\n"))
> +			 id == SCP_IPI_NS_SERVICE ||
> +			 len > sizeof(send_obj->share_buf) || !buf,
> +		 "failed to send ipi message\n"))
>  		return -EINVAL;
>  
>  	ret = clk_prepare_enable(scp->clk);
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index d0322b41eca54c..270eca663fb8c0 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -15,6 +15,16 @@ config RPMSG_CHAR
>  	  in /dev. They make it possible for user-space programs to send and
>  	  receive rpmsg packets.
>  
> +config RPMSG_MTK_SCP
> +	tristate "MediaTek SCP"
> +	depends on MTK_SCP
> +	select RPMSG
> +	help
> +	  Say y here to enable support providing communication channels to
> +	  remote processors in MediaTek platforms.
> +	  This use IPI and IPC to communicate with remote processors, with a
> +	  send and receive buffer of size 1.

is the buffer size really relevant in this context?

> +
>  config RPMSG_QCOM_GLINK_NATIVE
>  	tristate
>  	select RPMSG
> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> index 9aa859502d2752..ae92a7fb08f623 100644
> --- a/drivers/rpmsg/Makefile
> +++ b/drivers/rpmsg/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_RPMSG)		+= rpmsg_core.o
>  obj-$(CONFIG_RPMSG_CHAR)	+= rpmsg_char.o
> +obj-$(CONFIG_RPMSG_MTK_SCP)	+= mtk_rpmsg.o
>  obj-$(CONFIG_RPMSG_QCOM_GLINK_RPM) += qcom_glink_rpm.o
>  obj-$(CONFIG_RPMSG_QCOM_GLINK_NATIVE) += qcom_glink_native.o
>  obj-$(CONFIG_RPMSG_QCOM_GLINK_SMEM) += qcom_glink_smem.o
> diff --git a/drivers/rpmsg/mtk_rpmsg.c b/drivers/rpmsg/mtk_rpmsg.c
> new file mode 100644
> index 00000000000000..55498ff0a3497d
> --- /dev/null
> +++ b/drivers/rpmsg/mtk_rpmsg.c
> @@ -0,0 +1,384 @@
> +// 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"
> +
> +struct mtk_rpmsg_device {
> +	struct rpmsg_device rpdev;
> +	struct platform_device *scp_pdev;
> +};
> +
> +struct mtk_rpmsg_endpoint {
> +	struct rpmsg_endpoint ept;
> +	struct platform_device *scp_pdev;
> +};
> +
> +struct mtk_rpmsg_rproc_subdev {
> +	struct rproc *scp_rproc;
> +	struct platform_device *scp_pdev;
> +	struct rpmsg_endpoint *ns_ept;
> +	struct rproc_subdev subdev;
> +
> +	struct work_struct register_work;
> +	struct list_head channels;
> +	spinlock_t channels_lock;
> +};
> +
> +#define to_mtk_subdev(d) container_of(d, struct mtk_rpmsg_rproc_subdev, subdev)
> +
> +struct mtk_rpmsg_channel_info {
> +	struct rpmsg_channel_info info;
> +	bool registered;
> +	struct list_head list;
> +};
> +
> +/**
> + * 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. When we receive these
> + * messages, an appropriate rpmsg channel (i.e device) is created. 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;
> +	int ret;
> +
> +	ret = (*ept->cb)(ept->rpdev, data, len, ept->priv, ept->addr);
> +	if (ret)
> +		dev_warn(&ept->rpdev->dev, "rpmsg handler return error = %d",
> +			 ret);
> +}
> +
> +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)
> +{
> +	struct mtk_rpmsg_endpoint *mept;
> +	struct rpmsg_endpoint *ept;
> +

delete empty line

> +	int ret;
> +
> +	mept = kzalloc(sizeof(*mept), GFP_KERNEL);
> +	if (!mept)
> +		return NULL;
> +	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;
> +	ept->addr = id;
> +
> +	ret = scp_ipi_register(scp_pdev, id, mtk_rpmsg_ipi_handler, 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;
> +
> +	return __rpmsg_create_ept(scp_pdev, rpdev, cb, priv, chinfo.src);
> +}
> +
> +static const struct rpmsg_device_ops mtk_rpmsg_device_ops = {
> +	.create_ept = mtk_rpmsg_create_ept,
> +};

declare this closer to where it is used (i.e. before
mtk_rpmsg_register_device())?

> +
> +static void mtk_rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
> +{
> +	struct mtk_rpmsg_endpoint *mept = to_scp_endpoint(ept);
> +
> +	scp_ipi_unregister(mept->scp_pdev, ept->addr);
> +	kref_put(&ept->refcount, __ept_release);
> +}
> +
> +static int __mtk_rpmsg_send(struct mtk_rpmsg_endpoint *mept, void *data,
> +			    int len, bool wait)
> +{
> +	/*
> +	 * TODO: This currently ignore the "wait" argument, and always wait
> +	 * until SCP receive the last command.
> +	 */
> +	return scp_ipi_send(mept->scp_pdev, mept->ept.addr, data, len, 0);
> +}

I don't think this wrapper adds much value, there is little gain from
eliminating the access to the struct members in the call and it comes
at the expense of another level of function call nesting.

> +
> +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 int mtk_rpmsg_register_device(struct platform_device *scp_pdev,
> +				     struct rpmsg_channel_info *info)
> +{
> +	struct rpmsg_device *rpdev;
> +	struct mtk_rpmsg_device *mdev;
> +	int ret;
> +
> +	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> +	if (!mdev)
> +		return -ENOMEM;
> +
> +	mdev->scp_pdev = scp_pdev;
> +
> +	rpdev = &mdev->rpdev;
> +	rpdev->ops = &mtk_rpmsg_device_ops;
> +	rpdev->src = info->src;
> +	rpdev->dst = info->dst;
> +	strncpy(rpdev->id.name, info->name, RPMSG_NAME_SIZE);
> +
> +	rpdev->dev.parent = &scp_pdev->dev;
> +	rpdev->dev.release = mtk_rpmsg_release_device;
> +
> +	ret = rpmsg_register_device(rpdev);
> +	if (ret)

		kfree(mdev);

> +		return ret;
> +
> +	return 0;
> +}
> +
> +static void mtk_register_device_work_function(struct work_struct *register_work)
> +{
> +	struct mtk_rpmsg_rproc_subdev *mtk_subdev = container_of(
> +		register_work, struct mtk_rpmsg_rproc_subdev, register_work);
> +	struct platform_device *scp_pdev = mtk_subdev->scp_pdev;
> +	struct mtk_rpmsg_channel_info *info;
> +

delete empty line

> +	int ret;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&mtk_subdev->channels_lock, flags);
> +	list_for_each_entry(info, &mtk_subdev->channels, list) {
> +		if (info->registered)
> +			continue;
> +
> +		spin_unlock_irqrestore(&mtk_subdev->channels_lock, flags);

I wonder if it would make sense to use a threaded interrupt handler
for the IPI, which would allow to use a mutex and avoid the need to
unlock/relock when calling potentially blocking functions (same pattern
below in mtk_rpmsg_stop()).

> +		ret = mtk_rpmsg_register_device(scp_pdev, &info->info);
> +		spin_lock_irqsave(&mtk_subdev->channels_lock, flags);
> +
> +		if (ret) {
> +			dev_err(&scp_pdev->dev, "Can't create rpmsg_device\n");
> +			continue;
> +		}
> +
> +		info->registered = true;
> +	}
> +	spin_unlock_irqrestore(&mtk_subdev->channels_lock, flags);
> +}
> +
> +static int mtk_rpmsg_create_device(struct mtk_rpmsg_rproc_subdev *mtk_subdev,
> +				   char *name, u32 addr)
> +{
> +	struct mtk_rpmsg_channel_info *info;
> +	unsigned long flags;
> +
> +	/* This is called in interrupt context from name service callback. */
> +	info = kzalloc(sizeof(*info), GFP_ATOMIC);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	strncpy(info->info.name, name, RPMSG_NAME_SIZE);
> +	info->info.src = addr;
> +	info->info.dst = RPMSG_ADDR_ANY;
> +	spin_lock_irqsave(&mtk_subdev->channels_lock, flags);
> +	list_add(&info->list, &mtk_subdev->channels);
> +	spin_unlock_irqrestore(&mtk_subdev->channels_lock, flags);
> +
> +	schedule_work(&mtk_subdev->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 mtk_rpmsg_rproc_subdev *mtk_subdev = priv;
> +	struct device *dev = &mtk_subdev->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(mtk_subdev, 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;

initialization could be done together with declaration.

> +
> +	/* a dedicated endpoint handles the name service msgs */
> +	mtk_subdev->ns_ept =
> +		__rpmsg_create_ept(scp_pdev, NULL, mtk_rpmsg_ns_cb, mtk_subdev,
> +				   SCP_IPI_NS_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_stop(struct rproc_subdev *subdev, bool crashed)
> +{
> +	struct mtk_rpmsg_channel_info *info, *next;
> +	struct device *dev;
> +	struct mtk_rpmsg_rproc_subdev *mtk_subdev = to_mtk_subdev(subdev);
> +	unsigned long flags;
> +
> +	dev = &mtk_subdev->scp_pdev->dev;

this could be done above if the declaration order of dev and
mtk_subdev is swapped.

> +
> +	mtk_rpmsg_destroy_ept(mtk_subdev->ns_ept);
> +	cancel_work_sync(&mtk_subdev->register_work);
> +
> +	spin_lock_irqsave(&mtk_subdev->channels_lock, flags);
> +	list_for_each_entry(info, &mtk_subdev->channels, list) {
> +		if (!info->registered)
> +			continue;
> +		spin_unlock_irqrestore(&mtk_subdev->channels_lock, flags);
> +		if (rpmsg_unregister_device(dev, &info->info)) {
> +			dev_warn(
> +				dev,
> +				"rpmsg_unregister_device failed for %s.%d.%d\n",
> +				info->info.name, info->info.src,
> +				info->info.dst);
> +		}
> +		spin_lock_irqsave(&mtk_subdev->channels_lock, flags);
> +	}
> +
> +	list_for_each_entry_safe(info, next,
> +				 &mtk_subdev->channels, list) {
> +		list_del(&info->list);
> +		kfree(info);
> +	}
> +	spin_unlock_irqrestore(&mtk_subdev->channels_lock, flags);
> +}
> +
> +struct rproc_subdev *
> +mtk_rpmsg_create_rproc_subdev(struct platform_device *scp_pdev,
> +			      struct rproc *scp_rproc)
> +{
> +	struct mtk_rpmsg_rproc_subdev *mtk_subdev;
> +
> +	mtk_subdev = kzalloc(sizeof(*mtk_subdev), GFP_KERNEL);
> +	if (!mtk_subdev)
> +		return NULL;
> +
> +	mtk_subdev->scp_pdev = scp_pdev;
> +	mtk_subdev->scp_rproc = scp_rproc;
> +	mtk_subdev->subdev.prepare = mtk_rpmsg_prepare;
> +	mtk_subdev->subdev.stop = mtk_rpmsg_stop;
> +	INIT_LIST_HEAD(&mtk_subdev->channels);
> +	INIT_WORK(&mtk_subdev->register_work,
> +		  mtk_register_device_work_function);
> +	spin_lock_init(&mtk_subdev->channels_lock);
> +
> +	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 0e999ec319446b..643ef305a9e88d 100644
> --- a/include/linux/platform_data/mtk_scp.h
> +++ b/include/linux/platform_data/mtk_scp.h
> @@ -22,7 +22,7 @@ typedef void (*scp_ipi_handler_t) (void *data,
>   *			 command to SCP.
>   *			 For other IPI below, AP should send the request
>   *			 to SCP to trigger the interrupt.
> - * @IPI_MAX:		 The maximum IPI number
> + * @SCP_IPI_MAX:	 The maximum IPI number

fix in "[2/6] remoteproc/mediatek: add SCP support for mt8183"?

>   */
>  
>  enum scp_ipi_id {
> @@ -34,9 +34,11 @@ enum scp_ipi_id {
>  	SCP_IPI_VENC_VP8,
>  	SCP_IPI_MDP,
>  	SCP_IPI_CROS_HOST_CMD,
> -	SCP_IPI_MAX,
>  };
>  
> +#define SCP_IPI_NS_SERVICE 0xFF
> +#define SCP_IPI_MAX 0x100

shouldn't these (still) be part of enum scp_ipi_id?

> +
>  /**
>   * scp_ipi_register - register an ipi function
>   *
> diff --git a/include/linux/rpmsg/mtk_rpmsg.h b/include/linux/rpmsg/mtk_rpmsg.h
> new file mode 100644
> index 00000000000000..8d21cd3d35b012
> --- /dev/null
> +++ b/include/linux/rpmsg/mtk_rpmsg.h
> @@ -0,0 +1,35 @@
> +/* 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

Cheers

Matthias

[PATCH v4 4/6] mfd: add EC host command support using rpmsg.

[Pi-Hsun Shih]

Add EC host command / host event support through rpmsg.

Cc: Enric Balletbo Serra <eballetbo@gmail.com>
Cc: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
---
This patch depends on https://lore.kernel.org/patchwork/patch/1024376/

Changes from v3:
 - Add host event support by adding an extra bytes at the start of IPC
   message to indicate the type of the message (host event or host
   command), since there's no additional irq that can be used for host
   event.

Changes from v2:
 - Wait for ipi ack instead of depends on the behavior in mtk-rpmsg.

Changes from v1:
 - Code format fix based on feedback for cros_ec_rpmsg.c.
 - Extract feature detection for SCP into separate patch (Patch 6).
---
 drivers/platform/chrome/Kconfig         |   9 +
 drivers/platform/chrome/Makefile        |   1 +
 drivers/platform/chrome/cros_ec_rpmsg.c | 240 ++++++++++++++++++++++++
 3 files changed, 250 insertions(+)
 create mode 100644 drivers/platform/chrome/cros_ec_rpmsg.c

diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 16b1615958aa2d..f18b4586a7ad5d 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -72,6 +72,15 @@ 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 && OF
+	help
+	  If you say Y here, you get support for talking to the ChromeOS EC
+	  through rpmsg. This uses a simple byte-level protocol with a
+	  checksum. Also since there's no addition EC-to-host interrupt, this
+	  use a byte in message to distinguish host event from host command.
+
 config CROS_EC_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..51ba64305b487b
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_rpmsg.c
@@ -0,0 +1,240 @@
+// 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>
+#include <linux/workqueue.h>
+
+#define EC_MSG_TIMEOUT_MS 200
+#define HOST_COMMAND_MARK 1
+#define HOST_EVENT_MARK 2
+
+struct cros_ec_rpmsg_response {
+	uint8_t type;
+	uint8_t data[];
+};
+
+struct cros_ec_rpmsg {
+	struct rpmsg_device *rpdev;
+
+	wait_queue_head_t xfer_ack_wq;
+	bool xfer_acked;
+
+	struct work_struct host_event_work;
+};
+
+/**
+ * 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 cros_ec_rpmsg *ec_rpmsg = ec_dev->priv;
+	struct rpmsg_device *rpdev = ec_rpmsg->rpdev;
+	int len;
+	u8 sum;
+	int ret;
+	int i;
+	unsigned long timeout;
+
+	ec_msg->result = 0;
+	len = cros_ec_prepare_tx(ec_dev, ec_msg);
+	dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);
+
+	ec_rpmsg->xfer_acked = false;
+	ret = rpmsg_send(rpdev->ept, ec_dev->dout, len);
+	if (ret) {
+		dev_err(ec_dev->dev, "rpmsg send failed\n");
+		return ret;
+	}
+
+	timeout = msecs_to_jiffies(EC_MSG_TIMEOUT_MS);
+	ret = wait_event_timeout(ec_rpmsg->xfer_ack_wq, ec_rpmsg->xfer_acked,
+				 timeout);
+	if (!ret) {
+		dev_err(ec_dev->dev, "rpmsg send timeout\n");
+		return -EIO;
+	}
+
+	/* check response error code */
+	response = (struct ec_host_response *)ec_dev->din;
+	ec_msg->result = response->result;
+
+	ret = cros_ec_check_result(ec_dev, ec_msg);
+	if (ret)
+		goto exit;
+
+	if (response->data_len > ec_msg->insize) {
+		dev_err(ec_dev->dev, "packet too long (%d bytes, expected %d)",
+			response->data_len, ec_msg->insize);
+		ret = -EMSGSIZE;
+		goto exit;
+	}
+
+	/* copy response packet payload and compute checksum */
+	memcpy(ec_msg->data, ec_dev->din + sizeof(*response),
+	       response->data_len);
+
+	sum = 0;
+	for (i = 0; i < sizeof(*response) + response->data_len; i++)
+		sum += ec_dev->din[i];
+
+	if (sum) {
+		dev_err(ec_dev->dev, "bad packet checksum, calculated %x\n",
+			sum);
+		ret = -EBADMSG;
+		goto exit;
+	}
+
+	ret = response->data_len;
+exit:
+	if (ec_msg->command == EC_CMD_REBOOT_EC)
+		msleep(EC_REBOOT_DELAY_MS);
+
+	return ret;
+}
+
+static void
+cros_ec_rpmsg_host_event_function(struct work_struct *host_event_work)
+{
+	struct cros_ec_rpmsg *ec_rpmsg = container_of(
+		host_event_work, struct cros_ec_rpmsg, host_event_work);
+	struct cros_ec_device *ec_dev = dev_get_drvdata(&ec_rpmsg->rpdev->dev);
+	bool wake_event = true;
+	int ret;
+
+	ret = cros_ec_get_next_event(ec_dev, &wake_event);
+
+	/*
+	 * Signal only if wake host events or any interrupt if
+	 * cros_ec_get_next_event() returned an error (default value for
+	 * wake_event is true)
+	 */
+	if (wake_event && device_may_wakeup(ec_dev->dev))
+		pm_wakeup_event(ec_dev->dev, 0);
+
+	if (ret > 0)
+		blocking_notifier_call_chain(&ec_dev->event_notifier,
+					     0, ec_dev);
+}
+
+static int cros_ec_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
+				  int len, void *priv, u32 src)
+{
+	struct cros_ec_device *ec_dev = dev_get_drvdata(&rpdev->dev);
+	struct cros_ec_rpmsg *ec_rpmsg = ec_dev->priv;
+	struct cros_ec_rpmsg_response *resp;
+
+	if (!len) {
+		dev_warn(ec_dev->dev, "rpmsg received empty response");
+		return -EINVAL;
+	}
+
+	resp = data;
+	len--;
+	if (resp->type == HOST_COMMAND_MARK) {
+		if (len > ec_dev->din_size) {
+			dev_warn(
+				ec_dev->dev,
+				"received length %d > din_size %d, truncating",
+				len, ec_dev->din_size);
+			len = ec_dev->din_size;
+		}
+
+		memcpy(ec_dev->din, resp->data, len);
+		ec_rpmsg->xfer_acked = true;
+		wake_up(&ec_rpmsg->xfer_ack_wq);
+	} else if (resp->type == HOST_EVENT_MARK) {
+		schedule_work(&ec_rpmsg->host_event_work);
+	} else {
+		dev_warn(ec_dev->dev, "rpmsg received invalid type = %d",
+			 resp->type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int cros_ec_rpmsg_probe(struct rpmsg_device *rpdev)
+{
+	struct device *dev = &rpdev->dev;
+	struct cros_ec_device *ec_dev;
+	struct cros_ec_rpmsg *ec_rpmsg;
+	int ret;
+
+	ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
+	if (!ec_dev)
+		return -ENOMEM;
+
+	ec_rpmsg = devm_kzalloc(dev, sizeof(*ec_rpmsg), GFP_KERNEL);
+	if (!ec_rpmsg)
+		return -ENOMEM;
+
+	ec_dev->dev = dev;
+	ec_dev->priv = ec_rpmsg;
+	ec_dev->cmd_xfer = cros_ec_cmd_xfer_rpmsg;
+	ec_dev->pkt_xfer = cros_ec_pkt_xfer_rpmsg;
+	ec_dev->phys_name = dev_name(&rpdev->dev);
+	ec_dev->din_size = sizeof(struct ec_host_response) +
+			   sizeof(struct ec_response_get_protocol_info);
+	ec_dev->dout_size = sizeof(struct ec_host_request);
+	dev_set_drvdata(dev, ec_dev);
+
+	ec_rpmsg->rpdev = rpdev;
+	init_waitqueue_head(&ec_rpmsg->xfer_ack_wq);
+	INIT_WORK(&ec_rpmsg->host_event_work,
+		  cros_ec_rpmsg_host_event_function);
+
+	ret = cros_ec_register(ec_dev);
+	if (ret) {
+		dev_err(dev, "cannot register EC\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct rpmsg_device_id cros_ec_rpmsg_device_id[] = {
+	{ .name = "cros-ec-rpmsg", },
+	{ }
+};
+MODULE_DEVICE_TABLE(rpmsg, cros_ec_rpmsg_device_id);
+
+static struct rpmsg_driver cros_ec_driver_rpmsg = {
+	.drv.name       = KBUILD_MODNAME,
+	.id_table       = cros_ec_rpmsg_device_id,
+	.probe		= cros_ec_rpmsg_probe,
+	.callback	= cros_ec_rpmsg_callback,
+};
+
+module_rpmsg_driver(cros_ec_driver_rpmsg);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("ChromeOS EC multi function device (rpmsg)");
-- 
2.20.1.611.gfbb209baf1-goog

[PATCH v4 5/6] cros_ec: differentiate SCP from EC by feature bit.

[Pi-Hsun Shih]

Since a SCP and EC would both exist on a system, and use the cros_ec_dev
driver, we need to differentiate between them for the userspace, or they
would both be registered at /dev/cros_ec, causing a conflict.

Cc: Enric Balletbo Serra <eballetbo@gmail.com>
Cc: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
---
Changes from v3:
 - No change.

Changes from v2:
 - No change.

Changes from v1:
 - New patch extracted from Patch 5.
---
 drivers/mfd/cros_ec_dev.c            | 9 +++++++++
 include/linux/mfd/cros_ec.h          | 1 +
 include/linux/mfd/cros_ec_commands.h | 2 ++
 3 files changed, 12 insertions(+)

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/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.611.gfbb209baf1-goog

[PATCH v4 6/6] remoteproc/mediatek: Load ELF instead of bin file for mtk_scp.

[Pi-Hsun Shih]

For better flexibility on loading firmware image, change firmware load
for mtk_scp from a plain binary file to ELF format instead.

Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
---
Changes from v3:
  - New patch to load ELF instead of plain bin file.
---
 drivers/remoteproc/mtk_common.h  |  2 ++
 drivers/remoteproc/mtk_scp.c     | 59 ++++++++++++++++++++++++++++++--
 drivers/remoteproc/mtk_scp_ipi.c | 11 ++++--
 3 files changed, 68 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h
index 7efe2e7374e5ec..dffca9fbea9f1e 100644
--- a/drivers/remoteproc/mtk_common.h
+++ b/drivers/remoteproc/mtk_common.h
@@ -73,4 +73,6 @@ struct share_obj {
 	u8 share_buf[288];
 };
 
+void scp_memcpy_aligned(void *dst, const void *src, unsigned int len);
+
 #endif
diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
index c0375119c53289..314ae3f8a82cf7 100644
--- a/drivers/remoteproc/mtk_scp.c
+++ b/drivers/remoteproc/mtk_scp.c
@@ -140,6 +140,62 @@ static irqreturn_t scp_irq_handler(int irq, void *priv)
 	return IRQ_HANDLED;
 }
 
+int scp_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
+{
+	struct device *dev = &rproc->dev;
+	struct elf32_hdr *ehdr;
+	struct elf32_phdr *phdr;
+	int i, ret = 0;
+	const u8 *elf_data = fw->data;
+
+	ehdr = (struct elf32_hdr *)elf_data;
+	phdr = (struct elf32_phdr *)(elf_data + ehdr->e_phoff);
+
+	/* go through the available ELF segments */
+	for (i = 0; i < ehdr->e_phnum; i++, phdr++) {
+		u32 da = phdr->p_paddr;
+		u32 memsz = phdr->p_memsz;
+		u32 filesz = phdr->p_filesz;
+		u32 offset = phdr->p_offset;
+		void __iomem *ptr;
+
+		if (phdr->p_type != PT_LOAD)
+			continue;
+
+		dev_dbg(dev, "phdr: type %d da 0x%x memsz 0x%x filesz 0x%x\n",
+			phdr->p_type, da, memsz, filesz);
+
+		if (filesz > memsz) {
+			dev_err(dev, "bad phdr filesz 0x%x memsz 0x%x\n",
+				filesz, memsz);
+			ret = -EINVAL;
+			break;
+		}
+
+		if (offset + filesz > fw->size) {
+			dev_err(dev, "truncated fw: need 0x%x avail 0x%zx\n",
+				offset + filesz, fw->size);
+			ret = -EINVAL;
+			break;
+		}
+
+		/* grab the kernel address for this device address */
+		ptr = rproc_da_to_va(rproc, da, memsz);
+		if (!ptr) {
+			dev_err(dev, "bad phdr da 0x%x mem 0x%x\n", da, memsz);
+			ret = -EINVAL;
+			break;
+		}
+
+		/* put the segment where the remote processor expects it */
+		if (phdr->p_filesz)
+			scp_memcpy_aligned(ptr, elf_data + phdr->p_offset,
+					   filesz);
+	}
+
+	return ret;
+}
+
 static int mtk_scp_load(struct rproc *rproc, const struct firmware *fw)
 {
 	const struct mtk_scp *scp = rproc->priv;
@@ -157,8 +213,7 @@ static int mtk_scp_load(struct rproc *rproc, const struct firmware *fw)
 
 	writel(0x0, scp->reg_base + MT8183_SCP_SRAM_PDN);
 
-	memcpy(scp->sram_base, fw->data, fw->size);
-	return ret;
+	return scp_elf_load_segments(rproc, fw);
 }
 
 static int mtk_scp_start(struct rproc *rproc)
diff --git a/drivers/remoteproc/mtk_scp_ipi.c b/drivers/remoteproc/mtk_scp_ipi.c
index 1026089335a5c3..b6b248c9da50ac 100644
--- a/drivers/remoteproc/mtk_scp_ipi.c
+++ b/drivers/remoteproc/mtk_scp_ipi.c
@@ -55,7 +55,13 @@ void scp_ipi_unregister(struct platform_device *pdev, enum scp_ipi_id id)
 }
 EXPORT_SYMBOL_GPL(scp_ipi_unregister);
 
-static void memcpy_aligned(void *dst, const void *src, unsigned int len)
+/*
+ * Copy src to dst, where dst is in SCP SRAM region.
+ * Since AP access of SCP SRAM don't support byte write, this always write a
+ * full word at a time, and may cause some extra bytes to be written at the
+ * beginning & ending of dst.
+ */
+void scp_memcpy_aligned(void *dst, const void *src, unsigned int len)
 {
 	void *ptr;
 	u32 val;
@@ -80,6 +86,7 @@ static void memcpy_aligned(void *dst, const void *src, unsigned int len)
 		writel_relaxed(val, dst + i);
 	}
 }
+EXPORT_SYMBOL_GPL(scp_memcpy_aligned);
 
 int scp_ipi_send(struct platform_device *pdev,
 		 enum scp_ipi_id id,
@@ -117,7 +124,7 @@ int scp_ipi_send(struct platform_device *pdev,
 		}
 	} while (readl(scp->reg_base + MT8183_HOST_TO_SCP));
 
-	memcpy_aligned(send_obj->share_buf, buf, len);
+	scp_memcpy_aligned(send_obj->share_buf, buf, len);
 
 	send_obj->len = len;
 	send_obj->id = id;
-- 
2.20.1.611.gfbb209baf1-goog