[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 2/4] target/arm: Abstract the generic timer frequency

From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v2 2/4] target/arm: Abstract the generic timer frequency
Date: Tue, 3 Dec 2019 18:27:41 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2

On 12/3/19 1:48 PM, Andrew Jeffery wrote:
On Tue, 3 Dec 2019, at 16:39, Philippe Mathieu-Daudé wrote:
On 12/3/19 5:14 AM, Andrew Jeffery wrote:
Prepare for SoCs such as the ASPEED AST2600 whose firmware configures
CNTFRQ to values significantly larger than the static 62.5MHz value
currently derived from GTIMER_SCALE. As the OS potentially derives its
timer periods from the CNTFRQ value the lack of support for running
QEMUTimers at the appropriate rate leads to sticky behaviour in the

Substitute the GTIMER_SCALE constant with use of a helper to derive the
period from gt_cntfrq stored in struct ARMCPU. Initially set gt_cntfrq
to the frequency associated with GTIMER_SCALE so current behaviour is

Signed-off-by: Andrew Jeffery <address@hidden>
Reviewed-by: Richard Henderson <address@hidden>
   target/arm/cpu.c    |  2 ++
   target/arm/cpu.h    | 10 ++++++++++
   target/arm/helper.c | 10 +++++++---
   3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 7a4ac9339bf9..5698a74061bb 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -974,6 +974,8 @@ static void arm_cpu_initfn(Object *obj)
       if (tcg_enabled()) {
           cpu->psci_version = 2; /* TCG implements PSCI 0.2 */
static Property arm_cpu_reset_cbar_property =
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 83a809d4bac4..666c03871fdf 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -932,8 +932,18 @@ struct ARMCPU {
       DECLARE_BITMAP(sve_vq_map, ARM_MAX_VQ);
       DECLARE_BITMAP(sve_vq_init, ARM_MAX_VQ);
+    /* Generic timer counter frequency, in Hz */
+    uint64_t gt_cntfrq;

You can also explicit the unit by calling it 'gt_cntfrq_hz'.

Fair call, I'll fix that.

+static inline unsigned int gt_cntfrq_period_ns(ARMCPU *cpu)
+    /* XXX: Could include qemu/timer.h to get NANOSECONDS_PER_SECOND? */

Why inline this call? I doubt there is a significant performance gain.

It wasn't so much performance. It started out as a macro for a simple 
because I didn't want to duplicate it across a number of places, then I wanted 
safety for the pointer so  I switched the macro in the header to an inline 
function. So
it is an evolution of the patch rather than something that came from an 
explicit goal
of e.g. performance.

OK. Eventually NANOSECONDS_PER_SECOND will move to "qemu/units.h".

Should the XXX comment stay? I'm not sure, it is confusing.

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

reply via email to

[Prev in Thread] Current Thread [Next in Thread]