[PATCH v4 0/3] Introduce support for T-head TH1520 Mailbox

[PATCH v4 0/3] Introduce support for T-head TH1520 Mailbox

[Michal Wilczynski]

The T-head TH1520 SoC supports a hardware mailbox that enables two cores
within the SoC to communicate and coordinate [1]. One example of such
coordination would be cooperation with the T-Head E902 core, which is
responsible for power, clock, and resource management. For example, in
the specific case of the BXM-4-64 GPU, it needs to be powered on by the
E902 core, and the kernel running on the E910 needs to 'ask' the
firmware running on the E902 core to enable power to the GPU island.
Given recent advancements in work on the upstream GPU driver [2], there
is an emerging need to get this code in the mainline kernel.

Link: https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf [1]
Link: https://gitlab.freedesktop.org/imagination/linux-firmware/-/issues/1 [2]

Thanks, Krzysztof and Rob, for your review! Since this series is gaining
some interest, I've dropped the RFC prefix with the v3 update.

v4:
- fixed warning of unused variable
- added Reviewed-by from Krzysztof
- fixed minor cosmetic issues in dt-binding

v3:
- added a comment about mixing devm_ and non-devm resources in the context
  of shared interrupts and explained why it's safe to do so in this
  particular case
- changed the order of resource freeing in the .shutdown callback
- used a wrapper function for register mapping
- since the only conceivable use case for this mailbox driver is
  communication with cores not managed by the kernel, I’ve hard-coded
  this by removing the thead,icu-cpu-id property and adjusted the mailbox
  driver code accordingly.
- added a more detailed description for mbox-cells.
- made some cosmetic changes.
- retested by applying the patch with non-yet-upstreamed patches,
  confirming that the drm/imagination driver can read the registers
  correctly.

v2:
 - fixed thead,th1520-mbox.yaml binding file by dropping redundant
   descriptions, renaming reg-names, removing unnecessary clocks,
   providing constraints and defining ICU's
 - fixed the mailbox driver code to work well with updated binding-file,
   removed clocks support, as it's not necessary for mailbox to work
 - adjusted the device tree node instance of mbox_910t so it will work
   with updated bindings file

Michal Wilczynski (3):
  mailbox: Introduce support for T-head TH1520 Mailbox driver
  dt-bindings: mailbox: Add thead,th1520-mailbox bindings
  riscv: dts: thead: Add mailbox node

 .../bindings/mailbox/thead,th1520-mbox.yaml   |  80 +++
 MAINTAINERS                                   |   2 +
 arch/riscv/boot/dts/thead/th1520.dtsi         |  12 +
 drivers/mailbox/Kconfig                       |  10 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/mailbox-th1520.c              | 572 ++++++++++++++++++
 6 files changed, 678 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
 create mode 100644 drivers/mailbox/mailbox-th1520.c

-- 
2.34.1

[PATCH v4 1/3] mailbox: Introduce support for T-head TH1520 Mailbox driver

[Michal Wilczynski]

This driver was tested using the drm/imagination GPU driver. It was able
to successfully power on the GPU, by passing a command through mailbox
from E910 core to E902 that's responsible for powering up the GPU. The
GPU driver was able to read the BVC version from control registers,
which confirms it was successfully powered on.

[   33.957467] powervr ffef400000.gpu: [drm] loaded firmware
powervr/rogue_36.52.104.182_v1.fw
[   33.966008] powervr ffef400000.gpu: [drm] FW version v1.0 (build
6621747 OS)
[   38.978542] powervr ffef400000.gpu: [drm] *ERROR* Firmware failed to
boot

Though the driver still fails to boot the firmware, the mailbox driver
works when used with the not-yet-upstreamed firmware AON driver. There
is ongoing work to get the BXM-4-64 supported with the drm/imagination
driver [1], though it's not completed yet.

This work is based on the driver from the vendor kernel [2].

Link: https://gitlab.freedesktop.org/imagination/linux-firmware/-/issues/2 [1]
Link: https://github.com/revyos/thead-kernel.git [2]

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 MAINTAINERS                      |   1 +
 drivers/mailbox/Kconfig          |  10 +
 drivers/mailbox/Makefile         |   2 +
 drivers/mailbox/mailbox-th1520.c | 572 +++++++++++++++++++++++++++++++
 4 files changed, 585 insertions(+)
 create mode 100644 drivers/mailbox/mailbox-th1520.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7ad507f49324..0655c6ba5435 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19953,6 +19953,7 @@ T:	git https://github.com/pdp7/linux.git
 F:	Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
 F:	arch/riscv/boot/dts/thead/
 F:	drivers/clk/thead/clk-th1520-ap.c
+F:	drivers/mailbox/mailbox-th1520.c
 F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
 
 RNBD BLOCK DRIVERS
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 6fb995778636..52f8162896f5 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -295,4 +295,14 @@ config QCOM_IPCC
 	  acts as an interrupt controller for receiving interrupts from clients.
 	  Say Y here if you want to build this driver.
 
+config THEAD_TH1520_MBOX
+	tristate "T-head TH1520 Mailbox"
+	depends on ARCH_THEAD || COMPILE_TEST
+	help
+	  Mailbox driver implementation for the Thead TH-1520 platform. Enables
+	  two cores within the SoC to communicate and coordinate by passing
+	  messages. Could be used to communicate between E910 core, on which the
+	  kernel is running, and E902 core used for power management among other
+	  things.
+
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 3c3c27d54c13..5f4f5b0ce2cc 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -64,3 +64,5 @@ obj-$(CONFIG_SPRD_MBOX)		+= sprd-mailbox.o
 obj-$(CONFIG_QCOM_CPUCP_MBOX)	+= qcom-cpucp-mbox.o
 
 obj-$(CONFIG_QCOM_IPCC)		+= qcom-ipcc.o
+
+obj-$(CONFIG_THEAD_TH1520_MBOX)	+= mailbox-th1520.o
diff --git a/drivers/mailbox/mailbox-th1520.c b/drivers/mailbox/mailbox-th1520.c
new file mode 100644
index 000000000000..bf537c4e01a2
--- /dev/null
+++ b/drivers/mailbox/mailbox-th1520.c
@@ -0,0 +1,572 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021 Alibaba Group Holding Limited.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/* Status Register */
+#define TH_1520_MBOX_STA 0x0
+#define TH_1520_MBOX_CLR 0x4
+#define TH_1520_MBOX_MASK 0xc
+
+/* Transmit/receive data register:
+ * INFO0 ~ INFO6
+ */
+#define TH_1520_MBOX_INFO_NUM 8
+#define TH_1520_MBOX_DATA_INFO_NUM 7
+#define TH_1520_MBOX_INFO0 0x14
+/* Transmit ack register: INFO7 */
+#define TH_1520_MBOX_INFO7 0x30
+
+/* Generate remote icu IRQ Register */
+#define TH_1520_MBOX_GEN 0x10
+#define TH_1520_MBOX_GEN_RX_DATA BIT(6)
+#define TH_1520_MBOX_GEN_TX_ACK BIT(7)
+
+#define TH_1520_MBOX_CHAN_RES_SIZE 0x1000
+#define TH_1520_MBOX_CHANS 4
+#define TH_1520_MBOX_CHAN_NAME_SIZE 20
+
+#define TH_1520_MBOX_ACK_MAGIC 0xdeadbeaf
+
+#ifdef CONFIG_PM_SLEEP
+/* store MBOX context across system-wide suspend/resume transitions */
+struct th1520_mbox_context {
+	u32 intr_mask[TH_1520_MBOX_CHANS - 1];
+};
+#endif
+
+enum th1520_mbox_chan_type {
+	TH_1520_MBOX_TYPE_TXRX, /* Tx & Rx chan */
+	TH_1520_MBOX_TYPE_DB, /* Tx & Rx doorbell */
+};
+
+enum th1520_mbox_icu_cpu_id {
+	TH_1520_MBOX_ICU_KERNEL_CPU0, /* 910T */
+	TH_1520_MBOX_ICU_CPU1, /* 902 */
+	TH_1520_MBOX_ICU_CPU2, /* 906 */
+	TH_1520_MBOX_ICU_CPU3, /* 910R */
+};
+
+struct th1520_mbox_con_priv {
+	enum th1520_mbox_icu_cpu_id idx;
+	enum th1520_mbox_chan_type type;
+	void __iomem *comm_local_base;
+	void __iomem *comm_remote_base;
+	char irq_desc[TH_1520_MBOX_CHAN_NAME_SIZE];
+	struct mbox_chan *chan;
+	struct tasklet_struct txdb_tasklet;
+};
+
+struct th1520_mbox_priv {
+	struct device *dev;
+	void __iomem *local_icu[TH_1520_MBOX_CHANS];
+	void __iomem *remote_icu[TH_1520_MBOX_CHANS - 1];
+	void __iomem *cur_cpu_ch_base;
+	spinlock_t mbox_lock; /* control register lock */
+
+	struct mbox_controller mbox;
+	struct mbox_chan mbox_chans[TH_1520_MBOX_CHANS];
+
+	struct th1520_mbox_con_priv con_priv[TH_1520_MBOX_CHANS];
+	int irq;
+#ifdef CONFIG_PM_SLEEP
+	struct th1520_mbox_context *ctx;
+#endif
+};
+
+static struct th1520_mbox_priv *
+to_th1520_mbox_priv(struct mbox_controller *mbox)
+{
+	return container_of(mbox, struct th1520_mbox_priv, mbox);
+}
+
+static void th1520_mbox_write(struct th1520_mbox_priv *priv, u32 val, u32 offs)
+{
+	iowrite32(val, priv->cur_cpu_ch_base + offs);
+}
+
+static u32 th1520_mbox_read(struct th1520_mbox_priv *priv, u32 offs)
+{
+	return ioread32(priv->cur_cpu_ch_base + offs);
+}
+
+static u32 th1520_mbox_rmw(struct th1520_mbox_priv *priv, u32 off, u32 set,
+			   u32 clr)
+{
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&priv->mbox_lock, flags);
+	val = th1520_mbox_read(priv, off);
+	val &= ~clr;
+	val |= set;
+	th1520_mbox_write(priv, val, off);
+	spin_unlock_irqrestore(&priv->mbox_lock, flags);
+
+	return val;
+}
+
+static void th1520_mbox_chan_write(struct th1520_mbox_con_priv *cp, u32 val,
+				   u32 offs, bool is_remote)
+{
+	if (is_remote)
+		iowrite32(val, cp->comm_remote_base + offs);
+	else
+		iowrite32(val, cp->comm_local_base + offs);
+}
+
+static u32 th1520_mbox_chan_read(struct th1520_mbox_con_priv *cp, u32 offs,
+				 bool is_remote)
+{
+	if (is_remote)
+		return ioread32(cp->comm_remote_base + offs);
+	else
+		return ioread32(cp->comm_local_base + offs);
+}
+
+static void th1520_mbox_chan_rmw(struct th1520_mbox_con_priv *cp, u32 off,
+				 u32 set, u32 clr, bool is_remote)
+{
+	struct th1520_mbox_priv *priv = to_th1520_mbox_priv(cp->chan->mbox);
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&priv->mbox_lock, flags);
+	val = th1520_mbox_chan_read(cp, off, is_remote);
+	val &= ~clr;
+	val |= set;
+	th1520_mbox_chan_write(cp, val, off, is_remote);
+	spin_unlock_irqrestore(&priv->mbox_lock, flags);
+}
+
+static void th1520_mbox_chan_rd_data(struct th1520_mbox_con_priv *cp,
+				     void *data, bool is_remote)
+{
+	u32 off = TH_1520_MBOX_INFO0;
+	u32 *arg = data;
+	u32 i;
+
+	/* read info0 ~ info6, totally 28 bytes
+	 * requires data memory size is 28 bytes
+	 */
+	for (i = 0; i < TH_1520_MBOX_DATA_INFO_NUM; i++) {
+		*arg = th1520_mbox_chan_read(cp, off, is_remote);
+		off += 4;
+		arg++;
+	}
+}
+
+static void th1520_mbox_chan_wr_data(struct th1520_mbox_con_priv *cp,
+				     void *data, bool is_remote)
+{
+	u32 off = TH_1520_MBOX_INFO0;
+	u32 *arg = data;
+	u32 i;
+
+	/* write info0 ~ info6, totally 28 bytes
+	 * requires data memory is 28 bytes valid data
+	 */
+	for (i = 0; i < TH_1520_MBOX_DATA_INFO_NUM; i++) {
+		th1520_mbox_chan_write(cp, *arg, off, is_remote);
+		off += 4;
+		arg++;
+	}
+}
+
+static void th1520_mbox_chan_wr_ack(struct th1520_mbox_con_priv *cp, void *data,
+				    bool is_remote)
+{
+	u32 off = TH_1520_MBOX_INFO7;
+	u32 *arg = data;
+
+	th1520_mbox_chan_write(cp, *arg, off, is_remote);
+}
+
+static int th1520_mbox_chan_id_to_mapbit(struct th1520_mbox_con_priv *cp)
+{
+	int mapbit = 0;
+	int i;
+
+	for (i = 0; i < TH_1520_MBOX_CHANS; i++) {
+		if (i == cp->idx)
+			return mapbit;
+
+		if (i != TH_1520_MBOX_ICU_KERNEL_CPU0)
+			mapbit++;
+	}
+
+	if (i == TH_1520_MBOX_CHANS)
+		dev_err(cp->chan->mbox->dev, "convert to mapbit failed\n");
+
+	return 0;
+}
+
+static void th1520_mbox_txdb_tasklet(unsigned long data)
+{
+	struct th1520_mbox_con_priv *cp = (struct th1520_mbox_con_priv *)data;
+
+	mbox_chan_txdone(cp->chan, 0);
+}
+
+static irqreturn_t th1520_mbox_isr(int irq, void *p)
+{
+	struct mbox_chan *chan = p;
+	struct th1520_mbox_priv *priv = to_th1520_mbox_priv(chan->mbox);
+	struct th1520_mbox_con_priv *cp = chan->con_priv;
+	int mapbit = th1520_mbox_chan_id_to_mapbit(cp);
+	u32 sta, dat[TH_1520_MBOX_DATA_INFO_NUM];
+	u32 ack_magic = TH_1520_MBOX_ACK_MAGIC;
+	u32 info0_data, info7_data;
+
+	sta = th1520_mbox_read(priv, TH_1520_MBOX_STA);
+	if (!(sta & BIT(mapbit)))
+		return IRQ_NONE;
+
+	/* clear chan irq bit in STA register */
+	th1520_mbox_rmw(priv, TH_1520_MBOX_CLR, BIT(mapbit), 0);
+
+	/* rx doorbell */
+	if (cp->type == TH_1520_MBOX_TYPE_DB) {
+		mbox_chan_received_data(cp->chan, NULL);
+		return IRQ_HANDLED;
+	}
+
+	/* info0 is the protocol word, should not be zero! */
+	info0_data = th1520_mbox_chan_read(cp, TH_1520_MBOX_INFO0, false);
+	if (info0_data) {
+		/* read info0~info6 data */
+		th1520_mbox_chan_rd_data(cp, dat, false);
+
+		/* clear local info0 */
+		th1520_mbox_chan_write(cp, 0x0, TH_1520_MBOX_INFO0, false);
+
+		/* notify remote cpu */
+		th1520_mbox_chan_wr_ack(cp, &ack_magic, true);
+		/* CPU1 902/906 use polling mode to monitor info7 */
+		if (cp->idx != TH_1520_MBOX_ICU_CPU1 &&
+		    cp->idx != TH_1520_MBOX_ICU_CPU2)
+			th1520_mbox_chan_rmw(cp, TH_1520_MBOX_GEN,
+					     TH_1520_MBOX_GEN_TX_ACK, 0, true);
+
+		/* transfer the data to client */
+		mbox_chan_received_data(chan, (void *)dat);
+	}
+
+	/* info7 magic value mean the real ack signal, not generate bit7 */
+	info7_data = th1520_mbox_chan_read(cp, TH_1520_MBOX_INFO7, false);
+	if (info7_data == TH_1520_MBOX_ACK_MAGIC) {
+		/* clear local info7 */
+		th1520_mbox_chan_write(cp, 0x0, TH_1520_MBOX_INFO7, false);
+
+		/* notify framework the last TX has completed */
+		mbox_chan_txdone(chan, 0);
+	}
+
+	if (!info0_data && !info7_data)
+		return IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
+static int th1520_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+	struct th1520_mbox_con_priv *cp = chan->con_priv;
+
+	if (cp->type == TH_1520_MBOX_TYPE_DB)
+		tasklet_schedule(&cp->txdb_tasklet);
+	else
+		th1520_mbox_chan_wr_data(cp, data, true);
+
+	th1520_mbox_chan_rmw(cp, TH_1520_MBOX_GEN, TH_1520_MBOX_GEN_RX_DATA, 0,
+			     true);
+	return 0;
+}
+
+static int th1520_mbox_startup(struct mbox_chan *chan)
+{
+	struct th1520_mbox_priv *priv = to_th1520_mbox_priv(chan->mbox);
+	struct th1520_mbox_con_priv *cp = chan->con_priv;
+	u32 data[8] = {};
+	int mask_bit;
+	int ret;
+
+	/* clear local and remote generate and info0~info7 */
+	th1520_mbox_chan_rmw(cp, TH_1520_MBOX_GEN, 0x0, 0xff, true);
+	th1520_mbox_chan_rmw(cp, TH_1520_MBOX_GEN, 0x0, 0xff, false);
+	th1520_mbox_chan_wr_ack(cp, &data[7], true);
+	th1520_mbox_chan_wr_ack(cp, &data[7], false);
+	th1520_mbox_chan_wr_data(cp, &data[0], true);
+	th1520_mbox_chan_wr_data(cp, &data[0], false);
+
+	/* enable the chan mask */
+	mask_bit = th1520_mbox_chan_id_to_mapbit(cp);
+	th1520_mbox_rmw(priv, TH_1520_MBOX_MASK, BIT(mask_bit), 0);
+
+	if (cp->type == TH_1520_MBOX_TYPE_DB)
+		/* tx doorbell doesn't have ACK, rx doorbell requires isr */
+		tasklet_init(&cp->txdb_tasklet, th1520_mbox_txdb_tasklet,
+			     (unsigned long)cp);
+
+	/*
+	 * Mixing devm_ managed resources with manual IRQ handling is generally
+	 * discouraged due to potential complexities with resource management,
+	 * especially when dealing with shared interrupts. However, in this case,
+	 * the approach is safe and effective because:
+	 *
+	 * 1. Each mailbox channel requests its IRQ within the .startup() callback
+	 *    and frees it within the .shutdown() callback.
+	 * 2. During device unbinding, the devm_ managed mailbox controller first
+	 *    iterates through all channels, ensuring that their IRQs are freed before
+	 *    any other devm_ resources are released.
+	 *
+	 * This ordering guarantees that no interrupts can be triggered from the device
+	 * while it is being unbound, preventing race conditions and ensuring system
+	 * stability.
+	 */
+	ret = request_irq(priv->irq, th1520_mbox_isr,
+			  IRQF_SHARED | IRQF_NO_SUSPEND, cp->irq_desc, chan);
+	if (ret) {
+		dev_err(priv->dev, "Unable to acquire IRQ %d\n", priv->irq);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void th1520_mbox_shutdown(struct mbox_chan *chan)
+{
+	struct th1520_mbox_priv *priv = to_th1520_mbox_priv(chan->mbox);
+	struct th1520_mbox_con_priv *cp = chan->con_priv;
+	int mask_bit;
+
+	free_irq(priv->irq, chan);
+
+	/* clear the chan mask */
+	mask_bit = th1520_mbox_chan_id_to_mapbit(cp);
+	th1520_mbox_rmw(priv, TH_1520_MBOX_MASK, 0, BIT(mask_bit));
+}
+
+static const struct mbox_chan_ops th1520_mbox_ops = {
+	.send_data = th1520_mbox_send_data,
+	.startup = th1520_mbox_startup,
+	.shutdown = th1520_mbox_shutdown,
+};
+
+static int th1520_mbox_init_generic(struct th1520_mbox_priv *priv)
+{
+#ifdef CONFIG_PM_SLEEP
+	priv->ctx = devm_kzalloc(priv->dev, sizeof(*priv->ctx), GFP_KERNEL);
+	if (!priv->ctx)
+		return -ENOMEM;
+#endif
+	/* Set default configuration */
+	th1520_mbox_write(priv, 0xff, TH_1520_MBOX_CLR);
+	th1520_mbox_write(priv, 0x0, TH_1520_MBOX_MASK);
+	return 0;
+}
+
+static struct mbox_chan *th1520_mbox_xlate(struct mbox_controller *mbox,
+					   const struct of_phandle_args *sp)
+{
+	struct th1520_mbox_con_priv *cp;
+	u32 chan, type;
+
+	if (sp->args_count != 2) {
+		dev_err(mbox->dev, "Invalid argument count %d\n",
+			sp->args_count);
+		return ERR_PTR(-EINVAL);
+	}
+
+	chan = sp->args[0]; /* comm remote channel */
+	type = sp->args[1]; /* comm channel type */
+
+	if (chan >= mbox->num_chans) {
+		dev_err(mbox->dev, "Not supported channel number: %d\n", chan);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (chan == TH_1520_MBOX_ICU_KERNEL_CPU0) {
+		dev_err(mbox->dev, "Cannot communicate with yourself\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (type > TH_1520_MBOX_TYPE_DB) {
+		dev_err(mbox->dev, "Not supported the type for channel[%d]\n",
+			chan);
+		return ERR_PTR(-EINVAL);
+	}
+
+	cp = mbox->chans[chan].con_priv;
+	cp->type = type;
+
+	return &mbox->chans[chan];
+}
+
+static void __iomem *th1520_map_mmio(struct platform_device *pdev,
+				     char *res_name)
+{
+	void __iomem *mapped;
+	struct resource *res;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, res_name);
+
+	if (!res) {
+		dev_err(&pdev->dev, "Failed to get resource: %s\n", res_name);
+		return ERR_PTR(-EINVAL);
+	}
+
+	mapped = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(mapped))
+		dev_err(&pdev->dev, "Failed to map resource: %s\n", res_name);
+
+	return mapped;
+}
+
+static int th1520_mbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct th1520_mbox_priv *priv;
+	unsigned int remote_idx = 0;
+	unsigned int i;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
+	priv->local_icu[TH_1520_MBOX_ICU_KERNEL_CPU0] =
+		th1520_map_mmio(pdev, "local");
+	if (IS_ERR(priv->local_icu[TH_1520_MBOX_ICU_KERNEL_CPU0]))
+		return PTR_ERR(priv->local_icu[TH_1520_MBOX_ICU_KERNEL_CPU0]);
+
+	priv->remote_icu[0] = th1520_map_mmio(pdev, "remote-icu0");
+	if (IS_ERR(priv->remote_icu[0]))
+		return PTR_ERR(priv->remote_icu[0]);
+
+	priv->remote_icu[1] = th1520_map_mmio(pdev, "remote-icu1");
+	if (IS_ERR(priv->remote_icu[1]))
+		return PTR_ERR(priv->remote_icu[1]);
+
+	priv->remote_icu[2] = th1520_map_mmio(pdev, "remote-icu2");
+	if (IS_ERR(priv->remote_icu[2]))
+		return PTR_ERR(priv->remote_icu[2]);
+
+	priv->local_icu[TH_1520_MBOX_ICU_CPU1] =
+		priv->local_icu[TH_1520_MBOX_ICU_KERNEL_CPU0] +
+		TH_1520_MBOX_CHAN_RES_SIZE;
+	priv->local_icu[TH_1520_MBOX_ICU_CPU2] =
+		priv->local_icu[TH_1520_MBOX_ICU_CPU1] +
+		TH_1520_MBOX_CHAN_RES_SIZE;
+	priv->local_icu[TH_1520_MBOX_ICU_CPU3] =
+		priv->local_icu[TH_1520_MBOX_ICU_CPU2] +
+		TH_1520_MBOX_CHAN_RES_SIZE;
+
+	priv->cur_cpu_ch_base = priv->local_icu[TH_1520_MBOX_ICU_KERNEL_CPU0];
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq < 0)
+		return priv->irq;
+
+	/* init the chans */
+	for (i = 0; i < TH_1520_MBOX_CHANS; i++) {
+		struct th1520_mbox_con_priv *cp = &priv->con_priv[i];
+
+		cp->idx = i;
+		cp->chan = &priv->mbox_chans[i];
+		priv->mbox_chans[i].con_priv = cp;
+		snprintf(cp->irq_desc, sizeof(cp->irq_desc),
+			 "th1520_mbox_chan[%i]", cp->idx);
+
+		cp->comm_local_base = priv->local_icu[i];
+		if (i != TH_1520_MBOX_ICU_KERNEL_CPU0) {
+			cp->comm_remote_base = priv->remote_icu[remote_idx];
+			remote_idx++;
+		}
+	}
+
+	spin_lock_init(&priv->mbox_lock);
+
+	priv->mbox.dev = dev;
+	priv->mbox.ops = &th1520_mbox_ops;
+	priv->mbox.chans = priv->mbox_chans;
+	priv->mbox.num_chans = TH_1520_MBOX_CHANS;
+	priv->mbox.of_xlate = th1520_mbox_xlate;
+	priv->mbox.txdone_irq = true;
+
+	platform_set_drvdata(pdev, priv);
+
+	ret = th1520_mbox_init_generic(priv);
+	if (ret) {
+		dev_err(dev, "Failed to init mailbox context\n");
+		return ret;
+	}
+
+	return devm_mbox_controller_register(dev, &priv->mbox);
+}
+
+static const struct of_device_id th1520_mbox_dt_ids[] = {
+	{ .compatible = "thead,th1520-mbox" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, th1520_mbox_dt_ids);
+
+#ifdef CONFIG_PM_SLEEP
+static int __maybe_unused th1520_mbox_suspend_noirq(struct device *dev)
+{
+	struct th1520_mbox_priv *priv = dev_get_drvdata(dev);
+	struct th1520_mbox_context *ctx = priv->ctx;
+	u32 i;
+	/*
+	 * ONLY interrupt mask bit should be stored and restores.
+	 * INFO data all assumed to be lost.
+	 */
+	for (i = 0; i < TH_1520_MBOX_CHANS; i++) {
+		ctx->intr_mask[i] =
+			ioread32(priv->local_icu[i] + TH_1520_MBOX_MASK);
+	}
+	return 0;
+}
+
+static int __maybe_unused th1520_mbox_resume_noirq(struct device *dev)
+{
+	struct th1520_mbox_priv *priv = dev_get_drvdata(dev);
+	struct th1520_mbox_context *ctx = priv->ctx;
+	u32 i;
+
+	for (i = 0; i < TH_1520_MBOX_CHANS; i++) {
+		iowrite32(ctx->intr_mask[i],
+			  priv->local_icu[i] + TH_1520_MBOX_MASK);
+	}
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops th1520_mbox_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(th1520_mbox_suspend_noirq,
+				      th1520_mbox_resume_noirq)
+};
+
+static struct platform_driver th1520_mbox_driver = {
+	.probe		= th1520_mbox_probe,
+	.driver = {
+		.name	= "th1520-mbox",
+		.of_match_table = th1520_mbox_dt_ids,
+		.pm = &th1520_mbox_pm_ops,
+	},
+};
+module_platform_driver(th1520_mbox_driver);
+
+MODULE_DESCRIPTION("Thead TH-1520 mailbox IPC driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

[PATCH v4 2/3] dt-bindings: mailbox: Add thead,th1520-mailbox bindings

[Michal Wilczynski]

Add bindings for the mailbox controller. This work is based on the vendor
kernel. [1]

Link: https://github.com/revyos/thead-kernel.git [1]

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/mailbox/thead,th1520-mbox.yaml   | 80 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml

diff --git a/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
new file mode 100644
index 000000000000..12507c426691
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/thead,th1520-mbox.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: T-head TH1520 Mailbox Controller
+
+description:
+  The T-head mailbox controller enables communication and coordination between
+  cores within the SoC by passing messages (e.g., data, status, and control)
+  through mailbox channels. It also allows one core to signal another processor
+  using interrupts via the Interrupt Controller Unit (ICU).
+
+maintainers:
+  - Michal Wilczynski <m.wilczynski@samsung.com>
+
+properties:
+  compatible:
+    const: thead,th1520-mbox
+
+  reg:
+    items:
+      - description: Mailbox local base address
+      - description: Remote ICU 0 base address
+      - description: Remote ICU 1 base address
+      - description: Remote ICU 2 base address
+
+  reg-names:
+    items:
+      - const: local
+      - const: remote-icu0
+      - const: remote-icu1
+      - const: remote-icu2
+
+  interrupts:
+    maxItems: 1
+
+  '#mbox-cells':
+    const: 2
+    description: |
+      Specifies the number of cells needed to encode the mailbox specifier.
+      The mailbox specifier consists of two cells:
+        - Destination CPU ID.
+        - Type, which can be one of the following:
+            - 0:
+                - TX & RX channels share the same channel.
+                - Equipped with 7 info registers to facilitate data sharing.
+                - Supports IRQ for signaling.
+            - 1:
+                - TX & RX operate as doorbell channels.
+                - Does not have dedicated info registers.
+                - Lacks ACK support.
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - '#mbox-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+      mailbox@ffffc38000 {
+        compatible = "thead,th1520-mbox";
+        reg = <0xff 0xffc38000 0x0 0x4000>,
+              <0xff 0xffc44000 0x0 0x1000>,
+              <0xff 0xffc4c000 0x0 0x1000>,
+              <0xff 0xffc54000 0x0 0x1000>;
+        reg-names = "local", "remote-icu0", "remote-icu1", "remote-icu2";
+        interrupts = <28>;
+        #mbox-cells = <2>;
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 0655c6ba5435..7401c7cb6533 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19951,6 +19951,7 @@ L:	linux-riscv@lists.infradead.org
 S:	Maintained
 T:	git https://github.com/pdp7/linux.git
 F:	Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
+F:	Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
 F:	arch/riscv/boot/dts/thead/
 F:	drivers/clk/thead/clk-th1520-ap.c
 F:	drivers/mailbox/mailbox-th1520.c
-- 
2.34.1

[PATCH v4 3/3] riscv: dts: thead: Add mailbox node

[Michal Wilczynski]

Add mailbox device tree node. This work is based on the vendor kernel [1].

Link: https://github.com/revyos/thead-kernel.git [1]

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 arch/riscv/boot/dts/thead/th1520.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 6992060e6a54..435f0ab0174d 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -555,5 +555,17 @@ portf: gpio-controller@0 {
 				interrupts = <55 IRQ_TYPE_LEVEL_HIGH>;
 			};
 		};
+
+		mbox_910t: mailbox@ffffc38000 {
+		       compatible = "thead,th1520-mbox";
+		       reg = <0xff 0xffc38000 0x0 0x4000>,
+			     <0xff 0xffc44000 0x0 0x1000>,
+			     <0xff 0xffc4c000 0x0 0x1000>,
+			     <0xff 0xffc54000 0x0 0x1000>;
+		       reg-names = "local", "remote-icu0", "remote-icu1", "remote-icu2";
+		       interrupt-parent = <&plic>;
+		       interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
+		       #mbox-cells = <2>;
+		};
 	};
 };
