[PATCH 0/3] Add MediaTek APU Mailbox Support For MT8196

[PATCH 0/3] Add MediaTek APU Mailbox Support For MT8196

[Karl.Li]

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

Based on tag: next-20241021, linux-next/master

Hello,

This patch series introduces support for the MediaTek APU (AI Processing Unit) mailbox, a crucial component for facilitating communication between the APU and other system processors within MediaTek platforms. The APU subsystem relies on a message-passing mechanism built atop the mailbox infrastructure, necessitating enhancements to the existing mailbox framework to accommodate the APU's communication requirements.

The series begins by adding the necessary device tree bindings for the APU mailbox, followed by an enhancement to the mailbox framework allowing for bottom-half processing of received data. This is particularly important for the APU's operation, as it relies on a combination of mailbox messages and shared memory for data exchange. Finally, we introduce the MediaTek APU mailbox driver itself, which implements the communication protocol and exposes additional hardware features for broader system integration.

Patch Summary:
1. dt-bindings: mailbox: mediatek: Add apu-mailbox document
   - Introduces the device tree bindings necessary for describing the APU mailbox in device tree sources, enabling the kernel to correctly configure and utilize this component.

2. mailbox: add support for bottom half received data
   - Enhances the mailbox framework to support sleepable contexts in the processing of received messages. This is critical for APU communication, where message handling may require operations that cannot be performed in atomic contexts.

3. mailbox: mediatek: Add mtk-apu-mailbox driver
   - Adds the driver for the MediaTek APU mailbox, facilitating communication with the APU microprocessor and providing interfaces for other system components to interact with the APU through spare registers.

This work is a step towards fully integrating MediaTek's APU capabilities with the Linux kernel, enhancing support for AI features on MediaTek platforms.

Please review and provide feedback.

Best regards

Karl Li (3):
  dt-bindings: mailbox: mediatek: Add apu-mailbox document
  mailbox: add support for bottom half received data
  mailbox: mediatek: Add mtk-apu-mailbox driver

 .../mailbox/mediatek,apu-mailbox.yaml         |  55 +++++
 drivers/mailbox/Kconfig                       |   9 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/mailbox.c                     |  16 ++
 drivers/mailbox/mtk-apu-mailbox.c             | 222 ++++++++++++++++++
 include/linux/mailbox/mtk-apu-mailbox.h       |  20 ++
 include/linux/mailbox_client.h                |   2 +
 include/linux/mailbox_controller.h            |   1 +
 8 files changed, 327 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/mediatek,apu-mailbox.yaml
 create mode 100644 drivers/mailbox/mtk-apu-mailbox.c
 create mode 100644 include/linux/mailbox/mtk-apu-mailbox.h

-- 
2.18.0

[PATCH 1/3] dt-bindings: mailbox: mediatek: Add apu-mailbox document

[Karl.Li]

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

Add apu-mailbox dt-binding document.

Signed-off-by: Karl Li <karl.li@mediatek.com>
---
 .../mailbox/mediatek,apu-mailbox.yaml         | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/mediatek,apu-mailbox.yaml

diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,apu-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,apu-mailbox.yaml
new file mode 100644
index 000000000000..cb4530799bef
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/mediatek,apu-mailbox.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/mediatek,apu-mailbox.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek APU mailbox
+
+maintainers:
+  - Karl Li <Karl.Li@mediatek.com>
+
+description:
+  The MediaTek APU-Mailbox facilitates communication with the
+  APU microcontroller. Within the MediaTek APU subsystem, a
+  message passing mechanism is built on top of the mailbox system.
+  The mailbox only has limited space for each message. The firmware
+  expects the message header from the mailbox, while the message body
+  is passed through some fixed shared memory.
+
+properties:
+  compatible:
+    enum:
+      - mediatek,mt8188-apu-mailbox
+      - mediatek,mt8196-apu-mailbox
+
+  "#mbox-cells":
+    const: 1
+    description:
+      The cell describe which channel the device will use.
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - "#mbox-cells"
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    apu_mailbox: mailbox@4c200000 {
+      compatible = "mediatek,mt8196-apu-mailbox";
+      reg = <0 0x4c200000 0 0xfffff>;
+      interrupts = <GIC_SPI 638 IRQ_TYPE_LEVEL_HIGH 0>;
+      #mbox-cells = <1>;
+    };
-- 
2.18.0

