[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: |
Andrew Jeffery |
Subject: |
Re: [PATCH v2 2/4] target/arm: Abstract the generic timer frequency |
Date: |
Thu, 05 Dec 2019 15:34:15 +1030 |
User-agent: |
Cyrus-JMAP/3.1.7-612-g13027cc-fmstable-20191203v1 |
On Wed, 4 Dec 2019, at 03:57, Philippe Mathieu-Daudé wrote:
> 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
> >>> guest.
> >>>
> >>> 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
> >>> maintained.
> >>>
> >>> 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 */
> >>> }
> >>> +
> >>> + cpu->gt_cntfrq = NANOSECONDS_PER_SECOND / GTIMER_SCALE;
> >>> }
> >>>
> >>> 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
> > calculation
> > because I didn't want to duplicate it across a number of places, then I
> > wanted type
> > 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.
I'll remove that.
>
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
Thanks. However, did you still want your comment on 4/4 addressed (move
the comment to this patch)?
Andrew
[PATCH v2 3/4] target/arm: Prepare generic timer for per-platform CNTFRQ, Andrew Jeffery, 2019/12/02
[PATCH v2 4/4] ast2600: Configure CNTFRQ at 1125MHz, Andrew Jeffery, 2019/12/02
Re: [PATCH v2 0/4] Expose GT CNTFRQ as a CPU property to support AST2600, Philippe Mathieu-Daudé, 2019/12/03