On 03/14/2016 05:13 AM, Andrew Jeffery wrote: > Implement basic ASPEED timer functionality for the AST2400 SoC[1]: Up to > 8 timers can independently be configured, enabled, reset and disabled. > Some hardware features are not implemented, namely clock value matching > and pulse generation, but the implementation is enough to boot the Linux > kernel configured with aspeed_defconfig. > > [1] http://www.aspeedtech.com/products.php?fPath=20&rId=376 > > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
Looks good. One stylistic comment and a possible compile break in timer_to_ctrl(). > > --- > Since v3: > * Drop unnecessary mention of VMStateDescription in timer_to_ctrl > description > * Mention hw/timer/a9gtimer.c with respect to clock value matching > * Add missing VMSTATE_END_OF_LIST() to vmstate_aspeed_timer_state > > Since v2: > * Improve handling of timer configuration with respect to enabled state > * Remove redundant enabled member from AspeedTimer > * Implement VMStateDescriptions > * Fix interrupt behaviour (edge triggered, both edges) > * Fix various issues with trace-event declarations > * Include qemu/osdep.h > > Since v1: > * Refactor initialisation of and respect requested clock rates > (APB/External) > * Simplify some index calculations > * Use tracing infrastructure instead of internal DPRINTF > * Enforce access size constraints and alignment in MemoryRegionOps > > default-configs/arm-softmmu.mak | 1 + > hw/timer/Makefile.objs | 1 + > hw/timer/aspeed_timer.c | 451 > ++++++++++++++++++++++++++++++++++++++++ > include/hw/timer/aspeed_timer.h | 59 ++++++ > trace-events | 9 + > 5 files changed, 521 insertions(+) > create mode 100644 hw/timer/aspeed_timer.c > create mode 100644 include/hw/timer/aspeed_timer.h > > diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak > index a9f82a1..2bcd236 100644 > --- a/default-configs/arm-softmmu.mak > +++ b/default-configs/arm-softmmu.mak > @@ -110,3 +110,4 @@ CONFIG_IOH3420=y > CONFIG_I82801B11=y > CONFIG_ACPI=y > CONFIG_SMBIOS=y > +CONFIG_ASPEED_SOC=y > diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs > index 5cfea6e..003c14f 100644 > --- a/hw/timer/Makefile.objs > +++ b/hw/timer/Makefile.objs > @@ -32,3 +32,4 @@ obj-$(CONFIG_MC146818RTC) += mc146818rtc.o > obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o > > common-obj-$(CONFIG_STM32F2XX_TIMER) += stm32f2xx_timer.o > +common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c > new file mode 100644 > index 0000000..0e82178 > --- /dev/null > +++ b/hw/timer/aspeed_timer.c > @@ -0,0 +1,452 @@ > +/* > + * ASPEED AST2400 Timer > + * > + * Andrew Jeffery <and...@aj.id.au> > + * > + * Copyright (C) 2016 IBM Corp. > + * > + * This code is licensed under the GPL version 2 or later. See > + * the COPYING file in the top-level directory. > + */ > + > +#include "qemu/osdep.h" > +#include "hw/ptimer.h" > +#include "hw/sysbus.h" > +#include "hw/timer/aspeed_timer.h" > +#include "qemu-common.h" > +#include "qemu/bitops.h" > +#include "qemu/main-loop.h" > +#include "qemu/timer.h" > +#include "trace.h" > + > +#define TIMER_NR_REGS 4 > + > +#define TIMER_CTRL_BITS 4 > +#define TIMER_CTRL_MASK ((1 << TIMER_CTRL_BITS) - 1) > + > +#define TIMER_CLOCK_USE_EXT true > +#define TIMER_CLOCK_EXT_HZ 1000000 > +#define TIMER_CLOCK_USE_APB false > +#define TIMER_CLOCK_APB_HZ 24000000 > + > +#define TIMER_REG_STATUS 0 > +#define TIMER_REG_RELOAD 1 > +#define TIMER_REG_MATCH_FIRST 2 > +#define TIMER_REG_MATCH_SECOND 3 > + > +#define TIMER_FIRST_CAP_PULSE 4 > + > +enum timer_ctrl_op { > + op_enable = 0, > + op_external_clock, > + op_overflow_interrupt, > + op_pulse_enable > +}; > + > +/** > + * Avoid mutual references between AspeedTimerCtrlState and AspeedTimer > + * structs, as it's a waste of memory. The ptimer BH callback needs to know > + * whether a specific AspeedTimer is enabled, but this information is held in > + * AspeedTimerCtrlState. So, provide a helper to hoist ourselves from an > + * arbitrary AspeedTimer to AspeedTimerCtrlState. > + */ > +static inline struct AspeedTimerCtrlState *timer_to_ctrl(AspeedTimer *t) you can remove the 'struct' above. > +{ > + AspeedTimer (*timers)[] = (void *)t - (t->id * sizeof(*t)); This will not compile on gcc < 5.0. You need to add a 'const' : const AspeedTimer (*timers)[] = (void *)t - (t->id * sizeof(*t)); That should work on all versions. C. > + return container_of(timers, AspeedTimerCtrlState, timers); > +} > + > +static inline bool timer_ctrl_status(AspeedTimer *t, enum timer_ctrl_op op) > +{ > + return !!(timer_to_ctrl(t)->ctrl & BIT(t->id * TIMER_CTRL_BITS + op)); > +} > + > +static inline bool timer_enabled(AspeedTimer *t) > +{ > + return timer_ctrl_status(t, op_enable); > +} > + > +static inline bool timer_overflow_interrupt(AspeedTimer *t) > +{ > + return timer_ctrl_status(t, op_overflow_interrupt); > +} > + > +static inline bool timer_can_pulse(AspeedTimer *t) > +{ > + return t->id >= TIMER_FIRST_CAP_PULSE; > +} > + > +static void aspeed_timer_expire(void *opaque) > +{ > + AspeedTimer *t = opaque; > + > + /* Only support interrupts on match values of zero for the moment - this > is > + * sufficient to boot an aspeed_defconfig Linux kernel. > + * > + * TODO: matching on arbitrary values (see e.g. hw/timer/a9gtimer.c) > + */ > + bool match = !(t->match[0] && t->match[1]); > + bool interrupt = timer_overflow_interrupt(t) || match; > + if (timer_enabled(t) && interrupt) { > + t->level = !t->level; > + qemu_set_irq(t->irq, t->level); > + } > +} > + > +static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg) > +{ > + uint64_t value; > + > + switch (reg) { > + case TIMER_REG_STATUS: > + value = ptimer_get_count(t->timer); > + break; > + case TIMER_REG_RELOAD: > + value = t->reload; > + break; > + case TIMER_REG_MATCH_FIRST: > + case TIMER_REG_MATCH_SECOND: > + value = t->match[reg - 2]; > + break; > + default: > + qemu_log_mask(LOG_UNIMP, "%s: Programming error: unexpected reg: > %d\n", > + __func__, reg); > + value = 0; > + break; > + } > + return value; > +} > + > +static uint64_t aspeed_timer_read(void *opaque, hwaddr offset, unsigned size) > +{ > + AspeedTimerCtrlState *s = opaque; > + const int reg = (offset & 0xf) / 4; > + uint64_t value; > + > + switch (offset) { > + case 0x30: /* Control Register */ > + value = s->ctrl; > + break; > + case 0x34: /* Control Register 2 */ > + value = s->ctrl2; > + break; > + case 0x00 ... 0x2c: /* Timers 1 - 4 */ > + value = aspeed_timer_get_value(&s->timers[(offset >> 4)], reg); > + break; > + case 0x40 ... 0x8c: /* Timers 5 - 8 */ > + value = aspeed_timer_get_value(&s->timers[(offset >> 4) - 1], reg); > + break; > + /* Illegal */ > + case 0x38: > + case 0x3C: > + default: > + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n", > + __func__, offset); > + value = 0; > + break; > + } > + trace_aspeed_timer_read(offset, size, value); > + return value; > +} > + > +static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int > reg, > + uint32_t value) > +{ > + AspeedTimer *t; > + > + g_assert(timer >= 0 && timer < ASPEED_TIMER_NR_TIMERS); > + trace_aspeed_timer_set_value(timer, reg, value); > + t = &s->timers[timer]; > + switch (reg) { > + case TIMER_REG_STATUS: > + if (timer_enabled(t)) { > + ptimer_set_count(t->timer, value); > + } > + break; > + case TIMER_REG_RELOAD: > + t->reload = value; > + ptimer_set_limit(t->timer, value, 1); > + break; > + case TIMER_REG_MATCH_FIRST: > + case TIMER_REG_MATCH_SECOND: > + if (value) { > + /* Non-zero match values are unsupported. As such an interrupt > will > + * always be triggered when the timer reaches zero even if the > + * overflow interrupt control bit is clear. > + */ > + qemu_log_mask(LOG_UNIMP, "%s: Match value unsupported by device: > " > + "0x%" PRIx32 "\n", __func__, value); > + } else { > + t->match[reg - 2] = value; > + } > + break; > + default: > + qemu_log_mask(LOG_UNIMP, "%s: Programming error: unexpected reg: > %d\n", > + __func__, reg); > + break; > + } > +} > + > +/* Control register operations are broken out into helpers that can be > + * explictly called on aspeed_timer_reset(), but also from > + * aspeed_timer_ctrl_op(). > + */ > + > +static void aspeed_timer_ctrl_enable(AspeedTimer *t, bool enable) > +{ > + trace_aspeed_timer_ctrl_enable(t->id, enable); > + if (enable) { > + ptimer_run(t->timer, 0); > + } else { > + ptimer_stop(t->timer); > + ptimer_set_limit(t->timer, t->reload, 1); > + } > +} > + > +static void aspeed_timer_ctrl_external_clock(AspeedTimer *t, bool enable) > +{ > + trace_aspeed_timer_ctrl_external_clock(t->id, enable); > + if (enable) { > + ptimer_set_freq(t->timer, TIMER_CLOCK_EXT_HZ); > + } else { > + ptimer_set_freq(t->timer, TIMER_CLOCK_APB_HZ); > + } > +} > + > +static void aspeed_timer_ctrl_overflow_interrupt(AspeedTimer *t, bool enable) > +{ > + trace_aspeed_timer_ctrl_overflow_interrupt(t->id, enable); > +} > + > +static void aspeed_timer_ctrl_pulse_enable(AspeedTimer *t, bool enable) > +{ > + if (timer_can_pulse(t)) { > + trace_aspeed_timer_ctrl_pulse_enable(t->id, enable); > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: Timer does not support pulse mode\n", __func__); > + } > +} > + > +/** > + * Given the actions are fixed in number and completely described in helper > + * functions, dispatch with a lookup table rather than manage control flow > with > + * a switch statement. > + */ > +static void (*const ctrl_ops[])(AspeedTimer *, bool) = { > + [op_enable] = aspeed_timer_ctrl_enable, > + [op_external_clock] = aspeed_timer_ctrl_external_clock, > + [op_overflow_interrupt] = aspeed_timer_ctrl_overflow_interrupt, > + [op_pulse_enable] = aspeed_timer_ctrl_pulse_enable, > +}; > + > +/** > + * Conditionally affect changes chosen by a timer's control bit. > + * > + * The aspeed_timer_ctrl_op() interface is convenient for the > + * aspeed_timer_set_ctrl() function as the "no change" early exit can be > + * calculated for all operations, which cleans up the caller code. However > the > + * interface isn't convenient for the reset function where we want to enter a > + * specific state without artificially constructing old and new values that > + * will fall through the change guard (and motivates extracting the actions > + * out to helper functions). > + * > + * @t: The timer to manipulate > + * @op: The type of operation to be performed > + * @old: The old state of the timer's control bits > + * @new: The incoming state for the timer's control bits > + */ > +static void aspeed_timer_ctrl_op(AspeedTimer *t, enum timer_ctrl_op op, > + uint8_t old, uint8_t new) > +{ > + const uint8_t mask = BIT(op); > + const bool enable = !!(new & mask); > + const bool changed = ((old ^ new) & mask); > + if (!changed) { > + return; > + } > + ctrl_ops[op](t, enable); > +} > + > +static void aspeed_timer_set_ctrl(AspeedTimerCtrlState *s, uint32_t reg) > +{ > + int i; > + int shift; > + uint8_t t_old, t_new; > + AspeedTimer *t; > + const uint8_t enable_mask = BIT(op_enable); > + > + /* Handle a dependency between the 'enable' and remaining three > + * configuration bits - i.e. if more than one bit in the control set has > + * changed, including the 'enable' bit, then we want either disable the > + * timer and perform configuration, or perform configuration and then > + * enable the timer > + */ > + for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) { > + t = &s->timers[i]; > + shift = (i * TIMER_CTRL_BITS); > + t_old = (s->ctrl >> shift) & TIMER_CTRL_MASK; > + t_new = (reg >> shift) & TIMER_CTRL_MASK; > + > + /* If we are disabling, do so first */ > + if ((t_old & enable_mask) && !(t_new & enable_mask)) { > + aspeed_timer_ctrl_enable(t, false); > + } > + aspeed_timer_ctrl_op(t, op_external_clock, t_old, t_new); > + aspeed_timer_ctrl_op(t, op_overflow_interrupt, t_old, t_new); > + aspeed_timer_ctrl_op(t, op_pulse_enable, t_old, t_new); > + /* If we are enabling, do so last */ > + if (!(t_old & enable_mask) && (t_new & enable_mask)) { > + aspeed_timer_ctrl_enable(t, true); > + } > + } > + s->ctrl = reg; > +} > + > +static void aspeed_timer_set_ctrl2(AspeedTimerCtrlState *s, uint32_t value) > +{ > + trace_aspeed_timer_set_ctrl2(value); > +} > + > +static void aspeed_timer_write(void *opaque, hwaddr offset, uint64_t value, > + unsigned size) > +{ > + const uint32_t tv = (uint32_t)(value & 0xFFFFFFFF); > + const int reg = (offset & 0xf) / 4; > + AspeedTimerCtrlState *s = opaque; > + > + switch (offset) { > + /* Control Registers */ > + case 0x30: > + aspeed_timer_set_ctrl(s, tv); > + break; > + case 0x34: > + aspeed_timer_set_ctrl2(s, tv); > + break; > + /* Timer Registers */ > + case 0x00 ... 0x2c: > + aspeed_timer_set_value(s, (offset >> TIMER_NR_REGS), reg, tv); > + break; > + case 0x40 ... 0x8c: > + aspeed_timer_set_value(s, (offset >> TIMER_NR_REGS) - 1, reg, tv); > + break; > + /* Illegal */ > + case 0x38: > + case 0x3C: > + default: > + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n", > + __func__, offset); > + break; > + } > +} > + > +static const MemoryRegionOps aspeed_timer_ops = { > + .read = aspeed_timer_read, > + .write = aspeed_timer_write, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .valid.min_access_size = 4, > + .valid.max_access_size = 4, > + .valid.unaligned = false, > +}; > + > +static void aspeed_init_one_timer(AspeedTimerCtrlState *s, uint8_t id) > +{ > + QEMUBH *bh; > + AspeedTimer *t = &s->timers[id]; > + > + t->id = id; > + bh = qemu_bh_new(aspeed_timer_expire, t); > + assert(bh); > + t->timer = ptimer_init(bh); > + assert(t->timer); > +} > + > +static void aspeed_timer_realize(DeviceState *dev, Error **errp) > +{ > + int i; > + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > + AspeedTimerCtrlState *s = ASPEED_TIMER(dev); > + > + for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) { > + aspeed_init_one_timer(s, i); > + sysbus_init_irq(sbd, &s->timers[i].irq); > + } > + memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_timer_ops, s, > + TYPE_ASPEED_TIMER, 0x1000); > + sysbus_init_mmio(sbd, &s->iomem); > +} > + > +static void aspeed_timer_reset(DeviceState *dev) > +{ > + int i; > + AspeedTimerCtrlState *s = ASPEED_TIMER(dev); > + > + for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) { > + AspeedTimer *t = &s->timers[i]; > + /* Explictly call helpers to avoid any conditional behaviour through > + * aspeed_timer_set_ctrl(). > + */ > + aspeed_timer_ctrl_enable(t, false); > + aspeed_timer_ctrl_external_clock(t, TIMER_CLOCK_USE_APB); > + aspeed_timer_ctrl_overflow_interrupt(t, false); > + aspeed_timer_ctrl_pulse_enable(t, false); > + t->level = 0; > + t->reload = 0; > + t->match[0] = 0; > + t->match[1] = 0; > + } > + s->ctrl = 0; > + s->ctrl2 = 0; > +} > + > +static const VMStateDescription vmstate_aspeed_timer = { > + .name = "aspeed.timer", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT8(id, AspeedTimer), > + VMSTATE_INT32(level, AspeedTimer), > + VMSTATE_PTIMER(timer, AspeedTimer), > + VMSTATE_UINT32(reload, AspeedTimer), > + VMSTATE_UINT32_ARRAY(match, AspeedTimer, 2), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static const VMStateDescription vmstate_aspeed_timer_state = { > + .name = "aspeed.timerctrl", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(ctrl, AspeedTimerCtrlState), > + VMSTATE_UINT32(ctrl2, AspeedTimerCtrlState), > + VMSTATE_STRUCT_ARRAY(timers, AspeedTimerCtrlState, > + ASPEED_TIMER_NR_TIMERS, 1, vmstate_aspeed_timer, > + AspeedTimer), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static void timer_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->realize = aspeed_timer_realize; > + dc->reset = aspeed_timer_reset; > + dc->desc = "ASPEED Timer"; > + dc->vmsd = &vmstate_aspeed_timer_state; > +} > + > +static const TypeInfo aspeed_timer_info = { > + .name = TYPE_ASPEED_TIMER, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(AspeedTimerCtrlState), > + .class_init = timer_class_init, > +}; > + > +static void aspeed_timer_register_types(void) > +{ > + type_register_static(&aspeed_timer_info); > +} > + > +type_init(aspeed_timer_register_types) > diff --git a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h > new file mode 100644 > index 0000000..44dc2f8 > --- /dev/null > +++ b/include/hw/timer/aspeed_timer.h > @@ -0,0 +1,59 @@ > +/* > + * ASPEED AST2400 Timer > + * > + * Andrew Jeffery <and...@aj.id.au> > + * > + * Copyright (C) 2016 IBM Corp. > + * > + * 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, write to the Free Software Foundation, Inc., > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. > + */ > +#ifndef ASPEED_TIMER_H > +#define ASPEED_TIMER_H > + > +#include "hw/ptimer.h" > + > +#define ASPEED_TIMER(obj) \ > + OBJECT_CHECK(AspeedTimerCtrlState, (obj), TYPE_ASPEED_TIMER); > +#define TYPE_ASPEED_TIMER "aspeed.timer" > +#define ASPEED_TIMER_NR_TIMERS 8 > + > +typedef struct AspeedTimer { > + qemu_irq irq; > + > + uint8_t id; > + > + /** > + * Track the line level as the ASPEED timers implement edge triggered > + * interrupts, signalling with both the rising and falling edge. > + */ > + int32_t level; > + ptimer_state *timer; > + uint32_t reload; > + uint32_t match[2]; > +} AspeedTimer; > + > +typedef struct AspeedTimerCtrlState { > + /*< private >*/ > + SysBusDevice parent; > + > + /*< public >*/ > + MemoryRegion iomem; > + > + uint32_t ctrl; > + uint32_t ctrl2; > + AspeedTimer timers[ASPEED_TIMER_NR_TIMERS]; > +} AspeedTimerCtrlState; > + > +#endif /* ASPEED_TIMER_H */ > diff --git a/trace-events b/trace-events > index 6fba6cc..856425d 100644 > --- a/trace-events > +++ b/trace-events > @@ -1886,3 +1886,12 @@ qio_channel_command_new_pid(void *ioc, int writefd, > int readfd, int pid) "Comman > qio_channel_command_new_spawn(void *ioc, const char *binary, int flags) > "Command new spawn ioc=%p binary=%s flags=%d" > qio_channel_command_abort(void *ioc, int pid) "Command abort ioc=%p pid=%d" > qio_channel_command_wait(void *ioc, int pid, int ret, int status) "Command > abort ioc=%p pid=%d ret=%d status=%d" > + > +# hw/timer/aspeed_timer.c > +aspeed_timer_ctrl_enable(uint8_t i, bool enable) "Timer %" PRIu8 ": %d" > +aspeed_timer_ctrl_external_clock(uint8_t i, bool enable) "Timer %" PRIu8 ": > %d" > +aspeed_timer_ctrl_overflow_interrupt(uint8_t i, bool enable) "Timer %" PRIu8 > ": %d" > +aspeed_timer_ctrl_pulse_enable(uint8_t i, bool enable) "Timer %" PRIu8 ": %d" > +aspeed_timer_set_ctrl2(uint32_t value) "Value: 0x%" PRIx32 > +aspeed_timer_set_value(int timer, int reg, uint32_t value) "Timer %d > register %d: 0x%" PRIx32 > +aspeed_timer_read(uint64_t offset, unsigned size, uint64_t value) "From 0x%" > PRIx64 ": of size %u: 0x%" PRIx64 >