[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] replaced get_ticks_per_sec() with constant
From: |
Peter Crosthwaite |
Subject: |
Re: [Qemu-devel] [PATCH] replaced get_ticks_per_sec() with constant |
Date: |
Tue, 31 Mar 2015 09:46:32 +1000 |
On Tue, Mar 31, 2015 at 7:52 AM, Peter Maydell <address@hidden> wrote:
> On 30 March 2015 at 14:01, Stefan Hajnoczi <address@hidden> wrote:
>> On Sat, Mar 28, 2015 at 06:22:11PM +0200, Emil Condrea wrote:
>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>> index 10886c5..504530a 100644
>>> --- a/target-arm/helper.c
>>> +++ b/target-arm/helper.c
>>> @@ -649,7 +649,7 @@ void pmccntr_sync(CPUARMState *env)
>>> uint64_t temp_ticks;
>>>
>>> temp_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
>>> - get_ticks_per_sec(), 1000000);
>>> + NSEC_PER_SEC, 1000000);
>>>
>>> if (env->cp15.c9_pmcr & PMCRD) {
>>> /* Increment once every 64 processor clock cycles */
>>> @@ -688,7 +688,7 @@ static uint64_t pmccntr_read(CPUARMState *env, const
>>> ARMCPRegInfo *ri)
>>> }
>>>
>>> total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
>>> - get_ticks_per_sec(), 1000000);
>>> + NSEC_PER_SEC, 1000000);
>>>
>>> if (env->cp15.c9_pmcr & PMCRD) {
>>> /* Increment once every 64 processor clock cycles */
>>> @@ -709,7 +709,7 @@ static void pmccntr_write(CPUARMState *env, const
>>> ARMCPRegInfo *ri,
>>> }
>>>
>>> total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
>>> - get_ticks_per_sec(), 1000000);
>>> + NSEC_PER_SEC, 1000000);
>>
>> Peter: I wonder why the muldiv64() expression is necessary. Is it
>> intentionally returning the microsecond clock converted to nanoseconds?
>>
That is the effect but not the intention. In this context,
get_tick_per_sec() is really trying to be a clock signal frequency
rate and QEMU ARM is just hardcoding to 1GHz. Because its fixed and in
terms of real time, your proposed cancellation is possible.
For ARM CPU we are simplifying the CPU clock pin as ns (basically 1GHz
clock). Ideally this should be parameterisable as this clock is
controlled outside of the ARM processor.
I think the form of the current code has value for self documentation,
and maybe should have a comment explaining this 1GHz harcodedness.
When we get around to parameterising the ARM CPU frequency as a QOM
property this code should be fixed by doing:
s/NSEC_PER_SEC/s->clock_freq_hz/
So this muldiv in its current form is really preparation for that. Can
we keep it? Your patch as-is looks good to me.
Acked-by: Peter Crosthwaite <address@hidden>
Regards,
Peter
> -- PMM
>
>> Perhaps the expression should be replaced with just
>> qemu_get_clock_ns(QEMU_CLOCK_VIRTUAL)?
>>
>> Or if microsecond precision is required, use qemu_get_clock_us() * 1000.
>
> Ask Peter C -- IIRC this is his code.
>