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

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

[Philippe Mathieu-Daudé]

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

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

[Philippe Mathieu-Daudé]

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));
+        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

[PATCH 2/4] hw/core/clock: trace clock values in Hz instead of ns

[Philippe Mathieu-Daudé]

From: Luc Michel <luc@lmichel.fr>

The nanosecond unit greatly limits the dynamic range we can display in
clock value traces, for values in the order of 1GHz and more. The
internal representation can go way beyond this value and it is quite
common for today's clocks to be within those ranges.

For example, a frequency between 500MHz+ and 1GHz will be displayed as
1ns. Beyond 1GHz, it will show up as 0ns.

Replace nanosecond periods traces with frequencies in the Hz unit
to have more dynamic range in the trace output.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
Signed-off-by: Luc Michel <luc@lmichel.fr>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20201010135759.437903-3-luc@lmichel.fr>
[PMD: Replace CLOCK_PERIOD_TO_HZ() by clock_get_hz()]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/core/clock.c      | 8 +++++---
 hw/core/trace-events | 4 ++--
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/core/clock.c b/hw/core/clock.c
index f866717a835..5cc9599ba3a 100644
--- a/hw/core/clock.c
+++ b/hw/core/clock.c
@@ -51,12 +51,14 @@ void clock_clear_callback(Clock *clk)
 
 bool clock_set(Clock *clk, uint64_t period)
 {
+    uint64_t old_hz;
+
     if (clk->period == period) {
         return false;
     }
-    trace_clock_set(CLOCK_PATH(clk), CLOCK_PERIOD_TO_NS(clk->period),
-                    CLOCK_PERIOD_TO_NS(period));
+    old_hz = clock_get_hz(clk);
     clk->period = period;
+    trace_clock_set(CLOCK_PATH(clk), old_hz, clock_get_hz(clk));
 
     return true;
 }