[PATCH 2/3] mailbox: add support for bottom half received data

[Karl.Li]

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

Within the MediaTek APU subsystem, a message passing mechanism
is constructed on top of the mailbox system.

The mailbox only has limited space for each message. The MTK APU firmware
expects the message header from the mailbox, while the message body
is passed through some fixed shared memory.

The mailbox interrupt also serves as a mutex for the shared memory.
Thus the interrupt may only be cleared after the message is handled.
Add a new sleepable rx callback for mailbox clients for cases
where handling the incoming data needs to sleep.

Signed-off-by: Karl Li <karl.li@mediatek.com>
---
 drivers/mailbox/mailbox.c          | 16 ++++++++++++++++
 include/linux/mailbox_client.h     |  2 ++
 include/linux/mailbox_controller.h |  1 +
 3 files changed, 19 insertions(+)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index d3d26a2c9895..d58a77fcf804 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -164,6 +164,22 @@ void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
 }
 EXPORT_SYMBOL_GPL(mbox_chan_received_data);
 
+/**
+ * mbox_chan_received_data_bh - A way for controller driver to push data
+ *				received from remote to the upper layer.
+ * @chan: Pointer to the mailbox channel on which RX happened.
+ * @mssg: Client specific message typecasted as void *
+ *
+ * For the operations which is not atomic can be called from
+ * mbox_chan_received_data_bh().
+ */
+void mbox_chan_received_data_bh(struct mbox_chan *chan, void *mssg)
+{
+	if (chan->cl->rx_callback_bh)
+		chan->cl->rx_callback_bh(chan->cl, mssg);
+}
+EXPORT_SYMBOL_GPL(mbox_chan_received_data_bh);
+
 /**
  * mbox_chan_txdone - A way for controller driver to notify the
  *			framework that the last TX has completed.
diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
index 734694912ef7..2cc6fa4e1bf9 100644
--- a/include/linux/mailbox_client.h
+++ b/include/linux/mailbox_client.h
@@ -22,6 +22,7 @@ struct mbox_chan;
  *			if the client receives some ACK packet for transmission.
  *			Unused if the controller already has TX_Done/RTR IRQ.
  * @rx_callback:	Atomic callback to provide client the data received
+ * @rx_callback_bh:	Non-atomic callback to provide client the data received
  * @tx_prepare: 	Atomic callback to ask client to prepare the payload
  *			before initiating the transmission if required.
  * @tx_done:		Atomic callback to tell client of data transmission
@@ -33,6 +34,7 @@ struct mbox_client {
 	bool knows_txdone;
 
 	void (*rx_callback)(struct mbox_client *cl, void *mssg);
+	void (*rx_callback_bh)(struct mbox_client *cl, void *mssg);
 	void (*tx_prepare)(struct mbox_client *cl, void *mssg);
 	void (*tx_done)(struct mbox_client *cl, void *mssg, int r);
 };
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index 6fee33cb52f5..74c6a31cd313 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -130,6 +130,7 @@ struct mbox_chan {
 int mbox_controller_register(struct mbox_controller *mbox); /* can sleep */
 void mbox_controller_unregister(struct mbox_controller *mbox); /* can sleep */
 void mbox_chan_received_data(struct mbox_chan *chan, void *data); /* atomic */
+void mbox_chan_received_data_bh(struct mbox_chan *chan, void *data); /* can sleep */
 void mbox_chan_txdone(struct mbox_chan *chan, int r); /* atomic */
 
 int devm_mbox_controller_register(struct device *dev,
-- 
2.18.0

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

[Karl.Li]

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 1/3] dt-bindings: mailbox: mediatek: Add apu-mailbox document

[Krzysztof Kozlowski]

On 24/10/2024 11:25, Karl.Li wrote:
> From: Karl Li <karl.li@mediatek.com>
> 
> Add apu-mailbox dt-binding document.

We see from the diff. What we see is what is APU and this hardware?

