Messages in this thread Patch in this message | | | Date | Wed, 09 Dec 2015 14:15:10 +0000 | From | Marc Zyngier <> | Subject | Re: [PATCH 3/3] irqchip/gic: assign irqchip dynamically |
| |
On 23/10/15 23:15, Linus Walleij wrote: > Instead of having the irqchip being a static struct, make it part > of the per-instance data so we can assign it a dynamic name. This > has the usable side effect of displaying the GIC with an instance > number as GIC0, GIC1 ... GICn in /proc/interrupts, which is helpful > when debugging cascaded GICs, such as on the ARM PB11MPCore. > > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Jason Cooper <jason@lakedaemon.net> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v1->v2: > - Keep the static structs around, just delete the .name > field assign them to the chips at registration time, updating > the name field with the instance number. > - Also enumerate the EOIMODE1 sub-chips. > - Broke out this irqchip stuff from the rest of the series so as > not to stress the irqchip maintainers. It has no dependencies > on the other patches anyways, and can be merged stand-alone. > > Marc: can't test the EOIMODE1 thing, it's far above me, but it > "should work". Is it correct that there is one unique and coupled > EOIMODE1 instance per GIC instance like this? > --- > drivers/irqchip/irq-gic.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index 9ec8cf5137d9..fe87ac32fdbe 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -58,6 +58,8 @@ union gic_base { > }; > > struct gic_chip_data { > + struct irq_chip chip; > + struct irq_chip eoimode1_chip;
This seems overkill: A given GIC instance is either driven as a GIC or a GICv2. Never both at the same time.
> union gic_base dist_base; > union gic_base cpu_base; > #ifdef CONFIG_CPU_PM > @@ -370,7 +372,6 @@ static void gic_handle_cascade_irq(struct irq_desc *desc) > } > > static struct irq_chip gic_chip = { > - .name = "GIC", > .irq_mask = gic_mask_irq, > .irq_unmask = gic_unmask_irq, > .irq_eoi = gic_eoi_irq, > @@ -386,7 +387,6 @@ static struct irq_chip gic_chip = { > }; > > static struct irq_chip gic_eoimode1_chip = { > - .name = "GICv2", > .irq_mask = gic_eoimode1_mask_irq, > .irq_unmask = gic_unmask_irq, > .irq_eoi = gic_eoimode1_eoi_irq, > @@ -880,11 +880,12 @@ void __init gic_init_physaddr(struct device_node *node) > static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq, > irq_hw_number_t hw) > { > - struct irq_chip *chip = &gic_chip; > + struct gic_chip_data *gic = d->host_data; > + struct irq_chip *chip = &gic->chip; > > if (static_key_true(&supports_deactivate)) { > if (d->host_data == (void *)&gic_data[0]) > - chip = &gic_eoimode1_chip; > + chip = &gic->eoimode1_chip; > } > > if (hw < 32) { > @@ -989,6 +990,13 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start, > BUG_ON(gic_nr >= MAX_GIC_NR); > > gic = &gic_data[gic_nr]; > + > + /* Initialize irq_chip */ > + gic->chip = gic_chip; > + gic->eoimode1_chip = gic_eoimode1_chip; > + gic->chip.name = kasprintf(GFP_KERNEL, "GIC%d", gic_nr); > + gic->eoimode1_chip.name = kasprintf(GFP_KERNEL, "GICv2%d", gic_nr);
And there is only ever one GIC in the system that can be driven as a GICv2 - it can only be instance 0.
> + > #ifdef CONFIG_GIC_NON_BANKED > if (percpu_offset) { /* Frankein-GIC without banked registers... */ > unsigned int cpu; >
So instead of going for another round, how about this on top of your patch:
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 428f9c1..174990c 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -70,7 +70,6 @@ union gic_base { struct gic_chip_data { struct irq_chip chip; - struct irq_chip eoimode1_chip; union gic_base dist_base; union gic_base cpu_base; #ifdef CONFIG_CPU_PM @@ -400,6 +399,7 @@ static struct irq_chip gic_chip = { }; static struct irq_chip gic_eoimode1_chip = { + .name = "GICv2", .irq_mask = gic_eoimode1_mask_irq, .irq_unmask = gic_unmask_irq, .irq_eoi = gic_eoimode1_eoi_irq, @@ -926,20 +926,14 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw) { struct gic_chip_data *gic = d->host_data; - struct irq_chip *chip = &gic->chip; - - if (static_key_true(&supports_deactivate)) { - if (d->host_data == (void *)&gic_data[0]) - chip = &gic->eoimode1_chip; - } if (hw < 32) { irq_set_percpu_devid(irq); - irq_domain_set_info(d, irq, hw, chip, d->host_data, + irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data, handle_percpu_devid_irq, NULL, NULL); irq_set_status_flags(irq, IRQ_NOAUTOEN); } else { - irq_domain_set_info(d, irq, hw, chip, d->host_data, + irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data, handle_fasteoi_irq, NULL, NULL); irq_set_probe(irq); } @@ -1048,10 +1042,12 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start, gic = &gic_data[gic_nr]; /* Initialize irq_chip */ - gic->chip = gic_chip; - gic->eoimode1_chip = gic_eoimode1_chip; - gic->chip.name = kasprintf(GFP_KERNEL, "GIC%d", gic_nr); - gic->eoimode1_chip.name = kasprintf(GFP_KERNEL, "GICv2%d", gic_nr); + if (static_key_true(&supports_deactivate) && gic_nr == 0) { + gic->chip = gic_eoimode1_chip; + } else { + gic->chip = gic_chip; + gic->chip.name = kasprintf(GFP_KERNEL, "GIC-%d", gic_nr); + } #ifdef CONFIG_GIC_NON_BANKED if (percpu_offset) { /* Frankein-GIC without banked registers... */ If you're OK with this and the previous patch, I'll squash those into the original ones, and put it on a branch for Thomas to pick up for 4.5.
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |