* [Qemu-devel] [PATCH v3 1/3] hw/timer: Add ASPEED timer device model
2016-03-05 4:29 [Qemu-devel] [PATCH v3 0/3] Add ASPEED AST2400 machine model Andrew Jeffery
@ 2016-03-05 4:29 ` Andrew Jeffery
2016-03-11 8:56 ` Peter Maydell
2016-03-05 4:29 ` [Qemu-devel] [PATCH v3 2/3] hw/intc: Add (new) ASPEED VIC " Andrew Jeffery
2016-03-05 4:29 ` [Qemu-devel] [PATCH v3 3/3] hw/arm: Add ASPEED AST2400 machine model Andrew Jeffery
2 siblings, 1 reply; 9+ messages in thread
From: Andrew Jeffery @ 2016-03-05 4:29 UTC (permalink / raw)
To: Peter Maydell
Cc: Alexey Kardashevskiy, qemu-arm, Cédric Le Goater, qemu-devel
Implement basic AST2400 timer functionality: Up to 8 timers can
independently be configured, enabled, reset and disabled. A couple of
hardware features are not implemented, namely clock value matching and
pulse generation, but the implementation is enough to boot the Linux
kernel configured with aspeed_defconfig.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
Since v2:
* Improve handling of timer configuration with respect to enabled state
* Remove redundant enabled member from AspeedTimer
* Implement VMStateDescriptions
* Fix interrupt behaviour (edge triggered, both edges)
* Fix various issues with trace-event declarations
* Include qemu/osdep.h
Since v1:
* Refactor initialisation of and respect requested clock rates (APB/External)
* Simplify some index calculations
* Use tracing infrastructure instead of internal DPRINTF
* Enforce access size constraints and alignment in MemoryRegionOps
default-configs/arm-softmmu.mak | 1 +
hw/timer/Makefile.objs | 1 +
hw/timer/aspeed_timer.c | 452 ++++++++++++++++++++++++++++++++++++++++
include/hw/timer/aspeed_timer.h | 59 ++++++
trace-events | 9 +
5 files changed, 522 insertions(+)
create mode 100644 hw/timer/aspeed_timer.c
create mode 100644 include/hw/timer/aspeed_timer.h
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index a9f82a1..2bcd236 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -110,3 +110,4 @@ CONFIG_IOH3420=y
CONFIG_I82801B11=y
CONFIG_ACPI=y
CONFIG_SMBIOS=y
+CONFIG_ASPEED_SOC=y
diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index 5cfea6e..003c14f 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -32,3 +32,4 @@ obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o
common-obj-$(CONFIG_STM32F2XX_TIMER) += stm32f2xx_timer.o
+common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o
diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
new file mode 100644
index 0000000..f1d232c
--- /dev/null
+++ b/hw/timer/aspeed_timer.c
@@ -0,0 +1,452 @@
+/*
+ * ASPEED AST2400 Timer
+ *
+ * Andrew Jeffery <andrew@aj.id.au>
+ *
+ * Copyright (C) 2016 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/ptimer.h"
+#include "hw/sysbus.h"
+#include "hw/timer/aspeed_timer.h"
+#include "qemu-common.h"
+#include "qemu/bitops.h"
+#include "qemu/main-loop.h"
+#include "qemu/timer.h"
+#include "trace.h"
+
+#define TIMER_NR_REGS 4
+
+#define TIMER_CTRL_BITS 4
+#define TIMER_CTRL_MASK ((1 << TIMER_CTRL_BITS) - 1)
+
+#define TIMER_CLOCK_USE_EXT true
+#define TIMER_CLOCK_EXT_HZ 1000000
+#define TIMER_CLOCK_USE_APB false
+#define TIMER_CLOCK_APB_HZ 24000000
+
+#define TIMER_REG_STATUS 0
+#define TIMER_REG_RELOAD 1
+#define TIMER_REG_MATCH_FIRST 2
+#define TIMER_REG_MATCH_SECOND 3
+
+#define TIMER_FIRST_CAP_PULSE 4
+
+enum timer_ctrl_op {
+ op_enable = 0,
+ op_external_clock,
+ op_overflow_interrupt,
+ op_pulse_enable
+};
+
+/**
+ * Avoid mutual references between AspeedTimerCtrlState and AspeedTimer
+ * structs, as it's a waste of memory and it makes implementing
+ * VMStateDescription a little clunky. The ptimer BH callback needs to know
+ * whether a specific AspeedTimer is enabled, but this information is held in
+ * AspeedTimerCtrlState. So, provide a helper to hoist ourselves from an
+ * arbitrary AspeedTimer to AspeedTimerCtrlState.
+ */
+static inline struct AspeedTimerCtrlState *timer_to_ctrl(AspeedTimer *t)
+{
+ AspeedTimer (*timers)[] = (void *)t - (t->id * sizeof(*t));
+ return container_of(timers, AspeedTimerCtrlState, timers);
+}
+
+static inline bool timer_ctrl_status(AspeedTimer *t, enum timer_ctrl_op op)
+{
+ return !!(timer_to_ctrl(t)->ctrl & BIT(t->id * TIMER_CTRL_BITS + op));
+}
+
+static inline bool timer_enabled(AspeedTimer *t)
+{
+ return timer_ctrl_status(t, op_enable);
+}
+
+static inline bool timer_overflow_interrupt(AspeedTimer *t)
+{
+ return timer_ctrl_status(t, op_overflow_interrupt);
+}
+
+static inline bool timer_can_pulse(AspeedTimer *t)
+{
+ return t->id >= TIMER_FIRST_CAP_PULSE;
+}
+
+static void aspeed_timer_expire(void *opaque)
+{
+ AspeedTimer *t = opaque;
+
+ /* Only support interrupts on match values of zero for the moment - this is
+ * sufficient to boot an aspeed_defconfig Linux kernel. Non-zero match
+ * values need some further consideration given the current ptimer API.
+ * Maybe run multiple ptimers?
+ */
+ bool match = !(t->match[0] && t->match[1]);
+ bool interrupt = timer_overflow_interrupt(t) || match;
+ if (timer_enabled(t) && interrupt) {
+ t->level = !t->level;
+ qemu_set_irq(t->irq, t->level);
+ }
+}
+
+static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg)
+{
+ uint64_t value;
+
+ switch (reg) {
+ case TIMER_REG_STATUS:
+ value = ptimer_get_count(t->timer);
+ break;
+ case TIMER_REG_RELOAD:
+ value = t->reload;
+ break;
+ case TIMER_REG_MATCH_FIRST:
+ case TIMER_REG_MATCH_SECOND:
+ value = t->match[reg - 2];
+ break;
+ default:
+ qemu_log_mask(LOG_UNIMP, "%s: Programming error: unexpected reg: %d\n",
+ __func__, reg);
+ value = 0;
+ break;
+ }
+ return value;
+}
+
+static uint64_t aspeed_timer_read(void *opaque, hwaddr offset, unsigned size)
+{
+ AspeedTimerCtrlState *s = opaque;
+ const int reg = (offset & 0xf) / 4;
+ uint64_t value;
+
+ switch (offset) {
+ case 0x30: /* Control Register */
+ value = s->ctrl;
+ break;
+ case 0x34: /* Control Register 2 */
+ value = s->ctrl2;
+ break;
+ case 0x00 ... 0x2c: /* Timers 1 - 4 */
+ value = aspeed_timer_get_value(&s->timers[(offset >> 4)], reg);
+ break;
+ case 0x40 ... 0x8c: /* Timers 5 - 8 */
+ value = aspeed_timer_get_value(&s->timers[(offset >> 4) - 1], reg);
+ break;
+ /* Illegal */
+ case 0x38:
+ case 0x3C:
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ value = 0;
+ break;
+ }
+ trace_aspeed_timer_read(offset, size, value);
+ return value;
+}
+
+static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
+ uint32_t value)
+{
+ AspeedTimer *t;
+
+ g_assert(timer >= 0 && timer < ASPEED_TIMER_NR_TIMERS);
+ trace_aspeed_timer_set_value(timer, reg, value);
+ t = &s->timers[timer];
+ switch (reg) {
+ case TIMER_REG_STATUS:
+ if (timer_enabled(t)) {
+ ptimer_set_count(t->timer, value);
+ }
+ break;
+ case TIMER_REG_RELOAD:
+ t->reload = value;
+ ptimer_set_limit(t->timer, value, 1);
+ break;
+ case TIMER_REG_MATCH_FIRST:
+ case TIMER_REG_MATCH_SECOND:
+ if (value) {
+ /* Non-zero match values are unsupported. As such an interrupt will
+ * always be triggered when the timer reaches zero even if the
+ * overflow interrupt control bit is clear.
+ */
+ qemu_log_mask(LOG_UNIMP, "%s: Match value unsupported by device: "
+ "0x%" PRIx32 "\n", __func__, value);
+ } else {
+ t->match[reg - 2] = value;
+ }
+ break;
+ default:
+ qemu_log_mask(LOG_UNIMP, "%s: Programming error: unexpected reg: %d\n",
+ __func__, reg);
+ break;
+ }
+}
+
+/* Control register operations are broken out into helpers that can be
+ * explictly called on aspeed_timer_reset(), but also from
+ * aspeed_timer_ctrl_op().
+ */
+
+static void aspeed_timer_ctrl_enable(AspeedTimer *t, bool enable)
+{
+ trace_aspeed_timer_ctrl_enable(t->id, enable);
+ if (enable) {
+ ptimer_run(t->timer, 0);
+ } else {
+ ptimer_stop(t->timer);
+ ptimer_set_limit(t->timer, t->reload, 1);
+ }
+}
+
+static void aspeed_timer_ctrl_external_clock(AspeedTimer *t, bool enable)
+{
+ trace_aspeed_timer_ctrl_external_clock(t->id, enable);
+ if (enable) {
+ ptimer_set_freq(t->timer, TIMER_CLOCK_EXT_HZ);
+ } else {
+ ptimer_set_freq(t->timer, TIMER_CLOCK_APB_HZ);
+ }
+}
+
+static void aspeed_timer_ctrl_overflow_interrupt(AspeedTimer *t, bool enable)
+{
+ trace_aspeed_timer_ctrl_overflow_interrupt(t->id, enable);
+}
+
+static void aspeed_timer_ctrl_pulse_enable(AspeedTimer *t, bool enable)
+{
+ if (timer_can_pulse(t)) {
+ trace_aspeed_timer_ctrl_pulse_enable(t->id, enable);
+ } else {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Timer does not support pulse mode\n", __func__);
+ }
+}
+
+/**
+ * Given the actions are fixed in number and completely described in helper
+ * functions, dispatch with a lookup table rather than manage control flow with
+ * a switch statement.
+ */
+static void (*const ctrl_ops[])(AspeedTimer *, bool) = {
+ [op_enable] = aspeed_timer_ctrl_enable,
+ [op_external_clock] = aspeed_timer_ctrl_external_clock,
+ [op_overflow_interrupt] = aspeed_timer_ctrl_overflow_interrupt,
+ [op_pulse_enable] = aspeed_timer_ctrl_pulse_enable,
+};
+
+/**
+ * Conditionally affect changes chosen by a timer's control bit.
+ *
+ * The aspeed_timer_ctrl_op() interface is convenient for the
+ * aspeed_timer_set_ctrl() function as the "no change" early exit can be
+ * calculated for all operations, which cleans up the caller code. However the
+ * interface isn't convenient for the reset function where we want to enter a
+ * specific state without artificially constructing old and new values that
+ * will fall through the change guard (and motivates extracting the actions
+ * out to helper functions).
+ *
+ * @t: The timer to manipulate
+ * @op: The type of operation to be performed
+ * @old: The old state of the timer's control bits
+ * @new: The incoming state for the timer's control bits
+ */
+static void aspeed_timer_ctrl_op(AspeedTimer *t, enum timer_ctrl_op op,
+ uint8_t old, uint8_t new)
+{
+ const uint8_t mask = BIT(op);
+ const bool enable = !!(new & mask);
+ const bool changed = ((old ^ new) & mask);
+ if (!changed) {
+ return;
+ }
+ ctrl_ops[op](t, enable);
+}
+
+static void aspeed_timer_set_ctrl(AspeedTimerCtrlState *s, uint32_t reg)
+{
+ int i;
+ int shift;
+ uint8_t t_old, t_new;
+ AspeedTimer *t;
+ const uint8_t enable_mask = BIT(op_enable);
+
+ /* Handle a dependency between the 'enable' and remaining three
+ * configuration bits - i.e. if more than one bit in the control set has
+ * changed, including the 'enable' bit, then we want either disable the
+ * timer and perform configuration, or perform configuration and then
+ * enable the timer
+ */
+ for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
+ t = &s->timers[i];
+ shift = (i * TIMER_CTRL_BITS);
+ t_old = (s->ctrl >> shift) & TIMER_CTRL_MASK;
+ t_new = (reg >> shift) & TIMER_CTRL_MASK;
+
+ /* If we are disabling, do so first */
+ if ((t_old & enable_mask) && !(t_new & enable_mask)) {
+ aspeed_timer_ctrl_enable(t, false);
+ }
+ aspeed_timer_ctrl_op(t, op_external_clock, t_old, t_new);
+ aspeed_timer_ctrl_op(t, op_overflow_interrupt, t_old, t_new);
+ aspeed_timer_ctrl_op(t, op_pulse_enable, t_old, t_new);
+ /* If we are enabling, do so last */
+ if (!(t_old & enable_mask) && (t_new & enable_mask)) {
+ aspeed_timer_ctrl_enable(t, true);
+ }
+ }
+ s->ctrl = reg;
+}
+
+static void aspeed_timer_set_ctrl2(AspeedTimerCtrlState *s, uint32_t value)
+{
+ trace_aspeed_timer_set_ctrl2(value);
+}
+
+static void aspeed_timer_write(void *opaque, hwaddr offset, uint64_t value,
+ unsigned size)
+{
+ const uint32_t tv = (uint32_t)(value & 0xFFFFFFFF);
+ const int reg = (offset & 0xf) / 4;
+ AspeedTimerCtrlState *s = opaque;
+
+ switch (offset) {
+ /* Control Registers */
+ case 0x30:
+ aspeed_timer_set_ctrl(s, tv);
+ break;
+ case 0x34:
+ aspeed_timer_set_ctrl2(s, tv);
+ break;
+ /* Timer Registers */
+ case 0x00 ... 0x2c:
+ aspeed_timer_set_value(s, (offset >> TIMER_NR_REGS), reg, tv);
+ break;
+ case 0x40 ... 0x8c:
+ aspeed_timer_set_value(s, (offset >> TIMER_NR_REGS) - 1, reg, tv);
+ break;
+ /* Illegal */
+ case 0x38:
+ case 0x3C:
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ break;
+ }
+}
+
+static const MemoryRegionOps aspeed_timer_ops = {
+ .read = aspeed_timer_read,
+ .write = aspeed_timer_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .valid.min_access_size = 4,
+ .valid.max_access_size = 4,
+ .valid.unaligned = false,
+};
+
+static void aspeed_init_one_timer(AspeedTimerCtrlState *s, uint8_t id)
+{
+ QEMUBH *bh;
+ AspeedTimer *t = &s->timers[id];
+
+ t->id = id;
+ bh = qemu_bh_new(aspeed_timer_expire, t);
+ assert(bh);
+ t->timer = ptimer_init(bh);
+ assert(t->timer);
+}
+
+static void aspeed_timer_realize(DeviceState *dev, Error **errp)
+{
+ int i;
+ SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+ AspeedTimerCtrlState *s = ASPEED_TIMER(dev);
+
+ for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
+ aspeed_init_one_timer(s, i);
+ sysbus_init_irq(sbd, &s->timers[i].irq);
+ }
+ memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_timer_ops, s,
+ TYPE_ASPEED_TIMER, 0x1000);
+ sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static void aspeed_timer_reset(DeviceState *dev)
+{
+ int i;
+ AspeedTimerCtrlState *s = ASPEED_TIMER(dev);
+
+ for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
+ AspeedTimer *t = &s->timers[i];
+ /* Explictly call helpers to avoid any conditional behaviour through
+ * aspeed_timer_set_ctrl().
+ */
+ aspeed_timer_ctrl_enable(t, false);
+ aspeed_timer_ctrl_external_clock(t, TIMER_CLOCK_USE_APB);
+ aspeed_timer_ctrl_overflow_interrupt(t, false);
+ aspeed_timer_ctrl_pulse_enable(t, false);
+ t->level = 0;
+ t->reload = 0;
+ t->match[0] = 0;
+ t->match[1] = 0;
+ }
+ s->ctrl = 0;
+ s->ctrl2 = 0;
+}
+
+static const VMStateDescription vmstate_aspeed_timer = {
+ .name = "aspeed.timer",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8(id, AspeedTimer),
+ VMSTATE_INT32(level, AspeedTimer),
+ VMSTATE_PTIMER(timer, AspeedTimer),
+ VMSTATE_UINT32(reload, AspeedTimer),
+ VMSTATE_UINT32_ARRAY(match, AspeedTimer, 2),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static const VMStateDescription vmstate_aspeed_timer_state = {
+ .name = "aspeed.timerctrl",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(ctrl, AspeedTimerCtrlState),
+ VMSTATE_UINT32(ctrl2, AspeedTimerCtrlState),
+ VMSTATE_STRUCT_ARRAY(timers, AspeedTimerCtrlState,
+ ASPEED_TIMER_NR_TIMERS, 1, vmstate_aspeed_timer,
+ AspeedTimer)
+ }
+};
+
+static void timer_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = aspeed_timer_realize;
+ dc->reset = aspeed_timer_reset;
+ dc->desc = "ASPEED Timer";
+ dc->vmsd = &vmstate_aspeed_timer_state;
+}
+
+static const TypeInfo aspeed_timer_info = {
+ .name = TYPE_ASPEED_TIMER,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(AspeedTimerCtrlState),
+ .class_init = timer_class_init,
+};
+
+static void aspeed_timer_register_types(void)
+{
+ type_register_static(&aspeed_timer_info);
+}
+
+type_init(aspeed_timer_register_types)
diff --git a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h
new file mode 100644
index 0000000..44dc2f8
--- /dev/null
+++ b/include/hw/timer/aspeed_timer.h
@@ -0,0 +1,59 @@
+/*
+ * ASPEED AST2400 Timer
+ *
+ * Andrew Jeffery <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(AspeedTimerCtrlState, (obj), TYPE_ASPEED_TIMER);
+#define TYPE_ASPEED_TIMER "aspeed.timer"
+#define ASPEED_TIMER_NR_TIMERS 8
+
+typedef struct AspeedTimer {
+ qemu_irq irq;
+
+ uint8_t id;
+
+ /**
+ * Track the line level as the ASPEED timers implement edge triggered
+ * interrupts, signalling with both the rising and falling edge.
+ */
+ int32_t level;
+ ptimer_state *timer;
+ uint32_t reload;
+ uint32_t match[2];
+} AspeedTimer;
+
+typedef struct AspeedTimerCtrlState {
+ /*< private >*/
+ SysBusDevice parent;
+
+ /*< public >*/
+ MemoryRegion iomem;
+
+ uint32_t ctrl;
+ uint32_t ctrl2;
+ AspeedTimer timers[ASPEED_TIMER_NR_TIMERS];
+} AspeedTimerCtrlState;
+
+#endif /* ASPEED_TIMER_H */
diff --git a/trace-events b/trace-events
index 6fba6cc..856425d 100644
--- a/trace-events
+++ b/trace-events
@@ -1886,3 +1886,12 @@ qio_channel_command_new_pid(void *ioc, int writefd, int readfd, int pid) "Comman
qio_channel_command_new_spawn(void *ioc, const char *binary, int flags) "Command new spawn ioc=%p binary=%s flags=%d"
qio_channel_command_abort(void *ioc, int pid) "Command abort ioc=%p pid=%d"
qio_channel_command_wait(void *ioc, int pid, int ret, int status) "Command abort ioc=%p pid=%d ret=%d status=%d"
+
+# hw/timer/aspeed_timer.c
+aspeed_timer_ctrl_enable(uint8_t i, bool enable) "Timer %" PRIu8 ": %d"
+aspeed_timer_ctrl_external_clock(uint8_t i, bool enable) "Timer %" PRIu8 ": %d"
+aspeed_timer_ctrl_overflow_interrupt(uint8_t i, bool enable) "Timer %" PRIu8 ": %d"
+aspeed_timer_ctrl_pulse_enable(uint8_t i, bool enable) "Timer %" PRIu8 ": %d"
+aspeed_timer_set_ctrl2(uint32_t value) "Value: 0x%" PRIx32
+aspeed_timer_set_value(int timer, int reg, uint32_t value) "Timer %d register %d: 0x%" PRIx32
+aspeed_timer_read(uint64_t offset, unsigned size, uint64_t value) "From 0x%" PRIx64 ": of size %u: 0x%" PRIx64
--
2.5.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/3] hw/timer: Add ASPEED timer device model
2016-03-05 4:29 ` [Qemu-devel] [PATCH v3 1/3] hw/timer: Add ASPEED timer device model Andrew Jeffery
@ 2016-03-11 8:56 ` Peter Maydell
2016-03-12 3:06 ` Andrew Jeffery
0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2016-03-11 8:56 UTC (permalink / raw)
To: Andrew Jeffery
Cc: Alexey Kardashevskiy, qemu-arm, Cédric Le Goater,
QEMU Developers
On 5 March 2016 at 11:29, Andrew Jeffery <andrew@aj.id.au> wrote:
> Implement basic AST2400 timer functionality: Up to 8 timers can
> independently be configured, enabled, reset and disabled. A couple of
> hardware features are not implemented, namely clock value matching and
> pulse generation, but the implementation is enough to boot the Linux
> kernel configured with aspeed_defconfig.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> +/**
> + * Avoid mutual references between AspeedTimerCtrlState and AspeedTimer
> + * structs, as it's a waste of memory and it makes implementing
> + * VMStateDescription a little clunky.
Not sure what you have in mind with the reference to VMStateDescription
here. The vmstate struct only has to list the fields which contain
actual volatile state -- things like backreference pointers to other
structs aren't volatile state so don't appear.
> The ptimer BH callback needs to know
> + * whether a specific AspeedTimer is enabled, but this information is held in
> + * AspeedTimerCtrlState. So, provide a helper to hoist ourselves from an
> + * arbitrary AspeedTimer to AspeedTimerCtrlState.
> + */
> +static inline struct AspeedTimerCtrlState *timer_to_ctrl(AspeedTimer *t)
> +{
> + AspeedTimer (*timers)[] = (void *)t - (t->id * sizeof(*t));
> + return container_of(timers, AspeedTimerCtrlState, timers);
> +}
> +static void aspeed_timer_expire(void *opaque)
> +{
> + AspeedTimer *t = opaque;
> +
> + /* Only support interrupts on match values of zero for the moment - this is
> + * sufficient to boot an aspeed_defconfig Linux kernel. Non-zero match
> + * values need some further consideration given the current ptimer API.
> + * Maybe run multiple ptimers?
> + */
See hw/timer/a9gtimer.c for an example of a timer with a comparator
that can fire when the timer hits an arbitrary comparator value
(it doesn't use ptimers but the principle is the same -- you set
the timer to fire at the next interesting event, and then in the
timer-fired handler you reset the timer to fire whenever the next
event after that is, if any.) In any case this is probably ok for now.
> + bool match = !(t->match[0] && t->match[1]);
> + bool interrupt = timer_overflow_interrupt(t) || match;
> + if (timer_enabled(t) && interrupt) {
> + t->level = !t->level;
> + qemu_set_irq(t->irq, t->level);
> + }
> +}
> +
Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/3] hw/timer: Add ASPEED timer device model
2016-03-11 8:56 ` Peter Maydell
@ 2016-03-12 3:06 ` Andrew Jeffery
0 siblings, 0 replies; 9+ messages in thread
From: Andrew Jeffery @ 2016-03-12 3:06 UTC (permalink / raw)
To: Peter Maydell
Cc: Alexey Kardashevskiy, qemu-arm, Cédric Le Goater,
QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 3038 bytes --]
On Fri, 2016-03-11 at 15:56 +0700, Peter Maydell wrote:
> On 5 March 2016 at 11:29, Andrew Jeffery <andrew@aj.id.au> wrote:
> > Implement basic AST2400 timer functionality: Up to 8 timers can
> > independently be configured, enabled, reset and disabled. A couple of
> > hardware features are not implemented, namely clock value matching and
> > pulse generation, but the implementation is enough to boot the Linux
> > kernel configured with aspeed_defconfig.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> > +/**
> > + * Avoid mutual references between AspeedTimerCtrlState and AspeedTimer
> > + * structs, as it's a waste of memory and it makes implementing
> > + * VMStateDescription a little clunky.
>
> Not sure what you have in mind with the reference to VMStateDescription
> here. The vmstate struct only has to list the fields which contain
> actual volatile state -- things like backreference pointers to other
> structs aren't volatile state so don't appear.
Good point, looks like I was over-thinking things. I'll remove the part
referencing VMStateDescription.
>
> > The ptimer BH callback needs to know
> > + * whether a specific AspeedTimer is enabled, but this information is held in
> > + * AspeedTimerCtrlState. So, provide a helper to hoist ourselves from an
> > + * arbitrary AspeedTimer to AspeedTimerCtrlState.
> > + */
> > +static inline struct AspeedTimerCtrlState *timer_to_ctrl(AspeedTimer *t)
> > +{
> > + AspeedTimer (*timers)[] = (void *)t - (t->id * sizeof(*t));
> > + return container_of(timers, AspeedTimerCtrlState, timers);
> > +}
>
> > +static void aspeed_timer_expire(void *opaque)
> > +{
> > + AspeedTimer *t = opaque;
> > +
> > + /* Only support interrupts on match values of zero for the moment - this is
> > + * sufficient to boot an aspeed_defconfig Linux kernel. Non-zero match
> > + * values need some further consideration given the current ptimer API.
> > + * Maybe run multiple ptimers?
> > + */
>
> See hw/timer/a9gtimer.c for an example of a timer with a comparator
> that can fire when the timer hits an arbitrary comparator value
> (it doesn't use ptimers but the principle is the same -- you set
> the timer to fire at the next interesting event, and then in the
> timer-fired handler you reset the timer to fire whenever the next
> event after that is, if any.) In any case this is probably ok for now.
Thanks for the pointer. I'll leave that change to a future patch given
it looks like this is converging on being acceptable, though I'll
expand the comment to cover a9gtimer.
>
> > + bool match = !(t->match[0] && t->match[1]);
> > + bool interrupt = timer_overflow_interrupt(t) || match;
> > + if (timer_enabled(t) && interrupt) {
> > + t->level = !t->level;
> > + qemu_set_irq(t->irq, t->level);
> > + }
> > +}
> > +
>
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
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] 9+ messages in thread
* [Qemu-devel] [PATCH v3 2/3] hw/intc: Add (new) ASPEED VIC device model
2016-03-05 4:29 [Qemu-devel] [PATCH v3 0/3] Add ASPEED AST2400 machine model Andrew Jeffery
2016-03-05 4:29 ` [Qemu-devel] [PATCH v3 1/3] hw/timer: Add ASPEED timer device model Andrew Jeffery
@ 2016-03-05 4:29 ` Andrew Jeffery
2016-03-11 9:03 ` Peter Maydell
2016-03-05 4:29 ` [Qemu-devel] [PATCH v3 3/3] hw/arm: Add ASPEED AST2400 machine model Andrew Jeffery
2 siblings, 1 reply; 9+ messages in thread
From: Andrew Jeffery @ 2016-03-05 4:29 UTC (permalink / raw)
To: Peter Maydell
Cc: Alexey Kardashevskiy, qemu-arm, Cédric Le Goater, qemu-devel
Implement a basic ASPEED VIC device model, enough to boot a Linux kernel
configured with aspeed_defconfig. The model implements the 'new'
(revised) register set and while the hardware exposes both the new and
legacy register sets, accesses to the legacy register set will not
be serviced (though the access will be logged).
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
Since v2:
* Implement all supported interrupt types and configurations
* Implement a VMStateDescription
* Log accesses to legacy IO space
* Add documentation on some implementation and hardware details
* Switch to extract64/deposit64 where possible
* Drop int_ prefix from some struct member names
* Fix various issues with trace-event declarations
* Include qemu/osdep.h
hw/intc/Makefile.objs | 1 +
hw/intc/aspeed_vic.c | 335 +++++++++++++++++++++++++++++++++++++++++++
include/hw/intc/aspeed_vic.h | 48 +++++++
trace-events | 7 +
4 files changed, 391 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..8cfaa02
--- /dev/null
+++ b/hw/intc/aspeed_vic.c
@@ -0,0 +1,335 @@
+/*
+ * 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.
+ */
+
+/* The hardware exposes two register sets, a legacy set and a 'new' set. The
+ * model implements the 'new' register set, and logs warnings on accesses to
+ * the legacy IO space.
+ *
+ * The hardware uses 32bit registers to manage 51 IRQs, with low and high
+ * registers for each conceptual register. The device model's implementation
+ * uses 64bit data types to store both low and high register values (in the one
+ * member), 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 32 bits of the 64bit member.
+ *
+ * Additionally, the "Interrupt Enable", "Edge Status" and "Software Interrupt"
+ * fields have separate "enable"/"status" and "clear" registers, where set bits
+ * are written to one or the other to change state (avoiding a
+ * read-modify-write sequence).
+ */
+
+#include "qemu/osdep.h"
+#include <inttypes.h>
+#include "hw/intc/aspeed_vic.h"
+#include "qemu/bitops.h"
+#include "trace.h"
+
+#define AVIC_NEW_BASE_OFFSET 0x80
+
+#define AVIC_L_MASK 0xFFFFFFFFU
+#define AVIC_H_MASK 0x0007FFFFU
+#define AVIC_EVENT_W_MASK (0x78000ULL << 32)
+
+static void aspeed_vic_update(AspeedVICState *s)
+{
+ uint64_t new = (s->raw & s->enable);
+ uint64_t flags;
+
+ flags = new & s->select;
+ trace_aspeed_vic_update_fiq(!!flags);
+ qemu_set_irq(s->fiq, !!flags);
+
+ flags = new & ~s->select;
+ trace_aspeed_vic_update_irq(!!flags);
+ qemu_set_irq(s->irq, !!flags);
+}
+
+static void aspeed_vic_set_irq(void *opaque, int irq, int level)
+{
+ uint64_t irq_mask;
+ bool raise;
+ AspeedVICState *s = (AspeedVICState *)opaque;
+
+ if (irq > ASPEED_VIC_NR_IRQS) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: Out-of-range interrupt number: %d\n",
+ __func__, irq);
+ return;
+ }
+
+ trace_aspeed_vic_set_irq(irq, level);
+
+ irq_mask = BIT(irq);
+ if (s->sense & irq_mask) {
+ /* level-triggered */
+ if (s->event & irq_mask) {
+ /* high-sensitive */
+ raise = level;
+ } else {
+ /* low-sensitive */
+ raise = !level;
+ }
+ s->raw = deposit64(s->raw, irq, 1, raise);
+ } else {
+ uint64_t old_level = s->level & irq_mask;
+
+ /* edge-triggered */
+ if (s->dual_edge & irq_mask) {
+ raise = (!!old_level) != (!!level);
+ } else {
+ if (s->event & irq_mask) {
+ /* rising-sensitive */
+ raise = !old_level && level;
+ } else {
+ /* falling-sensitive */
+ raise = old_level && !level;
+ }
+ }
+ if (raise) {
+ s->raw = deposit64(s->raw, irq, 1, raise);
+ }
+ }
+ s->level = deposit64(s->level, irq, 1, 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);
+ hwaddr n_offset = (offset & ~0x4);
+ AspeedVICState *s = (AspeedVICState *)opaque;
+
+ if (offset < AVIC_NEW_BASE_OFFSET) {
+ qemu_log_mask(LOG_UNIMP, "%s: Ignoring read from legacy registers "
+ "at 0x%" HWADDR_PRIx "[%u]\n", __func__, offset, size);
+ return 0;
+ }
+
+ n_offset -= AVIC_NEW_BASE_OFFSET;
+
+ switch (n_offset) {
+ case 0x0: /* IRQ Status */
+ val = s->raw & ~s->select & s->enable;
+ break;
+ case 0x08: /* FIQ Status */
+ val = s->raw & s->select & s->enable;
+ break;
+ case 0x10: /* Raw Interrupt Status */
+ val = s->raw;
+ break;
+ case 0x18: /* Interrupt Selection */
+ val = s->select;
+ break;
+ case 0x20: /* Interrupt Enable */
+ val = s->enable;
+ break;
+ case 0x30: /* Software Interrupt */
+ val = s->trigger;
+ break;
+ case 0x40: /* Interrupt Sensitivity */
+ val = s->sense;
+ break;
+ case 0x48: /* Interrupt Both Edge Trigger Control */
+ val = s->dual_edge;
+ break;
+ case 0x50: /* Interrupt Event */
+ val = s->event;
+ break;
+ case 0x60: /* Edge Triggered Interrupt Status */
+ val = s->raw & ~s->sense;
+ break;
+ /* Illegal */
+ case 0x28: /* Interrupt Enable Clear */
+ case 0x38: /* Software Interrupt Clear */
+ case 0x58: /* Edge Triggered Interrupt Clear */
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Read of write-only register with offset 0x%"
+ HWADDR_PRIx "\n", __func__, offset);
+ val = 0;
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Bad register at offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ val = 0;
+ break;
+ }
+ if (high) {
+ val = extract64(val, 32, 19);
+ }
+ 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);
+ hwaddr n_offset = (offset & ~0x4);
+ AspeedVICState *s = (AspeedVICState *)opaque;
+
+ if (offset < AVIC_NEW_BASE_OFFSET) {
+ qemu_log_mask(LOG_UNIMP,
+ "%s: Ignoring write to legacy registers at 0x%"
+ HWADDR_PRIx "[%u] <- 0x%" PRIx64 "\n", __func__, offset,
+ size, data);
+ return;
+ }
+
+ n_offset -= AVIC_NEW_BASE_OFFSET;
+ trace_aspeed_vic_write(offset, size, data);
+
+ /* Given we have members using separate enable/clear registers, deposit64()
+ * isn't quite the tool for the job. Instead, relocate the incoming bits to
+ * the required bit offset based on the provided access address
+ */
+ if (high) {
+ data &= AVIC_H_MASK;
+ data <<= 32;
+ } else {
+ data &= AVIC_L_MASK;
+ }
+
+ switch (n_offset) {
+ case 0x18: /* Interrupt Selection */
+ /* Register has deposit64() semantics - overwrite requested 32 bits */
+ if (high) {
+ s->select &= AVIC_L_MASK;
+ } else {
+ s->select &= ((uint64_t) AVIC_H_MASK) << 32;
+ }
+ s->select |= data;
+ break;
+ case 0x20: /* Interrupt Enable */
+ s->enable |= data;
+ break;
+ case 0x28: /* Interrupt Enable Clear */
+ s->enable &= ~data;
+ break;
+ case 0x30: /* Software Interrupt */
+ qemu_log_mask(LOG_UNIMP, "%s: Software interrupts unavailable. "
+ "IRQs requested: 0x%016" PRIx64 "\n", __func__, data);
+ break;
+ case 0x38: /* Software Interrupt Clear */
+ qemu_log_mask(LOG_UNIMP, "%s: Software interrupts unavailable. "
+ "IRQs to be cleared: 0x%016" PRIx64 "\n", __func__, data);
+ break;
+ case 0x50: /* Interrupt Event */
+ /* Register has deposit64() semantics - overwrite the top four valid
+ * IRQ bits, as only the top four IRQs (GPIOs) can change their event
+ * type */
+ g_assert(high);
+ s->event &= ~AVIC_EVENT_W_MASK;
+ s->event |= (data & AVIC_EVENT_W_MASK);
+ break;
+ case 0x58: /* Edge Triggered Interrupt Clear */
+ s->raw &= ~(data & ~s->sense);
+ 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,
+ "%s: Write of read-only register with offset 0x%"
+ HWADDR_PRIx "\n", __func__, offset);
+ break;
+
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Bad register at offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ break;
+ }
+ aspeed_vic_update(s);
+}
+
+static const MemoryRegionOps aspeed_vic_ops = {
+ .read = aspeed_vic_read,
+ .write = aspeed_vic_write,
+ .endianness = DEVICE_LITTLE_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->level = 0;
+ s->raw = 0;
+ s->select = 0;
+ s->enable = 0;
+ s->trigger = 0;
+ s->sense = 0x1F07FFF8FFFFULL;
+ s->dual_edge = 0xF800070000ULL;
+ s->event = 0x5F07FFF8FFFFULL;
+}
+
+#define AVIC_IO_REGION_SIZE 0x20000
+
+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, AVIC_IO_REGION_SIZE);
+
+ 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 const VMStateDescription vmstate_aspeed_vic = {
+ .name = "aspeed.new-vic",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT64(level, AspeedVICState),
+ VMSTATE_UINT64(raw, AspeedVICState),
+ VMSTATE_UINT64(select, AspeedVICState),
+ VMSTATE_UINT64(enable, AspeedVICState),
+ VMSTATE_UINT64(trigger, AspeedVICState),
+ VMSTATE_UINT64(sense, AspeedVICState),
+ VMSTATE_UINT64(dual_edge, AspeedVICState),
+ VMSTATE_UINT64(event, AspeedVICState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+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)";
+ dc->vmsd = &vmstate_aspeed_vic;
+}
+
+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..107ff17
--- /dev/null
+++ b/include/hw/intc/aspeed_vic.h
@@ -0,0 +1,48 @@
+/*
+ * ASPEED Interrupt Controller (New)
+ *
+ * Andrew Jeffery <andrew@aj.id.au>
+ *
+ * Copyright 2016 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;
+ qemu_irq irq;
+ qemu_irq fiq;
+
+ uint64_t level;
+ uint64_t raw;
+ uint64_t select;
+ uint64_t enable;
+ uint64_t trigger;
+
+ /* 0=edge, 1=level */
+ uint64_t sense;
+
+ /* 0=single-edge, 1=dual-edge */
+ uint64_t dual_edge;
+
+ /* 0=low-sensitive/falling-edge, 1=high-sensitive/rising-edge */
+ uint64_t event;
+} AspeedVICState;
+
+#endif /* ASPEED_VIC_H */
diff --git a/trace-events b/trace-events
index 856425d..eecca15 100644
--- a/trace-events
+++ b/trace-events
@@ -1895,3 +1895,10 @@ aspeed_timer_ctrl_pulse_enable(uint8_t i, bool enable) "Timer %" PRIu8 ": %d"
aspeed_timer_set_ctrl2(uint32_t value) "Value: 0x%" PRIx32
aspeed_timer_set_value(int timer, int reg, uint32_t value) "Timer %d register %d: 0x%" PRIx32
aspeed_timer_read(uint64_t offset, unsigned size, uint64_t value) "From 0x%" PRIx64 ": of size %u: 0x%" PRIx64
+
+# 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_irq(int flags) "Raising IRQ %d"
+aspeed_vic_read(uint64_t offset, unsigned size, uint32_t value) "From 0x%" PRIx64 " of size %u: 0x%" PRIx32
+aspeed_vic_write(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64 " of size %u: 0x%" PRIx32
--
2.5.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/3] hw/intc: Add (new) ASPEED VIC device model
2016-03-05 4:29 ` [Qemu-devel] [PATCH v3 2/3] hw/intc: Add (new) ASPEED VIC " Andrew Jeffery
@ 2016-03-11 9:03 ` Peter Maydell
2016-03-12 3:24 ` Andrew Jeffery
0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2016-03-11 9:03 UTC (permalink / raw)
To: Andrew Jeffery
Cc: Alexey Kardashevskiy, qemu-arm, Cédric Le Goater,
QEMU Developers
On 5 March 2016 at 11:29, Andrew Jeffery <andrew@aj.id.au> wrote:
> Implement a basic ASPEED VIC device model, enough to boot a Linux kernel
> configured with aspeed_defconfig. The model implements the 'new'
> (revised) register set and while the hardware exposes both the new and
> legacy register sets, accesses to the legacy register set will not
> be serviced (though the access will be logged).
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> +static void aspeed_vic_write(void *opaque, hwaddr offset, uint64_t data,
> + unsigned size)
> +{
> + const bool high = !!(offset & 0x4);
> + hwaddr n_offset = (offset & ~0x4);
> + AspeedVICState *s = (AspeedVICState *)opaque;
> +
> + if (offset < AVIC_NEW_BASE_OFFSET) {
> + qemu_log_mask(LOG_UNIMP,
> + "%s: Ignoring write to legacy registers at 0x%"
> + HWADDR_PRIx "[%u] <- 0x%" PRIx64 "\n", __func__, offset,
> + size, data);
> + return;
> + }
> +
> + n_offset -= AVIC_NEW_BASE_OFFSET;
> + trace_aspeed_vic_write(offset, size, data);
> +
> + /* Given we have members using separate enable/clear registers, deposit64()
> + * isn't quite the tool for the job. Instead, relocate the incoming bits to
> + * the required bit offset based on the provided access address
> + */
> + if (high) {
> + data &= AVIC_H_MASK;
> + data <<= 32;
> + } else {
> + data &= AVIC_L_MASK;
> + }
> +
> + switch (n_offset) {
> + case 0x18: /* Interrupt Selection */
> + /* Register has deposit64() semantics - overwrite requested 32 bits */
> + if (high) {
> + s->select &= AVIC_L_MASK;
> + } else {
> + s->select &= ((uint64_t) AVIC_H_MASK) << 32;
> + }
> + s->select |= data;
> + break;
> + case 0x20: /* Interrupt Enable */
> + s->enable |= data;
> + break;
> + case 0x28: /* Interrupt Enable Clear */
> + s->enable &= ~data;
> + break;
> + case 0x30: /* Software Interrupt */
> + qemu_log_mask(LOG_UNIMP, "%s: Software interrupts unavailable. "
> + "IRQs requested: 0x%016" PRIx64 "\n", __func__, data);
> + break;
> + case 0x38: /* Software Interrupt Clear */
> + qemu_log_mask(LOG_UNIMP, "%s: Software interrupts unavailable. "
> + "IRQs to be cleared: 0x%016" PRIx64 "\n", __func__, data);
> + break;
> + case 0x50: /* Interrupt Event */
> + /* Register has deposit64() semantics - overwrite the top four valid
> + * IRQ bits, as only the top four IRQs (GPIOs) can change their event
> + * type */
> + g_assert(high);
Don't assert on conditions that can be triggered by a guest.
> + s->event &= ~AVIC_EVENT_W_MASK;
> + s->event |= (data & AVIC_EVENT_W_MASK);
> + break;
> + case 0x58: /* Edge Triggered Interrupt Clear */
> + s->raw &= ~(data & ~s->sense);
> + 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,
> + "%s: Write of read-only register with offset 0x%"
> + HWADDR_PRIx "\n", __func__, offset);
> + break;
> +
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Bad register at offset 0x%" HWADDR_PRIx "\n",
> + __func__, offset);
> + break;
> + }
> + aspeed_vic_update(s);
> +}
Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/3] hw/intc: Add (new) ASPEED VIC device model
2016-03-11 9:03 ` Peter Maydell
@ 2016-03-12 3:24 ` Andrew Jeffery
0 siblings, 0 replies; 9+ messages in thread
From: Andrew Jeffery @ 2016-03-12 3:24 UTC (permalink / raw)
To: Peter Maydell
Cc: Alexey Kardashevskiy, qemu-arm, Cédric Le Goater,
QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 4271 bytes --]
On Fri, 2016-03-11 at 16:03 +0700, Peter Maydell wrote:
> On 5 March 2016 at 11:29, Andrew Jeffery <andrew@aj.id.au> wrote:
> > Implement a basic ASPEED VIC device model, enough to boot a Linux kernel
> > configured with aspeed_defconfig. The model implements the 'new'
> > (revised) register set and while the hardware exposes both the new and
> > legacy register sets, accesses to the legacy register set will not
> > be serviced (though the access will be logged).
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>
> > +static void aspeed_vic_write(void *opaque, hwaddr offset, uint64_t data,
> > + unsigned size)
> > +{
> > + const bool high = !!(offset & 0x4);
> > + hwaddr n_offset = (offset & ~0x4);
> > + AspeedVICState *s = (AspeedVICState *)opaque;
> > +
> > + if (offset < AVIC_NEW_BASE_OFFSET) {
> > + qemu_log_mask(LOG_UNIMP,
> > + "%s: Ignoring write to legacy registers at 0x%"
> > + HWADDR_PRIx "[%u] <- 0x%" PRIx64 "\n", __func__, offset,
> > + size, data);
> > + return;
> > + }
> > +
> > + n_offset -= AVIC_NEW_BASE_OFFSET;
> > + trace_aspeed_vic_write(offset, size, data);
> > +
> > + /* Given we have members using separate enable/clear registers, deposit64()
> > + * isn't quite the tool for the job. Instead, relocate the incoming bits to
> > + * the required bit offset based on the provided access address
> > + */
> > + if (high) {
> > + data &= AVIC_H_MASK;
> > + data <<= 32;
> > + } else {
> > + data &= AVIC_L_MASK;
> > + }
> > +
> > + switch (n_offset) {
> > + case 0x18: /* Interrupt Selection */
> > + /* Register has deposit64() semantics - overwrite requested 32 bits */
> > + if (high) {
> > + s->select &= AVIC_L_MASK;
> > + } else {
> > + s->select &= ((uint64_t) AVIC_H_MASK) << 32;
> > + }
> > + s->select |= data;
> > + break;
> > + case 0x20: /* Interrupt Enable */
> > + s->enable |= data;
> > + break;
> > + case 0x28: /* Interrupt Enable Clear */
> > + s->enable &= ~data;
> > + break;
> > + case 0x30: /* Software Interrupt */
> > + qemu_log_mask(LOG_UNIMP, "%s: Software interrupts unavailable. "
> > + "IRQs requested: 0x%016" PRIx64 "\n", __func__, data);
> > + break;
> > + case 0x38: /* Software Interrupt Clear */
> > + qemu_log_mask(LOG_UNIMP, "%s: Software interrupts unavailable. "
> > + "IRQs to be cleared: 0x%016" PRIx64 "\n", __func__, data);
> > + break;
> > + case 0x50: /* Interrupt Event */
> > + /* Register has deposit64() semantics - overwrite the top four valid
> > + * IRQ bits, as only the top four IRQs (GPIOs) can change their event
> > + * type */
> > + g_assert(high);
>
> Don't assert on conditions that can be triggered by a guest.
Good point, I'll change it to qemu_log_mask(LOG_GUEST_ERROR, ...)
>
> > + s->event &= ~AVIC_EVENT_W_MASK;
> > + s->event |= (data & AVIC_EVENT_W_MASK);
> > + break;
> > + case 0x58: /* Edge Triggered Interrupt Clear */
> > + s->raw &= ~(data & ~s->sense);
> > + 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,
> > + "%s: Write of read-only register with offset 0x%"
> > + HWADDR_PRIx "\n", __func__, offset);
> > + break;
> > +
> > + default:
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Bad register at offset 0x%" HWADDR_PRIx "\n",
> > + __func__, offset);
> > + break;
> > + }
> > + aspeed_vic_update(s);
> > +}
>
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
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] 9+ messages in thread
* [Qemu-devel] [PATCH v3 3/3] hw/arm: Add ASPEED AST2400 machine model
2016-03-05 4:29 [Qemu-devel] [PATCH v3 0/3] Add ASPEED AST2400 machine model Andrew Jeffery
2016-03-05 4:29 ` [Qemu-devel] [PATCH v3 1/3] hw/timer: Add ASPEED timer device model Andrew Jeffery
2016-03-05 4:29 ` [Qemu-devel] [PATCH v3 2/3] hw/intc: Add (new) ASPEED VIC " Andrew Jeffery
@ 2016-03-05 4:29 ` Andrew Jeffery
2016-03-11 9:09 ` Peter Maydell
2 siblings, 1 reply; 9+ messages in thread
From: Andrew Jeffery @ 2016-03-05 4:29 UTC (permalink / raw)
To: Peter Maydell
Cc: Alexey Kardashevskiy, qemu-arm, Cédric Le Goater, qemu-devel
Adds an AST2400 ARM machine[1], based around an AST2400 SOC containing an
ARM926 processor, ASPEED VIC and timer devices, and a 8250 UART. The new
machine type is functional enough to boot an aspeed_defconfig Linux
kernel to userspace.
[1] http://www.aspeedtech.com/products.php?fPath=20&rId=376
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
Since v2:
* Implement a SOC model to move code out from the machine definition
* Rework the machine to better use QOM
* Include qemu/osdep.h
* Revert back to qemu_log_mask(LOG_UNIMP, ...) in IO handlers
Since v1:
hw/arm/Makefile.objs | 1 +
hw/arm/ast2400.c | 208 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 209 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..74aca49
--- /dev/null
+++ b/hw/arm/ast2400.c
@@ -0,0 +1,208 @@
+/*
+ * ASPEED AST2400
+ *
+ * Andrew Jeffery <andrew@aj.id.au>
+ * Jeremy Kerr <jk@ozlabs.org>
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "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 "target-arm/cpu.h"
+#include "trace.h"
+
+#define AST2400_UART_5_BASE 0x00184000
+#define AST2400_IOMEM_SIZE 0x00200000
+#define AST2400_IOMEM_BASE 0x1E600000
+#define AST2400_VIC_BASE 0x1E6C0000
+#define AST2400_TIMER_BASE 0x1E782000
+#define AST2400_SDRAM_BASE 0x40000000
+
+static const int uart_irqs[] = { 9, 32, 33, 34, 10 };
+static const int timer_irqs[] = { 16, 17, 18, 35, 36, 37, 38, 39, };
+
+static struct arm_boot_info ast2400_binfo = {
+ .loader_start = AST2400_SDRAM_BASE,
+ .board_id = 0,
+ .nb_cpus = 1,
+};
+
+/*
+ * IO handlers: simply catch any reads/writes to IO addresses that aren't
+ * handled by a device mapping.
+ */
+
+static uint64_t ast2400_soc_io_read(void *p, hwaddr offset, unsigned size)
+{
+ qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u]\n",
+ __func__, offset, size);
+ return 0;
+}
+
+static void ast2400_soc_io_write(void *opaque, hwaddr offset, uint64_t value,
+ unsigned size)
+{
+ qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
+ __func__, offset, value, size);
+}
+
+static const MemoryRegionOps ast2400_soc_io_ops = {
+ .read = ast2400_soc_io_read,
+ .write = ast2400_soc_io_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+typedef struct AST2400SOCState {
+ /*< private >*/
+ DeviceState parent;
+
+ /*< public >*/
+ ARMCPU *cpu;
+ MemoryRegion iomem;
+ AspeedVICState vic;
+ AspeedTimerCtrlState timerctrl;
+} AST2400SOCState;
+
+#define TYPE_AST2400_SOC "ast2400-soc"
+#define AST2400_SOC(obj) OBJECT_CHECK(AST2400SOCState, (obj), TYPE_AST2400_SOC)
+
+static void ast2400_soc_init(Object *obj)
+{
+ AST2400SOCState *s = AST2400_SOC(obj);
+
+ s->cpu = cpu_arm_init("arm926");
+
+ object_initialize(&s->vic, sizeof(s->vic), TYPE_ASPEED_VIC);
+ object_property_add_child(obj, "vic", OBJECT(&s->vic), NULL);
+ qdev_set_parent_bus(DEVICE(&s->vic), sysbus_get_default());
+
+ object_initialize(&s->timerctrl, sizeof(s->timerctrl), TYPE_ASPEED_TIMER);
+ object_property_add_child(obj, "timerctrl", OBJECT(&s->timerctrl), NULL);
+ qdev_set_parent_bus(DEVICE(&s->timerctrl), sysbus_get_default());
+}
+
+static void ast2400_soc_realize(DeviceState *dev, Error **errp)
+{
+ int i;
+ AST2400SOCState *s = AST2400_SOC(dev);
+ Error *err = NULL;
+
+ /* IO space */
+ memory_region_init_io(&s->iomem, NULL, &ast2400_soc_io_ops, NULL,
+ "ast2400.io", AST2400_IOMEM_SIZE);
+ memory_region_add_subregion(get_system_memory(), AST2400_IOMEM_BASE,
+ &s->iomem);
+
+ /* VIC */
+ object_property_set_bool(OBJECT(&s->vic), true, "realized", &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
+ sysbus_mmio_map(SYS_BUS_DEVICE(&s->vic), 0, AST2400_VIC_BASE);
+ sysbus_connect_irq(SYS_BUS_DEVICE(&s->vic), 0,
+ qdev_get_gpio_in(DEVICE(s->cpu), ARM_CPU_IRQ));
+ sysbus_connect_irq(SYS_BUS_DEVICE(&s->vic), 1,
+ qdev_get_gpio_in(DEVICE(s->cpu), ARM_CPU_FIQ));
+
+ /* Timer */
+ object_property_set_bool(OBJECT(&s->timerctrl), true, "realized", &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
+ sysbus_mmio_map(SYS_BUS_DEVICE(&s->timerctrl), 0, AST2400_TIMER_BASE);
+ for (i = 0; i < ARRAY_SIZE(timer_irqs); i++) {
+ qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->vic), timer_irqs[i]);
+ sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
+ }
+
+ /* UART - attach an 8250 to the IO space as our UART5 */
+ if (serial_hds[0]) {
+ qemu_irq uart5 = qdev_get_gpio_in(DEVICE(&s->vic), uart_irqs[4]);
+ serial_mm_init(&s->iomem, AST2400_UART_5_BASE, 2,
+ uart5, 38400, serial_hds[0], DEVICE_LITTLE_ENDIAN);
+ }
+}
+
+static void ast2400_soc_class_init(ObjectClass *oc, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(oc);
+
+ dc->realize = ast2400_soc_realize;
+
+ /*
+ * Reason: creates an ARM CPU, thus use after free(), see
+ * arm_cpu_class_init()
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
+}
+
+static const TypeInfo ast2400_soc_type_info = {
+ .name = TYPE_AST2400_SOC,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(AST2400SOCState),
+ .instance_init = ast2400_soc_init,
+ .class_init = ast2400_soc_class_init,
+};
+
+static void ast2400_soc_register_types(void)
+{
+ type_register_static(&ast2400_soc_type_info);
+}
+
+type_init(ast2400_soc_register_types)
+
+typedef struct AST2400State {
+ AST2400SOCState soc;
+ MemoryRegion ram;
+} AST2400State;
+
+static void ast2400_init(MachineState *machine)
+{
+ AST2400State *ast2400;
+
+ ast2400 = g_new0(AST2400State, 1);
+ object_initialize(&ast2400->soc, (sizeof(ast2400->soc)), TYPE_AST2400_SOC);
+ object_property_add_child(OBJECT(machine), "soc", OBJECT(&ast2400->soc),
+ &error_abort);
+
+ memory_region_allocate_system_memory(&ast2400->ram, NULL, "ram",
+ ram_size);
+ memory_region_add_subregion(get_system_memory(), AST2400_SDRAM_BASE,
+ &ast2400->ram);
+ object_property_add_const_link(OBJECT(&ast2400->soc), "ram",
+ OBJECT(&ast2400->ram), &error_abort);
+ object_property_set_bool(OBJECT(&ast2400->soc), true, "realized",
+ &error_abort);
+
+ 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(ARM_CPU(first_cpu), &ast2400_binfo);
+}
+
+static void ast2400_machine_init(MachineClass *mc)
+{
+ mc->desc = "ASPEED AST2400 BMC (ARM926EJ-S)";
+ mc->init = ast2400_init;
+ mc->max_cpus = 1;
+ mc->no_sdcard = 1;
+ mc->no_floppy = 1;
+ mc->no_cdrom = 1;
+ mc->no_sdcard = 1;
+ mc->no_parallel = 1;
+}
+
+DEFINE_MACHINE("ast2400", ast2400_machine_init);
--
2.5.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH v3 3/3] hw/arm: Add ASPEED AST2400 machine model
2016-03-05 4:29 ` [Qemu-devel] [PATCH v3 3/3] hw/arm: Add ASPEED AST2400 machine model Andrew Jeffery
@ 2016-03-11 9:09 ` Peter Maydell
0 siblings, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2016-03-11 9:09 UTC (permalink / raw)
To: Andrew Jeffery
Cc: Alexey Kardashevskiy, qemu-arm, Cédric Le Goater,
QEMU Developers
On 5 March 2016 at 11:29, Andrew Jeffery <andrew@aj.id.au> wrote:
> Adds an AST2400 ARM machine[1], based around an AST2400 SOC containing an
> ARM926 processor, ASPEED VIC and timer devices, and a 8250 UART. The new
> machine type is functional enough to boot an aspeed_defconfig Linux
> kernel to userspace.
>
> [1] http://www.aspeedtech.com/products.php?fPath=20&rId=376
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> Since v2:
> * Implement a SOC model to move code out from the machine definition
> * Rework the machine to better use QOM
> * Include qemu/osdep.h
> * Revert back to qemu_log_mask(LOG_UNIMP, ...) in IO handlers
>
> Since v1:
>
> hw/arm/Makefile.objs | 1 +
> hw/arm/ast2400.c | 208 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 209 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..74aca49
> --- /dev/null
> +++ b/hw/arm/ast2400.c
> @@ -0,0 +1,208 @@
> +/*
> + * ASPEED AST2400
> + *
> + * Andrew Jeffery <andrew@aj.id.au>
> + * Jeremy Kerr <jk@ozlabs.org>
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * This code is licensed under the GPL version 2 or later. See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "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 "target-arm/cpu.h"
> +#include "trace.h"
> +
> +#define AST2400_UART_5_BASE 0x00184000
> +#define AST2400_IOMEM_SIZE 0x00200000
> +#define AST2400_IOMEM_BASE 0x1E600000
> +#define AST2400_VIC_BASE 0x1E6C0000
> +#define AST2400_TIMER_BASE 0x1E782000
> +#define AST2400_SDRAM_BASE 0x40000000
> +
> +static void ast2400_soc_realize(DeviceState *dev, Error **errp)
> +{
> + int i;
> + AST2400SOCState *s = AST2400_SOC(dev);
> + Error *err = NULL;
> +
> + /* IO space */
> + memory_region_init_io(&s->iomem, NULL, &ast2400_soc_io_ops, NULL,
> + "ast2400.io", AST2400_IOMEM_SIZE);
> + memory_region_add_subregion(get_system_memory(), AST2400_IOMEM_BASE,
> + &s->iomem);
You need to add this memory region with a priority set so it is
lower priority than the VIC etc memory regions which it overlaps
(use memory_region_add_subregion_overlap()).
> +static void ast2400_soc_register_types(void)
> +{
> + type_register_static(&ast2400_soc_type_info);
> +}
> +
> +type_init(ast2400_soc_register_types)
Your SoC device should be in its own source file, not sharing
one with the board file.
Otherwise I think this looks OK.
thanks
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread