[Qemu-devel] [PATCH 0/7] Allwinner A10 fixes

[Qemu-devel] [PATCH 0/7] Allwinner A10 fixes

[Beniamino Galvani]

This series introduces some fixes and missing features found while
trying to run mainline Linux kernel on emulated Allwinner A10.

Most of the changes are related to interrupt handling but there are
also some fixes to EMAC and timer.

With this applied I'm able to boot Linux 3.14-rc2 using a NFS root:

https://gist.github.com/anonymous/3e09495652009c6b9da4

Beniamino Galvani (7):
  allwinner-a10-pic: set vector address when an interrupt is pending
  allwinner-a10-pic: fix interrupt clear behaviour
  allwinner-a10-pit: avoid generation of spurious interrupts
  allwinner-a10-pit: use level triggered interrupts
  allwinner-a10-pit: implement prescaler and source selection
  allwinner-emac: set autonegotiation complete bit on link up
  allwinner-emac: update irq status after writes to interrupt registers

 hw/intc/allwinner-a10-pic.c     |   17 +++++++++--
 hw/net/allwinner_emac.c         |    6 ++--
 hw/timer/allwinner-a10-pit.c    |   60 +++++++++++++++++++++++++++++++--------
 include/hw/net/allwinner_emac.h |    1 +
 4 files changed, 67 insertions(+), 17 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/7] allwinner-a10-pic: set vector address when an interrupt is pending

[Beniamino Galvani]

This patch implements proper updating of the vector register which
should hold, according to the A10 user manual, the vector address for
the interrupt currently active on the CPU IRQ input.

Interrupt priority is not implemented at the moment and thus the first
pending interrupt is returned.

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 hw/intc/allwinner-a10-pic.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
index 407d563..bb2351f 100644
--- a/hw/intc/allwinner-a10-pic.c
+++ b/hw/intc/allwinner-a10-pic.c
@@ -23,11 +23,20 @@
 static void aw_a10_pic_update(AwA10PICState *s)
 {
     uint8_t i;
-    int irq = 0, fiq = 0;
+    int irq = 0, fiq = 0, pending;
+
+    s->vector = 0;
 
     for (i = 0; i < AW_A10_PIC_REG_NUM; i++) {
         irq |= s->irq_pending[i] & ~s->mask[i];
         fiq |= s->select[i] & s->irq_pending[i] & ~s->mask[i];
+
+        if (!s->vector) {
+            pending = ffs(s->irq_pending[i] & ~s->mask[i]);
+            if (pending) {
+                s->vector = (i * 32 + pending - 1) * 4;
+            }
+        }
     }
 
     qemu_set_irq(s->parent_irq, !!irq);
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/7] allwinner-a10-pic: fix interrupt clear behaviour

[Beniamino Galvani]

According to this mail thread [1], writing to pending register seems
to have no effect on actual pending status of interrupts. This means
that the only way to clear a pending interrupt is to clear the
interrupt source. This patch implements such behaviour.

[1] http://lkml.org/lkml/2013/7/6/59

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 hw/intc/allwinner-a10-pic.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
index bb2351f..afd57ef 100644
--- a/hw/intc/allwinner-a10-pic.c
+++ b/hw/intc/allwinner-a10-pic.c
@@ -49,6 +49,8 @@ static void aw_a10_pic_set_irq(void *opaque, int irq, int level)
 
     if (level) {
         set_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
+    } else {
+        clear_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
     }
     aw_a10_pic_update(s);
 }
@@ -105,10 +107,10 @@ static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value,
         s->nmi = value;
         break;
     case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8:
-        s->irq_pending[index] &= ~value;
+        /* Nothing to do */
         break;
     case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
-        s->fiq_pending[index] &= ~value;
+        /* Ditto */
         break;
     case AW_A10_PIC_SELECT ... AW_A10_PIC_SELECT + 8:
         s->select[index] = value;
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/7] allwinner-a10-pit: avoid generation of spurious interrupts

[Beniamino Galvani]

The model was generating interrupts for all enabled timers after the
expiration of one of them. Avoid this by passing to the timer callback
function a structure containing the index of the expired timer.

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 hw/timer/allwinner-a10-pit.c |   30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index b27fce8..3e1c183 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -19,6 +19,11 @@
 #include "sysemu/sysemu.h"
 #include "hw/timer/allwinner-a10-pit.h"
 
+typedef struct TimerContext {
+    AwA10PITState *state;
+    int index;
+} TimerContext;
+
 static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
 {
     AwA10PITState *s = AW_A10_PIT(opaque);
@@ -193,18 +198,17 @@ static void a10_pit_reset(DeviceState *dev)
 
 static void a10_pit_timer_cb(void *opaque)
 {
-    AwA10PITState *s = AW_A10_PIT(opaque);
-    uint8_t i;
+    TimerContext *tc = opaque;
+    AwA10PITState *s = tc->state;
+    uint8_t i = tc->index;
 
-    for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) {
-        if (s->control[i] & AW_A10_PIT_TIMER_EN) {
-            s->irq_status |= 1 << i;
-            if (s->control[i] & AW_A10_PIT_TIMER_MODE) {
-                ptimer_stop(s->timer[i]);
-                s->control[i] &= ~AW_A10_PIT_TIMER_EN;
-            }
-            qemu_irq_pulse(s->irq[i]);
+    if (s->control[i] & AW_A10_PIT_TIMER_EN) {
+        s->irq_status |= 1 << i;
+        if (s->control[i] & AW_A10_PIT_TIMER_MODE) {
+            ptimer_stop(s->timer[i]);
+            s->control[i] &= ~AW_A10_PIT_TIMER_EN;
         }
+        qemu_irq_pulse(s->irq[i]);
     }
 }
 
@@ -213,6 +217,7 @@ static void a10_pit_init(Object *obj)
     AwA10PITState *s = AW_A10_PIT(obj);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     QEMUBH * bh[AW_A10_PIT_TIMER_NR];
+    TimerContext *tc;
     uint8_t i;
 
     for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) {
@@ -223,7 +228,10 @@ static void a10_pit_init(Object *obj)
     sysbus_init_mmio(sbd, &s->iomem);
 
     for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) {
-        bh[i] = qemu_bh_new(a10_pit_timer_cb, s);
+        tc = g_malloc(sizeof(TimerContext));
+        tc->state = s;
+        tc->index = i;
+        bh[i] = qemu_bh_new(a10_pit_timer_cb, tc);
         s->timer[i] = ptimer_init(bh[i]);
         ptimer_set_freq(s->timer[i], 240000);
     }
-- 
1.7.10.4

[Qemu-devel] [PATCH 4/7] allwinner-a10-pit: use level triggered interrupts

[Beniamino Galvani]

Converts the interrupt generation logic to the use of level triggered
interrupts.

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 hw/timer/allwinner-a10-pit.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index 3e1c183..4723b25 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -24,6 +24,15 @@ typedef struct TimerContext {
     int index;
 } TimerContext;
 
+static void a10_pit_update_irq(AwA10PITState *s)
+{
+    int i;
+
+    for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) {
+        qemu_set_irq(s->irq[i], s->irq_status & s->irq_enable & (1 << i));
+    }
+}
+
 static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
 {
     AwA10PITState *s = AW_A10_PIT(opaque);
@@ -79,9 +88,11 @@ static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
     switch (offset) {
     case AW_A10_PIT_TIMER_IRQ_EN:
         s->irq_enable = value;
+        a10_pit_update_irq(s);
         break;
     case AW_A10_PIT_TIMER_IRQ_ST:
         s->irq_status &= ~value;
+        a10_pit_update_irq(s);
         break;
     case AW_A10_PIT_TIMER_BASE ... AW_A10_PIT_TIMER_BASE_END:
         index = offset & 0xf0;
@@ -208,7 +219,7 @@ static void a10_pit_timer_cb(void *opaque)
             ptimer_stop(s->timer[i]);
             s->control[i] &= ~AW_A10_PIT_TIMER_EN;
         }
-        qemu_irq_pulse(s->irq[i]);
+        a10_pit_update_irq(s);
     }
 }
 
