| Message ID | 20201020182008.2240590-5-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/clock: Inline and remove CLOCK_PERIOD_TO_HZ/CLOCK_PERIOD_TO_NS macro | expand |
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
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; } /**
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(-)