All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH arm-devs v1 1/2] hw/timer: Introduce ARM A9 Global Timer.
@ 2013-07-10  5:08 peter.crosthwaite
  2013-07-10  5:08 ` [Qemu-devel] [PATCH arm-devs v1 2/2] cpu/a9mpcore: Add " peter.crosthwaite
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: peter.crosthwaite @ 2013-07-10  5:08 UTC (permalink / raw)
  To: peter.maydell; +Cc: devel, afaerber, qemu-devel

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

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.

Original version contributed by Francois LEGAL.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
Francois, do you want to re-add your SOB? I have changed the device
a lot since your V2.

 hw/timer/Makefile.objs |   2 +-
 hw/timer/a9gtimer.c    | 437 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 438 insertions(+), 1 deletion(-)
 create mode 100644 hw/timer/a9gtimer.c

diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index 32b5c1a..8ed379e 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -25,5 +25,5 @@ obj-$(CONFIG_PXA2XX) += pxa2xx_timer.o
 obj-$(CONFIG_SH4) += sh_timer.o
 obj-$(CONFIG_TUSB6010) += tusb6010.o
 
-obj-$(CONFIG_ARM_MPTIMER) += arm_mptimer.o
+obj-$(CONFIG_ARM_MPTIMER) += arm_mptimer.o a9gtimer.o
 obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c
new file mode 100644
index 0000000..f996169
--- /dev/null
+++ b/hw/timer/a9gtimer.c
@@ -0,0 +1,437 @@
+/*
+ * 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/sysbus.h"
+#include "qemu/timer.h"
+#include "qemu/bitops.h"
+#include "qemu/log.h"
+
+#ifndef ARM_GTIMER_ERR_DEBUG
+#define ARM_GTIMER_ERR_DEBUG 0
+#endif
+
+#define DB_PRINT_L(level, ...) do { \
+    if (ARM_GTIMER_ERR_DEBUG > (level)) { \
+        fprintf(stderr,  ": %s: ", __func__); \
+        fprintf(stderr, ## __VA_ARGS__); \
+    } \
+} while (0);
+
+#define DB_PRINT(...) DB_PRINT_L(0, ## __VA_ARGS__)
+
+/* This device implements the Cortex-A9MP global timer. */
+
+#define MAX_CPUS 4
+#define TYPE_ARM_GTIMER "arm.cortex-a9-global-timer"
+#define ARM_GTIMER(obj) OBJECT_CHECK(A9GlobalTimerState, (obj), TYPE_ARM_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 A9GlobalTimerPerCPU A9GlobalTimerPerCPU;
+typedef struct A9GlobalTimerState A9GlobalTimerState;
+
+struct A9GlobalTimerPerCPU {
+    A9GlobalTimerState *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 A9GlobalTimerState {
+    SysBusDevice parent_obj;
+
+    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 */
+
+    A9GlobalTimerPerCPU per_cpu[MAX_CPUS];
+};
+
+typedef struct A9GTimerUpdate {
+    uint64_t now;
+    uint64_t new;
+} A9GTimerUpdate;
+
+static inline int a9_gtimer_get_current_cpu(A9GlobalTimerState *s)
+{
+    CPUState *cpu_single_cpu = ENV_GET_CPU(cpu_single_env);
+
+    if (cpu_single_cpu->cpu_index >= s->num_cpu) {
+        hw_error("arm_mptimer: num-cpu %d but this cpu is %d!\n",
+                 s->num_cpu, cpu_single_cpu->cpu_index);
+    }
+    return cpu_single_cpu->cpu_index;
+}
+
+static inline uint64_t a9_gtimer_get_conv(A9GlobalTimerState *s)
+{
+    uint64_t prescale = extract32(s->control, R_CONTROL_PRESCALER_SHIFT,
+                                  R_CONTROL_PRESCALER_LEN);
+
+    return (prescale + 1) * 100;
+}
+
+static A9GTimerUpdate a9_gtimer_get_update(A9GlobalTimerState *s)
+{
+    A9GTimerUpdate ret;
+
+    ret.now = qemu_get_clock_ns(vm_clock);
+    ret.new = s->ref_counter +
+              (ret.now - s->cpu_ref_time) / a9_gtimer_get_conv(s);
+    return ret;
+}
+
+static void a9_gtimer_update(A9GlobalTimerState *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) {
+        A9GlobalTimerPerCPU *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));
+    }
+
+    qemu_del_timer(s->timer);
+    if (next_cdiff) {
+        DB_PRINT("scheduling qemu_timer to fire again in %"
+                 PRIx64 " cycles\n", next_cdiff);
+        qemu_mod_timer(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)
+{
+    A9GlobalTimerState *s = ARM_GTIMER(opaque);
+
+    return a9_gtimer_update(s, false);
+}
+
+static uint64_t a9_gtimer_read(void *opaque, hwaddr addr, unsigned size)
+{
+    A9GlobalTimerPerCPU *gtb = (A9GlobalTimerPerCPU *)opaque;
+    A9GlobalTimerState *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)
+{
+    A9GlobalTimerPerCPU *gtb = (A9GlobalTimerPerCPU *)opaque;
+    A9GlobalTimerState *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)
+{
+    A9GlobalTimerState *s = ARM_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)
+{
+    A9GlobalTimerState *s = ARM_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)
+{
+    A9GlobalTimerState *s = ARM_GTIMER(dev);
+    int i;
+
+    s->counter = 0;
+    s->control = 0;
+
+    for (i = 0; i < s->num_cpu; i++) {
+        A9GlobalTimerPerCPU *gtb = &s->per_cpu[i];
+
+        gtb->control = 0;
+        gtb->status = 0;
+        gtb->compare = 0;
+    }
+    a9_gtimer_update(s, false);
+}
+
+static void a9_gtimer_realize(DeviceState *dev, Error **errp)
+{
+    A9GlobalTimerState *s = ARM_GTIMER(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    int i;
+
+    if (s->num_cpu < 1 || s->num_cpu > MAX_CPUS) {
+        error_setg(errp, "%s: num-cpu must be between 1 and %d\n",
+                   __func__, MAX_CPUS);
+    }
+
+    memory_region_init_io(&s->iomem, OBJECT(dev), &a9_gtimer_this_ops, s,
+                          "MMIO shared", 0x20);
+    sysbus_init_mmio(sbd, &s->iomem);
+    s->timer = qemu_new_timer_ns(vm_clock, a9_gtimer_update_no_sync, s);
+
+    for (i = 0; i < s->num_cpu; i++) {
+        A9GlobalTimerPerCPU *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,
+                              "MMIO 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, A9GlobalTimerPerCPU),
+        VMSTATE_UINT64(compare, A9GlobalTimerPerCPU),
+        VMSTATE_UINT32(status, A9GlobalTimerPerCPU),
+        VMSTATE_UINT32(inc, A9GlobalTimerPerCPU),
+        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, A9GlobalTimerState),
+        VMSTATE_UINT64(counter, A9GlobalTimerState),
+        VMSTATE_UINT64(cpu_ref_time, A9GlobalTimerState),
+        VMSTATE_STRUCT_VARRAY_UINT32(per_cpu, A9GlobalTimerState, num_cpu,
+                                     1, vmstate_a9_gtimer_per_cpu,
+                                     A9GlobalTimerPerCPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property a9_gtimer_properties[] = {
+    DEFINE_PROP_UINT32("num-cpu", A9GlobalTimerState, 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_ARM_GTIMER,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(A9GlobalTimerState),
+    .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)
-- 
1.8.3.rc1.44.gb387c77.dirty

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH arm-devs v1 2/2] cpu/a9mpcore: Add Global Timer
  2013-07-10  5:08 [Qemu-devel] [PATCH arm-devs v1 1/2] hw/timer: Introduce ARM A9 Global Timer peter.crosthwaite
@ 2013-07-10  5:08 ` peter.crosthwaite
  2013-07-10  7:41 ` [Qemu-devel] [PATCH arm-devs v1 1/2] hw/timer: Introduce ARM A9 " Peter Maydell
  2013-07-10 14:56 ` Andreas Färber
  2 siblings, 0 replies; 6+ messages in thread
From: peter.crosthwaite @ 2013-07-10  5:08 UTC (permalink / raw)
  To: peter.maydell; +Cc: devel, afaerber, qemu-devel

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Add the global timer to A9 MPCore.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
This is From Francois LEGALs original patch (never had it as git patch
so has my authorship). Whats the protocol here?

Francois can you ack this?

This will likely conflict with Andreas' MPCore work.

 hw/cpu/a9mpcore.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index 6c00a59..56d101e 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -15,6 +15,7 @@ typedef struct A9MPPrivState {
     uint32_t num_cpu;
     MemoryRegion container;
     DeviceState *mptimer;
+    DeviceState *mpgtimer;
     DeviceState *wdt;
     DeviceState *gic;
     DeviceState *scu;
@@ -31,6 +32,7 @@ static int a9mp_priv_init(SysBusDevice *dev)
 {
     A9MPPrivState *s = FROM_SYSBUS(A9MPPrivState, dev);
     SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev, *scubusdev;
+    SysBusDevice *gtimerbusdev;
     int i;
 
     s->gic = qdev_create(NULL, "arm_gic");
@@ -50,6 +52,11 @@ static int a9mp_priv_init(SysBusDevice *dev)
     qdev_init_nofail(s->scu);
     scubusdev = SYS_BUS_DEVICE(s->scu);
 
+    s->mpgtimer = qdev_create(NULL, "a9_globaltimer");
+    qdev_prop_set_uint32(s->mpgtimer, "num-cpu", s->num_cpu);
+    qdev_init_nofail(s->mpgtimer);
+    gtimerbusdev = SYS_BUS_DEVICE(s->mpgtimer);
+
     s->mptimer = qdev_create(NULL, "arm_mptimer");
     qdev_prop_set_uint32(s->mptimer, "num-cpu", s->num_cpu);
     qdev_init_nofail(s->mptimer);
@@ -68,8 +75,6 @@ static int a9mp_priv_init(SysBusDevice *dev)
      *  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_init(&s->container, OBJECT(s), "a9mp-priv-container", 0x2000);
     memory_region_add_subregion(&s->container, 0,
@@ -80,6 +85,8 @@ static int a9mp_priv_init(SysBusDevice *dev)
     /* Note that the A9 exposes only the "timer/watchdog for this core"
      * memory region, not the "timer/watchdog for core X" ones 11MPcore has.
      */
+    memory_region_add_subregion(&s->container, 0x200,
+                                sysbus_mmio_get_region(gtimerbusdev, 0));
     memory_region_add_subregion(&s->container, 0x600,
                                 sysbus_mmio_get_region(timerbusdev, 0));
     memory_region_add_subregion(&s->container, 0x620,
@@ -90,10 +97,13 @@ static int a9mp_priv_init(SysBusDevice *dev)
     sysbus_init_mmio(dev, &s->container);
 
     /* 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(s->gic, ppibase + 27));
         sysbus_connect_irq(timerbusdev, i,
                            qdev_get_gpio_in(s->gic, ppibase + 29));
         sysbus_connect_irq(wdtbusdev, i,
-- 
1.8.3.rc1.44.gb387c77.dirty

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH arm-devs v1 1/2] hw/timer: Introduce ARM A9 Global Timer.
  2013-07-10  5:08 [Qemu-devel] [PATCH arm-devs v1 1/2] hw/timer: Introduce ARM A9 Global Timer peter.crosthwaite
  2013-07-10  5:08 ` [Qemu-devel] [PATCH arm-devs v1 2/2] cpu/a9mpcore: Add " peter.crosthwaite
@ 2013-07-10  7:41 ` Peter Maydell
  2013-07-10  8:25   ` Andreas Färber
  2013-07-10 14:56 ` Andreas Färber
  2 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2013-07-10  7:41 UTC (permalink / raw)
  To: peter.crosthwaite; +Cc: devel, afaerber, qemu-devel

On 10 July 2013 06:08,  <peter.crosthwaite@xilinx.com> wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> 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.
>
> Original version contributed by Francois LEGAL.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> Francois, do you want to re-add your SOB? I have changed the device
> a lot since your V2.

The usual way to indicate this (assuming Francois signed off the
original patchset) is something like:

  Signed-off-by: Original Author
  [You: <short description of subsequent changes>]
  Signed-off-by: Your Name

http://patchwork.ozlabs.org/patch/255071/ has an example.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH arm-devs v1 1/2] hw/timer: Introduce ARM A9 Global Timer.
  2013-07-10  7:41 ` [Qemu-devel] [PATCH arm-devs v1 1/2] hw/timer: Introduce ARM A9 " Peter Maydell
@ 2013-07-10  8:25   ` Andreas Färber
  2013-07-10 14:07     ` Peter Crosthwaite
  0 siblings, 1 reply; 6+ messages in thread
From: Andreas Färber @ 2013-07-10  8:25 UTC (permalink / raw)
  To: peter.crosthwaite; +Cc: devel, Peter Maydell, qemu-devel, Markus Armbruster

Am 10.07.2013 09:41, schrieb Peter Maydell:
> On 10 July 2013 06:08,  <peter.crosthwaite@xilinx.com> wrote:
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> 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.
>>
>> Original version contributed by Francois LEGAL.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>> Francois, do you want to re-add your SOB? I have changed the device
>> a lot since your V2.

If you have a DCO for a patch you're reusing, you should never "drop" it
from your patch. You can still leave the textual explanation there.

> The usual way to indicate this (assuming Francois signed off the
> original patchset) is something like:
> 
>   Signed-off-by: Original Author
>   [You: <short description of subsequent changes>]
>   Signed-off-by: Your Name

Add to that, the difference between From and SOB indicates that you did
more than just cosmetic touches. For example, when I wrote from scratch
a new SoC QOM object I used my From and the original author's Sob (to
put the blame on me while recording the DCO), whereas when I purely
updated code to MemoryRegions, Coding Style, QOM casts, etc. I use the
original author's From and [AF: ...].

Andreas

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH arm-devs v1 1/2] hw/timer: Introduce ARM A9 Global Timer.
  2013-07-10  8:25   ` Andreas Färber
@ 2013-07-10 14:07     ` Peter Crosthwaite
  0 siblings, 0 replies; 6+ messages in thread
From: Peter Crosthwaite @ 2013-07-10 14:07 UTC (permalink / raw)
  To: Andreas Färber; +Cc: devel, Peter Maydell, qemu-devel, Markus Armbruster

Hi Andreas,

On Wed, Jul 10, 2013 at 6:25 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 10.07.2013 09:41, schrieb Peter Maydell:
>> On 10 July 2013 06:08,  <peter.crosthwaite@xilinx.com> wrote:
>>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>>
>>> 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.
>>>
>>> Original version contributed by Francois LEGAL.
>>>
>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>> ---
>>> Francois, do you want to re-add your SOB? I have changed the device
>>> a lot since your V2.
>
> If you have a DCO for a patch you're reusing, you should never "drop" it
> from your patch. You can still leave the textual explanation there.
>
>> The usual way to indicate this (assuming Francois signed off the
>> original patchset) is something like:
>>
>>   Signed-off-by: Original Author
>>   [You: <short description of subsequent changes>]
>>   Signed-off-by: Your Name
>
> Add to that, the difference between From and SOB indicates that you did
> more than just cosmetic touches. For example, when I wrote from scratch
> a new SoC QOM object I used my From and the original author's Sob (to
> put the blame on me while recording the DCO), whereas when I purely
> updated code to MemoryRegions, Coding Style, QOM casts, etc. I use the
> original author's From and [AF: ...].

This is much more than a stylistic change, its very heavily refactored. I think
its better to --reset-author but ill bring the SOB accross. Whereas with P2
all I have done is split of the MPCore changes from original file verbatim, so
I guess that should be original author.

Regards,
Peter

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH arm-devs v1 1/2] hw/timer: Introduce ARM A9 Global Timer.
  2013-07-10  5:08 [Qemu-devel] [PATCH arm-devs v1 1/2] hw/timer: Introduce ARM A9 Global Timer peter.crosthwaite
  2013-07-10  5:08 ` [Qemu-devel] [PATCH arm-devs v1 2/2] cpu/a9mpcore: Add " peter.crosthwaite
  2013-07-10  7:41 ` [Qemu-devel] [PATCH arm-devs v1 1/2] hw/timer: Introduce ARM A9 " Peter Maydell
@ 2013-07-10 14:56 ` Andreas Färber
  2 siblings, 0 replies; 6+ messages in thread
From: Andreas Färber @ 2013-07-10 14:56 UTC (permalink / raw)
  To: peter.crosthwaite; +Cc: devel, peter.maydell, qemu-devel

Am 10.07.2013 07:08, schrieb peter.crosthwaite@xilinx.com:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> 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.
> 
> Original version contributed by Francois LEGAL.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> Francois, do you want to re-add your SOB? I have changed the device
> a lot since your V2.
> 
>  hw/timer/Makefile.objs |   2 +-
>  hw/timer/a9gtimer.c    | 437 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 438 insertions(+), 1 deletion(-)
>  create mode 100644 hw/timer/a9gtimer.c
> 
> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index 32b5c1a..8ed379e 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -25,5 +25,5 @@ obj-$(CONFIG_PXA2XX) += pxa2xx_timer.o
>  obj-$(CONFIG_SH4) += sh_timer.o
>  obj-$(CONFIG_TUSB6010) += tusb6010.o
>  
> -obj-$(CONFIG_ARM_MPTIMER) += arm_mptimer.o
> +obj-$(CONFIG_ARM_MPTIMER) += arm_mptimer.o a9gtimer.o
>  obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
> diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c
> new file mode 100644
> index 0000000..f996169
> --- /dev/null
> +++ b/hw/timer/a9gtimer.c
> @@ -0,0 +1,437 @@
> +/*
> + * 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/sysbus.h"
> +#include "qemu/timer.h"
> +#include "qemu/bitops.h"
> +#include "qemu/log.h"
> +
> +#ifndef ARM_GTIMER_ERR_DEBUG
> +#define ARM_GTIMER_ERR_DEBUG 0
> +#endif
> +
> +#define DB_PRINT_L(level, ...) do { \
> +    if (ARM_GTIMER_ERR_DEBUG > (level)) { \
> +        fprintf(stderr,  ": %s: ", __func__); \
> +        fprintf(stderr, ## __VA_ARGS__); \
> +    } \
> +} while (0);
> +
> +#define DB_PRINT(...) DB_PRINT_L(0, ## __VA_ARGS__)

I only spot DB_PRINT() being used below. Would be easier to grasp if it
were just #ifndef ARM_GTIMER_ERR_DEBUG #define
ARM_GTIMER_ERR_DEBUG_ENABLED 0 and if (ARM_GTIMER_ERR_DEBUG_ENABLED).

Either something is missing before ": %s: " or that's a duplicate.

> +
> +/* This device implements the Cortex-A9MP global timer. */
> +
> +#define MAX_CPUS 4
> +#define TYPE_ARM_GTIMER "arm.cortex-a9-global-timer"
> +#define ARM_GTIMER(obj) OBJECT_CHECK(A9GlobalTimerState, (obj), TYPE_ARM_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 A9GlobalTimerPerCPU A9GlobalTimerPerCPU;
> +typedef struct A9GlobalTimerState A9GlobalTimerState;
> +
> +struct A9GlobalTimerPerCPU {
> +    A9GlobalTimerState *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 A9GlobalTimerState {
> +    SysBusDevice parent_obj;
> +
> +    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 */
> +
> +    A9GlobalTimerPerCPU per_cpu[MAX_CPUS];
> +};

Can we move these to a new header from the start, while not using them
elsewhere yet? In that case please add the gtk-doc private/public markers.

> +
> +typedef struct A9GTimerUpdate {
> +    uint64_t now;
> +    uint64_t new;
> +} A9GTimerUpdate;
> +
> +static inline int a9_gtimer_get_current_cpu(A9GlobalTimerState *s)
> +{
> +    CPUState *cpu_single_cpu = ENV_GET_CPU(cpu_single_env);
> +
> +    if (cpu_single_cpu->cpu_index >= s->num_cpu) {
> +        hw_error("arm_mptimer: num-cpu %d but this cpu is %d!\n",
> +                 s->num_cpu, cpu_single_cpu->cpu_index);
> +    }
> +    return cpu_single_cpu->cpu_index;
> +}
[snip]

Seeing this, I have sent out my qom-cpu PULL, which replaces
cpu_single_env with current_cpu for your convenience.

Otherwise looks okay from a QOM/style viewpoint.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2013-07-10 14:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-10  5:08 [Qemu-devel] [PATCH arm-devs v1 1/2] hw/timer: Introduce ARM A9 Global Timer peter.crosthwaite
2013-07-10  5:08 ` [Qemu-devel] [PATCH arm-devs v1 2/2] cpu/a9mpcore: Add " peter.crosthwaite
2013-07-10  7:41 ` [Qemu-devel] [PATCH arm-devs v1 1/2] hw/timer: Introduce ARM A9 " Peter Maydell
2013-07-10  8:25   ` Andreas Färber
2013-07-10 14:07     ` Peter Crosthwaite
2013-07-10 14:56 ` Andreas Färber

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.