-- 
2.34.1

Re: [PATCH v4 3/3] riscv: dts: thead: Add mailbox node

[Emil Renner Berthing]

Michal Wilczynski wrote:
> Add mailbox device tree node. This work is based on the vendor kernel [1].
>
> Link: https://github.com/revyos/thead-kernel.git [1]
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  arch/riscv/boot/dts/thead/th1520.dtsi | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index 6992060e6a54..435f0ab0174d 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -555,5 +555,17 @@ portf: gpio-controller@0 {
>  				interrupts = <55 IRQ_TYPE_LEVEL_HIGH>;
>  			};
>  		};
> +
> +		mbox_910t: mailbox@ffffc38000 {

Hi Michal,

Thanks for your patch! Please sort this by address similar to the other nodes.

> +		       compatible = "thead,th1520-mbox";
> +		       reg = <0xff 0xffc38000 0x0 0x4000>,

The documentation[1] calls this area MBOX0_T, but it says it's 24kB long.

[1]: https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf

> +			     <0xff 0xffc44000 0x0 0x1000>,

According to the documentation this is inside the 24kB MBOX1_T area.

> +			     <0xff 0xffc4c000 0x0 0x1000>,

This is callod MBOX2_T, but is 8kB long.

> +			     <0xff 0xffc54000 0x0 0x1000>;

This is callod MBOX3_T, but is 8kB long.

> +		       reg-names = "local", "remote-icu0", "remote-icu1", "remote-icu2";

Maybe these should match the MBOXn_T names in the documentation?

> +		       interrupt-parent = <&plic>;
> +		       interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;

You should probably also add the clocks here:

	clocks = <&clk CLK_MBOX0>,  .., <&clk CLK_MBOX3>;

..and claim them in the driver. Otherwise the clock framework will consider
them unused and turn them off without the clk_ignore_unesed kernel command
line.

/Emil

> +		       #mbox-cells = <2>;
> +		};
>  	};
>  };
> --
> 2.34.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 0/3] Introduce support for T-head TH1520 Mailbox

