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

[Qemu-devel] [PATCH v2 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 concern interrupt handling, but there are also
improvements in the timer and the ethernet MAC.

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

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

Changelog:
v2: Address comments from Li Guang:
    * make pic vector register read-only
    * allow writing to pic pending register

Beniamino Galvani (7):
  allwinner-a10-pic: set vector address when an interrupt is pending
  allwinner-a10-pic: update pending register when an irq is cleared
  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     |   16 ++++++++---
 hw/net/allwinner_emac.c         |    6 ++--
 hw/timer/allwinner-a10-pit.c    |   60 +++++++++++++++++++++++++++++++--------
 include/hw/net/allwinner_emac.h |    1 +
 4 files changed, 65 insertions(+), 18 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH v2 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 |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
index 407d563..00f3c11 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);
@@ -84,9 +93,6 @@ static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value,
     uint8_t index = (offset & 0xc) / 4;
 
     switch (offset) {
-    case AW_A10_PIC_VECTOR:
-        s->vector = value & ~0x3;
-        break;
     case AW_A10_PIC_BASE_ADDR:
         s->base_addr = value & ~0x3;
     case AW_A10_PIC_PROTECT:
-- 
1.7.10.4

[Qemu-devel] [PATCH v2 2/7] allwinner-a10-pic: update pending register when an irq is cleared

[Beniamino Galvani]

The value of pending register was updated only when an irq was raised
from a device; it should also be updated when an interrupt is cleared.

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

diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
index 00f3c11..9011c82 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);
 }
-- 
1.7.10.4

[Qemu-devel] [PATCH v2 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 v2 4/7] allwinner-a10-pit: use level triggered interrupts

[Beniamino Galvani]

Convert 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 v2 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 v2 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 v2 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 v2 5/7] allwinner-a10-pit: implement prescaler and source selection

[Peter Crosthwaite]

On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> 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};
> +

These smell like external oscillator frequencies which is board level
information. Ideally, you should propertyify the input frequencies and
pass them in on the board, so others can make different boards with
with different crystals.

> +    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",

LOG_UNIMP is usually used for something that does exist but QEMU
chooses knowingly not to implement. Are there extra clock wires from
other sources not handled or are encodings 1 & 2 just unpopulated on
this board? (In which case it should be a GUEST_ERROR).

Regards,
Peter

> +                      __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
>
>

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

[Peter Crosthwaite]

On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> 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>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.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 v2 3/7] allwinner-a10-pit: avoid generation of spurious interrupts

[Peter Crosthwaite]

On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galvani@gmail.com> 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.
>

Nice catch! I think there was a conditional in the earlier versions of
the original series that caught this but it got lost in the later
reivew stages. I do however prefer this de-looped/coreified
implementation.

> 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;

"state" is a bit ambiguous for a variable name. I have seen "parent"
used in this context before (although I must confess that may get
confused with QOM terms)."container"?


> +    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;

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++) {

... is a local variable to this for loop iteration. So it can be
defined here for easier reading.

Otherwise:

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

Regards,
Peter

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

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

[Peter Crosthwaite]

On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galvani@gmail.com> 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.
>

With all these allwinner cores do we have docs for any of them? Ive
seen contributor claims that both enet and intc are undocumented but I
saw a passing reference to a document for the timer. Is there anything
useful resembling register specs for any of these patches? (not that
that stops us from contributing - it just makes accurate review
easier!).

> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>

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

> ---
>  hw/intc/allwinner-a10-pic.c |   14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> index 407d563..00f3c11 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);
> @@ -84,9 +93,6 @@ static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value,
>      uint8_t index = (offset & 0xc) / 4;
>
>      switch (offset) {
> -    case AW_A10_PIC_VECTOR:
> -        s->vector = value & ~0x3;
> -        break;
>      case AW_A10_PIC_BASE_ADDR:
>          s->base_addr = value & ~0x3;
>      case AW_A10_PIC_PROTECT:
> --
> 1.7.10.4
>
>

Re: [Qemu-devel] [PATCH v2 2/7] allwinner-a10-pic: update pending register when an irq is cleared

[Peter Crosthwaite]

On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> The value of pending register was updated only when an irq was raised
> from a device; it should also be updated when an interrupt is cleared.
>

So the reason for doing this is obviously the need for level sensitive
interrupts. Current implementation works under the assumption that all
interrupts are edge sensitive. This patch goes full on the other way
and mandates that all interrupts the edge-sensitive. In the absence of
definitive documentation, I guess that ok, but you do effecitively
defeature the interrupt controller transforming and edge to a level
(which is a very common interrupt controller feature). It is possible
to implement both schemes concurrently with some extra state. I.e. one
set of registers for the external pin state (no latching) and one set
for interrupts that have been detected and not serviced yet (via wtc
to IRQ_PENDING). If an interrupt is serviced while the pin state is
still high then it insta-retriggers (correct level sens. behav.).

You have left in the support for clearing the register from software.
Although I can see some wierd use cases of this. I.e. an interrupt can
be triggered and "serviced" (i.e. irq_pending wtc'd) by the intc then
the device level service is delayed while the device irq line remains
high. To, me this actually corresponds to a disabling of the interrupt
(assuming it is level sensitive!) rather than a servicing of a pending
interrupt. This match the kernel driver?

Regards,
Peter

> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> ---
>  hw/intc/allwinner-a10-pic.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> index 00f3c11..9011c82 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);
>  }
> --
> 1.7.10.4
>
>

Re: [Qemu-devel] [PATCH v2 2/7] allwinner-a10-pic: update pending register when an irq is cleared

[Beniamino Galvani]

On Mon, Mar 03, 2014 at 09:56:07PM +1000, Peter Crosthwaite wrote:
> On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> > The value of pending register was updated only when an irq was raised
> > from a device; it should also be updated when an interrupt is cleared.
> 
> So the reason for doing this is obviously the need for level sensitive
> interrupts. Current implementation works under the assumption that all
> interrupts are edge sensitive. This patch goes full on the other way
> and mandates that all interrupts the edge-sensitive. In the absence of
> definitive documentation, I guess that ok, but you do effecitively
> defeature the interrupt controller transforming and edge to a level
> (which is a very common interrupt controller feature). It is possible
> to implement both schemes concurrently with some extra state. I.e. one
> set of registers for the external pin state (no latching) and one set
> for interrupts that have been detected and not serviced yet (via wtc
> to IRQ_PENDING). If an interrupt is serviced while the pin state is
> still high then it insta-retriggers (correct level sens. behav.).
> 
> You have left in the support for clearing the register from software.
> Although I can see some wierd use cases of this. I.e. an interrupt can
> be triggered and "serviced" (i.e. irq_pending wtc'd) by the intc then
> the device level service is delayed while the device irq line remains
> high. To, me this actually corresponds to a disabling of the interrupt
> (assuming it is level sensitive!) rather than a servicing of a pending
> interrupt. This match the kernel driver?

Hi,
the kernel driver uses this function to ack an interrupt [1]:

static void sun4i_irq_ack(struct irq_data *irqd)
{
        unsigned int irq = irqd_to_hwirq(irqd);
        unsigned int irq_off = irq % 32;
        int reg = irq / 32;
        u32 val;

        val = readl(sun4i_irq_base + SUN4I_IRQ_PENDING_REG(reg));
        writel(val | (1 << irq_off),
                   sun4i_irq_base + SUN4I_IRQ_PENDING_REG(reg));
}

I don't understand how this function can work, because if we assume
that the SUN4I_IRQ_PENDING register is a wtc register as specified in
the datasheet [2] at p.114 (and ignoring that the register is also
described as read-only), the function would clear all the bits every
time is called.

As reported in the comment for v1, there was a thread on LKML [3] in
which a drop of all the writes to the pending register was proposed,
and a mail from the driver author explained that writes to the
register are basically uneffective.

So my initial idea was to remove the writability of pending register
and to change all interrupts to level-triggered; but since I was not
sure about the first point, I kept the writability.

Beniamino

[1] http://lxr.free-electrons.com/source/drivers/irqchip/irq-sun4i.c#L41
[2] http://dl.linux-sunxi.org/A10/A10%20User%20Manual%20-%20v1.20%20%282012-04-09,%20DECRYPTED%29.pdf
[3] https://lkml.org/lkml/2013/7/6/59

> Regards,
> Peter
> 
> > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> > ---
> >  hw/intc/allwinner-a10-pic.c |    2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> > index 00f3c11..9011c82 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);
> >  }
> > --
> > 1.7.10.4
> >
> >

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

[Beniamino Galvani]

On Mon, Mar 03, 2014 at 09:16:13PM +1000, Peter Crosthwaite wrote:
> On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galvani@gmail.com> 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.
> >
> 
> With all these allwinner cores do we have docs for any of them? Ive
> seen contributor claims that both enet and intc are undocumented but I
> saw a passing reference to a document for the timer. Is there anything
> useful resembling register specs for any of these patches? (not that
> that stops us from contributing - it just makes accurate review
> easier!).
> 
> > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> 
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 

AFAIK, this is the most complete datasheet available:

http://dl.linux-sunxi.org/A10/A10%20User%20Manual%20-%20v1.20%20%282012-04-09,%20DECRYPTED%29.pdf

Interrupt controller and timer are documented, EMAC is not.

Beniamino

> > ---
> >  hw/intc/allwinner-a10-pic.c |   14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> > index 407d563..00f3c11 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);
> > @@ -84,9 +93,6 @@ static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value,
> >      uint8_t index = (offset & 0xc) / 4;
> >
> >      switch (offset) {
> > -    case AW_A10_PIC_VECTOR:
> > -        s->vector = value & ~0x3;
> > -        break;
> >      case AW_A10_PIC_BASE_ADDR:
> >          s->base_addr = value & ~0x3;
> >      case AW_A10_PIC_PROTECT:
> > --
> > 1.7.10.4
> >
> >

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

[Beniamino Galvani]

On Mon, Mar 03, 2014 at 09:08:27PM +1000, Peter Crosthwaite wrote:
> On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galvani@gmail.com> 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.
> >
> 
> Nice catch! I think there was a conditional in the earlier versions of
> the original series that caught this but it got lost in the later
> reivew stages. I do however prefer this de-looped/coreified
> implementation.
> 
> > 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;
> 
> "state" is a bit ambiguous for a variable name. I have seen "parent"
> used in this context before (although I must confess that may get
> confused with QOM terms)."container"?

"container" gives me the idea that the TimerContext is a member of the
AwA10PITState struct, which is not the case.

Is "pit_state" better?

> 
> > +    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;
> 
> 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++) {
> 
> ... is a local variable to this for loop iteration. So it can be
> defined here for easier reading.

Ok.

Beniamino

> 
> Otherwise:
> 
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> Regards,
> Peter
> 
> > -        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
> >
> >

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

[Beniamino Galvani]

On Mon, Mar 03, 2014 at 08:57:33PM +1000, Peter Crosthwaite wrote:
> On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> > 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};
> > +
> 
> These smell like external oscillator frequencies which is board level
> information. Ideally, you should propertyify the input frequencies and
> pass them in on the board, so others can make different boards with
> with different crystals.

According to the A10 datasheet, the possible values for the field are:

  00: Low speed OSC
  01: OSC24M
  10: PLL6/6
  11: /

OSC24M is a fixed 24MHz oscillator; I suppose that "Low speed OSC" is
the 32768Hz oscillator described in section 6. So both frequencies
seem to be fixed in this case.
 
> > +    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",
> 
> LOG_UNIMP is usually used for something that does exist but QEMU
> chooses knowingly not to implement. Are there extra clock wires from
> other sources not handled or are encodings 1 & 2 just unpopulated on
> this board? (In which case it should be a GUEST_ERROR).

Encoding 2 is associated to PLL6/6 and should give a UNIMPL; encoding
3 is not defined and should give a GUEST_ERROR, right?

Thanks for the reviews,
Beniamino

> 
> Regards,
> Peter
> 
> > +                      __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
> >
> >

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

[Peter Crosthwaite]

On Tue, Mar 4, 2014 at 8:16 AM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> On Mon, Mar 03, 2014 at 09:08:27PM +1000, Peter Crosthwaite wrote:
>> On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galvani@gmail.com> 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.
>> >
>>
>> Nice catch! I think there was a conditional in the earlier versions of
>> the original series that caught this but it got lost in the later
>> reivew stages. I do however prefer this de-looped/coreified
>> implementation.
>>
>> > 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;
>>
>> "state" is a bit ambiguous for a variable name. I have seen "parent"
>> used in this context before (although I must confess that may get
>> confused with QOM terms)."container"?
>
> "container" gives me the idea that the TimerContext is a member of the
> AwA10PITState struct, which is not the case.
>

Thinking more about this, it probably should be. There is a movement
(although occasionally disputed) to minimise malloc/pointerified state
and inline structs as much as possible. Currently you just leak the
malloc'ed objects which is not the cleanest style and the leak becomes
real in the event of dynamically added/removed devices (will be a
reality one day). Just inline the structs and do away with the malloc.
Then container will be fitting.

Regards,
Peter

> Is "pit_state" better?
>
>>
>> > +    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;
>>
>> 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++) {
>>
>> ... is a local variable to this for loop iteration. So it can be
>> defined here for easier reading.
>
> Ok.
>
> Beniamino
>
>>
>> Otherwise:
>>
>> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> Regards,
>> Peter
>>
>> > -        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
>> >
>> >
>