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

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

The changes concern interrupt handling, timer and ethernet MAC.

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

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

Changelog:
v4: Address comments from Peter Crosthwaite:
    * force argument of qemu_set_irq to 0/1
    * update irq status in pit reset handler
    * add properties for setting input frequencies of pit
    * set pit input frequencies in cubieboard init function

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/arm/cubieboard.c                  |   13 ++++++
 hw/intc/allwinner-a10-pic.c          |   22 ++++++---
 hw/net/allwinner_emac.c              |    6 ++-
 hw/timer/allwinner-a10-pit.c         |   81 +++++++++++++++++++++++++++++-----
 include/hw/net/allwinner_emac.h      |    1 +
 include/hw/timer/allwinner-a10-pit.h |   12 +++++
 6 files changed, 116 insertions(+), 19 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH v4 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>
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:
-- 
1.7.10.4

[Qemu-devel] [PATCH v4 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>
Reviewed-by: Li Guang <lig.fnst@cn.fujitsu.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

[Qemu-devel] [PATCH v4 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>
Reviewed-by: Li Guang <lig.fnst@cn.fujitsu.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

[Qemu-devel] [PATCH v4 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>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/timer/allwinner-a10-pit.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index 696b7d9..5aa78a9 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;
@@ -178,6 +189,8 @@ static void a10_pit_reset(DeviceState *dev)
 
     s->irq_enable = 0;
     s->irq_status = 0;
+    a10_pit_update_irq(s);
+
     for (i = 0; i < 6; i++) {
         s->control[i] = AW_A10_PIT_DEFAULT_CLOCK;
         s->interval[i] = 0;
@@ -203,7 +216,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 v4 5/7] allwinner-a10-pit: implement prescaler and source selection

[Beniamino Galvani]

This implements the prescaler and source fields of the timer control
register. The source for each timer can be selected among 4 clock
inputs whose frequencies are set through model properties.

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 hw/arm/cubieboard.c                  |   13 ++++++++++
 hw/timer/allwinner-a10-pit.c         |   43 +++++++++++++++++++++++++++++++++-
 include/hw/timer/allwinner-a10-pit.h |    4 ++++
 3 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
index d95a7f3..9d158c7 100644
--- a/hw/arm/cubieboard.c
+++ b/hw/arm/cubieboard.c
@@ -43,6 +43,19 @@ static void cubieboard_init(QEMUMachineInitArgs *args)
         exit(1);
     }
 
+    object_property_set_int(OBJECT(&s->a10->timer), 32768, "clk0-freq", &err);
+    if (err != NULL) {
+        error_report("Couldn't set clk0 frequency: %s", error_get_pretty(err));
+        exit(1);
+    }
+
+    object_property_set_int(OBJECT(&s->a10->timer), 24000000, "clk1-freq",
+                            &err);
+    if (err != NULL) {
+        error_report("Couldn't set clk1 frequency: %s", error_get_pretty(err));
+        exit(1);
+    }
+
     object_property_set_bool(OBJECT(s->a10), true, "realized", &err);
     if (err != NULL) {
         error_report("Couldn't realize Allwinner A10: %s",
diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index 5aa78a9..5c1ffba 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -74,6 +74,37 @@ 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;
+
+    prescaler = 1 << extract32(s->control[index], 4, 3);
+    source = extract32(s->control[index], 2, 2);
+
+    switch (source) {
+    case 0:
+        source_freq = s->clk0_freq;
+        break;
+    case 1:
+        source_freq = s->clk1_freq;
+        break;
+    case 2:
+        source_freq = s->clk2_freq;
+        break;
+    case 3:
+        source_freq = s->clk3_freq;
+        break;
+    }
+
+    if (source_freq) {
+        ptimer_set_freq(s->timer[index], source_freq / prescaler);
+    } else {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid clock source %u\n",
+                      __func__, source);
+    }
+}
+
 static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
                             unsigned size)
 {
@@ -96,6 +127,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]);
             }
@@ -161,6 +193,14 @@ static const MemoryRegionOps a10_pit_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static Property a10_pit_properties[] = {
+    DEFINE_PROP_UINT32("clk0-freq", AwA10PITState, clk0_freq, 0),
+    DEFINE_PROP_UINT32("clk1-freq", AwA10PITState, clk1_freq, 0),
+    DEFINE_PROP_UINT32("clk2-freq", AwA10PITState, clk2_freq, 0),
+    DEFINE_PROP_UINT32("clk3-freq", AwA10PITState, clk3_freq, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static const VMStateDescription vmstate_a10_pit = {
     .name = "a10.pit",
     .version_id = 1,
@@ -196,6 +236,7 @@ static void a10_pit_reset(DeviceState *dev)
         s->interval[i] = 0;
         s->count[i] = 0;
         ptimer_stop(s->timer[i]);
+        a10_pit_set_freq(s, i);
     }
     s->watch_dog_mode = 0;
     s->watch_dog_control = 0;
@@ -241,7 +282,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);
     }
 }
 
@@ -250,6 +290,7 @@ static void a10_pit_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->reset = a10_pit_reset;
+    dc->props = a10_pit_properties;
     dc->desc = "allwinner a10 timer";
     dc->vmsd = &vmstate_a10_pit;
 }
diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
index a48d3c7..5388b2b 100644
--- a/include/hw/timer/allwinner-a10-pit.h
+++ b/include/hw/timer/allwinner-a10-pit.h
@@ -50,6 +50,10 @@ typedef struct AwA10PITState {
     ptimer_state * timer[AW_A10_PIT_TIMER_NR];
     AwA10TimerContext timer_context[AW_A10_PIT_TIMER_NR];
     MemoryRegion iomem;
+    uint32_t clk0_freq;
+    uint32_t clk1_freq;
+    uint32_t clk2_freq;
+    uint32_t clk3_freq;
 
     uint32_t irq_enable;
     uint32_t irq_status;
-- 
1.7.10.4

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

[Beniamino Galvani]

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

[Qemu-devel] [PATCH v4 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 v4 5/7] allwinner-a10-pit: implement prescaler and source selection

[Peter Crosthwaite]

On Fri, Mar 21, 2014 at 7:25 AM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> This implements the prescaler and source fields of the timer control
> register. The source for each timer can be selected among 4 clock
> inputs whose frequencies are set through model properties.
>
> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> ---
>  hw/arm/cubieboard.c                  |   13 ++++++++++
>  hw/timer/allwinner-a10-pit.c         |   43 +++++++++++++++++++++++++++++++++-
>  include/hw/timer/allwinner-a10-pit.h |    4 ++++
>  3 files changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
> index d95a7f3..9d158c7 100644
> --- a/hw/arm/cubieboard.c
> +++ b/hw/arm/cubieboard.c
> @@ -43,6 +43,19 @@ static void cubieboard_init(QEMUMachineInitArgs *args)
>          exit(1);
>      }
>
> +    object_property_set_int(OBJECT(&s->a10->timer), 32768, "clk0-freq", &err);
> +    if (err != NULL) {
> +        error_report("Couldn't set clk0 frequency: %s", error_get_pretty(err));
> +        exit(1);
> +    }
> +
> +    object_property_set_int(OBJECT(&s->a10->timer), 24000000, "clk1-freq",
> +                            &err);
> +    if (err != NULL) {
> +        error_report("Couldn't set clk1 frequency: %s", error_get_pretty(err));
> +        exit(1);
> +    }
> +
>      object_property_set_bool(OBJECT(s->a10), true, "realized", &err);
>      if (err != NULL) {
>          error_report("Couldn't realize Allwinner A10: %s",
> diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> index 5aa78a9..5c1ffba 100644
> --- a/hw/timer/allwinner-a10-pit.c
> +++ b/hw/timer/allwinner-a10-pit.c
> @@ -74,6 +74,37 @@ 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;
> +
> +    prescaler = 1 << extract32(s->control[index], 4, 3);
> +    source = extract32(s->control[index], 2, 2);
> +
> +    switch (source) {
> +    case 0:
> +        source_freq = s->clk0_freq;
> +        break;
> +    case 1:
> +        source_freq = s->clk1_freq;
> +        break;
> +    case 2:
> +        source_freq = s->clk2_freq;
> +        break;
> +    case 3:
> +        source_freq = s->clk3_freq;
> +        break;
> +    }

You can get rid of this [1] ...

> +
> +    if (source_freq) {
> +        ptimer_set_freq(s->timer[index], source_freq / prescaler);
> +    } else {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid clock source %u\n",
> +                      __func__, source);
> +    }
> +}
> +
>  static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
>                              unsigned size)
>  {
> @@ -96,6 +127,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]);
>              }
> @@ -161,6 +193,14 @@ static const MemoryRegionOps a10_pit_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>
> +static Property a10_pit_properties[] = {
> +    DEFINE_PROP_UINT32("clk0-freq", AwA10PITState, clk0_freq, 0),

... [1] by changing this to:

DEFINE_PROP_UINT32("clk0-freq", AwA10PITState, clk_freq[0], 0),

> +    DEFINE_PROP_UINT32("clk1-freq", AwA10PITState, clk1_freq, 0),
> +    DEFINE_PROP_UINT32("clk2-freq", AwA10PITState, clk2_freq, 0),
> +    DEFINE_PROP_UINT32("clk3-freq", AwA10PITState, clk3_freq, 0),


Note that there is an array property system (DEFINE_PROP_ARRAY) that
should kinda work here. Although it requires you to set the length of
the array on the setter level, which should be fixed to 4 in this
case. Check the "db-voltage" property of arm_sysctrl and its usage in
vexpress. But I can't see a clean way of fixing this imminently
without patching that property system. Curious it works nicely for
dynamic arrays but not static (unless I am missing something).

> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static const VMStateDescription vmstate_a10_pit = {
>      .name = "a10.pit",
>      .version_id = 1,
> @@ -196,6 +236,7 @@ static void a10_pit_reset(DeviceState *dev)
>          s->interval[i] = 0;
>          s->count[i] = 0;
>          ptimer_stop(s->timer[i]);
> +        a10_pit_set_freq(s, i);
>      }
>      s->watch_dog_mode = 0;
>      s->watch_dog_control = 0;
> @@ -241,7 +282,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);
>      }
>  }
>
> @@ -250,6 +290,7 @@ static void a10_pit_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
>      dc->reset = a10_pit_reset;
> +    dc->props = a10_pit_properties;
>      dc->desc = "allwinner a10 timer";
>      dc->vmsd = &vmstate_a10_pit;
>  }
> diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
> index a48d3c7..5388b2b 100644
> --- a/include/hw/timer/allwinner-a10-pit.h
> +++ b/include/hw/timer/allwinner-a10-pit.h
> @@ -50,6 +50,10 @@ typedef struct AwA10PITState {
>      ptimer_state * timer[AW_A10_PIT_TIMER_NR];
>      AwA10TimerContext timer_context[AW_A10_PIT_TIMER_NR];
>      MemoryRegion iomem;
> +    uint32_t clk0_freq;
> +    uint32_t clk1_freq;
> +    uint32_t clk2_freq;
> +    uint32_t clk3_freq;
>

And this becomes an array.

With just the arrayification change:

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

Regards,
Peter

>      uint32_t irq_enable;
>      uint32_t irq_status;
> --
> 1.7.10.4
>
>

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

[Beniamino Galvani]

On Tue, Mar 25, 2014 at 09:49:21AM +1000, Peter Crosthwaite wrote:
> On Fri, Mar 21, 2014 at 7:25 AM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> > This implements the prescaler and source fields of the timer control
> > register. The source for each timer can be selected among 4 clock
> > inputs whose frequencies are set through model properties.
> >
> > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> > ---
> >  hw/arm/cubieboard.c                  |   13 ++++++++++
> >  hw/timer/allwinner-a10-pit.c         |   43 +++++++++++++++++++++++++++++++++-
> >  include/hw/timer/allwinner-a10-pit.h |    4 ++++
> >  3 files changed, 59 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
> > index d95a7f3..9d158c7 100644
> > --- a/hw/arm/cubieboard.c
> > +++ b/hw/arm/cubieboard.c
> > @@ -43,6 +43,19 @@ static void cubieboard_init(QEMUMachineInitArgs *args)
> >          exit(1);
> >      }
> >
> > +    object_property_set_int(OBJECT(&s->a10->timer), 32768, "clk0-freq", &err);
> > +    if (err != NULL) {
> > +        error_report("Couldn't set clk0 frequency: %s", error_get_pretty(err));
> > +        exit(1);
> > +    }
> > +
> > +    object_property_set_int(OBJECT(&s->a10->timer), 24000000, "clk1-freq",
> > +                            &err);
> > +    if (err != NULL) {
> > +        error_report("Couldn't set clk1 frequency: %s", error_get_pretty(err));
> > +        exit(1);
> > +    }
> > +
> >      object_property_set_bool(OBJECT(s->a10), true, "realized", &err);
> >      if (err != NULL) {
> >          error_report("Couldn't realize Allwinner A10: %s",
> > diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> > index 5aa78a9..5c1ffba 100644
> > --- a/hw/timer/allwinner-a10-pit.c
> > +++ b/hw/timer/allwinner-a10-pit.c
> > @@ -74,6 +74,37 @@ 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;
> > +
> > +    prescaler = 1 << extract32(s->control[index], 4, 3);
> > +    source = extract32(s->control[index], 2, 2);
> > +
> > +    switch (source) {
> > +    case 0:
> > +        source_freq = s->clk0_freq;
> > +        break;
> > +    case 1:
> > +        source_freq = s->clk1_freq;
> > +        break;
> > +    case 2:
> > +        source_freq = s->clk2_freq;
> > +        break;
> > +    case 3:
> > +        source_freq = s->clk3_freq;
> > +        break;
> > +    }
> 
> You can get rid of this [1] ...
> 
> > +
> > +    if (source_freq) {
> > +        ptimer_set_freq(s->timer[index], source_freq / prescaler);
> > +    } else {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid clock source %u\n",
> > +                      __func__, source);
> > +    }
> > +}
> > +
> >  static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
> >                              unsigned size)
> >  {
> > @@ -96,6 +127,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]);
> >              }
> > @@ -161,6 +193,14 @@ static const MemoryRegionOps a10_pit_ops = {
> >      .endianness = DEVICE_NATIVE_ENDIAN,
> >  };
> >
> > +static Property a10_pit_properties[] = {
> > +    DEFINE_PROP_UINT32("clk0-freq", AwA10PITState, clk0_freq, 0),
> 
> ... [1] by changing this to:
> 
> DEFINE_PROP_UINT32("clk0-freq", AwA10PITState, clk_freq[0], 0),
> 
> > +    DEFINE_PROP_UINT32("clk1-freq", AwA10PITState, clk1_freq, 0),
> > +    DEFINE_PROP_UINT32("clk2-freq", AwA10PITState, clk2_freq, 0),
> > +    DEFINE_PROP_UINT32("clk3-freq", AwA10PITState, clk3_freq, 0),
> 
> 
> Note that there is an array property system (DEFINE_PROP_ARRAY) that
> should kinda work here. Although it requires you to set the length of
> the array on the setter level, which should be fixed to 4 in this
> case. Check the "db-voltage" property of arm_sysctrl and its usage in
> vexpress. But I can't see a clean way of fixing this imminently
> without patching that property system. Curious it works nicely for
> dynamic arrays but not static (unless I am missing something).
> 
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> >  static const VMStateDescription vmstate_a10_pit = {
> >      .name = "a10.pit",
> >      .version_id = 1,
> > @@ -196,6 +236,7 @@ static void a10_pit_reset(DeviceState *dev)
> >          s->interval[i] = 0;
> >          s->count[i] = 0;
> >          ptimer_stop(s->timer[i]);
> > +        a10_pit_set_freq(s, i);
> >      }
> >      s->watch_dog_mode = 0;
> >      s->watch_dog_control = 0;
> > @@ -241,7 +282,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);
> >      }
> >  }
> >
> > @@ -250,6 +290,7 @@ static void a10_pit_class_init(ObjectClass *klass, void *data)
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >
> >      dc->reset = a10_pit_reset;
> > +    dc->props = a10_pit_properties;
> >      dc->desc = "allwinner a10 timer";
> >      dc->vmsd = &vmstate_a10_pit;
> >  }
> > diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
> > index a48d3c7..5388b2b 100644
> > --- a/include/hw/timer/allwinner-a10-pit.h
> > +++ b/include/hw/timer/allwinner-a10-pit.h
> > @@ -50,6 +50,10 @@ typedef struct AwA10PITState {
> >      ptimer_state * timer[AW_A10_PIT_TIMER_NR];
> >      AwA10TimerContext timer_context[AW_A10_PIT_TIMER_NR];
> >      MemoryRegion iomem;
> > +    uint32_t clk0_freq;
> > +    uint32_t clk1_freq;
> > +    uint32_t clk2_freq;
> > +    uint32_t clk3_freq;
> >
> 
> And this becomes an array.

I will change this in the next version, thank you for the review.

Beniamino