[PATCH v4] cdx: add MSI support for CDX bus

[PATCH v4] cdx: add MSI support for CDX bus

[Nipun Gupta]

Add CDX-MSI domain per CDX controller with gic-its domain as
a parent, to support MSI for CDX devices. CDX devices allocate
MSIs from the CDX domain. Also, introduce APIs to alloc and free
IRQs for CDX domain.

In CDX subsystem firmware is a controller for all devices and
their configuration. CDX bus controller sends all the write_msi_msg
commands to firmware running on RPU and the firmware interfaces with
actual devices to pass this information to devices

Since, CDX controller is the only way to communicate with the Firmware
for MSI write info, CDX domain per controller required in contrast to
having a CDX domain per device.

Co-developed-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
Signed-off-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
Co-developed-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
---

Changes v3->v4:
- Rebased on Linux 6.6-rc1

Changes v2->v3:
- Rebased on Linux 6.5-rc1
- Used FW provided 'msi_dev_id' as device ID for GIC instead of 'req_id'.

Changes v1->v2:
- fixed scenario where msi write was called asyncronously in
  an atomic context, by using irq_chip_(un)lock, and using sync
  MCDI API for write MSI message.
- fixed broken Signed-off-by chain.

 drivers/cdx/Kconfig                     |   1 +
 drivers/cdx/Makefile                    |   2 +-
 drivers/cdx/cdx.c                       |   9 ++
 drivers/cdx/cdx.h                       |  12 ++
 drivers/cdx/cdx_msi.c                   | 183 ++++++++++++++++++++++++
 drivers/cdx/controller/cdx_controller.c |  23 +++
 drivers/cdx/controller/mc_cdx_pcol.h    |  64 +++++++++
 drivers/cdx/controller/mcdi_functions.c |  26 +++-
 drivers/cdx/controller/mcdi_functions.h |  20 +++
 include/linux/cdx/cdx_bus.h             |  32 +++++
 kernel/irq/msi.c                        |   1 +
 11 files changed, 370 insertions(+), 3 deletions(-)
 create mode 100644 drivers/cdx/cdx_msi.c

diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
index a08958485e31..86df7ccb76bb 100644
--- a/drivers/cdx/Kconfig
+++ b/drivers/cdx/Kconfig
@@ -8,6 +8,7 @@
 config CDX_BUS
 	bool "CDX Bus driver"
 	depends on OF && ARM64
+	select GENERIC_MSI_IRQ_DOMAIN
 	help
 	  Driver to enable Composable DMA Transfer(CDX) Bus. CDX bus
 	  exposes Fabric devices which uses composable DMA IP to the
diff --git a/drivers/cdx/Makefile b/drivers/cdx/Makefile
index 0324e4914f6e..4bad79d1d188 100644
--- a/drivers/cdx/Makefile
+++ b/drivers/cdx/Makefile
@@ -5,4 +5,4 @@
 # Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
 #
 
-obj-$(CONFIG_CDX_BUS) += cdx.o controller/
+obj-$(CONFIG_CDX_BUS) += cdx.o cdx_msi.o controller/
diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
index d2cad4c670a0..8d777cdacf1d 100644
--- a/drivers/cdx/cdx.c
+++ b/drivers/cdx/cdx.c
@@ -56,6 +56,7 @@
  */
 
 #include <linux/init.h>
+#include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/of_device.h>
 #include <linux/slab.h>
@@ -478,6 +479,7 @@ int cdx_device_add(struct cdx_dev_params *dev_params)
 
 	/* Populate CDX dev params */
 	cdx_dev->req_id = dev_params->req_id;
+	cdx_dev->msi_dev_id = dev_params->msi_dev_id;
 	cdx_dev->vendor = dev_params->vendor;
 	cdx_dev->device = dev_params->device;
 	cdx_dev->bus_num = dev_params->bus_num;
@@ -491,12 +493,19 @@ int cdx_device_add(struct cdx_dev_params *dev_params)
 	cdx_dev->dev.bus = &cdx_bus_type;
 	cdx_dev->dev.dma_mask = &cdx_dev->dma_mask;
 	cdx_dev->dev.release = cdx_device_release;
+	cdx_dev->msi_write_pending = false;
+	mutex_init(&cdx_dev->irqchip_lock);
 
 	/* Set Name */
 	dev_set_name(&cdx_dev->dev, "cdx-%02x:%02x",
 		     ((cdx->id << CDX_CONTROLLER_ID_SHIFT) | (cdx_dev->bus_num & CDX_BUS_NUM_MASK)),
 		     cdx_dev->dev_num);
 
