Re: [PATCH] hw/openrisc: Fixed undercounting of TTCR in continuous mode

[Stafford Horne]

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 <jholdsworth@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