Re: [PATCH 3/3] irqchip/gic: assign irqchip dynamically

[Marc Zyngier <marc.zyngier@arm.com>]

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...