[PATCH RFC v3 0/3] add VCP mailbox and IPC driver

[PATCH RFC v3 0/3] add VCP mailbox and IPC driver

[Jjian Zhou]

The VCP mailbox has 5 groups. Each group has corresponding interrupts,
registers, and 64 slots (each slot is 4 bytes). Since different features
share one of the mailbox groups, the VCP mailbox needs to establish a
send table and a receive table. The send table is used to record the
feature ID, mailbox ID, and the number of slots occupied. The receive table
is used to record the feature ID, mailbox ID, the number of slots occupied,
and the receive options. The API setup_mbox_table in mtk-vcp-ipc.c calculates
the slot offset and pin index for each feature ID based on the mailbox ID and
slot number in the send and receive tables (several slots form a pin, and
each pin can trigger an interrupt). These descriptions are written in the
mtk-vcp-ipc.c file -- we call it the IPC layer.

We have two questions:
How should we describe the mailbox and IPI?
Can the intermediate IPC layer be rewritten as a virtual mailbox layer?

Example of send and recve table:
Operation | mbox_id | ipi_id | msg_size | align_size | slot_ofs | pin_index |  notes
send          0          0       18          18           0          0
recv          0          1       18          18          18          9
send          1         15        8           8           0          0
send          1         16       18          18           8          4
send          1          9        2           2          26         13
recv          1         15        8           8          28         14       ack of send ipi_id=15
recv          1         17       18          18          36         18
recv          1         10        2           2          54         27       ack of send ipi_id=2
send          2         11       18          18           0          0
send          2          2        2           2          18          9
send          2          3        3           4          20         10
send          2         32        2           2          24         12
recv          2         12       18          18          26         13
recv          2          5        1           2          44         22
recv          2          2        1           2          46         23

Recv ipi_id=2 is the ack of send ipi_id=2(The ipi_id=15 is the same.)

Jjian Zhou (3):
  mailbox: mediatek: Add mtk-vcp-mailbox driver
  firmware: mediatek: Add vcp ipc protocol interface
  dt-bindings: mailbox: mtk,mt8196-vcp-mbox: add mtk vcp-mbox document

 .../bindings/mailbox/mtk,mt8196-vcp-mbox.yaml |  49 ++
 drivers/firmware/Kconfig                      |   9 +
 drivers/firmware/Makefile                     |   1 +
 drivers/firmware/mtk-vcp-ipc.c                | 481 ++++++++++++++++++
 drivers/mailbox/Kconfig                       |   9 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/mtk-vcp-mailbox.c             | 179 +++++++
 include/linux/firmware/mediatek/mtk-vcp-ipc.h | 151 ++++++
 include/linux/mailbox/mtk-vcp-mailbox.h       |  34 ++
 9 files changed, 915 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/mtk,mt8196-vcp-mbox.yaml
 create mode 100644 drivers/firmware/mtk-vcp-ipc.c
 create mode 100644 drivers/mailbox/mtk-vcp-mailbox.c
 create mode 100644 include/linux/firmware/mediatek/mtk-vcp-ipc.h
 create mode 100644 include/linux/mailbox/mtk-vcp-mailbox.h

-- 
2.45.2

[PATCH RFC v3 1/3] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Jjian Zhou]

Add mtk-vcp-mailbox driver to support the communication with
VCP remote microprocessor.

Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>
---
 drivers/mailbox/Kconfig                 |   9 ++
 drivers/mailbox/Makefile                |   2 +
 drivers/mailbox/mtk-vcp-mailbox.c       | 179 ++++++++++++++++++++++++
 include/linux/mailbox/mtk-vcp-mailbox.h |  34 +++++
 4 files changed, 224 insertions(+)
 create mode 100644 drivers/mailbox/mtk-vcp-mailbox.c
 create mode 100644 include/linux/mailbox/mtk-vcp-mailbox.h

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index ed52db272f4d..ffc4a5491462 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -275,6 +275,15 @@ config MTK_CMDQ_MBOX
 	  critical time limitation, such as updating display configuration
 	  during the vblank.
 
+config MTK_VCP_MBOX
+	tristate "MediaTek VCP Mailbox Support"
+	depends on ARCH_MEDIATEK || COMPILE_TEST
+	help
+	  Say yes here to add support for the MediaTek VCP mailbox driver.
+	  The mailbox implementation provides access from the application
+	  processor to the MediaTek Video Processing Unit.
+	  If unsure say N.
+
 config ZYNQMP_IPI_MBOX
 	tristate "Xilinx ZynqMP IPI Mailbox"
 	depends on ARCH_ZYNQMP && OF
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 9a1542b55539..75a200a9d2d2 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -59,6 +59,8 @@ obj-$(CONFIG_MTK_ADSP_MBOX)	+= mtk-adsp-mailbox.o
 
 obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o
 
+obj-$(CONFIG_MTK_VCP_MBOX)	+= mtk-vcp-mailbox.o
+
 obj-$(CONFIG_ZYNQMP_IPI_MBOX)	+= zynqmp-ipi-mailbox.o
 
 obj-$(CONFIG_SUN6I_MSGBOX)	+= sun6i-msgbox.o
diff --git a/drivers/mailbox/mtk-vcp-mailbox.c b/drivers/mailbox/mtk-vcp-mailbox.c
new file mode 100644
index 000000000000..fd7a123c71c8
--- /dev/null
+++ b/drivers/mailbox/mtk-vcp-mailbox.c
@@ -0,0 +1,179 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2025 MediaTek Corporation. All rights reserved.
+ * Author: Jjian Zhou <jjian.zhou.@mediatek.com>
+ */
+
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mailbox/mtk-vcp-mailbox.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct mtk_vcp_mbox_priv {
+	void __iomem *base;
+	struct device *dev;
+	struct mbox_controller mbox;
+	const struct mtk_vcp_mbox_cfg *cfg;
+	struct mtk_ipi_info ipi_recv;
+};
+
+struct mtk_vcp_mbox_cfg {
+	u32 set_in;
+	u32 clr_out;
+};
+
+static inline struct mtk_vcp_mbox_priv *get_mtk_vcp_mbox_priv(struct mbox_controller *mbox)
+{
+	return container_of(mbox, struct mtk_vcp_mbox_priv, mbox);
+}
+
+static irqreturn_t mtk_vcp_mbox_irq_thread(int irq, void *data)
+{
+	struct mbox_chan *chan = data;
+	struct mtk_vcp_mbox_priv *priv = get_mtk_vcp_mbox_priv(chan->mbox);
+
+	/* get irq status */
+	priv->ipi_recv.irq_status = readl(priv->base + priv->cfg->clr_out);
+
+	__ioread32_copy(priv->ipi_recv.msg, priv->base, MAX_SLOT_NUM);
+
+	mbox_chan_received_data(chan, &priv->ipi_recv);
+
+	/* clear irq status */
+	writel(priv->ipi_recv.irq_status, priv->base + priv->cfg->clr_out);
+
+	return IRQ_HANDLED;
+}
+
+static struct mbox_chan *mtk_vcp_mbox_xlate(struct mbox_controller *mbox,
+					    const struct of_phandle_args *sp)
+{
+	if (sp->args_count)
+		return NULL;
+
+	return mbox->chans;
+}
+
+static int mtk_vcp_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+	struct mtk_vcp_mbox_priv *priv = get_mtk_vcp_mbox_priv(chan->mbox);
+	struct mtk_ipi_info *ipi_info = data;
+	u32 status;
+
+	if (!ipi_info->msg) {
+		dev_err(priv->dev, "msg buffer is NULL.\n");
+		return -ENOMEM;
+	}
+
+	status = readl(priv->base + priv->cfg->set_in) & BIT(ipi_info->index);
+	if (status) {
+		dev_err(priv->dev, "mailbox IPI %d is busy.\n", ipi_info->id);
+		return -EBUSY;
+	}
+
+	if (ipi_info->slot_ofs + ipi_info->len > MBOX_SLOT_MAX_SIZE)
+		return -EINVAL;
+	__iowrite32_copy(priv->base + ipi_info->slot_ofs, ipi_info->msg,
+			 ipi_info->len);
+
+	writel(BIT(ipi_info->index), priv->base + priv->cfg->set_in);
+
+	return 0;
+}
+
+static bool mtk_vcp_mbox_last_tx_done(struct mbox_chan *chan)
+{
+	struct mtk_ipi_info *ipi_info = chan->active_req;
+	struct mtk_vcp_mbox_priv *priv = get_mtk_vcp_mbox_priv(chan->mbox);
+	u32 op;
+
+	op = readl(priv->base + priv->cfg->set_in) & BIT(ipi_info->index);
+	return !op;
+}
+
+static const struct mbox_chan_ops mtk_vcp_mbox_chan_ops = {
+	.send_data	= mtk_vcp_mbox_send_data,
+	.last_tx_done	= mtk_vcp_mbox_last_tx_done,
+};
+
+static int mtk_vcp_mbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mtk_vcp_mbox_priv *priv;
+	struct mbox_controller *mbox;
+	int ret, irq;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	mbox = &priv->mbox;
+	mbox->dev = dev;
+	mbox->ops = &mtk_vcp_mbox_chan_ops;
+	mbox->txdone_irq = false;
+	mbox->txdone_poll = true;
+	mbox->of_xlate = mtk_vcp_mbox_xlate;
+	mbox->num_chans = 1;
+	mbox->chans = devm_kzalloc(dev, sizeof(*mbox->chans), GFP_KERNEL);
+	if (!mbox->chans)
+		return -ENOMEM;
+
+	priv->ipi_recv.msg = devm_kzalloc(dev, sizeof(u8) * MBOX_SLOT_MAX_SIZE,
+					  GFP_KERNEL);
+	if (!priv->ipi_recv.msg)
+		return -ENOMEM;
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->cfg = of_device_get_match_data(dev);
+	if (!priv->cfg)
+		return -EINVAL;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_threaded_irq(dev, irq, NULL,
+					mtk_vcp_mbox_irq_thread, IRQF_ONESHOT,
+					dev_name(dev), mbox->chans);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, priv);
+
+	dev_dbg(dev, "MTK VCP mailbox initialized\n");
+
+	return devm_mbox_controller_register(dev, &priv->mbox);
+}
+
+static const struct mtk_vcp_mbox_cfg mt8196_cfg = {
+	.set_in		= 0x100,
+	.clr_out	= 0x10C,
+};
+
+static const struct of_device_id mtk_vcp_mbox_of_match[] = {
+	{ .compatible = "mediatek,mt8196-vcp-mbox", .data = &mt8196_cfg },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mtk_vcp_mbox_of_match);
+
+static struct platform_driver mtk_vcp_mbox_driver = {
+	.probe		= mtk_vcp_mbox_probe,
+	.driver = {
+		.name	= "mtk_vcp_mbox",
+		.of_match_table = mtk_vcp_mbox_of_match,
+	},
+};
+module_platform_driver(mtk_vcp_mbox_driver);
+
+MODULE_AUTHOR("Jjian Zhou <jjian.zhou@mediatek.com>");
+MODULE_DESCRIPTION("MTK VCP Mailbox Controller");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mailbox/mtk-vcp-mailbox.h b/include/linux/mailbox/mtk-vcp-mailbox.h
new file mode 100644
index 000000000000..953499b7cfeb
--- /dev/null
+++ b/include/linux/mailbox/mtk-vcp-mailbox.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+/*
+ * Copyright (c) 2024 MediaTek Inc.
+ */
+
+#ifndef __MTK_VCP_MAILBOX_H__
+#define __MTK_VCP_MAILBOX_H__
+
+#define MBOX_SLOT_MAX_SIZE	0x100 /* mbox max slot size */
+#define MAX_SLOT_NUM	64
+
+/**
+ * struct mtk_ipi_info - channel table that belong to mtk_ipi_device
+ * @msg: The share buffer between IPC and mailbox driver
+ * @len: Message length
+ * @id: IPI number
+ * @recv_opt: Recv option,  0:receive ,1: response
+ * @index: The pin groups number of the mailbox channel
+ * @slot_ofs: Slot offset of the mailbox channel
+ * @irq_status: Indicate which pin groups triggered the interrupt
+ *
+ * It is used between IPC with mailbox driver.
+ */
+struct mtk_ipi_info {
+	void *msg;
+	u32 len;
+	u32 id;
+	u32 recv_opt;
+	u32 index;
+	u32 slot_ofs;
+	u32 irq_status;
+};
+
+#endif
-- 
2.45.2

Re: [PATCH RFC v3 1/3] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Krzysztof Kozlowski]

On 17/03/2025 12:03, Jjian Zhou wrote:
> +
> +	ret = devm_request_threaded_irq(dev, irq, NULL,
> +					mtk_vcp_mbox_irq_thread, IRQF_ONESHOT,
> +					dev_name(dev), mbox->chans);
> +	if (ret < 0)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	dev_dbg(dev, "MTK VCP mailbox initialized\n");

Drop



Best regards,
Krzysztof

[PATCH RFC v3 2/3] firmware: mediatek: Add vcp ipc protocol interface

[Jjian Zhou]

Some of mediatek processors contain the Risc-V coprocessor.

The communication between Host CPU and vcp firmware is
taking place using a shared memory area for message passing.

VCP IPC protocol offers (send/recv) interfaces using
mediatek-mailbox APIs.

Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>
---
 drivers/firmware/Kconfig                      |   9 +
 drivers/firmware/Makefile                     |   1 +
 drivers/firmware/mtk-vcp-ipc.c                | 481 ++++++++++++++++++
 include/linux/firmware/mediatek/mtk-vcp-ipc.h | 151 ++++++
 4 files changed, 642 insertions(+)
 create mode 100644 drivers/firmware/mtk-vcp-ipc.c
 create mode 100644 include/linux/firmware/mediatek/mtk-vcp-ipc.h

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 37e43f287e78..98c4ff667836 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -179,6 +179,15 @@ config MTK_ADSP_IPC
 	  ADSP exists on some mtk processors.
 	  Client might use shared memory to exchange information with ADSP.
 
+config MTK_VCP_IPC
+	tristate "MTK VCP IPC Protocol driver"
+	depends on MTK_VCP_MBOX
+	help
+	  Say yes here to add support for the MediaTek VCP IPC
+	  between host AP (Linux) and the firmware running on VCP.
+	  VCP exists on some mtk processors.
+	  Client might use shared memory to exchange information with VCP.
+
 config SYSFB
 	bool
 	select BOOT_VESA_SUPPORT
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 91efcc868a05..2b9894e5169a 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_ISCSI_IBFT_FIND)	+= iscsi_ibft_find.o
 obj-$(CONFIG_ISCSI_IBFT)	+= iscsi_ibft.o
 obj-$(CONFIG_FIRMWARE_MEMMAP)	+= memmap.o
 obj-$(CONFIG_MTK_ADSP_IPC)	+= mtk-adsp-ipc.o
+obj-$(CONFIG_MTK_VCP_IPC)	+= mtk-vcp-ipc.o
 obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
 obj-$(CONFIG_FW_CFG_SYSFS)	+= qemu_fw_cfg.o
 obj-$(CONFIG_SYSFB)		+= sysfb.o
