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 <[email protected]>
> Signed-off-by: Nikhil Agarwal <[email protected]>
> Co-developed-by: Abhijit Gangurde <[email protected]>
> Signed-off-by: Abhijit Gangurde <[email protected]>
> Signed-off-by: Nipun Gupta <[email protected]>
> Reviewed-by: Pieter Jansen van Vuuren <[email protected]>
> Tested-by: Nikhil Agarwal <[email protected]>
> ---
>
> 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
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
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 <[email protected]>
>> Signed-off-by: Nikhil Agarwal <[email protected]>
>> Co-developed-by: Abhijit Gangurde <[email protected]>
>> Signed-off-by: Abhijit Gangurde <[email protected]>
>> Signed-off-by: Nipun Gupta <[email protected]>
>> Reviewed-by: Pieter Jansen van Vuuren <[email protected]>
>> Tested-by: Nikhil Agarwal <[email protected]>
>> ---
>>
>> 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
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 <[email protected]>
> > > Signed-off-by: Nikhil Agarwal <[email protected]>
> > > Co-developed-by: Abhijit Gangurde <[email protected]>
> > > Signed-off-by: Abhijit Gangurde <[email protected]>
> > > Signed-off-by: Nipun Gupta <[email protected]>
> > > Reviewed-by: Pieter Jansen van Vuuren <[email protected]>
> > > Tested-by: Nikhil Agarwal <[email protected]>
> > > ---
> > >
> > > 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
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 <[email protected]>
>>>> Signed-off-by: Nikhil Agarwal <[email protected]>
>>>> Co-developed-by: Abhijit Gangurde <[email protected]>
>>>> Signed-off-by: Abhijit Gangurde <[email protected]>
>>>> Signed-off-by: Nipun Gupta <[email protected]>
>>>> Reviewed-by: Pieter Jansen van Vuuren <[email protected]>
>>>> Tested-by: Nikhil Agarwal <[email protected]>
>>>> ---
>>>>
>>>> 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
> > +}
> > +
> > +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