Re: [PULL 09/31] hw/core/clock: introduce clock object

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

On 10/17/20 1:47 PM, Philippe Mathieu-Daudé wrote:
> Hi Damien, Peter,
> 
> On 4/30/20 1:51 PM, Peter Maydell wrote:
>> This object may be used to represent a clock inside a clock tree.
>>
>> A clock may be connected to another clock so that it receives update,
>> through a callback, whenever the source/parent clock is updated.
>>
>> Although only the root clock of a clock tree controls the values
>> (represented as periods) of all clocks in tree, each clock holds
>> a local state containing the current value so that it can be fetched
>> independently. It will allows us to fullfill migration requirements
>> by migrating each clock independently of others.
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>> Message-id: 20200406135251.157596-2-damien.hedde@greensocs.com
>> [PMM: Use uint64_t rather than unsigned long long in trace events;
>>   the dtrace backend can't handle the latter]
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   hw/core/Makefile.objs |   1 +
>>   include/hw/clock.h    | 216 ++++++++++++++++++++++++++++++++++++++++++
>>   hw/core/clock.c       | 130 +++++++++++++++++++++++++
>>   hw/core/trace-events  |   7 ++
>>   4 files changed, 354 insertions(+)
>>   create mode 100644 include/hw/clock.h
>>   create mode 100644 hw/core/clock.c
>>
>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>> index 6215e7c2085..1d9b0aa2057 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -7,6 +7,7 @@ common-obj-y += hotplug.o
>>   common-obj-y += vmstate-if.o
>>   # irq.o needed for qdev GPIO handling:
>>   common-obj-y += irq.o
>> +common-obj-y += clock.o
>>   common-obj-$(CONFIG_SOFTMMU) += reset.o
>>   common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>> diff --git a/include/hw/clock.h b/include/hw/clock.h
>> new file mode 100644
>> index 00000000000..82a7f3c6982
>> --- /dev/null
>> +++ b/include/hw/clock.h
>> @@ -0,0 +1,216 @@
>> +/*
>> + * Hardware Clocks
>> + *
>> + * Copyright GreenSocs 2016-2020
>> + *
>> + * Authors:
>> + *  Frederic Konrad
>> + *  Damien Hedde
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>> later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef QEMU_HW_CLOCK_H
>> +#define QEMU_HW_CLOCK_H
>> +
>> +#include "qom/object.h"
>> +#include "qemu/queue.h"
>> +
>> +#define TYPE_CLOCK "clock"
>> +#define CLOCK(obj) OBJECT_CHECK(Clock, (obj), TYPE_CLOCK)
>> +
>> +typedef void ClockCallback(void *opaque);
>> +
>> +/*
>> + * clock store a value representing the clock's period in 2^-32ns unit.
>> + * It can represent:
>> + *  + periods from 2^-32ns up to 4seconds
>> + *  + frequency from ~0.25Hz 2e10Ghz
>> + * Resolution of frequency representation decreases with frequency:
>> + * + at 100MHz, resolution is ~2mHz
>> + * + at 1Ghz,   resolution is ~0.2Hz
>> + * + at 10Ghz,  resolution is ~20Hz
>> + */
>> +#define CLOCK_SECOND (1000000000llu << 32)
>> +
>> +/*
>> + * macro helpers to convert to hertz / nanosecond
>> + */
>> +#define CLOCK_PERIOD_FROM_NS(ns) ((ns) * (CLOCK_SECOND / 1000000000llu))
>> +#define CLOCK_PERIOD_TO_NS(per) ((per) / (CLOCK_SECOND / 1000000000llu))
>> +#define CLOCK_PERIOD_FROM_HZ(hz) (((hz) != 0) ? CLOCK_SECOND / (hz) : 
>> 0u)
> 
> I'm having Floating Point Exception using a frequency of 1GHz.
> 
> Using frequency >=1GHz we have CLOCK_PERIOD_FROM_HZ(hz) > 0x100000000.
> 
> Then CLOCK_PERIOD_TO_NS(0x100000000) = 0.
> 
> So for frequency >=1GHz clock_get_ns() returns 0.

So Peter suggested on IRC to rewrite the code consuming this API
to avoid reaching this limit :)

Still some assert would help other developers triggering the same
issue to quicker figure how to bypass the problem.

> 
>> +#define CLOCK_PERIOD_TO_HZ(per) (((per) != 0) ? CLOCK_SECOND / (per) 
>> : 0u)
>> +
> [...]
>