Re: [Qemu-devel] [PATCH] i6300esb: correctly convert watchdog clock ticks into nanoseconds

[Richard W.M. Jones]

On Mon, Aug 03, 2015 at 04:28:14PM +0200, Laurent Vivier wrote:
> 
> 
> On 03/08/2015 15:52, Laurent Vivier wrote:
> > Originally, qemu_mod_timer() was using ticks to count time.
> > And i6300esb was converting internal clock ticks (33 MHz) to
> > QEMU timer ticks.
> > 
> > The timer has been changed by a script to use nanoseconds:
> > 
> >     7447545 change all other clock references to use
> >             nanosecond resolution accessors
> > 
> > As i6300esb takes nanoseconds, we don't need anymore to
> > multiply counter by get_ticks_per_sec()/33MHz, but instead
> > we must convert watchdog ticks into nanoseconds.
> > 
> > Signed-off-by: Laurent Vivier <address@hidden>
> > ---
> >  hw/watchdog/wdt_i6300esb.c | 25 +++++++++++++++----------
> >  1 file changed, 15 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
> > index cfa2b1b..21119ab 100644
> > --- a/hw/watchdog/wdt_i6300esb.c
> > +++ b/hw/watchdog/wdt_i6300esb.c
> > @@ -124,19 +124,24 @@ static void i6300esb_restart_timer(I6300State *d, int 
> > stage)
> >      else
> >          timeout = d->timer2_preload;
> >  
> > -    if (d->clock_scale == CLOCK_SCALE_1KHZ)
> > +    /* convert timeout to 33Mhz clock ticks */
> > +    if (d->clock_scale == CLOCK_SCALE_1KHZ) {
> > +        /* The 20-bit Preload Value is loaded into bits 34:15 of the
> > +         * main down counter. [...] The approximate clock generated
> > +         * is 1 KHz, (Default)
> > +         */
> >          timeout <<= 15;
> > -    else
> > +    } else {
> > +        /* The 20-bit Preload Value is loaded into bits 24:5 of the
> > +         * main down counter. [...] The approximate clock generated
> > +         * is 1 MHz.
> > +         */
> >          timeout <<= 5;
> > -
> > -    /* Get the timeout in units of ticks_per_sec.
> > -     *
> > -     * ticks_per_sec is typically 10^9 == 0x3B9ACA00 (30 bits), with
> > -     * 20 bits of user supplied preload, and 15 bits of scale, the
> > -     * multiply here can exceed 64-bits, before we divide by 33MHz, so
> > -     * we use a higher-precision intermediate result.
> > +    }
> > +    /* A 33 Mhz clock gives a 30 ns tick,
> > +     * convert timeout from ticks to ns
> >       */
> > -    timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000);
> > +    timeout *= 30;
> 
> I'm wondering if a 33 Mhz clock is 33000000 Hz or 33333333 Hz ?

>From the datasheet (chapter 16):

https://www-ssl.intel.com/content/www/us/en/chipsets/6300esb-io-controller-hub-datasheet.html

it says "33 MHz clock (30 ns clock ticks)" which is contradictory.

I suspect it does really mean 33,333,333 Mz (== 30 ns ticks) since
that is the base clock speed discussed elsewhere in that document.

> if this is the former, I should use "timeout = timeout * 1000 / 33" instead.
> (35 bit value * 10 bit value = 45 bit value, it fits in a 64 bit integer)

See also commit 4bc7b4d56657ebf75b986ad46e959cf7232ff26a and the
discussion here:

https://lists.gnu.org/archive/html/qemu-ppc/2015-03/threads.html#00448

The original code did `get_ticks_per_sec() * timeout / 33000000' which
definitely overflowed.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW