PTimer fixes/features and ARM MPTimer conversion

PATCH Superseded Dmitry Osipenko 2 participants 21 messages Jan 9, 2016 – Jan 20, 2016

lore

Share & tools

lore

Dmitry Osipenko

author

2016-01-09 17:42 UTC #0 lore raw

v10 cover PTimer fixes/features and ARM MPTimer conversion

Changelog for ARM MPTimer QEMUTimer to ptimer conversion:

V2: Fixed changing periodic timer counter 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.

V5: Code cleanup, now depends on ptimer_set_limit() fix.

V6: No code change, added test to check ptimer_get_count() with corrected
.limit value.

V7: No change.

V8: No change.

V9: No change.

V10: Correctly handle cases when counter = load = 0 and prescaler != 0,
i.e. triggering interrupt in that case. Call ptimer_* only when certain MPTimer state was changed, like prescaler change. Factor out timerblock_set_count from timerblock_run and inline both. Tests updated.

ARM MPTimer tests: https://gist.github.com/digetx/dbd46109503b1a91941a

Patches for ptimer are introduced since V5 of "ARM MPTimer conversion".

Changelog for the ptimer patches:

V5: Only fixed ptimer_set_limit() for the disabled timer.

V6: As was pointed by Peter Maydell, there are other issues beyond
ptimer_set_limit(), so V6 supposed to cover all those issues.

V7: Added accidentally removed !use_icount check.
Added missed "else" statement.

V8: Adjust period instead of the limit and do it for periodic timer only
(.limit adjusting bug). Added patch/fix for freq/period change and ptimer_get_count() improvement.

V9: Don't do wrap around if counter == 0, otherwise polled periodic
timer won't ever return counter = 0.

V10: Addressed V8/9 review comments.
Adjust timer period based on delta instead of limit. Don't wrap around when in icount mode.
New patches: "on the fly" mode switch, silence error msg when
delta = load = 0, introduce ptimer_get_limit.

Dmitry Osipenko (7):
hw/ptimer: Fix issues caused by the adjusted timer limit value hw/ptimer: Perform tick and counter wrap around if timer already
expired
hw/ptimer: Update .delta on period/freq change hw/ptimer: Support "on the fly" timer mode switch hw/ptimer: Legalize running with delta = load = 0 hw/ptimer: Introduce ptimer_get_limit
arm_mptimer: Convert to use ptimer

 hw/core/ptimer.c               | 109 +++++++++++++++++++++++------------
 hw/timer/arm_mptimer.c         | 127 ++++++++++++++++++++---------------------
 include/hw/ptimer.h            |   1 +
 include/hw/timer/arm_mptimer.h |   5 +-
 4 files changed, 137 insertions(+), 105 deletions(-)

Signature · 1 line

2.6.4

Dmitry Osipenko

author

2016-01-09 17:42 UTC #1 lore raw

PATCH v10 1/7 hw/ptimer: Fix issues caused by the adjusted timer limit value

Multiple issues here related to the timer with a adjusted .limit value:

1) ptimer_get_count() returns incorrect counter value for the disabled timer after loading the counter with a small value, because adjusted limit value is used instead of the original.

For instance:
1) ptimer_stop(t)
2) ptimer_set_period(t, 1)
3) ptimer_set_limit(t, 0, 1)
4) ptimer_get_count(t) <-- would return 10000 instead of 0

2) ptimer_get_count() might return incorrect value for the timer running with a adjusted limit value.

For instance:
1) ptimer_stop(t)
2) ptimer_set_period(t, 1)
3) ptimer_set_limit(t, 10, 1)
4) ptimer_run(t)
5) ptimer_get_count(t) <-- might return value > 10

3) Neither ptimer_set_period() nor ptimer_set_freq() are adjusting the limit value, so it is still possible to make timer timeout value arbitrary small.

For instance:
1) ptimer_set_period(t, 10000)
2) ptimer_set_limit(t, 1, 0)
3) ptimer_set_period(t, 1) <-- bypass limit correction

Fix all of the above issues by adjusting timer period instead of the limit. Perform the adjustment for periodic timer only. Use the delta value instead of the limit to make decision whether adjustment is required, as limit could be altered while timer is running, resulting in incorrect value returned by ptimer_get_count.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 hw/core/ptimer.c | 51 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 20 deletions(-)

hw/core/ptimer.c +31 ~~-20~~

index edf077c..c3d31cb 100644
@@ -34,6 +34,9 @@ static void ptimer_trigger(ptimer_state *s)
 
 static void ptimer_reload(ptimer_state *s)
 {
+    uint32_t period_frac = s->period_frac;
+    uint64_t period = s->period;
+
     if (s->delta == 0) {
         ptimer_trigger(s);
         s->delta = s->limit;

@@ -44,10 +47,24 @@ static void ptimer_reload(ptimer_state *s)
         return;
     }
 
+    /*
+     * Artificially limit timeout rate to something
+     * achievable under QEMU.  Otherwise, QEMU spends all
+     * its time generating timer interrupts, and there
+     * is no forward progress.
+     * About ten microseconds is the fastest that really works
+     * on the current generation of host machines.
+     */
+
+    if ((s->enabled == 1) && !use_icount && (s->delta * period < 10000)) {
+        period = 10000 / s->delta;
+        period_frac = 0;
+    }
+
     s->last_event = s->next_event;
-    s->next_event = s->last_event + s->delta * s->period;
-    if (s->period_frac) {
-        s->next_event += ((int64_t)s->period_frac * s->delta) >> 32;
+    s->next_event = s->last_event + s->delta * period;
+    if (period_frac) {
+        s->next_event += ((int64_t)period_frac * s->delta) >> 32;
     }
     timer_mod(s->timer, s->next_event);
 }

@@ -82,6 +99,13 @@ uint64_t ptimer_get_count(ptimer_state *s)
             uint64_t div;
             int clz1, clz2;
             int shift;
+            uint32_t period_frac = s->period_frac;
+            uint64_t period = s->period;
+
+            if ((s->enabled == 1) && !use_icount && (s->delta * period < 10000)) {
+                period = 10000 / s->delta;
+                period_frac = 0;
+            }
 
             /* We need to divide time by period, where time is stored in
                rem (64-bit integer) and period is stored in period/period_frac

@@ -94,7 +118,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
             */
 
             rem = s->next_event - now;
-            div = s->period;
+            div = period;
 
             clz1 = clz64(rem);
             clz2 = clz64(div);

@@ -103,13 +127,13 @@ uint64_t ptimer_get_count(ptimer_state *s)
             rem <<= shift;
             div <<= shift;
             if (shift >= 32) {
-                div |= ((uint64_t)s->period_frac << (shift - 32));
+                div |= ((uint64_t)period_frac << (shift - 32));
             } else {
                 if (shift != 0)
-                    div |= (s->period_frac >> (32 - shift));
+                    div |= (period_frac >> (32 - shift));
                 /* Look at remaining bits of period_frac and round div up if 
                    necessary.  */
-                if ((uint32_t)(s->period_frac << shift))
+                if ((uint32_t)(period_frac << shift))
                     div += 1;
             }
             counter = rem / div;

@@ -181,19 +205,6 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)
    count = limit.  */
 void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
 {
-    /*
-     * Artificially limit timeout rate to something
-     * achievable under QEMU.  Otherwise, QEMU spends all
-     * its time generating timer interrupts, and there
-     * is no forward progress.
-     * About ten microseconds is the fastest that really works
-     * on the current generation of host machines.
-     */
-
-    if (!use_icount && limit * s->period < 10000 && s->period) {
-        limit = 10000 / s->period;
-    }
-
     s->limit = limit;
     if (reload)
         s->delta = limit;

Signature · 1 line

2.6.4

Peter Crosthwaite

Reviewed-by on v10 1/7

2016-01-10 00:28 UTC #2 lore raw

Re: [Qemu-arm] [PATCH v10 1/7] hw/ptimer: Fix issues caused by the adjusted timer limit value

Quoted 31 lines · Dmitry Osipenko

Multiple issues here related to the timer with a adjusted .limit value:
1) ptimer_get_count() returns incorrect counter value for the disabled timer after loading the counter with a small value, because adjusted limit value is used instead of the original.
For instance:
1) ptimer_stop(t)
2) ptimer_set_period(t, 1)
3) ptimer_set_limit(t, 0, 1)
4) ptimer_get_count(t) <-- would return 10000 instead of 0
2) ptimer_get_count() might return incorrect value for the timer running with a adjusted limit value.
For instance:
1) ptimer_stop(t)
2) ptimer_set_period(t, 1)
3) ptimer_set_limit(t, 10, 1)
4) ptimer_run(t)
5) ptimer_get_count(t) <-- might return value > 10
3) Neither ptimer_set_period() nor ptimer_set_freq() are adjusting the limit value, so it is still possible to make timer timeout value arbitrary small.
For instance:
1) ptimer_set_period(t, 10000)
2) ptimer_set_limit(t, 1, 0)
3) ptimer_set_period(t, 1) <-- bypass limit correction

Quoted 7 lines

Fix all of the above issues by adjusting timer period instead of the limit. Perform the adjustment for periodic timer only. Use the delta value instead of the limit to make decision whether adjustment is required, as limit could be altered while timer is running, resulting in incorrect value returned by ptimer_get_count.
Signed-off-by Dmitry Osipenko <[email protected]>

Reviewed-by Peter Crosthwaite <[email protected]>

Quoted 109 lines

---
 hw/core/ptimer.c | 51 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 20 deletions(-)

hw/core/ptimer.c +31 ~~-21~~

index edf077c..c3d31cb 100644
@@ -34,6 +34,9 @@ static void ptimer_trigger(ptimer_state *s)
 
 static void ptimer_reload(ptimer_state *s)
 {
+    uint32_t period_frac = s->period_frac;
+    uint64_t period = s->period;
+
     if (s->delta == 0) {
         ptimer_trigger(s);
         s->delta = s->limit;

@@ -44,10 +47,24 @@ static void ptimer_reload(ptimer_state *s)
         return;
     }
 
+    /*
+     * Artificially limit timeout rate to something
+     * achievable under QEMU.  Otherwise, QEMU spends all
+     * its time generating timer interrupts, and there
+     * is no forward progress.
+     * About ten microseconds is the fastest that really works
+     * on the current generation of host machines.
+     */
+
+    if ((s->enabled == 1) && !use_icount && (s->delta * period < 10000)) {
+        period = 10000 / s->delta;
+        period_frac = 0;
+    }
+
     s->last_event = s->next_event;
-    s->next_event = s->last_event + s->delta * s->period;
-    if (s->period_frac) {
-        s->next_event += ((int64_t)s->period_frac * s->delta) >> 32;
+    s->next_event = s->last_event + s->delta * period;
+    if (period_frac) {
+        s->next_event += ((int64_t)period_frac * s->delta) >> 32;
     }
     timer_mod(s->timer, s->next_event);
 }

@@ -82,6 +99,13 @@ uint64_t ptimer_get_count(ptimer_state *s)
             uint64_t div;
             int clz1, clz2;
             int shift;
+            uint32_t period_frac = s->period_frac;
+            uint64_t period = s->period;
+
+            if ((s->enabled == 1) && !use_icount && (s->delta * period < 10000)) {
+                period = 10000 / s->delta;
+                period_frac = 0;
+            }
 
             /* We need to divide time by period, where time is stored in
                rem (64-bit integer) and period is stored in period/period_frac

@@ -94,7 +118,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
             */
 
             rem = s->next_event - now;
-            div = s->period;
+            div = period;
 
             clz1 = clz64(rem);
             clz2 = clz64(div);

@@ -103,13 +127,13 @@ uint64_t ptimer_get_count(ptimer_state *s)
             rem <<= shift;
             div <<= shift;
             if (shift >= 32) {
-                div |= ((uint64_t)s->period_frac << (shift - 32));
+                div |= ((uint64_t)period_frac << (shift - 32));
             } else {
                 if (shift != 0)
-                    div |= (s->period_frac >> (32 - shift));
+                    div |= (period_frac >> (32 - shift));
                 /* Look at remaining bits of period_frac and round div up if
                    necessary.  */
-                if ((uint32_t)(s->period_frac << shift))
+                if ((uint32_t)(period_frac << shift))
                     div += 1;
             }
             counter = rem / div;

@@ -181,19 +205,6 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)
    count = limit.  */
 void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
 {
-    /*
-     * Artificially limit timeout rate to something
-     * achievable under QEMU.  Otherwise, QEMU spends all
-     * its time generating timer interrupts, and there
-     * is no forward progress.
-     * About ten microseconds is the fastest that really works
-     * on the current generation of host machines.
-     */
-
-    if (!use_icount && limit * s->period < 10000 && s->period) {
-        limit = 10000 / s->period;
-    }
-
     s->limit = limit;
     if (reload)
         s->delta = limit;
--

2.6.4

Dmitry Osipenko

author

2016-01-09 17:42 UTC #3 lore raw

PATCH v10 2/7 hw/ptimer: Perform tick and counter wrap around if timer already expired

In reply to Dmitry Osipenko

ptimer_get_count() might be called while QEMU timer already been expired. In that case ptimer would return counter = 0, which might be undesirable in case of polled timer. Do counter wrap around for periodic timer to keep it distributed. In order to achieve more accurate emulation behaviour of certain hardware, don't perform wrap around when in icount mode and return counter = 0 in that case (that doesn't affect polled counter distribution).

In addition, there is no reason to keep expired timer tick deferred, so just perform the tick from ptimer_get_count().

Signed-off-by Dmitry Osipenko <[email protected]>

---
 hw/core/ptimer.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

hw/core/ptimer.c +19 -7

