[PATCH v3 0/2] ASPEED: Add mailbox driver for AST2700 series

[PATCH v3 0/2] ASPEED: Add mailbox driver for AST2700 series

[Jammy Huang]

Add mailbox controller driver for AST27XX SoCs, which provides
independent tx/rx mailbox between different processors. There are 4
channels for each tx/rx mailbox and each channel has an 32-byte FIFO.

 v2 changes:
  - Correct document
     1. Use 32-bit addressing in dts example property, reg.
  - Update document
     1. Correct error in dts example.
     2. Drop description for mbox-cell per suggestion previously.

Jammy Huang (2):
  dt-bindings: mailbox: Add ASPEED AST2700 series SoC
  mailbox: aspeed: add mailbox driver for AST27XX series SoC

 .../mailbox/aspeed,ast2700-mailbox.yaml       |  57 +++++
 drivers/mailbox/Kconfig                       |   8 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/ast2700-mailbox.c             | 226 ++++++++++++++++++
 4 files changed, 293 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/aspeed,ast2700-mailbox.yaml
 create mode 100644 drivers/mailbox/ast2700-mailbox.c


base-commit: ec7714e4947909190ffb3041a03311a975350fe0
-- 
2.25.1

[PATCH v3 1/2] dt-bindings: mailbox: Add ASPEED AST2700 series SoC

[Jammy Huang]

Introduce the mailbox module for AST27XX series SoC, which is responsible
for interchanging messages between asymmetric processors.

Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
---
 .../mailbox/aspeed,ast2700-mailbox.yaml       | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/aspeed,ast2700-mailbox.yaml

diff --git a/Documentation/devicetree/bindings/mailbox/aspeed,ast2700-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/aspeed,ast2700-mailbox.yaml
new file mode 100644
index 000000000000..9c5d7028e116
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/aspeed,ast2700-mailbox.yaml
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/aspeed,ast2700-mailbox.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ASPEED AST2700 mailbox controller
+
+maintainers:
+  - Jammy Huang <jammy_huang@aspeedtech.com>
+
+description:
+  ASPEED AST2700 has multiple processors that need to communicate with each
+  other. The mailbox controller provides a way for these processors to send
+  messages to each other. It is a hardware-based inter-processor communication
+  mechanism that allows processors to send and receive messages through
+  dedicated channels.
+  The mailbox's tx/rx are independent, meaning that one processor can send a
+  message while another processor is receiving a message simultaneously.
+  There are 4 channels available for both tx and rx operations. Each channel
+  has a FIFO buffer that can hold messages of a fixed size (32 bytes in this
+  case).
+  The mailbox controller also supports interrupt generation, allowing
+  processors to notify each other when a message is available or when an event
+  occurs.
+
+properties:
+  compatible:
+    const: aspeed,ast2700-mailbox
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  "#mbox-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - "#mbox-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    mailbox@12c1c200 {
+        compatible = "aspeed,ast2700-mailbox";
+        reg = <0x12c1c200 0x200>;
+        interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
+        #mbox-cells = <1>;
+    };
-- 
2.25.1

Re: [PATCH v3 1/2] dt-bindings: mailbox: Add ASPEED AST2700 series SoC

[Krzysztof Kozlowski]

On Tue, Jun 10, 2025 at 05:10:25PM GMT, Jammy Huang wrote:
> Introduce the mailbox module for AST27XX series SoC, which is responsible
> for interchanging messages between asymmetric processors.
> 
> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
> ---
>  .../mailbox/aspeed,ast2700-mailbox.yaml       | 57 +++++++++++++++++++
>  1 file changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/aspeed,ast2700-mailbox.yaml

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

<form letter>
This is an automated instruction, just in case, because many review
tags are being ignored. If you know the process, just skip it entirely
(please do not feel offended by me posting it here - no bad intentions
intended, no patronizing, I just want to avoid wasted efforts). If you
do not know the process, here is a short explanation:

Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions of patchset, under or above your Signed-off-by tag, unless
patch changed significantly (e.g. new properties added to the DT
bindings). Tag is "received", when provided in a message replied to you
on the mailing list. Tools like b4 can help here ('b4 trailers -u ...').
However, there's no need to repost patches *only* to add the tags. The
upstream maintainer will do that for tags received on the version they
apply.

https://elixir.bootlin.com/linux/v6.15/source/Documentation/process/submitting-patches.rst#L591
</form letter>

Best regards,
Krzysztof

[PATCH v3 2/2] mailbox: aspeed: add mailbox driver for AST27XX series SoC

[Jammy Huang]

Add mailbox controller driver for AST27XX SoCs, which provides
independent tx/rx mailbox between different processors. There are 4
channels for each tx/rx mailbox and each channel has an 32-byte FIFO.

Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
---
 drivers/mailbox/Kconfig           |   8 ++
 drivers/mailbox/Makefile          |   2 +
 drivers/mailbox/ast2700-mailbox.c | 226 ++++++++++++++++++++++++++++++
 3 files changed, 236 insertions(+)
 create mode 100644 drivers/mailbox/ast2700-mailbox.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 68eeed660a4a..1c38cd570091 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -340,4 +340,12 @@ config THEAD_TH1520_MBOX
 	  kernel is running, and E902 core used for power management among other
 	  things.
 
+config AST2700_MBOX
+	tristate "ASPEED AST2700 IPC driver"
+	depends on ARCH_ASPEED || COMPILE_TEST
+	help
+	  Mailbox driver implementation for ASPEED AST27XX SoCs. This driver
+	  can be used to send message between different processors in SoC.
+	  The driver provides mailbox support for sending interrupts to the
+	  clients. Say Y here if you want to build this driver.
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 13a3448b3271..9a9add9a7548 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -72,3 +72,5 @@ obj-$(CONFIG_QCOM_CPUCP_MBOX)	+= qcom-cpucp-mbox.o
 obj-$(CONFIG_QCOM_IPCC)		+= qcom-ipcc.o
 
 obj-$(CONFIG_THEAD_TH1520_MBOX)	+= mailbox-th1520.o
