PTimer fixes/features and ARM MPTimer conversion
v11 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.
V11: Fixed missed periodic timer stopping on setting counter => 0 with
load = 0 and prescaler = 0.
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.
V11: Dropped timer tick from "Perform tick and counter wrap around if
timer already expired" patch since it would cause bogus tick after QEMU been reset if ptimer was stopped and it's QEMUtimer expired during reset.
Patch "Legalize running with delta = load = 0" now explicitly forbids period = 0.
Dmitry Osipenko (7):
hw/ptimer: Fix issues caused by the adjusted timer limit value hw/ptimer: Perform 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: Introduce ptimer_get_limit
hw/ptimer: Legalize running with delta = load = 0 and abort on period
= 0
arm_mptimer: Convert to use ptimer
hw/core/ptimer.c | 111 ++++++++++++++++++++-------------- hw/timer/arm_mptimer.c | 131 +++++++++++++++++++++-------------------- include/hw/ptimer.h | 1 + include/hw/timer/arm_mptimer.h | 5 +- 4 files changed, 135 insertions(+), 113 deletions(-)
Signature · 1 line
2.7.0
PATCH v11 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.
--- hw/core/ptimer.c | 51 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 20 deletions(-)
hw/core/ptimer.c +31 -20
@@ -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 ) && (s->delta * period <10000 ) && !use_icount) { + 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.7.0
PATCH v11 2/7 hw/ptimer: Perform counter wrap around if timer already expired
In reply to Dmitry Osipenkoptimer_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).
--- hw/core/ptimer.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
hw/core/ptimer.c +13 -6
@@ -83,14 +83,16 @@ static void ptimer_tick(void *opaque)uint64_t ptimer_get_count (ptimer_state *s) { -int64_t now;uint64_t counter;if (s->enabled) { - now =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ); +int64_t now =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ); +int64_t next = s->next_event; +bool expired = (now - next >=0 ); +bool oneshot = (s->enabled ==2 ); +/* Figure out the current counter value. */ -if (now - s->next_event >0 - || s->period ==0 ) { +if (s->period ==0 || (expired && (oneshot || use_icount))) {/* Prevent timer underflowing if it should already have triggered. */ counter =0 ;
@@ -102,7 +104,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 && (s->delta * period <10000 ) && !use_icount) { period =10000 / s->delta; period_frac =0 ; }
@@ -117,7 +119,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 +139,11 @@ 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; + } } }else { counter = s->delta;
Signature · 1 line
2.7.0
Re: [Qemu-devel] [PATCH v11 2/7] hw/ptimer: Perform counter wrap around if timer already expired
Quoted 8 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).Signed-off-by Dmitry Osipenko <[email protected]>
Quoted 62 lines
--- hw/core/ptimer.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)hw/core/ptimer.c +13
-7@@ -83,14 +83,16 @@ static void ptimer_tick(void *opaque)uint64_t ptimer_get_count (ptimer_state *s) { -int64_t now;uint64_t counter;if (s->enabled) { - now =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ); +int64_t now =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ); +int64_t next = s->next_event; +bool expired = (now - next >=0 ); +bool oneshot = (s->enabled ==2 ); +/* Figure out the current counter value. */ -if (now - s->next_event >0 - || s->period ==0 ) { +if (s->period ==0 || (expired && (oneshot || use_icount))) {/* Prevent timer underflowing if it should already have triggered. */ counter =0 ;@@ -102,7 +104,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 && (s->delta * period <10000 ) && !use_icount) { period =10000 / s->delta; period_frac =0 ; }@@ -117,7 +119,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 +139,11 @@ 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; + } } }else { counter = s->delta; --2.7.0
Re: [Qemu-devel] [PATCH v11 2/7] hw/ptimer: Perform counter wrap around if timer already expired
Hello Peter,
24.01.2016 08:29, Peter Crosthwaite пишет:
[snip]
Quoted 4 lines
hw/core/ptimer.c uint64_t ptimer_get_count(ptimer_state *s) · line 146
Quoted 4 lines
hw/core/ptimer.c +4
-0+if (expired && (counter !=0 )) { +/* Wrap around periodic counter. */ + counter = s->limit - counter % s->limit; + }
Noticed another nit here. This should be:
counter = s->limit - (counter - 1) % s->limit;
So wraparound for counter = -1 is counter = limit (not limit - 1). Will fix it in V12.
I suppose your r-b is still valid, otherwise please let me know.
Signature · 1 line
Dmitry
PATCH v11 3/7 hw/ptimer: Update .delta on period/freq change
In reply to Dmitry OsipenkoDelta 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.
--- hw/core/ptimer.c | 2 ++ 1 file changed, 2 insertions(+)
hw/core/ptimer.c +2 -0
@@ -189,6 +189,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) {
@@ -200,6 +201,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.7.0
PATCH v11 4/7 hw/ptimer: Support "on the fly" timer mode switch
In reply to Dmitry OsipenkoAllow switching between periodic <-> oneshot modes while timer is running.
--- hw/core/ptimer.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
hw/core/ptimer.c +7 -6
@@ -162,16 +162,17 @@ 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 ) { +bool 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.7.0
PATCH v11 5/7 hw/ptimer: Introduce ptimer_get_limit
In reply to Dmitry OsipenkoCurrently 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.
--- hw/core/ptimer.c | 5 +++++ include/hw/ptimer.h | 1 + 2 files changed, 6 insertions(+)
hw/core/ptimer.c +5 -0
@@ -224,6 +224,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
@@ -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.7.0
PATCH v11 6/7 hw/ptimer: Legalize running with delta = load = 0 and abort on period = 0
In reply to Dmitry OsipenkoCurrently 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 removing the error message and stopping the timer when delta = 0. Abort execution when period = 0 instead of printing the error message, otherwise there is a chance to miss that error.
In addition, don't re-load oneshot timer when delta = 0 and remove duplicated code from ptimer_tick(), since ptimer_reload would invoke trigger and stop the timer.
--- hw/core/ptimer.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-)
hw/core/ptimer.c +12 -15
@@ -39,11 +39,14 @@ static void ptimer_reload(ptimer_state *s)if (s->delta ==0 ) {ptimer_trigger (s); + } + +if (s->delta ==0 && s->enabled ==1 ) { s->delta = s->limit; } -if (s->delta ==0 || s->period ==0 ) { -fprintf (stderr,"Timer with period zero, disabling\n" ); - s->enabled =0 ; + +if (s->delta ==0 ) { +ptimer_stop (s);return ; }
@@ -72,27 +75,22 @@ static void ptimer_reload(ptimer_state *s)static void ptimer_tick (void *opaque) {ptimer_state *s = (ptimer_state *)opaque; -ptimer_trigger (s); s->delta =0 ; -if (s->enabled ==2 ) { - s->enabled =0 ; - }else { -ptimer_reload (s); - } +ptimer_reload (s); }uint64_t ptimer_get_count (ptimer_state *s) {uint64_t counter; -if (s->enabled) { +if (s->enabled && s->delta !=0 ) {int64_t now =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL );int64_t next = s->next_event;bool expired = (now - next >=0 );bool oneshot = (s->enabled ==2 );/* Figure out the current counter value. */ -if (s->period ==0 || (expired && (oneshot || use_icount))) { +if (expired && (oneshot || use_icount)) {/* Prevent timer underflowing if it should already have triggered. */ counter =0 ;
@@ -164,10 +162,7 @@ void ptimer_run(ptimer_state *s, int oneshot) {bool was_disabled = !s->enabled; -if (was_disabled && s->period ==0 ) { -fprintf (stderr,"Timer with period zero, disabling\n" ); -return ; - } +g_assert (s->period !=0 ); s->enabled = oneshot ?2 :1 ;if (was_disabled) { s->next_event =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL );
@@ -190,6 +185,7 @@ void ptimer_stop(ptimer_state *s)/* Set counter increment interval in nanoseconds. */ void ptimer_set_period (ptimer_state *s,int64_t period) { +g_assert (period !=0 ); s->delta =ptimer_get_count (s); s->period = period; s->period_frac =0 ;
@@ -202,6 +198,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) { +g_assert (freq !=0 ); s->delta =ptimer_get_count (s); s->period =1000000000ll / freq; s->period_frac = (1000000000ll <<32 ) / freq;
Signature · 1 line
2.7.0
Re: [Qemu-arm] [PATCH v11 6/7] hw/ptimer: Legalize running with delta = load = 0 and abort on period = 0
Quoted 7 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 removing the error message and stopping the timer when delta = 0. Abort execution when period = 0 instead of printing the error message, otherwise there is a chance to miss that error.
Quoted 5 lines
In addition, don't re-load oneshot timer when delta = 0 and remove duplicated code from
ptimer_tick(), sinceptimer_reloadwould invoke trigger and stop the timer.Signed-off-by Dmitry Osipenko <[email protected]>
Quoted 88 lines
--- hw/core/ptimer.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-)hw/core/ptimer.c +12
-16@@ -39,11 +39,14 @@ static void ptimer_reload(ptimer_state *s)if (s->delta ==0 ) {ptimer_trigger (s); + } + +if (s->delta ==0 && s->enabled ==1 ) { s->delta = s->limit; } -if (s->delta ==0 || s->period ==0 ) { -fprintf (stderr,"Timer with period zero, disabling\n" ); - s->enabled =0 ; + +if (s->delta ==0 ) { +ptimer_stop (s);return ; }@@ -72,27 +75,22 @@ static void ptimer_reload(ptimer_state *s)static void ptimer_tick (void *opaque) {ptimer_state *s = (ptimer_state *)opaque; -ptimer_trigger (s); s->delta =0 ; -if (s->enabled ==2 ) { - s->enabled =0 ; - }else { -ptimer_reload (s); - } +ptimer_reload (s); }uint64_t ptimer_get_count (ptimer_state *s) {uint64_t counter; -if (s->enabled) { +if (s->enabled && s->delta !=0 ) {int64_t now =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL );int64_t next = s->next_event;bool expired = (now - next >=0 );bool oneshot = (s->enabled ==2 );/* Figure out the current counter value. */ -if (s->period ==0 || (expired && (oneshot || use_icount))) { +if (expired && (oneshot || use_icount)) {/* Prevent timer underflowing if it should already have triggered. */ counter =0 ;@@ -164,10 +162,7 @@ void ptimer_run(ptimer_state *s, int oneshot) {bool was_disabled = !s->enabled; -if (was_disabled && s->period ==0 ) { -fprintf (stderr,"Timer with period zero, disabling\n" ); -return ; - } +g_assert (s->period !=0 ); s->enabled = oneshot ?2 :1 ;if (was_disabled) { s->next_event =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL );@@ -190,6 +185,7 @@ void ptimer_stop(ptimer_state *s)/* Set counter increment interval in nanoseconds. */ void ptimer_set_period (ptimer_state *s,int64_t period) { +g_assert (period !=0 ); s->delta =ptimer_get_count (s); s->period = period; s->period_frac =0 ;@@ -202,6 +198,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) { +g_assert (freq !=0 ); s->delta =ptimer_get_count (s); s->period =1000000000ll / freq; s->period_frac = (1000000000ll <<32 ) / freq; --2.7.0
Re: [Qemu-arm] [PATCH v11 6/7] hw/ptimer: Legalize running with delta = load = 0 and abort on period = 0
24.01.2016 07:28, Peter Crosthwaite пишет:
[snip]
Quoted 7 lines
Quoted 7 lines
/* Set counter frequency in Hz. */
voidptimer_set_freq(ptimer_state *s, uint32_t freq){+ g_assert(freq != 0); s->delta = ptimer_get_count(s); s->period = 1000000000ll / freq; s->period_frac = (1000000000ll << 32) / freq;
I noticed that this should be g_assert(freq != 0 && freq <= 1000000000), otherwise it possible to make period = 0 for the running timer when setting freq > 1GHz, which doesn't make sense because we can't set period < 1ns.
Or maybe we should clamp period = max(1, period), freq = min(1000000000, freq) to "allow" higher timer freq's?
What do you think?
Signature · 1 line
Dmitry
PATCH v11 7/7 arm_mptimer: Convert to use ptimer
In reply to Dmitry OsipenkoCurrent 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.
--- hw/timer/arm_mptimer.c | 131 +++++++++++++++++++++-------------------- include/hw/timer/arm_mptimer.h | 5 +- 2 files changed, 67 insertions(+), 69 deletions(-)
hw/timer/arm_mptimer.c +66 -65
@@ -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. */ +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) +{ +if (cond) { +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. */ +if ((control &1 ) && (value ==0 ) && (control &0xff00 ) ==0 ) { +if (!(control &2 ) ||ptimer_get_limit (tb->timer) ==0 ) { +ptimer_stop (tb->timer); + control &= ~1 ; + } } +timerblock_set_count (tb->timer, control, &value); +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); } -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 () } };
include/hw/timer/arm_mptimer.h +1 -4
@@ -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.7.0
Re: [Qemu-arm] [PATCH v11 7/7] arm_mptimer: Convert to use ptimer
Quoted 43 lines · 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 reductionBump 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 | 131 +++++++++++++++++++++-------------------- include/hw/timer/arm_mptimer.h | 5 +- 2 files changed, 67 insertions(+), 69 deletions(-)hw/timer/arm_mptimer.c +4
-3@@ -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 ;
hw/timer/arm_mptimer.c static inline void timerblock_update_irq(TimerBlock *tb) · line 55
Quoted 10 lines
hw/timer/arm_mptimer.c +4
-3} -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 */
hw/timer/arm_mptimer.c static inline void timerblock_update_irq(TimerBlock *tb) · line 63
Quoted 10 lines
hw/timer/arm_mptimer.c +7
-2+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 ...
hw/timer/arm_mptimer.c static inline void timerblock_update_irq(TimerBlock *tb) · line 65
Quoted 2 lines
hw/timer/arm_mptimer.c +2
-0+{ +if (cond) {
... control & 1 ...
Quoted 61 lines
hw/timer/arm_mptimer.c +8
-27+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 ;
hw/timer/arm_mptimer.c static void timerblock_write(void *opaque, hwaddr addr, · line 112
Quoted 10 lines
hw/timer/arm_mptimer.c +6
-3} - 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 */
hw/timer/arm_mptimer.c static void timerblock_write(void *opaque, hwaddr addr, · line 114
Quoted 2 lines
hw/timer/arm_mptimer.c +2
-0+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.
hw/timer/arm_mptimer.c static void timerblock_write(void *opaque, hwaddr addr, · line 119
Quoted 5 lines
hw/timer/arm_mptimer.c +4
-0+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.
hw/timer/arm_mptimer.c static void timerblock_write(void *opaque, hwaddr addr, · line 126
Quoted 13 lines
hw/timer/arm_mptimer.c +5
-6+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
Quoted 108 lines
hw/timer/arm_mptimer.c +24
-21} -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 () } };include/hw/timer/arm_mptimer.h +1
-5@@ -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
Re: [Qemu-arm] [PATCH v11 7/7] arm_mptimer: Convert to use ptimer
Hello Peter,
24.01.2016 08:25, Peter Crosthwaite пишет:
[snip]
Quoted 14 lines
hw/timer/arm_mptimer.c static void timerblock_write(void *opaque, hwaddr addr, · line 126
Quoted 13 lines
hw/timer/arm_mptimer.c +5
-6+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);
Quoted 3 lines
This looks like a weird corner-case, what does it do exactly? I can't follow it so it needs a comment :)
It does the following:
mode | prescaler | reload if counter == 0 | tick immediately if counter == 0
--------------------------------------------------------------------------------
oneshot == 0 0 0 oneshot != 0 0 1 periodic == 0 1 0 periodic != 0 0 1
If writing control register with prescaler = 0, then for one-shot timer with counter == 0 or periodic timer with load = counter == 0 this is NOP. Will add a comment.
Thanks for review!
Signature · 1 line
Dmitry
Re: [Qemu-arm] [PATCH v11 7/7] arm_mptimer: Convert to use ptimer
24.01.2016 17:59, Dmitry Osipenko пишет:
Quoted 34 lines
Hello Peter,
24.01.2016 08:25, Peter Crosthwaite пишет:
[snip]
Quoted 15 lines
Quoted 12 lines
hw/timer/arm_mptimer.c +3
-6+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);hw/timer/arm_mptimer.c static void timerblock_write(void *opaque, hwaddr addr, · line 126
Quoted 2 lines
hw/timer/arm_mptimer.c +2
-0+if ((count ==0 ) && (value &2 )) { +timerblock_set_count (tb->timer, value, &count);Quoted 3 lines
This looks like a weird corner-case, what does it do exactly? I can't follow it so it needs a comment :)
It does the following:
mode | prescaler | reload if counter == 0 | tick immediately if counter == 0
--------------------------------------------------------------------------------
oneshot == 0 0 0oneshot != 0 0 1 periodic == 0 1 0 periodic != 0 0 1
Quoted 6 lines
If writing control register with prescaler = 0, then for one-shot timer with counter == 0 or periodic timer with load = counter == 0 this is NOP. Will add a comment.
Thanks for review!
Just realized that I missed timer re-run on prescaler change. So it was a good question! =)
Signature · 1 line
Dmitry