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

[Qemu-devel] [PATCH v3 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:
v3: Address comments from Peter Crosthwaite:
    * make pending register read-only (patch 2)
    * inline timer context structure into timer state (patch 3)
    * cleanup timer frequency initialization (patch 5)

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: fix behaviour of pending register
  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          |   22 +++++++++---
 hw/net/allwinner_emac.c              |    6 ++--
 hw/timer/allwinner-a10-pit.c         |   66 +++++++++++++++++++++++++++-------
 include/hw/net/allwinner_emac.h      |    1 +
 include/hw/timer/allwinner-a10-pit.h |   16 +++++++++
 5 files changed, 92 insertions(+), 19 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH v3 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>
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

[Qemu-devel] [PATCH v3 2/7] allwinner-a10-pic: fix behaviour of pending register

[Beniamino Galvani]

The pending register is read-only and the value returned upon a read
reflects the state of irq input pins (interrupts are level triggered).
This patch implements such behaviour.

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

diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
index 00f3c11..0924d98 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);
 }
@@ -102,7 +104,11 @@ 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;
+        /*
+         * The register is read-only; nevertheless, Linux (including
+         * the version originally shipped by Allwinner) pretends to
+         * write to the register. Just ignore it.
+         */
         break;
     case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
         s->fiq_pending[index] &= ~value;
-- 
1.7.10.4

[Qemu-devel] [PATCH v3 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 explicitly the timer
index to the callback function.

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 hw/timer/allwinner-a10-pit.c         |   25 ++++++++++++++-----------
 include/hw/timer/allwinner-a10-pit.h |    8 ++++++++
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index b27fce8..696b7d9 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -193,18 +193,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;
+    AwA10TimerContext *tc = opaque;
+    AwA10PITState *s = tc->container;
+    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]);
     }
 }
 
@@ -223,7 +222,11 @@ 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);
+        AwA10TimerContext *tc = &s->timer_context[i];
+
+        tc->container = 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);
     }
diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
index 15efab8..a48d3c7 100644
--- a/include/hw/timer/allwinner-a10-pit.h
+++ b/include/hw/timer/allwinner-a10-pit.h
@@ -35,12 +35,20 @@
 
 #define AW_A10_PIT_DEFAULT_CLOCK   0x4
 
+typedef struct AwA10PITState AwA10PITState;
+
+typedef struct AwA10TimerContext {
+    AwA10PITState *container;
+    int index;
+} AwA10TimerContext;
+
 typedef struct AwA10PITState {
     /*< private >*/
     SysBusDevice parent_obj;
     /*< public >*/
     qemu_irq irq[AW_A10_PIT_TIMER_NR];
     ptimer_state * timer[AW_A10_PIT_TIMER_NR];
+    AwA10TimerContext timer_context[AW_A10_PIT_TIMER_NR];
     MemoryRegion iomem;
 
     uint32_t irq_enable;
-- 
1.7.10.4

[Qemu-devel] [PATCH v3 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 696b7d9..f8c9236 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -19,6 +19,15 @@
 #include "sysemu/sysemu.h"
 #include "hw/timer/allwinner-a10-pit.h"
 
+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);
@@ -74,9 +83,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;
@@ -203,7 +214,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 v3 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         |   30 +++++++++++++++++++++++++++++-
 include/hw/timer/allwinner-a10-pit.h |    8 ++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index f8c9236..a448689 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -74,6 +74,34 @@ 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_freq = AW_A10_PIT_OSC24M_FREQ;
+
+    prescaler = 1 << extract32(s->control[index], 4, 3);
+    source = extract32(s->control[index], 2, 2);
+
+    switch (source) {
+    case AW_A10_PIT_SOURCE_LS_OSC:
+        source_freq = AW_A10_PIT_LS_OSC_FREQ;
+        break;
+    case AW_A10_PIT_SOURCE_OSC24M:
+        source_freq = AW_A10_PIT_OSC24M_FREQ;
+        break;
+    case AW_A10_PIT_SOURCE_PLL6:
+        qemu_log_mask(LOG_UNIMP, "%s: unimplemented clock source %u", __func__,
+                      source);
+        break;
+    case AW_A10_PIT_SOURCE_UNDEF:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid clock source %u", __func__,
+                      source);
+        break;
+    }
+
+    ptimer_set_freq(s->timer[index], source_freq / prescaler);
+}
+
 static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
                             unsigned size)
 {
@@ -96,6 +124,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]);
             }
