On Tue, 06 Sep 2022 14:47:58 +0100, Nipun Gupta <nipun.gupta@xxxxxxx> wrote: > > Devices on cdx bus are dynamically detected and registered using > platform_device_register API. As these devices are not linked to > of node they need a separate MSI domain for handling device ID > to be provided to the GIC ITS domain. > > This also introduces APIs to alloc and free IRQs for CDX domain. > > Signed-off-by: Nipun Gupta <nipun.gupta@xxxxxxx> > Signed-off-by: Nikhil Agarwal <nikhil.agarwal@xxxxxxx> > --- > drivers/bus/cdx/cdx.c | 18 +++ > drivers/bus/cdx/cdx.h | 19 +++ > drivers/bus/cdx/cdx_msi.c | 236 +++++++++++++++++++++++++++++++++++ > drivers/bus/cdx/mcdi_stubs.c | 1 + > include/linux/cdx/cdx_bus.h | 19 +++ > 5 files changed, 293 insertions(+) > create mode 100644 drivers/bus/cdx/cdx_msi.c > > diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c > index fc417c32c59b..02ececce1c84 100644 > --- a/drivers/bus/cdx/cdx.c > +++ b/drivers/bus/cdx/cdx.c > @@ -17,6 +17,7 @@ > #include <linux/dma-map-ops.h> > #include <linux/property.h> > #include <linux/iommu.h> > +#include <linux/msi.h> > #include <linux/cdx/cdx_bus.h> > > #include "cdx.h" > @@ -236,6 +237,7 @@ static int cdx_device_add(struct device *parent, > struct cdx_dev_params_t *dev_params) > { > struct cdx_device *cdx_dev; > + struct irq_domain *cdx_msi_domain; > int ret; > > cdx_dev = kzalloc(sizeof(*cdx_dev), GFP_KERNEL); > @@ -252,6 +254,7 @@ static int cdx_device_add(struct device *parent, > > /* Populate CDX dev params */ > cdx_dev->req_id = dev_params->req_id; > + cdx_dev->num_msi = dev_params->num_msi; > cdx_dev->vendor = dev_params->vendor; > cdx_dev->device = dev_params->device; > cdx_dev->bus_id = dev_params->bus_id; > @@ -269,6 +272,21 @@ static int cdx_device_add(struct device *parent, > dev_set_name(&cdx_dev->dev, "cdx-%02x:%02x", cdx_dev->bus_id, > cdx_dev->func_id); > > + /* If CDX MSI domain is not created, create one. */ > + cdx_msi_domain = cdx_find_msi_domain(parent); Why do we need such a wrapper around find_host_domain()? > + if (!cdx_msi_domain) { > + cdx_msi_domain = cdx_msi_domain_init(parent); This is racy. If device are populated in parallel, bad things will happen. > + if (!cdx_msi_domain) { > + dev_err(&cdx_dev->dev, > + "cdx_msi_domain_init() failed: %d", ret); > + kfree(cdx_dev); > + return -1; Use standard error codes. > + } > + } > + > + /* Set the MSI domain */ > + dev_set_msi_domain(&cdx_dev->dev, cdx_msi_domain); > + > ret = device_add(&cdx_dev->dev); > if (ret != 0) { > dev_err(&cdx_dev->dev, > diff --git a/drivers/bus/cdx/cdx.h b/drivers/bus/cdx/cdx.h > index db0569431c10..95df440ebd73 100644 > --- a/drivers/bus/cdx/cdx.h > +++ b/drivers/bus/cdx/cdx.h > @@ -20,6 +20,7 @@ > * @res: array of MMIO region entries > * @res_count: number of valid MMIO regions > * @req_id: Requestor ID associated with CDX device > + * @num_msi: Number of MSI's supported by the device > */ > struct cdx_dev_params_t { > u16 vendor; > @@ -29,6 +30,24 @@ struct cdx_dev_params_t { > struct resource res[MAX_CDX_DEV_RESOURCES]; > u8 res_count; > u32 req_id; > + u32 num_msi; > }; > > +/** > + * 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); > + > +/** > + * cdx_find_msi_domain - Get the CDX-MSI domain. > + * @dev: CDX controller generic device > + * > + * Return CDX MSI domain, NULL on error or if CDX-MSI domain is > + * not yet created. > + */ > +struct irq_domain *cdx_find_msi_domain(struct device *parent); > + > #endif /* _CDX_H_ */ > diff --git a/drivers/bus/cdx/cdx_msi.c b/drivers/bus/cdx/cdx_msi.c > new file mode 100644 > index 000000000000..2fb7bac18393 > --- /dev/null > +++ b/drivers/bus/cdx/cdx_msi.c > @@ -0,0 +1,236 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * AMD CDX bus driver MSI support > + * > + * Copyright (C) 2022, 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" > + > +#ifdef GENERIC_MSI_DOMAIN_OPS > +/* > + * Generate a unique ID identifying the interrupt (only used within the MSI > + * irqdomain. Combine the req_id with the interrupt index. > + */ > +static irq_hw_number_t cdx_domain_calc_hwirq(struct cdx_device *dev, > + struct msi_desc *desc) > +{ > + /* > + * Make the base hwirq value for req_id*10000 so it is readable > + * as a decimal value in /proc/interrupts. > + */ > + return (irq_hw_number_t)(desc->msi_index + (dev->req_id * 10000)); No, please. Use shifts, and use a script if decimal conversion fails you. We're not playing these games. And the cast is pointless. Yes, you have lifted it from the FSL code, bad move. /me makes a note to go and clean-up this crap. > +} > + > +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); > +} > +#else > +#define cdx_msi_set_desc NULL Why the ifdefery? This should *only* be supported with GENERIC_MSI_DOMAIN_OPS. > +#endif > + > +static void cdx_msi_update_dom_ops(struct msi_domain_info *info) > +{ > + struct msi_domain_ops *ops = info->ops; > + > + if (!ops) > + return; > + > + /* set_desc should not be set by the caller */ > + if (!ops->set_desc) > + ops->set_desc = cdx_msi_set_desc; Then why are you allowing this to be overridden? > +} > + > +static void cdx_msi_write_msg(struct irq_data *irq_data, > + struct msi_msg *msg) > +{ > + /* > + * Do nothing as CDX devices have these pre-populated > + * in the hardware itself. > + */ We talked about this in a separate thread. This is a major problem. > +} > + > +static void cdx_msi_update_chip_ops(struct msi_domain_info *info) > +{ > + struct irq_chip *chip = info->chip; > + > + if (!chip) > + return; > + > + /* > + * irq_write_msi_msg should not be set by the caller > + */ > + if (!chip->irq_write_msi_msg) > + chip->irq_write_msi_msg = cdx_msi_write_msg; Then why the check? > +} > +/** > + * cdx_msi_create_irq_domain - Create a CDX MSI interrupt domain > + * @fwnode: Optional firmware node of the interrupt controller > + * @info: MSI domain info > + * @parent: Parent irq domain > + * > + * Updates the domain and chip ops and creates a CDX MSI > + * interrupt domain. > + * > + * Returns: > + * A domain pointer or NULL in case of failure. > + */ > +static struct irq_domain *cdx_msi_create_irq_domain(struct fwnode_handle *fwnode, > + struct msi_domain_info *info, > + struct irq_domain *parent) > +{ > + if (WARN_ON((info->flags & MSI_FLAG_LEVEL_CAPABLE))) > + info->flags &= ~MSI_FLAG_LEVEL_CAPABLE; No. Just fail the domain creation. We shouldn't paper over these things. > + if (info->flags & MSI_FLAG_USE_DEF_DOM_OPS) > + cdx_msi_update_dom_ops(info); > + if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS) > + cdx_msi_update_chip_ops(info); Under what circumstances would the default ops not be used? The only caller is in this file and has pre-computed values. This looks like a copy/paste from platform-msi.c. > + info->flags |= MSI_FLAG_ALLOC_SIMPLE_MSI_DESCS | MSI_FLAG_FREE_MSI_DESCS; > + > + return msi_create_irq_domain(fwnode, info, parent); This whole function makes no sense. You should move everything to the relevant structures, and simply call msi_create_irq_domain() from the sole caller of this function. > +} > + > +int cdx_msi_domain_alloc_irqs(struct device *dev, unsigned int irq_count) > +{ > + struct irq_domain *msi_domain; > + int ret; > + > + msi_domain = dev_get_msi_domain(dev); > + if (!msi_domain) { How can that happen? > + dev_err(dev, "msi domain get failed\n"); > + return -EINVAL; > + } > + > + ret = msi_setup_device_data(dev); > + if (ret) { > + dev_err(dev, "msi setup device failed: %d\n", ret); > + return ret; > + } > + > + msi_lock_descs(dev); > + if (msi_first_desc(dev, MSI_DESC_ALL)) > + ret = -EINVAL; > + msi_unlock_descs(dev); > + if (ret) { > + dev_err(dev, "msi setup device failed: %d\n", ret); Same message twice, not very useful. Consider grouping these things at the end of the function and make use of a (oh Gawd) goto... > + return ret; > + } > + > + ret = msi_domain_alloc_irqs(msi_domain, dev, irq_count); > + if (ret) > + dev_err(dev, "Failed to allocate IRQs\n"); > + > + return ret; > +} > +EXPORT_SYMBOL(cdx_msi_domain_alloc_irqs); EXPORT_SYMBOL_GPL(), please, for all the exports. > + > +void cdx_msi_domain_free_irqs(struct device *dev) > +{ > + struct irq_domain *msi_domain; > + > + msi_domain = dev_get_msi_domain(dev); > + if (!msi_domain) Again, how can that happen? > + return; > + > + msi_domain_free_irqs(msi_domain, dev); > +} > +EXPORT_SYMBOL(cdx_msi_domain_free_irqs); This feels like a very pointless helper, and again a copy/paste from the FSL code. I'd rather you change msi_domain_free_irqs() to only take a device and use the implicit MSI domain. > + > +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 nit: please align things vertically. > +}; > + > +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 msi_domain_info *msi_info; > + struct device *parent = dev->parent; > + u32 dev_id; > + int ret; > + > + /* Retrieve device ID from requestor ID using parent device */ > + ret = of_map_id(parent->of_node, cdx_dev->req_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); > + > + /* Allocate at least 32 MSIs, and always as a power of 2 */ Where is this requirement coming from? > + nvec = max_t(int, 32, roundup_pow_of_two(nvec)); > + return msi_info->ops->msi_prepare(msi_domain->parent, dev, nvec, info); > +} > + > +static struct msi_domain_ops cdx_msi_ops __ro_after_init = { > + .msi_prepare = cdx_msi_prepare, > +}; > + > +static struct msi_domain_info cdx_msi_domain_info = { > + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS), > + .ops = &cdx_msi_ops, > + .chip = &cdx_msi_irq_chip, > +}; > + > +struct irq_domain *cdx_msi_domain_init(struct device *dev) > +{ > + struct irq_domain *parent; > + struct irq_domain *cdx_msi_domain; > + struct fwnode_handle *fwnode_handle; > + struct device_node *parent_node; > + struct device_node *np = dev->of_node; > + > + fwnode_handle = of_node_to_fwnode(np); > + > + parent_node = of_parse_phandle(np, "msi-map", 1); Huh. This only works because you are stuck with a single ITS per system. > + 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 = cdx_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; > +} > + > +struct irq_domain *cdx_find_msi_domain(struct device *parent) > +{ > + return irq_find_host(parent->of_node); > +} > diff --git a/drivers/bus/cdx/mcdi_stubs.c b/drivers/bus/cdx/mcdi_stubs.c > index cc9d30fa02f8..2c8db1f5a057 100644 > --- a/drivers/bus/cdx/mcdi_stubs.c > +++ b/drivers/bus/cdx/mcdi_stubs.c > @@ -45,6 +45,7 @@ int cdx_mcdi_get_func_config(struct cdx_mcdi_t *cdx_mcdi, > dev_params->res_count = 2; > > dev_params->req_id = 0x250; > + dev_params->num_msi = 4; Why the hardcoded 4? Is that part of the firmware emulation stuff? M. -- Without deviation from the norm, progress is not possible.