[microblaze-uclinux] xmbserial bug fix?

[John Williams <jwilliams@xxxxxxxxxxxxxx>]

Hi,

Attached is a patch that appears to fix the lingering, and deeply 
unsatisfactory xmbserial.c UARTLITE driver bug - whereby the driver can 
be hung by dumping a large number of characters via cut'n'paste or 
probably a zmodem style transfer as well.

It's based on a trial patch that Yashi posted months ago - however the 
reason I'm not overjoyed to nail this bug is that the solution is 
completely counter-intuitive.  It's too late at night to re-hash the 
technical discussion - see the comments in the patch and review earlier 
list emails about the behaviour of the interrupt controller and so on if 
you're interested.

What would be great is if a few people people could

(a) confirm that they see the bug in an unpatched kernel (easy to do, 
just copy a large amount of text to your clipboard, then paste it into a 
minicom session connected to microblaze)

and

(b) apply the patch and test again, see if it fixes things.

I tried and succesfully dumped over 160Kb of text at high speed without 
any data loss or crashing, before I got sick of pressing Shift-Insert :)

If it works, and doesn't have any other noticeable side-effects, then it 
can go in.

Thanks,

John


--- /opt/src/uClinux-2.4.x/arch/microblaze/kernel/microblaze_intc.c	2005-01-26 04:39:56.000000000 +1000
+++ microblaze_intc.c	2005-02-17 22:18:18.000000000 +1000
@@ -267,22 +267,50 @@
 }
 
   
+/* The "when-to-ack?" mystery continues.  Common sense says that level sensitive
+   interrupts should be acked after handling, and edge-sensitives before.
+   BZZZT!  Wrong!
+   Reversing this fixes the uartlite driver bug, so beats me what's going on.
+   Pragmatism argues for a confusing universe that works, over a sensible
+   universe where things are broken... sigh....
+
+   I have a sneaking suspicion that it matters more the behaviour of the 
+   individual peripheral, than whether or not the IRQ signal is edge or level
+   sensitive...  However, this is very hard to test in any clean, generic way..
+
+*/
+   
+#define MAKES_NO_SENSE_BUT_WORKS 1
+
 void microblaze_intc_disable_and_ack_irq(unsigned irq)
 {
 	microblaze_intc_disable_irq(irq);
 
+#if MAKES_NO_SENSE_BUT_WORKS
+	/* Acknowledge level sensitive interrupts immediately */
+	if(IRQ_LEVEL_SENSITIVE(irq,CONFIG_XILINX_INTC_0_KIND_OF_INTR))
+		microblaze_intc_ack_irq(irq);
+#else
 	/* Acknowledge edge sensitive interrupts immediately */
 	if(IRQ_EDGE_SENSITIVE(irq,CONFIG_XILINX_INTC_0_KIND_OF_INTR))
 		microblaze_intc_ack_irq(irq);
+#endif
 }
 
 void microblaze_intc_end(unsigned irq)
 {
 	if (!(irq_desc[irq].status & (IRQ_DISABLED | IRQ_INPROGRESS))) {
 		microblaze_intc_enable_irq(irq);
+
+#if MAKES_NO_SENSE_BUT_WORKS
+		/* Edge sensitive interrupts are acked after handling */
+		if(IRQ_EDGE_SENSITIVE(irq,CONFIG_XILINX_INTC_0_KIND_OF_INTR))
+			microblaze_intc_ack_irq(irq);
+#else
 		/* Level sensitive interrupts are acked after handling */
 		if(IRQ_LEVEL_SENSITIVE(irq,CONFIG_XILINX_INTC_0_KIND_OF_INTR))
 			microblaze_intc_ack_irq(irq);
+#endif
 	}
 }