Devicetree
 help / color / mirror / Atom feed
* [PATCH v2 0/6] Add AMD PL Mailbox and RPMsg support for the AMD BRAM remoteproc
@ 2026-11-05  5:18 Ben Levinsky
  2026-11-05  5:18 ` [PATCH v2 1/6] remoteproc: amd_mbv_bram: Bound device name formatting Ben Levinsky
                   ` (5 more replies)
  0 siblings, 6 replies; 10+ messages in thread
From: Ben Levinsky @ 2026-11-05  5:18 UTC (permalink / raw)
  To: jassisinghbrar, andersson, mathieu.poirier, robh, krzk+dt,
	conor+dt
  Cc: michal.simek, tanmay.shah, ben.levinsky, linux-remoteproc,
	devicetree, linux-arm-kernel, linux-kernel

This series adds a mailbox controller for the AMD programmable-logic (PL)
Mailbox soft IP and uses it to add RPMsg support to the AMD MicroBlaze/V
BRAM remoteproc driver, so that a soft processor in the PL can communicate
with Linux over virtio-rpmsg.

Patches 1-2 fix the existing BRAM remoteproc driver:
- Pass the device name to rproc_mem_entry_init() through a constant format
  string instead of using it as the format.
- Drive the reset GPIO with gpiod_direction_output() so a start still
  produces a reset pulse after the PL has been reconfigured, which restores
  the AXI GPIO to its default direction.

Patches 3-4 add the binding and the mailbox-framework controller for the
PL Mailbox IP. The controller exposes independent TX and RX channels. Its
interface clock is enabled only while a channel is started. RX uses a
threaded receive-threshold interrupt that drains the FIFO, with the FIFO
depth taken from the device tree. TX completion is reported by
mailbox-core polling of the send-threshold status bit.

Patches 5-6 extend the BRAM remoteproc binding and driver with optional
mailbox channels and named reserved-memory regions for the vrings and the
shared RPMsg buffer pool. The driver registers the carveouts, exposes the
loaded resource table, requests the mailbox channels at probe time, sends
virtqueue notification IDs through the mailbox framework, and defers
received notifications to process context. Without mailboxes the driver
keeps supporting load, start and stop only.

The series is based on mainline during the v7.4 merge window and uses the
remoteproc wc-ioremap and optional resource-table helpers merged for
v7.3-rc1.

Changes in v2:
- dt-bindings: mailbox:
  - Use the single compatible "amd,pl-mailbox-2.1". The Mailbox is a soft
    IP core in programmable logic, not a SoC block, so like other PL soft
    IP bindings the compatible names the IP core and its version instead
    of a SoC. "pl" distinguishes it from the IPI mailbox hardware.
  - Rename amd,mbox-pl-fifo-len to amd,pl-mailbox-depth, and explain that
    the depth is chosen per IP instance at synthesis time and cannot be
    read from the registers.
  - Add the MAINTAINERS entry together with the binding.
- mailbox:
  - Match only amd,pl-mailbox-2.1.
  - Depend on ARCH_ZYNQMP || COMPILE_TEST.
  - Do not access the FIFO when the mailbox core submits a queued message
    after the transmit channel has started to shut down.
  - Handle the receive interrupt in a threaded handler that acknowledges
    the event first and then drains the FIFO until it is empty, so a full
    FIFO can no longer stall reception.
  - Add a file header with the driver description and copyright.
- dt-bindings: remoteproc: drop the redundant dependentRequired block.
- remoteproc:
  - Keep the existing dependencies and add "depends on MAILBOX" instead
    of selecting user-visible symbols. Name the PL Mailbox controller and
    the virtio RPMsg bus in the help text.
  - Request the mailbox channels at probe time, so a missing controller
    defers the probe, and release them on unbind.
  - Free the mailbox channels before cancelling the receive work, so a
    late notification cannot queue it again.

v1: https://lore.kernel.org/r/20261014213426.2957480-1-ben.levinsky@amd.com

Ben Levinsky (6):
  remoteproc: amd_mbv_bram: Bound device name formatting
  remoteproc: amd_mbv_bram: Restore reset GPIO direction after PL
    reconfig
  dt-bindings: mailbox: Add AMD PL Mailbox IP
  mailbox: Add AMD PL Mailbox soft IP controller
  dt-bindings: remoteproc: Add RPMsg resources to AMD BRAM rproc
  remoteproc: amd_mbv_bram: Add RPMsg support

 .../bindings/mailbox/amd,pl-mailbox-2.1.yaml  |  73 ++++
 .../bindings/remoteproc/amd,bram-rproc.yaml   |  21 +
 MAINTAINERS                                   |   7 +
 drivers/mailbox/Kconfig                       |  11 +
 drivers/mailbox/Makefile                      |   1 +
 drivers/mailbox/amd-pl-mailbox.c              | 378 ++++++++++++++++++
 drivers/remoteproc/Kconfig                    |   5 +
 drivers/remoteproc/amd_mbv_bram_rproc.c       | 270 ++++++++++++-
 8 files changed, 761 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/amd,pl-mailbox-2.1.yaml
 create mode 100644 drivers/mailbox/amd-pl-mailbox.c


base-commit: 6b4b543fa18757297ac756a6fe6e64f506b7844a
-- 
2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/6] remoteproc: amd_mbv_bram: Bound device name formatting
  2026-11-05  5:18 [PATCH v2 0/6] Add AMD PL Mailbox and RPMsg support for the AMD BRAM remoteproc Ben Levinsky
@ 2026-11-05  5:18 ` Ben Levinsky
  2026-11-05  5:18 ` [PATCH v2 2/6] remoteproc: amd_mbv_bram: Restore reset GPIO direction after PL reconfig Ben Levinsky
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Ben Levinsky @ 2026-11-05  5:18 UTC (permalink / raw)
  To: jassisinghbrar, andersson, mathieu.poirier, robh, krzk+dt,
	conor+dt
  Cc: michal.simek, tanmay.shah, ben.levinsky, linux-remoteproc,
	devicetree, linux-arm-kernel, linux-kernel

rproc_mem_entry_init() interprets its name argument as a printf format
string. Passing the device name directly allows percent conversions in a
DT-derived platform device name to consume nonexistent variadic arguments.

Pass the device name through a constant format so it is always handled as
data. Limit the formatted name to 31 characters to match the space
available in rproc_mem_entry::name while leaving room for its terminating
null byte.

Fixes: f87b4998994e ("remoteproc: Add AMD MicroBlaze/V BRAM-based remote processor driver")
Reviewed-by: Tanmay Shah <tanmay.shah@amd.com>
Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
---
 drivers/remoteproc/amd_mbv_bram_rproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/remoteproc/amd_mbv_bram_rproc.c b/drivers/remoteproc/amd_mbv_bram_rproc.c
index e4a103cf8455..6cf33c7b7e64 100644
--- a/drivers/remoteproc/amd_mbv_bram_rproc.c
+++ b/drivers/remoteproc/amd_mbv_bram_rproc.c
@@ -72,7 +72,7 @@ static int amd_bram_rproc_prepare(struct rproc *rproc)
 				   resource_size(&res), da,
 				   rproc_mem_entry_ioremap_wc,
 				   rproc_mem_entry_iounmap,
-				   dev_name(priv->dev));
+				   "%.31s", dev_name(priv->dev));
 	if (!mem)
 		return -ENOMEM;
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/6] remoteproc: amd_mbv_bram: Restore reset GPIO direction after PL reconfig
  2026-11-05  5:18 [PATCH v2 0/6] Add AMD PL Mailbox and RPMsg support for the AMD BRAM remoteproc Ben Levinsky
  2026-11-05  5:18 ` [PATCH v2 1/6] remoteproc: amd_mbv_bram: Bound device name formatting Ben Levinsky
@ 2026-11-05  5:18 ` Ben Levinsky
  2026-11-05  5:29   ` sashiko-bot
  2026-11-05  5:18 ` [PATCH v2 3/6] dt-bindings: mailbox: Add AMD PL Mailbox IP Ben Levinsky
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 10+ messages in thread
From: Ben Levinsky @ 2026-11-05  5:18 UTC (permalink / raw)
  To: jassisinghbrar, andersson, mathieu.poirier, robh, krzk+dt,
	conor+dt
  Cc: michal.simek, tanmay.shah, ben.levinsky, linux-remoteproc,
	devicetree, linux-arm-kernel, linux-kernel

Partial reconfiguration of the programmable logic restores the AXI GPIO
that drives the soft-core reset to its hardware default direction, but
gpiolib keeps its cached output direction. gpiod_set_value_cansleep()
then only updates the data register, so the processor never sees a
reset transition: remoteproc reports it as running while the newly
loaded firmware never starts.

Use gpiod_direction_output() for every reset transition so the output
direction is re-established, and assert reset briefly before releasing
it so each start produces a clean reset pulse after the firmware has
been loaded.

Fixes: f87b4998994e ("remoteproc: Add AMD MicroBlaze/V BRAM-based remote processor driver")
Assisted-by: Claude:claude-opus-5.5
Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
---
 drivers/remoteproc/amd_mbv_bram_rproc.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/amd_mbv_bram_rproc.c b/drivers/remoteproc/amd_mbv_bram_rproc.c
index 6cf33c7b7e64..da621f62f7df 100644
--- a/drivers/remoteproc/amd_mbv_bram_rproc.c
+++ b/drivers/remoteproc/amd_mbv_bram_rproc.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
@@ -94,8 +95,24 @@ static int amd_bram_rproc_start(struct rproc *rproc)
 		return ret;
 	}
 
+	/*
+	 * Partial reconfiguration restores the PL GPIO hardware defaults but
+	 * leaves gpiolib's cached direction unchanged. Re-establish the output
+	 * direction and generate a reset pulse after firmware has been loaded.
+	 * gpiod_set_value_cansleep() is insufficient here because it changes the
+	 * value without restoring an output direction lost during DFX.
+	 */
+	ret = gpiod_direction_output(priv->reset, 1);
+	if (ret) {
+		dev_err(priv->dev, "failed to assert reset: %d\n", ret);
+		clk_disable_unprepare(priv->clk);
+		return ret;
+	}
+	/* Hold reset asserted long enough to provide a clean reset pulse. */
+	usleep_range(10, 20);
+
 	/* Deassert reset and let the processor run. */
-	ret = gpiod_set_value_cansleep(priv->reset, 0);
+	ret = gpiod_direction_output(priv->reset, 0);
 	if (ret) {
 		dev_err(priv->dev, "failed to deassert reset: %d\n", ret);
 		clk_disable_unprepare(priv->clk);
@@ -110,8 +127,8 @@ static int amd_bram_rproc_stop(struct rproc *rproc)
 	struct amd_bram_rproc *priv = rproc->priv;
 	int ret;
 
-	/* Assert reset before disabling the processor clock. */
-	ret = gpiod_set_value_cansleep(priv->reset, 1);
+	/* Restore the output direction after DFX and assert reset. */
+	ret = gpiod_direction_output(priv->reset, 1);
 	if (ret) {
 		dev_err(priv->dev, "failed to assert reset: %d\n", ret);
 		return ret;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/6] dt-bindings: mailbox: Add AMD PL Mailbox IP
  2026-11-05  5:18 [PATCH v2 0/6] Add AMD PL Mailbox and RPMsg support for the AMD BRAM remoteproc Ben Levinsky
  2026-11-05  5:18 ` [PATCH v2 1/6] remoteproc: amd_mbv_bram: Bound device name formatting Ben Levinsky
  2026-11-05  5:18 ` [PATCH v2 2/6] remoteproc: amd_mbv_bram: Restore reset GPIO direction after PL reconfig Ben Levinsky
@ 2026-11-05  5:18 ` Ben Levinsky
  2026-11-05  5:19 ` [PATCH v2 4/6] mailbox: Add AMD PL Mailbox soft IP controller Ben Levinsky
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Ben Levinsky @ 2026-11-05  5:18 UTC (permalink / raw)
  To: jassisinghbrar, andersson, mathieu.poirier, robh, krzk+dt,
	conor+dt
  Cc: michal.simek, tanmay.shah, ben.levinsky, linux-remoteproc,
	devicetree, linux-arm-kernel, linux-kernel

Document the AMD programmable-logic Mailbox IP used for bidirectional
FIFO communication between two processor-facing interfaces.

The Mailbox is a soft IP core instantiated in programmable logic, and the
same SoC can carry different versions of it, so it is not a SoC block.
Like other PL soft IP bindings, use a single compatible that names the IP
core and its version, amd,pl-mailbox-2.1. "pl" distinguishes it from the
IPI hardware that can also act as a mailbox.

Define the transmit and receive channel indices together with the register
aperture, interrupt, and interface clock. Describe the FIFO depth chosen
for each IP instance when the design is synthesized. It cannot be read
from the registers, and the driver needs it to size received messages.

Assisted-by: Claude:claude-opus-5.5
Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
---
 .../bindings/mailbox/amd,pl-mailbox-2.1.yaml  | 73 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 79 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/amd,pl-mailbox-2.1.yaml

diff --git a/Documentation/devicetree/bindings/mailbox/amd,pl-mailbox-2.1.yaml b/Documentation/devicetree/bindings/mailbox/amd,pl-mailbox-2.1.yaml
new file mode 100644
index 000000000000..dc16a2e1142b
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/amd,pl-mailbox-2.1.yaml
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/amd,pl-mailbox-2.1.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMD Programmable Logic Mailbox IP
+
+maintainers:
+  - Ben Levinsky <ben.levinsky@amd.com>
+
+description: |
+  The AMD Programmable Logic Mailbox IP provides bidirectional message passing
+  between two processor-facing interfaces. Each interface writes messages to
+  a send FIFO and reads messages from a receive FIFO. The register aperture,
+  interrupt, and clock described by a node belong to one processor-facing
+  interface. Channel 0 represents transmit and channel 1 represents receive
+  for that interface.
+
+  The Mailbox is a soft IP core instantiated in programmable logic, not a
+  SoC block, so the compatible names the IP core and its version (2.1).
+  The "pl" part distinguishes it from the IPI hardware that can also be
+  used as a mailbox.
+
+properties:
+  compatible:
+    const: amd,pl-mailbox-2.1
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  amd,pl-mailbox-depth:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 16
+    maximum: 8192
+    default: 16
+    description:
+      Depth of the send and receive FIFOs in 32-bit words. The depth is the
+      C_MAILBOX_DEPTH parameter chosen for each IP instance when the design
+      is synthesized, so instances in one design can differ, and it cannot
+      be read from the registers. Must match the synthesized depth.
+
+  '#mbox-cells':
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - '#mbox-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    mailbox@b2060000 {
+        compatible = "amd,pl-mailbox-2.1";
+        reg = <0xb2060000 0x10000>;
+        interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&pl_clk>;
+        amd,pl-mailbox-depth = <16>;
+        #mbox-cells = <1>;
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 1e3f9f2751fe..3722a66b9960 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1306,6 +1306,12 @@ S:	Supported
 T:	git https://gitlab.freedesktop.org/agd5f/linux.git
 F:	drivers/gpu/drm/amd/pm/
 
+AMD PROGRAMMABLE LOGIC MAILBOX DRIVER
+M:	Ben Levinsky <ben.levinsky@amd.com>
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/mailbox/amd,pl-mailbox-2.1.yaml
+
 AMD PSTATE DRIVER
 M:	Huang Rui <ray.huang@amd.com>
 M:	Mario Limonciello <mario.limonciello@amd.com>
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 4/6] mailbox: Add AMD PL Mailbox soft IP controller
  2026-11-05  5:18 [PATCH v2 0/6] Add AMD PL Mailbox and RPMsg support for the AMD BRAM remoteproc Ben Levinsky
                   ` (2 preceding siblings ...)
  2026-11-05  5:18 ` [PATCH v2 3/6] dt-bindings: mailbox: Add AMD PL Mailbox IP Ben Levinsky
@ 2026-11-05  5:19 ` Ben Levinsky
  2026-11-05  5:31   ` sashiko-bot
  2026-11-05  5:19 ` [PATCH v2 5/6] dt-bindings: remoteproc: Add RPMsg resources to AMD BRAM rproc Ben Levinsky
  2026-11-05  5:19 ` [PATCH v2 6/6] remoteproc: amd_mbv_bram: Add RPMsg support Ben Levinsky
  5 siblings, 1 reply; 10+ messages in thread
From: Ben Levinsky @ 2026-11-05  5:19 UTC (permalink / raw)
  To: jassisinghbrar, andersson, mathieu.poirier, robh, krzk+dt,
	conor+dt
  Cc: michal.simek, tanmay.shah, ben.levinsky, linux-remoteproc,
	devicetree, linux-arm-kernel, linux-kernel

Add a mailbox-framework controller for the AMD programmable-logic
Mailbox soft IP. Expose separate transmit and receive FIFO channels,
and match the amd,pl-mailbox-2.1 compatible.

Enable the interface clock only while a channel is started, before any
register access, and disable it last when the channel shuts down. Keep
provider probe free of register accesses and leave the IRQ disabled until
a client starts the receive channel. Initialize each FIFO when its channel
starts, use receive-threshold interrupts for incoming words, and poll the
send-threshold status for transmit completion. Once the transmit channel
begins to shut down, stop both the completion polling and the submission
of queued messages from accessing registers.

Carry mailbox payloads in struct zynqmp_ipi_message, limited to the FIFO
depth given by the device tree. The FIFO transfers 32-bit words and the
hardware conveys neither a byte count nor the FIFO occupancy, so a
transmit length that is not a multiple of four is zero-padded to a whole
word.

Handle the receive interrupt in a threaded handler. Acknowledge the
receive-threshold event first, then read the FIFO until it is empty and
deliver the words as messages of at most one FIFO depth each. The event
latches when the FIFO goes from empty to nonempty, so a word that arrives
after the drain has emptied the FIFO raises a new interrupt, and a full
FIFO is always drained.

Assisted-by: Claude:claude-opus-5.5
Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
---
 MAINTAINERS                      |   1 +
 drivers/mailbox/Kconfig          |  11 +
 drivers/mailbox/Makefile         |   1 +
 drivers/mailbox/amd-pl-mailbox.c | 378 +++++++++++++++++++++++++++++++
 4 files changed, 391 insertions(+)
 create mode 100644 drivers/mailbox/amd-pl-mailbox.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3722a66b9960..e7d93dcb60e6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1311,6 +1311,7 @@ M:	Ben Levinsky <ben.levinsky@amd.com>
 L:	linux-kernel@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/mailbox/amd,pl-mailbox-2.1.yaml
+F:	drivers/mailbox/amd-pl-mailbox.c
 
 AMD PSTATE DRIVER
 M:	Huang Rui <ray.huang@amd.com>
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index ab869769e278..ea6bbc5bd2d2 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -8,6 +8,17 @@ menuconfig MAILBOX
 
 if MAILBOX
 
+config AMD_PL_MAILBOX
+	tristate "AMD PL Mailbox soft IP controller"
+	depends on ARCH_ZYNQMP || COMPILE_TEST
+	depends on HAS_IOMEM
+	depends on OF
+	help
+	  Mailbox controller for the AMD Programmable Logic Mailbox soft IP.
+	  The controller exposes transmit and receive FIFO channels to mailbox
+	  clients. It supports interrupt-driven receive notification and
+	  polling-based transmit completion through the common mailbox framework.
+
 config ARM_MHU
 	tristate "ARM MHU Mailbox"
 	depends on ARM_AMBA
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 45dc59bb3e7f..5614d7e1e2f8 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -4,6 +4,7 @@
 obj-$(CONFIG_MAILBOX)		+= mailbox.o
 
 obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
+obj-$(CONFIG_AMD_PL_MAILBOX)	+= amd-pl-mailbox.o
 
 obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o arm_mhu_db.o
 
diff --git a/drivers/mailbox/amd-pl-mailbox.c b/drivers/mailbox/amd-pl-mailbox.c
new file mode 100644
index 000000000000..dc95fe4eff9b
--- /dev/null
+++ b/drivers/mailbox/amd-pl-mailbox.c
@@ -0,0 +1,378 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD Programmable Logic Mailbox soft IP controller driver
+ *
+ * Copyright (C) 2026 Advanced Micro Devices, Inc.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mailbox/zynqmp-ipi-message.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define AMD_PL_MBOX_WRDATA		0x00
+#define AMD_PL_MBOX_RDDATA		0x08
+#define AMD_PL_MBOX_STATUS		0x10
+#define AMD_PL_MBOX_ERROR		0x14
+#define AMD_PL_MBOX_SEND_THRESHOLD	0x18
+#define AMD_PL_MBOX_RECV_THRESHOLD	0x1c
+#define AMD_PL_MBOX_IRQ_STATUS		0x20
+#define AMD_PL_MBOX_IRQ_ENABLE		0x24
+#define AMD_PL_MBOX_CTRL		0x2c
+
+#define AMD_PL_MBOX_STS_RX_EMPTY	BIT(0)
+#define AMD_PL_MBOX_STS_TX_FULL		BIT(1)
+#define AMD_PL_MBOX_STS_TX_THRESHOLD	BIT(2)
+#define AMD_PL_MBOX_IRQ_RX_THRESHOLD	BIT(1)
+#define AMD_PL_MBOX_IRQ_ERROR		BIT(2)
+#define AMD_PL_MBOX_CTRL_TX_RESET	BIT(0)
+#define AMD_PL_MBOX_CTRL_RX_RESET	BIT(1)
+
+/* FIFO depth range of the IP, in 32-bit words. */
+#define AMD_PL_MBOX_FIFO_LEN_MIN	16
+#define AMD_PL_MBOX_FIFO_LEN_MAX	8192
+
+/*
+ * The send threshold is active while send FIFO occupancy is less than or
+ * equal to the programmed value. The receive threshold is active while
+ * receive FIFO occupancy is greater than the programmed value. The related
+ * interrupt status bits latch when a threshold condition becomes active,
+ * and a new event takes priority over a simultaneous status clear.
+ *
+ * A send threshold of zero makes the status bit indicate an empty TX FIFO.
+ * A receive threshold of zero raises an interrupt for the first received
+ * word after the RX FIFO becomes empty.
+ */
+
+enum {
+	AMD_PL_MBOX_CHAN_TX = 0,
+	AMD_PL_MBOX_CHAN_RX = 1,
+	AMD_PL_MBOX_NUM_CHANS = 2,
+};
+
+/**
+ * struct amd_pl_mbox - AMD PL Mailbox controller state
+ * @regs: mapped register aperture
+ * @clk: interface clock, enabled while a channel is started
+ * @irq: receive interrupt
+ * @fifo_len: FIFO depth in 32-bit words
+ * @tx_lock: serializes TX register access against TX shutdown
+ * @tx_active: TX channel is started and its registers may be accessed
+ * @controller: common mailbox controller
+ * @chans: transmit and receive mailbox channels
+ * @rx_msg: receive buffer passed to mailbox clients
+ */
+struct amd_pl_mbox {
+	void __iomem *regs;
+	struct clk *clk;
+	int irq;
+	u32 fifo_len;
+	spinlock_t tx_lock; /* protects tx_active */
+	bool tx_active;
+	struct mbox_controller controller;
+	struct mbox_chan chans[AMD_PL_MBOX_NUM_CHANS];
+	struct zynqmp_ipi_message *rx_msg;
+};
+
+static int amd_pl_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+	struct amd_pl_mbox *mb = chan->con_priv;
+	struct zynqmp_ipi_message *msg = data;
+	unsigned long flags;
+	size_t offset;
+	int ret = 0;
+
+	if (chan != &mb->chans[AMD_PL_MBOX_CHAN_TX])
+		return -EINVAL;
+
+	/* The TX FIFO is empty here, since TX completion waits for it. */
+	if (!msg || !msg->len || msg->len > mb->fifo_len * sizeof(u32))
+		return -EINVAL;
+
+	/*
+	 * The mailbox core can submit a queued message after shutdown() has
+	 * started, while the interface clock is being disabled. Do not access
+	 * registers then.
+	 */
+	spin_lock_irqsave(&mb->tx_lock, flags);
+	if (!mb->tx_active) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+
+	for (offset = 0; offset < msg->len; offset += sizeof(u32)) {
+		size_t count = min(sizeof(u32), msg->len - offset);
+		u32 word = 0;
+
+		if (readl(mb->regs + AMD_PL_MBOX_STATUS) &
+		    AMD_PL_MBOX_STS_TX_FULL) {
+			ret = -EBUSY;
+			goto unlock;
+		}
+
+		memcpy(&word, &msg->data[offset], count);
+		writel(word, mb->regs + AMD_PL_MBOX_WRDATA);
+	}
+
+unlock:
+	spin_unlock_irqrestore(&mb->tx_lock, flags);
+	return ret;
+}
+
+static bool amd_pl_mbox_last_tx_done(struct mbox_chan *chan)
+{
+	struct amd_pl_mbox *mb = chan->con_priv;
+	unsigned long flags;
+	bool done = true;
+
+	/*
+	 * The mailbox core can poll after shutdown() has started, while the
+	 * interface clock is being disabled. Do not access registers then.
+	 */
+	spin_lock_irqsave(&mb->tx_lock, flags);
+	if (mb->tx_active)
+		/* SIT is zero, so the threshold status means TX FIFO empty. */
+		done = readl(mb->regs + AMD_PL_MBOX_STATUS) &
+		       AMD_PL_MBOX_STS_TX_THRESHOLD;
+	spin_unlock_irqrestore(&mb->tx_lock, flags);
+
+	return done;
+}
+
+static int amd_pl_mbox_startup(struct mbox_chan *chan)
+{
+	struct amd_pl_mbox *mb = chan->con_priv;
+	unsigned long flags;
+	u32 irq_enable;
+	int ret;
+
+	if (chan != &mb->chans[AMD_PL_MBOX_CHAN_TX] &&
+	    chan != &mb->chans[AMD_PL_MBOX_CHAN_RX])
+		return -EINVAL;
+
+	ret = clk_prepare_enable(mb->clk);
+	if (ret)
+		return ret;
+
+	if (chan == &mb->chans[AMD_PL_MBOX_CHAN_TX]) {
+		writel(AMD_PL_MBOX_CTRL_TX_RESET, mb->regs + AMD_PL_MBOX_CTRL);
+		writel(0, mb->regs + AMD_PL_MBOX_SEND_THRESHOLD);
+
+		spin_lock_irqsave(&mb->tx_lock, flags);
+		mb->tx_active = true;
+		spin_unlock_irqrestore(&mb->tx_lock, flags);
+
+		return 0;
+	}
+
+	writel(AMD_PL_MBOX_CTRL_RX_RESET, mb->regs + AMD_PL_MBOX_CTRL);
+	/* Interrupt when the RX FIFO changes from empty to nonempty. */
+	writel(0, mb->regs + AMD_PL_MBOX_RECV_THRESHOLD);
+	writel(AMD_PL_MBOX_IRQ_RX_THRESHOLD | AMD_PL_MBOX_IRQ_ERROR,
+	       mb->regs + AMD_PL_MBOX_IRQ_STATUS);
+	irq_enable = readl(mb->regs + AMD_PL_MBOX_IRQ_ENABLE);
+	irq_enable |= AMD_PL_MBOX_IRQ_RX_THRESHOLD | AMD_PL_MBOX_IRQ_ERROR;
+	writel(irq_enable, mb->regs + AMD_PL_MBOX_IRQ_ENABLE);
+	enable_irq(mb->irq);
+
+	return 0;
+}
+
+static void amd_pl_mbox_shutdown(struct mbox_chan *chan)
+{
+	struct amd_pl_mbox *mb = chan->con_priv;
+	unsigned long flags;
+	u32 irq_enable;
+
+	if (chan == &mb->chans[AMD_PL_MBOX_CHAN_TX]) {
+		spin_lock_irqsave(&mb->tx_lock, flags);
+		mb->tx_active = false;
+		spin_unlock_irqrestore(&mb->tx_lock, flags);
+
+		writel(AMD_PL_MBOX_CTRL_TX_RESET, mb->regs + AMD_PL_MBOX_CTRL);
+		clk_disable_unprepare(mb->clk);
+		return;
+	}
+
+	if (chan != &mb->chans[AMD_PL_MBOX_CHAN_RX])
+		return;
+
+	disable_irq(mb->irq);
+	irq_enable = readl(mb->regs + AMD_PL_MBOX_IRQ_ENABLE);
+	irq_enable &= ~(AMD_PL_MBOX_IRQ_RX_THRESHOLD | AMD_PL_MBOX_IRQ_ERROR);
+	writel(irq_enable, mb->regs + AMD_PL_MBOX_IRQ_ENABLE);
+	writel(AMD_PL_MBOX_IRQ_RX_THRESHOLD | AMD_PL_MBOX_IRQ_ERROR,
+	       mb->regs + AMD_PL_MBOX_IRQ_STATUS);
+	writel(AMD_PL_MBOX_CTRL_RX_RESET, mb->regs + AMD_PL_MBOX_CTRL);
+	clk_disable_unprepare(mb->clk);
+}
+
+static const struct mbox_chan_ops amd_pl_mbox_ops = {
+	.send_data = amd_pl_mbox_send_data,
+	.last_tx_done = amd_pl_mbox_last_tx_done,
+	.startup = amd_pl_mbox_startup,
+	.shutdown = amd_pl_mbox_shutdown,
+};
+
+static void amd_pl_mbox_rx(struct amd_pl_mbox *mb)
+{
+	/* data[] follows a size_t in a kzalloc() buffer: u32 aligned. */
+	u32 *rx_words = (u32 *)mb->rx_msg->data;
+	u32 words;
+
+	/*
+	 * The hardware reports neither the peer's message length nor the FIFO
+	 * occupancy. Read until the FIFO is empty and deliver the words as
+	 * messages of at most one FIFO depth each.
+	 */
+	do {
+		for (words = 0; words < mb->fifo_len; words++) {
+			if (readl(mb->regs + AMD_PL_MBOX_STATUS) &
+			    AMD_PL_MBOX_STS_RX_EMPTY)
+				break;
+
+			rx_words[words] = readl(mb->regs + AMD_PL_MBOX_RDDATA);
+		}
+
+		if (!words)
+			break;
+
+		mb->rx_msg->len = words * sizeof(u32);
+		mbox_chan_received_data(&mb->chans[AMD_PL_MBOX_CHAN_RX],
+					mb->rx_msg);
+	} while (words == mb->fifo_len);
+}
+
+static irqreturn_t amd_pl_mbox_irq_thread(int irq, void *data)
+{
+	struct amd_pl_mbox *mb = data;
+	u32 pending_intr;
+	u32 irq_enable;
+	u32 irq_status;
+
+	irq_status = readl(mb->regs + AMD_PL_MBOX_IRQ_STATUS);
+	irq_enable = readl(mb->regs + AMD_PL_MBOX_IRQ_ENABLE);
+	pending_intr = irq_status & irq_enable;
+
+	if (!pending_intr) {
+		dev_warn_ratelimited(mb->controller.dev,
+				     "unexpected interrupt: status %#x enable %#x\n",
+				     irq_status, irq_enable);
+		return IRQ_NONE;
+	}
+
+	/*
+	 * Acknowledge before draining. The receive event latches when the RX
+	 * FIFO goes from empty to nonempty, so a word that arrives after the
+	 * drain has emptied the FIFO raises a new event.
+	 */
+	writel(pending_intr, mb->regs + AMD_PL_MBOX_IRQ_STATUS);
+
+	if (pending_intr & AMD_PL_MBOX_IRQ_ERROR)
+		dev_warn_ratelimited(mb->controller.dev,
+				     "mailbox FIFO error: %#x\n",
+				     readl(mb->regs + AMD_PL_MBOX_ERROR));
+
+	if (pending_intr & AMD_PL_MBOX_IRQ_RX_THRESHOLD)
+		amd_pl_mbox_rx(mb);
+
+	return IRQ_HANDLED;
+}
+
+static struct mbox_chan *amd_pl_mbox_xlate(struct mbox_controller *ctlr,
+					   const struct of_phandle_args *sp)
+{
+	if (sp->args_count != 1 || sp->args[0] >= AMD_PL_MBOX_NUM_CHANS)
+		return ERR_PTR(-EINVAL);
+
+	return &ctlr->chans[sp->args[0]];
+}
+
+static int amd_pl_mbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct amd_pl_mbox *mb;
+	int ret;
+	int i;
+
+	mb = devm_kzalloc(dev, sizeof(*mb), GFP_KERNEL);
+	if (!mb)
+		return -ENOMEM;
+
+	spin_lock_init(&mb->tx_lock);
+
+	mb->fifo_len = AMD_PL_MBOX_FIFO_LEN_MIN;
+	ret = of_property_read_u32(dev->of_node, "amd,pl-mailbox-depth",
+				   &mb->fifo_len);
+	if (ret && ret != -EINVAL)
+		return dev_err_probe(dev, ret, "failed to read FIFO length\n");
+
+	if (mb->fifo_len < AMD_PL_MBOX_FIFO_LEN_MIN ||
+	    mb->fifo_len > AMD_PL_MBOX_FIFO_LEN_MAX)
+		return dev_err_probe(dev, -EINVAL, "invalid FIFO length %u\n",
+				     mb->fifo_len);
+
+	mb->rx_msg = devm_kzalloc(dev, struct_size(mb->rx_msg, data,
+						   mb->fifo_len * sizeof(u32)),
+				  GFP_KERNEL);
+	if (!mb->rx_msg)
+		return -ENOMEM;
+
+	mb->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(mb->regs))
+		return PTR_ERR(mb->regs);
+
+	mb->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(mb->clk))
+		return dev_err_probe(dev, PTR_ERR(mb->clk), "failed to get clock\n");
+
+	mb->irq = platform_get_irq(pdev, 0);
+	if (mb->irq < 0)
+		return mb->irq;
+
+	for (i = 0; i < AMD_PL_MBOX_NUM_CHANS; i++)
+		mb->chans[i].con_priv = mb;
+
+	mb->controller.dev = dev;
+	mb->controller.ops = &amd_pl_mbox_ops;
+	mb->controller.chans = mb->chans;
+	mb->controller.num_chans = AMD_PL_MBOX_NUM_CHANS;
+	mb->controller.txdone_poll = true;
+	mb->controller.txpoll_period = 1;
+	mb->controller.of_xlate = amd_pl_mbox_xlate;
+
+	/* Draining up to one FIFO depth of words is done in process context. */
+	ret = devm_request_threaded_irq(dev, mb->irq, NULL,
+					amd_pl_mbox_irq_thread,
+					IRQF_ONESHOT | IRQF_NO_AUTOEN,
+					dev_name(dev), mb);
+	if (ret)
+		return ret;
+
+	return devm_mbox_controller_register(dev, &mb->controller);
+}
+
+static const struct of_device_id amd_pl_mbox_of_match[] = {
+	{ .compatible = "amd,pl-mailbox-2.1" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, amd_pl_mbox_of_match);
+
+static struct platform_driver amd_pl_mbox_driver = {
+	.probe = amd_pl_mbox_probe,
+	.driver = {
+		.name = "amd-pl-mailbox",
+		.of_match_table = amd_pl_mbox_of_match,
+	},
+};
+module_platform_driver(amd_pl_mbox_driver);
+
+MODULE_DESCRIPTION("AMD PL Mailbox soft IP controller");
+MODULE_LICENSE("GPL");
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 5/6] dt-bindings: remoteproc: Add RPMsg resources to AMD BRAM rproc
  2026-11-05  5:18 [PATCH v2 0/6] Add AMD PL Mailbox and RPMsg support for the AMD BRAM remoteproc Ben Levinsky
                   ` (3 preceding siblings ...)
  2026-11-05  5:19 ` [PATCH v2 4/6] mailbox: Add AMD PL Mailbox soft IP controller Ben Levinsky
@ 2026-11-05  5:19 ` Ben Levinsky
  2026-11-05  5:19 ` [PATCH v2 6/6] remoteproc: amd_mbv_bram: Add RPMsg support Ben Levinsky
  5 siblings, 0 replies; 10+ messages in thread
From: Ben Levinsky @ 2026-11-05  5:19 UTC (permalink / raw)
  To: jassisinghbrar, andersson, mathieu.poirier, robh, krzk+dt,
	conor+dt
  Cc: michal.simek, tanmay.shah, ben.levinsky, linux-remoteproc,
	devicetree, linux-arm-kernel, linux-kernel

Describe the optional mailbox channels and named reserved-memory
regions used for RPMsg communication with a BRAM-based MicroBlaze
remote processor.

The three named memory regions provide the two virtio vrings and the
shared RPMsg buffer pool. Firmware and its loaded resource table remain
in the executable BRAM described by reg.

Assisted-by: Claude:claude-opus-5.5
Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
---
 .../bindings/remoteproc/amd,bram-rproc.yaml   | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/amd,bram-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/amd,bram-rproc.yaml
index c0359f447ea8..462bd4be087d 100644
--- a/Documentation/devicetree/bindings/remoteproc/amd,bram-rproc.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/amd,bram-rproc.yaml
@@ -84,6 +84,23 @@ properties:
     description:
       GPIO specifier controlling the soft-core reset input.
 
+  mboxes:
+    maxItems: 2
+
+  mbox-names:
+    items:
+      - const: tx
+      - const: rx
+
+  memory-region:
+    maxItems: 3
+
+  memory-region-names:
+    items:
+      - const: vdev0vring0
+      - const: vdev0vring1
+      - const: vdev0buffer
+
 required:
   - compatible
   - reg
@@ -101,5 +118,9 @@ examples:
       clocks = <&pl_clk>;
       firmware-name = "firmware.elf";
       reset-gpios = <&gpio0 0 GPIO_ACTIVE_LOW>;
+      mboxes = <&mbv_mbox0 0>, <&mbv_mbox0 1>;
+      mbox-names = "tx", "rx";
+      memory-region = <&mbv_vring0>, <&mbv_vring1>, <&mbv_vdev0buffer>;
+      memory-region-names = "vdev0vring0", "vdev0vring1", "vdev0buffer";
     };
 ...
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 6/6] remoteproc: amd_mbv_bram: Add RPMsg support
  2026-11-05  5:18 [PATCH v2 0/6] Add AMD PL Mailbox and RPMsg support for the AMD BRAM remoteproc Ben Levinsky
                   ` (4 preceding siblings ...)
  2026-11-05  5:19 ` [PATCH v2 5/6] dt-bindings: remoteproc: Add RPMsg resources to AMD BRAM rproc Ben Levinsky
@ 2026-11-05  5:19 ` Ben Levinsky
  2026-11-05  5:39   ` sashiko-bot
  5 siblings, 1 reply; 10+ messages in thread
From: Ben Levinsky @ 2026-11-05  5:19 UTC (permalink / raw)
  To: jassisinghbrar, andersson, mathieu.poirier, robh, krzk+dt,
	conor+dt
  Cc: michal.simek, tanmay.shah, ben.levinsky, linux-remoteproc,
	devicetree, linux-arm-kernel, linux-kernel

