mbox series

[0/4] hw/clock: Inline and remove CLOCK_PERIOD_TO_HZ/CLOCK_PERIOD_TO_NS macro

Message ID 20201020182008.2240590-1-f4bug@amsat.org
Headers show
Series hw/clock: Inline and remove CLOCK_PERIOD_TO_HZ/CLOCK_PERIOD_TO_NS macro | expand

Message

Philippe Mathieu-Daudé Oct. 20, 2020, 6:20 p.m. UTC 
Following discussion and analysis from Peter:
https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg05853.html

Last patch is RFC because I doubt using 128-bit is what we want...
but I'm not sure how to round it neither.

Future possible cleanup: un-inline these helpers.

Luc Michel (1):
  hw/core/clock: trace clock values in Hz instead of ns

Philippe Mathieu-Daudé (3):
  qdev-monitor: Display frequencies scaled to SI unit
  hw/clock: Inline and remove CLOCK_PERIOD_TO_HZ()
  hw/clock: Inline and remove CLOCK_PERIOD_TO_NS()

 include/hw/clock.h     | 19 +++++++++++++------
 hw/core/clock.c        |  8 +++++---
 softmmu/qdev-monitor.c |  8 +++++---
 hw/core/trace-events   |  4 ++--
 4 files changed, 25 insertions(+), 14 deletions(-)

-- 
2.26.2

Comments

Peter Maydell Oct. 20, 2020, 8:32 p.m. UTC | #1 
On Tue, 20 Oct 2020 at 19:20, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Since commit 9f2ff99c7f2 ("qdev-monitor: print the device's clock
> with info qtree") we can display the clock frequencies in the
> monitor. Use the recently introduced freq_to_str() to display
> the frequencies using the closest SI unit (human friendlier).
>
> Before:
>
>   (qemu) info qtree
>   [...]
>   dev: xilinx,zynq_slcr, id ""
>     clock-in "ps_clk" freq_hz=3.333333e+07
>     mmio 00000000f8000000/0000000000001000
>
> After:
>
>   dev: xilinx,zynq_slcr, id ""
>     clock-in "ps_clk" freq_hz=33.3 MHz
>     mmio 00000000f8000000/0000000000001000
>
> Reviewed-by: Luc Michel <luc@lmichel.fr>
> Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  softmmu/qdev-monitor.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index bcfb90a08f3..1c5b509aea2 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -747,11 +747,13 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
>          }
>      }
>      QLIST_FOREACH(ncl, &dev->clocks, node) {
> -        qdev_printf("clock-%s%s \"%s\" freq_hz=%e\n",
> +        g_autofree char *freq = NULL;
> +
> +        freq = freq_to_str(clock_get_hz(ncl->clock));

I would prefer it if our clock.h API provided a function
for "return a human readable string representing this clock frequency"
(which can use freq_to_str() under the hood, of course).
clock_get_hz() itself is a dangerous API because it may
not be returning a precise result. In particular if the
clock has a period of greater than 1 second (eg 0.5Hz) then
it will unhelpfully return 0.

Having the clock API provide a human-readable representation
for debug purposes means it can internally produce a nice
result regardless of the period, and it's clear to callers
that what they get back is for debug printing and similar
tracing and not suitable for doing arithmetic on. And it
reduces the number of places that call clock_get_hz() which
will make it easier for us to try to delete it entirely.

> +        qdev_printf("clock-%s%s \"%s\" freq_hz=%s\n",
>                      ncl->output ? "out" : "in",
>                      ncl->alias ? " (alias)" : "",
> -                    ncl->name,
> -                    CLOCK_PERIOD_TO_HZ(1.0 * clock_get(ncl->clock)));
> +                    ncl->name, freq);
>      }
>      class = object_get_class(OBJECT(dev));
>      do {
> --
> 2.26.2

thanks
-- PMM