[v3,6/9] arm: clarify the use of muldiv64()

muldiv64() is used to convert microseconds into CPU ticks.

But it is not clear and not commented. This patch uses macro
to clearly identify what is used: time, CPU frequency and ticks.
For an elapsed time and a given frequency, we compute how many ticks
 we have.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
v2: replace "arm: remove muldiv64()"
 target-arm/helper.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

On Thu, Aug 27, 2015 at 12:33 PM, Laurent Vivier <lvivier@redhat.com> wrote:
> muldiv64() is used to convert microseconds into CPU ticks.
>
> But it is not clear and not commented. This patch uses macro
> to clearly identify what is used: time, CPU frequency and ticks.
> For an elapsed time and a given frequency, we compute how many ticks
>  we have.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

> ---
> v2: replace "arm: remove muldiv64()"
>  target-arm/helper.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 7df1f06..4455761 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -12,6 +12,8 @@
>  #include <zlib.h> /* For crc32 */
>  #include "exec/semihost.h"
>
> +#define ARM_CPU_FREQ 1000000000 /* FIXME: 1 GHz, should be configurable */
> +
>  #ifndef CONFIG_USER_ONLY
>  static inline bool get_phys_addr(CPUARMState *env, target_ulong address,
>                                   int access_type, ARMMMUIdx mmu_idx,
> @@ -678,8 +680,8 @@ void pmccntr_sync(CPUARMState *env)
>  {
>      uint64_t temp_ticks;
>
> -    temp_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
> -                          get_ticks_per_sec(), 1000000);
> +    temp_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> +                          ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
>
>      if (env->cp15.c9_pmcr & PMCRD) {
>          /* Increment once every 64 processor clock cycles */
> @@ -717,8 +719,8 @@ static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>          return env->cp15.c15_ccnt;
>      }
>
> -    total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
> -                           get_ticks_per_sec(), 1000000);
> +    total_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> +                           ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
>
>      if (env->cp15.c9_pmcr & PMCRD) {
>          /* Increment once every 64 processor clock cycles */
> @@ -738,8 +740,8 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>          return;
>      }
>
> -    total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
> -                           get_ticks_per_sec(), 1000000);
> +    total_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> +                           ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
>
>      if (env->cp15.c9_pmcr & PMCRD) {
>          /* Increment once every 64 processor clock cycles */
> --
> 2.1.0
>
>

On 27 August 2015 at 20:33, Laurent Vivier <lvivier@redhat.com> wrote:
> muldiv64() is used to convert microseconds into CPU ticks.
>
> But it is not clear and not commented. This patch uses macro
> to clearly identify what is used: time, CPU frequency and ticks.
> For an elapsed time and a given frequency, we compute how many ticks
>  we have.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> v2: replace "arm: remove muldiv64()"
>  target-arm/helper.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 7df1f06..4455761 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -12,6 +12,8 @@
>  #include <zlib.h> /* For crc32 */
>  #include "exec/semihost.h"
>
> +#define ARM_CPU_FREQ 1000000000 /* FIXME: 1 GHz, should be configurable */

LL suffix for large constants, please.

Otherwise
Acked-by: Peter Maydell <peter.maydell@linaro.org>

(assuming you're planning to put this somewhere other than the
target-arm tree).

thanks
-- PMM

On 01/09/2015 13:17, Peter Maydell wrote:
> On 27 August 2015 at 20:33, Laurent Vivier <lvivier@redhat.com> wrote:
>> muldiv64() is used to convert microseconds into CPU ticks.
>>
>> But it is not clear and not commented. This patch uses macro
>> to clearly identify what is used: time, CPU frequency and ticks.
>> For an elapsed time and a given frequency, we compute how many ticks
>>  we have.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>> v2: replace "arm: remove muldiv64()"
>>  target-arm/helper.c | 14 ++++++++------
>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 7df1f06..4455761 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -12,6 +12,8 @@
>>  #include <zlib.h> /* For crc32 */
>>  #include "exec/semihost.h"
>>
>> +#define ARM_CPU_FREQ 1000000000 /* FIXME: 1 GHz, should be configurable */
> 
> LL suffix for large constants, please.

In fact, I didn't put the LL suffix to not force the use of a 64bit on a
32bit machines. Moreover in muldiv64() it is used as a 32bit value.

But if you think it is better, I will. Have I to resend the whole series
or only this patch ? Perhaps the commiter can edit it ?

> 
> Otherwise
> Acked-by: Peter Maydell <peter.maydell@linaro.org>
> 
> (assuming you're planning to put this somewhere other than the
> target-arm tree).
> 
> thanks
> -- PMM
>

On 1 September 2015 at 12:23, Laurent Vivier <lvivier@redhat.com> wrote:
>
>
> On 01/09/2015 13:17, Peter Maydell wrote:
>> On 27 August 2015 at 20:33, Laurent Vivier <lvivier@redhat.com> wrote:
>>> +#define ARM_CPU_FREQ 1000000000 /* FIXME: 1 GHz, should be configurable */
>>
>> LL suffix for large constants, please.
>
> In fact, I didn't put the LL suffix to not force the use of a 64bit on a
> 32bit machines. Moreover in muldiv64() it is used as a 32bit value.
>
> But if you think it is better, I will. Have I to resend the whole series
> or only this patch ? Perhaps the commiter can edit it ?

Oh, you're right, it's a 32-bit value. I take back my suggestion.

thanks
-- PMM