A nit, subject: drop second/last, redundant "document". The
"dt-bindings" prefix is already stating that these are bindings so a
document.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> 
> Signed-off-by: Karl Li <karl.li@mediatek.com>
> ---
>  .../mailbox/mediatek,apu-mailbox.yaml         | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/mediatek,apu-mailbox.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,apu-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,apu-mailbox.yaml
> new file mode 100644
> index 000000000000..cb4530799bef
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/mediatek,apu-mailbox.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/mediatek,apu-mailbox.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek APU mailbox

What is APU?

> +
> +maintainers:
> +  - Karl Li <Karl.Li@mediatek.com>
> +
> +description:
> +  The MediaTek APU-Mailbox facilitates communication with the
> +  APU microcontroller. Within the MediaTek APU subsystem, a
> +  message passing mechanism is built on top of the mailbox system.
> +  The mailbox only has limited space for each message. The firmware
> +  expects the message header from the mailbox, while the message body
> +  is passed through some fixed shared memory.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt8188-apu-mailbox
> +      - mediatek,mt8196-apu-mailbox
> +
> +  "#mbox-cells":
> +    const: 1
> +    description:
> +      The cell describe which channel the device will use.
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - "#mbox-cells"
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    apu_mailbox: mailbox@4c200000 {

Drop unused label.

> +      compatible = "mediatek,mt8196-apu-mailbox";
> +      reg = <0 0x4c200000 0 0xfffff>;
> +      interrupts = <GIC_SPI 638 IRQ_TYPE_LEVEL_HIGH 0>;

4 cells? No warnings on this?

> +      #mbox-cells = <1>;
> +    };

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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

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

[AngeloGioacchino Del Regno]

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 2/3] mailbox: add support for bottom half received data

[AngeloGioacchino Del Regno]

Il 24/10/24 11:25, Karl.Li ha scritto:
> From: Karl Li <karl.li@mediatek.com>
> 
> Within the MediaTek APU subsystem, a message passing mechanism
> is constructed on top of the mailbox system.
> 
> The mailbox only has limited space for each message. The MTK APU firmware
> expects the message header from the mailbox, while the message body
> is passed through some fixed shared memory.
> 
> The mailbox interrupt also serves as a mutex for the shared memory.
> Thus the interrupt may only be cleared after the message is handled.
> Add a new sleepable rx callback for mailbox clients for cases
> where handling the incoming data needs to sleep.
> 
> Signed-off-by: Karl Li <karl.li@mediatek.com>

As said in the review of the ADSP MBOX driver, I really don't think that you need
an extra callback.

Regards,
Angelo

Re: [PATCH 1/3] dt-bindings: mailbox: mediatek: Add apu-mailbox document

[AngeloGioacchino Del Regno]

Il 24/10/24 11:25, Karl.Li ha scritto:
> From: Karl Li <karl.li@mediatek.com>
> 
> Add apu-mailbox dt-binding document.
> 
> Signed-off-by: Karl Li <karl.li@mediatek.com>
> ---
>   .../mailbox/mediatek,apu-mailbox.yaml         | 55 +++++++++++++++++++
>   1 file changed, 55 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/mailbox/mediatek,apu-mailbox.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,apu-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,apu-mailbox.yaml
> new file mode 100644
> index 000000000000..cb4530799bef
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/mediatek,apu-mailbox.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/mediatek,apu-mailbox.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek APU mailbox
> +
> +maintainers:
> +  - Karl Li <Karl.Li@mediatek.com>
> +
> +description:
> +  The MediaTek APU-Mailbox facilitates communication with the
> +  APU microcontroller. Within the MediaTek APU subsystem, a
> +  message passing mechanism is built on top of the mailbox system.
> +  The mailbox only has limited space for each message. The firmware
> +  expects the message header from the mailbox, while the message body
> +  is passed through some fixed shared memory.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt8188-apu-mailbox
> +      - mediatek,mt8196-apu-mailbox

In addition to Krzysztof's review......

oneOf:
   - const: mediatek,mt8188-apu-mailbox
   - items:
     - enum:
         - mediatek,mt8195-apu-mailbox
         - mediatek,mt8196-apu-mailbox
     - const: mediatek,mt8188-apu-mailbox

should be like that - I haven't checked if this gives any warnings, but anyway
you got the point, I think.

Regards,
Angelo

Re: [PATCH 1/3] dt-bindings: mailbox: mediatek: Add apu-mailbox document

[Rob Herring (Arm)]

On Thu, 24 Oct 2024 17:25:43 +0800, Karl.Li wrote:
> From: Karl Li <karl.li@mediatek.com>
> 
> Add apu-mailbox dt-binding document.
> 
> Signed-off-by: Karl Li <karl.li@mediatek.com>
> ---
>  .../mailbox/mediatek,apu-mailbox.yaml         | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/mediatek,apu-mailbox.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mailbox/mediatek,apu-mailbox.example.dtb: mailbox@4c200000: reg: [[0, 1277165568], [0, 1048575]] is too long
	from schema $id: http://devicetree.org/schemas/mailbox/mediatek,apu-mailbox.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241024092608.431581-2-karl.li@mediatek.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[kernel test robot]

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

[Chen-Yu Tsai]

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

[Karl Li (李智嘉)]

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

[Karl Li (李智嘉)]

Dead maintainers,

I hope you're doing well. Just a warm reminder that we're following up
on these patch and really appreciate any feedback you might have.

Thanks you in advance for your review.

Regards,
Karl

On Tue, 2024-10-29 at 16:27 +0800, Karl Li wrote:
> 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

[Karl Li (李智嘉)]

On Thu, 2024-12-05 at 07:05 +0000, Karl Li (李智嘉) wrote:
> Dead maintainers,
"Dear" maintainers. Really sorry for the typo...
> 
> I hope you're doing well. Just a warm reminder that we're following
> up
> on these patch and really appreciate any feedback you might have.
> 
> Thanks you in advance for your review.
> 
> Regards,
> Karl
> 
> On Tue, 2024-10-29 at 16:27 +0800, Karl Li wrote:
> > 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

[AngeloGioacchino Del Regno]

Il 05/12/24 08:32, Karl Li (李智嘉) ha scritto:
> On Thu, 2024-12-05 at 07:05 +0000, Karl Li (李智嘉) wrote:
>> Dead maintainers,

LOL :-D

> "Dear" maintainers. Really sorry for the typo...
>>
>> I hope you're doing well. Just a warm reminder that we're following
>> up
>> on these patch and really appreciate any feedback you might have.

I already gave you feedback on this patch.

Cheers,
Angelo

>>
>> Thanks you in advance for your review.
>>
>> Regards,
>> Karl
>>

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

[Krzysztof Kozlowski]

On 05/12/2024 08:32, Karl Li (李智嘉) wrote:
> On Thu, 2024-12-05 at 07:05 +0000, Karl Li (李智嘉) wrote:
>> Dead maintainers,
> "Dear" maintainers. Really sorry for the typo...
>>
>> I hope you're doing well. Just a warm reminder that we're following
>> up
>> on these patch and really appreciate any feedback you might have.
>>

You received like 6 or 7 reviews/replies for your patchset. What are you
implying here if feedback was not enough?

Respond and implement the feedback instead ignoring it.

NAK

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 10/12/2024 09:44, Krzysztof Kozlowski wrote:
> On 05/12/2024 08:32, Karl Li (李智嘉) wrote:
>> On Thu, 2024-12-05 at 07:05 +0000, Karl Li (李智嘉) wrote:
>>> Dead maintainers,
>> "Dear" maintainers. Really sorry for the typo...
>>>
>>> I hope you're doing well. Just a warm reminder that we're following
>>> up
>>> on these patch and really appreciate any feedback you might have.
>>>
> 
> You received like 6 or 7 reviews/replies for your patchset. What are you
> implying here if feedback was not enough?
> 
> Respond and implement the feedback instead ignoring it.
> 

Ah - and obvious static check warnings as well!

Please run standard kernel tools for static analysis, like coccinelle,
smatch and sparse, and fix reported warnings. Also please check for
warnings when building with W=1. Most of these commands (checks or W=1
build) can build specific targets, like some directory, to narrow the
scope to only your code. The code here looks like it needs a fix. Feel
free to get in touch if the warning is not clear.

Best regards,
Krzysztof