This patchseries makes some changes to the clock API:
 * Remove clock_get_ns()
 * Add clock_ticks_to_ns() to return number of nanoseconds
   it will take the clock to tick N times
 * clock_display_freq() to return prettily-formatted string
   for showing humans the approximate clock frequency

This is based on discussions we had about these APIs a little while
back.  The core driver here is that the clock objects internally
store the period in units of 2^-32 ns, so both clock_get_ns() and
clock_get_hz() are inherently returning a rounded-off result, which
can be badly inaccurate for fast clocks or if you want to multiply it
by a large tick count.

Ideally I'd like to get rid of clock_get_hz() as well, but
that looks trickier than handling clock_get_ns().

Patch 4 borrows a lot of the concept from one of Philippe's that he
sent out previously.

NB: tested with 'make check' and 'make check-acceptance' only.

thanks
-- PMM

Peter Maydell (4):
  clock: Introduce clock_ticks_to_ns()
  target/mips: Don't use clock_get_ns() in clock period calculation
  clock: Remove clock_get_ns()
  clock: Define and use new clock_display_freq()

 docs/devel/clocks.rst  | 37 +++++++++++++++++++++++++++++++++----
 include/hw/clock.h     | 41 ++++++++++++++++++++++++++++++++++++++---
 hw/core/clock.c        |  6 ++++++
 softmmu/qdev-monitor.c |  6 +++---
 target/mips/cpu.c      |  4 ++--
 5 files changed, 82 insertions(+), 12 deletions(-)

-- 
2.20.1

On 12/8/20 7:15 PM, Peter Maydell wrote:
> This patchseries makes some changes to the clock API:
>  * Remove clock_get_ns()
>  * Add clock_ticks_to_ns() to return number of nanoseconds
>    it will take the clock to tick N times
>  * clock_display_freq() to return prettily-formatted string
>    for showing humans the approximate clock frequency
> 
> This is based on discussions we had about these APIs a little while
> back.  The core driver here is that the clock objects internally
> store the period in units of 2^-32 ns, so both clock_get_ns() and
> clock_get_hz() are inherently returning a rounded-off result, which
> can be badly inaccurate for fast clocks or if you want to multiply it
> by a large tick count.
> 
> Ideally I'd like to get rid of clock_get_hz() as well, but
> that looks trickier than handling clock_get_ns().
> 
> Patch 4 borrows a lot of the concept from one of Philippe's that he
> sent out previously.

Thanks for tackling the clock_get_ns() part. I had some work in
progress I was procrastinating for after the release, but your
patches are much better documented :)

(I also started to get rid of clock_get_hz() but, as you figured,
this is not a trivial task).

> NB: tested with 'make check' and 'make check-acceptance' only.

I hit this issue while testing Huacai's MIPS Loongson3 virt machine
which sets the core freq at 1GHz IIRC. I still have the branch
so I'll test your series (or v2) during the week-end.

Regards,

Phil.

On Fri, 11 Dec 2020 at 13:50, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> (I also started to get rid of clock_get_hz() but, as you figured,
> this is not a trivial task).


Yeah; I haven't really looked at the users of clock_get_hz()
in detail to know whether it's really possible to remove it.
For the serial devices to some extent they really do want a
frequency to feed to the host serial baud-rate stuff...

Also, I have a timer device I'm working on which has a register
for "give number of ticks since simulation start" which should
thus read a value something like
 qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)  / clock_ticks_to_ns(clk, 1);

and this also suffers from possible rounding issues (though not
to the same extent as ticks-to-ns since it's a division rather
than a multiplication). I'm wondering if we should have a clock API
for "convert a duration in nanoseconds to a tick count" directly
as well. Dunno whether that helps with the clock_get_hz() use
cases, or if it's orthogonal to that.

thanks
-- PMM