AT91: Convert to fasteoi IRQ handler, and remove ARM irq_finish

AT91: Convert to fasteoi IRQ handler, and remove ARM irq_finish

[Andrew Victor]

The AT91's AIC interrupt controller will internally mask the current
(and lower-priority) interrupts until software acknowledges that the
interrupt was handled by writing to the AIC_EOICR register.

This matches the "fasteoi" IRQ flow-handler, so convert the AT91 to use
handle_fasteoi_irq rather than handle_level_irq.

This also allows the irq_finish() call to be removed from the low-level
ARM IRQ handler.


Signed-off-by: Andrew Victor <linux@maxim.org.za>



diff -urN -x CVS linux-2.6.38/arch/arm/kernel/irq.c linux-2.6/arch/arm/kernel/irq.c
--- linux-2.6.38/arch/arm/kernel/irq.c	2011-04-27 12:28:25.609111745 +0200
+++ linux-2.6/arch/arm/kernel/irq.c	2011-04-27 14:20:32.784167994 +0200
@@ -42,13 +42,6 @@
 #include <asm/mach/irq.h>
 #include <asm/mach/time.h>
 
-/*
- * No architecture-specific irq_finish function defined in arm/arch/irqs.h.
- */
-#ifndef irq_finish
-#define irq_finish(irq) do { } while (0)
-#endif
-
 unsigned long irq_err_count;
 
 int show_interrupts(struct seq_file *p, void *v)
@@ -135,9 +128,6 @@
 		generic_handle_irq(irq);
 	}
 
-	/* AT91 specific workaround */
-	irq_finish(irq);
-
 	irq_exit();
 	set_irq_regs(old_regs);
 }
diff -urN -x CVS linux-2.6.38/arch/arm/mach-at91/gpio.c linux-2.6/arch/arm/mach-at91/gpio.c
--- linux-2.6.38/arch/arm/mach-at91/gpio.c	2011-04-27 12:28:26.157080664 +0200
+++ linux-2.6/arch/arm/mach-at91/gpio.c	2011-04-27 14:45:42.672364605 +0200
@@ -392,8 +392,6 @@
 	at91_gpio = get_irq_chip_data(irq);
 	pio = at91_gpio->regbase;
 
-	/* temporarily mask (level sensitive) parent IRQ */
-	desc->irq_data.chip->irq_ack(&desc->irq_data);
 	for (;;) {
 		/* Reading ISR acks pending (edge triggered) GPIO interrupts.
 		 * When there none are pending, we're finished unless we need
@@ -429,8 +427,8 @@
 			isr >>= 1;
 		}
 	}
-	desc->irq_data.chip->irq_unmask(&desc->irq_data);
-	/* now it may re-trigger */
+	/* acknowledge interrupt - now it may re-trigger */
+	desc->irq_data.chip->irq_eoi(&desc->irq_data);
 }
 
 /*--------------------------------------------------------------------------*/
diff -urN -x CVS linux-2.6.38/arch/arm/mach-at91/include/mach/irqs.h linux-2.6/arch/arm/mach-at91/include/mach/irqs.h
--- linux-2.6.38/arch/arm/mach-at91/include/mach/irqs.h	2009-06-10 05:05:27.000000000 +0200
+++ linux-2.6/arch/arm/mach-at91/include/mach/irqs.h	2011-04-27 14:20:48.471334804 +0200
@@ -28,13 +28,6 @@
 
 
 /*
- * Acknowledge interrupt with AIC after interrupt has been handled.
- *   (by kernel/irq.c)
- */
-#define irq_finish(irq) do { at91_sys_write(AT91_AIC_EOICR, 0); } while (0)
-
-
-/*
  * IRQ interrupt symbols are the AT91xxx_ID_* symbols
  * for IRQs handled directly through the AIC, or else the AT91_PIN_*
  * symbols in gpio.h for ones handled indirectly as GPIOs.
diff -urN -x CVS linux-2.6.38/arch/arm/mach-at91/irq.c linux-2.6/arch/arm/mach-at91/irq.c
--- linux-2.6.38/arch/arm/mach-at91/irq.c	2011-04-27 12:28:26.197078404 +0200
+++ linux-2.6/arch/arm/mach-at91/irq.c	2011-04-27 14:23:29.817625842 +0200
@@ -46,6 +46,12 @@
 	at91_sys_write(AT91_AIC_IECR, 1 << d->irq);
 }
 
+static void at91_aic_eoi(struct irq_data *d)
+{
+	/* Acknowledge with AIC after interrupt has been handled */
+	at91_sys_write(AT91_AIC_EOICR, 0);	
+}
+
 unsigned int at91_extern_irq;
 
 #define is_extern_irq(irq) ((1 << (irq)) & at91_extern_irq)
@@ -122,6 +128,7 @@
 	.irq_ack	= at91_aic_mask_irq,
 	.irq_mask	= at91_aic_mask_irq,
 	.irq_unmask	= at91_aic_unmask_irq,
+	.irq_eoi	= at91_aic_eoi,
 	.irq_set_type	= at91_aic_set_type,
 	.irq_set_wake	= at91_aic_set_wake,
 };
@@ -144,7 +151,7 @@
 		at91_sys_write(AT91_AIC_SMR(i), AT91_AIC_SRCTYPE_LOW | priority[i]);
 
 		set_irq_chip(i, &at91_aic_chip);
-		set_irq_handler(i, handle_level_irq);
+		set_irq_handler(i, handle_fasteoi_irq);
 		set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
 
 		/* Perform 8 End Of Interrupt Command to make sure AIC will not Lock out nIRQ */

AT91: Convert to fasteoi IRQ handler, and remove ARM irq_finish

[Jean-Christophe PLAGNIOL-VILLARD]

On 15:09 Wed 27 Apr     , Andrew Victor wrote:
> The AT91's AIC interrupt controller will internally mask the current
> (and lower-priority) interrupts until software acknowledges that the
> interrupt was handled by writing to the AIC_EOICR register.
> 
> This matches the "fasteoi" IRQ flow-handler, so convert the AT91 to use
> handle_fasteoi_irq rather than handle_level_irq.
> 
> This also allows the irq_finish() call to be removed from the low-level
> ARM IRQ handler.
> 
> 
> Signed-off-by: Andrew Victor <linux@maxim.org.za>
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

Good for me

I put it in the next or you take Thomas?

Best Regards,
J.

AT91: Convert to fasteoi IRQ handler, and remove ARM irq_finish

[torbenh]

On Wed, Apr 27, 2011 at 03:09:02PM +0200, Andrew Victor wrote:
> The AT91's AIC interrupt controller will internally mask the current
> (and lower-priority) interrupts until software acknowledges that the
> interrupt was handled by writing to the AIC_EOICR register.
> 
> This matches the "fasteoi" IRQ flow-handler, so convert the AT91 to use
> handle_fasteoi_irq rather than handle_level_irq.
> 
> This also allows the irq_finish() call to be removed from the low-level
> ARM IRQ handler.
> 
> 
> Signed-off-by: Andrew Victor <linux@maxim.org.za>
> 
> 
> 
> diff -urN -x CVS linux-2.6.38/arch/arm/kernel/irq.c linux-2.6/arch/arm/kernel/irq.c
> --- linux-2.6.38/arch/arm/kernel/irq.c	2011-04-27 12:28:25.609111745 +0200
> +++ linux-2.6/arch/arm/kernel/irq.c	2011-04-27 14:20:32.784167994 +0200
> @@ -42,13 +42,6 @@
>  #include <asm/mach/irq.h>
>  #include <asm/mach/time.h>
>  
> -/*
> - * No architecture-specific irq_finish function defined in arm/arch/irqs.h.
> - */
> -#ifndef irq_finish
> -#define irq_finish(irq) do { } while (0)
> -#endif
> -
>  unsigned long irq_err_count;
>  
>  int show_interrupts(struct seq_file *p, void *v)
> @@ -135,9 +128,6 @@
>  		generic_handle_irq(irq);
>  	}
>  
> -	/* AT91 specific workaround */
> -	irq_finish(irq);
> -
>  	irq_exit();
>  	set_irq_regs(old_regs);
>  }
> diff -urN -x CVS linux-2.6.38/arch/arm/mach-at91/gpio.c linux-2.6/arch/arm/mach-at91/gpio.c
> --- linux-2.6.38/arch/arm/mach-at91/gpio.c	2011-04-27 12:28:26.157080664 +0200
> +++ linux-2.6/arch/arm/mach-at91/gpio.c	2011-04-27 14:45:42.672364605 +0200
> @@ -392,8 +392,6 @@
>  	at91_gpio = get_irq_chip_data(irq);
>  	pio = at91_gpio->regbase;
>  
> -	/* temporarily mask (level sensitive) parent IRQ */
> -	desc->irq_data.chip->irq_ack(&desc->irq_data);
>  	for (;;) {
>  		/* Reading ISR acks pending (edge triggered) GPIO interrupts.
>  		 * When there none are pending, we're finished unless we need
> @@ -429,8 +427,8 @@
>  			isr >>= 1;
>  		}
>  	}
> -	desc->irq_data.chip->irq_unmask(&desc->irq_data);
> -	/* now it may re-trigger */
> +	/* acknowledge interrupt - now it may re-trigger */
> +	desc->irq_data.chip->irq_eoi(&desc->irq_data);
>  }

