Re: [PATCH v6 0/9] Clock framework API

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 12/5/19 10:36 AM, Damien Hedde wrote:
> On 12/4/19 9:34 PM, Philippe Mathieu-Daudé wrote:
>> On 12/4/19 5:40 PM, Damien Hedde wrote:
>>> On 12/2/19 5:15 PM, Peter Maydell wrote:
>>>>
>>>> The one topic I think we could do with discussing is whether
>>>> a simple uint64_t giving the frequency of the clock in Hz is
>>>> the right representation. In particular in your patch 9 the
>>>> board has a clock frequency that's not a nice integer number
>>>> of Hz. I think Philippe also mentioned on irc some board where
>>>> the UART clock ends up at a weird frequency. Since the
>>>> representation of the frequency is baked into the migration
>>>> format it's going to be easier to get it right first rather
>>>> than trying to change it later.
>>
>> Important precision for Damien, IIUC we can not migrate float/double types.
>>
>>>> So what should the representation be? Some random thoughts:
>>>>
>>>> 1) ptimer internally uses a 'period plus fraction' representation:
>>>>    int64_t period is the integer part of the period in nanoseconds,
>>>>    uint32_t period_frac is the fractional part of the period
>>>> (if you like you can think of this as "96-bit integer
>>>> period measured in units of one-2^32nd of a nanosecond").
>>>> However its only public interfaces for setting the frequency
>>>> are (a) set the frequency in Hz (uint32_t) or (b) set
>>>> the period in nanoseconds (int64_t); the period_frac part
>>>> is used to handle frequencies which don't work out to
>>>> a nice whole number of nanoseconds per cycle.
>>
>> This is very clear, thanks Peter!
>>
>> The period+period_frac split allow us to migrate the 96 bits:
>>
>>          VMSTATE_UINT32(period_frac, ptimer_state),
>>          VMSTATE_INT64(period, ptimer_state),
>>
>>>> 2) I hear that SystemC uses "value plus a time unit", with
>>>> the smallest unit being a picosecond. (I think SystemC
>>>> also lets you specify the duty cycle, but we definitely
>>>> don't want to get into that!)
>>>
>>> The "value" is internally stored in a 64bits unsigned integer.
>>>
>>>>
>>>> 3) QEMUTimers are basically just nanosecond timers
>>
>> Similarly to SystemC, the QEMUTimers macro use a 'scale' unit, of:
>>
>> #define SCALE_MS 1000000
>> #define SCALE_US 1000
>> #define SCALE_NS 1
>>
>>>>
>>>> 4) The MAME emulator seems to work with periods of
>>>> 96-bit attoseconds (represented internally by a
>>>> 32-bit count of seconds plus a 64-bit count of
>>>> attoseconds). One attosecond is 1e-18 seconds.
>>>>
>>>> Does anybody else have experience with other modelling
>>>> or emulator technology and how it represents clocks ?
>>>
>>> 5) In linux, a clock rate is an "unsigned long" representing Hz.
>>>
>>>>
>>>> I feel we should at least be able to represent clocks
>>>> with the same accuracy that ptimer has.
>>>
>>> Then is a maybe a good idea to store the period and not the frequency in
>>> clocks so that we don't loose anything when we switch from a clock to a
>>> ptimer ?
>>
>> I think storing the period as an integer type is a good idea.
>>
>> However if we store the period in nanoseconds, we get at most 1GHz
>> frequency.
>>
>> The attosecond granularity feels overkill.
>>
>> If we use a 96-bit integer to store picoseconds and use similar SCALE
>> macros we get to 1THz.
>>
>> Regardless the unit chosen, as long it is integer, we can migrate it.
>> If can migrate the period, we don't need to migrate the frequency.
>> We can then use the float type in with the timer API to pass frequencies
>> (which in the modeled hardware are ratios, likely not integers).
>>
>> So we could use set_freq(100e6 / 3), set_freq(40e6 / 5.5) directly.
>>
>>> Regarding the clock, I don't see any strong obstacle to switch
>>> internally to a period based value.
>>> The only things we have to choose is how to represent a disabled clock.
>>> Since putting a "0" period to a ptimer will disable the timer in
>>> ptimer_reload(). We can choose that (and it's a good value because we
>>> can multiply or divide it, it stays the same).
>>>
>>> We could use the same representation as a ptimer. But if we don't keep a
>>> C number representation, then computation of frequencies/periods will be
>>> complicated at best and error prone.
>>>
>>>   From that point of view, if we could stick to a 64bits integer (or
>>> floating point number) it would be great. Can we use a sub nanosecond
>>> unit that fit our needs ?
>>>
>>> I did some test with a unit of 2^-32 of nanoseconds on 64bits (is that
>>> the unit of the ptimer fractional part ?) and if I'm not mistaken
>>> + we have a frequency range from ~0.2Hz up to 10^18Hz
>>> + the resolution is decreasing with the frequency (but at 100Mhz we have
>>> a ~2.3mHz resolution, at 1GHz it's ~0.23Hz and at 10GHz ~23Hz
>>> resolution). We hit 1Hz resolution around 2GHz.
>>>
>>> So it sounds to me we have largely enough resolution to model clocks in
>>> the range of frequencies we will have to handle. What do you think ?
>>
>> Back to your series, I wonder why you want to store the frequency in
>> ClockIn. ClockIn shouldn't be aware at what frequency it is clocked.
>> What matters is ClockOut, and each device exposing ClockOuts has a
>> (migrated) state of the output frequencies (rather in fields, or encoded
>> in registers). Once migrated, after the state is loaded back into the
>> device, we call post_load(). Isn't it a good place to call
>> clock_set_frequency(ClockOut[]) which will correctly set each ClockIn
>> frequency.
>>
>> IOW I don't think ClockIn/ClockOut require to migrate a frequency field.
>>
> 
> I agree it is more logical to store the frequency in clock out. But,
> regarding migration constraints, we have no choice I think because a
> device cannot rely on values that are migrated by another device for
> restoring its state. (when I checked, I add the impression that
> post_load()s are called on a per device migration basis not all at the
> end of migration).

Cc'ing David to clear that out.

> So we could store the frequency in clock out and migrate things there.
> But since we have no way to ensure all clock out states are migrated
> before some device fetch a ClockIn: we'll have to say "don't fetch one
> of your ClockIn frequency during migration and migrate the value
> yourself if you need it", pretty much like gpios.
> 
> So we will probably migrate all ClockOut and almost all ClockIn.
> 
> It would nice if we had a way to ensure clocks are migrated before
> devices try to use them. But I don't think this is possible.
> 
> --
> Damien
>