[PATCH v1 0/5] Add Microchip IPC mailbox and remoteproc support

[PATCH v1 0/5] Add Microchip IPC mailbox and remoteproc support

[Valentina Fernandez]

Hello all,

This series adds support for the Microchip Inter-Processor Communication
(IPC) mailbox controller, as well as an 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 Microchip IPC is used to send messages between processors using
an interrupt signaling mechanism. The driver uses the RISC-V Supervisor
Binary Interface (SBI) to communicate with software running in machine
mode (M-mode) to access the IPC hardware block.

Additional details on the Microchip vendor extension and the IPC
function IDs described in the driver can be found in the following
documentation:

https://github.com/linux4microchip/microchip-sbi-ecall-extension

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.

The PIC64GX MPU has a Mi-V IHC block, this will be added to the PIC64GX
dts after the initial upstreaming:

https://patchwork.kernel.org/project/linux-riscv/patch/20240725121609.13101-18-pierre-henry.moussay@microchip.com/

Thanks,
Valentina

Valentina Fernandez (5):
  riscv: asm: vendorid_list: Add Microchip Technology to the vendor list
  dt-bindings: mailbox: add binding for Microchip IPC mailbox driver
  mailbox: add Microchip IPC support
  dt-bindings: remoteproc: add binding for Microchip IPC remoteproc
  remoteproc: add support for Microchip IPC remoteproc platform driver

 .../bindings/mailbox/microchip,sbi-ipc.yaml   | 115 ++++
 .../remoteproc/microchip,ipc-remoteproc.yaml  |  84 +++
 arch/riscv/include/asm/vendorid_list.h        |   1 +
 drivers/mailbox/Kconfig                       |  12 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/mailbox-mchp-ipc-sbi.c        | 539 ++++++++++++++++++
 drivers/remoteproc/Kconfig                    |  12 +
 drivers/remoteproc/Makefile                   |   1 +
 drivers/remoteproc/mchp_ipc_remoteproc.c      | 461 +++++++++++++++
 include/linux/mailbox/mchp-ipc.h              |  23 +
 10 files changed, 1250 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
 create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
 create mode 100644 drivers/mailbox/mailbox-mchp-ipc-sbi.c
 create mode 100644 drivers/remoteproc/mchp_ipc_remoteproc.c
 create mode 100644 include/linux/mailbox/mchp-ipc.h

-- 
2.34.1

[PATCH v1 1/5] riscv: asm: vendorid_list: Add Microchip Technology to the vendor list

[Valentina Fernandez]

Add Microchip Technology to the RISC-V vendor list.

Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
---
 arch/riscv/include/asm/vendorid_list.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/include/asm/vendorid_list.h b/arch/riscv/include/asm/vendorid_list.h
index 2f2bb0c84f9a..a5150cdf34d8 100644
--- a/arch/riscv/include/asm/vendorid_list.h
+++ b/arch/riscv/include/asm/vendorid_list.h
@@ -6,6 +6,7 @@
 #define ASM_VENDOR_LIST_H
 
 #define ANDES_VENDOR_ID		0x31e
+#define MICROCHIP_VENDOR_ID	0x029
 #define SIFIVE_VENDOR_ID	0x489
 #define THEAD_VENDOR_ID		0x5b7
 
-- 
2.34.1

[PATCH v1 2/5] dt-bindings: mailbox: add binding for Microchip IPC mailbox driver

[Valentina Fernandez]

Add a dt-binding for the Microchip Inter-Processor Communication (IPC)
mailbox controller.

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

diff --git a/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml b/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
new file mode 100644
index 000000000000..dc2cbd5eb28f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
@@ -0,0 +1,115 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/microchip,sbi-ipc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip Inter-processor communication (IPC) mailbox controller
+
+maintainers:
+  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
+
+description:
+  The Microchip Inter-processor Communication (IPC) facilitates
+  message passing between processors using an interrupt signaling
+  mechanism.
+  This SBI interface is compatible with the Mi-V Inter-hart
+  Communication (IHC) IP.
+  The microchip,sbi-ipc compatible string is inteded for use by software
+  running in supervisor privileged mode (s-mode). The SoC-specific
+  compatibles are inteded for use by the SBI implementation in machine
+  mode (m-mode).
+
+properties:
+  compatible:
+    enum:
+      - microchip,sbi-ipc
+      - microchip,miv-ihc-rtl-v2
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 1
+    maxItems: 5
+
+  interrupt-names:
+    minItems: 1
+    maxItems: 5
+
+  "#mbox-cells":
+    description:
+      For the SBI "device", the cell represents the global "logical" channel IDs.
+      The meaning of channel IDs are platform firmware dependent. The
+      SoC-specific compatibles are intended for use by the SBI implementation,
+      rather than s-mode software. There the cell would represent the physical
+      channel and do not vary depending on platform firmware.
+    const: 1
+
+  microchip,ihc-chan-disabled-mask:
+    description:
+      Represents the enable/disable state of the bi-directional IHC channels
+      within the MIV-IHC IP configuration. The mask is a 16-bit value, but only
+      the first 15 bits are utilized.Each of the bits corresponds to
+      one of the 15 IHC channels.
+      A bit set to '1' indicates that the corresponding channel is disabled,
+      and any read or write operations to that channel will return zero.
+      A bit set to '0' indicates that the corresponding channel is enabled
+      and will be accessible through its dedicated address range registers.
+      The remaining bit of the 16-bit mask is reserved and should be ignored.
+      The actual enable/disable state of each channel is determined by the
+      IP block’s configuration.
+    $ref: /schemas/types.yaml#/definitions/uint16
+    default: 0
+
+required:
+  - compatible
+  - interrupts
+  - interrupt-names
+  - "#mbox-cells"
+
+additionalProperties: false
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: microchip,sbi-ipc
+    then:
+      properties:
+        reg: false
+    else:
+      required:
+        - reg
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: microchip,miv-ihc-rtl-v2
+    then:
+      properties:
+        interrupt-names:
+          items:
+            pattern: "^hart-[0-5]+$"
+
+examples:
+  - |
+    mailbox {
+      compatible = "microchip,sbi-ipc";
+      interrupt-parent = <&plic>;
+      interrupts = <180>, <179>, <178>;
+      interrupt-names = "hart-1", "hart-2", "hart-3";
+      #mbox-cells = <1>;
+    };
+  - |
+    mailbox@50000000 {
+      compatible = "microchip,miv-ihc-rtl-v2";
+      microchip,ihc-chan-disabled-mask=  /bits/ 16 <0>;
+      reg = <0x50000000 0x1C000>;
+      interrupt-parent = <&plic>;
+      interrupts = <180>, <179>, <178>;
+      interrupt-names = "hart-1", "hart-2", "hart-3";
+      #mbox-cells = <1>;
+    };
-- 
2.34.1

[PATCH v1 3/5] mailbox: add Microchip IPC support

[Valentina Fernandez]

Add a mailbox controller driver for the Microchip Inter-processor
Communication (IPC), which is used to send and receive data between
processors.

The driver uses the RISC-V Supervisor Binary Interface (SBI) to
communicate with software running in machine mode (M-mode) to access
the IPC hardware block.

Additional details on the Microchip vendor extension and the IPC
function IDs described in the driver can be found in the following
documentation:

https://github.com/linux4microchip/microchip-sbi-ecall-extension

This SBI interface in this driver is compatible with the Mi-V Inter-hart
Communication (IHC) IP.

Transmitting and receiving data through the mailbox framework is done
through struct mchp_ipc_msg.

Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
---
 drivers/mailbox/Kconfig                |  12 +
 drivers/mailbox/Makefile               |   2 +
 drivers/mailbox/mailbox-mchp-ipc-sbi.c | 539 +++++++++++++++++++++++++
 include/linux/mailbox/mchp-ipc.h       |  23 ++
 4 files changed, 576 insertions(+)
 create mode 100644 drivers/mailbox/mailbox-mchp-ipc-sbi.c
 create mode 100644 include/linux/mailbox/mchp-ipc.h

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 4eed97295927..7bb4190d465e 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -176,6 +176,18 @@ config POLARFIRE_SOC_MAILBOX
 
 	  If unsure, say N.
 
+config MCHP_SBI_IPC_MBOX
+	tristate "Microchip Inter-processor Communication (IPC) SBI driver"
+	depends on RISCV_SBI
+	help
+	  Mailbox implementation for Microchip devices with an
+	  Inter-process communication (IPC) controller.
+
+	  To compile this driver as a module, choose M here. the
+	  module will be called mailbox-mchp-ipc-sbi.
+
+	  If unsure, say N.
+
 config QCOM_APCS_IPC
 	tristate "Qualcomm APCS IPC driver"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 3c3c27d54c13..a78d1948e331 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -45,6 +45,8 @@ obj-$(CONFIG_BCM_FLEXRM_MBOX)	+= bcm-flexrm-mailbox.o
 
 obj-$(CONFIG_POLARFIRE_SOC_MAILBOX)	+= mailbox-mpfs.o
 
+obj-$(CONFIG_MCHP_SBI_IPC_MBOX)		+= mailbox-mchp-ipc-sbi.o
+
 obj-$(CONFIG_QCOM_APCS_IPC)	+= qcom-apcs-ipc-mailbox.o
 
 obj-$(CONFIG_TEGRA_HSP_MBOX)	+= tegra-hsp.o
diff --git a/drivers/mailbox/mailbox-mchp-ipc-sbi.c b/drivers/mailbox/mailbox-mchp-ipc-sbi.c
new file mode 100644
index 000000000000..c0cdb55c8a79
--- /dev/null
+++ b/drivers/mailbox/mailbox-mchp-ipc-sbi.c
@@ -0,0 +1,539 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Microchip Inter-Processor communication (IPC) driver
+ *
+ * Copyright (c) 2021 - 2024 Microchip Technology Inc. All rights reserved.
+ *
+ * Author: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
+ *
+ */
+
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/smp.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/of_device.h>
+#include <linux/interrupt.h>
+#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
+#include <linux/mailbox/mchp-ipc.h>
+#include <asm/sbi.h>
+#include <asm/vendorid_list.h>
+
+#define IRQ_STATUS_BITS			12
+#define NUM_CHANS_PER_CLUSTER		5
+#define IPC_DMA_BIT_MASK		32
+#define SBI_EXT_MICROCHIP_TECHNOLOGY	(SBI_EXT_VENDOR_START | \
+					 MICROCHIP_VENDOR_ID)
+
+enum {
+	SBI_EXT_IPC_PROBE = 0x100,
+	SBI_EXT_IPC_CH_INIT,
+	SBI_EXT_IPC_SEND,
+	SBI_EXT_IPC_RECEIVE,
+	SBI_EXT_IPC_STATUS,
+};
+
+enum ipc_hw {
+	MIV_IHC,
+};
+
+enum ipc_irq_type {
+	IPC_OPS_NOT_SUPPORTED	= 1,
+	IPC_MP_IRQ		= 2,
+	IPC_MC_IRQ		= 4,
+};
+
+/**
+ * struct mchp_ipc_probe - IPC probe message format
+ *
+ * @hw_type:		IPC implementation available in the hardware
+ * @num_channels:	number of IPC channels available in the hardware
+ *
+ * Used to retrieve information on the IPC implementation
+ * using the SBI_EXT_IPC_PROBE SBI function id.
+ */
+struct mchp_ipc_probe {
+	enum ipc_hw hw_type;
+	u8 num_channels;
+};
+
+/**
+ * struct mchp_ipc_init - IPC channel init message format
+ *
+ * @max_msg_size:	maxmimum message size in bytes of a given channel
+ *
+ * struct used by the SBI_EXT_IPC_CH_INIT SBI function id to get
+ * the max message size in bytes of the initialized channel.
+ */
+struct mchp_ipc_init {
+	u16 max_msg_size;
+};
+
+/**
+ * struct mchp_ipc_status - IPC status message format
+ *
+ * @status:	interrupt status for all channels associated to a cluster
+ * @cluster:	specifies the cluster instance that originated an irq
+ *
+ * struct used by the SBI_EXT_IPC_STATUS SBI function id to get
+ * the message present and message clear interrupt status for all the
+ * channels associated to a cluster.
+ */
+struct mchp_ipc_status {
+	u32 status;
+	u8 cluster;
+};
+
+/**
+ * struct mchp_ipc_sbi_msg - IPC SBI payload message
+ *
+ * @buf_addr:	physical address where the received data should be copied to
+ * @size:	maximum size(in bytes) that can be stored in the buffer pointed to by `buf`
+ * @irq_type:	mask representing the irq types that triggered an irq
+ *
+ * struct used by the SBI_EXT_IPC_SEND/SBI_EXT_IPC_RECEIVE SBI function
+ * ids to send/receive a message from an associated processor using
+ * the IPC.
+ */
+struct mchp_ipc_sbi_msg {
+	u64 buf_addr;
+	u16 size;
+	u8 irq_type;
+};
+
+struct mchp_ipc_cluster_cfg {
+	void *buf_base;
+	dma_addr_t dma_addr;
+	int irq;
+};
+
+struct ipc_chan_info {
+	void *buf_base_tx;
+	void *buf_base_rx;
+	void *msg_buf_tx;
+	void *msg_buf_rx;
+	dma_addr_t dma_addr_tx;
+	dma_addr_t dma_addr_rx;
+	dma_addr_t msg_buf_dma_tx;
+	dma_addr_t msg_buf_dma_rx;
+	int chan_aggregated_irq;
+	int mp_irq;
+	int mc_irq;
+	u32 id;
+	u32 max_msg_size;
+};
+
+struct microchip_ipc {
+	struct device *dev;
+	struct mbox_chan *chans;
+	struct mchp_ipc_cluster_cfg *cluster_cfg;
+	struct ipc_chan_info *priv;
+	void *buf_base;
+	dma_addr_t dma_addr;
+	struct mbox_controller controller;
+	u8 num_channels;
+	enum ipc_hw hw_type;
+};
+
+static int mchp_ipc_sbi_chan_send(u32 command, u32 channel, dma_addr_t address)
+{
+	struct sbiret ret;
+
+	ret = sbi_ecall(SBI_EXT_MICROCHIP_TECHNOLOGY, command, channel,
+			address, 0, 0, 0, 0);
+
+	if (ret.error)
+		return sbi_err_map_linux_errno(ret.error);
+	else
+		return ret.value;
+}
+
+static int mchp_ipc_sbi_send(u32 command, dma_addr_t address)
+{
+	struct sbiret ret;
+
+	ret = sbi_ecall(SBI_EXT_MICROCHIP_TECHNOLOGY, command, address,
+			0, 0, 0, 0, 0);
+
+	if (ret.error)
+		return sbi_err_map_linux_errno(ret.error);
+	else
+		return ret.value;
+}
+
+static struct microchip_ipc *to_mchp_ipc_mbox(struct mbox_controller *mbox)
+{
+	return container_of(mbox, struct microchip_ipc, controller);
+}
+
+u32 mchp_ipc_get_chan_id(struct mbox_chan *chan)
+{
+	struct ipc_chan_info *chan_info = (struct ipc_chan_info *)chan->con_priv;
+
+	return chan_info->id;
+}
+EXPORT_SYMBOL(mchp_ipc_get_chan_id);
+
+static inline void mchp_ipc_prepare_receive_req(struct mbox_chan *chan)
+{
+	struct ipc_chan_info *chan_info = (struct ipc_chan_info *)chan->con_priv;
+	struct mchp_ipc_sbi_msg request;
+
+	request.buf_addr = chan_info->msg_buf_dma_rx;
+	request.size = chan_info->max_msg_size;
+	memcpy(chan_info->buf_base_rx, &request, sizeof(struct mchp_ipc_sbi_msg));
+}
+
+static inline void mchp_ipc_process_received_data(struct mbox_chan *chan,
+						  struct mchp_ipc_msg *ipc_msg)
+{
+	struct ipc_chan_info *chan_info = (struct ipc_chan_info *)chan->con_priv;
+	struct mchp_ipc_sbi_msg sbi_msg;
+
+	memcpy(&sbi_msg, chan_info->buf_base_rx, sizeof(struct mchp_ipc_sbi_msg));
+	ipc_msg->buf = (u32 *)chan_info->msg_buf_rx;
+	ipc_msg->size = sbi_msg.size;
+}
+
+static irqreturn_t mchp_ipc_cluster_aggr_isr(int irq, void *data)
+{
+	struct mbox_chan *chan;
+	struct ipc_chan_info *chan_info;
+	struct microchip_ipc *ipc = (struct microchip_ipc *)data;
+	struct mchp_ipc_msg ipc_msg;
+	struct mchp_ipc_status status_msg;
+	int ret;
+	unsigned long hartid;
+	u32 i, chan_index, chan_id;
+
+	/* Find out the hart that originated the irq */
+	for_each_online_cpu(i) {
+		hartid = cpuid_to_hartid_map(i);
+		if (irq == ipc->cluster_cfg[hartid].irq)
+			break;
+	}
+
+	status_msg.cluster = hartid;
+	memcpy(ipc->cluster_cfg[hartid].buf_base, &status_msg, sizeof(struct mchp_ipc_status));
+
+	ret = mchp_ipc_sbi_send(SBI_EXT_IPC_STATUS, ipc->cluster_cfg[hartid].dma_addr);
+	if (ret < 0) {
+		dev_err_ratelimited(ipc->dev, "could not get IHC irq status ret=%d\n", ret);
+		return IRQ_HANDLED;
+	}
+
+	memcpy(&status_msg, ipc->cluster_cfg[hartid].buf_base, sizeof(struct mchp_ipc_status));
+
+	/*
+	 * Iterate over each bit set in the IHC interrupt status register (IRQ_STATUS) to identify
+	 * the channel(s) that have a message to be processed/acknowledged.
+	 * The bits are organized in alternating format, where each pair of bits represents
+	 * the status of the message present and message clear interrupts for each cluster/hart
+	 * (from hart 0 to hart 5). Each cluster can have up to 5 fixed channels associated.
+	 */
+
+	for_each_set_bit(i, (unsigned long *)&status_msg.status, IRQ_STATUS_BITS) {
+		/* Find out the destination hart that triggered the interrupt */
+		chan_index = i / 2;
+
+		/*
+		 * The IP has no loopback channels, so we need to decrement the index when
+		 * the target hart has a greater index than our own
+		 */
+		if (chan_index >= status_msg.cluster)
+			chan_index--;
+
+		/*
+		 * Calculate the channel id given the hart and channel index. Channel IDs
+		 * are unique across all clusters of an IPC, and iterate contiguously
+		 * across all clusters.
+		 */
+		chan_id = status_msg.cluster * (NUM_CHANS_PER_CLUSTER + chan_index);
+
+		chan = &ipc->chans[chan_id];
+		chan_info = (struct ipc_chan_info *)chan->con_priv;
+
+		if (i % 2 == 0) {
+			mchp_ipc_prepare_receive_req(chan);
+			ret = mchp_ipc_sbi_chan_send(SBI_EXT_IPC_RECEIVE, chan_id,
+						     chan_info->dma_addr_rx);
+			if (ret < 0)
+				continue;
+
+			mchp_ipc_process_received_data(chan, &ipc_msg);
+			mbox_chan_received_data(&ipc->chans[chan_id], (void *)&ipc_msg);
+
+		} else {
+			ret = mchp_ipc_sbi_chan_send(SBI_EXT_IPC_RECEIVE, chan_id,
+						     chan_info->dma_addr_rx);
+			mbox_chan_txdone(&ipc->chans[chan_id], ret);
+		}
+	}
+	return IRQ_HANDLED;
+}
+
+static int mchp_ipc_get_cluster_aggr_irq(struct microchip_ipc *ipc)
+{
+	struct platform_device *pdev = to_platform_device(ipc->dev);
+	char *irq_name;
+	int cpuid, ret;
+	unsigned long hartid;
+	bool irq_found = false;
+
+	for_each_online_cpu(cpuid) {
+		hartid = cpuid_to_hartid_map(cpuid);
+		irq_name = devm_kasprintf(ipc->dev, GFP_KERNEL, "hart-%lu", hartid);
+		ret = platform_get_irq_byname_optional(pdev, irq_name);
+		if (ret <= 0)
+			continue;
+
+		ipc->cluster_cfg[hartid].irq = ret;
+		ret = devm_request_irq(ipc->dev, ipc->cluster_cfg[hartid].irq,
+				       mchp_ipc_cluster_aggr_isr, IRQF_SHARED,
+				       "miv-ihc-irq", ipc);
+		if (ret)
+			return ret;
+
+		ipc->cluster_cfg[hartid].buf_base = dmam_alloc_coherent(ipc->dev,
+									sizeof(struct mchp_ipc_status),
+									&ipc->cluster_cfg[hartid].dma_addr,
+									GFP_KERNEL);
+
+		if (!ipc->cluster_cfg[hartid].buf_base)
+			return -ENOMEM;
+
+		irq_found = true;
+	}
+
+	return irq_found;
+}
+
+static int mchp_ipc_send_data(struct mbox_chan *chan, void *data)
+{
+	struct ipc_chan_info *chan_info = (struct ipc_chan_info *)chan->con_priv;
+	const struct mchp_ipc_msg *msg = data;
+	struct mchp_ipc_sbi_msg sbi_payload;
+
+	memcpy(chan_info->msg_buf_tx, msg->buf, msg->size);
+	sbi_payload.buf_addr = chan_info->msg_buf_dma_tx;
+	sbi_payload.size = msg->size;
+	memcpy(chan_info->buf_base_tx, &sbi_payload, sizeof(sbi_payload));
+
+	return mchp_ipc_sbi_chan_send(SBI_EXT_IPC_SEND, chan_info->id, chan_info->dma_addr_tx);
+}
+
+static int mchp_ipc_startup(struct mbox_chan *chan)
+{
+	struct ipc_chan_info *chan_info = (struct ipc_chan_info *)chan->con_priv;
+	struct microchip_ipc *ipc = to_mchp_ipc_mbox(chan->mbox);
+	struct mchp_ipc_init ch_init_msg;
+	int ret;
+
+	chan_info->buf_base_tx = dma_alloc_coherent(ipc->dev, sizeof(struct mchp_ipc_sbi_msg),
+						    &chan_info->dma_addr_tx, GFP_KERNEL);
+	if (!chan_info->buf_base_tx) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+
+	chan_info->buf_base_rx = dma_alloc_coherent(ipc->dev, sizeof(struct mchp_ipc_sbi_msg),
+						    &chan_info->dma_addr_rx, GFP_KERNEL);
+	if (!chan_info->buf_base_rx) {
+		ret = -ENOMEM;
+		goto fail_free_buf_base_tx;
+	}
+
+	ret = mchp_ipc_sbi_chan_send(SBI_EXT_IPC_CH_INIT, chan_info->id, chan_info->dma_addr_tx);
+	if (ret < 0) {
+		dev_err(ipc->dev, "channel %u init failed\n", chan_info->id);
+		goto fail_free_buf_base_rx;
+	}
+
+	memcpy(&ch_init_msg, chan_info->buf_base_tx, sizeof(struct mchp_ipc_init));
+	chan_info->max_msg_size = ch_init_msg.max_msg_size;
+
+	chan_info->msg_buf_tx = dma_alloc_coherent(ipc->dev, chan_info->max_msg_size,
+						   &chan_info->msg_buf_dma_tx, GFP_KERNEL);
+	if (!chan_info->msg_buf_tx) {
+		ret = -ENOMEM;
+		goto fail_free_buf_base_rx;
+	}
+
+	chan_info->msg_buf_rx = dma_alloc_coherent(ipc->dev, chan_info->max_msg_size,
+						   &chan_info->msg_buf_dma_rx, GFP_KERNEL);
+	if (!chan_info->msg_buf_rx) {
+		ret = -ENOMEM;
+		goto fail_free_buf_msg_tx;
+	}
+
+	switch (ipc->hw_type) {
+	case MIV_IHC:
+		return 0;
+	default:
+		goto fail_free_buf_msg_rx;
+	}
+
+	if (ret) {
+		dev_err(ipc->dev, "failed to register interrupt(s)\n");
+		goto fail_free_buf_msg_rx;
+	}
+
+	return ret;
+
+fail_free_buf_msg_rx:
+	dma_free_coherent(ipc->dev, chan_info->max_msg_size,
+			  chan_info->msg_buf_rx, chan_info->msg_buf_dma_rx);
+fail_free_buf_msg_tx:
+	dma_free_coherent(ipc->dev, chan_info->max_msg_size,
+			  chan_info->msg_buf_tx, chan_info->msg_buf_dma_tx);
+fail_free_buf_base_rx:
+	dma_free_coherent(ipc->dev, sizeof(struct mchp_ipc_sbi_msg),
+			  chan_info->buf_base_rx, chan_info->dma_addr_rx);
+fail_free_buf_base_tx:
+	dma_free_coherent(ipc->dev, sizeof(struct mchp_ipc_sbi_msg),
+			  chan_info->buf_base_tx, chan_info->dma_addr_tx);
+fail:
+	return ret;
+}
+
+static void mchp_ipc_shutdown(struct mbox_chan *chan)
+{
+	struct ipc_chan_info *chan_info = (struct ipc_chan_info *)chan->con_priv;
+	struct microchip_ipc *ipc = to_mchp_ipc_mbox(chan->mbox);
+
+	dma_free_coherent(ipc->dev, chan_info->max_msg_size,
+			  chan_info->msg_buf_tx, chan_info->msg_buf_dma_tx);
+
+	dma_free_coherent(ipc->dev, chan_info->max_msg_size,
+			  chan_info->msg_buf_rx, chan_info->msg_buf_dma_rx);
+
+	dma_free_coherent(ipc->dev, sizeof(struct mchp_ipc_sbi_msg),
+			  chan_info->buf_base_tx, chan_info->dma_addr_tx);
+
+	dma_free_coherent(ipc->dev, sizeof(struct mchp_ipc_sbi_msg),
+			  chan_info->buf_base_rx, chan_info->dma_addr_rx);
+}
+
+static const struct mbox_chan_ops mchp_ipc_ops = {
+	.startup = mchp_ipc_startup,
+	.send_data = mchp_ipc_send_data,
+	.shutdown = mchp_ipc_shutdown,
+};
+
+static struct mbox_chan *mchp_ipc_mbox_xlate(struct mbox_controller *controller,
+					     const struct of_phandle_args *spec)
+{
+	struct microchip_ipc *ipc = to_mchp_ipc_mbox(controller);
+	unsigned int chan_id = spec->args[0];
+
+	if (chan_id >= ipc->num_channels) {
+		dev_err(ipc->dev, "invalid channel id %d\n", chan_id);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return &ipc->chans[chan_id];
+}
+
+static const struct of_device_id mchp_ipc_of_match[] = {
+	{.compatible = "microchip,sbi-ipc", },
+	{}
+};
+
+static int mchp_ipc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mchp_ipc_probe ipc_info;
+	struct microchip_ipc *ipc;
+	struct ipc_chan_info *priv;
+	bool irq_avail = false;
+	int ret;
+	u32 chan_id;
+
+	ret = sbi_probe_extension(SBI_EXT_MICROCHIP_TECHNOLOGY);
+	if (ret <= 0)
+		return dev_err_probe(dev, ret, "Microchip SBI extension not detected\n");
+
+	ipc = devm_kzalloc(dev, sizeof(*ipc), GFP_KERNEL);
+	if (!ipc)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, ipc);
+
+	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(IPC_DMA_BIT_MASK));
+	if (ret)
+		return dev_err_probe(dev, ret, "dma_set_mask_and_coherent failed\n");
+
+	ipc->buf_base = dmam_alloc_coherent(dev, sizeof(u32), &ipc->dma_addr, GFP_KERNEL);
+
+	if (!ipc->buf_base)
+		return -ENOMEM;
+
+	ret = mchp_ipc_sbi_send(SBI_EXT_IPC_PROBE, ipc->dma_addr);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "could not probe IPC SBI service\n");
+
+	memcpy(&ipc_info, ipc->buf_base, sizeof(struct mchp_ipc_probe));
+	ipc->num_channels = ipc_info.num_channels;
+	ipc->hw_type = ipc_info.hw_type;
+
+	ipc->chans = devm_kcalloc(dev, ipc->num_channels, sizeof(*ipc->chans), GFP_KERNEL);
+	if (!ipc->chans)
+		return -ENOMEM;
+
+	ipc->dev = dev;
+	ipc->controller.txdone_irq = true;
+	ipc->controller.dev = ipc->dev;
+	ipc->controller.ops = &mchp_ipc_ops;
+	ipc->controller.chans = ipc->chans;
+	ipc->controller.num_chans = ipc->num_channels;
+	ipc->controller.of_xlate = mchp_ipc_mbox_xlate;
+
+	for (chan_id = 0; chan_id < ipc->num_channels; chan_id++) {
+		priv = devm_kmalloc(dev, sizeof(*priv), GFP_KERNEL);
+		if (!priv)
+			return -ENOMEM;
+
+		ipc->chans[chan_id].con_priv = priv;
+		priv->id = chan_id;
+	}
+
+	if (ipc->hw_type == MIV_IHC) {
+		ipc->cluster_cfg = devm_kcalloc(dev, num_online_cpus(),
+						sizeof(struct mchp_ipc_cluster_cfg),
+						GFP_KERNEL);
+		if (!ipc->cluster_cfg)
+			return -ENOMEM;
+
+		if (mchp_ipc_get_cluster_aggr_irq(ipc))
+			irq_avail = true;
+	}
+
+	if (!irq_avail)
+		return dev_err_probe(dev, -ENODEV, "missing interrupt property\n");
+
+	ret = devm_mbox_controller_register(dev, &ipc->controller);
+	if (ret)
+		return dev_err_probe(dev, ret,
+					 "Inter-Processor communication (IPC) registration failed\n");
+
+	return 0;
+}
+
+MODULE_DEVICE_TABLE(of, mchp_ipc_of_match);
+
+static struct platform_driver mchp_ipc_driver = {
+	.driver = {
+		.name = "microchip_ipc",
+		.of_match_table = of_match_ptr(mchp_ipc_of_match),
+	},
+	.probe = mchp_ipc_probe,
+};
+
+module_platform_driver(mchp_ipc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Valentina Fernandez <valentina.fernandezalanis@microchip.com>");
+MODULE_DESCRIPTION("Microchip Inter-Processor Communication (IPC) driver");
diff --git a/include/linux/mailbox/mchp-ipc.h b/include/linux/mailbox/mchp-ipc.h
new file mode 100644
index 000000000000..69352bcb1402
--- /dev/null
+++ b/include/linux/mailbox/mchp-ipc.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *Copyright (c) 2024 Microchip Technology Inc. All rights reserved.
+ */
+
+#ifndef _LINUX_MCHP_IPC_H_
+#define _LINUX_MCHP_IPC_H_
+
+#include <linux/mailbox_controller.h>
+#include <linux/types.h>
+
+struct mchp_ipc_msg {
+	u32 *buf;
+	u16 size;
+};
+
+#if IS_ENABLED(CONFIG_MCHP_SBI_IPC_MBOX)
+u32 mchp_ipc_get_chan_id(struct mbox_chan *chan);
+#else
+u32 mchp_ipc_get_chan_id(struct mbox_chan *chan) { return 0; }
+#endif
+
+#endif /* _LINUX_MCHP_IPC_H_ */
-- 
2.34.1

[PATCH v1 4/5] dt-bindings: remoteproc: add binding for Microchip IPC remoteproc

[Valentina Fernandez]

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

Add a dt-binding for the Microchip IPC Remoteproc platform driver.

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

diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
new file mode 100644
index 000000000000..1765c68d22cf
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
@@ -0,0 +1,84 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-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.
+
+  This document defines the binding for the remoteproc component that
+  loads and boots firmwares on remote clusters.
+
+  This SBI interface is compatible with the Mi-V Inter-hart
+  Communication (IHC) IP.
+
+maintainers:
+  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
+
+properties:
+  compatible:
+    const: microchip,ipc-remoteproc
+
+  mboxes:
+    description:
+      This property is required only if the rpmsg/virtio functionality is used.
+      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
+
+  microchip,auto-boot:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      If defined, when remoteproc is probed, it loads the default firmware and
+      starts the remote processor.
+
+  microchip,skip-ready-wait:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      If defined, the master processor will not expect a ready signal from the
+      remote processor indicating it has booted successfully. This allows the
+      master processor to proceed with its operations without waiting for
+      confirmation from the remote processor.
+
+  memory-region:
+    description:
+      If present, a phandle for a reserved memory area that used for vdev buffer,
+      resource table, vring region and others used by remote cluster.
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+
+    reserved-memory {
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        contextb: contextb_reserved@81000000 {
+          reg = <0x81000000 0x400000>;
+          no-map;
+        };
+    };
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      rproc-contextb {
+          compatible = "microchip,ipc-remoteproc";
+          memory-region = <&contextb>;
+          mboxes= <&ihc 8>;
+      };
+    };
+
+...
-- 
2.34.1

[PATCH v1 5/5] remoteproc: add support for Microchip IPC remoteproc platform driver

[Valentina Fernandez]

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

Add a remoteproc platform driver to be able to load and boot firmware
to the remote processor(s).

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

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_remoteproc.c | 461 +++++++++++++++++++++++
 3 files changed, 474 insertions(+)
 create mode 100644 drivers/remoteproc/mchp_ipc_remoteproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index dda2ada215b7..f973ed1d0635 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_REMOTEPROC
+	tristate "Microchip IPC remoteproc support"
+	depends on MCHP_SBI_IPC_MBOX || COMPILE_TEST
+	depends on RISCV_SBI
+	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 91314a9b43ce..3e3a27d2e848 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_REMOTEPROC)	+= mchp_ipc_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_remoteproc.c b/drivers/remoteproc/mchp_ipc_remoteproc.c
new file mode 100644
index 000000000000..95446dbc7993
--- /dev/null
+++ b/drivers/remoteproc/mchp_ipc_remoteproc.c
@@ -0,0 +1,461 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Microchip IPC Remoteproc driver
+ *
+ * Copyright (c) 2021 - 2024 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.
+ *
+ * @MIV_RP_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.
+ *
+ * @MIV_RP_MBOX_STOP: a stop request for the remote context
+ *
+ * @MIV_RP_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,
+};
+
+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 int mchp_ipc_rproc_start(struct rproc *rproc)
+{
+	struct mchp_ipc_rproc *priv = rproc->priv;
+	struct device_node *np = priv->dev->of_node;
+	struct sbiret ret;
+	int result;
+
+	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);
+
+	result = wait_for_completion_timeout(&priv->start_done,
+					     msecs_to_jiffies(5000));
+
+	if (!of_property_present(np, "microchip,skip-start-wait") && result == 0) {
+		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 reserved_mem *rmem;
+	struct of_phandle_iterator it;
+	u64 device_address;
+
+	reinit_completion(&priv->start_done);
+
+	of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
+	while (of_phandle_iterator_next(&it) == 0) {
+		/*
+		 * Ignore the first memory region which will be used vdev
+		 * buffer. No need to do extra handlings, rproc_add_virtio_dev
+		 * will handle it.
+		 */
+		if (!strcmp(it.node->name, "vdev0buffer"))
+			continue;
+
+		if (!strcmp(it.node->name, "rsc-table"))
+			continue;
+
+		rmem = of_reserved_mem_lookup(it.node);
+		if (!rmem) {
+			of_node_put(it.node);
+			dev_err(priv->dev, "unable to acquire memory-region\n");
+			return -EINVAL;
+		}
+
+		device_address = rmem->base;
+
+		mem = rproc_mem_entry_init(priv->dev, NULL, (dma_addr_t)rmem->base,
+					   rmem->size, device_address, mchp_ipc_rproc_mem_alloc,
+					   mchp_ipc_rproc_mem_release, it.node->name);
+
+		if (!mem) {
+			of_node_put(it.node);
+			return -ENOMEM;
+		}
+
+		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 int mchp_ipc_rproc_addr_init(struct mchp_ipc_rproc *priv,
+				    struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	int i, err, rmem_np;
+
+	rmem_np = of_count_phandle_with_args(np, "memory-region", NULL);
+	if (rmem_np <= 0)
+		return 0;
+
+	for (i = 0; i < rmem_np; i++) {
+		struct device_node *node;
+		struct resource res;
+
+		node = of_parse_phandle(np, "memory-region", i);
+		if (!node)
+			continue;
+
+		if (!strncmp(node->name, "vdev", strlen("vdev"))) {
+			of_node_put(node);
+			continue;
+		}
+
+		if (!strcmp(node->name, "rsc-table")) {
+			err = of_address_to_resource(node, 0, &res);
+			if (err) {
+				of_node_put(node);
+				return dev_err_probe(dev, err,
+						     "unable to resolve memory region\n");
+			}
+			priv->rsc_table = devm_ioremap(&pdev->dev,
+						       res.start, resource_size(&res));
+			of_node_put(node);
+		}
+	}
+
+	return 0;
+}
+
+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 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");
+
+	priv->chan_id = mchp_ipc_get_chan_id(priv->mbox_channel);
+
+	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;
+	int 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;
+
+	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;
+
+	ret = mchp_ipc_rproc_addr_init(priv, pdev);
+	if (ret)
+		goto err_put_mbox;
+
+	/*
+	 * 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 = of_property_present(np, "microchip,auto-boot");
+
+	/* 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-remoteproc", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mchp_ipc_rproc_of_match);
+
+static struct platform_driver mchp_ipc_rproc_driver = {
+	.probe = mchp_ipc_rproc_probe,
+	.remove_new = 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

Re: [PATCH v1 2/5] dt-bindings: mailbox: add binding for Microchip IPC mailbox driver

[Conor Dooley]

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

On Thu, Sep 12, 2024 at 06:00:22PM +0100, Valentina Fernandez wrote:
> Add a dt-binding for the Microchip Inter-Processor Communication (IPC)
> mailbox controller.

Before anyone else gets here, there's an erroneous "driver" in $subject
:)

> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> ---
>  .../bindings/mailbox/microchip,sbi-ipc.yaml   | 115 ++++++++++++++++++
>  1 file changed, 115 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml b/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> new file mode 100644
> index 000000000000..dc2cbd5eb28f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> @@ -0,0 +1,115 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/microchip,sbi-ipc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip Inter-processor communication (IPC) mailbox controller
> +
> +maintainers:
> +  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> +
> +description:
> +  The Microchip Inter-processor Communication (IPC) facilitates
> +  message passing between processors using an interrupt signaling
> +  mechanism.
> +  This SBI interface is compatible with the Mi-V Inter-hart
> +  Communication (IHC) IP.
> +  The microchip,sbi-ipc compatible string is inteded for use by software
> +  running in supervisor privileged mode (s-mode). The SoC-specific
> +  compatibles are inteded for use by the SBI implementation in machine
> +  mode (m-mode).

And it might be worth me pointing out to the !riscv folks that "SBI
implementation in machine mode" means that the
"microchip,miv-ihc-rtl-v2" compatible is intended for use by firmware
only.

Cheers,
Conor.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,sbi-ipc
> +      - microchip,miv-ihc-rtl-v2
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 5
> +
> +  interrupt-names:
> +    minItems: 1
> +    maxItems: 5
> +
> +  "#mbox-cells":
> +    description:
> +      For the SBI "device", the cell represents the global "logical" channel IDs.
> +      The meaning of channel IDs are platform firmware dependent. The
> +      SoC-specific compatibles are intended for use by the SBI implementation,
> +      rather than s-mode software. There the cell would represent the physical
> +      channel and do not vary depending on platform firmware.
> +    const: 1
> +
> +  microchip,ihc-chan-disabled-mask:
> +    description:
> +      Represents the enable/disable state of the bi-directional IHC channels
> +      within the MIV-IHC IP configuration. The mask is a 16-bit value, but only
> +      the first 15 bits are utilized.Each of the bits corresponds to
> +      one of the 15 IHC channels.
> +      A bit set to '1' indicates that the corresponding channel is disabled,
> +      and any read or write operations to that channel will return zero.
> +      A bit set to '0' indicates that the corresponding channel is enabled
> +      and will be accessible through its dedicated address range registers.
> +      The remaining bit of the 16-bit mask is reserved and should be ignored.
> +      The actual enable/disable state of each channel is determined by the
> +      IP block’s configuration.
> +    $ref: /schemas/types.yaml#/definitions/uint16
> +    default: 0
> +
> +required:
> +  - compatible
> +  - interrupts
> +  - interrupt-names
> +  - "#mbox-cells"
> +
> +additionalProperties: false
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: microchip,sbi-ipc
> +    then:
> +      properties:
> +        reg: false
> +    else:
> +      required:
> +        - reg
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: microchip,miv-ihc-rtl-v2
> +    then:
> +      properties:
> +        interrupt-names:
> +          items:
> +            pattern: "^hart-[0-5]+$"
> +
> +examples:
> +  - |
> +    mailbox {
> +      compatible = "microchip,sbi-ipc";
> +      interrupt-parent = <&plic>;
> +      interrupts = <180>, <179>, <178>;
> +      interrupt-names = "hart-1", "hart-2", "hart-3";
> +      #mbox-cells = <1>;
> +    };
> +  - |
> +    mailbox@50000000 {
> +      compatible = "microchip,miv-ihc-rtl-v2";
> +      microchip,ihc-chan-disabled-mask=  /bits/ 16 <0>;
> +      reg = <0x50000000 0x1C000>;
> +      interrupt-parent = <&plic>;
> +      interrupts = <180>, <179>, <178>;
> +      interrupt-names = "hart-1", "hart-2", "hart-3";
> +      #mbox-cells = <1>;
> +    };
> -- 
> 2.34.1
> 

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

Re: [PATCH v1 1/5] riscv: asm: vendorid_list: Add Microchip Technology to the vendor list

[Conor Dooley]

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

On Thu, Sep 12, 2024 at 06:00:21PM +0100, Valentina Fernandez wrote:
> Add Microchip Technology to the RISC-V vendor list.
> 
> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>

In $subject "asm" should probably be "sbi", otherwise:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

> ---
>  arch/riscv/include/asm/vendorid_list.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/riscv/include/asm/vendorid_list.h b/arch/riscv/include/asm/vendorid_list.h
> index 2f2bb0c84f9a..a5150cdf34d8 100644
> --- a/arch/riscv/include/asm/vendorid_list.h
> +++ b/arch/riscv/include/asm/vendorid_list.h
> @@ -6,6 +6,7 @@
>  #define ASM_VENDOR_LIST_H
>  
>  #define ANDES_VENDOR_ID		0x31e
> +#define MICROCHIP_VENDOR_ID	0x029
>  #define SIFIVE_VENDOR_ID	0x489
>  #define THEAD_VENDOR_ID		0x5b7
>  
> -- 
> 2.34.1
> 

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

Re: [PATCH v1 2/5] dt-bindings: mailbox: add binding for Microchip IPC mailbox driver

[Samuel Holland]

Hi Valentina,

On 2024-09-12 12:00 PM, Valentina Fernandez wrote:
> Add a dt-binding for the Microchip Inter-Processor Communication (IPC)
> mailbox controller.
> 
> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> ---
>  .../bindings/mailbox/microchip,sbi-ipc.yaml   | 115 ++++++++++++++++++
>  1 file changed, 115 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml b/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> new file mode 100644
> index 000000000000..dc2cbd5eb28f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> @@ -0,0 +1,115 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/microchip,sbi-ipc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip Inter-processor communication (IPC) mailbox controller
> +
> +maintainers:
> +  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> +
> +description:
> +  The Microchip Inter-processor Communication (IPC) facilitates
> +  message passing between processors using an interrupt signaling
> +  mechanism.
> +  This SBI interface is compatible with the Mi-V Inter-hart
> +  Communication (IHC) IP.
> +  The microchip,sbi-ipc compatible string is inteded for use by software
> +  running in supervisor privileged mode (s-mode). The SoC-specific
> +  compatibles are inteded for use by the SBI implementation in machine
> +  mode (m-mode).

There is a lot of conditional logic in this binding for how small it is. Would
it make sense to split this into two separate bindings? For example, with the
current binding microchip,ihc-chan-disabled-mask is allowed for the SBI
interface, but doesn't look like it belongs there.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,sbi-ipc
> +      - microchip,miv-ihc-rtl-v2
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 5
> +
> +  interrupt-names:
> +    minItems: 1
> +    maxItems: 5
> +
> +  "#mbox-cells":
> +    description:
> +      For the SBI "device", the cell represents the global "logical" channel IDs.
> +      The meaning of channel IDs are platform firmware dependent. The
> +      SoC-specific compatibles are intended for use by the SBI implementation,
> +      rather than s-mode software. There the cell would represent the physical
> +      channel and do not vary depending on platform firmware.
> +    const: 1
> +
> +  microchip,ihc-chan-disabled-mask:
> +    description:
> +      Represents the enable/disable state of the bi-directional IHC channels
> +      within the MIV-IHC IP configuration. The mask is a 16-bit value, but only
> +      the first 15 bits are utilized.Each of the bits corresponds to
> +      one of the 15 IHC channels.
> +      A bit set to '1' indicates that the corresponding channel is disabled,
> +      and any read or write operations to that channel will return zero.
> +      A bit set to '0' indicates that the corresponding channel is enabled
> +      and will be accessible through its dedicated address range registers.
> +      The remaining bit of the 16-bit mask is reserved and should be ignored.
> +      The actual enable/disable state of each channel is determined by the
> +      IP block’s configuration.
> +    $ref: /schemas/types.yaml#/definitions/uint16
> +    default: 0
> +
> +required:
> +  - compatible
> +  - interrupts
> +  - interrupt-names
> +  - "#mbox-cells"
> +
> +additionalProperties: false
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: microchip,sbi-ipc
> +    then:
> +      properties:
> +        reg: false
> +    else:
> +      required:
> +        - reg
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: microchip,miv-ihc-rtl-v2
> +    then:
> +      properties:
> +        interrupt-names:
> +          items:
> +            pattern: "^hart-[0-5]+$"

The driver in patch 3 uses this pattern for the SBI interface, so should it
apply there as well?

Regards,
Samuel

> +
> +examples:
> +  - |
> +    mailbox {
> +      compatible = "microchip,sbi-ipc";
> +      interrupt-parent = <&plic>;
> +      interrupts = <180>, <179>, <178>;
> +      interrupt-names = "hart-1", "hart-2", "hart-3";
> +      #mbox-cells = <1>;
> +    };
> +  - |
> +    mailbox@50000000 {
> +      compatible = "microchip,miv-ihc-rtl-v2";
> +      microchip,ihc-chan-disabled-mask=  /bits/ 16 <0>;
> +      reg = <0x50000000 0x1C000>;
> +      interrupt-parent = <&plic>;
> +      interrupts = <180>, <179>, <178>;
> +      interrupt-names = "hart-1", "hart-2", "hart-3";
> +      #mbox-cells = <1>;
> +    };

Re: [PATCH v1 3/5] mailbox: add Microchip IPC support

[Samuel Holland]

Hi Valentina,

On 2024-09-12 12:00 PM, Valentina Fernandez wrote:
> +static int mchp_ipc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mchp_ipc_probe ipc_info;
> +	struct microchip_ipc *ipc;
> +	struct ipc_chan_info *priv;
> +	bool irq_avail = false;
> +	int ret;
> +	u32 chan_id;
> +
> +	ret = sbi_probe_extension(SBI_EXT_MICROCHIP_TECHNOLOGY);
> +	if (ret <= 0)
> +		return dev_err_probe(dev, ret, "Microchip SBI extension not detected\n");
> +
> +	ipc = devm_kzalloc(dev, sizeof(*ipc), GFP_KERNEL);
> +	if (!ipc)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, ipc);
> +
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(IPC_DMA_BIT_MASK));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "dma_set_mask_and_coherent failed\n");
> +
> +	ipc->buf_base = dmam_alloc_coherent(dev, sizeof(u32), &ipc->dma_addr, GFP_KERNEL);
> +
> +	if (!ipc->buf_base)
> +		return -ENOMEM;

One drive-by comment here: you don't need to use the DMA API to get a physical
address for passing to the SBI interface. You can use __pa() on a kmalloc'd
buffer, since kmalloc() returns memory from the linear map. This has the
advantage of 1) using cacheable memory and 2) not rounding up the allocation
size to a whole page.

> +
> +	ret = mchp_ipc_sbi_send(SBI_EXT_IPC_PROBE, ipc->dma_addr);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "could not probe IPC SBI service\n");
> +
> +	memcpy(&ipc_info, ipc->buf_base, sizeof(struct mchp_ipc_probe));

Here sizeof(struct mchp_ipc_probe) > sizeof(u32), so if the DMA API wasn't
rounding up the allocation size, this would be a buffer overflow.

Regards,
Samuel

> +	ipc->num_channels = ipc_info.num_channels;
> +	ipc->hw_type = ipc_info.hw_type;
> +
> +	ipc->chans = devm_kcalloc(dev, ipc->num_channels, sizeof(*ipc->chans), GFP_KERNEL);
> +	if (!ipc->chans)
> +		return -ENOMEM;
> +
> +	ipc->dev = dev;
> +	ipc->controller.txdone_irq = true;
> +	ipc->controller.dev = ipc->dev;
> +	ipc->controller.ops = &mchp_ipc_ops;
> +	ipc->controller.chans = ipc->chans;
> +	ipc->controller.num_chans = ipc->num_channels;
> +	ipc->controller.of_xlate = mchp_ipc_mbox_xlate;
> +
> +	for (chan_id = 0; chan_id < ipc->num_channels; chan_id++) {
> +		priv = devm_kmalloc(dev, sizeof(*priv), GFP_KERNEL);
> +		if (!priv)
> +			return -ENOMEM;
> +
> +		ipc->chans[chan_id].con_priv = priv;
> +		priv->id = chan_id;
> +	}
> +
> +	if (ipc->hw_type == MIV_IHC) {
> +		ipc->cluster_cfg = devm_kcalloc(dev, num_online_cpus(),
> +						sizeof(struct mchp_ipc_cluster_cfg),
> +						GFP_KERNEL);
> +		if (!ipc->cluster_cfg)
> +			return -ENOMEM;
> +
> +		if (mchp_ipc_get_cluster_aggr_irq(ipc))
> +			irq_avail = true;
> +	}
> +
> +	if (!irq_avail)
> +		return dev_err_probe(dev, -ENODEV, "missing interrupt property\n");
> +
> +	ret = devm_mbox_controller_register(dev, &ipc->controller);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +					 "Inter-Processor communication (IPC) registration failed\n");
> +
> +	return 0;
> +}

Re: [PATCH v1 0/5] Add Microchip IPC mailbox and remoteproc support

[Mathieu Poirier]

Hi Valentina,

On Thu, 12 Sept 2024 at 10:48, Valentina Fernandez
<valentina.fernandezalanis@microchip.com> wrote:
>
> Hello all,
>
> This series adds support for the Microchip Inter-Processor Communication
> (IPC) mailbox controller, as well as an 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 Microchip IPC is used to send messages between processors using
> an interrupt signaling mechanism. The driver uses the RISC-V Supervisor
> Binary Interface (SBI) to communicate with software running in machine
> mode (M-mode) to access the IPC hardware block.
>
> Additional details on the Microchip vendor extension and the IPC
> function IDs described in the driver can be found in the following
> documentation:
>
> https://github.com/linux4microchip/microchip-sbi-ecall-extension
>
> 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.
>
> The PIC64GX MPU has a Mi-V IHC block, this will be added to the PIC64GX
> dts after the initial upstreaming:
>
> https://patchwork.kernel.org/project/linux-riscv/patch/20240725121609.13101-18-pierre-henry.moussay@microchip.com/
>
> Thanks,
> Valentina
>
> Valentina Fernandez (5):
>   riscv: asm: vendorid_list: Add Microchip Technology to the vendor list
>   dt-bindings: mailbox: add binding for Microchip IPC mailbox driver
>   mailbox: add Microchip IPC support
>   dt-bindings: remoteproc: add binding for Microchip IPC remoteproc
>   remoteproc: add support for Microchip IPC remoteproc platform driver
>
>  .../bindings/mailbox/microchip,sbi-ipc.yaml   | 115 ++++
>  .../remoteproc/microchip,ipc-remoteproc.yaml  |  84 +++
>  arch/riscv/include/asm/vendorid_list.h        |   1 +
>  drivers/mailbox/Kconfig                       |  12 +
>  drivers/mailbox/Makefile                      |   2 +
>  drivers/mailbox/mailbox-mchp-ipc-sbi.c        | 539 ++++++++++++++++++
>  drivers/remoteproc/Kconfig                    |  12 +
>  drivers/remoteproc/Makefile                   |   1 +
>  drivers/remoteproc/mchp_ipc_remoteproc.c      | 461 +++++++++++++++
>  include/linux/mailbox/mchp-ipc.h              |  23 +
>  10 files changed, 1250 insertions(+)

It might be easier to split this patchset in two and proceed
incrementally, i.e upstream the mailbox driver first and then the
remoteproc part.

Regards,
Mathieu

>  create mode 100644 Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
>  create mode 100644 drivers/mailbox/mailbox-mchp-ipc-sbi.c
>  create mode 100644 drivers/remoteproc/mchp_ipc_remoteproc.c
>  create mode 100644 include/linux/mailbox/mchp-ipc.h
>
> --
> 2.34.1
>

Re: [PATCH v1 3/5] mailbox: add Microchip IPC support

[Valentina.FernandezAlanis]

On 12/09/2024 22:30, Samuel Holland wrote:
> [You don't often get email from samuel.holland@sifive.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Valentina,
> 
> On 2024-09-12 12:00 PM, Valentina Fernandez wrote:
>> +static int mchp_ipc_probe(struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct mchp_ipc_probe ipc_info;
>> +     struct microchip_ipc *ipc;
>> +     struct ipc_chan_info *priv;
>> +     bool irq_avail = false;
>> +     int ret;
>> +     u32 chan_id;
>> +
>> +     ret = sbi_probe_extension(SBI_EXT_MICROCHIP_TECHNOLOGY);
>> +     if (ret <= 0)
>> +             return dev_err_probe(dev, ret, "Microchip SBI extension not detected\n");
>> +
>> +     ipc = devm_kzalloc(dev, sizeof(*ipc), GFP_KERNEL);
>> +     if (!ipc)
>> +             return -ENOMEM;
>> +
>> +     platform_set_drvdata(pdev, ipc);
>> +
>> +     ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(IPC_DMA_BIT_MASK));
>> +     if (ret)
>> +             return dev_err_probe(dev, ret, "dma_set_mask_and_coherent failed\n");
>> +
>> +     ipc->buf_base = dmam_alloc_coherent(dev, sizeof(u32), &ipc->dma_addr, GFP_KERNEL);
>> +
>> +     if (!ipc->buf_base)
>> +             return -ENOMEM;
> 
> One drive-by comment here: you don't need to use the DMA API to get a physical
> address for passing to the SBI interface. You can use __pa() on a kmalloc'd
> buffer, since kmalloc() returns memory from the linear map. This has the
> advantage of 1) using cacheable memory and 2) not rounding up the allocation
> size to a whole page.
Hi Samuel,

Thanks for the suggestion, I will take a look into that approach
for the next iterarion of the series.

Thanks,
Valentina
> 
>> +
>> +     ret = mchp_ipc_sbi_send(SBI_EXT_IPC_PROBE, ipc->dma_addr);
>> +     if (ret < 0)
>> +             return dev_err_probe(dev, ret, "could not probe IPC SBI service\n");
>> +
>> +     memcpy(&ipc_info, ipc->buf_base, sizeof(struct mchp_ipc_probe));
> 
> Here sizeof(struct mchp_ipc_probe) > sizeof(u32), so if the DMA API wasn't
> rounding up the allocation size, this would be a buffer overflow.
> 
> Regards,
> Samuel
> 
>> +     ipc->num_channels = ipc_info.num_channels;
>> +     ipc->hw_type = ipc_info.hw_type;
>> +
>> +     ipc->chans = devm_kcalloc(dev, ipc->num_channels, sizeof(*ipc->chans), GFP_KERNEL);
>> +     if (!ipc->chans)
>> +             return -ENOMEM;
>> +
>> +     ipc->dev = dev;
>> +     ipc->controller.txdone_irq = true;
>> +     ipc->controller.dev = ipc->dev;
>> +     ipc->controller.ops = &mchp_ipc_ops;
>> +     ipc->controller.chans = ipc->chans;
>> +     ipc->controller.num_chans = ipc->num_channels;
>> +     ipc->controller.of_xlate = mchp_ipc_mbox_xlate;
>> +
>> +     for (chan_id = 0; chan_id < ipc->num_channels; chan_id++) {
>> +             priv = devm_kmalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +             if (!priv)
>> +                     return -ENOMEM;
>> +
>> +             ipc->chans[chan_id].con_priv = priv;
>> +             priv->id = chan_id;
>> +     }
>> +
>> +     if (ipc->hw_type == MIV_IHC) {
>> +             ipc->cluster_cfg = devm_kcalloc(dev, num_online_cpus(),
>> +                                             sizeof(struct mchp_ipc_cluster_cfg),
>> +                                             GFP_KERNEL);
>> +             if (!ipc->cluster_cfg)
>> +                     return -ENOMEM;
>> +
>> +             if (mchp_ipc_get_cluster_aggr_irq(ipc))
>> +                     irq_avail = true;
>> +     }
>> +
>> +     if (!irq_avail)
>> +             return dev_err_probe(dev, -ENODEV, "missing interrupt property\n");
>> +
>> +     ret = devm_mbox_controller_register(dev, &ipc->controller);
>> +     if (ret)
>> +             return dev_err_probe(dev, ret,
>> +                                      "Inter-Processor communication (IPC) registration failed\n");
>> +
>> +     return 0;
>> +}
> 

Re: [PATCH v1 0/5] Add Microchip IPC mailbox and remoteproc support

[Mathieu Poirier]

Hi Valentina,

On Thu, 12 Sept 2024 at 10:48, Valentina Fernandez
<valentina.fernandezalanis@microchip.com> wrote:
>
> Hello all,
>
> This series adds support for the Microchip Inter-Processor Communication
> (IPC) mailbox controller, as well as an 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 Microchip IPC is used to send messages between processors using
> an interrupt signaling mechanism. The driver uses the RISC-V Supervisor
> Binary Interface (SBI) to communicate with software running in machine
> mode (M-mode) to access the IPC hardware block.
>
> Additional details on the Microchip vendor extension and the IPC
> function IDs described in the driver can be found in the following
> documentation:
>
> https://github.com/linux4microchip/microchip-sbi-ecall-extension
>
> 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.
>
> The PIC64GX MPU has a Mi-V IHC block, this will be added to the PIC64GX
> dts after the initial upstreaming:
>
> https://patchwork.kernel.org/project/linux-riscv/patch/20240725121609.13101-18-pierre-henry.moussay@microchip.com/
>
> Thanks,
> Valentina
>
> Valentina Fernandez (5):
>   riscv: asm: vendorid_list: Add Microchip Technology to the vendor list
>   dt-bindings: mailbox: add binding for Microchip IPC mailbox driver
>   mailbox: add Microchip IPC support
>   dt-bindings: remoteproc: add binding for Microchip IPC remoteproc
>   remoteproc: add support for Microchip IPC remoteproc platform driver
>
>  .../bindings/mailbox/microchip,sbi-ipc.yaml   | 115 ++++
>  .../remoteproc/microchip,ipc-remoteproc.yaml  |  84 +++
>  arch/riscv/include/asm/vendorid_list.h        |   1 +
>  drivers/mailbox/Kconfig                       |  12 +
>  drivers/mailbox/Makefile                      |   2 +
>  drivers/mailbox/mailbox-mchp-ipc-sbi.c        | 539 ++++++++++++++++++
>  drivers/remoteproc/Kconfig                    |  12 +
>  drivers/remoteproc/Makefile                   |   1 +
>  drivers/remoteproc/mchp_ipc_remoteproc.c      | 461 +++++++++++++++
>  include/linux/mailbox/mchp-ipc.h              |  23 +

I would advise splitting the two patchsets, i.e one for the mailbox
driver and another one for the remoteproc.  I would also start with
the mailbox and then go with the remoteproc.

Thanks,
Mathieu

>  10 files changed, 1250 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
>  create mode 100644 drivers/mailbox/mailbox-mchp-ipc-sbi.c
>  create mode 100644 drivers/remoteproc/mchp_ipc_remoteproc.c
>  create mode 100644 include/linux/mailbox/mchp-ipc.h
>
> --
> 2.34.1
>

Re: [PATCH v1 2/5] dt-bindings: mailbox: add binding for Microchip IPC mailbox driver

[Conor Dooley]

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

On Thu, Sep 12, 2024 at 04:23:44PM -0500, Samuel Holland wrote:
> Hi Valentina,
> 
> On 2024-09-12 12:00 PM, Valentina Fernandez wrote:
> > Add a dt-binding for the Microchip Inter-Processor Communication (IPC)
> > mailbox controller.
> > 
> > Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> > ---
> >  .../bindings/mailbox/microchip,sbi-ipc.yaml   | 115 ++++++++++++++++++
> >  1 file changed, 115 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml b/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> > new file mode 100644
> > index 000000000000..dc2cbd5eb28f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> > @@ -0,0 +1,115 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mailbox/microchip,sbi-ipc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Microchip Inter-processor communication (IPC) mailbox controller
> > +
> > +maintainers:
> > +  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> > +
> > +description:
> > +  The Microchip Inter-processor Communication (IPC) facilitates
> > +  message passing between processors using an interrupt signaling
> > +  mechanism.
> > +  This SBI interface is compatible with the Mi-V Inter-hart
> > +  Communication (IHC) IP.
> > +  The microchip,sbi-ipc compatible string is inteded for use by software
> > +  running in supervisor privileged mode (s-mode). The SoC-specific
> > +  compatibles are inteded for use by the SBI implementation in machine
> > +  mode (m-mode).
> 
> There is a lot of conditional logic in this binding for how small it is. Would
> it make sense to split this into two separate bindings? For example, with the
> current binding microchip,ihc-chan-disabled-mask is allowed for the SBI
> interface, but doesn't look like it belongs there.

I dunno. Part of me says that because this is two compatibles for the
same piece of hardware (the choice depending on which programming model
you use) they should be documented together. The other part of me is of
the opinion that they effectively describe different things, given one
describes the hardware and the other describes a firmware interface that
may have any sort of hardware backing it.

I suppose it's more of a problem for "us" (that being me/Rob/Krzysztof)
than for Valentina, and how to handle firmware interfaces to hardware
like this is one of the topics that's planned for Krzysztof's devicetree
BoF session at LPC.

Cheers,
Conor.

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

Re: [PATCH v1 4/5] dt-bindings: remoteproc: add binding for Microchip IPC remoteproc

[Krzysztof Kozlowski]

On 12/09/2024 19:00, Valentina Fernandez wrote:
> Microchip family of RISC-V SoCs typically has or more clusters. These
> clusters can be configured to run in Asymmetric Multi Processing (AMP)
> mode

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

> 
> Add a dt-binding for the Microchip IPC Remoteproc platform driver.
> 

Binding is for hardware, not driver. Please rephrase it to describe
hardware.


> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> ---
>  .../remoteproc/microchip,ipc-remoteproc.yaml  | 84 +++++++++++++++++++
>  1 file changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
> new file mode 100644
> index 000000000000..1765c68d22cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
> @@ -0,0 +1,84 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-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.
> +
> +  This document defines the binding for the remoteproc component that
> +  loads and boots firmwares on remote clusters.

Don't say that binding is a binding for. Say what this hardware piece is.

> +
> +  This SBI interface is compatible with the Mi-V Inter-hart
> +  Communication (IHC) IP.
> +
> +maintainers:
> +  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> +
> +properties:
> +  compatible:
> +    const: microchip,ipc-remoteproc

That's quite generic. Basically this says it will handle IPC of all
possible Microchip SoCs, not only RISC-V but also ARM and whatever you
come up with.



> +
> +  mboxes:
> +    description:
> +      This property is required only if the rpmsg/virtio functionality is used.
> +      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
> +
> +  microchip,auto-boot:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      If defined, when remoteproc is probed, it loads the default firmware and
> +      starts the remote processor.

You described the desired Linux feature or behavior, not the actual
hardware. The bindings are about the latter, so instead you need to
rephrase the property and its description to match actual hardware
capabilities/features/configuration etc.

> +
> +  microchip,skip-ready-wait:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      If defined, the master processor will not expect a ready signal from the
> +      remote processor indicating it has booted successfully. This allows the
> +      master processor to proceed with its operations without waiting for
> +      confirmation from the remote processor.
Same problem.


> +
> +  memory-region:
> +    description:
> +      If present, a phandle for a reserved memory area that used for vdev buffer,
> +      resource table, vring region and others used by remote cluster.

missing constraints

> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +

Drop blank line

> +    reserved-memory {
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        contextb: contextb_reserved@81000000 {
> +          reg = <0x81000000 0x400000>;
> +          no-map;
> +        };
> +    };

Drop entire reserved-node. Obvious.

> +
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      rproc-contextb {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

usually remoteproc

> +          compatible = "microchip,ipc-remoteproc";
> +          memory-region = <&contextb>;
> +          mboxes= <&ihc 8>;

Make the binding complete. Fix the white-space issues.

> +      };
> +    };
> +
> +...

Best regards,
Krzysztof

Re: [PATCH v1 5/5] remoteproc: add support for Microchip IPC remoteproc platform driver

[Krzysztof Kozlowski]

On 12/09/2024 19:00, Valentina Fernandez wrote:
> The Microchip family of RISC-V SoCs typically has one or more clusters.
> These clusters can be configured to run in Asymmetric Multi-Processing
> (AMP) mode.
> 
> Add a remoteproc platform driver to be able to load and boot firmware
> to the remote processor(s).

...

> +
> +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 reserved_mem *rmem;
> +	struct of_phandle_iterator it;
> +	u64 device_address;
> +
> +	reinit_completion(&priv->start_done);
> +
> +	of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
> +	while (of_phandle_iterator_next(&it) == 0) {
> +		/*
> +		 * Ignore the first memory region which will be used vdev
> +		 * buffer. No need to do extra handlings, rproc_add_virtio_dev
> +		 * will handle it.
> +		 */
> +		if (!strcmp(it.node->name, "vdev0buffer"))

What? If you ignore the first, then why are you checking names? This
does not make sense. Especially that your binding did not say anything
about these phandles being somehow special.

> +			continue;
> +
> +		if (!strcmp(it.node->name, "rsc-table"))

Nope.

> +			continue;
> +
> +		rmem = of_reserved_mem_lookup(it.node);
> +		if (!rmem) {
> +			of_node_put(it.node);
> +			dev_err(priv->dev, "unable to acquire memory-region\n");
> +			return -EINVAL;
> +		}
> +
> +		device_address = rmem->base;
> +
> +		mem = rproc_mem_entry_init(priv->dev, NULL, (dma_addr_t)rmem->base,
> +					   rmem->size, device_address, mchp_ipc_rproc_mem_alloc,
> +					   mchp_ipc_rproc_mem_release, it.node->name);
> +
> +		if (!mem) {
> +			of_node_put(it.node);
> +			return -ENOMEM;
> +		}
> +
> +		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 int mchp_ipc_rproc_addr_init(struct mchp_ipc_rproc *priv,
> +				    struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	int i, err, rmem_np;
> +
> +	rmem_np = of_count_phandle_with_args(np, "memory-region", NULL);
> +	if (rmem_np <= 0)
> +		return 0;
> +
> +	for (i = 0; i < rmem_np; i++) {
> +		struct device_node *node;
> +		struct resource res;
> +
> +		node = of_parse_phandle(np, "memory-region", i);
> +		if (!node)
> +			continue;
> +
> +		if (!strncmp(node->name, "vdev", strlen("vdev"))) {

Uh? Why do you look for node names? What if I call the name differently?
Why would that matter?

> +			of_node_put(node);
> +			continue;
> +		}
> +
> +		if (!strcmp(node->name, "rsc-table")) {

No, really. Why are you checking for these?

NAK


> +			err = of_address_to_resource(node, 0, &res);
> +			if (err) {
> +				of_node_put(node);
> +				return dev_err_probe(dev, err,
> +						     "unable to resolve memory region\n");
> +			}
> +			priv->rsc_table = devm_ioremap(&pdev->dev,
> +						       res.start, resource_size(&res));
> +			of_node_put(node);
> +		}
> +	}
> +
> +	return 0;
> +}


Best regards,
Krzysztof

Re: [PATCH v1 3/5] mailbox: add Microchip IPC support

[Krzysztof Kozlowski]

On 12/09/2024 19:00, Valentina Fernandez wrote:
> Add a mailbox controller driver for the Microchip Inter-processor
> Communication (IPC), which is used to send and receive data between
> processors.
> 
> The driver uses the RISC-V Supervisor Binary Interface (SBI) to
> communicate with software running in machine mode (M-mode) to access
> the IPC hardware block.
> 

...

> +
> +static struct microchip_ipc *to_mchp_ipc_mbox(struct mbox_controller *mbox)
> +{
> +	return container_of(mbox, struct microchip_ipc, controller);
> +}
> +

You need kerneldoc for exported functions.

> +u32 mchp_ipc_get_chan_id(struct mbox_chan *chan)
> +{
> +	struct ipc_chan_info *chan_info = (struct ipc_chan_info *)chan->con_priv;
> +
> +	return chan_info->id;
> +}
> +EXPORT_SYMBOL(mchp_ipc_get_chan_id);

EXPORT_SYMBOL_GPL

> +


...

> +static int mchp_ipc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mchp_ipc_probe ipc_info;
> +	struct microchip_ipc *ipc;
> +	struct ipc_chan_info *priv;
> +	bool irq_avail = false;
> +	int ret;
> +	u32 chan_id;
> +
> +	ret = sbi_probe_extension(SBI_EXT_MICROCHIP_TECHNOLOGY);
> +	if (ret <= 0)
> +		return dev_err_probe(dev, ret, "Microchip SBI extension not detected\n");
> +
> +	ipc = devm_kzalloc(dev, sizeof(*ipc), GFP_KERNEL);
> +	if (!ipc)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, ipc);
> +
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(IPC_DMA_BIT_MASK));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "dma_set_mask_and_coherent failed\n");
> +
> +	ipc->buf_base = dmam_alloc_coherent(dev, sizeof(u32), &ipc->dma_addr, GFP_KERNEL);
> +

Drop blank line.

> +	if (!ipc->buf_base)
> +		return -ENOMEM;
> +
> +	ret = mchp_ipc_sbi_send(SBI_EXT_IPC_PROBE, ipc->dma_addr);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "could not probe IPC SBI service\n");
> +
> +	memcpy(&ipc_info, ipc->buf_base, sizeof(struct mchp_ipc_probe));
> +	ipc->num_channels = ipc_info.num_channels;
> +	ipc->hw_type = ipc_info.hw_type;
> +
> +	ipc->chans = devm_kcalloc(dev, ipc->num_channels, sizeof(*ipc->chans), GFP_KERNEL);
> +	if (!ipc->chans)
> +		return -ENOMEM;
> +
> +	ipc->dev = dev;
> +	ipc->controller.txdone_irq = true;
> +	ipc->controller.dev = ipc->dev;
> +	ipc->controller.ops = &mchp_ipc_ops;
> +	ipc->controller.chans = ipc->chans;
> +	ipc->controller.num_chans = ipc->num_channels;
> +	ipc->controller.of_xlate = mchp_ipc_mbox_xlate;
> +
> +	for (chan_id = 0; chan_id < ipc->num_channels; chan_id++) {
> +		priv = devm_kmalloc(dev, sizeof(*priv), GFP_KERNEL);
> +		if (!priv)
> +			return -ENOMEM;
> +
> +		ipc->chans[chan_id].con_priv = priv;
> +		priv->id = chan_id;
> +	}
> +
> +	if (ipc->hw_type == MIV_IHC) {
> +		ipc->cluster_cfg = devm_kcalloc(dev, num_online_cpus(),
> +						sizeof(struct mchp_ipc_cluster_cfg),
> +						GFP_KERNEL);
> +		if (!ipc->cluster_cfg)
> +			return -ENOMEM;
> +
> +		if (mchp_ipc_get_cluster_aggr_irq(ipc))
> +			irq_avail = true;
> +	}
> +
> +	if (!irq_avail)
> +		return dev_err_probe(dev, -ENODEV, "missing interrupt property\n");
> +
> +	ret = devm_mbox_controller_register(dev, &ipc->controller);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +					 "Inter-Processor communication (IPC) registration failed\n");

Fix alignment.

> +
> +	return 0;
> +}
> +
> +MODULE_DEVICE_TABLE(of, mchp_ipc_of_match);

This is ALWAYS next to the definition.

> +
> +static struct platform_driver mchp_ipc_driver = {
> +	.driver = {
> +		.name = "microchip_ipc",
> +		.of_match_table = of_match_ptr(mchp_ipc_of_match),

Drop of_match_ptr. You have warnings here.

> +	},
> +	.probe = mchp_ipc_probe,
> +};
> +
> +module_platform_driver(mchp_ipc_driver);



Best regards,
Krzysztof

Re: [PATCH v1 0/5] Add Microchip IPC mailbox and remoteproc support

[Bo Gan]

Hi Valentina,

On 9/12/24 10:00, Valentina Fernandez wrote:
> Additional details on the Microchip vendor extension and the IPC
> function IDs described in the driver can be found in the following
> documentation:
> 
> https://github.com/linux4microchip/microchip-sbi-ecall-extension
> 
> 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.

Would you mind help clarifying the need for SBI_EXT_RPROC_STATE/STOP/...?
If I'm not mistaken, the HW you are targeting is described in
https://ww1.microchip.com/downloads/aemDocuments/documents/MPU64/ProductDocuments/SupportingCollateral/Asymmetric_Multi-Processing_on_PIC64GX_White_Paper.pdf
(typo in the page 4, U51 -> E51)
In SBI, do you put hart1-3 and hart4 into 2 separate domains? If not,
I don't see why you can't just use HSM extension from SBI to kick rproc.
Also, how stable is this microchip-sbi-ecall-extension? Is it subject
to changes down the road? I don't see a probe() like SBI function, so
the kernel kind of assume it can call those microchip extensions without
causing unintended effects. This means those extension FIDs must be
stable and can no longer change once this code is in. Perhaps checking-in
the microchip SBI extensions to major SBI implementations such as openSBI
first would be better?

Bo

Re: [PATCH v1 0/5] Add Microchip IPC mailbox and remoteproc support

[Valentina.FernandezAlanis]

On 16/09/2024 23:28, Bo Gan wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> the content is safe
> 
> Hi Valentina,
Hi Bo,
> 
> On 9/12/24 10:00, Valentina Fernandez wrote:
>> Additional details on the Microchip vendor extension and the IPC
>> function IDs described in the driver can be found in the following
>> documentation:
>>
>> https://github.com/linux4microchip/microchip-sbi-ecall-extension
>>
>> 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.
> 
> Would you mind help clarifying the need for SBI_EXT_RPROC_STATE/STOP/...?
Sure, I provided a detailed explanation below.
> If I'm not mistaken, the HW you are targeting is described in
> https://ww1.microchip.com/downloads/aemDocuments/documents/MPU64/ 
> ProductDocuments/SupportingCollateral/Asymmetric_Multi- 
> Processing_on_PIC64GX_White_Paper.pdf
> (typo in the page 4, U51 -> E51)
Yes, this is the HW that these drivers are targeted for.
> In SBI, do you put hart1-3 and hart4 into 2 separate domains? If not,
> I don't see why you can't just use HSM extension from SBI to kick rproc.

The first AMP context (harts 1-3) is in one OpenSBI domain. The second 
AMP context may or may not be in an OpenSBI domain. Typical AMP use case 
applications have Linux SMP in one AMP context and an RTOS or BM 
application running in the other context.

BM/RTOS applications running in m-mode won't have OpenSBI, which means 
they may not necessarily have an HSM. However, if the BM/RTOS is running 
in s-mode, then we do register them in another OpenSBI domain.

We use the SBI_EXT_RPROC_START and SBI_EXT_RPROC_STOP functions to 
handle both scenarios.

> Also, how stable is this microchip-sbi-ecall-extension? Is it subject
> to changes down the road?

All the FIDs described in the microchip-sbi-ecall-extension repository 
are stable and agreed upon between different business units within 
Microchip, so they will not change. There might be additional FIDs added 
in the future to extend functionality if ever needed, but we won't 
change existing FIDs.
I don't see a probe() like SBI function, so
> the kernel kind of assume it can call those microchip extensions without
> causing unintended effects. This means those extension FIDs must be
> stable and can no longer change once this code is in. Perhaps checking-in
> the microchip SBI extensions to major SBI implementations such as openSBI
> first would be better?
Are you referring to the remoteproc driver? If that's the case, then 
yes, I believe we should call 
sbi_probe_extension(SBI_EXT_MICROCHIP_TECHNOLOGY) within the probe 
function. I will look into this for v2.

Thanks,
Valentina
> 
> Bo

Re: [PATCH v1 0/5] Add Microchip IPC mailbox and remoteproc support

[Conor Dooley]

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

On Tue, Sep 17, 2024 at 10:45:02AM +0000, Valentina.FernandezAlanis@microchip.com wrote:
> On 16/09/2024 23:28, Bo Gan wrote:
> > Perhaps checking-in
> > the microchip SBI extensions to major SBI implementations such as openSBI
> > first would be better?

It's worth pointing out that the "major SBI implementations" do not
support the platform this is currently being used on.

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

Re: [PATCH v1 2/5] dt-bindings: mailbox: add binding for Microchip IPC mailbox driver

[Rob Herring]

On Mon, Sep 16, 2024 at 05:31:36PM +0100, Conor Dooley wrote:
> On Thu, Sep 12, 2024 at 04:23:44PM -0500, Samuel Holland wrote:
> > Hi Valentina,
> > 
> > On 2024-09-12 12:00 PM, Valentina Fernandez wrote:
> > > Add a dt-binding for the Microchip Inter-Processor Communication (IPC)
> > > mailbox controller.
> > > 
> > > Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> > > ---
> > >  .../bindings/mailbox/microchip,sbi-ipc.yaml   | 115 ++++++++++++++++++
> > >  1 file changed, 115 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml b/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> > > new file mode 100644
> > > index 000000000000..dc2cbd5eb28f
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> > > @@ -0,0 +1,115 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mailbox/microchip,sbi-ipc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Microchip Inter-processor communication (IPC) mailbox controller
> > > +
> > > +maintainers:
> > > +  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> > > +
> > > +description:
> > > +  The Microchip Inter-processor Communication (IPC) facilitates
> > > +  message passing between processors using an interrupt signaling
> > > +  mechanism.
> > > +  This SBI interface is compatible with the Mi-V Inter-hart
> > > +  Communication (IHC) IP.
> > > +  The microchip,sbi-ipc compatible string is inteded for use by software
> > > +  running in supervisor privileged mode (s-mode). The SoC-specific
> > > +  compatibles are inteded for use by the SBI implementation in machine
> > > +  mode (m-mode).
> > 
> > There is a lot of conditional logic in this binding for how small it is. Would
> > it make sense to split this into two separate bindings? For example, with the
> > current binding microchip,ihc-chan-disabled-mask is allowed for the SBI
> > interface, but doesn't look like it belongs there.
> 
> I dunno. Part of me says that because this is two compatibles for the
> same piece of hardware (the choice depending on which programming model
> you use) they should be documented together. The other part of me is of
> the opinion that they effectively describe different things, given one
> describes the hardware and the other describes a firmware interface that
> may have any sort of hardware backing it.
> 
> I suppose it's more of a problem for "us" (that being me/Rob/Krzysztof)
> than for Valentina, and how to handle firmware interfaces to hardware
> like this is one of the topics that's planned for Krzysztof's devicetree
> BoF session at LPC.

If how the client interacts with the device is fundamentally different, 
then I think different compatibles is fine.

Rob

Re: [PATCH v1 5/5] remoteproc: add support for Microchip IPC remoteproc platform driver

[Valentina.FernandezAlanis]

On 16/09/2024 21:18, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 12/09/2024 19:00, Valentina Fernandez wrote:
>> The Microchip family of RISC-V SoCs typically has one or more clusters.
>> These clusters can be configured to run in Asymmetric Multi-Processing
>> (AMP) mode.
>>
>> Add a remoteproc platform driver to be able to load and boot firmware
>> to the remote processor(s).
> 
> ...
> 
>> +
>> +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 reserved_mem *rmem;
>> +     struct of_phandle_iterator it;
>> +     u64 device_address;
>> +
>> +     reinit_completion(&priv->start_done);
>> +
>> +     of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
>> +     while (of_phandle_iterator_next(&it) == 0) {
>> +             /*
>> +              * Ignore the first memory region which will be used vdev
>> +              * buffer. No need to do extra handlings, rproc_add_virtio_dev
>> +              * will handle it.
>> +              */
>> +             if (!strcmp(it.node->name, "vdev0buffer"))
> 
> What? If you ignore the first, then why are you checking names? This
> does not make sense. Especially that your binding did not say anything
> about these phandles being somehow special.

The idea in the code above is to skip the vdev buffer allocation and 
carveout registration. Later, when the remoteproc virtio driver 
registers the virtio device (rproc_add_virtio_dev), it will attempt to 
find the carveout. Since the carveout wasn’t registered, it will use the 
first memory region from the parent by calling 
of_reserved_mem_device_init_by_idx.

This behavior is based on some existing platform drivers. However, upon 
further inspection, it seems that some newer drivers use 
rproc_of_resm_mem_entry_init to allocate vdev buffers.

I will restructure this section and rephase/drop the comment.

With regards the bindings, I'll explain better all the memory regions 
for v2.

Just for everyone’s information, we have the following use cases:

Early boot: Remote processors are booted by another entity before Linux, 
so we only need to attach. For this mode, we require the resource table 
as a memory region in the device tree.

Late boot - Linux is responsible for loading the firmware and starting 
it on the remote processors. For this, we need the region used for the 
firmware image.

In both cases, rpmsg communication is optional. This means that the vdev 
buffers and vrings memory regions are also optional.

There could also be a mixed case where we start with early boot mode by 
attaching to an existing remoteproc, and then stop, start, and load 
another firmware once Linux has booted. In this case, we would require 
the resource table and firmware image region, and optionally, vdev 
buffers and vrings.

> 
>> +                     continue;
>> +
>> +             if (!strcmp(it.node->name, "rsc-table"))
> 
> Nope.
Since the resource table is only needed for early boot mode and does not 
need to be a carveout region, we are skipping that.

I will work on making the resource table a fixed index in the 
memory-region property so that it doesn't have a fixed name.

Thanks,
Valentina

> 
>> +                     continue;
>> +
>> +             rmem = of_reserved_mem_lookup(it.node);
>> +             if (!rmem) {
>> +                     of_node_put(it.node);
>> +                     dev_err(priv->dev, "unable to acquire memory-region\n");
>> +                     return -EINVAL;
>> +             }
>> +
>> +             device_address = rmem->base;
>> +
>> +             mem = rproc_mem_entry_init(priv->dev, NULL, (dma_addr_t)rmem->base,
>> +                                        rmem->size, device_address, mchp_ipc_rproc_mem_alloc,
>> +                                        mchp_ipc_rproc_mem_release, it.node->name);
>> +
>> +             if (!mem) {
>> +                     of_node_put(it.node);
>> +                     return -ENOMEM;
>> +             }
>> +
>> +             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 int mchp_ipc_rproc_addr_init(struct mchp_ipc_rproc *priv,
>> +                                 struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct device_node *np = dev->of_node;
>> +     int i, err, rmem_np;
>> +
>> +     rmem_np = of_count_phandle_with_args(np, "memory-region", NULL);
>> +     if (rmem_np <= 0)
>> +             return 0;
>> +
>> +     for (i = 0; i < rmem_np; i++) {
>> +             struct device_node *node;
>> +             struct resource res;
>> +
>> +             node = of_parse_phandle(np, "memory-region", i);
>> +             if (!node)
>> +                     continue;
>> +
>> +             if (!strncmp(node->name, "vdev", strlen("vdev"))) {
> 
> Uh? Why do you look for node names? What if I call the name differently?
> Why would that matter?
> 
>> +                     of_node_put(node);
>> +                     continue;
>> +             }
>> +
>> +             if (!strcmp(node->name, "rsc-table")) {
> 
> No, really. Why are you checking for these?
> 
> NAK
> 
> 
>> +                     err = of_address_to_resource(node, 0, &res);
>> +                     if (err) {
>> +                             of_node_put(node);
>> +                             return dev_err_probe(dev, err,
>> +                                                  "unable to resolve memory region\n");
>> +                     }
>> +                     priv->rsc_table = devm_ioremap(&pdev->dev,
>> +                                                    res.start, resource_size(&res));
>> +                     of_node_put(node);
>> +             }
>> +     }
>> +
>> +     return 0;
>> +}
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 2/5] dt-bindings: mailbox: add binding for Microchip IPC mailbox driver

[Conor Dooley]

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

On Wed, Sep 18, 2024 at 10:35:58AM -0500, Rob Herring wrote:
> On Mon, Sep 16, 2024 at 05:31:36PM +0100, Conor Dooley wrote:
> > On Thu, Sep 12, 2024 at 04:23:44PM -0500, Samuel Holland wrote:
> > > Hi Valentina,
> > > 
> > > On 2024-09-12 12:00 PM, Valentina Fernandez wrote:
> > > > Add a dt-binding for the Microchip Inter-Processor Communication (IPC)
> > > > mailbox controller.
> > > > 
> > > > Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> > > > ---
> > > >  .../bindings/mailbox/microchip,sbi-ipc.yaml   | 115 ++++++++++++++++++
> > > >  1 file changed, 115 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml b/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> > > > new file mode 100644
> > > > index 000000000000..dc2cbd5eb28f
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> > > > @@ -0,0 +1,115 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/mailbox/microchip,sbi-ipc.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Microchip Inter-processor communication (IPC) mailbox controller
> > > > +
> > > > +maintainers:
> > > > +  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> > > > +
> > > > +description:
> > > > +  The Microchip Inter-processor Communication (IPC) facilitates
> > > > +  message passing between processors using an interrupt signaling
> > > > +  mechanism.
> > > > +  This SBI interface is compatible with the Mi-V Inter-hart
> > > > +  Communication (IHC) IP.
> > > > +  The microchip,sbi-ipc compatible string is inteded for use by software
> > > > +  running in supervisor privileged mode (s-mode). The SoC-specific
> > > > +  compatibles are inteded for use by the SBI implementation in machine
> > > > +  mode (m-mode).
> > > 
> > > There is a lot of conditional logic in this binding for how small it is. Would
> > > it make sense to split this into two separate bindings? For example, with the
> > > current binding microchip,ihc-chan-disabled-mask is allowed for the SBI
> > > interface, but doesn't look like it belongs there.
> > 
> > I dunno. Part of me says that because this is two compatibles for the
> > same piece of hardware (the choice depending on which programming model
> > you use) they should be documented together. The other part of me is of
> > the opinion that they effectively describe different things, given one
> > describes the hardware and the other describes a firmware interface that
> > may have any sort of hardware backing it.
> > 
> > I suppose it's more of a problem for "us" (that being me/Rob/Krzysztof)
> > than for Valentina, and how to handle firmware interfaces to hardware
> > like this is one of the topics that's planned for Krzysztof's devicetree
> > BoF session at LPC.
> 
> If how the client interacts with the device is fundamentally different, 
> then I think different compatibles is fine.

It wasn't about different compatibles (which I think are non-debatable
here) it's whether or not the different compatibles should be in their
own binding files.

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

Re: [PATCH v1 5/5] remoteproc: add support for Microchip IPC remoteproc platform driver

[Krzysztof Kozlowski]

On 18/09/2024 17:51, Valentina.FernandezAlanis@microchip.com wrote:
> On 16/09/2024 21:18, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 12/09/2024 19:00, Valentina Fernandez wrote:
>>> The Microchip family of RISC-V SoCs typically has one or more clusters.
>>> These clusters can be configured to run in Asymmetric Multi-Processing
>>> (AMP) mode.
>>>
>>> Add a remoteproc platform driver to be able to load and boot firmware
>>> to the remote processor(s).
>>
>> ...
>>
>>> +
>>> +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 reserved_mem *rmem;
>>> +     struct of_phandle_iterator it;
>>> +     u64 device_address;
>>> +
>>> +     reinit_completion(&priv->start_done);
>>> +
>>> +     of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
>>> +     while (of_phandle_iterator_next(&it) == 0) {
>>> +             /*
>>> +              * Ignore the first memory region which will be used vdev
>>> +              * buffer. No need to do extra handlings, rproc_add_virtio_dev
>>> +              * will handle it.
>>> +              */
>>> +             if (!strcmp(it.node->name, "vdev0buffer"))
>>
>> What? If you ignore the first, then why are you checking names? This
>> does not make sense. Especially that your binding did not say anything
>> about these phandles being somehow special.
> 
> The idea in the code above is to skip the vdev buffer allocation and 
> carveout registration. Later, when the remoteproc virtio driver 
> registers the virtio device (rproc_add_virtio_dev), it will attempt to 
> find the carveout. Since the carveout wasn’t registered, it will use the 
> first memory region from the parent by calling 
> of_reserved_mem_device_init_by_idx.
> 
> This behavior is based on some existing platform drivers. However, upon 
> further inspection, it seems that some newer drivers use 
> rproc_of_resm_mem_entry_init to allocate vdev buffers.
> 
> I will restructure this section and rephase/drop the comment.
> 
> With regards the bindings, I'll explain better all the memory regions 
> for v2.
> 
> Just for everyone’s information, we have the following use cases:
> 
> Early boot: Remote processors are booted by another entity before Linux, 
> so we only need to attach. For this mode, we require the resource table 
> as a memory region in the device tree.
> 
> Late boot - Linux is responsible for loading the firmware and starting 
> it on the remote processors. For this, we need the region used for the 
> firmware image.
> 
> In both cases, rpmsg communication is optional. This means that the vdev 
> buffers and vrings memory regions are also optional.
> 
> There could also be a mixed case where we start with early boot mode by 
> attaching to an existing remoteproc, and then stop, start, and load 
> another firmware once Linux has booted. In this case, we would require 
> the resource table and firmware image region, and optionally, vdev 
> buffers and vrings.
> 
>>
>>> +                     continue;
>>> +
>>> +             if (!strcmp(it.node->name, "rsc-table"))
>>
>> Nope.
> Since the resource table is only needed for early boot mode and does not 
> need to be a carveout region, we are skipping that.
> 
> I will work on making the resource table a fixed index in the 
> memory-region property so that it doesn't have a fixed name.

The list of memory-regions already HAS fixed indices. All this is not
only confusing, but incorrect. I commented that if I call the node
"rsc-not-a-table" your code will stop working.

Best regards,
Krzysztof

Re: [PATCH v1 4/5] dt-bindings: remoteproc: add binding for Microchip IPC remoteproc

[Valentina.FernandezAlanis]

On 16/09/2024 21:14, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 12/09/2024 19:00, Valentina Fernandez wrote:
>> Microchip family of RISC-V SoCs typically has or more clusters. These
>> clusters can be configured to run in Asymmetric Multi Processing (AMP)
>> mode
> 
> A nit, subject: drop second/last, redundant "binding for". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 
>>
>> Add a dt-binding for the Microchip IPC Remoteproc platform driver.
>>
> 
> Binding is for hardware, not driver. Please rephrase it to describe
> hardware.
> 
> 
>> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
>> ---
>>   .../remoteproc/microchip,ipc-remoteproc.yaml  | 84 +++++++++++++++++++
>>   1 file changed, 84 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
>> new file mode 100644
>> index 000000000000..1765c68d22cf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
>> @@ -0,0 +1,84 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-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.
>> +
>> +  This document defines the binding for the remoteproc component that
>> +  loads and boots firmwares on remote clusters.
> 
> Don't say that binding is a binding for. Say what this hardware piece is.
> 
>> +
>> +  This SBI interface is compatible with the Mi-V Inter-hart
>> +  Communication (IHC) IP.
>> +
>> +maintainers:
>> +  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: microchip,ipc-remoteproc
> 
> That's quite generic. Basically this says it will handle IPC of all
> possible Microchip SoCs, not only RISC-V but also ARM and whatever you
> come up with.
IPC is the actual name of the hardware block described in this binding. 
I'll update the description of the binding in v2 to mention this.

Additionally, I'll rename the compatible to microchip,ipc-sbi-remoteproc 
to further clarify that this binding is intended for devices using the 
Microchip IPC hardware block and for devices with an SBI interface (RISC-V).

Thanks,
Valentina
> 
> 
> 
>> +
>> +  mboxes:
>> +    description:
>> +      This property is required only if the rpmsg/virtio functionality is used.
>> +      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
>> +
>> +  microchip,auto-boot:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description:
>> +      If defined, when remoteproc is probed, it loads the default firmware and
>> +      starts the remote processor.
> 
> You described the desired Linux feature or behavior, not the actual
> hardware. The bindings are about the latter, so instead you need to
> rephrase the property and its description to match actual hardware
> capabilities/features/configuration etc.
> 
>> +
>> +  microchip,skip-ready-wait:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description:
>> +      If defined, the master processor will not expect a ready signal from the
>> +      remote processor indicating it has booted successfully. This allows the
>> +      master processor to proceed with its operations without waiting for
>> +      confirmation from the remote processor.
> Same problem.
> 
> 
>> +
>> +  memory-region:
>> +    description:
>> +      If present, a phandle for a reserved memory area that used for vdev buffer,
>> +      resource table, vring region and others used by remote cluster.
> 
> missing constraints
> 
>> +
>> +required:
>> +  - compatible
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +
> 
> Drop blank line
> 
>> +    reserved-memory {
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +
>> +        contextb: contextb_reserved@81000000 {
>> +          reg = <0x81000000 0x400000>;
>> +          no-map;
>> +        };
>> +    };
> 
> Drop entire reserved-node. Obvious.
> 
>> +
>> +    soc {
>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +
>> +      rproc-contextb {
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> usually remoteproc
> 
>> +          compatible = "microchip,ipc-remoteproc";
>> +          memory-region = <&contextb>;
>> +          mboxes= <&ihc 8>;
> 
> Make the binding complete. Fix the white-space issues.
> 
>> +      };
>> +    };
>> +
>> +...
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 4/5] dt-bindings: remoteproc: add binding for Microchip IPC remoteproc

[Krzysztof Kozlowski]

On 15/10/2024 14:09, Valentina.FernandezAlanis@microchip.com wrote:
> On 16/09/2024 21:14, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 12/09/2024 19:00, Valentina Fernandez wrote:
>>> Microchip family of RISC-V SoCs typically has or more clusters. These
>>> clusters can be configured to run in Asymmetric Multi Processing (AMP)
>>> mode
>>
>> A nit, subject: drop second/last, redundant "binding for". The
>> "dt-bindings" prefix is already stating that these are bindings.
>> See also:
>> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>>
>>>
>>> Add a dt-binding for the Microchip IPC Remoteproc platform driver.
>>>
>>
>> Binding is for hardware, not driver. Please rephrase it to describe
>> hardware.
>>
>>
>>> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
>>> ---
>>>   .../remoteproc/microchip,ipc-remoteproc.yaml  | 84 +++++++++++++++++++
>>>   1 file changed, 84 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
>>> new file mode 100644
>>> index 000000000000..1765c68d22cf
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
>>> @@ -0,0 +1,84 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-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.
>>> +
>>> +  This document defines the binding for the remoteproc component that
>>> +  loads and boots firmwares on remote clusters.
>>
>> Don't say that binding is a binding for. Say what this hardware piece is.
>>
>>> +
>>> +  This SBI interface is compatible with the Mi-V Inter-hart
>>> +  Communication (IHC) IP.
>>> +
>>> +maintainers:
>>> +  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: microchip,ipc-remoteproc
>>
>> That's quite generic. Basically this says it will handle IPC of all
>> possible Microchip SoCs, not only RISC-V but also ARM and whatever you
>> come up with.
> IPC is the actual name of the hardware block described in this binding. 
> I'll update the description of the binding in v2 to mention this.
> 
> Additionally, I'll rename the compatible to microchip,ipc-sbi-remoteproc 
> to further clarify that this binding is intended for devices using the 
> Microchip IPC hardware block and for devices with an SBI interface (RISC-V).

Well, still generic. Explain why this deserves exception from specific
SoC compatibles.

Best regards,
Krzysztof

Re: [PATCH v1 4/5] dt-bindings: remoteproc: add binding for Microchip IPC remoteproc

[Conor Dooley]

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

On Tue, Oct 15, 2024 at 03:35:46PM +0200, Krzysztof Kozlowski wrote:
> On 15/10/2024 14:09, Valentina.FernandezAlanis@microchip.com wrote:
> > On 16/09/2024 21:14, Krzysztof Kozlowski wrote:
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>
> >> On 12/09/2024 19:00, Valentina Fernandez wrote:
> >>> Microchip family of RISC-V SoCs typically has or more clusters. These
> >>> clusters can be configured to run in Asymmetric Multi Processing (AMP)
> >>> mode
> >>
> >> A nit, subject: drop second/last, redundant "binding for". The
> >> "dt-bindings" prefix is already stating that these are bindings.
> >> See also:
> >> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> >>
> >>>
> >>> Add a dt-binding for the Microchip IPC Remoteproc platform driver.
> >>>
> >>
> >> Binding is for hardware, not driver. Please rephrase it to describe
> >> hardware.
> >>
> >>
> >>> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> >>> ---
> >>>   .../remoteproc/microchip,ipc-remoteproc.yaml  | 84 +++++++++++++++++++
> >>>   1 file changed, 84 insertions(+)
> >>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
> >>> new file mode 100644
> >>> index 000000000000..1765c68d22cf
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml
> >>> @@ -0,0 +1,84 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-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.
> >>> +
> >>> +  This document defines the binding for the remoteproc component that
> >>> +  loads and boots firmwares on remote clusters.
> >>
> >> Don't say that binding is a binding for. Say what this hardware piece is.
> >>
> >>> +
> >>> +  This SBI interface is compatible with the Mi-V Inter-hart
> >>> +  Communication (IHC) IP.
> >>> +
> >>> +maintainers:
> >>> +  - Valentina Fernandez <valentina.fernandezalanis@microchip.com>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: microchip,ipc-remoteproc
> >>
> >> That's quite generic. Basically this says it will handle IPC of all
> >> possible Microchip SoCs, not only RISC-V but also ARM and whatever you
> >> come up with.
> > IPC is the actual name of the hardware block described in this binding. 
> > I'll update the description of the binding in v2 to mention this.
> > 
> > Additionally, I'll rename the compatible to microchip,ipc-sbi-remoteproc 
> > to further clarify that this binding is intended for devices using the 
> > Microchip IPC hardware block and for devices with an SBI interface (RISC-V).
> 
> Well, still generic. Explain why this deserves exception from specific
> SoC compatibles.

If I understand this correctly, some degree of generic-ness is actually
intended here. The IPC/IHC (the name depends on whether or not it is RTL
for the FPGA fabric or a hardened version) isn't quite like some of the
other remoteproc things here, that are intended for programming a DSP or
similar with some firmware - it's intended for asymmetric
multiprocessing stuff, where some of the CPU cores run Linux and the
others are running something like freertos or zephyr, with an abstracted
interface provided by the firmware/SBI implementation. The mailbox side
of this is also implemented using an SBI abstraction (similar to PSCI or
SCMI, I never recall which) and also has a compatible that isn't tied to
specific soc.
Granted, the mailbox side does also have a IP core specific version, but
that is not intended to be consumed by the OS, but rather by the SBI
implementation (e.g. OpenSBI).

What this binding is supposed to be describing is the "generic" ecall
interface to a set of remote processors provided by the SBI
implementation. The platform you're running on is meant to be abstracted
away by use of ecalls etc, just as the mailbox is - which is why
Valentina went something not soc-specific here. I can see much more of
an argument for encoding the version of the protocol that is implemented
by the SBI firmware than for having a soc-specific set of compatibles
here.

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