diff --git a/drivers/firmware/mtk-vcp-ipc.c b/drivers/firmware/mtk-vcp-ipc.c
new file mode 100644
index 000000000000..744937c56b67
--- /dev/null
+++ b/drivers/firmware/mtk-vcp-ipc.c
@@ -0,0 +1,481 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * Copyright (c) 2024 MediaTek Inc.
+ */
+
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/firmware/mediatek/mtk-vcp-ipc.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/sched/clock.h>
+#include <linux/time64.h>
+#include <linux/vmalloc.h>
+
+/**
+ * struct mtk_ipi_chan_table - channel table that belong to mtk_ipi_device
+ * @mbox: the mbox channel number
+ * @mbox_pin_cb: callback function
+ * @holder: keep 1 if there are ipi waiters (to wait the reply)
+ * @ipi_record: timestamp of each ipi transmission stage
+ * @pin_buf: buffer point
+ * @prdata: private data
+ * @recv_opt: recv option,  0:receive ,1: response
+ * @notify: completion notify process
+ * @send_ofs: message offset in the slots of a mbox
+ * @send_index: bit offset in the mbox
+ * @msg_zie: slot size of the ipi message
+ *
+ * All of these data should be initialized by mtk_ipi_device_register()
+ */
+struct mtk_ipi_chan_table {
+	u32 mbox;
+	mbox_pin_cb_t mbox_pin_cb;
+	atomic_t holder;
+	void *pin_buf;
+	void *prdata;
+	u32 recv_opt;
+	struct completion notify;
+	/* define a mutex for remote response */
+	struct mutex mutex_send;
+	u32 send_ofs;
+	u32 send_index;
+	u32 msg_size;
+};
+
+/**
+ * mbox information
+ *
+ * @mbdev: mbox device
+ * @mbox_id: mbox id
+ * @slot: how many slots that mbox used
+ * @opt: option for tx mode, 0:mbox, 1:share memory 2:queue
+ * @base: mbox base address
+ * @mbox_client: mbox client
+ * @mbox_chan: mbox channel
+ */
+struct mtk_mbox_info {
+	struct mtk_vcp_ipc *vcp_ipc;
+	u32 mbox_id;
+	u32 slot;
+	u32 opt;
+	/* lock of mbox */
+	spinlock_t mbox_lock;
+	struct mbox_client cl;
+	struct mbox_chan *ch;
+	struct mtk_ipi_info ipi_info;
+};
+
+static const char * const mbox_names[VCP_MBOX_NUM] = {
+	"mbox0", "mbox1", "mbox2", "mbox3", "mbox4"
+};
+
+/**
+ * mtk_vcp_ipc_recv - recv callback used by MTK VCP mailbox
+ *
+ * @c: mbox client
+ * @msg: message received
+ *
+ * Users of VCP IPC will need to provide handle_reply and handle_request
+ * callbacks.
+ */
+static void mtk_vcp_ipc_recv(struct mbox_client *c, void *msg)
+{
+	struct mtk_mbox_info *minfo = container_of(c, struct mtk_mbox_info, cl);
+	struct mtk_vcp_ipc *vcp_ipc = minfo->vcp_ipc;
+	struct mtk_ipi_info *ipi_info = msg;
+	struct mtk_ipi_device *ipidev = vcp_ipc->ipi_priv;
+	struct mtk_ipi_chan_table *table;
+	struct mtk_mbox_recv_table *mbox_recv;
+	u32 id;
+
+	/* execute all receive pin handler */
+	for (id = 0; id < vcp_ipc->mbdev->recv_count; id++) {
+		mbox_recv = &vcp_ipc->mbdev->recv_table[id];
+		if (mbox_recv->mbox_id != minfo->mbox_id)
+			continue;
+
+		if (!(BIT(mbox_recv->pin_index) & ipi_info->irq_status))
+			continue;
+
+		table = &ipidev->table[mbox_recv->ipi_id];
+		if (!table->pin_buf) {
+			dev_err(vcp_ipc->dev, "IPI%d buf is null.\n",
+				mbox_recv->ipi_id);
+			continue;
+		}
+
+		memcpy(table->pin_buf,
+		       ipi_info->msg + mbox_recv->offset * MBOX_SLOT_SIZE,
+		       mbox_recv->msg_size * MBOX_SLOT_SIZE);
+
+		if (!mbox_recv->recv_opt && table->mbox_pin_cb)
+			table->mbox_pin_cb(mbox_recv->ipi_id,
+					   table->prdata,
+					   table->pin_buf,
+					   mbox_recv->msg_size * MBOX_SLOT_SIZE);
+
+		/* notify task */
+		if (table->recv_opt == MBOX_RECV_MESSAGE ||
+		    atomic_read(&table->holder))
+			complete(&table->notify);
+	}
+}
+
+/*
+ * mtk_vcp_ipc_send - send ipc command to MTK VCP
+ *
+ * @ipidev: VCP struct mtk_ipi_device handle
+ * @id: id of the feature IPI
+ * @data: message address
+ * @len: message length
+ *
+ * Return: Zero for success from mbox_send_message
+ *         negative value for error
+ */
+int mtk_vcp_ipc_send(struct mtk_ipi_device *ipidev, u32 id, void *data, u32 len)
+{
+	struct device *dev;
+	struct mtk_mbox_info *minfo;
+	struct mtk_ipi_chan_table *table;
+	struct mtk_vcp_ipc *vcp_ipc;
+	int ret;
+
+	if (!ipidev || !ipidev->ipi_inited || !data)
+		return IPI_UNAVAILABLE;
+	vcp_ipc = ipidev->vcp_ipc;
+	if (!vcp_ipc)
+		return IPI_UNAVAILABLE;
+
+	table = ipidev->table;
+	dev = ipidev->vcp_ipc->dev;
+	minfo = &ipidev->vcp_ipc->info_table[table[id].mbox];
+	if (!minfo) {
+		dev_err(dev, "%s IPI%d minfo is invalid.\n", ipidev->name, id);
+		return IPI_UNAVAILABLE;
+	}
+
+	if (len > table[id].msg_size)
+		return IPI_MSG_TOO_BIG;
+	else if (!len)
+		len = table[id].msg_size;
+
+	mutex_lock(&table[id].mutex_send);
+
+	minfo->ipi_info.msg = data;
+	minfo->ipi_info.len = len;
+	minfo->ipi_info.id = id;
+	minfo->ipi_info.index = table[id].send_index;
+	minfo->ipi_info.slot_ofs = table[id].send_ofs * MBOX_SLOT_SIZE;
+
+	ret = mbox_send_message(minfo->ch, &minfo->ipi_info);
+	mutex_unlock(&table[id].mutex_send);
+	if (ret < 0) {
+		dev_err(dev, "%s IPI%d send failed.\n", ipidev->name, id);
+		return IPI_MBOX_ERR;
+	}
+
+	return IPI_ACTION_DONE;
+}
+EXPORT_SYMBOL(mtk_vcp_ipc_send);
+
+/*
+ * mtk_vcp_ipc_send_compl - send ipc command to MTK VCP
+ *
+ * @ipidev: VCP struct mtk_ipi_device handle
+ * @id: id of the feature IPI
+ * @data: message address
+ * @len: message length
+ * @timeout_ms:
+ *
+ * Return: Zero for success from mbox_send_message
+ *         negative value for error
+ */
+int mtk_vcp_ipc_send_compl(struct mtk_ipi_device *ipidev, u32 id,
+			   void *data, u32 len, u32 timeout_ms)
+{
+	struct device *dev;
+	struct mtk_mbox_info *minfo;
+	struct mtk_ipi_chan_table *table;
+	struct mtk_vcp_ipc *vcp_ipc;
+	int ret;
+
+	if (!ipidev || !ipidev->ipi_inited || !data)
+		return IPI_UNAVAILABLE;
+	vcp_ipc = ipidev->vcp_ipc;
+	if (!vcp_ipc)
+		return IPI_UNAVAILABLE;
+
+	table = ipidev->table;
+	dev = ipidev->vcp_ipc->dev;
+	minfo = &ipidev->vcp_ipc->info_table[table[id].mbox];
+	if (!minfo) {
+		dev_err(dev, "%s IPI%d minfo is invalid.\n", ipidev->name, id);
+		return IPI_UNAVAILABLE;
+	}
+
+	if (len > table[id].msg_size)
+		return IPI_MSG_TOO_BIG;
+	else if (!len)
+		len = table[id].msg_size;
+
+	mutex_lock(&table[id].mutex_send);
+
+	minfo->ipi_info.msg = data;
+	minfo->ipi_info.len = len;
+	minfo->ipi_info.id = id;
+	minfo->ipi_info.index = table[id].send_index;
+	minfo->ipi_info.slot_ofs = table[id].send_ofs * MBOX_SLOT_SIZE;
+
+	atomic_inc(&table[id].holder);
+
+	ret = mbox_send_message(minfo->ch, &minfo->ipi_info);
+	if (ret < 0) {
+		atomic_set(&table[id].holder, 0);
+		mutex_unlock(&table[id].mutex_send);
+		dev_err(dev, "%s IPI%d send failed.\n", ipidev->name, id);
+		return IPI_MBOX_ERR;
+	}
+
+	/* wait for completion */
+	ret = wait_for_completion_timeout(&table[id].notify,
+					  msecs_to_jiffies(timeout_ms));
+	atomic_set(&table[id].holder, 0);
+	if (ret > 0)
+		ret = IPI_ACTION_DONE;
+
+	mutex_unlock(&table[id].mutex_send);
+
+	return ret;
+}
+EXPORT_SYMBOL(mtk_vcp_ipc_send_compl);
+
+int mtk_vcp_mbox_ipc_register(struct mtk_ipi_device *ipidev, int id,
+			      mbox_pin_cb_t cb, void *prdata, void *msg)
+{
+	if (!ipidev || !ipidev->ipi_inited)
+		return IPI_DEV_ILLEGAL;
+	if (!msg)
+		return IPI_NO_MSGBUF;
+
+	if (ipidev->table[id].pin_buf)
+		return IPI_ALREADY_USED;
+	ipidev->table[id].mbox_pin_cb = cb;
+	ipidev->table[id].pin_buf = msg;
+	ipidev->table[id].prdata = prdata;
+
+	return IPI_ACTION_DONE;
+}
+EXPORT_SYMBOL(mtk_vcp_mbox_ipc_register);
+
+int mtk_vcp_mbox_ipc_unregister(struct mtk_ipi_device *ipidev, int id)
+{
+	if (!ipidev || !ipidev->ipi_inited)
+		return IPI_DEV_ILLEGAL;
+
+	/* Drop the ipi and reset the record */
+	complete(&ipidev->table[id].notify);
+
+	ipidev->table[id].mbox_pin_cb = NULL;
+	ipidev->table[id].pin_buf = NULL;
+	ipidev->table[id].prdata = NULL;
+
+	return IPI_ACTION_DONE;
+}
+EXPORT_SYMBOL(mtk_vcp_mbox_ipc_unregister);
+
+static void mtk_fill_in_entry(struct mtk_ipi_chan_table *entry, const u32 ipi_id,
+			      const struct mtk_mbox_table *mbdev)
+{
+	const struct mtk_mbox_send_table *mbox_send = mbdev->send_table;
+	u32 index;
+
+	for (index = 0; index < mbdev->send_count; index++) {
+		if (ipi_id != mbox_send[index].ipi_id)
+			continue;
+
+		entry->send_ofs = mbox_send[index].offset;
+		entry->send_index = mbox_send[index].pin_index;
+		entry->msg_size = mbox_send[index].msg_size;
+		entry->mbox = mbox_send[index].mbox_id;
+		return;
+	}
+
+	entry->mbox = -ENOENT;
+}
+
+int mtk_vcp_ipc_device_register(struct mtk_ipi_device *ipidev,
+				u32 ipi_chan_count, struct mtk_vcp_ipc *vcp_ipc)
+{
+	struct mtk_ipi_chan_table *ipi_chan_table;
+	struct mtk_mbox_table *mbdev;
+	u32 index;
+
+	if (!vcp_ipc || !ipidev)
+		return -EINVAL;
+
+	ipi_chan_table = kcalloc(ipi_chan_count,
+				 sizeof(struct mtk_ipi_chan_table), GFP_KERNEL);
+	if (!ipi_chan_table)
+		return -ENOMEM;
+
+	mbdev = vcp_ipc->mbdev;
+	vcp_ipc->ipi_priv = (void *)ipidev;
+	ipidev->table = ipi_chan_table;
+	ipidev->vcp_ipc = vcp_ipc;
+
+	for (index = 0; index < ipi_chan_count; index++) {
+		atomic_set(&ipi_chan_table[index].holder, 0);
+		mutex_init(&ipi_chan_table[index].mutex_send);
+		init_completion(&ipi_chan_table[index].notify);
+		mtk_fill_in_entry(&ipi_chan_table[index], index, mbdev);
+	}
+
+	ipidev->ipi_inited = 1;
+
+	dev_dbg(vcp_ipc->dev, "%s (with %d IPI) has registered.\n",
+		ipidev->name, ipi_chan_count);
+
+	return IPI_ACTION_DONE;
+}
+EXPORT_SYMBOL(mtk_vcp_ipc_device_register);
+
+static int setup_mbox_table(struct mtk_mbox_table *mbdev, u32 mbox)
+{
+	struct mtk_mbox_send_table *mbox_send = &mbdev->send_table[0];
+	struct mtk_mbox_recv_table *mbox_recv = &mbdev->recv_table[0];
+	u32 i, last_ofs = 0, last_idx = 0, last_slot = 0, last_sz = 0;
+
+	for (i = 0; i < mbdev->send_count; i++) {
+		if (mbox == mbox_send[i].mbox_id) {
+			mbox_send[i].offset = last_ofs + last_slot;
+			mbox_send[i].pin_index = last_idx + last_sz;
+			last_idx = mbox_send[i].pin_index;
+			last_sz = DIV_ROUND_UP(mbox_send[i].msg_size, MBOX_SLOT_ALIGN);
+			last_ofs = last_sz * MBOX_SLOT_ALIGN;
+			last_slot = last_idx * MBOX_SLOT_ALIGN;
+		} else if (mbox < mbox_send[i].mbox_id) {
+			/* no need to search the rest id */
+			break;
+		}
+	}
+
+	for (i = 0; i < mbdev->recv_count; i++) {
+		if (mbox == mbox_recv[i].mbox_id) {
+			mbox_recv[i].offset = last_ofs + last_slot;
+			mbox_recv[i].pin_index = last_idx + last_sz;
+			last_idx = mbox_recv[i].pin_index;
+			last_sz = DIV_ROUND_UP(mbox_recv[i].msg_size, MBOX_SLOT_ALIGN);
+			last_ofs = last_sz * MBOX_SLOT_ALIGN;
+			last_slot = last_idx * MBOX_SLOT_ALIGN;
+		} else if (mbox < mbox_recv[i].mbox_id) {
+			/* no need to search the rest id */
+			break;
+		}
+	}
+
+	if (last_idx > MBOX_MAX_PIN || (last_ofs + last_slot) > MAX_SLOT_NUM)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int mtk_vcp_ipc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mtk_vcp_ipc *vcp_ipc;
+	struct mbox_client *cl;
+	struct mtk_mbox_info *minfo;
+	int ret;
+	u32 mbox, i;
+	struct mtk_mbox_table *mbox_data = dev_get_platdata(dev);
+
+	device_set_of_node_from_dev(&pdev->dev, pdev->dev.parent);
+
+	vcp_ipc = devm_kzalloc(dev, sizeof(*vcp_ipc), GFP_KERNEL);
+	if (!vcp_ipc)
+		return -ENOMEM;
+
+	if (!mbox_data) {
+		dev_err(dev, "No platform data available\n");
+		return -EINVAL;
+	}
+	vcp_ipc->mbdev = mbox_data;
+
+	/* alloc and init mmup_mbox_info */
+	vcp_ipc->info_table = vzalloc(sizeof(*vcp_ipc->info_table) * VCP_MBOX_NUM);
+	if (!vcp_ipc->info_table)
+		return -ENOMEM;
+
+	/* create mbox dev */
+	for (mbox = 0; mbox < VCP_MBOX_NUM; mbox++) {
+		minfo = &vcp_ipc->info_table[mbox];
+		minfo->mbox_id = mbox;
+		minfo->vcp_ipc = vcp_ipc;
+		spin_lock_init(&minfo->mbox_lock);
+
+		ret = setup_mbox_table(vcp_ipc->mbdev, mbox);
+		if (ret)
+			return ret;
+
+		cl = &minfo->cl;
+		cl->dev = &pdev->dev;
+		cl->tx_block = false;
+		cl->knows_txdone = false;
+		cl->tx_prepare = NULL;
+		cl->rx_callback = mtk_vcp_ipc_recv;
+		minfo->ch = mbox_request_channel_byname(cl, mbox_names[mbox]);
+		if (IS_ERR(minfo->ch)) {
+			ret = PTR_ERR(minfo->ch);
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "Failed to request mbox channel %s ret %d\n",
+					mbox_names[mbox], ret);
+
+			for (i = 0; i < mbox; i++) {
+				minfo = &vcp_ipc->info_table[i];
+				mbox_free_channel(minfo->ch);
+			}
+
+			vfree(vcp_ipc->info_table);
+			return ret;
+		}
+	}
+
+	vcp_ipc->dev = dev;
+	dev_set_drvdata(dev, vcp_ipc);
+	dev_dbg(dev, "MTK VCP IPC initialized\n");
+
+	return 0;
+}
+
+static void mtk_vcp_ipc_remove(struct platform_device *pdev)
+{
+	struct mtk_vcp_ipc *vcp_ipc = dev_get_drvdata(&pdev->dev);
+	struct mtk_mbox_info *minfo;
+	int i;
+
+	for (i = 0; i < VCP_MBOX_NUM; i++) {
+		minfo = &vcp_ipc->info_table[i];
+		mbox_free_channel(minfo->ch);
+	}
+
+	vfree(vcp_ipc->info_table);
+}
+
+static struct platform_driver mtk_vcp_ipc_driver = {
+	.probe = mtk_vcp_ipc_probe,
+	.remove = mtk_vcp_ipc_remove,
+	.driver = {
+		.name = "mtk-vcp-ipc",
+	},
+};
+builtin_platform_driver(mtk_vcp_ipc_driver);
+
+MODULE_AUTHOR("Jjian Zhou <jjian.zhou@mediatek.com>");
+MODULE_DESCRIPTION("MediaTek VCP IPC Controller");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/firmware/mediatek/mtk-vcp-ipc.h b/include/linux/firmware/mediatek/mtk-vcp-ipc.h
new file mode 100644
index 000000000000..dc34b0ba9dd8
--- /dev/null
+++ b/include/linux/firmware/mediatek/mtk-vcp-ipc.h
@@ -0,0 +1,151 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+/*
+ * Copyright (c) 2024 MediaTek Inc.
+ */
+
+#ifndef __MTK_VCP_IPC_H__
+#define __MTK_VCP_IPC_H__
+
+#include <linux/completion.h>
+#include <linux/mailbox_client.h>
+#include <linux/mailbox/mtk-vcp-mailbox.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+/* IPI result definition */
+#define IPI_ACTION_DONE	  0
+#define IPI_DEV_ILLEGAL	 -1 /* ipi device is not initialized */
+#define IPI_ALREADY_USED	 -2 /* the ipi has be registered */
+#define IPI_UNAVAILABLE	 -3 /* the ipi can't be found */
+#define IPI_NO_MSGBUF		 -4 /* receiver doesn't have message buffer */
+#define IPI_MSG_TOO_BIG		 -5 /* message length is larger than defined */
+#define IPI_MBOX_ERR		-99 /* some error from rpmsg layer */
+
+/* mbox recv action definition */
+enum mtk_ipi_recv_opt {
+	MBOX_RECV_MESSAGE  = 0,
+	MBOX_RECV_ACK      = 1,
+};
+
+/* mbox table item number definition */
+#define send_item_num	3
+#define recv_item_num	4
+#define VCP_MBOX_NUM	5
+
+/* mbox slot size definition: 1 slot for 4 bytes */
+#define MBOX_SLOT_SIZE	0x4
+#define MBOX_MAX_PIN	32
+#define VCP_MBOX_NUM	5
+#define MBOX_SLOT_ALIGN	2
+
+struct mtk_vcp_ipc;
+struct mtk_ipi_chan_table;
+
+typedef int (*mbox_pin_cb_t)(u32 ipi_id, void *prdata, void *data, u32 len);
+
+/**
+ * mbox pin structure, this is for send definition,
+ * @offset: message offset in the slots of a mbox
+ * @msg_size: message used slots in the mbox, 4 bytes alignment
+ * @pin_index: bit offset in the mbox
+ * @ipi_id: ipi enum number
+ * @mbox_id: mbox number id
+ */
+struct mtk_mbox_send_table {
+	u32 offset;
+	u32 msg_size;
+	u32 pin_index;
+	u32 ipi_id;
+	u32 mbox_id;
+};
+
+/**
+ * mbox pin structure, this is for receive definition,
+ * @offset: message offset in the slots of a mbox
+ * @recv_opt: recv option,  0:receive ,1: response
+ * @msg_size: message used slots in the mbox, 4 bytes alignment
+ * @pin_index: bit offset in the mbox
+ * @ipi_id: ipi enum number
+ * @mbox_id: mbox number id
+ */
+struct mtk_mbox_recv_table {
+	u32 offset;
+	u32 recv_opt;
+	u32 msg_size;
+	u32 pin_index;
+	u32 ipi_id;
+	u32 mbox_id;
+};
+
+/**
+ * struct mtk_ipi_device - device for represent the tinysys using mtk ipi
+ * @name: name of tinysys device
+ * @id: device id (used to match between rpmsg drivers and devices)
+ * @vcp_ipc: vcp ipc structure for tinysys device
+ * @table: channel table with endpoint & channel_info & mbox_pin info
+ * @prdata: private data for the callback use
+ * @ipi_inited: set when vcp_ipi_device_register() done
+ */
+struct mtk_ipi_device  {
+	const char *name;
+	struct mtk_vcp_ipc *vcp_ipc;
+	struct mtk_ipi_chan_table *table;
+	void *prdata;
+	int ipi_inited;
+};
+
+/**
+ * The mtk_mbox_table is a structure used to record the send
+ * table and recv table. The send table is used to record
+ * the feature ID and size of the sent data. The recv table
+ * is used to record the feature ID and size of the received
+ * data, and whether a callback needs to be invoked.
+ *
+ * Following are platform specific interfacer
+ * @recv_table: structure mtk_mbox_recv_table
+ * @send_table: structure mtk_mbox_send_table
+ * @recv_count: receive feature number in this channel
+ * @send_count: send feature number in this channel
+ */
+struct mtk_mbox_table {
+	struct mtk_mbox_recv_table recv_table[32];
+	struct mtk_mbox_send_table send_table[32];
+	u32 recv_count;
+	u32 send_count;
+};
+
+/**
+ * Mbox is a dedicate hardware of a tinysys consists of:
+ * 1) a share memory tightly coupled to the tinysys
+ * 2) several IRQs
+ *
+ * Following are platform specific interface
+ * @dev: vcp device
+ * @name: identity of the device
+ * @info_table: mbox info structure
+ * @ipi_priv: private data for synchronization layer
+ * @mbox_id: mbox number
+ * @mbdev: mtk_mbox_table structure
+ */
+struct mtk_vcp_ipc {
+	struct device *dev;
+	const char *name;
+	struct mtk_mbox_info *info_table;
+	void *ipi_priv;
+	void *mbox_id;
+	struct mtk_mbox_table *mbdev;
+};
+
+int mtk_vcp_ipc_device_register(struct mtk_ipi_device *ipidev,
+				u32 ipi_chan_count,
+				struct mtk_vcp_ipc *vcp_ipc);
+int mtk_vcp_ipc_send(struct mtk_ipi_device *ipidev, u32 ipi_id,
+		     void *data, u32 len);
+int mtk_vcp_ipc_send_compl(struct mtk_ipi_device *ipidev, u32 ipi_id,
+			   void *data, u32 len, u32 timeout_ms);
+int mtk_vcp_mbox_ipc_register(struct mtk_ipi_device *ipidev, int ipi_id,
+			      mbox_pin_cb_t cb, void *prdata, void *msg);
+int mtk_vcp_mbox_ipc_unregister(struct mtk_ipi_device *ipidev, int ipi_id);
+
+#endif
-- 
2.45.2

Re: [PATCH RFC v3 2/3] firmware: mediatek: Add vcp ipc protocol interface

[Krzysztof Kozlowski]

On 17/03/2025 12:03, Jjian Zhou wrote:
> Some of mediatek processors contain the Risc-V coprocessor.
> 
> The communication between Host CPU and vcp firmware is
> taking place using a shared memory area for message passing.
> 
> VCP IPC protocol offers (send/recv) interfaces using
> mediatek-mailbox APIs.
> 
> Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>
> ---
>  drivers/firmware/Kconfig                      |   9 +
>  drivers/firmware/Makefile                     |   1 +
>  drivers/firmware/mtk-vcp-ipc.c                | 481 ++++++++++++++++++
>  include/linux/firmware/mediatek/mtk-vcp-ipc.h | 151 ++++++
>  4 files changed, 642 insertions(+)
>  create mode 100644 drivers/firmware/mtk-vcp-ipc.c
>  create mode 100644 include/linux/firmware/mediatek/mtk-vcp-ipc.h

Do not send new versions while previous discussion is still going.

You still did not respond to previous feedback and you already sent two
versions repeating the same mistake.

> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 37e43f287e78..98c4ff667836 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -179,6 +179,15 @@ config MTK_ADSP_IPC
>  	  ADSP exists on some mtk processors.
>  	  Client might use shared memory to exchange information with ADSP.
>  