+
+obj-$(CONFIG_AST2700_MBOX)	+= ast2700-mailbox.o
diff --git a/drivers/mailbox/ast2700-mailbox.c b/drivers/mailbox/ast2700-mailbox.c
new file mode 100644
index 000000000000..0ee10bd3a6e1
--- /dev/null
+++ b/drivers/mailbox/ast2700-mailbox.c
@@ -0,0 +1,226 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright Aspeed Technology Inc. (C) 2025. All rights reserved
+ */
+
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/* Each bit in the register represents an IPC ID */
+#define IPCR_TX_TRIG		0x00
+#define IPCR_TX_ENABLE		0x04
+#define IPCR_RX_ENABLE		0x104
+#define IPCR_TX_STATUS		0x08
+#define IPCR_RX_STATUS		0x108
+#define  RX_IRQ(n)		BIT(0 + 1 * (n))
+#define  RX_IRQ_MASK		0xf
+#define IPCR_TX_DATA		0x10
+#define IPCR_RX_DATA		0x110
+
+struct ast2700_mbox_data {
+	u8 num_chans;
+	u8 msg_size;
+};
+
+struct ast2700_mbox {
+	struct mbox_controller mbox;
+	const struct ast2700_mbox_data *drv_data;
+	void __iomem *regs;
+	u32 *rx_buff;
+};
+
+static inline int ch_num(struct mbox_chan *chan)
+{
+	return chan - chan->mbox->chans;
+}
+
+static inline int ast2700_mbox_tx_done(struct ast2700_mbox *mb, int idx)
+{
+	return !(readl(mb->regs + IPCR_TX_STATUS) & BIT(idx));
+}
+
+static irqreturn_t ast2700_mbox_irq(int irq, void *p)
+{
+	struct ast2700_mbox *mb = p;
+	void __iomem *data_reg;
+	int num_words;
+	u32 *word_data;
+	u32 status;
+	int n;
+
+	/* Only examine channels that are currently enabled. */
+	status = readl(mb->regs + IPCR_RX_ENABLE) &
+		 readl(mb->regs + IPCR_RX_STATUS);
+
+	if (!(status & RX_IRQ_MASK))
+		return IRQ_NONE;
+
+	for (n = 0; n < mb->mbox.num_chans; ++n) {
+		struct mbox_chan *chan = &mb->mbox.chans[n];
+
+		if (!(status & RX_IRQ(n)))
+			continue;
+
+		for (data_reg = mb->regs + IPCR_RX_DATA + mb->drv_data->msg_size * n,
+		     word_data = chan->con_priv,
+		     num_words = (mb->drv_data->msg_size / sizeof(u32));
+		     num_words; num_words--, data_reg += sizeof(u32), word_data++)
+			*word_data = readl(data_reg);
+
+		mbox_chan_received_data(chan, chan->con_priv);
+
+		/* The IRQ can be cleared only once the FIFO is empty. */
+		writel(RX_IRQ(n), mb->regs + IPCR_RX_STATUS);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int ast2700_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+	struct ast2700_mbox *mb = dev_get_drvdata(chan->mbox->dev);
+	void __iomem *data_reg;
+	u32 *word_data;
+	int num_words;
+	int idx = ch_num(chan);
+
+	if (!(readl(mb->regs + IPCR_TX_ENABLE) & BIT(idx))) {
+		dev_warn(mb->mbox.dev, "%s: Ch-%d not enabled yet\n", __func__, idx);
+		return -EBUSY;
+	}
+
+	if (!(ast2700_mbox_tx_done(mb, idx))) {
+		dev_warn(mb->mbox.dev, "%s: Ch-%d last data has not finished\n", __func__, idx);
+		return -EBUSY;
+	}
+
+	for (data_reg = mb->regs + IPCR_TX_DATA + mb->drv_data->msg_size * idx,
+	     num_words = (mb->drv_data->msg_size / sizeof(u32)),
+	     word_data = (u32 *)data;
+	     num_words; num_words--, data_reg += sizeof(u32), word_data++)
+		writel(*word_data, data_reg);
+
+	writel(BIT(idx), mb->regs + IPCR_TX_TRIG);
+	dev_dbg(mb->mbox.dev, "%s: Ch-%d sent\n", __func__, idx);
+
+	return 0;
+}
+
+static int ast2700_mbox_startup(struct mbox_chan *chan)
+{
+	struct ast2700_mbox *mb = dev_get_drvdata(chan->mbox->dev);
+	int idx = ch_num(chan);
+	void __iomem *reg = mb->regs + IPCR_RX_ENABLE;
+
+	writel_relaxed(readl_relaxed(reg) | BIT(idx), reg);
+
+	return 0;
+}
+
+static void ast2700_mbox_shutdown(struct mbox_chan *chan)
+{
+	struct ast2700_mbox *mb = dev_get_drvdata(chan->mbox->dev);
+	int idx = ch_num(chan);
+	void __iomem *reg = mb->regs + IPCR_RX_ENABLE;
+
+	writel_relaxed(readl_relaxed(reg) & ~BIT(idx), reg);
+}
+
+static bool ast2700_mbox_last_tx_done(struct mbox_chan *chan)
+{
+	struct ast2700_mbox *mb = dev_get_drvdata(chan->mbox->dev);
+	int idx = ch_num(chan);
+
+	return ast2700_mbox_tx_done(mb, idx) ? true : false;
+}
+
+static const struct mbox_chan_ops ast2700_mbox_chan_ops = {
+	.send_data	= ast2700_mbox_send_data,
+	.startup	= ast2700_mbox_startup,
+	.shutdown	= ast2700_mbox_shutdown,
+	.last_tx_done	= ast2700_mbox_last_tx_done,
+};
+
+static int ast2700_mbox_probe(struct platform_device *pdev)
+{
+	struct ast2700_mbox *mb;
+	const struct ast2700_mbox_data *drv_data;
+	struct device *dev = &pdev->dev;
+	int irq, ret;
+
+	if (!pdev->dev.of_node)
+		return -ENODEV;
+
+	drv_data = (const struct ast2700_mbox_data *)device_get_match_data(&pdev->dev);
+
+	mb = devm_kzalloc(dev, sizeof(*mb), GFP_KERNEL);
+	if (!mb)
+		return -ENOMEM;
+
+	mb->mbox.chans = devm_kcalloc(&pdev->dev, drv_data->num_chans,
+				      sizeof(*mb->mbox.chans), GFP_KERNEL);
+	if (!mb->mbox.chans)
+		return -ENOMEM;
+
+	for (int i = 0; i < drv_data->num_chans; i++) {
+		mb->mbox.chans[i].con_priv = devm_kcalloc(dev, drv_data->msg_size,
+							  sizeof(u8), GFP_KERNEL);
+		if (!mb->mbox.chans[i].con_priv)
+			return -ENOMEM;
+	}
+
+	platform_set_drvdata(pdev, mb);
+
+	mb->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(mb->regs))
+		return PTR_ERR(mb->regs);
+
+	mb->drv_data = drv_data;
+	mb->mbox.dev = dev;
+	mb->mbox.num_chans = drv_data->num_chans;
+	mb->mbox.ops = &ast2700_mbox_chan_ops;
+	mb->mbox.txdone_irq = false;
+	mb->mbox.txdone_poll = true;
+	mb->mbox.txpoll_period = 5;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_irq(dev, irq, ast2700_mbox_irq, 0, dev_name(dev), mb);
+	if (ret)
+		return ret;
+
+	return devm_mbox_controller_register(dev, &mb->mbox);
+}
+
+static const struct ast2700_mbox_data ast2700_drv_data = {
+	.num_chans = 4,
+	.msg_size = 0x20,
+};
+
+static const struct of_device_id ast2700_mbox_of_match[] = {
+	{ .compatible = "aspeed,ast2700-mailbox", .data = &ast2700_drv_data },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ast2700_mbox_of_match);
+
+static struct platform_driver ast2700_mbox_driver = {
+	.driver = {
+		.name = "ast2700-mailbox",
+		.of_match_table = ast2700_mbox_of_match,
+	},
+	.probe = ast2700_mbox_probe,
+};
+module_platform_driver(ast2700_mbox_driver);
+
+MODULE_AUTHOR("Jammy Huang <jammy_huang@aspeedtech.com>");
+MODULE_DESCRIPTION("ASPEED AST2700 IPC driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v3 2/2] mailbox: aspeed: add mailbox driver for AST27XX series SoC

[Andrew Jeffery]

Hi Jammy,

As far as I can tell this controller isn't documented in the datasheet
for the AST2700. Can you point me to the right place? Or, can we get
the documentation updated?

On Tue, 2025-06-10 at 17:10 +0800, Jammy Huang wrote:
> > Add mailbox controller driver for AST27XX SoCs, which provides
> > independent tx/rx mailbox between different processors. There are 4
> > channels for each tx/rx mailbox and each channel has an 32-byte FIFO.
> > 
> > Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
> > ---
> >  drivers/mailbox/Kconfig           |   8 ++
> >  drivers/mailbox/Makefile          |   2 +
> >  drivers/mailbox/ast2700-mailbox.c | 226 ++++++++++++++++++++++++++++++
> >  3 files changed, 236 insertions(+)
> >  create mode 100644 drivers/mailbox/ast2700-mailbox.c
> > 
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > index 68eeed660a4a..1c38cd570091 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -340,4 +340,12 @@ config THEAD_TH1520_MBOX
> >           kernel is running, and E902 core used for power management among other
> >           things.
> >  
> > +config AST2700_MBOX
> > +       tristate "ASPEED AST2700 IPC driver"
> > +       depends on ARCH_ASPEED || COMPILE_TEST
> > +       help
> > +         Mailbox driver implementation for ASPEED AST27XX SoCs. This driver
> > +         can be used to send message between different processors in SoC.
> > +         The driver provides mailbox support for sending interrupts to the
> > +         clients. Say Y here if you want to build this driver.
> >  endif
> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > index 13a3448b3271..9a9add9a7548 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -72,3 +72,5 @@ obj-$(CONFIG_QCOM_CPUCP_MBOX) += qcom-cpucp-mbox.o
> >  obj-$(CONFIG_QCOM_IPCC)                += qcom-ipcc.o
> >  
> >  obj-$(CONFIG_THEAD_TH1520_MBOX)        += mailbox-th1520.o
> > +
> > +obj-$(CONFIG_AST2700_MBOX)     += ast2700-mailbox.o
> > diff --git a/drivers/mailbox/ast2700-mailbox.c b/drivers/mailbox/ast2700-mailbox.c
> > new file mode 100644
> > index 000000000000..0ee10bd3a6e1
> > --- /dev/null
> > +++ b/drivers/mailbox/ast2700-mailbox.c
> > @@ -0,0 +1,226 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright Aspeed Technology Inc. (C) 2025. All rights reserved
> > + */
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mailbox_controller.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +/* Each bit in the register represents an IPC ID */
> > +#define IPCR_TX_TRIG           0x00
> > +#define IPCR_TX_ENABLE         0x04
> > +#define IPCR_RX_ENABLE         0x104
> > +#define IPCR_TX_STATUS         0x08
> > +#define IPCR_RX_STATUS         0x108
> > +#define  RX_IRQ(n)             BIT(0 + 1 * (n))
> > +#define  RX_IRQ_MASK           0xf
> > +#define IPCR_TX_DATA           0x10
> > +#define IPCR_RX_DATA           0x110
> > +
> > +struct ast2700_mbox_data {
> > +       u8 num_chans;
> > +       u8 msg_size;
> > +};
> > +
> > +struct ast2700_mbox {
> > +       struct mbox_controller mbox;
> > +       const struct ast2700_mbox_data *drv_data;
> > +       void __iomem *regs;
> > +       u32 *rx_buff;
> > +};
> > +
> > +static inline int ch_num(struct mbox_chan *chan)
> > +{
> > +       return chan - chan->mbox->chans;
> > +}
> > +
> > +static inline int ast2700_mbox_tx_done(struct ast2700_mbox *mb, int idx)
> > +{
> > +       return !(readl(mb->regs + IPCR_TX_STATUS) & BIT(idx));
> > +}
> > +
> > +static irqreturn_t ast2700_mbox_irq(int irq, void *p)
> > +{
> > +       struct ast2700_mbox *mb = p;
> > +       void __iomem *data_reg;
> > +       int num_words;
> > +       u32 *word_data;
> > +       u32 status;
> > +       int n;
> > +
> > +       /* Only examine channels that are currently enabled. */
> > +       status = readl(mb->regs + IPCR_RX_ENABLE) &
> > +                readl(mb->regs + IPCR_RX_STATUS);
> > +
> > +       if (!(status & RX_IRQ_MASK))
> > +               return IRQ_NONE;
> > +
> > +       for (n = 0; n < mb->mbox.num_chans; ++n) {
> > +               struct mbox_chan *chan = &mb->mbox.chans[n];
> > +
> > +               if (!(status & RX_IRQ(n)))
> > +                       continue;
> > +
> > +               for (data_reg = mb->regs + IPCR_RX_DATA + mb->drv_data->msg_size * n,
> > +                    word_data = chan->con_priv,
> > +                    num_words = (mb->drv_data->msg_size / sizeof(u32));
> > +                    num_words; num_words--, data_reg += sizeof(u32), word_data++)
> > +                       *word_data = readl(data_reg);
> > +
> > +               mbox_chan_received_data(chan, chan->con_priv);
> > +
> > +               /* The IRQ can be cleared only once the FIFO is empty. */
> > +               writel(RX_IRQ(n), mb->regs + IPCR_RX_STATUS);
> > +       }
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static int ast2700_mbox_send_data(struct mbox_chan *chan, void *data)
> > +{
> > +       struct ast2700_mbox *mb = dev_get_drvdata(chan->mbox->dev);
> > +       void __iomem *data_reg;
> > +       u32 *word_data;
> > +       int num_words;
> > +       int idx = ch_num(chan);
> > +
> > +       if (!(readl(mb->regs + IPCR_TX_ENABLE) & BIT(idx))) {
> > +               dev_warn(mb->mbox.dev, "%s: Ch-%d not enabled yet\n", __func__, idx);
> > +               return -EBUSY;
> > +       }
> > +
> > +       if (!(ast2700_mbox_tx_done(mb, idx))) {
> > +               dev_warn(mb->mbox.dev, "%s: Ch-%d last data has not finished\n", __func__, idx);
> > +               return -EBUSY;
> > +       }
> > +
> > +       for (data_reg = mb->regs + IPCR_TX_DATA + mb->drv_data->msg_size * idx,
> > +            num_words = (mb->drv_data->msg_size / sizeof(u32)),
> > +            word_data = (u32 *)data;
> > +            num_words; num_words--, data_reg += sizeof(u32), word_data++)

The readability of this is not great. Can you try to improve it? At
least put each header statement on its own line (at the moment the
condition statement is on the same line as the increment statement).

> > +               writel(*word_data, data_reg);

I'm not super familiar with the mailbox subsystem, but I feel some
commentary on the data size and alignment assumptions would be helpful,
given the APIs are all `void *` without a length parameter.

Should you define a type for clients to submit?

> > +
> > +       writel(BIT(idx), mb->regs + IPCR_TX_TRIG);
> > +       dev_dbg(mb->mbox.dev, "%s: Ch-%d sent\n", __func__, idx);
> > +
> > +       return 0;
> > +}
> > +
> > +static int ast2700_mbox_startup(struct mbox_chan *chan)
> > +{
> > +       struct ast2700_mbox *mb = dev_get_drvdata(chan->mbox->dev);
> > +       int idx = ch_num(chan);
> > +       void __iomem *reg = mb->regs + IPCR_RX_ENABLE;
> > +
> > +       writel_relaxed(readl_relaxed(reg) | BIT(idx), reg);
> > +
> > +       return 0;
> > +}
> > +
> > +static void ast2700_mbox_shutdown(struct mbox_chan *chan)
> > +{
> > +       struct ast2700_mbox *mb = dev_get_drvdata(chan->mbox->dev);
> > +       int idx = ch_num(chan);
> > +       void __iomem *reg = mb->regs + IPCR_RX_ENABLE;
> > +
> > +       writel_relaxed(readl_relaxed(reg) & ~BIT(idx), reg);

Why are we using relaxed operations for startup and shutdown? If this
is valid a comment would be helpful.

> > +}
> > +
> > +static bool ast2700_mbox_last_tx_done(struct mbox_chan *chan)
> > +{
> > +       struct ast2700_mbox *mb = dev_get_drvdata(chan->mbox->dev);
> > +       int idx = ch_num(chan);
> > +
> > +       return ast2700_mbox_tx_done(mb, idx) ? true : false;
> > +}
> > +
> > +static const struct mbox_chan_ops ast2700_mbox_chan_ops = {
> > +       .send_data      = ast2700_mbox_send_data,
> > +       .startup        = ast2700_mbox_startup,
> > +       .shutdown       = ast2700_mbox_shutdown,
> > +       .last_tx_done   = ast2700_mbox_last_tx_done,
> > +};
> > +
> > +static int ast2700_mbox_probe(struct platform_device *pdev)
> > +{
> > +       struct ast2700_mbox *mb;
> > +       const struct ast2700_mbox_data *drv_data;
> > +       struct device *dev = &pdev->dev;
> > +       int irq, ret;
> > +
> > +       if (!pdev->dev.of_node)
> > +               return -ENODEV;
> > +
> > +       drv_data = (const struct ast2700_mbox_data *)device_get_match_data(&pdev->dev);

There's no need for the cast here, device_get_match_data() returns
`const void *`.

> > +
> > +       mb = devm_kzalloc(dev, sizeof(*mb), GFP_KERNEL);
> > +       if (!mb)
> > +               return -ENOMEM;
> > +
> > +       mb->mbox.chans = devm_kcalloc(&pdev->dev, drv_data->num_chans,
> > +                                     sizeof(*mb->mbox.chans), GFP_KERNEL);
> > +       if (!mb->mbox.chans)
> > +               return -ENOMEM;
> > +
> > +       for (int i = 0; i < drv_data->num_chans; i++) {
> > +               mb->mbox.chans[i].con_priv = devm_kcalloc(dev, drv_data->msg_size,
> > +                                                         sizeof(u8), GFP_KERNEL);
> > +               if (!mb->mbox.chans[i].con_priv)
> > +                       return -ENOMEM;
> > +       }
> > +
> > +       platform_set_drvdata(pdev, mb);
> > +
> > +       mb->regs = devm_platform_ioremap_resource(pdev, 0);
> > +       if (IS_ERR(mb->regs))
> > +               return PTR_ERR(mb->regs);

Just checking the controller doesn't require any clock or reset
configuration before we access it?

Andrew

RE: [PATCH v3 2/2] mailbox: aspeed: add mailbox driver for AST27XX series SoC

[Jammy Huang]

Hello Andrew,

You can find it in chapter of ast2700 datasheet below.
  III Function Registers -> 12 Inter Processors Communication (IPC)

Regards,
Jammy Huang

> 
> Hi Jammy,
> 
> As far as I can tell this controller isn't documented in the datasheet for the
> AST2700. Can you point me to the right place? Or, can we get the
> documentation updated?
> 
> On Tue, 2025-06-10 at 17:10 +0800, Jammy Huang wrote:
> > > Add mailbox controller driver for AST27XX SoCs, which provides
> > > independent tx/rx mailbox between different processors. There are 4
> > > channels for each tx/rx mailbox and each channel has an 32-byte FIFO.
> > >
> > > Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
> > > ---
> > >  drivers/mailbox/Kconfig           |   8 ++
> > >  drivers/mailbox/Makefile          |   2 +
> > >  drivers/mailbox/ast2700-mailbox.c | 226
> > > ++++++++++++++++++++++++++++++
> > >  3 files changed, 236 insertions(+)
> > >  create mode 100644 drivers/mailbox/ast2700-mailbox.c
> > >
> > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > 68eeed660a4a..1c38cd570091 100644
> > > --- a/drivers/mailbox/Kconfig
> > > +++ b/drivers/mailbox/Kconfig
> > > @@ -340,4 +340,12 @@ config THEAD_TH1520_MBOX
> > >           kernel is running, and E902 core used for power
> management
> > > among other
> > >           things.
> > >
> > > +config AST2700_MBOX
> > > +       tristate "ASPEED AST2700 IPC driver"
> > > +       depends on ARCH_ASPEED || COMPILE_TEST
> > > +       help
> > > +         Mailbox driver implementation for ASPEED AST27XX SoCs.
> > > +This driver
> > > +         can be used to send message between different processors
> in SoC.
> > > +         The driver provides mailbox support for sending interrupts
> > > +to the
> > > +         clients. Say Y here if you want to build this driver.
> > >  endif
> > > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > > index 13a3448b3271..9a9add9a7548 100644
> > > --- a/drivers/mailbox/Makefile
> > > +++ b/drivers/mailbox/Makefile
> > > @@ -72,3 +72,5 @@ obj-$(CONFIG_QCOM_CPUCP_MBOX) +=
> qcom-cpucp-mbox.o
> > >  obj-$(CONFIG_QCOM_IPCC)                += qcom-ipcc.o
> > >
> > >  obj-$(CONFIG_THEAD_TH1520_MBOX)        += mailbox-th1520.o
> > > +
> > > +obj-$(CONFIG_AST2700_MBOX)     += ast2700-mailbox.o
> > > diff --git a/drivers/mailbox/ast2700-mailbox.c
> > > b/drivers/mailbox/ast2700-mailbox.c
> > > new file mode 100644
> > > index 000000000000..0ee10bd3a6e1
> > > --- /dev/null
> > > +++ b/drivers/mailbox/ast2700-mailbox.c
> > > @@ -0,0 +1,226 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright Aspeed Technology Inc. (C) 2025. All rights reserved
> > > +*/
> > > +
> > > +#include <linux/interrupt.h>
> > > +#include <linux/io.h>
> > > +#include <linux/iopoll.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/mailbox_controller.h> #include <linux/module.h>
> > > +#include <linux/of.h> #include <linux/platform_device.h> #include
> > > +<linux/slab.h>
> > > +
> > > +/* Each bit in the register represents an IPC ID */ #define
> > > +IPCR_TX_TRIG           0x00 #define
> IPCR_TX_ENABLE         0x04
> > > +#define IPCR_RX_ENABLE         0x104 #define
> IPCR_TX_STATUS
> > > +0x08 #define IPCR_RX_STATUS         0x108
> #define  RX_IRQ(n)
> > > +BIT(0 + 1 * (n)) #define  RX_IRQ_MASK           0xf #define
> > > +IPCR_TX_DATA           0x10 #define
> IPCR_RX_DATA           0x110
> > > +
> > > +struct ast2700_mbox_data {
> > > +       u8 num_chans;
> > > +       u8 msg_size;
> > > +};
> > > +
> > > +struct ast2700_mbox {
> > > +       struct mbox_controller mbox;
> > > +       const struct ast2700_mbox_data *drv_data;
> > > +       void __iomem *regs;
> > > +       u32 *rx_buff;
> > > +};
> > > +
> > > +static inline int ch_num(struct mbox_chan *chan) {
> > > +       return chan - chan->mbox->chans; }
> > > +
> > > +static inline int ast2700_mbox_tx_done(struct ast2700_mbox *mb, int
> > > +idx) {
> > > +       return !(readl(mb->regs + IPCR_TX_STATUS) & BIT(idx)); }
> > > +
> > > +static irqreturn_t ast2700_mbox_irq(int irq, void *p) {
> > > +       struct ast2700_mbox *mb = p;
> > > +       void __iomem *data_reg;
> > > +       int num_words;
> > > +       u32 *word_data;
> > > +       u32 status;
> > > +       int n;
> > > +
> > > +       /* Only examine channels that are currently enabled. */
> > > +       status = readl(mb->regs + IPCR_RX_ENABLE) &
> > > +                readl(mb->regs + IPCR_RX_STATUS);
> > > +
> > > +       if (!(status & RX_IRQ_MASK))
> > > +               return IRQ_NONE;
> > > +
> > > +       for (n = 0; n < mb->mbox.num_chans; ++n) {
> > > +               struct mbox_chan *chan = &mb->mbox.chans[n];
> > > +
> > > +               if (!(status & RX_IRQ(n)))
> > > +                       continue;
> > > +
> > > +               for (data_reg = mb->regs + IPCR_RX_DATA +
> > > +mb->drv_data->msg_size * n,
> > > +                    word_data = chan->con_priv,
> > > +                    num_words = (mb->drv_data->msg_size /
> > > +sizeof(u32));
> > > +                    num_words; num_words--, data_reg +=
> > > +sizeof(u32), word_data++)
> > > +                       *word_data = readl(data_reg);
> > > +
> > > +               mbox_chan_received_data(chan, chan->con_priv);
> > > +
> > > +               /* The IRQ can be cleared only once the FIFO is
> > > +empty. */
> > > +               writel(RX_IRQ(n), mb->regs + IPCR_RX_STATUS);
> > > +       }
> > > +
> > > +       return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int ast2700_mbox_send_data(struct mbox_chan *chan, void
> > > +*data) {
> > > +       struct ast2700_mbox *mb =
> dev_get_drvdata(chan->mbox->dev);
> > > +       void __iomem *data_reg;
> > > +       u32 *word_data;
> > > +       int num_words;
> > > +       int idx = ch_num(chan);
> > > +
> > > +       if (!(readl(mb->regs + IPCR_TX_ENABLE) & BIT(idx))) {
> > > +               dev_warn(mb->mbox.dev, "%s: Ch-%d not enabled
> > > +yet\n", __func__, idx);
> > > +               return -EBUSY;
> > > +       }
> > > +
> > > +       if (!(ast2700_mbox_tx_done(mb, idx))) {
> > > +               dev_warn(mb->mbox.dev, "%s: Ch-%d last data has
> not
> > > +finished\n", __func__, idx);
> > > +               return -EBUSY;
> > > +       }
> > > +
> > > +       for (data_reg = mb->regs + IPCR_TX_DATA +
> > > +mb->drv_data->msg_size * idx,
> > > +            num_words = (mb->drv_data->msg_size / sizeof(u32)),
> > > +            word_data = (u32 *)data;
> > > +            num_words; num_words--, data_reg += sizeof(u32),
> > > +word_data++)
> 
> The readability of this is not great. Can you try to improve it? At least put each
> header statement on its own line (at the moment the condition statement is on
> the same line as the increment statement).
> 
> > > +               writel(*word_data, data_reg);
> 
> I'm not super familiar with the mailbox subsystem, but I feel some
> commentary on the data size and alignment assumptions would be helpful,
> given the APIs are all `void *` without a length parameter.
> 
> Should you define a type for clients to submit?
> 
> > > +
> > > +       writel(BIT(idx), mb->regs + IPCR_TX_TRIG);
> > > +       dev_dbg(mb->mbox.dev, "%s: Ch-%d sent\n", __func__, idx);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int ast2700_mbox_startup(struct mbox_chan *chan) {
> > > +       struct ast2700_mbox *mb =
> dev_get_drvdata(chan->mbox->dev);
> > > +       int idx = ch_num(chan);
> > > +       void __iomem *reg = mb->regs + IPCR_RX_ENABLE;
> > > +
> > > +       writel_relaxed(readl_relaxed(reg) | BIT(idx), reg);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static void ast2700_mbox_shutdown(struct mbox_chan *chan) {
> > > +       struct ast2700_mbox *mb =
> dev_get_drvdata(chan->mbox->dev);
> > > +       int idx = ch_num(chan);
> > > +       void __iomem *reg = mb->regs + IPCR_RX_ENABLE;
> > > +
> > > +       writel_relaxed(readl_relaxed(reg) & ~BIT(idx), reg);
> 
> Why are we using relaxed operations for startup and shutdown? If this is valid
> a comment would be helpful.
> 
> > > +}
> > > +
> > > +static bool ast2700_mbox_last_tx_done(struct mbox_chan *chan) {
> > > +       struct ast2700_mbox *mb =
> dev_get_drvdata(chan->mbox->dev);
> > > +       int idx = ch_num(chan);
> > > +
> > > +       return ast2700_mbox_tx_done(mb, idx) ? true : false; }
> > > +
> > > +static const struct mbox_chan_ops ast2700_mbox_chan_ops = {
> > > +       .send_data      = ast2700_mbox_send_data,
> > > +       .startup        = ast2700_mbox_startup,
> > > +       .shutdown       = ast2700_mbox_shutdown,
> > > +       .last_tx_done   = ast2700_mbox_last_tx_done, };
> > > +
> > > +static int ast2700_mbox_probe(struct platform_device *pdev) {
> > > +       struct ast2700_mbox *mb;
> > > +       const struct ast2700_mbox_data *drv_data;
> > > +       struct device *dev = &pdev->dev;
> > > +       int irq, ret;
> > > +
> > > +       if (!pdev->dev.of_node)
> > > +               return -ENODEV;
> > > +
> > > +       drv_data = (const struct ast2700_mbox_data
> > > +*)device_get_match_data(&pdev->dev);
> 
> There's no need for the cast here, device_get_match_data() returns `const void
> *`.
> 
> > > +
> > > +       mb = devm_kzalloc(dev, sizeof(*mb), GFP_KERNEL);
> > > +       if (!mb)
> > > +               return -ENOMEM;
> > > +
> > > +       mb->mbox.chans = devm_kcalloc(&pdev->dev,
> > > +drv_data->num_chans,
> > >
> +                                     sizeof(*mb->mbox.c
> hans),
> > > +GFP_KERNEL);
> > > +       if (!mb->mbox.chans)
> > > +               return -ENOMEM;
> > > +
> > > +       for (int i = 0; i < drv_data->num_chans; i++) {
> > > +               mb->mbox.chans[i].con_priv = devm_kcalloc(dev,
> > > +drv_data->msg_size,
> > >
> +
> 
> > > +sizeof(u8), GFP_KERNEL);
> > > +               if (!mb->mbox.chans[i].con_priv)
> > > +                       return -ENOMEM;
> > > +       }
> > > +
> > > +       platform_set_drvdata(pdev, mb);
> > > +
> > > +       mb->regs = devm_platform_ioremap_resource(pdev, 0);
> > > +       if (IS_ERR(mb->regs))
> > > +               return PTR_ERR(mb->regs);
> 
> Just checking the controller doesn't require any clock or reset configuration
> before we access it?
> 
> Andrew

Re: [PATCH v3 2/2] mailbox: aspeed: add mailbox driver for AST27XX series SoC

[Andrew Jeffery]

On Fri, 2025-06-13 at 00:51 +0000, Jammy Huang wrote:
> Hello Andrew,
> 
> You can find it in chapter of ast2700 datasheet below.
>   III Function Registers -> 12 Inter Processors Communication (IPC)

Great, thanks. The description in the memory space allocation table
doesn't match the chapter heading, which is what tripped me up
("InterProcessor Controller" vs "Inter Processors Communication").

Andrew

RE: [PATCH v3 2/2] mailbox: aspeed: add mailbox driver for AST27XX series SoC

[Jammy Huang]

> 
> On Fri, 2025-06-13 at 00:51 +0000, Jammy Huang wrote:
> > Hello Andrew,
> >
> > You can find it in chapter of ast2700 datasheet below.
> >   III Function Registers -> 12 Inter Processors Communication (IPC)
> 
> Great, thanks. The description in the memory space allocation table doesn't
> match the chapter heading, which is what tripped me up ("InterProcessor
> Controller" vs "Inter Processors Communication").
> 
> Andrew

Thanks for your reminder. We will correct it in new datasheet.

Regards,
Jammy Huang

RE: [PATCH v3 2/2] mailbox: aspeed: add mailbox driver for AST27XX series SoC

[Jammy Huang]

Hi Andres,

Thanks for your review. I forget to reply some of your comments.

Best Regards
Jammy

> 
> Hi Jammy,
> 
> As far as I can tell this controller isn't documented in the datasheet for the
> AST2700. Can you point me to the right place? Or, can we get the
> documentation updated?
> 
> On Tue, 2025-06-10 at 17:10 +0800, Jammy Huang wrote:
> > > Add mailbox controller driver for AST27XX SoCs, which provides
> > > independent tx/rx mailbox between different processors. There are 4
> > > channels for each tx/rx mailbox and each channel has an 32-byte FIFO.
> > >
> > > Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
> > > ---
> > >  drivers/mailbox/Kconfig           |   8 ++
> > >  drivers/mailbox/Makefile          |   2 +
> > >  drivers/mailbox/ast2700-mailbox.c | 226
> > > ++++++++++++++++++++++++++++++
> > >  3 files changed, 236 insertions(+)
> > >  create mode 100644 drivers/mailbox/ast2700-mailbox.c
> > >
> > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > 68eeed660a4a..1c38cd570091 100644
> > > --- a/drivers/mailbox/Kconfig
> > > +++ b/drivers/mailbox/Kconfig
> > > @@ -340,4 +340,12 @@ config THEAD_TH1520_MBOX
> > >           kernel is running, and E902 core used for power
> management
> > > among other
> > >           things.
> > >
> > > +config AST2700_MBOX
> > > +       tristate "ASPEED AST2700 IPC driver"
> > > +       depends on ARCH_ASPEED || COMPILE_TEST
> > > +       help
> > > +         Mailbox driver implementation for ASPEED AST27XX SoCs.
> > > +This driver
> > > +         can be used to send message between different processors
> in SoC.
> > > +         The driver provides mailbox support for sending interrupts
> > > +to the
> > > +         clients. Say Y here if you want to build this driver.
> > >  endif
> > > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > > index 13a3448b3271..9a9add9a7548 100644
> > > --- a/drivers/mailbox/Makefile
> > > +++ b/drivers/mailbox/Makefile
> > > @@ -72,3 +72,5 @@ obj-$(CONFIG_QCOM_CPUCP_MBOX) +=
> qcom-cpucp-mbox.o
> > >  obj-$(CONFIG_QCOM_IPCC)                += qcom-ipcc.o
> > >
> > >  obj-$(CONFIG_THEAD_TH1520_MBOX)        += mailbox-th1520.o
> > > +
> > > +obj-$(CONFIG_AST2700_MBOX)     += ast2700-mailbox.o
> > > diff --git a/drivers/mailbox/ast2700-mailbox.c
> > > b/drivers/mailbox/ast2700-mailbox.c
> > > new file mode 100644
> > > index 000000000000..0ee10bd3a6e1
> > > --- /dev/null
> > > +++ b/drivers/mailbox/ast2700-mailbox.c
> > > @@ -0,0 +1,226 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright Aspeed Technology Inc. (C) 2025. All rights reserved
> > > +*/
> > > +
> > > +#include <linux/interrupt.h>
> > > +#include <linux/io.h>
> > > +#include <linux/iopoll.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/mailbox_controller.h> #include <linux/module.h>
> > > +#include <linux/of.h> #include <linux/platform_device.h> #include
> > > +<linux/slab.h>
> > > +
> > > +/* Each bit in the register represents an IPC ID */ #define
> > > +IPCR_TX_TRIG           0x00 #define
> IPCR_TX_ENABLE         0x04
> > > +#define IPCR_RX_ENABLE         0x104 #define
> IPCR_TX_STATUS
> > > +0x08 #define IPCR_RX_STATUS         0x108
> #define  RX_IRQ(n)
> > > +BIT(0 + 1 * (n)) #define  RX_IRQ_MASK           0xf #define
> > > +IPCR_TX_DATA           0x10 #define
> IPCR_RX_DATA           0x110
> > > +
> > > +struct ast2700_mbox_data {
> > > +       u8 num_chans;
> > > +       u8 msg_size;
> > > +};
> > > +
> > > +struct ast2700_mbox {
> > > +       struct mbox_controller mbox;
> > > +       const struct ast2700_mbox_data *drv_data;
> > > +       void __iomem *regs;
> > > +       u32 *rx_buff;
> > > +};
> > > +
> > > +static inline int ch_num(struct mbox_chan *chan) {
> > > +       return chan - chan->mbox->chans; }
> > > +
> > > +static inline int ast2700_mbox_tx_done(struct ast2700_mbox *mb, int
> > > +idx) {
> > > +       return !(readl(mb->regs + IPCR_TX_STATUS) & BIT(idx)); }
> > > +
> > > +static irqreturn_t ast2700_mbox_irq(int irq, void *p) {
> > > +       struct ast2700_mbox *mb = p;
> > > +       void __iomem *data_reg;
> > > +       int num_words;
> > > +       u32 *word_data;
> > > +       u32 status;
> > > +       int n;
> > > +
> > > +       /* Only examine channels that are currently enabled. */
> > > +       status = readl(mb->regs + IPCR_RX_ENABLE) &
> > > +                readl(mb->regs + IPCR_RX_STATUS);
> > > +
> > > +       if (!(status & RX_IRQ_MASK))
> > > +               return IRQ_NONE;
> > > +
> > > +       for (n = 0; n < mb->mbox.num_chans; ++n) {
> > > +               struct mbox_chan *chan = &mb->mbox.chans[n];
> > > +
> > > +               if (!(status & RX_IRQ(n)))
> > > +                       continue;
> > > +
> > > +               for (data_reg = mb->regs + IPCR_RX_DATA +
> > > +mb->drv_data->msg_size * n,
> > > +                    word_data = chan->con_priv,
> > > +                    num_words = (mb->drv_data->msg_size /
> > > +sizeof(u32));
> > > +                    num_words; num_words--, data_reg +=
> > > +sizeof(u32), word_data++)
> > > +                       *word_data = readl(data_reg);
> > > +
> > > +               mbox_chan_received_data(chan, chan->con_priv);
> > > +
> > > +               /* The IRQ can be cleared only once the FIFO is
> > > +empty. */
> > > +               writel(RX_IRQ(n), mb->regs + IPCR_RX_STATUS);
> > > +       }
> > > +
> > > +       return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int ast2700_mbox_send_data(struct mbox_chan *chan, void
> > > +*data) {
> > > +       struct ast2700_mbox *mb =
> dev_get_drvdata(chan->mbox->dev);
> > > +       void __iomem *data_reg;
> > > +       u32 *word_data;
> > > +       int num_words;
> > > +       int idx = ch_num(chan);
> > > +
> > > +       if (!(readl(mb->regs + IPCR_TX_ENABLE) & BIT(idx))) {
> > > +               dev_warn(mb->mbox.dev, "%s: Ch-%d not enabled
> > > +yet\n", __func__, idx);
> > > +               return -EBUSY;
> > > +       }
> > > +
> > > +       if (!(ast2700_mbox_tx_done(mb, idx))) {
> > > +               dev_warn(mb->mbox.dev, "%s: Ch-%d last data has
> not
> > > +finished\n", __func__, idx);
> > > +               return -EBUSY;
> > > +       }
> > > +
> > > +       for (data_reg = mb->regs + IPCR_TX_DATA +
> > > +mb->drv_data->msg_size * idx,
> > > +            num_words = (mb->drv_data->msg_size / sizeof(u32)),
> > > +            word_data = (u32 *)data;
> > > +            num_words; num_words--, data_reg += sizeof(u32),
> > > +word_data++)
> 
> The readability of this is not great. Can you try to improve it? At least put each
> header statement on its own line (at the moment the condition statement is on
> the same line as the increment statement).
> 
Agreed.

> > > +               writel(*word_data, data_reg);
> 
> I'm not super familiar with the mailbox subsystem, but I feel some
> commentary on the data size and alignment assumptions would be helpful,
> given the APIs are all `void *` without a length parameter.
> 
> Should you define a type for clients to submit?
> 
In mailbox's framework, there will be a mailbox-client who truly define how to use
and what to send. So, I just leave this part for the mailbox-client to define.
There are some samples in drimvers/firmware for your reference.

> > > +
> > > +       writel(BIT(idx), mb->regs + IPCR_TX_TRIG);
> > > +       dev_dbg(mb->mbox.dev, "%s: Ch-%d sent\n", __func__, idx);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int ast2700_mbox_startup(struct mbox_chan *chan) {
> > > +       struct ast2700_mbox *mb =
> dev_get_drvdata(chan->mbox->dev);
> > > +       int idx = ch_num(chan);
> > > +       void __iomem *reg = mb->regs + IPCR_RX_ENABLE;
> > > +
> > > +       writel_relaxed(readl_relaxed(reg) | BIT(idx), reg);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static void ast2700_mbox_shutdown(struct mbox_chan *chan) {
> > > +       struct ast2700_mbox *mb =
> dev_get_drvdata(chan->mbox->dev);
> > > +       int idx = ch_num(chan);
> > > +       void __iomem *reg = mb->regs + IPCR_RX_ENABLE;
> > > +
> > > +       writel_relaxed(readl_relaxed(reg) & ~BIT(idx), reg);
> 
> Why are we using relaxed operations for startup and shutdown? If this is valid
> a comment would be helpful.
> 
I will remove relaxed op in next patch.

> > > +}
> > > +
> > > +static bool ast2700_mbox_last_tx_done(struct mbox_chan *chan) {
> > > +       struct ast2700_mbox *mb =
> dev_get_drvdata(chan->mbox->dev);
> > > +       int idx = ch_num(chan);
> > > +
> > > +       return ast2700_mbox_tx_done(mb, idx) ? true : false; }
> > > +
> > > +static const struct mbox_chan_ops ast2700_mbox_chan_ops = {
> > > +       .send_data      = ast2700_mbox_send_data,
> > > +       .startup        = ast2700_mbox_startup,
> > > +       .shutdown       = ast2700_mbox_shutdown,
> > > +       .last_tx_done   = ast2700_mbox_last_tx_done, };
> > > +
> > > +static int ast2700_mbox_probe(struct platform_device *pdev) {
> > > +       struct ast2700_mbox *mb;
> > > +       const struct ast2700_mbox_data *drv_data;
> > > +       struct device *dev = &pdev->dev;
> > > +       int irq, ret;
> > > +
> > > +       if (!pdev->dev.of_node)
> > > +               return -ENODEV;
> > > +
> > > +       drv_data = (const struct ast2700_mbox_data
> > > +*)device_get_match_data(&pdev->dev);
> 
> There's no need for the cast here, device_get_match_data() returns `const void
> *`.
> 
Agreed.

> > > +
> > > +       mb = devm_kzalloc(dev, sizeof(*mb), GFP_KERNEL);
> > > +       if (!mb)
> > > +               return -ENOMEM;
> > > +
> > > +       mb->mbox.chans = devm_kcalloc(&pdev->dev,
> > > +drv_data->num_chans,
> > >
> +                                     sizeof(*mb->mbox.c
> hans),
> > > +GFP_KERNEL);
> > > +       if (!mb->mbox.chans)
> > > +               return -ENOMEM;
> > > +
> > > +       for (int i = 0; i < drv_data->num_chans; i++) {
> > > +               mb->mbox.chans[i].con_priv = devm_kcalloc(dev,
> > > +drv_data->msg_size,
> > >
> +
> 
> > > +sizeof(u8), GFP_KERNEL);
> > > +               if (!mb->mbox.chans[i].con_priv)
> > > +                       return -ENOMEM;
> > > +       }
> > > +
> > > +       platform_set_drvdata(pdev, mb);
> > > +
> > > +       mb->regs = devm_platform_ioremap_resource(pdev, 0);
> > > +       if (IS_ERR(mb->regs))
> > > +               return PTR_ERR(mb->regs);
> 
> Just checking the controller doesn't require any clock or reset configuration
> before we access it?
Yes, clk or reset is not required.


> 
> Andrew