| Message ID | 1454922971-17405-2-git-send-email-antoine.tenart@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Antoine, On 08/02/16 09:16, Antoine Tenart wrote: > This patch adds the Alpine MSIX interrupt controller driver. > > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> > Signed-off-by: Tsahee Zidenberg <tsahee@annapurnalabs.com> > --- > drivers/irqchip/Kconfig | 6 + > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-alpine-msi.c | 297 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 304 insertions(+) > create mode 100644 drivers/irqchip/irq-alpine-msi.c > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index 715923d5236c..f20e5b28eb5f 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -60,6 +60,12 @@ config ARM_VIC_NR > The maximum number of VICs available in the system, for > power management. > > +config ALPINE_MSI > + bool > + depends on PCI && PCI_MSI > + select GENERIC_IRQ_CHIP > + select PCI_MSI_IRQ_DOMAIN > + > config ATMEL_AIC_IRQ > bool > select GENERIC_IRQ_CHIP > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index 18caacb60d58..57f68e0eea74 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -1,5 +1,6 @@ > obj-$(CONFIG_IRQCHIP) += irqchip.o > > +obj-$(CONFIG_ALPINE_MSI) += irq-alpine-msi.o > obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o > obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o > obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o > diff --git a/drivers/irqchip/irq-alpine-msi.c b/drivers/irqchip/irq-alpine-msi.c > new file mode 100644 > index 000000000000..435dd4ab3626 > --- /dev/null > +++ b/drivers/irqchip/irq-alpine-msi.c > @@ -0,0 +1,297 @@ > +/* > + * Annapurna Labs MSIX support services > + * > + * Copyright (C) 2016, Amazon.com, Inc. or its affiliates. All Rights Reserved. > + * > + * Antoine Tenart <antoine.tenart@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/irqchip.h> > +#include <linux/irqchip/arm-gic.h> > +#include <linux/msi.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/of_pci.h> > +#include <linux/pci.h> > +#include <linux/slab.h> > + > +#include <asm/irq.h> > +#include <asm-generic/msi.h> > + > +/* MSIX message address format: local GIC target */ > +#define ALPINE_MSIX_SPI_TARGET_CLUSTER0 BIT(16) > + > +struct alpine_msix_data { > + spinlock_t msi_map_lock; > + u32 addr_high; > + u32 addr_low; As this looks to be a physical address, please consider using phys_addr_t. > + u32 spi_first; /* The SGI number that MSIs start */ > + u32 num_spis; /* The number of SGIs for MSIs */ > + unsigned long *msi_map; > +}; > + > +static void alpine_msix_mask_msi_irq(struct irq_data *d) > +{ > + pci_msi_mask_irq(d); > + irq_chip_mask_parent(d); > +} > + > +static void alpine_msix_unmask_msi_irq(struct irq_data *d) > +{ > + pci_msi_unmask_irq(d); > + irq_chip_unmask_parent(d); > +} > + > +static int alpine_msix_set_affinity(struct irq_data *irq_data, > + const struct cpumask *mask, bool force) > +{ > + int ret; > + > + ret = irq_chip_set_affinity_parent(irq_data, mask, force); > + return ret == IRQ_SET_MASK_OK ? IRQ_SET_MASK_OK_DONE : ret; > +} > + > +static struct irq_chip alpine_msix_irq_chip = { > + .name = "MSIx", > + .irq_mask = alpine_msix_mask_msi_irq, > + .irq_unmask = alpine_msix_unmask_msi_irq, > + .irq_eoi = irq_chip_eoi_parent, > + .irq_set_affinity = alpine_msix_set_affinity, > +}; > + > +static int alpine_msix_allocate_sgi(struct alpine_msix_data *priv, int num_req) > +{ > + int first, i; > + > + spin_lock(&priv->msi_map_lock); > + > + first = bitmap_find_next_zero_area(priv->msi_map, priv->num_spis, 0, > + num_req, 0); > + if (first >= priv->num_spis) { > + spin_unlock(&priv->msi_map_lock); > + return -ENOSPC; > + } > + > + for (i = 0; i < num_req; i++) > + set_bit(first + i, priv->msi_map); > + > + spin_unlock(&priv->msi_map_lock); > + > + return priv->spi_first + first; > +} > + > +static void alpine_msix_free_sgi(struct alpine_msix_data *priv, unsigned sgi, > + int num_req) > +{ > + int i, first; > + > + first = sgi - priv->spi_first; > + > + spin_lock(&priv->msi_map_lock); > + > + for (i = 0; i < num_req; i++) > + clear_bit(first + i, priv->msi_map); > + > + spin_unlock(&priv->msi_map_lock); > +} > + > +static void alpine_msix_compose_msi_msg(struct irq_data *data, > + struct msi_msg *msg) > +{ > + struct alpine_msix_data *priv = irq_data_get_irq_chip_data(data); > + > + msg->address_hi = priv->addr_high; > + msg->address_lo = priv->addr_low + (data->hwirq << 3); > + msg->data = 0; > +} > + > +static struct msi_domain_info alpine_msix_domain_info = { > + .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS | > + MSI_FLAG_PCI_MSIX, You can probably add MSI_FLAG_PCI_MSI, it should work as well (MULTI_MSI obviously won't). > + .chip = &alpine_msix_irq_chip, > +}; > + > +static struct irq_chip middle_irq_chip = { > + .name = "alpine_msix_middle", > + .irq_mask = irq_chip_mask_parent, > + .irq_unmask = irq_chip_unmask_parent, > + .irq_eoi = irq_chip_eoi_parent, > + .irq_set_affinity = irq_chip_set_affinity_parent, > + .irq_compose_msi_msg = alpine_msix_compose_msi_msg, > +}; > + > +static int alpine_msix_gic_domain_alloc(struct irq_domain *domain, > + unsigned int virq, int sgi) > +{ > + struct irq_fwspec fwspec; > + struct irq_data *d; > + int ret; > + > + if (!is_of_node(domain->parent->fwnode)) > + return -EINVAL; > + > + fwspec.fwnode = domain->parent->fwnode; > + fwspec.param_count = 3; > + fwspec.param[0] = 0; > + fwspec.param[1] = sgi; > + fwspec.param[2] = IRQ_TYPE_EDGE_RISING; > + > + ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec); > + if (ret) > + return ret; > + > + d = irq_domain_get_irq_data(domain->parent, virq); > + d->chip->irq_set_type(d, IRQ_TYPE_EDGE_RISING); > + > + return 0; > +} > + > +static int alpine_msix_middle_domain_alloc(struct irq_domain *domain, > + unsigned int virq, > + unsigned int nr_irqs, void *args) > +{ > + struct alpine_msix_data *priv = domain->host_data; > + int sgi, err, i; > + > + sgi = alpine_msix_allocate_sgi(priv, nr_irqs); > + if (sgi < 0) > + return sgi; > + > + for (i = 0; i < nr_irqs; i++) { > + err = alpine_msix_gic_domain_alloc(domain, virq + i, sgi + i); > + if (err) > + goto err_sgi; > + > + irq_domain_set_hwirq_and_chip(domain, virq + i, sgi + i, > + &middle_irq_chip, priv); > + } > + > + return 0; > + > +err_sgi: > + while (--i >= 0) > + irq_domain_free_irqs_parent(domain, virq, i); > + alpine_msix_free_sgi(priv, sgi, nr_irqs); > + return err; > +} > + > +static void alpine_msix_middle_domain_free(struct irq_domain *domain, > + unsigned int virq, > + unsigned int nr_irqs) > +{ > + struct irq_data *d = irq_domain_get_irq_data(domain, virq); > + struct alpine_msix_data *priv = irq_data_get_irq_chip_data(d); > + > + irq_domain_free_irqs_parent(domain, virq, nr_irqs); > + alpine_msix_free_sgi(priv, d->hwirq, nr_irqs); > +} > + > +static const struct irq_domain_ops alpine_msix_middle_domain_ops = { > + .alloc = alpine_msix_middle_domain_alloc, > + .free = alpine_msix_middle_domain_free, > +}; > + > +static int alpine_msix_init_domains(struct alpine_msix_data *priv, > + struct device_node *node) > +{ > + struct irq_domain *middle_domain, *msi_domain, *gic_domain; > + struct device_node *gic_node; > + > + gic_node = of_irq_find_parent(node); > + if (!gic_node) { > + pr_err("Failed to find the GIC node\n"); > + return -ENODEV; > + } > + > + gic_domain = irq_find_host(gic_node); > + if (!gic_domain) { > + pr_err("Failed to find the GIC domain\n"); > + return -ENXIO; > + } > + > + middle_domain = irq_domain_add_tree(NULL, > + &alpine_msix_middle_domain_ops, > + priv); > + if (!middle_domain) { > + pr_err("Failed to create the MSIX middle domain\n"); > + return -ENOMEM; > + } > + > + middle_domain->parent = gic_domain; > + > + msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(node), > + &alpine_msix_domain_info, > + middle_domain); > + if (!msi_domain) { > + pr_err("Failed to create MSI domain\n"); > + irq_domain_remove(msi_domain); > + return -ENOMEM; > + } > + > + return 0; > +} > + > +static int alpine_msix_init(struct device_node *node, > + struct device_node *parent) > +{ > + struct alpine_msix_data *priv; > + struct resource res; > + int ret; > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + spin_lock_init(&priv->msi_map_lock); > + > + ret = of_address_to_resource(node, 0, &res); > + if (ret) { > + pr_err("Failed to allocate resource\n"); > + goto err_priv; > + } > + > + priv->addr_high = upper_32_bits((u64)res.start); > + priv->addr_low = lower_32_bits(res.start) + ALPINE_MSIX_SPI_TARGET_CLUSTER0; This is a bit odd. If you always set bit 16, why isn't that reflected in the base address coming from the DT? > + > + if (of_property_read_u32(node, "al,msi-base-spi", &priv->spi_first)) { > + pr_err("Unable to parse MSI base\n"); > + ret = -EINVAL; > + goto err_priv; > + } > + > + if (of_property_read_u32(node, "al,msi-num-spis", &priv->num_spis)) { > + pr_err("Unable to parse MSI numbers\n"); > + ret = -EINVAL; > + goto err_priv; > + } > + > + priv->msi_map = kzalloc(sizeof(*priv->msi_map) * BITS_TO_LONGS(priv->num_spis), > + GFP_KERNEL); > + if (!priv->msi_map) { > + ret = -ENOMEM; > + goto err_priv; > + } > + > + pr_debug("Registering %d msixs, starting at %d\n", > + priv->num_spis, priv->spi_first); > + > + ret = alpine_msix_init_domains(priv, node); > + if (ret) > + goto err_map; > + > + return 0; > + > +err_map: > + kfree(priv->msi_map); > +err_priv: > + kfree(priv); > + return ret; > +} > +IRQCHIP_DECLARE(alpine_msix, "al,alpine-msix", alpine_msix_init); > Otherwise, looks pretty good. Thanks, M.
Hello Marc, On Mon, 8 Feb 2016 09:44:49 +0000, Marc Zyngier wrote: > > +static struct msi_domain_info alpine_msix_domain_info = { > > + .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS | > > + MSI_FLAG_PCI_MSIX, > > You can probably add MSI_FLAG_PCI_MSI, it should work as well (MULTI_MSI > obviously won't). Why wouldn't MULTI_MSI work? The code is using bitmap_find_next_zero_area() in alpine_msix_allocate_sgi() precisely to find num_req consecutive bits set to 0, in order to allocate multiple MSIs at once. Am I missing something? Thanks, Thomas
Hi Thomas, On 08/02/16 09:53, Thomas Petazzoni wrote: > Hello Marc, > > On Mon, 8 Feb 2016 09:44:49 +0000, Marc Zyngier wrote: > >>> +static struct msi_domain_info alpine_msix_domain_info = { >>> + .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS | >>> + MSI_FLAG_PCI_MSIX, >> >> You can probably add MSI_FLAG_PCI_MSI, it should work as well (MULTI_MSI >> obviously won't). > > Why wouldn't MULTI_MSI work? The code is using > bitmap_find_next_zero_area() in alpine_msix_allocate_sgi() precisely to > find num_req consecutive bits set to 0, in order to allocate multiple > MSIs at once. Am I missing something? The clue is in the patch: + msg->address_lo = priv->addr_low + (data->hwirq << 3); Multi-MSI imposes a single doorbell address, and consecutive message identifiers. So while the allocator deals perfectly with the consecutive IDs part, the fact that you have to encode the message in the address (instead of putting it in the data field) makes it completely incompatible with Multi-MSI. It is a bit silly that brand new HW comes out with such limitations (but Multi-MSI is such a pain anyway...). Thanks, M.
Hi Marc, On Mon, Feb 08, 2016 at 09:44:49AM +0000, Marc Zyngier wrote: > On 08/02/16 09:16, Antoine Tenart wrote: > > + > > +/* MSIX message address format: local GIC target */ > > +#define ALPINE_MSIX_SPI_TARGET_CLUSTER0 BIT(16) > > + > > +struct alpine_msix_data { > > + spinlock_t msi_map_lock; > > + u32 addr_high; > > + u32 addr_low; > > As this looks to be a physical address, please consider using phys_addr_t. Sure. […] > > +static int alpine_msix_init(struct device_node *node, > > + struct device_node *parent) > > +{ > > + struct alpine_msix_data *priv; > > + struct resource res; > > + int ret; > > + > > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + spin_lock_init(&priv->msi_map_lock); > > + > > + ret = of_address_to_resource(node, 0, &res); > > + if (ret) { > > + pr_err("Failed to allocate resource\n"); > > + goto err_priv; > > + } > > + > > + priv->addr_high = upper_32_bits((u64)res.start); > > + priv->addr_low = lower_32_bits(res.start) + ALPINE_MSIX_SPI_TARGET_CLUSTER0; > > This is a bit odd. If you always set bit 16, why isn't that reflected in > the base address coming from the DT? The 20 least significant bits of addr_low provide direct information regarding the interrupt destination, so I thought it would be clearer to have this explicitly in the driver so that we know what those bits mean. What do you think? Thanks for the review! Antoine
Antoine, On Mon, 8 Feb 2016, Antoine Tenart wrote: > +static int alpine_msix_set_affinity(struct irq_data *irq_data, > + const struct cpumask *mask, bool force) > +{ > + int ret; > + > + ret = irq_chip_set_affinity_parent(irq_data, mask, force); > + return ret == IRQ_SET_MASK_OK ? IRQ_SET_MASK_OK_DONE : ret; What's the point of this exercise? Why can't you just set the affinity callback to irq_chip_set_affinity_parent() ? > +static struct irq_chip alpine_msix_irq_chip = { > + .name = "MSIx", > + .irq_mask = alpine_msix_mask_msi_irq, > + .irq_unmask = alpine_msix_unmask_msi_irq, > + .irq_eoi = irq_chip_eoi_parent, > + .irq_set_affinity = alpine_msix_set_affinity, > +}; > + > +static int alpine_msix_allocate_sgi(struct alpine_msix_data *priv, int num_req) > +{ > + int first, i; > + > + spin_lock(&priv->msi_map_lock); > + > + first = bitmap_find_next_zero_area(priv->msi_map, priv->num_spis, 0, > + num_req, 0); > + if (first >= priv->num_spis) { > + spin_unlock(&priv->msi_map_lock); > + return -ENOSPC; > + } > + > + for (i = 0; i < num_req; i++) > + set_bit(first + i, priv->msi_map); bitmap_set() ?? > + > + spin_unlock(&priv->msi_map_lock); > + > + return priv->spi_first + first; > +} > + > +static void alpine_msix_free_sgi(struct alpine_msix_data *priv, unsigned sgi, > + int num_req) > +{ > + int i, first; > + > + first = sgi - priv->spi_first; > + > + spin_lock(&priv->msi_map_lock); > + > + for (i = 0; i < num_req; i++) > + clear_bit(first + i, priv->msi_map); bitmap_clear() ?? > + spin_unlock(&priv->msi_map_lock); > +} Thanks, tglx
On 08/02/16 10:26, Antoine Tenart wrote: >>> +static int alpine_msix_init(struct device_node *node, >>> + struct device_node *parent) >>> +{ >>> + struct alpine_msix_data *priv; >>> + struct resource res; >>> + int ret; >>> + >>> + priv = kzalloc(sizeof(*priv), GFP_KERNEL); >>> + if (!priv) >>> + return -ENOMEM; >>> + >>> + spin_lock_init(&priv->msi_map_lock); >>> + >>> + ret = of_address_to_resource(node, 0, &res); >>> + if (ret) { >>> + pr_err("Failed to allocate resource\n"); >>> + goto err_priv; >>> + } >>> + >>> + priv->addr_high = upper_32_bits((u64)res.start); >>> + priv->addr_low = lower_32_bits(res.start) + ALPINE_MSIX_SPI_TARGET_CLUSTER0; >> >> This is a bit odd. If you always set bit 16, why isn't that reflected in >> the base address coming from the DT? > > The 20 least significant bits of addr_low provide direct information > regarding the interrupt destination, so I thought it would be clearer > to have this explicitly in the driver so that we know what those bits > mean. So what is this information? TARGET_CLUSTER0 is not very expressive, and doesn't show what the alternatives are. Could you please elaborate a bit on that front? Thanks, M.
On Mon, Feb 08, 2016 at 10:32:08AM +0000, Marc Zyngier wrote: > On 08/02/16 10:26, Antoine Tenart wrote: > >>> +static int alpine_msix_init(struct device_node *node, > >>> + struct device_node *parent) > >>> +{ > >>> + struct alpine_msix_data *priv; > >>> + struct resource res; > >>> + int ret; > >>> + > >>> + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > >>> + if (!priv) > >>> + return -ENOMEM; > >>> + > >>> + spin_lock_init(&priv->msi_map_lock); > >>> + > >>> + ret = of_address_to_resource(node, 0, &res); > >>> + if (ret) { > >>> + pr_err("Failed to allocate resource\n"); > >>> + goto err_priv; > >>> + } > >>> + > >>> + priv->addr_high = upper_32_bits((u64)res.start); > >>> + priv->addr_low = lower_32_bits(res.start) + ALPINE_MSIX_SPI_TARGET_CLUSTER0; > >> > >> This is a bit odd. If you always set bit 16, why isn't that reflected in > >> the base address coming from the DT? > > > > The 20 least significant bits of addr_low provide direct information > > regarding the interrupt destination, so I thought it would be clearer > > to have this explicitly in the driver so that we know what those bits > > mean. > > So what is this information? TARGET_CLUSTER0 is not very expressive, and > doesn't show what the alternatives are. Could you please elaborate a bit > on that front? For now lots of bits are reserved, so there aren't many alternatives. Bits [18:17] are used to set the GIC to which to route the MSI and bit 16 must be set when this target GIC is the primary GIC (bits [18:17] set to 0x0). There aren't other options available for now (that I'm aware of) for the target GIC configuration. Antoine
On 08/02/16 10:44, Antoine Tenart wrote: > On Mon, Feb 08, 2016 at 10:32:08AM +0000, Marc Zyngier wrote: >> On 08/02/16 10:26, Antoine Tenart wrote: >>>>> +static int alpine_msix_init(struct device_node *node, >>>>> + struct device_node *parent) >>>>> +{ >>>>> + struct alpine_msix_data *priv; >>>>> + struct resource res; >>>>> + int ret; >>>>> + >>>>> + priv = kzalloc(sizeof(*priv), GFP_KERNEL); >>>>> + if (!priv) >>>>> + return -ENOMEM; >>>>> + >>>>> + spin_lock_init(&priv->msi_map_lock); >>>>> + >>>>> + ret = of_address_to_resource(node, 0, &res); >>>>> + if (ret) { >>>>> + pr_err("Failed to allocate resource\n"); >>>>> + goto err_priv; >>>>> + } >>>>> + >>>>> + priv->addr_high = upper_32_bits((u64)res.start); >>>>> + priv->addr_low = lower_32_bits(res.start) + ALPINE_MSIX_SPI_TARGET_CLUSTER0; >>>> >>>> This is a bit odd. If you always set bit 16, why isn't that reflected in >>>> the base address coming from the DT? >>> >>> The 20 least significant bits of addr_low provide direct information >>> regarding the interrupt destination, so I thought it would be clearer >>> to have this explicitly in the driver so that we know what those bits >>> mean. >> >> So what is this information? TARGET_CLUSTER0 is not very expressive, and >> doesn't show what the alternatives are. Could you please elaborate a bit >> on that front? > > For now lots of bits are reserved, so there aren't many alternatives. > Bits [18:17] are used to set the GIC to which to route the MSI and bit > 16 must be set when this target GIC is the primary GIC (bits [18:17] set > to 0x0). There aren't other options available for now (that I'm aware > of) for the target GIC configuration. OK. So maybe add that as a comment, so that people know what is happening there. And if the code gets updated to include new functionalities, it will be easier to track the changes. Thanks, M.
On Mon, Feb 08, 2016 at 10:56:16AM +0000, Marc Zyngier wrote: > On 08/02/16 10:44, Antoine Tenart wrote: > > On Mon, Feb 08, 2016 at 10:32:08AM +0000, Marc Zyngier wrote: > >> On 08/02/16 10:26, Antoine Tenart wrote: > >>>>> +static int alpine_msix_init(struct device_node *node, > >>>>> + struct device_node *parent) > >>>>> +{ > >>>>> + struct alpine_msix_data *priv; > >>>>> + struct resource res; > >>>>> + int ret; > >>>>> + > >>>>> + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > >>>>> + if (!priv) > >>>>> + return -ENOMEM; > >>>>> + > >>>>> + spin_lock_init(&priv->msi_map_lock); > >>>>> + > >>>>> + ret = of_address_to_resource(node, 0, &res); > >>>>> + if (ret) { > >>>>> + pr_err("Failed to allocate resource\n"); > >>>>> + goto err_priv; > >>>>> + } > >>>>> + > >>>>> + priv->addr_high = upper_32_bits((u64)res.start); > >>>>> + priv->addr_low = lower_32_bits(res.start) + ALPINE_MSIX_SPI_TARGET_CLUSTER0; > >>>> > >>>> This is a bit odd. If you always set bit 16, why isn't that reflected in > >>>> the base address coming from the DT? > >>> > >>> The 20 least significant bits of addr_low provide direct information > >>> regarding the interrupt destination, so I thought it would be clearer > >>> to have this explicitly in the driver so that we know what those bits > >>> mean. > >> > >> So what is this information? TARGET_CLUSTER0 is not very expressive, and > >> doesn't show what the alternatives are. Could you please elaborate a bit > >> on that front? > > > > For now lots of bits are reserved, so there aren't many alternatives. > > Bits [18:17] are used to set the GIC to which to route the MSI and bit > > 16 must be set when this target GIC is the primary GIC (bits [18:17] set > > to 0x0). There aren't other options available for now (that I'm aware > > of) for the target GIC configuration. > > OK. So maybe add that as a comment, so that people know what is > happening there. And if the code gets updated to include new > functionalities, it will be easier to track the changes. OK, I'll update. Antoine
Hi Marc, On Mon, Feb 08, 2016 at 09:44:49AM +0000, Marc Zyngier wrote: > On 08/02/16 09:16, Antoine Tenart wrote: > > + > > +static struct msi_domain_info alpine_msix_domain_info = { > > + .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS | > > + MSI_FLAG_PCI_MSIX, > > You can probably add MSI_FLAG_PCI_MSI Are you sure such a flag is available? (Or am I missing something obvious?). Antoine
On 08/02/16 14:04, Antoine Tenart wrote: > Hi Marc, > > On Mon, Feb 08, 2016 at 09:44:49AM +0000, Marc Zyngier wrote: >> On 08/02/16 09:16, Antoine Tenart wrote: >>> + >>> +static struct msi_domain_info alpine_msix_domain_info = { >>> + .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS | >>> + MSI_FLAG_PCI_MSIX, >> >> You can probably add MSI_FLAG_PCI_MSI > > Are you sure such a flag is available? (Or am I missing something > obvious?). No, I'm just confused (first patch in the morning, what did you expect?). It looks like we consider single MSI as a given, please ignore my rambling... Thanks, M.
Thomas, On Mon, Feb 08, 2016 at 11:31:47AM +0100, Thomas Gleixner wrote: > On Mon, 8 Feb 2016, Antoine Tenart wrote: > > +static int alpine_msix_set_affinity(struct irq_data *irq_data, > > + const struct cpumask *mask, bool force) > > +{ > > + int ret; > > + > > + ret = irq_chip_set_affinity_parent(irq_data, mask, force); > > + return ret == IRQ_SET_MASK_OK ? IRQ_SET_MASK_OK_DONE : ret; > > What's the point of this exercise? Why can't you just set the affinity > callback to irq_chip_set_affinity_parent() ? That's what done in irq-gic-v2m.c. Besides that, I see no point. I'll update for v2. > > +static struct irq_chip alpine_msix_irq_chip = { > > + .name = "MSIx", > > + .irq_mask = alpine_msix_mask_msi_irq, > > + .irq_unmask = alpine_msix_unmask_msi_irq, > > + .irq_eoi = irq_chip_eoi_parent, > > + .irq_set_affinity = alpine_msix_set_affinity, > > +}; > > + > > +static int alpine_msix_allocate_sgi(struct alpine_msix_data *priv, int num_req) > > +{ > > + int first, i; > > + > > + spin_lock(&priv->msi_map_lock); > > + > > + first = bitmap_find_next_zero_area(priv->msi_map, priv->num_spis, 0, > > + num_req, 0); > > + if (first >= priv->num_spis) { > > + spin_unlock(&priv->msi_map_lock); > > + return -ENOSPC; > > + } > > + > > + for (i = 0; i < num_req; i++) > > + set_bit(first + i, priv->msi_map); > > bitmap_set() ?? Indeed, that's better :) > > + spin_unlock(&priv->msi_map_lock); > > + > > + return priv->spi_first + first; > > +} > > + > > +static void alpine_msix_free_sgi(struct alpine_msix_data *priv, unsigned sgi, > > + int num_req) > > +{ > > + int i, first; > > + > > + first = sgi - priv->spi_first; > > + > > + spin_lock(&priv->msi_map_lock); > > + > > + for (i = 0; i < num_req; i++) > > + clear_bit(first + i, priv->msi_map); > > bitmap_clear() ?? Ditto. Antoine
On 08/02/16 14:17, Antoine Tenart wrote: > Thomas, > > On Mon, Feb 08, 2016 at 11:31:47AM +0100, Thomas Gleixner wrote: >> On Mon, 8 Feb 2016, Antoine Tenart wrote: >>> +static int alpine_msix_set_affinity(struct irq_data *irq_data, >>> + const struct cpumask *mask, bool force) >>> +{ >>> + int ret; >>> + >>> + ret = irq_chip_set_affinity_parent(irq_data, mask, force); >>> + return ret == IRQ_SET_MASK_OK ? IRQ_SET_MASK_OK_DONE : ret; >> >> What's the point of this exercise? Why can't you just set the affinity >> callback to irq_chip_set_affinity_parent() ? > > That's what done in irq-gic-v2m.c. Besides that, I see no point. I'll > update for v2. That's because there is no need to do another compose_msi_msg/write_msg in msi_domain_set_affinity() once the affinity has been updated at the GIC level. Alternatively, updating the GIC driver to always return IRQ_SET_MASK_OK_DONE would be perfectly acceptable. Thanks, M.
On Mon, Feb 08, 2016 at 02:29:12PM +0000, Marc Zyngier wrote: > On 08/02/16 14:17, Antoine Tenart wrote: > > Thomas, > > > > On Mon, Feb 08, 2016 at 11:31:47AM +0100, Thomas Gleixner wrote: > >> On Mon, 8 Feb 2016, Antoine Tenart wrote: > >>> +static int alpine_msix_set_affinity(struct irq_data *irq_data, > >>> + const struct cpumask *mask, bool force) > >>> +{ > >>> + int ret; > >>> + > >>> + ret = irq_chip_set_affinity_parent(irq_data, mask, force); > >>> + return ret == IRQ_SET_MASK_OK ? IRQ_SET_MASK_OK_DONE : ret; > >> > >> What's the point of this exercise? Why can't you just set the affinity > >> callback to irq_chip_set_affinity_parent() ? > > > > That's what done in irq-gic-v2m.c. Besides that, I see no point. I'll > > update for v2. > > That's because there is no need to do another compose_msi_msg/write_msg > in msi_domain_set_affinity() once the affinity has been updated at the > GIC level. Alternatively, updating the GIC driver to always return > IRQ_SET_MASK_OK_DONE would be perfectly acceptable. I'm using drivers/irqchip/irq-gic-v3.c which is indeed always returning IRQ_SET_MASK_OK. I'll make a new patch in the v2 of this series to return IRQ_SET_MASK_OK_DONE instead in the GIC driver (and then patch irq-gic-v2m.c). Thanks, Antoine
On 08/02/16 14:48, Antoine Tenart wrote: > On Mon, Feb 08, 2016 at 02:29:12PM +0000, Marc Zyngier wrote: >> On 08/02/16 14:17, Antoine Tenart wrote: >>> Thomas, >>> >>> On Mon, Feb 08, 2016 at 11:31:47AM +0100, Thomas Gleixner wrote: >>>> On Mon, 8 Feb 2016, Antoine Tenart wrote: >>>>> +static int alpine_msix_set_affinity(struct irq_data *irq_data, >>>>> + const struct cpumask *mask, bool force) >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + ret = irq_chip_set_affinity_parent(irq_data, mask, force); >>>>> + return ret == IRQ_SET_MASK_OK ? IRQ_SET_MASK_OK_DONE : ret; >>>> >>>> What's the point of this exercise? Why can't you just set the affinity >>>> callback to irq_chip_set_affinity_parent() ? >>> >>> That's what done in irq-gic-v2m.c. Besides that, I see no point. I'll >>> update for v2. >> >> That's because there is no need to do another compose_msi_msg/write_msg >> in msi_domain_set_affinity() once the affinity has been updated at the >> GIC level. Alternatively, updating the GIC driver to always return >> IRQ_SET_MASK_OK_DONE would be perfectly acceptable. > > I'm using drivers/irqchip/irq-gic-v3.c which is indeed always returning > IRQ_SET_MASK_OK. I'll make a new patch in the v2 of this series to > return IRQ_SET_MASK_OK_DONE instead in the GIC driver (and then patch > irq-gic-v2m.c). /me puzzled. GICv3, but no ITS??? WTF??? M.
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index 715923d5236c..f20e5b28eb5f 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -60,6 +60,12 @@ config ARM_VIC_NR The maximum number of VICs available in the system, for power management. +config ALPINE_MSI + bool + depends on PCI && PCI_MSI + select GENERIC_IRQ_CHIP + select PCI_MSI_IRQ_DOMAIN + config ATMEL_AIC_IRQ bool select GENERIC_IRQ_CHIP diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index 18caacb60d58..57f68e0eea74 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -1,5 +1,6 @@ obj-$(CONFIG_IRQCHIP) += irqchip.o +obj-$(CONFIG_ALPINE_MSI) += irq-alpine-msi.o obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o diff --git a/drivers/irqchip/irq-alpine-msi.c b/drivers/irqchip/irq-alpine-msi.c new file mode 100644 index 000000000000..435dd4ab3626 --- /dev/null +++ b/drivers/irqchip/irq-alpine-msi.c @@ -0,0 +1,297 @@ +/* + * Annapurna Labs MSIX support services + * + * Copyright (C) 2016, Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Antoine Tenart <antoine.tenart@free-electrons.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/irqchip.h> +#include <linux/irqchip/arm-gic.h> +#include <linux/msi.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/of_pci.h> +#include <linux/pci.h> +#include <linux/slab.h> + +#include <asm/irq.h> +#include <asm-generic/msi.h> + +/* MSIX message address format: local GIC target */ +#define ALPINE_MSIX_SPI_TARGET_CLUSTER0 BIT(16) + +struct alpine_msix_data { + spinlock_t msi_map_lock; + u32 addr_high; + u32 addr_low; + u32 spi_first; /* The SGI number that MSIs start */ + u32 num_spis; /* The number of SGIs for MSIs */ + unsigned long *msi_map; +}; + +static void alpine_msix_mask_msi_irq(struct irq_data *d) +{ + pci_msi_mask_irq(d); + irq_chip_mask_parent(d); +} + +static void alpine_msix_unmask_msi_irq(struct irq_data *d) +{ + pci_msi_unmask_irq(d); + irq_chip_unmask_parent(d); +} + +static int alpine_msix_set_affinity(struct irq_data *irq_data, + const struct cpumask *mask, bool force) +{ + int ret; + + ret = irq_chip_set_affinity_parent(irq_data, mask, force); + return ret == IRQ_SET_MASK_OK ? IRQ_SET_MASK_OK_DONE : ret; +} + +static struct irq_chip alpine_msix_irq_chip = { + .name = "MSIx", + .irq_mask = alpine_msix_mask_msi_irq, + .irq_unmask = alpine_msix_unmask_msi_irq, + .irq_eoi = irq_chip_eoi_parent, + .irq_set_affinity = alpine_msix_set_affinity, +}; + +static int alpine_msix_allocate_sgi(struct alpine_msix_data *priv, int num_req) +{ + int first, i; + + spin_lock(&priv->msi_map_lock); + + first = bitmap_find_next_zero_area(priv->msi_map, priv->num_spis, 0, + num_req, 0); + if (first >= priv->num_spis) { + spin_unlock(&priv->msi_map_lock); + return -ENOSPC; + } + + for (i = 0; i < num_req; i++) + set_bit(first + i, priv->msi_map); + + spin_unlock(&priv->msi_map_lock); + + return priv->spi_first + first; +} + +static void alpine_msix_free_sgi(struct alpine_msix_data *priv, unsigned sgi, + int num_req) +{ + int i, first; + + first = sgi - priv->spi_first; + + spin_lock(&priv->msi_map_lock); + + for (i = 0; i < num_req; i++) + clear_bit(first + i, priv->msi_map); + + spin_unlock(&priv->msi_map_lock); +} + +static void alpine_msix_compose_msi_msg(struct irq_data *data, + struct msi_msg *msg) +{ + struct alpine_msix_data *priv = irq_data_get_irq_chip_data(data); + + msg->address_hi = priv->addr_high; + msg->address_lo = priv->addr_low + (data->hwirq << 3); + msg->data = 0; +} + +static struct msi_domain_info alpine_msix_domain_info = { + .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS | + MSI_FLAG_PCI_MSIX, + .chip = &alpine_msix_irq_chip, +}; + +static struct irq_chip middle_irq_chip = { + .name = "alpine_msix_middle", + .irq_mask = irq_chip_mask_parent, + .irq_unmask = irq_chip_unmask_parent, + .irq_eoi = irq_chip_eoi_parent, + .irq_set_affinity = irq_chip_set_affinity_parent, + .irq_compose_msi_msg = alpine_msix_compose_msi_msg, +}; + +static int alpine_msix_gic_domain_alloc(struct irq_domain *domain, + unsigned int virq, int sgi) +{ + struct irq_fwspec fwspec; + struct irq_data *d; + int ret; + + if (!is_of_node(domain->parent->fwnode)) + return -EINVAL; + + fwspec.fwnode = domain->parent->fwnode; + fwspec.param_count = 3; + fwspec.param[0] = 0; + fwspec.param[1] = sgi; + fwspec.param[2] = IRQ_TYPE_EDGE_RISING; + + ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec); + if (ret) + return ret; + + d = irq_domain_get_irq_data(domain->parent, virq); + d->chip->irq_set_type(d, IRQ_TYPE_EDGE_RISING); + + return 0; +} + +static int alpine_msix_middle_domain_alloc(struct irq_domain *domain, + unsigned int virq, + unsigned int nr_irqs, void *args) +{ + struct alpine_msix_data *priv = domain->host_data; + int sgi, err, i; + + sgi = alpine_msix_allocate_sgi(priv, nr_irqs); + if (sgi < 0) + return sgi; + + for (i = 0; i < nr_irqs; i++) { + err = alpine_msix_gic_domain_alloc(domain, virq + i, sgi + i); + if (err) + goto err_sgi; + + irq_domain_set_hwirq_and_chip(domain, virq + i, sgi + i, + &middle_irq_chip, priv); + } + + return 0; + +err_sgi: + while (--i >= 0) + irq_domain_free_irqs_parent(domain, virq, i); + alpine_msix_free_sgi(priv, sgi, nr_irqs); + return err; +} + +static void alpine_msix_middle_domain_free(struct irq_domain *domain, + unsigned int virq, + unsigned int nr_irqs) +{ + struct irq_data *d = irq_domain_get_irq_data(domain, virq); + struct alpine_msix_data *priv = irq_data_get_irq_chip_data(d); + + irq_domain_free_irqs_parent(domain, virq, nr_irqs); + alpine_msix_free_sgi(priv, d->hwirq, nr_irqs); +} + +static const struct irq_domain_ops alpine_msix_middle_domain_ops = { + .alloc = alpine_msix_middle_domain_alloc, + .free = alpine_msix_middle_domain_free, +}; + +static int alpine_msix_init_domains(struct alpine_msix_data *priv, + struct device_node *node) +{ + struct irq_domain *middle_domain, *msi_domain, *gic_domain; + struct device_node *gic_node; + + gic_node = of_irq_find_parent(node); + if (!gic_node) { + pr_err("Failed to find the GIC node\n"); + return -ENODEV; + } + + gic_domain = irq_find_host(gic_node); + if (!gic_domain) { + pr_err("Failed to find the GIC domain\n"); + return -ENXIO; + } + + middle_domain = irq_domain_add_tree(NULL, + &alpine_msix_middle_domain_ops, + priv); + if (!middle_domain) { + pr_err("Failed to create the MSIX middle domain\n"); + return -ENOMEM; + } + + middle_domain->parent = gic_domain; + + msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(node), + &alpine_msix_domain_info, + middle_domain); + if (!msi_domain) { + pr_err("Failed to create MSI domain\n"); + irq_domain_remove(msi_domain); + return -ENOMEM; + } + + return 0; +} + +static int alpine_msix_init(struct device_node *node, + struct device_node *parent) +{ + struct alpine_msix_data *priv; + struct resource res; + int ret; + + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + spin_lock_init(&priv->msi_map_lock); + + ret = of_address_to_resource(node, 0, &res); + if (ret) { + pr_err("Failed to allocate resource\n"); + goto err_priv; + } + + priv->addr_high = upper_32_bits((u64)res.start); + priv->addr_low = lower_32_bits(res.start) + ALPINE_MSIX_SPI_TARGET_CLUSTER0; + + if (of_property_read_u32(node, "al,msi-base-spi", &priv->spi_first)) { + pr_err("Unable to parse MSI base\n"); + ret = -EINVAL; + goto err_priv; + } + + if (of_property_read_u32(node, "al,msi-num-spis", &priv->num_spis)) { + pr_err("Unable to parse MSI numbers\n"); + ret = -EINVAL; + goto err_priv; + } + + priv->msi_map = kzalloc(sizeof(*priv->msi_map) * BITS_TO_LONGS(priv->num_spis), + GFP_KERNEL); + if (!priv->msi_map) { + ret = -ENOMEM; + goto err_priv; + } + + pr_debug("Registering %d msixs, starting at %d\n", + priv->num_spis, priv->spi_first); + + ret = alpine_msix_init_domains(priv, node); + if (ret) + goto err_map; + + return 0; + +err_map: + kfree(priv->msi_map); +err_priv: + kfree(priv); + return ret; +} +IRQCHIP_DECLARE(alpine_msix, "al,alpine-msix", alpine_msix_init);