On Thu, Jan 21, 2016 at 11:03 AM, Dmitry Osipenko <dig...@gmail.com> wrote: > Current ARM MPTimer implementation uses QEMUTimer for the actual timer, > this implementation isn't complete and mostly tries to duplicate of what > generic ptimer is already doing fine. > > Conversion to ptimer brings the following benefits and fixes: > - Simple timer pausing implementation > - Fixes counter value preservation after stopping the timer > - Correctly handles prescaler != 0 cases > - Code simplification and reduction > > Bump VMSD to version 3, since VMState is changed and is not compatible > with the previous implementation. > > Signed-off-by: Dmitry Osipenko <dig...@gmail.com> > --- > hw/timer/arm_mptimer.c | 131 > +++++++++++++++++++++-------------------- > include/hw/timer/arm_mptimer.h | 5 +- > 2 files changed, 67 insertions(+), 69 deletions(-) > > diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c > index 3e59c2a..a5f46df 100644 > --- a/hw/timer/arm_mptimer.c > +++ b/hw/timer/arm_mptimer.c > @@ -19,8 +19,9 @@ > * with this program; if not, see <http://www.gnu.org/licenses/>. > */ > > +#include "hw/ptimer.h" > #include "hw/timer/arm_mptimer.h" > -#include "qemu/timer.h" > +#include "qemu/main-loop.h" > #include "qom/cpu.h" > > /* This device implements the per-cpu private timer and watchdog block > @@ -42,33 +43,34 @@ static inline void timerblock_update_irq(TimerBlock *tb) > } > > /* Return conversion factor from mpcore timer ticks to qemu timer ticks. */ > -static inline uint32_t timerblock_scale(TimerBlock *tb) > +static inline uint32_t timerblock_scale(uint32_t control) > { > - return (((tb->control >> 8) & 0xff) + 1) * 10; > + return (((control >> 8) & 0xff) + 1) * 10; > } > > -static void timerblock_reload(TimerBlock *tb, int restart) > +static inline void timerblock_set_count(struct ptimer_state *timer, > + uint32_t control, uint64_t *count) > { > - if (tb->count == 0) { > - return; > + /* PTimer would immediately trigger interrupt for periodic timer > + * when counter set to 0, MPtimer under certain condition only. */
newline before */ > + if ((control & 3) == 3 && (*count == 0) && (control & 0xff00) == 0) { > + *count = ptimer_get_limit(timer); > } > - if (restart) { > - tb->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > + ptimer_set_count(timer, *count); > +} > + > +static inline void timerblock_run(struct ptimer_state *timer, uint32_t > control, > + bool cond) drop cond ... > +{ > + if (cond) { ... control & 1 ... > + ptimer_run(timer, !(control & 2)); > } > - tb->tick += (int64_t)tb->count * timerblock_scale(tb); > - timer_mod(tb->timer, tb->tick); > } > > static void timerblock_tick(void *opaque) > { > TimerBlock *tb = (TimerBlock *)opaque; > tb->status = 1; > - if (tb->control & 2) { > - tb->count = tb->load; > - timerblock_reload(tb, 0); > - } else { > - tb->count = 0; > - } > timerblock_update_irq(tb); > } > > @@ -76,21 +78,11 @@ static uint64_t timerblock_read(void *opaque, hwaddr addr, > unsigned size) > { > TimerBlock *tb = (TimerBlock *)opaque; > - int64_t val; > switch (addr) { > case 0: /* Load */ > - return tb->load; > + return ptimer_get_limit(tb->timer); > case 4: /* Counter. */ > - if (((tb->control & 1) == 0) || (tb->count == 0)) { > - return 0; > - } > - /* Slow and ugly, but hopefully won't happen too often. */ > - val = tb->tick - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > - val /= timerblock_scale(tb); > - if (val < 0) { > - val = 0; > - } > - return val; > + return ptimer_get_count(tb->timer); > case 8: /* Control. */ > return tb->control; > case 12: /* Interrupt status. */ > @@ -104,39 +96,51 @@ static void timerblock_write(void *opaque, hwaddr addr, > uint64_t value, unsigned size) > { > TimerBlock *tb = (TimerBlock *)opaque; > - int64_t old; > + uint32_t control = tb->control; > switch (addr) { > case 0: /* Load */ > - tb->load = value; > - /* Fall through. */ > - case 4: /* Counter. */ > - if ((tb->control & 1) && tb->count) { > - /* Cancel the previous timer. */ > - timer_del(tb->timer); > + /* Setting load to 0 stops the timer if prescaler == 0. */ > + if ((control & 1) && (value == 0) && (control & 0xff00) == 0) { > + ptimer_stop(tb->timer); > + control &= ~1; > } > - tb->count = value; > - if (tb->control & 1) { > - timerblock_reload(tb, 1); > + ptimer_set_limit(tb->timer, value, 1); > + timerblock_run(tb->timer, control, (control & 1)); > + break; > + case 4: /* Counter. */ > + /* Setting counter to 0 stops the one-shot timer if prescaler == 0 > + * and periodic if load = 0. */ newline before */ > + if ((control & 1) && (value == 0) && (control & 0xff00) == 0) { > + if (!(control & 2) || ptimer_get_limit(tb->timer) == 0) { comment doesn't look to match code. The code looks like: "setting counter to 0 while prescaler == 0 will disable the timer if it is either oneshot, or periodic with load == 0", whearas your comment read as if it only applies to one-shot timers. ifs can be merged. > + ptimer_stop(tb->timer); > + control &= ~1; > + } > } > + timerblock_set_count(tb->timer, control, &value); ... and just add in the extra if in here with if (value != 0). The boolean cond argument that just conditionalizes the whole thing is a bit un-usual and the three call sites all pass in (control & 1) in addition to the full raw control. > + timerblock_run(tb->timer, control, (value != 0) && (control & 1)); > break; > case 8: /* Control. */ > - old = tb->control; > - tb->control = value; > - if (value & 1) { > - if ((old & 1) && (tb->count != 0)) { > - /* Do nothing if timer is ticking right now. */ > - break; > + if ((value & 1) && (control & 3) != (value & 3)) { > + uint64_t count = (value & 0xff00) ? 1 : > ptimer_get_count(tb->timer); > + if ((count == 0) && (value & 2)) { > + timerblock_set_count(tb->timer, value, &count); This looks like a weird corner-case, what does it do exactly? I can't follow it so it needs a comment :) Regards, Peter > } > - if (tb->control & 2) { > - tb->count = tb->load; > - } > - timerblock_reload(tb, 1); > - } else if (old & 1) { > - /* Shutdown the timer. */ > - timer_del(tb->timer); > + timerblock_run(tb->timer, value, (count != 0)); > + } else if ((control & 1) && !(value & 1)) { > + ptimer_stop(tb->timer); > + } > + if ((control & 0xff00) != (value & 0xff00)) { > + ptimer_set_period(tb->timer, timerblock_scale(value)); > } > + tb->control = value; > break; > case 12: /* Interrupt status. */ > + /* Simulate continuous IRQ gen. */ > + if ((control & 3) == 3 && (control & 0xff00) != 0) { > + if (ptimer_get_limit(tb->timer) == 0) { > + break; > + } > + } > tb->status &= ~value; > timerblock_update_irq(tb); > break; > @@ -184,13 +188,12 @@ static const MemoryRegionOps timerblock_ops = { > > static void timerblock_reset(TimerBlock *tb) > { > - tb->count = 0; > - tb->load = 0; > tb->control = 0; > tb->status = 0; > - tb->tick = 0; > if (tb->timer) { > - timer_del(tb->timer); > + ptimer_stop(tb->timer); > + ptimer_set_limit(tb->timer, 0, 1); > + ptimer_set_period(tb->timer, timerblock_scale(0)); > } > } > > @@ -235,7 +238,8 @@ static void arm_mptimer_realize(DeviceState *dev, Error > **errp) > */ > for (i = 0; i < s->num_cpu; i++) { > TimerBlock *tb = &s->timerblock[i]; > - tb->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, timerblock_tick, tb); > + QEMUBH *bh = qemu_bh_new(timerblock_tick, tb); > + tb->timer = ptimer_init(bh); > sysbus_init_irq(sbd, &tb->irq); > memory_region_init_io(&tb->iomem, OBJECT(s), &timerblock_ops, tb, > "arm_mptimer_timerblock", 0x20); > @@ -245,26 +249,23 @@ static void arm_mptimer_realize(DeviceState *dev, Error > **errp) > > static const VMStateDescription vmstate_timerblock = { > .name = "arm_mptimer_timerblock", > - .version_id = 2, > - .minimum_version_id = 2, > + .version_id = 3, > + .minimum_version_id = 3, > .fields = (VMStateField[]) { > - VMSTATE_UINT32(count, TimerBlock), > - VMSTATE_UINT32(load, TimerBlock), > VMSTATE_UINT32(control, TimerBlock), > VMSTATE_UINT32(status, TimerBlock), > - VMSTATE_INT64(tick, TimerBlock), > - VMSTATE_TIMER_PTR(timer, TimerBlock), > + VMSTATE_PTIMER(timer, TimerBlock), > VMSTATE_END_OF_LIST() > } > }; > > static const VMStateDescription vmstate_arm_mptimer = { > .name = "arm_mptimer", > - .version_id = 2, > - .minimum_version_id = 2, > + .version_id = 3, > + .minimum_version_id = 3, > .fields = (VMStateField[]) { > VMSTATE_STRUCT_VARRAY_UINT32(timerblock, ARMMPTimerState, num_cpu, > - 2, vmstate_timerblock, TimerBlock), > + 3, vmstate_timerblock, TimerBlock), > VMSTATE_END_OF_LIST() > } > }; > diff --git a/include/hw/timer/arm_mptimer.h b/include/hw/timer/arm_mptimer.h > index b34cba0..c46d8d2 100644 > --- a/include/hw/timer/arm_mptimer.h > +++ b/include/hw/timer/arm_mptimer.h > @@ -27,12 +27,9 @@ > > /* State of a single timer or watchdog block */ > typedef struct { > - uint32_t count; > - uint32_t load; > uint32_t control; > uint32_t status; > - int64_t tick; > - QEMUTimer *timer; > + struct ptimer_state *timer; > qemu_irq irq; > MemoryRegion iomem; > } TimerBlock; > -- > 2.7.0 >