[Qemu-devel] [PATCH arm-devs v4 0/4] A9 global timer + mpcore trivials

[Qemu-devel] [PATCH arm-devs v4 0/4] A9 global timer + mpcore trivials

[Peter Crosthwaite]

Hi Peter,

Another spin of the ARM MPCore global timer work. Patches 1 & 2 are some
trivial cleanup to MPCore I did along the way.

Regards,
Peter


François LEGAL (1):
  cpu/a9mpcore: Add Global Timer

Peter Crosthwaite (3):
  cpu/a9mpcore: rename timerbusdev variable
  cpu/a9mpcore: reorder operations/declarations
  hw/timer: Introduce ARM A9 Global Timer.

 default-configs/arm-softmmu.mak |   1 +
 hw/cpu/a9mpcore.c               |  44 +++--
 hw/timer/Makefile.objs          |   1 +
 hw/timer/a9gtimer.c             | 369 ++++++++++++++++++++++++++++++++++++++++
 include/hw/cpu/a9mpcore.h       |   4 +-
 include/hw/timer/a9gtimer.h     |  97 +++++++++++
 6 files changed, 501 insertions(+), 15 deletions(-)
 create mode 100644 hw/timer/a9gtimer.c
 create mode 100644 include/hw/timer/a9gtimer.h

-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v4 1/4] cpu/a9mpcore: rename timerbusdev variable

[Peter Crosthwaite]

Rename this variable for consistency with the above defined mptimerdev
variable.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---

 hw/cpu/a9mpcore.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index 918a7d1..1123101 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -42,7 +42,7 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     A9MPPrivState *s = A9MPCORE_PRIV(dev);
     DeviceState *gicdev, *scudev, *mptimerdev, *wdtdev;
-    SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev, *scubusdev;
+    SysBusDevice *mptimerbusdev, *wdtbusdev, *gicbusdev, *scubusdev;
     Error *err = NULL;
     int i;
 
@@ -78,7 +78,7 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
         error_propagate(errp, err);
         return;
     }
-    timerbusdev = SYS_BUS_DEVICE(&s->mptimer);
+    mptimerbusdev = SYS_BUS_DEVICE(&s->mptimer);
 
     wdtdev = DEVICE(&s->wdt);
     qdev_prop_set_uint32(wdtdev, "num-cpu", s->num_cpu);
@@ -109,7 +109,7 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
      * memory region, not the "timer/watchdog for core X" ones 11MPcore has.
      */
     memory_region_add_subregion(&s->container, 0x600,
-                                sysbus_mmio_get_region(timerbusdev, 0));
+                                sysbus_mmio_get_region(mptimerbusdev, 0));
     memory_region_add_subregion(&s->container, 0x620,
                                 sysbus_mmio_get_region(wdtbusdev, 0));
     memory_region_add_subregion(&s->container, 0x1000,
@@ -120,7 +120,7 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
      */
     for (i = 0; i < s->num_cpu; i++) {
         int ppibase = (s->num_irq - 32) + i * 32;
-        sysbus_connect_irq(timerbusdev, i,
+        sysbus_connect_irq(mptimerbusdev, i,
                            qdev_get_gpio_in(gicdev, ppibase + 29));
         sysbus_connect_irq(wdtbusdev, i,
                            qdev_get_gpio_in(gicdev, ppibase + 30));
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v4 2/4] cpu/a9mpcore: reorder operations/declarations

[Peter Crosthwaite]

To make it consistent for easier code reading. The order in which
variables are defined and functions are called is set to match the
address map ordering.

The new consistent order of doing stuff is:

SCU -> GIC -> MPTimer -> WDT.

0 functional change.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---

 hw/cpu/a9mpcore.c         | 28 ++++++++++++++--------------
 include/hw/cpu/a9mpcore.h |  2 +-
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index 1123101..a38464b 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -24,12 +24,12 @@ static void a9mp_priv_initfn(Object *obj)
     memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2000);
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container);
 
-    object_initialize(&s->gic, sizeof(s->gic), TYPE_ARM_GIC);
-    qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
-
     object_initialize(&s->scu, sizeof(s->scu), TYPE_A9_SCU);
     qdev_set_parent_bus(DEVICE(&s->scu), sysbus_get_default());
 
+    object_initialize(&s->gic, sizeof(s->gic), TYPE_ARM_GIC);
+    qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
+
     object_initialize(&s->mptimer, sizeof(s->mptimer), TYPE_ARM_MPTIMER);
     qdev_set_parent_bus(DEVICE(&s->mptimer), sysbus_get_default());
 
@@ -41,11 +41,20 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     A9MPPrivState *s = A9MPCORE_PRIV(dev);
-    DeviceState *gicdev, *scudev, *mptimerdev, *wdtdev;
-    SysBusDevice *mptimerbusdev, *wdtbusdev, *gicbusdev, *scubusdev;
+    DeviceState *scudev, *gicdev, *mptimerdev, *wdtdev;
+    SysBusDevice *scubusdev, *gicbusdev, *mptimerbusdev, *wdtbusdev;
     Error *err = NULL;
     int i;
 
+    scudev = DEVICE(&s->scu);
+    qdev_prop_set_uint32(scudev, "num-cpu", s->num_cpu);
+    object_property_set_bool(OBJECT(&s->scu), true, "realized", &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
+    }
+    scubusdev = SYS_BUS_DEVICE(&s->scu);
+
     gicdev = DEVICE(&s->gic);
     qdev_prop_set_uint32(gicdev, "num-cpu", s->num_cpu);
     qdev_prop_set_uint32(gicdev, "num-irq", s->num_irq);
@@ -62,15 +71,6 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
     /* Pass through inbound GPIO lines to the GIC */
     qdev_init_gpio_in(dev, a9mp_priv_set_irq, s->num_irq - 32);
 
-    scudev = DEVICE(&s->scu);
-    qdev_prop_set_uint32(scudev, "num-cpu", s->num_cpu);
-    object_property_set_bool(OBJECT(&s->scu), true, "realized", &err);
-    if (err != NULL) {
-        error_propagate(errp, err);
-        return;
-    }
-    scubusdev = SYS_BUS_DEVICE(&s->scu);
-
     mptimerdev = DEVICE(&s->mptimer);
     qdev_prop_set_uint32(mptimerdev, "num-cpu", s->num_cpu);
     object_property_set_bool(OBJECT(&s->mptimer), true, "realized", &err);
diff --git a/include/hw/cpu/a9mpcore.h b/include/hw/cpu/a9mpcore.h
index 010489b..8eece07 100644
--- a/include/hw/cpu/a9mpcore.h
+++ b/include/hw/cpu/a9mpcore.h
@@ -28,8 +28,8 @@ typedef struct A9MPPrivState {
     MemoryRegion container;
     uint32_t num_irq;
 
-    GICState gic;
     A9SCUState scu;
+    GICState gic;
     ARMMPTimerState mptimer;
     ARMMPTimerState wdt;
 } A9MPPrivState;
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v4 3/4] hw/timer: Introduce ARM A9 Global Timer.

[Peter Crosthwaite]

The ARM A9 MPCore has a timer that is global to all CPUs in the mpcore.
The timer is shared but each CPU has a private independent comparator
and interrupt.

Based on version contributed by Francois LEGAL.

Signed-off-by: François LEGAL <devel@thom.fr.eu.org>
[PC changes:
 * New commit message
 * Re-implemented as single timer model
 * Fixed backwards counting issue in polled mode
 * completed VMSD fields
 * macroified magic numbers (and headerified reg definitions)
 * split of as device-model-only patch
 * use bitops for 64 bit register access
 * Fixed auto increment mode to check condition properly
 * general cleanup (names/style etc).
]
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
Changed sinve v3:
Add seperate CONFIG_A9_GTIMER (PMM review)
Changed since v2 (PMM - review):
Refactored for container embedding
Made frequency scaler consistent with mptimer
Fixed missing VMSD ref_counter field
Fixed missing gtb->inc reset
Changed Memory region names
Changed since v1:
Added /*< private >*/ to struct definition.
Pulled register definitions out into a header (AF review)
SOB Francois LEGAL with PC changes indicated.

 default-configs/arm-softmmu.mak |   1 +
 hw/timer/Makefile.objs          |   1 +
 hw/timer/a9gtimer.c             | 369 ++++++++++++++++++++++++++++++++++++++++
 include/hw/timer/a9gtimer.h     |  97 +++++++++++
 4 files changed, 468 insertions(+)
 create mode 100644 hw/timer/a9gtimer.c
 create mode 100644 include/hw/timer/a9gtimer.h

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index a555eef..e48f102 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -41,6 +41,7 @@ CONFIG_ARM_GIC=y
 CONFIG_ARM_GIC_KVM=$(CONFIG_KVM)
 CONFIG_ARM_TIMER=y
 CONFIG_ARM_MPTIMER=y
+CONFIG_A9_GTIMER=y
 CONFIG_PL011=y
 CONFIG_PL022=y
 CONFIG_PL031=y
diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index eca5905..3ae091c 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -1,5 +1,6 @@
 common-obj-$(CONFIG_ARM_TIMER) += arm_timer.o
 common-obj-$(CONFIG_ARM_MPTIMER) += arm_mptimer.o
+common-obj-$(CONFIG_A9_GTIMER) += a9gtimer.o
 common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
 common-obj-$(CONFIG_DS1338) += ds1338.o
 common-obj-$(CONFIG_HPET) += hpet.o
diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c
new file mode 100644
index 0000000..0f08c67
--- /dev/null
+++ b/hw/timer/a9gtimer.c
@@ -0,0 +1,369 @@
+/*
+ * Global peripheral timer block for ARM A9MP
+ *
+ * (C) 2013 Xilinx Inc.
+ *
+ * Written by François LEGAL
+ * Written by Peter Crosthwaite <peter.crosthwaite@xilinx.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "hw/timer/a9gtimer.h"
+#include "qemu/timer.h"
+#include "qemu/bitops.h"
+#include "qemu/log.h"
+
+#ifndef A9_GTIMER_ERR_DEBUG
+#define A9_GTIMER_ERR_DEBUG 0
+#endif
+
+#define DB_PRINT_L(level, ...) do { \
+    if (A9_GTIMER_ERR_DEBUG > (level)) { \
+        fprintf(stderr,  ": %s: ", __func__); \
+        fprintf(stderr, ## __VA_ARGS__); \
+    } \
+} while (0);
+
+#define DB_PRINT(...) DB_PRINT_L(0, ## __VA_ARGS__)
+
+static inline int a9_gtimer_get_current_cpu(A9GTimerState *s)
+{
+    if (current_cpu->cpu_index >= s->num_cpu) {
+        hw_error("arm_mptimer: num-cpu %d but this cpu is %d!\n",
+                 s->num_cpu, current_cpu->cpu_index);
+    }
+    return current_cpu->cpu_index;
+}
+
+static inline uint64_t a9_gtimer_get_conv(A9GTimerState *s)
+{
+    uint64_t prescale = extract32(s->control, R_CONTROL_PRESCALER_SHIFT,
+                                  R_CONTROL_PRESCALER_LEN);
+
+    return (prescale + 1) * 10;
+}
+
+static A9GTimerUpdate a9_gtimer_get_update(A9GTimerState *s)
+{
+    A9GTimerUpdate ret;
+
+    ret.now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    ret.new = s->ref_counter +
+              (ret.now - s->cpu_ref_time) / a9_gtimer_get_conv(s);
+    return ret;
+}
+
+static void a9_gtimer_update(A9GTimerState *s, bool sync)
+{
+
+    A9GTimerUpdate update = a9_gtimer_get_update(s);
+    int i;
+    int64_t next_cdiff = 0;
+
+    for (i = 0; i < s->num_cpu; ++i) {
+        A9GTimerPerCPU *gtb = &s->per_cpu[i];
+        int64_t cdiff = 0;
+
+        if ((s->control & R_CONTROL_TIMER_ENABLE) &&
+                (gtb->control & R_CONTROL_COMP_ENABLE)) {
+            /* R2p0+, where the compare function is >= */
+            while (gtb->compare < update.new) {
+                DB_PRINT("Compare event happened for CPU %d\n", i);
+                gtb->status = 1;
+                if (gtb->control & R_CONTROL_AUTO_INCREMENT) {
+                    DB_PRINT("Auto incrementing timer compare by %" PRId32 "\n",
+                             gtb->inc);
+                    gtb->compare += gtb->inc;
+                } else {
+                    break;
+                }
+            }
+            cdiff = (int64_t)gtb->compare - (int64_t)update.new + 1;
+            if (cdiff > 0 && (cdiff < next_cdiff || !next_cdiff)) {
+                next_cdiff = cdiff;
+            }
+        }
+
+        qemu_set_irq(gtb->irq,
+                     gtb->status && (gtb->control & R_CONTROL_IRQ_ENABLE));
+    }
+
+    timer_del(s->timer);
+    if (next_cdiff) {
+        DB_PRINT("scheduling qemu_timer to fire again in %"
+                 PRIx64 " cycles\n", next_cdiff);
+        timer_mod(s->timer, update.now + next_cdiff * a9_gtimer_get_conv(s));
+    }
+
+    if (s->control & R_CONTROL_TIMER_ENABLE) {
+        s->counter = update.new;
+    }
+
+    if (sync) {
+        s->cpu_ref_time = update.now;
+        s->ref_counter = s->counter;
+    }
+}
+
+static void a9_gtimer_update_no_sync(void *opaque)
+{
+    A9GTimerState *s = A9_GTIMER(opaque);
+
+    return a9_gtimer_update(s, false);
+}
+
+static uint64_t a9_gtimer_read(void *opaque, hwaddr addr, unsigned size)
+{
+    A9GTimerPerCPU *gtb = (A9GTimerPerCPU *)opaque;
+    A9GTimerState *s = gtb->parent;
+    A9GTimerUpdate update;
+    uint64_t ret = 0;
+    int shift = 0;
+
+    switch (addr) {
+    case R_COUNTER_HI:
+        shift = 32;
+        /* fallthrough */
+    case R_COUNTER_LO:
+        update = a9_gtimer_get_update(s);
+        ret = extract64(update.new, shift, 32);
+        break;
+    case R_CONTROL:
+        ret = s->control | gtb->control;
+        break;
+    case R_INTERRUPT_STATUS:
+        ret = gtb->status;
+        break;
+    case R_COMPARATOR_HI:
+        shift = 32;
+        /* fallthrough */
+    case R_COMPARATOR_LO:
+        ret = extract64(gtb->compare, shift, 32);
+        break;
+    case R_AUTO_INCREMENT:
+        ret =  gtb->inc;
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "bad a9gtimer register: %x\n",
+                      (unsigned)addr);
+        return 0;
+    }
+
+    DB_PRINT("addr:%#x data:%#08" PRIx64 "\n", (unsigned)addr, ret);
+    return ret;
+}
+
+static void a9_gtimer_write(void *opaque, hwaddr addr, uint64_t value,
+                            unsigned size)
+{
+    A9GTimerPerCPU *gtb = (A9GTimerPerCPU *)opaque;
+    A9GTimerState *s = gtb->parent;
+    int shift = 0;
+
+    DB_PRINT("addr:%#x data:%#08" PRIx64 "\n", (unsigned)addr, value);
+
+    switch (addr) {
+    case R_COUNTER_HI:
+        shift = 32;
+        /* fallthrough */
+    case R_COUNTER_LO:
+        /*
+         * Keep it simple - ARM docco explicitly says to disable timer before
+         * modding it, so dont bother trying to do all the difficult on the fly
+         * timer modifications - (if they even work in real hardware??).
+         */
+        if (s->control & R_CONTROL_TIMER_ENABLE) {
+            qemu_log_mask(LOG_GUEST_ERROR, "Cannot mod running ARM gtimer\n");
+            return;
+        }
+        s->counter = deposit64(s->counter, shift, 32, value);
+        return;
+    case R_CONTROL:
+        a9_gtimer_update(s, (value ^ s->control) & R_CONTROL_NEEDS_SYNC);
+        gtb->control = value & R_CONTROL_BANKED;
+        s->control = value & ~R_CONTROL_BANKED;
+        break;
+    case R_INTERRUPT_STATUS:
+        a9_gtimer_update(s, false);
+        gtb->status &= ~value;
+        break;
+    case R_COMPARATOR_HI:
+        shift = 32;
+        /* fallthrough */
+    case R_COMPARATOR_LO:
+        a9_gtimer_update(s, false);
+        gtb->compare = deposit64(gtb->compare, shift, 32, value);
+        break;
+    case R_AUTO_INCREMENT:
+        gtb->inc = value;
+        return;
+    default:
+        return;
+    }
+
+    a9_gtimer_update(s, false);
+}
+
+/* Wrapper functions to implement the "read global timer for
+ * the current CPU" memory regions.
+ */
+static uint64_t a9_gtimer_this_read(void *opaque, hwaddr addr,
+                                    unsigned size)
+{
+    A9GTimerState *s = A9_GTIMER(opaque);
+    int id = a9_gtimer_get_current_cpu(s);
+
+    /* no \n so concatenates with message from read fn */
+    DB_PRINT("CPU:%d:", id);
+
+    return a9_gtimer_read(&s->per_cpu[id], addr, size);
+}
+
+static void a9_gtimer_this_write(void *opaque, hwaddr addr,
+                                 uint64_t value, unsigned size)
+{
+    A9GTimerState *s = A9_GTIMER(opaque);
+    int id = a9_gtimer_get_current_cpu(s);
+
+    /* no \n so concatentates with message from write fn */
+    DB_PRINT("CPU:%d:", id);
+
+    a9_gtimer_write(&s->per_cpu[id], addr, value, size);
+}
+
+static const MemoryRegionOps a9_gtimer_this_ops = {
+    .read = a9_gtimer_this_read,
+    .write = a9_gtimer_this_write,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static const MemoryRegionOps a9_gtimer_ops = {
+    .read = a9_gtimer_read,
+    .write = a9_gtimer_write,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void a9_gtimer_reset(DeviceState *dev)
+{
+    A9GTimerState *s = A9_GTIMER(dev);
+    int i;
+
+    s->counter = 0;
+    s->control = 0;
+
+    for (i = 0; i < s->num_cpu; i++) {
+        A9GTimerPerCPU *gtb = &s->per_cpu[i];
+
+        gtb->control = 0;
+        gtb->status = 0;
+        gtb->compare = 0;
+        gtb->inc = 0;
+    }
+    a9_gtimer_update(s, false);
+}
+
+static void a9_gtimer_realize(DeviceState *dev, Error **errp)
+{
+    A9GTimerState *s = A9_GTIMER(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    int i;
+
+    if (s->num_cpu < 1 || s->num_cpu > A9_GTIMER_MAX_CPUS) {
+        error_setg(errp, "%s: num-cpu must be between 1 and %d\n",
+                   __func__, A9_GTIMER_MAX_CPUS);
+    }
+
+    memory_region_init_io(&s->iomem, OBJECT(dev), &a9_gtimer_this_ops, s,
+                          "a9gtimer shared", 0x20);
+    sysbus_init_mmio(sbd, &s->iomem);
+    s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, a9_gtimer_update_no_sync, s);
+
+    for (i = 0; i < s->num_cpu; i++) {
+        A9GTimerPerCPU *gtb = &s->per_cpu[i];
+
+        gtb->parent = s;
+        sysbus_init_irq(sbd, &gtb->irq);
+        memory_region_init_io(&gtb->iomem, OBJECT(dev), &a9_gtimer_ops, gtb,
+                              "a9gtimer per cpu", 0x20);
+        sysbus_init_mmio(sbd, &gtb->iomem);
+    }
+}
+
+static const VMStateDescription vmstate_a9_gtimer_per_cpu = {
+    .name = "arm.cortex-a9-global-timer.percpu",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(control, A9GTimerPerCPU),
+        VMSTATE_UINT64(compare, A9GTimerPerCPU),
+        VMSTATE_UINT32(status, A9GTimerPerCPU),
+        VMSTATE_UINT32(inc, A9GTimerPerCPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_a9_gtimer = {
+    .name = "arm.cortex-a9-global-timer",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_TIMER(timer, A9GTimerState),
+        VMSTATE_UINT64(counter, A9GTimerState),
+        VMSTATE_UINT64(ref_counter, A9GTimerState),
+        VMSTATE_UINT64(cpu_ref_time, A9GTimerState),
+        VMSTATE_STRUCT_VARRAY_UINT32(per_cpu, A9GTimerState, num_cpu,
+                                     1, vmstate_a9_gtimer_per_cpu,
+                                     A9GTimerPerCPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property a9_gtimer_properties[] = {
+    DEFINE_PROP_UINT32("num-cpu", A9GTimerState, num_cpu, 0),
+    DEFINE_PROP_END_OF_LIST()
+};
+
+static void a9_gtimer_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = a9_gtimer_realize;
+    dc->vmsd = &vmstate_a9_gtimer;
+    dc->reset = a9_gtimer_reset;
+    dc->no_user = 1;
+    dc->props = a9_gtimer_properties;
+}
+
+static const TypeInfo a9_gtimer_info = {
+    .name          = TYPE_A9_GTIMER,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(A9GTimerState),
+    .class_init    = a9_gtimer_class_init,
+};
+
+static void a9_gtimer_register_types(void)
+{
+    type_register_static(&a9_gtimer_info);
+}
+
+type_init(a9_gtimer_register_types)
diff --git a/include/hw/timer/a9gtimer.h b/include/hw/timer/a9gtimer.h
new file mode 100644
index 0000000..b88c02a
--- /dev/null
+++ b/include/hw/timer/a9gtimer.h
@@ -0,0 +1,97 @@
+/*
+ * Global peripheral timer block for ARM A9MP
+ *
+ * (C) 2013 Xilinx Inc.
+ *
+ * Written by François LEGAL
+ * Written by Peter Crosthwaite <peter.crosthwaite@xilinx.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef HW_TIMER_A9_GTIMER_H_H
+#define HW_TIMER_A9_GTIMER_H_H
+
+#include "hw/sysbus.h"
+
+#define A9_GTIMER_MAX_CPUS 4
+
+#define TYPE_A9_GTIMER "arm.cortex-a9-global-timer"
+#define A9_GTIMER(obj) OBJECT_CHECK(A9GTimerState, (obj), TYPE_A9_GTIMER)
+
+#define R_COUNTER_LO                0x00
+#define R_COUNTER_HI                0x04
+
+#define R_CONTROL                   0x08
+#define R_CONTROL_TIMER_ENABLE      (1 << 0)
+#define R_CONTROL_COMP_ENABLE       (1 << 1)
+#define R_CONTROL_IRQ_ENABLE        (1 << 2)
+#define R_CONTROL_AUTO_INCREMENT    (1 << 2)
+#define R_CONTROL_PRESCALER_SHIFT   8
+#define R_CONTROL_PRESCALER_LEN     8
+#define R_CONTROL_PRESCALER_MASK    (((1 << R_CONTROL_PRESCALER_LEN) - 1) << \
+                                     R_CONTROL_PRESCALER_SHIFT)
+
+#define R_CONTROL_BANKED            (R_CONTROL_COMP_ENABLE | \
+                                     R_CONTROL_IRQ_ENABLE | \
+                                     R_CONTROL_AUTO_INCREMENT)
+#define R_CONTROL_NEEDS_SYNC        (R_CONTROL_TIMER_ENABLE | \
+                                     R_CONTROL_PRESCALER_MASK)
+
+#define R_INTERRUPT_STATUS          0x0C
+#define R_COMPARATOR_LO             0x10
+#define R_COMPARATOR_HI             0x14
+#define R_AUTO_INCREMENT            0x18
+
+typedef struct A9GTimerPerCPU A9GTimerPerCPU;
+typedef struct A9GTimerState A9GTimerState;
+
+struct A9GTimerPerCPU {
+    A9GTimerState *parent;
+
+    uint32_t control; /* only per cpu banked bits valid */
+    uint64_t compare;
+    uint32_t status;
+    uint32_t inc;
+
+    MemoryRegion iomem;
+    qemu_irq irq; /* PPI interrupts */
+};
+
+struct A9GTimerState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
+    MemoryRegion iomem;
+    /* static props */
+    uint32_t num_cpu;
+
+    QEMUTimer *timer;
+
+    uint64_t counter; /* current timer value */
+
+    uint64_t ref_counter;
+    uint64_t cpu_ref_time; /* the cpu time as of last update of ref_counter */
+    uint32_t control; /* only non per cpu banked bits valid */
+
+    A9GTimerPerCPU per_cpu[A9_GTIMER_MAX_CPUS];
+};
+
+typedef struct A9GTimerUpdate {
+    uint64_t now;
+    uint64_t new;
+} A9GTimerUpdate;
+
+#endif /* #ifdef HW_TIMER_A9_GTIMER_H_H */
-- 
1.8.4.4

Re: [Qemu-devel] [PATCH arm-devs v4 3/4] hw/timer: Introduce ARM A9 Global Timer.

[Andreas Färber]

Am 02.12.2013 08:36, schrieb Peter Crosthwaite:
> The ARM A9 MPCore has a timer that is global to all CPUs in the mpcore.
> The timer is shared but each CPU has a private independent comparator
> and interrupt.
> 
> Based on version contributed by Francois LEGAL.
> 
> Signed-off-by: François LEGAL <devel@thom.fr.eu.org>
> [PC changes:
>  * New commit message
>  * Re-implemented as single timer model
>  * Fixed backwards counting issue in polled mode
>  * completed VMSD fields
>  * macroified magic numbers (and headerified reg definitions)
>  * split of as device-model-only patch

"off"

>  * use bitops for 64 bit register access
>  * Fixed auto increment mode to check condition properly
>  * general cleanup (names/style etc).
> ]
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> Changed sinve v3:
> Add seperate CONFIG_A9_GTIMER (PMM review)
> Changed since v2 (PMM - review):
> Refactored for container embedding
> Made frequency scaler consistent with mptimer
> Fixed missing VMSD ref_counter field
> Fixed missing gtb->inc reset
> Changed Memory region names
> Changed since v1:
> Added /*< private >*/ to struct definition.
> Pulled register definitions out into a header (AF review)
> SOB Francois LEGAL with PC changes indicated.
> 
>  default-configs/arm-softmmu.mak |   1 +
>  hw/timer/Makefile.objs          |   1 +
>  hw/timer/a9gtimer.c             | 369 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/timer/a9gtimer.h     |  97 +++++++++++
>  4 files changed, 468 insertions(+)
>  create mode 100644 hw/timer/a9gtimer.c
>  create mode 100644 include/hw/timer/a9gtimer.h
> 
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index a555eef..e48f102 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -41,6 +41,7 @@ CONFIG_ARM_GIC=y
>  CONFIG_ARM_GIC_KVM=$(CONFIG_KVM)
>  CONFIG_ARM_TIMER=y
>  CONFIG_ARM_MPTIMER=y
> +CONFIG_A9_GTIMER=y
>  CONFIG_PL011=y
>  CONFIG_PL022=y
>  CONFIG_PL031=y
> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index eca5905..3ae091c 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -1,5 +1,6 @@
>  common-obj-$(CONFIG_ARM_TIMER) += arm_timer.o
>  common-obj-$(CONFIG_ARM_MPTIMER) += arm_mptimer.o
> +common-obj-$(CONFIG_A9_GTIMER) += a9gtimer.o
>  common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
>  common-obj-$(CONFIG_DS1338) += ds1338.o
>  common-obj-$(CONFIG_HPET) += hpet.o
> diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c
> new file mode 100644
> index 0000000..0f08c67
> --- /dev/null
> +++ b/hw/timer/a9gtimer.c
> @@ -0,0 +1,369 @@
> +/*
> + * Global peripheral timer block for ARM A9MP
> + *
> + * (C) 2013 Xilinx Inc.
> + *
> + * Written by François LEGAL
> + * Written by Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "hw/timer/a9gtimer.h"
> +#include "qemu/timer.h"
> +#include "qemu/bitops.h"
> +#include "qemu/log.h"
> +
> +#ifndef A9_GTIMER_ERR_DEBUG
> +#define A9_GTIMER_ERR_DEBUG 0
> +#endif
> +
> +#define DB_PRINT_L(level, ...) do { \
> +    if (A9_GTIMER_ERR_DEBUG > (level)) { \
> +        fprintf(stderr,  ": %s: ", __func__); \
> +        fprintf(stderr, ## __VA_ARGS__); \
> +    } \
> +} while (0);
> +
> +#define DB_PRINT(...) DB_PRINT_L(0, ## __VA_ARGS__)
> +
> +static inline int a9_gtimer_get_current_cpu(A9GTimerState *s)
> +{
> +    if (current_cpu->cpu_index >= s->num_cpu) {
> +        hw_error("arm_mptimer: num-cpu %d but this cpu is %d!\n",

Copy&paste?

> +                 s->num_cpu, current_cpu->cpu_index);
> +    }
> +    return current_cpu->cpu_index;
> +}
> +
> +static inline uint64_t a9_gtimer_get_conv(A9GTimerState *s)
> +{
> +    uint64_t prescale = extract32(s->control, R_CONTROL_PRESCALER_SHIFT,
> +                                  R_CONTROL_PRESCALER_LEN);
> +
> +    return (prescale + 1) * 10;
> +}
> +
> +static A9GTimerUpdate a9_gtimer_get_update(A9GTimerState *s)
> +{
> +    A9GTimerUpdate ret;
> +
> +    ret.now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    ret.new = s->ref_counter +
> +              (ret.now - s->cpu_ref_time) / a9_gtimer_get_conv(s);
> +    return ret;
> +}
> +
> +static void a9_gtimer_update(A9GTimerState *s, bool sync)
> +{
> +
> +    A9GTimerUpdate update = a9_gtimer_get_update(s);
> +    int i;
> +    int64_t next_cdiff = 0;
> +
> +    for (i = 0; i < s->num_cpu; ++i) {
> +        A9GTimerPerCPU *gtb = &s->per_cpu[i];
> +        int64_t cdiff = 0;
> +
> +        if ((s->control & R_CONTROL_TIMER_ENABLE) &&
> +                (gtb->control & R_CONTROL_COMP_ENABLE)) {
> +            /* R2p0+, where the compare function is >= */
> +            while (gtb->compare < update.new) {
> +                DB_PRINT("Compare event happened for CPU %d\n", i);
> +                gtb->status = 1;
> +                if (gtb->control & R_CONTROL_AUTO_INCREMENT) {
> +                    DB_PRINT("Auto incrementing timer compare by %" PRId32 "\n",
> +                             gtb->inc);
> +                    gtb->compare += gtb->inc;
> +                } else {
> +                    break;
> +                }
> +            }
> +            cdiff = (int64_t)gtb->compare - (int64_t)update.new + 1;
> +            if (cdiff > 0 && (cdiff < next_cdiff || !next_cdiff)) {
> +                next_cdiff = cdiff;
> +            }
> +        }
> +
> +        qemu_set_irq(gtb->irq,
> +                     gtb->status && (gtb->control & R_CONTROL_IRQ_ENABLE));
> +    }
> +
> +    timer_del(s->timer);
> +    if (next_cdiff) {
> +        DB_PRINT("scheduling qemu_timer to fire again in %"
> +                 PRIx64 " cycles\n", next_cdiff);
> +        timer_mod(s->timer, update.now + next_cdiff * a9_gtimer_get_conv(s));
> +    }
> +
> +    if (s->control & R_CONTROL_TIMER_ENABLE) {
> +        s->counter = update.new;
> +    }
> +
> +    if (sync) {
> +        s->cpu_ref_time = update.now;
> +        s->ref_counter = s->counter;
> +    }
> +}
> +
> +static void a9_gtimer_update_no_sync(void *opaque)
> +{
> +    A9GTimerState *s = A9_GTIMER(opaque);
> +
> +    return a9_gtimer_update(s, false);
> +}
> +
> +static uint64_t a9_gtimer_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    A9GTimerPerCPU *gtb = (A9GTimerPerCPU *)opaque;
> +    A9GTimerState *s = gtb->parent;
> +    A9GTimerUpdate update;
> +    uint64_t ret = 0;
> +    int shift = 0;
> +
> +    switch (addr) {
> +    case R_COUNTER_HI:
> +        shift = 32;
> +        /* fallthrough */
> +    case R_COUNTER_LO:
> +        update = a9_gtimer_get_update(s);
> +        ret = extract64(update.new, shift, 32);
> +        break;
> +    case R_CONTROL:
> +        ret = s->control | gtb->control;
> +        break;
> +    case R_INTERRUPT_STATUS:
> +        ret = gtb->status;
> +        break;
> +    case R_COMPARATOR_HI:
> +        shift = 32;
> +        /* fallthrough */
> +    case R_COMPARATOR_LO:
> +        ret = extract64(gtb->compare, shift, 32);
> +        break;
> +    case R_AUTO_INCREMENT:
> +        ret =  gtb->inc;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "bad a9gtimer register: %x\n",
> +                      (unsigned)addr);
> +        return 0;
> +    }
> +
> +    DB_PRINT("addr:%#x data:%#08" PRIx64 "\n", (unsigned)addr, ret);
> +    return ret;
> +}
> +
> +static void a9_gtimer_write(void *opaque, hwaddr addr, uint64_t value,
> +                            unsigned size)
> +{
> +    A9GTimerPerCPU *gtb = (A9GTimerPerCPU *)opaque;
> +    A9GTimerState *s = gtb->parent;
> +    int shift = 0;
> +
> +    DB_PRINT("addr:%#x data:%#08" PRIx64 "\n", (unsigned)addr, value);
> +
> +    switch (addr) {
> +    case R_COUNTER_HI:
> +        shift = 32;
> +        /* fallthrough */
> +    case R_COUNTER_LO:
> +        /*
> +         * Keep it simple - ARM docco explicitly says to disable timer before
> +         * modding it, so dont bother trying to do all the difficult on the fly
> +         * timer modifications - (if they even work in real hardware??).
> +         */
> +        if (s->control & R_CONTROL_TIMER_ENABLE) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "Cannot mod running ARM gtimer\n");
> +            return;
> +        }
> +        s->counter = deposit64(s->counter, shift, 32, value);
> +        return;
> +    case R_CONTROL:
> +        a9_gtimer_update(s, (value ^ s->control) & R_CONTROL_NEEDS_SYNC);
> +        gtb->control = value & R_CONTROL_BANKED;
> +        s->control = value & ~R_CONTROL_BANKED;
> +        break;
> +    case R_INTERRUPT_STATUS:
> +        a9_gtimer_update(s, false);
> +        gtb->status &= ~value;
> +        break;
> +    case R_COMPARATOR_HI:
> +        shift = 32;
> +        /* fallthrough */
> +    case R_COMPARATOR_LO:
> +        a9_gtimer_update(s, false);
> +        gtb->compare = deposit64(gtb->compare, shift, 32, value);
> +        break;
> +    case R_AUTO_INCREMENT:
> +        gtb->inc = value;
> +        return;
> +    default:
> +        return;
> +    }
> +
> +    a9_gtimer_update(s, false);
> +}
> +
> +/* Wrapper functions to implement the "read global timer for
> + * the current CPU" memory regions.
> + */
> +static uint64_t a9_gtimer_this_read(void *opaque, hwaddr addr,
> +                                    unsigned size)
> +{
> +    A9GTimerState *s = A9_GTIMER(opaque);
> +    int id = a9_gtimer_get_current_cpu(s);
> +
> +    /* no \n so concatenates with message from read fn */
> +    DB_PRINT("CPU:%d:", id);
> +
> +    return a9_gtimer_read(&s->per_cpu[id], addr, size);
> +}
> +
> +static void a9_gtimer_this_write(void *opaque, hwaddr addr,
> +                                 uint64_t value, unsigned size)
> +{
> +    A9GTimerState *s = A9_GTIMER(opaque);
> +    int id = a9_gtimer_get_current_cpu(s);
> +
> +    /* no \n so concatentates with message from write fn */

"concatenates"

> +    DB_PRINT("CPU:%d:", id);
> +
> +    a9_gtimer_write(&s->per_cpu[id], addr, value, size);
> +}
> +
> +static const MemoryRegionOps a9_gtimer_this_ops = {
> +    .read = a9_gtimer_this_read,
> +    .write = a9_gtimer_this_write,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static const MemoryRegionOps a9_gtimer_ops = {
> +    .read = a9_gtimer_read,
> +    .write = a9_gtimer_write,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void a9_gtimer_reset(DeviceState *dev)
> +{
> +    A9GTimerState *s = A9_GTIMER(dev);
> +    int i;
> +
> +    s->counter = 0;
> +    s->control = 0;
> +
> +    for (i = 0; i < s->num_cpu; i++) {
> +        A9GTimerPerCPU *gtb = &s->per_cpu[i];
> +
> +        gtb->control = 0;
> +        gtb->status = 0;
> +        gtb->compare = 0;
> +        gtb->inc = 0;
> +    }
> +    a9_gtimer_update(s, false);
> +}
> +
> +static void a9_gtimer_realize(DeviceState *dev, Error **errp)
> +{
> +    A9GTimerState *s = A9_GTIMER(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    int i;
> +
> +    if (s->num_cpu < 1 || s->num_cpu > A9_GTIMER_MAX_CPUS) {
> +        error_setg(errp, "%s: num-cpu must be between 1 and %d\n",
> +                   __func__, A9_GTIMER_MAX_CPUS);

return; missing.

> +    }
> +
> +    memory_region_init_io(&s->iomem, OBJECT(dev), &a9_gtimer_this_ops, s,
> +                          "a9gtimer shared", 0x20);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +    s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, a9_gtimer_update_no_sync, s);
> +
> +    for (i = 0; i < s->num_cpu; i++) {
> +        A9GTimerPerCPU *gtb = &s->per_cpu[i];
> +
> +        gtb->parent = s;
> +        sysbus_init_irq(sbd, &gtb->irq);
> +        memory_region_init_io(&gtb->iomem, OBJECT(dev), &a9_gtimer_ops, gtb,
> +                              "a9gtimer per cpu", 0x20);
> +        sysbus_init_mmio(sbd, &gtb->iomem);
> +    }
> +}
> +
> +static const VMStateDescription vmstate_a9_gtimer_per_cpu = {
> +    .name = "arm.cortex-a9-global-timer.percpu",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(control, A9GTimerPerCPU),
> +        VMSTATE_UINT64(compare, A9GTimerPerCPU),
> +        VMSTATE_UINT32(status, A9GTimerPerCPU),
> +        VMSTATE_UINT32(inc, A9GTimerPerCPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription vmstate_a9_gtimer = {
> +    .name = "arm.cortex-a9-global-timer",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_TIMER(timer, A9GTimerState),
> +        VMSTATE_UINT64(counter, A9GTimerState),
> +        VMSTATE_UINT64(ref_counter, A9GTimerState),
> +        VMSTATE_UINT64(cpu_ref_time, A9GTimerState),
> +        VMSTATE_STRUCT_VARRAY_UINT32(per_cpu, A9GTimerState, num_cpu,
> +                                     1, vmstate_a9_gtimer_per_cpu,
> +                                     A9GTimerPerCPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static Property a9_gtimer_properties[] = {
> +    DEFINE_PROP_UINT32("num-cpu", A9GTimerState, num_cpu, 0),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void a9_gtimer_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = a9_gtimer_realize;
> +    dc->vmsd = &vmstate_a9_gtimer;
> +    dc->reset = a9_gtimer_reset;
> +    dc->no_user = 1;

There is a series from Markus waiting on my review that renames and
investigates all uses of this field. Please explain why you are using it
(in form of a comment) or drop it. There's only a numeric property
above, so likely the latter.

On a general note, please be aware that use of HWADDR_PRIx is preferred
over the (unsigned) casts I'm seeing in this patch.

Andreas

> +    dc->props = a9_gtimer_properties;
> +}
> +
> +static const TypeInfo a9_gtimer_info = {
> +    .name          = TYPE_A9_GTIMER,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(A9GTimerState),
> +    .class_init    = a9_gtimer_class_init,
> +};
> +
> +static void a9_gtimer_register_types(void)
> +{
> +    type_register_static(&a9_gtimer_info);
> +}
> +
> +type_init(a9_gtimer_register_types)
> diff --git a/include/hw/timer/a9gtimer.h b/include/hw/timer/a9gtimer.h
> new file mode 100644
> index 0000000..b88c02a
> --- /dev/null
> +++ b/include/hw/timer/a9gtimer.h
> @@ -0,0 +1,97 @@
> +/*
> + * Global peripheral timer block for ARM A9MP
> + *
> + * (C) 2013 Xilinx Inc.
> + *
> + * Written by François LEGAL
> + * Written by Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef HW_TIMER_A9_GTIMER_H_H
> +#define HW_TIMER_A9_GTIMER_H_H
> +
> +#include "hw/sysbus.h"
> +
> +#define A9_GTIMER_MAX_CPUS 4
> +
> +#define TYPE_A9_GTIMER "arm.cortex-a9-global-timer"
> +#define A9_GTIMER(obj) OBJECT_CHECK(A9GTimerState, (obj), TYPE_A9_GTIMER)
> +
> +#define R_COUNTER_LO                0x00
> +#define R_COUNTER_HI                0x04
> +
> +#define R_CONTROL                   0x08
> +#define R_CONTROL_TIMER_ENABLE      (1 << 0)
> +#define R_CONTROL_COMP_ENABLE       (1 << 1)
> +#define R_CONTROL_IRQ_ENABLE        (1 << 2)
> +#define R_CONTROL_AUTO_INCREMENT    (1 << 2)
> +#define R_CONTROL_PRESCALER_SHIFT   8
> +#define R_CONTROL_PRESCALER_LEN     8
> +#define R_CONTROL_PRESCALER_MASK    (((1 << R_CONTROL_PRESCALER_LEN) - 1) << \
> +                                     R_CONTROL_PRESCALER_SHIFT)
> +
> +#define R_CONTROL_BANKED            (R_CONTROL_COMP_ENABLE | \
> +                                     R_CONTROL_IRQ_ENABLE | \
> +                                     R_CONTROL_AUTO_INCREMENT)
> +#define R_CONTROL_NEEDS_SYNC        (R_CONTROL_TIMER_ENABLE | \
> +                                     R_CONTROL_PRESCALER_MASK)
> +
> +#define R_INTERRUPT_STATUS          0x0C
> +#define R_COMPARATOR_LO             0x10
> +#define R_COMPARATOR_HI             0x14
> +#define R_AUTO_INCREMENT            0x18
> +
> +typedef struct A9GTimerPerCPU A9GTimerPerCPU;
> +typedef struct A9GTimerState A9GTimerState;
> +
> +struct A9GTimerPerCPU {
> +    A9GTimerState *parent;
> +
> +    uint32_t control; /* only per cpu banked bits valid */
> +    uint64_t compare;
> +    uint32_t status;
> +    uint32_t inc;
> +
> +    MemoryRegion iomem;
> +    qemu_irq irq; /* PPI interrupts */
> +};
> +
> +struct A9GTimerState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    /*< public >*/
> +
> +    MemoryRegion iomem;
> +    /* static props */
> +    uint32_t num_cpu;
> +
> +    QEMUTimer *timer;
> +
> +    uint64_t counter; /* current timer value */
> +
> +    uint64_t ref_counter;
> +    uint64_t cpu_ref_time; /* the cpu time as of last update of ref_counter */
> +    uint32_t control; /* only non per cpu banked bits valid */
> +
> +    A9GTimerPerCPU per_cpu[A9_GTIMER_MAX_CPUS];
> +};
> +
> +typedef struct A9GTimerUpdate {
> +    uint64_t now;
> +    uint64_t new;
> +} A9GTimerUpdate;
> +
> +#endif /* #ifdef HW_TIMER_A9_GTIMER_H_H */
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH arm-devs v4 3/4] hw/timer: Introduce ARM A9 Global Timer.

[Peter Maydell]

On 2 December 2013 12:28, Andreas Färber <afaerber@suse.de> wrote:
> Am 02.12.2013 08:36, schrieb Peter Crosthwaite:
>> The ARM A9 MPCore has a timer that is global to all CPUs in the mpcore.
>> The timer is shared but each CPU has a private independent comparator
>> and interrupt.
>> +    dc->no_user = 1;
>
> There is a series from Markus waiting on my review that renames and
> investigates all uses of this field. Please explain why you are using it
> (in form of a comment) or drop it. There's only a numeric property
> above, so likely the latter.

It's a sysbus device so it's never going to be usable via device-add.
I think Markus' patchset puts that check in the sysbus base class
though, which would make dropping the no_user here correct.

thanks
-- PMM

Re: [Qemu-devel] [PATCH arm-devs v4 3/4] hw/timer: Introduce ARM A9 Global Timer.

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 2 December 2013 12:28, Andreas Färber <afaerber@suse.de> wrote:
>> Am 02.12.2013 08:36, schrieb Peter Crosthwaite:
>>> The ARM A9 MPCore has a timer that is global to all CPUs in the mpcore.
>>> The timer is shared but each CPU has a private independent comparator
>>> and interrupt.
>>> +    dc->no_user = 1;
>>
>> There is a series from Markus waiting on my review that renames and
>> investigates all uses of this field. Please explain why you are using it
>> (in form of a comment) or drop it. There's only a numeric property
>> above, so likely the latter.
>
> It's a sysbus device so it's never going to be usable via device-add.

Not sure about "never", but it doesn't matter now.

> I think Markus' patchset puts that check in the sysbus base class
> though, which would make dropping the no_user here correct.

Yes.

Re: [Qemu-devel] [PATCH arm-devs v4 3/4] hw/timer: Introduce ARM A9 Global Timer.

[Peter Maydell]

On 2 December 2013 12:28, Andreas Färber <afaerber@suse.de> wrote:
> Am 02.12.2013 08:36, schrieb Peter Crosthwaite:
>> The ARM A9 MPCore has a timer that is global to all CPUs in the mpcore.

Preferred ARM terminology here I think is "all cores in the processor".

("core": one processing unit, something with its own PC
 "cluster": a group of cores
 "processor": the top level thing which might have several cores
    in a single cluster; or which might just have a single core)

I've fixed up this and the minor typos/errors Andreas pointed
out, since I'd already put these patches in target-arm.next.

thanks
-- PMM

[Qemu-devel] [PATCH arm-devs v4 4/4] cpu/a9mpcore: Add Global Timer

[Peter Crosthwaite]

From: François LEGAL <devel@thom.fr.eu.org>

Add the global timer to A9 MPCore.

Signed-off-by: François LEGAL <devel@thom.fr.eu.org>
[PC Changes:
 * new commit message
 * split off original version as a separate patch
 * Rebased against new mpcore implementation (with struct embedding)
]
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
changed from v2:
Rebased against major mpcore changed.
Changed from v1:
Set authorship to Francois and added SOB.

 hw/cpu/a9mpcore.c         | 26 +++++++++++++++++++++-----
 include/hw/cpu/a9mpcore.h |  2 ++
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index a38464b..c09358c 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -30,6 +30,9 @@ static void a9mp_priv_initfn(Object *obj)
     object_initialize(&s->gic, sizeof(s->gic), TYPE_ARM_GIC);
     qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
 
+    object_initialize(&s->gtimer, sizeof(s->gtimer), TYPE_A9_GTIMER);
+    qdev_set_parent_bus(DEVICE(&s->gtimer), sysbus_get_default());
+
     object_initialize(&s->mptimer, sizeof(s->mptimer), TYPE_ARM_MPTIMER);
     qdev_set_parent_bus(DEVICE(&s->mptimer), sysbus_get_default());
 
@@ -41,8 +44,9 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     A9MPPrivState *s = A9MPCORE_PRIV(dev);
-    DeviceState *scudev, *gicdev, *mptimerdev, *wdtdev;
-    SysBusDevice *scubusdev, *gicbusdev, *mptimerbusdev, *wdtbusdev;
+    DeviceState *scudev, *gicdev, *gtimerdev, *mptimerdev, *wdtdev;
+    SysBusDevice *scubusdev, *gicbusdev, *gtimerbusdev, *mptimerbusdev,
+                 *wdtbusdev;
     Error *err = NULL;
     int i;
 
@@ -71,6 +75,15 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
     /* Pass through inbound GPIO lines to the GIC */
     qdev_init_gpio_in(dev, a9mp_priv_set_irq, s->num_irq - 32);
 
+    gtimerdev = DEVICE(&s->gtimer);
+    qdev_prop_set_uint32(gtimerdev, "num-cpu", s->num_cpu);
+    object_property_set_bool(OBJECT(&s->gtimer), true, "realized", &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
+    }
+    gtimerbusdev = SYS_BUS_DEVICE(&s->gtimer);
+
     mptimerdev = DEVICE(&s->mptimer);
     qdev_prop_set_uint32(mptimerdev, "num-cpu", s->num_cpu);
     object_property_set_bool(OBJECT(&s->mptimer), true, "realized", &err);
@@ -97,14 +110,14 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
      *  0x0600-0x06ff -- private timers and watchdogs
      *  0x0700-0x0fff -- nothing
      *  0x1000-0x1fff -- GIC Distributor
-     *
-     * We should implement the global timer but don't currently do so.
      */
     memory_region_add_subregion(&s->container, 0,
                                 sysbus_mmio_get_region(scubusdev, 0));
     /* GIC CPU interface */
     memory_region_add_subregion(&s->container, 0x100,
                                 sysbus_mmio_get_region(gicbusdev, 1));
+    memory_region_add_subregion(&s->container, 0x200,
+                                sysbus_mmio_get_region(gtimerbusdev, 0));
     /* Note that the A9 exposes only the "timer/watchdog for this core"
      * memory region, not the "timer/watchdog for core X" ones 11MPcore has.
      */
@@ -116,10 +129,13 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
                                 sysbus_mmio_get_region(gicbusdev, 0));
 
     /* Wire up the interrupt from each watchdog and timer.
-     * For each core the timer is PPI 29 and the watchdog PPI 30.
+     * For each core the global timer is PPI 27, the private
+     * timer is PPI 29 and the watchdog PPI 30.
      */
     for (i = 0; i < s->num_cpu; i++) {
         int ppibase = (s->num_irq - 32) + i * 32;
+        sysbus_connect_irq(gtimerbusdev, i,
+                           qdev_get_gpio_in(gicdev, ppibase + 27));
         sysbus_connect_irq(mptimerbusdev, i,
                            qdev_get_gpio_in(gicdev, ppibase + 29));
         sysbus_connect_irq(wdtbusdev, i,
diff --git a/include/hw/cpu/a9mpcore.h b/include/hw/cpu/a9mpcore.h
index 8eece07..5d67ca2 100644
--- a/include/hw/cpu/a9mpcore.h
+++ b/include/hw/cpu/a9mpcore.h
@@ -14,6 +14,7 @@
 #include "hw/intc/arm_gic.h"
 #include "hw/misc/a9scu.h"
 #include "hw/timer/arm_mptimer.h"
+#include "hw/timer/a9gtimer.h"
 
 #define TYPE_A9MPCORE_PRIV "a9mpcore_priv"
 #define A9MPCORE_PRIV(obj) \
@@ -30,6 +31,7 @@ typedef struct A9MPPrivState {
 
     A9SCUState scu;
     GICState gic;
+    A9GTimerState gtimer;
     ARMMPTimerState mptimer;
     ARMMPTimerState wdt;
 } A9MPPrivState;
-- 
1.8.4.4

Re: [Qemu-devel] [PATCH arm-devs v4 4/4] cpu/a9mpcore: Add Global Timer

[Andreas Färber]

Hi,

A general observation...

Am 02.12.2013 08:37, schrieb Peter Crosthwaite:
> diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
> index a38464b..c09358c 100644
> --- a/hw/cpu/a9mpcore.c
> +++ b/hw/cpu/a9mpcore.c
> @@ -30,6 +30,9 @@ static void a9mp_priv_initfn(Object *obj)
>      object_initialize(&s->gic, sizeof(s->gic), TYPE_ARM_GIC);
>      qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
>  
> +    object_initialize(&s->gtimer, sizeof(s->gtimer), TYPE_A9_GTIMER);
> +    qdev_set_parent_bus(DEVICE(&s->gtimer), sysbus_get_default());
> +
>      object_initialize(&s->mptimer, sizeof(s->mptimer), TYPE_ARM_MPTIMER);
>      qdev_set_parent_bus(DEVICE(&s->mptimer), sysbus_get_default());
>  

This code resulted from inlining qdev_create() and switching to in-place
instantiation.

Seeing this repetitive code again and again makes me wonder whether we
can just set a SysBusDevice's parent_bus in its instance_init function
to simplify it?

The only downsides I can think of is that SysBus can't instantiate
SysBusDevices in its instance_init then to avoid a circular dependency
and that the global SysBus might get a few refs more if set both in
instance_init and via qdev_set_parent_bus(), but we're not expecting it
to be destroyed anyway. CC'ing Paolo.

Outside the scope of this patch, obviously.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH arm-devs v4 4/4] cpu/a9mpcore: Add Global Timer

[Paolo Bonzini]

Il 02/12/2013 14:08, Andreas Färber ha scritto:
>> > +    object_initialize(&s->gtimer, sizeof(s->gtimer), TYPE_A9_GTIMER);
>> > +    qdev_set_parent_bus(DEVICE(&s->gtimer), sysbus_get_default());
>> > +
>> >      object_initialize(&s->mptimer, sizeof(s->mptimer), TYPE_ARM_MPTIMER);
>> >      qdev_set_parent_bus(DEVICE(&s->mptimer), sysbus_get_default());
>> >  
> This code resulted from inlining qdev_create() and switching to in-place
> instantiation.
> 
> Seeing this repetitive code again and again makes me wonder whether we
> can just set a SysBusDevice's parent_bus in its instance_init function
> to simplify it?

Could we do it instead in the init function, only if parent_bus is still
NULL?

Paolo

Re: [Qemu-devel] [PATCH arm-devs v4 4/4] cpu/a9mpcore: Add Global Timer

[Andreas Färber]

Am 02.12.2013 14:21, schrieb Paolo Bonzini:
> Il 02/12/2013 14:08, Andreas Färber ha scritto:
>>>> +    object_initialize(&s->gtimer, sizeof(s->gtimer), TYPE_A9_GTIMER);
>>>> +    qdev_set_parent_bus(DEVICE(&s->gtimer), sysbus_get_default());
>>>> +
>>>>      object_initialize(&s->mptimer, sizeof(s->mptimer), TYPE_ARM_MPTIMER);
>>>>      qdev_set_parent_bus(DEVICE(&s->mptimer), sysbus_get_default());
>>>>  
>> This code resulted from inlining qdev_create() and switching to in-place
>> instantiation.
>>
>> Seeing this repetitive code again and again makes me wonder whether we
>> can just set a SysBusDevice's parent_bus in its instance_init function
>> to simplify it?
> 
> Could we do it instead in the init function, only if parent_bus is still
> NULL?

I'll interpret that as realize function. ;)

Apart from assuming that no sysbus_*() or qdev_*() API depends on the
bus being non-NULL, converted SysBusDevices have started to assume that
SysBusDevice itself has no realize implementation. (At the time, we'd
have needed to know what it does to determine when to call parent_*.)
It would also make SysBusDevices differ from all others, which I find
not so nice.

Most devices will be instantiated with qdev_create(NULL, ...), so it may
be feasible to just drop the NULL -> sysbus_get_default() conversion
there and for device_add(), leaving only 14 occurrences to convert in
qemu.git. :)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH arm-devs v4 0/4] A9 global timer + mpcore trivials

[Peter Maydell]

On 2 December 2013 07:34, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Hi Peter,
>
> Another spin of the ARM MPCore global timer work. Patches 1 & 2 are some
> trivial cleanup to MPCore I did along the way.

Thanks, applied all to target-arm.next.

-- PMM