Messages in this thread | | | Date | Wed, 07 Sep 2022 14:18:27 +0100 | From | Marc Zyngier <> | Subject | Re: [RFC PATCH v3 4/7] bus/cdx: add cdx-MSI domain with gic-its domain as parent |
| |
On Tue, 06 Sep 2022 14:47:58 +0100, Nipun Gupta <nipun.gupta@amd.com> 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@amd.com> > Signed-off-by: Nikhil Agarwal <nikhil.agarwal@amd.com> > --- > 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.
| |