[0/3] UIC fixes

[0/3] UIC fixes

[David Gibson]

This series fixes several small problems with the driver for the UIC
interrupt controller found in PowerPC 4xx embedded CPUs.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[PATCH 2/3] Fix irq flow handler for 4xx UIC

[David Gibson]

At present the driver for the UIC (the embedded interrupt controller
in 4xx chips) uses the handle_level_irq() flow handler.  It turns out
this does not correctly handle level triggered interrupts on the UIC.

Specifically, acknowledging an irq on the UIC (i.e. clearing the
relevant bit in UIC_SR) will have no effect for a level interrupt
which is still asserted by the external device, even if the irq is
already masked.  Therefore, unlike handle_level_irq() we must ack the
interrupt after invoking the ISR (which should cause the device to
stop asserting the irq) instead of acking it when we mask it, before
the ISR.

This patch implements this change, in a new handle_uic_irq(), a
customised irq flow handler for the UIC.  For edge triggered
interrupts, handle_uic_irq() still uses the old flow - we must ack
edge triggered interrupt before the ISR not after, or we could miss a
second event which occurred between invoking the ISR and acking the
irq.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

---
 arch/powerpc/sysdev/uic.c |   61 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 1 deletion(-)

Index: working-2.6/arch/powerpc/sysdev/uic.c
===================================================================
--- working-2.6.orig/arch/powerpc/sysdev/uic.c	2007-08-14 13:17:52.000000000 +1000
+++ working-2.6/arch/powerpc/sysdev/uic.c	2007-08-14 13:17:57.000000000 +1000
@@ -24,6 +24,7 @@
 #include <linux/spinlock.h>
 #include <linux/irq.h>
 #include <linux/interrupt.h>
+#include <linux/kernel_stat.h>
 #include <asm/irq.h>
 #include <asm/io.h>
 #include <asm/prom.h>
@@ -159,6 +160,64 @@ static struct irq_chip uic_irq_chip = {
 	.set_type	= uic_set_irq_type,
 };
 
+/**
+ *	handle_uic_irq - irq flow handler for UIC
+ *	@irq:	the interrupt number
+ *	@desc:	the interrupt description structure for this irq
+ *
+ * This is modified version of the generic handle_level_irq() suitable
+ * for the UIC.  On the UIC, acking (i.e. clearing the SR bit) a level
+ * irq will have no effect if the interrupt is still asserted by the
+ * device, even if the interrupt is already masked.  Therefore, unlike
+ * the standard handle_level_irq(), we must ack the interrupt *after*
+ * invoking the ISR (which should have de-asserted the interrupt in
+ * the external source).  For edge interrupts we ack at the beginning
+ * instead of the end, to keep the window in which we can miss an
+ * interrupt as small as possible.
+ */
+void fastcall handle_uic_irq(unsigned int irq, struct irq_desc *desc)
+{
+	unsigned int cpu = smp_processor_id();
+	struct irqaction *action;
+	irqreturn_t action_ret;
+
+	spin_lock(&desc->lock);
+	if (desc->status & IRQ_LEVEL)
+		desc->chip->mask(irq);
+	else
+		desc->chip->mask_ack(irq);
+
+	if (unlikely(desc->status & IRQ_INPROGRESS))
+		goto out_unlock;
+	desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
+	kstat_cpu(cpu).irqs[irq]++;
+
+	/*
+	 * If its disabled or no action available
+	 * keep it masked and get out of here
+	 */
+	action = desc->action;
+	if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
+		desc->status |= IRQ_PENDING;
+		goto out_unlock;
+	}
+
+	desc->status |= IRQ_INPROGRESS;
+	desc->status &= ~IRQ_PENDING;
+	spin_unlock(&desc->lock);
+
+	action_ret = handle_IRQ_event(irq, action);
+
+	spin_lock(&desc->lock);
+	desc->status &= ~IRQ_INPROGRESS;
+	if (desc->status & IRQ_LEVEL)
+		desc->chip->ack(irq);
+	if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
+		desc->chip->unmask(irq);
+out_unlock:
+	spin_unlock(&desc->lock);
+}
+
 static int uic_host_match(struct irq_host *h, struct device_node *node)
 {
 	struct uic *uic = h->host_data;
@@ -173,7 +232,7 @@ static int uic_host_map(struct irq_host 
 	set_irq_chip_data(virq, uic);
 	/* Despite the name, handle_level_irq() works for both level
 	 * and edge irqs on UIC.  FIXME: check this is correct */
-	set_irq_chip_and_handler(virq, &uic_irq_chip, handle_level_irq);
+	set_irq_chip_and_handler(virq, &uic_irq_chip, handle_uic_irq);
 
 	/* Set default irq type */
 	set_irq_type(virq, IRQ_TYPE_NONE);

[PATCH 1/3] Fix setting of irq trigger type in UIC driver

[David Gibson]

The UIC (interrupt controller in 4xx embedded CPUs) driver currently
missets the IRQ_lEVEL flag in desc->status, due to a thinko.  This
patch fixes the bug.

Currently this is only a cosmetic problem (affects the output in
/proc/interrupts), however subsequent patches will use the IRQ_LEVEL
flag to affect flow handling.

Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

---
 arch/powerpc/sysdev/uic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: working-2.6/arch/powerpc/sysdev/uic.c
===================================================================
--- working-2.6.orig/arch/powerpc/sysdev/uic.c	2007-08-14 13:17:44.000000000 +1000
+++ working-2.6/arch/powerpc/sysdev/uic.c	2007-08-14 13:17:52.000000000 +1000
@@ -142,7 +142,7 @@ static int uic_set_irq_type(unsigned int
 
 	desc->status &= ~(IRQ_TYPE_SENSE_MASK | IRQ_LEVEL);
 	desc->status |= flow_type & IRQ_TYPE_SENSE_MASK;
-	if (trigger)
+	if (!trigger)
 		desc->status |= IRQ_LEVEL;
 
 	spin_unlock_irqrestore(&uic->lock, flags);

[PATCH 3/3] Improve robustness of the UIC cascade handler

[David Gibson]

At present the cascade interrupt handler for the UIC (interrupt
controller on 4xx embedded chips) will misbehave badly if it is called
spuriously - that is if the handler is invoked when no interrupts are
asserted in the child UIC.

Although spurious interrupts shouldn't happen, it's good to behave
robustly if they do.  This patch does so by checking for and ignoring
spurious interrupts.

Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

---
 arch/powerpc/sysdev/uic.c |    3 +++
 1 file changed, 3 insertions(+)

Index: working-2.6/arch/powerpc/sysdev/uic.c
===================================================================
--- working-2.6.orig/arch/powerpc/sysdev/uic.c	2007-08-14 13:46:02.000000000 +1000
+++ working-2.6/arch/powerpc/sysdev/uic.c	2007-08-14 13:46:02.000000000 +1000
@@ -266,6 +266,9 @@ irqreturn_t uic_cascade(int virq, void *
 	int subvirq;
 
 	msr = mfdcr(uic->dcrbase + UIC_MSR);
+	if (!msr) /* spurious interrupt */
+		return IRQ_HANDLED;
+
 	src = 32 - ffs(msr);
 
 	subvirq = irq_linear_revmap(uic->irqhost, src);

Re: [PATCH 1/3] Fix setting of irq trigger type in UIC driver

[Josh Boyer]

On Tue, 14 Aug 2007 13:52:42 +1000 (EST)
David Gibson <david@gibson.dropbear.id.au> wrote:

> The UIC (interrupt controller in 4xx embedded CPUs) driver currently
> missets the IRQ_lEVEL flag in desc->status, due to a thinko.  This
> patch fixes the bug.
> 
> Currently this is only a cosmetic problem (affects the output in
> /proc/interrupts), however subsequent patches will use the IRQ_LEVEL
> flag to affect flow handling.
> 
> Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Acked-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>

josh

Re: [PATCH 2/3] Fix irq flow handler for 4xx UIC

[Josh Boyer]

On Tue, 14 Aug 2007 13:52:42 +1000 (EST)
David Gibson <david@gibson.dropbear.id.au> wrote:

> At present the driver for the UIC (the embedded interrupt controller
> in 4xx chips) uses the handle_level_irq() flow handler.  It turns out
> this does not correctly handle level triggered interrupts on the UIC.
> 
> Specifically, acknowledging an irq on the UIC (i.e. clearing the
> relevant bit in UIC_SR) will have no effect for a level interrupt
> which is still asserted by the external device, even if the irq is
> already masked.  Therefore, unlike handle_level_irq() we must ack the
> interrupt after invoking the ISR (which should cause the device to
> stop asserting the irq) instead of acking it when we mask it, before
> the ISR.
> 
> This patch implements this change, in a new handle_uic_irq(), a
> customised irq flow handler for the UIC.  For edge triggered
> interrupts, handle_uic_irq() still uses the old flow - we must ack
> edge triggered interrupt before the ISR not after, or we could miss a
> second event which occurred between invoking the ISR and acking the
> irq.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Acked-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>

josh

Re: [PATCH 3/3] Improve robustness of the UIC cascade handler

[Josh Boyer]

On Tue, 14 Aug 2007 13:52:42 +1000 (EST)
David Gibson <david@gibson.dropbear.id.au> wrote:

> At present the cascade interrupt handler for the UIC (interrupt
> controller on 4xx embedded chips) will misbehave badly if it is called
> spuriously - that is if the handler is invoked when no interrupts are
> asserted in the child UIC.
> 
> Although spurious interrupts shouldn't happen, it's good to behave
> robustly if they do.  This patch does so by checking for and ignoring
> spurious interrupts.
> 
> Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> ---
>  arch/powerpc/sysdev/uic.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> Index: working-2.6/arch/powerpc/sysdev/uic.c
> ===================================================================
> --- working-2.6.orig/arch/powerpc/sysdev/uic.c	2007-08-14 13:46:02.000000000 +1000
> +++ working-2.6/arch/powerpc/sysdev/uic.c	2007-08-14 13:46:02.000000000 +1000
> @@ -266,6 +266,9 @@ irqreturn_t uic_cascade(int virq, void *
>  	int subvirq;
> 
>  	msr = mfdcr(uic->dcrbase + UIC_MSR);
> +	if (!msr) /* spurious interrupt */
> +		return IRQ_HANDLED;

Hm.  Is there was a way we could have this case increment
ppc_spurious_interrupts so that the BAD entry in /proc/interrupts would
be updated with these?  Not a huge deal, but it might be nice to have.
Otherwise the patch looks fine.

Acked-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>

josh