[Qemu-arm] [PATCH v2 1/3] hw/timer: Add ASPEED AST2400 timer device model
Andrew Jeffery
andrew at aj.id.au
Fri Feb 26 14:14:20 AEDT 2016
Hi Peter,
On Thu, 2016-02-25 at 16:11 +0000, Peter Maydell wrote:
> On 16 February 2016 at 11:34, Andrew Jeffery <andrew at aj.id.au> wrote:
> > Implement basic AST2400 timer functionality: Timers can be configured,
> > enabled, reset and disabled.
> >
> > A number of hardware features are not implemented:
> >
> > * Timer Overflow interrupts
> > * Clock value matching
> > * Pulse generation
> >
> > The implementation is enough to boot the Linux kernel configured with
> > aspeed_defconfig.
>
> Thanks; this mostly looks in reasonable shape; I have some comments below.
>
> Do we have a datasheet for this chip ?
Unfortunately I don't know of a publicly available datasheet. What's the best way to proceed in this case?
>
> >
> > Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> > ---
> > default-configs/arm-softmmu.mak | 2 +
> > hw/timer/Makefile.objs | 2 +
> > hw/timer/aspeed_timer.c | 313 ++++++++++++++++++++++++++++++++++++++++
> > include/hw/timer/aspeed_timer.h | 55 +++++++
> > trace-events | 9 ++
> > 5 files changed, 381 insertions(+)
> > create mode 100644 hw/timer/aspeed_timer.c
> > create mode 100644 include/hw/timer/aspeed_timer.h
> >
> > diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> > index a9f82a1..4072174 100644
> > --- a/default-configs/arm-softmmu.mak
> > +++ b/default-configs/arm-softmmu.mak
> > @@ -110,3 +110,5 @@ CONFIG_IOH3420=y
> > CONFIG_I82801B11=y
> > CONFIG_ACPI=y
> > CONFIG_SMBIOS=y
> > +
> > +CONFIG_ASPEED_SOC=y
> > diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> > index 133bd0d..f6f7351 100644
> > --- a/hw/timer/Makefile.objs
> > +++ b/hw/timer/Makefile.objs
> > @@ -33,3 +33,5 @@ obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
> > obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o
> >
> > common-obj-$(CONFIG_STM32F2XX_TIMER) += stm32f2xx_timer.o
> > +
> > +common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o
> > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> > new file mode 100644
> > index 0000000..0359528
> > --- /dev/null
> > +++ b/hw/timer/aspeed_timer.c
> > @@ -0,0 +1,313 @@
> > +/*
> > + * ASPEED AST2400 Timer
> > + *
> > + * Andrew Jeffery <andrew at aj.id.au>
> > + *
> > + * Copyright (C) 2015, 2016 IBM Corp.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program; if not, write to the Free Software Foundation, Inc.,
> > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> > + */
> > +
> > +#include
> > +#include
> > +#include "hw/sysbus.h"
> > +#include "qemu/timer.h"
> > +#include "qemu-common.h"
> > +#include "hw/ptimer.h"
> > +#include "qemu/main-loop.h"
> > +#include "hw/timer/aspeed_timer.h"
> > +#include "trace.h"
>
> All source files need to #include "qemu/osdep.h" as their first
> include. That then means you don't need to include assert.h or
> stdio.h yourself.
>
> What do we need from qemu/main-loop.h?
I'm using it for qemu_bh_new() which is required by ptimer, who registers the aspeed_timer_tick() callback into the main loop timer handling. I think this callback has a poorly chosen name - it's probably better called aspeed_timer_expire(), which I'll fix. ptimer seemed like a logical choice for implementing the functionality to me, so main-loop.h is required?
> > +#define TIMER_NR_REGS 4
> > +
> > +#define TIMER_CTRL_BITS 4
> > +
> > +#define TIMER_CLOCK_USE_EXT true
> > +#define TIMER_CLOCK_EXT_HZ 1000000
> > +#define TIMER_CLOCK_USE_APB false
> > +#define TIMER_CLOCK_APB_HZ 24000000
> > +
> > +#define TIMER_CTRL_OP_ENABLE 0
> > +#define TIMER_CTRL_OP_CLOCK_SELECT 1
> > +#define TIMER_CTRL_OP_OVERFLOW_INTERRUPT 2
> > +#define TIMER_CTRL_OP_PULSE_ENABLE 3
> > +
> > +#define TIMER_REG_STATUS 0
> > +#define TIMER_REG_RELOAD 1
> > +#define TIMER_REG_MATCH_FIRST 2
> > +#define TIMER_REG_MATCH_SECOND 3
> > +
> > +static inline bool timer_can_pulse(AspeedTimer *t)
> > +{
> > + return t->id >= 4;
> > +}
> > +
> > +static void aspeed_timer_irq_update(AspeedTimer *t)
> > +{
> > + qemu_set_irq(t->irq, t->enabled);
>
> Surely the timer doesn't assert its IRQ line all
> the time it's enabled? This doesn't look like the right condition.
So I think this is correct despite how it looks. There's some cruft from modelling the implementation off of another timer that's probably obscuring things, which should be fixed. aspeed_timer_irq_update() is only called from aspeed_timer_tick(), so I'll just merge the two. Then by renaming aspeed_timer_tick() to aspeed_timer_expire() as mentioned above, this won't look so strange? I've read through the timer handling code (the processing loop in timerlist_run_timers()) and my understanding is it has the behaviour we want - callback on expiry, not on ticks - which is not how it reads as above.
>
> > +}
> > +
> > +static void aspeed_timer_tick(void *opaque)
> > +{
> > + AspeedTimer *t = opaque;
> > +
> > + aspeed_timer_irq_update(t);
> > +}
> > +
> > +static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg)
> > +{
> > + uint64_t value;
> > +
> > + switch (reg) {
> > + case TIMER_REG_STATUS:
> > + value = ptimer_get_count(t->timer);
> > + break;
> > + case TIMER_REG_RELOAD:
> > + value = t->reload;
> > + break;
> > + case TIMER_REG_MATCH_FIRST:
> > + case TIMER_REG_MATCH_SECOND:
> > + value = t->match[reg - 2];
> > + break;
> > + default:
> > + qemu_log_mask(LOG_UNIMP, "Programming error: unexpected reg: %d\n",
> > + reg);
> > + value = 0;
> > + break;
> > + }
> > + return value;
> > +}
> > +
> > +static uint64_t aspeed_timer_read(void *opaque, hwaddr offset, unsigned size)
> > +{
> > + AspeedTimerState *s = opaque;
> > + const int reg = (offset & 0xf) / 4;
> > + uint64_t value;
> > +
> > + switch (offset) {
> > + case 0x30: /* Control Register */
> > + value = s->ctrl;
> > + break;
> > + case 0x34: /* Control Register 2 */
> > + value = s->ctrl2;
> > + break;
> > + case 0x00 ... 0x2c: /* Timers 1 - 4 */
> > + value = aspeed_timer_get_value(&s->timers[(offset >> 4)], reg);
> > + break;
> > + case 0x40 ... 0x8c: /* Timers 5 - 8 */
> > + value = aspeed_timer_get_value(&s->timers[(offset >> 4) - 1], reg);
> > + break;
> > + /* Illegal */
> > + case 0x38:
> > + case 0x3C:
> > + default:
> > + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
> > + __func__, offset);
> > + value = 0;
> > + break;
> > + }
> > + trace_aspeed_timer_read(offset, size, value);
> > + return value;
> > +}
> > +
> > +static void aspeed_timer_set_value(AspeedTimerState *s, int timer, int reg,
> > + uint32_t value)
> > +{
> > + AspeedTimer *t;
> > +
> > + assert(timer >= 0 && timer < ASPEED_TIMER_NR_TIMERS &&
> > + "Programming error: Unexpected timer index");
> > + trace_aspeed_timer_set_value(timer, reg, value);
> > + t = &s->timers[timer];
> > + switch (reg) {
> > + case TIMER_REG_STATUS:
> > + if (t->enabled) {
> > + ptimer_set_count(t->timer, value);
> > + }
> > + break;
> > + case TIMER_REG_RELOAD:
> > + t->reload = value;
> > + ptimer_set_limit(t->timer, value, 1);
> > + break;
> > + case TIMER_REG_MATCH_FIRST:
> > + case TIMER_REG_MATCH_SECOND:
> > + /* Nothing is done to make matching work, we just record the value */
> > + t->match[reg - 2] = value;
> > + break;
> > + default:
> > + qemu_log_mask(LOG_UNIMP, "Programming error: unexpected reg: %d\n",
> > + reg);
> > + break;
> > + }
> > +}
> > +
> > +static void aspeed_timer_ctrl_op(AspeedTimer *t, int op, bool set)
> > +{
> > + switch (op) {
> > + case TIMER_CTRL_OP_ENABLE:
> > + trace_aspeed_timer_ctrl_op_timer_enable(t->id, set);
> > + if (set) {
> > + ptimer_run(t->timer, 0);
> > + } else {
> > + ptimer_stop(t->timer);
> > + ptimer_set_limit(t->timer, t->reload, 1);
> > + }
> > + t->enabled = set;
>
> Is this enabled bit really separate state within the h/w from
> the bit in the control register? Storing the same state twice
> in different places in the qemu device state struct is usually
> a bad idea if you can avoid it.
I didn't like it either, and it likely is redundant as you point out. I was more concentrating on why ptimer didn't seem to expose this state and probably overlooked what I already had, somehow.
>
> > + break;
> > + case TIMER_CTRL_OP_CLOCK_SELECT:
> > + trace_aspeed_timer_ctrl_op_clock_select(t->id, set);
> > + if (set) {
> > + ptimer_set_freq(t->timer, TIMER_CLOCK_EXT_HZ);
> > + } else {
> > + ptimer_set_freq(t->timer, TIMER_CLOCK_APB_HZ);
> > + }
> > + break;
> > + case TIMER_CTRL_OP_OVERFLOW_INTERRUPT:
> > + trace_aspeed_timer_ctrl_op_overflow_interrupt(t->id, set);
> > + break;
> > + case TIMER_CTRL_OP_PULSE_ENABLE:
> > + if (timer_can_pulse(t)) {
> > + trace_aspeed_timer_ctrl_op_pulse_enable(t->id, set);
> > + } else {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "Timer does not support pulse mode\n");
> > + }
> > + break;
> > + default:
> > + qemu_log_mask(LOG_UNIMP, "Programming error, unexpected op: %d\n",
> > + op);
> > + break;
> > + }
> > +}
> > +
> > +static void aspeed_timer_set_ctrl(AspeedTimerState *s, uint32_t new)
> > +{
> > + int i;
> > + uint32_t changed = s->ctrl ^ new;
> > +
> > + while (32 > (i = ctz32(changed))) {
> > + int timer, op;
> > + bool set;
> > + AspeedTimer *t;
> > +
> > + timer = i / TIMER_CTRL_BITS;
> > + assert(timer < ASPEED_TIMER_NR_TIMERS);
> > + t = &s->timers[timer];
> > + op = i % TIMER_CTRL_BITS;
> > + set = new & (1U << i);
> > + aspeed_timer_ctrl_op(t, op, set);
> > + changed &= ~(1U << i);
> > + }
>
> This is effectively processing the bits in order from
> low to high (and doing the loop-through-bits in a confusing
> to read way as well).
Just how the condition is expressed, or more? The condition can be simplified to while(changed), which I've done locally.
That doesn't seem right to me --
> if the guest does one write to say "enable timer with this clock"
> then you want to first pick the right clock and then enable
> the timer, not enable the timer first and then change the
> frequency. (Conversely if the guest writes to disable the timer
> you want to disable it first and then reconfigure it.)
Fair point! I'll have a play with reworking it.
>
> > + s->ctrl = new;
> > +}
> > +
> > +static void aspeed_timer_set_ctrl2(AspeedTimerState *s, uint32_t value)
> > +{
> > + trace_aspeed_timer_set_ctrl2(value);
> > +}
> > +
> > +static void aspeed_timer_write(void *opaque, hwaddr offset, uint64_t value,
> > + unsigned size)
> > +{
> > + const uint32_t tv = (uint32_t)(value & 0xFFFFFFFF);
> > + const int reg = (offset & 0xf) / 4;
> > + AspeedTimerState *s = opaque;
> > +
> > + switch (offset) {
> > + /* Control Registers */
> > + case 0x30:
> > + aspeed_timer_set_ctrl(s, tv);
> > + break;
> > + case 0x34:
> > + aspeed_timer_set_ctrl2(s, tv);
> > + break;
> > + /* Timer Registers */
> > + case 0x00 ... 0x2c:
> > + aspeed_timer_set_value(s, (offset >> TIMER_NR_REGS), reg, tv);
> > + break;
> > + case 0x40 ... 0x8c:
> > + aspeed_timer_set_value(s, (offset >> TIMER_NR_REGS) - 1, reg, tv);
> > + break;
> > + /* Illegal */
> > + case 0x38:
> > + case 0x3C:
> > + default:
> > + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
> > + __func__, offset);
> > + break;
> > + }
> > +}
> > +
> > +static const MemoryRegionOps aspeed_timer_ops = {
> > + .read = aspeed_timer_read,
> > + .write = aspeed_timer_write,
> > + .endianness = DEVICE_NATIVE_ENDIAN,
> > + .valid.min_access_size = 4,
> > + .valid.max_access_size = 4,
> > + .valid.unaligned = false,
> > +};
> > +
> > +static void aspeed_timer_init(AspeedTimer *t, uint8_t id, bool ext_clock)
>
> You should call this aspeed_init_one_timer() or something,
> because with this function name it looks like it ought to
> be the instance_init function for the class.
I wasn't satisfied with the naming here either, so thanks for the suggestion.
>
> > +{
> > + QEMUBH *bh;
> > +
> > + t->id = id;
> > + bh = qemu_bh_new(aspeed_timer_tick, t);
> > + assert(bh);
> > + t->timer = ptimer_init(bh);
> > + assert(t->timer);
> > + aspeed_timer_ctrl_op(t, TIMER_CTRL_OP_CLOCK_SELECT, ext_clock);
> > +}
> > +
> > +static void aspeed_timers_realize(DeviceState *dev, Error **errp)
>
> Stray 's' in function name.
Yep, will fix.
>
> > +{
> > + int i;
> > + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > + AspeedTimerState *s = ASPEED_TIMER(dev);
> > + uint32_t clock_mask = 0;
> > +
> > + for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
> > + aspeed_timer_init(&s->timers[i], i, TIMER_CLOCK_USE_APB);
> > + clock_mask |= (1 << (1 + i * TIMER_CTRL_BITS));
> > + sysbus_init_irq(sbd, &s->timers[i].irq);
> > + }
> > + /* Ensure control reg has timers configured with APB clock selected */
> > + s->ctrl &= ~clock_mask;
>
> Reset of register values should go in your device's reset
> function (which you need to implement and register as dc->reset).
Good point, I'll move it.
>
> > + memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_timer_ops, s,
> > + TYPE_ASPEED_TIMER, 0x1000);
> > + sysbus_init_mmio(sbd, &s->iomem);
> > +}
> > +
> > +static void timer_class_init(ObjectClass *klass, void *data)
> > +{
> > + DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > + dc->realize = aspeed_timers_realize;
> > + dc->desc = "ASPEED Timer";
>
> You should implement a VMState struct for device migration,
> and wire it up here via dc->vmsd.
I'll look into it. I started experimenting with a VMState struct early on in the implementation but threw it away as it wasn't my primary focus at the time.
>
> > +}
> > +
> > +static const TypeInfo aspeed_timer_info = {
> > + .name = TYPE_ASPEED_TIMER,
> > + .parent = TYPE_SYS_BUS_DEVICE,
> > + .instance_size = sizeof(AspeedTimerState),
> > + .class_init = timer_class_init,
> > +};
> > +
> > +static void aspeed_timer_register_types(void)
> > +{
> > + type_register_static(&aspeed_timer_info);
> > +}
> > +
> > +type_init(aspeed_timer_register_types);
>
> Usual convention is to omit the ';' on type_init() etc, though as usual
> there is some code in the tree that doesn't do so.
Okay, I wasn't aware of that. I'll remove it.
Thanks,
Andrew
>
> thanks
> -- PMM
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160226/046d7487/attachment.sig>
More information about the openbmc
mailing list