-- 
1.7.10.4

[Qemu-devel] [PATCH 5/7] allwinner-a10-pit: implement prescaler and source selection

[Beniamino Galvani]

This implements the prescaler and source fields of the timer control
register as described in the A10 user manual.

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 hw/timer/allwinner-a10-pit.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index 4723b25..f2f2567 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -79,6 +79,23 @@ static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
     return 0;
 }
 
+static void a10_pit_set_freq(AwA10PITState *s, int index)
+{
+    uint32_t prescaler, source;
+    uint32_t source_freqs[] = {32768, 24000000};
+
+    prescaler = 1 << extract32(s->control[index], 4, 3);
+    source = extract32(s->control[index], 2, 2);
+
+    if (source > 1) {
+        qemu_log_mask(LOG_UNIMP, "%s: unimplemented clock source %d",
+                      __func__, source);
+        source = 0;
+    }
+
+    ptimer_set_freq(s->timer[index], source_freqs[source] / prescaler);
+}
+
 static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
                             unsigned size)
 {
@@ -101,6 +118,7 @@ static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
         switch (offset & 0x0f) {
         case AW_A10_PIT_TIMER_CONTROL:
             s->control[index] = value;
+            a10_pit_set_freq(s, index);
             if (s->control[index] & AW_A10_PIT_TIMER_RELOAD) {
                 ptimer_set_count(s->timer[index], s->interval[index]);
             }
@@ -244,7 +262,6 @@ static void a10_pit_init(Object *obj)
         tc->index = i;
         bh[i] = qemu_bh_new(a10_pit_timer_cb, tc);
         s->timer[i] = ptimer_init(bh[i]);
-        ptimer_set_freq(s->timer[i], 240000);
     }
 }
 
-- 
1.7.10.4

[Qemu-devel] [PATCH 6/7] allwinner-emac: set autonegotiation complete bit on link up

[Beniamino Galvani]

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 hw/net/allwinner_emac.c         |    4 ++--
 include/hw/net/allwinner_emac.h |    1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/net/allwinner_emac.c b/hw/net/allwinner_emac.c
index 469f2f0..91931ac 100644
--- a/hw/net/allwinner_emac.c
+++ b/hw/net/allwinner_emac.c
@@ -27,11 +27,11 @@ static uint8_t padding[60];
 static void mii_set_link(RTL8201CPState *mii, bool link_ok)
 {
     if (link_ok) {
-        mii->bmsr |= MII_BMSR_LINK_ST;
+        mii->bmsr |= MII_BMSR_LINK_ST | MII_BMSR_AN_COMP;
         mii->anlpar |= MII_ANAR_TXFD | MII_ANAR_10FD | MII_ANAR_10 |
                        MII_ANAR_CSMACD;
     } else {
-        mii->bmsr &= ~MII_BMSR_LINK_ST;
+        mii->bmsr &= ~(MII_BMSR_LINK_ST | MII_BMSR_AN_COMP);
         mii->anlpar = MII_ANAR_TX;
     }
 }
diff --git a/include/hw/net/allwinner_emac.h b/include/hw/net/allwinner_emac.h
index a5e944a..5ae7717 100644
--- a/include/hw/net/allwinner_emac.h
+++ b/include/hw/net/allwinner_emac.h
@@ -144,6 +144,7 @@
 #define MII_BMSR_10T_FD     (1 << 12)
 #define MII_BMSR_10T_HD     (1 << 11)
 #define MII_BMSR_MFPS       (1 << 6)
+#define MII_BMSR_AN_COMP    (1 << 5)
 #define MII_BMSR_AUTONEG    (1 << 3)
 #define MII_BMSR_LINK_ST    (1 << 2)
 
-- 
1.7.10.4

[Qemu-devel] [PATCH 7/7] allwinner-emac: update irq status after writes to interrupt registers

[Beniamino Galvani]

The irq line status must be updated after writes to the INT_CTL and
INT_STA registers.

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 hw/net/allwinner_emac.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/net/allwinner_emac.c b/hw/net/allwinner_emac.c
index 91931ac..d780ba0 100644
--- a/hw/net/allwinner_emac.c
+++ b/hw/net/allwinner_emac.c
@@ -391,9 +391,11 @@ static void aw_emac_write(void *opaque, hwaddr offset, uint64_t value,
         break;
     case EMAC_INT_CTL_REG:
         s->int_ctl = value;
+        aw_emac_update_irq(s);
         break;
     case EMAC_INT_STA_REG:
         s->int_sta &= ~value;
+        aw_emac_update_irq(s);
         break;
     case EMAC_MAC_MADR_REG:
         s->phy_target = value;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/7] allwinner-a10-pic: set vector address when an interrupt is pending

[Li Guang]

Hi,

Beniamino Galvani wrote:
> This patch implements proper updating of the vector register which
> should hold, according to the A10 user manual, the vector address for
> the interrupt currently active on the CPU IRQ input.
>
> Interrupt priority is not implemented at the moment and thus the first
> pending interrupt is returned.
>
> Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
> ---
>   hw/intc/allwinner-a10-pic.c |   11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> index 407d563..bb2351f 100644
> --- a/hw/intc/allwinner-a10-pic.c
> +++ b/hw/intc/allwinner-a10-pic.c
> @@ -23,11 +23,20 @@
>   static void aw_a10_pic_update(AwA10PICState *s)
>   {
>       uint8_t i;
> -    int irq = 0, fiq = 0;
> +    int irq = 0, fiq = 0, pending;
> +
> +    s->vector = 0;
>
>       for (i = 0; i<  AW_A10_PIC_REG_NUM; i++) {
>           irq |= s->irq_pending[i]&  ~s->mask[i];
>           fiq |= s->select[i]&  s->irq_pending[i]&  ~s->mask[i];
> +
> +        if (!s->vector) {
> +            pending = ffs(s->irq_pending[i]&  ~s->mask[i]);
> +            if (pending) {
> +                s->vector = (i * 32 + pending - 1) * 4;
>    

this maybe should determined also by interrupt priority,
and you should also remove s->vector assignment at register write phase.

Thanks!
> +            }
> +        }
>       }
>
>       qemu_set_irq(s->parent_irq, !!irq);
>    

Re: [Qemu-devel] [PATCH 2/7] allwinner-a10-pic: fix interrupt clear behaviour

[Li Guang]

pending registers are also clear registers by a10 datasheet,
also you found bits are marked as 'R', so, ..., contradict itself.

Beniamino Galvani wrote:
> According to this mail thread [1], writing to pending register seems
> to have no effect on actual pending status of interrupts. This means
> that the only way to clear a pending interrupt is to clear the
> interrupt source. This patch implements such behaviour.
>
> [1] http://lkml.org/lkml/2013/7/6/59
>
> Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
> ---
>   hw/intc/allwinner-a10-pic.c |    6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> index bb2351f..afd57ef 100644
> --- a/hw/intc/allwinner-a10-pic.c
> +++ b/hw/intc/allwinner-a10-pic.c
> @@ -49,6 +49,8 @@ static void aw_a10_pic_set_irq(void *opaque, int irq, int level)
>
>       if (level) {
>           set_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
> +    } else {
> +        clear_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
>       }
>       aw_a10_pic_update(s);
>   }
> @@ -105,10 +107,10 @@ static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value,
>           s->nmi = value;
>           break;
>       case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8:
> -        s->irq_pending[index]&= ~value;
> +        /* Nothing to do */
>           break;
>       case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
> -        s->fiq_pending[index]&= ~value;
> +        /* Ditto */
>           break;
>       case AW_A10_PIC_SELECT ... AW_A10_PIC_SELECT + 8:
>           s->select[index] = value;
>    

Re: [Qemu-devel] [PATCH 4/7] allwinner-a10-pit: use level triggered interrupts

[Li Guang]

Beniamino Galvani wrote:
> Converts the interrupt generation logic to the use of level triggered
> interrupts.
>
>    

any real difference, or block something?

> Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
> ---
>   hw/timer/allwinner-a10-pit.c |   13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> index 3e1c183..4723b25 100644
> --- a/hw/timer/allwinner-a10-pit.c
> +++ b/hw/timer/allwinner-a10-pit.c
> @@ -24,6 +24,15 @@ typedef struct TimerContext {
>       int index;
>   } TimerContext;
>
> +static void a10_pit_update_irq(AwA10PITState *s)
> +{
> +    int i;
> +
> +    for (i = 0; i<  AW_A10_PIT_TIMER_NR; i++) {
> +        qemu_set_irq(s->irq[i], s->irq_status&  s->irq_enable&  (1<<  i));
> +    }
> +}
> +
>   static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
>   {
>       AwA10PITState *s = AW_A10_PIT(opaque);
> @@ -79,9 +88,11 @@ static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
>       switch (offset) {
>       case AW_A10_PIT_TIMER_IRQ_EN:
>           s->irq_enable = value;
> +        a10_pit_update_irq(s);
>           break;
>       case AW_A10_PIT_TIMER_IRQ_ST:
>           s->irq_status&= ~value;
> +        a10_pit_update_irq(s);
>           break;
>       case AW_A10_PIT_TIMER_BASE ... AW_A10_PIT_TIMER_BASE_END:
>           index = offset&  0xf0;
> @@ -208,7 +219,7 @@ static void a10_pit_timer_cb(void *opaque)
>               ptimer_stop(s->timer[i]);
>               s->control[i]&= ~AW_A10_PIT_TIMER_EN;
>           }
> -        qemu_irq_pulse(s->irq[i]);
> +        a10_pit_update_irq(s);
>       }
>   }
>
>    

Re: [Qemu-devel] [PATCH 3/7] allwinner-a10-pit: avoid generation of spurious interrupts

[Li Guang]

did you detect spurious?
didn't by my limited test,
code will disable any expired timer, unless be re-armed,
so it will never generate interrupt any more.

Thanks!

Beniamino Galvani wrote:
> The model was generating interrupts for all enabled timers after the
> expiration of one of them. Avoid this by passing to the timer callback
> function a structure containing the index of the expired timer.
>
> Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
> ---
>   hw/timer/allwinner-a10-pit.c |   30 +++++++++++++++++++-----------
>   1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> index b27fce8..3e1c183 100644
> --- a/hw/timer/allwinner-a10-pit.c
> +++ b/hw/timer/allwinner-a10-pit.c
> @@ -19,6 +19,11 @@
>   #include "sysemu/sysemu.h"
>   #include "hw/timer/allwinner-a10-pit.h"
>
> +typedef struct TimerContext {
> +    AwA10PITState *state;
> +    int index;
> +} TimerContext;
> +
>   static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
>   {
>       AwA10PITState *s = AW_A10_PIT(opaque);
> @@ -193,18 +198,17 @@ static void a10_pit_reset(DeviceState *dev)
>
>   static void a10_pit_timer_cb(void *opaque)
>   {
> -    AwA10PITState *s = AW_A10_PIT(opaque);
> -    uint8_t i;
> +    TimerContext *tc = opaque;
> +    AwA10PITState *s = tc->state;
> +    uint8_t i = tc->index;
>
> -    for (i = 0; i<  AW_A10_PIT_TIMER_NR; i++) {
> -        if (s->control[i]&  AW_A10_PIT_TIMER_EN) {
> -            s->irq_status |= 1<<  i;
> -            if (s->control[i]&  AW_A10_PIT_TIMER_MODE) {
> -                ptimer_stop(s->timer[i]);
> -                s->control[i]&= ~AW_A10_PIT_TIMER_EN;
> -            }
> -            qemu_irq_pulse(s->irq[i]);
> +    if (s->control[i]&  AW_A10_PIT_TIMER_EN) {
> +        s->irq_status |= 1<<  i;
> +        if (s->control[i]&  AW_A10_PIT_TIMER_MODE) {
> +            ptimer_stop(s->timer[i]);
> +            s->control[i]&= ~AW_A10_PIT_TIMER_EN;
>           }
> +        qemu_irq_pulse(s->irq[i]);
>       }
>   }
>
> @@ -213,6 +217,7 @@ static void a10_pit_init(Object *obj)
>       AwA10PITState *s = AW_A10_PIT(obj);
>       SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>       QEMUBH * bh[AW_A10_PIT_TIMER_NR];
> +    TimerContext *tc;
>       uint8_t i;
>
>       for (i = 0; i<  AW_A10_PIT_TIMER_NR; i++) {
> @@ -223,7 +228,10 @@ static void a10_pit_init(Object *obj)
>       sysbus_init_mmio(sbd,&s->iomem);
>
>       for (i = 0; i<  AW_A10_PIT_TIMER_NR; i++) {
> -        bh[i] = qemu_bh_new(a10_pit_timer_cb, s);
> +        tc = g_malloc(sizeof(TimerContext));
> +        tc->state = s;
> +        tc->index = i;
> +        bh[i] = qemu_bh_new(a10_pit_timer_cb, tc);
>           s->timer[i] = ptimer_init(bh[i]);
>           ptimer_set_freq(s->timer[i], 240000);
>       }
>    

Re: [Qemu-devel] [PATCH 1/7] allwinner-a10-pic: set vector address when an interrupt is pending

[Beniamino Galvani]

On Tue, Feb 18, 2014 at 11:27:11AM +0800, Li Guang wrote:
> Hi,
> 
> Beniamino Galvani wrote:
> >This patch implements proper updating of the vector register which
> >should hold, according to the A10 user manual, the vector address for
> >the interrupt currently active on the CPU IRQ input.
> >
> >Interrupt priority is not implemented at the moment and thus the first
> >pending interrupt is returned.
> >
> >Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
> >---
> >  hw/intc/allwinner-a10-pic.c |   11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> >diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> >index 407d563..bb2351f 100644
> >--- a/hw/intc/allwinner-a10-pic.c
> >+++ b/hw/intc/allwinner-a10-pic.c
> >@@ -23,11 +23,20 @@
> >  static void aw_a10_pic_update(AwA10PICState *s)
> >  {
> >      uint8_t i;
> >-    int irq = 0, fiq = 0;
> >+    int irq = 0, fiq = 0, pending;
> >+
> >+    s->vector = 0;
> >
> >      for (i = 0; i<  AW_A10_PIC_REG_NUM; i++) {
> >          irq |= s->irq_pending[i]&  ~s->mask[i];
> >          fiq |= s->select[i]&  s->irq_pending[i]&  ~s->mask[i];
> >+
> >+        if (!s->vector) {
> >+            pending = ffs(s->irq_pending[i]&  ~s->mask[i]);
> >+            if (pending) {
> >+                s->vector = (i * 32 + pending - 1) * 4;
> 
> this maybe should determined also by interrupt priority,

We can add interrupt priority logic later if it's needed, but at the
moment I don't think it's used by Linux.

> and you should also remove s->vector assignment at register write phase.

You're right, the register is read-only; I will remove it from the
write function.

Beniamino

Re: [Qemu-devel] [PATCH 2/7] allwinner-a10-pic: fix interrupt clear behaviour

[Beniamino Galvani]

On Tue, Feb 18, 2014 at 11:49:51AM +0800, Li Guang wrote:
> Beniamino Galvani wrote:
> >According to this mail thread [1], writing to pending register seems
> >to have no effect on actual pending status of interrupts. This means
> >that the only way to clear a pending interrupt is to clear the
> >interrupt source. This patch implements such behaviour.
> >
> >[1] http://lkml.org/lkml/2013/7/6/59
> >
> >Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
> >---
> >  hw/intc/allwinner-a10-pic.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> >diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> >index bb2351f..afd57ef 100644
> >--- a/hw/intc/allwinner-a10-pic.c
> >+++ b/hw/intc/allwinner-a10-pic.c
> >@@ -49,6 +49,8 @@ static void aw_a10_pic_set_irq(void *opaque, int irq, int level)
> >
> >      if (level) {
> >          set_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
> >+    } else {
> >+        clear_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
> >      }
> >      aw_a10_pic_update(s);
> >  }
> >@@ -105,10 +107,10 @@ static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value,
> >          s->nmi = value;
> >          break;
> >      case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8:
> >-        s->irq_pending[index]&= ~value;
> >+        /* Nothing to do */
> >          break;
> >      case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
> >-        s->fiq_pending[index]&= ~value;
> >+        /* Ditto */
> >          break;
> >      case AW_A10_PIC_SELECT ... AW_A10_PIC_SELECT + 8:
> >          s->select[index] = value;
> 
> pending registers are also clear registers by a10 datasheet,
> also you found bits are marked as 'R', so, ..., contradict itself.

Yes, the datasheet is inconsistent about this because the register
can't be read-only and 'clear' at the same time.

Unfortunately at the moment I cannot test if the clearing
functionality of the pending register works on real hardware but the
idea I got from the linked discussion is that it's either not
implemented or broken and therefore interrupts remain pending until
they are disabled at the source.

Do you have a chance to try it on a real board?

Beniamino

Re: [Qemu-devel] [PATCH 3/7] allwinner-a10-pit: avoid generation of spurious interrupts

[Beniamino Galvani]

On Tue, Feb 18, 2014 at 12:17:04PM +0800, Li Guang wrote:
> Beniamino Galvani wrote:
> >The model was generating interrupts for all enabled timers after the
> >expiration of one of them. Avoid this by passing to the timer callback
> >function a structure containing the index of the expired timer.
> >
> did you detect spurious?
> didn't by my limited test,
> code will disable any expired timer, unless be re-armed,
> so it will never generate interrupt any more.

Yes, when both timer0 and timer1 were enabled and timer0 expired, the
previous implementation raised two interrupts because the callback
function iterated over the array of timers. Instead it should consider
only the timer that generated the event.

Beniamino

> >Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
> >---
> >  hw/timer/allwinner-a10-pit.c |   30 +++++++++++++++++++-----------
> >  1 file changed, 19 insertions(+), 11 deletions(-)
> >
> >diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> >index b27fce8..3e1c183 100644
> >--- a/hw/timer/allwinner-a10-pit.c
> >+++ b/hw/timer/allwinner-a10-pit.c
> >@@ -19,6 +19,11 @@
> >  #include "sysemu/sysemu.h"
> >  #include "hw/timer/allwinner-a10-pit.h"
> >
> >+typedef struct TimerContext {
> >+    AwA10PITState *state;
> >+    int index;
> >+} TimerContext;
> >+
> >  static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
> >  {
> >      AwA10PITState *s = AW_A10_PIT(opaque);
> >@@ -193,18 +198,17 @@ static void a10_pit_reset(DeviceState *dev)
> >
> >  static void a10_pit_timer_cb(void *opaque)
> >  {
> >-    AwA10PITState *s = AW_A10_PIT(opaque);
> >-    uint8_t i;
> >+    TimerContext *tc = opaque;
> >+    AwA10PITState *s = tc->state;
> >+    uint8_t i = tc->index;
> >
> >-    for (i = 0; i<  AW_A10_PIT_TIMER_NR; i++) {
> >-        if (s->control[i]&  AW_A10_PIT_TIMER_EN) {
> >-            s->irq_status |= 1<<  i;
> >-            if (s->control[i]&  AW_A10_PIT_TIMER_MODE) {
> >-                ptimer_stop(s->timer[i]);
> >-                s->control[i]&= ~AW_A10_PIT_TIMER_EN;
> >-            }
> >-            qemu_irq_pulse(s->irq[i]);
> >+    if (s->control[i]&  AW_A10_PIT_TIMER_EN) {
> >+        s->irq_status |= 1<<  i;
> >+        if (s->control[i]&  AW_A10_PIT_TIMER_MODE) {
> >+            ptimer_stop(s->timer[i]);
> >+            s->control[i]&= ~AW_A10_PIT_TIMER_EN;
> >          }
> >+        qemu_irq_pulse(s->irq[i]);
> >      }
> >  }
> >
> >@@ -213,6 +217,7 @@ static void a10_pit_init(Object *obj)
> >      AwA10PITState *s = AW_A10_PIT(obj);
> >      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> >      QEMUBH * bh[AW_A10_PIT_TIMER_NR];
> >+    TimerContext *tc;
> >      uint8_t i;
> >
> >      for (i = 0; i<  AW_A10_PIT_TIMER_NR; i++) {
> >@@ -223,7 +228,10 @@ static void a10_pit_init(Object *obj)
> >      sysbus_init_mmio(sbd,&s->iomem);
> >
> >      for (i = 0; i<  AW_A10_PIT_TIMER_NR; i++) {
> >-        bh[i] = qemu_bh_new(a10_pit_timer_cb, s);
> >+        tc = g_malloc(sizeof(TimerContext));
> >+        tc->state = s;
> >+        tc->index = i;
> >+        bh[i] = qemu_bh_new(a10_pit_timer_cb, tc);
> >          s->timer[i] = ptimer_init(bh[i]);
> >          ptimer_set_freq(s->timer[i], 240000);
> >      }
> 

Re: [Qemu-devel] [PATCH 4/7] allwinner-a10-pit: use level triggered interrupts

[Beniamino Galvani]

On Tue, Feb 18, 2014 at 11:51:42AM +0800, Li Guang wrote:
> Beniamino Galvani wrote:
> >Converts the interrupt generation logic to the use of level triggered
> >interrupts.
> 
> any real difference, or block something?

This is a consequence of the change to the implementation of pending
register of the interrupt controller in patch 2.

Beniamino

> 
> >Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
> >---
> >  hw/timer/allwinner-a10-pit.c |   13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> >diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> >index 3e1c183..4723b25 100644
> >--- a/hw/timer/allwinner-a10-pit.c
> >+++ b/hw/timer/allwinner-a10-pit.c
> >@@ -24,6 +24,15 @@ typedef struct TimerContext {
> >      int index;
> >  } TimerContext;
> >
> >+static void a10_pit_update_irq(AwA10PITState *s)
> >+{
> >+    int i;
> >+
> >+    for (i = 0; i<  AW_A10_PIT_TIMER_NR; i++) {
> >+        qemu_set_irq(s->irq[i], s->irq_status&  s->irq_enable&  (1<<  i));
> >+    }
> >+}
> >+
> >  static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
> >  {
> >      AwA10PITState *s = AW_A10_PIT(opaque);
> >@@ -79,9 +88,11 @@ static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
> >      switch (offset) {
> >      case AW_A10_PIT_TIMER_IRQ_EN:
> >          s->irq_enable = value;
> >+        a10_pit_update_irq(s);
> >          break;
> >      case AW_A10_PIT_TIMER_IRQ_ST:
> >          s->irq_status&= ~value;
> >+        a10_pit_update_irq(s);
> >          break;
> >      case AW_A10_PIT_TIMER_BASE ... AW_A10_PIT_TIMER_BASE_END:
> >          index = offset&  0xf0;
> >@@ -208,7 +219,7 @@ static void a10_pit_timer_cb(void *opaque)
> >              ptimer_stop(s->timer[i]);
> >              s->control[i]&= ~AW_A10_PIT_TIMER_EN;
> >          }
> >-        qemu_irq_pulse(s->irq[i]);
> >+        a10_pit_update_irq(s);
> >      }
> >  }
> >
> 

Re: [Qemu-devel] [PATCH 3/7] allwinner-a10-pit: avoid generation of spurious interrupts

[Li Guang]

Beniamino Galvani wrote:
> On Tue, Feb 18, 2014 at 12:17:04PM +0800, Li Guang wrote:
>    
>> Beniamino Galvani wrote:
>>      
>>> The model was generating interrupts for all enabled timers after the
>>> expiration of one of them. Avoid this by passing to the timer callback
>>> function a structure containing the index of the expired timer.
>>>
>>>        
>> did you detect spurious?
>> didn't by my limited test,
>> code will disable any expired timer, unless be re-armed,
>> so it will never generate interrupt any more.
>>      
> Yes, when both timer0 and timer1 were enabled and timer0 expired, the
> previous implementation raised two interrupts because the callback
> function iterated over the array of timers. Instead it should consider
> only the timer that generated the event.
>
>    

Ok, I like it this if it really can eliminate spurious interrupts.

Thanks!

>    
>>> Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
>>> ---
>>>   hw/timer/allwinner-a10-pit.c |   30 +++++++++++++++++++-----------
>>>   1 file changed, 19 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
>>> index b27fce8..3e1c183 100644
>>> --- a/hw/timer/allwinner-a10-pit.c
>>> +++ b/hw/timer/allwinner-a10-pit.c
>>> @@ -19,6 +19,11 @@
>>>   #include "sysemu/sysemu.h"
>>>   #include "hw/timer/allwinner-a10-pit.h"
>>>
>>> +typedef struct TimerContext {
>>> +    AwA10PITState *state;
>>> +    int index;
>>> +} TimerContext;
>>> +
>>>   static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
>>>   {
>>>       AwA10PITState *s = AW_A10_PIT(opaque);
>>> @@ -193,18 +198,17 @@ static void a10_pit_reset(DeviceState *dev)
>>>
>>>   static void a10_pit_timer_cb(void *opaque)
>>>   {
>>> -    AwA10PITState *s = AW_A10_PIT(opaque);
>>> -    uint8_t i;
>>> +    TimerContext *tc = opaque;
>>> +    AwA10PITState *s = tc->state;
>>> +    uint8_t i = tc->index;
>>>
>>> -    for (i = 0; i<   AW_A10_PIT_TIMER_NR; i++) {
>>> -        if (s->control[i]&   AW_A10_PIT_TIMER_EN) {
>>> -            s->irq_status |= 1<<   i;
>>> -            if (s->control[i]&   AW_A10_PIT_TIMER_MODE) {
>>> -                ptimer_stop(s->timer[i]);
>>> -                s->control[i]&= ~AW_A10_PIT_TIMER_EN;
>>> -            }
>>> -            qemu_irq_pulse(s->irq[i]);
>>> +    if (s->control[i]&   AW_A10_PIT_TIMER_EN) {
>>> +        s->irq_status |= 1<<   i;
>>> +        if (s->control[i]&   AW_A10_PIT_TIMER_MODE) {
>>> +            ptimer_stop(s->timer[i]);
>>> +            s->control[i]&= ~AW_A10_PIT_TIMER_EN;
>>>           }
>>> +        qemu_irq_pulse(s->irq[i]);
>>>       }
>>>   }
>>>
>>> @@ -213,6 +217,7 @@ static void a10_pit_init(Object *obj)
>>>       AwA10PITState *s = AW_A10_PIT(obj);
>>>       SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>>>       QEMUBH * bh[AW_A10_PIT_TIMER_NR];
>>> +    TimerContext *tc;
>>>       uint8_t i;
>>>
>>>       for (i = 0; i<   AW_A10_PIT_TIMER_NR; i++) {
>>> @@ -223,7 +228,10 @@ static void a10_pit_init(Object *obj)
>>>       sysbus_init_mmio(sbd,&s->iomem);
>>>
>>>       for (i = 0; i<   AW_A10_PIT_TIMER_NR; i++) {
>>> -        bh[i] = qemu_bh_new(a10_pit_timer_cb, s);
>>> +        tc = g_malloc(sizeof(TimerContext));
>>> +        tc->state = s;
>>> +        tc->index = i;
>>> +        bh[i] = qemu_bh_new(a10_pit_timer_cb, tc);
>>>           s->timer[i] = ptimer_init(bh[i]);
>>>           ptimer_set_freq(s->timer[i], 240000);
>>>       }
>>>        
>>      
>
>    

Re: [Qemu-devel] [PATCH 2/7] allwinner-a10-pic: fix interrupt clear behaviour

[Li Guang]

Beniamino Galvani wrote:
> On Tue, Feb 18, 2014 at 11:49:51AM +0800, Li Guang wrote:
>    
>> Beniamino Galvani wrote:
>>      
>>> According to this mail thread [1], writing to pending register seems
>>> to have no effect on actual pending status of interrupts. This means
>>> that the only way to clear a pending interrupt is to clear the
>>> interrupt source. This patch implements such behaviour.
>>>
>>> [1] http://lkml.org/lkml/2013/7/6/59
>>>
>>> Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
>>> ---
>>>   hw/intc/allwinner-a10-pic.c |    6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
>>> index bb2351f..afd57ef 100644
>>> --- a/hw/intc/allwinner-a10-pic.c
>>> +++ b/hw/intc/allwinner-a10-pic.c
>>> @@ -49,6 +49,8 @@ static void aw_a10_pic_set_irq(void *opaque, int irq, int level)
>>>
>>>       if (level) {
>>>           set_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
>>> +    } else {
>>> +        clear_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
>>>       }
>>>       aw_a10_pic_update(s);
>>>   }
>>> @@ -105,10 +107,10 @@ static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value,
>>>           s->nmi = value;
>>>           break;
>>>       case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8:
>>> -        s->irq_pending[index]&= ~value;
>>> +        /* Nothing to do */
>>>           break;
>>>       case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
>>> -        s->fiq_pending[index]&= ~value;
>>> +        /* Ditto */
>>>           break;
>>>       case AW_A10_PIC_SELECT ... AW_A10_PIC_SELECT + 8:
>>>           s->select[index] = value;
>>>        
>> pending registers are also clear registers by a10 datasheet,
>> also you found bits are marked as 'R', so, ..., contradict itself.
>>      
> Yes, the datasheet is inconsistent about this because the register
> can't be read-only and 'clear' at the same time.
>
> Unfortunately at the moment I cannot test if the clearing
> functionality of the pending register works on real hardware but the
> idea I got from the linked discussion is that it's either not
> implemented or broken and therefore interrupts remain pending until
> they are disabled at the source.
>
> Do you have a chance to try it on a real board?
>
>    
Ah? even kernel code from allwinner wrote pending registers
to clear pending interrupt, didn't you see it?
so should be no doubt that these registers are writable.

Thanks!

Re: [Qemu-devel] [PATCH 2/7] allwinner-a10-pic: fix interrupt clear behaviour

[Beniamino Galvani]

On Wed, Feb 19, 2014 at 10:02:36AM +0800, Li Guang wrote:
> Beniamino Galvani wrote:
> >On Tue, Feb 18, 2014 at 11:49:51AM +0800, Li Guang wrote:
> >>Beniamino Galvani wrote:
> >>>According to this mail thread [1], writing to pending register seems
> >>>to have no effect on actual pending status of interrupts. This means
> >>>that the only way to clear a pending interrupt is to clear the
> >>>interrupt source. This patch implements such behaviour.
> >>>
> >>>[1] http://lkml.org/lkml/2013/7/6/59
> >>>
> >>>Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
> >>>---
> >>>  hw/intc/allwinner-a10-pic.c |    6 ++++--
> >>>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>>diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> >>>index bb2351f..afd57ef 100644
> >>>--- a/hw/intc/allwinner-a10-pic.c
> >>>+++ b/hw/intc/allwinner-a10-pic.c
> >>>@@ -49,6 +49,8 @@ static void aw_a10_pic_set_irq(void *opaque, int irq, int level)
> >>>
> >>>      if (level) {
> >>>          set_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
> >>>+    } else {
> >>>+        clear_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
> >>>      }
> >>>      aw_a10_pic_update(s);
> >>>  }
> >>>@@ -105,10 +107,10 @@ static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value,
> >>>          s->nmi = value;
> >>>          break;
> >>>      case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8:
> >>>-        s->irq_pending[index]&= ~value;
> >>>+        /* Nothing to do */
> >>>          break;
> >>>      case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
> >>>-        s->fiq_pending[index]&= ~value;
> >>>+        /* Ditto */
> >>>          break;
> >>>      case AW_A10_PIC_SELECT ... AW_A10_PIC_SELECT + 8:
> >>>          s->select[index] = value;
> >>pending registers are also clear registers by a10 datasheet,
> >>also you found bits are marked as 'R', so, ..., contradict itself.
> >Yes, the datasheet is inconsistent about this because the register
> >can't be read-only and 'clear' at the same time.
> >
> >Unfortunately at the moment I cannot test if the clearing
> >functionality of the pending register works on real hardware but the
> >idea I got from the linked discussion is that it's either not
> >implemented or broken and therefore interrupts remain pending until
> >they are disabled at the source.
> >
> >Do you have a chance to try it on a real board?
> >
> Ah? even kernel code from allwinner wrote pending registers
> to clear pending interrupt, didn't you see it?
> so should be no doubt that these registers are writable.

Well, if you look closely at that code, it's a bit strange:

void sw_irq_ack(struct irq_data *irqd)
{
    unsigned int irq = irqd->irq;

    [...]
    writel(readl(SW_INT_IRQ_PENDING_REG0) | (1<<irq), SW_INT_IRQ_PENDING_REG0);
    [...]
}

In order to clear a single interrupt, it is or'ing the irq bit with
the previous value of the register, so it's basically clearing all
pending interrupts. 

This is discussed in the mentioned thread from lkml and the final
explanation is that the pending register is always updated with the
actual status of irq lines, even if you write to it. In other words,
writes to the register are ignored, otherwise the code above would
produce random loss of interrupts.

Beniamino

Re: [Qemu-devel] [PATCH 2/7] allwinner-a10-pic: fix interrupt clear behaviour

[Li Guang]

Beniamino Galvani wrote:
> On Wed, Feb 19, 2014 at 10:02:36AM +0800, Li Guang wrote:
>    
>> Beniamino Galvani wrote:
>>      
>>> On Tue, Feb 18, 2014 at 11:49:51AM +0800, Li Guang wrote:
>>>        
>>>> Beniamino Galvani wrote:
>>>>          
>>>>> According to this mail thread [1], writing to pending register seems
>>>>> to have no effect on actual pending status of interrupts. This means
>>>>> that the only way to clear a pending interrupt is to clear the
>>>>> interrupt source. This patch implements such behaviour.
>>>>>
>>>>> [1] http://lkml.org/lkml/2013/7/6/59
>>>>>
>>>>> Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
>>>>> ---
>>>>>   hw/intc/allwinner-a10-pic.c |    6 ++++--
>>>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
>>>>> index bb2351f..afd57ef 100644
>>>>> --- a/hw/intc/allwinner-a10-pic.c
>>>>> +++ b/hw/intc/allwinner-a10-pic.c
>>>>> @@ -49,6 +49,8 @@ static void aw_a10_pic_set_irq(void *opaque, int irq, int level)
>>>>>
>>>>>       if (level) {
>>>>>           set_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
>>>>> +    } else {
>>>>> +        clear_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
>>>>>       }
>>>>>       aw_a10_pic_update(s);
>>>>>   }
>>>>> @@ -105,10 +107,10 @@ static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value,
>>>>>           s->nmi = value;
>>>>>           break;
>>>>>       case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8:
>>>>> -        s->irq_pending[index]&= ~value;
>>>>> +        /* Nothing to do */
>>>>>           break;
>>>>>       case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
>>>>> -        s->fiq_pending[index]&= ~value;
>>>>> +        /* Ditto */
>>>>>           break;
>>>>>       case AW_A10_PIC_SELECT ... AW_A10_PIC_SELECT + 8:
>>>>>           s->select[index] = value;
>>>>>            
>>>> pending registers are also clear registers by a10 datasheet,
>>>> also you found bits are marked as 'R', so, ..., contradict itself.
>>>>          
>>> Yes, the datasheet is inconsistent about this because the register
>>> can't be read-only and 'clear' at the same time.
>>>
>>> Unfortunately at the moment I cannot test if the clearing
>>> functionality of the pending register works on real hardware but the
>>> idea I got from the linked discussion is that it's either not
>>> implemented or broken and therefore interrupts remain pending until
>>> they are disabled at the source.
>>>
>>> Do you have a chance to try it on a real board?
>>>
>>>        
>> Ah? even kernel code from allwinner wrote pending registers
>> to clear pending interrupt, didn't you see it?
>> so should be no doubt that these registers are writable.
>>      
> Well, if you look closely at that code, it's a bit strange:
>
> void sw_irq_ack(struct irq_data *irqd)
> {
>      unsigned int irq = irqd->irq;
>
>      [...]
>      writel(readl(SW_INT_IRQ_PENDING_REG0) | (1<<irq), SW_INT_IRQ_PENDING_REG0);
>      [...]
> }
>
> In order to clear a single interrupt, it is or'ing the irq bit with
> the previous value of the register, so it's basically clearing all
> pending interrupts.
>
> This is discussed in the mentioned thread from lkml and the final
> explanation is that the pending register is always updated with the
> actual status of irq lines, even if you write to it. In other words,
> writes to the register are ignored, otherwise the code above would
> produce random loss of interrupts.
>
>    

Hmm..., sorry,  I also can't test this operation on A10 board now,
but why not they just wipe out these writings(kernel 3.12)?

Re: [Qemu-devel] [PATCH 2/7] allwinner-a10-pic: fix interrupt clear behaviour

[Beniamino Galvani]

On Mon, Feb 24, 2014 at 02:45:06PM +0800, Li Guang wrote:
> Beniamino Galvani wrote:
> >On Wed, Feb 19, 2014 at 10:02:36AM +0800, Li Guang wrote:
> >>Beniamino Galvani wrote:
> >>>On Tue, Feb 18, 2014 at 11:49:51AM +0800, Li Guang wrote:
> >>>>Beniamino Galvani wrote:
> >>>>>According to this mail thread [1], writing to pending register seems
> >>>>>to have no effect on actual pending status of interrupts. This means
> >>>>>that the only way to clear a pending interrupt is to clear the
> >>>>>interrupt source. This patch implements such behaviour.
> >>>>>
> >>>>>[1] http://lkml.org/lkml/2013/7/6/59
> >>>>>
> >>>>>Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
> >>>>>---
> >>>>>  hw/intc/allwinner-a10-pic.c |    6 ++++--
> >>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>>>>
> >>>>>diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> >>>>>index bb2351f..afd57ef 100644
> >>>>>--- a/hw/intc/allwinner-a10-pic.c
> >>>>>+++ b/hw/intc/allwinner-a10-pic.c
> >>>>>@@ -49,6 +49,8 @@ static void aw_a10_pic_set_irq(void *opaque, int irq, int level)
> >>>>>
> >>>>>      if (level) {
> >>>>>          set_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
> >>>>>+    } else {
> >>>>>+        clear_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
> >>>>>      }
> >>>>>      aw_a10_pic_update(s);
> >>>>>  }
> >>>>>@@ -105,10 +107,10 @@ static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value,
> >>>>>          s->nmi = value;
> >>>>>          break;
> >>>>>      case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8:
> >>>>>-        s->irq_pending[index]&= ~value;
> >>>>>+        /* Nothing to do */
> >>>>>          break;
> >>>>>      case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
> >>>>>-        s->fiq_pending[index]&= ~value;
> >>>>>+        /* Ditto */
> >>>>>          break;
> >>>>>      case AW_A10_PIC_SELECT ... AW_A10_PIC_SELECT + 8:
> >>>>>          s->select[index] = value;
> >>>>pending registers are also clear registers by a10 datasheet,
> >>>>also you found bits are marked as 'R', so, ..., contradict itself.
> >>>Yes, the datasheet is inconsistent about this because the register
> >>>can't be read-only and 'clear' at the same time.
> >>>
> >>>Unfortunately at the moment I cannot test if the clearing
> >>>functionality of the pending register works on real hardware but the
> >>>idea I got from the linked discussion is that it's either not
> >>>implemented or broken and therefore interrupts remain pending until
> >>>they are disabled at the source.
> >>>
> >>>Do you have a chance to try it on a real board?
> >>>
> >>Ah? even kernel code from allwinner wrote pending registers
> >>to clear pending interrupt, didn't you see it?
> >>so should be no doubt that these registers are writable.
> >Well, if you look closely at that code, it's a bit strange:
> >
> >void sw_irq_ack(struct irq_data *irqd)
> >{
> >     unsigned int irq = irqd->irq;
> >
> >     [...]
> >     writel(readl(SW_INT_IRQ_PENDING_REG0) | (1<<irq), SW_INT_IRQ_PENDING_REG0);
> >     [...]
> >}
> >
> >In order to clear a single interrupt, it is or'ing the irq bit with
> >the previous value of the register, so it's basically clearing all
> >pending interrupts.
> >
> >This is discussed in the mentioned thread from lkml and the final
> >explanation is that the pending register is always updated with the
> >actual status of irq lines, even if you write to it. In other words,
> >writes to the register are ignored, otherwise the code above would
> >produce random loss of interrupts.
> >
> 
> Hmm..., sorry,  I also can't test this operation on A10 board now,
> but why not they just wipe out these writings(kernel 3.12)?

I don't know, there was a proposed patch that removed those writes in
the lkml discussion but probably it never reached mainline.

To be on the safe side I can restore the writability of the register
and then, when we will figure out how it really works, correct it if
needed.

Beniamino

Re: [Qemu-devel] [PATCH 2/7] allwinner-a10-pic: fix interrupt clear behaviour

[Li Guang]

Beniamino Galvani wrote:
> On Mon, Feb 24, 2014 at 02:45:06PM +0800, Li Guang wrote:
>    
>> Beniamino Galvani wrote:
>>      
>>> On Wed, Feb 19, 2014 at 10:02:36AM +0800, Li Guang wrote:
>>>        
>>>> Beniamino Galvani wrote:
>>>>          
>>>>> On Tue, Feb 18, 2014 at 11:49:51AM +0800, Li Guang wrote:
>>>>>            
>>>>>> Beniamino Galvani wrote:
>>>>>>              
>>>>>>> According to this mail thread [1], writing to pending register seems
>>>>>>> to have no effect on actual pending status of interrupts. This means
>>>>>>> that the only way to clear a pending interrupt is to clear the
>>>>>>> interrupt source. This patch implements such behaviour.
>>>>>>>
>>>>>>> [1] http://lkml.org/lkml/2013/7/6/59
>>>>>>>
>>>>>>> Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
>>>>>>> ---
>>>>>>>   hw/intc/allwinner-a10-pic.c |    6 ++++--
>>>>>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
>>>>>>> index bb2351f..afd57ef 100644
>>>>>>> --- a/hw/intc/allwinner-a10-pic.c
>>>>>>> +++ b/hw/intc/allwinner-a10-pic.c
>>>>>>> @@ -49,6 +49,8 @@ static void aw_a10_pic_set_irq(void *opaque, int irq, int level)
>>>>>>>
>>>>>>>       if (level) {
>>>>>>>           set_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
>>>>>>> +    } else {
>>>>>>> +        clear_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
>>>>>>>       }
>>>>>>>       aw_a10_pic_update(s);
>>>>>>>   }
>>>>>>> @@ -105,10 +107,10 @@ static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value,
>>>>>>>           s->nmi = value;
>>>>>>>           break;
>>>>>>>       case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8:
>>>>>>> -        s->irq_pending[index]&= ~value;
>>>>>>> +        /* Nothing to do */
>>>>>>>           break;
>>>>>>>       case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
>>>>>>> -        s->fiq_pending[index]&= ~value;
>>>>>>> +        /* Ditto */
>>>>>>>           break;
>>>>>>>       case AW_A10_PIC_SELECT ... AW_A10_PIC_SELECT + 8:
>>>>>>>           s->select[index] = value;
>>>>>>>                
>>>>>> pending registers are also clear registers by a10 datasheet,
>>>>>> also you found bits are marked as 'R', so, ..., contradict itself.
>>>>>>              
>>>>> Yes, the datasheet is inconsistent about this because the register
>>>>> can't be read-only and 'clear' at the same time.
>>>>>
>>>>> Unfortunately at the moment I cannot test if the clearing
>>>>> functionality of the pending register works on real hardware but the
>>>>> idea I got from the linked discussion is that it's either not
>>>>> implemented or broken and therefore interrupts remain pending until
>>>>> they are disabled at the source.
>>>>>
>>>>> Do you have a chance to try it on a real board?
>>>>>
>>>>>            
>>>> Ah? even kernel code from allwinner wrote pending registers
>>>> to clear pending interrupt, didn't you see it?
>>>> so should be no doubt that these registers are writable.
>>>>          
>>> Well, if you look closely at that code, it's a bit strange:
>>>
>>> void sw_irq_ack(struct irq_data *irqd)
>>> {
>>>      unsigned int irq = irqd->irq;
>>>
>>>      [...]
>>>      writel(readl(SW_INT_IRQ_PENDING_REG0) | (1<<irq), SW_INT_IRQ_PENDING_REG0);
>>>      [...]
>>> }
>>>
>>> In order to clear a single interrupt, it is or'ing the irq bit with
>>> the previous value of the register, so it's basically clearing all
>>> pending interrupts.
>>>
>>> This is discussed in the mentioned thread from lkml and the final
>>> explanation is that the pending register is always updated with the
>>> actual status of irq lines, even if you write to it. In other words,
>>> writes to the register are ignored, otherwise the code above would
>>> produce random loss of interrupts.
>>>
>>>        
>> Hmm..., sorry,  I also can't test this operation on A10 board now,
>> but why not they just wipe out these writings(kernel 3.12)?
>>      
> I don't know, there was a proposed patch that removed those writes in
> the lkml discussion but probably it never reached mainline.
>
> To be on the safe side I can restore the writability of the register
> and then, when we will figure out how it really works, correct it if
> needed.
>
>
>    

Agreed,

Thanks!