On 15 July 2013 06:19, <peter.crosthwa...@xilinx.com> wrote: > From: Peter Crosthwaite <peter.crosthwa...@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. > > Based on version contributed by Francois LEGAL. > > Signed-off-by: François LEGAL <de...@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 etc). > ] > Signed-off-by: Peter Crosthwaite <peter.crosthwa...@xilinx.com> > --- > Changed since v1: > Added /*< private >*/ to struct definition. > Pulled register definitions out into a header (AF review) > SOB Francois LEGAL with PC changes indicated. > > hw/timer/Makefile.objs | 2 +- > hw/timer/a9gtimer.c | 412 > ++++++++++++++++++++++++++++++++++++++++++++ > include/hw/timer/a9gtimer.h | 33 ++++ > 3 files changed, 446 insertions(+), 1 deletion(-) > create mode 100644 hw/timer/a9gtimer.c > create mode 100644 include/hw/timer/a9gtimer.h > > diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs > index eca5905..42d96df 100644 > --- a/hw/timer/Makefile.objs > +++ b/hw/timer/Makefile.objs > @@ -1,5 +1,5 @@ > common-obj-$(CONFIG_ARM_TIMER) += arm_timer.o > -common-obj-$(CONFIG_ARM_MPTIMER) += arm_mptimer.o > +common-obj-$(CONFIG_ARM_MPTIMER) += arm_mptimer.o a9gtimer.o
As I understand it, a new device should have its own CONFIG_WHATEVER switch set in default-configs/arm-softmmu.mak; we shouldn't share them. (The eventual idea is that you might want to disable some boards, in which case a config with only 11mpcore boards and no a9 boards would want the arm_mptimer but not the a9gtimer compiled.) > 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..72ebeba > --- /dev/null > +++ b/hw/timer/a9gtimer.c > @@ -0,0 +1,412 @@ > +/* > + * Global peripheral timer block for ARM A9MP > + * > + * (C) 2013 Xilinx Inc. > + * > + * Written by François LEGAL > + * Written by Peter Crosthwaite <peter.crosthwa...@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" > + > +#include "hw/timer/a9gtimer.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__) > + > +#define MAX_CPUS 4 > + > +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 { > + /*< 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 > */ Is it really correct that we migrate one of these fields but not the other, and don't clear either on reset? > + 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) > +{ > + 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(A9GlobalTimerState *s) > +{ > + uint64_t prescale = extract32(s->control, R_CONTROL_PRESCALER_SHIFT, > + R_CONTROL_PRESCALER_LEN); > + > + return (prescale + 1) * 100; Why do we use * 100 here, but only * 10 in the arm_mptimer scaling function? > +} > + > +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 > */ The TRM says it's "greater than or equal to". I think your code is correct, but this comment is wrong. > + 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; It looks odd that we're doing our "time to next comparison" in signed arithmetic, but our "have we fired yet?" check above is doing an unsigned comparison. > + 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_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??). > + */ I'm pretty sure the remarks in the TRM are intended as a note to software authors about how to reliably modify the timer, given it's a 64 bit field that you can only write 32 bits at a time. I haven't tested but you'd probably have to go out of your way to make the h/w not just update the appropriate 32 bits... I'm happy with this log-and-do-nothing implementation though. > + 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; > +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; Missing gtb->inc reset. > + } > + 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); This makes the memory region pretty much anonymous in the debugger: (gdb) print s->iomem $1 = {ops = 0x555555dca4a0, iommu_ops = 0x0, opaque = 0x5555563e5ab0, owner = 0x0, parent = 0x0, size = {lo = 32, hi = 0}, addr = 0, destructor = 0x5555559420d7 <memory_region_destructor_none>, ram_addr = 18446744073709551615, subpage = false, terminates = true, romd_mode = true, ram = false, readonly = false, enabled = true, rom_device = false, warning_printed = false, flush_coalesced_mmio = false, alias = 0x0, alias_offset = 0, priority = 0, may_overlap = false, subregions = {tqh_first = 0x0, tqh_last = 0x5555563e7e50}, subregions_link = {tqe_next = 0x0, tqe_prev = 0x0}, coalesced = {tqh_first = 0x0, tqh_last = 0x5555563e7e70}, name = 0x555556348620 "MMIO shared", dirty_log_mask = 0 '\000', ioeventfd_nb = 0, ioeventfds = 0x0, iommu_notify = {notifiers = {lh_first = 0x0}}} which isn't very nice given that debugging is the main reason for naming MMIO regions. Can we have something slightly less ambiguous, please? > diff --git a/include/hw/timer/a9gtimer.h b/include/hw/timer/a9gtimer.h > new file mode 100644 > index 0000000..f4c1499 > --- /dev/null > +++ b/include/hw/timer/a9gtimer.h > @@ -0,0 +1,33 @@ > +#ifndef A9GTIMER_H > +#define A9GTIMER_H > + > +#include "qom/object.h" > + > +#define TYPE_ARM_GTIMER "arm.cortex-a9-global-timer" This doesn't match the name you pass to qdev_create() in patch 2, and so we assert on startup. > +#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 Shouldn't most of these defines be in the .c file, given that they're only needed by the implementation, not by users of the device ? thanks -- PMM
