Level IRQ handling on Xilinx INTC with ARCH=powerpc

Level IRQ handling on Xilinx INTC with ARCH=powerpc

[Sergey Temerkhanov]

Hi, all!

I'm currently in process of development of Xilinx I2C IP core driver and I've run into the problem: the ISR for level-based IRQ is called twice.

It looks like level IRQ handling on Xilinx INTC with ARCH=powerpc is currently broken as Xilinx INTC requires IRQ acknowledge with IRQ signal deasserted (i.e. after ISR call). ISR is supposed to acknowledge level IRQ in the core which asserted it thus deasserting appropriate IRQ signal.
And handle_level_irq() which is currently used as high-level IRQ handler for Xilinx INTC only tries to acknowledge IRQ before ISR call. So that the IRQ remains asserted in INTC and after the call to desc->chip->unmask() causes spurious attempt to process the same IRQ again. However, call to desc->chip->ack() this time finishes the required procedure of IRQ acknowledge.

I see a few ways to resolve this:
1. Change high-level handler to __do_IRQ() (as for ARCH=ppc) with proper methods modification (which is deprecated).
2. Change handle_level_irq() so that it calls desc->chip->ack() or desc->chip->end() (and set chip->end to point to xilinx_intc_ack() for Xilinx INTC) before desc->chip->unmask() (may potentially cause breakage for other arches. But I've tried it and it works).
3. Acknowledge IRQ at INTC in ISR (quick'n'dirty hack).

Regards, Sergey Temerkhanov

Re: Level IRQ handling on Xilinx INTC with ARCH=powerpc

[David Howells]

Sergey Temerkhanov <temerkhanov@yandex.ru> wrote:

> And handle_level_irq() which is currently used as high-level IRQ handler for
> Xilinx INTC only tries to acknowledge IRQ before ISR call. So that the IRQ
> remains asserted in INTC and after the call to desc->chip->unmask() causes
> spurious attempt to process the same IRQ again. However, call to
> desc->chip->ack() this time finishes the required procedure of IRQ
> acknowledge.

I think I'm seeing the same on the MN10300 arch with its builtin PIC.  My
soultion was to make unmask() also clear the IRQ latch in the PIC for that
channel.  We perhaps want an unmask_ack() op.

David

Re: Level IRQ handling on Xilinx INTC with ARCH=powerpc

[Benjamin Herrenschmidt]

On Tue, 2008-07-29 at 15:14 +0100, David Howells wrote:
> Sergey Temerkhanov <temerkhanov@yandex.ru> wrote:
> 
> > And handle_level_irq() which is currently used as high-level IRQ handler for
> > Xilinx INTC only tries to acknowledge IRQ before ISR call. So that the IRQ
> > remains asserted in INTC and after the call to desc->chip->unmask() causes
> > spurious attempt to process the same IRQ again. However, call to
> > desc->chip->ack() this time finishes the required procedure of IRQ
> > acknowledge.
> 
> I think I'm seeing the same on the MN10300 arch with its builtin PIC.  My
> soultion was to make unmask() also clear the IRQ latch in the PIC for that
> channel.  We perhaps want an unmask_ack() op.

I've heard about similar issues on other setups... I dislike having a
separate op though, not sure what's the best approach. Another one is to
write a different level handler for such PICs, though that somewhat
sucks too. CC'ing Ingo and Thomas who may have a better idea.

Ben.

[PATCH] Re: Level IRQ handling on Xilinx INTC with ARCH=powerpc

[Sergey Temerkhanov]

[-- Attachment #1: Type: text/plain, Size: 221 bytes --]

I've prepared the patch to fix the problem being discussed. It adds a field 
flags to struct irq_chip. If IRQ_CHIP_UNMASK_ACK is set in this field, 
acknowledge is performed before unmasking.

The patch is against 2.6.26

[-- Attachment #2: fix-level-irq-handling.patch --]
[-- Type: text/x-diff, Size: 1696 bytes --]

diff -r 6b0915754563 arch/powerpc/sysdev/xilinx_intc.c
--- a/arch/powerpc/sysdev/xilinx_intc.c	Mon Jul 28 19:59:22 2008 +0400
+++ b/arch/powerpc/sysdev/xilinx_intc.c	Fri Aug 08 13:13:52 2008 +0400
@@ -73,6 +73,7 @@
 	.mask = xilinx_intc_mask,
 	.unmask = xilinx_intc_unmask,
 	.ack = xilinx_intc_ack,
+	.flags = IRQ_CHIP_UNMASK_ACK,
 };
 
 /*
@@ -107,8 +108,8 @@
 	}
 	regs = ioremap(res.start, 32);
 
-	printk(KERN_INFO "Xilinx intc at 0x%08LX mapped to 0x%p\n",
-		res.start, regs);
+	printk(KERN_INFO "Xilinx intc at 0x%p mapped to 0x%p\n",
+		(void *)res.start, regs);
 
 	/* Setup interrupt controller */
 	out_be32(regs + XINTC_IER, 0); /* disable all irqs */
diff -r 6b0915754563 include/linux/irq.h
--- a/include/linux/irq.h	Mon Jul 28 19:59:22 2008 +0400
+++ b/include/linux/irq.h	Fri Aug 08 13:13:52 2008 +0400
@@ -114,6 +114,9 @@
 	int		(*retrigger)(unsigned int irq);
 	int		(*set_type)(unsigned int irq, unsigned int flow_type);
 	int		(*set_wake)(unsigned int irq, unsigned int on);
+
+	unsigned int	flags;
+#define IRQ_CHIP_UNMASK_ACK	0x00000001
 
 	/* Currently used only by UML, might disappear one day.*/
 #ifdef CONFIG_IRQ_RELEASE_METHOD
diff -r 6b0915754563 kernel/irq/chip.c
--- a/kernel/irq/chip.c	Mon Jul 28 19:59:22 2008 +0400
+++ b/kernel/irq/chip.c	Fri Aug 08 13:13:52 2008 +0400
@@ -377,8 +377,12 @@
 
 	spin_lock(&desc->lock);
 	desc->status &= ~IRQ_INPROGRESS;
-	if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
-		desc->chip->unmask(irq);
+	if (!(desc->status & IRQ_DISABLED)) {
+		if (desc->chip->flags & IRQ_CHIP_UNMASK_ACK)
+			desc->chip->ack(irq);
+		if (desc->chip->unmask)
+			desc->chip->unmask(irq);
+	}
 out_unlock:
 	spin_unlock(&desc->lock);
 }