Use chained handler entry/exit functions in platform code

This patch updates the Tegra gpio chained IRQ handler to use the chained IRQ enter/exit functions in order to function correctly on primary controllers with different methods of flow control.

This is required for the GIC to move to fasteoi interrupt handling.

Cc Colin Cross <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
?arch/arm/mach-tegra/gpio.c | ? ?7 ++++---
?1 files changed, 4 insertions(+), 3 deletions(-)

arch/arm/mach-tegra/gpio.c +0 -0

index 76a3f65..08c43dd 100644

arch/arm/mach-tegra/gpio.c +0 -0

 ? ? ? ?bank = irq_get_handler_data(irq);
 

@@ -241,7 +242,7 @@ static void tegra_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)

? ? ? ? ? ? ? ? ? ? ? ? */
? ? ? ? ? ? ? ? ? ? ? ?if (lvl & (0x100 << pin)) {
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unmasked = 1;
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? desc->irq_data.chip->irq_unmask(&desc->irq_data);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? chained_irq_exit(chip, desc);
? ? ? ? ? ? ? ? ? ? ? ?}

? ? ? ? ? ? ? ? ? ? ? ?generic_handle_irq(gpio_to_irq(gpio + pin));

arch/arm/mach-tegra/gpio.c +1 -2

@@ -249,7 +250,7 @@ static void tegra_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 ? ? ? ?}
 
 ? ? ? ?if (!unmasked)
- ? ? ? ? ? ? ? desc->irq_data.chip->irq_unmask(&desc->irq_data);
+ ? ? ? ? ? ? ? chained_irq_exit(chip, desc);
 
 ?}
 
--

1.7.0.4

arch/arm/mach-tegra/gpio.c static void tegra_gpio_irq_handler(unsigned int irq, struct irq_desc *desc) · line 224

Quoted 9 lines

arch/arm/mach-tegra/gpio.c +1 -0

index 76a3f65..08c43dd 100644
@@ -221,8 +221,9 @@ static void tegra_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
       int port;
       int pin;
       int unmasked = 0;
+       struct irq_chip chip = get_irq_desc_chip(desc);

Should be a pointer, and 35e857c changed get_irq_desc_chip to irq_desc_get_chip

arch/arm/mach-tegra/gpio.c static void tegra_gpio_irq_handler(unsigned int irq, struct irq_desc *desc) · line 226

Quoted 2 lines

arch/arm/mach-tegra/gpio.c +1 -1

-       desc->irq_data.chip->irq_ack(&desc->irq_data);
+       chained_irq_enter(chip, desc);

asm/mach/irq.h needs to be included to use this function

Currently, the gic uses handle_level_irq for handling SPIs (Shared Peripheral Interrupts), requiring active interrupts to be masked at the distributor level during IRQ handling.

On a virtualised system, only the CPU interfaces are virtualised in hardware. Accesses to the distributor must be trapped by the hypervisor, adding latency to the critical interrupt path in Linux.

This patch modifies the GIC code to use handle_fasteoi_irq for handling interrupts, which only requires us to signal EOI to the CPU interface when handling is complete. Cascaded IRQ handling is also updated to use the chained IRQ enter/exit functions to honour the flow control of the parent chip.

Note that commit 846afbd1 ("GIC: Dont disable INT in ack callback") broke cascading interrupts by forgetting to add IRQ masking. This is no longer an issue because the unmask call is now unnecessary.

Tested on Versatile Express and Realview EB (1176 w/ cascaded GICs).

Cc Abhijeet Dharmapurikar <[email protected]>

Cc Russell King - ARM Linux <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
?arch/arm/common/gic.c | ? 24 +++++++++--------------- ?1 files changed, 9 insertions(+), 15 deletions(-)

arch/arm/common/gic.c +0 -0

index f70ec7d..f8719ec 100644
@@ -84,15 +84,6 @@ static inline unsigned int gic_irq(struct irq_data *d)

?/*
?* Routines to acknowledge, disable and enable interrupts ?*/

arch/arm/common/gic.c +0 -9

-static void gic_ack_irq(struct irq_data *d)
-{
- ? ? ? spin_lock(&irq_controller_lock);
- ? ? ? if (gic_arch_extn.irq_ack)
- ? ? ? ? ? ? ? gic_arch_extn.irq_ack(d);
- ? ? ? writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
- ? ? ? spin_unlock(&irq_controller_lock);
-}
-

?static void gic_mask_irq(struct irq_data *d)
?{
? ? ? ?u32 mask = 1 << (d->irq % 32);

arch/arm/common/gic.c +6 -2

@@ -115,6 +106,11 @@ static void gic_unmask_irq(struct irq_data *d)
 ? ? ? ?spin_unlock(&irq_controller_lock);
 ?}
 
+static void gic_eoi_irq(struct irq_data *d)
+{
+ ? ? ? writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
+}
+
 ?static int gic_set_type(struct irq_data *d, unsigned int type)
 ?{
 ? ? ? ?void __iomem *base = gic_dist_base(d);

@@ -218,8 +214,7 @@ static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
 ? ? ? ?unsigned int cascade_irq, gic_irq;
 ? ? ? ?unsigned long status;
 
- ? ? ? /* primary controller ack'ing */
- ? ? ? chip->irq_ack(&desc->irq_data);
+ ? ? ? chained_irq_enter(chip, desc);
 
 ? ? ? ?spin_lock(&irq_controller_lock);
 ? ? ? ?status = readl(chip_data->cpu_base + GIC_CPU_INTACK);

@@ -236,15 +231,14 @@ static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
 ? ? ? ? ? ? ? ?generic_handle_irq(cascade_irq);
 

?out:
- ? ? ? /* primary controller unmasking */
- ? ? ? chip->irq_unmask(&desc->irq_data);
+ ? ? ? chained_irq_exit(chip, desc);
?}

?static struct irq_chip gic_chip = {
? ? ? ?.name ? ? ? ? ? ? ? ? ? = "GIC",
- ? ? ? .irq_ack ? ? ? ? ? ? ? ?= gic_ack_irq,
? ? ? ?.irq_mask ? ? ? ? ? ? ? = gic_mask_irq,
? ? ? ?.irq_unmask ? ? ? ? ? ? = gic_unmask_irq,
+ ? ? ? .irq_eoi ? ? ? ? ? ? ? ?= gic_eoi_irq,
? ? ? ?.irq_set_type ? ? ? ? ? = gic_set_type,
? ? ? ?.irq_retrigger ? ? ? ? ?= gic_retrigger,
?#ifdef CONFIG_SMP

arch/arm/common/gic.c +0 -0

@@ -319,7 +313,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic,

? ? ? ? * Setup the Linux IRQ subsystem.
? ? ? ? */

? ? ? ?for (i = irq_start; i < irq_limit; i++) {
- ? ? ? ? ? ? ? irq_set_chip_and_handler(i, &gic_chip, handle_level_irq);
+ ? ? ? ? ? ? ? irq_set_chip_and_handler(i, &gic_chip, handle_fasteoi_irq); ? ? ? ? ? ? ? ?irq_set_chip_data(i, gic);
? ? ? ? ? ? ? ?set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
? ? ? ?}
--
1.7.0.4

Quoted 91 lines · Will Deacon

Currently, the gic uses handle_level_irq for handling SPIs (Shared Peripheral Interrupts), requiring active interrupts to be masked at the distributor level during IRQ handling.

On a virtualised system, only the CPU interfaces are virtualised in hardware. Accesses to the distributor must be trapped by the hypervisor, adding latency to the critical interrupt path in Linux.

This patch modifies the GIC code to use handle_fasteoi_irq for handling interrupts, which only requires us to signal EOI to the CPU interface when handling is complete. Cascaded IRQ handling is also updated to use the chained IRQ enter/exit functions to honour the flow control of the parent chip.

Note that commit 846afbd1 ("GIC: Dont disable INT in ack callback") broke cascading interrupts by forgetting to add IRQ masking. This is no longer an issue because the unmask call is now unnecessary.

Tested on Versatile Express and Realview EB (1176 w/ cascaded GICs).

Cc Abhijeet Dharmapurikar <[email protected]>

Cc Russell King - ARM Linux <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
?arch/arm/common/gic.c | ? 24 +++++++++--------------- ?1 files changed, 9 insertions(+), 15 deletions(-)

arch/arm/common/gic.c +0 -0

index f70ec7d..f8719ec 100644
@@ -84,15 +84,6 @@ static inline unsigned int gic_irq(struct irq_data *d)

?/*
?* Routines to acknowledge, disable and enable interrupts ?*/

arch/arm/common/gic.c +0 -9

-static void gic_ack_irq(struct irq_data *d)
-{
- ? ? ? spin_lock(&irq_controller_lock);
- ? ? ? if (gic_arch_extn.irq_ack)
- ? ? ? ? ? ? ? gic_arch_extn.irq_ack(d);
- ? ? ? writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
- ? ? ? spin_unlock(&irq_controller_lock);
-}
-

?static void gic_mask_irq(struct irq_data *d)
?{
? ? ? ?u32 mask = 1 << (d->irq % 32);

arch/arm/common/gic.c +6 -2

@@ -115,6 +106,11 @@ static void gic_unmask_irq(struct irq_data *d)
 ? ? ? ?spin_unlock(&irq_controller_lock);
 ?}
 
+static void gic_eoi_irq(struct irq_data *d)
+{
+ ? ? ? writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
+}
+
 ?static int gic_set_type(struct irq_data *d, unsigned int type)
 ?{
 ? ? ? ?void __iomem *base = gic_dist_base(d);

@@ -218,8 +214,7 @@ static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
 ? ? ? ?unsigned int cascade_irq, gic_irq;
 ? ? ? ?unsigned long status;
 
- ? ? ? /* primary controller ack'ing */
- ? ? ? chip->irq_ack(&desc->irq_data);
+ ? ? ? chained_irq_enter(chip, desc);
 
 ? ? ? ?spin_lock(&irq_controller_lock);
 ? ? ? ?status = readl(chip_data->cpu_base + GIC_CPU_INTACK);

@@ -236,15 +231,14 @@ static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
 ? ? ? ? ? ? ? ?generic_handle_irq(cascade_irq);
 

?out:
- ? ? ? /* primary controller unmasking */
- ? ? ? chip->irq_unmask(&desc->irq_data);
+ ? ? ? chained_irq_exit(chip, desc);
?}

?static struct irq_chip gic_chip = {
? ? ? ?.name ? ? ? ? ? ? ? ? ? = "GIC",
- ? ? ? .irq_ack ? ? ? ? ? ? ? ?= gic_ack_irq,
? ? ? ?.irq_mask ? ? ? ? ? ? ? = gic_mask_irq,
? ? ? ?.irq_unmask ? ? ? ? ? ? = gic_unmask_irq,
+ ? ? ? .irq_eoi ? ? ? ? ? ? ? ?= gic_eoi_irq,
? ? ? ?.irq_set_type ? ? ? ? ? = gic_set_type,
? ? ? ?.irq_retrigger ? ? ? ? ?= gic_retrigger,
?#ifdef CONFIG_SMP

arch/arm/common/gic.c +0 -0

@@ -319,7 +313,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic,

? ? ? ? * Setup the Linux IRQ subsystem.
? ? ? ? */

Quoted 10 lines

? ? ? ?for (i = irq_start; i < irq_limit; i++) {
- ? ? ? ? ? ? ? irq_set_chip_and_handler(i, &gic_chip, handle_level_irq);
+ ? ? ? ? ? ? ? irq_set_chip_and_handler(i, &gic_chip, handle_fasteoi_irq); ? ? ? ? ? ? ? ?irq_set_chip_data(i, gic);
? ? ? ? ? ? ? ?set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
? ? ? ?}
--
1.7.0.4

Tested-by Colin Cross <[email protected]>

Works on tegra after fixing the problems with the tegra gpio patch.

Quoted 104 lines · Colin Cross

Quoted 98 lines · Will Deacon

Currently, the gic uses handle_level_irq for handling SPIs (Shared Peripheral Interrupts), requiring active interrupts to be masked at the distributor level during IRQ handling.

On a virtualised system, only the CPU interfaces are virtualised in hardware. Accesses to the distributor must be trapped by the hypervisor, adding latency to the critical interrupt path in Linux.

This patch modifies the GIC code to use handle_fasteoi_irq for handling interrupts, which only requires us to signal EOI to the CPU interface when handling is complete. Cascaded IRQ handling is also updated to use the chained IRQ enter/exit functions to honour the flow control of the parent chip.

Note that commit 846afbd1 ("GIC: Dont disable INT in ack callback") broke cascading interrupts by forgetting to add IRQ masking. This is no longer an issue because the unmask call is now unnecessary.

Tested on Versatile Express and Realview EB (1176 w/ cascaded GICs).

Cc Abhijeet Dharmapurikar<[email protected]>

Cc Russell King - ARM Linux<[email protected]>

Acked-by Catalin Marinas<[email protected]>

Signed-off-by Will Deacon<[email protected]>

---
  arch/arm/common/gic.c |   24 +++++++++---------------
  1 files changed, 9 insertions(+), 15 deletions(-)

arch/arm/common/gic.c +9 -16

index f70ec7d..f8719ec 100644
@@ -84,15 +84,6 @@ static inline unsigned int gic_irq(struct irq_data *d)
  /*
  * Routines to acknowledge, disable and enable interrupts
  */
-static void gic_ack_irq(struct irq_data *d)
-{
-       spin_lock(&irq_controller_lock);
-       if (gic_arch_extn.irq_ack)
-               gic_arch_extn.irq_ack(d);
-       writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
-       spin_unlock(&irq_controller_lock);
-}
-
  static void gic_mask_irq(struct irq_data *d)
  {
        u32 mask = 1<<  (d->irq % 32);

@@ -115,6 +106,11 @@ static void gic_unmask_irq(struct irq_data *d)
        spin_unlock(&irq_controller_lock);
  }
 
+static void gic_eoi_irq(struct irq_data *d)
+{
+       writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
+}
+
  static int gic_set_type(struct irq_data *d, unsigned int type)
  {
        void __iomem *base = gic_dist_base(d);

@@ -218,8 +214,7 @@ static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
        unsigned int cascade_irq, gic_irq;
        unsigned long status;
 
-       /* primary controller ack'ing */
-       chip->irq_ack(&desc->irq_data);
+       chained_irq_enter(chip, desc);
 
        spin_lock(&irq_controller_lock);
        status = readl(chip_data->cpu_base + GIC_CPU_INTACK);

@@ -236,15 +231,14 @@ static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
                generic_handle_irq(cascade_irq);
 
  out:
-       /* primary controller unmasking */
-       chip->irq_unmask(&desc->irq_data);
+       chained_irq_exit(chip, desc);
  }
 
  static struct irq_chip gic_chip = {
        .name                   = "GIC",
-       .irq_ack                = gic_ack_irq,
        .irq_mask               = gic_mask_irq,
        .irq_unmask             = gic_unmask_irq,
+       .irq_eoi                = gic_eoi_irq,
        .irq_set_type           = gic_set_type,
        .irq_retrigger          = gic_retrigger,
  #ifdef CONFIG_SMP

@@ -319,7 +313,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
         * Setup the Linux IRQ subsystem.
         */
        for (i = irq_start; i<  irq_limit; i++) {
-               irq_set_chip_and_handler(i,&gic_chip, handle_level_irq);
+               irq_set_chip_and_handler(i,&gic_chip, handle_fasteoi_irq);
                irq_set_chip_data(i, gic);
                set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
        }
--

Quoted 3 lines

1.7.0.4

Tested-by Colin Cross<[email protected]>

Quoted 2 lines

Works on tegra after fixing the problems with the tegra gpio patch.

In further testing I found one bug. d7ed36a added gic_arch_extn,
which needs to be used in gic_eoi. arch/arm/mach-tegra/irq.c will
need to be fixed to replace tegra_ack with tegra_eoi, and any other platform that uses gic_arch_extn also needs to be checked (omap4?). This patch fixes gic.c:

arch/arm/common/gic.c +4 -0

index 6ecd5c7..8e46dac 100644
@@ -118,7 +118,11 @@ static void gic_unmask_irq(struct irq_data *d)
 
  static void gic_eoi_irq(struct irq_data *d)
  {
+       spin_lock(&irq_controller_lock);
+       if (gic_arch_extn.irq_eoi)
+               gic_arch_extn.irq_eoi(d);
         writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
+       spin_unlock(&irq_controller_lock);
  }
 
  static int gic_set_type(struct irq_data *d, unsigned int type)

Will,

Have already tested your V3 vesrion while generating the GIC IO_relaxed patch and it worked OK. This series seems to be same. I shall try this on OMAP4 and let you know ?

Quoted 1 line · Santosh Shilimkar

Will,

Hi Santosh,

Quoted 3 lines

Have already tested your V3 vesrion while generating the GIC IO_relaxed patch and it worked OK. This series seems to be same. I shall try this on OMAP4 and let you know ?

If you hold your horses then I'll post a newer series addressing the points from Colin. Testing that on OMAP4 would be much appreciated!

In further testing I found one bug. d7ed36a added gic_arch_extn,
which needs to be used in gic_eoi. arch/arm/mach-tegra/irq.c will
need to be fixed to replace tegra_ack with tegra_eoi, and any other platform that uses gic_arch_extn also needs to be checked (omap4?).

This patch fixes gic.c:

arch/arm/common/gic.c +0 -0

index 6ecd5c7..8e46dac 100644
@@ -118,7 +118,11 @@ static void gic_unmask_irq(struct irq_data *d)
 

Hello,

Quoted 4 lines · Colin Cross

In further testing I found one bug. ?d7ed36a added gic_arch_extn, which needs to be used in gic_eoi. ? arch/arm/mach-tegra/irq.c will need to be fixed to replace tegra_ack with tegra_eoi, and any other platform that uses gic_arch_extn also needs to be checked (omap4?).

Ok, I'll do some grepping around for those guys. I didn't realise they were already being used.

Quoted 17 lines

This patch fixes gic.c:

arch/arm/common/gic.c +4 -0

index 6ecd5c7..8e46dac 100644
@@ -118,7 +118,11 @@ static void gic_unmask_irq(struct irq_data *d)
 
 ?static void gic_eoi_irq(struct irq_data *d)
 ?{
+ ? ? ? spin_lock(&irq_controller_lock);
+ ? ? ? if (gic_arch_extn.irq_eoi)
+ ? ? ? ? ? ? ? gic_arch_extn.irq_eoi(d);
 ? ? ? ? writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
+ ? ? ? spin_unlock(&irq_controller_lock);
 ?}
 

arch/arm/common/gic.c static void gic_unmask_irq(struct irq_data *d) · line 128

Quoted 1 line

arch/arm/common/gic.c +0 -0

 ?static int gic_set_type(struct irq_data *d, unsigned int type)

Hmm, I don't like that spinlock. Are the gic_arch_extn.* pointers ever modified after being set initially? If not, can we make the locking conditional on that pointer being non-NULL?

In further testing I found one bug. d7ed36a added gic_arch_extn,
which needs to be used in gic_eoi. arch/arm/mach-tegra/irq.c will
need to be fixed to replace tegra_ack with tegra_eoi, and any other platform that uses gic_arch_extn also needs to be checked (omap4?).