Date
1 - 3 of 3
[PATCH 1/2] arc_timer: fix tickless idle
|
Desfarges, Simon <simon.desfarges@...>
From: Simon Desfarges <simon.desfarges(a)intel.com>
When exiting from tickless idle uppon an external IRQ, the TICK timer is set to fire at next TICK boundary. The current algorithm can lead to a point that timer0_count register is higher than the timer0_limit register. In this situation the next TICK will fire after a full wrapp of the counter (~133 seconds). This condition appears when the counter reaches the limit after the Interrupt Pending flag is checked. At this point the counter is automatically wrapped to 0, but is set just next the limit to fire at next TICK boundary by SW. At exit of the _timer_idle_exit function, the timer handler is called, and sets the limit to 1 TICK. At this point the situation is: - limit register == 1 TICK - count register is just below the old limit register and higher than 1 TICK To fix this issue, at _timer_idle_exit, the limit register is always set to 1 TICK and the count register set such as the next TICK fires on time. Change-Id: Ifa002809d426aa04109592e53d2b02a224f51101 Tracked-On: https://jira.ndg.intel.com/browse/FIRE-4213 Signed-off-by: Simon Desfarges <simon.desfarges(a)intel.com> --- drivers/timer/arcv2_timer0.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/drivers/timer/arcv2_timer0.c b/drivers/timer/arcv2_timer0.c index 932ceac..5fec988 100644 --- a/drivers/timer/arcv2_timer0.c +++ b/drivers/timer/arcv2_timer0.c @@ -220,7 +220,7 @@ void _timer_idle_enter(int32_t ticks) } programmed_ticks = ticks; - programmed_limit = (programmed_ticks * cycles_per_tick); + programmed_limit = (programmed_ticks * cycles_per_tick) - 1; timer0_limit_register_set(programmed_limit); @@ -276,9 +276,6 @@ void _timer_idle_exit(void) * A non-timer interrupt occurred. Announce any * ticks that have elapsed during the tickless idle. */ - - uint32_t remaining_cycles = programmed_limit - current_count; - _sys_idle_elapsed_ticks = current_count / cycles_per_tick; if (_sys_idle_elapsed_ticks > 0) { update_accumulated_count(); @@ -289,11 +286,8 @@ void _timer_idle_exit(void) * Ensure the timer will expire at the end of the next tick in case * the ISR makes any tasks and/or fibers ready to run. */ - - if (remaining_cycles >= cycles_per_tick) { - timer0_count_register_set(programmed_limit - - ((remaining_cycles - 1) % cycles_per_tick) - 1); - } + timer0_limit_register_set(cycles_per_tick - 1); + timer0_count_register_set(timer0_count_register_get() % cycles_per_tick); } #else static void tickless_idle_init(void) {} -- 1.9.1 |
|
|
|
Mitsis, Peter <Peter.Mitsis@...>
See [Peter]
toggle quoted message
Show quoted text
-----Original Message-----[Peter]: Good catch on this off by one error. timer0_limit_register_set(programmed_limit);[Peter]: First an historical note. The reason the original code only touched the count register was that it was not known if lowering the limit register to a value below that of the count register would trigger an interrupt. (I did not have access to a board with the ARCv2 timer and so was unable to confirm or deny that behavior.) That being said, if changing the limit value from a value higher than the count to a value lower than the count does not generate an interrupt, then I have no objections on that front. Second, Using timer0_count_register_get() at this point is a no-no as it introduces a race condition that can cause us to lose track of a whole tick. Remember, the timer is always counting. If the elapsed ticks were announced just prior to a "tick boundary", the call to timer0_count_register_set() could then occur just after the "tick boundary". The result of this scenario is that nearly one whole tick may be lost when the modulus operation is performed. This is why we use the <current_count> retrieved from the start of _timer_idle_exit(). Yes, it does introduce some gradual drift, but this can be ameliorated somewhat by only modifying the count if there is more than one tick left in the tickles idle. Hope this helps. } |
|
|
|
Desfarges, Simon <simon.desfarges@...>
Hello,
toggle quoted message
Show quoted text
Thanks for the answer, I will review my copy. BTW, the same test bench shows no jitter in the Quark TICK. So I would like to have the same behavior for ARC. Do you think it is achievable ? I have doubts because of the fact that loapic decrements its counter (logic is reversed) and looks to be safer. See my [Simon] below. -----Original Message-----[Simon] I will separate in another patch and try to make it merged asap so we can forget this point. [Simon] This is the case I experimented. The ISR is generated only if count == limit.timer0_limit_register_set(programmed_limit);[Peter]: [Simon] I especially call this function to limit this gradual drift. As far as I understand, in every point of this function, it is possible to have a race condition or an overlap of the counter. The case where the counter overlaps after checking the 'IP' bit and before setting the count register is handled, because the ISR will fire anyway and 1 tick will be announced. I see in the loapic timer there is a 'timer_known_to_have_expired' variable, handling this exact case, I will take a look at it (I guess you already did it...).
|
|
|