Xilinx Intc Fixes

From: Peter Crosthwaite <[email protected]>

Various fixups to the Xilinx Interrupt controller following a review against TRM and RTL descriptions.

Tested as working for microblazeel Linux.

Peter Crosthwaite (5):
intc/xilinx_intc: Use qemu_set_irq intc/xilinx_intc: Don't clear level sens. IRQs without ACK intc/xilinx_intc: Handle level interrupt retriggering intc/xilinx_intc: Inhibit write to ISR when HIE intc/xilinx_intc: Dont lower IRQ when HIE cleared

 hw/intc/xilinx_intc.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

1.8.3.rc1.44.gb387c77.dirty

From: Peter Crosthwaite <[email protected]>

Use qemu_set_irq rather than if-elsing qemu_irq_(lower|raise). No functional change, just reduces verbosity.

---

 hw/intc/xilinx_intc.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

index b106e72..5df7008 100644
@@ -66,11 +66,7 @@ static void update_irq(struct xlx_pic *p)
         i = ~0;
 
     p->regs[R_IVR] = i;
-    if ((p->regs[R_MER] & 1) && p->regs[R_IPR]) {
-        qemu_irq_raise(p->parent_irq);
-    } else {
-        qemu_irq_lower(p->parent_irq);
-    }
+    qemu_set_irq(p->parent_irq, (p->regs[R_MER] & 1) && p->regs[R_IPR]);
 }
 
 static uint64_t

1.8.3.rc1.44.gb387c77.dirty

email accidentally sent with cc supression, so cc qemu-trivial as intended.

Thanks, applied to the trivial patches queue.

/mjt

Thanks,

I need to respin the later patches in this series but as you have taken this ill drop it from the respin. No conflict issues.

Regards,
Peter

heh. I haven't even noticed this is 1/5 ;)

Please don't send just some patches from a series for applying to other trees. Because if you do the respin, at least in some cases you'll have to wait for the applied bits to reach master first... ;)

It's as trivial to remove this one from -trivial as to keep it there, it's your call ;)

Thanks,

/mjt

From: Peter Crosthwaite <[email protected]>

For level sensitive interrupts, ISR bits are cleared when the input pin is lowered. This is incorrect. Only software can clear ISR bits (via IAR or direct write to ISR with !MER(2)).

---

 hw/intc/xilinx_intc.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

index 5df7008..d243a00 100644
@@ -135,13 +135,7 @@ static void irq_handler(void *opaque, int irq, int level)
         return;
     }
 
-    /* Update source flops. Don't clear unless level triggered.
-       Edge triggered interrupts only go away when explicitely acked to
-       the interrupt controller.  */
-    if (!(p->c_kind_of_intr & (1 << irq)) || level) {
-        p->regs[R_ISR] &= ~(1 << irq);
-        p->regs[R_ISR] |= (level << irq);
-    }
+    p->regs[R_ISR] |= (level << irq);
     update_irq(p);
 }
 

1.8.3.rc1.44.gb387c77.dirty

From: Peter Crosthwaite <[email protected]>

Acking a level sensitive interrupt should have no effect if the interrupt pin is still asserted. The current implementation requires and edge condition to occur for setting a level sensitive IRQ, which means an ACK can clear a level sensitive interrupt, until the original source strobes the interrupt again.

Fix by keeping track of the interrupt pin state and setting ISR based on this every time update_irq() is called.

---

 hw/intc/xilinx_intc.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

index d243a00..09b4453 100644
@@ -49,11 +49,19 @@ struct xlx_pic
 
     /* Runtime control registers.  */
     uint32_t regs[R_MAX];
+    /* state of the interrupt input pins */
+    uint32_t irq_pin_state;
 };
 
 static void update_irq(struct xlx_pic *p)
 {
     uint32_t i;
+
+    /* level triggered interrupt */
+    if (p->regs[R_MER] & 2) {
+        p->regs[R_ISR] |= p->irq_pin_state & ~p->c_kind_of_intr;
+    }
+
     /* Update the pending register.  */
     p->regs[R_IPR] = p->regs[R_ISR] & p->regs[R_IER];
 

@@ -135,7 +143,13 @@ static void irq_handler(void *opaque, int irq, int level)
         return;
     }
 
-    p->regs[R_ISR] |= (level << irq);
+    /* edge triggered interrupt */
+    if (p->c_kind_of_intr & (1 << irq) && p->regs[R_MER] & 2) {
+        p->regs[R_ISR] |= (level << irq);
+    }
+
+    p->irq_pin_state &= ~(1 << irq);
+    p->irq_pin_state |= level << irq;
     update_irq(p);
 }
 

1.8.3.rc1.44.gb387c77.dirty

From: Peter Crosthwaite <[email protected]>

When the Hardware Interrupt Enable (HIE) bit is set, software cannot change ISR. Add write guard accordingly.

---

 hw/intc/xilinx_intc.c | 5 +++++
 1 file changed, 5 insertions(+)

index 09b4453..ddedfa3 100644
@@ -116,6 +116,11 @@ pic_write(void *opaque, hwaddr addr,
         case R_CIE:
             p->regs[R_IER] &= ~value; /* Atomic clear ie.  */
             break;
+        case R_ISR:
+            if ((p->regs[R_MER] & 2)) {
+                break;
+            }
+            /* fallthrough */
         default:
             if (addr < ARRAY_SIZE(p->regs))
                 p->regs[addr] = value;

1.8.3.rc1.44.gb387c77.dirty

From: Peter Crosthwaite <[email protected]>

This is a little strange. It is lowering the parent IRQ pin on input when HIE is cleared. There is no such behaviour in the real hardware.

ISR changes based on interrupt pin state are already guarded on HIE being set. So we can just delete this if in its entirety.

---

 hw/intc/xilinx_intc.c | 5 -----
 1 file changed, 5 deletions(-)

index ddedfa3..297f537 100644
@@ -143,11 +143,6 @@ static void irq_handler(void *opaque, int irq, int level)
 {
     struct xlx_pic *p = opaque;
 
-    if (!(p->regs[R_MER] & 2)) {
-        qemu_irq_lower(p->parent_irq);
-        return;
-    }
-
     /* edge triggered interrupt */
     if (p->c_kind_of_intr & (1 << irq) && p->regs[R_MER] & 2) {
         p->regs[R_ISR] |= (level << irq);

1.8.3.rc1.44.gb387c77.dirty