[Drew Fustini]

On Mon, Oct 14, 2024 at 02:33:11PM +0200, Michal Wilczynski wrote:
> The T-head TH1520 SoC supports a hardware mailbox that enables two cores
> within the SoC to communicate and coordinate [1]. One example of such
> coordination would be cooperation with the T-Head E902 core, which is
> responsible for power, clock, and resource management. For example, in
> the specific case of the BXM-4-64 GPU, it needs to be powered on by the
> E902 core, and the kernel running on the E910 needs to 'ask' the
> firmware running on the E902 core to enable power to the GPU island.
> Given recent advancements in work on the upstream GPU driver [2], there
> is an emerging need to get this code in the mainline kernel.
> 
> Link: https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf [1]
> Link: https://gitlab.freedesktop.org/imagination/linux-firmware/-/issues/1 [2]
> 
> Thanks, Krzysztof and Rob, for your review! Since this series is gaining
> some interest, I've dropped the RFC prefix with the v3 update.

I've applied this series and booted okay. I see the driver loaded:

 /sys/devices/platform/soc/ffffc38000.mailbox/driver points to
 /sys/bus/platform/drivers/th1520-mbox

How do you test that the communication with the E902 is working
correctly?

Thanks,
Drew

Re: [PATCH v4 3/3] riscv: dts: thead: Add mailbox node

