On Tue, 26 Feb 2019 at 11:38, bzt <bztem...@gmail.com> wrote: > > Dear qemu developers, > > Honestly SubmitAPatch is a bit complicated for me, so I'm hoping I've > done everything right. To be sure, you can find my patch here: > https://github.com/bztsrc/qemu-local-timer and diff against the latest > github repo here: > https://github.com/bztsrc/qemu-local-timer/blob/patches/3.1.50.diff > > Currently the IRQ_TIMER in bcm2836_control is defined, but not > implemented. This patch adds the basic functionality to qemu by > implementing 3 new registers in bcm2836_control. You can route the > interrupt to one of the cores' IRQ or FIQ line by writing 0x40000024, > and you can set up a periodic interrupt with 38.4MHz frequency by > writing the divider into 0x40000034. Prescaler are not taken into > account. When the IRQ fired, you'll see it in 0x40000040+4*N bit 11 > with the rest of the local IRQs, and you can acknowledge it by writing > 1<<31 to 0x40000038. > > The patch is pretty simple, should be easy to review: it does not > create new classes, does not delete anything, does not change class > interface, and only two files modified, the bcm2836_control.c and it's > header. > > Sign-off-by: Zoltán Baldaszti <bztem...@gmail.com> > Subject: [PATCH] Added periodic IRQ support for bcm2836_control local timer
OK, here are my substantive review comments. > diff --git a/hw/intc/bcm2836_control.c b/hw/intc/bcm2836_control.c > index cfa5bc7365..fbd31de0f1 100644 > --- a/hw/intc/bcm2836_control.c > +++ b/hw/intc/bcm2836_control.c > @@ -7,7 +7,11 @@ > * This code is licensed under the GNU GPLv2 and later. > * > * At present, only implements interrupt routing, and mailboxes (i.e., > - * not local timer, PMU interrupt, or AXI counters). > + * not PMU interrupt, or AXI counters). > + * > + * 64-bit ARM Local Timer Copyright (c) 2019. Zoltán Baldaszti > + * The IRQ_TIMER support is still very basic, does not handle timer access, > + * and such there's no point in enabling it without the interrupt flag set. Can you be more precise about what's missing here? I didn't see anything in the spec that looked like a register for reading the current timer value (though it would certainly be usual to provide one). > * > * Ref: > * > https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf > @@ -18,6 +22,9 @@ > #include "qemu/log.h" > > #define REG_GPU_ROUTE 0x0c > +#define REG_LOCALTIMERROUTING 0x24 > +#define REG_LOCALTIMERCONTROL 0x34 > +#define REG_LOCALTIMERACK 0x38 > #define REG_TIMERCONTROL 0x40 > #define REG_MBOXCONTROL 0x50 > #define REG_IRQSRC 0x60 > @@ -43,6 +50,13 @@ > #define IRQ_TIMER 11 > #define IRQ_MAX IRQ_TIMER > > +#define LOCALTIMER_FREQ 38400000 > +#define LOCALTIMER_INTFLAG (1 << 31) > +#define LOCALTIMER_RELOAD (1 << 30) > +#define LOCALTIMER_INTENABLE (1 << 29) > +#define LOCALTIMER_ENABLE (1 << 28) > +#define LOCALTIMER_VALUE(x) ((x) & 0xfffffff) > + > static void deliver_local(BCM2836ControlState *s, uint8_t core, uint8_t irq, > uint32_t controlreg, uint8_t controlidx) > { > @@ -78,6 +92,15 @@ static void bcm2836_control_update(BCM2836ControlState *s) > s->fiqsrc[s->route_gpu_fiq] |= (uint32_t)1 << IRQ_GPU; > } > > + /* handle THE local timer interrupt for one of the cores' IRQ/FIQ */ > + if (s->triggered) { > + if (s->route_localtimer & 4) { > + s->fiqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 << IRQ_TIMER; > + } else { > + s->irqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 << IRQ_TIMER; > + } > + } > + > for (i = 0; i < BCM2836_NCORES; i++) { > /* handle local timer interrupts for this core */ > if (s->timerirqs[i]) { > @@ -162,6 +185,62 @@ static void bcm2836_control_set_gpu_fiq(void > *opaque, int irq, int level) > bcm2836_control_update(s); > } > > +static void bcm2836_control_local_timer_set_next(void *opaque) > +{ > + BCM2836ControlState *s = opaque; > + uint64_t next_event; > + > + assert(s->period > 0); > + > + next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + > + (NANOSECONDS_PER_SECOND * s->period / LOCALTIMER_FREQ); This sort of X * Y / Z calculation for timers should be done with muldiv64() which uses a larger intermediate result to avoid overflow problems: next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + muldiv64(s->period, NANOSECONDS_PER_SECOND, LOCALTIMER_FREQ); > + timer_mod(s->timer, next_event); > +} > + > +static void bcm2836_control_local_timer_tick(void *opaque) > +{ > + BCM2836ControlState *s = opaque; > + > + bcm2836_control_local_timer_set_next(s); > + > + if (!s->triggered) { > + s->triggered = 1; > + bcm2836_control_update(s); > + } > +} > + > +static void bcm2836_control_local_timer_control(void *opaque, uint32_t val) > +{ > + BCM2836ControlState *s = opaque; > + > + s->period = LOCALTIMER_VALUE(val); > + if (val & LOCALTIMER_INTENABLE) { > + if (!s->timer) { > + s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, > + bcm2836_control_local_timer_tick, s); You should just create the timer once, at device initialization, not every time the guest enables it. Enabling the timer can be done with timer_mod(). > + } > + bcm2836_control_local_timer_set_next(s); > + } else { > + if (s->timer) { > + timer_del(s->timer); > + s->timer = NULL; This leaks the timer, because timer_del() is just "turn off the timer", not "deinitialize and free the timer memory". You just want to use timer_del() here, and leave s->timer set so that when it's enabled you can use timer_mod() to restart it. > + } > + s->triggered = 0; > + } > +} > + > +static void bcm2836_control_local_timer_ack(void *opaque, uint32_t val) > +{ > + BCM2836ControlState *s = opaque; > + > + if (val & LOCALTIMER_INTFLAG) { > + s->triggered = 0; > + } > + if (val & LOCALTIMER_RELOAD) { > + bcm2836_control_local_timer_set_next(s); > + } > +} > + > static uint64_t bcm2836_control_read(void *opaque, hwaddr offset, > unsigned size) > { > BCM2836ControlState *s = opaque; > @@ -170,6 +249,14 @@ static uint64_t bcm2836_control_read(void > *opaque, hwaddr offset, unsigned size) > assert(s->route_gpu_fiq < BCM2836_NCORES > && s->route_gpu_irq < BCM2836_NCORES); > return ((uint32_t)s->route_gpu_fiq << 2) | s->route_gpu_irq; > + } else if (offset == REG_LOCALTIMERROUTING) { > + return s->route_localtimer; > + } else if (offset == REG_LOCALTIMERCONTROL) { > + return s->period | > + (s->timer ? LOCALTIMER_ENABLE | LOCALTIMER_INTENABLE : 0) | This looks odd - the timer enable and interrupt enable bits should be independent, not always either both set or both cleared. I suspect this register will be simpler to model if you have the state struct have a uint32_t local_timer_control; which just stores the whole register value (so no separate s->triggered and s->period -- just pull the fields out of the s->local_timer_control with extract32() or masking when you need them). > + (s->triggered ? LOCALTIMER_INTFLAG : 0); > + } else if (offset == REG_LOCALTIMERACK) { > + return 0; > } else if (offset >= REG_TIMERCONTROL && offset < REG_MBOXCONTROL) { > return s->timercontrol[(offset - REG_TIMERCONTROL) >> 2]; > } else if (offset >= REG_MBOXCONTROL && offset < REG_IRQSRC) { > @@ -195,6 +282,12 @@ static void bcm2836_control_write(void *opaque, > hwaddr offset, > if (offset == REG_GPU_ROUTE) { > s->route_gpu_irq = val & 0x3; > s->route_gpu_fiq = (val >> 2) & 0x3; > + } else if (offset == REG_LOCALTIMERROUTING) { > + s->route_localtimer = val & 7; > + } else if (offset == REG_LOCALTIMERCONTROL) { > + bcm2836_control_local_timer_control(s, val); > + } else if (offset == REG_LOCALTIMERACK) { > + bcm2836_control_local_timer_ack(s, val); > } else if (offset >= REG_TIMERCONTROL && offset < REG_MBOXCONTROL) { > s->timercontrol[(offset - REG_TIMERCONTROL) >> 2] = val & 0xff; > } else if (offset >= REG_MBOXCONTROL && offset < REG_IRQSRC) { > @@ -227,6 +320,13 @@ static void bcm2836_control_reset(DeviceState *d) > > s->route_gpu_irq = s->route_gpu_fiq = 0; > > + s->route_localtimer = s->triggered = 0; > + s->period = 0; > + if(s->timer) { > + timer_del(s->timer); > + s->timer = NULL; As above, this shouldn't be NULLing out s->timer; you can just unconditionally call timer_del(). > + } > + > for (i = 0; i < BCM2836_NCORES; i++) { > s->timercontrol[i] = 0; > s->mailboxcontrol[i] = 0; > diff --git a/include/hw/intc/bcm2836_control.h > b/include/hw/intc/bcm2836_control.h > index 613f3c4186..873bd52253 100644 > --- a/include/hw/intc/bcm2836_control.h > +++ b/include/hw/intc/bcm2836_control.h > @@ -5,6 +5,9 @@ > * Rasperry Pi 2 emulation and refactoring Copyright (c) 2015, Microsoft > * Written by Andrew Baumann > * > + * 64-bit ARM Local Timer Copyright (c) 2019. Zoltán Baldaszti > + * Added basic IRQ_TIMER interrupt support > + * > * This code is licensed under the GNU GPLv2 and later. > */ > > @@ -12,6 +15,7 @@ > #define BCM2836_CONTROL_H > > #include "hw/sysbus.h" > +#include "qemu/timer.h" > > /* 4 mailboxes per core, for 16 total */ > #define BCM2836_NCORES 4 > @@ -39,6 +43,12 @@ typedef struct BCM2836ControlState { > bool gpu_irq, gpu_fiq; > uint8_t timerirqs[BCM2836_NCORES]; > > + /* local timer */ > + uint8_t route_localtimer; > + uint32_t period; > + bool triggered; > + QEMUTimer *timer; Slightly nicer to just embed the QEMUTimer struct (and then initialize it with timer_init_ns() rather than using timer_new_ns(), which does 'allocate and init'.) > + > /* interrupt source registers, post-routing (also input-derived; > visible) */ > uint32_t irqsrc[BCM2836_NCORES]; > uint32_t fiqsrc[BCM2836_NCORES]; New fields in this struct require new entries in the vmstate_bcm2836_control struct so the data is migrated. In this case we don't care about maintaining migration compatibility between QEMU versions, so the easiest thing is to bump the .version_id field to 2, and use VMSTATE_*_V(..., 2) macros to mark the new fields as being only present in version 2. If you take my suggestion about using a single uint32_t local_timer_control and an embedded QEMUTimer struct, that works out at VMSTATE_TIMER_V(timer, BCM2836ControlState, 2), VMSTATE_UINT32_V(local_timer_control, BCM2836ControlState, 2), VMSTATE_UINT8_V(route_localtimer, BCM2836ControlState, 2), thanks -- PMM
