devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Add Microchip IPC remoteproc support
@ 2025-11-21 14:21 Valentina Fernandez
  2025-11-21 14:21 ` [PATCH v2 1/2] dt-bindings: remoteproc: add Microchip IPC remoteproc Valentina Fernandez
  2025-11-21 14:21 ` [PATCH v2 2/2] remoteproc: add support for Microchip IPC remoteproc platform driver Valentina Fernandez
  0 siblings, 2 replies; 11+ messages in thread
From: Valentina Fernandez @ 2025-11-21 14:21 UTC (permalink / raw)
  To: andersson, mathieu.poirier, robh, krzk+dt, conor+dt,
	valentina.fernandezalanis
  Cc: linux-remoteproc, devicetree, linux-kernel

Hello all,

This series adds support for the Microchip Inter-Processor Communication
(IPC) remoteproc platform driver.

Microchip's family of RISC-V SoCs typically has one or more clusters
that can be configured to run in Asymmetric Multi-Processing (AMP) mode.

The IPC remoteproc platform driver allows for starting and stopping
firmware on the remote cluster(s) and facilitates RPMsg communication.
The remoteproc attach/detach operations are also supported for use cases
where the firmware is loaded by the Hart Software Services
(zero-stage bootloader) before Linux boots.

Error Recovery and Power Management features are not currently
supported in the remoteproc platform driver.

Changes in v2:
 - simplify memory region handling with memory-region-names
 - rename compatible to "microchip,ipc-sbi-remoteproc"
 - rephrase dt binding commit  subject, message and description property
 - drop microchip,auto-boot and microchip,skip-ready-wait properties
 - fix memory-region constraints and add memory-region-names
 - fix binding example and add examples for all use cases
 - Link to v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20240912170025.455167-6-valentina.fernandezalanis@microchip.com/

Thanks,
Valentina

Valentina Fernandez (2):
  dt-bindings: remoteproc: add Microchip IPC remoteproc
  remoteproc: add support for Microchip IPC remoteproc platform driver

 .../microchip,ipc-sbi-remoteproc.yaml         |  95 ++++
 drivers/remoteproc/Kconfig                    |  12 +
 drivers/remoteproc/Makefile                   |   1 +
 drivers/remoteproc/mchp_ipc_sbi_remoteproc.c  | 465 ++++++++++++++++++
 4 files changed, 573 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
 create mode 100644 drivers/remoteproc/mchp_ipc_sbi_remoteproc.c

-- 
2.34.1


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

* [PATCH v2 1/2] dt-bindings: remoteproc: add Microchip IPC remoteproc
  2025-11-21 14:21 [PATCH v2 0/2] Add Microchip IPC remoteproc support Valentina Fernandez
@ 2025-11-21 14:21 ` Valentina Fernandez
  2025-11-21 18:28   ` Conor Dooley
                     ` (2 more replies)
  2025-11-21 14:21 ` [PATCH v2 2/2] remoteproc: add support for Microchip IPC remoteproc platform driver Valentina Fernandez
  1 sibling, 3 replies; 11+ messages in thread
From: Valentina Fernandez @ 2025-11-21 14:21 UTC (permalink / raw)
  To: andersson, mathieu.poirier, robh, krzk+dt, conor+dt,
	valentina.fernandezalanis
  Cc: linux-remoteproc, devicetree, linux-kernel

Microchip family of RISC-V SoCs typically have one or more application
clusters. These clusters can be configured to run in an Asymmetric
Multi Processing (AMP) mode.

Add a dt-binding for these application clusters.

Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
---
 .../microchip,ipc-sbi-remoteproc.yaml         | 95 +++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml

diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
new file mode 100644
index 000000000000..348902f9a202
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-sbi-remoteproc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip IPC Remote Processor
+
+description:
+  Microchip family of RISC-V SoCs typically have one or more
+  clusters. These clusters can be configured to run in an Asymmetric
+  Multi Processing (AMP) mode where clusters are split in independent
+  software contexts.
+
+maintainers:
+  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
+
+properties:
+  compatible:
+    const: microchip,ipc-sbi-remoteproc
+
+  mboxes:
+    description:
+      Microchip IPC mailbox specifier. To be used for communication with
+      a remote cluster. The specifier format is as per the bindings,
+      Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
+    maxItems: 1
+
+  memory-region:
+    minItems: 1
+    maxItems: 5
+    description:
+      List of phandles to the reserved memory regions associated wih the remoteproc
+      device. This is variable and describes the memories shared with the remote cluster
+      (e.g. firmware, resource table, rpmsg vrings, etc.)
+    items:
+      anyOf:
+        - description: region used for the resource table when firmware is started by the bootloader
+        - description: region used for the remote cluster firmware image section
+        - description: virtio device (vdev) buffer
+        - description: virtqueue for sending messages to the remote cluster (vring0)
+        - description: virtqueue for receiving messages from the remote cluster (vring1)
+
+  memory-region-names:
+    minItems: 1
+    maxItems: 5
+    items:
+      anyOf:
+        - const: rsc-table
+        - const: firmware
+        - const: buffer
+        - const: vring0
+        - const: vring1
+
+required:
+  - compatible
+  - mboxes
+  - memory-region
+  - memory-region-names
+
+additionalProperties: false
+
+examples:
+  - |
+    // Early boot mode example - firmware started by bootloader
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        remoteproc {
+            compatible = "microchip,ipc-sbi-remoteproc";
+            mboxes= <&ihc 8>;
+            memory-region = <&rsctable>, <&vdev0buffer>,
+                            <&vdev0vring0>, <&vdev0vring1>;
+            memory-region-names = "rsc-table", "buffer",
+                                  "vring0", "vring1";
+        };
+    };
+
+  - |
+    // Late boot mode example - firmware started by Linux (remoteproc)
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        remoteproc {
+            compatible = "microchip,ipc-sbi-remoteproc";
+            mboxes= <&ihc 8>;
+            memory-region = <&cluster_firmware>, <&vdev0buffer>,
+                            <&vdev0vring0>, <&vdev0vring1>;
+            memory-region-names = "firmware", "buffer",
+                                  "vring0", "vring1";
+        };
+    };
+...
-- 
2.34.1


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

* [PATCH v2 2/2] remoteproc: add support for Microchip IPC remoteproc platform driver
  2025-11-21 14:21 [PATCH v2 0/2] Add Microchip IPC remoteproc support Valentina Fernandez
  2025-11-21 14:21 ` [PATCH v2 1/2] dt-bindings: remoteproc: add Microchip IPC remoteproc Valentina Fernandez
@ 2025-11-21 14:21 ` Valentina Fernandez
  2025-12-09 17:33   ` Mathieu Poirier
  1 sibling, 1 reply; 11+ messages in thread
From: Valentina Fernandez @ 2025-11-21 14:21 UTC (permalink / raw)
  To: andersson, mathieu.poirier, robh, krzk+dt, conor+dt,
	valentina.fernandezalanis
  Cc: linux-remoteproc, devicetree, linux-kernel

Microchip family of RISC-V SoCs typically have one or more
clusters. These clusters can be configured to run in an Asymmetric
Multi Processing (AMP) mode where clusters are split in independent
software contexts.

Add a remoteproc platform driver to be able to load and boot firmware
to remote processors.

The driver uses SBI (RISC-V Supervisor Binary Interface) ecalls to
request software running in machine privilege mode (M-mode) to
start/stop the remote processor.

Inter-processor communication is supported through the
virtio rpmsg stack using shared memory and the Microchip IPC mailbox
driver.

Currently, the driver the following features are supported:
- Start/stop a remote software context
- Kick function implementation for RPMsg Communication
- Attach to a remote context loaded by another entity (bootloader)

Error Recovery and Power Management features are not currently supported.

Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
---
 drivers/remoteproc/Kconfig                   |  12 +
 drivers/remoteproc/Makefile                  |   1 +
 drivers/remoteproc/mchp_ipc_sbi_remoteproc.c | 465 +++++++++++++++++++
 3 files changed, 478 insertions(+)
 create mode 100644 drivers/remoteproc/mchp_ipc_sbi_remoteproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 48a0d3a69ed0..68bd68f553ea 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -54,6 +54,18 @@ config INGENIC_VPU_RPROC
 	  This can be either built-in or a loadable module.
 	  If unsure say N.
 
+config MCHP_IPC_SBI_REMOTEPROC
+	tristate "Microchip IPC remoteproc support"
+	depends on MCHP_SBI_IPC_MBOX || COMPILE_TEST
+	depends on ARCH_MICROCHIP
+	help
+	  Say y here to support booting and loading firmware to remote
+	  processors on various Microchip family of RISC-V SoCs via the
+	  remote processor framework.
+	  This can be either built-in or a loadable module.
+	  If compiled as module, the module will be called mchp_ipc_remoteproc.
+	  If unsure say N.
+
 config MTK_SCP
 	tristate "Mediatek SCP support"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 1c7598b8475d..afb28dff5666 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_REMOTEPROC_CDEV)		+= remoteproc_cdev.o
 obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
 obj-$(CONFIG_IMX_DSP_REMOTEPROC)	+= imx_dsp_rproc.o
 obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
+obj-$(CONFIG_MCHP_IPC_SBI_REMOTEPROC)	+= mchp_ipc_sbi_remoteproc.o
 obj-$(CONFIG_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
 obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
diff --git a/drivers/remoteproc/mchp_ipc_sbi_remoteproc.c b/drivers/remoteproc/mchp_ipc_sbi_remoteproc.c
new file mode 100644
index 000000000000..55d182c1eee7
--- /dev/null
+++ b/drivers/remoteproc/mchp_ipc_sbi_remoteproc.c
@@ -0,0 +1,465 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Microchip IPC Remoteproc driver
+ *
+ * Copyright (c) 2021 - 2025 Microchip Technology Inc. All rights reserved.
+ *
+ * Author: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
+ *
+ * Derived from the imx_rproc implementation:
+ * Copyright (c) 2017 Pengutronix, Oleksij Rempel <kernel@pengutronix.de>
+ */
+
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc.h>
+#include <linux/workqueue.h>
+
+#include <asm/sbi.h>
+#include <asm/vendorid_list.h>
+#include <linux/mailbox/mchp-ipc.h>
+
+#include "remoteproc_internal.h"
+
+#define SBI_EXT_MICROCHIP_TECHNOLOGY	(SBI_EXT_VENDOR_START | \
+					 MICROCHIP_VENDOR_ID)
+
+#define MIV_RPROC_MEM_MAX		2
+
+enum {
+	SBI_EXT_RPROC_STATE = 0x3,
+	SBI_EXT_RPROC_START,
+	SBI_EXT_RPROC_STOP,
+};
+
+/**
+ * enum mchp_ipc_rproc_mbox_messages - predefined mailbox messages
+ *
+ * @MCHP_IPC_RPROC_MBOX_READY: a ready message response from a remote context indicating
+ * that the remote context is up and running.
+ *
+ * @MCHP_IPC_RPROC_MBOX_PENDING_MSG: Not currently in use, but reserved for future use
+ * to inform the receiver that there is a message awaiting in its receive-side
+ * vring. At the moment, one can explicitly send the index of the triggered
+ * virtqueue as a payload.
+ *
+ * @MCHP_IPC_RPROC_MBOX_STOP: a stop request for the remote context
+ *
+ * @MCHP_IPC_RPROC_MBOX_END_MSG: Indicates end of known/defined messages.
+ * This should be the last definition.
+ *
+ */
+enum mchp_ipc_rproc_mbox_messages {
+	MCHP_IPC_RPROC_MBOX_READY = 0xFFFFFF00,
+	MCHP_IPC_RPROC_MBOX_PENDING_MSG = 0xFFFFFF01,
+	MCHP_IPC_RPROC_MBOX_STOP = 0xFFFFFF02,
+	MCHP_IPC_RPROC_MBOX_END_MSG = 0xFFFFFF03,
+};
+
+enum {
+	MCHP_IPC_RPROC_RSC_TABLE_REGION,
+	MCHP_IPC_RPROC_FW_REGION,
+	MCHP_IPC_RPROC_BUFF_REGION,
+	MCHP_IPC_RPROC_VRING0_REGION,
+	MCHP_IPC_RPROC_VRING1_REGION,
+	MCHP_IPC_RPROC_REGION_MAX,
+};
+
+struct mchp_ipc_rproc_mem_region {
+	const char *name;
+	const char *prefix;
+};
+
+struct mchp_ipc_rproc {
+	struct device *dev;
+	struct rproc *rproc;
+	struct mbox_chan *mbox_channel;
+	struct workqueue_struct *workqueue;
+	struct mbox_client mbox_client;
+	struct completion start_done;
+	struct work_struct rproc_work;
+	struct mchp_ipc_msg message;
+	void __iomem *rsc_table;
+	bool initialized;
+	u32 chan_id;
+};
+
+static struct mchp_ipc_rproc_mem_region mchp_rproc_mem_regions[] = {
+	[MCHP_IPC_RPROC_RSC_TABLE_REGION] = {
+		.name = "rsc-table",
+		.prefix = NULL,
+	},
+	[MCHP_IPC_RPROC_FW_REGION] = {
+		.name = "firmware",
+		.prefix = NULL,
+	},
+	[MCHP_IPC_RPROC_BUFF_REGION] = {
+		.name = "buffer",
+		.prefix = "vdev0",
+	},
+	[MCHP_IPC_RPROC_VRING0_REGION] = {
+		.name = "vring0",
+		.prefix = "vdev0",
+	},
+	[MCHP_IPC_RPROC_VRING1_REGION] = {
+		.name = "vring1",
+		.prefix = "vdev0",
+	},
+};
+
+static int mchp_ipc_rproc_start(struct rproc *rproc)
+{
+	struct mchp_ipc_rproc *priv = rproc->priv;
+	struct sbiret ret;
+
+	ret = sbi_ecall(SBI_EXT_MICROCHIP_TECHNOLOGY, SBI_EXT_RPROC_START,
+			priv->chan_id, rproc->bootaddr, 0, 0, 0, 0);
+
+	if (ret.error)
+		return sbi_err_map_linux_errno(ret.error);
+
+	if (!wait_for_completion_timeout(&priv->start_done,
+					 msecs_to_jiffies(5000))) {
+		dev_err(priv->dev, "timeout waiting for ready notification\n");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static int mchp_ipc_rproc_stop(struct rproc *rproc)
+{
+	struct mchp_ipc_rproc *priv = rproc->priv;
+	struct sbiret ret;
+
+	ret = sbi_ecall(SBI_EXT_MICROCHIP_TECHNOLOGY, SBI_EXT_RPROC_STOP,
+			priv->chan_id, MCHP_IPC_RPROC_MBOX_STOP, 0, 0, 0, 0);
+
+	if (ret.error)
+		return sbi_err_map_linux_errno(ret.error);
+
+	return ret.value;
+}
+
+static int mchp_ipc_rproc_mem_alloc(struct rproc *rproc, struct rproc_mem_entry *mem)
+{
+	struct device *dev = rproc->dev.parent;
+	void *va;
+
+	dev_dbg(dev, "map memory: %pad+%zx\n", &mem->dma, mem->len);
+	va = ioremap_wc(mem->dma, mem->len);
+	if (IS_ERR_OR_NULL(va)) {
+		dev_err(dev, "Unable to map memory region: %p+%zx\n",
+			&mem->dma, mem->len);
+		return -ENOMEM;
+	}
+
+	mem->va = va;
+
+	return 0;
+}
+
+static int mchp_ipc_rproc_mem_release(struct rproc *rproc,
+				      struct rproc_mem_entry *mem)
+{
+	dev_dbg(rproc->dev.parent, "unmap memory: %pad\n", &mem->dma);
+	iounmap(mem->va);
+
+	return 0;
+}
+
+static int mchp_ipc_rproc_prepare(struct rproc *rproc)
+{
+	struct mchp_ipc_rproc *priv = rproc->priv;
+	struct device_node *np = priv->dev->of_node;
+	struct rproc_mem_entry *mem;
+	struct resource res;
+	int i, ret;
+
+	reinit_completion(&priv->start_done);
+
+	for (i = 0; i < ARRAY_SIZE(mchp_rproc_mem_regions); i++) {
+		char *mem_region_name = devm_kstrdup(priv->dev,
+						     mchp_rproc_mem_regions[i].name, GFP_KERNEL);
+		int index = of_property_match_string(np, "memory-region-names", mem_region_name);
+
+		if (index < 0)
+			continue;
+
+		ret = of_reserved_mem_region_to_resource_byname(np, mem_region_name, &res);
+		if (ret)
+			return ret;
+
+		if (mchp_rproc_mem_regions[i].prefix) {
+			mem_region_name = devm_kasprintf(priv->dev, GFP_KERNEL, "%s%s",
+							 mchp_rproc_mem_regions[i].prefix,
+							 mem_region_name);
+		}
+
+		if (i == MCHP_IPC_RPROC_BUFF_REGION) {
+			mem = rproc_of_resm_mem_entry_init(priv->dev, index, resource_size(&res),
+							   res.start, mem_region_name);
+		} else {
+			mem = rproc_mem_entry_init(priv->dev, NULL,
+						   (dma_addr_t)res.start,
+						   resource_size(&res), res.start,
+						   mchp_ipc_rproc_mem_alloc,
+						   mchp_ipc_rproc_mem_release,
+						   mem_region_name);
+		}
+		rproc_add_carveout(rproc, mem);
+	}
+
+	return 0;
+}
+
+static int mchp_ipc_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
+{
+	int ret;
+
+	ret = rproc_elf_load_rsc_table(rproc, fw);
+	if (ret)
+		dev_info(&rproc->dev, "No resource table in elf\n");
+
+	return 0;
+}
+
+static void mchp_ipc_rproc_kick(struct rproc *rproc, int vqid)
+{
+	struct mchp_ipc_rproc *priv = (struct mchp_ipc_rproc *)rproc->priv;
+	struct mchp_ipc_msg msg;
+	int ret;
+
+	msg.buf = (void *)&vqid;
+	msg.size = sizeof(vqid);
+
+	ret = mbox_send_message(priv->mbox_channel, (void *)&msg);
+	if (ret < 0)
+		dev_err(priv->dev,
+			"failed to send mbox message, status = %d\n", ret);
+}
+
+static int mchp_ipc_rproc_attach(struct rproc *rproc)
+{
+	return 0;
+}
+
+static struct resource_table
+*mchp_ipc_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
+{
+	struct mchp_ipc_rproc *priv = rproc->priv;
+
+	if (!priv->rsc_table)
+		return NULL;
+
+	*table_sz = SZ_1K;
+	return (struct resource_table *)priv->rsc_table;
+}
+
+static const struct rproc_ops mchp_ipc_rproc_ops = {
+	.prepare = mchp_ipc_rproc_prepare,
+	.start = mchp_ipc_rproc_start,
+	.get_loaded_rsc_table = mchp_ipc_rproc_get_loaded_rsc_table,
+	.attach = mchp_ipc_rproc_attach,
+	.stop = mchp_ipc_rproc_stop,
+	.kick = mchp_ipc_rproc_kick,
+	.load = rproc_elf_load_segments,
+	.parse_fw = mchp_ipc_rproc_parse_fw,
+	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
+	.sanity_check = rproc_elf_sanity_check,
+	.get_boot_addr = rproc_elf_get_boot_addr,
+};
+
+static void mchp_ipc_rproc_vq_work(struct work_struct *work)
+{
+	struct mchp_ipc_rproc *priv = container_of(work, struct mchp_ipc_rproc, rproc_work);
+	struct device *dev = priv->rproc->dev.parent;
+
+	u32 msg = priv->message.buf[0];
+
+	/*
+	 * Currently, we are expected to receive the following messages
+	 * from the remote cluster: a ready message or receive the index
+	 * of the triggered virtqueue as a payload.
+	 * We can silently ignore any other type of mailbox messages since
+	 * they are not meant for us and are meant to be received by the
+	 * remote cluster only.
+	 */
+	switch (msg) {
+	case MCHP_IPC_RPROC_MBOX_READY:
+		complete(&priv->start_done);
+		break;
+	default:
+		if (msg >= MCHP_IPC_RPROC_MBOX_READY && msg < MCHP_IPC_RPROC_MBOX_END_MSG)
+			return;
+		if (msg > priv->rproc->max_notifyid) {
+			dev_info(dev, "dropping unknown message 0x%x", msg);
+			return;
+		}
+		/* msg contains the index of the triggered vring */
+		if (rproc_vq_interrupt(priv->rproc, msg) == IRQ_NONE)
+			dev_dbg(dev, "no message was found in vqid %d\n", msg);
+	}
+}
+
+static void mchp_ipc_rproc_rx_callback(struct mbox_client *mbox_client, void *msg)
+{
+	struct rproc *rproc = dev_get_drvdata(mbox_client->dev);
+	struct mchp_ipc_rproc *priv = rproc->priv;
+
+	priv->message = *(struct mchp_ipc_msg *)msg;
+	queue_work(priv->workqueue, &priv->rproc_work);
+}
+
+static int mchp_ipc_rproc_mbox_init(struct rproc *rproc)
+{
+	struct mchp_ipc_rproc *priv = rproc->priv;
+	struct device *dev = priv->dev;
+	struct mchp_ipc_sbi_chan *chan_priv;
+	struct mbox_client *mbox_client;
+
+	mbox_client = &priv->mbox_client;
+	mbox_client->dev = dev;
+	mbox_client->tx_block = true;
+	mbox_client->tx_tout = 100;
+	mbox_client->knows_txdone = false;
+	mbox_client->rx_callback = mchp_ipc_rproc_rx_callback;
+
+	priv->mbox_channel = mbox_request_channel(mbox_client, 0);
+	if (IS_ERR(priv->mbox_channel))
+		return dev_err_probe(mbox_client->dev,
+				     PTR_ERR(priv->mbox_channel),
+				     "failed to request mailbox channel\n");
+
+	chan_priv = (struct mchp_ipc_sbi_chan *)priv->mbox_channel->con_priv;
+	priv->chan_id = chan_priv->id;
+
+	return 0;
+}
+
+static int mchp_ipc_rproc_get_state(u32 chan)
+{
+	struct sbiret ret;
+
+	ret = sbi_ecall(SBI_EXT_MICROCHIP_TECHNOLOGY, SBI_EXT_RPROC_STATE,
+			chan, 0, 0, 0, 0, 0);
+
+	if (ret.error)
+		return sbi_err_map_linux_errno(ret.error);
+
+	return ret.value;
+}
+
+static int mchp_ipc_rproc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct mchp_ipc_rproc *priv;
+	struct rproc *rproc;
+	struct resource res;
+	int num_rmems, ret;
+
+	rproc = devm_rproc_alloc(dev, np->name, &mchp_ipc_rproc_ops,
+				 NULL, sizeof(*priv));
+	if (!rproc)
+		return -ENOMEM;
+
+	priv = rproc->priv;
+	priv->rproc = rproc;
+	priv->dev = dev;
+
+	num_rmems = of_property_count_elems_of_size(np, "memory-region",
+						    sizeof(phandle));
+	if (num_rmems < 1)
+		return dev_err_probe(dev, -EINVAL,
+				     "device needs at least one memory regions to be defined\n");
+
+	ret = of_reserved_mem_region_to_resource_byname(np, "rsc-table", &res);
+	if (!ret) {
+		priv->rsc_table  = devm_ioremap_resource(dev, &res);
+		if (IS_ERR(priv->rsc_table)) {
+			return dev_err_probe(dev, PTR_ERR(priv->rsc_table),
+					     "failed to map resource table\n");
+		}
+	}
+
+	priv->workqueue = create_workqueue(dev_name(dev));
+	if (!priv->workqueue)
+		return dev_err_probe(dev, -ENOMEM, "cannot create workqueue\n");
+
+	INIT_WORK(&priv->rproc_work, mchp_ipc_rproc_vq_work);
+	init_completion(&priv->start_done);
+
+	ret = mchp_ipc_rproc_mbox_init(rproc);
+	if (ret)
+		goto err_put_wkq;
+
+	/*
+	 * Check if the remote cluster has been booted by another entity
+	 * (i.e. bootloader) and set rproc state accordingly
+	 */
+	rproc->state = mchp_ipc_rproc_get_state(priv->chan_id);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "Couldn't get cluster boot mode\n");
+		goto err_put_mbox;
+	}
+
+	if (rproc->state != RPROC_DETACHED)
+		rproc->auto_boot = false;
+
+	/* error recovery is not supported at present */
+	rproc->recovery_disabled = true;
+
+	dev_set_drvdata(dev, rproc);
+
+	ret = devm_rproc_add(dev, rproc);
+	if (ret) {
+		dev_err_probe(dev, ret, "rproc_add failed\n");
+		goto err_put_mbox;
+	}
+
+	return 0;
+
+err_put_mbox:
+	mbox_free_channel(priv->mbox_channel);
+err_put_wkq:
+	destroy_workqueue(priv->workqueue);
+
+	return ret;
+}
+
+static void mchp_ipc_rproc_remove(struct platform_device *pdev)
+{
+	struct rproc *rproc = platform_get_drvdata(pdev);
+	struct mchp_ipc_rproc *priv = rproc->priv;
+
+	mbox_free_channel(priv->mbox_channel);
+	destroy_workqueue(priv->workqueue);
+}
+
+static const struct of_device_id mchp_ipc_rproc_of_match[] __maybe_unused = {
+	{ .compatible = "microchip,ipc-sbi-remoteproc", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mchp_ipc_rproc_of_match);
+
+static struct platform_driver mchp_ipc_rproc_driver = {
+	.probe = mchp_ipc_rproc_probe,
+	.remove = mchp_ipc_rproc_remove,
+	.driver = {
+		.name = "microchip-ipc-rproc",
+		.of_match_table = of_match_ptr(mchp_ipc_rproc_of_match),
+	},
+};
+
+module_platform_driver(mchp_ipc_rproc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Valentina Fernandez <valentina.fernandezalanis@microchip.com>");
+MODULE_DESCRIPTION("Microchip IPC Remote Processor control driver");
-- 
2.34.1


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

* Re: [PATCH v2 1/2] dt-bindings: remoteproc: add Microchip IPC remoteproc
  2025-11-21 14:21 ` [PATCH v2 1/2] dt-bindings: remoteproc: add Microchip IPC remoteproc Valentina Fernandez
