[PATCH 1/3] timers: Add ASPEED AST2400 timer device model
Cédric Le Goater
clg at fr.ibm.com
Fri Feb 12 01:32:49 AEDT 2016
Hello Andrew,
Some comments below.
On 02/03/2016 06:46 AM, Andrew Jeffery wrote:
> Implement basic AST2400 timer functionality: Timers can be configured,
> enabled, reset and disabled.
>
> A number of hardware features are not implemented:
>
> * Clock selection (internal vs external)
> * Timer Overflow interrupts
> * Clock value matching
> * Pulse generation
>
> The implementation is enough to boot the Linux kernel configured with
> aspeed_defconfig.
>
> Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> ---
> hw/timer/Makefile.objs | 1 +
> hw/timer/aspeed_timer.c | 301 ++++++++++++++++++++++++++++++++++++++++
> include/hw/timer/aspeed_timer.h | 52 +++++++
> 3 files changed, 354 insertions(+)
> create mode 100644 hw/timer/aspeed_timer.c
> create mode 100644 include/hw/timer/aspeed_timer.h
>
> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index 133bd0d..c22ccf3 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -17,6 +17,7 @@ common-obj-$(CONFIG_IMX) += imx_epit.o
> common-obj-$(CONFIG_IMX) += imx_gpt.o
> common-obj-$(CONFIG_LM32) += lm32_timer.o
> common-obj-$(CONFIG_MILKYMIST) += milkymist-sysctl.o
> +common-obj-y += aspeed_timer.o
Don't we have a config option for ASPEED ? It would look better.
> obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
> obj-$(CONFIG_EXYNOS4) += exynos4210_pwm.o
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> new file mode 100644
> index 0000000..88b5cef
> --- /dev/null
> +++ b/hw/timer/aspeed_timer.c
> @@ -0,0 +1,301 @@
> +/*
> + * ASPEED AST2400 Timer
> + *
> + * Andrew Jeffery <andrew at aj.id.au>
> + *
> + * Copyright (C) 2015 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.
> + */
> +
> +#include <assert.h>
> +#include <stdio.h>
> +#include "hw/sysbus.h"
> +#include "qemu/timer.h"
> +#include "qemu-common.h"
> +#include "hw/ptimer.h"
> +#include "qemu/main-loop.h"
> +#include "hw/timer/aspeed_timer.h"
> +
> +#define TIMER_NUM_TIMERS 8
> +#define TIMER_NUM_REGS 4
> +
> +#define TIMER_CTRL 0
> +#define TIMER_CTRL2 1
> +#define TIMER_CTRL_BITS 4
> +
> +#ifdef DEBUG
> +# define DPRINTF(format, ...) fprintf(stderr, format , ## __VA_ARGS__)
> +#else
> +# define DPRINTF(format, ...) do { } while (0)
> +#endif
We should use trace events instead of printfs.
> +static void aspeed_timer_update(AspeedTimer *t)
> +{
> + if (t->enabled) {
> + qemu_irq_raise(t->irq);
> + } else {
> + qemu_irq_lower(t->irq);
> + }
> +}
> +
> +static void aspeed_timer_tick(void *opaque)
> +{
> + AspeedTimer *t = opaque;
> +
> + aspeed_timer_update(t);
> +}
> +
> +#define L_OFFSET_TO_TIMER(o) (o / (TIMER_NUM_REGS * sizeof(uint32_t)))
> +#define H_OFFSET_TO_TIMER(o) (L_OFFSET_TO_TIMER((o) - 0x40) + 4)
> +#define OFFSET_TO_REG(o) ((o / sizeof(uint32_t)) % TIMER_NUM_REGS)
These defines looks complex to me. See a proposal below.
> +static uint64_t aspeed_timer_get_value(AspeedTimerState *s, int timer, int reg)
> +{
> + if (timer > TIMER_NUM_TIMERS) {
> + qemu_log_mask(LOG_UNIMP, "Programming error, unknown timer: %d\n",
> + timer);
> + return 0;
> + }
I am not sure we need this test. It would mean we got something wrong
in aspeed_timer_read() which can not happen with the arithmetic we are
doing.
> + switch (reg) {
> + case 0: /* Status */
> + return ptimer_get_count(s->timers[timer].timer);
> + case 1: /* Reload */
> + return s->timers[timer].reload;
> + case 2: /* First match */
> + case 3: /* Second match */
> + return s->timers[timer].match[reg - 2];
> + default:
> + qemu_log_mask(LOG_UNIMP, "Programming error: unexpected reg: %d\n",
> + reg);
> + break;
> + }
> + return 0;
> +}
> +
> +static uint64_t aspeed_timer_read(void *opaque, hwaddr offset, unsigned size)
> +{
If we define :
int reg = (offset & 0xf) / 4;
then we can use it below :
> + AspeedTimerState *s = opaque;
> + switch (offset) {
> + case 0x30: /* Control Register */
> + return s->control[TIMER_CTRL];
> + case 0x34: /* Control Register 2 */
> + return s->control[TIMER_CTRL2];
> + case 0x00 ... 0x2c: /* Timers 1 - 4 */
> + return aspeed_timer_get_value(s, L_OFFSET_TO_TIMER(offset),
> + OFFSET_TO_REG(offset));
return aspeed_timer_get_value(s, (offset >> 4) + 1, reg);
> + case 0x40 ... 0x8c: /* Timers 5 - 8 */
> + return aspeed_timer_get_value(s, H_OFFSET_TO_TIMER(offset),
> + OFFSET_TO_REG(offset));
return aspeed_timer_get_value(s, (offset >> 4), reg);
I believe this is correct and does the same ?
> + /* Illegal */
> + case 0x38:
> + case 0x3C:
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset %x\n", __func__,
> + (int)offset);
> + break;
> + }
> + return 0;
> +}
> +
> +static void aspeed_timer_set_value(AspeedTimerState *s, int timer, int reg,
> + uint32_t value)
> +{
> + AspeedTimer *t;
> +
> + if (timer > TIMER_NUM_TIMERS) {
> + qemu_log_mask(LOG_UNIMP, "Programming error, unknown timer: %d\n",
> + timer);
> + return;
> + }
> + t = &s->timers[timer];
> + switch (reg) {
> + case 0: /* Status */
> + ptimer_get_count(t->timer);
> + break;
> + case 1: /* Reload */
> + t->reload = value;
> + ptimer_set_limit(t->timer, value, 1);
> + break;
> + case 2: /* First match */
> + case 3: /* Second match */
> + /* Nothing is done to make matching work, we just record the value */
> + t->match[reg - 2] = value;
> + break;
> + default:
> + qemu_log_mask(LOG_UNIMP, "Programming error: unexpected reg: %d\n",
> + reg);
> + break;
> + }
> +}
> +
> +static void aspeed_timer_set_ctrl(AspeedTimerState *s, uint32_t new)
> +{
> + int i;
> + uint32_t changed = s->control[TIMER_CTRL] ^ new;
> +
> + while (32 > (i = ctz32(changed))) {
> + int timer, op;
> + bool set;
> + AspeedTimer *t;
> +
> + timer = i / TIMER_CTRL_BITS;
> + assert(timer < TIMER_NUM_TIMERS);
> + t = &s->timers[timer];
> + op = i % TIMER_CTRL_BITS;
> + set = new & (1U << i);
> + switch (op) {
> + case 0: /* Timer Enable */
> + if (set) {
> + ptimer_run(t->timer, 0);
> + DPRINTF("Timer %d (%p) running\n", timer, t->timer);
> + } else {
> + DPRINTF("Stopping timer %d\n", timer);
> + ptimer_stop(t->timer);
> + ptimer_set_limit(t->timer, t->reload, 1);
> + }
> + t->enabled = set;
> + break;
> + case 1: /* Clock Selection */
> + if (set) {
> + DPRINTF("External clock\n");
> + } else {
> + DPRINTF("Internal clock\n");
> + }
if we are interested with these debug messages, may be we could use a
global trace event for the routine aspeed_timer_set_ctrl() and remove
all the DPRINTF()
> + break;
> + case 2: /* Overflow Interrupt */
> + if (set) {
> + DPRINTF("Overflow interrupt enabled\n");
> + } else {
> + DPRINTF("Overflow interrupt disabled\n");
> + }
> + break;
> + case 3: /* Pulse Generation and Output Enable */
> + if (timer < 4) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "Timer %d does not support pulse mode\n", timer);
> + } else if (set) {
> + DPRINTF("Pulse enabled\n");
> + } else {
> + DPRINTF("Pulse disabled\n");
> + }
> + break;
> + default:
> + qemu_log_mask(LOG_UNIMP, "Programming error, unexpected op: %d\n",
> + op);
> + break;
> + }
> + changed &= ~(1U << i);
> + }
> + s->control[TIMER_CTRL] = new;
> +}
> +
> +static void aspeed_timer_set_ctrl2(AspeedTimerState *s, uint32_t value)
> +{
> + DPRINTF("value=0x%" PRIx32 "\n", value);
> +}
> +
> +static void aspeed_timer_write(void *opaque, hwaddr offset, uint64_t value,
> + unsigned size)
> +{
> + AspeedTimerState *s = opaque;
> + uint32_t tv = (uint32_t)(value & 0xFFFFFFFF);
> +
> + if (size != sizeof(uint32_t)) {
> + qemu_log_mask(LOG_GUEST_ERROR, "Unexpected register write size: %d\n",
> + size);
> + return;
> + }
This should be caught by the MemoryRegionOps ops if you define :
.min_access_size = 4,
.max_access_size = 4,
> + 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, L_OFFSET_TO_TIMER(offset),
> + OFFSET_TO_REG(offset), tv);
same remark than for the read.
> + break;
> + case 0x40 ... 0x8c:
> + aspeed_timer_set_value(s, H_OFFSET_TO_TIMER(offset),
> + OFFSET_TO_REG(offset), tv);
> + break;
> + /* Illegal */
> + case 0x38:
> + case 0x3C:
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset %" HWADDR_PRIx "\n",
> + __func__, offset);
> + break;
> + }
> +}
> +
> +static const MemoryRegionOps aspeed_timer_ops = {
> + .read = aspeed_timer_read,
> + .write = aspeed_timer_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void aspeed_timer_init(AspeedTimer *t, uint32_t freq)
> +{
> + QEMUBH *bh;
> +
> + bh = qemu_bh_new(aspeed_timer_tick, t);
> + assert(bh);
> + t->timer = ptimer_init(bh);
> + assert(t->timer);
> + ptimer_set_freq(t->timer, freq);
> +}
> +
> +static void aspeed_timers_realize(DeviceState *dev, Error **errp)
> +{
> + int i;
> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> + AspeedTimerState *s = ASPEED_TIMER(dev);
> +
> + /* Hard-code the APB 24MHz clock rate for the moment */
> + for (i = 0; i < TIMER_NUM_TIMERS; i++) {
we could use ARRAY_SIZE() instead of TIMER_NUM_TIMERS
> + aspeed_timer_init(&s->timers[i], 24000000);
The freq can be changed with the control register 1. May be we could
have a freq attribute to struct AspeedTimer to begin with ?
> + 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 timer_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->realize = aspeed_timers_realize;
> + dc->desc = "ASPEED Timer";
> +}
> +
> +static const TypeInfo aspeed_timer_info = {
> + .name = TYPE_ASPEED_TIMER,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(AspeedTimerState),
> + .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..d6e0efc
> --- /dev/null
> +++ b/include/hw/timer/aspeed_timer.h
> @@ -0,0 +1,52 @@
> +/*
> + * ASPEED AST2400 Timer
> + *
> + * Andrew Jeffery <andrew at 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(AspeedTimerState, (obj), TYPE_ASPEED_TIMER);
> +#define TYPE_ASPEED_TIMER "aspeed.timer"
> +#define TIMER_BASE 0x1E782000
I think this address define should be in ast2400.c
> +#define TIMER_NR_TIMERS 8
May be use a ASPEED_ prefix to protect the namespace.
#define ASPEED_TIMER_NR_TIMERS 8
> +
> +typedef struct AspeedTimer {
> + ptimer_state *timer;
> + uint32_t reload;
> + uint32_t match[2];
> + qemu_irq irq;
> + bool enabled;
freq ? just to say we might need to take it into account ?
C.
> +} AspeedTimer;
> +
> +typedef struct AspeedTimerState {
> + /* < private > */
> + SysBusDevice parent;
> +
> + /* < public > */
> + MemoryRegion iomem;
> + AspeedTimer timers[TIMER_NR_TIMERS];
> + uint32_t control[2];
> + qemu_irq irq;
> +} AspeedTimerState;
> +
> +#endif /* ASPEED_TIMER_H */
>
More information about the openbmc
mailing list