[PATCH 05/13 v3] irqchip/gic: assign irqchip dynamically

[PATCH 05/13 v3] irqchip/gic: assign irqchip dynamically

[Linus Walleij]

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 v2->v3:
- Handle the EOIMODE1 chips properly: select a proper &chip pointer
  at chip registration and store in the per-GIC .chip member.
  Saves hard-to-grasp code in the .map function too. We select the
  EOIMODE1 chip if and only if it's GIC0 and the machine suppors
  deactivation.
- Name ordinary GIC chips GIC0, ... GICN and the EOIMODE1 chip
  GICEOI, as there can be only one such chip.
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 | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 9ec8cf5137d9..9c61ca182433 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -58,6 +58,7 @@ union gic_base {
 };
 
 struct gic_chip_data {
+	struct irq_chip chip;
 	union gic_base dist_base;
 	union gic_base cpu_base;
 #ifdef CONFIG_CPU_PM
@@ -370,7 +371,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 +386,7 @@ static struct irq_chip gic_chip = {
 };
 
 static struct irq_chip gic_eoimode1_chip = {
-	.name			= "GICv2",
+	.name			= "GICEOI",
 	.irq_mask		= gic_eoimode1_mask_irq,
 	.irq_unmask		= gic_unmask_irq,
 	.irq_eoi		= gic_eoimode1_eoi_irq,
@@ -880,12 +880,8 @@ 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;
-
-	if (static_key_true(&supports_deactivate)) {
-		if (d->host_data == (void *)&gic_data[0])
-			chip = &gic_eoimode1_chip;
-	}
+	struct gic_chip_data *gic = d->host_data;
+	struct irq_chip *chip = &gic->chip;
 
 	if (hw < 32) {
 		irq_set_percpu_devid(irq);
@@ -989,6 +985,9 @@ 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];
+	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... */
 		unsigned int cpu;
@@ -1079,8 +1078,10 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 		register_cpu_notifier(&gic_cpu_notifier);
 #endif
 		set_handle_irq(gic_handle_irq);
-		if (static_key_true(&supports_deactivate))
+		if (static_key_true(&supports_deactivate)) {
 			pr_info("GIC: Using split EOI/Deactivate mode\n");
+			gic->chip = gic_eoimode1_chip;
+		}
 	}
 
 	gic_dist_init(gic);
-- 
2.4.3

Re: [PATCH 05/13 v3] irqchip/gic: assign irqchip dynamically

[Marc Zyngier]

On 02/11/15 14:34, 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 v2->v3:
> - Handle the EOIMODE1 chips properly: select a proper &chip pointer
>   at chip registration and store in the per-GIC .chip member.
>   Saves hard-to-grasp code in the .map function too. We select the
>   EOIMODE1 chip if and only if it's GIC0 and the machine suppors
>   deactivation.
> - Name ordinary GIC chips GIC0, ... GICN and the EOIMODE1 chip
>   GICEOI, as there can be only one such chip.
> 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 | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 9ec8cf5137d9..9c61ca182433 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -58,6 +58,7 @@ union gic_base {
>  };
>  
>  struct gic_chip_data {
> +	struct irq_chip chip;
>  	union gic_base dist_base;
>  	union gic_base cpu_base;
>  #ifdef CONFIG_CPU_PM
> @@ -370,7 +371,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 +386,7 @@ static struct irq_chip gic_chip = {
>  };
>  
>  static struct irq_chip gic_eoimode1_chip = {
> -	.name			= "GICv2",
> +	.name			= "GICEOI",

Hmmm. I'd rather leave this to GICv2, because EOImode==1 is a feature of
the architecture, rather than a piece of HW. Only nitpicking, really.

>  	.irq_mask		= gic_eoimode1_mask_irq,
>  	.irq_unmask		= gic_unmask_irq,
>  	.irq_eoi		= gic_eoimode1_eoi_irq,
> @@ -880,12 +880,8 @@ 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;
> -
> -	if (static_key_true(&supports_deactivate)) {
> -		if (d->host_data == (void *)&gic_data[0])
> -			chip = &gic_eoimode1_chip;
> -	}
> +	struct gic_chip_data *gic = d->host_data;
> +	struct irq_chip *chip = &gic->chip;
>  
>  	if (hw < 32) {
>  		irq_set_percpu_devid(irq);
> @@ -989,6 +985,9 @@ 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];
> +	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... */
>  		unsigned int cpu;
> @@ -1079,8 +1078,10 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
>  		register_cpu_notifier(&gic_cpu_notifier);
>  #endif
>  		set_handle_irq(gic_handle_irq);
> -		if (static_key_true(&supports_deactivate))
> +		if (static_key_true(&supports_deactivate)) {
>  			pr_info("GIC: Using split EOI/Deactivate mode\n");
> +			gic->chip = gic_eoimode1_chip;
> +		}
>  	}
>  
>  	gic_dist_init(gic);
> 

Other than the above:

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...