[PATCH v2 0/3] Add Microchip IPC mailbox

[PATCH v2 0/3] Add Microchip IPC mailbox

[Valentina Fernandez]

Hello all,

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

I am submitting this v2 with the IPC mailbox patches and without the
IPC remoteproc drivers, as suggested in v1 [1].

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 PIC64GX MPU has a Mi-V IHC block, this will be added to the PIC64GX
dts after the initial upstreaming [2].

[1] https://patchwork.kernel.org/project/linux-remoteproc/cover/20240912170025.455167-1-valentina.fernandezalanis@microchip.com/
[2] https://patchwork.kernel.org/project/linux-riscv/patch/20240725121609.13101-18-pierre-henry.moussay@microchip.com/


changes since v1:
  - use kmalloc and __pa() instead of DMA API
  - fix size of buf_base to avoid potential buffer overflow
  - add kernel doc for exported functions (mchp_ipc_get_chan_id)
  - use EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL
  - drop unnecessary blank line and fix alignment issues
  - drop of_match_ptr
  - move MODULE_DEVICE_TABLE next to the definition
  - reword subject from riscv: asm: vendorid_list to riscv: sbi: vendorid_list
  - remove the word "driver" from dt-binding commit subject
  - make interrupt-names a required property for all cases
  - add dependency on COMPILE_TEST and ARCH_MICROCHIP

Regards,
Valentina

Valentina Fernandez (3):
  riscv: sbi: vendorid_list: Add Microchip Technology to the vendor list
  dt-bindings: mailbox: add binding for Microchip IPC mailbox controller
  mailbox: add Microchip IPC support

 .../bindings/mailbox/microchip,sbi-ipc.yaml   | 108 ++++
 arch/riscv/include/asm/vendorid_list.h        |   1 +
 drivers/mailbox/Kconfig                       |  13 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/mailbox-mchp-ipc-sbi.c        | 537 ++++++++++++++++++
 include/linux/mailbox/mchp-ipc.h              |  23 +
 6 files changed, 684 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
 create mode 100644 drivers/mailbox/mailbox-mchp-ipc-sbi.c
 create mode 100644 include/linux/mailbox/mchp-ipc.h

-- 
2.34.1

[PATCH v2 1/3] riscv: sbi: 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 v2 2/3] dt-bindings: mailbox: add binding for Microchip IPC mailbox controller

[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   | 108 ++++++++++++++++++
 1 file changed, 108 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..90a7932118b5
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
@@ -0,0 +1,108 @@
+# 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
+    items:
+      pattern: "^hart-[0-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
+        microchip,ihc-chan-disabled-mask: false
+    else:
+      required:
+        - reg
+        - microchip,ihc-chan-disabled-mask
+
+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 v2 3/3] 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                |  13 +
 drivers/mailbox/Makefile               |   2 +
 drivers/mailbox/mailbox-mchp-ipc-sbi.c | 537 +++++++++++++++++++++++++
 include/linux/mailbox/mchp-ipc.h       |  23 ++
 4 files changed, 575 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 6fb995778636..a61b3b0c5da3 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -177,6 +177,19 @@ 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 || COMPILE_TEST
+	depends on ARCH_MICROCHIP
+	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..2416e0d740e7
--- /dev/null
+++ b/drivers/mailbox/mailbox-mchp-ipc-sbi.c
@@ -0,0 +1,537 @@
+// 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;
+	unsigned long buf_base_addr;
+	int irq;
+};
+
+struct ipc_chan_info {
+	void *buf_base_tx;
+	void *buf_base_rx;
+	void *msg_buf_tx;
+	void *msg_buf_rx;
+	unsigned long buf_base_tx_addr;
+	unsigned long buf_base_rx_addr;
+	unsigned long msg_buf_tx_addr;
+	unsigned long msg_buf_rx_addr;
+	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;
+	unsigned long buf_base_addr;
+	struct mbox_controller controller;
+	u8 num_channels;
+	enum ipc_hw hw_type;
+};
+
+static int mchp_ipc_sbi_chan_send(u32 command, u32 channel, unsigned long 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, unsigned long 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);
+}
+
+/**
+ * mchp_ipc_get_chan_id - Retrieve the channel ID from a mailbox channel
+ * @chan: Pointer to the mailbox channel
+ *
+ * This function extracts the channel ID from the given mailbox channel.
+ *
+ * Return: The channel ID as a 32-bit unsigned integer.
+ */
+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_GPL(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_rx_addr;
+	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].buf_base_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->buf_base_rx_addr);
+			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->buf_base_rx_addr);
+			mbox_chan_txdone(&ipc->chans[chan_id], ret);
+		}
+	}
+	return IRQ_HANDLED;
+}
+
+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_tx_addr;
+	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->buf_base_tx_addr);
+}
+
+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 = kmalloc(sizeof(*chan_info->buf_base_tx), GFP_KERNEL);
+	if (!chan_info->buf_base_tx) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+
+	chan_info->buf_base_tx_addr = __pa(chan_info->buf_base_tx);
+
+	chan_info->buf_base_rx = kmalloc(sizeof(*chan_info->buf_base_rx), GFP_KERNEL);
+	if (!chan_info->buf_base_rx) {
+		ret = -ENOMEM;
+		goto fail_free_buf_base_tx;
+	}
+
+	chan_info->buf_base_rx_addr = __pa(chan_info->buf_base_rx);
+
+	ret = mchp_ipc_sbi_chan_send(SBI_EXT_IPC_CH_INIT, chan_info->id,
+				     chan_info->buf_base_tx_addr);
+	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 = kmalloc(chan_info->max_msg_size, GFP_KERNEL);
+	if (!chan_info->msg_buf_tx) {
+		ret = -ENOMEM;
+		goto fail_free_buf_base_rx;
+	}
+
+	chan_info->msg_buf_tx_addr = __pa(chan_info->msg_buf_tx);
+
+	chan_info->msg_buf_rx = kmalloc(chan_info->max_msg_size, GFP_KERNEL);
+	if (!chan_info->msg_buf_rx) {
+		ret = -ENOMEM;
+		goto fail_free_buf_msg_tx;
+	}
+
+	chan_info->msg_buf_rx_addr = __pa(chan_info->msg_buf_rx);
+
+	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:
+	kfree(chan_info->msg_buf_rx);
+fail_free_buf_msg_tx:
+	kfree(chan_info->msg_buf_tx);
+fail_free_buf_base_rx:
+	kfree(chan_info->buf_base_rx);
+fail_free_buf_base_tx:
+	kfree(chan_info->buf_base_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;
+
+	kfree(chan_info->buf_base_tx);
+	kfree(chan_info->buf_base_rx);
+	kfree(chan_info->msg_buf_tx);
+	kfree(chan_info->msg_buf_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 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 = devm_kmalloc(ipc->dev,
+								 sizeof(struct mchp_ipc_status),
+								 GFP_KERNEL);
+
+		if (!ipc->cluster_cfg[hartid].buf_base)
+			return -ENOMEM;
+
+		ipc->cluster_cfg[hartid].buf_base_addr = __pa(ipc->cluster_cfg[hartid].buf_base);
+
+		irq_found = true;
+	}
+
+	return irq_found;
+}
+
+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);
+
+	ipc->buf_base = devm_kmalloc(dev, sizeof(struct mchp_ipc_probe), GFP_KERNEL);
+	if (!ipc->buf_base)
+		return -ENOMEM;
+
+	ipc->buf_base_addr = __pa(ipc->buf_base);
+
+	ret = mchp_ipc_sbi_send(SBI_EXT_IPC_PROBE, ipc->buf_base_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;
+}
+
+static const struct of_device_id mchp_ipc_of_match[] = {
+	{.compatible = "microchip,sbi-ipc", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mchp_ipc_of_match);
+
+static struct platform_driver mchp_ipc_driver = {
+	.driver = {
+		.name = "microchip_ipc",
+		.of_match_table = 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

Re: [PATCH v2 2/3] dt-bindings: mailbox: add binding for Microchip IPC mailbox controller

[Rob Herring]

On Fri, Oct 25, 2024 at 01:51:09PM +0100, 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   | 108 ++++++++++++++++++
>  1 file changed, 108 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..90a7932118b5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
> @@ -0,0 +1,108 @@
> +# 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:

You need '>' and blank line in between each paragraph if you want the 
paragraphs maintained.

Elsewhere too.

> +  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

intended

> +  mode (m-mode).

This per compatible information should be with the compatibles.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,sbi-ipc
> +      - microchip,miv-ihc-rtl-v2

To add per compatible descriptions, you can do:

oneOf:
  - const:
    description: ...
  - const:
    description: ...



> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 5
> +
> +  interrupt-names:
> +    minItems: 1
> +    maxItems: 5
> +    items:
> +      pattern: "^hart-[0-5]+$"

Is the numbering always contiguous (e.g. not "hart-0, hart-3")? If so, 
drop the names. A name matching the index of the entry is not useful.

> +
> +  "#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
> +        microchip,ihc-chan-disabled-mask: false
> +    else:
> +      required:
> +        - reg
> +        - microchip,ihc-chan-disabled-mask
> +
> +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>;

Incorrect formatting around the '='.

> +      reg = <0x50000000 0x1C000>;
> +      interrupt-parent = <&plic>;
> +      interrupts = <180>, <179>, <178>;
> +      interrupt-names = "hart-1", "hart-2", "hart-3";
> +      #mbox-cells = <1>;
> +    };
> -- 
> 2.34.1
> 

Re: [PATCH v2 2/3] dt-bindings: mailbox: add binding for Microchip IPC mailbox controller

[Valentina.FernandezAlanis]

On 29/10/2024 12:48, Rob Herring wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Oct 25, 2024 at 01:51:09PM +0100, 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   | 108 ++++++++++++++++++
>>   1 file changed, 108 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..90a7932118b5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
>> @@ -0,0 +1,108 @@
>> +# 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:
> 
> You need '>' and blank line in between each paragraph if you want the
> paragraphs maintained.
> 
> Elsewhere too.
> 
>> +  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
> 
> intended
> 
>> +  mode (m-mode).
> 
> This per compatible information should be with the compatibles.
> 
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - microchip,sbi-ipc
>> +      - microchip,miv-ihc-rtl-v2
> 
> To add per compatible descriptions, you can do:
> 
> oneOf:
>    - const:
>      description: ...
>    - const:
>      description: ...
> 
> 
> 
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    minItems: 1
>> +    maxItems: 5
>> +
>> +  interrupt-names:
>> +    minItems: 1
>> +    maxItems: 5
>> +    items:
>> +      pattern: "^hart-[0-5]+$"
> 
> Is the numbering always contiguous (e.g. not "hart-0, hart-3")? If so,
> drop the names. A name matching the index of the entry is not useful.
Numbering can be non-contiguous based on the user's AMP configuration 
(e.g., 'hart-3', 'hart-4', 'hart-1').

Thanks for the rest of the feedback, I'll implement those changes in v3.
> 
>> +
>> +  "#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
>> +        microchip,ihc-chan-disabled-mask: false
>> +    else:
>> +      required:
>> +        - reg
>> +        - microchip,ihc-chan-disabled-mask
>> +
>> +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>;
> 
> Incorrect formatting around the '='.
> 
>> +      reg = <0x50000000 0x1C000>;
>> +      interrupt-parent = <&plic>;
>> +      interrupts = <180>, <179>, <178>;
>> +      interrupt-names = "hart-1", "hart-2", "hart-3";
>> +      #mbox-cells = <1>;
>> +    };
>> --
>> 2.34.1
>>

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

[Jassi Brar]

On Fri, Oct 25, 2024 at 7:36 AM Valentina Fernandez
<valentina.fernandezalanis@microchip.com> wrote:

....
> +
> +enum ipc_irq_type {
> +       IPC_OPS_NOT_SUPPORTED   = 1,
> +       IPC_MP_IRQ              = 2,
> +       IPC_MC_IRQ              = 4,
> +};
totally unused.

> +
> +/**
> + * 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 {
 same as the driver.probe(), so maybe call this microchip_mbox_info

......

> +struct mchp_ipc_cluster_cfg {
> +       void *buf_base;
> +       unsigned long buf_base_addr;
> +       int irq;
> +};
> +
> +struct ipc_chan_info {
 I suggest s/ipc_chan_info/microchip_sbi_chan and hooking it to
mbox_chan.con_priv

....

> +       unsigned long buf_base_tx_addr;
> +       unsigned long buf_base_rx_addr;
> +       unsigned long msg_buf_tx_addr;
> +       unsigned long msg_buf_rx_addr;
If these are __pa(), then phys_addr_t please.

> +       int chan_aggregated_irq;
> +       int mp_irq;
> +       int mc_irq;
> +       u32 id;
> +       u32 max_msg_size;
> +};
> +
> +struct microchip_ipc {
 Maybe s/microchip_ipc/microchip_sbi_mbox ?


> +       struct device *dev;
> +       struct mbox_chan *chans;
> +       struct mchp_ipc_cluster_cfg *cluster_cfg;
> +       struct ipc_chan_info *priv;
  replace this with 'struct mbox_chan *chan' and hook
     chan[i].con_priv = priv[i]
  this will help avoid having to EXPORT mchp_ipc_get_chan_id


> +       void *buf_base;
> +       unsigned long buf_base_addr;
phys_addr_t buf_base_addr ?

> +       struct mbox_controller controller;
> +       u8 num_channels;
this could be dropped by directly using 'controller.num_chans'

......

> +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_tx_addr;
> +       sbi_payload.size = msg->size;
> +       memcpy(chan_info->buf_base_tx, &sbi_payload, sizeof(sbi_payload));
How does this work? sizeof(sbi_payload) is more than
sizeof(*chan_info->buf_base_tx)
I think buf_base_tx needs to be u8 array of max{sizeof(struct
mchp_ipc_init), sizeof(struct mchp_ipc_sbi_msg)}, if there are
alignment requirements then maybe kmalloc that size.
Similarly for buf_base_rx.

...

> +static struct platform_driver mchp_ipc_driver = {
> +       .driver = {
> +               .name = "microchip_ipc",
> +               .of_match_table = mchp_ipc_of_match,
> +       },
> +       .probe = mchp_ipc_probe,
The driver could be built as a module, so please provide .remove()
even if you never intend to unload it.

cheers.

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

[Valentina.FernandezAlanis]

On 03/11/2024 00:23, Jassi Brar wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Oct 25, 2024 at 7:36 AM Valentina Fernandez
> <valentina.fernandezalanis@microchip.com> wrote:
> 
> ....
>> +
>> +enum ipc_irq_type {
>> +       IPC_OPS_NOT_SUPPORTED   = 1,
>> +       IPC_MP_IRQ              = 2,
>> +       IPC_MC_IRQ              = 4,
>> +};
> totally unused.
> 
>> +
>> +/**
>> + * 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 {
>   same as the driver.probe(), so maybe call this microchip_mbox_info
> 
> ......
> 
>> +struct mchp_ipc_cluster_cfg {
>> +       void *buf_base;
>> +       unsigned long buf_base_addr;
>> +       int irq;
>> +};
>> +
>> +struct ipc_chan_info {
>   I suggest s/ipc_chan_info/microchip_sbi_chan and hooking it to
> mbox_chan.con_priv
> 
> ....
> 
>> +       unsigned long buf_base_tx_addr;
>> +       unsigned long buf_base_rx_addr;
>> +       unsigned long msg_buf_tx_addr;
>> +       unsigned long msg_buf_rx_addr;
> If these are __pa(), then phys_addr_t please.
> 
>> +       int chan_aggregated_irq;
>> +       int mp_irq;
>> +       int mc_irq;
>> +       u32 id;
>> +       u32 max_msg_size;
>> +};
>> +
>> +struct microchip_ipc {
>   Maybe s/microchip_ipc/microchip_sbi_mbox ?
> 
> 
>> +       struct device *dev;
>> +       struct mbox_chan *chans;
>> +       struct mchp_ipc_cluster_cfg *cluster_cfg;
>> +       struct ipc_chan_info *priv;
>    replace this with 'struct mbox_chan *chan' and hook
>       chan[i].con_priv = priv[i]
>    this will help avoid having to EXPORT mchp_ipc_get_chan_id

Thanks for the feedback, I'll implement the suggestions in v3.

Regarding the EXPORT function, I understand that it’s also possible to 
retrieve con_priv from mbox_chan in the client. However, I felt it would 
be cleaner to export the function to obtain the channel ID directly, 
rather than declaring the struct ipc_chan_info in a header file to make 
it accessible to the client.

If necessary, I can remove the function and instead expose struct 
ipc_chan_info in linux/mailbox/mchp-ipc.h.
> 
> 
>> +       void *buf_base;
>> +       unsigned long buf_base_addr;
> phys_addr_t buf_base_addr ?
> 
>> +       struct mbox_controller controller;
>> +       u8 num_channels;
> this could be dropped by directly using 'controller.num_chans'
> 
> ......
> 
>> +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_tx_addr;
>> +       sbi_payload.size = msg->size;
>> +       memcpy(chan_info->buf_base_tx, &sbi_payload, sizeof(sbi_payload));
> How does this work? sizeof(sbi_payload) is more than
> sizeof(*chan_info->buf_base_tx)
> I think buf_base_tx needs to be u8 array of max{sizeof(struct
> mchp_ipc_init), sizeof(struct mchp_ipc_sbi_msg)}, if there are
> alignment requirements then maybe kmalloc that size.
> Similarly for buf_base_rx.
> 
> ...
> 
>> +static struct platform_driver mchp_ipc_driver = {
>> +       .driver = {
>> +               .name = "microchip_ipc",
>> +               .of_match_table = mchp_ipc_of_match,
>> +       },
>> +       .probe = mchp_ipc_probe,
> The driver could be built as a module, so please provide .remove()
> even if you never intend to unload it.
In this particular case, there is nothing specific to implement in the 
.remove() function because all resources allocated in the .probe() 
function are managed using devm_*
> 
> cheers.

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

[Jassi Brar]

On Mon, Nov 4, 2024 at 1:01 PM <Valentina.FernandezAlanis@microchip.com> wrote:
>
> On 03/11/2024 00:23, Jassi Brar wrote:

> Regarding the EXPORT function, I understand that it’s also possible to
> retrieve con_priv from mbox_chan in the client. However, I felt it would
> be cleaner to export the function to obtain the channel ID directly,
> rather than declaring the struct ipc_chan_info in a header file to make
> it accessible to the client.
>
> If necessary, I can remove the function and instead expose struct
> ipc_chan_info in linux/mailbox/mchp-ipc.h.
>
Yes please avoid EXPORT at any cost. They are only acceptable when no
other means exist.

> >
> >> +static struct platform_driver mchp_ipc_driver = {
> >> +       .driver = {
> >> +               .name = "microchip_ipc",
> >> +               .of_match_table = mchp_ipc_of_match,
> >> +       },
> >> +       .probe = mchp_ipc_probe,
> > The driver could be built as a module, so please provide .remove()
> > even if you never intend to unload it.
> In this particular case, there is nothing specific to implement in the
> .remove() function because all resources allocated in the .probe()
> function are managed using devm_*

OK.