[PATCH 0/3] soc: renesas: add MFIS driver

[PATCH 0/3] soc: renesas: add MFIS driver

[Wolfram Sang]

Renesas R-Car MFIS offers multiple features but most importantly
mailboxes and hwspinlocks. Because they share a common register space
and a common register unprotection mechanism, a single driver was chosen
to handle all dependencies. (MFD and auxiliary bus have been tried as
well, but they failed because of circular dependencies.)

In this first step, the driver implements common register access and a
mailbox controller. hwspinlock support will be added incrementally, once
the subsystem allows out-of-directory drivers (patches already under
review). This driver has been tested on a Renesas Ironhide board (R-Car
X5H) and is able to communicate with the SCP via mailboxes. Also, the
mailbox-test driver was used to confirm back-and-forth communication
between two application cores.

Because of its multifunctional nature, the driver lives in
drivers/soc/renesas. As large parts of these patches handle mailbox
support, review from mailbox expeirenced people is much appreciated.

A branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/x5h/mfis-single-driver

Thanks and happy hacking,

   Wolfram


Changes since last internal RFC:
* no functional change
* extended parts of the binding description
* reworded parts of commit messages
* dual licence the dt-binding header
* dropped unneeded Kconfig dependency on HWSPINLOCK
* rebased to linux-next as of 20260316


Wolfram Sang (3):
  dt-bindings: soc: renesas: add MFIS binding documentation
  soc: renesas: Add Renesas R-Car MFIS driver
  soc: renesas: add X5H PRR support

 .../soc/renesas/renesas,r8a78000-mfis.yaml    | 160 +++++++++
 drivers/soc/renesas/Kconfig                   |   9 +
 drivers/soc/renesas/Makefile                  |   1 +
 drivers/soc/renesas/rcar-mfis.c               | 325 ++++++++++++++++++
 drivers/soc/renesas/renesas-soc.c             |   8 +-
 .../mailbox/renesas,r8a78000-mfis.h           |  27 ++
 6 files changed, 529 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/soc/renesas/renesas,r8a78000-mfis.yaml
 create mode 100644 drivers/soc/renesas/rcar-mfis.c
 create mode 100644 include/dt-bindings/mailbox/renesas,r8a78000-mfis.h

-- 
2.51.0

[PATCH 1/3] dt-bindings: soc: renesas: add MFIS binding documentation

[Wolfram Sang]

Add device tree bindings for the Renesas Multifunctional Interface
(MFIS) as found on the Renesas R-Car X5H (r8a78000) SoC. MFIS includes
features like Mailbox/HW Spinlock/Product Register.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Checked with 'dt_bindings_check'. Family-compatible values are not
introduced here because MFIS is usually very different per SoC.

 .../soc/renesas/renesas,r8a78000-mfis.yaml    | 160 ++++++++++++++++++
 .../mailbox/renesas,r8a78000-mfis.h           |  27 +++
 2 files changed, 187 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/renesas/renesas,r8a78000-mfis.yaml
 create mode 100644 include/dt-bindings/mailbox/renesas,r8a78000-mfis.h

diff --git a/Documentation/devicetree/bindings/soc/renesas/renesas,r8a78000-mfis.yaml b/Documentation/devicetree/bindings/soc/renesas/renesas,r8a78000-mfis.yaml
new file mode 100644
index 000000000000..dbda28ac781c
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/renesas/renesas,r8a78000-mfis.yaml
@@ -0,0 +1,160 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/renesas/renesas,r8a78000-mfis.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas MFIS (Multifunctional Interface) controller
+
+maintainers:
+  - Wolfram Sang <wsa+renesas@sang-engineering.com>
+
+description:
+  Renesas Multifunctional Interface (MFIS) provides functionality for
+  communication between different CPU cores. Those cores can be in various
+  domains like AP, RT, or SCP. Functionality includes features like
+  mailboxes, hardware spinlocks and such.
+
+properties:
+  compatible:
+    enum:
+      - renesas,r8a78000-mfis       # R-Car X5H (AP<->AP, with PRR)
+      - renesas,r8a78000-mfis-scp   # R-Car X5H (AP<->SCP, without PRR)
+
+  reg:
+    minItems: 2
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: common
+      - const: mboxes
+
+  interrupts:
+    minItems: 32
+    maxItems: 128
+    description:
+      The interrupts raised by the remote doorbells.
+
+  interrupt-names:
+    minItems: 32
+    maxItems: 128
+    items:
+      pattern: "^ch[0-9]+[ie]$"
+    description:
+      An interrupt name is constructed with the prefix 'ch'. Then, the
+      channel number as specified in the documentation of the SoC. Finally,
+      the letter 'i' if the interrupt is raised by the IICR register. Or 'e'
+      if it is raised by the EICR register.
+
+  "#hwlock-cells":
+    const: 1
+
+  "#mbox-cells":
+    const: 2
+    description:
+      The first cell is the channel number as specified in the documentation
+      of the SoC. The second cell may specify flags as described in the file
+      <dt-bindings/mailbox/renesas,r8a78000-mfis.h>.
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - interrupt-names
+  - "#hwlock-cells"
+  - "#mbox-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    mfis: syscon@189e0000 {
+            compatible = "renesas,r8a78000-mfis";
+            reg = <0x189e0000 0x1000>, <0x18800000 0x40000>;
+            reg-names = "common", "mboxes";
+            interrupts = <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 113 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 127 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 128 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 129 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 132 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 136 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 139 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 141 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 142 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 151 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 152 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 166 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 167 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 168 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 171 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 172 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 173 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 175 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 178 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 180 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 181 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 182 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 183 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 184 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 185 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 186 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 187 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 190 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 191 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 199 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 200 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 201 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 202 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 203 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 204 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 205 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 206 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 210 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 211 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 212 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 213 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 214 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 215 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 216 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 217 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 218 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 219 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 220 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 221 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 222 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 225 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 227 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 228 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "ch0i", "ch0e", "ch1i", "ch1e", "ch2i", "ch2e", "ch3i", "ch3e",
+                              "ch4i", "ch4e", "ch5i", "ch5e", "ch6i", "ch6e", "ch7i", "ch7e",
+                              "ch8i", "ch8e", "ch9i", "ch9e", "ch10i", "ch10e", "ch11i", "ch11e",
+                              "ch12i", "ch12e", "ch13i", "ch13e", "ch14i", "ch14e", "ch15i", "ch15e",
+                              "ch16i", "ch16e", "ch17i", "ch17e", "ch18i", "ch18e", "ch19i", "ch19e",
+                              "ch20i", "ch20e", "ch21i", "ch21e", "ch22i", "ch22e", "ch23i", "ch23e",
+                              "ch24i", "ch24e", "ch25i", "ch25e", "ch26i", "ch26e", "ch27i", "ch27e",
+                              "ch28i", "ch28e", "ch29i", "ch29e", "ch30i", "ch30e", "ch31i", "ch31e",
+                              "ch32i", "ch32e", "ch33i", "ch33e", "ch34i", "ch34e", "ch35i", "ch35e",
+                              "ch36i", "ch36e", "ch37i", "ch37e", "ch38i", "ch38e", "ch39i", "ch39e",
+                              "ch40i", "ch40e", "ch41i", "ch41e", "ch42i", "ch42e", "ch43i", "ch43e",
+                              "ch44i", "ch44e", "ch45i", "ch45e", "ch46i", "ch46e", "ch47i", "ch47e",
+                              "ch48i", "ch48e", "ch49i", "ch49e", "ch50i", "ch50e", "ch51i", "ch51e",
+                              "ch52i", "ch52e", "ch53i", "ch53e", "ch54i", "ch54e", "ch55i", "ch55e",
+                              "ch56i", "ch56e", "ch57i", "ch57e", "ch58i", "ch58e", "ch59i", "ch59e",
+                              "ch60i", "ch60e", "ch61i", "ch61e", "ch62i", "ch62e", "ch63i", "ch63e";
+            #hwlock-cells = <1>;
+            #mbox-cells = <2>;
+    };
diff --git a/include/dt-bindings/mailbox/renesas,r8a78000-mfis.h b/include/dt-bindings/mailbox/renesas,r8a78000-mfis.h
new file mode 100644
index 000000000000..89489c2a4847
--- /dev/null
+++ b/include/dt-bindings/mailbox/renesas,r8a78000-mfis.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Constants for the mailbox part of the Renesas MFIS IP core.
+ */
+
+#ifndef _DT_BINDINGS_MAILBOX_RENESAS_MFIS_H
+#define _DT_BINDINGS_MAILBOX_RENESAS_MFIS_H
+
+/*
+ * MFIS HW design before r8a78001 requires a channel to be marked as either
+ * TX or RX.
+ */
+#define MFIS_CHANNEL_TX	(0 << 0)
+#define MFIS_CHANNEL_RX	(1 << 0)
+
+/*
+ * MFIS variants before r8a78001 work with pairs of IICR and EICR registers.
+ * Usually, it is specified in the datasheets which of the two a specific core
+ * should use. Then, it does not need extra description in DT. For plain MFIS
+ * of r8a78000, this is selectable, though. According to the system design and
+ * the firmware in use, these channels need to be marked. This is not needed
+ * with other versions of the MFIS, not even with MFIS-SCP of r8a78000.
+ */
+#define MFIS_CHANNEL_IICR	(0 << 1)
+#define MFIS_CHANNEL_EICR	(1 << 1)
+
+#endif
-- 
2.51.0

[PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver

[Wolfram Sang]

Renesas R-Car MFIS offers multiple features but most importantly
mailboxes and hwspinlocks. Because they share a common register space
and a common register unprotection mechanism, a single driver was chosen
to handle all dependencies. (MFD and auxiliary bus have been tried as
well, but they failed because of circular dependencies.)

In this first step, the driver implements common register access and a
mailbox controller. hwspinlock support will be added incrementally, once
the subsystem allows out-of-directory drivers.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

This driver is already prepared for upcoming r8a78001 support which will
break away from the IICR/EICR register pair. Actual support will be
added later once HW is available.

 drivers/soc/renesas/Kconfig     |   9 +
 drivers/soc/renesas/Makefile    |   1 +
 drivers/soc/renesas/rcar-mfis.c | 325 ++++++++++++++++++++++++++++++++
 3 files changed, 335 insertions(+)
 create mode 100644 drivers/soc/renesas/rcar-mfis.c

diff --git a/drivers/soc/renesas/Kconfig b/drivers/soc/renesas/Kconfig
index 26bed0fdceb0..6c472c951ab3 100644
--- a/drivers/soc/renesas/Kconfig
+++ b/drivers/soc/renesas/Kconfig
@@ -465,6 +465,15 @@ config ARCH_R9A07G043
 
 endif # RISCV
 
+config RCAR_MFIS
+	tristate "Renesas R-Car MFIS driver"
+	depends on ARCH_RENESAS || COMPILE_TEST
+	depends on MAILBOX
+	help
+	  Select this option to enable the Renesas R-Car MFIS core driver for
+	  the MFIS device found on SoCs like R-Car. On families like Gen5, this
+	  is needed to communicate with the SCP.
+
 config PWC_RZV2M
 	bool "Renesas RZ/V2M PWC support" if COMPILE_TEST
 
diff --git a/drivers/soc/renesas/Makefile b/drivers/soc/renesas/Makefile
index 655dbcb08747..21eb78a05fc0 100644
--- a/drivers/soc/renesas/Makefile
+++ b/drivers/soc/renesas/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_SYS_R9A09G056)	+= r9a09g056-sys.o
 obj-$(CONFIG_SYS_R9A09G057)	+= r9a09g057-sys.o
 
 # Family