@@ -69,7 +71,7 @@ static void clock_propagate_period(Clock *clk, bool call_callbacks)
         if (child->period != clk->period) {
             child->period = clk->period;
             trace_clock_update(CLOCK_PATH(child), CLOCK_PATH(clk),
-                               CLOCK_PERIOD_TO_NS(clk->period),
+                               clock_get_hz(clk),
                                call_callbacks);
             if (call_callbacks && child->callback) {
                 child->callback(child->callback_opaque);
diff --git a/hw/core/trace-events b/hw/core/trace-events
index 1ac60ede6b7..7b2869cbaab 100644
--- a/hw/core/trace-events
+++ b/hw/core/trace-events
@@ -31,6 +31,6 @@ resettable_transitional_function(void *obj, const char *objtype) "obj=%p(%s)"
 # clock.c
 clock_set_source(const char *clk, const char *src) "'%s', src='%s'"
 clock_disconnect(const char *clk) "'%s'"
-clock_set(const char *clk, uint64_t old, uint64_t new) "'%s', ns=%"PRIu64"->%"PRIu64
+clock_set(const char *clk, uint64_t old, uint64_t new) "'%s', %"PRIu64" Hz-> %"PRIu64" Hz"
 clock_propagate(const char *clk) "'%s'"
-clock_update(const char *clk, const char *src, uint64_t val, int cb) "'%s', src='%s', ns=%"PRIu64", cb=%d"
+clock_update(const char *clk, const char *src, uint64_t hz, int cb) "'%s', src='%s', val=%"PRIu64" Hz cb=%d"
-- 
2.26.2

[PATCH 3/4] hw/clock: Inline and remove CLOCK_PERIOD_TO_HZ()

[Philippe Mathieu-Daudé]

This macro is only used once. Inline and remove it.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/clock.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/hw/clock.h b/include/hw/clock.h
index cbc5e6ced1e..b58038f1e7d 100644
--- a/include/hw/clock.h
+++ b/include/hw/clock.h
@@ -40,7 +40,6 @@ typedef void ClockCallback(void *opaque);
 #define CLOCK_PERIOD_FROM_NS(ns) ((ns) * (CLOCK_PERIOD_1SEC / 1000000000llu))
 #define CLOCK_PERIOD_TO_NS(per) ((per) / (CLOCK_PERIOD_1SEC / 1000000000llu))
 #define CLOCK_PERIOD_FROM_HZ(hz) (((hz) != 0) ? CLOCK_PERIOD_1SEC / (hz) : 0u)
-#define CLOCK_PERIOD_TO_HZ(per) (((per) != 0) ? CLOCK_PERIOD_1SEC / (per) : 0u)
 
 /**
  * Clock:
@@ -203,9 +202,12 @@ static inline uint64_t clock_get(const Clock *clk)
     return clk->period;
 }
 
-static inline unsigned clock_get_hz(Clock *clk)
+static inline uint64_t clock_get_hz(Clock *clk)
 {
-    return CLOCK_PERIOD_TO_HZ(clock_get(clk));
+    if (!clk->period) {
+        return 0u;
+    }
+    return CLOCK_PERIOD_1SEC / clk->period;
 }
 
 static inline unsigned clock_get_ns(Clock *clk)
-- 
2.26.2

[RFC PATCH 4/4] hw/clock: Inline and remove CLOCK_PERIOD_TO_NS()

[Philippe Mathieu-Daudé]

This macro is only used once. Inline caring about 64-bit
multiplication, and remove it.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/clock.h | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/hw/clock.h b/include/hw/clock.h
index b58038f1e7d..f329fcf0ea5 100644
--- a/include/hw/clock.h
+++ b/include/hw/clock.h
@@ -16,6 +16,7 @@
 
 #include "qom/object.h"
 #include "qemu/queue.h"
+#include "qemu/host-utils.h"
 
 #define TYPE_CLOCK "clock"
 OBJECT_DECLARE_SIMPLE_TYPE(Clock, CLOCK)
@@ -38,7 +39,6 @@ typedef void ClockCallback(void *opaque);
  * macro helpers to convert to hertz / nanosecond
  */
 #define CLOCK_PERIOD_FROM_NS(ns) ((ns) * (CLOCK_PERIOD_1SEC / 1000000000llu))
-#define CLOCK_PERIOD_TO_NS(per) ((per) / (CLOCK_PERIOD_1SEC / 1000000000llu))
 #define CLOCK_PERIOD_FROM_HZ(hz) (((hz) != 0) ? CLOCK_PERIOD_1SEC / (hz) : 0u)
 
 /**
@@ -210,9 +210,14 @@ static inline uint64_t clock_get_hz(Clock *clk)
     return CLOCK_PERIOD_1SEC / clk->period;
 }
 
-static inline unsigned clock_get_ns(Clock *clk)
+static inline uint64_t clock_get_ns(Clock *clk)
 {
-    return CLOCK_PERIOD_TO_NS(clock_get(clk));
+    uint64_t lo, hi;
+
+    mulu64(&lo, &hi, clock_get(clk), 1000000000llu);
+    divu128(&lo, &hi, CLOCK_PERIOD_1SEC);
+
+    return lo;
 }
 
 /**
-- 
2.26.2

Re: [RFC PATCH 4/4] hw/clock: Inline and remove CLOCK_PERIOD_TO_NS()

[Peter Maydell]

On Tue, 20 Oct 2020 at 19:20, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> This macro is only used once. Inline caring about 64-bit
> multiplication, and remove it.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/clock.h | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> index b58038f1e7d..f329fcf0ea5 100644
> --- a/include/hw/clock.h
> +++ b/include/hw/clock.h
> @@ -16,6 +16,7 @@
>
>  #include "qom/object.h"
>  #include "qemu/queue.h"
> +#include "qemu/host-utils.h"
>
>  #define TYPE_CLOCK "clock"
>  OBJECT_DECLARE_SIMPLE_TYPE(Clock, CLOCK)
> @@ -38,7 +39,6 @@ typedef void ClockCallback(void *opaque);
>   * macro helpers to convert to hertz / nanosecond
>   */
>  #define CLOCK_PERIOD_FROM_NS(ns) ((ns) * (CLOCK_PERIOD_1SEC / 1000000000llu))
> -#define CLOCK_PERIOD_TO_NS(per) ((per) / (CLOCK_PERIOD_1SEC / 1000000000llu))
>  #define CLOCK_PERIOD_FROM_HZ(hz) (((hz) != 0) ? CLOCK_PERIOD_1SEC / (hz) : 0u)
>
>  /**
> @@ -210,9 +210,14 @@ static inline uint64_t clock_get_hz(Clock *clk)
>      return CLOCK_PERIOD_1SEC / clk->period;
>  }
>
> -static inline unsigned clock_get_ns(Clock *clk)
> +static inline uint64_t clock_get_ns(Clock *clk)
>  {
> -    return CLOCK_PERIOD_TO_NS(clock_get(clk));
> +    uint64_t lo, hi;
> +
> +    mulu64(&lo, &hi, clock_get(clk), 1000000000llu);
> +    divu128(&lo, &hi, CLOCK_PERIOD_1SEC);
> +
> +    return lo;
>  }

I think the clock_get_ns() function is still flawed
regardless of its return type or internal implementation.
If you have a 2GHz clock then the correct answer is
"0.5" and so an integer representation is going to be
wrong by 0.5ns. If the reason why you wanted the period
in nanoseconds was so you could multiply it by a number
of ticks in order to work out when to set a timer, you
cannot live with the error resulting from rounding it
either way. We need to replace this function with one
which does the whole job of "tell me how many nanoseconds
X ticks of this clock will take" and does the entire
calculation so it can do it without introducing rounding errors.

thanks
-- PMM

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