@ 2025-11-21 18:28   ` Conor Dooley
  2025-12-01 16:16     ` Valentina.FernandezAlanis
  2025-11-25  9:46   ` Krzysztof Kozlowski
  2025-12-13  5:42   ` Tanmay Shah
  2 siblings, 1 reply; 11+ messages in thread
From: Conor Dooley @ 2025-11-21 18:28 UTC (permalink / raw)
  To: Valentina Fernandez
  Cc: andersson, mathieu.poirier, robh, krzk+dt, conor+dt,
	linux-remoteproc, devicetree, linux-kernel

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

On Fri, Nov 21, 2025 at 02:21:56PM +0000, Valentina Fernandez wrote:
> Microchip family of RISC-V SoCs typically have one or more application
> clusters. These clusters can be configured to run in an Asymmetric
> Multi Processing (AMP) mode.
> 
> Add a dt-binding for these application clusters.
> 
> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> ---
>  .../microchip,ipc-sbi-remoteproc.yaml         | 95 +++++++++++++++++++
>  1 file changed, 95 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
> new file mode 100644
> index 000000000000..348902f9a202
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-sbi-remoteproc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip IPC Remote Processor
> +
> +description:
> +  Microchip family of RISC-V SoCs typically have one or more
> +  clusters. These clusters can be configured to run in an Asymmetric
> +  Multi Processing (AMP) mode where clusters are split in independent
> +  software contexts.
> +
> +maintainers:
> +  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> +
> +properties:
> +  compatible:
> +    const: microchip,ipc-sbi-remoteproc
> +
> +  mboxes:
> +    description:
> +      Microchip IPC mailbox specifier. To be used for communication with
> +      a remote cluster. The specifier format is as per the bindings,
> +      Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> +    maxItems: 1
> +
> +  memory-region:
> +    minItems: 1
> +    maxItems: 5
> +    description:
> +      List of phandles to the reserved memory regions associated wih the remoteproc
> +      device. This is variable and describes the memories shared with the remote cluster
> +      (e.g. firmware, resource table, rpmsg vrings, etc.)
> +    items:
> +      anyOf:

Is this genuinely any of these, with no restrictions?
Can you have rsc-table and firmware?

> +        - description: region used for the resource table when firmware is started by the bootloader
> +        - description: region used for the remote cluster firmware image section
> +        - description: virtio device (vdev) buffer
> +        - description: virtqueue for sending messages to the remote cluster (vring0)
> +        - description: virtqueue for receiving messages from the remote cluster (vring1)
> +
> +  memory-region-names:
> +    minItems: 1
> +    maxItems: 5
> +    items:
> +      anyOf:
> +        - const: rsc-table
> +        - const: firmware
> +        - const: buffer
> +        - const: vring0
> +        - const: vring1
> +
> +required:
> +  - compatible
> +  - mboxes
> +  - memory-region
> +  - memory-region-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    // Early boot mode example - firmware started by bootloader
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        remoteproc {
> +            compatible = "microchip,ipc-sbi-remoteproc";
> +            mboxes= <&ihc 8>;
> +            memory-region = <&rsctable>, <&vdev0buffer>,
> +                            <&vdev0vring0>, <&vdev0vring1>;
> +            memory-region-names = "rsc-table", "buffer",
> +                                  "vring0", "vring1";
> +        };
> +    };
> +
> +  - |
> +    // Late boot mode example - firmware started by Linux (remoteproc)
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        remoteproc {
> +            compatible = "microchip,ipc-sbi-remoteproc";
> +            mboxes= <&ihc 8>;
> +            memory-region = <&cluster_firmware>, <&vdev0buffer>,
> +                            <&vdev0vring0>, <&vdev0vring1>;
> +            memory-region-names = "firmware", "buffer",
> +                                  "vring0", "vring1";
> +        };
> +    };
> +...
> -- 
> 2.34.1
> 

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

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

* Re: [PATCH v2 1/2] dt-bindings: remoteproc: add Microchip IPC remoteproc
  2025-11-21 14:21 ` [PATCH v2 1/2] dt-bindings: remoteproc: add Microchip IPC remoteproc Valentina Fernandez
  2025-11-21 18:28   ` Conor Dooley
@ 2025-11-25  9:46   ` Krzysztof Kozlowski
  2025-12-01 16:04     ` Valentina.FernandezAlanis
  2025-12-13  5:42   ` Tanmay Shah
  2 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2025-11-25  9:46 UTC (permalink / raw)
  To: Valentina Fernandez
  Cc: andersson, mathieu.poirier, robh, krzk+dt, conor+dt,
	linux-remoteproc, devicetree, linux-kernel

On Fri, Nov 21, 2025 at 02:21:56PM +0000, Valentina Fernandez wrote:
> Microchip family of RISC-V SoCs typically have one or more application
> clusters. These clusters can be configured to run in an Asymmetric
> Multi Processing (AMP) mode.
> 
> Add a dt-binding for these application clusters.
> 
> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> ---
>  .../microchip,ipc-sbi-remoteproc.yaml         | 95 +++++++++++++++++++
>  1 file changed, 95 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
> new file mode 100644
> index 000000000000..348902f9a202
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-sbi-remoteproc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip IPC Remote Processor
> +
> +description:
> +  Microchip family of RISC-V SoCs typically have one or more
> +  clusters. These clusters can be configured to run in an Asymmetric
> +  Multi Processing (AMP) mode where clusters are split in independent
> +  software contexts.
> +
> +maintainers:
> +  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> +
> +properties:
> +  compatible:
> +    const: microchip,ipc-sbi-remoteproc

This should be SoC specific compatible.

Best regards,
Krzysztof


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

* Re: [PATCH v2 1/2] dt-bindings: remoteproc: add Microchip IPC remoteproc
  2025-11-25  9:46   ` Krzysztof Kozlowski
@ 2025-12-01 16:04     ` Valentina.FernandezAlanis
  2025-12-01 16:22       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 11+ messages in thread
From: Valentina.FernandezAlanis @ 2025-12-01 16:04 UTC (permalink / raw)
  To: krzk
  Cc: andersson, mathieu.poirier, robh, krzk+dt, conor+dt,
	linux-remoteproc, devicetree, linux-kernel,
	Valentina.FernandezAlanis

On 25/11/2025 09:46, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Fri, Nov 21, 2025 at 02:21:56PM +0000, Valentina Fernandez wrote:
>> Microchip family of RISC-V SoCs typically have one or more application
>> clusters. These clusters can be configured to run in an Asymmetric
>> Multi Processing (AMP) mode.
>>
>> Add a dt-binding for these application clusters.
>>
>> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
>> ---
>>   .../microchip,ipc-sbi-remoteproc.yaml         | 95 +++++++++++++++++++
>>   1 file changed, 95 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
>> new file mode 100644
>> index 000000000000..348902f9a202
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
>> @@ -0,0 +1,95 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-sbi-remoteproc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Microchip IPC Remote Processor
>> +
>> +description:
>> +  Microchip family of RISC-V SoCs typically have one or more
>> +  clusters. These clusters can be configured to run in an Asymmetric
>> +  Multi Processing (AMP) mode where clusters are split in independent
>> +  software contexts.
>> +
>> +maintainers:
>> +  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: microchip,ipc-sbi-remoteproc
> This should be SoC specific compatible.
There was some discussion on this in v1: 
https://lore.kernel.org/all/20241015-distrust-chatty-9e723e670fef@spud/

The compatible is intentionally generic, representing a “generic” SBI ecall
interface to a set of remote processors, with the platform abstracted 
via SBI
ecalls. The IPC/IHC (named differently depending on whether it is RTL 
for the
FPGA fabric or a hardened version) is intended for Asymmetric 
Multiprocessing,
where a set of cores can run other firmware, such as Zephyr.

Unlike platforms with a fixed DSP, the configuration here is variable 
even for
a single SoC. For example, which memory regions are used for the
remote cluster or which mailbox channel is selected.

Because the configuration can vary even on the same SoC, adding a 
SOC-specific
compatible string provides no additional clarity, as it does not 
correspond to
a unique configuration. That said, if SOC-specific compatible strings are
needed, I can add them.

Thanks,
Valentina
>
> Best regards,
> Krzysztof
>


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

* Re: [PATCH v2 1/2] dt-bindings: remoteproc: add Microchip IPC remoteproc
  2025-11-21 18:28   ` Conor Dooley
@ 2025-12-01 16:16     ` Valentina.FernandezAlanis
  0 siblings, 0 replies; 11+ messages in thread
From: Valentina.FernandezAlanis @ 2025-12-01 16:16 UTC (permalink / raw)
  To: conor
  Cc: andersson, mathieu.poirier, robh, krzk+dt, conor+dt,
	linux-remoteproc, devicetree, linux-kernel,
	Valentina.FernandezAlanis

On 21/11/2025 18:28, Conor Dooley wrote:
> On Fri, Nov 21, 2025 at 02:21:56PM +0000, Valentina Fernandez wrote:
>> Microchip family of RISC-V SoCs typically have one or more application
>> clusters. These clusters can be configured to run in an Asymmetric
>> Multi Processing (AMP) mode.
>>
>> Add a dt-binding for these application clusters.
>>
>> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
>> ---
>>   .../microchip,ipc-sbi-remoteproc.yaml         | 95 +++++++++++++++++++
>>   1 file changed, 95 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
>> new file mode 100644
>> index 000000000000..348902f9a202
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
>> @@ -0,0 +1,95 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-sbi-remoteproc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Microchip IPC Remote Processor
>> +
>> +description:
>> +  Microchip family of RISC-V SoCs typically have one or more
>> +  clusters. These clusters can be configured to run in an Asymmetric
>> +  Multi Processing (AMP) mode where clusters are split in independent
>> +  software contexts.
>> +
>> +maintainers:
>> +  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: microchip,ipc-sbi-remoteproc
>> +
>> +  mboxes:
>> +    description:
>> +      Microchip IPC mailbox specifier. To be used for communication with
>> +      a remote cluster. The specifier format is as per the bindings,
>> +      Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
>> +    maxItems: 1
>> +
>> +  memory-region:
>> +    minItems: 1
>> +    maxItems: 5
>> +    description:
>> +      List of phandles to the reserved memory regions associated wih the remoteproc
>> +      device. This is variable and describes the memories shared with the remote cluster
>> +      (e.g. firmware, resource table, rpmsg vrings, etc.)
>> +    items:
>> +      anyOf:
> Is this genuinely any of these, with no restrictions?
> Can you have rsc-table and firmware?
Yes, it is possible to include both rsc-table and firmware. This 
typically occurs
when early boot is used, followed by remoteproc start/stop to launch
additional firmware.

However, there are a few restrictions worth adding. For example, vring0 and
vring1 memory regions make no sense without a buffer region, and at least
one of rsc-table or firmware must be present. I can include these rules
in v3.

Thanks,
Valentina
>> +        - description: region used for the resource table when firmware is started by the bootloader
>> +        - description: region used for the remote cluster firmware image section
>> +        - description: virtio device (vdev) buffer
>> +        - description: virtqueue for sending messages to the remote cluster (vring0)
>> +        - description: virtqueue for receiving messages from the remote cluster (vring1)
>> +
>> +  memory-region-names:
>> +    minItems: 1
>> +    maxItems: 5
>> +    items:
>> +      anyOf:
>> +        - const: rsc-table
>> +        - const: firmware
>> +        - const: buffer
>> +        - const: vring0
>> +        - const: vring1
>> +
>> +required:
>> +  - compatible
>> +  - mboxes
>> +  - memory-region
>> +  - memory-region-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    // Early boot mode example - firmware started by bootloader
>> +    soc {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        remoteproc {
>> +            compatible = "microchip,ipc-sbi-remoteproc";
>> +            mboxes= <&ihc 8>;
>> +            memory-region = <&rsctable>, <&vdev0buffer>,
>> +                            <&vdev0vring0>, <&vdev0vring1>;
>> +            memory-region-names = "rsc-table", "buffer",
>> +                                  "vring0", "vring1";
>> +        };
>> +    };
>> +
>> +  - |
>> +    // Late boot mode example - firmware started by Linux (remoteproc)
>> +    soc {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        remoteproc {
>> +            compatible = "microchip,ipc-sbi-remoteproc";
>> +            mboxes= <&ihc 8>;
>> +            memory-region = <&cluster_firmware>, <&vdev0buffer>,
>> +                            <&vdev0vring0>, <&vdev0vring1>;
>> +            memory-region-names = "firmware", "buffer",
>> +                                  "vring0", "vring1";
>> +        };
>> +    };
>> +...
>> -- 
>> 2.34.1
>>


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

* Re: [PATCH v2 1/2] dt-bindings: remoteproc: add Microchip IPC remoteproc
  2025-12-01 16:04     ` Valentina.FernandezAlanis
@ 2025-12-01 16:22       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2025-12-01 16:22 UTC (permalink / raw)
  To: Valentina.FernandezAlanis
  Cc: andersson, mathieu.poirier, robh, krzk+dt, conor+dt,
	linux-remoteproc, devicetree, linux-kernel

On 01/12/2025 17:04, Valentina.FernandezAlanis@microchip.com wrote:
> On 25/11/2025 09:46, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On Fri, Nov 21, 2025 at 02:21:56PM +0000, Valentina Fernandez wrote:
>>> Microchip family of RISC-V SoCs typically have one or more application
>>> clusters. These clusters can be configured to run in an Asymmetric
>>> Multi Processing (AMP) mode.
>>>
>>> Add a dt-binding for these application clusters.
>>>
>>> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
>>> ---
>>>   .../microchip,ipc-sbi-remoteproc.yaml         | 95 +++++++++++++++++++
>>>   1 file changed, 95 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
>>> new file mode 100644
>>> index 000000000000..348902f9a202
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
>>> @@ -0,0 +1,95 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-sbi-remoteproc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Microchip IPC Remote Processor
>>> +
>>> +description:
>>> +  Microchip family of RISC-V SoCs typically have one or more
>>> +  clusters. These clusters can be configured to run in an Asymmetric
>>> +  Multi Processing (AMP) mode where clusters are split in independent
>>> +  software contexts.
>>> +
>>> +maintainers:
>>> +  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: microchip,ipc-sbi-remoteproc
>> This should be SoC specific compatible.
> There was some discussion on this in v1: 
> https://lore.kernel.org/all/20241015-distrust-chatty-9e723e670fef@spud/


I don't find anything from that explained in commit msg or device
description, so next time you send you will get exactly the same comment.

Best regards,
Krzysztof

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

* Re: [PATCH v2 2/2] remoteproc: add support for Microchip IPC remoteproc platform driver
  2025-11-21 14:21 ` [PATCH v2 2/2] remoteproc: add support for Microchip IPC remoteproc platform driver Valentina Fernandez
@ 2025-12-09 17:33   ` Mathieu Poirier
  2025-12-10 18:37     ` Valentina.FernandezAlanis
  0 siblings, 1 reply; 11+ messages in thread
From: Mathieu Poirier @ 2025-12-09 17:33 UTC (permalink / raw)
  To: Valentina Fernandez
  Cc: andersson, robh, krzk+dt, conor+dt, linux-remoteproc, devicetree,
	linux-kernel

Good day,

On Fri, Nov 21, 2025 at 02:21:57PM +0000, Valentina Fernandez wrote:
> Microchip family of RISC-V SoCs typically have one or more
> clusters. These clusters can be configured to run in an Asymmetric
> Multi Processing (AMP) mode where clusters are split in independent
> software contexts.
> 
> Add a remoteproc platform driver to be able to load and boot firmware
> to remote processors.
> 
> The driver uses SBI (RISC-V Supervisor Binary Interface) ecalls to
> request software running in machine privilege mode (M-mode) to
> start/stop the remote processor.
> 
> Inter-processor communication is supported through the
> virtio rpmsg stack using shared memory and the Microchip IPC mailbox
> driver.
> 
> Currently, the driver the following features are supported:
> - Start/stop a remote software context
> - Kick function implementation for RPMsg Communication
> - Attach to a remote context loaded by another entity (bootloader)
> 
> Error Recovery and Power Management features are not currently supported.
> 
> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> ---
>  drivers/remoteproc/Kconfig                   |  12 +
>  drivers/remoteproc/Makefile                  |   1 +
>  drivers/remoteproc/mchp_ipc_sbi_remoteproc.c | 465 +++++++++++++++++++
>  3 files changed, 478 insertions(+)
>  create mode 100644 drivers/remoteproc/mchp_ipc_sbi_remoteproc.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 48a0d3a69ed0..68bd68f553ea 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -54,6 +54,18 @@ config INGENIC_VPU_RPROC
>  	  This can be either built-in or a loadable module.
>  	  If unsure say N.
>  
> +config MCHP_IPC_SBI_REMOTEPROC
> +	tristate "Microchip IPC remoteproc support"
> +	depends on MCHP_SBI_IPC_MBOX || COMPILE_TEST
> +	depends on ARCH_MICROCHIP
> +	help
> +	  Say y here to support booting and loading firmware to remote
> +	  processors on various Microchip family of RISC-V SoCs via the
> +	  remote processor framework.
> +	  This can be either built-in or a loadable module.a

No line break needed.

> +	  If compiled as module, the module will be called mchp_ipc_remoteproc.
> +	  If unsure say N.
> +
>  config MTK_SCP
>  	tristate "Mediatek SCP support"
>  	depends on ARCH_MEDIATEK || COMPILE_TEST
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 1c7598b8475d..afb28dff5666 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_REMOTEPROC_CDEV)		+= remoteproc_cdev.o
>  obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
>  obj-$(CONFIG_IMX_DSP_REMOTEPROC)	+= imx_dsp_rproc.o
>  obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
> +obj-$(CONFIG_MCHP_IPC_SBI_REMOTEPROC)	+= mchp_ipc_sbi_remoteproc.o

Please drop the "ipc_sbi", it is redundant and doesn't any useful information.
Here and everywhere in this file.

>  obj-$(CONFIG_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
>  obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>  obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
> diff --git a/drivers/remoteproc/mchp_ipc_sbi_remoteproc.c b/drivers/remoteproc/mchp_ipc_sbi_remoteproc.c
> new file mode 100644
> index 000000000000..55d182c1eee7
> --- /dev/null
> +++ b/drivers/remoteproc/mchp_ipc_sbi_remoteproc.c
> @@ -0,0 +1,465 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Microchip IPC Remoteproc driver
> + *
> + * Copyright (c) 2021 - 2025 Microchip Technology Inc. All rights reserved.
> + *
> + * Author: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> + *
> + * Derived from the imx_rproc implementation:
> + * Copyright (c) 2017 Pengutronix, Oleksij Rempel <kernel@pengutronix.de>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +#include <linux/workqueue.h>
> +
> +#include <asm/sbi.h>
> +#include <asm/vendorid_list.h>
> +#include <linux/mailbox/mchp-ipc.h>
> +
> +#include "remoteproc_internal.h"
> +
> +#define SBI_EXT_MICROCHIP_TECHNOLOGY	(SBI_EXT_VENDOR_START | \
> +					 MICROCHIP_VENDOR_ID)
> +
> +#define MIV_RPROC_MEM_MAX		2
> +
> +enum {
> +	SBI_EXT_RPROC_STATE = 0x3,
> +	SBI_EXT_RPROC_START,
> +	SBI_EXT_RPROC_STOP,
> +};
> +
> +/**
> + * enum mchp_ipc_rproc_mbox_messages - predefined mailbox messages
> + *
> + * @MCHP_IPC_RPROC_MBOX_READY: a ready message response from a remote context indicating
> + * that the remote context is up and running.
> + *
> + * @MCHP_IPC_RPROC_MBOX_PENDING_MSG: Not currently in use, but reserved for future use
> + * to inform the receiver that there is a message awaiting in its receive-side
> + * vring. At the moment, one can explicitly send the index of the triggered
> + * virtqueue as a payload.
> + *
> + * @MCHP_IPC_RPROC_MBOX_STOP: a stop request for the remote context
> + *
> + * @MCHP_IPC_RPROC_MBOX_END_MSG: Indicates end of known/defined messages.
> + * This should be the last definition.
> + *
> + */
> +enum mchp_ipc_rproc_mbox_messages {
> +	MCHP_IPC_RPROC_MBOX_READY = 0xFFFFFF00,
> +	MCHP_IPC_RPROC_MBOX_PENDING_MSG = 0xFFFFFF01,
> +	MCHP_IPC_RPROC_MBOX_STOP = 0xFFFFFF02,
> +	MCHP_IPC_RPROC_MBOX_END_MSG = 0xFFFFFF03,
> +};
> +
> +enum {
> +	MCHP_IPC_RPROC_RSC_TABLE_REGION,
> +	MCHP_IPC_RPROC_FW_REGION,
> +	MCHP_IPC_RPROC_BUFF_REGION,
> +	MCHP_IPC_RPROC_VRING0_REGION,
> +	MCHP_IPC_RPROC_VRING1_REGION,
> +	MCHP_IPC_RPROC_REGION_MAX,
> +};
> +
> +struct mchp_ipc_rproc_mem_region {
> +	const char *name;
> +	const char *prefix;
> +};
> +
> +struct mchp_ipc_rproc {
> +	struct device *dev;
> +	struct rproc *rproc;
> +	struct mbox_chan *mbox_channel;
> +	struct workqueue_struct *workqueue;
> +	struct mbox_client mbox_client;
> +	struct completion start_done;
> +	struct work_struct rproc_work;
> +	struct mchp_ipc_msg message;
> +	void __iomem *rsc_table;
> +	bool initialized;
> +	u32 chan_id;
> +};
> +
> +static struct mchp_ipc_rproc_mem_region mchp_rproc_mem_regions[] = {
> +	[MCHP_IPC_RPROC_RSC_TABLE_REGION] = {
> +		.name = "rsc-table",
> +		.prefix = NULL,
> +	},
> +	[MCHP_IPC_RPROC_FW_REGION] = {
> +		.name = "firmware",
> +		.prefix = NULL,
> +	},
> +	[MCHP_IPC_RPROC_BUFF_REGION] = {
> +		.name = "buffer",
> +		.prefix = "vdev0",
> +	},
> +	[MCHP_IPC_RPROC_VRING0_REGION] = {
> +		.name = "vring0",
> +		.prefix = "vdev0",
> +	},
> +	[MCHP_IPC_RPROC_VRING1_REGION] = {
> +		.name = "vring1",
> +		.prefix = "vdev0",
> +	},
> +};
> +
> +static int mchp_ipc_rproc_start(struct rproc *rproc)
> +{
> +	struct mchp_ipc_rproc *priv = rproc->priv;
> +	struct sbiret ret;
> +
> +	ret = sbi_ecall(SBI_EXT_MICROCHIP_TECHNOLOGY, SBI_EXT_RPROC_START,
> +			priv->chan_id, rproc->bootaddr, 0, 0, 0, 0);
> +
> +	if (ret.error)
> +		return sbi_err_map_linux_errno(ret.error);
> +
> +	if (!wait_for_completion_timeout(&priv->start_done,
> +					 msecs_to_jiffies(5000))) {
> +		dev_err(priv->dev, "timeout waiting for ready notification\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mchp_ipc_rproc_stop(struct rproc *rproc)
> +{
> +	struct mchp_ipc_rproc *priv = rproc->priv;
> +	struct sbiret ret;
> +
> +	ret = sbi_ecall(SBI_EXT_MICROCHIP_TECHNOLOGY, SBI_EXT_RPROC_STOP,
> +			priv->chan_id, MCHP_IPC_RPROC_MBOX_STOP, 0, 0, 0, 0);
> +
> +	if (ret.error)
> +		return sbi_err_map_linux_errno(ret.error);
> +
> +	return ret.value;
> +}
> +
> +static int mchp_ipc_rproc_mem_alloc(struct rproc *rproc, struct rproc_mem_entry *mem)
> +{
> +	struct device *dev = rproc->dev.parent;
> +	void *va;
> +
> +	dev_dbg(dev, "map memory: %pad+%zx\n", &mem->dma, mem->len);
> +	va = ioremap_wc(mem->dma, mem->len);
> +	if (IS_ERR_OR_NULL(va)) {
> +		dev_err(dev, "Unable to map memory region: %p+%zx\n",
> +			&mem->dma, mem->len);
> +		return -ENOMEM;
> +	}
> +
> +	mem->va = va;
> +
> +	return 0;
> +}
> +
> +static int mchp_ipc_rproc_mem_release(struct rproc *rproc,
> +				      struct rproc_mem_entry *mem)
> +{
> +	dev_dbg(rproc->dev.parent, "unmap memory: %pad\n", &mem->dma);
> +	iounmap(mem->va);
> +
> +	return 0;
> +}
> +
> +static int mchp_ipc_rproc_prepare(struct rproc *rproc)
> +{
> +	struct mchp_ipc_rproc *priv = rproc->priv;
> +	struct device_node *np = priv->dev->of_node;
> +	struct rproc_mem_entry *mem;
> +	struct resource res;
> +	int i, ret;
> +
> +	reinit_completion(&priv->start_done);
> +
> +	for (i = 0; i < ARRAY_SIZE(mchp_rproc_mem_regions); i++) {
> +		char *mem_region_name = devm_kstrdup(priv->dev,
> +						     mchp_rproc_mem_regions[i].name, GFP_KERNEL);
> +		int index = of_property_match_string(np, "memory-region-names", mem_region_name);
> +
> +		if (index < 0)
> +			continue;
> +
> +		ret = of_reserved_mem_region_to_resource_byname(np, mem_region_name, &res);
> +		if (ret)
> +			return ret;
> +
> +		if (mchp_rproc_mem_regions[i].prefix) {
> +			mem_region_name = devm_kasprintf(priv->dev, GFP_KERNEL, "%s%s",
> +							 mchp_rproc_mem_regions[i].prefix,
> +							 mem_region_name);
> +		}
> +
> +		if (i == MCHP_IPC_RPROC_BUFF_REGION) {
> +			mem = rproc_of_resm_mem_entry_init(priv->dev, index, resource_size(&res),
> +							   res.start, mem_region_name);
> +		} else {
> +			mem = rproc_mem_entry_init(priv->dev, NULL,
> +						   (dma_addr_t)res.start,
> +						   resource_size(&res), res.start,
> +						   mchp_ipc_rproc_mem_alloc,
> +						   mchp_ipc_rproc_mem_release,
> +						   mem_region_name);
> +		}
> +		rproc_add_carveout(rproc, mem);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mchp_ipc_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> +{
> +	int ret;
> +
> +	ret = rproc_elf_load_rsc_table(rproc, fw);
> +	if (ret)
> +		dev_info(&rproc->dev, "No resource table in elf\n");
> +
> +	return 0;
> +}
> +
> +static void mchp_ipc_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +	struct mchp_ipc_rproc *priv = (struct mchp_ipc_rproc *)rproc->priv;
> +	struct mchp_ipc_msg msg;
> +	int ret;
> +
> +	msg.buf = (void *)&vqid;
> +	msg.size = sizeof(vqid);
> +
> +	ret = mbox_send_message(priv->mbox_channel, (void *)&msg);
> +	if (ret < 0)
> +		dev_err(priv->dev,
> +			"failed to send mbox message, status = %d\n", ret);
> +}
> +
> +static int mchp_ipc_rproc_attach(struct rproc *rproc)
> +{
> +	return 0;
> +}
> +
> +static struct resource_table
> +*mchp_ipc_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
> +{
> +	struct mchp_ipc_rproc *priv = rproc->priv;
> +
> +	if (!priv->rsc_table)
> +		return NULL;
> +
> +	*table_sz = SZ_1K;
> +	return (struct resource_table *)priv->rsc_table;
> +}
> +
> +static const struct rproc_ops mchp_ipc_rproc_ops = {
> +	.prepare = mchp_ipc_rproc_prepare,
> +	.start = mchp_ipc_rproc_start,
> +	.get_loaded_rsc_table = mchp_ipc_rproc_get_loaded_rsc_table,
> +	.attach = mchp_ipc_rproc_attach,
> +	.stop = mchp_ipc_rproc_stop,
> +	.kick = mchp_ipc_rproc_kick,
> +	.load = rproc_elf_load_segments,
> +	.parse_fw = mchp_ipc_rproc_parse_fw,
> +	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> +	.sanity_check = rproc_elf_sanity_check,
> +	.get_boot_addr = rproc_elf_get_boot_addr,
> +};
> +
> +static void mchp_ipc_rproc_vq_work(struct work_struct *work)
> +{
> +	struct mchp_ipc_rproc *priv = container_of(work, struct mchp_ipc_rproc, rproc_work);
> +	struct device *dev = priv->rproc->dev.parent;
> +
> +	u32 msg = priv->message.buf[0];
> +
> +	/*
> +	 * Currently, we are expected to receive the following messages
> +	 * from the remote cluster: a ready message or receive the index
> +	 * of the triggered virtqueue as a payload.
> +	 * We can silently ignore any other type of mailbox messages since
> +	 * they are not meant for us and are meant to be received by the
> +	 * remote cluster only.
> +	 */
> +	switch (msg) {
> +	case MCHP_IPC_RPROC_MBOX_READY:
> +		complete(&priv->start_done);
> +		break;
> +	default:
> +		if (msg >= MCHP_IPC_RPROC_MBOX_READY && msg < MCHP_IPC_RPROC_MBOX_END_MSG)

The condition (msg == MCHP_IPC_RPROC_MBOX_READY) is already checked as part of
the previous case statement.  If you need to repeat this check, the design of
the case state likely needs more work.

> +			return;
> +		if (msg > priv->rproc->max_notifyid) {
> +			dev_info(dev, "dropping unknown message 0x%x", msg);
> +			return;
> +		}
> +		/* msg contains the index of the triggered vring */
> +		if (rproc_vq_interrupt(priv->rproc, msg) == IRQ_NONE)
> +			dev_dbg(dev, "no message was found in vqid %d\n", msg);
> +	}
> +}
> +
> +static void mchp_ipc_rproc_rx_callback(struct mbox_client *mbox_client, void *msg)
> +{
> +	struct rproc *rproc = dev_get_drvdata(mbox_client->dev);
> +	struct mchp_ipc_rproc *priv = rproc->priv;
> +
> +	priv->message = *(struct mchp_ipc_msg *)msg;
> +	queue_work(priv->workqueue, &priv->rproc_work);
> +}
> +
> +static int mchp_ipc_rproc_mbox_init(struct rproc *rproc)
> +{
> +	struct mchp_ipc_rproc *priv = rproc->priv;
> +	struct device *dev = priv->dev;
> +	struct mchp_ipc_sbi_chan *chan_priv;
> +	struct mbox_client *mbox_client;
> +
> +	mbox_client = &priv->mbox_client;
> +	mbox_client->dev = dev;
> +	mbox_client->tx_block = true;
> +	mbox_client->tx_tout = 100;
> +	mbox_client->knows_txdone = false;
> +	mbox_client->rx_callback = mchp_ipc_rproc_rx_callback;
> +
> +	priv->mbox_channel = mbox_request_channel(mbox_client, 0);
> +	if (IS_ERR(priv->mbox_channel))
> +		return dev_err_probe(mbox_client->dev,
> +				     PTR_ERR(priv->mbox_channel),
> +				     "failed to request mailbox channel\n");
> +
> +	chan_priv = (struct mchp_ipc_sbi_chan *)priv->mbox_channel->con_priv;

Where is the memory for ->con_priv allocated?

> +	priv->chan_id = chan_priv->id;
> +
> +	return 0;
> +}
> +
> +static int mchp_ipc_rproc_get_state(u32 chan)
> +{
> +	struct sbiret ret;
> +
> +	ret = sbi_ecall(SBI_EXT_MICROCHIP_TECHNOLOGY, SBI_EXT_RPROC_STATE,
> +			chan, 0, 0, 0, 0, 0);
> +
> +	if (ret.error)
> +		return sbi_err_map_linux_errno(ret.error);
> +
> +	return ret.value;
> +}
> +
> +static int mchp_ipc_rproc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct mchp_ipc_rproc *priv;
> +	struct rproc *rproc;
> +	struct resource res;
> +	int num_rmems, ret;
> +
> +	rproc = devm_rproc_alloc(dev, np->name, &mchp_ipc_rproc_ops,
> +				 NULL, sizeof(*priv));
> +	if (!rproc)
> +		return -ENOMEM;
> +
> +	priv = rproc->priv;
> +	priv->rproc = rproc;
> +	priv->dev = dev;
> +
> +	num_rmems = of_property_count_elems_of_size(np, "memory-region",
> +						    sizeof(phandle));

Please add a comment to describe why a minimum of 1 memory region is needed.
Otherwise I have to guess.

> +	if (num_rmems < 1)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "device needs at least one memory regions to be defined\n");
> +
> +	ret = of_reserved_mem_region_to_resource_byname(np, "rsc-table", &res);
> +	if (!ret) {
> +		priv->rsc_table  = devm_ioremap_resource(dev, &res);
> +		if (IS_ERR(priv->rsc_table)) {
> +			return dev_err_probe(dev, PTR_ERR(priv->rsc_table),
> +					     "failed to map resource table\n");
> +		}
> +	}

The above makes me suspicious but I currently don't have time to look in the
rproc_ops for answers.  You can either spin off a  new revision to address the
comments already received or wait for me to look at the rproc_ops in January -
the choice is yours.

Thanks,
Mathieu 

> +
> +	priv->workqueue = create_workqueue(dev_name(dev));
> +	if (!priv->workqueue)
> +		return dev_err_probe(dev, -ENOMEM, "cannot create workqueue\n");
> +
> +	INIT_WORK(&priv->rproc_work, mchp_ipc_rproc_vq_work);
> +	init_completion(&priv->start_done);
> +
> +	ret = mchp_ipc_rproc_mbox_init(rproc);
> +	if (ret)
> +		goto err_put_wkq;
> +
> +	/*
> +	 * Check if the remote cluster has been booted by another entity
> +	 * (i.e. bootloader) and set rproc state accordingly
> +	 */
> +	rproc->state = mchp_ipc_rproc_get_state(priv->chan_id);
> +	if (ret < 0) {
> +		dev_err_probe(dev, ret, "Couldn't get cluster boot mode\n");
> +		goto err_put_mbox;
> +	}
> +
> +	if (rproc->state != RPROC_DETACHED)
> +		rproc->auto_boot = false;
> +
> +	/* error recovery is not supported at present */
> +	rproc->recovery_disabled = true;
> +
> +	dev_set_drvdata(dev, rproc);
> +
> +	ret = devm_rproc_add(dev, rproc);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "rproc_add failed\n");
> +		goto err_put_mbox;
> +	}
> +
> +	return 0;
> +
> +err_put_mbox:
> +	mbox_free_channel(priv->mbox_channel);
> +err_put_wkq:
> +	destroy_workqueue(priv->workqueue);
> +
> +	return ret;
> +}
> +
> +static void mchp_ipc_rproc_remove(struct platform_device *pdev)
> +{
> +	struct rproc *rproc = platform_get_drvdata(pdev);
> +	struct mchp_ipc_rproc *priv = rproc->priv;
> +
> +	mbox_free_channel(priv->mbox_channel);
> +	destroy_workqueue(priv->workqueue);
> +}
> +
> +static const struct of_device_id mchp_ipc_rproc_of_match[] __maybe_unused = {
> +	{ .compatible = "microchip,ipc-sbi-remoteproc", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mchp_ipc_rproc_of_match);
> +
> +static struct platform_driver mchp_ipc_rproc_driver = {
> +	.probe = mchp_ipc_rproc_probe,
> +	.remove = mchp_ipc_rproc_remove,
> +	.driver = {
> +		.name = "microchip-ipc-rproc",
> +		.of_match_table = of_match_ptr(mchp_ipc_rproc_of_match),
> +	},
> +};
> +
> +module_platform_driver(mchp_ipc_rproc_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Valentina Fernandez <valentina.fernandezalanis@microchip.com>");
> +MODULE_DESCRIPTION("Microchip IPC Remote Processor control driver");
> -- 
> 2.34.1
> 

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

* Re: [PATCH v2 2/2] remoteproc: add support for Microchip IPC remoteproc platform driver
  2025-12-09 17:33   ` Mathieu Poirier
