Add MediaTek APU Mailbox Support For MT8196

[PATCH 3/3] mailbox: mediatek: Add mtk-apu-mailbox driver

Posted by Karl.Li 1 month ago

From: Karl Li <karl.li@mediatek.com>

Add mtk-apu-mailbox driver to support the communication with
APU remote microprocessor.

Also, the mailbox hardware contains extra spare (scratch) registers
that other hardware blocks use to communicate through.
Expose these with custom mtk_apu_mbox_(read|write)() functions.

Signed-off-by: Karl Li <karl.li@mediatek.com>
---
 drivers/mailbox/Kconfig                 |   9 +
 drivers/mailbox/Makefile                |   2 +
 drivers/mailbox/mtk-apu-mailbox.c       | 222 ++++++++++++++++++++++++
 include/linux/mailbox/mtk-apu-mailbox.h |  20 +++
 4 files changed, 253 insertions(+)
 create mode 100644 drivers/mailbox/mtk-apu-mailbox.c
 create mode 100644 include/linux/mailbox/mtk-apu-mailbox.h

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 6fb995778636..2338e08a110a 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -240,6 +240,15 @@ config MTK_ADSP_MBOX
           between processors with ADSP. It will place the message to share
 	  buffer and will access the ipc control.
 
+config MTK_APU_MBOX
+	tristate "MediaTek APU Mailbox Support"
+	depends on ARCH_MEDIATEK || COMPILE_TEST
+	help
+	  Say yes here to add support for the MediaTek APU Mailbox
+	  driver. The mailbox implementation provides access from the
+	  application processor to the MediaTek AI Processing Unit.
+	  If unsure say N.
+
 config MTK_CMDQ_MBOX
 	tristate "MediaTek CMDQ Mailbox Support"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 3c3c27d54c13..6b6dcc78d644 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -53,6 +53,8 @@ obj-$(CONFIG_STM32_IPCC) 	+= stm32-ipcc.o
 
 obj-$(CONFIG_MTK_ADSP_MBOX)	+= mtk-adsp-mailbox.o
 
+obj-$(CONFIG_MTK_APU_MBOX)	+= mtk-apu-mailbox.o
+
 obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o
 
 obj-$(CONFIG_ZYNQMP_IPI_MBOX)	+= zynqmp-ipi-mailbox.o
diff --git a/drivers/mailbox/mtk-apu-mailbox.c b/drivers/mailbox/mtk-apu-mailbox.c
new file mode 100644
index 000000000000..b347ebd34ef7
--- /dev/null
+++ b/drivers/mailbox/mtk-apu-mailbox.c
@@ -0,0 +1,222 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2024 MediaTek Inc.
+ */
+
+#include <asm/io.h>
+#include <linux/bits.h>
+#include <linux/interrupt.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mailbox/mtk-apu-mailbox.h>
+#include <linux/platform_device.h>
+
+#define INBOX		(0x0)
+#define OUTBOX		(0x20)
+#define INBOX_IRQ	(0xc0)
+#define OUTBOX_IRQ	(0xc4)
+#define INBOX_IRQ_MASK	(0xd0)
+
+#define SPARE_OFF_START	(0x40)
+#define SPARE_OFF_END	(0xB0)
+
+struct mtk_apu_mailbox {
+	struct device *dev;
+	void __iomem *regs;
+	struct mbox_controller controller;
+	u32 msgs[MSG_MBOX_SLOTS];
+};
+
+struct mtk_apu_mailbox *g_mbox;
+
+static irqreturn_t mtk_apu_mailbox_irq_top_half(int irq, void *dev_id)
+{
+	struct mtk_apu_mailbox *mbox = dev_id;
+	struct mbox_chan *link = &mbox->controller.chans[0];
+	int i;
+
+	for (i = 0; i < MSG_MBOX_SLOTS; i++)
+		mbox->msgs[i] = readl(mbox->regs + OUTBOX + i * sizeof(u32));
+
+	mbox_chan_received_data(link, &mbox->msgs);
+
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t mtk_apu_mailbox_irq_btm_half(int irq, void *dev_id)
+{
+	struct mtk_apu_mailbox *mbox = dev_id;
+	struct mbox_chan *link = &mbox->controller.chans[0];
+
+	mbox_chan_received_data_bh(link, &mbox->msgs);
+	writel(readl(mbox->regs + OUTBOX_IRQ), mbox->regs + OUTBOX_IRQ);
+
+	return IRQ_HANDLED;
+}
+
+static int mtk_apu_mailbox_send_data(struct mbox_chan *chan, void *data)
+{
+	struct mtk_apu_mailbox *mbox = container_of(chan->mbox,
+						    struct mtk_apu_mailbox,
+						    controller);
+	struct mtk_apu_mailbox_msg *msg = data;
+	int i;
+
+	if (msg->send_cnt <= 0 || msg->send_cnt > MSG_MBOX_SLOTS) {
+		dev_err(mbox->dev, "%s: invalid send_cnt %d\n", __func__, msg->send_cnt);
+		return -EINVAL;
+	}
+
+	/*
+	 *	Mask lowest "send_cnt-1" interrupts bits, so the interrupt on the other side
+	 *	triggers only after the last data slot is written (sent).
+	 */
+	writel(GENMASK(msg->send_cnt - 2, 0), mbox->regs + INBOX_IRQ_MASK);
+	for (i = 0; i < msg->send_cnt; i++)
+		writel(msg->data[i], mbox->regs + INBOX + i * sizeof(u32));
+
+	return 0;
+}
+
+static bool mtk_apu_mailbox_last_tx_done(struct mbox_chan *chan)
+{
+	struct mtk_apu_mailbox *mbox = container_of(chan->mbox,
+						    struct mtk_apu_mailbox,
+						    controller);
+
+	return readl(mbox->regs + INBOX_IRQ) == 0;
+}
+
+static const struct mbox_chan_ops mtk_apu_mailbox_ops = {
+	.send_data = mtk_apu_mailbox_send_data,
+	.last_tx_done = mtk_apu_mailbox_last_tx_done,
+};
+
+/**
+ * mtk_apu_mbox_write - Write value to specifice mtk_apu_mbox spare register.
+ * @val: Value to be written.
+ * @offset: Offset of the spare register.
+ *
+ * Return: 0 if successful
+ *	   negative value if error happened
+ */
+int mtk_apu_mbox_write(u32 val, u32 offset)
+{
+	if (!g_mbox) {
+		pr_err("mtk apu mbox was not initialized, stop writing register\n");
+		return -ENODEV;
+	}
+
+	if (offset < SPARE_OFF_START || offset >= SPARE_OFF_END) {
+		dev_err(g_mbox->dev, "Invalid offset %d for mtk apu mbox spare register\n", offset);
+		return -EINVAL;
+	}
+
+	writel(val, g_mbox->regs + offset);
+	return 0;
+}
+EXPORT_SYMBOL_NS(mtk_apu_mbox_write, MTK_APU_MAILBOX);
+
+/**
+ * mtk_apu_mbox_read - Read value to specifice mtk_apu_mbox spare register.
+ * @offset: Offset of the spare register.
+ * @val: Pointer to store read value.
+ *
+ * Return: 0 if successful
+ *	   negative value if error happened
+ */
+int mtk_apu_mbox_read(u32 offset, u32 *val)
+{
+	if (!g_mbox) {
+		pr_err("mtk apu mbox was not initialized, stop reading register\n");
+		return -ENODEV;
+	}
+
+	if (offset < SPARE_OFF_START || offset >= SPARE_OFF_END) {
+		dev_err(g_mbox->dev, "Invalid offset %d for mtk apu mbox spare register\n", offset);
+		return -EINVAL;
+	}
+
+	*val = readl(g_mbox->regs + offset);
+
+	return 0;
+}
+EXPORT_SYMBOL_NS(mtk_apu_mbox_read, MTK_APU_MAILBOX);
+
+static int mtk_apu_mailbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mtk_apu_mailbox *mbox;
+	int irq = -1, ret = 0;
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+
+	mbox->dev = dev;
+	platform_set_drvdata(pdev, mbox);
+
+	mbox->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(mbox->regs))
+		return PTR_ERR(mbox->regs);
+
+	mbox->controller.txdone_irq = false;
+	mbox->controller.txdone_poll = true;
+	mbox->controller.txpoll_period = 1;
+	mbox->controller.ops = &mtk_apu_mailbox_ops;
+	mbox->controller.dev = dev;
+	/*
+	 * Here we only register 1 mbox channel.
+	 * The remaining channels are used by other modules.
+	 */
+	mbox->controller.num_chans = 1;
+	mbox->controller.chans = devm_kcalloc(dev, mbox->controller.num_chans,
+					      sizeof(*mbox->controller.chans),
+					      GFP_KERNEL);
+	if (!mbox->controller.chans)
+		return -ENOMEM;
+
+	ret = devm_mbox_controller_register(dev, &mbox->controller);
+	if (ret)
+		return ret;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_threaded_irq(dev, irq, mtk_apu_mailbox_irq_top_half,
+					mtk_apu_mailbox_irq_btm_half, IRQF_ONESHOT,
+					dev_name(dev), mbox);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to request IRQ\n");
+
+	g_mbox = mbox;
+
+	dev_dbg(dev, "registered mtk apu mailbox\n");
+
+	return 0;
+}
+
+static void mtk_apu_mailbox_remove(struct platform_device *pdev)
+{
+	g_mbox = NULL;
+}
+
+static const struct of_device_id mtk_apu_mailbox_of_match[] = {
+	{ .compatible = "mediatek,mt8188-apu-mailbox" },
+	{ .compatible = "mediatek,mt8196-apu-mailbox" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mtk_apu_mailbox_of_match);
+
+static struct platform_driver mtk_apu_mailbox_driver = {
+	.probe = mtk_apu_mailbox_probe,
+	.remove = mtk_apu_mailbox_remove,
+	.driver = {
+		.name = "mtk-apu-mailbox",
+		.of_match_table = mtk_apu_mailbox_of_match,
+	},
+};
+
+module_platform_driver(mtk_apu_mailbox_driver);
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("MediaTek APU Mailbox Driver");
diff --git a/include/linux/mailbox/mtk-apu-mailbox.h b/include/linux/mailbox/mtk-apu-mailbox.h
new file mode 100644
index 000000000000..d1457d16ce9b
--- /dev/null
+++ b/include/linux/mailbox/mtk-apu-mailbox.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2024 MediaTek Inc.
+ *
+ */
+
+#ifndef __MTK_APU_MAILBOX_H__
+#define __MTK_APU_MAILBOX_H__
+
+#define MSG_MBOX_SLOTS	(8)
+
+struct mtk_apu_mailbox_msg {
+	int send_cnt;
+	u32 data[MSG_MBOX_SLOTS];
+};
+
+int mtk_apu_mbox_write(u32 val, u32 offset);
+int mtk_apu_mbox_read(u32 offset, u32 *val);
+
+#endif /* __MTK_APU_MAILBOX_H__ */
-- 
2.18.0

Re: [PATCH 3/3] mailbox: mediatek: Add mtk-apu-mailbox driver

Posted by kernel test robot 4 weeks, 1 day ago

Hi Karl.Li,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.12-rc4 next-20241025]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Karl-Li/dt-bindings-mailbox-mediatek-Add-apu-mailbox-document/20241024-173032
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20241024092608.431581-4-karl.li%40mediatek.com
patch subject: [PATCH 3/3] mailbox: mediatek: Add mtk-apu-mailbox driver
config: nios2-randconfig-r111-20241027 (https://download.01.org/0day-ci/archive/20241027/202410271207.42FIkHwC-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 14.1.0
reproduce: (https://download.01.org/0day-ci/archive/20241027/202410271207.42FIkHwC-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410271207.42FIkHwC-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/mailbox/mtk-apu-mailbox.c:29:24: sparse: sparse: symbol 'g_mbox' was not declared. Should it be static?

vim +/g_mbox +29 drivers/mailbox/mtk-apu-mailbox.c

    28	
  > 29	struct mtk_apu_mailbox *g_mbox;
    30	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/3] mailbox: mediatek: Add mtk-apu-mailbox driver

Posted by AngeloGioacchino Del Regno 1 month ago

Il 24/10/24 11:25, Karl.Li ha scritto:
> From: Karl Li <karl.li@mediatek.com>
> 
> Add mtk-apu-mailbox driver to support the communication with
> APU remote microprocessor.
> 
> Also, the mailbox hardware contains extra spare (scratch) registers
> that other hardware blocks use to communicate through.
> Expose these with custom mtk_apu_mbox_(read|write)() functions.
> 
> Signed-off-by: Karl Li <karl.li@mediatek.com>
> ---
>   drivers/mailbox/Kconfig                 |   9 +
>   drivers/mailbox/Makefile                |   2 +
>   drivers/mailbox/mtk-apu-mailbox.c       | 222 ++++++++++++++++++++++++
>   include/linux/mailbox/mtk-apu-mailbox.h |  20 +++
>   4 files changed, 253 insertions(+)
>   create mode 100644 drivers/mailbox/mtk-apu-mailbox.c
>   create mode 100644 include/linux/mailbox/mtk-apu-mailbox.h
> 
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 6fb995778636..2338e08a110a 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -240,6 +240,15 @@ config MTK_ADSP_MBOX
>             between processors with ADSP. It will place the message to share
>   	  buffer and will access the ipc control.
>   
> +config MTK_APU_MBOX
> +	tristate "MediaTek APU Mailbox Support"
> +	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	help
> +	  Say yes here to add support for the MediaTek APU Mailbox
> +	  driver. The mailbox implementation provides access from the
> +	  application processor to the MediaTek AI Processing Unit.
> +	  If unsure say N.
> +
>   config MTK_CMDQ_MBOX
>   	tristate "MediaTek CMDQ Mailbox Support"
>   	depends on ARCH_MEDIATEK || COMPILE_TEST
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index 3c3c27d54c13..6b6dcc78d644 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -53,6 +53,8 @@ obj-$(CONFIG_STM32_IPCC) 	+= stm32-ipcc.o
>   
>   obj-$(CONFIG_MTK_ADSP_MBOX)	+= mtk-adsp-mailbox.o
>   
> +obj-$(CONFIG_MTK_APU_MBOX)	+= mtk-apu-mailbox.o
> +
>   obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o
>   
>   obj-$(CONFIG_ZYNQMP_IPI_MBOX)	+= zynqmp-ipi-mailbox.o
> diff --git a/drivers/mailbox/mtk-apu-mailbox.c b/drivers/mailbox/mtk-apu-mailbox.c
> new file mode 100644
> index 000000000000..b347ebd34ef7
> --- /dev/null
> +++ b/drivers/mailbox/mtk-apu-mailbox.c
> @@ -0,0 +1,222 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2024 MediaTek Inc.
> + */
> +
> +#include <asm/io.h>
> +#include <linux/bits.h>
> +#include <linux/interrupt.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/mailbox/mtk-apu-mailbox.h>
> +#include <linux/platform_device.h>
> +
> +#define INBOX		(0x0)
> +#define OUTBOX		(0x20)
> +#define INBOX_IRQ	(0xc0)
> +#define OUTBOX_IRQ	(0xc4)
> +#define INBOX_IRQ_MASK	(0xd0)
> +
> +#define SPARE_OFF_START	(0x40)
> +#define SPARE_OFF_END	(0xB0)
> +
> +struct mtk_apu_mailbox {
> +	struct device *dev;
> +	void __iomem *regs;
> +	struct mbox_controller controller;

struct mbox_controller mbox;

...it's shorter and consistent with at least other MTK mailbox drivers.

> +	u32 msgs[MSG_MBOX_SLOTS];

Just reuse struct mtk_apu_mailbox_msg instead.....

> +};
> +
> +struct mtk_apu_mailbox *g_mbox;

That global struct must disappear - and if you use the mailbox API correctly
it's even simple.

Also, you want something like....

static inline struct mtk_apu_mailbox *get_mtk_apu_mailbox(struct mbox_controller *mbox)
{
	return container_of(mbox, struct mtk_apu_mailbox, mbox);
}

> +
> +static irqreturn_t mtk_apu_mailbox_irq_top_half(int irq, void *dev_id)
> +{
static irqreturn_t mtk_apu_mailbox_irq(int irq, void *data)
{
	struct mbox_chan *chan = data;
	struct mtk_apu_mailbox = get_mtk_apu_mailbox(chan->mbox);

> +	struct mtk_apu_mailbox *mbox = dev_id;
> +	struct mbox_chan *link = &mbox->controller.chans[0];
> +	int i;
> +
> +	for (i = 0; i < MSG_MBOX_SLOTS; i++)
> +		mbox->msgs[i] = readl(mbox->regs + OUTBOX + i * sizeof(u32));
> +
> +	mbox_chan_received_data(link, &mbox->msgs);
> +
> +	return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t mtk_apu_mailbox_irq_btm_half(int irq, void *dev_id)

....mtk_apu_mailbox_irq_thread(...)

> +{
> +	struct mtk_apu_mailbox *mbox = dev_id;
> +	struct mbox_chan *link = &mbox->controller.chans[0];
> +
> +	mbox_chan_received_data_bh(link, &mbox->msgs);

I don't think that you really need this _bh variant, looks more like you wanted
to have two callbacks instead of one.

You can instead have one callback and vary functionality based based on reading
a variable to decide what to actually do inside. Not a big deal.

> +	writel(readl(mbox->regs + OUTBOX_IRQ), mbox->regs + OUTBOX_IRQ);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mtk_apu_mailbox_send_data(struct mbox_chan *chan, void *data)
> +{
> +	struct mtk_apu_mailbox *mbox = container_of(chan->mbox,
> +						    struct mtk_apu_mailbox,
> +						    controller);
> +	struct mtk_apu_mailbox_msg *msg = data;
> +	int i;
> +
> +	if (msg->send_cnt <= 0 || msg->send_cnt > MSG_MBOX_SLOTS) {
> +		dev_err(mbox->dev, "%s: invalid send_cnt %d\n", __func__, msg->send_cnt);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 *	Mask lowest "send_cnt-1" interrupts bits, so the interrupt on the other side
> +	 *	triggers only after the last data slot is written (sent).
> +	 */
> +	writel(GENMASK(msg->send_cnt - 2, 0), mbox->regs + INBOX_IRQ_MASK);
> +	for (i = 0; i < msg->send_cnt; i++)
> +		writel(msg->data[i], mbox->regs + INBOX + i * sizeof(u32));
> +
> +	return 0;
> +}
> +
> +static bool mtk_apu_mailbox_last_tx_done(struct mbox_chan *chan)
> +{
> +	struct mtk_apu_mailbox *mbox = container_of(chan->mbox,
> +						    struct mtk_apu_mailbox,
> +						    controller);
> +
> +	return readl(mbox->regs + INBOX_IRQ) == 0;
> +}
> +
> +static const struct mbox_chan_ops mtk_apu_mailbox_ops = {
> +	.send_data = mtk_apu_mailbox_send_data,
> +	.last_tx_done = mtk_apu_mailbox_last_tx_done,
> +};
> +
> +/**
> + * mtk_apu_mbox_write - Write value to specifice mtk_apu_mbox spare register.
> + * @val: Value to be written.
> + * @offset: Offset of the spare register.
> + *
> + * Return: 0 if successful
> + *	   negative value if error happened
> + */
> +int mtk_apu_mbox_write(u32 val, u32 offset)
> +{
> +	if (!g_mbox) {
> +		pr_err("mtk apu mbox was not initialized, stop writing register\n");
> +		return -ENODEV;
> +	}
> +
> +	if (offset < SPARE_OFF_START || offset >= SPARE_OFF_END) {
> +		dev_err(g_mbox->dev, "Invalid offset %d for mtk apu mbox spare register\n", offset);
> +		return -EINVAL;
> +	}
> +
> +	writel(val, g_mbox->regs + offset);

There's something odd in what you're doing here, why would you ever need
a function that performs a writel just like that? What's the purpose?

What are you writing to the spare registers?
For which reason?

I think you can avoid (read this as: you *have to* avoid) having such a
function around.

> +	return 0;
> +}
> +EXPORT_SYMBOL_NS(mtk_apu_mbox_write, MTK_APU_MAILBOX);
> +
> +/**
> + * mtk_apu_mbox_read - Read value to specifice mtk_apu_mbox spare register.
> + * @offset: Offset of the spare register.
> + * @val: Pointer to store read value.
> + *
> + * Return: 0 if successful
> + *	   negative value if error happened
> + */
> +int mtk_apu_mbox_read(u32 offset, u32 *val)
> +{
> +	if (!g_mbox) {
> +		pr_err("mtk apu mbox was not initialized, stop reading register\n");
> +		return -ENODEV;
> +	}
> +
> +	if (offset < SPARE_OFF_START || offset >= SPARE_OFF_END) {
> +		dev_err(g_mbox->dev, "Invalid offset %d for mtk apu mbox spare register\n", offset);
> +		return -EINVAL;
> +	}
> +
> +	*val = readl(g_mbox->regs + offset);
> +

Same goes for this one.

> +	return 0;
> +}
> +EXPORT_SYMBOL_NS(mtk_apu_mbox_read, MTK_APU_MAILBOX);
> +
> +static int mtk_apu_mailbox_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mtk_apu_mailbox *mbox;
> +	int irq = -1, ret = 0;
> +
> +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> +	if (!mbox)
> +		return -ENOMEM;
> +
> +	mbox->dev = dev;
> +	platform_set_drvdata(pdev, mbox);
> +

Please move the platform_get_irq call here or anyway before registering the
mbox controller: in case anything goes wrong, devm won't have to unregister
the mbox afterwards because it never got registered in the first place.

> +	mbox->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(mbox->regs))
> +		return PTR_ERR(mbox->regs);
> +
> +	mbox->controller.txdone_irq = false;
> +	mbox->controller.txdone_poll = true;
> +	mbox->controller.txpoll_period = 1;
> +	mbox->controller.ops = &mtk_apu_mailbox_ops;
> +	mbox->controller.dev = dev;
> +	/*
> +	 * Here we only register 1 mbox channel.
> +	 * The remaining channels are used by other modules.

What other modules? I don't really see any - so please at least explain that in the
commit description.

> +	 */
> +	mbox->controller.num_chans = 1;
> +	mbox->controller.chans = devm_kcalloc(dev, mbox->controller.num_chans,
> +					      sizeof(*mbox->controller.chans),
> +					      GFP_KERNEL);
> +	if (!mbox->controller.chans)
> +		return -ENOMEM;
> +
> +	ret = devm_mbox_controller_register(dev, &mbox->controller);
> +	if (ret)
> +		return ret;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = devm_request_threaded_irq(dev, irq, mtk_apu_mailbox_irq_top_half,
> +					mtk_apu_mailbox_irq_btm_half, IRQF_ONESHOT,
> +					dev_name(dev), mbox);

pass mbox->chans to the isr

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to request IRQ\n");
> +
> +	g_mbox = mbox;
> +
> +	dev_dbg(dev, "registered mtk apu mailbox\n");
> +
> +	return 0;
> +}
> +
> +static void mtk_apu_mailbox_remove(struct platform_device *pdev)
> +{
> +	g_mbox = NULL;
> +}
> +
> +static const struct of_device_id mtk_apu_mailbox_of_match[] = {
> +	{ .compatible = "mediatek,mt8188-apu-mailbox" },
> +	{ .compatible = "mediatek,mt8196-apu-mailbox" },

Just mediatek,mt8188-apu-mailbox is fine; you can allow mt8196==mt8188 in the
binding instead.

> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mtk_apu_mailbox_of_match);
> +
> +static struct platform_driver mtk_apu_mailbox_driver = {
> +	.probe = mtk_apu_mailbox_probe,
> +	.remove = mtk_apu_mailbox_remove,

You don't need this remove callback, since g_mbox has to disappear :-)

> +	.driver = {
> +		.name = "mtk-apu-mailbox",
> +		.of_match_table = mtk_apu_mailbox_of_match,
> +	},
> +};
> +
> +module_platform_driver(mtk_apu_mailbox_driver);
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("MediaTek APU Mailbox Driver");
> diff --git a/include/linux/mailbox/mtk-apu-mailbox.h b/include/linux/mailbox/mtk-apu-mailbox.h
> new file mode 100644
> index 000000000000..d1457d16ce9b
> --- /dev/null
> +++ b/include/linux/mailbox/mtk-apu-mailbox.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2024 MediaTek Inc.
> + *
> + */
> +
> +#ifndef __MTK_APU_MAILBOX_H__
> +#define __MTK_APU_MAILBOX_H__
> +
> +#define MSG_MBOX_SLOTS	(8)
> +
> +struct mtk_apu_mailbox_msg {
> +	int send_cnt;

u8 data_cnt;

> +	u32 data[MSG_MBOX_SLOTS];

With hardcoded slots, what happens when we get a new chip in the future that
supports more slots?

Please think about this now and make the implementation flexible before that
happens because, at a later time, it'll be harder.

Regards,
Angelo

> +};
> +
> +int mtk_apu_mbox_write(u32 val, u32 offset);
> +int mtk_apu_mbox_read(u32 offset, u32 *val);
> +
> +#endif /* __MTK_APU_MAILBOX_H__ */

Re: [PATCH 3/3] mailbox: mediatek: Add mtk-apu-mailbox driver

Posted by Chen-Yu Tsai 4 weeks ago

On Thu, Oct 24, 2024 at 7:13 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 24/10/24 11:25, Karl.Li ha scritto:
> > From: Karl Li <karl.li@mediatek.com>
> >
> > Add mtk-apu-mailbox driver to support the communication with
> > APU remote microprocessor.
> >
> > Also, the mailbox hardware contains extra spare (scratch) registers
> > that other hardware blocks use to communicate through.
> > Expose these with custom mtk_apu_mbox_(read|write)() functions.
> >
> > Signed-off-by: Karl Li <karl.li@mediatek.com>
> > ---
> >   drivers/mailbox/Kconfig                 |   9 +
> >   drivers/mailbox/Makefile                |   2 +
> >   drivers/mailbox/mtk-apu-mailbox.c       | 222 ++++++++++++++++++++++++
> >   include/linux/mailbox/mtk-apu-mailbox.h |  20 +++
> >   4 files changed, 253 insertions(+)
> >   create mode 100644 drivers/mailbox/mtk-apu-mailbox.c
> >   create mode 100644 include/linux/mailbox/mtk-apu-mailbox.h
> >
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > index 6fb995778636..2338e08a110a 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -240,6 +240,15 @@ config MTK_ADSP_MBOX
> >             between processors with ADSP. It will place the message to share
> >         buffer and will access the ipc control.
> >
> > +config MTK_APU_MBOX
> > +     tristate "MediaTek APU Mailbox Support"
> > +     depends on ARCH_MEDIATEK || COMPILE_TEST
> > +     help
> > +       Say yes here to add support for the MediaTek APU Mailbox
> > +       driver. The mailbox implementation provides access from the
> > +       application processor to the MediaTek AI Processing Unit.
> > +       If unsure say N.
> > +
> >   config MTK_CMDQ_MBOX
> >       tristate "MediaTek CMDQ Mailbox Support"
> >       depends on ARCH_MEDIATEK || COMPILE_TEST
> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > index 3c3c27d54c13..6b6dcc78d644 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -53,6 +53,8 @@ obj-$(CONFIG_STM32_IPCC)    += stm32-ipcc.o
> >
> >   obj-$(CONFIG_MTK_ADSP_MBOX) += mtk-adsp-mailbox.o
> >
> > +obj-$(CONFIG_MTK_APU_MBOX)   += mtk-apu-mailbox.o
> > +
> >   obj-$(CONFIG_MTK_CMDQ_MBOX) += mtk-cmdq-mailbox.o
> >
> >   obj-$(CONFIG_ZYNQMP_IPI_MBOX)       += zynqmp-ipi-mailbox.o
> > diff --git a/drivers/mailbox/mtk-apu-mailbox.c b/drivers/mailbox/mtk-apu-mailbox.c
> > new file mode 100644
> > index 000000000000..b347ebd34ef7
> > --- /dev/null
> > +++ b/drivers/mailbox/mtk-apu-mailbox.c
> > @@ -0,0 +1,222 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2024 MediaTek Inc.
> > + */
> > +
> > +#include <asm/io.h>
> > +#include <linux/bits.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mailbox_controller.h>
> > +#include <linux/mailbox/mtk-apu-mailbox.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define INBOX                (0x0)
> > +#define OUTBOX               (0x20)
> > +#define INBOX_IRQ    (0xc0)
> > +#define OUTBOX_IRQ   (0xc4)
> > +#define INBOX_IRQ_MASK       (0xd0)
> > +
> > +#define SPARE_OFF_START      (0x40)
> > +#define SPARE_OFF_END        (0xB0)
> > +
> > +struct mtk_apu_mailbox {
> > +     struct device *dev;
> > +     void __iomem *regs;
> > +     struct mbox_controller controller;
>
> struct mbox_controller mbox;
>
> ...it's shorter and consistent with at least other MTK mailbox drivers.
>
> > +     u32 msgs[MSG_MBOX_SLOTS];
>
> Just reuse struct mtk_apu_mailbox_msg instead.....
>
> > +};
> > +
> > +struct mtk_apu_mailbox *g_mbox;
>
> That global struct must disappear - and if you use the mailbox API correctly
> it's even simple.
>
> Also, you want something like....
>
> static inline struct mtk_apu_mailbox *get_mtk_apu_mailbox(struct mbox_controller *mbox)
> {
>         return container_of(mbox, struct mtk_apu_mailbox, mbox);
> }
>
> > +
> > +static irqreturn_t mtk_apu_mailbox_irq_top_half(int irq, void *dev_id)
> > +{
> static irqreturn_t mtk_apu_mailbox_irq(int irq, void *data)
> {
>         struct mbox_chan *chan = data;
>         struct mtk_apu_mailbox = get_mtk_apu_mailbox(chan->mbox);
>
> > +     struct mtk_apu_mailbox *mbox = dev_id;
> > +     struct mbox_chan *link = &mbox->controller.chans[0];
> > +     int i;
> > +
> > +     for (i = 0; i < MSG_MBOX_SLOTS; i++)
> > +             mbox->msgs[i] = readl(mbox->regs + OUTBOX + i * sizeof(u32));
> > +
> > +     mbox_chan_received_data(link, &mbox->msgs);
> > +
> > +     return IRQ_WAKE_THREAD;
> > +}
> > +
> > +static irqreturn_t mtk_apu_mailbox_irq_btm_half(int irq, void *dev_id)
>
> ....mtk_apu_mailbox_irq_thread(...)
>
> > +{
> > +     struct mtk_apu_mailbox *mbox = dev_id;
> > +     struct mbox_chan *link = &mbox->controller.chans[0];
> > +
> > +     mbox_chan_received_data_bh(link, &mbox->msgs);
>
> I don't think that you really need this _bh variant, looks more like you wanted
> to have two callbacks instead of one.
>
> You can instead have one callback and vary functionality based based on reading
> a variable to decide what to actually do inside. Not a big deal.

The problem is that they need something with different semantics.
mbox_chan_received_data() is atomic only.

> > +     writel(readl(mbox->regs + OUTBOX_IRQ), mbox->regs + OUTBOX_IRQ);
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static int mtk_apu_mailbox_send_data(struct mbox_chan *chan, void *data)
> > +{
> > +     struct mtk_apu_mailbox *mbox = container_of(chan->mbox,
> > +                                                 struct mtk_apu_mailbox,
> > +                                                 controller);
> > +     struct mtk_apu_mailbox_msg *msg = data;
> > +     int i;
> > +
> > +     if (msg->send_cnt <= 0 || msg->send_cnt > MSG_MBOX_SLOTS) {
> > +             dev_err(mbox->dev, "%s: invalid send_cnt %d\n", __func__, msg->send_cnt);
> > +             return -EINVAL;
> > +     }
> > +
> > +     /*
> > +      *      Mask lowest "send_cnt-1" interrupts bits, so the interrupt on the other side
> > +      *      triggers only after the last data slot is written (sent).
> > +      */
> > +     writel(GENMASK(msg->send_cnt - 2, 0), mbox->regs + INBOX_IRQ_MASK);
> > +     for (i = 0; i < msg->send_cnt; i++)
> > +             writel(msg->data[i], mbox->regs + INBOX + i * sizeof(u32));
> > +
> > +     return 0;
> > +}
> > +
> > +static bool mtk_apu_mailbox_last_tx_done(struct mbox_chan *chan)
> > +{
> > +     struct mtk_apu_mailbox *mbox = container_of(chan->mbox,
> > +                                                 struct mtk_apu_mailbox,
> > +                                                 controller);
> > +
> > +     return readl(mbox->regs + INBOX_IRQ) == 0;
> > +}
> > +
> > +static const struct mbox_chan_ops mtk_apu_mailbox_ops = {
> > +     .send_data = mtk_apu_mailbox_send_data,
> > +     .last_tx_done = mtk_apu_mailbox_last_tx_done,
> > +};
> > +
> > +/**
> > + * mtk_apu_mbox_write - Write value to specifice mtk_apu_mbox spare register.
> > + * @val: Value to be written.
> > + * @offset: Offset of the spare register.
> > + *
> > + * Return: 0 if successful
> > + *      negative value if error happened
> > + */
> > +int mtk_apu_mbox_write(u32 val, u32 offset)
> > +{
> > +     if (!g_mbox) {
> > +             pr_err("mtk apu mbox was not initialized, stop writing register\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     if (offset < SPARE_OFF_START || offset >= SPARE_OFF_END) {
> > +             dev_err(g_mbox->dev, "Invalid offset %d for mtk apu mbox spare register\n", offset);
> > +             return -EINVAL;
> > +     }
> > +
> > +     writel(val, g_mbox->regs + offset);
>
> There's something odd in what you're doing here, why would you ever need
> a function that performs a writel just like that? What's the purpose?
>
> What are you writing to the spare registers?
> For which reason?

I'll leave the explaining to Karl, but based on internal reviews for the
previous generation, it looked like passing values to/from the MCU.

> I think you can avoid (read this as: you *have to* avoid) having such a
> function around.

Again, during the previous round of internal reviews, I had thought
about modeling these as extra mbox channels. I may have even asked
about this on IRC.

The problem is that it doesn't really have mbox semantics. They are
just shared registers with no send/receive notification. So at the
very least, there's nothing that will trigger a reception. I suppose
we could make the .peek_data op trigger RX, but that's a really
convoluted way to read just a register.

The other option would be to have a syscon / custom exported regmap?

> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_NS(mtk_apu_mbox_write, MTK_APU_MAILBOX);
> > +
> > +/**
> > + * mtk_apu_mbox_read - Read value to specifice mtk_apu_mbox spare register.
> > + * @offset: Offset of the spare register.
> > + * @val: Pointer to store read value.
> > + *
> > + * Return: 0 if successful
> > + *      negative value if error happened
> > + */
> > +int mtk_apu_mbox_read(u32 offset, u32 *val)
> > +{
> > +     if (!g_mbox) {
> > +             pr_err("mtk apu mbox was not initialized, stop reading register\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     if (offset < SPARE_OFF_START || offset >= SPARE_OFF_END) {
> > +             dev_err(g_mbox->dev, "Invalid offset %d for mtk apu mbox spare register\n", offset);
> > +             return -EINVAL;
> > +     }
> > +
> > +     *val = readl(g_mbox->regs + offset);
> > +
>
> Same goes for this one.
>
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_NS(mtk_apu_mbox_read, MTK_APU_MAILBOX);
> > +
> > +static int mtk_apu_mailbox_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct mtk_apu_mailbox *mbox;
> > +     int irq = -1, ret = 0;
> > +
> > +     mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> > +     if (!mbox)
> > +             return -ENOMEM;
> > +
> > +     mbox->dev = dev;
> > +     platform_set_drvdata(pdev, mbox);
> > +
>
> Please move the platform_get_irq call here or anyway before registering the
> mbox controller: in case anything goes wrong, devm won't have to unregister
> the mbox afterwards because it never got registered in the first place.

To clarify, you mean _just_ platform_get_irq() and not request_irq as
well.

> > +     mbox->regs = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(mbox->regs))
> > +             return PTR_ERR(mbox->regs);
> > +
> > +     mbox->controller.txdone_irq = false;
> > +     mbox->controller.txdone_poll = true;
> > +     mbox->controller.txpoll_period = 1;
> > +     mbox->controller.ops = &mtk_apu_mailbox_ops;
> > +     mbox->controller.dev = dev;
> > +     /*
> > +      * Here we only register 1 mbox channel.
> > +      * The remaining channels are used by other modules.
>
> What other modules? I don't really see any - so please at least explain that in the
> commit description.
>
> > +      */
> > +     mbox->controller.num_chans = 1;
> > +     mbox->controller.chans = devm_kcalloc(dev, mbox->controller.num_chans,
> > +                                           sizeof(*mbox->controller.chans),
> > +                                           GFP_KERNEL);
> > +     if (!mbox->controller.chans)
> > +             return -ENOMEM;
> > +
> > +     ret = devm_mbox_controller_register(dev, &mbox->controller);
> > +     if (ret)
> > +             return ret;
> > +
> > +     irq = platform_get_irq(pdev, 0);
> > +     if (irq < 0)
> > +             return irq;
> > +
> > +     ret = devm_request_threaded_irq(dev, irq, mtk_apu_mailbox_irq_top_half,
> > +                                     mtk_apu_mailbox_irq_btm_half, IRQF_ONESHOT,
> > +                                     dev_name(dev), mbox);
>
> pass mbox->chans to the isr
>
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "Failed to request IRQ\n");
> > +
> > +     g_mbox = mbox;
> > +
> > +     dev_dbg(dev, "registered mtk apu mailbox\n");
> > +
> > +     return 0;
> > +}
> > +
> > +static void mtk_apu_mailbox_remove(struct platform_device *pdev)
> > +{
> > +     g_mbox = NULL;
> > +}
> > +
> > +static const struct of_device_id mtk_apu_mailbox_of_match[] = {
> > +     { .compatible = "mediatek,mt8188-apu-mailbox" },
> > +     { .compatible = "mediatek,mt8196-apu-mailbox" },
>
> Just mediatek,mt8188-apu-mailbox is fine; you can allow mt8196==mt8188 in the
> binding instead.
>
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_apu_mailbox_of_match);
> > +
> > +static struct platform_driver mtk_apu_mailbox_driver = {
> > +     .probe = mtk_apu_mailbox_probe,
> > +     .remove = mtk_apu_mailbox_remove,
>
> You don't need this remove callback, since g_mbox has to disappear :-)
>
> > +     .driver = {
> > +             .name = "mtk-apu-mailbox",
> > +             .of_match_table = mtk_apu_mailbox_of_match,
> > +     },
> > +};
> > +
> > +module_platform_driver(mtk_apu_mailbox_driver);
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("MediaTek APU Mailbox Driver");
> > diff --git a/include/linux/mailbox/mtk-apu-mailbox.h b/include/linux/mailbox/mtk-apu-mailbox.h
> > new file mode 100644
> > index 000000000000..d1457d16ce9b
> > --- /dev/null
> > +++ b/include/linux/mailbox/mtk-apu-mailbox.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2024 MediaTek Inc.
> > + *
> > + */
> > +
> > +#ifndef __MTK_APU_MAILBOX_H__
> > +#define __MTK_APU_MAILBOX_H__
> > +
> > +#define MSG_MBOX_SLOTS       (8)
> > +
> > +struct mtk_apu_mailbox_msg {
> > +     int send_cnt;
>
> u8 data_cnt;
>
> > +     u32 data[MSG_MBOX_SLOTS];
>
> With hardcoded slots, what happens when we get a new chip in the future that
> supports more slots?

Seems like we can make it a flexible array member? But the problem then
becomes how does the client know what the maximum length is. Or maybe
it should already know given it's tied to a particular platform.

In any case it becomes:

    struct mtk_apu_mailbox_msg {
        u8 data_size;
        u8 data[] __counted_by(data_size);
    };

This can't be allocated on the stack if `data_size` isn't a compile
time constant though; but again it shouldn't be a problem given the
message size is tied to the client & its platform and should be
constant anyway.

The controller should just error out if the message is larger than
what it can atomically send.


ChenYu

> Please think about this now and make the implementation flexible before that
> happens because, at a later time, it'll be harder.
>
> Regards,
> Angelo
>
> > +};
> > +
> > +int mtk_apu_mbox_write(u32 val, u32 offset);
> > +int mtk_apu_mbox_read(u32 offset, u32 *val);
> > +
> > +#endif /* __MTK_APU_MAILBOX_H__ */
>
>

Re: [PATCH 3/3] mailbox: mediatek: Add mtk-apu-mailbox driver

Posted by Karl Li (李智嘉) 3 weeks, 6 days ago

On Mon, 2024-10-28 at 14:16 +0800, Chen-Yu Tsai wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Thu, Oct 24, 2024 at 7:13 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
> > 
> > Il 24/10/24 11:25, Karl.Li ha scritto:
> > > From: Karl Li <karl.li@mediatek.com>
> > > 
> > > Add mtk-apu-mailbox driver to support the communication with
> > > APU remote microprocessor.
> > > 
> > > Also, the mailbox hardware contains extra spare (scratch)
> > > registers
> > > that other hardware blocks use to communicate through.
> > > Expose these with custom mtk_apu_mbox_(read|write)() functions.
> > > 
> > > Signed-off-by: Karl Li <karl.li@mediatek.com>
> > > ---
> > >   drivers/mailbox/Kconfig                 |   9 +
> > >   drivers/mailbox/Makefile                |   2 +
> > >   drivers/mailbox/mtk-apu-mailbox.c       | 222
> > > ++++++++++++++++++++++++
> > >   include/linux/mailbox/mtk-apu-mailbox.h |  20 +++
> > >   4 files changed, 253 insertions(+)
> > >   create mode 100644 drivers/mailbox/mtk-apu-mailbox.c
> > >   create mode 100644 include/linux/mailbox/mtk-apu-mailbox.h
> > > 
> > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > > index 6fb995778636..2338e08a110a 100644
> > > --- a/drivers/mailbox/Kconfig
> > > +++ b/drivers/mailbox/Kconfig
> > > @@ -240,6 +240,15 @@ config MTK_ADSP_MBOX
> > >             between processors with ADSP. It will place the
> > > message to share
> > >         buffer and will access the ipc control.
> > > 
> > > +config MTK_APU_MBOX
> > > +     tristate "MediaTek APU Mailbox Support"
> > > +     depends on ARCH_MEDIATEK || COMPILE_TEST
> > > +     help
> > > +       Say yes here to add support for the MediaTek APU Mailbox
> > > +       driver. The mailbox implementation provides access from
> > > the
> > > +       application processor to the MediaTek AI Processing Unit.
> > > +       If unsure say N.
> > > +
> > >   config MTK_CMDQ_MBOX
> > >       tristate "MediaTek CMDQ Mailbox Support"
> > >       depends on ARCH_MEDIATEK || COMPILE_TEST
> > > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > > index 3c3c27d54c13..6b6dcc78d644 100644
> > > --- a/drivers/mailbox/Makefile
> > > +++ b/drivers/mailbox/Makefile
> > > @@ -53,6 +53,8 @@ obj-$(CONFIG_STM32_IPCC)    += stm32-ipcc.o
> > > 
> > >   obj-$(CONFIG_MTK_ADSP_MBOX) += mtk-adsp-mailbox.o
> > > 
> > > +obj-$(CONFIG_MTK_APU_MBOX)   += mtk-apu-mailbox.o
> > > +
> > >   obj-$(CONFIG_MTK_CMDQ_MBOX) += mtk-cmdq-mailbox.o
> > > 
> > >   obj-$(CONFIG_ZYNQMP_IPI_MBOX)       += zynqmp-ipi-mailbox.o
> > > diff --git a/drivers/mailbox/mtk-apu-mailbox.c
> > > b/drivers/mailbox/mtk-apu-mailbox.c
> > > new file mode 100644
> > > index 000000000000..b347ebd34ef7
> > > --- /dev/null
> > > +++ b/drivers/mailbox/mtk-apu-mailbox.c
> > > @@ -0,0 +1,222 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2024 MediaTek Inc.
> > > + */
> > > +
> > > +#include <asm/io.h>
> > > +#include <linux/bits.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/mailbox_controller.h>
> > > +#include <linux/mailbox/mtk-apu-mailbox.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#define INBOX                (0x0)
> > > +#define OUTBOX               (0x20)
> > > +#define INBOX_IRQ    (0xc0)
> > > +#define OUTBOX_IRQ   (0xc4)
> > > +#define INBOX_IRQ_MASK       (0xd0)
> > > +
> > > +#define SPARE_OFF_START      (0x40)
> > > +#define SPARE_OFF_END        (0xB0)
> > > +
> > > +struct mtk_apu_mailbox {
> > > +     struct device *dev;
> > > +     void __iomem *regs;
> > > +     struct mbox_controller controller;
> > 
> > struct mbox_controller mbox;
> > 
> > ...it's shorter and consistent with at least other MTK mailbox
> > drivers.
> > 
> > > +     u32 msgs[MSG_MBOX_SLOTS];
> > 
> > Just reuse struct mtk_apu_mailbox_msg instead.....
> > 
> > > +};
> > > +
> > > +struct mtk_apu_mailbox *g_mbox;
> > 
> > That global struct must disappear - and if you use the mailbox API
> > correctly
> > it's even simple.
> > 
> > Also, you want something like....
> > 
> > static inline struct mtk_apu_mailbox *get_mtk_apu_mailbox(struct
> > mbox_controller *mbox)
> > {
> >         return container_of(mbox, struct mtk_apu_mailbox, mbox);
> > }
> > 
> > > +
> > > +static irqreturn_t mtk_apu_mailbox_irq_top_half(int irq, void
> > > *dev_id)
> > > +{
> > static irqreturn_t mtk_apu_mailbox_irq(int irq, void *data)
> > {
> >         struct mbox_chan *chan = data;
> >         struct mtk_apu_mailbox = get_mtk_apu_mailbox(chan->mbox);
> > 
> > > +     struct mtk_apu_mailbox *mbox = dev_id;
> > > +     struct mbox_chan *link = &mbox->controller.chans[0];
> > > +     int i;
> > > +
> > > +     for (i = 0; i < MSG_MBOX_SLOTS; i++)
> > > +             mbox->msgs[i] = readl(mbox->regs + OUTBOX + i *
> > > sizeof(u32));
> > > +
> > > +     mbox_chan_received_data(link, &mbox->msgs);
> > > +
> > > +     return IRQ_WAKE_THREAD;
> > > +}
> > > +
> > > +static irqreturn_t mtk_apu_mailbox_irq_btm_half(int irq, void
> > > *dev_id)
> > 
> > ....mtk_apu_mailbox_irq_thread(...)
> > 
> > > +{
> > > +     struct mtk_apu_mailbox *mbox = dev_id;
> > > +     struct mbox_chan *link = &mbox->controller.chans[0];
> > > +
> > > +     mbox_chan_received_data_bh(link, &mbox->msgs);
> > 
> > I don't think that you really need this _bh variant, looks more
> > like you wanted
> > to have two callbacks instead of one.
> > 
> > You can instead have one callback and vary functionality based
> > based on reading
> > a variable to decide what to actually do inside. Not a big deal.
> 
> The problem is that they need something with different semantics.
> mbox_chan_received_data() is atomic only.

Yes, as Chen-Yu said, we want to have another callback which can run
under non-atomic semantic.
Even though we change the function based in the callback function of
mbox_chan_received_data(), it is still non-atomic for the bottom-half
handler.

> 
> > > +     writel(readl(mbox->regs + OUTBOX_IRQ), mbox->regs +
> > > OUTBOX_IRQ);
> > > +
> > > +     return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int mtk_apu_mailbox_send_data(struct mbox_chan *chan,
> > > void *data)
> > > +{
> > > +     struct mtk_apu_mailbox *mbox = container_of(chan->mbox,
> > > +                                                 struct
> > > mtk_apu_mailbox,
> > > +                                                 controller);
> > > +     struct mtk_apu_mailbox_msg *msg = data;
> > > +     int i;
> > > +
> > > +     if (msg->send_cnt <= 0 || msg->send_cnt > MSG_MBOX_SLOTS) {
> > > +             dev_err(mbox->dev, "%s: invalid send_cnt %d\n",
> > > __func__, msg->send_cnt);
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     /*
> > > +      *      Mask lowest "send_cnt-1" interrupts bits, so the
> > > interrupt on the other side
> > > +      *      triggers only after the last data slot is written
> > > (sent).
> > > +      */
> > > +     writel(GENMASK(msg->send_cnt - 2, 0), mbox->regs +
> > > INBOX_IRQ_MASK);
> > > +     for (i = 0; i < msg->send_cnt; i++)
> > > +             writel(msg->data[i], mbox->regs + INBOX + i *
> > > sizeof(u32));
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static bool mtk_apu_mailbox_last_tx_done(struct mbox_chan *chan)
> > > +{
> > > +     struct mtk_apu_mailbox *mbox = container_of(chan->mbox,
> > > +                                                 struct
> > > mtk_apu_mailbox,
> > > +                                                 controller);
> > > +
> > > +     return readl(mbox->regs + INBOX_IRQ) == 0;
> > > +}
> > > +
> > > +static const struct mbox_chan_ops mtk_apu_mailbox_ops = {
> > > +     .send_data = mtk_apu_mailbox_send_data,
> > > +     .last_tx_done = mtk_apu_mailbox_last_tx_done,
> > > +};
> > > +
> > > +/**
> > > + * mtk_apu_mbox_write - Write value to specifice mtk_apu_mbox
> > > spare register.
> > > + * @val: Value to be written.
> > > + * @offset: Offset of the spare register.
> > > + *
> > > + * Return: 0 if successful
> > > + *      negative value if error happened
> > > + */
> > > +int mtk_apu_mbox_write(u32 val, u32 offset)
> > > +{
> > > +     if (!g_mbox) {
> > > +             pr_err("mtk apu mbox was not initialized, stop
> > > writing register\n");
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     if (offset < SPARE_OFF_START || offset >= SPARE_OFF_END) {
> > > +             dev_err(g_mbox->dev, "Invalid offset %d for mtk apu
> > > mbox spare register\n", offset);
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     writel(val, g_mbox->regs + offset);
> > 
> > There's something odd in what you're doing here, why would you ever
> > need
> > a function that performs a writel just like that? What's the
> > purpose?
> > 
> > What are you writing to the spare registers?
> > For which reason?
> 
> I'll leave the explaining to Karl, but based on internal reviews for
> the
> previous generation, it looked like passing values to/from the MCU.
> 

The main reason we want to access the APU mailbox spare registers is to
ensure that we can configure the necessary settings before the APU
firmware becomes fully operational.

At the early stage, the communication pathways between the APU and the
Linux Kernel aren't yet available, so these spare registers are needed
for passing the initial configuration data.

> > I think you can avoid (read this as: you *have to* avoid) having
> > such a
> > function around.
> 
> Again, during the previous round of internal reviews, I had thought
> about modeling these as extra mbox channels. I may have even asked
> about this on IRC.
> 
> The problem is that it doesn't really have mbox semantics. They are
> just shared registers with no send/receive notification. So at the
> very least, there's nothing that will trigger a reception. I suppose
> we could make the .peek_data op trigger RX, but that's a really
> convoluted way to read just a register.
> 
> The other option would be to have a syscon / custom exported regmap?
> 
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_NS(mtk_apu_mbox_write, MTK_APU_MAILBOX);
> > > +
> > > +/**
> > > + * mtk_apu_mbox_read - Read value to specifice mtk_apu_mbox
> > > spare register.
> > > + * @offset: Offset of the spare register.
> > > + * @val: Pointer to store read value.
> > > + *
> > > + * Return: 0 if successful
> > > + *      negative value if error happened
> > > + */
> > > +int mtk_apu_mbox_read(u32 offset, u32 *val)
> > > +{
> > > +     if (!g_mbox) {
> > > +             pr_err("mtk apu mbox was not initialized, stop
> > > reading register\n");
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     if (offset < SPARE_OFF_START || offset >= SPARE_OFF_END) {
> > > +             dev_err(g_mbox->dev, "Invalid offset %d for mtk apu
> > > mbox spare register\n", offset);
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     *val = readl(g_mbox->regs + offset);
> > > +
> > 
> > Same goes for this one.
> > 
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_NS(mtk_apu_mbox_read, MTK_APU_MAILBOX);
> > > +
> > > +static int mtk_apu_mailbox_probe(struct platform_device *pdev)
> > > +{
> > > +     struct device *dev = &pdev->dev;
> > > +     struct mtk_apu_mailbox *mbox;
> > > +     int irq = -1, ret = 0;
> > > +
> > > +     mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> > > +     if (!mbox)
> > > +             return -ENOMEM;
> > > +
> > > +     mbox->dev = dev;
> > > +     platform_set_drvdata(pdev, mbox);
> > > +
> > 
> > Please move the platform_get_irq call here or anyway before
> > registering the
> > mbox controller: in case anything goes wrong, devm won't have to
> > unregister
> > the mbox afterwards because it never got registered in the first
> > place.
> 
> To clarify, you mean _just_ platform_get_irq() and not request_irq as
> well.
> 
> > > +     mbox->regs = devm_platform_ioremap_resource(pdev, 0);
> > > +     if (IS_ERR(mbox->regs))
> > > +             return PTR_ERR(mbox->regs);
> > > +
> > > +     mbox->controller.txdone_irq = false;
> > > +     mbox->controller.txdone_poll = true;
> > > +     mbox->controller.txpoll_period = 1;
> > > +     mbox->controller.ops = &mtk_apu_mailbox_ops;
> > > +     mbox->controller.dev = dev;
> > > +     /*
> > > +      * Here we only register 1 mbox channel.
> > > +      * The remaining channels are used by other modules.
> > 
> > What other modules? I don't really see any - so please at least
> > explain that in the
> > commit description.

Sorry for any confusion caused by the above comment. To clarify, the
comment was specific to the MT8188 platform, which is the legacy
platform compared to MT8196.
In the context of MT8188, the APU mailbox has multiple in/out boxes,
and Linux only utilizes in/out box 0, while the others are reserved for
different VMs.

However, the APU mailbox hardware design in MT8196 differs from that of
MT8188, and in MT8196, Linux has full access to the APU mailbox.

Given that this patch is primarily for the MT8196 platform, we will
remove the above comment in the next version of the patch.

Thanks for your asking.

Karl

> > 
> > > +      */
> > > +     mbox->controller.num_chans = 1;
> > > +     mbox->controller.chans = devm_kcalloc(dev, mbox-
> > > >controller.num_chans,
> > > +                                           sizeof(*mbox-
> > > >controller.chans),
> > > +                                           GFP_KERNEL);
> > > +     if (!mbox->controller.chans)
> > > +             return -ENOMEM;
> > > +
> > > +     ret = devm_mbox_controller_register(dev, &mbox-
> > > >controller);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     irq = platform_get_irq(pdev, 0);
> > > +     if (irq < 0)
> > > +             return irq;
> > > +
> > > +     ret = devm_request_threaded_irq(dev, irq,
> > > mtk_apu_mailbox_irq_top_half,
> > > +                                    
> > > mtk_apu_mailbox_irq_btm_half, IRQF_ONESHOT,
> > > +                                     dev_name(dev), mbox);
> > 
> > pass mbox->chans to the isr
> > 
> > > +     if (ret)
> > > +             return dev_err_probe(dev, ret, "Failed to request
> > > IRQ\n");
> > > +
> > > +     g_mbox = mbox;
> > > +
> > > +     dev_dbg(dev, "registered mtk apu mailbox\n");
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static void mtk_apu_mailbox_remove(struct platform_device *pdev)
> > > +{
> > > +     g_mbox = NULL;
> > > +}
> > > +
> > > +static const struct of_device_id mtk_apu_mailbox_of_match[] = {
> > > +     { .compatible = "mediatek,mt8188-apu-mailbox" },
> > > +     { .compatible = "mediatek,mt8196-apu-mailbox" },
> > 
> > Just mediatek,mt8188-apu-mailbox is fine; you can allow
> > mt8196==mt8188 in the
> > binding instead.
> > 
> > > +     {}
> > > +};
> > > +MODULE_DEVICE_TABLE(of, mtk_apu_mailbox_of_match);
> > > +
> > > +static struct platform_driver mtk_apu_mailbox_driver = {
> > > +     .probe = mtk_apu_mailbox_probe,
> > > +     .remove = mtk_apu_mailbox_remove,
> > 
> > You don't need this remove callback, since g_mbox has to disappear
> > :-)
> > 
> > > +     .driver = {
> > > +             .name = "mtk-apu-mailbox",
> > > +             .of_match_table = mtk_apu_mailbox_of_match,
> > > +     },
> > > +};
> > > +
> > > +module_platform_driver(mtk_apu_mailbox_driver);
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_DESCRIPTION("MediaTek APU Mailbox Driver");
> > > diff --git a/include/linux/mailbox/mtk-apu-mailbox.h
> > > b/include/linux/mailbox/mtk-apu-mailbox.h
> > > new file mode 100644
> > > index 000000000000..d1457d16ce9b
> > > --- /dev/null
> > > +++ b/include/linux/mailbox/mtk-apu-mailbox.h
> > > @@ -0,0 +1,20 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright (c) 2024 MediaTek Inc.
> > > + *
> > > + */
> > > +
> > > +#ifndef __MTK_APU_MAILBOX_H__
> > > +#define __MTK_APU_MAILBOX_H__
> > > +
> > > +#define MSG_MBOX_SLOTS       (8)
> > > +
> > > +struct mtk_apu_mailbox_msg {
> > > +     int send_cnt;
> > 
> > u8 data_cnt;
> > 
> > > +     u32 data[MSG_MBOX_SLOTS];
> > 
> > With hardcoded slots, what happens when we get a new chip in the
> > future that
> > supports more slots?
> 
> Seems like we can make it a flexible array member? But the problem
> then
> becomes how does the client know what the maximum length is. Or maybe
> it should already know given it's tied to a particular platform.
> 
> In any case it becomes:
> 
>     struct mtk_apu_mailbox_msg {
>         u8 data_size;
>         u8 data[] __counted_by(data_size);
>     };
> 
> This can't be allocated on the stack if `data_size` isn't a compile
> time constant though; but again it shouldn't be a problem given the
> message size is tied to the client & its platform and should be
> constant anyway.
> 
> The controller should just error out if the message is larger than
> what it can atomically send.
> 
> 
> ChenYu
> 
> > Please think about this now and make the implementation flexible
> > before that
> > happens because, at a later time, it'll be harder.
> > 
> > Regards,
> > Angelo
> > 
> > > +};
> > > +
> > > +int mtk_apu_mbox_write(u32 val, u32 offset);
> > > +int mtk_apu_mbox_read(u32 offset, u32 *val);
> > > +
> > > +#endif /* __MTK_APU_MAILBOX_H__ */
> > 
> >

Re: [PATCH 3/3] mailbox: mediatek: Add mtk-apu-mailbox driver

Posted by Krzysztof Kozlowski 1 month ago

On 24/10/2024 11:25, Karl.Li wrote:
> From: Karl Li <karl.li@mediatek.com>
> 
> Add mtk-apu-mailbox driver to support the communication with
> APU remote microprocessor.
> 
> Also, the mailbox hardware contains extra spare (scratch) registers
> that other hardware blocks use to communicate through.
> Expose these with custom mtk_apu_mbox_(read|write)() functions.
> 
> Signed-off-by: Karl Li <karl.li@mediatek.com>
> ---
>  drivers/mailbox/Kconfig                 |   9 +
>  drivers/mailbox/Makefile                |   2 +
>  drivers/mailbox/mtk-apu-mailbox.c       | 222 ++++++++++++++++++++++++
>  include/linux/mailbox/mtk-apu-mailbox.h |  20 +++
>  4 files changed, 253 insertions(+)
>  create mode 100644 drivers/mailbox/mtk-apu-mailbox.c
>  create mode 100644 include/linux/mailbox/mtk-apu-mailbox.h
> 
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 6fb995778636..2338e08a110a 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -240,6 +240,15 @@ config MTK_ADSP_MBOX
>            between processors with ADSP. It will place the message to share
>  	  buffer and will access the ipc control.
>  
> +config MTK_APU_MBOX
> +	tristate "MediaTek APU Mailbox Support"
> +	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	help
> +	  Say yes here to add support for the MediaTek APU Mailbox
> +	  driver. The mailbox implementation provides access from the
> +	  application processor to the MediaTek AI Processing Unit.
> +	  If unsure say N.
> +
>  config MTK_CMDQ_MBOX
>  	tristate "MediaTek CMDQ Mailbox Support"
>  	depends on ARCH_MEDIATEK || COMPILE_TEST
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index 3c3c27d54c13..6b6dcc78d644 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -53,6 +53,8 @@ obj-$(CONFIG_STM32_IPCC) 	+= stm32-ipcc.o
>  
>  obj-$(CONFIG_MTK_ADSP_MBOX)	+= mtk-adsp-mailbox.o
>  
> +obj-$(CONFIG_MTK_APU_MBOX)	+= mtk-apu-mailbox.o
> +
>  obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o
>  
>  obj-$(CONFIG_ZYNQMP_IPI_MBOX)	+= zynqmp-ipi-mailbox.o
> diff --git a/drivers/mailbox/mtk-apu-mailbox.c b/drivers/mailbox/mtk-apu-mailbox.c
> new file mode 100644
> index 000000000000..b347ebd34ef7
> --- /dev/null
> +++ b/drivers/mailbox/mtk-apu-mailbox.c
> @@ -0,0 +1,222 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2024 MediaTek Inc.
> + */
> +
> +#include <asm/io.h>
> +#include <linux/bits.h>
> +#include <linux/interrupt.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/mailbox/mtk-apu-mailbox.h>
> +#include <linux/platform_device.h>
> +
> +#define INBOX		(0x0)
> +#define OUTBOX		(0x20)
> +#define INBOX_IRQ	(0xc0)
> +#define OUTBOX_IRQ	(0xc4)
> +#define INBOX_IRQ_MASK	(0xd0)
> +
> +#define SPARE_OFF_START	(0x40)
> +#define SPARE_OFF_END	(0xB0)
> +
> +struct mtk_apu_mailbox {
> +	struct device *dev;
> +	void __iomem *regs;
> +	struct mbox_controller controller;
> +	u32 msgs[MSG_MBOX_SLOTS];
> +};
> +
> +struct mtk_apu_mailbox *g_mbox;

Why this is global? And why do you support only one device?

No, drop.

> +
> +static irqreturn_t mtk_apu_mailbox_irq_top_half(int irq, void *dev_id)
> +{
> +	struct mtk_apu_mailbox *mbox = dev_id;
> +	struct mbox_chan *link = &mbox->controller.chans[0];
> +	int i;
> +
> +	for (i = 0; i < MSG_MBOX_SLOTS; i++)
> +		mbox->msgs[i] = readl(mbox->regs + OUTBOX + i * sizeof(u32));
> +
> +	mbox_chan_received_data(link, &mbox->msgs);
> +
> +	return IRQ_WAKE_THREAD;
> +}
> +

...

> +/**
> + * mtk_apu_mbox_write - Write value to specifice mtk_apu_mbox spare register.
> + * @val: Value to be written.
> + * @offset: Offset of the spare register.
> + *
> + * Return: 0 if successful
> + *	   negative value if error happened
> + */
> +int mtk_apu_mbox_write(u32 val, u32 offset)
> +{
> +	if (!g_mbox) {
> +		pr_err("mtk apu mbox was not initialized, stop writing register\n");
> +		return -ENODEV;
> +	}
> +
> +	if (offset < SPARE_OFF_START || offset >= SPARE_OFF_END) {
> +		dev_err(g_mbox->dev, "Invalid offset %d for mtk apu mbox spare register\n", offset);
> +		return -EINVAL;
> +	}
> +
> +	writel(val, g_mbox->regs + offset);
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS(mtk_apu_mbox_write, MTK_APU_MAILBOX);

Use mailbox API. This is really poor solution.

> +
> +/**
> + * mtk_apu_mbox_read - Read value to specifice mtk_apu_mbox spare register.
> + * @offset: Offset of the spare register.
> + * @val: Pointer to store read value.
> + *
> + * Return: 0 if successful
> + *	   negative value if error happened
> + */
> +int mtk_apu_mbox_read(u32 offset, u32 *val)
> +{
> +	if (!g_mbox) {
> +		pr_err("mtk apu mbox was not initialized, stop reading register\n");
> +		return -ENODEV;
> +	}
> +
> +	if (offset < SPARE_OFF_START || offset >= SPARE_OFF_END) {
> +		dev_err(g_mbox->dev, "Invalid offset %d for mtk apu mbox spare register\n", offset);
> +		return -EINVAL;
> +	}
> +
> +	*val = readl(g_mbox->regs + offset);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS(mtk_apu_mbox_read, MTK_APU_MAILBOX);
> +
> +static int mtk_apu_mailbox_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mtk_apu_mailbox *mbox;
> +	int irq = -1, ret = 0;
> +
> +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> +	if (!mbox)
> +		return -ENOMEM;
> +
> +	mbox->dev = dev;
> +	platform_set_drvdata(pdev, mbox);
> +
> +	mbox->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(mbox->regs))
> +		return PTR_ERR(mbox->regs);
> +
> +	mbox->controller.txdone_irq = false;
> +	mbox->controller.txdone_poll = true;
> +	mbox->controller.txpoll_period = 1;
> +	mbox->controller.ops = &mtk_apu_mailbox_ops;
> +	mbox->controller.dev = dev;
> +	/*
> +	 * Here we only register 1 mbox channel.
> +	 * The remaining channels are used by other modules.
> +	 */
> +	mbox->controller.num_chans = 1;
> +	mbox->controller.chans = devm_kcalloc(dev, mbox->controller.num_chans,
> +					      sizeof(*mbox->controller.chans),
> +					      GFP_KERNEL);
> +	if (!mbox->controller.chans)
> +		return -ENOMEM;
> +
> +	ret = devm_mbox_controller_register(dev, &mbox->controller);
> +	if (ret)
> +		return ret;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = devm_request_threaded_irq(dev, irq, mtk_apu_mailbox_irq_top_half,
> +					mtk_apu_mailbox_irq_btm_half, IRQF_ONESHOT,
> +					dev_name(dev), mbox);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to request IRQ\n");
> +
> +	g_mbox = mbox;
> +
> +	dev_dbg(dev, "registered mtk apu mailbox\n");

No, drop such stuff.

> +
> +	return 0;
> +}
> +
> +static void mtk_apu_mailbox_remove(struct platform_device *pdev)
> +{
> +	g_mbox = NULL;
> +}
> +
> +static const struct of_device_id mtk_apu_mailbox_of_match[] = {
> +	{ .compatible = "mediatek,mt8188-apu-mailbox" },
> +	{ .compatible = "mediatek,mt8196-apu-mailbox" },

So devices are compatible? Make them compatible in the binding and drop
unneeded compatible here.

Best regards,
Krzysztof

[PATCH 1/3] dt-bindings: mailbox: mediatek: Add apu-mailbox document
[PATCH 2/3] mailbox: add support for bottom half received data
[PATCH 3/3] mailbox: mediatek: Add mtk-apu-mailbox driver