[Qemu-devel] [RFC v1 2/2] timer: arm_mp: consolidate control and counter write logic

[Peter Crosthwaite]

Writing to any of the load, counter or control registers can require a
reload of the timer. Currently load and counter share a code path, but
the control logic is separate. Consolidate them by reducing the switch
to only sync the timer state. For load/counter this just means setting
tb->count to the new value. For control, this means setting tb->count
to the current value. Then outside the switch, any old timers are
discarded, and if the timer is (still is, or has just become) enabled,
setup a new one.

A fast escape path is added to control writes. If it is detected that
the timer was, and still is running without change of prescaler, don't
do the restart. This avoid an un-needed restart that could potentially
cause timer rounding errors.

For further consolidation, move the auto-load refresh logic to
timerblock_reload.

This change fixes two bugs and implements a missing feature.

Previously a running timer could not be stopped, the commonifying of
the timer_del call now means clearing the enable bit in the control
register now correctly stops a running timer.

There was a bug where if starting a timer in periodic mode after
one-shot expiration, the timer would not restart. This was because of
a bad conditional for the old do-nothing fast path.

This implements pause and resumption of the timer. Clearing the enable
bit, and the setting it again later will cause the timer to pick up
where it left off. The paused value of the timer can be read by the
guest. Another use of this stop and restart feature, is it also now
models a change of prescaler for a running timer.

Signed-off-by: Peter Crosthwaite <address@hidden>
---
 hw/timer/arm_mptimer.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 04dfb63..69899cf 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -50,7 +50,11 @@ static inline uint32_t timerblock_scale(TimerBlock *tb)
 static void timerblock_reload(TimerBlock *tb, int restart)
 {
     if (tb->count == 0) {
-        return;
+        if (tb->control & 2) {
+            tb->count = tb->load;
+        } else {
+            return;
+        }
     }
     if (restart) {
         tb->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
@@ -63,12 +67,8 @@ 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;
-    }
+    tb->count = 0;
+    timerblock_reload(tb, 0);
     timerblock_update_irq(tb);
 }
 
@@ -113,33 +113,37 @@ static void timerblock_write(void *opaque, hwaddr addr,
     TimerBlock *tb = (TimerBlock *)opaque;
     int64_t old;
     switch (addr) {
+    /* Breaking from this switch implies that timer needs to be refreshed.
+     * Operations that do not affect the running timer must return directly
+     * to avoid a spurious reload of the timer.
+     */
     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);
-        }
         tb->count = value;
-        if (tb->control & 1) {
-            timerblock_reload(tb, 1);
-        }
         break;
     case 8: /* Control.  */
         old = tb->control;
         tb->control = value;
-        if (((old & 1) == 0) && (value & 1)) {
-            if (tb->count == 0 && (tb->control & 2)) {
-                tb->count = tb->load;
-            }
-            timerblock_reload(tb, 1);
+        if ((value & 1) && (old & 1) && tb->count != 0 &&
+            !extract64(value ^ old, 8, 8)) {
+            /* Timer was running, and still is, without prescale change */
+            return;
         }
+        timerblock_sync(tb);
         break;
     case 12: /* Interrupt status.  */
         tb->status &= ~value;
         timerblock_update_irq(tb);
-        break;
+        return;
+    }
+
+    /* Cancel any previous timer.  */
+    timer_del(tb->timer);
+
+    if (tb->control & 1) {
+        timerblock_reload(tb, 1);
     }
 }
 
-- 
1.9.1