target/mips/cpu.h | 1 - target/mips/cp0_timer.c | 34 +++++++++++++++++++++++++--------- target/mips/cpu.c | 10 ---------- 3 files changed, 25 insertions(+), 20 deletions(-)
Use the new clock_ns_to_ticks() function in cp0_timer where
appropriate. This allows us to remove CPUMIPSState::cp0_count_ns
and mips_cp0_period_set().
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Based-on: <20210209132040.5091-1-peter.maydell@linaro.org>
RFC because this is just a starter patch to test Peter's series
providing a handy function which "tells me how many times this
clock would tick in this length of time".
Also because we could rethink the MIPS CP0 timer logic to avoid
too frequent divu128 calls (painful on 32-bit hosts).
The style should be updated, using more variables for easier
review. env_archcpu() could eventually be dropped (by passing
MIPSCPU* instead of CPUMIPSState*).
This passes my MIPS tier1 tests, kicking CI before going to bed.
Posting meanwhile for comments.
---
target/mips/cpu.h | 1 -
target/mips/cp0_timer.c | 34 +++++++++++++++++++++++++---------
target/mips/cpu.c | 10 ----------
3 files changed, 25 insertions(+), 20 deletions(-)
diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index b9e227a30e9..946748d8cc7 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -1156,7 +1156,6 @@ struct CPUMIPSState {
struct MIPSITUState *itu;
MemoryRegion *itc_tag; /* ITC Configuration Tags */
target_ulong exception_base; /* ExceptionBase input to the core */
- uint64_t cp0_count_ns; /* CP0_Count clock period (in nanoseconds) */
};
/**
diff --git a/target/mips/cp0_timer.c b/target/mips/cp0_timer.c
index 70de95d338f..3a128302122 100644
--- a/target/mips/cp0_timer.c
+++ b/target/mips/cp0_timer.c
@@ -30,13 +30,17 @@
/* MIPS R4K timer */
static void cpu_mips_timer_update(CPUMIPSState *env)
{
+ MIPSCPU *cpu = env_archcpu(env);
uint64_t now_ns, next_ns;
uint32_t wait;
+ uint64_t now_ticks;
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;
+ now_ticks = clock_ns_to_ticks(cpu->clock, now_ns);
+ wait = env->CP0_Compare - env->CP0_Count
+ - (uint32_t)(now_ticks / cpu->cp0_count_rate);
+ next_ns = now_ns + (uint64_t)wait * clock_ticks_to_ns(cpu->clock,
+ cpu->cp0_count_rate);
timer_mod(env->timer, next_ns);
}
@@ -55,6 +59,7 @@ uint32_t cpu_mips_get_count(CPUMIPSState *env)
if (env->CP0_Cause & (1 << CP0Ca_DC)) {
return env->CP0_Count;
} else {
+ MIPSCPU *cpu = env_archcpu(env);
uint64_t now_ns;
now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
@@ -64,12 +69,15 @@ 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 env->CP0_Count + (uint32_t)(clock_ns_to_ticks(cpu->clock, now_ns)
+ / cpu->cp0_count_rate);
}
}
void cpu_mips_store_count(CPUMIPSState *env, uint32_t count)
{
+ MIPSCPU *cpu = env_archcpu(env);
+
/*
* This gets called from cpu_state_reset(), potentially before timer init.
* So env->timer may be NULL, which is also the case with KVM enabled so
@@ -78,10 +86,14 @@ void cpu_mips_store_count(CPUMIPSState *env, uint32_t count)
if (env->CP0_Cause & (1 << CP0Ca_DC) || !env->timer) {
env->CP0_Count = count;
} else {
+ uint64_t cp0_count_ticks;
+
+ cp0_count_ticks = clock_ns_to_ticks(cpu->clock,
+ qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
/* 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)(cp0_count_ticks / cpu->cp0_count_rate);
+
+
/* Update timer timer */
cpu_mips_timer_update(env);
}
@@ -106,9 +118,13 @@ void cpu_mips_start_count(CPUMIPSState *env)
void cpu_mips_stop_count(CPUMIPSState *env)
{
+ MIPSCPU *cpu = env_archcpu(env);
+ uint64_t cp0_count_ticks;
+
+ cp0_count_ticks = clock_ns_to_ticks(cpu->clock,
+ qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
/* 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)(cp0_count_ticks / cpu->cp0_count_rate);
}
static void mips_timer_cb(void *opaque)
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 2f3d9d2ce2c..17caf236da9 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -578,15 +578,6 @@ static void mips_cpu_disas_set_info(CPUState *s, disassemble_info *info)
#define CPU_FREQ_HZ_DEFAULT 200000000
#define CP0_COUNT_RATE_DEFAULT 2
-static void mips_cp0_period_set(MIPSCPU *cpu)
-{
- CPUMIPSState *env = &cpu->env;
-
- env->cp0_count_ns = clock_ticks_to_ns(MIPS_CPU(cpu)->clock,
- cpu->cp0_count_rate);
- assert(env->cp0_count_ns);
-}
-
static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
{
CPUState *cs = CPU(dev);
@@ -607,7 +598,6 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
/* Initialize the frequency in case the clock remains unconnected. */
clock_set_hz(cpu->clock, CPU_FREQ_HZ_DEFAULT);
}
- mips_cp0_period_set(cpu);
cpu_exec_realizefn(cs, &local_err);
if (local_err != NULL) {
--
2.26.2
Patchew URL: https://patchew.org/QEMU/20210210234334.3750022-1-f4bug@amsat.org/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20210210234334.3750022-1-f4bug@amsat.org
Subject: [RFC PATCH] target/mips/cp0_timer: Use new clock_ns_to_ticks()
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
86c0e95 target/mips/cp0_timer: Use new clock_ns_to_ticks()
=== OUTPUT BEGIN ===
ERROR: space prohibited after that '-' (ctx:ExW)
#39: FILE: target/mips/cp0_timer.c:41:
+ - (uint32_t)(now_ticks / cpu->cp0_count_rate);
^
WARNING: line over 80 characters
#77: FILE: target/mips/cp0_timer.c:92:
+ qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
WARNING: line over 80 characters
#82: FILE: target/mips/cp0_timer.c:94:
+ env->CP0_Count = count - (uint32_t)(cp0_count_ticks / cpu->cp0_count_rate);
total: 1 errors, 2 warnings, 104 lines checked
Commit 86c0e959ed23 (target/mips/cp0_timer: Use new clock_ns_to_ticks()) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20210210234334.3750022-1-f4bug@amsat.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On 2/11/21 12:25 PM, no-reply@patchew.org wrote: > Patchew URL: https://patchew.org/QEMU/20210210234334.3750022-1-f4bug@amsat.org/ > > > > Hi, > > This series seems to have some coding style problems. See output below for > more information: > > Type: series > Message-id: 20210210234334.3750022-1-f4bug@amsat.org > Subject: [RFC PATCH] target/mips/cp0_timer: Use new clock_ns_to_ticks() > > Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 > Switched to a new branch 'test' > 86c0e95 target/mips/cp0_timer: Use new clock_ns_to_ticks() > > === OUTPUT BEGIN === > ERROR: space prohibited after that '-' (ctx:ExW) > #39: FILE: target/mips/cp0_timer.c:41: > + - (uint32_t)(now_ticks / cpu->cp0_count_rate); > ^ Well, having the operator at the beginning of the line makes it more obvious than having it at the end of the previous line. I can remove the space to make checkpatch happy but it seems wrong.
On 2/11/21 12:43 AM, Philippe Mathieu-Daudé wrote: > Use the new clock_ns_to_ticks() function in cp0_timer where > appropriate. This allows us to remove CPUMIPSState::cp0_count_ns > and mips_cp0_period_set(). > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > Based-on: <20210209132040.5091-1-peter.maydell@linaro.org> > > RFC because this is just a starter patch to test Peter's series > providing a handy function which "tells me how many times this > clock would tick in this length of time". > > Also because we could rethink the MIPS CP0 timer logic to avoid > too frequent divu128 calls (painful on 32-bit hosts). > > The style should be updated, using more variables for easier > review. env_archcpu() could eventually be dropped (by passing > MIPSCPU* instead of CPUMIPSState*). I guess it is better to wait for Peter patches to get merged before posting the updated patches (not much, just one previous patch MIPSCPU* instead of CPUMIPSState*). Regards, Phil.
On Sun, 21 Feb 2021 at 20:07, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 2/11/21 12:43 AM, Philippe Mathieu-Daudé wrote: > > Use the new clock_ns_to_ticks() function in cp0_timer where > > appropriate. This allows us to remove CPUMIPSState::cp0_count_ns > > and mips_cp0_period_set(). > > > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > > Based-on: <20210209132040.5091-1-peter.maydell@linaro.org> > > > > RFC because this is just a starter patch to test Peter's series > > providing a handy function which "tells me how many times this > > clock would tick in this length of time". > > > > Also because we could rethink the MIPS CP0 timer logic to avoid > > too frequent divu128 calls (painful on 32-bit hosts). > > > > The style should be updated, using more variables for easier > > review. env_archcpu() could eventually be dropped (by passing > > MIPSCPU* instead of CPUMIPSState*). > > I guess it is better to wait for Peter patches to get merged > before posting the updated patches (not much, just one previous > patch MIPSCPU* instead of CPUMIPSState*). This patch is still on my to-review queue, fwiw (though I am taking a week off work next week, so I probably won't get to it til March.) -- PMM
On 2/21/21 9:12 PM, Peter Maydell wrote: > On Sun, 21 Feb 2021 at 20:07, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> On 2/11/21 12:43 AM, Philippe Mathieu-Daudé wrote: >>> Use the new clock_ns_to_ticks() function in cp0_timer where >>> appropriate. This allows us to remove CPUMIPSState::cp0_count_ns >>> and mips_cp0_period_set(). >>> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- >>> Based-on: <20210209132040.5091-1-peter.maydell@linaro.org> >>> >>> RFC because this is just a starter patch to test Peter's series >>> providing a handy function which "tells me how many times this >>> clock would tick in this length of time". >>> >>> Also because we could rethink the MIPS CP0 timer logic to avoid >>> too frequent divu128 calls (painful on 32-bit hosts). >>> >>> The style should be updated, using more variables for easier >>> review. env_archcpu() could eventually be dropped (by passing >>> MIPSCPU* instead of CPUMIPSState*). >> >> I guess it is better to wait for Peter patches to get merged >> before posting the updated patches (not much, just one previous >> patch MIPSCPU* instead of CPUMIPSState*). > > This patch is still on my to-review queue, fwiw (though I am > taking a week off work next week, so I probably won't get to > it til March.) Thanks for the update, I'll repost during the first day of March then :) Also I'll try to rebase my "hw/char/serial: Use the Clock API to feed the UART reference clock" too. Regards, Phil.
© 2016 - 2024 Red Hat, Inc.