Register named reserved-memory carveouts and expose the firmware
resource table after it has been loaded into BRAM. This lets remoteproc
create virtqueues backed by reserved shared DDR without deriving
carveout roles from DT node names. Map the shared-memory carveouts with
the common rproc_mem_entry_ioremap_wc() and rproc_mem_entry_iounmap()
callbacks.

Use the mailbox framework to send virtqueue notification IDs. Defer
received notifications to process context before dispatching them to
the remoteproc virtqueues. Keep mailbox payload storage valid until
transmit completion and order write-combined vring updates before each
kick.

Request the mailbox channels at probe time, so that a missing mailbox
controller defers the probe instead of failing a later start. Release
them when the device is unbound, after the remote processor has been
stopped. Free the channels before cancelling the receive work, so that a
late notification cannot queue the work again.

The mailboxes are optional. Without them the driver keeps supporting
load, start, and stop only.

The driver now calls the mailbox client API, which has no stubs for
!MAILBOX, so depend on MAILBOX. This makes the driver unavailable in
configurations without the mailbox framework, even for designs that do
not use RPMsg. The PL Mailbox controller and the virtio RPMsg bus are
only needed at run time; the Kconfig help text names them.

Assisted-by: Claude:claude-opus-5.5
Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
---
 drivers/remoteproc/Kconfig              |   5 +
 drivers/remoteproc/amd_mbv_bram_rproc.c | 245 +++++++++++++++++++++++-
 2 files changed, 249 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 5b56b2dcc725..4efb60a953a7 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -26,10 +26,15 @@ config REMOTEPROC_CDEV
 config AMD_MBV_BRAM_REMOTEPROC
 	tristate "AMD MicroBlaze/V BRAM-based remoteproc support"
 	depends on OF && COMMON_CLK && (GPIOLIB || COMPILE_TEST)
+	depends on MAILBOX
 	help
 	  Say y or m here to support a MicroBlaze/V BRAM-based remote
 	  processor managed through the remoteproc framework.
 
+	  RPMsg communication additionally needs the mailbox controller
+	  referenced by the mboxes property, such as AMD_PL_MAILBOX, and
+	  RPMSG_VIRTIO.
+
 	  If unsure, say N.
 
 config IMX_REMOTEPROC
diff --git a/drivers/remoteproc/amd_mbv_bram_rproc.c b/drivers/remoteproc/amd_mbv_bram_rproc.c
index da621f62f7df..9c2694fd645a 100644
--- a/drivers/remoteproc/amd_mbv_bram_rproc.c
+++ b/drivers/remoteproc/amd_mbv_bram_rproc.c
@@ -15,15 +15,20 @@
  * Reset is controlled via GPIO connected to Processor System Reset IP.
  */
 
+#include <linux/bitmap.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/gpio/consumer.h>
+#include <linux/mailbox/zynqmp-ipi-message.h>
+#include <linux/mailbox_client.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
 #include <linux/remoteproc.h>
+#include <linux/workqueue.h>
 
 #include "remoteproc_internal.h"
 
@@ -32,13 +37,220 @@
  * @dev: device pointer
  * @reset: GPIO descriptor for reset control (active-low)
  * @clk: processor clock
+ * @rproc: remote processor instance
+ * @tx_client: mailbox client used for virtqueue kicks
+ * @rx_client: mailbox client used for remote notifications
+ * @tx_chan: transmit mailbox channel
+ * @rx_chan: receive mailbox channel
+ * @notify_msgs: persistent mailbox payload storage indexed by notify ID
+ * @pending_notify_ids: notifications awaiting process-context dispatch
+ * @rx_work: work item that dispatches received virtqueue notifications
  */
 struct amd_bram_rproc {
 	struct device *dev;
 	struct gpio_desc *reset;
 	struct clk *clk;
+	struct rproc *rproc;
+	struct mbox_client tx_client;
+	struct mbox_client rx_client;
+	struct mbox_chan *tx_chan;
+	struct mbox_chan *rx_chan;
+	struct zynqmp_ipi_message *notify_msgs[256];
+	DECLARE_BITMAP(pending_notify_ids, 256);
+	struct work_struct rx_work;
 };
 
+static void amd_bram_rproc_rx_work(struct work_struct *work)
+{
+	struct amd_bram_rproc *priv =
+		container_of(work, struct amd_bram_rproc, rx_work);
+	unsigned long notifyid;
+
+	for_each_set_bit(notifyid, priv->pending_notify_ids,
+			 ARRAY_SIZE(priv->notify_msgs)) {
+		clear_bit(notifyid, priv->pending_notify_ids);
+		if (rproc_vq_interrupt(priv->rproc, notifyid) == IRQ_NONE)
+			dev_dbg(priv->dev, "no virtqueue for notifyid %lu\n",
+				notifyid);
+	}
+}
+
+static void amd_bram_rproc_rx_callback(struct mbox_client *client, void *msg)
+{
+	struct amd_bram_rproc *priv =
+		container_of(client, struct amd_bram_rproc, rx_client);
+	struct zynqmp_ipi_message *mb_msg = msg;
+	size_t offset;
+	u32 notifyid;
+
+	if (!msg) {
+		dev_warn_ratelimited(priv->dev,
+				     "received mailbox notification without data\n");
+		return;
+	}
+
+	/* Each notification is one 32-bit notify ID. */
+	if (!mb_msg->len || mb_msg->len % sizeof(u32)) {
+		dev_warn_ratelimited(priv->dev,
+				     "malformed mailbox notification: %zu bytes\n",
+				     mb_msg->len);
+		return;
+	}
+
+	for (offset = 0; offset < mb_msg->len; offset += sizeof(u32)) {
+		memcpy(&notifyid, &mb_msg->data[offset], sizeof(notifyid));
+		if (notifyid >= ARRAY_SIZE(priv->notify_msgs)) {
+			dev_warn_ratelimited(priv->dev, "invalid notify id %u\n",
+					     notifyid);
+			continue;
+		}
+
+		set_bit(notifyid, priv->pending_notify_ids);
+	}
+
+	schedule_work(&priv->rx_work);
+}
+
+static void amd_bram_rproc_kick(struct rproc *rproc, int vqid)
+{
+	struct amd_bram_rproc *priv = rproc->priv;
+	struct zynqmp_ipi_message *msg;
+	int ret;
+
+	if (!priv->tx_chan) {
+		dev_warn_ratelimited(priv->dev,
+				     "cannot kick vq%d: no TX mailbox\n", vqid);
+		return;
+	}
+
+	if (vqid < 0 || vqid >= ARRAY_SIZE(priv->notify_msgs)) {
+		dev_err_ratelimited(priv->dev, "invalid notify id %d\n", vqid);
+		return;
+	}
+
+	/*
+	 * The mailbox core queues message pointers. Keep each notification word
+	 * in persistent storage until the controller reports TX completion.
+	 */
+	msg = priv->notify_msgs[vqid];
+	msg->len = sizeof(u32);
+	memcpy(msg->data, &vqid, sizeof(u32));
+	/* Order writes to WC vring mappings before notifying the remote. */
+	wmb();
+	ret = mbox_send_message(priv->tx_chan, msg);
+	if (ret < 0)
+		dev_err_ratelimited(priv->dev, "failed to kick vq%d: %d\n",
+				    vqid, ret);
+}
+
+static void amd_bram_rproc_free_mboxes(struct amd_bram_rproc *priv)
+{
+	if (priv->rx_chan) {
+		mbox_free_channel(priv->rx_chan);
+		priv->rx_chan = NULL;
+	}
+	if (priv->tx_chan) {
+		mbox_free_channel(priv->tx_chan);
+		priv->tx_chan = NULL;
+	}
+}
+
+static int amd_bram_rproc_request_mboxes(struct amd_bram_rproc *priv)
+{
+	int ret;
+
+	/* Without mailboxes the driver supports load, start, and stop only. */
+	if (!of_property_present(priv->dev->of_node, "mboxes"))
+		return 0;
+
+	priv->tx_chan = mbox_request_channel_byname(&priv->tx_client, "tx");
+	if (IS_ERR(priv->tx_chan)) {
+		ret = PTR_ERR(priv->tx_chan);
+		priv->tx_chan = NULL;
+		return dev_err_probe(priv->dev, ret,
+				     "failed to request tx mailbox\n");
+	}
+
+	priv->rx_chan = mbox_request_channel_byname(&priv->rx_client, "rx");
+	if (IS_ERR(priv->rx_chan)) {
+		ret = PTR_ERR(priv->rx_chan);
+		priv->rx_chan = NULL;
+		mbox_free_channel(priv->tx_chan);
+		priv->tx_chan = NULL;
+		return dev_err_probe(priv->dev, ret,
+				     "failed to request rx mailbox\n");
+	}
+
+	return 0;
+}
+
+static void amd_bram_rproc_release_mboxes(void *data)
+{
+	struct amd_bram_rproc *priv = data;
+
+	/*
+	 * Free the channels before cancelling the receive work. RX channel
+	 * shutdown disables the controller interrupt synchronously, so no
+	 * notification can queue the work again once it has been cancelled.
+	 */
+	amd_bram_rproc_free_mboxes(priv);
+	cancel_work_sync(&priv->rx_work);
+}
+
+static int amd_bram_rproc_add_reserved_mem(struct rproc *rproc)
+{
+	struct amd_bram_rproc *priv = rproc->priv;
+	struct device_node *np = priv->dev->of_node;
+	struct rproc_mem_entry *mem;
+	int count;
+	int ret;
+	int i;
+
+	if (!of_property_present(np, "memory-region"))
+		return 0;
+
+	count = of_reserved_mem_region_count(np);
+	if (count < 0)
+		return count;
+
+	for (i = 0; i < count; i++) {
+		struct resource res;
+		const char *name;
+
+		ret = of_property_read_string_index(np, "memory-region-names",
+						    i, &name);
+		if (ret)
+			return dev_err_probe(priv->dev, ret,
+					     "failed to read memory-region name %d\n",
+					     i);
+
+		ret = of_reserved_mem_region_to_resource(np, i, &res);
+		if (ret)
+			return dev_err_probe(priv->dev, ret,
+					     "failed to parse memory-region %d\n", i);
+
+		if (!strcmp(name, "vdev0buffer"))
+			mem = rproc_of_resm_mem_entry_init(priv->dev, i,
+							   resource_size(&res),
+							   res.start, name);
+		else
+			mem = rproc_mem_entry_init(priv->dev, NULL,
+						   (dma_addr_t)res.start,
+						   resource_size(&res), res.start,
+						   rproc_mem_entry_ioremap_wc,
+						   rproc_mem_entry_iounmap,
+						   "%s", name);
+		if (!mem)
+			return -ENOMEM;
+
+		rproc_add_carveout(rproc, mem);
+		rproc_coredump_add_segment(rproc, res.start, resource_size(&res));
+		dev_dbg(priv->dev, "reserved memory carveout %pR\n", &res);
+	}
+
+	return 0;
+}
+
 static int amd_bram_rproc_prepare(struct rproc *rproc)
 {
 	struct amd_bram_rproc *priv = rproc->priv;
@@ -80,7 +292,7 @@ static int amd_bram_rproc_prepare(struct rproc *rproc)
 	rproc_add_carveout(rproc, mem);
 	rproc_coredump_add_segment(rproc, da, resource_size(&res));
 
-	return 0;
+	return amd_bram_rproc_add_reserved_mem(rproc);
 }
 
 static int amd_bram_rproc_start(struct rproc *rproc)
@@ -133,6 +345,9 @@ static int amd_bram_rproc_stop(struct rproc *rproc)
 		dev_err(priv->dev, "failed to assert reset: %d\n", ret);
 		return ret;
 	}
+	cancel_work_sync(&priv->rx_work);
+	bitmap_zero(priv->pending_notify_ids,
+		    ARRAY_SIZE(priv->notify_msgs));
 
 	/* Disable clock after asserting reset */
 	clk_disable_unprepare(priv->clk);
@@ -156,6 +371,8 @@ static const struct rproc_ops amd_bram_rproc_ops = {
 	.sanity_check	= rproc_elf_sanity_check,
 	.get_boot_addr	= rproc_elf_get_boot_addr,
 	.parse_fw	= amd_bram_rproc_parse_fw,
+	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
+	.kick		= amd_bram_rproc_kick,
 };
 
 static int amd_bram_rproc_probe(struct platform_device *pdev)
@@ -165,6 +382,7 @@ static int amd_bram_rproc_probe(struct platform_device *pdev)
 	const char *fw_name = NULL;
 	struct rproc *rproc;
 	int ret;
+	int i;
 
 	ret = rproc_of_parse_firmware(dev, 0, &fw_name);
 	if (ret < 0 && ret != -EINVAL)
@@ -178,6 +396,8 @@ static int amd_bram_rproc_probe(struct platform_device *pdev)
 
 	priv = rproc->priv;
 	priv->dev = dev;
+	priv->rproc = rproc;
+	INIT_WORK(&priv->rx_work, amd_bram_rproc_rx_work);
 
 	/* Get the processor clock */
 	priv->clk = devm_clk_get(dev, NULL);
@@ -195,6 +415,29 @@ static int amd_bram_rproc_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(priv->reset),
 				     "failed to get reset gpio\n");
 
+	priv->tx_client.dev = dev;
+	priv->tx_client.tx_block = false;
+	priv->tx_client.knows_txdone = false;
+	priv->rx_client.dev = dev;
+	priv->rx_client.rx_callback = amd_bram_rproc_rx_callback;
+	for (i = 0; i < ARRAY_SIZE(priv->notify_msgs); i++) {
+		priv->notify_msgs[i] =
+			devm_kzalloc(dev, struct_size(priv->notify_msgs[i],
+						      data, sizeof(u32)),
+				     GFP_KERNEL);
+		if (!priv->notify_msgs[i])
+			return -ENOMEM;
+	}
+
+	ret = amd_bram_rproc_request_mboxes(priv);
+	if (ret)
+		return ret;
+
+	/* Registered before the rproc, so it runs after the rproc is stopped. */
+	ret = devm_add_action_or_reset(dev, amd_bram_rproc_release_mboxes, priv);
+	if (ret)
+		return ret;
+
 	rproc->auto_boot = false;
 
 	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/6] remoteproc: amd_mbv_bram: Restore reset GPIO direction after PL reconfig
  2026-11-05  5:18 ` [PATCH v2 2/6] remoteproc: amd_mbv_bram: Restore reset GPIO direction after PL reconfig Ben Levinsky
@ 2026-11-05  5:29   ` sashiko-bot
  0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-11-05  5:29 UTC (permalink / raw)
  To: Ben Levinsky; +Cc: devicetree, robh, conor+dt

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] remoteproc: amd_mbv_bram: processor not held in reset during fw load after DFX
--

commit 1347fae87b8f238d03e271a968bc4d43270edfb5
Author: Ben Levinsky <ben.levinsky@amd.com>

remoteproc: amd_mbv_bram: Restore reset GPIO direction after PL reconfig

This commit replaces gpiod_set_value_cansleep() with gpiod_direction_output()
when asserting and deasserting the soft-core reset line, and adds a reset
pulse in amd_bram_rproc_start() so that the GPIO output direction is restored
after programmable logic partial reconfiguration.

> diff --git a/drivers/remoteproc/amd_mbv_bram_rproc.c b/drivers/remoteproc/amd_mbv_bram_rproc.c
> index 6cf33c7b7e641..da621f62f7df5 100644
> --- a/drivers/remoteproc/amd_mbv_bram_rproc.c
> +++ b/drivers/remoteproc/amd_mbv_bram_rproc.c
[ ... ]
> @@ -94,8 +95,24 @@ static int amd_bram_rproc_start(struct rproc *rproc)
>  		return ret;
>  	}
>  
> +	/*
> +	 * Partial reconfiguration restores the PL GPIO hardware defaults but
> +	 * leaves gpiolib's cached direction unchanged. Re-establish the output
> +	 * direction and generate a reset pulse after firmware has been loaded.
> +	 * gpiod_set_value_cansleep() is insufficient here because it changes the
> +	 * value without restoring an output direction lost during DFX.
> +	 */
> +	ret = gpiod_direction_output(priv->reset, 1);

[Severity: Medium]
If partial reconfiguration restores the PL GPIO to its hardware default input
direction while the remoteproc is offline, is restoring the output direction
and asserting reset in amd_bram_rproc_start() too late to keep the processor
in reset while firmware is loaded into BRAM?

In amd_bram_rproc_probe(), the driver notes that the processor must be held in
reset during firmware loading:

	/*
	 * Keep the processor in reset until remoteproc has finished loading
	 * firmware into the executable memory window described by reg and
	 * translated through the parent bus ranges property.
	 */
	priv->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);

However, during rproc_boot() -> rproc_fw_boot(), remoteproc calls
rproc_prepare_device() -> amd_bram_rproc_prepare() and then rproc_start(),
which runs rproc_load_segments() -> rproc_elf_load_segments() and copies the
loaded resource table into dual-port BRAM before invoking
amd_bram_rproc_start().

If the PL clock is already running (such as a shared PL clock required to
access the AXI BRAM controller during rproc_elf_load_segments()) and DFX left
the reset GPIO released in input mode, the soft-core processor can begin
executing from BRAM while rproc_load_segments() is writing the ELF image.
Even though amd_bram_rproc_start() subsequently pulses reset for 10-20 us,
any writable segments (.data, .bss, or the resource table) modified in BRAM
by that premature execution will not be reloaded before the processor starts
again.

Should gpiod_direction_output(priv->reset, 1) also be called in
amd_bram_rproc_prepare() so that the processor is guaranteed to be held in
reset before rproc_load_segments() writes to BRAM?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105051902.1646651-1-ben.levinsky@amd.com?part=2

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 4/6] mailbox: Add AMD PL Mailbox soft IP controller
  2026-11-05  5:19 ` [PATCH v2 4/6] mailbox: Add AMD PL Mailbox soft IP controller Ben Levinsky
@ 2026-11-05  5:31   ` sashiko-bot
  0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-11-05  5:31 UTC (permalink / raw)
  To: Ben Levinsky; +Cc: devicetree, robh, conor+dt

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mailbox: partial FIFO write on TX busy in amd_pl_mbox_send_data()
- [High] mailbox: lost RX threshold interrupt race in amd_pl_mbox_startup()
- [Medium] mailbox: skipped error read in amd_pl_mbox_irq_thread() when ratelimited
--

commit f34934c4693ba6bdc80f192703145aea6291afe7
Author: Ben Levinsky <ben.levinsky@amd.com>

mailbox: Add AMD PL Mailbox soft IP controller

This commit adds a mailbox controller driver for the AMD Programmable Logic
Mailbox soft IP, exposing separate transmit and receive FIFO channels using
zynqmp_ipi_message payloads. It manages interface clock gating across channel
startup and shutdown, polling-based TX completion, and threaded IRQ-driven RX
FIFO draining.

> diff --git a/drivers/mailbox/amd-pl-mailbox.c b/drivers/mailbox/amd-pl-mailbox.c
> new file mode 100644
> index 0000000000000..dc95fe4eff9b6
> --- /dev/null
> +++ b/drivers/mailbox/amd-pl-mailbox.c
> @@ -0,0 +1,378 @@

[ ... ]

> +static int amd_pl_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
> +	struct amd_pl_mbox *mb = chan->con_priv;
> +	struct zynqmp_ipi_message *msg = data;
> +	unsigned long flags;
> +	size_t offset;
> +	int ret = 0;
> +
> +	if (chan != &mb->chans[AMD_PL_MBOX_CHAN_TX])
> +		return -EINVAL;
> +
> +	/* The TX FIFO is empty here, since TX completion waits for it. */
> +	if (!msg || !msg->len || msg->len > mb->fifo_len * sizeof(u32))
> +		return -EINVAL;
> +
> +	/*
> +	 * The mailbox core can submit a queued message after shutdown() has
> +	 * started, while the interface clock is being disabled. Do not access
> +	 * registers then.
> +	 */
> +	spin_lock_irqsave(&mb->tx_lock, flags);
> +	if (!mb->tx_active) {
> +		ret = -ENODEV;
> +		goto unlock;
> +	}
> +
> +	for (offset = 0; offset < msg->len; offset += sizeof(u32)) {
> +		size_t count = min(sizeof(u32), msg->len - offset);
> +		u32 word = 0;
> +
> +		if (readl(mb->regs + AMD_PL_MBOX_STATUS) &
> +		    AMD_PL_MBOX_STS_TX_FULL) {
> +			ret = -EBUSY;
> +			goto unlock;
> +		}
> +
> +		memcpy(&word, &msg->data[offset], count);
> +		writel(word, mb->regs + AMD_PL_MBOX_WRDATA);
> +	}
> +
> +unlock:
> +	spin_unlock_irqrestore(&mb->tx_lock, flags);
> +	return ret;
> +}

[Severity: High]
In drivers/mailbox/amd-pl-mailbox.c, amd_pl_mbox_send_data() assumes the
hardware TX FIFO is empty on entry and checks AMD_PL_MBOX_STS_TX_FULL before
each word inside the loop. Can this corrupt or coalesce messages in the
hardware TX FIFO if a previous transfer timed out or if the FIFO becomes full
mid-message?

In drivers/mailbox/mailbox.c, when a blocking transfer (cl->tx_block) times
out in mbox_send_message(), it calls tx_tick(chan, -ETIME) -> msg_submit():

drivers/mailbox/mailbox.c:mbox_send_message()
	ret = wait_for_completion_timeout(&chan->tx_complete, wait);
	if (ret == 0) {
		t = -ETIME;
		tx_tick(chan, t);
	}

tx_tick() clears chan->active_req and invokes msg_submit(chan) to submit the
next queued message via ops->send_data() even if the remote peer has not yet
drained the timed-out message from the hardware TX FIFO (and similarly if a
client sets cl->knows_txdone).

Because amd_pl_mbox_send_data() does not check that the TX FIFO is empty
(AMD_PL_MBOX_STS_TX_THRESHOLD in AMD_PL_MBOX_STATUS) before writing:

1. If unconsumed words remain in the TX FIFO and space is still available, the
new message's words are written directly behind the old words, coalescing two
messages in the unframed hardware FIFO.

2. If AMD_PL_MBOX_STS_TX_FULL is hit after one or more words have already been
written (offset > 0), amd_pl_mbox_send_data() returns -EBUSY after partially
writing the message to AMD_PL_MBOX_WRDATA. In msg_submit():

drivers/mailbox/mailbox.c:msg_submit()
	err = chan->mbox->ops->send_data(chan, data);
	if (!err) {
		chan->active_req = data;
		chan->msg_count--;
	}

When send_data() returns -EBUSY, the message is not dequeued and poll_hrt is
not started, leaving the partial words in the hardware FIFO while the full
message remains queued at the head of chan->msg_data[] to be sent again from
offset 0.

Should amd_pl_mbox_send_data() verify that AMD_PL_MBOX_STS_TX_THRESHOLD is set
before writing any words to AMD_PL_MBOX_WRDATA?

[ ... ]

> +static int amd_pl_mbox_startup(struct mbox_chan *chan)
> +{
> +	struct amd_pl_mbox *mb = chan->con_priv;
> +	unsigned long flags;
> +	u32 irq_enable;
> +	int ret;
> +
> +	if (chan != &mb->chans[AMD_PL_MBOX_CHAN_TX] &&
> +	    chan != &mb->chans[AMD_PL_MBOX_CHAN_RX])
> +		return -EINVAL;
> +
> +	ret = clk_prepare_enable(mb->clk);
> +	if (ret)
> +		return ret;
> +
> +	if (chan == &mb->chans[AMD_PL_MBOX_CHAN_TX]) {
> +		writel(AMD_PL_MBOX_CTRL_TX_RESET, mb->regs + AMD_PL_MBOX_CTRL);
> +		writel(0, mb->regs + AMD_PL_MBOX_SEND_THRESHOLD);
> +
> +		spin_lock_irqsave(&mb->tx_lock, flags);
> +		mb->tx_active = true;
> +		spin_unlock_irqrestore(&mb->tx_lock, flags);
> +
> +		return 0;
> +	}
> +
> +	writel(AMD_PL_MBOX_CTRL_RX_RESET, mb->regs + AMD_PL_MBOX_CTRL);
> +	/* Interrupt when the RX FIFO changes from empty to nonempty. */
> +	writel(0, mb->regs + AMD_PL_MBOX_RECV_THRESHOLD);
> +	writel(AMD_PL_MBOX_IRQ_RX_THRESHOLD | AMD_PL_MBOX_IRQ_ERROR,
> +	       mb->regs + AMD_PL_MBOX_IRQ_STATUS);
> +	irq_enable = readl(mb->regs + AMD_PL_MBOX_IRQ_ENABLE);
> +	irq_enable |= AMD_PL_MBOX_IRQ_RX_THRESHOLD | AMD_PL_MBOX_IRQ_ERROR;
> +	writel(irq_enable, mb->regs + AMD_PL_MBOX_IRQ_ENABLE);
> +	enable_irq(mb->irq);
> +
> +	return 0;
> +}

[Severity: High]
In drivers/mailbox/amd-pl-mailbox.c, when amd_pl_mbox_startup() initializes
the RX channel, can a word arriving from the remote peer between the RX FIFO
reset and the AMD_PL_MBOX_IRQ_STATUS clear permanently wedge RX interrupts?

As noted in the driver comments, with AMD_PL_MBOX_RECV_THRESHOLD set to 0,
AMD_PL_MBOX_IRQ_RX_THRESHOLD is edge-latched only when the RX FIFO transitions
from empty to nonempty. In amd_pl_mbox_startup():

	writel(AMD_PL_MBOX_CTRL_RX_RESET, mb->regs + AMD_PL_MBOX_CTRL);
	/* Interrupt when the RX FIFO changes from empty to nonempty. */
	writel(0, mb->regs + AMD_PL_MBOX_RECV_THRESHOLD);
	writel(AMD_PL_MBOX_IRQ_RX_THRESHOLD | AMD_PL_MBOX_IRQ_ERROR,
	       mb->regs + AMD_PL_MBOX_IRQ_STATUS);

If the remote peer writes a word after AMD_PL_MBOX_CTRL_RX_RESET empties the
FIFO, that 0-to-1 occupancy transition latches AMD_PL_MBOX_IRQ_RX_THRESHOLD.
The subsequent writel() to AMD_PL_MBOX_IRQ_STATUS then clears that latched
interrupt status bit while the received word remains unread in the RX FIFO.

Because the RX FIFO is now already nonempty and amd_pl_mbox_startup() does not
reset or drain the FIFO after clearing AMD_PL_MBOX_IRQ_STATUS, neither the
unread word nor any subsequent words written by the peer will cause an
empty-to-nonempty transition, so AMD_PL_MBOX_IRQ_RX_THRESHOLD will never
latch again.

Should AMD_PL_MBOX_RECV_THRESHOLD and AMD_PL_MBOX_IRQ_STATUS be programmed
before issuing AMD_PL_MBOX_CTRL_RX_RESET (or should the RX FIFO be checked
after enabling the interrupt)?

[ ... ]

> +static irqreturn_t amd_pl_mbox_irq_thread(int irq, void *data)
> +{
> +	struct amd_pl_mbox *mb = data;
> +	u32 pending_intr;
> +	u32 irq_enable;
> +	u32 irq_status;
> +
> +	irq_status = readl(mb->regs + AMD_PL_MBOX_IRQ_STATUS);
> +	irq_enable = readl(mb->regs + AMD_PL_MBOX_IRQ_ENABLE);
> +	pending_intr = irq_status & irq_enable;
> +
> +	if (!pending_intr) {
> +		dev_warn_ratelimited(mb->controller.dev,
> +				     "unexpected interrupt: status %#x enable %#x\n",
> +				     irq_status, irq_enable);
> +		return IRQ_NONE;
> +	}
> +
> +	/*
> +	 * Acknowledge before draining. The receive event latches when the RX
> +	 * FIFO goes from empty to nonempty, so a word that arrives after the
> +	 * drain has emptied the FIFO raises a new event.
> +	 */
> +	writel(pending_intr, mb->regs + AMD_PL_MBOX_IRQ_STATUS);
> +
> +	if (pending_intr & AMD_PL_MBOX_IRQ_ERROR)
> +		dev_warn_ratelimited(mb->controller.dev,
> +				     "mailbox FIFO error: %#x\n",
> +				     readl(mb->regs + AMD_PL_MBOX_ERROR));

[Severity: Medium]
In drivers/mailbox/amd-pl-mailbox.c, amd_pl_mbox_irq_thread() passes
readl(mb->regs + AMD_PL_MBOX_ERROR) directly as an argument to
dev_warn_ratelimited().

In include/linux/dev_printk.h, dev_warn_ratelimited() expands to
dev_level_ratelimited(), which only evaluates its variadic arguments when
__ratelimit(&_rs) returns true:

#define dev_level_ratelimited(dev_level, dev, fmt, ...)			\
do {									\
	static DEFINE_RATELIMIT_STATE(_rs,				\
				      DEFAULT_RATELIMIT_INTERVAL,	\
				      DEFAULT_RATELIMIT_BURST);		\
	if (__ratelimit(&_rs))						\
		dev_level(dev, fmt, ##__VA_ARGS__);			\
} while (0)

When AMD_PL_MBOX_IRQ_ERROR fires frequently enough to be rate-limited, will
skipping readl(mb->regs + AMD_PL_MBOX_ERROR) leave sticky error bits uncleared
in the hardware error register?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105051902.1646651-1-ben.levinsky@amd.com?part=4

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 6/6] remoteproc: amd_mbv_bram: Add RPMsg support
  2026-11-05  5:19 ` [PATCH v2 6/6] remoteproc: amd_mbv_bram: Add RPMsg support Ben Levinsky
@ 2026-11-05  5:39   ` sashiko-bot
  0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-11-05  5:39 UTC (permalink / raw)
  To: Ben Levinsky; +Cc: devicetree, robh, conor+dt

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] remoteproc: amd_mbv_bram: active mailboxes in amd_bram_rproc_stop() race
- [Medium] remoteproc: amd_mbv_bram: leak of carveouts on amd_bram_rproc_prepare() error
- [Medium] remoteproc: amd_mbv_bram: unmapped vdev0buffer added to coredump segments
--