+obj-$(CONFIG_RCAR_MFIS)		+= rcar-mfis.o
 obj-$(CONFIG_PWC_RZV2M)		+= pwc-rzv2m.o
 obj-$(CONFIG_RST_RCAR)		+= rcar-rst.o
 obj-$(CONFIG_RZN1_IRQMUX)	+= rzn1_irqmux.o
diff --git a/drivers/soc/renesas/rcar-mfis.c b/drivers/soc/renesas/rcar-mfis.c
new file mode 100644
index 000000000000..5fb9cd352b76
--- /dev/null
+++ b/drivers/soc/renesas/rcar-mfis.c
@@ -0,0 +1,325 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Renesas R-Car MFIS (Multifunctional Interface) driver
+ *
+ * Copyright (C) Renesas Solutions Corp.
+ * Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
+ * Wolfram Sang <wsa+renesas@sang-engineering.com>
+ */
+#include <dt-bindings/mailbox/renesas,r8a78000-mfis.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+#define MFISWPCNTR	0x0900
+#define MFISWACNTR	0x0904
+
+#define MFIS_UNPROTECT_KEY 0xACCE0000
+
+struct mfis_priv;
+
+struct mfis_reg {
+	void __iomem *base;
+	resource_size_t start;
+	struct mfis_priv *priv;
+};
+
+struct mfis_info {
+	u32 unprotect_mask;
+	unsigned int mb_num_channels;
+	unsigned int mb_reg_comes_from_dt:1;
+	unsigned int mb_tx_uses_eicr:1;
+	unsigned int mb_channels_are_unidir:1;
+};
+
+struct mfis_per_chan_priv {
+	u32 reg;
+	int irq;
+};
+
+struct mfis_priv {
+	struct device *dev;
+	struct mfis_reg common_reg;
+	struct mfis_reg mbox_reg;
+	const struct mfis_info *info;
+
+	/* mailbox private data */
+	struct mbox_controller mbox;
+	struct mfis_per_chan_priv *per_chan_privs;
+};
+
+static u32 mfis_read(struct mfis_reg *mreg, unsigned int reg)
+{
+	return ioread32(mreg->base + reg);
+}
+
+static void mfis_write(struct mfis_reg *mreg, u32 reg, u32 val)
+{
+	struct mfis_priv *priv = mreg->priv;
+	u32 unprotect_mask = priv->info->unprotect_mask;
+	u32 unprotect_code;
+
+	/*
+	 * [Gen4] key: 0xACCE0000, mask: 0x0000FFFF
+	 * [Gen5] key: 0xACC00000, mask: 0x000FFFFF
+	 */
+	unprotect_code = (MFIS_UNPROTECT_KEY & ~unprotect_mask) |
+			 ((mreg->start | reg) & unprotect_mask);
+
+	iowrite32(unprotect_code, priv->common_reg.base + MFISWACNTR);
+	iowrite32(val, mreg->base + reg);
+}
+
+/****************************************************
+ *			Mailbox
+ ****************************************************/
+
+#define mfis_mb_mbox_to_priv(_m) container_of((_m), struct mfis_priv, mbox)
+
+static irqreturn_t mfis_mb_iicr_interrupt(int irq, void *data)
+{
+	struct mbox_chan *chan = data;
+	struct mfis_priv *priv = mfis_mb_mbox_to_priv(chan->mbox);
+	struct mfis_per_chan_priv *per_chan_priv = chan->con_priv;
+
+	mbox_chan_received_data(chan, NULL);
+	/* Stop remote(!) doorbell */
+	mfis_write(&priv->mbox_reg, per_chan_priv->reg, 0);
+
+	return IRQ_HANDLED;
+}
+
+static int mfis_mb_startup(struct mbox_chan *chan)
+{
+	struct mfis_per_chan_priv *per_chan_priv = chan->con_priv;
+	int ret = 0;
+
+	if (per_chan_priv->irq)
+		ret = request_irq(per_chan_priv->irq, mfis_mb_iicr_interrupt, 0,
+				  dev_name(chan->mbox->dev), chan);
+
+	return ret;
+}
+
+static void mfis_mb_shutdown(struct mbox_chan *chan)
+{
+	struct mfis_per_chan_priv *per_chan_priv = chan->con_priv;
+
+	free_irq(per_chan_priv->irq, chan);
+}
+
+static int mfis_mb_iicr_send_data(struct mbox_chan *chan, void *data)
+{
+	struct mfis_priv *priv = mfis_mb_mbox_to_priv(chan->mbox);
+	struct mfis_per_chan_priv *per_chan_priv = chan->con_priv;
+
+	/* Our doorbell still active? */
+	if (mfis_read(&priv->mbox_reg, per_chan_priv->reg) & 1)
+		return -EBUSY;
+
+	/* Start our doorbell */
+	mfis_write(&priv->mbox_reg, per_chan_priv->reg, 1);
+
+	return 0;
+}
+
+static bool mfis_mb_iicr_last_tx_done(struct mbox_chan *chan)
+{
+	struct mfis_priv *priv = mfis_mb_mbox_to_priv(chan->mbox);
+	struct mfis_per_chan_priv *per_chan_priv = chan->con_priv;
+
+	/* Our doorbell still active? */
+	return !(mfis_read(&priv->mbox_reg, per_chan_priv->reg) & 1);
+}
+
+/* For MFIS variants using the IICR/EICR register pair */
+static const struct mbox_chan_ops mfis_iicr_ops = {
+	.startup = mfis_mb_startup,
+	.shutdown = mfis_mb_shutdown,
+	.send_data = mfis_mb_iicr_send_data,
+	.last_tx_done = mfis_mb_iicr_last_tx_done,
+};
+
+static struct mbox_chan *mfis_mb_of_xlate(struct mbox_controller *mbox,
+					  const struct of_phandle_args *sp)
+{
+	struct mfis_priv *priv = mfis_mb_mbox_to_priv(mbox);
+	struct mfis_per_chan_priv *per_chan_priv;
+	struct mbox_chan *chan;
+	u32 chan_num, chan_flags;
+	bool tx_uses_eicr, is_only_rx;
+
+	if (sp->args_count != 2)
+		return ERR_PTR(-EINVAL);
+
+	chan_num = sp->args[0];
+	chan_flags = sp->args[1];
+
+	/* Channel layout is described in mfis_mb_probe() */
+	if (priv->info->mb_channels_are_unidir) {
+		is_only_rx = chan_flags & MFIS_CHANNEL_RX;
+		chan = mbox->chans + 2 * chan_num + is_only_rx;
+	} else {
+		is_only_rx = false;
+		chan = mbox->chans + chan_num;
+	}
+
+	if (priv->info->mb_reg_comes_from_dt) {
+		tx_uses_eicr = chan_flags & MFIS_CHANNEL_EICR;
+		if (tx_uses_eicr)
+			chan += mbox->num_chans / 2;
+	} else {
+		tx_uses_eicr = priv->info->mb_tx_uses_eicr;
+	}
+
+	per_chan_priv = chan->con_priv;
+	per_chan_priv->reg = chan_num * 0x1000 + (tx_uses_eicr ^ is_only_rx) * 4;
+
+	if (!priv->info->mb_channels_are_unidir || is_only_rx) {
+		char irqname[8];
+		char suffix = tx_uses_eicr ? 'i' : 'e';
+
+		/* "ch0i" or "ch0e" */
+		scnprintf(irqname, sizeof(irqname), "ch%d%c", chan_num, suffix);
+
+		per_chan_priv->irq = of_irq_get_byname(mbox->dev->of_node, irqname);
+		if (per_chan_priv->irq < 0)
+			return ERR_PTR(per_chan_priv->irq);
+		else if (per_chan_priv->irq == 0)
+			return ERR_PTR(-ENOENT);
+	}
+
+	return chan;
+}
+
+static int mfis_mb_probe(struct mfis_priv *priv)
+{
+	struct device *dev = priv->dev;
+	struct mbox_chan *chan;
+	struct mbox_controller *mbox;
+	unsigned int i, num_chan = priv->info->mb_num_channels;
+
+	if (priv->info->mb_channels_are_unidir)
+		/* Channel layout: Ch0-TX, Ch0-RX, Ch1-TX... */
+		num_chan *= 2;
+
+	if (priv->info->mb_reg_comes_from_dt)
+		/* Channel layout: <n> IICR channels, <n> EICR channels */
+		num_chan *= 2;
+
+	chan  = devm_kcalloc(dev, num_chan, sizeof(*chan), GFP_KERNEL);
+	if (!chan)
+		return -ENOMEM;
+
+	priv->per_chan_privs = devm_kcalloc(dev, num_chan, sizeof(*priv->per_chan_privs),
+					    GFP_KERNEL);
+	if (!priv->per_chan_privs)
+		return -ENOMEM;
+
+	mbox = &priv->mbox;
+
+	for (i = 0; i < num_chan; i++)
+		chan[i].con_priv = &priv->per_chan_privs[i];
+
+	mbox->chans = chan;
+	mbox->num_chans = num_chan;
+	mbox->txdone_poll = true;
+	mbox->ops = &mfis_iicr_ops;
+	mbox->dev = dev;
+	mbox->of_xlate = mfis_mb_of_xlate;
+
+	return mbox_controller_register(mbox);
+}
+
+/****************************************************
+ *		Common
+ ****************************************************/
+static int mfis_reg_probe(struct platform_device *pdev, struct mfis_priv *priv,
+			  struct mfis_reg *mreg, const char *name, bool required)
+{
+	struct resource *res;
+	void __iomem *base;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
+
+	/* If there is no mailbox resource, registers are in the common space */
+	if (!res && !required) {
+		priv->mbox_reg = priv->common_reg;
+	} else {
+		base = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(base))
+			return PTR_ERR(base);
+
+		mreg->base = base;
+		mreg->start = res->start;
+		mreg->priv = priv;
+	}
+
+	return 0;
+}
+
+static int mfis_probe(struct platform_device *pdev)
+{
+	struct mfis_priv *priv;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	priv->info = of_device_get_match_data(dev);
+
+	ret = mfis_reg_probe(pdev, priv, &priv->common_reg, "common", true);
+	if (ret)
+		return ret;
+
+	ret = mfis_reg_probe(pdev, priv, &priv->mbox_reg, "mboxes", false);
+	if (ret)
+		return ret;
+
+	return mfis_mb_probe(priv);
+}
+
+static const struct mfis_info mfis_info_r8a78000 = {
+	.unprotect_mask	= 0x000fffff,
+	.mb_num_channels = 64,
+	.mb_reg_comes_from_dt = true,
+	.mb_channels_are_unidir = true,
+};
+
+static const struct mfis_info mfis_info_r8a78000_scp = {
+	.unprotect_mask	= 0x000fffff,
+	.mb_num_channels = 32,
+	.mb_tx_uses_eicr = true,
+	.mb_channels_are_unidir = true,
+};
+
+static const struct of_device_id mfis_mfd_of_match[] = {
+	{ .compatible = "renesas,r8a78000-mfis", .data = &mfis_info_r8a78000, },
+	{ .compatible = "renesas,r8a78000-mfis-scp", .data = &mfis_info_r8a78000_scp, },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mfis_mfd_of_match);
+
+static struct platform_driver mfis_driver = {
+	.driver = {
+		.name = "rcar-mfis",
+		.of_match_table = mfis_mfd_of_match,
+	},
+	.probe	= mfis_probe,
+};
+module_platform_driver(mfis_driver);
+
+MODULE_AUTHOR("Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>");
+MODULE_AUTHOR("Wolfram Sang <wsa+renesas@sang-engineering.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Renesas R-Car MFIS driver");
-- 
2.51.0

