* [PATCH 5/7] ACPI / irq: Add GSI IRQ domain getter function @ 2019-03-31 12:35 Hanna Hawa 2019-03-31 12:35 ` [PATCH 6/7] irqchip/al-msi: Refactor in preparation to add ACPI support Hanna Hawa 2019-03-31 12:35 ` [PATCH 7/7] irqchip/al-msi: Add " Hanna Hawa 0 siblings, 2 replies; 8+ messages in thread From: Hanna Hawa @ 2019-03-31 12:35 UTC (permalink / raw) To: tsahee, antoine.tenart, linux, catalin.marinas, will.deacon, rjw, lenb, tglx, jason, marc.zyngier Cc: linux-arm-kernel, barakw, hhhawa, vaerov, linux-kernel, hanochu, linux-acpi, zeev, dwmw, jonnyc, ronenk, talel, alisaidi Add GSI IRQ domain getter function (acpi_get_gsi_domain_id), for IRQ drivers that use ACPI and need the IRQ parent domain to register their irq-chip device. Signed-off-by: Hanna Hawa <hhhawa@amazon.com> Co-developed-by: Vladimir Aerov <vaerov@amazon.com> Signed-off-by: Vladimir Aerov <vaerov@amazon.com> --- drivers/acpi/irq.c | 13 +++++++++++++ include/linux/acpi.h | 1 + 2 files changed, 14 insertions(+) diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c index 7c352cb..efc57b1 100644 --- a/drivers/acpi/irq.c +++ b/drivers/acpi/irq.c @@ -295,3 +295,16 @@ void __init acpi_set_irq_model(enum acpi_irq_model_id model, acpi_irq_model = model; acpi_gsi_domain_id = fwnode; } + +/** + * acpi_get_gsi_domain_id - getter for the GSI irqdomain information + * + * return: + * @fwnode: return the irq_domain identifier for mapping and looking up + * GSI interrupts + */ +struct fwnode_handle *acpi_get_gsi_domain_id(void) +{ + return acpi_gsi_domain_id; +} +EXPORT_SYMBOL_GPL(acpi_get_gsi_domain_id); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 87715f2..642c2e2 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -322,6 +322,7 @@ int acpi_isa_irq_to_gsi (unsigned isa_irq, u32 *gsi); void acpi_set_irq_model(enum acpi_irq_model_id model, struct fwnode_handle *fwnode); +struct fwnode_handle *acpi_get_gsi_domain_id(void); #ifdef CONFIG_X86_IO_APIC extern int acpi_get_override_irq(u32 gsi, int *trigger, int *polarity); -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 6/7] irqchip/al-msi: Refactor in preparation to add ACPI support 2019-03-31 12:35 [PATCH 5/7] ACPI / irq: Add GSI IRQ domain getter function Hanna Hawa @ 2019-03-31 12:35 ` Hanna Hawa 2019-03-31 12:35 ` [PATCH 7/7] irqchip/al-msi: Add " Hanna Hawa 1 sibling, 0 replies; 8+ messages in thread From: Hanna Hawa @ 2019-03-31 12:35 UTC (permalink / raw) To: tsahee, antoine.tenart, linux, catalin.marinas, will.deacon, rjw, lenb, tglx, jason, marc.zyngier Cc: linux-arm-kernel, barakw, hhhawa, vaerov, linux-kernel, hanochu, linux-acpi, zeev, dwmw, jonnyc, ronenk, talel, alisaidi Move common parts which will be shared between DT and ACPI parsing, to a common function. Signed-off-by: Hanna Hawa <hhhawa@amazon.com> --- drivers/irqchip/irq-al-msi.c | 87 +++++++++++++++++++++++++------------------- 1 file changed, 49 insertions(+), 38 deletions(-) diff --git a/drivers/irqchip/irq-al-msi.c b/drivers/irqchip/irq-al-msi.c index f04a311c..ec27455 100644 --- a/drivers/irqchip/irq-al-msi.c +++ b/drivers/irqchip/irq-al-msi.c @@ -26,6 +26,7 @@ #define AL_MSIX_SPI_TARGET_CLUSTER0 BIT(16) struct al_msix_data { + struct fwnode_handle *msi_domain_handle; spinlock_t msi_map_lock; phys_addr_t addr; u32 spi_first; /* The SPI number that MSIs start */ @@ -190,22 +191,9 @@ static const struct irq_domain_ops al_msix_middle_domain_ops = { }; static int al_msix_init_domains(struct al_msix_data *priv, - struct device_node *node) + struct irq_domain *gic_domain) { - 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; - } + struct irq_domain *middle_domain, *msi_domain; middle_domain = irq_domain_add_tree(NULL, &al_msix_middle_domain_ops, priv); @@ -216,7 +204,7 @@ static int al_msix_init_domains(struct al_msix_data *priv, middle_domain->parent = gic_domain; - msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(node), + msi_domain = pci_msi_create_irq_domain(priv->msi_domain_handle, &al_msix_domain_info, middle_domain); if (!msi_domain) { @@ -228,34 +216,62 @@ static int al_msix_init_domains(struct al_msix_data *priv, return 0; } +static int al_msix_init_common(struct al_msix_data *priv, u64 base_address) +{ + spin_lock_init(&priv->msi_map_lock); + + /* + * The 20 least significant bits of addr provide direct information + * regarding the interrupt destination. + * + * To select the primary GIC as the target GIC, bits [18:17] must be set + * to 0x0. In this case, bit 16 (SPI_TARGET_CLUSTER0) must be set. + */ + priv->addr = base_address & GENMASK_ULL(63, 20); + priv->addr |= AL_MSIX_SPI_TARGET_CLUSTER0; + + priv->msi_map = kcalloc(BITS_TO_LONGS(priv->num_spis), + sizeof(*priv->msi_map), + GFP_KERNEL); + if (!priv->msi_map) + return -ENOMEM; + + pr_debug("Registering %d msixs, starting at %d\n", priv->num_spis, + priv->spi_first); + + return 0; +} + static int al_msix_init(struct device_node *node, struct device_node *parent) { struct al_msix_data *priv; struct resource res; + struct device_node *gic_node; + struct irq_domain *gic_domain; int ret; + 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; + } + 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; } - /* - * The 20 least significant bits of addr provide direct information - * regarding the interrupt destination. - * - * To select the primary GIC as the target GIC, bits [18:17] must be set - * to 0x0. In this case, bit 16 (SPI_TARGET_CLUSTER0) must be set. - */ - priv->addr = res.start & GENMASK_ULL(63, 20); - priv->addr |= AL_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; @@ -268,18 +284,13 @@ static int al_msix_init(struct device_node *node, struct device_node *parent) goto err_priv; } - priv->msi_map = kcalloc(BITS_TO_LONGS(priv->num_spis), - sizeof(*priv->msi_map), - GFP_KERNEL); - if (!priv->msi_map) { - ret = -ENOMEM; - goto err_priv; - } + priv->msi_domain_handle = of_node_to_fwnode(node); - pr_debug("Registering %d msixs, starting at %d\n", - priv->num_spis, priv->spi_first); + ret = al_msix_init_common(priv, res.start); + if (ret) + goto err_priv; - ret = al_msix_init_domains(priv, node); + ret = al_msix_init_domains(priv, gic_domain); if (ret) goto err_map; -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 7/7] irqchip/al-msi: Add ACPI support 2019-03-31 12:35 [PATCH 5/7] ACPI / irq: Add GSI IRQ domain getter function Hanna Hawa 2019-03-31 12:35 ` [PATCH 6/7] irqchip/al-msi: Refactor in preparation to add ACPI support Hanna Hawa @ 2019-03-31 12:35 ` Hanna Hawa 2019-04-01 2:15 ` Marc Zyngier 1 sibling, 1 reply; 8+ messages in thread From: Hanna Hawa @ 2019-03-31 12:35 UTC (permalink / raw) To: tsahee, antoine.tenart, linux, catalin.marinas, will.deacon, rjw, lenb, tglx, jason, marc.zyngier Cc: linux-arm-kernel, barakw, hhhawa, vaerov, linux-kernel, hanochu, linux-acpi, zeev, dwmw, jonnyc, ronenk, talel, alisaidi This patch adds ACPI support for AL-MSIx driver. The AL-MSIx controller is not standard, is not included in the UEFI specification, and will not be added. The driver ACPI binding is performed when the following conditions are true: - OEM ID is AMAZON - MADT table type is 0x80 (part of the OEM reserved range). Signed-off-by: Hanna Hawa <hhhawa@amazon.com> Co-developed-by: Vladimir Aerov <vaerov@amazon.com> Signed-off-by: Vladimir Aerov <vaerov@amazon.com> --- drivers/irqchip/irq-al-msi.c | 118 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 111 insertions(+), 7 deletions(-) diff --git a/drivers/irqchip/irq-al-msi.c b/drivers/irqchip/irq-al-msi.c index ec27455..cb80c1e 100644 --- a/drivers/irqchip/irq-al-msi.c +++ b/drivers/irqchip/irq-al-msi.c @@ -9,6 +9,7 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt +#include <linux/acpi.h> #include <linux/dma-iommu.h> #include <linux/irqchip.h> #include <linux/irqchip/arm-gic.h> @@ -126,14 +127,20 @@ static int al_msix_gic_domain_alloc(struct irq_domain *domain, struct irq_data *d; int ret; - if (!is_of_node(domain->parent->fwnode)) + if (is_of_node(domain->parent->fwnode)) { + fwspec.fwnode = domain->parent->fwnode; + fwspec.param_count = 3; + fwspec.param[0] = 0; + fwspec.param[1] = spi; + fwspec.param[2] = IRQ_TYPE_EDGE_RISING; + } else if (is_fwnode_irqchip(domain->parent->fwnode)) { + fwspec.fwnode = domain->parent->fwnode; + fwspec.param_count = 2; + fwspec.param[0] = spi + 32; + fwspec.param[1] = IRQ_TYPE_EDGE_RISING; + } else { return -EINVAL; - - fwspec.fwnode = domain->parent->fwnode; - fwspec.param_count = 3; - fwspec.param[0] = 0; - fwspec.param[1] = spi; - fwspec.param[2] = IRQ_TYPE_EDGE_RISING; + } ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec); if (ret) @@ -304,3 +311,100 @@ static int al_msix_init(struct device_node *node, struct device_node *parent) } IRQCHIP_DECLARE(alpine_msix, "al,alpine-msix", al_msix_init); IRQCHIP_DECLARE(al_msix, "amazon,al-msix", al_msix_init); + +#ifdef CONFIG_ACPI +static struct al_msix_data *priv; + +#define ACPI_AMZN_MADT_OEM_TYPE 0x80 +#define ACPI_AMZN_OEM_ID "AMAZON" + +struct acpi_madt_msix_oem_frame { + struct acpi_subtable_header header; + u64 base_address; + u32 base_address_len; + u16 spi_count; + u16 spi_base; +}; + +static struct fwnode_handle *al_msi_get_fwnode(struct device *dev) +{ + return priv->msi_domain_handle; +} + +static int __init al_msix_acpi_probe(struct acpi_subtable_header *header, + const unsigned long end) +{ + struct irq_domain *gic_domain; + struct fwnode_handle *gic_domain_handle; + struct acpi_madt_msix_oem_frame *m; + int ret; + + m = container_of(header, struct acpi_madt_msix_oem_frame, header); + if (BAD_MADT_ENTRY(m, end)) + return -EINVAL; + + gic_domain_handle = acpi_get_gsi_domain_id(); + if (!gic_domain_handle) { + pr_err("Failed to find the GIC domain handle\n"); + return -ENXIO; + } + + gic_domain = irq_find_matching_fwnode(gic_domain_handle, + DOMAIN_BUS_ANY); + if (!gic_domain) { + pr_err("Failed to find the GIC domain\n"); + return -ENXIO; + } + + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->spi_first = m->spi_base; + priv->num_spis = m->spi_count; + + priv->msi_domain_handle = irq_domain_alloc_fwnode((void *) + m->base_address); + if (!priv->msi_domain_handle) { + pr_err("Unable to allocate msi domain token\n"); + ret = -EINVAL; + goto err_acpi_priv; + } + + ret = al_msix_init_common(priv, m->base_address); + if (ret) + goto err_acpi_priv; + + ret = al_msix_init_domains(priv, gic_domain); + if (ret) + goto err_acpi_map; + + pci_msi_register_fwnode_provider(&al_msi_get_fwnode); + + return 0; + +err_acpi_map: + kfree(priv->msi_map); +err_acpi_priv: + kfree(priv); + return ret; +} + +static int __init al_msix_acpi_init(void) +{ + static struct acpi_table_madt *madt; + acpi_status status; + + /* if ACPI MADT table is not Amazon defined return */ + status = acpi_get_table(ACPI_SIG_MADT, 0, + (struct acpi_table_header **)&madt); + if (ACPI_FAILURE(status) || (madt && memcmp(madt->header.oem_id, + ACPI_AMZN_OEM_ID, + ACPI_OEM_ID_SIZE))) + return -ENODEV; + + return acpi_table_parse_madt(ACPI_AMZN_MADT_OEM_TYPE, + al_msix_acpi_probe, 0); +} +early_initcall(al_msix_acpi_init); +#endif /* CONFIG_ACPI */ -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 7/7] irqchip/al-msi: Add ACPI support 2019-03-31 12:35 ` [PATCH 7/7] irqchip/al-msi: Add " Hanna Hawa @ 2019-04-01 2:15 ` Marc Zyngier 2019-04-04 14:45 ` Zeev Zilberman 0 siblings, 1 reply; 8+ messages in thread From: Marc Zyngier @ 2019-04-01 2:15 UTC (permalink / raw) To: Hanna Hawa Cc: catalin.marinas, will.deacon, jonnyc, ronenk, Lorenzo Pieralisi, vaerov, linux, talel, linux-acpi, alisaidi, lenb, jason, antoine.tenart, zeev, tglx, hanochu, linux-arm-kernel, barakw, rjw, linux-kernel, tsahee, dwmw On Sun, 31 Mar 2019 13:35:33 +0100, Hanna Hawa <hhhawa@amazon.com> wrote: > > This patch adds ACPI support for AL-MSIx driver. > > The AL-MSIx controller is not standard, is not included in the UEFI > specification, and will not be added. The driver ACPI binding is > performed when the following conditions are true: > - OEM ID is AMAZON > - MADT table type is 0x80 (part of the OEM reserved range). > > Signed-off-by: Hanna Hawa <hhhawa@amazon.com> > Co-developed-by: Vladimir Aerov <vaerov@amazon.com> > Signed-off-by: Vladimir Aerov <vaerov@amazon.com> > --- > drivers/irqchip/irq-al-msi.c | 118 ++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 111 insertions(+), 7 deletions(-) > > diff --git a/drivers/irqchip/irq-al-msi.c b/drivers/irqchip/irq-al-msi.c > index ec27455..cb80c1e 100644 > --- a/drivers/irqchip/irq-al-msi.c > +++ b/drivers/irqchip/irq-al-msi.c > @@ -9,6 +9,7 @@ > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > +#include <linux/acpi.h> > #include <linux/dma-iommu.h> > #include <linux/irqchip.h> > #include <linux/irqchip/arm-gic.h> > @@ -126,14 +127,20 @@ static int al_msix_gic_domain_alloc(struct irq_domain *domain, > struct irq_data *d; > int ret; > > - if (!is_of_node(domain->parent->fwnode)) > + if (is_of_node(domain->parent->fwnode)) { > + fwspec.fwnode = domain->parent->fwnode; > + fwspec.param_count = 3; > + fwspec.param[0] = 0; > + fwspec.param[1] = spi; > + fwspec.param[2] = IRQ_TYPE_EDGE_RISING; > + } else if (is_fwnode_irqchip(domain->parent->fwnode)) { > + fwspec.fwnode = domain->parent->fwnode; > + fwspec.param_count = 2; > + fwspec.param[0] = spi + 32; > + fwspec.param[1] = IRQ_TYPE_EDGE_RISING; > + } else { > return -EINVAL; > - > - fwspec.fwnode = domain->parent->fwnode; > - fwspec.param_count = 3; > - fwspec.param[0] = 0; > - fwspec.param[1] = spi; > - fwspec.param[2] = IRQ_TYPE_EDGE_RISING; > + } > > ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec); > if (ret) > @@ -304,3 +311,100 @@ static int al_msix_init(struct device_node *node, struct device_node *parent) > } > IRQCHIP_DECLARE(alpine_msix, "al,alpine-msix", al_msix_init); > IRQCHIP_DECLARE(al_msix, "amazon,al-msix", al_msix_init); > + > +#ifdef CONFIG_ACPI > +static struct al_msix_data *priv; > + > +#define ACPI_AMZN_MADT_OEM_TYPE 0x80 > +#define ACPI_AMZN_OEM_ID "AMAZON" > + > +struct acpi_madt_msix_oem_frame { > + struct acpi_subtable_header header; > + u64 base_address; > + u32 base_address_len; > + u16 spi_count; > + u16 spi_base; > +}; > + > +static struct fwnode_handle *al_msi_get_fwnode(struct device *dev) > +{ > + return priv->msi_domain_handle; > +} > + > +static int __init al_msix_acpi_probe(struct acpi_subtable_header *header, > + const unsigned long end) > +{ > + struct irq_domain *gic_domain; > + struct fwnode_handle *gic_domain_handle; > + struct acpi_madt_msix_oem_frame *m; > + int ret; > + > + m = container_of(header, struct acpi_madt_msix_oem_frame, header); > + if (BAD_MADT_ENTRY(m, end)) > + return -EINVAL; > + > + gic_domain_handle = acpi_get_gsi_domain_id(); > + if (!gic_domain_handle) { > + pr_err("Failed to find the GIC domain handle\n"); > + return -ENXIO; > + } > + > + gic_domain = irq_find_matching_fwnode(gic_domain_handle, > + DOMAIN_BUS_ANY); > + if (!gic_domain) { > + pr_err("Failed to find the GIC domain\n"); > + return -ENXIO; > + } Why do you do this in the iterator? It won't change over time, right? > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->spi_first = m->spi_base; > + priv->num_spis = m->spi_count; > + > + priv->msi_domain_handle = irq_domain_alloc_fwnode((void *) > + m->base_address); > + if (!priv->msi_domain_handle) { > + pr_err("Unable to allocate msi domain token\n"); > + ret = -EINVAL; > + goto err_acpi_priv; > + } > + > + ret = al_msix_init_common(priv, m->base_address); > + if (ret) > + goto err_acpi_priv; > + > + ret = al_msix_init_domains(priv, gic_domain); > + if (ret) > + goto err_acpi_map; > + > + pci_msi_register_fwnode_provider(&al_msi_get_fwnode); > + > + return 0; > + > +err_acpi_map: > + kfree(priv->msi_map); > +err_acpi_priv: > + kfree(priv); > + return ret; > +} > + > +static int __init al_msix_acpi_init(void) > +{ > + static struct acpi_table_madt *madt; > + acpi_status status; > + > + /* if ACPI MADT table is not Amazon defined return */ > + status = acpi_get_table(ACPI_SIG_MADT, 0, > + (struct acpi_table_header **)&madt); > + if (ACPI_FAILURE(status) || (madt && memcmp(madt->header.oem_id, > + ACPI_AMZN_OEM_ID, > + ACPI_OEM_ID_SIZE))) > + return -ENODEV; > + > + return acpi_table_parse_madt(ACPI_AMZN_MADT_OEM_TYPE, > + al_msix_acpi_probe, 0); > +} > +early_initcall(al_msix_acpi_init); These initcalls are not maintainable in the long run. Either this is part of the core ACPI discovery (IRQCHIP_ACPI_DECLARE), or it should use another probing method. If you need to express dependencies, make it an actual device driver (I suspect you're in complete control of the FW anyway), which would make a lot more sense. Thanks, M. -- Jazz is not dead, it just smell funny. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 7/7] irqchip/al-msi: Add ACPI support 2019-04-01 2:15 ` Marc Zyngier @ 2019-04-04 14:45 ` Zeev Zilberman 2019-04-12 12:08 ` Marc Zyngier 0 siblings, 1 reply; 8+ messages in thread From: Zeev Zilberman @ 2019-04-04 14:45 UTC (permalink / raw) To: Marc Zyngier, Hanna Hawa Cc: linux-arm-kernel, barakw, Lorenzo Pieralisi, linux-kernel, jason, antoine.tenart, catalin.marinas, vaerov, rjw, linux, will.deacon, hanochu, linux-acpi, alisaidi, jonnyc, ronenk, tglx, lenb, talel, dwmw, tsahee Hi Marc, We have considered exposing our interrupt controller both via MADT OEM-specific entry and via DSDT. We've chosen MADT OEM-specific entry, because it seemed like a reasonable placeholder for custom interrupt controller, but we can move to DSDT, if this seems like a better way to represent it. Either way we chose, we'll need to solve the ordering issue of the drivers probing. The desired order of driver probing in the system, because of the dependencies, is: GIC -> AL MSIX controller driver -> PCI If we keep using MADT, we can't just use IRQCHIP_DECLARE, because there is no way we found to control ordering of MADT probing. So, GIC might be probed after our driver in this case. The reason we used early_initcall, is that the early_initcalls are invoked after MADT probing in the system (and before DSDT probing). If we move to using DSDT we need to solve the ordering problem from another direction - make sure that MSIX driver is probed before PCI. In the patches that were posted for xgene interrupt controller (and weren't accepted) we saw that they proposed to solve the same issue by modifying ACPI subsystem code by defining a new type for msi drivers and probing them before PCI drivers (https://patchwork.ozlabs.org/patch/818771/). From the feedback on that patch (https://patchwork.ozlabs.org/cover/818767/#1788415) it seemed that alternative solution is in the works, but we didn't manage to find any followup on this. We would be glad to hear what you propose for fixing the ordering issue and rework the patches accordingly. Thanks, Ze'ev On 4/1/19 05:15, Marc Zyngier wrote: > On Sun, 31 Mar 2019 13:35:33 +0100, > Hanna Hawa <hhhawa@amazon.com> wrote: >> >> This patch adds ACPI support for AL-MSIx driver. >> >> The AL-MSIx controller is not standard, is not included in the UEFI >> specification, and will not be added. The driver ACPI binding is >> performed when the following conditions are true: >> - OEM ID is AMAZON >> - MADT table type is 0x80 (part of the OEM reserved range). >> >> Signed-off-by: Hanna Hawa <hhhawa@amazon.com> >> Co-developed-by: Vladimir Aerov <vaerov@amazon.com> >> Signed-off-by: Vladimir Aerov <vaerov@amazon.com> >> --- >> drivers/irqchip/irq-al-msi.c | 118 ++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 111 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/irqchip/irq-al-msi.c b/drivers/irqchip/irq-al-msi.c >> index ec27455..cb80c1e 100644 >> --- a/drivers/irqchip/irq-al-msi.c >> +++ b/drivers/irqchip/irq-al-msi.c >> @@ -9,6 +9,7 @@ >> >> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> >> +#include <linux/acpi.h> >> #include <linux/dma-iommu.h> >> #include <linux/irqchip.h> >> #include <linux/irqchip/arm-gic.h> >> @@ -126,14 +127,20 @@ static int al_msix_gic_domain_alloc(struct irq_domain *domain, >> struct irq_data *d; >> int ret; >> >> - if (!is_of_node(domain->parent->fwnode)) >> + if (is_of_node(domain->parent->fwnode)) { >> + fwspec.fwnode = domain->parent->fwnode; >> + fwspec.param_count = 3; >> + fwspec.param[0] = 0; >> + fwspec.param[1] = spi; >> + fwspec.param[2] = IRQ_TYPE_EDGE_RISING; >> + } else if (is_fwnode_irqchip(domain->parent->fwnode)) { >> + fwspec.fwnode = domain->parent->fwnode; >> + fwspec.param_count = 2; >> + fwspec.param[0] = spi + 32; >> + fwspec.param[1] = IRQ_TYPE_EDGE_RISING; >> + } else { >> return -EINVAL; >> - >> - fwspec.fwnode = domain->parent->fwnode; >> - fwspec.param_count = 3; >> - fwspec.param[0] = 0; >> - fwspec.param[1] = spi; >> - fwspec.param[2] = IRQ_TYPE_EDGE_RISING; >> + } >> >> ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec); >> if (ret) >> @@ -304,3 +311,100 @@ static int al_msix_init(struct device_node *node, struct device_node *parent) >> } >> IRQCHIP_DECLARE(alpine_msix, "al,alpine-msix", al_msix_init); >> IRQCHIP_DECLARE(al_msix, "amazon,al-msix", al_msix_init); >> + >> +#ifdef CONFIG_ACPI >> +static struct al_msix_data *priv; >> + >> +#define ACPI_AMZN_MADT_OEM_TYPE 0x80 >> +#define ACPI_AMZN_OEM_ID "AMAZON" >> + >> +struct acpi_madt_msix_oem_frame { >> + struct acpi_subtable_header header; >> + u64 base_address; >> + u32 base_address_len; >> + u16 spi_count; >> + u16 spi_base; >> +}; >> + >> +static struct fwnode_handle *al_msi_get_fwnode(struct device *dev) >> +{ >> + return priv->msi_domain_handle; >> +} >> + >> +static int __init al_msix_acpi_probe(struct acpi_subtable_header *header, >> + const unsigned long end) >> +{ >> + struct irq_domain *gic_domain; >> + struct fwnode_handle *gic_domain_handle; >> + struct acpi_madt_msix_oem_frame *m; >> + int ret; >> + >> + m = container_of(header, struct acpi_madt_msix_oem_frame, header); >> + if (BAD_MADT_ENTRY(m, end)) >> + return -EINVAL; >> + >> + gic_domain_handle = acpi_get_gsi_domain_id(); >> + if (!gic_domain_handle) { >> + pr_err("Failed to find the GIC domain handle\n"); >> + return -ENXIO; >> + } >> + >> + gic_domain = irq_find_matching_fwnode(gic_domain_handle, >> + DOMAIN_BUS_ANY); >> + if (!gic_domain) { >> + pr_err("Failed to find the GIC domain\n"); >> + return -ENXIO; >> + } > > Why do you do this in the iterator? It won't change over time, right? > >> + >> + priv = kzalloc(sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + priv->spi_first = m->spi_base; >> + priv->num_spis = m->spi_count; >> + >> + priv->msi_domain_handle = irq_domain_alloc_fwnode((void *) >> + m->base_address); >> + if (!priv->msi_domain_handle) { >> + pr_err("Unable to allocate msi domain token\n"); >> + ret = -EINVAL; >> + goto err_acpi_priv; >> + } >> + >> + ret = al_msix_init_common(priv, m->base_address); >> + if (ret) >> + goto err_acpi_priv; >> + >> + ret = al_msix_init_domains(priv, gic_domain); >> + if (ret) >> + goto err_acpi_map; >> + >> + pci_msi_register_fwnode_provider(&al_msi_get_fwnode); >> + >> + return 0; >> + >> +err_acpi_map: >> + kfree(priv->msi_map); >> +err_acpi_priv: >> + kfree(priv); >> + return ret; >> +} >> + >> +static int __init al_msix_acpi_init(void) >> +{ >> + static struct acpi_table_madt *madt; >> + acpi_status status; >> + >> + /* if ACPI MADT table is not Amazon defined return */ >> + status = acpi_get_table(ACPI_SIG_MADT, 0, >> + (struct acpi_table_header **)&madt); >> + if (ACPI_FAILURE(status) || (madt && memcmp(madt->header.oem_id, >> + ACPI_AMZN_OEM_ID, >> + ACPI_OEM_ID_SIZE))) >> + return -ENODEV; >> + >> + return acpi_table_parse_madt(ACPI_AMZN_MADT_OEM_TYPE, >> + al_msix_acpi_probe, 0); >> +} >> +early_initcall(al_msix_acpi_init); > > These initcalls are not maintainable in the long run. Either this is > part of the core ACPI discovery (IRQCHIP_ACPI_DECLARE), or it should > use another probing method. If you need to express dependencies, make > it an actual device driver (I suspect you're in complete control of > the FW anyway), which would make a lot more sense. > > Thanks, > > M. > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 7/7] irqchip/al-msi: Add ACPI support 2019-04-04 14:45 ` Zeev Zilberman @ 2019-04-12 12:08 ` Marc Zyngier 2019-04-12 16:45 ` Lorenzo Pieralisi 2019-04-26 9:49 ` Alexander Graf 0 siblings, 2 replies; 8+ messages in thread From: Marc Zyngier @ 2019-04-12 12:08 UTC (permalink / raw) To: Zeev Zilberman, Hanna Hawa Cc: linux-arm-kernel, barakw, Lorenzo Pieralisi, linux-kernel, jason, antoine.tenart, catalin.marinas, vaerov, rjw, linux, will.deacon, hanochu, linux-acpi, alisaidi, jonnyc, ronenk, tglx, lenb, talel, dwmw, tsahee Hi Zeev, On 04/04/2019 15:45, Zeev Zilberman wrote: > Hi Marc, > > We have considered exposing our interrupt controller both via MADT > OEM-specific entry and via DSDT. > We've chosen MADT OEM-specific entry, because it seemed like a > reasonable placeholder for custom interrupt controller, but we can move > to DSDT, if this seems like a better way to represent it. > > Either way we chose, we'll need to solve the ordering issue of the > drivers probing. > The desired order of driver probing in the system, because of the > dependencies, is: > GIC -> AL MSIX controller driver -> PCI > If we keep using MADT, we can't just use IRQCHIP_DECLARE, because there > is no way we found to control ordering of MADT probing. So, GIC might be > probed after our driver in this case. > The reason we used early_initcall, is that the early_initcalls are > invoked after MADT probing in the system (and before DSDT probing). > > If we move to using DSDT we need to solve the ordering problem from > another direction - make sure that MSIX driver is probed before PCI. > In the patches that were posted for xgene interrupt controller (and > weren't accepted) we saw that they proposed to solve the same issue > by modifying ACPI subsystem code by defining a new type for msi drivers > and probing them before PCI drivers > (https://patchwork.ozlabs.org/patch/818771/). > From the feedback on that patch > (https://patchwork.ozlabs.org/cover/818767/#1788415) it seemed that > alternative solution is in the works, > but we didn't manage to find any followup on this. > > We would be glad to hear what you propose for fixing the ordering issue > and rework the patches accordingly. There are multiple issues here, but the main one is that you're trying to do something that is completely contrary to the ACPI spec by inventing a new interrupt controller. The use case for ACPI is quite simple: you have HW that matches the ACPI spec, and everything will work out of the box. This means GICv2+GICv2m or GICv3+ITS. There is zero space for creativity. Now, if you want your own interrupt controller, the only choice is to stick with DT. That's the place for weird and wonderful stuff that ACPI cannot support. We've been around the block with XGene, and every "solution" was just utter crap, prone to failure and in the end unmaintainable. Anything involving an initcall definitely falls into that category. I'll let Lorenzo speak his mind as the arm64 ACPI maintainer, but from an irqchip perspective, I can't see how to support this unless we get the ACPI spec to support this type of configuration. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 7/7] irqchip/al-msi: Add ACPI support 2019-04-12 12:08 ` Marc Zyngier @ 2019-04-12 16:45 ` Lorenzo Pieralisi 2019-04-26 9:49 ` Alexander Graf 1 sibling, 0 replies; 8+ messages in thread From: Lorenzo Pieralisi @ 2019-04-12 16:45 UTC (permalink / raw) To: Marc Zyngier Cc: catalin.marinas, will.deacon, jonnyc, ronenk, vaerov, linux, talel, linux-acpi, alisaidi, lenb, jason, antoine.tenart, Zeev Zilberman, tglx, hanochu, linux-arm-kernel, barakw, Hanna Hawa, rjw, linux-kernel, tsahee, dwmw On Fri, Apr 12, 2019 at 01:08:00PM +0100, Marc Zyngier wrote: > Hi Zeev, > > On 04/04/2019 15:45, Zeev Zilberman wrote: > > Hi Marc, > > > > We have considered exposing our interrupt controller both via MADT > > OEM-specific entry and via DSDT. > > We've chosen MADT OEM-specific entry, because it seemed like a > > reasonable placeholder for custom interrupt controller, but we can move > > to DSDT, if this seems like a better way to represent it. > > > > Either way we chose, we'll need to solve the ordering issue of the > > drivers probing. > > The desired order of driver probing in the system, because of the > > dependencies, is: > > GIC -> AL MSIX controller driver -> PCI > > If we keep using MADT, we can't just use IRQCHIP_DECLARE, because there > > is no way we found to control ordering of MADT probing. So, GIC might be > > probed after our driver in this case. > > The reason we used early_initcall, is that the early_initcalls are > > invoked after MADT probing in the system (and before DSDT probing). > > > > If we move to using DSDT we need to solve the ordering problem from > > another direction - make sure that MSIX driver is probed before PCI. > > In the patches that were posted for xgene interrupt controller (and > > weren't accepted) we saw that they proposed to solve the same issue > > by modifying ACPI subsystem code by defining a new type for msi drivers > > and probing them before PCI drivers > > (https://patchwork.ozlabs.org/patch/818771/). > > From the feedback on that patch > > (https://patchwork.ozlabs.org/cover/818767/#1788415) it seemed that > > alternative solution is in the works, > > but we didn't manage to find any followup on this. > > > > We would be glad to hear what you propose for fixing the ordering issue > > and rework the patches accordingly. > > There are multiple issues here, but the main one is that you're trying > to do something that is completely contrary to the ACPI spec by > inventing a new interrupt controller. > > The use case for ACPI is quite simple: you have HW that matches the ACPI > spec, and everything will work out of the box. This means GICv2+GICv2m > or GICv3+ITS. There is zero space for creativity. Now, if you want your > own interrupt controller, the only choice is to stick with DT. That's > the place for weird and wonderful stuff that ACPI cannot support. > > We've been around the block with XGene, and every "solution" was just > utter crap, prone to failure and in the end unmaintainable. Anything > involving an initcall definitely falls into that category. > > I'll let Lorenzo speak his mind as the arm64 ACPI maintainer, but from > an irqchip perspective, I can't see how to support this unless we get > the ACPI spec to support this type of configuration. That's pretty much it, as a matter of fact there is not much we can do, actually it is a problem that {should have been/can be} solved first at ACPI spec level, every kludge we put together to fix eg Xgene ended up having implicit dependencies/requirements on firmware that are non-existing from an ACPI spec binding perspective (eg DSDT objects ordering). It is not a kernel problem, however we put it, we can't guess an IRQ controllers dependency that can't be described. Lorenzo _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 7/7] irqchip/al-msi: Add ACPI support 2019-04-12 12:08 ` Marc Zyngier 2019-04-12 16:45 ` Lorenzo Pieralisi @ 2019-04-26 9:49 ` Alexander Graf 1 sibling, 0 replies; 8+ messages in thread From: Alexander Graf @ 2019-04-26 9:49 UTC (permalink / raw) To: Marc Zyngier, Zeev Zilberman, Hanna Hawa Cc: linux-arm-kernel, barakw, Lorenzo Pieralisi, linux-kernel, jason, antoine.tenart, catalin.marinas, vaerov, rjw, linux, will.deacon, hanochu, linux-acpi, alisaidi, jonnyc, ronenk, tglx, lenb, talel, dwmw, tsahee On 12.04.19 14:08, Marc Zyngier wrote: > Hi Zeev, > > On 04/04/2019 15:45, Zeev Zilberman wrote: >> Hi Marc, >> >> We have considered exposing our interrupt controller both via MADT >> OEM-specific entry and via DSDT. >> We've chosen MADT OEM-specific entry, because it seemed like a >> reasonable placeholder for custom interrupt controller, but we can move >> to DSDT, if this seems like a better way to represent it. >> >> Either way we chose, we'll need to solve the ordering issue of the >> drivers probing. >> The desired order of driver probing in the system, because of the >> dependencies, is: >> GIC -> AL MSIX controller driver -> PCI >> If we keep using MADT, we can't just use IRQCHIP_DECLARE, because there >> is no way we found to control ordering of MADT probing. So, GIC might be >> probed after our driver in this case. >> The reason we used early_initcall, is that the early_initcalls are >> invoked after MADT probing in the system (and before DSDT probing). >> >> If we move to using DSDT we need to solve the ordering problem from >> another direction - make sure that MSIX driver is probed before PCI. >> In the patches that were posted for xgene interrupt controller (and >> weren't accepted) we saw that they proposed to solve the same issue >> by modifying ACPI subsystem code by defining a new type for msi drivers >> and probing them before PCI drivers >> (https://patchwork.ozlabs.org/patch/818771/). >> From the feedback on that patch >> (https://patchwork.ozlabs.org/cover/818767/#1788415) it seemed that >> alternative solution is in the works, >> but we didn't manage to find any followup on this. >> >> We would be glad to hear what you propose for fixing the ordering issue >> and rework the patches accordingly. > > There are multiple issues here, but the main one is that you're trying > to do something that is completely contrary to the ACPI spec by > inventing a new interrupt controller. > > The use case for ACPI is quite simple: you have HW that matches the ACPI > spec, and everything will work out of the box. This means GICv2+GICv2m > or GICv3+ITS. There is zero space for creativity. Now, if you want your > own interrupt controller, the only choice is to stick with DT. That's > the place for weird and wonderful stuff that ACPI cannot support. I've had a quick chat about this with Marc at Linaro Connect and there might be a 3rd viable option: Create a standard ACPI binding for GICv3+MBI (akin to GICv2m) and use that. Zeev, Hanna, what exactly is the reason you need to use your own MSI translation engine here? Can't you just reuse the GICv3 built-in one? After all, I would assume that your PCIe complex is able to send DMA requests to external devices? If you could, we could start working towards an ACPI definition for that instead and make everyone happy. Alex _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-04-26 9:56 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-03-31 12:35 [PATCH 5/7] ACPI / irq: Add GSI IRQ domain getter function Hanna Hawa 2019-03-31 12:35 ` [PATCH 6/7] irqchip/al-msi: Refactor in preparation to add ACPI support Hanna Hawa 2019-03-31 12:35 ` [PATCH 7/7] irqchip/al-msi: Add " Hanna Hawa 2019-04-01 2:15 ` Marc Zyngier 2019-04-04 14:45 ` Zeev Zilberman 2019-04-12 12:08 ` Marc Zyngier 2019-04-12 16:45 ` Lorenzo Pieralisi 2019-04-26 9:49 ` Alexander Graf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).