Messages in this thread | | | Date | Tue, 12 Mar 2019 21:59:09 +0800 (CST) | From | "Liu Xiang" <> | Subject | Re:Re: [PATCH] irqchip/gic: fix passing wrong start irq number to irq_alloc_descs() for secondary GICs |
| |
Hi, Marc
Thanks for your reply!
At 2019-03-11 23:55:11, "Marc Zyngier" <marc.zyngier@arm.com> wrote: >On 11/03/2019 14:52, Liu Xiang wrote: >> For secondary GICs, the start irq number should skip over SGIs >> and PPIs. Its value should be 32. So we should pass hwirq_base to >> irq_alloc_descs() rather than a constant number 16. >> >> Signed-off-by: Liu Xiang <liu.xiang6@zte.com.cn> >> --- >> drivers/irqchip/irq-gic.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >> index ba2a37a..351f576 100644 >> --- a/drivers/irqchip/irq-gic.c >> +++ b/drivers/irqchip/irq-gic.c >> @@ -1157,7 +1157,7 @@ static int gic_init_bases(struct gic_chip_data *gic, int irq_start, >> >> gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */ >> >> - irq_base = irq_alloc_descs(irq_start, 16, gic_irqs, >> + irq_base = irq_alloc_descs(irq_start, hwirq_base, gic_irqs, >> numa_node_id()); >> if (irq_base < 0) { >> WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n", >> > >I suggest you look at __irq_alloc_descs(), and understand what the >various parameters mean. What you're doing here has absolutely no >effect. > >The right thing to do would be to get rid of this altogether, except >that we have exactly *one* platform in the tree that is still non-DT >(some unmaintained Cavium piece of junk). But we can still simplify it, >as this guy doesn't have a secondary GIC (it is braindead enough). > >What I'm suggesting instead is: > >From b41fdc4a7bf9045e4871c5b15905ea732ffd044f Mon Sep 17 00:00:00 2001 >From: Marc Zyngier <marc.zyngier@arm.com> >Date: Mon, 11 Mar 2019 15:38:10 +0000 >Subject: [PATCH] irqchip/gic: Drop support for secondary GIC in non-DT systems > >We do not have any in-tree platform with this pathological setup, >and only a single system (Cavium's cns3xxx) isn't DT aware. > >Let's drop the secondary GIC support for now, until we remove >the above horror altogether. > >Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >--- > arch/arm/mach-cns3xxx/core.c | 2 +- > drivers/irqchip/irq-gic.c | 45 ++++++++++++--------------------- > include/linux/irqchip/arm-gic.h | 3 +-- > 3 files changed, 18 insertions(+), 32 deletions(-) > >diff --git a/arch/arm/mach-cns3xxx/core.c b/arch/arm/mach-cns3xxx/core.c >index 7d5a44a06648..f676592d8402 100644 >--- a/arch/arm/mach-cns3xxx/core.c >+++ b/arch/arm/mach-cns3xxx/core.c >@@ -90,7 +90,7 @@ void __init cns3xxx_map_io(void) > /* used by entry-macro.S */ > void __init cns3xxx_init_irq(void) > { >- gic_init(0, 29, IOMEM(CNS3XXX_TC11MP_GIC_DIST_BASE_VIRT), >+ gic_init(IOMEM(CNS3XXX_TC11MP_GIC_DIST_BASE_VIRT), > IOMEM(CNS3XXX_TC11MP_GIC_CPU_BASE_VIRT)); > } > >diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >index ba2a37a27a54..fd3110c171ba 100644 >--- a/drivers/irqchip/irq-gic.c >+++ b/drivers/irqchip/irq-gic.c >@@ -1089,11 +1089,10 @@ static void gic_init_chip(struct gic_chip_data *gic, struct device *dev, > #endif > } > >-static int gic_init_bases(struct gic_chip_data *gic, int irq_start, >+static int gic_init_bases(struct gic_chip_data *gic, > struct fwnode_handle *handle) > { >- irq_hw_number_t hwirq_base; >- int gic_irqs, irq_base, ret; >+ int gic_irqs, ret; > > if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && gic->percpu_offset) { > /* Frankein-GIC without banked registers... */ >@@ -1145,28 +1144,21 @@ static int gic_init_bases(struct gic_chip_data *gic, int irq_start, > } else { /* Legacy support */ > /* > * For primary GICs, skip over SGIs. >- * For secondary GICs, skip over PPIs, too. >+ * No secondary GIC support whatsoever. > */ >- if (gic == &gic_data[0] && (irq_start & 31) > 0) { >- hwirq_base = 16; >- if (irq_start != -1) >- irq_start = (irq_start & ~31) + 16; >- } else { >- hwirq_base = 32; >- } >+ int irq_base; > >- gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */ >+ gic_irqs -= 16; /* calculate # of irqs to allocate */ > >- irq_base = irq_alloc_descs(irq_start, 16, gic_irqs, >+ irq_base = irq_alloc_descs(16, 16, gic_irqs, > numa_node_id()); > if (irq_base < 0) { >- WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n", >- irq_start); >- irq_base = irq_start; >+ WARN(1, "Cannot allocate irq_descs @ IRQ16, assuming pre-allocated\n"); >+ irq_base = 16; > } > > gic->domain = irq_domain_add_legacy(NULL, gic_irqs, irq_base, >- hwirq_base, &gic_irq_domain_ops, gic); >+ 16, &gic_irq_domain_ops, gic); > } > > if (WARN_ON(!gic->domain)) { >@@ -1195,7 +1187,6 @@ static int gic_init_bases(struct gic_chip_data *gic, int irq_start, > } > > static int __init __gic_init_bases(struct gic_chip_data *gic, >- int irq_start, > struct fwnode_handle *handle) > { > char *name; >@@ -1231,32 +1222,28 @@ static int __init __gic_init_bases(struct gic_chip_data *gic, > gic_init_chip(gic, NULL, name, false); > } > >- ret = gic_init_bases(gic, irq_start, handle); >+ ret = gic_init_bases(gic, handle); > if (ret) > kfree(name); > > return ret; > } > >-void __init gic_init(unsigned int gic_nr, int irq_start, >- void __iomem *dist_base, void __iomem *cpu_base) >+void __init gic_init(void __iomem *dist_base, void __iomem *cpu_base) > { > struct gic_chip_data *gic; > >- if (WARN_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR)) >- return; >- > /* > * Non-DT/ACPI systems won't run a hypervisor, so let's not > * bother with these... > */ > static_branch_disable(&supports_deactivate_key); > >- gic = &gic_data[gic_nr]; >+ gic = &gic_data[0]; > gic->raw_dist_base = dist_base; > gic->raw_cpu_base = cpu_base; > >- __gic_init_bases(gic, irq_start, NULL); >+ __gic_init_bases(gic, NULL); > } > > static void gic_teardown(struct gic_chip_data *gic) >@@ -1399,7 +1386,7 @@ int gic_of_init_child(struct device *dev, struct gic_chip_data **gic, int irq) > if (ret) > return ret; > >- ret = gic_init_bases(*gic, -1, &dev->of_node->fwnode); >+ ret = gic_init_bases(*gic, &dev->of_node->fwnode); > if (ret) { > gic_teardown(*gic); > return ret; >@@ -1459,7 +1446,7 @@ gic_of_init(struct device_node *node, struct device_node *parent) > if (gic_cnt == 0 && !gic_check_eoimode(node, &gic->raw_cpu_base)) > static_branch_disable(&supports_deactivate_key); > >- ret = __gic_init_bases(gic, -1, &node->fwnode); >+ ret = __gic_init_bases(gic, &node->fwnode); > if (ret) { > gic_teardown(gic); > return ret; >@@ -1650,7 +1637,7 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header, > return -ENOMEM; > } > >- ret = __gic_init_bases(gic, -1, domain_handle); >+ ret = __gic_init_bases(gic, domain_handle); > if (ret) { > pr_err("Failed to initialise GIC\n"); > irq_domain_free_fwnode(domain_handle); >diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h >index 626179077bb0..0f049b384ccd 100644 >--- a/include/linux/irqchip/arm-gic.h >+++ b/include/linux/irqchip/arm-gic.h >@@ -158,8 +158,7 @@ int gic_of_init_child(struct device *dev, struct gic_chip_data **gic, int irq); > * Legacy platforms not converted to DT yet must use this to init > * their GIC > */ >-void gic_init(unsigned int nr, int start, >- void __iomem *dist , void __iomem *cpu); >+void gic_init(void __iomem *dist , void __iomem *cpu); > > int gicv2m_init(struct fwnode_handle *parent_handle, > struct irq_domain *parent); >-- >2.20.1 > >Thanks, > > M. >-- >Jazz is not dead. It just smells funny...
| |