[Michal Wilczynski]

On 10/14/24 16:57, Emil Renner Berthing wrote:
> Michal Wilczynski wrote:
>> Add mailbox device tree node. This work is based on the vendor kernel [1].
>>
>> Link: https://protect2.fireeye.com/v1/url?k=0bc95f25-545267d8-0bc8d46a-000babff317b-85a52eab21db9d22&q=1&e=63a49acd-e343-43d2-a57d-b4f6fcd23b61&u=https%3A%2F%2Fgithub.com%2Frevyos%2Fthead-kernel.git [1]
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  arch/riscv/boot/dts/thead/th1520.dtsi | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
>> index 6992060e6a54..435f0ab0174d 100644
>> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
>> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
>> @@ -555,5 +555,17 @@ portf: gpio-controller@0 {
>>  				interrupts = <55 IRQ_TYPE_LEVEL_HIGH>;
>>  			};
>>  		};
>> +
>> +		mbox_910t: mailbox@ffffc38000 {
> 
> Hi Michal,
> 
> Thanks for your patch! Please sort this by address similar to the other nodes.

Thank you for your review. Will do.

> 
>> +		       compatible = "thead,th1520-mbox";
>> +		       reg = <0xff 0xffc38000 0x0 0x4000>,
> 
> The documentation[1] calls this area MBOX0_T, but it says it's 24kB long.
> 
> [1]: https://protect2.fireeye.com/v1/url?k=182b68d6-47b0502b-182ae399-000babff317b-d2b05f97b85a09ff&q=1&e=63a49acd-e343-43d2-a57d-b4f6fcd23b61&u=https%3A%2F%2Fgit.beagleboard.org%2Fbeaglev-ahead%2Fbeaglev-ahead%2F-%2Fblob%2Fmain%2Fdocs%2FTH1520%2520System%2520User%2520Manual.pdf
> 
>> +			     <0xff 0xffc44000 0x0 0x1000>,
> 
> According to the documentation this is inside the 24kB MBOX1_T area.
> 
>> +			     <0xff 0xffc4c000 0x0 0x1000>,
> 
> This is callod MBOX2_T, but is 8kB long.
> 
>> +			     <0xff 0xffc54000 0x0 0x1000>;
> 
> This is callod MBOX3_T, but is 8kB long.
> 
>> +		       reg-names = "local", "remote-icu0", "remote-icu1", "remote-icu2";
> 
> Maybe these should match the MBOXn_T names in the documentation?

Indeed, those are excellent points. I wondered about this today, trying
to understand why the mapping was done this way.

For the MBOX0_T mapping, the mailbox driver needs to map the M0_*
registers, including the M0_Cn registers, where other cores write their
messages. This setup requires a total of 16KB, with an additional 8KB
that remains unused.

Regarding MBOX1_T, MBOX2_T, and MBOX3_T, only one set of registers is
necessary - specifically, Mn_C0 since the kernel always sends messages
from the 910t core with CPU_IDX=0.

The MBOX1_T mapping is particularly confusing, as the relevant
registers, M1_C0*, start with an offset of 0x4000 relative to the
beginning of the mapping.

For MBOX2_T and MBOX3_T, the necessary register sets, M2_C0 and M3_C0,
each occupy 4KB of address space, leaving extra 4kB unused.

I assume the hardware designers found these mappings more
straightforward to implement this way. I’m fairly confident that these
numbers are accurate, as I have tested them and confirmed they work.

> 
>> +		       interrupt-parent = <&plic>;
>> +		       interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
> 
> You should probably also add the clocks here:
> 
> 	clocks = <&clk CLK_MBOX0>,  .., <&clk CLK_MBOX3>;
> 
> ..and claim them in the driver. Otherwise the clock framework will consider
> them unused and turn them off without the clk_ignore_unesed kernel command
> line.

Thanks for the suggestion! I’ll re-test with the clocks added. I had
clk_ignore_unused enabled permanently in my U-Boot configuration, so I
might have overlooked this detail.

> 
> /Emil
> 
>> +		       #mbox-cells = <2>;
>> +		};
>>  	};
>>  };
>> --
>> 2.34.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> https://protect2.fireeye.com/v1/url?k=cb1a1bba-94812347-cb1b90f5-000babff317b-3fe071fc8037390e&q=1&e=63a49acd-e343-43d2-a57d-b4f6fcd23b61&u=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-riscv
> 

Re: [PATCH v4 0/3] Introduce support for T-head TH1520 Mailbox

[Michal Wilczynski]

On 10/14/24 20:31, Drew Fustini wrote:
> On Mon, Oct 14, 2024 at 02:33:11PM +0200, Michal Wilczynski wrote:
>> The T-head TH1520 SoC supports a hardware mailbox that enables two cores
>> within the SoC to communicate and coordinate [1]. One example of such
>> coordination would be cooperation with the T-Head E902 core, which is
>> responsible for power, clock, and resource management. For example, in
>> the specific case of the BXM-4-64 GPU, it needs to be powered on by the
>> E902 core, and the kernel running on the E910 needs to 'ask' the
>> firmware running on the E902 core to enable power to the GPU island.
>> Given recent advancements in work on the upstream GPU driver [2], there
>> is an emerging need to get this code in the mainline kernel.
>>
>> Link: https://protect2.fireeye.com/v1/url?k=2021d256-7fbdfb7c-20205919-000babe598f7-ca654d1a9bc866ac&q=1&e=11e97355-e6e9-4aac-a996-cc475156b3c8&u=https%3A%2F%2Fopenbeagle.org%2Fbeaglev-ahead%2Fbeaglev-ahead%2F-%2Fblob%2Fmain%2Fdocs%2FTH1520%2520System%2520User%2520Manual.pdf [1]
>> Link: https://gitlab.freedesktop.org/imagination/linux-firmware/-/issues/1 [2]
>>
>> Thanks, Krzysztof and Rob, for your review! Since this series is gaining
>> some interest, I've dropped the RFC prefix with the v3 update.
> 
> I've applied this series and booted okay. I see the driver loaded:
> 
>  /sys/devices/platform/soc/ffffc38000.mailbox/driver points to
>  /sys/bus/platform/drivers/th1520-mbox
> 
> How do you test that the communication with the E902 is working
> correctly?

Thank you for your interest. To test this, I've prepared a diff that
includes the missing drivers utilizing the mailbox and enabled the GPU
node in the device tree to use the drm/imagination driver.

I've observed that when the power was turned off through the E902 core
using the mailbox, the drm/imagination driver would hang in
pvr_load_gpu_id() while attempting to read its BVNC from the register.
However, when the GPU was turned on via the mailbox, the BVNC could be
read correctly. Still, the firmware fails to boot due to some missing
programming in the drm/imagination driver, which is currently being
worked on. I've briefly explained this in the first commit of this
series.

If you'd like to try this yourself, I'd be happy to push these setups to
a GitHub repository and provide you with a link, so you can see the
setup in action.

Michał

> 
> Thanks,
> Drew
> 

Re: [PATCH v4 2/3] dt-bindings: mailbox: Add thead,th1520-mailbox bindings

[Samuel Holland]

Hi Michal,

