* [Qemu-devel] [PATCH v4 1/4] hw/timer: Add ASPEED timer device model
2016-03-14 4:13 [Qemu-devel] [PATCH v4 0/4] Add ASPEED AST2400 SoC and OpenPower BMC machine Andrew Jeffery
@ 2016-03-14 4:13 ` Andrew Jeffery
2016-03-15 13:14 ` Cédric Le Goater
2016-03-15 18:14 ` Dmitry Osipenko
2016-03-14 4:13 ` [Qemu-devel] [PATCH v4 2/4] hw/intc: Add (new) ASPEED VIC " Andrew Jeffery
` (3 subsequent siblings)
4 siblings, 2 replies; 13+ messages in thread
From: Andrew Jeffery @ 2016-03-14 4:13 UTC (permalink / raw)
To: Peter Maydell
Cc: Alexey Kardashevskiy, qemu-devel, qemu-arm, Cédric Le Goater,
Jeremy Kerr
Implement basic ASPEED timer functionality for the AST2400 SoC[1]: Up to
8 timers can independently be configured, enabled, reset and disabled.
Some hardware features are not implemented, namely clock value matching
and pulse generation, but the implementation is enough to boot the Linux
kernel configured with aspeed_defconfig.
[1] http://www.aspeedtech.com/products.php?fPath=20&rId=376
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
Since v3:
* Drop unnecessary mention of VMStateDescription in timer_to_ctrl description
* Mention hw/timer/a9gtimer.c with respect to clock value matching
* Add missing VMSTATE_END_OF_LIST() to vmstate_aspeed_timer_state
Since v2:
* Improve handling of timer configuration with respect to enabled state
* Remove redundant enabled member from AspeedTimer
* Implement VMStateDescriptions
* Fix interrupt behaviour (edge triggered, both edges)
* Fix various issues with trace-event declarations
* Include qemu/osdep.h
Since v1:
* Refactor initialisation of and respect requested clock rates (APB/External)
* Simplify some index calculations
* Use tracing infrastructure instead of internal DPRINTF
* Enforce access size constraints and alignment in MemoryRegionOps
default-configs/arm-softmmu.mak | 1 +
hw/timer/Makefile.objs | 1 +
hw/timer/aspeed_timer.c | 451 ++++++++++++++++++++++++++++++++++++++++
include/hw/timer/aspeed_timer.h | 59 ++++++
trace-events | 9 +
5 files changed, 521 insertions(+)
create mode 100644 hw/timer/aspeed_timer.c
create mode 100644 include/hw/timer/aspeed_timer.h
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index a9f82a1..2bcd236 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -110,3 +110,4 @@ CONFIG_IOH3420=y
CONFIG_I82801B11=y
CONFIG_ACPI=y
CONFIG_SMBIOS=y
+CONFIG_ASPEED_SOC=y
diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index 5cfea6e..003c14f 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -32,3 +32,4 @@ obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o
common-obj-$(CONFIG_STM32F2XX_TIMER) += stm32f2xx_timer.o
+common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o
diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
new file mode 100644
index 0000000..0e82178
--- /dev/null
+++ b/hw/timer/aspeed_timer.c
@@ -0,0 +1,452 @@
+/*
+ * ASPEED AST2400 Timer
+ *
+ * Andrew Jeffery <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. 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.
+ *
+ * TODO: matching on arbitrary values (see e.g. hw/timer/a9gtimer.c)
+ */
+ bool match = !(t->match[0] && t->match[1]);
+ bool interrupt = timer_overflow_interrupt(t) || match;
+ if (timer_enabled(t) && interrupt) {
+ t->level = !t->level;
+ qemu_set_irq(t->irq, t->level);
+ }
+}
+
+static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg)
+{
+ uint64_t value;
+
+ switch (reg) {
+ case TIMER_REG_STATUS:
+ value = ptimer_get_count(t->timer);
+ break;
+ case TIMER_REG_RELOAD:
+ value = t->reload;
+ break;
+ case TIMER_REG_MATCH_FIRST:
+ case TIMER_REG_MATCH_SECOND:
+ value = t->match[reg - 2];
+ break;
+ default:
+ qemu_log_mask(LOG_UNIMP, "%s: Programming error: unexpected reg: %d\n",
+ __func__, reg);
+ value = 0;
+ break;
+ }
+ return value;
+}
+
+static uint64_t aspeed_timer_read(void *opaque, hwaddr offset, unsigned size)
+{
+ AspeedTimerCtrlState *s = opaque;
+ const int reg = (offset & 0xf) / 4;
+ uint64_t value;
+
+ switch (offset) {
+ case 0x30: /* Control Register */
+ value = s->ctrl;
+ break;
+ case 0x34: /* Control Register 2 */
+ value = s->ctrl2;
+ break;
+ case 0x00 ... 0x2c: /* Timers 1 - 4 */
+ value = aspeed_timer_get_value(&s->timers[(offset >> 4)], reg);
+ break;
+ case 0x40 ... 0x8c: /* Timers 5 - 8 */
+ value = aspeed_timer_get_value(&s->timers[(offset >> 4) - 1], reg);
+ break;
+ /* Illegal */
+ case 0x38:
+ case 0x3C:
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ value = 0;
+ break;
+ }
+ trace_aspeed_timer_read(offset, size, value);
+ return value;
+}
+
+static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
+ uint32_t value)
+{
+ AspeedTimer *t;
+
+ g_assert(timer >= 0 && timer < ASPEED_TIMER_NR_TIMERS);
+ trace_aspeed_timer_set_value(timer, reg, value);
+ t = &s->timers[timer];
+ switch (reg) {
+ case TIMER_REG_STATUS:
+ if (timer_enabled(t)) {
+ ptimer_set_count(t->timer, value);
+ }
+ break;
+ case TIMER_REG_RELOAD:
+ t->reload = value;
+ ptimer_set_limit(t->timer, value, 1);
+ break;
+ case TIMER_REG_MATCH_FIRST:
+ case TIMER_REG_MATCH_SECOND:
+ if (value) {
+ /* Non-zero match values are unsupported. As such an interrupt will
+ * always be triggered when the timer reaches zero even if the
+ * overflow interrupt control bit is clear.
+ */
+ qemu_log_mask(LOG_UNIMP, "%s: Match value unsupported by device: "
+ "0x%" PRIx32 "\n", __func__, value);
+ } else {
+ t->match[reg - 2] = value;
+ }
+ break;
+ default:
+ qemu_log_mask(LOG_UNIMP, "%s: Programming error: unexpected reg: %d\n",
+ __func__, reg);
+ break;
+ }
+}
+
+/* Control register operations are broken out into helpers that can be
+ * explictly called on aspeed_timer_reset(), but also from
+ * aspeed_timer_ctrl_op().
+ */
+
+static void aspeed_timer_ctrl_enable(AspeedTimer *t, bool enable)
+{
+ trace_aspeed_timer_ctrl_enable(t->id, enable);
+ if (enable) {
+ ptimer_run(t->timer, 0);
+ } else {
+ ptimer_stop(t->timer);
+ ptimer_set_limit(t->timer, t->reload, 1);
+ }
+}
+
+static void aspeed_timer_ctrl_external_clock(AspeedTimer *t, bool enable)
+{
+ trace_aspeed_timer_ctrl_external_clock(t->id, enable);
+ if (enable) {
+ ptimer_set_freq(t->timer, TIMER_CLOCK_EXT_HZ);
+ } else {
+ ptimer_set_freq(t->timer, TIMER_CLOCK_APB_HZ);
+ }
+}
+
+static void aspeed_timer_ctrl_overflow_interrupt(AspeedTimer *t, bool enable)
+{
+ trace_aspeed_timer_ctrl_overflow_interrupt(t->id, enable);
+}
+
+static void aspeed_timer_ctrl_pulse_enable(AspeedTimer *t, bool enable)
+{
+ if (timer_can_pulse(t)) {
+ trace_aspeed_timer_ctrl_pulse_enable(t->id, enable);
+ } else {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Timer does not support pulse mode\n", __func__);
+ }
+}
+
+/**
+ * Given the actions are fixed in number and completely described in helper
+ * functions, dispatch with a lookup table rather than manage control flow with
+ * a switch statement.
+ */
+static void (*const ctrl_ops[])(AspeedTimer *, bool) = {
+ [op_enable] = aspeed_timer_ctrl_enable,
+ [op_external_clock] = aspeed_timer_ctrl_external_clock,
+ [op_overflow_interrupt] = aspeed_timer_ctrl_overflow_interrupt,
+ [op_pulse_enable] = aspeed_timer_ctrl_pulse_enable,
+};
+
+/**
+ * Conditionally affect changes chosen by a timer's control bit.
+ *
+ * The aspeed_timer_ctrl_op() interface is convenient for the
+ * aspeed_timer_set_ctrl() function as the "no change" early exit can be
+ * calculated for all operations, which cleans up the caller code. However the
+ * interface isn't convenient for the reset function where we want to enter a
+ * specific state without artificially constructing old and new values that
+ * will fall through the change guard (and motivates extracting the actions
+ * out to helper functions).
+ *
+ * @t: The timer to manipulate
+ * @op: The type of operation to be performed
+ * @old: The old state of the timer's control bits
+ * @new: The incoming state for the timer's control bits
+ */
+static void aspeed_timer_ctrl_op(AspeedTimer *t, enum timer_ctrl_op op,
+ uint8_t old, uint8_t new)
+{
+ const uint8_t mask = BIT(op);
+ const bool enable = !!(new & mask);
+ const bool changed = ((old ^ new) & mask);
+ if (!changed) {
+ return;
+ }
+ ctrl_ops[op](t, enable);
+}
+
+static void aspeed_timer_set_ctrl(AspeedTimerCtrlState *s, uint32_t reg)
+{
+ int i;
+ int shift;
+ uint8_t t_old, t_new;
+ AspeedTimer *t;
+ const uint8_t enable_mask = BIT(op_enable);
+
+ /* Handle a dependency between the 'enable' and remaining three
+ * configuration bits - i.e. if more than one bit in the control set has
+ * changed, including the 'enable' bit, then we want either disable the
+ * timer and perform configuration, or perform configuration and then
+ * enable the timer
+ */
+ for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
+ t = &s->timers[i];
+ shift = (i * TIMER_CTRL_BITS);
+ t_old = (s->ctrl >> shift) & TIMER_CTRL_MASK;
+ t_new = (reg >> shift) & TIMER_CTRL_MASK;
+
+ /* If we are disabling, do so first */
+ if ((t_old & enable_mask) && !(t_new & enable_mask)) {
+ aspeed_timer_ctrl_enable(t, false);
+ }
+ aspeed_timer_ctrl_op(t, op_external_clock, t_old, t_new);
+ aspeed_timer_ctrl_op(t, op_overflow_interrupt, t_old, t_new);
+ aspeed_timer_ctrl_op(t, op_pulse_enable, t_old, t_new);
+ /* If we are enabling, do so last */
+ if (!(t_old & enable_mask) && (t_new & enable_mask)) {
+ aspeed_timer_ctrl_enable(t, true);
+ }
+ }
+ s->ctrl = reg;
+}
+
+static void aspeed_timer_set_ctrl2(AspeedTimerCtrlState *s, uint32_t value)
+{
+ trace_aspeed_timer_set_ctrl2(value);
+}
+
+static void aspeed_timer_write(void *opaque, hwaddr offset, uint64_t value,
+ unsigned size)
+{
+ const uint32_t tv = (uint32_t)(value & 0xFFFFFFFF);
+ const int reg = (offset & 0xf) / 4;
+ AspeedTimerCtrlState *s = opaque;
+
+ switch (offset) {
+ /* Control Registers */
+ case 0x30:
+ aspeed_timer_set_ctrl(s, tv);
+ break;
+ case 0x34:
+ aspeed_timer_set_ctrl2(s, tv);
+ break;
+ /* Timer Registers */
+ case 0x00 ... 0x2c:
+ aspeed_timer_set_value(s, (offset >> TIMER_NR_REGS), reg, tv);
+ break;
+ case 0x40 ... 0x8c:
+ aspeed_timer_set_value(s, (offset >> TIMER_NR_REGS) - 1, reg, tv);
+ break;
+ /* Illegal */
+ case 0x38:
+ case 0x3C:
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ break;
+ }
+}
+
+static const MemoryRegionOps aspeed_timer_ops = {
+ .read = aspeed_timer_read,
+ .write = aspeed_timer_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .valid.min_access_size = 4,
+ .valid.max_access_size = 4,
+ .valid.unaligned = false,
+};
+
+static void aspeed_init_one_timer(AspeedTimerCtrlState *s, uint8_t id)
+{
+ QEMUBH *bh;
+ AspeedTimer *t = &s->timers[id];
+
+ t->id = id;
+ bh = qemu_bh_new(aspeed_timer_expire, t);
+ assert(bh);
+ t->timer = ptimer_init(bh);
+ assert(t->timer);
+}
+
+static void aspeed_timer_realize(DeviceState *dev, Error **errp)
+{
+ int i;
+ SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+ AspeedTimerCtrlState *s = ASPEED_TIMER(dev);
+
+ for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
+ aspeed_init_one_timer(s, i);
+ sysbus_init_irq(sbd, &s->timers[i].irq);
+ }
+ memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_timer_ops, s,
+ TYPE_ASPEED_TIMER, 0x1000);
+ sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static void aspeed_timer_reset(DeviceState *dev)
+{
+ int i;
+ AspeedTimerCtrlState *s = ASPEED_TIMER(dev);
+
+ for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
+ AspeedTimer *t = &s->timers[i];
+ /* Explictly call helpers to avoid any conditional behaviour through
+ * aspeed_timer_set_ctrl().
+ */
+ aspeed_timer_ctrl_enable(t, false);
+ aspeed_timer_ctrl_external_clock(t, TIMER_CLOCK_USE_APB);
+ aspeed_timer_ctrl_overflow_interrupt(t, false);
+ aspeed_timer_ctrl_pulse_enable(t, false);
+ t->level = 0;
+ t->reload = 0;
+ t->match[0] = 0;
+ t->match[1] = 0;
+ }
+ s->ctrl = 0;
+ s->ctrl2 = 0;
+}
+
+static const VMStateDescription vmstate_aspeed_timer = {
+ .name = "aspeed.timer",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8(id, AspeedTimer),
+ VMSTATE_INT32(level, AspeedTimer),
+ VMSTATE_PTIMER(timer, AspeedTimer),
+ VMSTATE_UINT32(reload, AspeedTimer),
+ VMSTATE_UINT32_ARRAY(match, AspeedTimer, 2),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static const VMStateDescription vmstate_aspeed_timer_state = {
+ .name = "aspeed.timerctrl",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(ctrl, AspeedTimerCtrlState),
+ VMSTATE_UINT32(ctrl2, AspeedTimerCtrlState),
+ VMSTATE_STRUCT_ARRAY(timers, AspeedTimerCtrlState,
+ ASPEED_TIMER_NR_TIMERS, 1, vmstate_aspeed_timer,
+ AspeedTimer),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void timer_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = aspeed_timer_realize;
+ dc->reset = aspeed_timer_reset;
+ dc->desc = "ASPEED Timer";
+ dc->vmsd = &vmstate_aspeed_timer_state;
+}
+
+static const TypeInfo aspeed_timer_info = {
+ .name = TYPE_ASPEED_TIMER,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(AspeedTimerCtrlState),
+ .class_init = timer_class_init,
+};
+
+static void aspeed_timer_register_types(void)
+{
+ type_register_static(&aspeed_timer_info);
+}
+
+type_init(aspeed_timer_register_types)
diff --git a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h
new file mode 100644
index 0000000..44dc2f8
--- /dev/null
+++ b/include/hw/timer/aspeed_timer.h
@@ -0,0 +1,59 @@
+/*
+ * ASPEED AST2400 Timer
+ *
+ * Andrew Jeffery <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] 13+ messages in thread* Re: [Qemu-devel] [PATCH v4 1/4] hw/timer: Add ASPEED timer device model
2016-03-14 4:13 ` [Qemu-devel] [PATCH v4 1/4] hw/timer: Add ASPEED timer device model Andrew Jeffery
@ 2016-03-15 13:14 ` Cédric Le Goater
2016-03-15 23:06 ` Andrew Jeffery
2016-03-15 18:14 ` Dmitry Osipenko
1 sibling, 1 reply; 13+ messages in thread
From: Cédric Le Goater @ 2016-03-15 13:14 UTC (permalink / raw)
To: Andrew Jeffery, Peter Maydell
Cc: Alexey Kardashevskiy, qemu-arm, Jeremy Kerr, qemu-devel
On 03/14/2016 05:13 AM, Andrew Jeffery wrote:
> Implement basic ASPEED timer functionality for the AST2400 SoC[1]: Up to
> 8 timers can independently be configured, enabled, reset and disabled.
> Some hardware features are not implemented, namely clock value matching
> and pulse generation, but the implementation is enough to boot the Linux
> kernel configured with aspeed_defconfig.
>
> [1] http://www.aspeedtech.com/products.php?fPath=20&rId=376
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Looks good. One stylistic comment and a possible compile break in
timer_to_ctrl().
>
> ---
> Since v3:
> * Drop unnecessary mention of VMStateDescription in timer_to_ctrl description
> * Mention hw/timer/a9gtimer.c with respect to clock value matching
> * Add missing VMSTATE_END_OF_LIST() to vmstate_aspeed_timer_state
>
> Since v2:
> * Improve handling of timer configuration with respect to enabled state
> * Remove redundant enabled member from AspeedTimer
> * Implement VMStateDescriptions
> * Fix interrupt behaviour (edge triggered, both edges)
> * Fix various issues with trace-event declarations
> * Include qemu/osdep.h
>
> Since v1:
> * Refactor initialisation of and respect requested clock rates (APB/External)
> * Simplify some index calculations
> * Use tracing infrastructure instead of internal DPRINTF
> * Enforce access size constraints and alignment in MemoryRegionOps
>
> default-configs/arm-softmmu.mak | 1 +
> hw/timer/Makefile.objs | 1 +
> hw/timer/aspeed_timer.c | 451 ++++++++++++++++++++++++++++++++++++++++
> include/hw/timer/aspeed_timer.h | 59 ++++++
> trace-events | 9 +
> 5 files changed, 521 insertions(+)
> create mode 100644 hw/timer/aspeed_timer.c
> create mode 100644 include/hw/timer/aspeed_timer.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index a9f82a1..2bcd236 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -110,3 +110,4 @@ CONFIG_IOH3420=y
> CONFIG_I82801B11=y
> CONFIG_ACPI=y
> CONFIG_SMBIOS=y
> +CONFIG_ASPEED_SOC=y
> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index 5cfea6e..003c14f 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -32,3 +32,4 @@ obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
> obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o
>
> common-obj-$(CONFIG_STM32F2XX_TIMER) += stm32f2xx_timer.o
> +common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> new file mode 100644
> index 0000000..0e82178
> --- /dev/null
> +++ b/hw/timer/aspeed_timer.c
> @@ -0,0 +1,452 @@
> +/*
> + * ASPEED AST2400 Timer
> + *
> + * Andrew Jeffery <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. The ptimer BH callback needs to know
> + * whether a specific AspeedTimer is enabled, but this information is held in
> + * AspeedTimerCtrlState. So, provide a helper to hoist ourselves from an
> + * arbitrary AspeedTimer to AspeedTimerCtrlState.
> + */
> +static inline struct AspeedTimerCtrlState *timer_to_ctrl(AspeedTimer *t)
you can remove the 'struct' above.
> +{
> + AspeedTimer (*timers)[] = (void *)t - (t->id * sizeof(*t));
This will not compile on gcc < 5.0. You need to add a 'const' :
const AspeedTimer (*timers)[] = (void *)t - (t->id * sizeof(*t));
That should work on all versions.
C.
> + return container_of(timers, AspeedTimerCtrlState, timers);
> +}
> +
> +static inline bool timer_ctrl_status(AspeedTimer *t, enum timer_ctrl_op op)
> +{
> + return !!(timer_to_ctrl(t)->ctrl & BIT(t->id * TIMER_CTRL_BITS + op));
> +}
> +
> +static inline bool timer_enabled(AspeedTimer *t)
> +{
> + return timer_ctrl_status(t, op_enable);
> +}
> +
> +static inline bool timer_overflow_interrupt(AspeedTimer *t)
> +{
> + return timer_ctrl_status(t, op_overflow_interrupt);
> +}
> +
> +static inline bool timer_can_pulse(AspeedTimer *t)
> +{
> + return t->id >= TIMER_FIRST_CAP_PULSE;
> +}
> +
> +static void aspeed_timer_expire(void *opaque)
> +{
> + AspeedTimer *t = opaque;
> +
> + /* Only support interrupts on match values of zero for the moment - this is
> + * sufficient to boot an aspeed_defconfig Linux kernel.
> + *
> + * TODO: matching on arbitrary values (see e.g. hw/timer/a9gtimer.c)
> + */
> + bool match = !(t->match[0] && t->match[1]);
> + bool interrupt = timer_overflow_interrupt(t) || match;
> + if (timer_enabled(t) && interrupt) {
> + t->level = !t->level;
> + qemu_set_irq(t->irq, t->level);
> + }
> +}
> +
> +static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg)
> +{
> + uint64_t value;
> +
> + switch (reg) {
> + case TIMER_REG_STATUS:
> + value = ptimer_get_count(t->timer);
> + break;
> + case TIMER_REG_RELOAD:
> + value = t->reload;
> + break;
> + case TIMER_REG_MATCH_FIRST:
> + case TIMER_REG_MATCH_SECOND:
> + value = t->match[reg - 2];
> + break;
> + default:
> + qemu_log_mask(LOG_UNIMP, "%s: Programming error: unexpected reg: %d\n",
> + __func__, reg);
> + value = 0;
> + break;
> + }
> + return value;
> +}
> +
> +static uint64_t aspeed_timer_read(void *opaque, hwaddr offset, unsigned size)
> +{
> + AspeedTimerCtrlState *s = opaque;
> + const int reg = (offset & 0xf) / 4;
> + uint64_t value;
> +
> + switch (offset) {
> + case 0x30: /* Control Register */
> + value = s->ctrl;
> + break;
> + case 0x34: /* Control Register 2 */
> + value = s->ctrl2;
> + break;
> + case 0x00 ... 0x2c: /* Timers 1 - 4 */
> + value = aspeed_timer_get_value(&s->timers[(offset >> 4)], reg);
> + break;
> + case 0x40 ... 0x8c: /* Timers 5 - 8 */
> + value = aspeed_timer_get_value(&s->timers[(offset >> 4) - 1], reg);
> + break;
> + /* Illegal */
> + case 0x38:
> + case 0x3C:
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
> + __func__, offset);
> + value = 0;
> + break;
> + }
> + trace_aspeed_timer_read(offset, size, value);
> + return value;
> +}
> +
> +static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
> + uint32_t value)
> +{
> + AspeedTimer *t;
> +
> + g_assert(timer >= 0 && timer < ASPEED_TIMER_NR_TIMERS);
> + trace_aspeed_timer_set_value(timer, reg, value);
> + t = &s->timers[timer];
> + switch (reg) {
> + case TIMER_REG_STATUS:
> + if (timer_enabled(t)) {
> + ptimer_set_count(t->timer, value);
> + }
> + break;
> + case TIMER_REG_RELOAD:
> + t->reload = value;
> + ptimer_set_limit(t->timer, value, 1);
> + break;
> + case TIMER_REG_MATCH_FIRST:
> + case TIMER_REG_MATCH_SECOND:
> + if (value) {
> + /* Non-zero match values are unsupported. As such an interrupt will
> + * always be triggered when the timer reaches zero even if the
> + * overflow interrupt control bit is clear.
> + */
> + qemu_log_mask(LOG_UNIMP, "%s: Match value unsupported by device: "
> + "0x%" PRIx32 "\n", __func__, value);
> + } else {
> + t->match[reg - 2] = value;
> + }
> + break;
> + default:
> + qemu_log_mask(LOG_UNIMP, "%s: Programming error: unexpected reg: %d\n",
> + __func__, reg);
> + break;
> + }
> +}
> +
> +/* Control register operations are broken out into helpers that can be
> + * explictly called on aspeed_timer_reset(), but also from
> + * aspeed_timer_ctrl_op().
> + */
> +
> +static void aspeed_timer_ctrl_enable(AspeedTimer *t, bool enable)
> +{
> + trace_aspeed_timer_ctrl_enable(t->id, enable);
> + if (enable) {
> + ptimer_run(t->timer, 0);
> + } else {
> + ptimer_stop(t->timer);
> + ptimer_set_limit(t->timer, t->reload, 1);
> + }
> +}
> +
> +static void aspeed_timer_ctrl_external_clock(AspeedTimer *t, bool enable)
> +{
> + trace_aspeed_timer_ctrl_external_clock(t->id, enable);
> + if (enable) {
> + ptimer_set_freq(t->timer, TIMER_CLOCK_EXT_HZ);
> + } else {
> + ptimer_set_freq(t->timer, TIMER_CLOCK_APB_HZ);
> + }
> +}
> +
> +static void aspeed_timer_ctrl_overflow_interrupt(AspeedTimer *t, bool enable)
> +{
> + trace_aspeed_timer_ctrl_overflow_interrupt(t->id, enable);
> +}
> +
> +static void aspeed_timer_ctrl_pulse_enable(AspeedTimer *t, bool enable)
> +{
> + if (timer_can_pulse(t)) {
> + trace_aspeed_timer_ctrl_pulse_enable(t->id, enable);
> + } else {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Timer does not support pulse mode\n", __func__);
> + }
> +}
> +
> +/**
> + * Given the actions are fixed in number and completely described in helper
> + * functions, dispatch with a lookup table rather than manage control flow with
> + * a switch statement.
> + */
> +static void (*const ctrl_ops[])(AspeedTimer *, bool) = {
> + [op_enable] = aspeed_timer_ctrl_enable,
> + [op_external_clock] = aspeed_timer_ctrl_external_clock,
> + [op_overflow_interrupt] = aspeed_timer_ctrl_overflow_interrupt,
> + [op_pulse_enable] = aspeed_timer_ctrl_pulse_enable,
> +};
> +
> +/**
> + * Conditionally affect changes chosen by a timer's control bit.
> + *
> + * The aspeed_timer_ctrl_op() interface is convenient for the
> + * aspeed_timer_set_ctrl() function as the "no change" early exit can be
> + * calculated for all operations, which cleans up the caller code. However the
> + * interface isn't convenient for the reset function where we want to enter a
> + * specific state without artificially constructing old and new values that
> + * will fall through the change guard (and motivates extracting the actions
> + * out to helper functions).
> + *
> + * @t: The timer to manipulate
> + * @op: The type of operation to be performed
> + * @old: The old state of the timer's control bits
> + * @new: The incoming state for the timer's control bits
> + */
> +static void aspeed_timer_ctrl_op(AspeedTimer *t, enum timer_ctrl_op op,
> + uint8_t old, uint8_t new)
> +{
> + const uint8_t mask = BIT(op);
> + const bool enable = !!(new & mask);
> + const bool changed = ((old ^ new) & mask);
> + if (!changed) {
> + return;
> + }
> + ctrl_ops[op](t, enable);
> +}
> +
> +static void aspeed_timer_set_ctrl(AspeedTimerCtrlState *s, uint32_t reg)
> +{
> + int i;
> + int shift;
> + uint8_t t_old, t_new;
> + AspeedTimer *t;
> + const uint8_t enable_mask = BIT(op_enable);
> +
> + /* Handle a dependency between the 'enable' and remaining three
> + * configuration bits - i.e. if more than one bit in the control set has
> + * changed, including the 'enable' bit, then we want either disable the
> + * timer and perform configuration, or perform configuration and then
> + * enable the timer
> + */
> + for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
> + t = &s->timers[i];
> + shift = (i * TIMER_CTRL_BITS);
> + t_old = (s->ctrl >> shift) & TIMER_CTRL_MASK;
> + t_new = (reg >> shift) & TIMER_CTRL_MASK;
> +
> + /* If we are disabling, do so first */
> + if ((t_old & enable_mask) && !(t_new & enable_mask)) {
> + aspeed_timer_ctrl_enable(t, false);
> + }
> + aspeed_timer_ctrl_op(t, op_external_clock, t_old, t_new);
> + aspeed_timer_ctrl_op(t, op_overflow_interrupt, t_old, t_new);
> + aspeed_timer_ctrl_op(t, op_pulse_enable, t_old, t_new);
> + /* If we are enabling, do so last */
> + if (!(t_old & enable_mask) && (t_new & enable_mask)) {
> + aspeed_timer_ctrl_enable(t, true);
> + }
> + }
> + s->ctrl = reg;
> +}
> +
> +static void aspeed_timer_set_ctrl2(AspeedTimerCtrlState *s, uint32_t value)
> +{
> + trace_aspeed_timer_set_ctrl2(value);
> +}
> +
> +static void aspeed_timer_write(void *opaque, hwaddr offset, uint64_t value,
> + unsigned size)
> +{
> + const uint32_t tv = (uint32_t)(value & 0xFFFFFFFF);
> + const int reg = (offset & 0xf) / 4;
> + AspeedTimerCtrlState *s = opaque;
> +
> + switch (offset) {
> + /* Control Registers */
> + case 0x30:
> + aspeed_timer_set_ctrl(s, tv);
> + break;
> + case 0x34:
> + aspeed_timer_set_ctrl2(s, tv);
> + break;
> + /* Timer Registers */
> + case 0x00 ... 0x2c:
> + aspeed_timer_set_value(s, (offset >> TIMER_NR_REGS), reg, tv);
> + break;
> + case 0x40 ... 0x8c:
> + aspeed_timer_set_value(s, (offset >> TIMER_NR_REGS) - 1, reg, tv);
> + break;
> + /* Illegal */
> + case 0x38:
> + case 0x3C:
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
> + __func__, offset);
> + break;
> + }
> +}
> +
> +static const MemoryRegionOps aspeed_timer_ops = {
> + .read = aspeed_timer_read,
> + .write = aspeed_timer_write,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> + .valid.min_access_size = 4,
> + .valid.max_access_size = 4,
> + .valid.unaligned = false,
> +};
> +
> +static void aspeed_init_one_timer(AspeedTimerCtrlState *s, uint8_t id)
> +{
> + QEMUBH *bh;
> + AspeedTimer *t = &s->timers[id];
> +
> + t->id = id;
> + bh = qemu_bh_new(aspeed_timer_expire, t);
> + assert(bh);
> + t->timer = ptimer_init(bh);
> + assert(t->timer);
> +}
> +
> +static void aspeed_timer_realize(DeviceState *dev, Error **errp)
> +{
> + int i;
> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> + AspeedTimerCtrlState *s = ASPEED_TIMER(dev);
> +
> + for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
> + aspeed_init_one_timer(s, i);
> + sysbus_init_irq(sbd, &s->timers[i].irq);
> + }
> + memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_timer_ops, s,
> + TYPE_ASPEED_TIMER, 0x1000);
> + sysbus_init_mmio(sbd, &s->iomem);
> +}
> +
> +static void aspeed_timer_reset(DeviceState *dev)
> +{
> + int i;
> + AspeedTimerCtrlState *s = ASPEED_TIMER(dev);
> +
> + for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
> + AspeedTimer *t = &s->timers[i];
> + /* Explictly call helpers to avoid any conditional behaviour through
> + * aspeed_timer_set_ctrl().
> + */
> + aspeed_timer_ctrl_enable(t, false);
> + aspeed_timer_ctrl_external_clock(t, TIMER_CLOCK_USE_APB);
> + aspeed_timer_ctrl_overflow_interrupt(t, false);
> + aspeed_timer_ctrl_pulse_enable(t, false);
> + t->level = 0;
> + t->reload = 0;
> + t->match[0] = 0;
> + t->match[1] = 0;
> + }
> + s->ctrl = 0;
> + s->ctrl2 = 0;
> +}
> +
> +static const VMStateDescription vmstate_aspeed_timer = {
> + .name = "aspeed.timer",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT8(id, AspeedTimer),
> + VMSTATE_INT32(level, AspeedTimer),
> + VMSTATE_PTIMER(timer, AspeedTimer),
> + VMSTATE_UINT32(reload, AspeedTimer),
> + VMSTATE_UINT32_ARRAY(match, AspeedTimer, 2),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static const VMStateDescription vmstate_aspeed_timer_state = {
> + .name = "aspeed.timerctrl",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(ctrl, AspeedTimerCtrlState),
> + VMSTATE_UINT32(ctrl2, AspeedTimerCtrlState),
> + VMSTATE_STRUCT_ARRAY(timers, AspeedTimerCtrlState,
> + ASPEED_TIMER_NR_TIMERS, 1, vmstate_aspeed_timer,
> + AspeedTimer),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static void timer_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->realize = aspeed_timer_realize;
> + dc->reset = aspeed_timer_reset;
> + dc->desc = "ASPEED Timer";
> + dc->vmsd = &vmstate_aspeed_timer_state;
> +}
> +
> +static const TypeInfo aspeed_timer_info = {
> + .name = TYPE_ASPEED_TIMER,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(AspeedTimerCtrlState),
> + .class_init = timer_class_init,
> +};
> +
> +static void aspeed_timer_register_types(void)
> +{
> + type_register_static(&aspeed_timer_info);
> +}
> +
> +type_init(aspeed_timer_register_types)
> diff --git a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h
> new file mode 100644
> index 0000000..44dc2f8
> --- /dev/null
> +++ b/include/hw/timer/aspeed_timer.h
> @@ -0,0 +1,59 @@
> +/*
> + * ASPEED AST2400 Timer
> + *
> + * Andrew Jeffery <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
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v4 1/4] hw/timer: Add ASPEED timer device model
2016-03-15 13:14 ` Cédric Le Goater
@ 2016-03-15 23:06 ` Andrew Jeffery
0 siblings, 0 replies; 13+ messages in thread
From: Andrew Jeffery @ 2016-03-15 23:06 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell
Cc: Alexey Kardashevskiy, qemu-arm, Jeremy Kerr, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5236 bytes --]
On Tue, 2016-03-15 at 14:14 +0100, Cédric Le Goater wrote:
> On 03/14/2016 05:13 AM, Andrew Jeffery wrote:
> > Implement basic ASPEED timer functionality for the AST2400 SoC[1]: Up to
> > 8 timers can independently be configured, enabled, reset and disabled.
> > Some hardware features are not implemented, namely clock value matching
> > and pulse generation, but the implementation is enough to boot the Linux
> > kernel configured with aspeed_defconfig.
> >
> > [1] http://www.aspeedtech.com/products.php?fPath=20&rId=376
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>
> Looks good. One stylistic comment and a possible compile break in
> timer_to_ctrl().
>
> >
> > ---
> > Since v3:
> > * Drop unnecessary mention of VMStateDescription in timer_to_ctrl description
> > * Mention hw/timer/a9gtimer.c with respect to clock value matching
> > * Add missing VMSTATE_END_OF_LIST() to vmstate_aspeed_timer_state
> >
> > Since v2:
> > * Improve handling of timer configuration with respect to enabled state
> > * Remove redundant enabled member from AspeedTimer
> > * Implement VMStateDescriptions
> > * Fix interrupt behaviour (edge triggered, both edges)
> > * Fix various issues with trace-event declarations
> > * Include qemu/osdep.h
> >
> > Since v1:
> > * Refactor initialisation of and respect requested clock rates (APB/External)
> > * Simplify some index calculations
> > * Use tracing infrastructure instead of internal DPRINTF
> > * Enforce access size constraints and alignment in MemoryRegionOps
> >
> > default-configs/arm-softmmu.mak | 1 +
> > hw/timer/Makefile.objs | 1 +
> > hw/timer/aspeed_timer.c | 451 ++++++++++++++++++++++++++++++++++++++++
> > include/hw/timer/aspeed_timer.h | 59 ++++++
> > trace-events | 9 +
> > 5 files changed, 521 insertions(+)
> > create mode 100644 hw/timer/aspeed_timer.c
> > create mode 100644 include/hw/timer/aspeed_timer.h
> >
> > diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> > index a9f82a1..2bcd236 100644
> > --- a/default-configs/arm-softmmu.mak
> > +++ b/default-configs/arm-softmmu.mak
> > @@ -110,3 +110,4 @@ CONFIG_IOH3420=y
> > CONFIG_I82801B11=y
> > CONFIG_ACPI=y
> > CONFIG_SMBIOS=y
> > +CONFIG_ASPEED_SOC=y
> > diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> > index 5cfea6e..003c14f 100644
> > --- a/hw/timer/Makefile.objs
> > +++ b/hw/timer/Makefile.objs
> > @@ -32,3 +32,4 @@ obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
> > obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o
> >
> > common-obj-$(CONFIG_STM32F2XX_TIMER) += stm32f2xx_timer.o
> > +common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o
> > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> > new file mode 100644
> > index 0000000..0e82178
> > --- /dev/null
> > +++ b/hw/timer/aspeed_timer.c
> > @@ -0,0 +1,452 @@
> > +/*
> > + * ASPEED AST2400 Timer
> > + *
> > + * Andrew Jeffery <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. The ptimer BH callback needs to know
> > + * whether a specific AspeedTimer is enabled, but this information is held in
> > + * AspeedTimerCtrlState. So, provide a helper to hoist ourselves from an
> > + * arbitrary AspeedTimer to AspeedTimerCtrlState.
> > + */
> > +static inline struct AspeedTimerCtrlState *timer_to_ctrl(AspeedTimer *t)
>
>
> you can remove the 'struct' above.
Good catch, will do.
>
> > +{
> > + AspeedTimer (*timers)[] = (void *)t - (t->id * sizeof(*t));
>
> This will not compile on gcc < 5.0. You need to add a 'const' :
>
> const AspeedTimer (*timers)[] = (void *)t - (t->id * sizeof(*t));
>
> That should work on all versions.
Thanks, I've confirmed the failure and fix with gcc-4.7. I'll make sure
to test with the earliest gcc easily available to me (looks like it's
4.7, on Ubuntu Wily) going forward.
Cheers,
Andrew
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/4] hw/timer: Add ASPEED timer device model
2016-03-14 4:13 ` [Qemu-devel] [PATCH v4 1/4] hw/timer: Add ASPEED timer device model Andrew Jeffery
2016-03-15 13:14 ` Cédric Le Goater
@ 2016-03-15 18:14 ` Dmitry Osipenko
2016-03-15 22:48 ` Andrew Jeffery
1 sibling, 1 reply; 13+ messages in thread
From: Dmitry Osipenko @ 2016-03-15 18:14 UTC (permalink / raw)
To: Andrew Jeffery, Peter Maydell
Cc: Alexey Kardashevskiy, Jeremy Kerr, qemu-arm, qemu-devel,
Cédric Le Goater
Hello Andrew,
14.03.2016 07:13, Andrew Jeffery пишет:
> Implement basic ASPEED timer functionality for the AST2400 SoC[1]: Up to
> 8 timers can independently be configured, enabled, reset and disabled.
> Some hardware features are not implemented, namely clock value matching
> and pulse generation, but the implementation is enough to boot the Linux
> kernel configured with aspeed_defconfig.
>
[snip]
> +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);
This would never fail, wouldn't it?
[snip]
> +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;
[snip]
> +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);
> +}
I'm wondering why do you need those asserts, it's very unlikely that this code
would fail. Have you had any weird issues with it?
--
Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v4 1/4] hw/timer: Add ASPEED timer device model
2016-03-15 18:14 ` Dmitry Osipenko
@ 2016-03-15 22:48 ` Andrew Jeffery
0 siblings, 0 replies; 13+ messages in thread
From: Andrew Jeffery @ 2016-03-15 22:48 UTC (permalink / raw)
To: Dmitry Osipenko, Peter Maydell
Cc: Alexey Kardashevskiy, Jeremy Kerr, qemu-arm, qemu-devel,
Cédric Le Goater
[-- Attachment #1: Type: text/plain, Size: 2574 bytes --]
Hi Dmitry,
On Tue, 2016-03-15 at 21:14 +0300, Dmitry Osipenko wrote:
> Hello Andrew,
>
> 14.03.2016 07:13, Andrew Jeffery пишет:
> > Implement basic ASPEED timer functionality for the AST2400 SoC[1]: Up to
> > 8 timers can independently be configured, enabled, reset and disabled.
> > Some hardware features are not implemented, namely clock value matching
> > and pulse generation, but the implementation is enough to boot the Linux
> > kernel configured with aspeed_defconfig.
> >
>
> [snip]
>
> > +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);
>
> This would never fail, wouldn't it?
You're right, it shouldn't: I put it in as a sanity check and some
"active" documentation. I'm happy to remove it if you think just adds
noise.
>
> [snip]
>
> > +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;
>
>
> [snip]
>
> > +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);
> > +}
>
> I'm wondering why do you need those asserts, it's very unlikely that this code
> would fail. Have you had any weird issues with it?
No, no weird issues - thanks for pointing them out as I'll remove them:
I put them in when I started developing the series, before
understanding that either call should already have aborted if the
allocations failed.
Thanks for taking a look at the patch!
Andrew
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v4 2/4] hw/intc: Add (new) ASPEED VIC device model
2016-03-14 4:13 [Qemu-devel] [PATCH v4 0/4] Add ASPEED AST2400 SoC and OpenPower BMC machine Andrew Jeffery
2016-03-14 4:13 ` [Qemu-devel] [PATCH v4 1/4] hw/timer: Add ASPEED timer device model Andrew Jeffery
@ 2016-03-14 4:13 ` Andrew Jeffery
2016-03-14 4:13 ` [Qemu-devel] [PATCH v4 3/4] hw/arm: Add ASPEED AST2400 SoC model Andrew Jeffery
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Andrew Jeffery @ 2016-03-14 4:13 UTC (permalink / raw)
To: Peter Maydell
Cc: Alexey Kardashevskiy, qemu-devel, qemu-arm, Cédric Le Goater,
Jeremy Kerr
Implement a basic ASPEED VIC device model for the AST2400 SoC[1], with
enough functionality to boot an aspeed_defconfig Linux kernel. The model
implements the 'new' (revised) register set: While the hardware exposes
both the new and legacy register sets, accesses to the model's legacy
register set will not be serviced (however the access will be logged).
[1] http://www.aspeedtech.com/products.php?fPath=20&rId=376
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
Since v3:
* Switch from g_assert() to qemu_log_mask(LOG_GUEST_ERROR, ...) in guest path
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 | 339 +++++++++++++++++++++++++++++++++++++++++++
include/hw/intc/aspeed_vic.h | 48 ++++++
trace-events | 7 +
4 files changed, 395 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..b025de9
--- /dev/null
+++ b/hw/intc/aspeed_vic.c
@@ -0,0 +1,339 @@
+/*
+ * 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: Invalid 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 */
+ if (high) {
+ s->event &= ~AVIC_EVENT_W_MASK;
+ s->event |= (data & AVIC_EVENT_W_MASK);
+ } else {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "Ignoring invalid write to interrupt event register");
+ }
+ 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] 13+ messages in thread* [Qemu-devel] [PATCH v4 3/4] hw/arm: Add ASPEED AST2400 SoC model
2016-03-14 4:13 [Qemu-devel] [PATCH v4 0/4] Add ASPEED AST2400 SoC and OpenPower BMC machine Andrew Jeffery
2016-03-14 4:13 ` [Qemu-devel] [PATCH v4 1/4] hw/timer: Add ASPEED timer device model Andrew Jeffery
2016-03-14 4:13 ` [Qemu-devel] [PATCH v4 2/4] hw/intc: Add (new) ASPEED VIC " Andrew Jeffery
@ 2016-03-14 4:13 ` Andrew Jeffery
2016-03-14 4:13 ` [Qemu-devel] [PATCH v4 4/4] hw/arm: Add opbmc2400, an AST2400 OpenPOWER BMC machine Andrew Jeffery
2016-03-15 4:34 ` [Qemu-devel] [PATCH v4 0/4] Add ASPEED AST2400 SoC and OpenPower " Jeremy Kerr
4 siblings, 0 replies; 13+ messages in thread
From: Andrew Jeffery @ 2016-03-14 4:13 UTC (permalink / raw)
To: Peter Maydell
Cc: Alexey Kardashevskiy, qemu-devel, qemu-arm, Cédric Le Goater,
Jeremy Kerr
While the ASPEED AST2400 SoC[1] has a broad range of capabilities this
implementation is minimal, comprising an ARM926 processor, ASPEED VIC
and timer devices, and a 8250 UART.
[1] http://www.aspeedtech.com/products.php?fPath=20&rId=376
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
Since v3:
* Split the SoC from the machine implementation
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
hw/arm/Makefile.objs | 1 +
hw/arm/ast2400.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++
include/hw/arm/ast2400.h | 35 ++++++++++++
3 files changed, 173 insertions(+)
create mode 100644 hw/arm/ast2400.c
create mode 100644 include/hw/arm/ast2400.h
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..ecd9e1e
--- /dev/null
+++ b/hw/arm/ast2400.c
@@ -0,0 +1,137 @@
+/*
+ * AST2400 SoC
+ *
+ * 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/ast2400.h"
+#include "hw/char/serial.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
+
+static const int uart_irqs[] = { 9, 32, 33, 34, 10 };
+static const int timer_irqs[] = { 16, 17, 18, 35, 36, 37, 38, 39, };
+
+/*
+ * IO handlers: 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)
+{
+ qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u]\n",
+ __func__, offset, size);
+ return 0;
+}
+
+static void ast2400_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_io_ops = {
+ .read = ast2400_io_read,
+ .write = ast2400_io_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void ast2400_init(Object *obj)
+{
+ AST2400State *s = AST2400(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_realize(DeviceState *dev, Error **errp)
+{
+ int i;
+ AST2400State *s = AST2400(dev);
+ Error *err = NULL;
+
+ /* IO space */
+ memory_region_init_io(&s->iomem, NULL, &ast2400_io_ops, NULL,
+ "ast2400.io", AST2400_IOMEM_SIZE);
+ memory_region_add_subregion_overlap(get_system_memory(), AST2400_IOMEM_BASE,
+ &s->iomem, -1);
+
+ /* 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_class_init(ObjectClass *oc, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(oc);
+
+ dc->realize = ast2400_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_type_info = {
+ .name = TYPE_AST2400,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(AST2400State),
+ .instance_init = ast2400_init,
+ .class_init = ast2400_class_init,
+};
+
+static void ast2400_register_types(void)
+{
+ type_register_static(&ast2400_type_info);
+}
+
+type_init(ast2400_register_types)
diff --git a/include/hw/arm/ast2400.h b/include/hw/arm/ast2400.h
new file mode 100644
index 0000000..f16a1ed
--- /dev/null
+++ b/include/hw/arm/ast2400.h
@@ -0,0 +1,35 @@
+/*
+ * ASPEED AST2400 SoC
+ *
+ * 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.
+ */
+
+#ifndef AST2400_H
+#define AST2400_H
+
+#include "hw/arm/arm.h"
+#include "hw/intc/aspeed_vic.h"
+#include "hw/timer/aspeed_timer.h"
+
+typedef struct AST2400State {
+ /*< private >*/
+ DeviceState parent;
+
+ /*< public >*/
+ ARMCPU *cpu;
+ MemoryRegion iomem;
+ AspeedVICState vic;
+ AspeedTimerCtrlState timerctrl;
+} AST2400State;
+
+#define TYPE_AST2400 "ast2400"
+#define AST2400(obj) OBJECT_CHECK(AST2400State, (obj), TYPE_AST2400)
+
+#define AST2400_SDRAM_BASE 0x40000000
+
+#endif /* AST2400_H */
--
2.5.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PATCH v4 4/4] hw/arm: Add opbmc2400, an AST2400 OpenPOWER BMC machine
2016-03-14 4:13 [Qemu-devel] [PATCH v4 0/4] Add ASPEED AST2400 SoC and OpenPower BMC machine Andrew Jeffery
` (2 preceding siblings ...)
2016-03-14 4:13 ` [Qemu-devel] [PATCH v4 3/4] hw/arm: Add ASPEED AST2400 SoC model Andrew Jeffery
@ 2016-03-14 4:13 ` Andrew Jeffery
2016-03-15 4:34 ` [Qemu-devel] [PATCH v4 0/4] Add ASPEED AST2400 SoC and OpenPower " Jeremy Kerr
4 siblings, 0 replies; 13+ messages in thread
From: Andrew Jeffery @ 2016-03-14 4:13 UTC (permalink / raw)
To: Peter Maydell
Cc: Alexey Kardashevskiy, qemu-devel, qemu-arm, Cédric Le Goater,
Jeremy Kerr
The new machine, opbmc2400, is a thin layer over the AST2400
ARM926-based SoC[1]. Between the minimal machine and the current SoC
implementation there is enough functionality to boot an aspeed_defconfig
Linux kernel to userspace.
The machine name is a pragmatic choice, as there doesn't appear to be a
common, formal name for the hardware configuration that isn't generic
(e.g. 'BMC' or 'AST2400').
[1] http://www.aspeedtech.com/products.php?fPath=20&rId=376
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
Since v3:
* Split the machine from the SoC implementation
I've gone the route of inventing a machine name as it seemed there are very few
cases of appending _soc to the chip name or beginning a machine name with mach_
as an outlet for a lack of creativity. It seems only fair to leave the 'ast2400'
namespace to the SoC itself, as it's unlikely that the OpenPOWER BMC is the
SoC's only consumer. I feel that 'opbmc2400' is a reasonable choice; it's not
overly long, and appending the SoC's marketing number leaves the door open to
future OpenPOWER BMC machines with e.g. the AST2500 series SoC.
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
hw/arm/Makefile.objs | 2 +-
hw/arm/opbmc2400.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 67 insertions(+), 1 deletion(-)
create mode 100644 hw/arm/opbmc2400.c
diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index f333b7f..7b80460 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -16,4 +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
+obj-$(CONFIG_ASPEED_SOC) += ast2400.o opbmc2400.o
diff --git a/hw/arm/opbmc2400.c b/hw/arm/opbmc2400.c
new file mode 100644
index 0000000..a428364
--- /dev/null
+++ b/hw/arm/opbmc2400.c
@@ -0,0 +1,66 @@
+/*
+ * OpenPOWER BMC with AST2400 SoC
+ *
+ * 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.
+ */
+
+#include "qemu/osdep.h"
+#include "exec/address-spaces.h"
+#include "hw/arm/arm.h"
+#include "hw/arm/ast2400.h"
+#include "hw/boards.h"
+
+static struct arm_boot_info opbmc2400_binfo = {
+ .loader_start = AST2400_SDRAM_BASE,
+ .board_id = 0,
+ .nb_cpus = 1,
+};
+
+typedef struct OPBMC2400State {
+ AST2400State soc;
+ MemoryRegion ram;
+} OPBMC2400State;
+
+static void opbmc2400_init(MachineState *machine)
+{
+ OPBMC2400State *opbmc2400;
+
+ opbmc2400 = g_new0(OPBMC2400State, 1);
+ object_initialize(&opbmc2400->soc, (sizeof(opbmc2400->soc)), TYPE_AST2400);
+ object_property_add_child(OBJECT(machine), "soc", OBJECT(&opbmc2400->soc),
+ &error_abort);
+
+ memory_region_allocate_system_memory(&opbmc2400->ram, NULL, "ram",
+ ram_size);
+ memory_region_add_subregion(get_system_memory(), AST2400_SDRAM_BASE,
+ &opbmc2400->ram);
+ object_property_add_const_link(OBJECT(&opbmc2400->soc), "ram",
+ OBJECT(&opbmc2400->ram), &error_abort);
+ object_property_set_bool(OBJECT(&opbmc2400->soc), true, "realized",
+ &error_abort);
+
+ opbmc2400_binfo.kernel_filename = machine->kernel_filename;
+ opbmc2400_binfo.initrd_filename = machine->initrd_filename;
+ opbmc2400_binfo.kernel_cmdline = machine->kernel_cmdline;
+ opbmc2400_binfo.ram_size = ram_size;
+ arm_load_kernel(ARM_CPU(first_cpu), &opbmc2400_binfo);
+}
+
+static void opbmc2400_machine_init(MachineClass *mc)
+{
+ mc->desc = "OpenPOWER AST2400 BMC (ARM926EJ-S)";
+ mc->init = opbmc2400_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("opbmc2400", opbmc2400_machine_init);
--
2.5.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v4 0/4] Add ASPEED AST2400 SoC and OpenPower BMC machine
2016-03-14 4:13 [Qemu-devel] [PATCH v4 0/4] Add ASPEED AST2400 SoC and OpenPower BMC machine Andrew Jeffery
` (3 preceding siblings ...)
2016-03-14 4:13 ` [Qemu-devel] [PATCH v4 4/4] hw/arm: Add opbmc2400, an AST2400 OpenPOWER BMC machine Andrew Jeffery
@ 2016-03-15 4:34 ` Jeremy Kerr
2016-03-15 5:01 ` Andrew Jeffery
4 siblings, 1 reply; 13+ messages in thread
From: Jeremy Kerr @ 2016-03-15 4:34 UTC (permalink / raw)
To: Andrew Jeffery, Peter Maydell
Cc: Alexey Kardashevskiy, qemu-arm, Cédric Le Goater, qemu-devel
Hi Andrew,
> This patch series models enough of the ASPEED AST2400 ARM9 SoC[0] to
> boot an aspeed_defconfig Linux kernel[1][2]. Specifically, the series
> implements the ASPEED timer and VIC devices, integrates them into an
> AST2400 SoC and exposes it all through a new opbmc2400 machine. The
> device model patches only partially implement the hardware features of
> the timer and VIC, again mostly just enough to boot Linux.
Awesome! Nice to have these patches escaping the lab :)
In terms of naming suggestions: I think this depends on what we're
looking to emulate here. I see two options:
The qemu platform becomes a "reference" for OpenPOWER bmc hardware, but
doesn't necessarily align with an actual machine. In that case,
something generic like opbmc-<SoC> would make sense.
On the other hand, if we'd like to create qemu platforms that represent
actual machines (eg, the OpenPOWER "palmetto" machine), then
<PLATFORM>-bmc would seem more appropriate. In this case, the machine
name would be palmetto-bmc. No need to include the SoC name in that, as
it's defined by the hardware implementation.
I think the latter option may be more generally useful.
Regards,
Jeremy
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v4 0/4] Add ASPEED AST2400 SoC and OpenPower BMC machine
2016-03-15 4:34 ` [Qemu-devel] [PATCH v4 0/4] Add ASPEED AST2400 SoC and OpenPower " Jeremy Kerr
@ 2016-03-15 5:01 ` Andrew Jeffery
2016-03-15 10:25 ` Cédric Le Goater
0 siblings, 1 reply; 13+ messages in thread
From: Andrew Jeffery @ 2016-03-15 5:01 UTC (permalink / raw)
To: Jeremy Kerr, Peter Maydell
Cc: Alexey Kardashevskiy, qemu-arm, Cédric Le Goater, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]
On Tue, 2016-03-15 at 12:34 +0800, Jeremy Kerr wrote:
> Hi Andrew,
>
> > This patch series models enough of the ASPEED AST2400 ARM9 SoC[0] to
> > boot an aspeed_defconfig Linux kernel[1][2]. Specifically, the series
> > implements the ASPEED timer and VIC devices, integrates them into an
> > AST2400 SoC and exposes it all through a new opbmc2400 machine. The
> > device model patches only partially implement the hardware features of
> > the timer and VIC, again mostly just enough to boot Linux.
>
> Awesome! Nice to have these patches escaping the lab :)
>
> In terms of naming suggestions: I think this depends on what we're
> looking to emulate here. I see two options:
>
> The qemu platform becomes a "reference" for OpenPOWER bmc hardware, but
> doesn't necessarily align with an actual machine. In that case,
> something generic like opbmc- would make sense.
>
> On the other hand, if we'd like to create qemu platforms that represent
> actual machines (eg, the OpenPOWER "palmetto" machine), then
> -bmc would seem more appropriate. In this case, the machine
> name would be palmetto-bmc. No need to include the SoC name in that, as
> it's defined by the hardware implementation.
>
> I think the latter option may be more generally useful.
Okay, agreed, I'll rework the change to use palmetto-bmc for the
machine name. Thanks for the feedback.
Andrew
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/4] Add ASPEED AST2400 SoC and OpenPower BMC machine
2016-03-15 5:01 ` Andrew Jeffery
@ 2016-03-15 10:25 ` Cédric Le Goater
2016-03-15 23:09 ` Andrew Jeffery
0 siblings, 1 reply; 13+ messages in thread
From: Cédric Le Goater @ 2016-03-15 10:25 UTC (permalink / raw)
To: andrew, Jeremy Kerr, Peter Maydell
Cc: Alexey Kardashevskiy, qemu-arm, qemu-devel
On 03/15/2016 06:01 AM, Andrew Jeffery wrote:
> On Tue, 2016-03-15 at 12:34 +0800, Jeremy Kerr wrote:
>> Hi Andrew,
>>
>>> This patch series models enough of the ASPEED AST2400 ARM9 SoC[0] to
>>> boot an aspeed_defconfig Linux kernel[1][2]. Specifically, the series
>>> implements the ASPEED timer and VIC devices, integrates them into an
>>> AST2400 SoC and exposes it all through a new opbmc2400 machine. The
>>> device model patches only partially implement the hardware features of
>>> the timer and VIC, again mostly just enough to boot Linux.
>>
>> Awesome! Nice to have these patches escaping the lab :)
>>
>> In terms of naming suggestions: I think this depends on what we're
>> looking to emulate here. I see two options:
>>
>> The qemu platform becomes a "reference" for OpenPOWER bmc hardware, but
>> doesn't necessarily align with an actual machine. In that case,
>> something generic like opbmc- would make sense.
>>
>> On the other hand, if we'd like to create qemu platforms that represent
>> actual machines (eg, the OpenPOWER "palmetto" machine), then
>> -bmc would seem more appropriate. In this case, the machine
>> name would be palmetto-bmc. No need to include the SoC name in that, as
>> it's defined by the hardware implementation.
>>
>> I think the latter option may be more generally useful.
>
> Okay, agreed, I'll rework the change to use palmetto-bmc for the
> machine name. Thanks for the feedback.
Yes. palmetto-bmc is good choice. Palmetto is a reference machine
for OpenPOWER.
May be change also :
+ mc->desc = "OpenPOWER AST2400 BMC (ARM926EJ-S)";
to reflect that choice.
Thanks,
C.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/4] Add ASPEED AST2400 SoC and OpenPower BMC machine
2016-03-15 10:25 ` Cédric Le Goater
@ 2016-03-15 23:09 ` Andrew Jeffery
0 siblings, 0 replies; 13+ messages in thread
From: Andrew Jeffery @ 2016-03-15 23:09 UTC (permalink / raw)
To: Cédric Le Goater, Jeremy Kerr, Peter Maydell
Cc: Alexey Kardashevskiy, qemu-arm, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1859 bytes --]
On Tue, 2016-03-15 at 11:25 +0100, Cédric Le Goater wrote:
> On 03/15/2016 06:01 AM, Andrew Jeffery wrote:
> > On Tue, 2016-03-15 at 12:34 +0800, Jeremy Kerr wrote:
> > > Hi Andrew,
> > >
> > > > This patch series models enough of the ASPEED AST2400 ARM9 SoC[0] to
> > > > boot an aspeed_defconfig Linux kernel[1][2]. Specifically, the series
> > > > implements the ASPEED timer and VIC devices, integrates them into an
> > > > AST2400 SoC and exposes it all through a new opbmc2400 machine. The
> > > > device model patches only partially implement the hardware features of
> > > > the timer and VIC, again mostly just enough to boot Linux.
> > >
> > > Awesome! Nice to have these patches escaping the lab :)
> > >
> > > In terms of naming suggestions: I think this depends on what we're
> > > looking to emulate here. I see two options:
> > >
> > > The qemu platform becomes a "reference" for OpenPOWER bmc hardware, but
> > > doesn't necessarily align with an actual machine. In that case,
> > > something generic like opbmc- would make sense.
> > >
> > > On the other hand, if we'd like to create qemu platforms that represent
> > > actual machines (eg, the OpenPOWER "palmetto" machine), then
> > > -bmc would seem more appropriate. In this case, the machine
> > > name would be palmetto-bmc. No need to include the SoC name in that, as
> > > it's defined by the hardware implementation.
> > >
> > > I think the latter option may be more generally useful.
> >
> > Okay, agreed, I'll rework the change to use palmetto-bmc for the
> > machine name. Thanks for the feedback.
>
> Yes. palmetto-bmc is good choice. Palmetto is a reference machine
> for OpenPOWER.
>
> May be change also :
>
> + mc->desc = "OpenPOWER AST2400 BMC (ARM926EJ-S)";
>
> to reflect that choice.
Will do!
Thanks,
Andrew
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread