[PATCH] target/mips: Rework cp0_timer with clock API

[PATCH] target/mips: Rework cp0_timer with clock API

[Jiaxun Yang]

Previous implementation of MIPS cp0_timer computes a
cp0_count_ns based on input clock. However rounding
error of cp0_count_ns can affect precision of cp0_timer.

Using clock API and a divider for cp0_timer, so we can
use clock_ns_to_ticks/clock_ns_to_ticks to avoid rounding
issue.

Also workaround the situation that in such handler flow:

count = read_c0_count()
write_c0_compare(count)

If timer had not progressed when compare was written, the
interrupt would trigger again.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
This seems fixed MTTCG booting issue on malta 5kEc with SMP.
I'm going to do more test and see if we can enable MTTCG for
mips64el.
---
 target/mips/cpu.c              |  8 +++++---
 target/mips/cpu.h              |  3 ++-
 target/mips/sysemu/cp0_timer.c | 35 ++++++++++++++++++----------------
 3 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 01e0fbe10db2..b7119cbbb459 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -449,9 +449,9 @@ static void mips_cp0_period_set(MIPSCPU *cpu)
 {
     CPUMIPSState *env = &cpu->env;
 
-    env->cp0_count_ns = clock_ticks_to_ns(MIPS_CPU(cpu)->clock,
-                                          env->cpu_model->CCRes);
-    assert(env->cp0_count_ns);
+    clock_set_mul_div(cpu->count_div, env->cpu_model->CCRes, 1);
+    clock_set_source(cpu->count_div, cpu->clock);
+    clock_set_source(env->count_clock, cpu->count_div);
 }
 
 static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
@@ -504,6 +504,8 @@ static void mips_cpu_initfn(Object *obj)
 
     cpu_set_cpustate_pointers(cpu);
     cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk-in", NULL, cpu, 0);
+    cpu->count_div = clock_new(OBJECT(obj), "clk-div-count");
+    env->count_clock = clock_new(OBJECT(obj), "clk-count");
     env->cpu_model = mcc->cpu_def;
 }
 
diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 142c55af478b..1b8107b0af86 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -1160,8 +1160,8 @@ typedef struct CPUArchState {
 
     const mips_def_t *cpu_model;
     QEMUTimer *timer; /* Internal timer */
+    Clock *count_clock; /* CP0_Count clock */
     target_ulong exception_base; /* ExceptionBase input to the core */
-    uint64_t cp0_count_ns; /* CP0_Count clock period (in nanoseconds) */
 } CPUMIPSState;
 
 /**
@@ -1178,6 +1178,7 @@ struct ArchCPU {
     /*< public >*/
 
     Clock *clock;
+    Clock *count_div; /* Divider for CP0_Count clock */
     CPUNegativeOffsetState neg;
     CPUMIPSState env;
 };
diff --git a/target/mips/sysemu/cp0_timer.c b/target/mips/sysemu/cp0_timer.c
index 70de95d338f8..9d2bcb0dea21 100644
--- a/target/mips/sysemu/cp0_timer.c
+++ b/target/mips/sysemu/cp0_timer.c
@@ -28,15 +28,26 @@
 #include "internal.h"
 
 /* MIPS R4K timer */
+static uint32_t cpu_mips_get_count_val(CPUMIPSState *env)
+{
+    int64_t now_ns;
+    now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    return env->CP0_Count +
+            (uint32_t)clock_ns_to_ticks(env->count_clock, now_ns);
+}
+
 static void cpu_mips_timer_update(CPUMIPSState *env)
 {
     uint64_t now_ns, next_ns;
     uint32_t wait;
 
     now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    wait = env->CP0_Compare - env->CP0_Count -
-           (uint32_t)(now_ns / env->cp0_count_ns);
-    next_ns = now_ns + (uint64_t)wait * env->cp0_count_ns;
+    wait = env->CP0_Compare - cpu_mips_get_count_val(env);
+    /* Clamp interval to overflow if virtual time had not progressed */
+    if (!wait) {
+        wait = UINT32_MAX;
+    }
+    next_ns = now_ns + clock_ticks_to_ns(env->count_clock, wait);
     timer_mod(env->timer, next_ns);
 }
 
@@ -64,7 +75,7 @@ uint32_t cpu_mips_get_count(CPUMIPSState *env)
             cpu_mips_timer_expire(env);
         }
 
-        return env->CP0_Count + (uint32_t)(now_ns / env->cp0_count_ns);
+        return cpu_mips_get_count_val(env);
     }
 }
 
@@ -79,9 +90,8 @@ void cpu_mips_store_count(CPUMIPSState *env, uint32_t count)
         env->CP0_Count = count;
     } else {
         /* Store new count register */
-        env->CP0_Count = count -
-               (uint32_t)(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) /
-                          env->cp0_count_ns);
+        env->CP0_Count = count - (uint32_t)clock_ns_to_ticks(env->count_clock,
+                        qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
         /* Update timer timer */
         cpu_mips_timer_update(env);
     }
@@ -107,8 +117,8 @@ void cpu_mips_start_count(CPUMIPSState *env)
 void cpu_mips_stop_count(CPUMIPSState *env)
 {
     /* Store the current value */
-    env->CP0_Count += (uint32_t)(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) /
-                                 env->cp0_count_ns);
+    env->CP0_Count += (uint32_t)clock_ns_to_ticks(env->count_clock,
+                        qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
 }
 
 static void mips_timer_cb(void *opaque)
@@ -121,14 +131,7 @@ static void mips_timer_cb(void *opaque)
         return;
     }
 
-    /*
-     * ??? This callback should occur when the counter is exactly equal to
-     * the comparator value.  Offset the count by one to avoid immediately
-     * retriggering the callback before any virtual time has passed.
-     */
-    env->CP0_Count++;
     cpu_mips_timer_expire(env);
-    env->CP0_Count--;
 }
 
 void cpu_mips_clock_init(MIPSCPU *cpu)
-- 
2.39.2 (Apple Git-143)

Re: [PATCH] target/mips: Rework cp0_timer with clock API

[Philippe Mathieu-Daudé]

On 21/5/23 13:00, Jiaxun Yang wrote:
> Previous implementation of MIPS cp0_timer computes a
> cp0_count_ns based on input clock. However rounding
> error of cp0_count_ns can affect precision of cp0_timer.
> 
> Using clock API and a divider for cp0_timer, so we can
> use clock_ns_to_ticks/clock_ns_to_ticks to avoid rounding
> issue.
> 
> Also workaround the situation that in such handler flow:
> 
> count = read_c0_count()
> write_c0_compare(count)
> 
> If timer had not progressed when compare was written, the
> interrupt would trigger again.
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> This seems fixed MTTCG booting issue on malta 5kEc with SMP.
> I'm going to do more test and see if we can enable MTTCG for
> mips64el.
> ---
>   target/mips/cpu.c              |  8 +++++---
>   target/mips/cpu.h              |  3 ++-
>   target/mips/sysemu/cp0_timer.c | 35 ++++++++++++++++++----------------
>   3 files changed, 26 insertions(+), 20 deletions(-)

Nice!

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH] target/mips: Rework cp0_timer with clock API

[Philippe Mathieu-Daudé]

On 21/5/23 13:00, Jiaxun Yang wrote:
> Previous implementation of MIPS cp0_timer computes a
> cp0_count_ns based on input clock. However rounding
> error of cp0_count_ns can affect precision of cp0_timer.
> 
> Using clock API and a divider for cp0_timer, so we can
> use clock_ns_to_ticks/clock_ns_to_ticks to avoid rounding
> issue.
> 
> Also workaround the situation that in such handler flow:
> 
> count = read_c0_count()
> write_c0_compare(count)
> 
> If timer had not progressed when compare was written, the
> interrupt would trigger again.
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> This seems fixed MTTCG booting issue on malta 5kEc with SMP.
> I'm going to do more test and see if we can enable MTTCG for
> mips64el.
> ---
>   target/mips/cpu.c              |  8 +++++---
>   target/mips/cpu.h              |  3 ++-
>   target/mips/sysemu/cp0_timer.c | 35 ++++++++++++++++++----------------
>   3 files changed, 26 insertions(+), 20 deletions(-)

Thanks, queued to mips-next.