On 2024-10-14 7:33 AM, Michal Wilczynski wrote:
> Add bindings for the mailbox controller. This work is based on the vendor
> kernel. [1]
> 
> Link: https://github.com/revyos/thead-kernel.git [1]
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  .../bindings/mailbox/thead,th1520-mbox.yaml   | 80 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
> new file mode 100644
> index 000000000000..12507c426691
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/thead,th1520-mbox.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: T-head TH1520 Mailbox Controller
> +
> +description:
> +  The T-head mailbox controller enables communication and coordination between
> +  cores within the SoC by passing messages (e.g., data, status, and control)
> +  through mailbox channels. It also allows one core to signal another processor
> +  using interrupts via the Interrupt Controller Unit (ICU).
> +
> +maintainers:
> +  - Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +properties:
> +  compatible:
> +    const: thead,th1520-mbox
> +
> +  reg:
> +    items:
> +      - description: Mailbox local base address
> +      - description: Remote ICU 0 base address
> +      - description: Remote ICU 1 base address
> +      - description: Remote ICU 2 base address

From the SoC documentation, it looks like these are four different instances of
the same IP block, each containing 4 unidirectional mailbox channels and an
interrupt output. So these should be four separate nodes, no? The C910 would
receive on channels 1-3 of instance 0, and send on channel 0 of instances 1-3.

> +
> +  reg-names:
> +    items:
> +      - const: local
> +      - const: remote-icu0
> +      - const: remote-icu1
> +      - const: remote-icu2
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  '#mbox-cells':
> +    const: 2
> +    description: |
> +      Specifies the number of cells needed to encode the mailbox specifier.
> +      The mailbox specifier consists of two cells:
> +        - Destination CPU ID.
> +        - Type, which can be one of the following:
> +            - 0:
> +                - TX & RX channels share the same channel.
> +                - Equipped with 7 info registers to facilitate data sharing.
> +                - Supports IRQ for signaling.
> +            - 1:
> +                - TX & RX operate as doorbell channels.
> +                - Does not have dedicated info registers.
> +                - Lacks ACK support.

It appears that these types are not describing hardware, but the protocol used
by the Linux driver to glue two unidirectional hardware channels together to
make a virtual bidirectional channel. This is really the responsibility of the
mailbox client to know what protocol it needs, not the devicetree.

Regards,
Samuel

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - '#mbox-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +      mailbox@ffffc38000 {
> +        compatible = "thead,th1520-mbox";
> +        reg = <0xff 0xffc38000 0x0 0x4000>,
> +              <0xff 0xffc44000 0x0 0x1000>,
> +              <0xff 0xffc4c000 0x0 0x1000>,
> +              <0xff 0xffc54000 0x0 0x1000>;
> +        reg-names = "local", "remote-icu0", "remote-icu1", "remote-icu2";
> +        interrupts = <28>;
> +        #mbox-cells = <2>;
> +      };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0655c6ba5435..7401c7cb6533 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19951,6 +19951,7 @@ L:	linux-riscv@lists.infradead.org
>  S:	Maintained
>  T:	git https://github.com/pdp7/linux.git
>  F:	Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
> +F:	Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
>  F:	arch/riscv/boot/dts/thead/
>  F:	drivers/clk/thead/clk-th1520-ap.c
>  F:	drivers/mailbox/mailbox-th1520.c

Re: [PATCH v4 2/3] dt-bindings: mailbox: Add thead,th1520-mailbox bindings

[Krzysztof Kozlowski]

On 16/10/2024 01:14, Samuel Holland wrote:
>> +  reg-names:
>> +    items:
>> +      - const: local
>> +      - const: remote-icu0
>> +      - const: remote-icu1
>> +      - const: remote-icu2
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  '#mbox-cells':
>> +    const: 2
>> +    description: |
>> +      Specifies the number of cells needed to encode the mailbox specifier.
>> +      The mailbox specifier consists of two cells:
>> +        - Destination CPU ID.
>> +        - Type, which can be one of the following:
>> +            - 0:
>> +                - TX & RX channels share the same channel.
>> +                - Equipped with 7 info registers to facilitate data sharing.
>> +                - Supports IRQ for signaling.
>> +            - 1:
>> +                - TX & RX operate as doorbell channels.
>> +                - Does not have dedicated info registers.
>> +                - Lacks ACK support.
> 
> It appears that these types are not describing hardware, but the protocol used
> by the Linux driver to glue two unidirectional hardware channels together to
> make a virtual bidirectional channel. This is really the responsibility of the
> mailbox client to know what protocol it needs, not the devicetree.
> 

Hm, where is the DTS with consumers of this mailbox provider?

Best regards,
Krzysztof

Re: [PATCH v4 0/3] Introduce support for T-head TH1520 Mailbox

[Drew Fustini]

On Wed, Oct 16, 2024 at 12:03:05AM +0200, Michal Wilczynski wrote:
> 
> 
> On 10/14/24 20:31, Drew Fustini wrote:
> > On Mon, Oct 14, 2024 at 02:33:11PM +0200, Michal Wilczynski wrote:
> >> The T-head TH1520 SoC supports a hardware mailbox that enables two cores
> >> within the SoC to communicate and coordinate [1]. One example of such
> >> coordination would be cooperation with the T-Head E902 core, which is
> >> responsible for power, clock, and resource management. For example, in
> >> the specific case of the BXM-4-64 GPU, it needs to be powered on by the
> >> E902 core, and the kernel running on the E910 needs to 'ask' the
> >> firmware running on the E902 core to enable power to the GPU island.
> >> Given recent advancements in work on the upstream GPU driver [2], there
> >> is an emerging need to get this code in the mainline kernel.
> >>
> >> Link: https://protect2.fireeye.com/v1/url?k=2021d256-7fbdfb7c-20205919-000babe598f7-ca654d1a9bc866ac&q=1&e=11e97355-e6e9-4aac-a996-cc475156b3c8&u=https%3A%2F%2Fopenbeagle.org%2Fbeaglev-ahead%2Fbeaglev-ahead%2F-%2Fblob%2Fmain%2Fdocs%2FTH1520%2520System%2520User%2520Manual.pdf [1]
> >> Link: https://gitlab.freedesktop.org/imagination/linux-firmware/-/issues/1 [2]
> >>
> >> Thanks, Krzysztof and Rob, for your review! Since this series is gaining
> >> some interest, I've dropped the RFC prefix with the v3 update.
> > 
> > I've applied this series and booted okay. I see the driver loaded:
> > 
> >  /sys/devices/platform/soc/ffffc38000.mailbox/driver points to
> >  /sys/bus/platform/drivers/th1520-mbox
> > 
> > How do you test that the communication with the E902 is working
> > correctly?
> 
> Thank you for your interest. To test this, I've prepared a diff that
> includes the missing drivers utilizing the mailbox and enabled the GPU
> node in the device tree to use the drm/imagination driver.
> 
> I've observed that when the power was turned off through the E902 core
> using the mailbox, the drm/imagination driver would hang in
> pvr_load_gpu_id() while attempting to read its BVNC from the register.
> However, when the GPU was turned on via the mailbox, the BVNC could be
> read correctly. Still, the firmware fails to boot due to some missing
> programming in the drm/imagination driver, which is currently being
> worked on. I've briefly explained this in the first commit of this
> series.
> 
> If you'd like to try this yourself, I'd be happy to push these setups to
> a GitHub repository and provide you with a link, so you can see the
> setup in action.

I think that would be helpful for myself and others to be able to see
the interaction.

Thanks,
Drew

Re: [PATCH v4 3/3] riscv: dts: thead: Add mailbox node

[Emil Renner Berthing]