@@ -239,7 +268,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);
     }
 }
 
diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
index a48d3c7..37a2662 100644
--- a/include/hw/timer/allwinner-a10-pit.h
+++ b/include/hw/timer/allwinner-a10-pit.h
@@ -33,6 +33,14 @@
 #define AW_A10_PIT_TIMER_BASE_END  \
     (AW_A10_PIT_TIMER_BASE * 6 + AW_A10_PIT_TIMER_COUNT)
 
+#define AW_A10_PIT_SOURCE_LS_OSC   0
+#define AW_A10_PIT_SOURCE_OSC24M   1
+#define AW_A10_PIT_SOURCE_PLL6     2
+#define AW_A10_PIT_SOURCE_UNDEF    3
+
+#define AW_A10_PIT_LS_OSC_FREQ     32768
+#define AW_A10_PIT_OSC24M_FREQ     24000000
+
 #define AW_A10_PIT_DEFAULT_CLOCK   0x4
 
 typedef struct AwA10PITState AwA10PITState;
-- 
1.7.10.4

[Qemu-devel] [PATCH v3 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 v3 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>
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 v3 2/7] allwinner-a10-pic: fix behaviour of pending register

[Peter Crosthwaite]

On Sat, Mar 15, 2014 at 11:01 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> The pending register is read-only and the value returned upon a read
> reflects the state of irq input pins (interrupts are level triggered).
> This patch implements such behaviour.
>
> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>

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

> ---
>  hw/intc/allwinner-a10-pic.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> index 00f3c11..0924d98 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);
>  }
> @@ -102,7 +104,11 @@ 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;
> +        /*
> +         * The register is read-only; nevertheless, Linux (including
> +         * the version originally shipped by Allwinner) pretends to
> +         * write to the register. Just ignore it.
> +         */
>          break;
>      case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
>          s->fiq_pending[index] &= ~value;
> --
> 1.7.10.4
>
>

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

[Peter Crosthwaite]

On Sat, Mar 15, 2014 at 11:01 PM, 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 explicitly the timer
> index to the callback function.
>
> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>

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

> ---
>  hw/timer/allwinner-a10-pit.c         |   25 ++++++++++++++-----------
>  include/hw/timer/allwinner-a10-pit.h |    8 ++++++++
>  2 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> index b27fce8..696b7d9 100644
> --- a/hw/timer/allwinner-a10-pit.c
> +++ b/hw/timer/allwinner-a10-pit.c
> @@ -193,18 +193,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;
> +    AwA10TimerContext *tc = opaque;
> +    AwA10PITState *s = tc->container;
> +    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]);
>      }
>  }
>
> @@ -223,7 +222,11 @@ 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);
> +        AwA10TimerContext *tc = &s->timer_context[i];
> +
> +        tc->container = 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);
>      }
> diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
> index 15efab8..a48d3c7 100644
> --- a/include/hw/timer/allwinner-a10-pit.h
> +++ b/include/hw/timer/allwinner-a10-pit.h
> @@ -35,12 +35,20 @@
>
>  #define AW_A10_PIT_DEFAULT_CLOCK   0x4
>
> +typedef struct AwA10PITState AwA10PITState;
> +
> +typedef struct AwA10TimerContext {
> +    AwA10PITState *container;
> +    int index;
> +} AwA10TimerContext;
> +
>  typedef struct AwA10PITState {
>      /*< private >*/
>      SysBusDevice parent_obj;
>      /*< public >*/
>      qemu_irq irq[AW_A10_PIT_TIMER_NR];
>      ptimer_state * timer[AW_A10_PIT_TIMER_NR];
> +    AwA10TimerContext timer_context[AW_A10_PIT_TIMER_NR];
>      MemoryRegion iomem;
>
>      uint32_t irq_enable;
> --
> 1.7.10.4
>
>

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

[Peter Crosthwaite]

On Sat, Mar 15, 2014 at 11:01 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> 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 696b7d9..f8c9236 100644
> --- a/hw/timer/allwinner-a10-pit.c
> +++ b/hw/timer/allwinner-a10-pit.c
> @@ -19,6 +19,15 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/timer/allwinner-a10-pit.h"
>
> +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));

People do sometimes use a !! on the level arg to qemu_set_irq to force
0/1 behavior but I think our most recent on-list conclusion is it's
not a requirement. I'm all for the change however, as it makes the
eventual cleanup of qemu_set_irq perhaps a little easier for someone.

> +    }
> +}
> +
>  static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      AwA10PITState *s = AW_A10_PIT(opaque);
> @@ -74,9 +83,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;
> @@ -203,7 +214,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);
>      }
>  }
>

I think you need to a10_pit_update_irq() in your reset handler.
Otherwise your level sensitive IRQs can stay high through your
peripheral's (or system's) hard reset.

Otherwise:

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

Regards,
Peter

> --
> 1.7.10.4
>
>

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

[Li Guang]

在 2014-03-15六的 14:01 +0100，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>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Li Guang <lig.fnst@cn.fujitsu.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:

Re: [Qemu-devel] [PATCH v3 2/7] allwinner-a10-pic: fix behaviour of pending register

[Li Guang]

在 2014-03-15六的 14:01 +0100，Beniamino Galvani写道：
> The pending register is read-only and the value returned upon a read
> reflects the state of irq input pins (interrupts are level triggered).
> This patch implements such behaviour.
> 
> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>

Reviewed-by: Li Guang <lig.fnst@cn.fujitsu.com>

> ---
>  hw/intc/allwinner-a10-pic.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> index 00f3c11..0924d98 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);
>  }
> @@ -102,7 +104,11 @@ 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;
> +        /*
> +         * The register is read-only; nevertheless, Linux (including
> +         * the version originally shipped by Allwinner) pretends to
> +         * write to the register. Just ignore it.
> +         */
>          break;
>      case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
>          s->fiq_pending[index] &= ~value;

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

[Li Guang]

在 2014-03-15六的 14:01 +0100，Beniamino Galvani写道：
> The model was generating interrupts for all enabled timers after the
> expiration of one of them. Avoid this by passing explicitly the timer
> index to the callback function.
> 
> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>

Reviewed-by: Li Guang <lig.fnst@cn.fujitsu.com>

> ---
>  hw/timer/allwinner-a10-pit.c         |   25 ++++++++++++++-----------
>  include/hw/timer/allwinner-a10-pit.h |    8 ++++++++
>  2 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> index b27fce8..696b7d9 100644
> --- a/hw/timer/allwinner-a10-pit.c
> +++ b/hw/timer/allwinner-a10-pit.c
> @@ -193,18 +193,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;
> +    AwA10TimerContext *tc = opaque;
> +    AwA10PITState *s = tc->container;
> +    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]);
>      }
>  }
>  
> @@ -223,7 +222,11 @@ 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);
> +        AwA10TimerContext *tc = &s->timer_context[i];
> +
> +        tc->container = 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);
>      }
> diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
> index 15efab8..a48d3c7 100644
> --- a/include/hw/timer/allwinner-a10-pit.h
> +++ b/include/hw/timer/allwinner-a10-pit.h
> @@ -35,12 +35,20 @@
>  
>  #define AW_A10_PIT_DEFAULT_CLOCK   0x4
>  
> +typedef struct AwA10PITState AwA10PITState;
> +
> +typedef struct AwA10TimerContext {
> +    AwA10PITState *container;
> +    int index;
> +} AwA10TimerContext;
> +
>  typedef struct AwA10PITState {
>      /*< private >*/
>      SysBusDevice parent_obj;
>      /*< public >*/
>      qemu_irq irq[AW_A10_PIT_TIMER_NR];
>      ptimer_state * timer[AW_A10_PIT_TIMER_NR];
> +    AwA10TimerContext timer_context[AW_A10_PIT_TIMER_NR];
>      MemoryRegion iomem;
>  
>      uint32_t irq_enable;

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

[Li Guang]

在 2014-03-15六的 14:01 +0100，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>

Reviewed-by: Li Guang <lig.fnst@cn.fujitsu.com>

> ---
>  hw/timer/allwinner-a10-pit.c         |   30 +++++++++++++++++++++++++++++-
>  include/hw/timer/allwinner-a10-pit.h |    8 ++++++++
>  2 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> index f8c9236..a448689 100644
> --- a/hw/timer/allwinner-a10-pit.c
> +++ b/hw/timer/allwinner-a10-pit.c
> @@ -74,6 +74,34 @@ 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_freq = AW_A10_PIT_OSC24M_FREQ;
> +
> +    prescaler = 1 << extract32(s->control[index], 4, 3);
> +    source = extract32(s->control[index], 2, 2);
> +
> +    switch (source) {
> +    case AW_A10_PIT_SOURCE_LS_OSC:
> +        source_freq = AW_A10_PIT_LS_OSC_FREQ;
> +        break;
> +    case AW_A10_PIT_SOURCE_OSC24M:
> +        source_freq = AW_A10_PIT_OSC24M_FREQ;
> +        break;
> +    case AW_A10_PIT_SOURCE_PLL6:
> +        qemu_log_mask(LOG_UNIMP, "%s: unimplemented clock source %u", __func__,
> +                      source);
> +        break;
> +    case AW_A10_PIT_SOURCE_UNDEF:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid clock source %u", __func__,
> +                      source);
> +        break;
> +    }
> +
> +    ptimer_set_freq(s->timer[index], source_freq / prescaler);
> +}
> +
>  static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
>                              unsigned size)
>  {
> @@ -96,6 +124,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]);
>              }
> @@ -239,7 +268,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);
>      }
>  }
>  
> diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
> index a48d3c7..37a2662 100644
> --- a/include/hw/timer/allwinner-a10-pit.h
> +++ b/include/hw/timer/allwinner-a10-pit.h
> @@ -33,6 +33,14 @@
>  #define AW_A10_PIT_TIMER_BASE_END  \
>      (AW_A10_PIT_TIMER_BASE * 6 + AW_A10_PIT_TIMER_COUNT)
>  
> +#define AW_A10_PIT_SOURCE_LS_OSC   0
> +#define AW_A10_PIT_SOURCE_OSC24M   1
> +#define AW_A10_PIT_SOURCE_PLL6     2
> +#define AW_A10_PIT_SOURCE_UNDEF    3
> +
> +#define AW_A10_PIT_LS_OSC_FREQ     32768
> +#define AW_A10_PIT_OSC24M_FREQ     24000000
> +
>  #define AW_A10_PIT_DEFAULT_CLOCK   0x4
>  
>  typedef struct AwA10PITState AwA10PITState;

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

[Peter Crosthwaite]

On Sat, Mar 15, 2014 at 11:01 PM, 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         |   30 +++++++++++++++++++++++++++++-
>  include/hw/timer/allwinner-a10-pit.h |    8 ++++++++
>  2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> index f8c9236..a448689 100644
> --- a/hw/timer/allwinner-a10-pit.c
> +++ b/hw/timer/allwinner-a10-pit.c
> @@ -74,6 +74,34 @@ 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_freq = AW_A10_PIT_OSC24M_FREQ;
> +
> +    prescaler = 1 << extract32(s->control[index], 4, 3);
> +    source = extract32(s->control[index], 2, 2);
> +
> +    switch (source) {
> +    case AW_A10_PIT_SOURCE_LS_OSC:
> +        source_freq = AW_A10_PIT_LS_OSC_FREQ;
> +        break;
> +    case AW_A10_PIT_SOURCE_OSC24M:
> +        source_freq = AW_A10_PIT_OSC24M_FREQ;
> +        break;
> +    case AW_A10_PIT_SOURCE_PLL6:
> +        qemu_log_mask(LOG_UNIMP, "%s: unimplemented clock source %u", __func__,
> +                      source);
> +        break;
> +    case AW_A10_PIT_SOURCE_UNDEF:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid clock source %u", __func__,
> +                      source);
> +        break;
> +    }
> +
> +    ptimer_set_freq(s->timer[index], source_freq / prescaler);
> +}
> +
>  static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
>                              unsigned size)
>  {
> @@ -96,6 +124,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);

Similar to comment in previous patch, I think you need to call this
from the reset handler. Otherwise you are reliant on a control write
to set the timer frequency in the first place. Looking at reset
handler, control is reset to a default value, so reset may cause the
side effect of a timer frequency change as well.

>              if (s->control[index] & AW_A10_PIT_TIMER_RELOAD) {
>                  ptimer_set_count(s->timer[index], s->interval[index]);
>              }
> @@ -239,7 +268,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);
>      }
>  }
>
> diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
> index a48d3c7..37a2662 100644
> --- a/include/hw/timer/allwinner-a10-pit.h
> +++ b/include/hw/timer/allwinner-a10-pit.h
> @@ -33,6 +33,14 @@
>  #define AW_A10_PIT_TIMER_BASE_END  \
>      (AW_A10_PIT_TIMER_BASE * 6 + AW_A10_PIT_TIMER_COUNT)
>
> +#define AW_A10_PIT_SOURCE_LS_OSC   0
> +#define AW_A10_PIT_SOURCE_OSC24M   1
> +#define AW_A10_PIT_SOURCE_PLL6     2
> +#define AW_A10_PIT_SOURCE_UNDEF    3
> +
> +#define AW_A10_PIT_LS_OSC_FREQ     32768
> +#define AW_A10_PIT_OSC24M_FREQ     24000000
> +

So a quick look at a cubieboard schematic:

http://dl.cubieboard.org/hardware/cubieboard_schematic_2012-08-08.pdf

Both of these clocks are pinned out on the A10 SoC top level. (sheet 2
pins CLK32K_IN / OUT etc). Seems for cubieboard in particular, the 32K
clock is absent. The pins names are somewhat ugly, one would assume
you could have a range of crystal frequencies when populating a
crystal on a board, but naming your pin "CLK24M" suggests otherwise. I
do think the best org for this is to just define 4 arbitrary
prameterisable clock inputs and push the policy of whats what up to
the board level. Its highly likely that that is the design intent of
the Timer IP.

Are you using the clk 32k in your test case at all?

Regards,
Peter

>  #define AW_A10_PIT_DEFAULT_CLOCK   0x4
>
>  typedef struct AwA10PITState AwA10PITState;
> --
> 1.7.10.4
>
>

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

[Peter Crosthwaite]

On Sat, Mar 15, 2014 at 11:01 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>

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

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

[Beniamino Galvani]

On Mon, Mar 17, 2014 at 11:27:01AM +1000, Peter Crosthwaite wrote:
> On Sat, Mar 15, 2014 at 11:01 PM, 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         |   30 +++++++++++++++++++++++++++++-
> >  include/hw/timer/allwinner-a10-pit.h |    8 ++++++++
> >  2 files changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> > index f8c9236..a448689 100644
> > --- a/hw/timer/allwinner-a10-pit.c
> > +++ b/hw/timer/allwinner-a10-pit.c
> > @@ -74,6 +74,34 @@ 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_freq = AW_A10_PIT_OSC24M_FREQ;
> > +
> > +    prescaler = 1 << extract32(s->control[index], 4, 3);
> > +    source = extract32(s->control[index], 2, 2);
> > +
> > +    switch (source) {
> > +    case AW_A10_PIT_SOURCE_LS_OSC:
> > +        source_freq = AW_A10_PIT_LS_OSC_FREQ;
> > +        break;
> > +    case AW_A10_PIT_SOURCE_OSC24M:
> > +        source_freq = AW_A10_PIT_OSC24M_FREQ;
> > +        break;
> > +    case AW_A10_PIT_SOURCE_PLL6:
> > +        qemu_log_mask(LOG_UNIMP, "%s: unimplemented clock source %u", __func__,
> > +                      source);
> > +        break;
> > +    case AW_A10_PIT_SOURCE_UNDEF:
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid clock source %u", __func__,
> > +                      source);
> > +        break;
> > +    }
> > +
> > +    ptimer_set_freq(s->timer[index], source_freq / prescaler);
> > +}
> > +
> >  static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
> >                              unsigned size)
> >  {
> > @@ -96,6 +124,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);
> 
> Similar to comment in previous patch, I think you need to call this
> from the reset handler. Otherwise you are reliant on a control write
> to set the timer frequency in the first place. Looking at reset
> handler, control is reset to a default value, so reset may cause the
> side effect of a timer frequency change as well.

Hi,
the code calls set_frequency() only when the control register gets
written and the motivation behind this is that the frequency of the
ptimer doesn't matter while the timer is stopped.

After a reset all timers are stopped and to enable them you have to
write to the control register, thus setting the frequency to the right
value.

> 
> >              if (s->control[index] & AW_A10_PIT_TIMER_RELOAD) {
> >                  ptimer_set_count(s->timer[index], s->interval[index]);
> >              }
> > @@ -239,7 +268,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);
> >      }
> >  }
> >
> > diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
> > index a48d3c7..37a2662 100644
> > --- a/include/hw/timer/allwinner-a10-pit.h
> > +++ b/include/hw/timer/allwinner-a10-pit.h
> > @@ -33,6 +33,14 @@
> >  #define AW_A10_PIT_TIMER_BASE_END  \
> >      (AW_A10_PIT_TIMER_BASE * 6 + AW_A10_PIT_TIMER_COUNT)
> >
> > +#define AW_A10_PIT_SOURCE_LS_OSC   0
> > +#define AW_A10_PIT_SOURCE_OSC24M   1
> > +#define AW_A10_PIT_SOURCE_PLL6     2
> > +#define AW_A10_PIT_SOURCE_UNDEF    3
> > +
> > +#define AW_A10_PIT_LS_OSC_FREQ     32768
> > +#define AW_A10_PIT_OSC24M_FREQ     24000000
> > +
> 
> So a quick look at a cubieboard schematic:
> 
> http://dl.cubieboard.org/hardware/cubieboard_schematic_2012-08-08.pdf
> 
> Both of these clocks are pinned out on the A10 SoC top level. (sheet 2
> pins CLK32K_IN / OUT etc). Seems for cubieboard in particular, the 32K
> clock is absent. The pins names are somewhat ugly, one would assume
> you could have a range of crystal frequencies when populating a
> crystal on a board, but naming your pin "CLK24M" suggests otherwise. I
> do think the best org for this is to just define 4 arbitrary
> prameterisable clock inputs and push the policy of whats what up to
> the board level. Its highly likely that that is the design intent of
> the Timer IP.

Ok, seems a good idea.
Aren't 2 properties (one for clk32k and one clk24m) enough?

> 
> Are you using the clk 32k in your test case at all?

No, Linux uses clk24m as source for timers.

Beniamino