@ 2025-12-10 18:37     ` Valentina.FernandezAlanis
  0 siblings, 0 replies; 11+ messages in thread
From: Valentina.FernandezAlanis @ 2025-12-10 18:37 UTC (permalink / raw)
  To: mathieu.poirier
  Cc: andersson, robh, krzk+dt, conor+dt, linux-remoteproc, devicetree,
	linux-kernel, Valentina.FernandezAlanis

On 09/12/2025 17:33, Mathieu Poirier wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Good day,
>
> On Fri, Nov 21, 2025 at 02:21:57PM +0000, Valentina Fernandez wrote:
>> Microchip family of RISC-V SoCs typically have one or more
>> clusters. These clusters can be configured to run in an Asymmetric
>> Multi Processing (AMP) mode where clusters are split in independent
>> software contexts.
>>
>> Add a remoteproc platform driver to be able to load and boot firmware
>> to remote processors.
>>
>> The driver uses SBI (RISC-V Supervisor Binary Interface) ecalls to
>> request software running in machine privilege mode (M-mode) to
>> start/stop the remote processor.
>>
>> Inter-processor communication is supported through the
>> virtio rpmsg stack using shared memory and the Microchip IPC mailbox
>> driver.
>>
>> Currently, the driver the following features are supported:
>> - Start/stop a remote software context
>> - Kick function implementation for RPMsg Communication
>> - Attach to a remote context loaded by another entity (bootloader)
>>
>> Error Recovery and Power Management features are not currently supported.
>>
>> Signed-off-by: Valentina Fernandez<valentina.fernandezalanis@microchip.com>
>> ---
>>   drivers/remoteproc/Kconfig                   |  12 +
>>   drivers/remoteproc/Makefile                  |   1 +
>>   drivers/remoteproc/mchp_ipc_sbi_remoteproc.c | 465 +++++++++++++++++++
>>   3 files changed, 478 insertions(+)
>>   create mode 100644 drivers/remoteproc/mchp_ipc_sbi_remoteproc.c
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index 48a0d3a69ed0..68bd68f553ea 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -54,6 +54,18 @@ config INGENIC_VPU_RPROC
>>          This can be either built-in or a loadable module.
>>          If unsure say N.
>>
>> +config MCHP_IPC_SBI_REMOTEPROC
>> +     tristate "Microchip IPC remoteproc support"
>> +     depends on MCHP_SBI_IPC_MBOX || COMPILE_TEST
>> +     depends on ARCH_MICROCHIP
>> +     help
>> +       Say y here to support booting and loading firmware to remote
>> +       processors on various Microchip family of RISC-V SoCs via the
>> +       remote processor framework.
>> +       This can be either built-in or a loadable module.a
> No line break needed.
>
>> +       If compiled as module, the module will be called mchp_ipc_remoteproc.
>> +       If unsure say N.
>> +
>>   config MTK_SCP
>>        tristate "Mediatek SCP support"
>>        depends on ARCH_MEDIATEK || COMPILE_TEST
>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>> index 1c7598b8475d..afb28dff5666 100644
>> --- a/drivers/remoteproc/Makefile
>> +++ b/drivers/remoteproc/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_REMOTEPROC_CDEV)               += remoteproc_cdev.o
>>   obj-$(CONFIG_IMX_REMOTEPROC)         += imx_rproc.o
>>   obj-$(CONFIG_IMX_DSP_REMOTEPROC)     += imx_dsp_rproc.o
>>   obj-$(CONFIG_INGENIC_VPU_RPROC)              += ingenic_rproc.o
>> +obj-$(CONFIG_MCHP_IPC_SBI_REMOTEPROC)        += mchp_ipc_sbi_remoteproc.o
> Please drop the "ipc_sbi", it is redundant and doesn't any useful information.
> Here and everywhere in this file.
>
>>   obj-$(CONFIG_MTK_SCP)                        += mtk_scp.o mtk_scp_ipi.o
>>   obj-$(CONFIG_OMAP_REMOTEPROC)                += omap_remoteproc.o
>>   obj-$(CONFIG_WKUP_M3_RPROC)          += wkup_m3_rproc.o
>> diff --git a/drivers/remoteproc/mchp_ipc_sbi_remoteproc.c b/drivers/remoteproc/mchp_ipc_sbi_remoteproc.c
>> new file mode 100644
>> index 000000000000..55d182c1eee7
>> --- /dev/null
>> +++ b/drivers/remoteproc/mchp_ipc_sbi_remoteproc.c
>> @@ -0,0 +1,465 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Microchip IPC Remoteproc driver
>> + *
>> + * Copyright (c) 2021 - 2025 Microchip Technology Inc. All rights reserved.
>> + *
>> + * Author: Valentina Fernandez<valentina.fernandezalanis@microchip.com>
>> + *
>> + * Derived from the imx_rproc implementation:
>> + * Copyright (c) 2017 Pengutronix, Oleksij Rempel<kernel@pengutronix.de>
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mailbox_client.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_reserved_mem.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/remoteproc.h>
>> +#include <linux/workqueue.h>
>> +
>> +#include <asm/sbi.h>
>> +#include <asm/vendorid_list.h>
>> +#include <linux/mailbox/mchp-ipc.h>
>> +
>> +#include "remoteproc_internal.h"
>> +
>> +#define SBI_EXT_MICROCHIP_TECHNOLOGY (SBI_EXT_VENDOR_START | \
>> +                                      MICROCHIP_VENDOR_ID)
>> +
>> +#define MIV_RPROC_MEM_MAX            2
>> +
>> +enum {
>> +     SBI_EXT_RPROC_STATE = 0x3,
>> +     SBI_EXT_RPROC_START,
>> +     SBI_EXT_RPROC_STOP,
>> +};
>> +
>> +/**
>> + * enum mchp_ipc_rproc_mbox_messages - predefined mailbox messages
>> + *
>> + * @MCHP_IPC_RPROC_MBOX_READY: a ready message response from a remote context indicating
>> + * that the remote context is up and running.
>> + *
>> + * @MCHP_IPC_RPROC_MBOX_PENDING_MSG: Not currently in use, but reserved for future use
>> + * to inform the receiver that there is a message awaiting in its receive-side
>> + * vring. At the moment, one can explicitly send the index of the triggered
>> + * virtqueue as a payload.
>> + *
>> + * @MCHP_IPC_RPROC_MBOX_STOP: a stop request for the remote context
>> + *
>> + * @MCHP_IPC_RPROC_MBOX_END_MSG: Indicates end of known/defined messages.
>> + * This should be the last definition.
>> + *
>> + */
>> +enum mchp_ipc_rproc_mbox_messages {
>> +     MCHP_IPC_RPROC_MBOX_READY = 0xFFFFFF00,
>> +     MCHP_IPC_RPROC_MBOX_PENDING_MSG = 0xFFFFFF01,
>> +     MCHP_IPC_RPROC_MBOX_STOP = 0xFFFFFF02,
>> +     MCHP_IPC_RPROC_MBOX_END_MSG = 0xFFFFFF03,
>> +};
>> +
>> +enum {
>> +     MCHP_IPC_RPROC_RSC_TABLE_REGION,
>> +     MCHP_IPC_RPROC_FW_REGION,
>> +     MCHP_IPC_RPROC_BUFF_REGION,
>> +     MCHP_IPC_RPROC_VRING0_REGION,
>> +     MCHP_IPC_RPROC_VRING1_REGION,
>> +     MCHP_IPC_RPROC_REGION_MAX,
>> +};
>> +
>> +struct mchp_ipc_rproc_mem_region {
>> +     const char *name;
>> +     const char *prefix;
>> +};
>> +
>> +struct mchp_ipc_rproc {
>> +     struct device *dev;
>> +     struct rproc *rproc;
>> +     struct mbox_chan *mbox_channel;
>> +     struct workqueue_struct *workqueue;
>> +     struct mbox_client mbox_client;
>> +     struct completion start_done;
>> +     struct work_struct rproc_work;
>> +     struct mchp_ipc_msg message;
>> +     void __iomem *rsc_table;
>> +     bool initialized;
>> +     u32 chan_id;
>> +};
>> +
>> +static struct mchp_ipc_rproc_mem_region mchp_rproc_mem_regions[] = {
>> +     [MCHP_IPC_RPROC_RSC_TABLE_REGION] = {
>> +             .name = "rsc-table",
>> +             .prefix = NULL,
>> +     },
>> +     [MCHP_IPC_RPROC_FW_REGION] = {
>> +             .name = "firmware",
>> +             .prefix = NULL,
>> +     },
>> +     [MCHP_IPC_RPROC_BUFF_REGION] = {
>> +             .name = "buffer",
>> +             .prefix = "vdev0",
>> +     },
>> +     [MCHP_IPC_RPROC_VRING0_REGION] = {
>> +             .name = "vring0",
>> +             .prefix = "vdev0",
>> +     },
>> +     [MCHP_IPC_RPROC_VRING1_REGION] = {
>> +             .name = "vring1",
>> +             .prefix = "vdev0",
>> +     },
>> +};
>> +
>> +static int mchp_ipc_rproc_start(struct rproc *rproc)
>> +{
>> +     struct mchp_ipc_rproc *priv = rproc->priv;
>> +     struct sbiret ret;
>> +
>> +     ret = sbi_ecall(SBI_EXT_MICROCHIP_TECHNOLOGY, SBI_EXT_RPROC_START,
>> +                     priv->chan_id, rproc->bootaddr, 0, 0, 0, 0);
>> +
>> +     if (ret.error)
>> +             return sbi_err_map_linux_errno(ret.error);
>> +
>> +     if (!wait_for_completion_timeout(&priv->start_done,
>> +                                      msecs_to_jiffies(5000))) {
>> +             dev_err(priv->dev, "timeout waiting for ready notification\n");
>> +             return -ETIMEDOUT;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int mchp_ipc_rproc_stop(struct rproc *rproc)
>> +{
>> +     struct mchp_ipc_rproc *priv = rproc->priv;
>> +     struct sbiret ret;
>> +
>> +     ret = sbi_ecall(SBI_EXT_MICROCHIP_TECHNOLOGY, SBI_EXT_RPROC_STOP,
>> +                     priv->chan_id, MCHP_IPC_RPROC_MBOX_STOP, 0, 0, 0, 0);
>> +
>> +     if (ret.error)
>> +             return sbi_err_map_linux_errno(ret.error);
>> +
>> +     return ret.value;
>> +}
>> +
>> +static int mchp_ipc_rproc_mem_alloc(struct rproc *rproc, struct rproc_mem_entry *mem)
>> +{
>> +     struct device *dev = rproc->dev.parent;
>> +     void *va;
>> +
>> +     dev_dbg(dev, "map memory: %pad+%zx\n", &mem->dma, mem->len);
>> +     va = ioremap_wc(mem->dma, mem->len);
>> +     if (IS_ERR_OR_NULL(va)) {
>> +             dev_err(dev, "Unable to map memory region: %p+%zx\n",
>> +                     &mem->dma, mem->len);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     mem->va = va;
>> +
>> +     return 0;
>> +}
>> +
>> +static int mchp_ipc_rproc_mem_release(struct rproc *rproc,
>> +                                   struct rproc_mem_entry *mem)
>> +{
>> +     dev_dbg(rproc->dev.parent, "unmap memory: %pad\n", &mem->dma);
>> +     iounmap(mem->va);
>> +
>> +     return 0;
>> +}
>> +
>> +static int mchp_ipc_rproc_prepare(struct rproc *rproc)
>> +{
>> +     struct mchp_ipc_rproc *priv = rproc->priv;
>> +     struct device_node *np = priv->dev->of_node;
>> +     struct rproc_mem_entry *mem;
>> +     struct resource res;
>> +     int i, ret;
>> +
>> +     reinit_completion(&priv->start_done);
>> +
>> +     for (i = 0; i < ARRAY_SIZE(mchp_rproc_mem_regions); i++) {
>> +             char *mem_region_name = devm_kstrdup(priv->dev,
>> +                                                  mchp_rproc_mem_regions[i].name, GFP_KERNEL);
>> +             int index = of_property_match_string(np, "memory-region-names", mem_region_name);
>> +
>> +             if (index < 0)
>> +                     continue;
>> +
>> +             ret = of_reserved_mem_region_to_resource_byname(np, mem_region_name, &res);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             if (mchp_rproc_mem_regions[i].prefix) {
>> +                     mem_region_name = devm_kasprintf(priv->dev, GFP_KERNEL, "%s%s",
>> +                                                      mchp_rproc_mem_regions[i].prefix,
>> +                                                      mem_region_name);
>> +             }
>> +
>> +             if (i == MCHP_IPC_RPROC_BUFF_REGION) {
>> +                     mem = rproc_of_resm_mem_entry_init(priv->dev, index, resource_size(&res),
>> +                                                        res.start, mem_region_name);
>> +             } else {
>> +                     mem = rproc_mem_entry_init(priv->dev, NULL,
>> +                                                (dma_addr_t)res.start,
>> +                                                resource_size(&res), res.start,
>> +                                                mchp_ipc_rproc_mem_alloc,
>> +                                                mchp_ipc_rproc_mem_release,
>> +                                                mem_region_name);
>> +             }
>> +             rproc_add_carveout(rproc, mem);
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int mchp_ipc_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
>> +{
>> +     int ret;
>> +
>> +     ret = rproc_elf_load_rsc_table(rproc, fw);
>> +     if (ret)
>> +             dev_info(&rproc->dev, "No resource table in elf\n");
>> +
>> +     return 0;
>> +}
>> +
>> +static void mchp_ipc_rproc_kick(struct rproc *rproc, int vqid)
>> +{
>> +     struct mchp_ipc_rproc *priv = (struct mchp_ipc_rproc *)rproc->priv;
>> +     struct mchp_ipc_msg msg;
>> +     int ret;
>> +
>> +     msg.buf = (void *)&vqid;
>> +     msg.size = sizeof(vqid);
>> +
>> +     ret = mbox_send_message(priv->mbox_channel, (void *)&msg);
>> +     if (ret < 0)
>> +             dev_err(priv->dev,
>> +                     "failed to send mbox message, status = %d\n", ret);
>> +}
>> +
>> +static int mchp_ipc_rproc_attach(struct rproc *rproc)
>> +{
>> +     return 0;
>> +}
>> +
>> +static struct resource_table
>> +*mchp_ipc_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
>> +{
>> +     struct mchp_ipc_rproc *priv = rproc->priv;
>> +
>> +     if (!priv->rsc_table)
>> +             return NULL;
>> +
>> +     *table_sz = SZ_1K;
>> +     return (struct resource_table *)priv->rsc_table;
>> +}
>> +
>> +static const struct rproc_ops mchp_ipc_rproc_ops = {
>> +     .prepare = mchp_ipc_rproc_prepare,
>> +     .start = mchp_ipc_rproc_start,
>> +     .get_loaded_rsc_table = mchp_ipc_rproc_get_loaded_rsc_table,
>> +     .attach = mchp_ipc_rproc_attach,
>> +     .stop = mchp_ipc_rproc_stop,
>> +     .kick = mchp_ipc_rproc_kick,
>> +     .load = rproc_elf_load_segments,
>> +     .parse_fw = mchp_ipc_rproc_parse_fw,
>> +     .find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
>> +     .sanity_check = rproc_elf_sanity_check,
>> +     .get_boot_addr = rproc_elf_get_boot_addr,
>> +};
>> +
>> +static void mchp_ipc_rproc_vq_work(struct work_struct *work)
>> +{
>> +     struct mchp_ipc_rproc *priv = container_of(work, struct mchp_ipc_rproc, rproc_work);
>> +     struct device *dev = priv->rproc->dev.parent;
>> +
>> +     u32 msg = priv->message.buf[0];
>> +
>> +     /*
>> +      * Currently, we are expected to receive the following messages
>> +      * from the remote cluster: a ready message or receive the index
>> +      * of the triggered virtqueue as a payload.
>> +      * We can silently ignore any other type of mailbox messages since
>> +      * they are not meant for us and are meant to be received by the
>> +      * remote cluster only.
>> +      */
>> +     switch (msg) {
>> +     case MCHP_IPC_RPROC_MBOX_READY:
>> +             complete(&priv->start_done);
>> +             break;
>> +     default:
>> +             if (msg >= MCHP_IPC_RPROC_MBOX_READY && msg < MCHP_IPC_RPROC_MBOX_END_MSG)
> The condition (msg == MCHP_IPC_RPROC_MBOX_READY) is already checked as part of
> the previous case statement.  If you need to repeat this check, the design of
> the case state likely needs more work.
Thanks for the feedback. I'll rework the case handling in v3.
>> +                     return;
>> +             if (msg > priv->rproc->max_notifyid) {
>> +                     dev_info(dev, "dropping unknown message 0x%x", msg);
>> +                     return;
>> +             }
>> +             /* msg contains the index of the triggered vring */
>> +             if (rproc_vq_interrupt(priv->rproc, msg) == IRQ_NONE)
>> +                     dev_dbg(dev, "no message was found in vqid %d\n", msg);
>> +     }
>> +}
>> +
>> +static void mchp_ipc_rproc_rx_callback(struct mbox_client *mbox_client, void *msg)
>> +{
>> +     struct rproc *rproc = dev_get_drvdata(mbox_client->dev);
>> +     struct mchp_ipc_rproc *priv = rproc->priv;
>> +
>> +     priv->message = *(struct mchp_ipc_msg *)msg;
>> +     queue_work(priv->workqueue, &priv->rproc_work);
>> +}
>> +
>> +static int mchp_ipc_rproc_mbox_init(struct rproc *rproc)
>> +{
>> +     struct mchp_ipc_rproc *priv = rproc->priv;
>> +     struct device *dev = priv->dev;
>> +     struct mchp_ipc_sbi_chan *chan_priv;
>> +     struct mbox_client *mbox_client;
>> +
>> +     mbox_client = &priv->mbox_client;
>> +     mbox_client->dev = dev;
>> +     mbox_client->tx_block = true;
>> +     mbox_client->tx_tout = 100;
>> +     mbox_client->knows_txdone = false;
>> +     mbox_client->rx_callback = mchp_ipc_rproc_rx_callback;
>> +
>> +     priv->mbox_channel = mbox_request_channel(mbox_client, 0);
>> +     if (IS_ERR(priv->mbox_channel))
>> +             return dev_err_probe(mbox_client->dev,
>> +                                  PTR_ERR(priv->mbox_channel),
>> +                                  "failed to request mailbox channel\n");
>> +
>> +     chan_priv = (struct mchp_ipc_sbi_chan *)priv->mbox_channel->con_priv;
> Where is the memory for ->con_priv allocated?
The ->con_priv field is allocated in the Microchip IPC mailbox driver’s 
probe
function (drivers/mailbox/mailbox-mchp-ipc-sbi.c).
This remoteproc driver uses it to retrieve the channel id from the mailbox,
which is needed to communicate with the remote processor via the SBI 
ecall interface.
>> +     priv->chan_id = chan_priv->id;
>> +
>> +     return 0;
>> +}
>> +
>> +static int mchp_ipc_rproc_get_state(u32 chan)
>> +{
>> +     struct sbiret ret;
>> +
>> +     ret = sbi_ecall(SBI_EXT_MICROCHIP_TECHNOLOGY, SBI_EXT_RPROC_STATE,
>> +                     chan, 0, 0, 0, 0, 0);
>> +
>> +     if (ret.error)
>> +             return sbi_err_map_linux_errno(ret.error);
>> +
>> +     return ret.value;
>> +}
>> +
>> +static int mchp_ipc_rproc_probe(struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct device_node *np = dev->of_node;
>> +     struct mchp_ipc_rproc *priv;
>> +     struct rproc *rproc;
>> +     struct resource res;
>> +     int num_rmems, ret;
>> +
>> +     rproc = devm_rproc_alloc(dev, np->name, &mchp_ipc_rproc_ops,
>> +                              NULL, sizeof(*priv));
>> +     if (!rproc)
>> +             return -ENOMEM;
>> +
>> +     priv = rproc->priv;
>> +     priv->rproc = rproc;
>> +     priv->dev = dev;
>> +
>> +     num_rmems = of_property_count_elems_of_size(np, "memory-region",
>> +                                                 sizeof(phandle));
> Please add a comment to describe why a minimum of 1 memory region is needed.
> Otherwise I have to guess.
>
>> +     if (num_rmems < 1)
>> +             return dev_err_probe(dev, -EINVAL,
>> +                                  "device needs at least one memory regions to be defined\n");
>> +
>> +     ret = of_reserved_mem_region_to_resource_byname(np, "rsc-table", &res);
>> +     if (!ret) {
>> +             priv->rsc_table  = devm_ioremap_resource(dev, &res);
>> +             if (IS_ERR(priv->rsc_table)) {
>> +                     return dev_err_probe(dev, PTR_ERR(priv->rsc_table),
>> +                                          "failed to map resource table\n");
>> +             }
>> +     }
> The above makes me suspicious but I currently don't have time to look in the
> rproc_ops for answers.  You can either spin off a  new revision to address the
> comments already received or wait for me to look at the rproc_ops in January -
> the choice is yours.
Thanks for the early feedback. I’ll send a v3 incorporating all
comments so far, along with the feedback on the bindings, so it can be
reviewed in January.

Thanks,
Valentina
>
> Thanks,
> Mathieu
>
>> +
>> +     priv->workqueue = create_workqueue(dev_name(dev));
>> +     if (!priv->workqueue)
>> +             return dev_err_probe(dev, -ENOMEM, "cannot create workqueue\n");
>> +
>> +     INIT_WORK(&priv->rproc_work, mchp_ipc_rproc_vq_work);
>> +     init_completion(&priv->start_done);
>> +
>> +     ret = mchp_ipc_rproc_mbox_init(rproc);
>> +     if (ret)
>> +             goto err_put_wkq;
>> +
>> +     /*
>> +      * Check if the remote cluster has been booted by another entity
>> +      * (i.e. bootloader) and set rproc state accordingly
>> +      */
>> +     rproc->state = mchp_ipc_rproc_get_state(priv->chan_id);
>> +     if (ret < 0) {
>> +             dev_err_probe(dev, ret, "Couldn't get cluster boot mode\n");
>> +             goto err_put_mbox;
>> +     }
>> +
>> +     if (rproc->state != RPROC_DETACHED)
>> +             rproc->auto_boot = false;
>> +
>> +     /* error recovery is not supported at present */
>> +     rproc->recovery_disabled = true;
>> +
>> +     dev_set_drvdata(dev, rproc);
>> +
>> +     ret = devm_rproc_add(dev, rproc);
>> +     if (ret) {
>> +             dev_err_probe(dev, ret, "rproc_add failed\n");
>> +             goto err_put_mbox;
>> +     }
>> +
>> +     return 0;
>> +
>> +err_put_mbox:
>> +     mbox_free_channel(priv->mbox_channel);
>> +err_put_wkq:
>> +     destroy_workqueue(priv->workqueue);
>> +
>> +     return ret;
>> +}
>> +
>> +static void mchp_ipc_rproc_remove(struct platform_device *pdev)
>> +{
>> +     struct rproc *rproc = platform_get_drvdata(pdev);
>> +     struct mchp_ipc_rproc *priv = rproc->priv;
>> +
>> +     mbox_free_channel(priv->mbox_channel);
>> +     destroy_workqueue(priv->workqueue);
>> +}
>> +
>> +static const struct of_device_id mchp_ipc_rproc_of_match[] __maybe_unused = {
>> +     { .compatible = "microchip,ipc-sbi-remoteproc", },
>> +     {}
>> +};
>> +MODULE_DEVICE_TABLE(of, mchp_ipc_rproc_of_match);
>> +
>> +static struct platform_driver mchp_ipc_rproc_driver = {
>> +     .probe = mchp_ipc_rproc_probe,
>> +     .remove = mchp_ipc_rproc_remove,
>> +     .driver = {
>> +             .name = "microchip-ipc-rproc",
>> +             .of_match_table = of_match_ptr(mchp_ipc_rproc_of_match),
>> +     },
>> +};
>> +
>> +module_platform_driver(mchp_ipc_rproc_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Valentina Fernandez<valentina.fernandezalanis@microchip.com>");
>> +MODULE_DESCRIPTION("Microchip IPC Remote Processor control driver");
>> --
>> 2.34.1
>>


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

* Re: [PATCH v2 1/2] dt-bindings: remoteproc: add Microchip IPC remoteproc
  2025-11-21 14:21 ` [PATCH v2 1/2] dt-bindings: remoteproc: add Microchip IPC remoteproc Valentina Fernandez
  2025-11-21 18:28   ` Conor Dooley
  2025-11-25  9:46   ` Krzysztof Kozlowski
