* [PATCH 1/3] timers: Add ASPEED AST2400 timer device model
2016-02-03 5:46 [PATCH 0/3] QEMU: Add ASPEED AST2400 machine model Andrew Jeffery
@ 2016-02-03 5:46 ` Andrew Jeffery
2016-02-11 14:32 ` Cédric Le Goater
2016-02-03 5:46 ` [PATCH 2/3] intc: Add (new) ASPEED AST2400 AVIC " Andrew Jeffery
2016-02-03 5:46 ` [PATCH 3/3] ast2400: Add ASPEED AST2400 machine type Andrew Jeffery
2 siblings, 1 reply; 6+ messages in thread
From: Andrew Jeffery @ 2016-02-03 5:46 UTC (permalink / raw)
To: openbmc
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@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
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@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
+
+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)
+
+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;
+ }
+ 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)
+{
+ 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));
+ case 0x40 ... 0x8c: /* Timers 5 - 8 */
+ return aspeed_timer_get_value(s, H_OFFSET_TO_TIMER(offset),
+ OFFSET_TO_REG(offset));
+ /* 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");
+ }
+ 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;
+ }
+ 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);
+ 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++) {
+ aspeed_timer_init(&s->timers[i], 24000000);
+ 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@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
+#define TIMER_NR_TIMERS 8
+
+typedef struct AspeedTimer {
+ ptimer_state *timer;
+ uint32_t reload;
+ uint32_t match[2];
+ qemu_irq irq;
+ bool enabled;
+} 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 */
--
2.5.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] timers: Add ASPEED AST2400 timer device model
2016-02-03 5:46 ` [PATCH 1/3] timers: Add ASPEED AST2400 timer device model Andrew Jeffery
@ 2016-02-11 14:32 ` Cédric Le Goater
2016-02-16 10:50 ` Andrew Jeffery
0 siblings, 1 reply; 6+ messages in thread
From: Cédric Le Goater @ 2016-02-11 14:32 UTC (permalink / raw)
To: Andrew Jeffery, openbmc
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@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@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@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 */
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] timers: Add ASPEED AST2400 timer device model
2016-02-11 14:32 ` Cédric Le Goater
@ 2016-02-16 10:50 ` Andrew Jeffery
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Jeffery @ 2016-02-16 10:50 UTC (permalink / raw)
To: Cédric Le Goater, openbmc
[-- Attachment #1: Type: text/plain, Size: 17373 bytes --]
Hi Cédric,
Thanks for reviewing the changes!
On Thu, 2016-02-11 at 15:32 +0100, Cédric Le Goater wrote:
> 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@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.
I had fixed this up in the last patch, but I've now introduced the
config option in this first patch and used through the remaining two.
>
> > 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@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
> > +#include
> > +#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.
Done - hopefully tastefully!
>
> > +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.
Agree - done.
>
> > +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.
Yes, it was a bit defensive. Removed.
>
> > + 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);
I don't think the '+ 1' is right here - the value is used to index into
an array so needs to be from 0.
>
> > + 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 ?
Sort of, similar to above - the control registers are placed in the
middle of two sets of four timer register sets (of four registers).
There are only two control registers, followed by 2x32bits of undefined
space, followed by the remaining four timer register sets. As such I've
used '(offset >> 4) - 1' to account for the four non-timer registers.
>
> > + /* 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()
Done
>
> > + 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,
Oh, right. I based the code off of another timer implementation, so
didn't look in enough detail at MemoryRegionOps.
Done.
I also explicitly set '.unaligned = false' for clarity.
>
> > + 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
Turns out I had second define already in the header, and forgot about
it. I've switched to the original (in the header) since I don't want to
eliminate it. Maybe ARRAY_SIZE() is more correct though? The values
should never be different.
>
> > + 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 ?
So freq can either be 24MHz (APB) or 1MHz (external). Given it's not an
arbitrary choice I've now properly handled the clock source selection
in the control reg, and pushed the configuration into ptimer. That way
we don't have to maintain a (redundant) freq member. This lead to a bit
of refactoring, hopefully the outcome is acceptable!
>
> > + 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@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
Done
>
> > +#define TIMER_NR_TIMERS 8
>
> May be use a ASPEED_ prefix to protect the namespace.
>
> #define ASPEED_TIMER_NR_TIMERS 8
Done
> > +
> > +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 ?
Yep, see comment above.
Andrew
>
> 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 */
> >
>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/3] intc: Add (new) ASPEED AST2400 AVIC device model
2016-02-03 5:46 [PATCH 0/3] QEMU: Add ASPEED AST2400 machine model Andrew Jeffery
2016-02-03 5:46 ` [PATCH 1/3] timers: Add ASPEED AST2400 timer device model Andrew Jeffery
@ 2016-02-03 5:46 ` Andrew Jeffery
2016-02-03 5:46 ` [PATCH 3/3] ast2400: Add ASPEED AST2400 machine type Andrew Jeffery
2 siblings, 0 replies; 6+ messages in thread
From: Andrew Jeffery @ 2016-02-03 5:46 UTC (permalink / raw)
To: openbmc
Implement a minimal ASPEED AVIC device model, enough to boot a Linux
kernel configured with aspeed_defconfig. The VIC implements the 'new'
register set and expects this to be reflected in the device tree.
The implementation is a little awkward as the hardware uses 32bit
registers to manage 51 IRQs, and makes use of low and high registers for
each conceptual register. The model's implementation uses 64bit data
types to store the register values but must cope with access offset values
in multiples of 4 passed to the callbacks. As such the read() and
write() implementations process the provided offset to understand
whether the access is requesting the lower or upper 32bits of the 64bit
quantity.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
hw/intc/Makefile.objs | 1 +
hw/intc/aspeed_vic.c | 288 +++++++++++++++++++++++++++++++++++++++++++
include/hw/intc/aspeed_vic.h | 42 +++++++
3 files changed, 331 insertions(+)
create mode 100644 hw/intc/aspeed_vic.c
create mode 100644 include/hw/intc/aspeed_vic.h
diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index 004b0c2..9763e50 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -14,6 +14,7 @@ common-obj-$(CONFIG_ARM_GIC) += arm_gic.o
common-obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
common-obj-$(CONFIG_ARM_GIC) += arm_gicv3_common.o
common-obj-$(CONFIG_OPENPIC) += openpic.o
+common-obj-y += aspeed_vic.o
obj-$(CONFIG_APIC) += apic.o apic_common.o
obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
diff --git a/hw/intc/aspeed_vic.c b/hw/intc/aspeed_vic.c
new file mode 100644
index 0000000..0bcce76
--- /dev/null
+++ b/hw/intc/aspeed_vic.c
@@ -0,0 +1,288 @@
+/*
+ * ASPEED Interrupt Controller (New)
+ *
+ * Andrew Jeffery <andrew@aj.id.au>
+ *
+ * Copyright 2015 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ *
+ * Based off of the i.MX31 Vectored Interrupt Controller
+ *
+ * Note that this device model does not implement the legacy register space.
+ * The assumption is that the device base address is exposed such that all
+ * offsets are calculated relative to the address of the first "new" register.
+ */
+#include <inttypes.h>
+#include "hw/intc/aspeed_vic.h"
+
+#ifdef DEBUG
+# define DPRINTF(format, ...) fprintf(stderr, format , ## __VA_ARGS__)
+#else
+# define DPRINTF(format, ...) do { } while (0)
+#endif
+
+#define AVIC_EDGE_STATUS 0x60
+
+#define AVIC_L_MASK 0xFFFFFFFF
+#define AVIC_H_MASK 0x00007FFF
+#define AVIC_INT_EVENT_W_MASK (UINT64_C(0x78000) << 32)
+
+static void aspeed_vic_update(AspeedVICState *s)
+{
+ int i;
+ uint64_t new = (s->edge_status & s->int_enable);
+ uint64_t flags;
+
+ flags = new & s->int_select;
+ qemu_set_irq(s->fiq, !!flags);
+
+ flags = new & ~s->int_select;
+ if (!flags) {
+ qemu_set_irq(s->irq, !!flags);
+ return;
+ }
+
+ for (i = 0; i < ASPEED_VIC_NR_IRQS; i++) {
+ if (flags & (UINT64_C(1) << i)) {
+ qemu_set_irq(s->irq, 1);
+ return;
+ }
+ }
+
+ qemu_set_irq(s->irq, 0);
+}
+
+static void aspeed_vic_set_irq(void *opaque, int irq, int level)
+{
+ AspeedVICState *s = (AspeedVICState *)opaque;
+ if (irq > ASPEED_VIC_NR_IRQS) {
+ qemu_log_mask(LOG_GUEST_ERROR, "Out-of-range interrupt number: %d\n",
+ irq);
+ return;
+ }
+
+ if (level) {
+ DPRINTF("Raising IRQ %d\n", irq);
+ s->edge_status |= (UINT64_C(1) << irq);
+ } else {
+ DPRINTF("Clearing IRQ %d\n", irq);
+ s->edge_status &= ~(UINT64_C(1) << irq);
+ }
+
+ aspeed_vic_update(s);
+}
+
+static uint64_t aspeed_vic_read(void *opaque, hwaddr offset, unsigned size)
+{
+ uint64_t val;
+ const bool high = !!(offset & 0x4);
+ const hwaddr n_offset = (offset & ~0x4);
+ AspeedVICState *s = (AspeedVICState *)opaque;
+
+ if (offset > AVIC_EDGE_STATUS + 4) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "Out of bounds register access with offset %ld\n",
+ offset);
+ return 0;
+ }
+
+ if (size != sizeof(uint32_t)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "Unexpected read size %d with offset %ld\n", size,
+ offset);
+ return 0;
+ }
+
+ switch (n_offset) {
+ case 0x0: /* AVIC_IRQ_STATUS */
+ val = s->edge_status & ~s->int_select & s->int_enable;
+ break;
+ case 0x08: /* AVIC_FIQ_STATUS */
+ val = s->edge_status & s->int_select & s->int_enable;
+ break;
+ case 0x10: /* AVIC_RAW_STATUS */
+ val = s->edge_status;
+ break;
+ case 0x18: /* AVIC_INT_SELECT */
+ val = s->int_select;
+ break;
+ case 0x20: /* AVIC_INT_ENABLE */
+ val = s->int_enable;
+ break;
+ case 0x30: /* AVIC_INT_TRIGGER */
+ val = s->int_trigger;
+ break;
+ case 0x40: /* AVIC_INT_SENSE */
+ val = s->int_sense;
+ break;
+ case 0x48: /* AVIC_INT_DUAL_EDGE */
+ val = s->int_dual_edge;
+ break;
+ case 0x50: /* AVIC_INT_EVENT */
+ val = s->int_event;
+ break;
+ case 0x60: /* AVIC_EDGE_STATUS */
+ val = s->edge_status;
+ break;
+ /* Illegal */
+ case 0x28: /* AVIC_INT_ENABLE_CLR */
+ case 0x38: /* AVIC_INT_TRIGGER_CLR */
+ case 0x58: /* AVIC_EDGE_CLR */
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "Read of write-only register with offset 0x%" HWADDR_PRIx "\n",
+ offset);
+ val = 0;
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "[%s]%s: Bad register at offset 0x%" HWADDR_PRIx "\n",
+ TYPE_ASPEED_VIC, __func__, offset);
+ val = 0;
+ break;
+ }
+ if (high) {
+ val >>= 32;
+ val &= AVIC_H_MASK;
+ }
+ DPRINTF("offset=0x%" HWADDR_PRIx ", size=%u, val=0x%" PRIx64 "\n",
+ offset, size, val);
+ return val;
+}
+
+static void aspeed_vic_write(void *opaque, hwaddr offset, uint64_t data,
+ unsigned size)
+{
+ const bool high = !!(offset & 0x4);
+ const hwaddr n_offset = (offset & ~0x4);
+ AspeedVICState *s = (AspeedVICState *)opaque;
+
+ if (size != sizeof(uint32_t)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "Write of unexpected size %u with offset 0x%" HWADDR_PRIx "\n",
+ size, offset);
+ return;
+ }
+
+ if (offset > AVIC_EDGE_STATUS + 4) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "Out of bounds access with offset 0x%" HWADDR_PRIx "\n",
+ offset);
+ return;
+ }
+ DPRINTF("offset=0x%" HWADDR_PRIx ", size=%u, data=0x%" PRIx64 "\n",
+ offset, data, size);
+
+ if (high) {
+ data &= AVIC_H_MASK;
+ data <<= 32;
+ } else {
+ data &= AVIC_L_MASK;
+ }
+ DPRINTF("After %s masking, value is 0x%" PRIx64 "\n",
+ high ? "high" : "low", data);
+
+ switch (n_offset) {
+ case 0x18: /* AVIC_INT_SELECT */
+ if (high) {
+ s->int_select &= AVIC_L_MASK;
+ } else {
+ s->int_select &= ((UINT64_C(0) | AVIC_H_MASK) << 32);
+ }
+ s->int_select |= data;
+ break;
+ case 0x20: /* AVIC_INT_ENABLE */
+ s->int_enable |= data;
+ break;
+ case 0x28: /* AVIC_INT_ENABLE_CLR */
+ s->int_enable &= ~data;
+ break;
+ case 0x30: /* AVIC_INT_TRIGGER */
+ s->int_trigger |= data;
+ break;
+ case 0x38: /* AVIC_INT_TRIGGER_CLR */
+ s->int_trigger &= ~data;
+ break;
+ case 0x50: /* AVIC_INT_EVENT */
+ s->int_event &= ~AVIC_INT_EVENT_W_MASK;
+ s->int_event |= (data & AVIC_INT_EVENT_W_MASK);
+ break;
+ case 0x58: /* AVIC_EDGE_CLR */
+ s->edge_status &= ~data;
+ break;
+ case 0x00: /* AVIC_IRQ_STATUS */
+ case 0x08: /* AVIC_FIQ_STATUS */
+ case 0x10: /* AVIC_RAW_STATUS */
+ case 0x40: /* AVIC_INT_SENSE */
+ case 0x48: /* AVIC_INT_DUAL_EDGE */
+ case 0x60: /* AVIC_EDGE_STATUS */
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "Write of read-only register with offset %" HWADDR_PRIx "\n",
+ offset);
+ break;
+
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "[%s]%s: Bad register at offset 0x%" HWADDR_PRIx "\n",
+ TYPE_ASPEED_VIC, __func__, offset);
+ break;
+ }
+ aspeed_vic_update(s);
+}
+
+static const MemoryRegionOps aspeed_vic_ops = {
+ .read = aspeed_vic_read,
+ .write = aspeed_vic_write,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void aspeed_vic_reset(DeviceState *dev)
+{
+ AspeedVICState *s = ASPEED_VIC(dev);
+
+ s->int_select = 0;
+ s->int_enable = 0;
+ s->int_trigger = 0;
+ s->int_sense = (UINT64_C(0x1F07) << 32) | 0xFFF8FFFF;
+ s->int_dual_edge = (UINT64_C(0xF8) << 32) | 0x00070000;
+ s->int_event = (UINT64_C(0x5F07) << 32) | 0xFFF8FFFF;
+ s->edge_status = 0;
+}
+
+static void aspeed_vic_realize(DeviceState *dev, Error **errp)
+{
+ SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+ AspeedVICState *s = ASPEED_VIC(dev);
+
+ memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_vic_ops, s,
+ TYPE_ASPEED_VIC, 0x20000);
+
+ sysbus_init_mmio(sbd, &s->iomem);
+
+ qdev_init_gpio_in(dev, aspeed_vic_set_irq, ASPEED_VIC_NR_IRQS);
+ sysbus_init_irq(sbd, &s->irq);
+ sysbus_init_irq(sbd, &s->fiq);
+}
+
+static void aspeed_vic_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ dc->realize = aspeed_vic_realize;
+ dc->reset = aspeed_vic_reset;
+ dc->desc = "ASPEED Interrupt Controller (New)";
+}
+
+static const TypeInfo aspeed_vic_info = {
+ .name = TYPE_ASPEED_VIC,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(AspeedVICState),
+ .class_init = aspeed_vic_class_init,
+};
+
+static void aspeed_vic_register_types(void)
+{
+ type_register_static(&aspeed_vic_info);
+}
+
+type_init(aspeed_vic_register_types);
diff --git a/include/hw/intc/aspeed_vic.h b/include/hw/intc/aspeed_vic.h
new file mode 100644
index 0000000..1eed03d
--- /dev/null
+++ b/include/hw/intc/aspeed_vic.h
@@ -0,0 +1,42 @@
+/*
+ * ASPEED Interrupt Controller (New)
+ *
+ * Andrew Jeffery <andrew@aj.id.au>
+ *
+ * Copyright 2015 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ *
+ * Need to add SVIC and CVIC support
+ */
+#ifndef ASPEED_VIC_H
+#define ASPEED_VIC_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_ASPEED_VIC "aspeed.vic"
+#define ASPEED_VIC(obj) OBJECT_CHECK(AspeedVICState, (obj), TYPE_ASPEED_VIC)
+
+#define ASPEED_VIC_NR_IRQS 51
+
+#define ASPEED_VIC_BASE 0x1e6c0080
+
+typedef struct AspeedVICState {
+ /*< private >*/
+ SysBusDevice parent_obj;
+
+ /*< public >*/
+ MemoryRegion iomem;
+ uint64_t int_select;
+ uint64_t int_enable;
+ uint64_t int_trigger;
+ uint64_t int_sense;
+ uint64_t int_dual_edge;
+ uint64_t int_event;
+ uint64_t edge_status;
+ qemu_irq irq;
+ qemu_irq fiq;
+} AspeedVICState;
+
+#endif /* ASPEED_VIC_H */
--
2.5.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] ast2400: Add ASPEED AST2400 machine type
2016-02-03 5:46 [PATCH 0/3] QEMU: Add ASPEED AST2400 machine model Andrew Jeffery
2016-02-03 5:46 ` [PATCH 1/3] timers: Add ASPEED AST2400 timer device model Andrew Jeffery
2016-02-03 5:46 ` [PATCH 2/3] intc: Add (new) ASPEED AST2400 AVIC " Andrew Jeffery
@ 2016-02-03 5:46 ` Andrew Jeffery
2 siblings, 0 replies; 6+ messages in thread
From: Andrew Jeffery @ 2016-02-03 5:46 UTC (permalink / raw)
To: openbmc
Adds the AST2400 machine type with ASPEED AVIC and timer models. The
new machine type is functional enough to boot Linux to userspace.
Also, update the build infrastructure to only build the AST2400 code if
arm-softmmu is in target-list.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
default-configs/arm-softmmu.mak | 2 +
hw/arm/Makefile.objs | 1 +
hw/arm/ast2400.c | 126 ++++++++++++++++++++++++++++++++++++++++
hw/intc/Makefile.objs | 2 +-
hw/timer/Makefile.objs | 3 +-
5 files changed, 132 insertions(+), 2 deletions(-)
create mode 100644 hw/arm/ast2400.c
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index d9b90a5..ef79e69 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -109,3 +109,5 @@ CONFIG_IOH3420=y
CONFIG_I82801B11=y
CONFIG_ACPI=y
CONFIG_SMBIOS=y
+
+CONFIG_ASPEED_SOC=y
diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 2195b60..2045110 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -15,3 +15,4 @@ obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
obj-$(CONFIG_XLNX_ZYNQMP) += xlnx-zynqmp.o xlnx-ep108.o
obj-$(CONFIG_FSL_IMX25) += fsl-imx25.o imx25_pdk.o
obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
+obj-$(CONFIG_ASPEED_SOC) += ast2400.o
diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
new file mode 100644
index 0000000..b85f5b6
--- /dev/null
+++ b/hw/arm/ast2400.c
@@ -0,0 +1,126 @@
+/*
+ * Aspeed 2400
+ *
+ * Copyright (c) 2015 IBM
+ *
+ * Authors: Jeremy Kerr, Andrew Jeffery
+ */
+
+#include "exec/address-spaces.h"
+#include "hw/arm/arm.h"
+#include "hw/boards.h"
+#include "hw/char/serial.h"
+#include "hw/sysbus.h"
+#include "hw/intc/aspeed_vic.h"
+#include "hw/timer/aspeed_timer.h"
+
+#define ASPEED_SDRAM_ADDR 0x40000000
+#define ASPEED_IOMEM_BASE 0x1e600000
+#define ASPEED_IOMEM_SIZE 0x00200000
+
+static struct arm_boot_info ast2400_binfo = {
+ .loader_start = ASPEED_SDRAM_ADDR,
+ .board_id = 0,
+};
+
+/*
+ * IO handler: simply catch any reads/writes to IO addresses that aren't
+ * handled by a device mapping.
+ */
+static uint64_t ast2400_io_read(void *p, hwaddr offset, unsigned size)
+{
+ printf("%s: 0x%" HWADDR_PRIx "[%d]\n", __func__, offset, size);
+ return 0;
+}
+
+static void ast2400_io_write(void *opaque, hwaddr offset, uint64_t value,
+ unsigned size)
+{
+ printf("%s: 0x%" HWADDR_PRIx "[%d] <- 0x%" PRIx64 "\n",
+ __func__, offset, size, value);
+}
+
+static const MemoryRegionOps ast2400_io_ops = {
+ .read = ast2400_io_read,
+ .write = ast2400_io_write,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+typedef struct AspeedState {
+ ARMCPU *cpu;
+ MemoryRegion *address_space;
+ MemoryRegion *ram;
+ ram_addr_t ram_size;
+ MemoryRegion *iomem;
+ AspeedVICState avic;
+ AspeedTimerState timer;
+} AspeedState;
+
+static void ast2400_init(MachineState *machine)
+{
+ AspeedState ast2400;
+ DeviceState *dev;
+ qemu_irq pic[ASPEED_VIC_NR_IRQS] = {0};
+ int i;
+
+ if (!machine->cpu_model) {
+ machine->cpu_model = "arm926";
+ }
+
+ ast2400.cpu = cpu_arm_init(machine->cpu_model);
+ if (!ast2400.cpu) {
+ fprintf(stderr, "Unable to find CPU definition\n");
+ exit(1);
+ }
+
+ ast2400.address_space = get_system_memory();
+ ram_size = machine->ram_size;
+
+ /* System RAM */
+ ast2400.ram = g_new(MemoryRegion, 1);
+ memory_region_allocate_system_memory(ast2400.ram, NULL, "ast2400.ram",
+ ram_size);
+ memory_region_add_subregion(ast2400.address_space, ASPEED_SDRAM_ADDR,
+ ast2400.ram);
+
+ /* IO space */
+ ast2400.iomem = g_new(MemoryRegion, 1);
+ memory_region_init_io(ast2400.iomem, NULL, &ast2400_io_ops, NULL,
+ "ast2400.io", ASPEED_IOMEM_SIZE);
+ memory_region_add_subregion(ast2400.address_space, ASPEED_IOMEM_BASE,
+ ast2400.iomem);
+
+ /* AVIC */
+ dev = sysbus_create_varargs(TYPE_ASPEED_VIC, ASPEED_VIC_BASE,
+ qdev_get_gpio_in(DEVICE(ast2400.cpu), ARM_CPU_IRQ),
+ qdev_get_gpio_in(DEVICE(ast2400.cpu), ARM_CPU_FIQ),
+ NULL);
+
+ for (i = 0; i < ASPEED_VIC_NR_IRQS; i++) {
+ pic[i] = qdev_get_gpio_in(dev, i);
+ }
+
+ /* Timer */
+ sysbus_create_varargs(TYPE_ASPEED_TIMER, TIMER_BASE, pic[16], pic[17],
+ pic[18], pic[35], pic[36], pic[37], pic[38], pic[39], NULL);
+
+ /* UART - attach an 8250 to the IO space as our UART5 */
+ if (serial_hds[0]) {
+ serial_mm_init(ast2400.iomem, 0x184000, 2, pic[10], 38400,
+ serial_hds[0], DEVICE_NATIVE_ENDIAN);
+ }
+
+ ast2400_binfo.kernel_filename = machine->kernel_filename;
+ ast2400_binfo.initrd_filename = machine->initrd_filename;
+ ast2400_binfo.kernel_cmdline = machine->kernel_cmdline;
+ ast2400_binfo.ram_size = ram_size;
+ arm_load_kernel(ast2400.cpu, &ast2400_binfo);
+}
+
+static void ast2400_machine_init(MachineClass *mc)
+{
+ mc->desc = "ASpeed ast2400 BMC (ARM926EJ-S)";
+ mc->init = ast2400_init;
+}
+
+DEFINE_MACHINE("ast2400", ast2400_machine_init);
diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index 9763e50..6c52a8b 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -14,7 +14,6 @@ common-obj-$(CONFIG_ARM_GIC) += arm_gic.o
common-obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
common-obj-$(CONFIG_ARM_GIC) += arm_gicv3_common.o
common-obj-$(CONFIG_OPENPIC) += openpic.o
-common-obj-y += aspeed_vic.o
obj-$(CONFIG_APIC) += apic.o apic_common.o
obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
@@ -31,3 +30,4 @@ obj-$(CONFIG_XICS_KVM) += xics_kvm.o
obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
obj-$(CONFIG_S390_FLIC) += s390_flic.o
obj-$(CONFIG_S390_FLIC_KVM) += s390_flic_kvm.o
+obj-$(CONFIG_ASPEED_SOC) += aspeed_vic.o
diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index c22ccf3..f6f7351 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -17,7 +17,6 @@ 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
obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
obj-$(CONFIG_EXYNOS4) += exynos4210_pwm.o
@@ -34,3 +33,5 @@ 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
--
2.5.0
^ permalink raw reply related [flat|nested] 6+ messages in thread