index c3d31cb..cf329eb 100644
@@ -83,14 +83,17 @@ static void ptimer_tick(void *opaque)
 
 uint64_t ptimer_get_count(ptimer_state *s)
 {
-    int64_t now;
+    int enabled = s->enabled;
     uint64_t counter;
 
-    if (s->enabled) {
-        now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    if (enabled) {
+        int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        int64_t next = s->next_event;
+        int expired = (now - next >= 0);
+        int oneshot = (enabled == 2);
+
         /* Figure out the current counter value.  */
-        if (now - s->next_event > 0
-            || s->period == 0) {
+        if (s->period == 0 || (expired && (use_icount || oneshot))) {
             /* Prevent timer underflowing if it should already have
                triggered.  */
             counter = 0;

@@ -102,7 +105,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
             uint32_t period_frac = s->period_frac;
             uint64_t period = s->period;
 
-            if ((s->enabled == 1) && !use_icount && (s->delta * period < 10000)) {
+            if (!oneshot && !use_icount && (s->delta * period < 10000)) {
                 period = 10000 / s->delta;
                 period_frac = 0;
             }

@@ -117,7 +120,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
                backwards.
             */
 
-            rem = s->next_event - now;
+            rem = expired ? now - next : next - now;
             div = period;
 
             clz1 = clz64(rem);

@@ -137,6 +140,15 @@ uint64_t ptimer_get_count(ptimer_state *s)
                     div += 1;
             }
             counter = rem / div;
+
+            if (expired && (counter != 0)) {
+                /* Wrap around periodic counter.  */
+                counter = s->limit - counter % s->limit;
+            }
+        }
+
+        if (expired) {
+            ptimer_tick(s);
         }
     } else {
         counter = s->delta;

Signature · 1 line

2.6.4

Peter Crosthwaite

Reviewed-by on v10 2/7

2016-01-10 00:44 UTC #4 lore raw

Re: [Qemu-arm] [PATCH v10 2/7] hw/ptimer: Perform tick and counter wrap around if timer already expired

Quoted 20 lines · Dmitry Osipenko

ptimer_get_count() might be called while QEMU timer already been expired. In that case ptimer would return counter = 0, which might be undesirable in case of polled timer. Do counter wrap around for periodic timer to keep it distributed. In order to achieve more accurate emulation behaviour of certain hardware, don't perform wrap around when in icount mode and return counter = 0 in that case (that doesn't affect polled counter distribution).
In addition, there is no reason to keep expired timer tick deferred, so just perform the tick from ptimer_get_count().
Signed-off-by Dmitry Osipenko <[email protected]>
---
 hw/core/ptimer.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)
hw/core/ptimer.c +0 -0
index c3d31cb..cf329eb 100644

hw/core/ptimer.c static void ptimer_tick(void *opaque) · line 86

Quoted 5 lines

hw/core/ptimer.c +1 -1

@@ -83,14 +83,17 @@ static void ptimer_tick(void *opaque)
 
 uint64_t ptimer_get_count(ptimer_state *s)
 {
-    int64_t now;
+    int enabled = s->enabled;

You haven't added any additional usages of s->enabled to really warrant the new local variable, I think it should just stay as s->enabled to avoid churn.

hw/core/ptimer.c static void ptimer_tick(void *opaque) · line 92

Quoted 8 lines

hw/core/ptimer.c +4 -2

     uint64_t counter;
 
-    if (s->enabled) {
-        now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    if (enabled) {
+        int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        int64_t next = s->next_event;
+        int expired = (now - next >= 0);

bool.

hw/core/ptimer.c static void ptimer_tick(void *opaque) · line 93

Quoted 1 line

hw/core/ptimer.c +1 -0

+        int oneshot = (enabled == 2);

bool.

Otherwise:

Reviewed-by Peter Crosthwaite <[email protected]>

Quoted 45 lines

hw/core/ptimer.c +13 -5

+
         /* Figure out the current counter value.  */
-        if (now - s->next_event > 0
-            || s->period == 0) {
+        if (s->period == 0 || (expired && (use_icount || oneshot))) {
             /* Prevent timer underflowing if it should already have
                triggered.  */
             counter = 0;

@@ -102,7 +105,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
             uint32_t period_frac = s->period_frac;
             uint64_t period = s->period;
 
-            if ((s->enabled == 1) && !use_icount && (s->delta * period < 10000)) {
+            if (!oneshot && !use_icount && (s->delta * period < 10000)) {
                 period = 10000 / s->delta;
                 period_frac = 0;
             }

@@ -117,7 +120,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
                backwards.
             */
 
-            rem = s->next_event - now;
+            rem = expired ? now - next : next - now;
             div = period;
 
             clz1 = clz64(rem);

@@ -137,6 +140,15 @@ uint64_t ptimer_get_count(ptimer_state *s)
                     div += 1;
             }
             counter = rem / div;
+
+            if (expired && (counter != 0)) {
+                /* Wrap around periodic counter.  */
+                counter = s->limit - counter % s->limit;
+            }
+        }
+
+        if (expired) {
+            ptimer_tick(s);
         }
     } else {
         counter = s->delta;
--

2.6.4

Dmitry Osipenko

author

2016-01-10 14:10 UTC #5 lore raw

Re: [Qemu-arm] [PATCH v10 2/7] hw/ptimer: Perform tick and counter wrap around if timer already expired

10.01.2016 03:44, Peter Crosthwaite пишет:

Quoted 22 lines · Dmitry Osipenko

Quoted 21 lines
ptimer_get_count() might be called while QEMU timer already been expired. In that case ptimer would return counter = 0, which might be undesirable in case of polled timer. Do counter wrap around for periodic timer to keep it distributed. In order to achieve more accurate emulation behaviour of certain hardware, don't perform wrap around when in icount mode and return counter = 0 in that case (that doesn't affect polled counter distribution).
In addition, there is no reason to keep expired timer tick deferred, so just perform the tick from ptimer_get_count().
Signed-off-by Dmitry Osipenko <[email protected]>
---
  hw/core/ptimer.c | 26 +++++++++++++++++++-------
  1 file changed, 19 insertions(+), 7 deletions(-)
hw/core/ptimer.c +0 -0
index c3d31cb..cf329eb 100644
@@ -83,14 +83,17 @@ static void ptimer_tick(void *opaque)
 

Quoted 9 lines

hw/core/ptimer.c static void ptimer_tick(void *opaque) · line 86
Quoted 4 lines
hw/core/ptimer.c +1 -1
  uint64_t ptimer_get_count(ptimer_state *s)
  {
-    int64_t now;
+    int enabled = s->enabled;
You haven't added any additional usages of s->enabled to really warrant the new local variable, I think it should just stay as s->enabled to avoid churn.

Compiler doesn't know that ptimer struct is supposed to be thread-safe there and infers memory load instructions rather than use pure cpu registers. Local variable would help to produce somewhat more rational. However, given how optimized and complex modern cpu's, I'm not really sure that it would bring any benefit and agree that it might not worth churning. Thanks for review!

Signature · 1 line

Dmitry

Dmitry Osipenko

author

2016-01-20 17:04 UTC #6 lore raw

Re: [Qemu-arm] [PATCH v10 2/7] hw/ptimer: Perform tick and counter wrap around if timer already expired

In reply to Peter Crosthwaite

Hi Peter,

10.01.2016 03:44, Peter Crosthwaite пишет:

Quoted 1 line · Dmitry Osipenko

[cut]

Quoted 3 lines

Quoted 3 lines
In addition, there is no reason to keep expired timer tick deferred, so just perform the tick from ptimer_get_count().

[cut]

I noticed an issue here... The problem is that device reset invokes ptimer_stop() that invokes ptimer_get_count() that might cause the tick after reset, i.e. bogus bh would be invoked after QEMU reset. I have reproduced that issue.

The solution might be to introduce ptimer_reset() that would stop QEMU timer and reset delta/load/period without invoking ptimer_get_count. And of course all devices should be updated to use new ptimer_reset() prior to "Perform tick and counter wrap around if timer already expired" patch, but that's not an issue I suppose. Please let me know if you have any objections, I'm leaning to do it in V11.

void ptimer_reset(ptimer_state *s)

{
    timer_del(s->timer);
    s->enabled = 0;
    s->period_frac = 0;
    s->period = 0;
    s->delta = 0;
    s->limit = 0;
}

Signature · 1 line

Dmitry

Dmitry Osipenko

author

2016-01-20 17:22 UTC #7 lore raw

Re: [Qemu-devel] [PATCH v10 2/7] hw/ptimer: Perform tick and counter wrap around if timer already expired

20.01.2016 20:03, Dmitry Osipenko пишет:

Quoted 23 lines

Hi Peter,
10.01.2016 03:44, Peter Crosthwaite пишет:
Quoted 1 line · Dmitry Osipenko
[cut]
Quoted 3 lines
Quoted 3 lines
In addition, there is no reason to keep expired timer tick deferred, so just perform the tick from ptimer_get_count().
[cut]
I noticed an issue here... The problem is that device reset invokes ptimer_stop() that invokes ptimer_get_count() that might cause the tick after reset, i.e. bogus bh would be invoked after QEMU reset. I have reproduced that issue.
The solution might be to introduce ptimer_reset() that would stop QEMU timer and reset delta/load/period without invoking ptimer_get_count. And of course all devices should be updated to use new ptimer_reset() prior to "Perform tick and counter wrap around if timer already expired" patch, but that's not an issue I suppose. Please let me know if you have any objections, I'm leaning to do it in V11.
void ptimer_reset(ptimer_state *s)

Quoted 9 lines

{
     timer_del(s->timer);
     s->enabled = 0;
     s->period_frac = 0;
     s->period = 0;
     s->delta = 0;
     s->limit = 0;
}

However, ptimer_stop() still would be broken. So maybe it's better to keep tick deferred.

Signature · 1 line

Dmitry

Dmitry Osipenko

author

2016-01-09 17:42 UTC #8 lore raw

PATCH v10 3/7 hw/ptimer: Update .delta on period/freq change

In reply to Dmitry Osipenko

Delta value must be updated on period/freq change, otherwise running timer would be restarted (counter reloaded with old delta). Only m68k/mcf520x and arm/arm_timer devices are currently doing freq change correctly, i.e. stopping the timer. Perform delta update to fix affected devices and eliminate potential further mistakes.

Signed-off-by Dmitry Osipenko <[email protected]>

Reviewed-by Peter Crosthwaite <[email protected]>

---
 hw/core/ptimer.c | 2 ++
 1 file changed, 2 insertions(+)

hw/core/ptimer.c +2 -0

index cf329eb..0a54212 100644
@@ -194,6 +194,7 @@ void ptimer_stop(ptimer_state *s)
 /* Set counter increment interval in nanoseconds.  */
 void ptimer_set_period(ptimer_state *s, int64_t period)
 {
+    s->delta = ptimer_get_count(s);
     s->period = period;
     s->period_frac = 0;
     if (s->enabled) {

@@ -205,6 +206,7 @@ void ptimer_set_period(ptimer_state *s, int64_t period)
 /* Set counter frequency in Hz.  */
 void ptimer_set_freq(ptimer_state *s, uint32_t freq)
 {
+    s->delta = ptimer_get_count(s);
     s->period = 1000000000ll / freq;
     s->period_frac = (1000000000ll << 32) / freq;
     if (s->enabled) {

Signature · 1 line

2.6.4

Dmitry Osipenko

author

2016-01-09 17:42 UTC #9 lore raw

PATCH v10 4/7 hw/ptimer: Support "on the fly" timer mode switch

In reply to Dmitry Osipenko

Allow to switch between periodic <-> oneshot modes while timer is running.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 hw/core/ptimer.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

hw/core/ptimer.c +6 -6

index 0a54212..6960738 100644
@@ -167,16 +167,16 @@ void ptimer_set_count(ptimer_state *s, uint64_t count)
 
 void ptimer_run(ptimer_state *s, int oneshot)
 {
-    if (s->enabled) {
-        return;
-    }
-    if (s->period == 0) {
+    int was_disabled = !s->enabled;
+    if (was_disabled && s->period == 0) {
         fprintf(stderr, "Timer with period zero, disabling\n");
         return;
     }
     s->enabled = oneshot ? 2 : 1;
-    s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    ptimer_reload(s);
+    if (was_disabled) {
+        s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        ptimer_reload(s);
+    }
 }
 
 /* Pause a timer.  Note that this may cause it to "lose" time, even if it

Signature · 1 line

2.6.4

Peter Crosthwaite

Reviewed-by on v10 4/7

2016-01-10 00:50 UTC #10 lore raw

Re: [Qemu-arm] [PATCH v10 4/7] hw/ptimer: Support "on the fly" timer mode switch

Quoted 2 lines · Dmitry Osipenko

Allow to switch between periodic <-> oneshot modes while timer is running.

"Allow switching"

Quoted 10 lines

Signed-off-by Dmitry Osipenko <[email protected]>
---
 hw/core/ptimer.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
hw/core/ptimer.c +0 -0
index 0a54212..6960738 100644

hw/core/ptimer.c void ptimer_set_count(ptimer_state *s, uint64_t count) · line 170

Quoted 8 lines

hw/core/ptimer.c +1 -4

@@ -167,16 +167,16 @@ void ptimer_set_count(ptimer_state *s, uint64_t count)
 
 void ptimer_run(ptimer_state *s, int oneshot)
 {
-    if (s->enabled) {
-        return;
-    }
-    if (s->period == 0) {
+    int was_disabled = !s->enabled;

bool

blank line here.

Otherwise:

Reviewed-by Peter Crosthwaite <[email protected]>

Regards,
Peter

Quoted 17 lines

hw/core/ptimer.c +5 -3

+    if (was_disabled && s->period == 0) {
         fprintf(stderr, "Timer with period zero, disabling\n");
         return;
     }
     s->enabled = oneshot ? 2 : 1;
-    s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    ptimer_reload(s);
+    if (was_disabled) {
+        s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        ptimer_reload(s);
+    }
 }
 
 /* Pause a timer.  Note that this may cause it to "lose" time, even if it
--

2.6.4

Dmitry Osipenko

author

2016-01-09 17:42 UTC #11 lore raw

PATCH v10 5/7 hw/ptimer: Legalize running with delta = load = 0

In reply to Dmitry Osipenko

Currently ptimer would print error message and clear enable flag for an arming timer that has delta = load = 0. That actually could be a valid case for some hardware, like instant IRQ trigger for oneshot timer or continuous in periodic mode. Support those cases by printing error message only when period = 0.

In addition, don't load one-shot timer when delta = 0 and actually stop the timer by timer_del().

Signed-off-by Dmitry Osipenko <[email protected]>

---
 hw/core/ptimer.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

hw/core/ptimer.c +14 -7

index 6960738..42e44f9 100644
@@ -36,13 +36,20 @@ static void ptimer_reload(ptimer_state *s)
 {
     uint32_t period_frac = s->period_frac;
     uint64_t period = s->period;
+    int periodic = (s->enabled == 1);
 
-    if (s->delta == 0) {
+    if (s->delta == 0 && period != 0) {
         ptimer_trigger(s);
-        s->delta = s->limit;
+        if (periodic) {
+            s->delta = s->limit;
+        }
     }
-    if (s->delta == 0 || s->period == 0) {
-        fprintf(stderr, "Timer with period zero, disabling\n");
+    if (s->delta == 0 || period == 0) {
+        if (period == 0) {
+            fprintf(stderr, "Timer with period zero, disabling\n");
+            s->delta = 0;
+        }
+        timer_del(s->timer);
         s->enabled = 0;
         return;
     }

@@ -56,7 +63,7 @@ static void ptimer_reload(ptimer_state *s)
      * on the current generation of host machines.
      */
 
-    if ((s->enabled == 1) && !use_icount && (s->delta * period < 10000)) {
+    if (periodic && !use_icount && (s->delta * period < 10000)) {
         period = 10000 / s->delta;
         period_frac = 0;
     }

@@ -86,14 +93,14 @@ uint64_t ptimer_get_count(ptimer_state *s)
     int enabled = s->enabled;
     uint64_t counter;
 
-    if (enabled) {
+    if (enabled && s->delta != 0) {
         int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
         int64_t next = s->next_event;
         int expired = (now - next >= 0);
         int oneshot = (enabled == 2);
 
         /* Figure out the current counter value.  */
-        if (s->period == 0 || (expired && (use_icount || oneshot))) {
+        if (expired && (use_icount || oneshot)) {
             /* Prevent timer underflowing if it should already have
                triggered.  */
             counter = 0;

Signature · 1 line

2.6.4

Peter Crosthwaite

2016-01-12 03:59 UTC #12 lore raw

Re: [Qemu-arm] [PATCH v10 5/7] hw/ptimer: Legalize running with delta = load = 0

Quoted 6 lines · Dmitry Osipenko

Currently ptimer would print error message and clear enable flag for an arming timer that has delta = load = 0. That actually could be a valid case for some hardware, like instant IRQ trigger for oneshot timer or continuous in periodic mode. Support those cases by printing error message only when period = 0.

Isn't the continuous-periodic the same as period = 0, so if we were to really support this, there should be no error message. This would simplify as we can remove the conditionals of 0 period completely and rely only on the too-fast clamps you add in previous patches.

Regards,
Peter

Quoted 66 lines

In addition, don't load one-shot timer when delta = 0 and actually stop the timer by timer_del().

Signed-off-by Dmitry Osipenko <[email protected]>

---
 hw/core/ptimer.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

hw/core/ptimer.c +14 -7

index 6960738..42e44f9 100644
@@ -36,13 +36,20 @@ static void ptimer_reload(ptimer_state *s)
 {
     uint32_t period_frac = s->period_frac;
     uint64_t period = s->period;
+    int periodic = (s->enabled == 1);
 
-    if (s->delta == 0) {
+    if (s->delta == 0 && period != 0) {
         ptimer_trigger(s);
-        s->delta = s->limit;
+        if (periodic) {
+            s->delta = s->limit;
+        }
     }
-    if (s->delta == 0 || s->period == 0) {
-        fprintf(stderr, "Timer with period zero, disabling\n");
+    if (s->delta == 0 || period == 0) {
+        if (period == 0) {
+            fprintf(stderr, "Timer with period zero, disabling\n");
+            s->delta = 0;
+        }
+        timer_del(s->timer);
         s->enabled = 0;
         return;
     }

@@ -56,7 +63,7 @@ static void ptimer_reload(ptimer_state *s)
      * on the current generation of host machines.
      */
 
-    if ((s->enabled == 1) && !use_icount && (s->delta * period < 10000)) {
+    if (periodic && !use_icount && (s->delta * period < 10000)) {
         period = 10000 / s->delta;
         period_frac = 0;
     }

@@ -86,14 +93,14 @@ uint64_t ptimer_get_count(ptimer_state *s)
     int enabled = s->enabled;
     uint64_t counter;
 
-    if (enabled) {
+    if (enabled && s->delta != 0) {
         int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
         int64_t next = s->next_event;
         int expired = (now - next >= 0);
         int oneshot = (enabled == 2);
 
         /* Figure out the current counter value.  */
-        if (s->period == 0 || (expired && (use_icount || oneshot))) {
+        if (expired && (use_icount || oneshot)) {
             /* Prevent timer underflowing if it should already have
                triggered.  */
             counter = 0;

Signature · 1 line

2.6.4

Dmitry Osipenko

author

2016-01-12 18:13 UTC #13 lore raw

Re: [Qemu-arm] [PATCH v10 5/7] hw/ptimer: Legalize running with delta = load = 0

12.01.2016 06:58, Peter Crosthwaite пишет:

Quoted 8 lines · Dmitry Osipenko

Quoted 6 lines
Currently ptimer would print error message and clear enable flag for an arming timer that has delta = load = 0. That actually could be a valid case for some hardware, like instant IRQ trigger for oneshot timer or continuous in periodic mode. Support those cases by printing error message only when period = 0.

Quoted 5 lines

Isn't the continuous-periodic the same as period = 0, so if we were to really support this, there should be no error message. This would simplify as we can remove the conditionals of 0 period completely and rely only on the too-fast clamps you add in previous patches.

I don't think that clamping is needed. Instead doing the ptimer_tick might be necessary, so ptimer user could handle that case on it own.

BTW, that printf isn't quite reliable, hw_error or other way of execution abort should been used instead.

Thanks for the comment.

Signature · 1 line

Dmitry

Dmitry Osipenko

author

2016-01-15 18:10 UTC #14 lore raw

Re: [Qemu-arm] [PATCH v10 5/7] hw/ptimer: Legalize running with delta = load = 0

12.01.2016 21:12, Dmitry Osipenko пишет:

Quoted 15 lines

12.01.2016 06:58, Peter Crosthwaite пишет:
Quoted 8 lines · Dmitry Osipenko
Quoted 6 lines
Currently ptimer would print error message and clear enable flag for an arming timer that has delta = load = 0. That actually could be a valid case for some hardware, like instant IRQ trigger for oneshot timer or continuous in periodic mode. Support those cases by printing error message only when period = 0.
Quoted 5 lines
Isn't the continuous-periodic the same as period = 0, so if we were to really support this, there should be no error message. This would simplify as we can remove the conditionals of 0 period completely and rely only on the too-fast clamps you add in previous patches.

Quoted 8 lines

I don't think that clamping is needed. Instead doing the ptimer_tick might be necessary, so ptimer user could handle that case on it own.
BTW, that printf isn't quite reliable, hw_error or other way of execution abort should been used instead.
Thanks for the comment.

Looking more at it, I think we should keep period = 0 forbidden. So it's treated as undefined behaviour and ptimer user should take care of it. If we'd want to handle period = 0 within ptimer, then we should also handle freq = 0, which implies potential ptimer VMSD version bump. Also it is uncertain what default behaviour should be chosen for period = 0, my guess is that pausing (_not_disabling_) the timer (like in freq = 0 case) should be more common among various hardware. If you have any thoughts on it, please let me know.

Signature · 1 line

Dmitry

Dmitry Osipenko

author

2016-01-20 00:51 UTC #15 lore raw

Re: [Qemu-arm] [PATCH v10 5/7] hw/ptimer: Legalize running with delta = load = 0

In reply to Dmitry Osipenko

12.01.2016 21:12, Dmitry Osipenko пишет:

Quoted 15 lines

12.01.2016 06:58, Peter Crosthwaite пишет:
Quoted 8 lines · Dmitry Osipenko
Quoted 6 lines
Currently ptimer would print error message and clear enable flag for an arming timer that has delta = load = 0. That actually could be a valid case for some hardware, like instant IRQ trigger for oneshot timer or continuous in periodic mode. Support those cases by printing error message only when period = 0.
Quoted 5 lines
Isn't the continuous-periodic the same as period = 0, so if we were to really support this, there should be no error message. This would simplify as we can remove the conditionals of 0 period completely and rely only on the too-fast clamps you add in previous patches.

Quoted 8 lines

I don't think that clamping is needed. Instead doing the ptimer_tick might be necessary, so ptimer user could handle that case on it own.
BTW, that printf isn't quite reliable, hw_error or other way of execution abort should been used instead.
Thanks for the comment.

Just tried with the clamping. It works, but what clamping should be chosen for icount mode? delta * period = 1ns is damn slow.

However, I'm still not sure what benefit has clamping over unclearable IRQ... only disadvantages in form of slowdown in icount mode, +2 lines of ptimer code and possible misbehaviour of blazing fast host machine in non-icount mode.

Signature · 1 line

Dmitry

Dmitry Osipenko

author

2016-01-09 17:42 UTC #16 lore raw

PATCH v10 6/7 hw/ptimer: Introduce ptimer_get_limit

In reply to Dmitry Osipenko

Currently ptimer users are used to store copy of the limit value, because ptimer doesn't provide facility to retrieve the limit. Let's provide it.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 hw/core/ptimer.c    | 5 +++++
 include/hw/ptimer.h | 1 +
 2 files changed, 6 insertions(+)

hw/core/ptimer.c +5 -0

index 42e44f9..0201d1b 100644
@@ -235,6 +235,11 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
     }
 }
 
+uint64_t ptimer_get_limit(ptimer_state *s)
+{
+    return s->limit;
+}
+
 const VMStateDescription vmstate_ptimer = {
     .name = "ptimer",
     .version_id = 1,

include/hw/ptimer.h +1 -0

index 8ebacbb..e397db5 100644
@@ -19,6 +19,7 @@ typedef void (*ptimer_cb)(void *opaque);
 ptimer_state *ptimer_init(QEMUBH *bh);
 void ptimer_set_period(ptimer_state *s, int64_t period);
 void ptimer_set_freq(ptimer_state *s, uint32_t freq);
+uint64_t ptimer_get_limit(ptimer_state *s);
 void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload);
 uint64_t ptimer_get_count(ptimer_state *s);
 void ptimer_set_count(ptimer_state *s, uint64_t count);

Signature · 1 line

2.6.4

Peter Crosthwaite

Reviewed-by on v10 6/7

2016-01-12 03:59 UTC #17 lore raw

Re: [Qemu-devel] [PATCH v10 6/7] hw/ptimer: Introduce ptimer_get_limit

Quoted 4 lines · Dmitry Osipenko

Currently ptimer users are used to store copy of the limit value, because ptimer doesn't provide facility to retrieve the limit. Let's provide it.
Signed-off-by Dmitry Osipenko <[email protected]>

Fair call. One less piece of duped state for the VMSDs.

Reviewed-by Peter Crosthwaite <[email protected]>

Regards,
Peter

Quoted 36 lines

---
 hw/core/ptimer.c    | 5 +++++
 include/hw/ptimer.h | 1 +
 2 files changed, 6 insertions(+)

hw/core/ptimer.c +5 -0

index 42e44f9..0201d1b 100644
@@ -235,6 +235,11 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
     }
 }
 
+uint64_t ptimer_get_limit(ptimer_state *s)
+{
+    return s->limit;
+}
+
 const VMStateDescription vmstate_ptimer = {
     .name = "ptimer",
     .version_id = 1,

include/hw/ptimer.h +1 -0

index 8ebacbb..e397db5 100644
@@ -19,6 +19,7 @@ typedef void (*ptimer_cb)(void *opaque);
 ptimer_state *ptimer_init(QEMUBH *bh);
 void ptimer_set_period(ptimer_state *s, int64_t period);
 void ptimer_set_freq(ptimer_state *s, uint32_t freq);
+uint64_t ptimer_get_limit(ptimer_state *s);
 void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload);
 uint64_t ptimer_get_count(ptimer_state *s);
 void ptimer_set_count(ptimer_state *s, uint64_t count);

Signature · 1 line

2.6.4

Dmitry Osipenko

author

2016-01-09 17:42 UTC #18 lore raw

PATCH v10 7/7 arm_mptimer: Convert to use ptimer

In reply to Dmitry Osipenko

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 <[email protected]>

---
 hw/timer/arm_mptimer.c         | 127 ++++++++++++++++++++---------------------
 include/hw/timer/arm_mptimer.h |   5 +-
 2 files changed, 63 insertions(+), 69 deletions(-)

hw/timer/arm_mptimer.c +62 ~~-65~~

index 3e59c2a..5568300 100644
@@ -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,33 @@ 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(TimerBlock *tb, 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.  */
+    if ((control & 3) == 3 && (*count == 0) && (control & 0xff00) == 0) {
+        *count = ptimer_get_limit(tb->timer);
     }
-    if (restart) {
-        tb->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    ptimer_set_count(tb->timer, *count);
+}
+
+static inline void timerblock_run(TimerBlock *tb, uint32_t control, int cond)
+{
+    if (cond) {
+        ptimer_run(tb->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 +77,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 +95,48 @@ 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, control, (control & 1));
+        break;
+    case 4: /* Counter.  */
+        /* Setting counter to 0 stops the one-shot timer if prescaler == 0.  */
+        if ((control & 3) == 1 && (value == 0) && (control & 0xff00) == 0) {
+            ptimer_stop(tb->timer);
+            control &= ~1;
         }
+        timerblock_set_count(tb, control, &value);
+        timerblock_run(tb, 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, value, &count);
             }
-            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, 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 +184,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 +234,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 +245,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()
     }
 };

include/hw/timer/arm_mptimer.h +1 -4

index b34cba0..c46d8d2 100644
@@ -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;

Signature · 1 line

2.6.4

Dmitry Osipenko

author

2016-01-10 18:06 UTC #19 lore raw

Re: [Qemu-arm] [PATCH v10 7/7] arm_mptimer: Convert to use ptimer

I missed case where periodic timer should stop in the following case:

load = 0, counter != 0 -> run -> set counter = 0 -> should stop

Test added. Will fix it in V11.

Signature · 1 line

Dmitry

Dmitry Osipenko

author

2016-01-10 18:13 UTC #20 lore raw

Re: [Qemu-arm] [PATCH v10 7/7] arm_mptimer: Convert to use ptimer

10.01.2016 21:05, Dmitry Osipenko пишет:

Quoted 6 lines

I missed case where periodic timer should stop in the following case:
load = 0, counter != 0 -> run -> set counter = 0 -> should stop
Test added. Will fix it in V11.

Forgot to mention that prescaler must be 0 in that case. Prescaler != 0 would result in unclearable IRQ while periodic timer is enabled and counter = load = 0.

Signature · 1 line

Dmitry