On 10/04/2019 05:13, Lokesh Vutla wrote: > With the system coprocessor managing the range allocation of the > inputs to Interrupt Aggregator, it is difficult to represent > the device IRQs from DT. > > The suggestion is to use MSI in such cases where devices wants > to allocate and group interrupts dynamically. > > Create a MSI domain bus layer that allocates and frees MSIs for > a device. > > APIs that are implemented: > - ti_sci_inta_msi_create_irq_domain() that creates a MSI domain > - ti_sci_inta_msi_domain_alloc_irqs() that creates MSIs for the > specified device and resource. > - ti_sci_inta_msi_domain_free_irqs() frees the irqs attached to the device. > - ti_sci_inta_msi_get_virq() for getting the virq attached to a specific > event. > > Signed-off-by: Lokesh Vutla <lokeshvu...@ti.com> > --- > Changes since v5: > - Updated the input parametes to all apis > - Updated the default chip ops. > - Prefixed all the apis with ti_sci_inta_ > > Marc, > Right now ti_sci_resource is being passed for irq allocatons. > I couldn't get to use resources attached to platform_device. Because > platform_device resources are allocated in of_device_alloc() and number > of resources are fixed in it. In order to update the resources, driver > has to do a krealloc(pdev->resources) and update the num of resources. > Is it allowed to update the pdev->resources during probe time? If yes, > Ill be happy to update the patch to use platform_dev resources.
My suggestion was for you to define your own bus, device type and co (much like the fsl-mc stuff), and not reuse platform devices at all. > > > MAINTAINERS | 2 + > drivers/soc/ti/Kconfig | 6 + > drivers/soc/ti/Makefile | 1 + > drivers/soc/ti/ti_sci_inta_msi.c | 167 +++++++++++++++++++++++++ > include/linux/irqdomain.h | 1 + > include/linux/msi.h | 6 + > include/linux/soc/ti/ti_sci_inta_msi.h | 23 ++++ > 7 files changed, 206 insertions(+) > create mode 100644 drivers/soc/ti/ti_sci_inta_msi.c > create mode 100644 include/linux/soc/ti/ti_sci_inta_msi.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index ba88b3033fe4..dd31d7cb2fc6 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -15353,6 +15353,8 @@ F: > Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt > F: Documentation/devicetree/bindings/interrupt-controller/ti,sci-inta.txt > F: drivers/irqchip/irq-ti-sci-intr.c > F: drivers/irqchip/irq-ti-sci-inta.c > +F: include/linux/soc/ti/ti_sci_inta_msi.h > +F: drivers/soc/ti/ti_sci_inta_msi.c > > Texas Instruments ASoC drivers > M: Peter Ujfalusi <peter.ujfal...@ti.com> > diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig > index be4570baad96..82f110fe4953 100644 > --- a/drivers/soc/ti/Kconfig > +++ b/drivers/soc/ti/Kconfig > @@ -73,4 +73,10 @@ config TI_SCI_PM_DOMAINS > called ti_sci_pm_domains. Note this is needed early in boot before > rootfs may be available. > > +config TI_SCI_INTA_MSI_DOMAIN > + bool > + select GENERIC_MSI_IRQ_DOMAIN > + help > + Driver to enable Interrupt Aggregator specific MSI Domain. > + > endif # SOC_TI > diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile > index a22edc0b258a..b3868d392d4f 100644 > --- a/drivers/soc/ti/Makefile > +++ b/drivers/soc/ti/Makefile > @@ -8,3 +8,4 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA) += knav_dma.o > obj-$(CONFIG_AMX3_PM) += pm33xx.o > obj-$(CONFIG_WKUP_M3_IPC) += wkup_m3_ipc.o > obj-$(CONFIG_TI_SCI_PM_DOMAINS) += ti_sci_pm_domains.o > +obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN) += ti_sci_inta_msi.o > diff --git a/drivers/soc/ti/ti_sci_inta_msi.c > b/drivers/soc/ti/ti_sci_inta_msi.c > new file mode 100644 > index 000000000000..247a5e5f216b > --- /dev/null > +++ b/drivers/soc/ti/ti_sci_inta_msi.c > @@ -0,0 +1,167 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Texas Instruments' K3 Interrupt Aggregator MSI bus > + * > + * Copyright (C) 2018-2019 Texas Instruments Incorporated - > http://www.ti.com/ > + * Lokesh Vutla <lokeshvu...@ti.com> > + */ > + > +#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> Alphabetical ordering, please. > +#include <linux/soc/ti/ti_sci_inta_msi.h> > +#include <linux/soc/ti/ti_sci_protocol.h> > + > +static void ti_sci_inta_msi_write_msg(struct irq_data *data, > + struct msi_msg *msg) > +{ > + /* Nothing to do */ > +} > + > +static void ti_sci_inta_msi_compose_msi_msg(struct irq_data *data, > + struct msi_msg *msg) > +{ > + /* Nothing to do */ > +} > + > +static int ti_sci_inta_msi_request_resources(struct irq_data *data) > +{ > + data = data->parent_data; > + > + return data->chip->irq_request_resources(data); > +} > + > +static void ti_sci_inta_msi_release_resources(struct irq_data *data) > +{ > + data = data->parent_data; > + data->chip->irq_release_resources(data); > +} The two functions above are an implementation of irq_chip_{request,release}_resource_parent(). Please make them generic functions, use them and fix drivers/gpio/gpio-thunderx.c to use them too. > + > +static void ti_sci_inta_msi_update_chip_ops(struct msi_domain_info *info) > +{ > + struct irq_chip *chip = info->chip; > + > + WARN_ON(!chip); Just doing that isn't going to help, as you'll crash on the following line... > + if (!chip->irq_mask) > + chip->irq_mask = irq_chip_mask_parent; > + if (!chip->irq_unmask) > + chip->irq_unmask = irq_chip_unmask_parent; > + if (!chip->irq_ack) > + chip->irq_ack = irq_chip_ack_parent; > + if (!chip->irq_set_type) > + chip->irq_set_type = irq_chip_set_type_parent; > + if (!chip->irq_write_msi_msg) > + chip->irq_write_msi_msg = ti_sci_inta_msi_write_msg; > + if (!chip->irq_compose_msi_msg) > + chip->irq_compose_msi_msg = ti_sci_inta_msi_compose_msi_msg; > + if (!chip->irq_request_resources) > + chip->irq_request_resources = ti_sci_inta_msi_request_resources; > + if (!chip->irq_release_resources) > + chip->irq_release_resources = ti_sci_inta_msi_release_resources; Is there any case where a client driver wouldn't use the default all the time? > +} > + > +struct irq_domain > +*ti_sci_inta_msi_create_irq_domain(struct fwnode_handle *fwnode, > + struct msi_domain_info *info, > + struct irq_domain *parent) > +{ > + struct irq_domain *domain; > + > + if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS) > + ti_sci_inta_msi_update_chip_ops(info); If the answer above is "no", then you can happily ignore this flag and always populate the callbacks. > + > + domain = msi_create_irq_domain(fwnode, info, parent); > + if (domain) > + irq_domain_update_bus_token(domain, DOMAIN_BUS_TI_SCI_INTA_MSI); > + > + return domain; > +} > +EXPORT_SYMBOL_GPL(ti_sci_inta_msi_create_irq_domain); > + > +static void ti_sci_inta_msi_free_descs(struct device *dev) > +{ > + struct msi_desc *desc, *tmp; > + > + list_for_each_entry_safe(desc, tmp, dev_to_msi_list(dev), list) { > + list_del(&desc->list); > + free_msi_entry(desc); > + } > +} > + > +static int ti_sci_inta_msi_alloc_descs(struct device *dev, u32 dev_id, > + struct ti_sci_resource *res) > +{ > + struct msi_desc *msi_desc; > + int set, i, count = 0; > + > + for (set = 0; set < res->sets; set++) { > + for (i = 0; i < res->desc[set].num; i++) { > + msi_desc = alloc_msi_entry(dev, 1, NULL); > + if (!msi_desc) { > + ti_sci_inta_msi_free_descs(dev); > + return -ENOMEM; > + } > + > + msi_desc->inta.index = res->desc[set].start + i; > + msi_desc->inta.dev_id = dev_id; I'm highly suspiscious of this. See further down. > + INIT_LIST_HEAD(&msi_desc->list); > + list_add_tail(&msi_desc->list, dev_to_msi_list(dev)); > + count++; > + } > + } > + > + return count; > +} > + > +int ti_sci_inta_msi_domain_alloc_irqs(struct platform_device *pdev, > + struct ti_sci_resource *res) > +{ > + struct irq_domain *msi_domain; > + int ret, nvec; > + > + msi_domain = dev_get_msi_domain(&pdev->dev); > + if (!msi_domain) > + return -EINVAL; > + > + if (pdev->id < 0) > + return -ENODEV; > + > + nvec = ti_sci_inta_msi_alloc_descs(&pdev->dev, pdev->id, res); > + if (nvec <= 0) > + return nvec; > + > + ret = msi_domain_alloc_irqs(msi_domain, &pdev->dev, nvec); > + if (ret) { > + dev_err(&pdev->dev, "Failed to allocate IRQs %d\n", ret); > + goto cleanup; > + } > + > + return 0; > + > +cleanup: > + ti_sci_inta_msi_free_descs(&pdev->dev); > + return ret; > +} > +EXPORT_SYMBOL_GPL(ti_sci_inta_msi_domain_alloc_irqs); > + > +void ti_sci_inta_msi_domain_free_irqs(struct device *dev) > +{ > + msi_domain_free_irqs(dev->msi_domain, dev); > + ti_sci_inta_msi_free_descs(dev); > +} > +EXPORT_SYMBOL_GPL(ti_sci_inta_msi_domain_free_irqs); > + > +unsigned int ti_sci_inta_msi_get_virq(struct platform_device *pdev, u32 > index) > +{ > + struct msi_desc *desc; > + > + for_each_msi_entry(desc, &pdev->dev) > + if (desc->inta.index == index && desc->inta.dev_id == pdev->id) What is this "index"? Why isn't the right entry the index-th element in the msi_desc list? Worse, the dev_id check. The whole point of having a per-device MSI list is that it is, well, per device. > + return desc->irq; > + > + return -ENODEV; > +} > +EXPORT_SYMBOL_GPL(ti_sci_inta_msi_get_virq); > diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h > index 61706b430907..07ec8b390161 100644 > --- a/include/linux/irqdomain.h > +++ b/include/linux/irqdomain.h > @@ -82,6 +82,7 @@ enum irq_domain_bus_token { > DOMAIN_BUS_NEXUS, > DOMAIN_BUS_IPI, > DOMAIN_BUS_FSL_MC_MSI, > + DOMAIN_BUS_TI_SCI_INTA_MSI, > }; > > /** > diff --git a/include/linux/msi.h b/include/linux/msi.h > index 7e9b81c3b50d..7e12dfc4cb39 100644 > --- a/include/linux/msi.h > +++ b/include/linux/msi.h > @@ -47,6 +47,11 @@ struct fsl_mc_msi_desc { > u16 msi_index; > }; > > +struct ti_sci_inta_msi_desc { > + u16 dev_id; > + u16 index; > +}; > + > /** > * struct msi_desc - Descriptor structure for MSI based interrupts > * @list: List head for management > @@ -106,6 +111,7 @@ struct msi_desc { > */ > struct platform_msi_desc platform; > struct fsl_mc_msi_desc fsl_mc; > + struct ti_sci_inta_msi_desc inta; > }; > }; > > diff --git a/include/linux/soc/ti/ti_sci_inta_msi.h > b/include/linux/soc/ti/ti_sci_inta_msi.h > new file mode 100644 > index 000000000000..b0ca20ab3f49 > --- /dev/null > +++ b/include/linux/soc/ti/ti_sci_inta_msi.h > @@ -0,0 +1,23 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Texas Instruments' K3 TI SCI INTA MSI helper > + * > + * Copyright (C) 2018-2019 Texas Instruments Incorporated - > http://www.ti.com/ > + * Lokesh Vutla <lokeshvu...@ti.com> > + */ > + > +#ifndef __INCLUDE_LINUX_TI_SCI_INTA_MSI_H > +#define __INCLUDE_LINUX_TI_SCI_INTA_MSI_H > + > +#include <linux/msi.h> > +#include <linux/soc/ti/ti_sci_protocol.h> > + > +struct irq_domain > +*ti_sci_inta_msi_create_irq_domain(struct fwnode_handle *fwnode, > + struct msi_domain_info *info, > + struct irq_domain *parent); > +int ti_sci_inta_msi_domain_alloc_irqs(struct platform_device *pdev, > + struct ti_sci_resource *res); > +unsigned int ti_sci_inta_msi_get_virq(struct platform_device *dev, u32 > index); > +void ti_sci_inta_msi_domain_free_irqs(struct device *dev); > +#endif /* __INCLUDE_LINUX_IRQCHIP_TI_SCI_INTA_H */ > Thanks, M. -- Jazz is not dead. It just smells funny...
