On Thu, Jan 7, 2021 at 12:51 PM Peter Maydell <peter.mayd...@linaro.org>
wrote:
> On Thu, 17 Dec 2020 at 00:45, Hao Wu <wuhao...@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 <hskinnem...@google.com>
> > Reviewed-by: Tyrone Ting <kft...@nuvoton.com>
> > Signed-off-by: Hao Wu <wuhao...@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.
>
In this particular case, rounding error is less of a concern since the
clock should be ~25MHz (in the old implementation it was a fixed value.)
Since the prescaler is always an integer, a possible alternative might be
ns = clock_ticks_to_ns(t->ctrl->clock, count *
npcm7xx_tcsr_prescaler(t->tcsr))
and for code to convert ns to count can go
count = ns / clock_ticks_to_ns(t->ctrl->clock,
npcm7xx_tcsr_prescaler(t->tcsr))
or use the new API you proposed.
We'll also need to apply similar changes to other places in the patchset
(ADC and/or PWM) that need to compute clock value.
>
> thanks
> -- PMM
>