..

> +
> +/*
> + * mtk_vcp_ipc_send - send ipc command to MTK VCP
> + *
> + * @ipidev: VCP struct mtk_ipi_device handle
> + * @id: id of the feature IPI
> + * @data: message address
> + * @len: message length
> + *
> + * Return: Zero for success from mbox_send_message
> + *         negative value for error
> + */
> +int mtk_vcp_ipc_send(struct mtk_ipi_device *ipidev, u32 id, void *data, u32 len)
> +{
> +	struct device *dev;
> +	struct mtk_mbox_info *minfo;
> +	struct mtk_ipi_chan_table *table;
> +	struct mtk_vcp_ipc *vcp_ipc;
> +	int ret;
> +
> +	if (!ipidev || !ipidev->ipi_inited || !data)
> +		return IPI_UNAVAILABLE;
> +	vcp_ipc = ipidev->vcp_ipc;
> +	if (!vcp_ipc)
> +		return IPI_UNAVAILABLE;
> +
> +	table = ipidev->table;
> +	dev = ipidev->vcp_ipc->dev;
> +	minfo = &ipidev->vcp_ipc->info_table[table[id].mbox];
> +	if (!minfo) {
> +		dev_err(dev, "%s IPI%d minfo is invalid.\n", ipidev->name, id);
> +		return IPI_UNAVAILABLE;
> +	}
> +
> +	if (len > table[id].msg_size)
> +		return IPI_MSG_TOO_BIG;
> +	else if (!len)
> +		len = table[id].msg_size;
> +
> +	mutex_lock(&table[id].mutex_send);
> +
> +	minfo->ipi_info.msg = data;
> +	minfo->ipi_info.len = len;
> +	minfo->ipi_info.id = id;
> +	minfo->ipi_info.index = table[id].send_index;
> +	minfo->ipi_info.slot_ofs = table[id].send_ofs * MBOX_SLOT_SIZE;
> +
> +	ret = mbox_send_message(minfo->ch, &minfo->ipi_info);
> +	mutex_unlock(&table[id].mutex_send);
> +	if (ret < 0) {
> +		dev_err(dev, "%s IPI%d send failed.\n", ipidev->name, id);
> +		return IPI_MBOX_ERR;
> +	}
> +
> +	return IPI_ACTION_DONE;
> +}
> +EXPORT_SYMBOL(mtk_vcp_ipc_send);

Drop export - no users

Anyway, every export must be GPL.

