Re: [PATCH 1/4] qdev-monitor: Display frequencies scaled to SI unit

[Peter Maydell]

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