Message ID | d9e91eca501f8ce54e5042dc80b2ee35ecdc883f.1373862169.git.peter.crosthwaite@xilinx.com |
---|---|
State | New |
Headers | show |
On 15 July 2013 06:19, <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. > > 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 etc). > ] > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@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.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" > + > +#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
On Mon, Jul 15, 2013 at 11:20 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 15 July 2013 06:19, <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. >> >> 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 etc). >> ] >> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@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.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" >> + >> +#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, Migrated ref_counter. Thanks. > and don't clear either on reset? > These don't matter on reset as they are only used for differential time calculation for a running timer. They are guest invisible. Reset de-activates the timer which implictly invalidates them. They can only be set again by starting a stopped timer which refreshes them. >> + 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? > Changed to * 10 for consistency. >> +} >> + >> +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. > Fixed. >> + 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. > Fixed. >> + } >> + 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? > I was running on the assumption that Paolo's memory_region owner field would be used to generate nice names for which IP owns the memory region. Then this name is used for which region with the IP. But fixed. >> 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. > Fixed >> +#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 ? > My understanding is that programmers model belongs in the header for the sake of qtest. Regards, Peter > thanks > -- PMM >
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 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.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" + +#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 */ + 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; +} + +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, >b->irq); + memory_region_init_io(>b->iomem, OBJECT(dev), &a9_gtimer_ops, gtb, + "MMIO per cpu", 0x20); + sysbus_init_mmio(sbd, >b->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) 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" +#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 + +#endif /* #ifdef A9GTIMER_H */