> +
> +/*
> + * mtk_vcp_ipc_send_compl - send ipc command to MTK VCP
> + *
> + * @ipidev: VCP struct mtk_ipi_device handle
> + * @id: id of the feature IPI
> + * @data: message address
> + * @len: message length
> + * @timeout_ms:
> + *
> + * Return: Zero for success from mbox_send_message
> + *         negative value for error
> + */
> +int mtk_vcp_ipc_send_compl(struct mtk_ipi_device *ipidev, u32 id,
> +			   void *data, u32 len, u32 timeout_ms)
> +{
> +	struct device *dev;
> +	struct mtk_mbox_info *minfo;
> +	struct mtk_ipi_chan_table *table;
> +	struct mtk_vcp_ipc *vcp_ipc;
> +	int ret;
> +
> +	if (!ipidev || !ipidev->ipi_inited || !data)
> +		return IPI_UNAVAILABLE;
> +	vcp_ipc = ipidev->vcp_ipc;
> +	if (!vcp_ipc)
> +		return IPI_UNAVAILABLE;
> +
> +	table = ipidev->table;
> +	dev = ipidev->vcp_ipc->dev;
> +	minfo = &ipidev->vcp_ipc->info_table[table[id].mbox];
> +	if (!minfo) {
> +		dev_err(dev, "%s IPI%d minfo is invalid.\n", ipidev->name, id);
> +		return IPI_UNAVAILABLE;
> +	}
> +
> +	if (len > table[id].msg_size)
> +		return IPI_MSG_TOO_BIG;
> +	else if (!len)
> +		len = table[id].msg_size;
> +
> +	mutex_lock(&table[id].mutex_send);
> +
> +	minfo->ipi_info.msg = data;
> +	minfo->ipi_info.len = len;
> +	minfo->ipi_info.id = id;
> +	minfo->ipi_info.index = table[id].send_index;
> +	minfo->ipi_info.slot_ofs = table[id].send_ofs * MBOX_SLOT_SIZE;
> +
> +	atomic_inc(&table[id].holder);
> +
> +	ret = mbox_send_message(minfo->ch, &minfo->ipi_info);
> +	if (ret < 0) {
> +		atomic_set(&table[id].holder, 0);
> +		mutex_unlock(&table[id].mutex_send);
> +		dev_err(dev, "%s IPI%d send failed.\n", ipidev->name, id);
> +		return IPI_MBOX_ERR;
> +	}
> +
> +	/* wait for completion */
> +	ret = wait_for_completion_timeout(&table[id].notify,
> +					  msecs_to_jiffies(timeout_ms));
> +	atomic_set(&table[id].holder, 0);
> +	if (ret > 0)
> +		ret = IPI_ACTION_DONE;
> +
> +	mutex_unlock(&table[id].mutex_send);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(mtk_vcp_ipc_send_compl);

NAK, no users.

> +
> +int mtk_vcp_mbox_ipc_register(struct mtk_ipi_device *ipidev, int id,
> +			      mbox_pin_cb_t cb, void *prdata, void *msg)
> +{
> +	if (!ipidev || !ipidev->ipi_inited)
> +		return IPI_DEV_ILLEGAL;
> +	if (!msg)
> +		return IPI_NO_MSGBUF;
> +
> +	if (ipidev->table[id].pin_buf)
> +		return IPI_ALREADY_USED;
> +	ipidev->table[id].mbox_pin_cb = cb;
> +	ipidev->table[id].pin_buf = msg;
> +	ipidev->table[id].prdata = prdata;
> +
> +	return IPI_ACTION_DONE;
> +}
> +EXPORT_SYMBOL(mtk_vcp_mbox_ipc_register);

NAK, no users.


> +
> +int mtk_vcp_mbox_ipc_unregister(struct mtk_ipi_device *ipidev, int id)
> +{
> +	if (!ipidev || !ipidev->ipi_inited)
> +		return IPI_DEV_ILLEGAL;
> +
> +	/* Drop the ipi and reset the record */
> +	complete(&ipidev->table[id].notify);
> +
> +	ipidev->table[id].mbox_pin_cb = NULL;
> +	ipidev->table[id].pin_buf = NULL;
> +	ipidev->table[id].prdata = NULL;
> +
> +	return IPI_ACTION_DONE;
> +}
> +EXPORT_SYMBOL(mtk_vcp_mbox_ipc_unregister);

NAK, no users.


> +
> +static void mtk_fill_in_entry(struct mtk_ipi_chan_table *entry, const u32 ipi_id,
> +			      const struct mtk_mbox_table *mbdev)
> +{
> +	const struct mtk_mbox_send_table *mbox_send = mbdev->send_table;
> +	u32 index;
> +
> +	for (index = 0; index < mbdev->send_count; index++) {
> +		if (ipi_id != mbox_send[index].ipi_id)
> +			continue;
> +
> +		entry->send_ofs = mbox_send[index].offset;
> +		entry->send_index = mbox_send[index].pin_index;
> +		entry->msg_size = mbox_send[index].msg_size;
> +		entry->mbox = mbox_send[index].mbox_id;
> +		return;
> +	}
> +
> +	entry->mbox = -ENOENT;
> +}
> +
> +int mtk_vcp_ipc_device_register(struct mtk_ipi_device *ipidev,
> +				u32 ipi_chan_count, struct mtk_vcp_ipc *vcp_ipc)
> +{
> +	struct mtk_ipi_chan_table *ipi_chan_table;
> +	struct mtk_mbox_table *mbdev;
> +	u32 index;
> +
> +	if (!vcp_ipc || !ipidev)
> +		return -EINVAL;
> +
> +	ipi_chan_table = kcalloc(ipi_chan_count,
> +				 sizeof(struct mtk_ipi_chan_table), GFP_KERNEL);
> +	if (!ipi_chan_table)
> +		return -ENOMEM;
> +
> +	mbdev = vcp_ipc->mbdev;
> +	vcp_ipc->ipi_priv = (void *)ipidev;
> +	ipidev->table = ipi_chan_table;
> +	ipidev->vcp_ipc = vcp_ipc;
> +
> +	for (index = 0; index < ipi_chan_count; index++) {
> +		atomic_set(&ipi_chan_table[index].holder, 0);
> +		mutex_init(&ipi_chan_table[index].mutex_send);
> +		init_completion(&ipi_chan_table[index].notify);
> +		mtk_fill_in_entry(&ipi_chan_table[index], index, mbdev);
> +	}
> +
> +	ipidev->ipi_inited = 1;
> +
> +	dev_dbg(vcp_ipc->dev, "%s (with %d IPI) has registered.\n",
> +		ipidev->name, ipi_chan_count);
> +
> +	return IPI_ACTION_DONE;
> +}
> +EXPORT_SYMBOL(mtk_vcp_ipc_device_register);

NAK, no users.


> +
> +static int setup_mbox_table(struct mtk_mbox_table *mbdev, u32 mbox)
> +{
> +	struct mtk_mbox_send_table *mbox_send = &mbdev->send_table[0];
> +	struct mtk_mbox_recv_table *mbox_recv = &mbdev->recv_table[0];
> +	u32 i, last_ofs = 0, last_idx = 0, last_slot = 0, last_sz = 0;
> +
> +	for (i = 0; i < mbdev->send_count; i++) {
> +		if (mbox == mbox_send[i].mbox_id) {
> +			mbox_send[i].offset = last_ofs + last_slot;
> +			mbox_send[i].pin_index = last_idx + last_sz;
> +			last_idx = mbox_send[i].pin_index;
> +			last_sz = DIV_ROUND_UP(mbox_send[i].msg_size, MBOX_SLOT_ALIGN);
> +			last_ofs = last_sz * MBOX_SLOT_ALIGN;
> +			last_slot = last_idx * MBOX_SLOT_ALIGN;
> +		} else if (mbox < mbox_send[i].mbox_id) {
> +			/* no need to search the rest id */
> +			break;
> +		}
> +	}
> +
> +	for (i = 0; i < mbdev->recv_count; i++) {
> +		if (mbox == mbox_recv[i].mbox_id) {
> +			mbox_recv[i].offset = last_ofs + last_slot;
> +			mbox_recv[i].pin_index = last_idx + last_sz;
> +			last_idx = mbox_recv[i].pin_index;
> +			last_sz = DIV_ROUND_UP(mbox_recv[i].msg_size, MBOX_SLOT_ALIGN);
> +			last_ofs = last_sz * MBOX_SLOT_ALIGN;
> +			last_slot = last_idx * MBOX_SLOT_ALIGN;
> +		} else if (mbox < mbox_recv[i].mbox_id) {
> +			/* no need to search the rest id */
> +			break;
> +		}
> +	}
> +
> +	if (last_idx > MBOX_MAX_PIN || (last_ofs + last_slot) > MAX_SLOT_NUM)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int mtk_vcp_ipc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mtk_vcp_ipc *vcp_ipc;
> +	struct mbox_client *cl;
> +	struct mtk_mbox_info *minfo;
> +	int ret;
> +	u32 mbox, i;
> +	struct mtk_mbox_table *mbox_data = dev_get_platdata(dev);
> +
> +	device_set_of_node_from_dev(&pdev->dev, pdev->dev.parent);
> +
> +	vcp_ipc = devm_kzalloc(dev, sizeof(*vcp_ipc), GFP_KERNEL);
> +	if (!vcp_ipc)
> +		return -ENOMEM;
> +
> +	if (!mbox_data) {

Check goes immediately after declaration. I doubt it is useful in the
first place as this cannot even happen...


> +		dev_err(dev, "No platform data available\n");

No, drop. Cannot happen or fix your drivers. Who provides the platdata here?

> +		return -EINVAL;
> +	}
> +	vcp_ipc->mbdev = mbox_data;
> +
> +	/* alloc and init mmup_mbox_info */
> +	vcp_ipc->info_table = vzalloc(sizeof(*vcp_ipc->info_table) * VCP_MBOX_NUM);
> +	if (!vcp_ipc->info_table)
> +		return -ENOMEM;
> +
> +	/* create mbox dev */
> +	for (mbox = 0; mbox < VCP_MBOX_NUM; mbox++) {
> +		minfo = &vcp_ipc->info_table[mbox];
> +		minfo->mbox_id = mbox;
> +		minfo->vcp_ipc = vcp_ipc;
> +		spin_lock_init(&minfo->mbox_lock);
> +
> +		ret = setup_mbox_table(vcp_ipc->mbdev, mbox);
> +		if (ret)
> +			return ret;
> +
> +		cl = &minfo->cl;
> +		cl->dev = &pdev->dev;
> +		cl->tx_block = false;
> +		cl->knows_txdone = false;
> +		cl->tx_prepare = NULL;
> +		cl->rx_callback = mtk_vcp_ipc_recv;
> +		minfo->ch = mbox_request_channel_byname(cl, mbox_names[mbox]);
> +		if (IS_ERR(minfo->ch)) {
> +			ret = PTR_ERR(minfo->ch);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(dev, "Failed to request mbox channel %s ret %d\n",
> +					mbox_names[mbox], ret);

Do not open code dev_err_probe.

> +
> +			for (i = 0; i < mbox; i++) {
> +				minfo = &vcp_ipc->info_table[i];
> +				mbox_free_channel(minfo->ch);
> +			}
> +
> +			vfree(vcp_ipc->info_table);
> +			return ret;
> +		}
> +	}
> +
> +	vcp_ipc->dev = dev;
> +	dev_set_drvdata(dev, vcp_ipc);
> +	dev_dbg(dev, "MTK VCP IPC initialized\n");

No, drop


Best regards,
Krzysztof

Re: [PATCH RFC v3 2/3] firmware: mediatek: Add vcp ipc protocol interface

[Jjian Zhou (周建)]

On Mon, 2025-03-17 at 18:16 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 17/03/2025 12:03, Jjian Zhou wrote:
> > Some of mediatek processors contain the Risc-V coprocessor.
> > 
> > The communication between Host CPU and vcp firmware is
> > taking place using a shared memory area for message passing.
> > 
> > VCP IPC protocol offers (send/recv) interfaces using
> > mediatek-mailbox APIs.
> > 
> > Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>
> > ---
> >  drivers/firmware/Kconfig                      |   9 +
> >  drivers/firmware/Makefile                     |   1 +
> >  drivers/firmware/mtk-vcp-ipc.c                | 481
> > ++++++++++++++++++
> >  include/linux/firmware/mediatek/mtk-vcp-ipc.h | 151 ++++++
> >  4 files changed, 642 insertions(+)
> >  create mode 100644 drivers/firmware/mtk-vcp-ipc.c
> >  create mode 100644 include/linux/firmware/mediatek/mtk-vcp-ipc.h
> 
> Do not send new versions while previous discussion is still going.
> 
> You still did not respond to previous feedback and you already sent
> two
> versions repeating the same mistake.
> 

I'm sorry for my mistake. 
After we have discussed it, I will fix it in the next version.


> > 
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index 37e43f287e78..98c4ff667836 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -179,6 +179,15 @@ config MTK_ADSP_IPC
> >         ADSP exists on some mtk processors.
> >         Client might use shared memory to exchange information with
> > ADSP.
> > 
> 
> ..
> 
> > +
> > +/*
> > + * mtk_vcp_ipc_send - send ipc command to MTK VCP
> > + *
> > + * @ipidev: VCP struct mtk_ipi_device handle
> > + * @id: id of the feature IPI
> > + * @data: message address
> > + * @len: message length
> > + *
> > + * Return: Zero for success from mbox_send_message
> > + *         negative value for error
> > + */
> > +int mtk_vcp_ipc_send(struct mtk_ipi_device *ipidev, u32 id, void
> > *data, u32 len)
> > +{
> > +     struct device *dev;
> > +     struct mtk_mbox_info *minfo;
> > +     struct mtk_ipi_chan_table *table;
> > +     struct mtk_vcp_ipc *vcp_ipc;
> > +     int ret;
> > +
> > +     if (!ipidev || !ipidev->ipi_inited || !data)
> > +             return IPI_UNAVAILABLE;
> > +     vcp_ipc = ipidev->vcp_ipc;
> > +     if (!vcp_ipc)
> > +             return IPI_UNAVAILABLE;
> > +
> > +     table = ipidev->table;
> > +     dev = ipidev->vcp_ipc->dev;
> > +     minfo = &ipidev->vcp_ipc->info_table[table[id].mbox];
> > +     if (!minfo) {
> > +             dev_err(dev, "%s IPI%d minfo is invalid.\n", ipidev-
> > >name, id);
> > +             return IPI_UNAVAILABLE;
> > +     }
> > +
> > +     if (len > table[id].msg_size)
> > +             return IPI_MSG_TOO_BIG;
> > +     else if (!len)
> > +             len = table[id].msg_size;
> > +
> > +     mutex_lock(&table[id].mutex_send);
> > +
> > +     minfo->ipi_info.msg = data;
> > +     minfo->ipi_info.len = len;
> > +     minfo->ipi_info.id = id;
> > +     minfo->ipi_info.index = table[id].send_index;
> > +     minfo->ipi_info.slot_ofs = table[id].send_ofs *
> > MBOX_SLOT_SIZE;
> > +
> > +     ret = mbox_send_message(minfo->ch, &minfo->ipi_info);
> > +     mutex_unlock(&table[id].mutex_send);
> > +     if (ret < 0) {
> > +             dev_err(dev, "%s IPI%d send failed.\n", ipidev->name, 
> > id);
> > +             return IPI_MBOX_ERR;
> > +     }
> > +
> > +     return IPI_ACTION_DONE;
> > +}
> > +EXPORT_SYMBOL(mtk_vcp_ipc_send);
> 
> Drop export - no users
> 
> Anyway, every export must be GPL.

The Video Companion Processor (VCP) driver (currently being prepared
for submission to the community) will call it.

> 
> > +
> > +/*
> > + * mtk_vcp_ipc_send_compl - send ipc command to MTK VCP
> > + *
> > + * @ipidev: VCP struct mtk_ipi_device handle
> > + * @id: id of the feature IPI
> > + * @data: message address
> > + * @len: message length
> > + * @timeout_ms:
> > + *
> > + * Return: Zero for success from mbox_send_message
> > + *         negative value for error
> > + */
> > +int mtk_vcp_ipc_send_compl(struct mtk_ipi_device *ipidev, u32 id,
> > +                        void *data, u32 len, u32 timeout_ms)
> > +{
> > +     struct device *dev;
> > +     struct mtk_mbox_info *minfo;
> > +     struct mtk_ipi_chan_table *table;
> > +     struct mtk_vcp_ipc *vcp_ipc;
> > +     int ret;
> > +
> > +     if (!ipidev || !ipidev->ipi_inited || !data)
> > +             return IPI_UNAVAILABLE;
> > +     vcp_ipc = ipidev->vcp_ipc;
> > +     if (!vcp_ipc)
> > +             return IPI_UNAVAILABLE;
> > +
> > +     table = ipidev->table;
> > +     dev = ipidev->vcp_ipc->dev;
> > +     minfo = &ipidev->vcp_ipc->info_table[table[id].mbox];
> > +     if (!minfo) {
> > +             dev_err(dev, "%s IPI%d minfo is invalid.\n", ipidev-
> > >name, id);
> > +             return IPI_UNAVAILABLE;
> > +     }
> > +
> > +     if (len > table[id].msg_size)
> > +             return IPI_MSG_TOO_BIG;
> > +     else if (!len)
> > +             len = table[id].msg_size;
> > +
> > +     mutex_lock(&table[id].mutex_send);
> > +
> > +     minfo->ipi_info.msg = data;
> > +     minfo->ipi_info.len = len;
> > +     minfo->ipi_info.id = id;
> > +     minfo->ipi_info.index = table[id].send_index;
> > +     minfo->ipi_info.slot_ofs = table[id].send_ofs *
> > MBOX_SLOT_SIZE;
> > +
> > +     atomic_inc(&table[id].holder);
> > +
> > +     ret = mbox_send_message(minfo->ch, &minfo->ipi_info);
> > +     if (ret < 0) {
> > +             atomic_set(&table[id].holder, 0);
> > +             mutex_unlock(&table[id].mutex_send);
> > +             dev_err(dev, "%s IPI%d send failed.\n", ipidev->name, 
> > id);
> > +             return IPI_MBOX_ERR;
> > +     }
> > +
> > +     /* wait for completion */
> > +     ret = wait_for_completion_timeout(&table[id].notify,
> > +                                       msecs_to_jiffies(timeout_ms
> > ));
> > +     atomic_set(&table[id].holder, 0);
> > +     if (ret > 0)
> > +             ret = IPI_ACTION_DONE;
> > +
> > +     mutex_unlock(&table[id].mutex_send);
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL(mtk_vcp_ipc_send_compl);
> 
> NAK, no users.
> 

The VCP driver will call it.

> > +
> > +int mtk_vcp_mbox_ipc_register(struct mtk_ipi_device *ipidev, int
> > id,
> > +                           mbox_pin_cb_t cb, void *prdata, void
> > *msg)
> > +{
> > +     if (!ipidev || !ipidev->ipi_inited)
> > +             return IPI_DEV_ILLEGAL;
> > +     if (!msg)
> > +             return IPI_NO_MSGBUF;
> > +
> > +     if (ipidev->table[id].pin_buf)
> > +             return IPI_ALREADY_USED;
> > +     ipidev->table[id].mbox_pin_cb = cb;
> > +     ipidev->table[id].pin_buf = msg;
> > +     ipidev->table[id].prdata = prdata;
> > +
> > +     return IPI_ACTION_DONE;
> > +}
> > +EXPORT_SYMBOL(mtk_vcp_mbox_ipc_register);
> 
> NAK, no users.
> 

The VCP driver will call it.

> 
> > +
> > +int mtk_vcp_mbox_ipc_unregister(struct mtk_ipi_device *ipidev, int
> > id)
> > +{
> > +     if (!ipidev || !ipidev->ipi_inited)
> > +             return IPI_DEV_ILLEGAL;
> > +
> > +     /* Drop the ipi and reset the record */
> > +     complete(&ipidev->table[id].notify);
> > +
> > +     ipidev->table[id].mbox_pin_cb = NULL;
> > +     ipidev->table[id].pin_buf = NULL;
> > +     ipidev->table[id].prdata = NULL;
> > +
> > +     return IPI_ACTION_DONE;
> > +}
> > +EXPORT_SYMBOL(mtk_vcp_mbox_ipc_unregister);
> 
> NAK, no users.
> 

The VCP driver will call it.

> 
> > +
> > +static void mtk_fill_in_entry(struct mtk_ipi_chan_table *entry,
> > const u32 ipi_id,
> > +                           const struct mtk_mbox_table *mbdev)
> > +{
> > +     const struct mtk_mbox_send_table *mbox_send = mbdev-
> > >send_table;
> > +     u32 index;
> > +
> > +     for (index = 0; index < mbdev->send_count; index++) {
> > +             if (ipi_id != mbox_send[index].ipi_id)
> > +                     continue;
> > +
> > +             entry->send_ofs = mbox_send[index].offset;
> > +             entry->send_index = mbox_send[index].pin_index;
> > +             entry->msg_size = mbox_send[index].msg_size;
> > +             entry->mbox = mbox_send[index].mbox_id;
> > +             return;
> > +     }
> > +
> > +     entry->mbox = -ENOENT;
> > +}
> > +
> > +int mtk_vcp_ipc_device_register(struct mtk_ipi_device *ipidev,
> > +                             u32 ipi_chan_count, struct
> > mtk_vcp_ipc *vcp_ipc)
> > +{
> > +     struct mtk_ipi_chan_table *ipi_chan_table;
> > +     struct mtk_mbox_table *mbdev;
> > +     u32 index;
> > +
> > +     if (!vcp_ipc || !ipidev)
> > +             return -EINVAL;
> > +
> > +     ipi_chan_table = kcalloc(ipi_chan_count,
> > +                              sizeof(struct mtk_ipi_chan_table),
> > GFP_KERNEL);
> > +     if (!ipi_chan_table)
> > +             return -ENOMEM;
> > +
> > +     mbdev = vcp_ipc->mbdev;
> > +     vcp_ipc->ipi_priv = (void *)ipidev;
> > +     ipidev->table = ipi_chan_table;
> > +     ipidev->vcp_ipc = vcp_ipc;
> > +
> > +     for (index = 0; index < ipi_chan_count; index++) {
> > +             atomic_set(&ipi_chan_table[index].holder, 0);
> > +             mutex_init(&ipi_chan_table[index].mutex_send);
> > +             init_completion(&ipi_chan_table[index].notify);
> > +             mtk_fill_in_entry(&ipi_chan_table[index], index,
> > mbdev);
> > +     }
> > +
> > +     ipidev->ipi_inited = 1;
> > +
> > +     dev_dbg(vcp_ipc->dev, "%s (with %d IPI) has registered.\n",
> > +             ipidev->name, ipi_chan_count);
> > +
> > +     return IPI_ACTION_DONE;
> > +}
> > +EXPORT_SYMBOL(mtk_vcp_ipc_device_register);
> 
> NAK, no users.
> 

The VCP driver will call it.

> 
> > +
> > +static int setup_mbox_table(struct mtk_mbox_table *mbdev, u32
> > mbox)
> > +{
> > +     struct mtk_mbox_send_table *mbox_send = &mbdev-
> > >send_table[0];
> > +     struct mtk_mbox_recv_table *mbox_recv = &mbdev-
> > >recv_table[0];
> > +     u32 i, last_ofs = 0, last_idx = 0, last_slot = 0, last_sz =
> > 0;
> > +
> > +     for (i = 0; i < mbdev->send_count; i++) {
> > +             if (mbox == mbox_send[i].mbox_id) {
> > +                     mbox_send[i].offset = last_ofs + last_slot;
> > +                     mbox_send[i].pin_index = last_idx + last_sz;
> > +                     last_idx = mbox_send[i].pin_index;
> > +                     last_sz = DIV_ROUND_UP(mbox_send[i].msg_size,
> > MBOX_SLOT_ALIGN);
> > +                     last_ofs = last_sz * MBOX_SLOT_ALIGN;
> > +                     last_slot = last_idx * MBOX_SLOT_ALIGN;
> > +             } else if (mbox < mbox_send[i].mbox_id) {
> > +                     /* no need to search the rest id */
> > +                     break;
> > +             }
> > +     }
> > +
> > +     for (i = 0; i < mbdev->recv_count; i++) {
> > +             if (mbox == mbox_recv[i].mbox_id) {
> > +                     mbox_recv[i].offset = last_ofs + last_slot;
> > +                     mbox_recv[i].pin_index = last_idx + last_sz;
> > +                     last_idx = mbox_recv[i].pin_index;
> > +                     last_sz = DIV_ROUND_UP(mbox_recv[i].msg_size,
> > MBOX_SLOT_ALIGN);
> > +                     last_ofs = last_sz * MBOX_SLOT_ALIGN;
> > +                     last_slot = last_idx * MBOX_SLOT_ALIGN;
> > +             } else if (mbox < mbox_recv[i].mbox_id) {
> > +                     /* no need to search the rest id */
> > +                     break;
> > +             }
> > +     }
> > +
> > +     if (last_idx > MBOX_MAX_PIN || (last_ofs + last_slot) >
> > MAX_SLOT_NUM)
> > +             return -EINVAL;
> > +
> > +     return 0;
> > +}
> > +
> > +static int mtk_vcp_ipc_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct mtk_vcp_ipc *vcp_ipc;
> > +     struct mbox_client *cl;
> > +     struct mtk_mbox_info *minfo;
> > +     int ret;
> > +     u32 mbox, i;
> > +     struct mtk_mbox_table *mbox_data = dev_get_platdata(dev);
> > +
> > +     device_set_of_node_from_dev(&pdev->dev, pdev->dev.parent);
> > +
> > +     vcp_ipc = devm_kzalloc(dev, sizeof(*vcp_ipc), GFP_KERNEL);
> > +     if (!vcp_ipc)
> > +             return -ENOMEM;
> > +
> > +     if (!mbox_data) {
> 
> Check goes immediately after declaration. I doubt it is useful in the
> first place as this cannot even happen...
> 
> 
> > +             dev_err(dev, "No platform data available\n");
> 
> No, drop. Cannot happen or fix your drivers. Who provides the
> platdata here?

The VCP driver will call platform_device_register_data to register the
structure data. mtk_vcp_ipc_probe will be triggered by vcp_probe. This
structure data is the structure we described in the cover letter.

> 
> > +             return -EINVAL;
> > +     }
> > +     vcp_ipc->mbdev = mbox_data;
> > +
> > +     /* alloc and init mmup_mbox_info */
> > +     vcp_ipc->info_table = vzalloc(sizeof(*vcp_ipc->info_table) *
> > VCP_MBOX_NUM);
> > +     if (!vcp_ipc->info_table)
> > +             return -ENOMEM;
> > +
> > +     /* create mbox dev */
> > +     for (mbox = 0; mbox < VCP_MBOX_NUM; mbox++) {
> > +             minfo = &vcp_ipc->info_table[mbox];
> > +             minfo->mbox_id = mbox;
> > +             minfo->vcp_ipc = vcp_ipc;
> > +             spin_lock_init(&minfo->mbox_lock);
> > +
> > +             ret = setup_mbox_table(vcp_ipc->mbdev, mbox);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             cl = &minfo->cl;
> > +             cl->dev = &pdev->dev;
> > +             cl->tx_block = false;
> > +             cl->knows_txdone = false;
> > +             cl->tx_prepare = NULL;
> > +             cl->rx_callback = mtk_vcp_ipc_recv;
> > +             minfo->ch = mbox_request_channel_byname(cl,
> > mbox_names[mbox]);
> > +             if (IS_ERR(minfo->ch)) {
> > +                     ret = PTR_ERR(minfo->ch);
> > +                     if (ret != -EPROBE_DEFER)
> > +                             dev_err(dev, "Failed to request mbox
> > channel %s ret %d\n",
> > +                                     mbox_names[mbox], ret);
> 
> Do not open code dev_err_probe.

OK, I modify it as below:
    dev_err_probe(dev, PTR_ERR(minfo->ch),
                  "Failed to request mbox channel %s \n",
                   mbox_names[mbox])

> 
> > +
> > +                     for (i = 0; i < mbox; i++) {
> > +                             minfo = &vcp_ipc->info_table[i];
> > +                             mbox_free_channel(minfo->ch);
> > +                     }
> > +
> > +                     vfree(vcp_ipc->info_table);
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     vcp_ipc->dev = dev;
> > +     dev_set_drvdata(dev, vcp_ipc);
> > +     dev_dbg(dev, "MTK VCP IPC initialized\n");
> 
> No, drop

OK. I will remove this debug log.

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH RFC v3 2/3] firmware: mediatek: Add vcp ipc protocol interface

[Krzysztof Kozlowski]

On 18/03/2025 09:32, Jjian Zhou (周建) wrote:
>>> +
>>> +     return IPI_ACTION_DONE;
>>> +}
>>> +EXPORT_SYMBOL(mtk_vcp_ipc_send);
>>
>> Drop export - no users
>>
>> Anyway, every export must be GPL.
> 
> The Video Companion Processor (VCP) driver (currently being prepared
> for submission to the community) will call it.
> 

It does not work like that. You must post the users NOW.

NAK


...

>> Check goes immediately after declaration. I doubt it is useful in the
>> first place as this cannot even happen...
>>
>>
>>> +             dev_err(dev, "No platform data available\n");
>>
>> No, drop. Cannot happen or fix your drivers. Who provides the
>> platdata here?
> 
> The VCP driver will call platform_device_register_data to register the
> structure data. mtk_vcp_ipc_probe will be triggered by vcp_probe. This
> structure data is the structure we described in the cover letter.


Comment is still valid.

Best regards,
Krzysztof

Re: [PATCH RFC v3 2/3] firmware: mediatek: Add vcp ipc protocol interface

[Jjian Zhou (周建)]

On Tue, 2025-03-18 at 10:00 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 18/03/2025 09:32, Jjian Zhou (周建) wrote:
> > > > +
> > > > +     return IPI_ACTION_DONE;
> > > > +}
> > > > +EXPORT_SYMBOL(mtk_vcp_ipc_send);
> > > 
> > > Drop export - no users
> > > 
> > > Anyway, every export must be GPL.
> > 
> > The Video Companion Processor (VCP) driver (currently being
> > prepared
> > for submission to the community) will call it.
> > 
> 
> It does not work like that. You must post the users NOW.
> 
> NAK
> 

Let's review this patch together after the VCP driver is ready. Patch 1
and Patch 3 will be combined into v4 for review. How do you think?

> 
> ...
> 
> > > Check goes immediately after declaration. I doubt it is useful in
> > > the
> > > first place as this cannot even happen...
> > > 
> > > 
> > > > +             dev_err(dev, "No platform data available\n");
> > > 
> > > No, drop. Cannot happen or fix your drivers. Who provides the
> > > platdata here?
> > 
> > The VCP driver will call platform_device_register_data to register
> > the
> > structure data. mtk_vcp_ipc_probe will be triggered by vcp_probe.
> > This
> > structure data is the structure we described in the cover letter.
> 
> 
> Comment is still valid.

Let's review this patch together after the VCP driver is ready. Patch 1
and Patch 3 will be combined into v4 for review. How do you think?

> 
> Best regards,
> Krzysztof

[PATCH RFC v3 3/3] dt-bindings: mailbox: mtk,mt8196-vcp-mbox: add mtk vcp-mbox document

[Jjian Zhou]

This patch adds document for mediatek vcp mbox.

Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>
---
 .../bindings/mailbox/mtk,mt8196-vcp-mbox.yaml | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/mtk,mt8196-vcp-mbox.yaml

diff --git a/Documentation/devicetree/bindings/mailbox/mtk,mt8196-vcp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/mtk,mt8196-vcp-mbox.yaml
new file mode 100644
index 000000000000..bd1b024e22f1
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/mtk,mt8196-vcp-mbox.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/mtk,mt8196-vcp-mbox.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek Video Companion Processor (VCP) mailbox
+
+maintainers:
+  - Jjian Zhou <Jjian.Zhou@mediatek.com>
+
+description:
+  The MTK VCP mailbox enables the SoC to communicate with the VCP by passing
+  messages through 64 32-bit wide registers. It has 32 interrupt vectors in
+  either direction for signalling purposes.
+
+properties:
+  compatible:
+    enum:
+      - mediatek,mt8196-vcp-mbox
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  "#mbox-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - "#mbox-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    mailbox@31b80000 {
+        compatible = "mediatek,mt8196-vcp-mbox";
+        reg = <0x31b80000 0x1000>;
+        interrupts = <GIC_SPI 789 IRQ_TYPE_LEVEL_HIGH 0>;
+        #mbox-cells = <0>;
+    };
-- 
2.45.2

Re: [PATCH RFC v3 3/3] dt-bindings: mailbox: mtk,mt8196-vcp-mbox: add mtk vcp-mbox document

[Rob Herring]

On Mon, Mar 17, 2025 at 07:03:25PM +0800, Jjian Zhou wrote:
> This patch adds document for mediatek vcp mbox.
> 
> Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>
> ---
>  .../bindings/mailbox/mtk,mt8196-vcp-mbox.yaml | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/mtk,mt8196-vcp-mbox.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/mtk,mt8196-vcp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/mtk,mt8196-vcp-mbox.yaml
> new file mode 100644
> index 000000000000..bd1b024e22f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/mtk,mt8196-vcp-mbox.yaml

Still wrong...

> +  compatible:
> +    enum:
> +      - mediatek,mt8196-vcp-mbox

mediatek,mt8196-vcp-mbox != mtk,mt8196-vcp-mbox

Re: [PATCH RFC v3 3/3] dt-bindings: mailbox: mtk,mt8196-vcp-mbox: add mtk vcp-mbox document

[Jjian Zhou (周建)]

On Mon, 2025-03-17 at 11:33 -0500, Rob Herring wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Mon, Mar 17, 2025 at 07:03:25PM +0800, Jjian Zhou wrote:
> > This patch adds document for mediatek vcp mbox.
> > 
> > Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>
> > ---
> >  .../bindings/mailbox/mtk,mt8196-vcp-mbox.yaml | 49
> > +++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/mailbox/mtk,mt8196-vcp-mbox.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/mailbox/mtk,mt8196-
> > vcp-mbox.yaml
> > b/Documentation/devicetree/bindings/mailbox/mtk,mt8196-vcp-
> > mbox.yaml
> > new file mode 100644
> > index 000000000000..bd1b024e22f1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/mtk,mt8196-vcp-
> > mbox.yaml
> 
> Still wrong...
> 
> > +  compatible:
> > +    enum:
> > +      - mediatek,mt8196-vcp-mbox
> 
> mediatek,mt8196-vcp-mbox != mtk,mt8196-vcp-mbox
> 
Hi Rob,

I'm sorry for my mistake.
I will change the file name to mediatek,mt8196-vcp-mbox.yaml.