Michal Wilczynski wrote:
>
>
> On 10/14/24 16:57, Emil Renner Berthing wrote:
> > Michal Wilczynski wrote:
> >> Add mailbox device tree node. This work is based on the vendor kernel [1].
> >>
> >> Link: https://protect2.fireeye.com/v1/url?k=0bc95f25-545267d8-0bc8d46a-000babff317b-85a52eab21db9d22&q=1&e=63a49acd-e343-43d2-a57d-b4f6fcd23b61&u=https%3A%2F%2Fgithub.com%2Frevyos%2Fthead-kernel.git [1]
> >>
> >> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> >> ---
> >>  arch/riscv/boot/dts/thead/th1520.dtsi | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> >> index 6992060e6a54..435f0ab0174d 100644
> >> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> >> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> >> @@ -555,5 +555,17 @@ portf: gpio-controller@0 {
> >>  				interrupts = <55 IRQ_TYPE_LEVEL_HIGH>;
> >>  			};
> >>  		};
> >> +
> >> +		mbox_910t: mailbox@ffffc38000 {
> >
> > Hi Michal,
> >
> > Thanks for your patch! Please sort this by address similar to the other nodes.
>
> Thank you for your review. Will do.

Thanks!

> >
> >> +		       compatible = "thead,th1520-mbox";
> >> +		       reg = <0xff 0xffc38000 0x0 0x4000>,
> >
> > The documentation[1] calls this area MBOX0_T, but it says it's 24kB long.
> >
> > [1]: https://protect2.fireeye.com/v1/url?k=182b68d6-47b0502b-182ae399-000babff317b-d2b05f97b85a09ff&q=1&e=63a49acd-e343-43d2-a57d-b4f6fcd23b61&u=https%3A%2F%2Fgit.beagleboard.org%2Fbeaglev-ahead%2Fbeaglev-ahead%2F-%2Fblob%2Fmain%2Fdocs%2FTH1520%2520System%2520User%2520Manual.pdf
> >
> >> +			     <0xff 0xffc44000 0x0 0x1000>,
> >
> > According to the documentation this is inside the 24kB MBOX1_T area.
> >
> >> +			     <0xff 0xffc4c000 0x0 0x1000>,
> >
> > This is callod MBOX2_T, but is 8kB long.
> >
> >> +			     <0xff 0xffc54000 0x0 0x1000>;
> >
> > This is callod MBOX3_T, but is 8kB long.
> >
> >> +		       reg-names = "local", "remote-icu0", "remote-icu1", "remote-icu2";
> >
> > Maybe these should match the MBOXn_T names in the documentation?
>
> Indeed, those are excellent points. I wondered about this today, trying
> to understand why the mapping was done this way.
>
> For the MBOX0_T mapping, the mailbox driver needs to map the M0_*
> registers, including the M0_Cn registers, where other cores write their
> messages. This setup requires a total of 16KB, with an additional 8KB
> that remains unused.
>
> Regarding MBOX1_T, MBOX2_T, and MBOX3_T, only one set of registers is
> necessary - specifically, Mn_C0 since the kernel always sends messages
> from the 910t core with CPU_IDX=0.
>
> The MBOX1_T mapping is particularly confusing, as the relevant
> registers, M1_C0*, start with an offset of 0x4000 relative to the
> beginning of the mapping.
>
> For MBOX2_T and MBOX3_T, the necessary register sets, M2_C0 and M3_C0,
> each occupy 4KB of address space, leaving extra 4kB unused.
>
> I assume the hardware designers found these mappings more
> straightforward to implement this way. I’m fairly confident that these
> numbers are accurate, as I have tested them and confirmed they work.

I don't doubt these mappings work, but usually the device tree should describe
the hardware and not the driver. So unless you think the documentation is
wrong, I'd still suggest you do the bindings and device tree so they describe
the hardware, and then let the driver handle the irregular register offsets.

/Emil

Re: [PATCH v4 2/3] dt-bindings: mailbox: Add thead,th1520-mailbox bindings

[Michal Wilczynski]

On 10/16/24 08:31, Krzysztof Kozlowski wrote:
> On 16/10/2024 01:14, Samuel Holland wrote:
>>> +  reg-names:
>>> +    items:
>>> +      - const: local
>>> +      - const: remote-icu0
>>> +      - const: remote-icu1
>>> +      - const: remote-icu2
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  '#mbox-cells':
>>> +    const: 2
>>> +    description: |
>>> +      Specifies the number of cells needed to encode the mailbox specifier.
>>> +      The mailbox specifier consists of two cells:
>>> +        - Destination CPU ID.
>>> +        - Type, which can be one of the following:
>>> +            - 0:
>>> +                - TX & RX channels share the same channel.
>>> +                - Equipped with 7 info registers to facilitate data sharing.
>>> +                - Supports IRQ for signaling.
>>> +            - 1:
>>> +                - TX & RX operate as doorbell channels.
>>> +                - Does not have dedicated info registers.
>>> +                - Lacks ACK support.
>>
>> It appears that these types are not describing hardware, but the protocol used
>> by the Linux driver to glue two unidirectional hardware channels together to
>> make a virtual bidirectional channel. This is really the responsibility of the
>> mailbox client to know what protocol it needs, not the devicetree.
>>
> 
> Hm, where is the DTS with consumers of this mailbox provider?

The DTS with consumers of this mailbox provider is not included in this
series. Since the mailbox users depend on this driver being upstreamed,
I decided to submit this driver first to gather feedback on whether it
can be accepted upstream. If successful, I will follow up with another
series for the aon driver, which will use this mailbox to send commands
to the E902 core, such as powering the GPU on or off.

The consumer DTS would look something like this:

aon: aon {
	compatible = "thead,th1520-aon";
	mbox-names = "aon";
	mboxes = <&mbox_910t 1 0>;
	status = "okay";

	pd: light-aon-pd {
		compatible = "thead,light-aon-pd";
		#power-domain-cells = <1>;
	};
};

Thanks,
Michał

> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH v4 2/3] dt-bindings: mailbox: Add thead,th1520-mailbox bindings

[Michal Wilczynski]

On 10/16/24 01:14, Samuel Holland wrote:
> Hi Michal,
> 
> On 2024-10-14 7:33 AM, Michal Wilczynski wrote:
>> Add bindings for the mailbox controller. This work is based on the vendor
>> kernel. [1]
>>
>> Link: https://protect2.fireeye.com/v1/url?k=deccc9a8-815707cc-decd42e7-000babda0201-ff79d4aaff73f36c&q=1&e=7028c1ef-1c3d-4e17-b7ed-76d362b80caf&u=https%3A%2F%2Fgithub.com%2Frevyos%2Fthead-kernel.git [1]
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>  .../bindings/mailbox/thead,th1520-mbox.yaml   | 80 +++++++++++++++++++
>>  MAINTAINERS                                   |  1 +
>>  2 files changed, 81 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
>> new file mode 100644
>> index 000000000000..12507c426691
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
>> @@ -0,0 +1,80 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: https://protect2.fireeye.com/v1/url?k=7fcda92a-2056674e-7fcc2265-000babda0201-c5d541bdd4cc5f35&q=1&e=7028c1ef-1c3d-4e17-b7ed-76d362b80caf&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fmailbox%2Fthead%2Cth1520-mbox.yaml%23
>> +$schema: https://protect2.fireeye.com/v1/url?k=068c7881-5917b6e5-068df3ce-000babda0201-b045bd7290e64f0e&q=1&e=7028c1ef-1c3d-4e17-b7ed-76d362b80caf&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
>> +
>> +title: T-head TH1520 Mailbox Controller
>> +
>> +description:
>> +  The T-head mailbox controller enables communication and coordination between
>> +  cores within the SoC by passing messages (e.g., data, status, and control)
>> +  through mailbox channels. It also allows one core to signal another processor
>> +  using interrupts via the Interrupt Controller Unit (ICU).
>> +
>> +maintainers:
>> +  - Michal Wilczynski <m.wilczynski@samsung.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: thead,th1520-mbox
>> +
>> +  reg:
>> +    items:
>> +      - description: Mailbox local base address
>> +      - description: Remote ICU 0 base address
>> +      - description: Remote ICU 1 base address
>> +      - description: Remote ICU 2 base address
> 
>>From the SoC documentation, it looks like these are four different instances of
> the same IP block, each containing 4 unidirectional mailbox channels and an
> interrupt output. So these should be four separate nodes, no? The C910 would
> receive on channels 1-3 of instance 0, and send on channel 0 of instances 1-3.
> 

Hi, and thank you for your review.

I wanted to take some time to familiarize myself with the device tree
documentation and review best practices for mailbox drivers before
responding.

Upon reviewing the Linux device tree documentation, I couldn’t find any
specific rule that mandates having one device node per IP block in the
hardware. The T-Head manual is more focused on the hardware from a
programmer’s perspective rather than describing how exaclty the ICU's
are implemented in the HW. The device tree documentation provides
guidelines for how hardware blocks should be represented, but it doesn't
impose a strict requirement for separate device nodes per hardware
block, especially when it comes to devices like mailbox controllers.

Technically, your suggestion to create separate nodes for each ICU
instance is possible. However, doing so would require additional
complexity in the driver, as it would need to handle each node
individually. For instance, the driver would need to request interrupts
only in the case of mailbox910_t_0, while handling other device nodes
like mailbox910_t_1, mailbox910_t_2, and mailbox910_t_3 separately.