[PATCH 3/3] soc: renesas: add X5H PRR support

[Wolfram Sang]

On this SoC, PRR is now inside the MFIS memory block, so we need to
access it similar to e.g. RZ/G2L.

Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/soc/renesas/renesas-soc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/renesas/renesas-soc.c b/drivers/soc/renesas/renesas-soc.c
index 38ff0b823bda..60b09020c935 100644
--- a/drivers/soc/renesas/renesas-soc.c
+++ b/drivers/soc/renesas/renesas-soc.c
@@ -442,8 +442,14 @@ static const struct renesas_id id_prr __initconst = {
 	.mask = 0xff00,
 };
 
+static const struct renesas_id id_mfis __initconst = {
+	.offset = 0x44,
+	.mask = 0xff00,
+};
+
 static const struct of_device_id renesas_ids[] __initconst = {
 	{ .compatible = "renesas,bsid",			.data = &id_bsid },
+	{ .compatible = "renesas,r8a78000-mfis",	.data = &id_mfis },
 	{ .compatible = "renesas,r9a07g043-sysc",	.data = &id_rzg2l },
 	{ .compatible = "renesas,r9a07g044-sysc",	.data = &id_rzg2l },
 	{ .compatible = "renesas,r9a07g054-sysc",	.data = &id_rzg2l },
@@ -501,7 +507,7 @@ static int __init renesas_soc_init(void)
 		product = readl(chipid + id->offset);
 		iounmap(chipid);
 
-		if (id == &id_prr) {
+		if (id == &id_prr || id == &id_mfis) {
 			/* R-Car M3-W ES1.1 incorrectly identifies as ES2.0 */
 			if ((product & 0x7fff) == 0x5210)
 				product ^= 0x11;
-- 
2.51.0

Re: [PATCH 1/3] dt-bindings: soc: renesas: add MFIS binding documentation

[Krzysztof Kozlowski]

On Tue, Mar 17, 2026 at 02:06:34PM +0100, Wolfram Sang wrote:
> Add device tree bindings for the Renesas Multifunctional Interface
> (MFIS) as found on the Renesas R-Car X5H (r8a78000) SoC. MFIS includes
> features like Mailbox/HW Spinlock/Product Register.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Checked with 'dt_bindings_check'. Family-compatible values are not
> introduced here because MFIS is usually very different per SoC.

Not sure with what family this would be compatible, but anyway this
should be explained in commit msg.

> 
>  .../soc/renesas/renesas,r8a78000-mfis.yaml    | 160 ++++++++++++++++++
>  .../mailbox/renesas,r8a78000-mfis.h           |  27 +++
>  2 files changed, 187 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/renesas/renesas,r8a78000-mfis.yaml
>  create mode 100644 include/dt-bindings/mailbox/renesas,r8a78000-mfis.h
> 
> diff --git a/Documentation/devicetree/bindings/soc/renesas/renesas,r8a78000-mfis.yaml b/Documentation/devicetree/bindings/soc/renesas/renesas,r8a78000-mfis.yaml
> new file mode 100644
> index 000000000000..dbda28ac781c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/renesas/renesas,r8a78000-mfis.yaml
> @@ -0,0 +1,160 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/renesas/renesas,r8a78000-mfis.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas MFIS (Multifunctional Interface) controller
> +
> +maintainers:
> +  - Wolfram Sang <wsa+renesas@sang-engineering.com>
> +
> +description:
> +  Renesas Multifunctional Interface (MFIS) provides functionality for
> +  communication between different CPU cores. Those cores can be in various

so kind of remoteproc? The soc directory is dumping ground, so you
should find something more suitable if possible.

> +  domains like AP, RT, or SCP. Functionality includes features like
> +  mailboxes, hardware spinlocks and such.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - renesas,r8a78000-mfis       # R-Car X5H (AP<->AP, with PRR)
> +      - renesas,r8a78000-mfis-scp   # R-Car X5H (AP<->SCP, without PRR)
> +
> +  reg:
> +    minItems: 2

Drop

> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: common
> +      - const: mboxes
> +
> +  interrupts:
> +    minItems: 32
> +    maxItems: 128
> +    description:
> +      The interrupts raised by the remote doorbells.
> +
> +  interrupt-names:
> +    minItems: 32
> +    maxItems: 128
> +    items:
> +      pattern: "^ch[0-9]+[ie]$"
> +    description:
> +      An interrupt name is constructed with the prefix 'ch'. Then, the
> +      channel number as specified in the documentation of the SoC. Finally,
> +      the letter 'i' if the interrupt is raised by the IICR register. Or 'e'
> +      if it is raised by the EICR register.

Describe why is this flexible. These are fixed devices, very specific
SoCs. They do not come with randomly routed interrupts, usually.

> +
> +  "#hwlock-cells":
> +    const: 1
> +
> +  "#mbox-cells":
> +    const: 2
> +    description:
> +      The first cell is the channel number as specified in the documentation
> +      of the SoC. The second cell may specify flags as described in the file
> +      <dt-bindings/mailbox/renesas,r8a78000-mfis.h>.
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - interrupt-names
> +  - "#hwlock-cells"
> +  - "#mbox-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    mfis: syscon@189e0000 {

Drop label and rename node - that's not syscon.

> +            compatible = "renesas,r8a78000-mfis";
> +            reg = <0x189e0000 0x1000>, <0x18800000 0x40000>;
> +            reg-names = "common", "mboxes";
> +            interrupts = <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 113 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 127 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 128 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 129 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 132 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 136 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 139 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 141 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 142 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 151 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 152 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 166 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 167 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 168 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 171 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 172 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 173 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 175 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 178 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 180 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 181 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 182 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 183 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 184 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 185 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 186 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 187 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 190 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 191 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 199 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 200 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 201 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 202 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 203 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 204 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 205 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 206 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 210 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 211 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 212 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 213 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 214 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 215 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 216 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 217 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 218 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 219 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 220 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 221 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 222 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 225 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 227 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 228 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "ch0i", "ch0e", "ch1i", "ch1e", "ch2i", "ch2e", "ch3i", "ch3e",
> +                              "ch4i", "ch4e", "ch5i", "ch5e", "ch6i", "ch6e", "ch7i", "ch7e",
> +                              "ch8i", "ch8e", "ch9i", "ch9e", "ch10i", "ch10e", "ch11i", "ch11e",
> +                              "ch12i", "ch12e", "ch13i", "ch13e", "ch14i", "ch14e", "ch15i", "ch15e",
> +                              "ch16i", "ch16e", "ch17i", "ch17e", "ch18i", "ch18e", "ch19i", "ch19e",
> +                              "ch20i", "ch20e", "ch21i", "ch21e", "ch22i", "ch22e", "ch23i", "ch23e",
> +                              "ch24i", "ch24e", "ch25i", "ch25e", "ch26i", "ch26e", "ch27i", "ch27e",
> +                              "ch28i", "ch28e", "ch29i", "ch29e", "ch30i", "ch30e", "ch31i", "ch31e",
> +                              "ch32i", "ch32e", "ch33i", "ch33e", "ch34i", "ch34e", "ch35i", "ch35e",
> +                              "ch36i", "ch36e", "ch37i", "ch37e", "ch38i", "ch38e", "ch39i", "ch39e",
> +                              "ch40i", "ch40e", "ch41i", "ch41e", "ch42i", "ch42e", "ch43i", "ch43e",
> +                              "ch44i", "ch44e", "ch45i", "ch45e", "ch46i", "ch46e", "ch47i", "ch47e",
> +                              "ch48i", "ch48e", "ch49i", "ch49e", "ch50i", "ch50e", "ch51i", "ch51e",
> +                              "ch52i", "ch52e", "ch53i", "ch53e", "ch54i", "ch54e", "ch55i", "ch55e",
> +                              "ch56i", "ch56e", "ch57i", "ch57e", "ch58i", "ch58e", "ch59i", "ch59e",
> +                              "ch60i", "ch60e", "ch61i", "ch61e", "ch62i", "ch62e", "ch63i", "ch63e";
> +            #hwlock-cells = <1>;
> +            #mbox-cells = <2>;
> +    };
> diff --git a/include/dt-bindings/mailbox/renesas,r8a78000-mfis.h b/include/dt-bindings/mailbox/renesas,r8a78000-mfis.h
> new file mode 100644
> index 000000000000..89489c2a4847
> --- /dev/null
> +++ b/include/dt-bindings/mailbox/renesas,r8a78000-mfis.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * Constants for the mailbox part of the Renesas MFIS IP core.
> + */
> +
> +#ifndef _DT_BINDINGS_MAILBOX_RENESAS_MFIS_H
> +#define _DT_BINDINGS_MAILBOX_RENESAS_MFIS_H
> +
> +/*
> + * MFIS HW design before r8a78001 requires a channel to be marked as either
> + * TX or RX.
> + */
> +#define MFIS_CHANNEL_TX	(0 << 0)

0, bindings constants are abstract (so without dedicated meaning)
numbers, starting from 0 or 1 and incremented by 1. Shifting this
implies there is some other logic and that would mean - not a binding.

> +#define MFIS_CHANNEL_RX	(1 << 0)

1


> +
> +/*
> + * MFIS variants before r8a78001 work with pairs of IICR and EICR registers.
> + * Usually, it is specified in the datasheets which of the two a specific core
> + * should use. Then, it does not need extra description in DT. For plain MFIS
> + * of r8a78000, this is selectable, though. According to the system design and
> + * the firmware in use, these channels need to be marked. This is not needed
> + * with other versions of the MFIS, not even with MFIS-SCP of r8a78000.
> + */
> +#define MFIS_CHANNEL_IICR	(0 << 1)
> +#define MFIS_CHANNEL_EICR	(1 << 1)

Same here.

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: soc: renesas: add MFIS binding documentation

[Krzysztof Kozlowski]

On Tue, Mar 17, 2026 at 02:06:34PM +0100, Wolfram Sang wrote:
> Add device tree bindings for the Renesas Multifunctional Interface
> (MFIS) as found on the Renesas R-Car X5H (r8a78000) SoC. MFIS includes
> features like Mailbox/HW Spinlock/Product Register.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Checked with 'dt_bindings_check'. Family-compatible values are not
> introduced here because MFIS is usually very different per SoC.

Ah, also, you hit 2/3 combo in your subject. You only need third for the
bingo - YAML.

A nit, subject: drop second/last, redundant "binding documentation". The
"dt-bindings" prefix is already stating that these are bindings and
documentation.
See also:
https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18


Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: soc: renesas: add MFIS binding documentation

[Krzysztof Kozlowski]

On 18/03/2026 10:17, Krzysztof Kozlowski wrote:
>> +
>> +title: Renesas MFIS (Multifunctional Interface) controller
>> +
>> +maintainers:
>> +  - Wolfram Sang <wsa+renesas@sang-engineering.com>
>> +
>> +description:
>> +  Renesas Multifunctional Interface (MFIS) provides functionality for
>> +  communication between different CPU cores. Those cores can be in various
> 
> so kind of remoteproc? The soc directory is dumping ground, so you
> should find something more suitable if possible.

or mailbox?

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: soc: renesas: add MFIS binding documentation

[Geert Uytterhoeven]

Hi Krzysztof,

On Wed, 18 Mar 2026 at 10:17, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Tue, Mar 17, 2026 at 02:06:34PM +0100, Wolfram Sang wrote:
> > Add device tree bindings for the Renesas Multifunctional Interface
> > (MFIS) as found on the Renesas R-Car X5H (r8a78000) SoC. MFIS includes
> > features like Mailbox/HW Spinlock/Product Register.
> >
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/renesas/renesas,r8a78000-mfis.yaml

> > +  "#hwlock-cells":
> > +    const: 1
> > +
> > +  "#mbox-cells":
> > +    const: 2
> > +    description:
> > +      The first cell is the channel number as specified in the documentation
> > +      of the SoC. The second cell may specify flags as described in the file
> > +      <dt-bindings/mailbox/renesas,r8a78000-mfis.h>.

> > --- /dev/null
> > +++ b/include/dt-bindings/mailbox/renesas,r8a78000-mfis.h
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > +/*
> > + * Constants for the mailbox part of the Renesas MFIS IP core.
> > + */
> > +
> > +#ifndef _DT_BINDINGS_MAILBOX_RENESAS_MFIS_H
> > +#define _DT_BINDINGS_MAILBOX_RENESAS_MFIS_H
> > +
> > +/*
> > + * MFIS HW design before r8a78001 requires a channel to be marked as either
> > + * TX or RX.
> > + */
> > +#define MFIS_CHANNEL_TX      (0 << 0)
>
> 0, bindings constants are abstract (so without dedicated meaning)
> numbers, starting from 0 or 1 and incremented by 1. Shifting this
> implies there is some other logic and that would mean - not a binding.
>
> > +#define MFIS_CHANNEL_RX      (1 << 0)
>
> 1
>
>
> > +
> > +/*
> > + * MFIS variants before r8a78001 work with pairs of IICR and EICR registers.
> > + * Usually, it is specified in the datasheets which of the two a specific core
> > + * should use. Then, it does not need extra description in DT. For plain MFIS
> > + * of r8a78000, this is selectable, though. According to the system design and
> > + * the firmware in use, these channels need to be marked. This is not needed
> > + * with other versions of the MFIS, not even with MFIS-SCP of r8a78000.
> > + */
> > +#define MFIS_CHANNEL_IICR    (0 << 1)
> > +#define MFIS_CHANNEL_EICR    (1 << 1)
>
> Same here.

These are flags as the documentation for the #mbox-cells property in the
bindings file states, to be ORed.

E.g. include/dt-bindings/i2c/i2c.h and include/dt-bindings/i3c/i3c.h
also use shifts to make this clear.
include/dt-bindings/gpio/gpio.h uses "Bit N express ..." comments instead.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/3] dt-bindings: soc: renesas: add MFIS binding documentation

[Krzysztof Kozlowski]

On 19/03/2026 09:54, Geert Uytterhoeven wrote:
>>
>>> +
>>> +/*
>>> + * MFIS variants before r8a78001 work with pairs of IICR and EICR registers.
>>> + * Usually, it is specified in the datasheets which of the two a specific core
>>> + * should use. Then, it does not need extra description in DT. For plain MFIS
>>> + * of r8a78000, this is selectable, though. According to the system design and
>>> + * the firmware in use, these channels need to be marked. This is not needed
>>> + * with other versions of the MFIS, not even with MFIS-SCP of r8a78000.
>>> + */
>>> +#define MFIS_CHANNEL_IICR    (0 << 1)
>>> +#define MFIS_CHANNEL_EICR    (1 << 1)
>>
>> Same here.
> 
> These are flags as the documentation for the #mbox-cells property in the
> bindings file states, to be ORed.

I did not get the driver so I cannot verify that. What sort of Linux ABI
does this bind?

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: soc: renesas: add MFIS binding documentation

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, 17 Mar 2026 at 14:06, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Add device tree bindings for the Renesas Multifunctional Interface
> (MFIS) as found on the Renesas R-Car X5H (r8a78000) SoC. MFIS includes
> features like Mailbox/HW Spinlock/Product Register.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/renesas/renesas,r8a78000-mfis.yaml
> @@ -0,0 +1,160 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/renesas/renesas,r8a78000-mfis.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas MFIS (Multifunctional Interface) controller
> +
> +maintainers:
> +  - Wolfram Sang <wsa+renesas@sang-engineering.com>
> +
> +description:
> +  Renesas Multifunctional Interface (MFIS) provides functionality for

The Renesas Multifunctional Interface ...

> +  communication between different CPU cores. Those cores can be in various
> +  domains like AP, RT, or SCP. Functionality includes features like
> +  mailboxes, hardware spinlocks and such.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - renesas,r8a78000-mfis       # R-Car X5H (AP<->AP, with PRR)
> +      - renesas,r8a78000-mfis-scp   # R-Car X5H (AP<->SCP, without PRR)

[...]

> +  interrupts:
> +    minItems: 32
> +    maxItems: 128
> +    description:
> +      The interrupts raised by the remote doorbells.
> +
> +  interrupt-names:
> +    minItems: 32
> +    maxItems: 128
> +    items:
> +      pattern: "^ch[0-9]+[ie]$"
> +    description:
> +      An interrupt name is constructed with the prefix 'ch'. Then, the
> +      channel number as specified in the documentation of the SoC. Finally,
> +      the letter 'i' if the interrupt is raised by the IICR register. Or 'e'
> +      if it is raised by the EICR register.

maxItems could be moved to a conditional schema, based on the compatible
value.
The same is true for the pattern rule, as MFIS has both "i" and "e"
interrupts, while MFIS-SCP has only the "i" variants.

> --- /dev/null
> +++ b/include/dt-bindings/mailbox/renesas,r8a78000-mfis.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * Constants for the mailbox part of the Renesas MFIS IP core.
> + */
> +
> +#ifndef _DT_BINDINGS_MAILBOX_RENESAS_MFIS_H
> +#define _DT_BINDINGS_MAILBOX_RENESAS_MFIS_H

The include protection does not match the filename.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/3] soc: renesas: add X5H PRR support

