[RFC 0/4] New APIs for the Clock framework

* [RFC 0/4] New APIs for the Clock framework
@ 2021-02-01 12:30 Peter Maydell
  2021-02-01 12:30 ` [RFC 1/4] clock: Add ClockEvent parameter to callbacks Peter Maydell
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Peter Maydell @ 2021-02-01 12:30 UTC (permalink / raw)
  To: qemu-arm, qemu-devel
  Cc: Edgar E. Iglesias, Philippe Mathieu-Daudé,
	Luc Michel, Havard Skinnemoen, Tyrone Ting

Hi; this patchset proposes a couple of new APIs for Clock, which I
found I needed/wanted for a work-in-progress patchset that includes a
model of a new timer device, the "System Timer" documented in the Arm
SSE-123 Example Subsystem Technical Reference Manual:
  https://developer.arm.com/documentation/101370/0000/system-time-components/system-timer?lang=en

It's going to be a little while before I post the patchseries with the
new timer device implementation, because I need to complete a model of
the SSE-300 SoC and an MPS board model that uses it; so I wanted to
put these out as an RFC to see if people agree on the APIs I'm
suggesting.

The problem the first two patches are trying to solve is that I found
that I wanted the old value of the Clock's period when my device got a
notification about a frequency/period change. The current
ClockCallback API calls you after the period has changed, so the Clock
is already using the new period. I wanted this because my timer device
has a register that's basically a free-running up-counter; the value
of that counter can be calculated with:

  s->ticks_then + clock_ns_to_ticks(s->clk, now - s->ns_then);

where (ns_then, ticks_then) are a tuple of a QEMU_CLOCK_VIRTUAL time
and the tick count at that point. Whenever the clock frequency changes
we calculate a new (ns_then, ticks_then) to use as the baseline for
future counter value reads, but in order to do that we must calculate
ticks_then using the *old* clock period.

My solution to this is to add a ClockEvent argument to the callback
function, which is an enum:

  ClockPreUpdate : callback called before period change
  ClockUpdate : callback called after period change

All callback functions are called for all events, and they look at the
'event' argument to see whether they need to do anything.  This means
that the patch has to update every ClockCallback in the codebase to
take the new argument and do nothing if it is not ClockUpdate, but
luckily there aren't many of them; this seemed better than trying to
manage multiple separate callback pointers.

The problem the third patch addresses is that we don't have a function
for "tell me how many times this clock would tick in this length of
time". clock_ns_to_ticks() does the inverse of the clock_ticks_to_ns()
that we already have. Two points in particular where comment would be
useful:

 * I chose to make the overflow case (where a clock has a very short
   period and the specified length of time is very long, so the clock
   would tick more than UINT64_MAX times) just let the value wrap
   around, on the basis that usually this is being used to calculate a
   guest register value that's in a 64 bit or 32 bit register, and so
   wrap-around is the right behaviour.  But I'm not 100% set on this
   if somebody has a better idea.

 * The calculation needs to do a 96-bit / 64 bit => 64 bit division,
   for which the best thing we have is divu128(). This is particularly
   painful on 32-bit hosts. I don't suppose there's anything clever we
   can do to make this better ?

Patch 4 just uses clock_ns_to_ticks() in the one place in the
current codebase where we're currently using clock_ticks_to_ns()
and manual calculation.

Side note: there is currently no MAINTAINERS entry for the
clock framework. Any volunteers? It would cover

F: include/hw/clock.h
F: include/hw/qdev-clock.h
F: hw/core/clock.c
F: hw/core/qdev-clock.c
F: docs/devel/clocks.rst

thanks
-- PMM

Peter Maydell (4):
  clock: Add ClockEvent parameter to callbacks
  clock: Add ClockPreUpdate callback event type
  clock: Add clock_ns_to_ticks() function
  hw/timer/npcm7xx_timer: Use new clock_ns_to_ticks()

 docs/devel/clocks.rst            | 54 ++++++++++++++++++++++++++++++--
 include/hw/clock.h               | 52 +++++++++++++++++++++++++++++-
 hw/arm/armsse.c                  |  8 +++--
 hw/char/cadence_uart.c           |  5 ++-
 hw/char/ibex_uart.c              |  5 ++-
 hw/char/pl011.c                  |  5 ++-
 hw/core/clock.c                  |  5 ++-
 hw/misc/bcm2835_cprman.c         | 20 +++++++++---
 hw/misc/npcm7xx_clk.c            | 31 ++++++++++++++++--
 hw/misc/zynq_slcr.c              |  6 +++-
 hw/timer/cmsdk-apb-dualtimer.c   |  5 ++-
 hw/timer/cmsdk-apb-timer.c       |  5 ++-
 hw/timer/npcm7xx_timer.c         |  4 +--
 hw/watchdog/cmsdk-apb-watchdog.c |  5 ++-
 14 files changed, 188 insertions(+), 22 deletions(-)

-- 
2.20.1

^ permalink raw reply	[flat|nested] 8+ messages in thread