In my opinion, this approach would add unnecessary complexity to the
driver code. The current approach — using a single device node for the
mailbox controller and utilizing channels to steer messages seems to
align better with existing best practices for mailbox drivers. Many
mailbox drivers create a single mailbox controller and leverage multiple
channels for different communication paths, which simplifies both the
device tree and the driver implementation.

I hope this explanation clarifies my perspective, and I’d like to
propose keeping the current design as it stands.

>> +
>> +  reg-names:
>> +    items:
>> +      - const: local
>> +      - const: remote-icu0
>> +      - const: remote-icu1
>> +      - const: remote-icu2
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  '#mbox-cells':
>> +    const: 2
>> +    description: |
>> +      Specifies the number of cells needed to encode the mailbox specifier.
>> +      The mailbox specifier consists of two cells:
>> +        - Destination CPU ID.
>> +        - Type, which can be one of the following:
>> +            - 0:
>> +                - TX & RX channels share the same channel.
>> +                - Equipped with 7 info registers to facilitate data sharing.
>> +                - Supports IRQ for signaling.
>> +            - 1:
>> +                - TX & RX operate as doorbell channels.
>> +                - Does not have dedicated info registers.
>> +                - Lacks ACK support.
> 
> It appears that these types are not describing hardware, but the protocol used
> by the Linux driver to glue two unidirectional hardware channels together to
> make a virtual bidirectional channel. This is really the responsibility of the
> mailbox client to know what protocol it needs, not the devicetree.
> 

The protocols in question are actually handled by the firmware running
on the other cores, like the E902. I couldn’t find the firmware source
code, as it’s distributed as binary blobs. For example, the firmware
binary [1] gets loaded by U-Boot at specific addresses.

For the current case — communicating with the E902 core — the Type ‘0’
protocol is all we need. So, I don’t see any issue in removing the
second cell, since we’re only using one protocol here. 

[1] -
https://git.beagleboard.org/beaglev-ahead/xuantie-ubuntu/-/blob/48bc51286483257f153ba58813bb601267d0f087/bins/light_aon_fpga.bin

Thanks,
Michał

> Regards,
> Samuel
> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - interrupts
>> +  - '#mbox-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +
>> +    soc {
>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +      mailbox@ffffc38000 {
>> +        compatible = "thead,th1520-mbox";
>> +        reg = <0xff 0xffc38000 0x0 0x4000>,
>> +              <0xff 0xffc44000 0x0 0x1000>,
>> +              <0xff 0xffc4c000 0x0 0x1000>,
>> +              <0xff 0xffc54000 0x0 0x1000>;
>> +        reg-names = "local", "remote-icu0", "remote-icu1", "remote-icu2";
>> +        interrupts = <28>;
>> +        #mbox-cells = <2>;
>> +      };
>> +    };
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0655c6ba5435..7401c7cb6533 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19951,6 +19951,7 @@ L:	linux-riscv@lists.infradead.org
>>  S:	Maintained
>>  T:	git https://protect2.fireeye.com/v1/url?k=9b23b6b0-c4b878d4-9b223dff-000babda0201-68366f7c65442b8f&q=1&e=7028c1ef-1c3d-4e17-b7ed-76d362b80caf&u=https%3A%2F%2Fgithub.com%2Fpdp7%2Flinux.git
>>  F:	Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
>> +F:	Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
>>  F:	arch/riscv/boot/dts/thead/
>>  F:	drivers/clk/thead/clk-th1520-ap.c
>>  F:	drivers/mailbox/mailbox-th1520.c
> 
> 

Re: [PATCH v4 0/3] Introduce support for T-head TH1520 Mailbox

[Michal Wilczynski]

On 10/16/24 20:08, Drew Fustini wrote:
> On Wed, Oct 16, 2024 at 12:03:05AM +0200, Michal Wilczynski wrote:
>>
>>
>> On 10/14/24 20:31, Drew Fustini wrote:
>>> On Mon, Oct 14, 2024 at 02:33:11PM +0200, Michal Wilczynski wrote:
>>>> The T-head TH1520 SoC supports a hardware mailbox that enables two cores
>>>> within the SoC to communicate and coordinate [1]. One example of such
>>>> coordination would be cooperation with the T-Head E902 core, which is
>>>> responsible for power, clock, and resource management. For example, in
>>>> the specific case of the BXM-4-64 GPU, it needs to be powered on by the
>>>> E902 core, and the kernel running on the E910 needs to 'ask' the
>>>> firmware running on the E902 core to enable power to the GPU island.
>>>> Given recent advancements in work on the upstream GPU driver [2], there
>>>> is an emerging need to get this code in the mainline kernel.
>>>>
>>>> Link: https://protect2.fireeye.com/v1/url?k=2021d256-7fbdfb7c-20205919-000babe598f7-ca654d1a9bc866ac&q=1&e=11e97355-e6e9-4aac-a996-cc475156b3c8&u=https%3A%2F%2Fopenbeagle.org%2Fbeaglev-ahead%2Fbeaglev-ahead%2F-%2Fblob%2Fmain%2Fdocs%2FTH1520%2520System%2520User%2520Manual.pdf [1]
>>>> Link: https://gitlab.freedesktop.org/imagination/linux-firmware/-/issues/1 [2]
>>>>
>>>> Thanks, Krzysztof and Rob, for your review! Since this series is gaining
>>>> some interest, I've dropped the RFC prefix with the v3 update.
>>>
>>> I've applied this series and booted okay. I see the driver loaded:
>>>
>>>  /sys/devices/platform/soc/ffffc38000.mailbox/driver points to
>>>  /sys/bus/platform/drivers/th1520-mbox
>>>
>>> How do you test that the communication with the E902 is working
>>> correctly?
>>
>> Thank you for your interest. To test this, I've prepared a diff that
>> includes the missing drivers utilizing the mailbox and enabled the GPU
>> node in the device tree to use the drm/imagination driver.
>>
>> I've observed that when the power was turned off through the E902 core
>> using the mailbox, the drm/imagination driver would hang in
>> pvr_load_gpu_id() while attempting to read its BVNC from the register.
>> However, when the GPU was turned on via the mailbox, the BVNC could be
>> read correctly. Still, the firmware fails to boot due to some missing
>> programming in the drm/imagination driver, which is currently being
>> worked on. I've briefly explained this in the first commit of this
>> series.
>>
>> If you'd like to try this yourself, I'd be happy to push these setups to
>> a GitHub repository and provide you with a link, so you can see the
>> setup in action.
> 
> I think that would be helpful for myself and others to be able to see
> the interaction.

I’ve cleaned up the code and it’s ready to share. I’ll include this
description and the GitHub links in the cover letter for the next
revision.

I’ve created two branches. Both contain the same code, including this
patch series and some extra commits with mailbox consumers. The only
difference is that one branch has an additional commit that switches ON
calls to OFF calls (and vice versa) to observe the behavior when the GPU
is turned off via the mailbox.

To reproduce, simply clone the repository, add the following extra options
in the config:

CONFIG_DRM_POWERVR=y
CONFIG_LIGHT_AON=y
CONFIG_LIGHT_AON_PD=y

Then build and deploy on the target.

In case [1], the output should be:
[    2.478394] light_aon_probe: virtual_log_mem=0x000000005faf564a, phy base=0x33600000, size:2097152
[    2.488589] succeed to create power domain debugfs direntry
[    2.494987] powervr ffef400000.gpu: Before reading BVNC
At this point, the system will hang because the driver is trying to read
memory-mapped registers while the GPU isn’t powered on.

In case [2], the GPU powers on correctly, and the BVNC can be read
without issues. If the specific firmware file isn’t available, the
output would look like this:
root@revyos-lpi4a:~# dmesg | grep gpu
[    2.408207] powervr ffef400000.gpu: Before reading BVNC
[    2.413533] powervr ffef400000.gpu: After reading BVNC
[    2.418930] powervr ffef400000.gpu: Direct firmware load for powervr/rogue_36.52.104.182_v1.fw failed with error -2
[    2.429568] powervr ffef400000.gpu: [drm] *ERROR* failed to load firmware powervr/rogue_36.52.104.182_v1.fw (err=-2)
[    2.440403] powervr ffef400000.gpu: probe with driver powervr failed with error -2

Here are the links:
[1] - https://github.com/mwilczy/linux/tree/22_october_demonstrate_mailbox_not_working
[2] - https://github.com/mwilczy/linux/tree/22_october_demonstrate_mailbox_working

Note: U-Boot must load the AON firmware at startup to the address mapped
for AON for this to work properly.

Thanks,
Michał

> 
> Thanks,
> Drew
>