+	if (cdx->msi_domain) {
+		cdx_dev->num_msi = dev_params->num_msi;
+		dev_set_msi_domain(&cdx_dev->dev, cdx->msi_domain);
+	}
+
 	ret = device_add(&cdx_dev->dev);
 	if (ret) {
 		dev_err(&cdx_dev->dev,
diff --git a/drivers/cdx/cdx.h b/drivers/cdx/cdx.h
index c436ac7ac86f..ece11c04d646 100644
--- a/drivers/cdx/cdx.h
+++ b/drivers/cdx/cdx.h
@@ -21,6 +21,8 @@
  * @res: array of MMIO region entries
  * @res_count: number of valid MMIO regions
  * @req_id: Requestor ID associated with CDX device
+ * @msi_dev_id: MSI device ID associated with CDX device
+ * @num_msi: Number of MSI's supported by the device
  */
 struct cdx_dev_params {
 	struct cdx_controller *cdx;
@@ -31,6 +33,8 @@ struct cdx_dev_params {
 	struct resource res[MAX_CDX_DEV_RESOURCES];
 	u8 res_count;
 	u32 req_id;
+	u32 msi_dev_id;
+	u32 num_msi;
 };
 
 /**
@@ -59,4 +63,12 @@ void cdx_unregister_controller(struct cdx_controller *cdx);
  */
 int cdx_device_add(struct cdx_dev_params *dev_params);
 
+/**
+ * cdx_msi_domain_init - Init the CDX bus MSI domain.
+ * @dev: Device of the CDX bus controller
+ *
+ * Return: CDX MSI domain, NULL on failure
+ */
+struct irq_domain *cdx_msi_domain_init(struct device *dev);
+
 #endif /* _CDX_H_ */
diff --git a/drivers/cdx/cdx_msi.c b/drivers/cdx/cdx_msi.c
new file mode 100644
index 000000000000..d7f4c88428d6
--- /dev/null
+++ b/drivers/cdx/cdx_msi.c
@@ -0,0 +1,183 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD CDX bus driver MSI support
+ *
+ * Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/msi.h>
+#include <linux/cdx/cdx_bus.h>
+
+#include "cdx.h"
+
+static void cdx_msi_write_msg(struct irq_data *irq_data,
+			      struct msi_msg *msg)
+{
+	struct msi_desc *msi_desc = irq_data_get_msi_desc(irq_data);
+	struct cdx_device *cdx_dev = to_cdx_device(msi_desc->dev);
+
+	/* We would not operate on msg here rather we wait for
+	 * irq_bus_sync_unlock() to be called from preemptible
+	 * task context.
+	 */
+	msi_desc->msg = *msg;
+	cdx_dev->msi_write_pending = true;
+}
+
+static void cdx_msi_write_irq_lock(struct irq_data *irq_data)
+{
+	struct msi_desc *msi_desc = irq_data_get_msi_desc(irq_data);
+	struct cdx_device *cdx_dev = to_cdx_device(msi_desc->dev);
+
+	mutex_lock(&cdx_dev->irqchip_lock);
+}
+
+static void cdx_msi_write_irq_unlock(struct irq_data *irq_data)
+{
+	struct msi_desc *msi_desc = irq_data_get_msi_desc(irq_data);
+	struct cdx_device *cdx_dev = to_cdx_device(msi_desc->dev);
+	struct cdx_controller *cdx = cdx_dev->cdx;
+	struct cdx_device_config dev_config;
+	int ret;
+
+	if (!cdx_dev->msi_write_pending) {
+		mutex_unlock(&cdx_dev->irqchip_lock);
+		return;
+	}
+
+	cdx_dev->msi_write_pending = false;
+	mutex_unlock(&cdx_dev->irqchip_lock);
+
+	dev_config.msi.msi_index = msi_desc->msi_index;
+	dev_config.msi.data = msi_desc->msg.data;
+	dev_config.msi.addr = ((u64)(msi_desc->msg.address_hi) << 32) |
+			      msi_desc->msg.address_lo;
+
+	dev_config.type = CDX_DEV_MSI_CONF;
+	ret = cdx->ops->dev_configure(cdx, cdx_dev->bus_num, cdx_dev->dev_num,
+				      &dev_config);
+	if (ret)
+		dev_err(&cdx_dev->dev, "Write MSI failed to CDX controller\n");
+}
+
+static struct irq_chip cdx_msi_irq_chip = {
+	.name			= "CDX-MSI",
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_affinity	= msi_domain_set_affinity,
+	.irq_write_msi_msg	= cdx_msi_write_msg,
+	.irq_bus_lock		= cdx_msi_write_irq_lock,
+	.irq_bus_sync_unlock	= cdx_msi_write_irq_unlock
+};
+
+int cdx_msi_domain_alloc_irqs(struct device *dev, unsigned int irq_count)
+{
+	int ret;
+
+	ret = msi_setup_device_data(dev);
+	if (ret)
+		return ret;
+
+	ret = msi_domain_alloc_irqs_range(dev, MSI_DEFAULT_DOMAIN,
+					  0, irq_count - 1);
+	if (ret)
+		dev_err(dev, "Failed to allocate IRQs: %d\n", ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cdx_msi_domain_alloc_irqs);
+
+/* Convert an msi_desc to a globally unique identifier. */
+static irq_hw_number_t cdx_domain_calc_hwirq(struct cdx_device *dev,
+					     struct msi_desc *desc)
+{
+	return ((irq_hw_number_t)dev->msi_dev_id << 10) | desc->msi_index;
+}
+
+static void cdx_msi_set_desc(msi_alloc_info_t *arg,
+			     struct msi_desc *desc)
+{
+	arg->desc = desc;
+	arg->hwirq = cdx_domain_calc_hwirq(to_cdx_device(desc->dev), desc);
+}
+
+static int cdx_msi_prepare(struct irq_domain *msi_domain,
+			   struct device *dev,
+			   int nvec, msi_alloc_info_t *info)
+{
+	struct cdx_device *cdx_dev = to_cdx_device(dev);
+	struct device *parent = dev->parent;
+	struct msi_domain_info *msi_info;
+	u32 dev_id = 0;
+	int ret;
+
+	/* Retrieve device ID from requestor ID using parent device */
+	ret = of_map_id(parent->of_node, cdx_dev->msi_dev_id, "msi-map",
+			"msi-map-mask", NULL, &dev_id);
+	if (ret) {
+		dev_err(dev, "of_map_id failed for MSI: %d\n", ret);
+		return ret;
+	}
+
+	/* Set the device Id to be passed to the GIC-ITS */
+	info->scratchpad[0].ul = dev_id;
+
+	msi_info = msi_get_domain_info(msi_domain->parent);
+
+	return msi_info->ops->msi_prepare(msi_domain->parent, dev, nvec, info);
+}
+
+static struct msi_domain_ops cdx_msi_ops = {
+	.msi_prepare	= cdx_msi_prepare,
+	.set_desc	= cdx_msi_set_desc
+};
+
+static struct msi_domain_info cdx_msi_domain_info = {
+	.ops	= &cdx_msi_ops,
+	.chip	= &cdx_msi_irq_chip,
+	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+		  MSI_FLAG_ALLOC_SIMPLE_MSI_DESCS | MSI_FLAG_FREE_MSI_DESCS
+};
+
+struct irq_domain *cdx_msi_domain_init(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct fwnode_handle *fwnode_handle;
+	struct irq_domain *cdx_msi_domain;
+	struct device_node *parent_node;
+	struct irq_domain *parent;
+
+	fwnode_handle = of_node_to_fwnode(np);
+
+	parent_node = of_parse_phandle(np, "msi-map", 1);
+	if (!parent_node) {
+		dev_err(dev, "msi-map not present on cdx controller\n");
+		return NULL;
+	}
+
+	parent = irq_find_matching_fwnode(of_node_to_fwnode(parent_node),
+					  DOMAIN_BUS_NEXUS);
+	if (!parent || !msi_get_domain_info(parent)) {
+		dev_err(dev, "unable to locate ITS domain\n");
+		return NULL;
+	}
+
+	cdx_msi_domain = msi_create_irq_domain(fwnode_handle, &cdx_msi_domain_info,
+					       parent);
+	if (!cdx_msi_domain) {
+		dev_err(dev, "unable to create CDX-MSI domain\n");
+		return NULL;
+	}
+
+	dev_dbg(dev, "CDX-MSI domain created\n");
+
+	return cdx_msi_domain;
+}
+EXPORT_SYMBOL_GPL(cdx_msi_domain_init);
diff --git a/drivers/cdx/controller/cdx_controller.c b/drivers/cdx/controller/cdx_controller.c
index bb4ae7970e21..22f53896cd97 100644
--- a/drivers/cdx/controller/cdx_controller.c
+++ b/drivers/cdx/controller/cdx_controller.c
@@ -9,6 +9,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/cdx/cdx_bus.h>
+#include <linux/irqdomain.h>
 
 #include "cdx_controller.h"
 #include "../cdx.h"
@@ -50,9 +51,20 @@ static int cdx_configure_device(struct cdx_controller *cdx,
 				u8 bus_num, u8 dev_num,
 				struct cdx_device_config *dev_config)
 {
+	u16 msi_index;
 	int ret = 0;
+	u32 data;
+	u64 addr;
 
 	switch (dev_config->type) {
+	case CDX_DEV_MSI_CONF:
+		msi_index = dev_config->msi.msi_index;
+		data = dev_config->msi.data;
+		addr = dev_config->msi.addr;
+
+		ret = cdx_mcdi_write_msi(cdx->priv, bus_num, dev_num,
+					 msi_index, addr, data);
+		break;
 	case CDX_DEV_RESET_CONF:
 		ret = cdx_mcdi_reset_device(cdx->priv, bus_num, dev_num);
 		break;
@@ -155,6 +167,14 @@ static int xlnx_cdx_probe(struct platform_device *pdev)
 	cdx->priv = cdx_mcdi;
 	cdx->ops = &cdx_ops;
 
+	/* Create MSI domain */
+	cdx->msi_domain = cdx_msi_domain_init(&pdev->dev);
+	if (!cdx->msi_domain) {
+		dev_err(&pdev->dev, "cdx_msi_domain_init() failed");
+		ret = -ENODEV;
+		goto cdx_msi_fail;
+	}
+
 	ret = cdx_setup_rpmsg(pdev);
 	if (ret) {
 		if (ret != -EPROBE_DEFER)
@@ -166,6 +186,8 @@ static int xlnx_cdx_probe(struct platform_device *pdev)
 	return 0;
 
 cdx_rpmsg_fail:
+	irq_domain_remove(cdx->msi_domain);
+cdx_msi_fail:
 	kfree(cdx);
 cdx_alloc_fail:
 	cdx_mcdi_finish(cdx_mcdi);
@@ -182,6 +204,7 @@ static int xlnx_cdx_remove(struct platform_device *pdev)
 
 	cdx_destroy_rpmsg(pdev);
 
+	irq_domain_remove(cdx->msi_domain);
 	kfree(cdx);
 
 	cdx_mcdi_finish(cdx_mcdi);
diff --git a/drivers/cdx/controller/mc_cdx_pcol.h b/drivers/cdx/controller/mc_cdx_pcol.h
index 4ccb7b52951b..ff2d8d2bc9ef 100644
--- a/drivers/cdx/controller/mc_cdx_pcol.h
+++ b/drivers/cdx/controller/mc_cdx_pcol.h
@@ -455,6 +455,12 @@
 #define MC_CMD_CDX_BUS_GET_DEVICE_CONFIG_OUT_REQUESTER_ID_OFST			84
 #define MC_CMD_CDX_BUS_GET_DEVICE_CONFIG_OUT_REQUESTER_ID_LEN			4
 
+/* MC_CMD_CDX_BUS_GET_DEVICE_CONFIG_OUT_V2 msgresponse */
+#define MC_CMD_CDX_BUS_GET_DEVICE_CONFIG_OUT_V2_LEN				92
+/* Requester ID used by device for GIC ITS DeviceID */
+#define MC_CMD_CDX_BUS_GET_DEVICE_CONFIG_OUT_V2_REQUESTER_DEVICE_ID_OFST	88
+#define MC_CMD_CDX_BUS_GET_DEVICE_CONFIG_OUT_V2_REQUESTER_DEVICE_ID_LEN		4
+
 /***********************************/
 /*
  * MC_CMD_CDX_DEVICE_RESET
@@ -562,6 +568,64 @@
 #define MC_CMD_CDX_DEVICE_CONTROL_GET_OUT_MMIO_REGIONS_ENABLE_LBN	2
 #define MC_CMD_CDX_DEVICE_CONTROL_GET_OUT_MMIO_REGIONS_ENABLE_WIDTH	1
 
+/***********************************/
+/*
+ * MC_CMD_CDX_DEVICE_WRITE_MSI_MSG
+ * Populates the MSI message to be used by the hardware to raise the specified
+ * interrupt vector. Versal-net implementation specific limitations are that
+ * only 4 CDX devices with MSI interrupt capability are supported and all
+ * vectors within a device must use the same write address. The command will
+ * return EINVAL if any of these limitations is violated.
+ */
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG					0x9
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_MSGSET				0x9
+#undef MC_CMD_0x9_PRIVILEGE_CTG
+
+#define MC_CMD_0x9_PRIVILEGE_CTG SRIOV_CTG_ADMIN
+
+/* MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN msgrequest */
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_LEN				28
+/* Device bus number, in range 0 to BUS_COUNT-1 */
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_BUS_OFST			0
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_BUS_LEN			4
+/* Device number relative to the bus, in range 0 to DEVICE_COUNT-1 for that bus */
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_DEVICE_OFST			4
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_DEVICE_LEN			4
+/*
+ * Device-relative MSI vector number. Must be < MSI_COUNT reported for the
+ * device.
+ */
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_VECTOR_OFST		8
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_VECTOR_LEN		4
+/* Reserved (alignment) */
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_RESERVED_OFST		12
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_RESERVED_LEN			4
+/*
+ * MSI address to be used by the hardware. Typically, on ARM systems this
+ * address is translated by the IOMMU (if enabled) and it is the responsibility
+ * of the entity managing the IOMMU (APU kernel) to supply the correct IOVA
+ * here.
+ */
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_ADDRESS_OFST		16
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_ADDRESS_LEN		8
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_ADDRESS_LO_OFST		16
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_ADDRESS_LO_LEN		4
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_ADDRESS_LO_LBN		128
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_ADDRESS_LO_WIDTH		32
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_ADDRESS_HI_OFST		20
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_ADDRESS_HI_LEN		4
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_ADDRESS_HI_LBN		160
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_ADDRESS_HI_WIDTH		32
+/*
+ * MSI data to be used by the hardware. On versal-net, only the lower 16-bits
+ * are used, the remaining bits are ignored and should be set to zero.
+ */
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_DATA_OFST		24
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_DATA_LEN			4
+
+/* MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_OUT msgresponse */
+#define MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_OUT_LEN				0
+
 /***********************************/
 /* MC_CMD_V2_EXTN - Encapsulation for a v2 extended command */
 #define MC_CMD_V2_EXTN					0x7f
diff --git a/drivers/cdx/controller/mcdi_functions.c b/drivers/cdx/controller/mcdi_functions.c
index 0158f26533dd..901ec1f53c85 100644
--- a/drivers/cdx/controller/mcdi_functions.c
+++ b/drivers/cdx/controller/mcdi_functions.c
@@ -49,7 +49,7 @@ int cdx_mcdi_get_dev_config(struct cdx_mcdi *cdx,
 			    u8 bus_num, u8 dev_num,
 			    struct cdx_dev_params *dev_params)
 {
-	MCDI_DECLARE_BUF(outbuf, MC_CMD_CDX_BUS_GET_DEVICE_CONFIG_OUT_LEN);
+	MCDI_DECLARE_BUF(outbuf, MC_CMD_CDX_BUS_GET_DEVICE_CONFIG_OUT_V2_LEN);
 	MCDI_DECLARE_BUF(inbuf, MC_CMD_CDX_BUS_GET_DEVICE_CONFIG_IN_LEN);
 	struct resource *res = &dev_params->res[0];
 	size_t outlen;
@@ -64,7 +64,7 @@ int cdx_mcdi_get_dev_config(struct cdx_mcdi *cdx,
 	if (ret)
 		return ret;
 
-	if (outlen != MC_CMD_CDX_BUS_GET_DEVICE_CONFIG_OUT_LEN)
+	if (outlen != MC_CMD_CDX_BUS_GET_DEVICE_CONFIG_OUT_V2_LEN)
 		return -EIO;
 
 	dev_params->bus_num = bus_num;
@@ -73,6 +73,9 @@ int cdx_mcdi_get_dev_config(struct cdx_mcdi *cdx,
 	req_id = MCDI_DWORD(outbuf, CDX_BUS_GET_DEVICE_CONFIG_OUT_REQUESTER_ID);
 	dev_params->req_id = req_id;
 
+	dev_params->msi_dev_id = MCDI_DWORD(outbuf,
+					    CDX_BUS_GET_DEVICE_CONFIG_OUT_V2_REQUESTER_DEVICE_ID);
+
 	dev_params->res_count = 0;
 	if (MCDI_QWORD(outbuf, CDX_BUS_GET_DEVICE_CONFIG_OUT_MMIO_REGION0_SIZE) != 0) {
 		res[dev_params->res_count].start =
@@ -120,10 +123,29 @@ int cdx_mcdi_get_dev_config(struct cdx_mcdi *cdx,
 
 	dev_params->vendor = MCDI_WORD(outbuf, CDX_BUS_GET_DEVICE_CONFIG_OUT_VENDOR_ID);
 	dev_params->device = MCDI_WORD(outbuf, CDX_BUS_GET_DEVICE_CONFIG_OUT_DEVICE_ID);
+	dev_params->num_msi = MCDI_DWORD(outbuf, CDX_BUS_GET_DEVICE_CONFIG_OUT_MSI_COUNT);
 
 	return 0;
 }
 
+int cdx_mcdi_write_msi(struct cdx_mcdi *cdx, u8 bus_num, u8 dev_num,
+		       u32 msi_vector, u64 msi_address, u32 msi_data)
+{
+	MCDI_DECLARE_BUF(inbuf, MC_CMD_CDX_DEVICE_WRITE_MSI_MSG_IN_LEN);
+	int ret;
+
+	MCDI_SET_DWORD(inbuf, CDX_DEVICE_WRITE_MSI_MSG_IN_BUS, bus_num);
+	MCDI_SET_DWORD(inbuf, CDX_DEVICE_WRITE_MSI_MSG_IN_DEVICE, dev_num);
+	MCDI_SET_DWORD(inbuf, CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_VECTOR, msi_vector);
+	MCDI_SET_QWORD(inbuf, CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_ADDRESS, msi_address);
+	MCDI_SET_DWORD(inbuf, CDX_DEVICE_WRITE_MSI_MSG_IN_MSI_DATA, msi_data);
+
+	ret = cdx_mcdi_rpc(cdx, MC_CMD_CDX_DEVICE_WRITE_MSI_MSG, inbuf, sizeof(inbuf),
+			   NULL, 0, NULL);
+
+	return ret;
+}
+
 int cdx_mcdi_reset_device(struct cdx_mcdi *cdx, u8 bus_num, u8 dev_num)
 {
 	MCDI_DECLARE_BUF(inbuf, MC_CMD_CDX_DEVICE_RESET_IN_LEN);
diff --git a/drivers/cdx/controller/mcdi_functions.h b/drivers/cdx/controller/mcdi_functions.h
index 7440ace5539a..35a300727f34 100644
--- a/drivers/cdx/controller/mcdi_functions.h
+++ b/drivers/cdx/controller/mcdi_functions.h
@@ -47,6 +47,26 @@ int cdx_mcdi_get_dev_config(struct cdx_mcdi *cdx,
 			    u8 bus_num, u8 dev_num,
 			    struct cdx_dev_params *dev_params);
 
+/**
+ * cdx_mcdi_write_msi - Write MSI configuration for CDX device
+ * @cdx: pointer to MCDI interface.
+ * @bus_num: Bus number.
+ * @dev_num: Device number.
+ * @msi_vector: Device-relative MSI vector number.
+ *	Must be < MSI_COUNT reported for the device.
+ * @msi_address: MSI address to be used by the hardware. Typically, on ARM
+ *	systems this address is translated by the IOMMU (if enabled) and
+ *	it is the responsibility of the entity managing the IOMMU (APU kernel)
+ *	to supply the correct IOVA here.
+ * @msi_data: MSI data to be used by the hardware. On versal-net, only the
+ *	lower 16-bits are used, the remaining bits are ignored and should be
+ *	set to zero.
+ *
+ * Return: 0 on success, <0 on failure
+ */
+int cdx_mcdi_write_msi(struct cdx_mcdi *cdx, u8 bus_num, u8 dev_num,
+		       u32 msi_vector, u64 msi_address, u32 msi_data);
+
 /**
  * cdx_mcdi_reset_device - Reset cdx device represented by bus_num:dev_num
  * @cdx: pointer to MCDI interface.
diff --git a/include/linux/cdx/cdx_bus.h b/include/linux/cdx/cdx_bus.h
index bead71b7bc73..7ff8dbf2c1f1 100644
--- a/include/linux/cdx/cdx_bus.h
+++ b/include/linux/cdx/cdx_bus.h
@@ -21,11 +21,19 @@
 struct cdx_controller;
 
 enum {
+	CDX_DEV_MSI_CONF,
 	CDX_DEV_RESET_CONF,
 };
 
+struct cdx_msi_config {
+	u16 msi_index;
+	u32 data;
+	u64 addr;
+};
+
 struct cdx_device_config {
 	u8 type;
+	struct cdx_msi_config msi;
 };
 
 typedef int (*cdx_scan_cb)(struct cdx_controller *cdx);
@@ -62,12 +70,14 @@ struct cdx_ops {
  * struct cdx_controller: CDX controller object
  * @dev: Linux device associated with the CDX controller.
  * @priv: private data
+ * @msi_domain: MSI domain
  * @id: Controller ID
  * @ops: CDX controller ops
  */
 struct cdx_controller {
 	struct device *dev;
 	void *priv;
+	struct irq_domain *msi_domain;
 	u32 id;
 	struct cdx_ops *ops;
 };
@@ -86,9 +96,13 @@ struct cdx_controller {
  * @dma_mask: Default DMA mask
  * @flags: CDX device flags
  * @req_id: Requestor ID associated with CDX device
+ * @msi_dev_id: MSI Device ID associated with CDX device
+ * @num_msi: Number of MSI's supported by the device
  * @driver_override: driver name to force a match; do not set directly,
  *                   because core frees it; use driver_set_override() to
  *                   set or clear it.
+ * @irqchip_lock: lock to synchronize irq/msi configuration
+ * @msi_write_pending: MSI write pending for this device
  */
 struct cdx_device {
 	struct device dev;
@@ -102,7 +116,11 @@ struct cdx_device {
 	u64 dma_mask;
 	u16 flags;
 	u32 req_id;
+	u32 msi_dev_id;
+	u32 num_msi;
 	const char *driver_override;
+	struct mutex irqchip_lock; /* Serialize write msi configuration */
+	bool msi_write_pending;
 };
 
 #define to_cdx_device(_dev) \
@@ -162,6 +180,20 @@ void cdx_driver_unregister(struct cdx_driver *cdx_driver);
 
 extern struct bus_type cdx_bus_type;
 
+/**
+ * cdx_msi_domain_alloc_irqs - Allocate MSI's for the CDX device
+ * @dev: device pointer
+ * @irq_count: Number of MSI's to be allocated
+ *
+ * Return: 0 for success, -errno on failure
+ */
+int cdx_msi_domain_alloc_irqs(struct device *dev, unsigned int irq_count);
+
+/**
+ * cdx_msi_domain_free_irqs - Free MSI's for CDX device
+ */
+#define cdx_msi_domain_free_irqs msi_domain_free_irqs_all
+
 /**
  * cdx_dev_reset - Reset CDX device
  * @dev: device pointer
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index b4c31a5c1147..ffdd61a90027 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -1637,6 +1637,7 @@ void msi_domain_free_irqs_all(struct device *dev, unsigned int domid)
 	msi_domain_free_irqs_all_locked(dev, domid);
 	msi_unlock_descs(dev);
 }
+EXPORT_SYMBOL_GPL(msi_domain_free_irqs_all);
 
 /**
  * msi_get_domain_info - Get the MSI interrupt domain info for @domain
-- 
2.17.1

Re: [PATCH v4] cdx: add MSI support for CDX bus

[Gupta, Nipun]

Hi Greg,

On 9/11/2023 7:22 PM, Nipun Gupta wrote:
> Add CDX-MSI domain per CDX controller with gic-its domain as
> a parent, to support MSI for CDX devices. CDX devices allocate
> MSIs from the CDX domain. Also, introduce APIs to alloc and free
> IRQs for CDX domain.
> 
> In CDX subsystem firmware is a controller for all devices and
> their configuration. CDX bus controller sends all the write_msi_msg
> commands to firmware running on RPU and the firmware interfaces with
> actual devices to pass this information to devices
> 
> Since, CDX controller is the only way to communicate with the Firmware
> for MSI write info, CDX domain per controller required in contrast to
> having a CDX domain per device.
> 
> Co-developed-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> Signed-off-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> Co-developed-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
> Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>

Please help in making progress on this patch. Is there anything that 
needs to be updated in this patch for CDX bus?

Thanks,
Nipun

Re: [PATCH v4] cdx: add MSI support for CDX bus

[Greg KH]

On Tue, Sep 26, 2023 at 03:18:58PM +0530, Gupta, Nipun wrote:
> Hi Greg,
> 
> On 9/11/2023 7:22 PM, Nipun Gupta wrote:
> > Add CDX-MSI domain per CDX controller with gic-its domain as
> > a parent, to support MSI for CDX devices. CDX devices allocate
> > MSIs from the CDX domain. Also, introduce APIs to alloc and free
> > IRQs for CDX domain.
> > 
> > In CDX subsystem firmware is a controller for all devices and
> > their configuration. CDX bus controller sends all the write_msi_msg
> > commands to firmware running on RPU and the firmware interfaces with
> > actual devices to pass this information to devices
> > 
> > Since, CDX controller is the only way to communicate with the Firmware
> > for MSI write info, CDX domain per controller required in contrast to
> > having a CDX domain per device.
> > 
> > Co-developed-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> > Signed-off-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> > Co-developed-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> > Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
> > Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> 
> Please help in making progress on this patch. Is there anything that needs
> to be updated in this patch for CDX bus?

$ mdfrm -c ~/mail/todo/
2031 messages in /home/gregkh/mail/todo/

So perhaps help in reviewing other pending patches for other subsystems?
It's in my queue, but have been traveling for 2 weeks for conferences,
will be catching up next week when I get a chance.

Also, you need to get the msi/interrupt developers to agree with this,
why not get their review and acceptance first?

thanks,

greg k-h

Re: [PATCH v4] cdx: add MSI support for CDX bus

[Gupta, Nipun]

On 9/26/2023 3:25 PM, Greg KH wrote:
> On Tue, Sep 26, 2023 at 03:18:58PM +0530, Gupta, Nipun wrote:
>> Hi Greg,
>>
>> On 9/11/2023 7:22 PM, Nipun Gupta wrote:
>>> Add CDX-MSI domain per CDX controller with gic-its domain as
>>> a parent, to support MSI for CDX devices. CDX devices allocate
>>> MSIs from the CDX domain. Also, introduce APIs to alloc and free
>>> IRQs for CDX domain.
>>>
>>> In CDX subsystem firmware is a controller for all devices and
>>> their configuration. CDX bus controller sends all the write_msi_msg
>>> commands to firmware running on RPU and the firmware interfaces with
>>> actual devices to pass this information to devices
>>>
>>> Since, CDX controller is the only way to communicate with the Firmware
>>> for MSI write info, CDX domain per controller required in contrast to
>>> having a CDX domain per device.
>>>
>>> Co-developed-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
>>> Signed-off-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
>>> Co-developed-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
>>> Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
>>> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
>>> Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
>>> Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
>>
>> Please help in making progress on this patch. Is there anything that needs
>> to be updated in this patch for CDX bus?
> 
> $ mdfrm -c ~/mail/todo/
> 2031 messages in /home/gregkh/mail/todo/
> 
> So perhaps help in reviewing other pending patches for other subsystems?
> It's in my queue, but have been traveling for 2 weeks for conferences,
> will be catching up next week when I get a chance.
> 
> Also, you need to get the msi/interrupt developers to agree with this,
> why not get their review and acceptance first?

Thanks for quick response and sorry for prodding. I will check with 
Thomas/Marc for their feedback.

Regards,
Nipun

> 
> thanks,
> 
> greg k-h

Re: [PATCH v4] cdx: add MSI support for CDX bus

[Gupta, Nipun]

Hi Thomas/Marc,

On 9/11/2023 7:22 PM, Nipun Gupta wrote:
> Add CDX-MSI domain per CDX controller with gic-its domain as
> a parent, to support MSI for CDX devices. CDX devices allocate
> MSIs from the CDX domain. Also, introduce APIs to alloc and free
> IRQs for CDX domain.
> 
> In CDX subsystem firmware is a controller for all devices and
> their configuration. CDX bus controller sends all the write_msi_msg
> commands to firmware running on RPU and the firmware interfaces with
> actual devices to pass this information to devices
> 
> Since, CDX controller is the only way to communicate with the Firmware
> for MSI write info, CDX domain per controller required in contrast to
> having a CDX domain per device.
> 
> Co-developed-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> Signed-off-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> Co-developed-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
> Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> ---
> 
> Changes v3->v4:
> - Rebased on Linux 6.6-rc1

Can you please help in reviewing and making progress on this patch?

Thanks,
Nipun

Re: [PATCH v4] cdx: add MSI support for CDX bus

[Greg KH]

On Mon, Sep 11, 2023 at 07:22:59PM +0530, Nipun Gupta wrote:
> Add CDX-MSI domain per CDX controller with gic-its domain as
> a parent, to support MSI for CDX devices. CDX devices allocate
> MSIs from the CDX domain. Also, introduce APIs to alloc and free
> IRQs for CDX domain.
> 
> In CDX subsystem firmware is a controller for all devices and
> their configuration. CDX bus controller sends all the write_msi_msg
> commands to firmware running on RPU and the firmware interfaces with
> actual devices to pass this information to devices
> 
> Since, CDX controller is the only way to communicate with the Firmware
> for MSI write info, CDX domain per controller required in contrast to
> having a CDX domain per device.
> 
> Co-developed-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> Signed-off-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> Co-developed-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
> Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> ---
> 
> Changes v3->v4:
> - Rebased on Linux 6.6-rc1
> 
> Changes v2->v3:
> - Rebased on Linux 6.5-rc1
> - Used FW provided 'msi_dev_id' as device ID for GIC instead of 'req_id'.
> 
> Changes v1->v2:
> - fixed scenario where msi write was called asyncronously in
>   an atomic context, by using irq_chip_(un)lock, and using sync
>   MCDI API for write MSI message.
> - fixed broken Signed-off-by chain.
> 
>  drivers/cdx/Kconfig                     |   1 +
>  drivers/cdx/Makefile                    |   2 +-
>  drivers/cdx/cdx.c                       |   9 ++
>  drivers/cdx/cdx.h                       |  12 ++
>  drivers/cdx/cdx_msi.c                   | 183 ++++++++++++++++++++++++
>  drivers/cdx/controller/cdx_controller.c |  23 +++
>  drivers/cdx/controller/mc_cdx_pcol.h    |  64 +++++++++
>  drivers/cdx/controller/mcdi_functions.c |  26 +++-
>  drivers/cdx/controller/mcdi_functions.h |  20 +++
>  include/linux/cdx/cdx_bus.h             |  32 +++++
>  kernel/irq/msi.c                        |   1 +
>  11 files changed, 370 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/cdx/cdx_msi.c
> 
> diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
> index a08958485e31..86df7ccb76bb 100644
> --- a/drivers/cdx/Kconfig
> +++ b/drivers/cdx/Kconfig
> @@ -8,6 +8,7 @@
>  config CDX_BUS
>  	bool "CDX Bus driver"
>  	depends on OF && ARM64
> +	select GENERIC_MSI_IRQ_DOMAIN

This config option isn't in my tree anywhere, where did it come from?
What is it supposed to do?

>  	help
>  	  Driver to enable Composable DMA Transfer(CDX) Bus. CDX bus
>  	  exposes Fabric devices which uses composable DMA IP to the
> diff --git a/drivers/cdx/Makefile b/drivers/cdx/Makefile
> index 0324e4914f6e..4bad79d1d188 100644
> --- a/drivers/cdx/Makefile
> +++ b/drivers/cdx/Makefile
> @@ -5,4 +5,4 @@
>  # Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
>  #
>  
> -obj-$(CONFIG_CDX_BUS) += cdx.o controller/
> +obj-$(CONFIG_CDX_BUS) += cdx.o cdx_msi.o controller/

So you are always building this in even if the build doesn't support
MSI?  Why will that not break the build?


> diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
> index d2cad4c670a0..8d777cdacf1d 100644
> --- a/drivers/cdx/cdx.c
> +++ b/drivers/cdx/cdx.c
> @@ -56,6 +56,7 @@
>   */
>  
>  #include <linux/init.h>
> +#include <linux/irqdomain.h>
>  #include <linux/kernel.h>
>  #include <linux/of_device.h>
>  #include <linux/slab.h>
> @@ -478,6 +479,7 @@ int cdx_device_add(struct cdx_dev_params *dev_params)
>  
>  	/* Populate CDX dev params */
>  	cdx_dev->req_id = dev_params->req_id;
> +	cdx_dev->msi_dev_id = dev_params->msi_dev_id;
>  	cdx_dev->vendor = dev_params->vendor;
>  	cdx_dev->device = dev_params->device;
>  	cdx_dev->bus_num = dev_params->bus_num;
> @@ -491,12 +493,19 @@ int cdx_device_add(struct cdx_dev_params *dev_params)
>  	cdx_dev->dev.bus = &cdx_bus_type;
>  	cdx_dev->dev.dma_mask = &cdx_dev->dma_mask;
>  	cdx_dev->dev.release = cdx_device_release;
> +	cdx_dev->msi_write_pending = false;
> +	mutex_init(&cdx_dev->irqchip_lock);
>  
>  	/* Set Name */
>  	dev_set_name(&cdx_dev->dev, "cdx-%02x:%02x",
>  		     ((cdx->id << CDX_CONTROLLER_ID_SHIFT) | (cdx_dev->bus_num & CDX_BUS_NUM_MASK)),
>  		     cdx_dev->dev_num);
>  
> +	if (cdx->msi_domain) {
> +		cdx_dev->num_msi = dev_params->num_msi;
> +		dev_set_msi_domain(&cdx_dev->dev, cdx->msi_domain);
> +	}
> +
>  	ret = device_add(&cdx_dev->dev);
>  	if (ret) {
>  		dev_err(&cdx_dev->dev,
> diff --git a/drivers/cdx/cdx.h b/drivers/cdx/cdx.h
> index c436ac7ac86f..ece11c04d646 100644
> --- a/drivers/cdx/cdx.h
> +++ b/drivers/cdx/cdx.h
> @@ -21,6 +21,8 @@
>   * @res: array of MMIO region entries
>   * @res_count: number of valid MMIO regions
>   * @req_id: Requestor ID associated with CDX device
> + * @msi_dev_id: MSI device ID associated with CDX device
> + * @num_msi: Number of MSI's supported by the device
>   */
>  struct cdx_dev_params {
>  	struct cdx_controller *cdx;
> @@ -31,6 +33,8 @@ struct cdx_dev_params {
>  	struct resource res[MAX_CDX_DEV_RESOURCES];
>  	u8 res_count;
>  	u32 req_id;
> +	u32 msi_dev_id;
> +	u32 num_msi;
>  };
>  
>  /**
> @@ -59,4 +63,12 @@ void cdx_unregister_controller(struct cdx_controller *cdx);
>   */
>  int cdx_device_add(struct cdx_dev_params *dev_params);
>  
> +/**
> + * cdx_msi_domain_init - Init the CDX bus MSI domain.
> + * @dev: Device of the CDX bus controller
> + *
> + * Return: CDX MSI domain, NULL on failure
> + */
> +struct irq_domain *cdx_msi_domain_init(struct device *dev);
> +
>  #endif /* _CDX_H_ */
> diff --git a/drivers/cdx/cdx_msi.c b/drivers/cdx/cdx_msi.c
> new file mode 100644
> index 000000000000..d7f4c88428d6
> --- /dev/null
> +++ b/drivers/cdx/cdx_msi.c
> @@ -0,0 +1,183 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD CDX bus driver MSI support
> + *
> + * Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
> + */
> +
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/msi.h>
> +#include <linux/cdx/cdx_bus.h>
> +
> +#include "cdx.h"
> +
> +static void cdx_msi_write_msg(struct irq_data *irq_data,
> +			      struct msi_msg *msg)
> +{
> +	struct msi_desc *msi_desc = irq_data_get_msi_desc(irq_data);
> +	struct cdx_device *cdx_dev = to_cdx_device(msi_desc->dev);
> +
> +	/* We would not operate on msg here rather we wait for
> +	 * irq_bus_sync_unlock() to be called from preemptible
> +	 * task context.
> +	 */
> +	msi_desc->msg = *msg;
> +	cdx_dev->msi_write_pending = true;
> +}
> +
> +static void cdx_msi_write_irq_lock(struct irq_data *irq_data)
> +{
> +	struct msi_desc *msi_desc = irq_data_get_msi_desc(irq_data);
> +	struct cdx_device *cdx_dev = to_cdx_device(msi_desc->dev);
> +
> +	mutex_lock(&cdx_dev->irqchip_lock);
> +}
> +
> +static void cdx_msi_write_irq_unlock(struct irq_data *irq_data)
> +{
> +	struct msi_desc *msi_desc = irq_data_get_msi_desc(irq_data);
> +	struct cdx_device *cdx_dev = to_cdx_device(msi_desc->dev);
> +	struct cdx_controller *cdx = cdx_dev->cdx;
> +	struct cdx_device_config dev_config;
> +	int ret;
> +
> +	if (!cdx_dev->msi_write_pending) {
> +		mutex_unlock(&cdx_dev->irqchip_lock);
> +		return;
> +	}
> +
> +	cdx_dev->msi_write_pending = false;
> +	mutex_unlock(&cdx_dev->irqchip_lock);
> +
> +	dev_config.msi.msi_index = msi_desc->msi_index;
> +	dev_config.msi.data = msi_desc->msg.data;
> +	dev_config.msi.addr = ((u64)(msi_desc->msg.address_hi) << 32) |
> +			      msi_desc->msg.address_lo;
> +
> +	dev_config.type = CDX_DEV_MSI_CONF;
> +	ret = cdx->ops->dev_configure(cdx, cdx_dev->bus_num, cdx_dev->dev_num,
> +				      &dev_config);
> +	if (ret)
> +		dev_err(&cdx_dev->dev, "Write MSI failed to CDX controller\n");

How noisy iks this going to be in an irq handler if something goes
wrong?

And you are doing this write outside of the lock, is that intentional?
If so, why not document that?


> +}
> +
> +static struct irq_chip cdx_msi_irq_chip = {
> +	.name			= "CDX-MSI",
> +	.irq_mask		= irq_chip_mask_parent,
> +	.irq_unmask		= irq_chip_unmask_parent,
> +	.irq_eoi		= irq_chip_eoi_parent,
> +	.irq_set_affinity	= msi_domain_set_affinity,
> +	.irq_write_msi_msg	= cdx_msi_write_msg,
> +	.irq_bus_lock		= cdx_msi_write_irq_lock,
> +	.irq_bus_sync_unlock	= cdx_msi_write_irq_unlock
> +};
> +
> +int cdx_msi_domain_alloc_irqs(struct device *dev, unsigned int irq_count)
> +{
> +	int ret;
> +
> +	ret = msi_setup_device_data(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = msi_domain_alloc_irqs_range(dev, MSI_DEFAULT_DOMAIN,
> +					  0, irq_count - 1);
> +	if (ret)
> +		dev_err(dev, "Failed to allocate IRQs: %d\n", ret);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(cdx_msi_domain_alloc_irqs);

meta-comment, CDX really should have a module namespace one of these
days, right?

> +
> +/* Convert an msi_desc to a globally unique identifier. */
> +static irq_hw_number_t cdx_domain_calc_hwirq(struct cdx_device *dev,
> +					     struct msi_desc *desc)
> +{
> +	return ((irq_hw_number_t)dev->msi_dev_id << 10) | desc->msi_index;

How does this make a unique number?  Is msi_dev_id guaranteed to be
unique across all cdx busses?  That's not normally the case for most bus
types, unless you are guaranteeing this in the cdx core somewhere?

And why shift 10 bits?  That's a fun magic number...

> +}
> +
> +static void cdx_msi_set_desc(msi_alloc_info_t *arg,
> +			     struct msi_desc *desc)
> +{
> +	arg->desc = desc;
> +	arg->hwirq = cdx_domain_calc_hwirq(to_cdx_device(desc->dev), desc);
> +}
> +
> +static int cdx_msi_prepare(struct irq_domain *msi_domain,
> +			   struct device *dev,
> +			   int nvec, msi_alloc_info_t *info)
> +{
> +	struct cdx_device *cdx_dev = to_cdx_device(dev);
> +	struct device *parent = dev->parent;
> +	struct msi_domain_info *msi_info;
> +	u32 dev_id = 0;

No need to set this, right?

> +	int ret;
> +
> +	/* Retrieve device ID from requestor ID using parent device */
> +	ret = of_map_id(parent->of_node, cdx_dev->msi_dev_id, "msi-map",
> +			"msi-map-mask", NULL, &dev_id);
> +	if (ret) {
> +		dev_err(dev, "of_map_id failed for MSI: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Set the device Id to be passed to the GIC-ITS */
> +	info->scratchpad[0].ul = dev_id;
> +
> +	msi_info = msi_get_domain_info(msi_domain->parent);
> +
> +	return msi_info->ops->msi_prepare(msi_domain->parent, dev, nvec, info);

Do you know that there will be a ops pointer here?  And the msi_prepare
callback?  Or will this always just be:

> +}
> +
> +static struct msi_domain_ops cdx_msi_ops = {
> +	.msi_prepare	= cdx_msi_prepare,
> +	.set_desc	= cdx_msi_set_desc

This structure?

> +};
> +
> +static struct msi_domain_info cdx_msi_domain_info = {
> +	.ops	= &cdx_msi_ops,
> +	.chip	= &cdx_msi_irq_chip,
> +	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> +		  MSI_FLAG_ALLOC_SIMPLE_MSI_DESCS | MSI_FLAG_FREE_MSI_DESCS
> +};
> +
> +struct irq_domain *cdx_msi_domain_init(struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct fwnode_handle *fwnode_handle;
> +	struct irq_domain *cdx_msi_domain;
> +	struct device_node *parent_node;
> +	struct irq_domain *parent;
> +
> +	fwnode_handle = of_node_to_fwnode(np);
> +
> +	parent_node = of_parse_phandle(np, "msi-map", 1);
> +	if (!parent_node) {
> +		dev_err(dev, "msi-map not present on cdx controller\n");

Is this a requirement now?  What about systems without it?

> +struct cdx_msi_config {
> +	u16 msi_index;
> +	u32 data;
> +	u64 addr;
> +};

Are you ok with the "hole" in this structure?

And I really need a msi maintainer to review this before I can take it,
thanks.

greg k-h

Re: [PATCH v4] cdx: add MSI support for CDX bus

[Thomas Gleixner]

On Thu, Oct 05 2023 at 12:24, Greg KH wrote:
>> diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
>> index a08958485e31..86df7ccb76bb 100644
>> --- a/drivers/cdx/Kconfig
>> +++ b/drivers/cdx/Kconfig
>> @@ -8,6 +8,7 @@
>>  config CDX_BUS
>>  	bool "CDX Bus driver"
>>  	depends on OF && ARM64
>> +	select GENERIC_MSI_IRQ_DOMAIN
>
> This config option isn't in my tree anywhere, where did it come from?
> What is it supposed to do?

13e7accb81d6 ("genirq: Get rid of GENERIC_MSI_IRQ_DOMAIN") :)

> And I really need a msi maintainer to review this before I can take it,
> thanks.

I've put it on my ever growing pile of things to look at.

Thanks,

        tglx

Re: [PATCH v4] cdx: add MSI support for CDX bus

[Greg KH]

On Thu, Oct 05, 2023 at 03:46:34PM +0200, Thomas Gleixner wrote:
> On Thu, Oct 05 2023 at 12:24, Greg KH wrote:
> >> diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
> >> index a08958485e31..86df7ccb76bb 100644
> >> --- a/drivers/cdx/Kconfig
> >> +++ b/drivers/cdx/Kconfig
> >> @@ -8,6 +8,7 @@
> >>  config CDX_BUS
> >>  	bool "CDX Bus driver"
> >>  	depends on OF && ARM64
> >> +	select GENERIC_MSI_IRQ_DOMAIN
> >
> > This config option isn't in my tree anywhere, where did it come from?
> > What is it supposed to do?
> 
> 13e7accb81d6 ("genirq: Get rid of GENERIC_MSI_IRQ_DOMAIN") :)

Ok, so this hasn't been tested since the 6.2 release?  Wow, I think
someone from AMD needs to take a deep look at this and verify that it
actually is doing what it is supposed to be doing...

thanks,

greg k-h

Re: [PATCH v4] cdx: add MSI support for CDX bus

[Gupta, Nipun]

Hi Greg,

On 10/5/2023 7:30 PM, Greg KH wrote:
> On Thu, Oct 05, 2023 at 03:46:34PM +0200, Thomas Gleixner wrote:
>> On Thu, Oct 05 2023 at 12:24, Greg KH wrote:
>>>> diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
>>>> index a08958485e31..86df7ccb76bb 100644
>>>> --- a/drivers/cdx/Kconfig
>>>> +++ b/drivers/cdx/Kconfig
>>>> @@ -8,6 +8,7 @@
>>>>   config CDX_BUS
>>>>   	bool "CDX Bus driver"
>>>>   	depends on OF && ARM64
>>>> +	select GENERIC_MSI_IRQ_DOMAIN
>>>
>>> This config option isn't in my tree anywhere, where did it come from?
>>> What is it supposed to do?
>>
>> 13e7accb81d6 ("genirq: Get rid of GENERIC_MSI_IRQ_DOMAIN") :)
> 
> Ok, so this hasn't been tested since the 6.2 release?  Wow, I think
> someone from AMD needs to take a deep look at this and verify that it
> actually is doing what it is supposed to be doing...

The patch Thomas mentioned renames "GENERIC_MSI_IRQ_DOMAIN" to 
"GENERIC_MSI_IRQ"; and in our testing "GENERIC_MSI_IRQ" is also selected 
as 'ARM64' is enabled which enables 'ARM_GIC_V3_ITS' which in-turn 
selects 'GENERIC_MSI_IRQ'.

The patch is tested for MSI functionality on 6.6-rc1. We will re-look 
into the config dependencies to avoid such issues, but please be assured 
that the patch has been validated.

Thanks,
Nipun

> 
> thanks,
> 
> greg k-h

Re: [PATCH v4] cdx: add MSI support for CDX bus

[Greg KH]

On Thu, Oct 05, 2023 at 08:07:35PM +0530, Gupta, Nipun wrote:
> Hi Greg,
> 
> On 10/5/2023 7:30 PM, Greg KH wrote:
> > On Thu, Oct 05, 2023 at 03:46:34PM +0200, Thomas Gleixner wrote:
> > > On Thu, Oct 05 2023 at 12:24, Greg KH wrote:
> > > > > diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
> > > > > index a08958485e31..86df7ccb76bb 100644
> > > > > --- a/drivers/cdx/Kconfig
> > > > > +++ b/drivers/cdx/Kconfig
> > > > > @@ -8,6 +8,7 @@
> > > > >   config CDX_BUS
> > > > >   	bool "CDX Bus driver"
> > > > >   	depends on OF && ARM64
> > > > > +	select GENERIC_MSI_IRQ_DOMAIN
> > > > 
> > > > This config option isn't in my tree anywhere, where did it come from?
> > > > What is it supposed to do?
> > > 
> > > 13e7accb81d6 ("genirq: Get rid of GENERIC_MSI_IRQ_DOMAIN") :)
> > 
> > Ok, so this hasn't been tested since the 6.2 release?  Wow, I think
> > someone from AMD needs to take a deep look at this and verify that it
> > actually is doing what it is supposed to be doing...
> 
> The patch Thomas mentioned renames "GENERIC_MSI_IRQ_DOMAIN" to
> "GENERIC_MSI_IRQ"; and in our testing "GENERIC_MSI_IRQ" is also selected as
> 'ARM64' is enabled which enables 'ARM_GIC_V3_ITS' which in-turn selects
> 'GENERIC_MSI_IRQ'.

Ok, but that's not what this patch "selects" at all :(

> The patch is tested for MSI functionality on 6.6-rc1. We will re-look into
> the config dependencies to avoid such issues, but please be assured that the
> patch has been validated.

How has the dependancy been validated as correct if there is no such
thing in this kernel?

confused,

greg k-h

Re: [PATCH v4] cdx: add MSI support for CDX bus

[Gupta, Nipun]

On 10/5/2023 8:24 PM, Greg KH wrote:
> On Thu, Oct 05, 2023 at 08:07:35PM +0530, Gupta, Nipun wrote:
>> Hi Greg,
>>
>> On 10/5/2023 7:30 PM, Greg KH wrote:
>>> On Thu, Oct 05, 2023 at 03:46:34PM +0200, Thomas Gleixner wrote:
>>>> On Thu, Oct 05 2023 at 12:24, Greg KH wrote:
>>>>>> diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
>>>>>> index a08958485e31..86df7ccb76bb 100644
>>>>>> --- a/drivers/cdx/Kconfig
>>>>>> +++ b/drivers/cdx/Kconfig
>>>>>> @@ -8,6 +8,7 @@
>>>>>>    config CDX_BUS
>>>>>>    	bool "CDX Bus driver"
>>>>>>    	depends on OF && ARM64
>>>>>> +	select GENERIC_MSI_IRQ_DOMAIN
>>>>>
>>>>> This config option isn't in my tree anywhere, where did it come from?
>>>>> What is it supposed to do?
>>>>
>>>> 13e7accb81d6 ("genirq: Get rid of GENERIC_MSI_IRQ_DOMAIN") :)
>>>
>>> Ok, so this hasn't been tested since the 6.2 release?  Wow, I think
>>> someone from AMD needs to take a deep look at this and verify that it
>>> actually is doing what it is supposed to be doing...
>>
>> The patch Thomas mentioned renames "GENERIC_MSI_IRQ_DOMAIN" to
>> "GENERIC_MSI_IRQ"; and in our testing "GENERIC_MSI_IRQ" is also selected as
>> 'ARM64' is enabled which enables 'ARM_GIC_V3_ITS' which in-turn selects
>> 'GENERIC_MSI_IRQ'.
> 
> Ok, but that's not what this patch "selects" at all :(

"GENERIC_MSI_IRQ" gets enabled because of enabling "ARM64", and you are 
right that what we are selecting here does not have any meaning now 
(after 6.2).

> 
>> The patch is tested for MSI functionality on 6.6-rc1. We will re-look into
>> the config dependencies to avoid such issues, but please be assured that the
>> patch has been validated.
> 
> How has the dependancy been validated as correct if there is no such
> thing in this kernel?

Apology for the confusion. By validated I meant tested for MSI 
functionality in CDX. Agree that "select GENERIC_MSI_IRQ_DOMAIN" needs 
to be fixed here. Will update in next spin. :)

Thanks,
Nipun

> 
> confused,
> 
> greg k-h

Re: [PATCH v4] cdx: add MSI support for CDX bus

[Gupta, Nipun]

On 10/5/2023 3:54 PM, Greg KH wrote:
> On Mon, Sep 11, 2023 at 07:22:59PM +0530, Nipun Gupta wrote:
>> Add CDX-MSI domain per CDX controller with gic-its domain as
>> a parent, to support MSI for CDX devices. CDX devices allocate
>> MSIs from the CDX domain. Also, introduce APIs to alloc and free
>> IRQs for CDX domain.
>>
>> In CDX subsystem firmware is a controller for all devices and
>> their configuration. CDX bus controller sends all the write_msi_msg
>> commands to firmware running on RPU and the firmware interfaces with
>> actual devices to pass this information to devices
>>
>> Since, CDX controller is the only way to communicate with the Firmware
>> for MSI write info, CDX domain per controller required in contrast to
>> having a CDX domain per device.
>>
>> Co-developed-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
>> Signed-off-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
>> Co-developed-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
>> Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
>> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
>> Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
>> Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
>> ---
>>
>> Changes v3->v4:
>> - Rebased on Linux 6.6-rc1
>>
>> Changes v2->v3:
>> - Rebased on Linux 6.5-rc1
>> - Used FW provided 'msi_dev_id' as device ID for GIC instead of 'req_id'.
>>
>> Changes v1->v2:
>> - fixed scenario where msi write was called asyncronously in
>>    an atomic context, by using irq_chip_(un)lock, and using sync
>>    MCDI API for write MSI message.
>> - fixed broken Signed-off-by chain.
>>
>>   drivers/cdx/Kconfig                     |   1 +
>>   drivers/cdx/Makefile                    |   2 +-
>>   drivers/cdx/cdx.c                       |   9 ++
>>   drivers/cdx/cdx.h                       |  12 ++
>>   drivers/cdx/cdx_msi.c                   | 183 ++++++++++++++++++++++++
>>   drivers/cdx/controller/cdx_controller.c |  23 +++
>>   drivers/cdx/controller/mc_cdx_pcol.h    |  64 +++++++++
>>   drivers/cdx/controller/mcdi_functions.c |  26 +++-
>>   drivers/cdx/controller/mcdi_functions.h |  20 +++
>>   include/linux/cdx/cdx_bus.h             |  32 +++++
>>   kernel/irq/msi.c                        |   1 +
>>   11 files changed, 370 insertions(+), 3 deletions(-)
>>   create mode 100644 drivers/cdx/cdx_msi.c
>>
>> diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
>> index a08958485e31..86df7ccb76bb 100644
>> --- a/drivers/cdx/Kconfig
>> +++ b/drivers/cdx/Kconfig
>> @@ -8,6 +8,7 @@
>>   config CDX_BUS
>>   	bool "CDX Bus driver"
>>   	depends on OF && ARM64
>> +	select GENERIC_MSI_IRQ_DOMAIN
> 
> This config option isn't in my tree anywhere, where did it come from?
> What is it supposed to do?
> 
>>   	help
>>   	  Driver to enable Composable DMA Transfer(CDX) Bus. CDX bus
>>   	  exposes Fabric devices which uses composable DMA IP to the
>> diff --git a/drivers/cdx/Makefile b/drivers/cdx/Makefile
>> index 0324e4914f6e..4bad79d1d188 100644
>> --- a/drivers/cdx/Makefile
>> +++ b/drivers/cdx/Makefile
>> @@ -5,4 +5,4 @@
>>   # Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
>>   #
>>   
>> -obj-$(CONFIG_CDX_BUS) += cdx.o controller/
>> +obj-$(CONFIG_CDX_BUS) += cdx.o cdx_msi.o controller/
> 
> So you are always building this in even if the build doesn't support
> MSI?  Why will that not break the build?

CDX bus will select GENERIC_MSI_IRQ, so I think we can have this only 
with CONFIG_CDX_BUS?

> 
> 
>> diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
>> index d2cad4c670a0..8d777cdacf1d 100644
>> --- a/drivers/cdx/cdx.c
>> +++ b/drivers/cdx/cdx.c
>> @@ -56,6 +56,7 @@
>>    */
>>   
>>   #include <linux/init.h>
>> +#include <linux/irqdomain.h>
>>   #include <linux/kernel.h>
>>   #include <linux/of_device.h>
>>   #include <linux/slab.h>
>> @@ -478,6 +479,7 @@ int cdx_device_add(struct cdx_dev_params *dev_params)
>>   
>>   	/* Populate CDX dev params */
>>   	cdx_dev->req_id = dev_params->req_id;
>> +	cdx_dev->msi_dev_id = dev_params->msi_dev_id;
>>   	cdx_dev->vendor = dev_params->vendor;
>>   	cdx_dev->device = dev_params->device;
>>   	cdx_dev->bus_num = dev_params->bus_num;
>> @@ -491,12 +493,19 @@ int cdx_device_add(struct cdx_dev_params *dev_params)
>>   	cdx_dev->dev.bus = &cdx_bus_type;
>>   	cdx_dev->dev.dma_mask = &cdx_dev->dma_mask;
>>   	cdx_dev->dev.release = cdx_device_release;
>> +	cdx_dev->msi_write_pending = false;
>> +	mutex_init(&cdx_dev->irqchip_lock);
>>   
>>   	/* Set Name */
>>   	dev_set_name(&cdx_dev->dev, "cdx-%02x:%02x",
>>   		     ((cdx->id << CDX_CONTROLLER_ID_SHIFT) | (cdx_dev->bus_num & CDX_BUS_NUM_MASK)),
>>   		     cdx_dev->dev_num);
>>   
>> +	if (cdx->msi_domain) {
>> +		cdx_dev->num_msi = dev_params->num_msi;
>> +		dev_set_msi_domain(&cdx_dev->dev, cdx->msi_domain);
>> +	}
>> +
>>   	ret = device_add(&cdx_dev->dev);
>>   	if (ret) {
>>   		dev_err(&cdx_dev->dev,
>> diff --git a/drivers/cdx/cdx.h b/drivers/cdx/cdx.h
>> index c436ac7ac86f..ece11c04d646 100644
>> --- a/drivers/cdx/cdx.h
>> +++ b/drivers/cdx/cdx.h
>> @@ -21,6 +21,8 @@
>>    * @res: array of MMIO region entries
>>    * @res_count: number of valid MMIO regions
>>    * @req_id: Requestor ID associated with CDX device
>> + * @msi_dev_id: MSI device ID associated with CDX device
>> + * @num_msi: Number of MSI's supported by the device
>>    */
>>   struct cdx_dev_params {
>>   	struct cdx_controller *cdx;
>> @@ -31,6 +33,8 @@ struct cdx_dev_params {
>>   	struct resource res[MAX_CDX_DEV_RESOURCES];
>>   	u8 res_count;
>>   	u32 req_id;
>> +	u32 msi_dev_id;
>> +	u32 num_msi;
>>   };
>>   
>>   /**
>> @@ -59,4 +63,12 @@ void cdx_unregister_controller(struct cdx_controller *cdx);
>>    */
>>   int cdx_device_add(struct cdx_dev_params *dev_params);
>>   
>> +/**
>> + * cdx_msi_domain_init - Init the CDX bus MSI domain.
>> + * @dev: Device of the CDX bus controller
>> + *
>> + * Return: CDX MSI domain, NULL on failure
>> + */
>> +struct irq_domain *cdx_msi_domain_init(struct device *dev);
>> +
>>   #endif /* _CDX_H_ */
>> diff --git a/drivers/cdx/cdx_msi.c b/drivers/cdx/cdx_msi.c
>> new file mode 100644
>> index 000000000000..d7f4c88428d6
>> --- /dev/null
>> +++ b/drivers/cdx/cdx_msi.c
>> @@ -0,0 +1,183 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * AMD CDX bus driver MSI support
>> + *
>> + * Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/msi.h>
>> +#include <linux/cdx/cdx_bus.h>
>> +
>> +#include "cdx.h"
>> +
>> +static void cdx_msi_write_msg(struct irq_data *irq_data,
>> +			      struct msi_msg *msg)
>> +{
>> +	struct msi_desc *msi_desc = irq_data_get_msi_desc(irq_data);
>> +	struct cdx_device *cdx_dev = to_cdx_device(msi_desc->dev);
>> +
>> +	/* We would not operate on msg here rather we wait for
>> +	 * irq_bus_sync_unlock() to be called from preemptible
>> +	 * task context.
>> +	 */
>> +	msi_desc->msg = *msg;
>> +	cdx_dev->msi_write_pending = true;
>> +}
>> +
>> +static void cdx_msi_write_irq_lock(struct irq_data *irq_data)
>> +{
>> +	struct msi_desc *msi_desc = irq_data_get_msi_desc(irq_data);
>> +	struct cdx_device *cdx_dev = to_cdx_device(msi_desc->dev);
>> +
>> +	mutex_lock(&cdx_dev->irqchip_lock);
>> +}
>> +
>> +static void cdx_msi_write_irq_unlock(struct irq_data *irq_data)
>> +{
>> +	struct msi_desc *msi_desc = irq_data_get_msi_desc(irq_data);
>> +	struct cdx_device *cdx_dev = to_cdx_device(msi_desc->dev);
>> +	struct cdx_controller *cdx = cdx_dev->cdx;
>> +	struct cdx_device_config dev_config;
>> +	int ret;
>> +
>> +	if (!cdx_dev->msi_write_pending) {
>> +		mutex_unlock(&cdx_dev->irqchip_lock);
>> +		return;
>> +	}
>> +
>> +	cdx_dev->msi_write_pending = false;
>> +	mutex_unlock(&cdx_dev->irqchip_lock);
>> +
>> +	dev_config.msi.msi_index = msi_desc->msi_index;
>> +	dev_config.msi.data = msi_desc->msg.data;
>> +	dev_config.msi.addr = ((u64)(msi_desc->msg.address_hi) << 32) |
>> +			      msi_desc->msg.address_lo;
>> +
>> +	dev_config.type = CDX_DEV_MSI_CONF;
>> +	ret = cdx->ops->dev_configure(cdx, cdx_dev->bus_num, cdx_dev->dev_num,
>> +				      &dev_config);
>> +	if (ret)
>> +		dev_err(&cdx_dev->dev, "Write MSI failed to CDX controller\n");
> 
> How noisy iks this going to be in an irq handler if something goes
> wrong?

sure. will remove this.

> 
> And you are doing this write outside of the lock, is that intentional?
> If so, why not document that?

This is required to be done outside lock only, as Thomas pointed out in 
review (https://lore.kernel.org/all/87ednnes6o.ffs@tglx/).
Sure will add a comment on top of this.

> 
> 
>> +}
>> +
>> +static struct irq_chip cdx_msi_irq_chip = {
>> +	.name			= "CDX-MSI",
>> +	.irq_mask		= irq_chip_mask_parent,
>> +	.irq_unmask		= irq_chip_unmask_parent,
>> +	.irq_eoi		= irq_chip_eoi_parent,
>> +	.irq_set_affinity	= msi_domain_set_affinity,
>> +	.irq_write_msi_msg	= cdx_msi_write_msg,
>> +	.irq_bus_lock		= cdx_msi_write_irq_lock,
>> +	.irq_bus_sync_unlock	= cdx_msi_write_irq_unlock
>> +};
>> +
>> +int cdx_msi_domain_alloc_irqs(struct device *dev, unsigned int irq_count)
>> +{
>> +	int ret;
>> +
>> +	ret = msi_setup_device_data(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = msi_domain_alloc_irqs_range(dev, MSI_DEFAULT_DOMAIN,
>> +					  0, irq_count - 1);
>> +	if (ret)
>> +		dev_err(dev, "Failed to allocate IRQs: %d\n", ret);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(cdx_msi_domain_alloc_irqs);
> 
> meta-comment, CDX really should have a module namespace one of these
> days, right?

Agree would evaluate and send out a patch soon for this.

> 
>> +
>> +/* Convert an msi_desc to a globally unique identifier. */
>> +static irq_hw_number_t cdx_domain_calc_hwirq(struct cdx_device *dev,
>> +					     struct msi_desc *desc)
>> +{
>> +	return ((irq_hw_number_t)dev->msi_dev_id << 10) | desc->msi_index;
> 
> How does this make a unique number?  Is msi_dev_id guaranteed to be
> unique across all cdx busses?  That's not normally the case for most bus
> types, unless you are guaranteeing this in the cdx core somewhere?
> 
> And why shift 10 bits?  That's a fun magic number...

The msi_dev_id_is unique in a domain, and thus it will provide unique 
hwirq for the domain. I will fix the comment above.

> 
>> +}
>> +
>> +static void cdx_msi_set_desc(msi_alloc_info_t *arg,
>> +			     struct msi_desc *desc)
>> +{
>> +	arg->desc = desc;
>> +	arg->hwirq = cdx_domain_calc_hwirq(to_cdx_device(desc->dev), desc);
>> +}
>> +
>> +static int cdx_msi_prepare(struct irq_domain *msi_domain,
>> +			   struct device *dev,
>> +			   int nvec, msi_alloc_info_t *info)
>> +{
>> +	struct cdx_device *cdx_dev = to_cdx_device(dev);
>> +	struct device *parent = dev->parent;
>> +	struct msi_domain_info *msi_info;
>> +	u32 dev_id = 0;
> 
> No need to set this, right?

right! will remove.

> 
>> +	int ret;
>> +
>> +	/* Retrieve device ID from requestor ID using parent device */
>> +	ret = of_map_id(parent->of_node, cdx_dev->msi_dev_id, "msi-map",
>> +			"msi-map-mask", NULL, &dev_id);
>> +	if (ret) {
>> +		dev_err(dev, "of_map_id failed for MSI: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* Set the device Id to be passed to the GIC-ITS */
>> +	info->scratchpad[0].ul = dev_id;
>> +
>> +	msi_info = msi_get_domain_info(msi_domain->parent);
>> +
>> +	return msi_info->ops->msi_prepare(msi_domain->parent, dev, nvec, info);
> 
> Do you know that there will be a ops pointer here? And the msi_prepare
> callback?  Or will this always just be:

Yes these would be set as we are setting these in below structures 
"struct msi_domain_ops cdx_msi_ops" and "msi_domain_info 
cdx_msi_domain_info".

> 
>> +}
>> +
>> +static struct msi_domain_ops cdx_msi_ops = {
>> +	.msi_prepare	= cdx_msi_prepare,
>> +	.set_desc	= cdx_msi_set_desc
> 
> This structure?
> 
>> +};
>> +
>> +static struct msi_domain_info cdx_msi_domain_info = {
>> +	.ops	= &cdx_msi_ops,
>> +	.chip	= &cdx_msi_irq_chip,
>> +	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
>> +		  MSI_FLAG_ALLOC_SIMPLE_MSI_DESCS | MSI_FLAG_FREE_MSI_DESCS
>> +};
>> +
>> +struct irq_domain *cdx_msi_domain_init(struct device *dev)
>> +{
>> +	struct device_node *np = dev->of_node;
>> +	struct fwnode_handle *fwnode_handle;
>> +	struct irq_domain *cdx_msi_domain;
>> +	struct device_node *parent_node;
>> +	struct irq_domain *parent;
>> +
>> +	fwnode_handle = of_node_to_fwnode(np);
>> +
>> +	parent_node = of_parse_phandle(np, "msi-map", 1);
>> +	if (!parent_node) {
>> +		dev_err(dev, "msi-map not present on cdx controller\n");
> 
> Is this a requirement now?  What about systems without it?

For the system supporting MSI, this propery is required. The system 
which does not have MSI, would not be calling this API as this is MSI 
specific.

> 
>> +struct cdx_msi_config {
>> +	u16 msi_index;
>> +	u32 data;
>> +	u64 addr;
>> +};
> 
> Are you ok with the "hole" in this structure?

This is only a software placeholder for information to be passed to 
hardware in a different message format (using MCDI).

Thanks,
Nipun

> 
> And I really need a msi maintainer to review this before I can take it,
> thanks.
> 
> greg k-h

Re: [PATCH v4] cdx: add MSI support for CDX bus

[Greg KH]

On Sat, Oct 07, 2023 at 02:13:15PM +0530, Gupta, Nipun wrote:
> 
> 
> On 10/5/2023 3:54 PM, Greg KH wrote:
> > On Mon, Sep 11, 2023 at 07:22:59PM +0530, Nipun Gupta wrote:
> > > Add CDX-MSI domain per CDX controller with gic-its domain as
> > > a parent, to support MSI for CDX devices. CDX devices allocate
> > > MSIs from the CDX domain. Also, introduce APIs to alloc and free
> > > IRQs for CDX domain.
> > > 
> > > In CDX subsystem firmware is a controller for all devices and
> > > their configuration. CDX bus controller sends all the write_msi_msg
> > > commands to firmware running on RPU and the firmware interfaces with
> > > actual devices to pass this information to devices
> > > 
> > > Since, CDX controller is the only way to communicate with the Firmware
> > > for MSI write info, CDX domain per controller required in contrast to
> > > having a CDX domain per device.
> > > 
> > > Co-developed-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> > > Signed-off-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> > > Co-developed-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> > > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> > > Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> > > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
> > > Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
> > > ---
> > > 
> > > Changes v3->v4:
> > > - Rebased on Linux 6.6-rc1
> > > 
> > > Changes v2->v3:
> > > - Rebased on Linux 6.5-rc1
> > > - Used FW provided 'msi_dev_id' as device ID for GIC instead of 'req_id'.
> > > 
> > > Changes v1->v2:
> > > - fixed scenario where msi write was called asyncronously in
> > >    an atomic context, by using irq_chip_(un)lock, and using sync
> > >    MCDI API for write MSI message.
> > > - fixed broken Signed-off-by chain.
> > > 
> > >   drivers/cdx/Kconfig                     |   1 +
> > >   drivers/cdx/Makefile                    |   2 +-
> > >   drivers/cdx/cdx.c                       |   9 ++
> > >   drivers/cdx/cdx.h                       |  12 ++
> > >   drivers/cdx/cdx_msi.c                   | 183 ++++++++++++++++++++++++
> > >   drivers/cdx/controller/cdx_controller.c |  23 +++
> > >   drivers/cdx/controller/mc_cdx_pcol.h    |  64 +++++++++
> > >   drivers/cdx/controller/mcdi_functions.c |  26 +++-
> > >   drivers/cdx/controller/mcdi_functions.h |  20 +++
> > >   include/linux/cdx/cdx_bus.h             |  32 +++++
> > >   kernel/irq/msi.c                        |   1 +
> > >   11 files changed, 370 insertions(+), 3 deletions(-)
> > >   create mode 100644 drivers/cdx/cdx_msi.c
> > > 
> > > diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
> > > index a08958485e31..86df7ccb76bb 100644
> > > --- a/drivers/cdx/Kconfig
> > > +++ b/drivers/cdx/Kconfig
> > > @@ -8,6 +8,7 @@
> > >   config CDX_BUS
> > >   	bool "CDX Bus driver"
> > >   	depends on OF && ARM64
> > > +	select GENERIC_MSI_IRQ_DOMAIN
> > 
> > This config option isn't in my tree anywhere, where did it come from?
> > What is it supposed to do?
> > 
> > >   	help
> > >   	  Driver to enable Composable DMA Transfer(CDX) Bus. CDX bus
> > >   	  exposes Fabric devices which uses composable DMA IP to the
> > > diff --git a/drivers/cdx/Makefile b/drivers/cdx/Makefile
> > > index 0324e4914f6e..4bad79d1d188 100644
> > > --- a/drivers/cdx/Makefile
> > > +++ b/drivers/cdx/Makefile
> > > @@ -5,4 +5,4 @@
> > >   # Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
> > >   #
> > > -obj-$(CONFIG_CDX_BUS) += cdx.o controller/
> > > +obj-$(CONFIG_CDX_BUS) += cdx.o cdx_msi.o controller/
> > 
> > So you are always building this in even if the build doesn't support
> > MSI?  Why will that not break the build?
> 
> CDX bus will select GENERIC_MSI_IRQ, so I think we can have this only with
> CONFIG_CDX_BUS?

As CDX works today without MSI, why are you adding this requirement to
the codebase forcing everyone to have it?

> > > +struct cdx_msi_config {
> > > +	u16 msi_index;
> > > +	u32 data;
> > > +	u64 addr;
> > > +};
> > 
> > Are you ok with the "hole" in this structure?
> 
> This is only a software placeholder for information to be passed to hardware
> in a different message format (using MCDI).

Great, then how about reording things so there isn't a hole?

thanks,

greg k-h

Re: [PATCH v4] cdx: add MSI support for CDX bus

[Gupta, Nipun]

On 10/7/2023 2:21 PM, Greg KH wrote:
> On Sat, Oct 07, 2023 at 02:13:15PM +0530, Gupta, Nipun wrote:
>>
>>
>> On 10/5/2023 3:54 PM, Greg KH wrote:
>>> On Mon, Sep 11, 2023 at 07:22:59PM +0530, Nipun Gupta wrote:
>>>> Add CDX-MSI domain per CDX controller with gic-its domain as
>>>> a parent, to support MSI for CDX devices. CDX devices allocate
>>>> MSIs from the CDX domain. Also, introduce APIs to alloc and free
>>>> IRQs for CDX domain.
>>>>
>>>> In CDX subsystem firmware is a controller for all devices and
>>>> their configuration. CDX bus controller sends all the write_msi_msg
>>>> commands to firmware running on RPU and the firmware interfaces with
>>>> actual devices to pass this information to devices
>>>>
>>>> Since, CDX controller is the only way to communicate with the Firmware
>>>> for MSI write info, CDX domain per controller required in contrast to
>>>> having a CDX domain per device.
>>>>
>>>> Co-developed-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
>>>> Signed-off-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
>>>> Co-developed-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
>>>> Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
>>>> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
>>>> Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
>>>> Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com>
>>>> ---
>>>>
>>>> Changes v3->v4:
>>>> - Rebased on Linux 6.6-rc1
>>>>
>>>> Changes v2->v3:
>>>> - Rebased on Linux 6.5-rc1
>>>> - Used FW provided 'msi_dev_id' as device ID for GIC instead of 'req_id'.
>>>>
>>>> Changes v1->v2:
>>>> - fixed scenario where msi write was called asyncronously in
>>>>     an atomic context, by using irq_chip_(un)lock, and using sync
>>>>     MCDI API for write MSI message.
>>>> - fixed broken Signed-off-by chain.
>>>>
>>>>    drivers/cdx/Kconfig                     |   1 +
>>>>    drivers/cdx/Makefile                    |   2 +-
>>>>    drivers/cdx/cdx.c                       |   9 ++
>>>>    drivers/cdx/cdx.h                       |  12 ++
>>>>    drivers/cdx/cdx_msi.c                   | 183 ++++++++++++++++++++++++
>>>>    drivers/cdx/controller/cdx_controller.c |  23 +++
>>>>    drivers/cdx/controller/mc_cdx_pcol.h    |  64 +++++++++
>>>>    drivers/cdx/controller/mcdi_functions.c |  26 +++-
>>>>    drivers/cdx/controller/mcdi_functions.h |  20 +++
>>>>    include/linux/cdx/cdx_bus.h             |  32 +++++
>>>>    kernel/irq/msi.c                        |   1 +
>>>>    11 files changed, 370 insertions(+), 3 deletions(-)
>>>>    create mode 100644 drivers/cdx/cdx_msi.c
>>>>
>>>> diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
>>>> index a08958485e31..86df7ccb76bb 100644
>>>> --- a/drivers/cdx/Kconfig
>>>> +++ b/drivers/cdx/Kconfig
>>>> @@ -8,6 +8,7 @@
>>>>    config CDX_BUS
>>>>    	bool "CDX Bus driver"
>>>>    	depends on OF && ARM64
>>>> +	select GENERIC_MSI_IRQ_DOMAIN
>>>
>>> This config option isn't in my tree anywhere, where did it come from?
>>> What is it supposed to do?
>>>
>>>>    	help
>>>>    	  Driver to enable Composable DMA Transfer(CDX) Bus. CDX bus
>>>>    	  exposes Fabric devices which uses composable DMA IP to the
>>>> diff --git a/drivers/cdx/Makefile b/drivers/cdx/Makefile
>>>> index 0324e4914f6e..4bad79d1d188 100644
>>>> --- a/drivers/cdx/Makefile
>>>> +++ b/drivers/cdx/Makefile
>>>> @@ -5,4 +5,4 @@
>>>>    # Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
>>>>    #
>>>> -obj-$(CONFIG_CDX_BUS) += cdx.o controller/
>>>> +obj-$(CONFIG_CDX_BUS) += cdx.o cdx_msi.o controller/
>>>
>>> So you are always building this in even if the build doesn't support
>>> MSI?  Why will that not break the build?
>>
>> CDX bus will select GENERIC_MSI_IRQ, so I think we can have this only with
>> CONFIG_CDX_BUS?
> 
> As CDX works today without MSI, why are you adding this requirement to
> the codebase forcing everyone to have it?

Agree, CDX bus can work without MSI. GENERIC_MSI_IRQ can be selected by 
a controller if it is relying on MSI. Will update the code accordingly.

> 
>>>> +struct cdx_msi_config {
>>>> +	u16 msi_index;
>>>> +	u32 data;
>>>> +	u64 addr;
>>>> +};
>>>
>>> Are you ok with the "hole" in this structure?
>>
>> This is only a software placeholder for information to be passed to hardware
>> in a different message format (using MCDI).
> 
> Great, then how about reording things so there isn't a hole?

sure.. will update this in next spin.

Thanks,
Nipun

> 
> thanks,
> 
> greg k-h

RE: [PATCH v4] cdx: add MSI support for CDX bus

[Gangurde, Abhijit]

> > +}
> > +
> > +static struct irq_chip cdx_msi_irq_chip = {
> > +	.name			= "CDX-MSI",
> > +	.irq_mask		= irq_chip_mask_parent,
> > +	.irq_unmask		= irq_chip_unmask_parent,
> > +	.irq_eoi		= irq_chip_eoi_parent,
> > +	.irq_set_affinity	= msi_domain_set_affinity,
> > +	.irq_write_msi_msg	= cdx_msi_write_msg,
> > +	.irq_bus_lock		= cdx_msi_write_irq_lock,
> > +	.irq_bus_sync_unlock	= cdx_msi_write_irq_unlock
> > +};
> > +
> > +int cdx_msi_domain_alloc_irqs(struct device *dev, unsigned int irq_count)
> > +{
> > +	int ret;
> > +
> > +	ret = msi_setup_device_data(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = msi_domain_alloc_irqs_range(dev, MSI_DEFAULT_DOMAIN,
> > +					  0, irq_count - 1);
> > +	if (ret)
> > +		dev_err(dev, "Failed to allocate IRQs: %d\n", ret);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(cdx_msi_domain_alloc_irqs);
> 
> meta-comment, CDX really should have a module namespace one of these
> days, right?
> 

Will include the module namespace change in other cdx patch series.

Thanks,
Abhijit