why does this handler mess with the parent irq chip ?
this looks pretty wrong to me.

>  
>  /*--------------------------------------------------------------------------*/
> diff -urN -x CVS linux-2.6.38/arch/arm/mach-at91/include/mach/irqs.h linux-2.6/arch/arm/mach-at91/include/mach/irqs.h
> --- linux-2.6.38/arch/arm/mach-at91/include/mach/irqs.h	2009-06-10 05:05:27.000000000 +0200
> +++ linux-2.6/arch/arm/mach-at91/include/mach/irqs.h	2011-04-27 14:20:48.471334804 +0200
> @@ -28,13 +28,6 @@
>  
>  
>  /*
> - * Acknowledge interrupt with AIC after interrupt has been handled.
> - *   (by kernel/irq.c)
> - */
> -#define irq_finish(irq) do { at91_sys_write(AT91_AIC_EOICR, 0); } while (0)
> -
> -
> -/*
>   * IRQ interrupt symbols are the AT91xxx_ID_* symbols
>   * for IRQs handled directly through the AIC, or else the AT91_PIN_*
>   * symbols in gpio.h for ones handled indirectly as GPIOs.
> diff -urN -x CVS linux-2.6.38/arch/arm/mach-at91/irq.c linux-2.6/arch/arm/mach-at91/irq.c
> --- linux-2.6.38/arch/arm/mach-at91/irq.c	2011-04-27 12:28:26.197078404 +0200
> +++ linux-2.6/arch/arm/mach-at91/irq.c	2011-04-27 14:23:29.817625842 +0200
> @@ -46,6 +46,12 @@
>  	at91_sys_write(AT91_AIC_IECR, 1 << d->irq);
>  }
>  
> +static void at91_aic_eoi(struct irq_data *d)
> +{
> +	/* Acknowledge with AIC after interrupt has been handled */
> +	at91_sys_write(AT91_AIC_EOICR, 0);	
> +}
> +
>  unsigned int at91_extern_irq;
>  
>  #define is_extern_irq(irq) ((1 << (irq)) & at91_extern_irq)
> @@ -122,6 +128,7 @@
>  	.irq_ack	= at91_aic_mask_irq,

you should probably remove the ack. its just confusing.

>  	.irq_mask	= at91_aic_mask_irq,
>  	.irq_unmask	= at91_aic_unmask_irq,
> +	.irq_eoi	= at91_aic_eoi,
>  	.irq_set_type	= at91_aic_set_type,
>  	.irq_set_wake	= at91_aic_set_wake,
>  };
> @@ -144,7 +151,7 @@
>  		at91_sys_write(AT91_AIC_SMR(i), AT91_AIC_SRCTYPE_LOW | priority[i]);
>  
>  		set_irq_chip(i, &at91_aic_chip);
> -		set_irq_handler(i, handle_level_irq);
> +		set_irq_handler(i, handle_fasteoi_irq);
>  		set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
>  
>  		/* Perform 8 End Of Interrupt Command to make sure AIC will not Lock out nIRQ */
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
torben Hohn

AT91: Convert to fasteoi IRQ handler, and remove ARM irq_finish

[Andrew Victor]

hi,

>> - ? ? desc->irq_data.chip->irq_unmask(&desc->irq_data);
>> - ? ? /* now it may re-trigger */
>> + ? ? /* acknowledge interrupt - now it may re-trigger */
>> + ? ? desc->irq_data.chip->irq_eoi(&desc->irq_data);
>> ?}
>
> why does this handler mess with the parent irq chip ?
> this looks pretty wrong to me.

This is a chained-handler, and without that line the end-of-interrupt
doesn't seem to be signalled.


Regards,
  Andrew Victor

AT91: Convert to fasteoi IRQ handler, and remove ARM irq_finish

[Sebastian Andrzej Siewior]

* Andrew Victor | 2011-04-28 19:12:23 [+0200]:

