From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48556) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zm0J7-0001qJ-1h for qemu-devel@nongnu.org; Tue, 13 Oct 2015 10:12:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zm0J2-0004LK-4X for qemu-devel@nongnu.org; Tue, 13 Oct 2015 10:12:00 -0400 Received: from mail-lb0-x233.google.com ([2a00:1450:4010:c04::233]:36625) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zm0J1-0004LG-Pr for qemu-devel@nongnu.org; Tue, 13 Oct 2015 10:11:56 -0400 Received: by lbcao8 with SMTP id ao8so21713183lbc.3 for ; Tue, 13 Oct 2015 07:11:55 -0700 (PDT) From: Dmitry Osipenko Date: Tue, 13 Oct 2015 17:11:03 +0300 Message-Id: <1444745463-10918-1-git-send-email-digetx@gmail.com> Subject: [Qemu-devel] [PATCH v4] arm_mptimer: Convert to use ptimer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite , Peter Maydell Cc: QEMU Developers 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 - Code simplification and reduction Bump VMSD to version 3, since VMState is changed and is not compatible with the previuos implementation. Signed-off-by: Dmitry Osipenko --- Changelog: V2: Fixed changing periodic timer conter value "on the fly". I added a test to the gist to cover that issue. V3: Fixed starting the timer with load = 0 and counter != 0, added tests to the gist for this issue. Changed vmstate version for all VMSD's, since loadvm doesn't check version of nested VMSD. V4: Fixed spurious IT bit set for the timer starting in the periodic mode with counter = 0. Test added. Tests: https://gist.github.com/digetx/dbd46109503b1a91941a hw/timer/arm_mptimer.c | 120 +++++++++++++++++++++-------------------- include/hw/timer/arm_mptimer.h | 4 +- 2 files changed, 64 insertions(+), 60 deletions(-) diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c index 3e59c2a..8c13f01 100644 --- a/hw/timer/arm_mptimer.c +++ b/hw/timer/arm_mptimer.c @@ -19,8 +19,9 @@ * with this program; if not, see . */ +#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 @@ -47,28 +48,10 @@ static inline uint32_t timerblock_scale(TimerBlock *tb) return (((tb->control >> 8) & 0xff) + 1) * 10; } -static void timerblock_reload(TimerBlock *tb, int restart) -{ - if (tb->count == 0) { - return; - } - if (restart) { - tb->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); - } - 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 +59,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; 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. */ @@ -108,33 +81,68 @@ static void timerblock_write(void *opaque, hwaddr addr, switch (addr) { case 0: /* Load */ tb->load = value; - /* Fall through. */ + /* Setting load to 0 for the running timer stops it without + * triggering the interrupt. */ + if (value == 0) { + ptimer_stop(tb->timer); + } + ptimer_set_limit(tb->timer, value, 0); + ptimer_set_count(tb->timer, value); + if ((tb->control & 1) && (value != 0)) { + ptimer_run(tb->timer, !(tb->control & 2)); + } + break; case 4: /* Counter. */ - if ((tb->control & 1) && tb->count) { - /* Cancel the previous timer. */ - timer_del(tb->timer); + /* Setting counter to 0 for the timer running in one-shot mode + * stops it without triggering the interrupt. */ + if (!(tb->control & 2) && (value == 0)) { + ptimer_stop(tb->timer); } - tb->count = value; - if (tb->control & 1) { - timerblock_reload(tb, 1); + /* While in the periodic mode, the running timer should restart + * counting without triggering the interrupt. Do it by setting + * the counter to the load value. */ + if (((tb->control & 3) == 3) && (value == 0)) { + ptimer_set_count(tb->timer, tb->load); + break; + } + ptimer_set_count(tb->timer, value); + /* Run the timer only if it is enabled and if mode is one-shot + * and counter != 0. */ + if ((tb->control & 1) && (value != 0)) { + ptimer_run(tb->timer, !(tb->control & 2)); } 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. */ + /* Timer mode switch requires ptimer to be stopped. */ + if (!(tb->control & 1) || ((old & 2) != (tb->control & 2))) { + ptimer_stop(tb->timer); + } + if (!(tb->control & 1)) { + break; + } + ptimer_set_period(tb->timer, timerblock_scale(tb)); + /* Just change the period if the timer state wasn't changed. */ + if ((old & 1) && ((old & 2) == (tb->control & 2))) { + break; + } + /* The timer must not run if mode is one-shot and counter = 0 + * or mode is periodic and counter = load = 0. */ + if (ptimer_get_count(tb->timer) == 0) { + if (!(tb->control & 2)) { + break; + } + if (tb->load == 0) { break; } + /* Avoid spurious interrupt trigger for the timer starting in + * the periodic mode and counter = 0. */ if (tb->control & 2) { - tb->count = tb->load; + ptimer_set_count(tb->timer, tb->load); } - timerblock_reload(tb, 1); - } else if (old & 1) { - /* Shutdown the timer. */ - timer_del(tb->timer); } + ptimer_run(tb->timer, !(tb->control & 2)); break; case 12: /* Interrupt status. */ tb->status &= ~value; @@ -184,13 +192,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_count(tb->timer, 0); } } @@ -235,7 +242,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 +253,24 @@ 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..93db61b 100644 --- a/include/hw/timer/arm_mptimer.h +++ b/include/hw/timer/arm_mptimer.h @@ -27,12 +27,10 @@ /* 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.6.0