[Qemu-devel] [PATCH v1 0/5] Xilinx Intc Fixes

[Qemu-devel] [PATCH v1 0/5] Xilinx Intc Fixes

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>


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

[Qemu-devel] [PATCH v1 1/5] intc/xilinx_intc: Use qemu_set_irq

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

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

Cc: qemu-trivial@nongnu.org

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

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

diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
index b106e72..5df7008 100644
--- a/hw/intc/xilinx_intc.c
+++ b/hw/intc/xilinx_intc.c
@@ -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

Re: [Qemu-devel] [PATCH v1 1/5] intc/xilinx_intc: Use qemu_set_irq

[Peter Crosthwaite]

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

On Fri, Jun 7, 2013 at 12:38 PM,  <peter.crosthwaite@xilinx.com> wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> Use qemu_set_irq rather than if-elsing qemu_irq_(lower|raise). No
> functional change, just reduces verbosity.
>
> Cc: qemu-trivial@nongnu.org
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/intc/xilinx_intc.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
> index b106e72..5df7008 100644
> --- a/hw/intc/xilinx_intc.c
> +++ b/hw/intc/xilinx_intc.c
> @@ -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
>

Re: [Qemu-devel] [Qemu-trivial] [PATCH v1 1/5] intc/xilinx_intc: Use qemu_set_irq

[Michael Tokarev]

07.06.2013 06:39, Peter Crosthwaite wrote:
>> Use qemu_set_irq rather than if-elsing qemu_irq_(lower|raise). No
>> functional change, just reduces verbosity.

Thanks, applied to the trivial patches queue.

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH v1 1/5] intc/xilinx_intc: Use qemu_set_irq

[Peter Crosthwaite]

On Sat, Jun 8, 2013 at 8:36 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 07.06.2013 06:39, Peter Crosthwaite wrote:
>>> Use qemu_set_irq rather than if-elsing qemu_irq_(lower|raise). No
>>> functional change, just reduces verbosity.
>
> Thanks, applied to the trivial patches queue.
>

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

> /mjt
>

Re: [Qemu-devel] [Qemu-trivial] [PATCH v1 1/5] intc/xilinx_intc: Use qemu_set_irq

[Michael Tokarev]

11.06.2013 04:41, Peter Crosthwaite wrote:
> On Sat, Jun 8, 2013 at 8:36 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> Thanks, applied to the trivial patches queue.
> 
> 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.

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

[Qemu-devel] [PATCH v1 2/5] intc/xilinx_intc: Don't clear level sens. IRQs without ACK

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

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)).

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

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

diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
index 5df7008..d243a00 100644
--- a/hw/intc/xilinx_intc.c
+++ b/hw/intc/xilinx_intc.c
@@ -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

[Qemu-devel] [PATCH v1 3/5] intc/xilinx_intc: Handle level interrupt retriggering

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

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.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

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

diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
index d243a00..09b4453 100644
--- a/hw/intc/xilinx_intc.c
+++ b/hw/intc/xilinx_intc.c
@@ -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

[Qemu-devel] [PATCH v1 4/5] intc/xilinx_intc: Inhibit write to ISR when HIE

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

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

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

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

diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
index 09b4453..ddedfa3 100644
--- a/hw/intc/xilinx_intc.c
+++ b/hw/intc/xilinx_intc.c
@@ -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

[Qemu-devel] [PATCH v1 5/5] intc/xilinx_intc: Dont lower IRQ when HIE cleared

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

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.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

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

diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
index ddedfa3..297f537 100644
--- a/hw/intc/xilinx_intc.c
+++ b/hw/intc/xilinx_intc.c
@@ -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