On Fri, Jun 07, 2024 at 03:29:33PM -0700, Joel Holdsworth via wrote:
> In the existing design, TTCR is prone to undercounting when running in
> continuous mode. This manifests as a timer interrupt appearing to
> trigger a few cycles prior to the deadline set in SPR_TTMR_TP.
>
> When the timer triggers, the virtual time delta in nanoseconds between
> the time when the timer was set, and when it triggers is calculated.
> This nanoseconds value is then divided by TIMER_PERIOD (50) to compute
> an increment of cycles to apply to TTCR.
>
> However, this calculation rounds down the number of cycles causing the
> undercounting.
>
> A simplistic solution would be to instead round up the number of cycles,
> however this will result in the accumulation of timing error over time.
>
> This patch corrects the issue by calculating the time delta in
> nanoseconds between when the timer was last reset and the timer event.
> This approach allows the TTCR value to be rounded up, but without
> accumulating error over time.
>
> Signed-off-by: Joel Holdsworth <jholdswo...@nvidia.com>
> ---
> hw/openrisc/cputimer.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/hw/openrisc/cputimer.c b/hw/openrisc/cputimer.c
> index 835986c4db..ddc129aa48 100644
> --- a/hw/openrisc/cputimer.c
> +++ b/hw/openrisc/cputimer.c
> @@ -29,7 +29,8 @@
> /* Tick Timer global state to allow all cores to be in sync */
> typedef struct OR1KTimerState {
> uint32_t ttcr;
> - uint64_t last_clk;
> + uint32_t ttcr_offset;
> + uint64_t clk_offset;
> } OR1KTimerState;
>
> static OR1KTimerState *or1k_timer;
> @@ -37,6 +38,8 @@ static OR1KTimerState *or1k_timer;
> void cpu_openrisc_count_set(OpenRISCCPU *cpu, uint32_t val)
> {
> or1k_timer->ttcr = val;
> + or1k_timer->ttcr_offset = val;
> + or1k_timer->clk_offset = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> }
>
> uint32_t cpu_openrisc_count_get(OpenRISCCPU *cpu)
> @@ -53,9 +56,8 @@ void cpu_openrisc_count_update(OpenRISCCPU *cpu)
> return;
> }
> now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> - or1k_timer->ttcr += (uint32_t)((now - or1k_timer->last_clk)
> - / TIMER_PERIOD);
> - or1k_timer->last_clk = now;
> + or1k_timer->ttcr = (now - or1k_timer->clk_offset + TIMER_PERIOD - 1) /
> TIMER_PERIOD +
> + or1k_timer->ttcr_offset;
> }
>
> /* Update the next timeout time as difference between ttmr and ttcr */
> @@ -69,7 +71,7 @@ void cpu_openrisc_timer_update(OpenRISCCPU *cpu)
> }
>
> cpu_openrisc_count_update(cpu);
> - now = or1k_timer->last_clk;
> + now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>
> if ((cpu->env.ttmr & TTMR_TP) <= (or1k_timer->ttcr & TTMR_TP)) {
> wait = TTMR_TP - (or1k_timer->ttcr & TTMR_TP) + 1;
> @@ -110,7 +112,8 @@ static void openrisc_timer_cb(void *opaque)
> case TIMER_NONE:
> break;
> case TIMER_INTR:
> - or1k_timer->ttcr = 0;
> + /* Zero the count by applying a negative offset to the counter */
> + or1k_timer->ttcr_offset += UINT32_MAX - (cpu->env.ttmr & TTMR_TP);
Hi Joel,
I am trying to get this merged as I am finally getting some time for this again
after a long project at work.
Why here do you do += UINT32_MAX - (cpu->env.ttmr & TTMR_TP)?
Is there an edge case I am not thinking of that is making you use UINT32_MAX?
Wouldn't this be the same as
r1k_timer->ttcr_offset -= 1 - (cpu->env.ttmr & TTMR_TP);
-Stafford
