QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Damien Hedde <damien.hedde@greensocs.com>,
	Peter Maydell <peter.maydell@linaro.org>
Cc: "Daniel P. Berrange" <berrange@redhat.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	"Alistair Francis" <alistair@alistair23.me>,
	"Mark Burton" <mark.burton@greensocs.com>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	qemu-arm <qemu-arm@nongnu.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Subject: Re: [PATCH v6 0/9] Clock framework API
Date: Wed, 4 Dec 2019 21:34:33 +0100
Message-ID: <febdb64d-9d02-66ad-84d2-dc795879e48b@redhat.com> (raw)
In-Reply-To: <be199ad6-1b79-f77d-afad-faeb042151ed@greensocs.com>

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.



  reply index

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-04 12:55 [Qemu-devel] " Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 1/9] hw/core/clock: introduce clock objects Damien Hedde
2019-11-25 13:07   ` Philippe Mathieu-Daudé
2019-11-25 13:37   ` Philippe Mathieu-Daudé
2019-12-03 15:14     ` Damien Hedde
2019-12-02 13:42   ` Peter Maydell
2019-12-03 15:28     ` Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 2/9] hw/core/clock-vmstate: define a vmstate entry for clock state Damien Hedde
2019-11-25 13:05   ` Philippe Mathieu-Daudé
2019-12-02 13:44   ` Peter Maydell
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 3/9] qdev: add clock input&output support to devices Damien Hedde
2019-11-25 13:30   ` Philippe Mathieu-Daudé
2019-12-03 15:35     ` Damien Hedde
2019-12-02 14:34   ` Peter Maydell
2019-12-04  9:05     ` Damien Hedde
2019-12-04  9:53       ` Philippe Mathieu-Daudé
2019-12-04 11:58         ` Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 4/9] qdev-monitor: print the device's clock with info qtree Damien Hedde
2019-12-02 14:35   ` Peter Maydell
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 5/9] qdev-clock: introduce an init array to ease the device construction Damien Hedde
2019-12-02 15:13   ` Peter Maydell
2019-12-04 11:04     ` Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 6/9] docs/clocks: add device's clock documentation Damien Hedde
2019-12-02 15:17   ` Peter Maydell
2019-12-04 12:11     ` Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 7/9] hw/misc/zynq_slcr: add clock generation for uarts Damien Hedde
2019-12-02 15:20   ` Peter Maydell
2019-12-04 12:51     ` Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 8/9] hw/char/cadence_uart: add clock support Damien Hedde
2019-12-02 15:24   ` Peter Maydell
2019-12-04 13:35     ` Damien Hedde
2019-09-04 12:55 ` [Qemu-devel] [PATCH v6 9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr Damien Hedde
2019-12-02 15:34   ` Peter Maydell
2019-12-03 14:59     ` Damien Hedde
2019-12-03 15:29       ` Philippe Mathieu-Daudé
2019-12-02 16:15 ` [PATCH v6 0/9] Clock framework API Peter Maydell
2019-12-04 16:40   ` Damien Hedde
2019-12-04 20:34     ` Philippe Mathieu-Daudé [this message]
2019-12-05  9:36       ` Damien Hedde
2019-12-05  9:59         ` Philippe Mathieu-Daudé
2019-12-05 10:21           ` Dr. David Alan Gilbert
2019-12-05 10:44             ` Philippe Mathieu-Daudé
2019-12-05 10:56               ` Dr. David Alan Gilbert
2019-12-05 11:01                 ` Philippe Mathieu-Daudé
2019-12-06 12:46                   ` Cleber Rosa
2019-12-06 13:48                     ` Dr. David Alan Gilbert

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=febdb64d-9d02-66ad-84d2-dc795879e48b@redhat.com \
    --to=philmd@redhat.com \
    --cc=alistair@alistair23.me \
    --cc=berrange@redhat.com \
    --cc=damien.hedde@greensocs.com \
    --cc=edgar.iglesias@gmail.com \
    --cc=ehabkost@redhat.com \
    --cc=marcandre.lureau@redhat.com \
    --cc=mark.burton@greensocs.com \
    --cc=pbonzini@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git