Re: [PATCH v4 2/6] hw/timer: Refactor NPCM7XX Timer to use CLK clock

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 17 Dec 2020 at 00:45, Hao Wu <wuhaotsh@google.com> wrote:
>
> This patch makes NPCM7XX Timer to use a the timer clock generated by the
> CLK module instead of the magic number TIMER_REF_HZ.
>
> Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>
> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>
> ---
>  hw/arm/npcm7xx.c                 |  5 +++++
>  hw/timer/npcm7xx_timer.c         | 25 ++++++++++++++-----------
>  include/hw/misc/npcm7xx_clk.h    |  6 ------
>  include/hw/timer/npcm7xx_timer.h |  1 +
>  4 files changed, 20 insertions(+), 17 deletions(-)

> @@ -130,7 +130,7 @@ static int64_t npcm7xx_timer_count_to_ns(NPCM7xxTimer *t, uint32_t count)
>  {
>      int64_t ns = count;
>
> -    ns *= NANOSECONDS_PER_SECOND / NPCM7XX_TIMER_REF_HZ;
> +    ns *= clock_get_ns(t->ctrl->clock);
>      ns *= npcm7xx_tcsr_prescaler(t->tcsr);

I'm afraid that since you wrote and sent this we've updated the
clock API (in commits 554d523785ef868 and de6a65f11d7e5a2a93f),
so clock_get_ns() doesn't exist any more. Instead there is
a new function clock_ticks_to_ns().

The idea of the new function is that clocks don't necessarily
have a period which is a whole number of nanoseconds, so
doing arithmetic on the return value from clock_get_ns()
introduces rounding errors, especially in the case of
"multiply clock_get_ns() by a tick count to get a duration".
(The worst case for this is "clock frequency >1GHz", at which
point the rounding means that clock_get_ns() returns 0...)

There is as yet no function for "convert duration to
tick count", so code like:
   count = ns / clock_get_ns(t->ctrl->clock);

should probably for the moment call clock_ticks_to_ns()
passing a tick count of 1. But I plan to write a patchset
soon which will avoid the need to do that.

Strictly speaking, even "call clock_ticks_to_ns() and
then multiply by the prescaler value" can introduce
rounding error; to deal with that I think you'd need to
either have an internal Clock object whose period you
adjusted as the prescalar value was updated by the guest,
or else do arithmetic with the return value of clock_get()
(which is in units of 2^-32 ns); I'm not sure either is
worth it.

thanks
-- PMM