@ 2025-12-13  5:42   ` Tanmay Shah
  2 siblings, 0 replies; 11+ messages in thread
From: Tanmay Shah @ 2025-12-13  5:42 UTC (permalink / raw)
  To: Valentina Fernandez, andersson, mathieu.poirier, robh, krzk+dt,
	conor+dt
  Cc: linux-remoteproc, devicetree, linux-kernel


Hello, Please find my comment below:

On 11/21/25 8:21 AM, Valentina Fernandez wrote:
> Microchip family of RISC-V SoCs typically have one or more application
> clusters. These clusters can be configured to run in an Asymmetric
> Multi Processing (AMP) mode.
> 
> Add a dt-binding for these application clusters.
> 
> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> ---
>  .../microchip,ipc-sbi-remoteproc.yaml         | 95 +++++++++++++++++++
>  1 file changed, 95 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
> new file mode 100644
> index 000000000000..348902f9a202
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-sbi-remoteproc.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-sbi-remoteproc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip IPC Remote Processor
> +
> +description:
> +  Microchip family of RISC-V SoCs typically have one or more
> +  clusters. These clusters can be configured to run in an Asymmetric
> +  Multi Processing (AMP) mode where clusters are split in independent
> +  software contexts.
> +
> +maintainers:
> +  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> +
> +properties:
> +  compatible:
> +    const: microchip,ipc-sbi-remoteproc
> +
> +  mboxes:
> +    description:
> +      Microchip IPC mailbox specifier. To be used for communication with
> +      a remote cluster. The specifier format is as per the bindings,
> +      Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> +    maxItems: 1
> +
> +  memory-region:
> +    minItems: 1
> +    maxItems: 5
> +    description:
> +      List of phandles to the reserved memory regions associated wih the remoteproc
> +      device. This is variable and describes the memories shared with the remote cluster
> +      (e.g. firmware, resource table, rpmsg vrings, etc.)
> +    items:
> +      anyOf:
> +        - description: region used for the resource table when firmware is started by the bootloader
> +        - description: region used for the remote cluster firmware image section
> +        - description: virtio device (vdev) buffer
> +        - description: virtqueue for sending messages to the remote cluster (vring0)

This is in-accurate as per the implementation:
https://github.com/torvalds/linux/blob/a919610db43b34621d0c3b333e12db9002caf5da/drivers/rpmsg/virtio_rpmsg_bus.c#L878

Also the implementation can be changed. The description doesn't need to mention
if vring0 is used for rx or tx.

> +        - description: virtqueue for receiving messages from the remote cluster (vring1)

Same here.

> +
> +  memory-region-names:
> +    minItems: 1
> +    maxItems: 5
> +    items:
> +      anyOf:
> +        - const: rsc-table
> +        - const: firmware
> +        - const: buffer
> +        - const: vring0
> +        - const: vring1
> +
> +required:
> +  - compatible
> +  - mboxes
> +  - memory-region
> +  - memory-region-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    // Early boot mode example - firmware started by bootloader
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        remoteproc {
> +            compatible = "microchip,ipc-sbi-remoteproc";
> +            mboxes= <&ihc 8>;

In the driver, this "mboxes" id is used for powering on/off remote processor.

I think, "power-domains" is more suitable property over "mboxes" for this purpose.

It is possible to only load, start and stop remote processor without any
communication. So ideally "mboxes" can be optional, but in this case it can't be
because remote's power-domain id is used from "mboxes" id. Even if both are the
same number, they should be different properties and should be used for
different purpose.

Thanks,
Tanmay

> +            memory-region = <&rsctable>, <&vdev0buffer>,
> +                            <&vdev0vring0>, <&vdev0vring1>;
> +            memory-region-names = "rsc-table", "buffer",
> +                                  "vring0", "vring1";
> +        };
> +    };
> +
> +  - |
> +    // Late boot mode example - firmware started by Linux (remoteproc)
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        remoteproc {
> +            compatible = "microchip,ipc-sbi-remoteproc";
> +            mboxes= <&ihc 8>;
> +            memory-region = <&cluster_firmware>, <&vdev0buffer>,
> +                            <&vdev0vring0>, <&vdev0vring1>;
> +            memory-region-names = "firmware", "buffer",
> +                                  "vring0", "vring1";
> +        };
> +    };
> +...


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

end of thread, other threads:[~2025-12-13  5:42 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-21 14:21 [PATCH v2 0/2] Add Microchip IPC remoteproc support Valentina Fernandez
2025-11-21 14:21 ` [PATCH v2 1/2] dt-bindings: remoteproc: add Microchip IPC remoteproc Valentina Fernandez
2025-11-21 18:28   ` Conor Dooley
2025-12-01 16:16     ` Valentina.FernandezAlanis
2025-11-25  9:46   ` Krzysztof Kozlowski
2025-12-01 16:04     ` Valentina.FernandezAlanis
2025-12-01 16:22       ` Krzysztof Kozlowski
2025-12-13  5:42   ` Tanmay Shah
2025-11-21 14:21 ` [PATCH v2 2/2] remoteproc: add support for Microchip IPC remoteproc platform driver Valentina Fernandez
2025-12-09 17:33   ` Mathieu Poirier
2025-12-10 18:37     ` Valentina.FernandezAlanis

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).