[Geert Uytterhoeven]

On Tue, 17 Mar 2026 at 14:06, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> On this SoC, PRR is now inside the MFIS memory block, so we need to
> access it similar to e.g. RZ/G2L.
>
> Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel, pending acceptance of the DT bindings.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/3] dt-bindings: soc: renesas: add MFIS binding documentation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 4736 bytes --]

Hi Krzysztof,

thanks for the review!

> > 
> > Checked with 'dt_bindings_check'. Family-compatible values are not
> > introduced here because MFIS is usually very different per SoC.
> 
> Not sure with what family this would be compatible, but anyway this
> should be explained in commit msg.

The family would be "rcar-gen5". It was a close call where to put this
paragraph. I can put it to the commit messge as well.

> > +description:
> > +  Renesas Multifunctional Interface (MFIS) provides functionality for
> > +  communication between different CPU cores. Those cores can be in various
> 
> so kind of remoteproc? The soc directory is dumping ground, so you
> should find something more suitable if possible.

I did this. And this was the conclusion because the IP core *is* kind of
a "dumping ground". This paragraph currently might be biased by what we
want support in Linux as of now, like mailboxes and hwspinlock and
product register. But it actually can have more like boot status, error
injection, error detection... and who knows what will be added in the
future. Since DT is HW description, I will update this paragraph to
mention the extra MFIS features even if there is no Linux support
planned.

> > +  reg:
> > +    minItems: 2
> 
> Drop

Ack.

> > +    description:
> > +      An interrupt name is constructed with the prefix 'ch'. Then, the
> > +      channel number as specified in the documentation of the SoC. Finally,
> > +      the letter 'i' if the interrupt is raised by the IICR register. Or 'e'
> > +      if it is raised by the EICR register.
> 
> Describe why is this flexible. These are fixed devices, very specific
> SoCs. They do not come with randomly routed interrupts, usually.

It is not flexible. The interrupts are static and so should be the
naming. The above paragraph aims to describe that. Let me cite part of
the example for "renesas,r8a78000-mfis" from below where all interrupts
(triggered by both, the IICR/EICR registers) are routed to the AP core
interrupt controller. Note the alternating 'i/e' pattern:

	interrupt-names = "ch0i", "ch0e", "ch1i", "ch1e", "ch2i", "ch2e", "ch3i", "ch3e",
			  "ch4i", "ch4e", "ch5i", "ch5e", "ch6i", "ch6e", "ch7i", "ch7e",
			  ...

In comparison, "renesas,r8a78000-mfis-scp" where only the interrupts
triggered by the IICR register are routed to the AP (the EICR interrupts
are routed to the firmware controller). 'i' pattern only:

	interrupt-names = "ch0i", "ch1i", "ch2i", "ch3i", "ch4i", "ch5i", "ch6i", "ch7i",
			  "ch8i", "ch9i", "ch10i", "ch11i", "ch12i", "ch13i", "ch14i", "ch15i",
			  ...

I think the above description is technically correct. Maybe it is not
easy to understand, though. One improvement I could think of: add the
SCP case to the example section as well, so the difference is more
obvious. Note: I decided to use 'pattern:' for describing the
interrupt-names to avoid 128 const entries for the IICR/EICR case, and
64 const entries per IICR-only or EICR-only case.

> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    mfis: syscon@189e0000 {
> 
> Drop label and rename node - that's not syscon.

Yes to drop label. Not sure about renaming the node. Given the above of
the "dumping ground IP core", it is way more of a syscon than of a
mailbox controller. If there is a better common name of such a device, I
don't know of it and couldn't find it when grepping around. "remoteproc"
might be somewhat closer but still...

> > diff --git a/include/dt-bindings/mailbox/renesas,r8a78000-mfis.h b/include/dt-bindings/mailbox/renesas,r8a78000-mfis.h
> > new file mode 100644
> > index 000000000000..89489c2a4847
> > --- /dev/null
> > +++ b/include/dt-bindings/mailbox/renesas,r8a78000-mfis.h

I am wondering now if this is the right place for the include file. My
original intention was that it should go to the mailbox-dir because it
handles only the mailbox-part (second #mbox-cell) of the MFIS. But
maybe all-in-one is better? Other flags are not in flight currently,
though.

> > +/*
> > + * MFIS HW design before r8a78001 requires a channel to be marked as either
> > + * TX or RX.
> > + */
> > +#define MFIS_CHANNEL_TX	(0 << 0)
> 
> 0, bindings constants are abstract (so without dedicated meaning)
> numbers, starting from 0 or 1 and incremented by 1. Shifting this
> implies there is some other logic and that would mean - not a binding.

I'll give in and do it. Using fixed numbers instead of bit numbers
sounds more error prone to me, though. But we don't need to discuss
this, I'll adapt.

And I will change the $subject, too.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, 17 Mar 2026 at 14:06, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Renesas R-Car MFIS offers multiple features but most importantly
> mailboxes and hwspinlocks. Because they share a common register space
> and a common register unprotection mechanism, a single driver was chosen
> to handle all dependencies. (MFD and auxiliary bus have been tried as
> well, but they failed because of circular dependencies.)
>
> In this first step, the driver implements common register access and a
> mailbox controller. hwspinlock support will be added incrementally, once
> the subsystem allows out-of-directory drivers.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/soc/renesas/rcar-mfis.c

> +struct mfis_info {
> +       u32 unprotect_mask;
> +       unsigned int mb_num_channels;
> +       unsigned int mb_reg_comes_from_dt:1;
> +       unsigned int mb_tx_uses_eicr:1;
> +       unsigned int mb_channels_are_unidir:1;
> +};
> +
> +struct mfis_per_chan_priv {

I would drop the "per_".

> +       u32 reg;
> +       int irq;
> +};
> +
> +struct mfis_priv {
> +       struct device *dev;
> +       struct mfis_reg common_reg;
> +       struct mfis_reg mbox_reg;
> +       const struct mfis_info *info;
> +
> +       /* mailbox private data */
> +       struct mbox_controller mbox;
> +       struct mfis_per_chan_priv *per_chan_privs;

Likewise.
This could be a flexible array, if it weren't for the hwspinlock array
you will have to add later, right?

> +};
> +
> +static u32 mfis_read(struct mfis_reg *mreg, unsigned int reg)
> +{
> +       return ioread32(mreg->base + reg);
> +}
> +
> +static void mfis_write(struct mfis_reg *mreg, u32 reg, u32 val)

Both writel() and iowrite32() use the inverse order of "reg" and "val".
But I can understand you want to keep "mreg" and "reg" together.

> +{
> +       struct mfis_priv *priv = mreg->priv;
> +       u32 unprotect_mask = priv->info->unprotect_mask;
> +       u32 unprotect_code;
> +
> +       /*
> +        * [Gen4] key: 0xACCE0000, mask: 0x0000FFFF
> +        * [Gen5] key: 0xACC00000, mask: 0x000FFFFF
> +        */
> +       unprotect_code = (MFIS_UNPROTECT_KEY & ~unprotect_mask) |
> +                        ((mreg->start | reg) & unprotect_mask);
> +
> +       iowrite32(unprotect_code, priv->common_reg.base + MFISWACNTR);
> +       iowrite32(val, mreg->base + reg);
> +}
> +
> +/****************************************************
> + *                     Mailbox

Missing closing asterisk ;-)

> + ****************************************************/
> +
> +#define mfis_mb_mbox_to_priv(_m) container_of((_m), struct mfis_priv, mbox)

Both "mb" and "mbox", so perhaps mfis_mbox_to_priv()?
And perhaps s/mb_/mbox_/ everywhere?

> +static int mfis_mb_startup(struct mbox_chan *chan)
> +{
> +       struct mfis_per_chan_priv *per_chan_priv = chan->con_priv;
> +       int ret = 0;
> +
> +       if (per_chan_priv->irq)
> +               ret = request_irq(per_chan_priv->irq, mfis_mb_iicr_interrupt, 0,
> +                                 dev_name(chan->mbox->dev), chan);
> +
> +       return ret;

You can reduce indentation, and get rid of ret, by inverting the
conditional.

> +}
> +
> +static void mfis_mb_shutdown(struct mbox_chan *chan)
> +{
> +       struct mfis_per_chan_priv *per_chan_priv = chan->con_priv;
> +
> +       free_irq(per_chan_priv->irq, chan);

if (per_chan_priv->irq) ...

free_irq() seems to support zero, but irq_to_desc() still has to
traverse the mtree.

> +}

> +static struct mbox_chan *mfis_mb_of_xlate(struct mbox_controller *mbox,
> +                                         const struct of_phandle_args *sp)
> +{
> +       struct mfis_priv *priv = mfis_mb_mbox_to_priv(mbox);
> +       struct mfis_per_chan_priv *per_chan_priv;
> +       struct mbox_chan *chan;
> +       u32 chan_num, chan_flags;
> +       bool tx_uses_eicr, is_only_rx;
> +
> +       if (sp->args_count != 2)
> +               return ERR_PTR(-EINVAL);
> +
> +       chan_num = sp->args[0];

"chan_num" is the hardware channel number, and should be validated
against mpriv->info->mb_num_channels.

> +       chan_flags = sp->args[1];
> +
> +       /* Channel layout is described in mfis_mb_probe() */

Given you already use "chan += ..." below, perhaps:

    chan = mbox->chans + chan_num;

> +       if (priv->info->mb_channels_are_unidir) {
> +               is_only_rx = chan_flags & MFIS_CHANNEL_RX;
> +               chan = mbox->chans + 2 * chan_num + is_only_rx;

...and:

    chan += chan_num + is_only_rx;

> +       } else {
> +               is_only_rx = false;
> +               chan = mbox->chans + chan_num;

... and drop this line?
With a proper preinitalization of is_only_rx, the whole "else" branch
can be dropped.

> +       }
> +
> +       if (priv->info->mb_reg_comes_from_dt) {
> +               tx_uses_eicr = chan_flags & MFIS_CHANNEL_EICR;
> +               if (tx_uses_eicr)
> +                       chan += mbox->num_chans / 2;
> +       } else {
> +               tx_uses_eicr = priv->info->mb_tx_uses_eicr;
> +       }

"chan - mbox->chans" is the logical channel number, and should be
validated against mbox_num_chans, to avoid out-of-bound accesses.

> +
> +       per_chan_priv = chan->con_priv;
> +       per_chan_priv->reg = chan_num * 0x1000 + (tx_uses_eicr ^ is_only_rx) * 4;

I think it would be good to document this register is either an IICR
or EICR register offset, through:
  1. A comment, or
  2. Definitions and code, e.g

         #define MFISIICR 0x00
         #define MFISEICR 0x04

         if (tx_uses_eicr ^ is_only_rx)
             per_chan_priv->reg = chan_num * 0x1000 + MFISEICR;
         else
             per_chan_priv->reg = chan_num * 0x1000 + MFISIICR;

     Or:

         #define MFISIICR(i) ((i) * 0x1000 + 0x00)
         #define MFISEICR(i) ((i) * 0x1000 + 0x04)

         per_chan_priv->reg = (tx_uses_eicr ^ is_only_rx) ? MFISEICR(chan_num)
                                                          : MFISIICR(chan_num);

> +
> +       if (!priv->info->mb_channels_are_unidir || is_only_rx) {
> +               char irqname[8];
> +               char suffix = tx_uses_eicr ? 'i' : 'e';
> +
> +               /* "ch0i" or "ch0e" */
> +               scnprintf(irqname, sizeof(irqname), "ch%d%c", chan_num, suffix);

"%u" for unsigned chan_num.

> +
> +               per_chan_priv->irq = of_irq_get_byname(mbox->dev->of_node, irqname);
> +               if (per_chan_priv->irq < 0)
> +                       return ERR_PTR(per_chan_priv->irq);
> +               else if (per_chan_priv->irq == 0)

No need for "else" after "return".

> +                       return ERR_PTR(-ENOENT);
> +       }
> +
> +       return chan;
> +}
> +
> +static int mfis_mb_probe(struct mfis_priv *priv)
> +{
> +       struct device *dev = priv->dev;
> +       struct mbox_chan *chan;
> +       struct mbox_controller *mbox;
> +       unsigned int i, num_chan = priv->info->mb_num_channels;

"i" is only used in the for-loop below, so you could declare it in the
for-statement. As a bonus, that would avoid mixing the declaration of
uninitialized and initialized variables.

> +
> +       if (priv->info->mb_channels_are_unidir)
> +               /* Channel layout: Ch0-TX, Ch0-RX, Ch1-TX... */
> +               num_chan *= 2;
> +
> +       if (priv->info->mb_reg_comes_from_dt)
> +               /* Channel layout: <n> IICR channels, <n> EICR channels */
> +               num_chan *= 2;

Curly braces around multi-line if-branches (even if they are comments)?

> +
> +       chan  = devm_kcalloc(dev, num_chan, sizeof(*chan), GFP_KERNEL);
> +       if (!chan)
> +               return -ENOMEM;
> +
> +       priv->per_chan_privs = devm_kcalloc(dev, num_chan, sizeof(*priv->per_chan_privs),
> +                                           GFP_KERNEL);
> +       if (!priv->per_chan_privs)
> +               return -ENOMEM;
> +
> +       mbox = &priv->mbox;
> +
> +       for (i = 0; i < num_chan; i++)
> +               chan[i].con_priv = &priv->per_chan_privs[i];
> +
> +       mbox->chans = chan;
> +       mbox->num_chans = num_chan;
> +       mbox->txdone_poll = true;
> +       mbox->ops = &mfis_iicr_ops;
> +       mbox->dev = dev;
> +       mbox->of_xlate = mfis_mb_of_xlate;
> +
> +       return mbox_controller_register(mbox);
> +}
> +
> +/****************************************************
> + *             Common

Missing closing asterisk.

> + ****************************************************/
>
> +static int mfis_reg_probe(struct platform_device *pdev, struct mfis_priv *priv,
> +                         struct mfis_reg *mreg, const char *name, bool required)
> +{
> +       struct resource *res;
> +       void __iomem *base;
> +
> +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> +
> +       /* If there is no mailbox resource, registers are in the common space */
> +       if (!res && !required) {

Given you only test for the negated "!required", perhaps invert the
logic, and replace "required" by "optional"?

> +               priv->mbox_reg = priv->common_reg;

This left me looking for an assignment to priv->mbox_reg in the "else"
branch ;-) Then I realized "&priv->mbox_reg" is passed as the "mreg"
parameter.  Hence perhaps:

    *mreg = priv->common_reg;

?

> +       } else {
> +               base = devm_ioremap_resource(&pdev->dev, res);
> +               if (IS_ERR(base))
> +                       return PTR_ERR(base);
> +
> +               mreg->base = base;
> +               mreg->start = res->start;
> +               mreg->priv = priv;
> +       }
> +
> +       return 0;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Sashiko review feedback (was Re: [PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver)

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 5267 bytes --]

Hi all,

I am usually not into AI but I can definitely see value in its review
assistance. So, I checked what Sashiko thinks of my series and found the
comments to be largely reasonable. I'll copy and answer them here, a)
for a more complete review process and b) to give feedback to the
Sashiko-devs (CCed). Thank you for the service and efforts, I hope we
can justify the additional energy consumption here. Please let me know
if such replies are actually helpful for you.

> > +static void mfis_write(struct mfis_reg *mreg, u32 reg, u32 val)
> > +{
> > +	struct mfis_priv *priv = mreg->priv;
> > +	u32 unprotect_mask = priv->info->unprotect_mask;
> > +	u32 unprotect_code;
> > +
> > +	/*
> > +	 * [Gen4] key: 0xACCE0000, mask: 0x0000FFFF
> > +	 * [Gen5] key: 0xACC00000, mask: 0x000FFFFF
> > +	 */
> > +	unprotect_code = (MFIS_UNPROTECT_KEY & ~unprotect_mask) |
> > +			 ((mreg->start | reg) & unprotect_mask);
> 
> The mreg->start and reg variables are combined with a bitwise OR. If
> mreg->start has overlapping bits with the reg offset, could this yield an
> incorrect physical address for the unprotect code? Should this use
> addition like mreg->start + reg instead?

No, if a device tree is so wrongly setup that the bits overlap, then '+'
instead of '|' will only lead to a different wrong address.

> > +
> > +	iowrite32(unprotect_code, priv->common_reg.base + MFISWACNTR);
> > +	iowrite32(val, mreg->base + reg);
> > +}

> Can mfis_write() be called concurrently by different processors or
> interrupted by an interrupt handler like mfis_mb_iicr_interrupt()?

Yes, it can. It even needs a hwspinlock instead of a regular spinlock
because it needs protection against cores which may not run Linux.
hwspinlock support will only be added later because of dependency
issues. My plan was to add the protection once we got hwspinlocks. But
maybe I should add just spinlocks now and convert them to hwspinlocks
once their support is in.

> > +static void mfis_mb_shutdown(struct mbox_chan *chan)
> > +{
> > +	struct mfis_per_chan_priv *per_chan_priv = chan->con_priv;
> > +
> > +	free_irq(per_chan_priv->irq, chan);
> > +}

> Since mfis_mb_of_xlate() skips interrupt lookup for transmit-only channels,
> leaving per_chan_priv->irq as 0, and mfis_mb_startup() guards the request
> with if (per_chan_priv->irq), will this unconditional free_irq() trigger a
> kernel warning when shutting down a transmit-only channel?

Yes, I seemed to remember that free_irq() can handle this, but it will
in-deed trigger a warning. Also, the reasons Geert mentioned in his
review apply.

> [ ... ]
> > +static struct mbox_chan *mfis_mb_of_xlate(struct mbox_controller *mbox,
> > +					  const struct of_phandle_args *sp)
> > +{
> > +	struct mfis_priv *priv = mfis_mb_mbox_to_priv(mbox);
> > +	struct mfis_per_chan_priv *per_chan_priv;
> > +	struct mbox_chan *chan;
> > +	u32 chan_num, chan_flags;
> > +	bool tx_uses_eicr, is_only_rx;
> > +
> > +	if (sp->args_count != 2)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	chan_num = sp->args[0];
> > +	chan_flags = sp->args[1];
> > +
> > +	/* Channel layout is described in mfis_mb_probe() */
> > +	if (priv->info->mb_channels_are_unidir) {
> > +		is_only_rx = chan_flags & MFIS_CHANNEL_RX;
> > +		chan = mbox->chans + 2 * chan_num + is_only_rx;

> Does this code allow out-of-bounds access if a malformed device tree
> provides an arbitrarily large chan_num? Should chan_num be checked against
> priv->info->mb_num_channels before computing the chan pointer?

Yes, Geert pointed this out as well.

> [ ... ]
> > +static int mfis_mb_probe(struct mfis_priv *priv)
> > +{
> [ ... ]
> > +	mbox->chans = chan;
> > +	mbox->num_chans = num_chan;
> > +	mbox->txdone_poll = true;
> > +	mbox->ops = &mfis_iicr_ops;
> > +	mbox->dev = dev;
> > +	mbox->of_xlate = mfis_mb_of_xlate;
> > +
> > +	return mbox_controller_register(mbox);
> > +}
> The mailbox controller is registered using mbox_controller_register(), but
> the driver lacks a remove callback to unregister it.
> If the driver is unbound, devres will free the devm_kzalloc structures
> backing the mailbox. Will this result in a use-after-free regression since
> the mailbox subsystem still retains the controller in its global lists?
> Should devm_mbox_controller_register() be used instead?
> [ ... ]

Yes, this was overlooked so far.

> > +static int mfis_probe(struct platform_device *pdev)
> > +{
> > +	struct mfis_priv *priv;
> > +	struct device *dev = &pdev->dev;
> > +	int ret;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->dev = dev;
> > +	priv->info = of_device_get_match_data(dev);
> Can of_device_get_match_data() return a null pointer here?
> If the driver is bound manually via sysfs or instantiated in an environment
> lacking device tree match data, would this lead to a null pointer
> dereference regression later when accessing priv->info->mb_num_channels in
> mfis_mb_probe()?

The latter case would be clearly a driver bug. In addition with the
former case, it probably makes sense to handle this more gracefully,
though.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 8120 bytes --]

Hi Geert,

thank you for the review!

> > +
> > +struct mfis_per_chan_priv {
> 
> I would drop the "per_".

Probably OK, will think about it.

> 
> > +       u32 reg;
> > +       int irq;
> > +};
> > +
> > +struct mfis_priv {
> > +       struct device *dev;
> > +       struct mfis_reg common_reg;
> > +       struct mfis_reg mbox_reg;
> > +       const struct mfis_info *info;
> > +
> > +       /* mailbox private data */
> > +       struct mbox_controller mbox;
> > +       struct mfis_per_chan_priv *per_chan_privs;
> 
> Likewise.
> This could be a flexible array, if it weren't for the hwspinlock array
> you will have to add later, right?

No, hwspinlock doesn't need any private data here. But something else
could come in the future maybe? I also don't see a big advantage of the
flexible array, too. Maybe it's too late in the evening...

> > +static void mfis_write(struct mfis_reg *mreg, u32 reg, u32 val)
> 
> Both writel() and iowrite32() use the inverse order of "reg" and "val".
> But I can understand you want to keep "mreg" and "reg" together.

Yes, please!

> > +/****************************************************
> > + *                     Mailbox
> 
> Missing closing asterisk ;-)

OK.

> 
> > + ****************************************************/
> > +
> > +#define mfis_mb_mbox_to_priv(_m) container_of((_m), struct mfis_priv, mbox)
> 
> Both "mb" and "mbox", so perhaps mfis_mbox_to_priv()?
> And perhaps s/mb_/mbox_/ everywhere?

Well, not sure here. 'mb' marks the mailbox part of the driver. 'mbox'
is the variable we work on. So, it has a pattern.

> > +       struct mfis_per_chan_priv *per_chan_priv = chan->con_priv;
> > +       int ret = 0;
> > +
> > +       if (per_chan_priv->irq)
> > +               ret = request_irq(per_chan_priv->irq, mfis_mb_iicr_interrupt, 0,
> > +                                 dev_name(chan->mbox->dev), chan);
> > +
> > +       return ret;
> 
> You can reduce indentation, and get rid of ret, by inverting the
> conditional.

I like this.

> > +static void mfis_mb_shutdown(struct mbox_chan *chan)
> > +{
> > +       struct mfis_per_chan_priv *per_chan_priv = chan->con_priv;
> > +
> > +       free_irq(per_chan_priv->irq, chan);
> 
> if (per_chan_priv->irq) ...
> 
> free_irq() seems to support zero, but irq_to_desc() still has to
> traverse the mtree.

I checked it and it prints a warning, so 'if' is good.

> > +       chan_num = sp->args[0];
> 
> "chan_num" is the hardware channel number, and should be validated
> against mpriv->info->mb_num_channels.

Ack!

> 
> > +       chan_flags = sp->args[1];
> > +
> > +       /* Channel layout is described in mfis_mb_probe() */
> 
> Given you already use "chan += ..." below, perhaps:
> 
>     chan = mbox->chans + chan_num;
> 
> > +       if (priv->info->mb_channels_are_unidir) {
> > +               is_only_rx = chan_flags & MFIS_CHANNEL_RX;
> > +               chan = mbox->chans + 2 * chan_num + is_only_rx;
> 
> ...and:
> 
>     chan += chan_num + is_only_rx;
> 
> > +       } else {
> > +               is_only_rx = false;
> > +               chan = mbox->chans + chan_num;
> 
> ... and drop this line?
> With a proper preinitalization of is_only_rx, the whole "else" branch
> can be dropped.

I agree it saves a few lines, but I really think the original code is
easier to understand. Channel layout is already wickes and doing 'channel
+=' twice is harder to understand than a simple assignment IMO.

> 
> > +       }
> > +
> > +       if (priv->info->mb_reg_comes_from_dt) {
> > +               tx_uses_eicr = chan_flags & MFIS_CHANNEL_EICR;
> > +               if (tx_uses_eicr)
> > +                       chan += mbox->num_chans / 2;
> > +       } else {
> > +               tx_uses_eicr = priv->info->mb_tx_uses_eicr;
> > +       }
> 
> "chan - mbox->chans" is the logical channel number, and should be
> validated against mbox_num_chans, to avoid out-of-bound accesses.

"chan - ..."? You mean "chan + ..."?

> 
> > +
> > +       per_chan_priv = chan->con_priv;
> > +       per_chan_priv->reg = chan_num * 0x1000 + (tx_uses_eicr ^ is_only_rx) * 4;
> 
> I think it would be good to document this register is either an IICR
> or EICR register offset, through:
>   1. A comment, or
>   2. Definitions and code, e.g
> 
>          #define MFISIICR 0x00
>          #define MFISEICR 0x04
> 
>          if (tx_uses_eicr ^ is_only_rx)
>              per_chan_priv->reg = chan_num * 0x1000 + MFISEICR;
>          else
>              per_chan_priv->reg = chan_num * 0x1000 + MFISIICR;
> 
>      Or:
> 
>          #define MFISIICR(i) ((i) * 0x1000 + 0x00)
>          #define MFISEICR(i) ((i) * 0x1000 + 0x04)
> 
>          per_chan_priv->reg = (tx_uses_eicr ^ is_only_rx) ? MFISEICR(chan_num)
>                                                           : MFISIICR(chan_num);
> 

I like the latter one. Let my try this. But maybe it will be just a
comment in the end ;)

> > +
> > +       if (!priv->info->mb_channels_are_unidir || is_only_rx) {
> > +               char irqname[8];
> > +               char suffix = tx_uses_eicr ? 'i' : 'e';
> > +
> > +               /* "ch0i" or "ch0e" */
> > +               scnprintf(irqname, sizeof(irqname), "ch%d%c", chan_num, suffix);
> 
> "%u" for unsigned chan_num.

OK:

> 
> > +
> > +               per_chan_priv->irq = of_irq_get_byname(mbox->dev->of_node, irqname);
> > +               if (per_chan_priv->irq < 0)
> > +                       return ERR_PTR(per_chan_priv->irq);
> > +               else if (per_chan_priv->irq == 0)
> 
> No need for "else" after "return".

OK.

> 
> > +                       return ERR_PTR(-ENOENT);
> > +       }
> > +
> > +       return chan;
> > +}
> > +
> > +static int mfis_mb_probe(struct mfis_priv *priv)
> > +{
> > +       struct device *dev = priv->dev;
> > +       struct mbox_chan *chan;
> > +       struct mbox_controller *mbox;
> > +       unsigned int i, num_chan = priv->info->mb_num_channels;
> 
> "i" is only used in the for-loop below, so you could declare it in the
> for-statement. As a bonus, that would avoid mixing the declaration of
> uninitialized and initialized variables.

Yes!

> 
> > +
> > +       if (priv->info->mb_channels_are_unidir)
> > +               /* Channel layout: Ch0-TX, Ch0-RX, Ch1-TX... */
> > +               num_chan *= 2;
> > +
> > +       if (priv->info->mb_reg_comes_from_dt)
> > +               /* Channel layout: <n> IICR channels, <n> EICR channels */
> > +               num_chan *= 2;
> 
> Curly braces around multi-line if-branches (even if they are comments)?

OK? I don't mind.

> > +/****************************************************
> > + *             Common
> 
> Missing closing asterisk.

OK.

> 
> > + ****************************************************/
> >
> > +static int mfis_reg_probe(struct platform_device *pdev, struct mfis_priv *priv,
> > +                         struct mfis_reg *mreg, const char *name, bool required)
> > +{
> > +       struct resource *res;
> > +       void __iomem *base;
> > +
> > +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> > +
> > +       /* If there is no mailbox resource, registers are in the common space */
> > +       if (!res && !required) {
> 
> Given you only test for the negated "!required", perhaps invert the
> logic, and replace "required" by "optional"?

I had this first and it looked weird that the first call had "false" and
the second one "true". I liked it better this way but don't really mind.

> 
> > +               priv->mbox_reg = priv->common_reg;
> 
> This left me looking for an assignment to priv->mbox_reg in the "else"
> branch ;-) Then I realized "&priv->mbox_reg" is passed as the "mreg"
> parameter.  Hence perhaps:
> 
>     *mreg = priv->common_reg;
> 
> ?

Yup, that should work.

I will work on the next version tomorrow. Thank you for your input!

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver

[Geert Uytterhoeven]

Hi Wolfram,

On Sun, 22 Mar 2026 at 20:58, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > +struct mfis_priv {
> > > +       struct device *dev;
> > > +       struct mfis_reg common_reg;
> > > +       struct mfis_reg mbox_reg;
> > > +       const struct mfis_info *info;
> > > +
> > > +       /* mailbox private data */
> > > +       struct mbox_controller mbox;
> > > +       struct mfis_per_chan_priv *per_chan_privs;
> >
> > This could be a flexible array, if it weren't for the hwspinlock array
> > you will have to add later, right?
>
> No, hwspinlock doesn't need any private data here. But something else
> could come in the future maybe? I also don't see a big advantage of the
> flexible array, too. Maybe it's too late in the evening...

You're right. Somehow I thought you were allocating priv and
per_chan_privs next to each other.

> > > +       chan_num = sp->args[0];
> >
> > "chan_num" is the hardware channel number, and should be validated
> > against mpriv->info->mb_num_channels.
>
> Ack!
>
> > > +       chan_flags = sp->args[1];
> > > +
> > > +       /* Channel layout is described in mfis_mb_probe() */
> >
> > Given you already use "chan += ..." below, perhaps:
> >
> >     chan = mbox->chans + chan_num;
> >
> > > +       if (priv->info->mb_channels_are_unidir) {
> > > +               is_only_rx = chan_flags & MFIS_CHANNEL_RX;
> > > +               chan = mbox->chans + 2 * chan_num + is_only_rx;
> >
> > ...and:
> >
> >     chan += chan_num + is_only_rx;
> >
> > > +       } else {
> > > +               is_only_rx = false;
> > > +               chan = mbox->chans + chan_num;
> >
> > ... and drop this line?
> > With a proper preinitalization of is_only_rx, the whole "else" branch
> > can be dropped.
>
> I agree it saves a few lines, but I really think the original code is
> easier to understand. Channel layout is already wickes and doing 'channel
> +=' twice is harder to understand than a simple assignment IMO.
>
> >
> > > +       }
> > > +
> > > +       if (priv->info->mb_reg_comes_from_dt) {
> > > +               tx_uses_eicr = chan_flags & MFIS_CHANNEL_EICR;
> > > +               if (tx_uses_eicr)
> > > +                       chan += mbox->num_chans / 2;
> > > +       } else {
> > > +               tx_uses_eicr = priv->info->mb_tx_uses_eicr;
> > > +       }
> >
> > "chan - mbox->chans" is the logical channel number, and should be
> > validated against mbox_num_chans, to avoid out-of-bound accesses.
>
> "chan - ..."? You mean "chan + ..."?

No, I did mean "-": you do have a pointer "chan" to the channel,
instead of an index into the mbox->chans[] array.
Using a  index would  make validation easier to read, though.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]


> > > > +       if (priv->info->mb_reg_comes_from_dt) {
> > > > +               tx_uses_eicr = chan_flags & MFIS_CHANNEL_EICR;
> > > > +               if (tx_uses_eicr)
> > > > +                       chan += mbox->num_chans / 2;
> > > > +       } else {
> > > > +               tx_uses_eicr = priv->info->mb_tx_uses_eicr;
> > > > +       }
> > >
> > > "chan - mbox->chans" is the logical channel number, and should be
> > > validated against mbox_num_chans, to avoid out-of-bound accesses.
> >
> > "chan - ..."? You mean "chan + ..."?
> 
> No, I did mean "-": you do have a pointer "chan" to the channel,
> instead of an index into the mbox->chans[] array.
> Using a  index would  make validation easier to read, though.

Sorry, I still don't get it: If 'chan_num' has been sanitized above to
be in the range of 0..priv->info->mb_num_channels - 1, then how can a
OOB happen here? "chan += mbox->num_chans / 2" only happens when
'mb_reg_comes_from_dt' is set. But when it is set, the array of chans is
also always doubled to have the "<n> IICR, then <n> EICR" layout.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Sashiko review feedback (was Re: [PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver)

[Geert Uytterhoeven]

Hi Wolfram,

On Sun, 22 Mar 2026 at 09:59, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> I am usually not into AI but I can definitely see value in its review
> assistance. So, I checked what Sashiko thinks of my series and found the
> comments to be largely reasonable. I'll copy and answer them here, a)
> for a more complete review process and b) to give feedback to the
> Sashiko-devs (CCed). Thank you for the service and efforts, I hope we
> can justify the additional energy consumption here. Please let me know
> if such replies are actually helpful for you.
>
> > > +static void mfis_write(struct mfis_reg *mreg, u32 reg, u32 val)
> > > +{
> > > +   struct mfis_priv *priv = mreg->priv;
> > > +   u32 unprotect_mask = priv->info->unprotect_mask;
> > > +   u32 unprotect_code;
> > > +
> > > +   /*
> > > +    * [Gen4] key: 0xACCE0000, mask: 0x0000FFFF
> > > +    * [Gen5] key: 0xACC00000, mask: 0x000FFFFF
> > > +    */
> > > +   unprotect_code = (MFIS_UNPROTECT_KEY & ~unprotect_mask) |
> > > +                    ((mreg->start | reg) & unprotect_mask);
> >
> > The mreg->start and reg variables are combined with a bitwise OR. If
> > mreg->start has overlapping bits with the reg offset, could this yield an
> > incorrect physical address for the unprotect code? Should this use
> > addition like mreg->start + reg instead?
>
> No, if a device tree is so wrongly setup that the bits overlap, then '+'
> instead of '|' will only lead to a different wrong address.

While there are no overlapping bits, and thus both OR and PLUS have
the same effect, I agree PLUS is More Correct(TM), as that matches
what you are using in the second iowrite32() below.

> > > +
> > > +   iowrite32(unprotect_code, priv->common_reg.base + MFISWACNTR);
> > > +   iowrite32(val, mreg->base + reg);
> > > +}
>
> > Can mfis_write() be called concurrently by different processors or
> > interrupted by an interrupt handler like mfis_mb_iicr_interrupt()?
>
> Yes, it can. It even needs a hwspinlock instead of a regular spinlock
> because it needs protection against cores which may not run Linux.
> hwspinlock support will only be added later because of dependency
> issues. My plan was to add the protection once we got hwspinlocks. But
> maybe I should add just spinlocks now and convert them to hwspinlocks
> once their support is in.

Have you ever triggered this race condition when running the mbox
test? Or is that impossible due to locking elsewhere?

> > > +static struct mbox_chan *mfis_mb_of_xlate(struct mbox_controller *mbox,
> > > +                                     const struct of_phandle_args *sp)
> > > +{
> > > +   struct mfis_priv *priv = mfis_mb_mbox_to_priv(mbox);
> > > +   struct mfis_per_chan_priv *per_chan_priv;
> > > +   struct mbox_chan *chan;
> > > +   u32 chan_num, chan_flags;
> > > +   bool tx_uses_eicr, is_only_rx;
> > > +
> > > +   if (sp->args_count != 2)
> > > +           return ERR_PTR(-EINVAL);
> > > +
> > > +   chan_num = sp->args[0];
> > > +   chan_flags = sp->args[1];
> > > +
> > > +   /* Channel layout is described in mfis_mb_probe() */
> > > +   if (priv->info->mb_channels_are_unidir) {
> > > +           is_only_rx = chan_flags & MFIS_CHANNEL_RX;
> > > +           chan = mbox->chans + 2 * chan_num + is_only_rx;
>
> > Does this code allow out-of-bounds access if a malformed device tree
> > provides an arbitrarily large chan_num? Should chan_num be checked against
> > priv->info->mb_num_channels before computing the chan pointer?
>
> Yes, Geert pointed this out as well.
>
> > [ ... ]
> > > +static int mfis_mb_probe(struct mfis_priv *priv)
> > > +{
> > [ ... ]
> > > +   mbox->chans = chan;
> > > +   mbox->num_chans = num_chan;
> > > +   mbox->txdone_poll = true;
> > > +   mbox->ops = &mfis_iicr_ops;
> > > +   mbox->dev = dev;
> > > +   mbox->of_xlate = mfis_mb_of_xlate;
> > > +
> > > +   return mbox_controller_register(mbox);
> > > +}
> > The mailbox controller is registered using mbox_controller_register(), but
> > the driver lacks a remove callback to unregister it.
> > If the driver is unbound, devres will free the devm_kzalloc structures
> > backing the mailbox. Will this result in a use-after-free regression since
> > the mailbox subsystem still retains the controller in its global lists?
> > Should devm_mbox_controller_register() be used instead?
> > [ ... ]
>
> Yes, this was overlooked so far.

.suppress_bind_attrs to the rescue? ;-)

> > > +static int mfis_probe(struct platform_device *pdev)
> > > +{
> > > +   struct mfis_priv *priv;
> > > +   struct device *dev = &pdev->dev;
> > > +   int ret;
> > > +
> > > +   priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +   if (!priv)
> > > +           return -ENOMEM;
> > > +
> > > +   priv->dev = dev;
> > > +   priv->info = of_device_get_match_data(dev);
> > Can of_device_get_match_data() return a null pointer here?
> > If the driver is bound manually via sysfs or instantiated in an environment
> > lacking device tree match data, would this lead to a null pointer
> > dereference regression later when accessing priv->info->mb_num_channels in
> > mfis_mb_probe()?
>
> The latter case would be clearly a driver bug. In addition with the
> former case, it probably makes sense to handle this more gracefully,
> though.

That is an interesting one I never considered before!
So far we always assumed of_device_get_match_data() cannot return
NULL if all match table entries have their .data members filled in.
However, you can indeed still trigger this by using driver_override
to bind to the wrong device, which obviously has no match entry at all:

On Salvator-XS:

# echo fe960000.vsp > /sys/bus/platform/drivers/vsp1/unbind
# echo renesas_spi > /sys/bus/platform/devices/fe960000.vsp/driver_override
# echo fe960000.vsp > /sys/bus/platform/drivers/renesas_spi/bind

BOOM!

Do we care about that?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, 23 Mar 2026 at 10:29, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > > > +       if (priv->info->mb_reg_comes_from_dt) {
> > > > > +               tx_uses_eicr = chan_flags & MFIS_CHANNEL_EICR;
> > > > > +               if (tx_uses_eicr)
> > > > > +                       chan += mbox->num_chans / 2;
> > > > > +       } else {
> > > > > +               tx_uses_eicr = priv->info->mb_tx_uses_eicr;
> > > > > +       }
> > > >
> > > > "chan - mbox->chans" is the logical channel number, and should be
> > > > validated against mbox_num_chans, to avoid out-of-bound accesses.
> > >
> > > "chan - ..."? You mean "chan + ..."?
> >
> > No, I did mean "-": you do have a pointer "chan" to the channel,
> > instead of an index into the mbox->chans[] array.
> > Using a  index would  make validation easier to read, though.
>
> Sorry, I still don't get it: If 'chan_num' has been sanitized above to
> be in the range of 0..priv->info->mb_num_channels - 1, then how can a
> OOB happen here? "chan += mbox->num_chans / 2" only happens when
> 'mb_reg_comes_from_dt' is set. But when it is set, the array of chans is
> also always doubled to have the "<n> IICR, then <n> EICR" layout.

Sorry, you are right.
I guess I was thinking the flags specified in DT could cause an invalid
index doubling, but those can indeed not happen due to the
priv->info->mb_* flag checks.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Sashiko review feedback (was Re: [PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver)

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1846 bytes --]

Hi Geert,

> While there are no overlapping bits, and thus both OR and PLUS have
> the same effect, I agree PLUS is More Correct(TM), as that matches
> what you are using in the second iowrite32() below.

Yes, it is more correct, yes I will change it, yes I don't like
bike-shedding ;)

> > Yes, it can. It even needs a hwspinlock instead of a regular spinlock
> > because it needs protection against cores which may not run Linux.
> > hwspinlock support will only be added later because of dependency
> > issues. My plan was to add the protection once we got hwspinlocks. But
> > maybe I should add just spinlocks now and convert them to hwspinlocks
> > once their support is in.
> 
> Have you ever triggered this race condition when running the mbox
> test? Or is that impossible due to locking elsewhere?

Haven't encountered that yet. But since we are talking about generic
mfis_write() here which can be used in further ways in the future, we
should not rely on external serialization here IMO.

> > Yes, this was overlooked so far.
> 
> .suppress_bind_attrs to the rescue? ;-)

I was considering this for sure, on top of the suggested improvement. I
guess it can be argued for a mailbox and hwspinlock driver, or?

> That is an interesting one I never considered before!
> So far we always assumed of_device_get_match_data() cannot return
> NULL if all match table entries have their .data members filled in.
> However, you can indeed still trigger this by using driver_override
> to bind to the wrong device, which obviously has no match entry at all:

...

> Do we care about that?

I tend to say: yes. On the one hand, using "driver_override" is a
you-better-know-what-you-do command. On the other hand, failing
gracefully is not hard to do and not much code.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Sashiko review feedback (was Re: [PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver)

[Roman Gushchin]

Wolfram Sang <wsa+renesas@sang-engineering.com> writes:

> Hi all,
>
> I am usually not into AI but I can definitely see value in its review
> assistance. So, I checked what Sashiko thinks of my series and found the
> comments to be largely reasonable. I'll copy and answer them here, a)
> for a more complete review process and b) to give feedback to the
> Sashiko-devs (CCed). Thank you for the service and efforts, I hope we
> can justify the additional energy consumption here. Please let me know
> if such replies are actually helpful for you.

Hi Wolfram!

These replies are definitely helpful! I can't realistically look into
all cases across all subsystems (I simple have not enough expertise),
but I'll try to look into most cases and hope that other engineers will
help here.

In your case it looks like most findings were real?

Thank you!

>
>> > +static void mfis_write(struct mfis_reg *mreg, u32 reg, u32 val)
>> > +{
>> > +	struct mfis_priv *priv = mreg->priv;
>> > +	u32 unprotect_mask = priv->info->unprotect_mask;
>> > +	u32 unprotect_code;
>> > +
>> > +	/*
>> > +	 * [Gen4] key: 0xACCE0000, mask: 0x0000FFFF
>> > +	 * [Gen5] key: 0xACC00000, mask: 0x000FFFFF
>> > +	 */
>> > +	unprotect_code = (MFIS_UNPROTECT_KEY & ~unprotect_mask) |
>> > +			 ((mreg->start | reg) & unprotect_mask);
>> 
>> The mreg->start and reg variables are combined with a bitwise OR. If
>> mreg->start has overlapping bits with the reg offset, could this yield an
>> incorrect physical address for the unprotect code? Should this use
>> addition like mreg->start + reg instead?
>
> No, if a device tree is so wrongly setup that the bits overlap, then '+'
> instead of '|' will only lead to a different wrong address.
>
>> > +
>> > +	iowrite32(unprotect_code, priv->common_reg.base + MFISWACNTR);
>> > +	iowrite32(val, mreg->base + reg);
>> > +}
>
>> Can mfis_write() be called concurrently by different processors or
>> interrupted by an interrupt handler like mfis_mb_iicr_interrupt()?
>
> Yes, it can. It even needs a hwspinlock instead of a regular spinlock
> because it needs protection against cores which may not run Linux.
> hwspinlock support will only be added later because of dependency
> issues. My plan was to add the protection once we got hwspinlocks. But
> maybe I should add just spinlocks now and convert them to hwspinlocks
> once their support is in.
>
>> > +static void mfis_mb_shutdown(struct mbox_chan *chan)
>> > +{
>> > +	struct mfis_per_chan_priv *per_chan_priv = chan->con_priv;
>> > +
>> > +	free_irq(per_chan_priv->irq, chan);
>> > +}
>
>> Since mfis_mb_of_xlate() skips interrupt lookup for transmit-only channels,
>> leaving per_chan_priv->irq as 0, and mfis_mb_startup() guards the request
>> with if (per_chan_priv->irq), will this unconditional free_irq() trigger a
>> kernel warning when shutting down a transmit-only channel?
>
> Yes, I seemed to remember that free_irq() can handle this, but it will
> in-deed trigger a warning. Also, the reasons Geert mentioned in his
> review apply.
>
>> [ ... ]
>> > +static struct mbox_chan *mfis_mb_of_xlate(struct mbox_controller *mbox,
>> > +					  const struct of_phandle_args *sp)
>> > +{
>> > +	struct mfis_priv *priv = mfis_mb_mbox_to_priv(mbox);
>> > +	struct mfis_per_chan_priv *per_chan_priv;
>> > +	struct mbox_chan *chan;
>> > +	u32 chan_num, chan_flags;
>> > +	bool tx_uses_eicr, is_only_rx;
>> > +
>> > +	if (sp->args_count != 2)
>> > +		return ERR_PTR(-EINVAL);
>> > +
>> > +	chan_num = sp->args[0];
>> > +	chan_flags = sp->args[1];
>> > +
>> > +	/* Channel layout is described in mfis_mb_probe() */
>> > +	if (priv->info->mb_channels_are_unidir) {
>> > +		is_only_rx = chan_flags & MFIS_CHANNEL_RX;
>> > +		chan = mbox->chans + 2 * chan_num + is_only_rx;
>
>> Does this code allow out-of-bounds access if a malformed device tree
>> provides an arbitrarily large chan_num? Should chan_num be checked against
>> priv->info->mb_num_channels before computing the chan pointer?
>
> Yes, Geert pointed this out as well.
>
>> [ ... ]
>> > +static int mfis_mb_probe(struct mfis_priv *priv)
>> > +{
>> [ ... ]
>> > +	mbox->chans = chan;
>> > +	mbox->num_chans = num_chan;
>> > +	mbox->txdone_poll = true;
>> > +	mbox->ops = &mfis_iicr_ops;
>> > +	mbox->dev = dev;
>> > +	mbox->of_xlate = mfis_mb_of_xlate;
>> > +
>> > +	return mbox_controller_register(mbox);
>> > +}
>> The mailbox controller is registered using mbox_controller_register(), but
>> the driver lacks a remove callback to unregister it.
>> If the driver is unbound, devres will free the devm_kzalloc structures
>> backing the mailbox. Will this result in a use-after-free regression since
>> the mailbox subsystem still retains the controller in its global lists?
>> Should devm_mbox_controller_register() be used instead?
>> [ ... ]
>
> Yes, this was overlooked so far.
>
>> > +static int mfis_probe(struct platform_device *pdev)
>> > +{
>> > +	struct mfis_priv *priv;
>> > +	struct device *dev = &pdev->dev;
>> > +	int ret;
>> > +
>> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> > +	if (!priv)
>> > +		return -ENOMEM;
>> > +
>> > +	priv->dev = dev;
>> > +	priv->info = of_device_get_match_data(dev);
>> Can of_device_get_match_data() return a null pointer here?
>> If the driver is bound manually via sysfs or instantiated in an environment
>> lacking device tree match data, would this lead to a null pointer
>> dereference regression later when accessing priv->info->mb_num_channels in
>> mfis_mb_probe()?
>
> The latter case would be clearly a driver bug. In addition with the
> former case, it probably makes sense to handle this more gracefully,
> though.
>
> Happy hacking,
>
>    Wolfram

Re: Sashiko review feedback (was Re: [PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver)

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

Hi Roman,

> These replies are definitely helpful! I can't realistically look into
> all cases across all subsystems (I simple have not enough expertise),
> but I'll try to look into most cases and hope that other engineers will
> help here.

Sure thing. Is there a dedicated mailing-list or better email address I
can add?

> In your case it looks like most findings were real?

To my surprise, yes :)

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Sashiko review feedback (was Re: [PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver)

[Roman Gushchin]

Wolfram Sang <wsa+renesas@sang-engineering.com> writes:

> Hi Roman,
>
>> These replies are definitely helpful! I can't realistically look into
>> all cases across all subsystems (I simple have not enough expertise),
>> but I'll try to look into most cases and hope that other engineers will
>> help here.
>
> Sure thing. Is there a dedicated mailing-list or better email address I
> can add?

Not yet, but I think of creating one.

Thanks!