>hi,
>
>>> - ? ? desc->irq_data.chip->irq_unmask(&desc->irq_data);
>>> - ? ? /* now it may re-trigger */
>>> + ? ? /* acknowledge interrupt - now it may re-trigger */
>>> + ? ? desc->irq_data.chip->irq_eoi(&desc->irq_data);
>>> ?}
>>
>> why does this handler mess with the parent irq chip ?
>> this looks pretty wrong to me.
>
>This is a chained-handler, and without that line the end-of-interrupt
>doesn't seem to be signalled.

Most chain handler don't mess with other irq chips. Why is the parent
irq hanlder set level and not eoi? Wouldn't this make this superflous?

btw: tglx removed direct desc access so this patch won't apply.

>Regards,
>  Andrew Victor

Sebastian

AT91: Convert to fasteoi IRQ handler, and remove ARM irq_finish

[Thomas Gleixner]

On Thu, 28 Apr 2011, Sebastian Andrzej Siewior wrote:

> * Andrew Victor | 2011-04-28 19:12:23 [+0200]:
> 
> >hi,
> >
> >>> - ? ? desc->irq_data.chip->irq_unmask(&desc->irq_data);
> >>> - ? ? /* now it may re-trigger */
> >>> + ? ? /* acknowledge interrupt - now it may re-trigger */
> >>> + ? ? desc->irq_data.chip->irq_eoi(&desc->irq_data);
> >>> ?}
> >>
> >> why does this handler mess with the parent irq chip ?
> >> this looks pretty wrong to me.
> >
> >This is a chained-handler, and without that line the end-of-interrupt
> >doesn't seem to be signalled.
> 
> Most chain handler don't mess with other irq chips. Why is the parent
> irq hanlder set level and not eoi? Wouldn't this make this superflous?
> 
> btw: tglx removed direct desc access so this patch won't apply.

It does, you still can access irq_data, but you should use the
wrappers.

Thanks,

	tglx

AT91: Convert to fasteoi IRQ handler, and remove ARM irq_finish

[Andrew Victor]

hi Sebastian,

>>> why does this handler mess with the parent irq chip ?
>>> this looks pretty wrong to me.
>>
>>This is a chained-handler, and without that line the end-of-interrupt
>>doesn't seem to be signalled.
>
> Most chain handler don't mess with other irq chips. Why is the parent
> irq hanlder set level and not eoi? Wouldn't this make this superflous?

This patch changes the parent from level to eoi.

AT91: Convert to fasteoi IRQ handler, and remove ARM irq_finish

[Sebastian Andrzej Siewior]

* Thomas Gleixner | 2011-04-28 21:48:52 [+0200]:

>On Thu, 28 Apr 2011, Sebastian Andrzej Siewior wrote:
>
>> * Andrew Victor | 2011-04-28 19:12:23 [+0200]:
>> 
>> >hi,
>> >
>> >>> - ? ? desc->irq_data.chip->irq_unmask(&desc->irq_data);
>> >>> - ? ? /* now it may re-trigger */
>> >>> + ? ? /* acknowledge interrupt - now it may re-trigger */
>> >>> + ? ? desc->irq_data.chip->irq_eoi(&desc->irq_data);

>> btw: tglx removed direct desc access so this patch won't apply.
>
>It does, you still can access irq_data, but you should use the
>wrappers.

commit ac93cdbd6e5e2547a031e2739e0dd445824bf1c8
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Thu Mar 24 12:48:18 2011 +0100

    arm: at91: Cleanup irq chip

This commit removed "desc->irq_data.chip->irq_unmask(&desc->irq_data);"
among others so it does not apply anymore.

>Thanks,
>
>	tglx

Sebastian

AT91: Convert to fasteoi IRQ handler, and remove ARM irq_finish

[Sebastian Andrzej Siewior]

* Andrew Victor | 2011-04-28 22:03:03 [+0200]:

>hi Sebastian,
Hi Andrew,

>>>> why does this handler mess with the parent irq chip ?
>>>> this looks pretty wrong to me.
>>>
>>>This is a chained-handler, and without that line the end-of-interrupt
>>>doesn't seem to be signalled.
>>
>> Most chain handler don't mess with other irq chips. Why is the parent
>> irq hanlder set level and not eoi? Wouldn't this make this superflous?
>
>This patch changes the parent from level to eoi.

Yes. So why does your gpio chip still call ->irq_eoi() of the parent
irq-chip? This is done by done the eoi-handler of the parent.

Sebastian