commit 65a1b80207879c4cf9d647ab712871d29690bd39
Author: Ben Levinsky <ben.levinsky@amd.com>

remoteproc: amd_mbv_bram: Add RPMsg support

This commit adds RPMsg support to the AMD MicroBlaze/V BRAM remoteproc
driver by registering reserved-memory carveouts, exposing the loaded
resource table, and using the mailbox framework for virtqueue kicks and
notifications.

> diff --git a/drivers/remoteproc/amd_mbv_bram_rproc.c b/drivers/remoteproc/amd_mbv_bram_rproc.c
> index da621f62f7df5..9c2694fd645a6 100644
> --- a/drivers/remoteproc/amd_mbv_bram_rproc.c
> +++ b/drivers/remoteproc/amd_mbv_bram_rproc.c

[ ... ]

> +static void amd_bram_rproc_release_mboxes(void *data)
> +{
> +	struct amd_bram_rproc *priv = data;
> +
> +	/*
> +	 * Free the channels before cancelling the receive work. RX channel
> +	 * shutdown disables the controller interrupt synchronously, so no
> +	 * notification can queue the work again once it has been cancelled.
> +	 */
> +	amd_bram_rproc_free_mboxes(priv);
> +	cancel_work_sync(&priv->rx_work);
> +}
> +
> +static int amd_bram_rproc_add_reserved_mem(struct rproc *rproc)
> +{
> +	struct amd_bram_rproc *priv = rproc->priv;
> +	struct device_node *np = priv->dev->of_node;
> +	struct rproc_mem_entry *mem;
> +	int count;
> +	int ret;
> +	int i;
> +
> +	if (!of_property_present(np, "memory-region"))
> +		return 0;
> +
> +	count = of_reserved_mem_region_count(np);
> +	if (count < 0)
> +		return count;
> +
> +	for (i = 0; i < count; i++) {
> +		struct resource res;
> +		const char *name;
> +
> +		ret = of_property_read_string_index(np, "memory-region-names",
> +						    i, &name);
> +		if (ret)
> +			return dev_err_probe(priv->dev, ret,
> +					     "failed to read memory-region name %d\n",
> +					     i);
> +
> +		ret = of_reserved_mem_region_to_resource(np, i, &res);
> +		if (ret)
> +			return dev_err_probe(priv->dev, ret,
> +					     "failed to parse memory-region %d\n", i);
> +
> +		if (!strcmp(name, "vdev0buffer"))
> +			mem = rproc_of_resm_mem_entry_init(priv->dev, i,
> +							   resource_size(&res),
> +							   res.start, name);
> +		else
> +			mem = rproc_mem_entry_init(priv->dev, NULL,
> +						   (dma_addr_t)res.start,
> +						   resource_size(&res), res.start,
> +						   rproc_mem_entry_ioremap_wc,
> +						   rproc_mem_entry_iounmap,
> +						   "%s", name);
> +		if (!mem)
> +			return -ENOMEM;
> +
> +		rproc_add_carveout(rproc, mem);
> +		rproc_coredump_add_segment(rproc, res.start, resource_size(&res));

[Severity: Medium]
Should vdev0buffer be excluded from rproc_coredump_add_segment() in
amd_bram_rproc_add_reserved_mem()?

When name is "vdev0buffer", the entry is initialized with
rproc_of_resm_mem_entry_init(), which leaves mem->alloc and mem->va as
NULL. During boot, rproc_alloc_registered_carveouts() only allocates
entries with an alloc callback, and rproc_add_virtio_dev() attaches the
reserved memory region to the child virtio device without setting mem->va.

Because amd_bram_rproc_ops does not provide a custom da_to_va callback,
rproc_coredump() -> rproc_copy_segment() -> rproc_da_to_va() skips any
carveout with a NULL va, logs an invalid copy request error, and fills the
vdev0buffer coredump segment with 0xff bytes.

> +		dev_dbg(priv->dev, "reserved memory carveout %pR\n", &res);
> +	}
> +
> +	return 0;
> +}
> +
>  static int amd_bram_rproc_prepare(struct rproc *rproc)
>  {
>  	struct amd_bram_rproc *priv = rproc->priv;
> @@ -80,7 +292,7 @@ static int amd_bram_rproc_prepare(struct rproc *rproc)
>  	rproc_add_carveout(rproc, mem);
>  	rproc_coredump_add_segment(rproc, da, resource_size(&res));
>  
> -	return 0;
> +	return amd_bram_rproc_add_reserved_mem(rproc);
>  }

[Severity: Medium]
As reported by Sashiko on v1 [1], if amd_bram_rproc_add_reserved_mem()
fails in amd_bram_rproc_prepare(), does this leak the struct
rproc_mem_entry and struct rproc_dump_segment entries already added to
rproc->carveouts and rproc->dump_segments?

In amd_bram_rproc_prepare(), the executable BRAM carveout and coredump
segment are queued onto rproc->carveouts and rproc->dump_segments before
calling amd_bram_rproc_add_reserved_mem(), which also queues entries in a
loop over memory-region.

In drivers/remoteproc/remoteproc_core.c, when rproc_prepare_device() fails
in rproc_fw_boot(), execution jumps directly to disable_iommu and bypasses
clean_up_resources and rproc_resource_cleanup():

drivers/remoteproc/remoteproc_core.c:rproc_fw_boot() {
    ...
	ret = rproc_prepare_device(rproc);
	if (ret) {
		dev_err(dev, "can't prepare rproc %s: %d\n", rproc->name, ret);
		goto disable_iommu;
	}
    ...
}

If amd_bram_rproc_add_reserved_mem() fails at of_reserved_mem_region_count(),
of_property_read_string_index(), of_reserved_mem_region_to_resource(), or
carveout allocation, those queued entries remain on rproc->carveouts and
rproc->dump_segments, causing duplicate entries on subsequent rproc_boot()
calls and leaking them if the device is unbound while offline.

>  static int amd_bram_rproc_start(struct rproc *rproc)
> @@ -133,6 +345,9 @@ static int amd_bram_rproc_stop(struct rproc *rproc)
>  		dev_err(priv->dev, "failed to assert reset: %d\n", ret);
>  		return ret;
>  	}
> +	cancel_work_sync(&priv->rx_work);
> +	bitmap_zero(priv->pending_notify_ids,
> +		    ARRAY_SIZE(priv->notify_msgs));

[Severity: High]
As reported by Sashiko on v1 [2], since priv->rx_chan and priv->tx_chan
stay active across amd_bram_rproc_stop(), can amd_bram_rproc_rx_callback()
still fire during or after amd_bram_rproc_stop() and re-queue priv->rx_work?

As the comment in amd_bram_rproc_release_mboxes() notes, shutting down the
mailbox channels disables the controller interrupt synchronously and resets
the hardware FIFOs. Because amd_bram_rproc_stop() keeps the channels open,
any notification words still in the hardware RX FIFO or threaded IRQ handler
(amd_pl_mbox_irq_thread() -> amd_pl_mbox_rx() -> mbox_chan_received_data()
-> amd_bram_rproc_rx_callback()) can race with the non-atomic bitmap_zero()
and call schedule_work(&priv->rx_work) after cancel_work_sync() returns.

That allows amd_bram_rproc_rx_work() -> rproc_vq_interrupt() to run
concurrently with __rproc_shutdown() -> rproc_resource_cleanup() ->
rproc_free_vring() as the virtqueues and notify IDs are torn down.

Keeping priv->tx_chan active across stop also leaves any unread kick word in
the hardware TX FIFO while the remote processor is held in reset with its
clock disabled, causing the mailbox core hrtimer to poll last_tx_done every
1 ms while offline and deliver a stale kick on the next start, and loses PL
Mailbox register state across partial reconfiguration (DFX).

[1] https://lore.kernel.org/r/20261014213426.2957480-7-ben.levinsky@amd.com
[2] https://lore.kernel.org/r/20261014213426.2957480-7-ben.levinsky@amd.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105051902.1646651-1-ben.levinsky@amd.com?part=6

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2026-11-05  5:39 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-05  5:18 [PATCH v2 0/6] Add AMD PL Mailbox and RPMsg support for the AMD BRAM remoteproc Ben Levinsky
2026-11-05  5:18 ` [PATCH v2 1/6] remoteproc: amd_mbv_bram: Bound device name formatting Ben Levinsky
2026-11-05  5:18 ` [PATCH v2 2/6] remoteproc: amd_mbv_bram: Restore reset GPIO direction after PL reconfig Ben Levinsky
2026-11-05  5:29   ` sashiko-bot
2026-11-05  5:18 ` [PATCH v2 3/6] dt-bindings: mailbox: Add AMD PL Mailbox IP Ben Levinsky
2026-11-05  5:19 ` [PATCH v2 4/6] mailbox: Add AMD PL Mailbox soft IP controller Ben Levinsky
2026-11-05  5:31   ` sashiko-bot
2026-11-05  5:19 ` [PATCH v2 5/6] dt-bindings: remoteproc: Add RPMsg resources to AMD BRAM rproc Ben Levinsky
2026-11-05  5:19 ` [PATCH v2 6/6] remoteproc: amd_mbv_bram: Add RPMsg support Ben Levinsky
2026-11-05  5:39   ` sashiko-bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox