* [Qemu-devel] [PATCH v2 1/3] hw/timer: Add ASPEED AST2400 timer device model
2016-02-16 11:34 [Qemu-devel] [PATCH v2 0/3] Add ASPEED AST2400 machine model Andrew Jeffery
@ 2016-02-16 11:34 ` Andrew Jeffery
2016-02-25 16:11 ` [Qemu-devel] [Qemu-arm] " Peter Maydell
2016-02-16 11:34 ` [Qemu-devel] [PATCH v2 2/3] hw/intc: Add (new) ASPEED AST2400 AVIC " Andrew Jeffery
2016-02-16 11:34 ` [Qemu-devel] [PATCH v2 3/3] hw/arm: Add ASPEED AST2400 machine type Andrew Jeffery
2 siblings, 1 reply; 15+ messages in thread
From: Andrew Jeffery @ 2016-02-16 11:34 UTC (permalink / raw)
To: qemu-devel; +Cc: openbmc, qemu-arm
Implement basic AST2400 timer functionality: Timers can be configured,
enabled, reset and disabled.
A number of hardware features are not implemented:
* 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>
---
default-configs/arm-softmmu.mak | 2 +
hw/timer/Makefile.objs | 2 +
hw/timer/aspeed_timer.c | 313 ++++++++++++++++++++++++++++++++++++++++
include/hw/timer/aspeed_timer.h | 55 +++++++
trace-events | 9 ++
5 files changed, 381 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..4072174 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -110,3 +110,5 @@ 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 133bd0d..f6f7351 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -33,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
diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
new file mode 100644
index 0000000..0359528
--- /dev/null
+++ b/hw/timer/aspeed_timer.c
@@ -0,0 +1,313 @@
+/*
+ * ASPEED AST2400 Timer
+ *
+ * Andrew Jeffery <andrew@aj.id.au>
+ *
+ * Copyright (C) 2015, 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.
+ */
+
+#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"
+#include "trace.h"
+
+#define TIMER_NR_REGS 4
+
+#define TIMER_CTRL_BITS 4
+
+#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_CTRL_OP_ENABLE 0
+#define TIMER_CTRL_OP_CLOCK_SELECT 1
+#define TIMER_CTRL_OP_OVERFLOW_INTERRUPT 2
+#define TIMER_CTRL_OP_PULSE_ENABLE 3
+
+#define TIMER_REG_STATUS 0
+#define TIMER_REG_RELOAD 1
+#define TIMER_REG_MATCH_FIRST 2
+#define TIMER_REG_MATCH_SECOND 3
+
+static inline bool timer_can_pulse(AspeedTimer *t)
+{
+ return t->id >= 4;
+}
+
+static void aspeed_timer_irq_update(AspeedTimer *t)
+{
+ qemu_set_irq(t->irq, t->enabled);
+}
+
+static void aspeed_timer_tick(void *opaque)
+{
+ AspeedTimer *t = opaque;
+
+ aspeed_timer_irq_update(t);
+}
+
+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, "Programming error: unexpected reg: %d\n",
+ reg);
+ value = 0;
+ break;
+ }
+ return value;
+}
+
+static uint64_t aspeed_timer_read(void *opaque, hwaddr offset, unsigned size)
+{
+ AspeedTimerState *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(AspeedTimerState *s, int timer, int reg,
+ uint32_t value)
+{
+ AspeedTimer *t;
+
+ assert(timer >= 0 && timer < ASPEED_TIMER_NR_TIMERS &&
+ "Programming error: Unexpected timer index");
+ trace_aspeed_timer_set_value(timer, reg, value);
+ t = &s->timers[timer];
+ switch (reg) {
+ case TIMER_REG_STATUS:
+ if (t->enabled) {
+ 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:
+ /* 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_ctrl_op(AspeedTimer *t, int op, bool set)
+{
+ switch (op) {
+ case TIMER_CTRL_OP_ENABLE:
+ trace_aspeed_timer_ctrl_op_timer_enable(t->id, set);
+ if (set) {
+ ptimer_run(t->timer, 0);
+ } else {
+ ptimer_stop(t->timer);
+ ptimer_set_limit(t->timer, t->reload, 1);
+ }
+ t->enabled = set;
+ break;
+ case TIMER_CTRL_OP_CLOCK_SELECT:
+ trace_aspeed_timer_ctrl_op_clock_select(t->id, set);
+ if (set) {
+ ptimer_set_freq(t->timer, TIMER_CLOCK_EXT_HZ);
+ } else {
+ ptimer_set_freq(t->timer, TIMER_CLOCK_APB_HZ);
+ }
+ break;
+ case TIMER_CTRL_OP_OVERFLOW_INTERRUPT:
+ trace_aspeed_timer_ctrl_op_overflow_interrupt(t->id, set);
+ break;
+ case TIMER_CTRL_OP_PULSE_ENABLE:
+ if (timer_can_pulse(t)) {
+ trace_aspeed_timer_ctrl_op_pulse_enable(t->id, set);
+ } else {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "Timer does not support pulse mode\n");
+ }
+ break;
+ default:
+ qemu_log_mask(LOG_UNIMP, "Programming error, unexpected op: %d\n",
+ op);
+ break;
+ }
+}
+
+static void aspeed_timer_set_ctrl(AspeedTimerState *s, uint32_t new)
+{
+ int i;
+ uint32_t changed = s->ctrl ^ new;
+
+ while (32 > (i = ctz32(changed))) {
+ int timer, op;
+ bool set;
+ AspeedTimer *t;
+
+ timer = i / TIMER_CTRL_BITS;
+ assert(timer < ASPEED_TIMER_NR_TIMERS);
+ t = &s->timers[timer];
+ op = i % TIMER_CTRL_BITS;
+ set = new & (1U << i);
+ aspeed_timer_ctrl_op(t, op, set);
+ changed &= ~(1U << i);
+ }
+ s->ctrl = new;
+}
+
+static void aspeed_timer_set_ctrl2(AspeedTimerState *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;
+ AspeedTimerState *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_NATIVE_ENDIAN,
+ .valid.min_access_size = 4,
+ .valid.max_access_size = 4,
+ .valid.unaligned = false,
+};
+
+static void aspeed_timer_init(AspeedTimer *t, uint8_t id, bool ext_clock)
+{
+ QEMUBH *bh;
+
+ t->id = id;
+ bh = qemu_bh_new(aspeed_timer_tick, t);
+ assert(bh);
+ t->timer = ptimer_init(bh);
+ assert(t->timer);
+ aspeed_timer_ctrl_op(t, TIMER_CTRL_OP_CLOCK_SELECT, ext_clock);
+}
+
+static void aspeed_timers_realize(DeviceState *dev, Error **errp)
+{
+ int i;
+ SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+ AspeedTimerState *s = ASPEED_TIMER(dev);
+ uint32_t clock_mask = 0;
+
+ for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
+ aspeed_timer_init(&s->timers[i], i, TIMER_CLOCK_USE_APB);
+ clock_mask |= (1 << (1 + i * TIMER_CTRL_BITS));
+ sysbus_init_irq(sbd, &s->timers[i].irq);
+ }
+ /* Ensure control reg has timers configured with APB clock selected */
+ s->ctrl &= ~clock_mask;
+ 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..a14db74
--- /dev/null
+++ b/include/hw/timer/aspeed_timer.h
@@ -0,0 +1,55 @@
+/*
+ * 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 ASPEED_TIMER_NR_TIMERS 8
+
+typedef struct AspeedTimer {
+ uint8_t id;
+ ptimer_state *timer;
+ uint32_t reload;
+ uint32_t match[2];
+ qemu_irq irq;
+ /* ptimer doesn't expose any method to check whether it's enabled, which we
+ * require for updating the timer IRQ state */
+ bool enabled;
+} AspeedTimer;
+
+typedef struct AspeedTimerState {
+ /* < private > */
+ SysBusDevice parent;
+
+ /* < public > */
+ MemoryRegion iomem;
+ AspeedTimer timers[ASPEED_TIMER_NR_TIMERS];
+ uint32_t ctrl;
+ uint32_t ctrl2;
+ qemu_irq irq;
+} AspeedTimerState;
+
+#endif /* ASPEED_TIMER_H */
diff --git a/trace-events b/trace-events
index f986c81..5325a23 100644
--- a/trace-events
+++ b/trace-events
@@ -1890,3 +1890,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_op_timer_enable(uint8_t i, bool enable) "Configure timer %" PRIu8 ": %d"
+aspeed_timer_ctrl_op_clock_select(uint8_t i, bool enable) "Use external clock on %" PRIu8 ": %d"
+aspeed_timer_ctrl_op_pulse_enable(uint8_t i, bool enable) "Configure pulse mode on %" PRIu8 ": %d"
+aspeed_timer_ctrl_op_overflow_interrupt(uint8_t i, bool enable) "Overflow interrupt on %" PRIu8 ": %d"
+aspeed_timer_set_ctrl2(uint32_t value) "CTRL2 set to 0x%" PRIx32
+aspeed_timer_set_value(int timer, int reg, uint32_t value) "Write to register %d of timer %d: 0x%" PRIx32
+aspeed_timer_read(uint64_t offset, unsigned size, uint64_t value) "Read at 0x%" PRIx64 ": of size %u 0x%" PRIx64
--
2.5.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH v2 1/3] hw/timer: Add ASPEED AST2400 timer device model
2016-02-16 11:34 ` [Qemu-devel] [PATCH v2 1/3] hw/timer: Add ASPEED AST2400 timer device model Andrew Jeffery
@ 2016-02-25 16:11 ` Peter Maydell
2016-02-26 3:14 ` Andrew Jeffery
0 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2016-02-25 16:11 UTC (permalink / raw)
To: Andrew Jeffery; +Cc: openbmc, qemu-arm, QEMU Developers
On 16 February 2016 at 11:34, Andrew Jeffery <andrew@aj.id.au> wrote:
> Implement basic AST2400 timer functionality: Timers can be configured,
> enabled, reset and disabled.
>
> A number of hardware features are not implemented:
>
> * Timer Overflow interrupts
> * Clock value matching
> * Pulse generation
>
> The implementation is enough to boot the Linux kernel configured with
> aspeed_defconfig.
Thanks; this mostly looks in reasonable shape; I have some comments below.
Do we have a datasheet for this chip ?
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> default-configs/arm-softmmu.mak | 2 +
> hw/timer/Makefile.objs | 2 +
> hw/timer/aspeed_timer.c | 313 ++++++++++++++++++++++++++++++++++++++++
> include/hw/timer/aspeed_timer.h | 55 +++++++
> trace-events | 9 ++
> 5 files changed, 381 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..4072174 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -110,3 +110,5 @@ 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 133bd0d..f6f7351 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -33,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
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> new file mode 100644
> index 0000000..0359528
> --- /dev/null
> +++ b/hw/timer/aspeed_timer.c
> @@ -0,0 +1,313 @@
> +/*
> + * ASPEED AST2400 Timer
> + *
> + * Andrew Jeffery <andrew@aj.id.au>
> + *
> + * Copyright (C) 2015, 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.
> + */
> +
> +#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"
> +#include "trace.h"
All source files need to #include "qemu/osdep.h" as their first
include. That then means you don't need to include assert.h or
stdio.h yourself.
What do we need from qemu/main-loop.h?
> +
> +#define TIMER_NR_REGS 4
> +
> +#define TIMER_CTRL_BITS 4
> +
> +#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_CTRL_OP_ENABLE 0
> +#define TIMER_CTRL_OP_CLOCK_SELECT 1
> +#define TIMER_CTRL_OP_OVERFLOW_INTERRUPT 2
> +#define TIMER_CTRL_OP_PULSE_ENABLE 3
> +
> +#define TIMER_REG_STATUS 0
> +#define TIMER_REG_RELOAD 1
> +#define TIMER_REG_MATCH_FIRST 2
> +#define TIMER_REG_MATCH_SECOND 3
> +
> +static inline bool timer_can_pulse(AspeedTimer *t)
> +{
> + return t->id >= 4;
> +}
> +
> +static void aspeed_timer_irq_update(AspeedTimer *t)
> +{
> + qemu_set_irq(t->irq, t->enabled);
Surely the timer doesn't assert its IRQ line all
the time it's enabled? This doesn't look like the right condition.
> +}
> +
> +static void aspeed_timer_tick(void *opaque)
> +{
> + AspeedTimer *t = opaque;
> +
> + aspeed_timer_irq_update(t);
> +}
> +
> +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, "Programming error: unexpected reg: %d\n",
> + reg);
> + value = 0;
> + break;
> + }
> + return value;
> +}
> +
> +static uint64_t aspeed_timer_read(void *opaque, hwaddr offset, unsigned size)
> +{
> + AspeedTimerState *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(AspeedTimerState *s, int timer, int reg,
> + uint32_t value)
> +{
> + AspeedTimer *t;
> +
> + assert(timer >= 0 && timer < ASPEED_TIMER_NR_TIMERS &&
> + "Programming error: Unexpected timer index");
> + trace_aspeed_timer_set_value(timer, reg, value);
> + t = &s->timers[timer];
> + switch (reg) {
> + case TIMER_REG_STATUS:
> + if (t->enabled) {
> + 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:
> + /* 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_ctrl_op(AspeedTimer *t, int op, bool set)
> +{
> + switch (op) {
> + case TIMER_CTRL_OP_ENABLE:
> + trace_aspeed_timer_ctrl_op_timer_enable(t->id, set);
> + if (set) {
> + ptimer_run(t->timer, 0);
> + } else {
> + ptimer_stop(t->timer);
> + ptimer_set_limit(t->timer, t->reload, 1);
> + }
> + t->enabled = set;
Is this enabled bit really separate state within the h/w from
the bit in the control register? Storing the same state twice
in different places in the qemu device state struct is usually
a bad idea if you can avoid it.
> + break;
> + case TIMER_CTRL_OP_CLOCK_SELECT:
> + trace_aspeed_timer_ctrl_op_clock_select(t->id, set);
> + if (set) {
> + ptimer_set_freq(t->timer, TIMER_CLOCK_EXT_HZ);
> + } else {
> + ptimer_set_freq(t->timer, TIMER_CLOCK_APB_HZ);
> + }
> + break;
> + case TIMER_CTRL_OP_OVERFLOW_INTERRUPT:
> + trace_aspeed_timer_ctrl_op_overflow_interrupt(t->id, set);
> + break;
> + case TIMER_CTRL_OP_PULSE_ENABLE:
> + if (timer_can_pulse(t)) {
> + trace_aspeed_timer_ctrl_op_pulse_enable(t->id, set);
> + } else {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "Timer does not support pulse mode\n");
> + }
> + break;
> + default:
> + qemu_log_mask(LOG_UNIMP, "Programming error, unexpected op: %d\n",
> + op);
> + break;
> + }
> +}
> +
> +static void aspeed_timer_set_ctrl(AspeedTimerState *s, uint32_t new)
> +{
> + int i;
> + uint32_t changed = s->ctrl ^ new;
> +
> + while (32 > (i = ctz32(changed))) {
> + int timer, op;
> + bool set;
> + AspeedTimer *t;
> +
> + timer = i / TIMER_CTRL_BITS;
> + assert(timer < ASPEED_TIMER_NR_TIMERS);
> + t = &s->timers[timer];
> + op = i % TIMER_CTRL_BITS;
> + set = new & (1U << i);
> + aspeed_timer_ctrl_op(t, op, set);
> + changed &= ~(1U << i);
> + }
This is effectively processing the bits in order from
low to high (and doing the loop-through-bits in a confusing
to read way as well). That doesn't seem right to me --
if the guest does one write to say "enable timer with this clock"
then you want to first pick the right clock and then enable
the timer, not enable the timer first and then change the
frequency. (Conversely if the guest writes to disable the timer
you want to disable it first and then reconfigure it.)
> + s->ctrl = new;
> +}
> +
> +static void aspeed_timer_set_ctrl2(AspeedTimerState *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;
> + AspeedTimerState *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_NATIVE_ENDIAN,
> + .valid.min_access_size = 4,
> + .valid.max_access_size = 4,
> + .valid.unaligned = false,
> +};
> +
> +static void aspeed_timer_init(AspeedTimer *t, uint8_t id, bool ext_clock)
You should call this aspeed_init_one_timer() or something,
because with this function name it looks like it ought to
be the instance_init function for the class.
> +{
> + QEMUBH *bh;
> +
> + t->id = id;
> + bh = qemu_bh_new(aspeed_timer_tick, t);
> + assert(bh);
> + t->timer = ptimer_init(bh);
> + assert(t->timer);
> + aspeed_timer_ctrl_op(t, TIMER_CTRL_OP_CLOCK_SELECT, ext_clock);
> +}
> +
> +static void aspeed_timers_realize(DeviceState *dev, Error **errp)
Stray 's' in function name.
> +{
> + int i;
> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> + AspeedTimerState *s = ASPEED_TIMER(dev);
> + uint32_t clock_mask = 0;
> +
> + for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
> + aspeed_timer_init(&s->timers[i], i, TIMER_CLOCK_USE_APB);
> + clock_mask |= (1 << (1 + i * TIMER_CTRL_BITS));
> + sysbus_init_irq(sbd, &s->timers[i].irq);
> + }
> + /* Ensure control reg has timers configured with APB clock selected */
> + s->ctrl &= ~clock_mask;
Reset of register values should go in your device's reset
function (which you need to implement and register as dc->reset).
> + 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";
You should implement a VMState struct for device migration,
and wire it up here via dc->vmsd.
> +}
> +
> +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);
Usual convention is to omit the ';' on type_init() etc, though as usual
there is some code in the tree that doesn't do so.
thanks
-- PMM
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH v2 1/3] hw/timer: Add ASPEED AST2400 timer device model
2016-02-25 16:11 ` [Qemu-devel] [Qemu-arm] " Peter Maydell
@ 2016-02-26 3:14 ` Andrew Jeffery
2016-02-26 10:20 ` Peter Maydell
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Jeffery @ 2016-02-26 3:14 UTC (permalink / raw)
To: Peter Maydell; +Cc: openbmc, qemu-arm, QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 16191 bytes --]
Hi Peter,
On Thu, 2016-02-25 at 16:11 +0000, Peter Maydell wrote:
> On 16 February 2016 at 11:34, Andrew Jeffery <andrew@aj.id.au> wrote:
> > Implement basic AST2400 timer functionality: Timers can be configured,
> > enabled, reset and disabled.
> >
> > A number of hardware features are not implemented:
> >
> > * Timer Overflow interrupts
> > * Clock value matching
> > * Pulse generation
> >
> > The implementation is enough to boot the Linux kernel configured with
> > aspeed_defconfig.
>
> Thanks; this mostly looks in reasonable shape; I have some comments below.
>
> Do we have a datasheet for this chip ?
Unfortunately I don't know of a publicly available datasheet. What's the best way to proceed in this case?
>
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> > default-configs/arm-softmmu.mak | 2 +
> > hw/timer/Makefile.objs | 2 +
> > hw/timer/aspeed_timer.c | 313 ++++++++++++++++++++++++++++++++++++++++
> > include/hw/timer/aspeed_timer.h | 55 +++++++
> > trace-events | 9 ++
> > 5 files changed, 381 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..4072174 100644
> > --- a/default-configs/arm-softmmu.mak
> > +++ b/default-configs/arm-softmmu.mak
> > @@ -110,3 +110,5 @@ 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 133bd0d..f6f7351 100644
> > --- a/hw/timer/Makefile.objs
> > +++ b/hw/timer/Makefile.objs
> > @@ -33,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
> > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> > new file mode 100644
> > index 0000000..0359528
> > --- /dev/null
> > +++ b/hw/timer/aspeed_timer.c
> > @@ -0,0 +1,313 @@
> > +/*
> > + * ASPEED AST2400 Timer
> > + *
> > + * Andrew Jeffery <andrew@aj.id.au>
> > + *
> > + * Copyright (C) 2015, 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.
> > + */
> > +
> > +#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"
> > +#include "trace.h"
>
> All source files need to #include "qemu/osdep.h" as their first
> include. That then means you don't need to include assert.h or
> stdio.h yourself.
>
> What do we need from qemu/main-loop.h?
I'm using it for qemu_bh_new() which is required by ptimer, who registers the aspeed_timer_tick() callback into the main loop timer handling. I think this callback has a poorly chosen name - it's probably better called aspeed_timer_expire(), which I'll fix. ptimer seemed like a logical choice for implementing the functionality to me, so main-loop.h is required?
> > +#define TIMER_NR_REGS 4
> > +
> > +#define TIMER_CTRL_BITS 4
> > +
> > +#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_CTRL_OP_ENABLE 0
> > +#define TIMER_CTRL_OP_CLOCK_SELECT 1
> > +#define TIMER_CTRL_OP_OVERFLOW_INTERRUPT 2
> > +#define TIMER_CTRL_OP_PULSE_ENABLE 3
> > +
> > +#define TIMER_REG_STATUS 0
> > +#define TIMER_REG_RELOAD 1
> > +#define TIMER_REG_MATCH_FIRST 2
> > +#define TIMER_REG_MATCH_SECOND 3
> > +
> > +static inline bool timer_can_pulse(AspeedTimer *t)
> > +{
> > + return t->id >= 4;
> > +}
> > +
> > +static void aspeed_timer_irq_update(AspeedTimer *t)
> > +{
> > + qemu_set_irq(t->irq, t->enabled);
>
> Surely the timer doesn't assert its IRQ line all
> the time it's enabled? This doesn't look like the right condition.
So I think this is correct despite how it looks. There's some cruft from modelling the implementation off of another timer that's probably obscuring things, which should be fixed. aspeed_timer_irq_update() is only called from aspeed_timer_tick(), so I'll just merge the two. Then by renaming aspeed_timer_tick() to aspeed_timer_expire() as mentioned above, this won't look so strange? I've read through the timer handling code (the processing loop in timerlist_run_timers()) and my understanding is it has the behaviour we want - callback on expiry, not on ticks - which is not how it reads as above.
>
> > +}
> > +
> > +static void aspeed_timer_tick(void *opaque)
> > +{
> > + AspeedTimer *t = opaque;
> > +
> > + aspeed_timer_irq_update(t);
> > +}
> > +
> > +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, "Programming error: unexpected reg: %d\n",
> > + reg);
> > + value = 0;
> > + break;
> > + }
> > + return value;
> > +}
> > +
> > +static uint64_t aspeed_timer_read(void *opaque, hwaddr offset, unsigned size)
> > +{
> > + AspeedTimerState *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(AspeedTimerState *s, int timer, int reg,
> > + uint32_t value)
> > +{
> > + AspeedTimer *t;
> > +
> > + assert(timer >= 0 && timer < ASPEED_TIMER_NR_TIMERS &&
> > + "Programming error: Unexpected timer index");
> > + trace_aspeed_timer_set_value(timer, reg, value);
> > + t = &s->timers[timer];
> > + switch (reg) {
> > + case TIMER_REG_STATUS:
> > + if (t->enabled) {
> > + 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:
> > + /* 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_ctrl_op(AspeedTimer *t, int op, bool set)
> > +{
> > + switch (op) {
> > + case TIMER_CTRL_OP_ENABLE:
> > + trace_aspeed_timer_ctrl_op_timer_enable(t->id, set);
> > + if (set) {
> > + ptimer_run(t->timer, 0);
> > + } else {
> > + ptimer_stop(t->timer);
> > + ptimer_set_limit(t->timer, t->reload, 1);
> > + }
> > + t->enabled = set;
>
> Is this enabled bit really separate state within the h/w from
> the bit in the control register? Storing the same state twice
> in different places in the qemu device state struct is usually
> a bad idea if you can avoid it.
I didn't like it either, and it likely is redundant as you point out. I was more concentrating on why ptimer didn't seem to expose this state and probably overlooked what I already had, somehow.
>
> > + break;
> > + case TIMER_CTRL_OP_CLOCK_SELECT:
> > + trace_aspeed_timer_ctrl_op_clock_select(t->id, set);
> > + if (set) {
> > + ptimer_set_freq(t->timer, TIMER_CLOCK_EXT_HZ);
> > + } else {
> > + ptimer_set_freq(t->timer, TIMER_CLOCK_APB_HZ);
> > + }
> > + break;
> > + case TIMER_CTRL_OP_OVERFLOW_INTERRUPT:
> > + trace_aspeed_timer_ctrl_op_overflow_interrupt(t->id, set);
> > + break;
> > + case TIMER_CTRL_OP_PULSE_ENABLE:
> > + if (timer_can_pulse(t)) {
> > + trace_aspeed_timer_ctrl_op_pulse_enable(t->id, set);
> > + } else {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "Timer does not support pulse mode\n");
> > + }
> > + break;
> > + default:
> > + qemu_log_mask(LOG_UNIMP, "Programming error, unexpected op: %d\n",
> > + op);
> > + break;
> > + }
> > +}
> > +
> > +static void aspeed_timer_set_ctrl(AspeedTimerState *s, uint32_t new)
> > +{
> > + int i;
> > + uint32_t changed = s->ctrl ^ new;
> > +
> > + while (32 > (i = ctz32(changed))) {
> > + int timer, op;
> > + bool set;
> > + AspeedTimer *t;
> > +
> > + timer = i / TIMER_CTRL_BITS;
> > + assert(timer < ASPEED_TIMER_NR_TIMERS);
> > + t = &s->timers[timer];
> > + op = i % TIMER_CTRL_BITS;
> > + set = new & (1U << i);
> > + aspeed_timer_ctrl_op(t, op, set);
> > + changed &= ~(1U << i);
> > + }
>
> This is effectively processing the bits in order from
> low to high (and doing the loop-through-bits in a confusing
> to read way as well).
Just how the condition is expressed, or more? The condition can be simplified to while(changed), which I've done locally.
That doesn't seem right to me --
> if the guest does one write to say "enable timer with this clock"
> then you want to first pick the right clock and then enable
> the timer, not enable the timer first and then change the
> frequency. (Conversely if the guest writes to disable the timer
> you want to disable it first and then reconfigure it.)
Fair point! I'll have a play with reworking it.
>
> > + s->ctrl = new;
> > +}
> > +
> > +static void aspeed_timer_set_ctrl2(AspeedTimerState *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;
> > + AspeedTimerState *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_NATIVE_ENDIAN,
> > + .valid.min_access_size = 4,
> > + .valid.max_access_size = 4,
> > + .valid.unaligned = false,
> > +};
> > +
> > +static void aspeed_timer_init(AspeedTimer *t, uint8_t id, bool ext_clock)
>
> You should call this aspeed_init_one_timer() or something,
> because with this function name it looks like it ought to
> be the instance_init function for the class.
I wasn't satisfied with the naming here either, so thanks for the suggestion.
>
> > +{
> > + QEMUBH *bh;
> > +
> > + t->id = id;
> > + bh = qemu_bh_new(aspeed_timer_tick, t);
> > + assert(bh);
> > + t->timer = ptimer_init(bh);
> > + assert(t->timer);
> > + aspeed_timer_ctrl_op(t, TIMER_CTRL_OP_CLOCK_SELECT, ext_clock);
> > +}
> > +
> > +static void aspeed_timers_realize(DeviceState *dev, Error **errp)
>
> Stray 's' in function name.
Yep, will fix.
>
> > +{
> > + int i;
> > + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > + AspeedTimerState *s = ASPEED_TIMER(dev);
> > + uint32_t clock_mask = 0;
> > +
> > + for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
> > + aspeed_timer_init(&s->timers[i], i, TIMER_CLOCK_USE_APB);
> > + clock_mask |= (1 << (1 + i * TIMER_CTRL_BITS));
> > + sysbus_init_irq(sbd, &s->timers[i].irq);
> > + }
> > + /* Ensure control reg has timers configured with APB clock selected */
> > + s->ctrl &= ~clock_mask;
>
> Reset of register values should go in your device's reset
> function (which you need to implement and register as dc->reset).
Good point, I'll move it.
>
> > + 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";
>
> You should implement a VMState struct for device migration,
> and wire it up here via dc->vmsd.
I'll look into it. I started experimenting with a VMState struct early on in the implementation but threw it away as it wasn't my primary focus at the time.
>
> > +}
> > +
> > +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);
>
> Usual convention is to omit the ';' on type_init() etc, though as usual
> there is some code in the tree that doesn't do so.
Okay, I wasn't aware of that. I'll remove it.
Thanks,
Andrew
>
> thanks
> -- PMM
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH v2 1/3] hw/timer: Add ASPEED AST2400 timer device model
2016-02-26 3:14 ` Andrew Jeffery
@ 2016-02-26 10:20 ` Peter Maydell
2016-02-29 2:10 ` Andrew Jeffery
0 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2016-02-26 10:20 UTC (permalink / raw)
To: Andrew Jeffery; +Cc: openbmc, qemu-arm, QEMU Developers
On 26 February 2016 at 03:14, Andrew Jeffery <andrew@aj.id.au> wrote:
> Hi Peter,
>
> On Thu, 2016-02-25 at 16:11 +0000, Peter Maydell wrote:
>> On 16 February 2016 at 11:34, Andrew Jeffery <andrew@aj.id.au> wrote:
>> > Implement basic AST2400 timer functionality: Timers can be configured,
>> > enabled, reset and disabled.
>> >
>> > A number of hardware features are not implemented:
>> >
>> > * Timer Overflow interrupts
>> > * Clock value matching
>> > * Pulse generation
>> >
>> > The implementation is enough to boot the Linux kernel configured with
>> > aspeed_defconfig.
>>
>> Thanks; this mostly looks in reasonable shape; I have some comments below.
>>
>> Do we have a datasheet for this chip ?
>
> Unfortunately I don't know of a publicly available datasheet. What's
> the best way to proceed in this case?
We have devices in the tree that are either based on non-public datasheets
or occasionally reverse engineered from Linux kernel drivers. That's OK,
but it's nice to be clear in a comment at the top what the source is,
so people maintaining it later know how much to trust the current code
and (if possible) where to look for clarification.
>> All source files need to #include "qemu/osdep.h" as their first
>> include. That then means you don't need to include assert.h or
>> stdio.h yourself.
>>
>> What do we need from qemu/main-loop.h?
>
> I'm using it for qemu_bh_new() which is required by ptimer, who registers
> the aspeed_timer_tick() callback into the main loop timer handling.
OK, no problem.
>> > +static void aspeed_timer_irq_update(AspeedTimer *t)
>> > +{
>> > + qemu_set_irq(t->irq, t->enabled);
>>
>> Surely the timer doesn't assert its IRQ line all
>> the time it's enabled? This doesn't look like the right condition.
>
> So I think this is correct despite how it looks. There's some cruft
> from modelling the implementation off of another timer that's probably
> obscuring things, which should be fixed. aspeed_timer_irq_update()
> is only called from aspeed_timer_tick(), so I'll just merge the two.
> Then by renaming aspeed_timer_tick() to aspeed_timer_expire() as
> mentioned above, this won't look so strange? I've read through the
> timer handling code (the processing loop in timerlist_run_timers())
> and my understanding is it has the behaviour we want - callback on
> expiry, not on ticks - which is not how it reads as above.
Usually functions in QEMU called thingy_irq_update() are the ones
that do "look at current state of device and assert IRQ as
necessary"; often this is "mask irq raw state against some
irq-masking bit". Merging this into the timer expire function will
probably help. (Is there no register bit that the guest can query
that indicates "timer expired" or "raw interrupt state" ?)
>> You should implement a VMState struct for device migration,
>> and wire it up here via dc->vmsd.
>
> I'll look into it. I started experimenting with a VMState struct
> early on in the implementation but threw it away as it wasn't my
> primary focus at the time.
We insist on vmstate structs for all new devices, because
they're fairly easy to implement, and if the original
submitter doesn't implement one then the device becomes
a landmine for any user trying migration or vmstate snapshots,
because it will silently misbehave.
thanks
-- PMM
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH v2 1/3] hw/timer: Add ASPEED AST2400 timer device model
2016-02-26 10:20 ` Peter Maydell
@ 2016-02-29 2:10 ` Andrew Jeffery
0 siblings, 0 replies; 15+ messages in thread
From: Andrew Jeffery @ 2016-02-29 2:10 UTC (permalink / raw)
To: Peter Maydell; +Cc: openbmc, qemu-arm, QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 4553 bytes --]
On Fri, 2016-02-26 at 10:20 +0000, Peter Maydell wrote:
> On 26 February 2016 at 03:14, Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > Hi Peter,
> >
> > On Thu, 2016-02-25 at 16:11 +0000, Peter Maydell wrote:
> > >
> > > On 16 February 2016 at 11:34, Andrew Jeffery <andrew@aj.id.au> wrote:
> > > >
> > > > Implement basic AST2400 timer functionality: Timers can be configured,
> > > > enabled, reset and disabled.
> > > >
> > > > A number of hardware features are not implemented:
> > > >
> > > > * Timer Overflow interrupts
> > > > * Clock value matching
> > > > * Pulse generation
> > > >
> > > > The implementation is enough to boot the Linux kernel configured with
> > > > aspeed_defconfig.
> > > Thanks; this mostly looks in reasonable shape; I have some comments below.
> > >
> > > Do we have a datasheet for this chip ?
> > Unfortunately I don't know of a publicly available datasheet. What's
> > the best way to proceed in this case?
> We have devices in the tree that are either based on non-public datasheets
> or occasionally reverse engineered from Linux kernel drivers. That's OK,
> but it's nice to be clear in a comment at the top what the source is,
> so people maintaining it later know how much to trust the current code
> and (if possible) where to look for clarification.
No worries, I'll add notes in the header comments for each of the new
files.
>
> >
> > >
> > > All source files need to #include "qemu/osdep.h" as their first
> > > include. That then means you don't need to include assert.h or
> > > stdio.h yourself.
> > >
> > > What do we need from qemu/main-loop.h?
> > I'm using it for qemu_bh_new() which is required by ptimer, who registers
> > the aspeed_timer_tick() callback into the main loop timer handling.
> OK, no problem.
>
> >
> > >
> > > >
> > > > +static void aspeed_timer_irq_update(AspeedTimer *t)
> > > > +{
> > > > + qemu_set_irq(t->irq, t->enabled);
> > > Surely the timer doesn't assert its IRQ line all
> > > the time it's enabled? This doesn't look like the right condition.
> > So I think this is correct despite how it looks. There's some cruft
> > from modelling the implementation off of another timer that's probably
> > obscuring things, which should be fixed. aspeed_timer_irq_update()
> > is only called from aspeed_timer_tick(), so I'll just merge the two.
> > Then by renaming aspeed_timer_tick() to aspeed_timer_expire() as
> > mentioned above, this won't look so strange? I've read through the
> > timer handling code (the processing loop in timerlist_run_timers())
> > and my understanding is it has the behaviour we want - callback on
> > expiry, not on ticks - which is not how it reads as above.
> Usually functions in QEMU called thingy_irq_update() are the ones
> that do "look at current state of device and assert IRQ as
> necessary"; often this is "mask irq raw state against some
> irq-masking bit". Merging this into the timer expire function will
> probably help. (Is there no register bit that the guest can query
> that indicates "timer expired" or "raw interrupt state" ?)
It doesn't appear so - overflow interrupts can be enabled or disabled,
but it doesn't appear that there's a way to poll whether the timer has
expired. It doesn't look like it can be inferred either as the counter
status register doesn't stick at zero, rather resets back to the reload
register value (and my interpretation is it continues to count down if
the enabled bit remains set). Further, the interrupt on overflow bit
doesn't appear to be set by the kernel driver, but the two match
registers are initialised to zero which ensures an interrupt will be
fired. Non-zero match registers aren't currently supported by the
device model. I'll try to make this all clearer in the code.
>
> >
> > >
> > > You should implement a VMState struct for device migration,
> > > and wire it up here via dc->vmsd.
> > I'll look into it. I started experimenting with a VMState struct
> > early on in the implementation but threw it away as it wasn't my
> > primary focus at the time.
> We insist on vmstate structs for all new devices, because
> they're fairly easy to implement, and if the original
> submitter doesn't implement one then the device becomes
> a landmine for any user trying migration or vmstate snapshots,
> because it will silently misbehave.
Yeah, no worries, v3 of the series will have VMState structs for all
devices.
Cheers,
Andrew
>
> thanks
> -- PMM
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v2 2/3] hw/intc: Add (new) ASPEED AST2400 AVIC device model
2016-02-16 11:34 [Qemu-devel] [PATCH v2 0/3] Add ASPEED AST2400 machine model Andrew Jeffery
2016-02-16 11:34 ` [Qemu-devel] [PATCH v2 1/3] hw/timer: Add ASPEED AST2400 timer device model Andrew Jeffery
@ 2016-02-16 11:34 ` Andrew Jeffery
2016-02-25 16:29 ` [Qemu-devel] [Qemu-arm] " Peter Maydell
2016-02-16 11:34 ` [Qemu-devel] [PATCH v2 3/3] hw/arm: Add ASPEED AST2400 machine type Andrew Jeffery
2 siblings, 1 reply; 15+ messages in thread
From: Andrew Jeffery @ 2016-02-16 11:34 UTC (permalink / raw)
To: qemu-devel; +Cc: openbmc, qemu-arm
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 | 256 +++++++++++++++++++++++++++++++++++++++++++
include/hw/intc/aspeed_vic.h | 40 +++++++
trace-events | 9 ++
4 files changed, 306 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 6a13a39..0e47f0f 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -31,3 +31,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/intc/aspeed_vic.c b/hw/intc/aspeed_vic.c
new file mode 100644
index 0000000..c000936
--- /dev/null
+++ b/hw/intc/aspeed_vic.c
@@ -0,0 +1,256 @@
+/*
+ * ASPEED Interrupt Controller (New)
+ *
+ * Andrew Jeffery <andrew@aj.id.au>
+ *
+ * Copyright 2015, 2016 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"
+#include "trace.h"
+
+#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);
+ trace_aspeed_vic_update_fiq(!!flags);
+
+ flags = new & ~s->int_select;
+ if (!flags) {
+ qemu_set_irq(s->irq, !!flags);
+ trace_aspeed_vic_update_all_fiq();
+ return;
+ }
+
+ for (i = 0; i < ASPEED_VIC_NR_IRQS; i++) {
+ if (flags & (UINT64_C(1) << i)) {
+ qemu_set_irq(s->irq, 1);
+ trace_aspeed_vic_update_i(i);
+ return;
+ }
+ }
+
+ qemu_set_irq(s->irq, 0);
+ trace_aspeed_vic_update_none();
+}
+
+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) {
+ s->edge_status |= (UINT64_C(1) << irq);
+ } else {
+ s->edge_status &= ~(UINT64_C(1) << irq);
+ }
+
+ trace_aspeed_vic_set_irq(irq, level);
+ 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;
+
+ switch (n_offset) {
+ case 0x0: /* IRQ Status */
+ val = s->edge_status & ~s->int_select & s->int_enable;
+ break;
+ case 0x08: /* FIQ Status */
+ val = s->edge_status & s->int_select & s->int_enable;
+ break;
+ case 0x10: /* Raw Interrupt Status */
+ val = s->edge_status;
+ break;
+ case 0x18: /* Interrupt Selection */
+ val = s->int_select;
+ break;
+ case 0x20: /* Interrupt Enable */
+ val = s->int_enable;
+ break;
+ case 0x30: /* Software Interrupt */
+ val = s->int_trigger;
+ break;
+ case 0x40: /* Interrupt Sensitivity */
+ val = s->int_sense;
+ break;
+ case 0x48: /* Interrupt Both Edge Trigger Control */
+ val = s->int_dual_edge;
+ break;
+ case 0x50: /* Interrupt Event */
+ val = s->int_event;
+ break;
+ case 0x60: /* Edge Triggered Interrupt Status */
+ val = s->edge_status;
+ break;
+ /* Illegal */
+ case 0x28: /* Interrupt Enable Clear */
+ case 0x38: /* Software Interrupt Clear */
+ case 0x58: /* Edge Triggered Interrupt Clear */
+ 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;
+ }
+ trace_aspeed_vic_read(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;
+
+ trace_aspeed_vic_write(offset, size, data);
+
+ if (high) {
+ data &= AVIC_H_MASK;
+ data <<= 32;
+ } else {
+ data &= AVIC_L_MASK;
+ }
+
+ switch (n_offset) {
+ case 0x18: /* Interrupt Selection */
+ 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: /* Interrupt Enable */
+ s->int_enable |= data;
+ break;
+ case 0x28: /* Interrupt Enable Clear */
+ s->int_enable &= ~data;
+ break;
+ case 0x30: /* Software Interrupt */
+ s->int_trigger |= data;
+ break;
+ case 0x38: /* Software Interrupt Clear */
+ s->int_trigger &= ~data;
+ break;
+ case 0x50: /* Interrupt Event */
+ s->int_event &= ~AVIC_INT_EVENT_W_MASK;
+ s->int_event |= (data & AVIC_INT_EVENT_W_MASK);
+ break;
+ case 0x58: /* Edge Triggered Interrupt Clear */
+ s->edge_status &= ~data;
+ break;
+ case 0x00: /* IRQ Status */
+ case 0x08: /* FIQ Status */
+ case 0x10: /* Raw Interrupt Status */
+ case 0x40: /* Interrupt Sensitivity */
+ case 0x48: /* Interrupt Both Edge Trigger Control */
+ case 0x60: /* Edge Triggered Interrupt Status */
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "Write of read-only register with offset 0x%" 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,
+ .valid.min_access_size = 4,
+ .valid.max_access_size = 4,
+ .valid.unaligned = false,
+};
+
+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..43b2ec6
--- /dev/null
+++ b/include/hw/intc/aspeed_vic.h
@@ -0,0 +1,40 @@
+/*
+ * 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
+
+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 */
diff --git a/trace-events b/trace-events
index 5325a23..5718bdf 100644
--- a/trace-events
+++ b/trace-events
@@ -1899,3 +1899,12 @@ aspeed_timer_ctrl_op_overflow_interrupt(uint8_t i, bool enable) "Overflow interr
aspeed_timer_set_ctrl2(uint32_t value) "CTRL2 set to 0x%" PRIx32
aspeed_timer_set_value(int timer, int reg, uint32_t value) "Write to register %d of timer %d: 0x%" PRIx32
aspeed_timer_read(uint64_t offset, unsigned size, uint64_t value) "Read at 0x%" PRIx64 ": of size %u 0x%" PRIx64
+
+# hw/intc/aspeed_vic.c
+aspeed_vic_set_irq(int irq, int level) "Enabling IRQ %d: %d"
+aspeed_vic_update_fiq(int flags) "Raising FIQ %d"
+aspeed_vic_update_all_fiq(void) "All IRQs marked as fast"
+aspeed_vic_update_i(int irq) "Raising IRQ %d"
+aspeed_vic_update_none(void) "Lowering IRQs"
+aspeed_vic_read(uint64_t offset, unsigned size, uint32_t value) "Read at 0x%" PRIx64 " of size %u: 0x% " PRIx64
+aspeed_vic_write(uint64_t offset, unsigned size, uint32_t data) "Write at 0x% " PRIx64 " of size %u: %0x" PRIx64
--
2.5.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH v2 2/3] hw/intc: Add (new) ASPEED AST2400 AVIC device model
2016-02-16 11:34 ` [Qemu-devel] [PATCH v2 2/3] hw/intc: Add (new) ASPEED AST2400 AVIC " Andrew Jeffery
@ 2016-02-25 16:29 ` Peter Maydell
2016-03-02 1:09 ` Andrew Jeffery
2016-03-03 5:14 ` Andrew Jeffery
0 siblings, 2 replies; 15+ messages in thread
From: Peter Maydell @ 2016-02-25 16:29 UTC (permalink / raw)
To: Andrew Jeffery; +Cc: openbmc, qemu-arm, QEMU Developers
On 16 February 2016 at 11:34, Andrew Jeffery <andrew@aj.id.au> wrote:
> 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.
What do we mean by "new" here? Were there multiple revisions of
this hardware?
> 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 | 256 +++++++++++++++++++++++++++++++++++++++++++
> include/hw/intc/aspeed_vic.h | 40 +++++++
> trace-events | 9 ++
> 4 files changed, 306 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 6a13a39..0e47f0f 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -31,3 +31,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/intc/aspeed_vic.c b/hw/intc/aspeed_vic.c
> new file mode 100644
> index 0000000..c000936
> --- /dev/null
> +++ b/hw/intc/aspeed_vic.c
> @@ -0,0 +1,256 @@
> +/*
> + * ASPEED Interrupt Controller (New)
> + *
> + * Andrew Jeffery <andrew@aj.id.au>
> + *
> + * Copyright 2015, 2016 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.
Is there real hardware that doesn't implement the legacy register
space? Should we be at least implementing it enough to do a
LOG_UNIMP log of the fact we don't implement it?
> + */
> +#include <inttypes.h>
Missing newline before #include. As with the other file, include
qemu/osdep.h first. (If you rebase on current master you'll find you
get compile errors otherwise.)
> +#include "hw/intc/aspeed_vic.h"
> +#include "trace.h"
> +
> +#define AVIC_L_MASK 0xFFFFFFFF
This needs a 'U' suffix or at least one of the compilers we build
with will complain.
> +#define AVIC_H_MASK 0x00007FFF
> +#define AVIC_INT_EVENT_W_MASK (UINT64_C(0x78000) << 32)
We generally use the ULL suffix rather than UINT64_C().
> +
> +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);
> + trace_aspeed_vic_update_fiq(!!flags);
> +
> + flags = new & ~s->int_select;
> + if (!flags) {
> + qemu_set_irq(s->irq, !!flags);
> + trace_aspeed_vic_update_all_fiq();
> + return;
> + }
> +
> + for (i = 0; i < ASPEED_VIC_NR_IRQS; i++) {
> + if (flags & (UINT64_C(1) << i)) {
> + qemu_set_irq(s->irq, 1);
> + trace_aspeed_vic_update_i(i);
> + return;
> + }
> + }
> +
> + qemu_set_irq(s->irq, 0);
I don't understand why you've written the FIQ setting code
in one (fairly straightforward) way, but the IRQ setting
code in a completely different way with an unnecessary loop
and a lot of redundant code.
> + trace_aspeed_vic_update_none();
> +}
> +
> +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) {
> + s->edge_status |= (UINT64_C(1) << irq);
> + } else {
> + s->edge_status &= ~(UINT64_C(1) << irq);
> + }
You can do this in one line with
s->edge_status = deposit64(s->edge_status, irq, 1, level);
Why is the field called "edge status" when the code here appears
to be treating it as a line level? Are we missing edge-detect-and-latch
logic ?
> +
> + trace_aspeed_vic_set_irq(irq, level);
> + 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;
> +
> + switch (n_offset) {
> + case 0x0: /* IRQ Status */
> + val = s->edge_status & ~s->int_select & s->int_enable;
> + break;
> + case 0x08: /* FIQ Status */
> + val = s->edge_status & s->int_select & s->int_enable;
> + break;
> + case 0x10: /* Raw Interrupt Status */
> + val = s->edge_status;
> + break;
> + case 0x18: /* Interrupt Selection */
> + val = s->int_select;
> + break;
> + case 0x20: /* Interrupt Enable */
> + val = s->int_enable;
> + break;
> + case 0x30: /* Software Interrupt */
> + val = s->int_trigger;
> + break;
> + case 0x40: /* Interrupt Sensitivity */
> + val = s->int_sense;
> + break;
> + case 0x48: /* Interrupt Both Edge Trigger Control */
> + val = s->int_dual_edge;
> + break;
> + case 0x50: /* Interrupt Event */
> + val = s->int_event;
> + break;
> + case 0x60: /* Edge Triggered Interrupt Status */
> + val = s->edge_status;
> + break;
> + /* Illegal */
> + case 0x28: /* Interrupt Enable Clear */
> + case 0x38: /* Software Interrupt Clear */
> + case 0x58: /* Edge Triggered Interrupt Clear */
> + 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;
val = extract64(val, 32, 15); ?
> + }
> + trace_aspeed_vic_read(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;
> +
> + trace_aspeed_vic_write(offset, size, data);
> +
> + if (high) {
> + data &= AVIC_H_MASK;
> + data <<= 32;
> + } else {
> + data &= AVIC_L_MASK;
> + }
> +
> + switch (n_offset) {
> + case 0x18: /* Interrupt Selection */
> + 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: /* Interrupt Enable */
> + s->int_enable |= data;
Are you sure this only ORs in new 1 bits?
I suspect you may want s->value = deposit64(s->value, start, length, data)
for at least some of these registers.
> + break;
> + case 0x28: /* Interrupt Enable Clear */
> + s->int_enable &= ~data;
> + break;
> + case 0x30: /* Software Interrupt */
> + s->int_trigger |= data;
> + break;
> + case 0x38: /* Software Interrupt Clear */
> + s->int_trigger &= ~data;
> + break;
> + case 0x50: /* Interrupt Event */
> + s->int_event &= ~AVIC_INT_EVENT_W_MASK;
> + s->int_event |= (data & AVIC_INT_EVENT_W_MASK);
> + break;
> + case 0x58: /* Edge Triggered Interrupt Clear */
> + s->edge_status &= ~data;
> + break;
> + case 0x00: /* IRQ Status */
> + case 0x08: /* FIQ Status */
> + case 0x10: /* Raw Interrupt Status */
> + case 0x40: /* Interrupt Sensitivity */
> + case 0x48: /* Interrupt Both Edge Trigger Control */
> + case 0x60: /* Edge Triggered Interrupt Status */
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "Write of read-only register with offset 0x%" 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,
> + .valid.min_access_size = 4,
> + .valid.max_access_size = 4,
> + .valid.unaligned = false,
> +};
> +
> +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;
Just do s->whatever = 0x123456789abcdef0ULL .
> + 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)";
You should have a VMState and register it here.
> +}
> +
> +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..43b2ec6
> --- /dev/null
> +++ b/include/hw/intc/aspeed_vic.h
> @@ -0,0 +1,40 @@
> +/*
> + * 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
> +
> +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;
You might like to order the fields in this struct so all the ones which
aren't device state that needs to be saved (iomem, irqs) are first,
and then all the fields corresponding to device hw state.
> +} AspeedVICState;
> +
> +#endif /* ASPEED_VIC_H */
> diff --git a/trace-events b/trace-events
> index 5325a23..5718bdf 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1899,3 +1899,12 @@ aspeed_timer_ctrl_op_overflow_interrupt(uint8_t i, bool enable) "Overflow interr
> aspeed_timer_set_ctrl2(uint32_t value) "CTRL2 set to 0x%" PRIx32
> aspeed_timer_set_value(int timer, int reg, uint32_t value) "Write to register %d of timer %d: 0x%" PRIx32
> aspeed_timer_read(uint64_t offset, unsigned size, uint64_t value) "Read at 0x%" PRIx64 ": of size %u 0x%" PRIx64
> +
> +# hw/intc/aspeed_vic.c
> +aspeed_vic_set_irq(int irq, int level) "Enabling IRQ %d: %d"
> +aspeed_vic_update_fiq(int flags) "Raising FIQ %d"
> +aspeed_vic_update_all_fiq(void) "All IRQs marked as fast"
> +aspeed_vic_update_i(int irq) "Raising IRQ %d"
> +aspeed_vic_update_none(void) "Lowering IRQs"
> +aspeed_vic_read(uint64_t offset, unsigned size, uint32_t value) "Read at 0x%" PRIx64 " of size %u: 0x% " PRIx64
> +aspeed_vic_write(uint64_t offset, unsigned size, uint32_t data) "Write at 0x% " PRIx64 " of size %u: %0x" PRIx64
Third argument is a uint32_t but format string is PRIx64 ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH v2 2/3] hw/intc: Add (new) ASPEED AST2400 AVIC device model
2016-02-25 16:29 ` [Qemu-devel] [Qemu-arm] " Peter Maydell
@ 2016-03-02 1:09 ` Andrew Jeffery
2016-03-02 12:41 ` Peter Maydell
2016-03-03 5:14 ` Andrew Jeffery
1 sibling, 1 reply; 15+ messages in thread
From: Andrew Jeffery @ 2016-03-02 1:09 UTC (permalink / raw)
To: Peter Maydell; +Cc: openbmc, qemu-arm, QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 16247 bytes --]
Hi Peter,
On Thu, 2016-02-25 at 16:29 +0000, Peter Maydell wrote:
> On 16 February 2016 at 11:34, Andrew Jeffery <andrew@aj.id.au> wrote:
> > 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.
>
> What do we mean by "new" here? Were there multiple revisions of
> this hardware?
Yes, I'll try rework the commit message to clear this up.
>
> > 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.
FWIW I've moved this to a comment in the source as suggested in an off
-list review by Alexey Kardashevskiy.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> > hw/intc/Makefile.objs | 1 +
> > hw/intc/aspeed_vic.c | 256 +++++++++++++++++++++++++++++++++++++++++++
> > include/hw/intc/aspeed_vic.h | 40 +++++++
> > trace-events | 9 ++
> > 4 files changed, 306 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 6a13a39..0e47f0f 100644
> > --- a/hw/intc/Makefile.objs
> > +++ b/hw/intc/Makefile.objs
> > @@ -31,3 +31,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/intc/aspeed_vic.c b/hw/intc/aspeed_vic.c
> > new file mode 100644
> > index 0000000..c000936
> > --- /dev/null
> > +++ b/hw/intc/aspeed_vic.c
> > @@ -0,0 +1,256 @@
> > +/*
> > + * ASPEED Interrupt Controller (New)
> > + *
> > + * Andrew Jeffery <andrew@aj.id.au>
> > + *
> > + * Copyright 2015, 2016 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.
>
> Is there real hardware that doesn't implement the legacy register
> space?
I don't immediately know the answer, but both the AST2400 and AST2500
retain the legacy register set (in addition to providing the new set).
> Should we be at least implementing it enough to do a
> LOG_UNIMP log of the fact we don't implement it?
Yeah that sounds useful, I'll look into it.
>
> > + */
> > +#include
>
> Missing newline before #include. As with the other file, include
> qemu/osdep.h first. (If you rebase on current master you'll find you
> get compile errors otherwise.)
> > +#include "hw/intc/aspeed_vic.h"
> > +#include "trace.h"
> > +
> > +#define AVIC_L_MASK 0xFFFFFFFF
>
> This needs a 'U' suffix or at least one of the compilers we build
> with will complain.
Okay. Out of interest, what compiler will complain?
>
> > +#define AVIC_H_MASK 0x00007FFF
> > +#define AVIC_INT_EVENT_W_MASK (UINT64_C(0x78000) << 32)
>
> We generally use the ULL suffix rather than UINT64_C().
Okay.
> > +
> > +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);
> > + trace_aspeed_vic_update_fiq(!!flags);
> > +
> > + flags = new & ~s->int_select;
> > + if (!flags) {
> > + qemu_set_irq(s->irq, !!flags);
> > + trace_aspeed_vic_update_all_fiq();
> > + return;
> > + }
> > +
> > + for (i = 0; i < ASPEED_VIC_NR_IRQS; i++) {
> > + if (flags & (UINT64_C(1) << i)) {
> > + qemu_set_irq(s->irq, 1);
> > + trace_aspeed_vic_update_i(i);
> > + return;
> > + }
> > + }
> > +
> > + qemu_set_irq(s->irq, 0);
>
> I don't understand why you've written the FIQ setting code
> in one (fairly straightforward) way, but the IRQ setting
> code in a completely different way with an unnecessary loop
> and a lot of redundant code.
Good point - I've changed to the fairly-straightfoward way for both. As
a justification I started with the i.MX31 VIC update implementation and
removed the intmask feature (as my reading of the datahsheet doesn't
show an equivalent, just provides the ability to mask specific
interrupts), and left the rest alone. I should have reasoned about it a
bit further.
> > + trace_aspeed_vic_update_none();
> > +}
> > +
> > +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) {
> > + s->edge_status |= (UINT64_C(1) << irq);
> > + } else {
> > + s->edge_status &= ~(UINT64_C(1) << irq);
> > + }
>
> You can do this in one line with
> s->edge_status = deposit64(s->edge_status, irq, 1, level);
Okay.
>
> Why is the field called "edge status" when the code here appears
> to be treating it as a line level? Are we missing edge-detect-and-latch
> logic ?
Agh, yep, this is an abuse of the edge_status register. I'll rework the
code to respect edge vs level.
>
> > +
> > + trace_aspeed_vic_set_irq(irq, level);
> > + 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;
> > +
> > + switch (n_offset) {
> > + case 0x0: /* IRQ Status */
> > + val = s->edge_status & ~s->int_select & s->int_enable;
> > + break;
> > + case 0x08: /* FIQ Status */
> > + val = s->edge_status & s->int_select & s->int_enable;
> > + break;
> > + case 0x10: /* Raw Interrupt Status */
> > + val = s->edge_status;
> > + break;
> > + case 0x18: /* Interrupt Selection */
> > + val = s->int_select;
> > + break;
> > + case 0x20: /* Interrupt Enable */
> > + val = s->int_enable;
> > + break;
> > + case 0x30: /* Software Interrupt */
> > + val = s->int_trigger;
> > + break;
> > + case 0x40: /* Interrupt Sensitivity */
> > + val = s->int_sense;
> > + break;
> > + case 0x48: /* Interrupt Both Edge Trigger Control */
> > + val = s->int_dual_edge;
> > + break;
> > + case 0x50: /* Interrupt Event */
> > + val = s->int_event;
> > + break;
> > + case 0x60: /* Edge Triggered Interrupt Status */
> > + val = s->edge_status;
> > + break;
> > + /* Illegal */
> > + case 0x28: /* Interrupt Enable Clear */
> > + case 0x38: /* Software Interrupt Clear */
> > + case 0x58: /* Edge Triggered Interrupt Clear */
> > + 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;
>
> val = extract64(val, 32, 15); ?
Will do.
>
> > + }
> > + trace_aspeed_vic_read(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;
> > +
> > + trace_aspeed_vic_write(offset, size, data);
> > +
> > + if (high) {
> > + data &= AVIC_H_MASK;
> > + data <<= 32;
> > + } else {
> > + data &= AVIC_L_MASK;
> > + }
> > +
> > + switch (n_offset) {
> > + case 0x18: /* Interrupt Selection */
> > + 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: /* Interrupt Enable */
> > + s->int_enable |= data;
>
> Are you sure this only ORs in new 1 bits?
>
> I suspect you may want s->value = deposit64(s->value, start, length, data)
> for at least some of these registers.
I agree.
>
> > + break;
> > + case 0x28: /* Interrupt Enable Clear */
> > + s->int_enable &= ~data;
> > + break;
> > + case 0x30: /* Software Interrupt */
> > + s->int_trigger |= data;
> > + break;
> > + case 0x38: /* Software Interrupt Clear */
> > + s->int_trigger &= ~data;
> > + break;
> > + case 0x50: /* Interrupt Event */
> > + s->int_event &= ~AVIC_INT_EVENT_W_MASK;
> > + s->int_event |= (data & AVIC_INT_EVENT_W_MASK);
> > + break;
> > + case 0x58: /* Edge Triggered Interrupt Clear */
> > + s->edge_status &= ~data;
> > + break;
> > + case 0x00: /* IRQ Status */
> > + case 0x08: /* FIQ Status */
> > + case 0x10: /* Raw Interrupt Status */
> > + case 0x40: /* Interrupt Sensitivity */
> > + case 0x48: /* Interrupt Both Edge Trigger Control */
> > + case 0x60: /* Edge Triggered Interrupt Status */
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "Write of read-only register with offset 0x%" 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,
> > + .valid.min_access_size = 4,
> > + .valid.max_access_size = 4,
> > + .valid.unaligned = false,
> > +};
> > +
> > +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;
>
> Just do s->whatever = 0x123456789abcdef0ULL .
Will do
>
> > + 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)";
>
> You should have a VMState and register it here.
Will do.
>
> > +}
> > +
> > +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..43b2ec6
> > --- /dev/null
> > +++ b/include/hw/intc/aspeed_vic.h
> > @@ -0,0 +1,40 @@
> > +/*
> > + * 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
> > +
> > +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;
>
> You might like to order the fields in this struct so all the ones which
> aren't device state that needs to be saved (iomem, irqs) are first,
> and then all the fields corresponding to device hw state.
Will do.
>
> > +} AspeedVICState;
> > +
> > +#endif /* ASPEED_VIC_H */
> > diff --git a/trace-events b/trace-events
> > index 5325a23..5718bdf 100644
> > --- a/trace-events
> > +++ b/trace-events
> > @@ -1899,3 +1899,12 @@ aspeed_timer_ctrl_op_overflow_interrupt(uint8_t i, bool enable) "Overflow interr
> > aspeed_timer_set_ctrl2(uint32_t value) "CTRL2 set to 0x%" PRIx32
> > aspeed_timer_set_value(int timer, int reg, uint32_t value) "Write to register %d of timer %d: 0x%" PRIx32
> > aspeed_timer_read(uint64_t offset, unsigned size, uint64_t value) "Read at 0x%" PRIx64 ": of size %u 0x%" PRIx64
> > +
> > +# hw/intc/aspeed_vic.c
> > +aspeed_vic_set_irq(int irq, int level) "Enabling IRQ %d: %d"
> > +aspeed_vic_update_fiq(int flags) "Raising FIQ %d"
> > +aspeed_vic_update_all_fiq(void) "All IRQs marked as fast"
> > +aspeed_vic_update_i(int irq) "Raising IRQ %d"
> > +aspeed_vic_update_none(void) "Lowering IRQs"
> > +aspeed_vic_read(uint64_t offset, unsigned size, uint32_t value) "Read at 0x%" PRIx64 " of size %u: 0x% " PRIx64
> > +aspeed_vic_write(uint64_t offset, unsigned size, uint32_t data) "Write at 0x% " PRIx64 " of size %u: %0x" PRIx64
>
> Third argument is a uint32_t but format string is PRIx64 ?
Turns out there are several issues with the the traces I've added, none
of them good. I had tested the tracing by enabling the simple backend
(following docs/tracing.txt), which ignored all the problems :( I'll be
more thorough in the future.
>
> thanks
> -- PMM
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH v2 2/3] hw/intc: Add (new) ASPEED AST2400 AVIC device model
2016-03-02 1:09 ` Andrew Jeffery
@ 2016-03-02 12:41 ` Peter Maydell
0 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2016-03-02 12:41 UTC (permalink / raw)
To: Andrew Jeffery; +Cc: openbmc, qemu-arm, QEMU Developers
On 2 March 2016 at 01:09, Andrew Jeffery <andrew@aj.id.au> wrote:
> Hi Peter,
>
> On Thu, 2016-02-25 at 16:29 +0000, Peter Maydell wrote:
>> On 16 February 2016 at 11:34, Andrew Jeffery <andrew@aj.id.au> wrote:
>> > +#define AVIC_L_MASK 0xFFFFFFFF
>>
>> This needs a 'U' suffix or at least one of the compilers we build
>> with will complain.
>
> Okay. Out of interest, what compiler will complain?
Ancient gcc 4.2. (Actually I've now dropped that from the testing
set, but I still tend to recommend U suffixes in review.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH v2 2/3] hw/intc: Add (new) ASPEED AST2400 AVIC device model
2016-02-25 16:29 ` [Qemu-devel] [Qemu-arm] " Peter Maydell
2016-03-02 1:09 ` Andrew Jeffery
@ 2016-03-03 5:14 ` Andrew Jeffery
2016-03-03 8:39 ` Peter Maydell
1 sibling, 1 reply; 15+ messages in thread
From: Andrew Jeffery @ 2016-03-03 5:14 UTC (permalink / raw)
To: Peter Maydell; +Cc: openbmc, qemu-arm, QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 885 bytes --]
On Thu, 2016-02-25 at 16:29 +0000, Peter Maydell wrote:
> > + case 0x20: /* Interrupt Enable */
> > + s->int_enable |= data;
>
> Are you sure this only ORs in new 1 bits?
As in, am I sure I only want to take the newly set bits? If so, yes, as
the the following register serves to clear the field's set bits:
>
> > + break;
> > + case 0x28: /* Interrupt Enable Clear */
> > + s->int_enable &= ~data;
> > + break;
The 'int_enable', 'int_trigger' and 'edge_status' fields all use the pa
ttern of separate set and clear registers (the remaining registers may
benefit from the extract64/deposit64 helpers, I'll think about that
further). I'll add some comments to help clear this up.
Otherwise, can you rephrase the question? At face value it seems like
you're implying that I'm doing more than ORing in the new 1 bits?
Andrew
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH v2 2/3] hw/intc: Add (new) ASPEED AST2400 AVIC device model
2016-03-03 5:14 ` Andrew Jeffery
@ 2016-03-03 8:39 ` Peter Maydell
2016-03-03 10:16 ` Andrew Jeffery
0 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2016-03-03 8:39 UTC (permalink / raw)
To: Andrew Jeffery; +Cc: openbmc, qemu-arm, QEMU Developers
On 3 March 2016 at 05:14, Andrew Jeffery <andrew@aj.id.au> wrote:
> On Thu, 2016-02-25 at 16:29 +0000, Peter Maydell wrote:
>> > + case 0x20: /* Interrupt Enable */
>> > + s->int_enable |= data;
>>
>> Are you sure this only ORs in new 1 bits?
>
> As in, am I sure I only want to take the newly set bits? If so, yes, as
> the the following register serves to clear the field's set bits:
>
>>
>> > + break;
>> > + case 0x28: /* Interrupt Enable Clear */
>> > + s->int_enable &= ~data;
>> > + break;
>
> The 'int_enable', 'int_trigger' and 'edge_status' fields all use the pa
> ttern of separate set and clear registers (the remaining registers may
> benefit from the extract64/deposit64 helpers, I'll think about that
> further). I'll add some comments to help clear this up.
>
> Otherwise, can you rephrase the question? At face value it seems like
> you're implying that I'm doing more than ORing in the new 1 bits?
It was just that the register name didn't imply a set-bits-only
semantic and some of the other registers looked like they were
also incorrectly not handling updates right.
thanks
-- PMM
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH v2 2/3] hw/intc: Add (new) ASPEED AST2400 AVIC device model
2016-03-03 8:39 ` Peter Maydell
@ 2016-03-03 10:16 ` Andrew Jeffery
0 siblings, 0 replies; 15+ messages in thread
From: Andrew Jeffery @ 2016-03-03 10:16 UTC (permalink / raw)
To: Peter Maydell; +Cc: openbmc, qemu-arm, QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 1690 bytes --]
On Thu, 2016-03-03 at 08:39 +0000, Peter Maydell wrote:
> On 3 March 2016 at 05:14, Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > On Thu, 2016-02-25 at 16:29 +0000, Peter Maydell wrote:
> > >
> > > >
> > > > + case 0x20: /* Interrupt Enable */
> > > > + s->int_enable |= data;
> > > Are you sure this only ORs in new 1 bits?
> > As in, am I sure I only want to take the newly set bits? If so, yes, as
> > the the following register serves to clear the field's set bits:
> >
> > >
> > >
> > > >
> > > > + break;
> > > > + case 0x28: /* Interrupt Enable Clear */
> > > > + s->int_enable &= ~data;
> > > > + break;
> > The 'int_enable', 'int_trigger' and 'edge_status' fields all use the pa
> > ttern of separate set and clear registers (the remaining registers may
> > benefit from the extract64/deposit64 helpers, I'll think about that
> > further). I'll add some comments to help clear this up.
> >
> > Otherwise, can you rephrase the question? At face value it seems like
> > you're implying that I'm doing more than ORing in the new 1 bits?
> It was just that the register name didn't imply a set-bits-only
> semantic and some of the other registers looked like they were
> also incorrectly not handling updates right.
That's a fair call: The comments I added were the documented register
names, which don't provide huge insight on their own. This is probably
a less-than-ideal approach given the datasheet is largely unavailable.
As mentioned above I'll add some comments on the different access
patterns and where they apply to try clear this up.
Cheers,
Andrew
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v2 3/3] hw/arm: Add ASPEED AST2400 machine type
2016-02-16 11:34 [Qemu-devel] [PATCH v2 0/3] Add ASPEED AST2400 machine model Andrew Jeffery
2016-02-16 11:34 ` [Qemu-devel] [PATCH v2 1/3] hw/timer: Add ASPEED AST2400 timer device model Andrew Jeffery
2016-02-16 11:34 ` [Qemu-devel] [PATCH v2 2/3] hw/intc: Add (new) ASPEED AST2400 AVIC " Andrew Jeffery
@ 2016-02-16 11:34 ` Andrew Jeffery
2016-02-25 16:36 ` [Qemu-devel] [Qemu-arm] " Peter Maydell
2 siblings, 1 reply; 15+ messages in thread
From: Andrew Jeffery @ 2016-02-16 11:34 UTC (permalink / raw)
To: qemu-devel; +Cc: openbmc, qemu-arm
Adds the AST2400 machine type with ASPEED AVIC and timer models. The
new machine type is functional enough to boot Linux to userspace.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
hw/arm/Makefile.objs | 1 +
hw/arm/ast2400.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++
trace-events | 4 ++
3 files changed, 144 insertions(+)
create mode 100644 hw/arm/ast2400.c
diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index a711e4d..f333b7f 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -16,3 +16,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..e3cd496
--- /dev/null
+++ b/hw/arm/ast2400.c
@@ -0,0 +1,139 @@
+/*
+ * ASPEED AST2400
+ *
+ * Jeremy Kerr <jk@ozlabs.org>
+ * Andrew Jeffery <andrew@aj.id.au>
+ *
+ * Copyright 2015, 2016 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 "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"
+#include "trace.h"
+
+#define ASPEED_SDRAM_BASE 0x40000000
+#define ASPEED_IOMEM_BASE 0x1e600000
+#define ASPEED_IOMEM_SIZE 0x00200000
+#define ASPEED_TIMER_BASE 0x1E782000
+#define ASPEED_VIC_BASE 0x1E6C0080
+
+static struct arm_boot_info ast2400_binfo = {
+ .loader_start = ASPEED_SDRAM_BASE,
+ .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)
+{
+ trace_ast2400_io_read(offset, size);
+ return 0;
+}
+
+static void ast2400_io_write(void *opaque, hwaddr offset, uint64_t value,
+ unsigned size)
+{
+ trace_ast2400_io_write(offset, value, size);
+}
+
+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_BASE,
+ 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, ASPEED_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/trace-events b/trace-events
index 5718bdf..e19dfc6 100644
--- a/trace-events
+++ b/trace-events
@@ -1908,3 +1908,7 @@ aspeed_vic_update_i(int irq) "Raising IRQ %d"
aspeed_vic_update_none(void) "Lowering IRQs"
aspeed_vic_read(uint64_t offset, unsigned size, uint32_t value) "Read at 0x%" PRIx64 " of size %u: 0x% " PRIx64
aspeed_vic_write(uint64_t offset, unsigned size, uint32_t data) "Write at 0x% " PRIx64 " of size %u: %0x" PRIx64
+
+# hw/arm/ast2400.c
+ast2400_io_read(uint64_t offset, unsigned size) "0x%" HWADDR_PRIx "[%d]"
+ast2400_io_write(uint64_t offset, uint64_t value, unsigned size) "0x%" HWADDR_PRIx "[%d] <- 0x" PRIx64
--
2.5.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH v2 3/3] hw/arm: Add ASPEED AST2400 machine type
2016-02-16 11:34 ` [Qemu-devel] [PATCH v2 3/3] hw/arm: Add ASPEED AST2400 machine type Andrew Jeffery
@ 2016-02-25 16:36 ` Peter Maydell
0 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2016-02-25 16:36 UTC (permalink / raw)
To: Andrew Jeffery; +Cc: openbmc, qemu-arm, QEMU Developers
On 16 February 2016 at 11:34, Andrew Jeffery <andrew@aj.id.au> wrote:
> Adds the AST2400 machine type with ASPEED AVIC and timer models. The
> new machine type is functional enough to boot Linux to userspace.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
This seems to be missing an "SoC object" level of modelling between the
devices and the board code. Have a look at the Xilinx, raspi, etc board
models for examples.
> ---
> hw/arm/Makefile.objs | 1 +
> hw/arm/ast2400.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++
> trace-events | 4 ++
> 3 files changed, 144 insertions(+)
> create mode 100644 hw/arm/ast2400.c
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index a711e4d..f333b7f 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -16,3 +16,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..e3cd496
> --- /dev/null
> +++ b/hw/arm/ast2400.c
> @@ -0,0 +1,139 @@
> +/*
> + * ASPEED AST2400
> + *
> + * Jeremy Kerr <jk@ozlabs.org>
> + * Andrew Jeffery <andrew@aj.id.au>
> + *
> + * Copyright 2015, 2016 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
Really? :-)
> + *
> + * 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.
This looks like copy-n-paste error too.
> + */
> +
#include "qemu/osdep.h".
> +#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"
> +#include "trace.h"
> +
> +#define ASPEED_SDRAM_BASE 0x40000000
> +#define ASPEED_IOMEM_BASE 0x1e600000
> +#define ASPEED_IOMEM_SIZE 0x00200000
> +#define ASPEED_TIMER_BASE 0x1E782000
> +#define ASPEED_VIC_BASE 0x1E6C0080
> +
> +static struct arm_boot_info ast2400_binfo = {
> + .loader_start = ASPEED_SDRAM_BASE,
> + .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)
> +{
> + trace_ast2400_io_read(offset, size);
This is just to diagnose "missing device implementation", right?
Shouldn't we be using a LOG_UNIMP instead?
> + return 0;
> +}
> +
> +static void ast2400_io_write(void *opaque, hwaddr offset, uint64_t value,
> + unsigned size)
> +{
> + trace_ast2400_io_write(offset, value, size);
> +}
> +
> +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";
What we do now is not let the user override the cpu model at all;
presumably this SoC only ever has an ARM926 and it doesn't make
any sense to have some frankenstein "this SoC but with a different
CPU in it" config.
> + }
> +
> + ast2400.cpu = cpu_arm_init(machine->cpu_model);
> + if (!ast2400.cpu) {
> + fprintf(stderr, "Unable to find CPU definition\n");
Prefer error_report() to plain fprintf.
> + 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_BASE,
> + 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, ASPEED_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);
> + }
Most of this should live in the SoC object rather than the board model.
> +
> + 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;
> +}
Should we be setting more of the MachineClass fields here?
(eg block_default_type, no_parallel, no_floppy, no_cdrom, max_cpus,
default_ram_size ?)
thanks
-- PMM
^ permalink raw reply [flat|nested] 15+ messages in thread