From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35538) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZMH0n-0005Vu-3i for qemu-devel@nongnu.org; Mon, 03 Aug 2015 10:46:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZMH0i-0006jr-Ai for qemu-devel@nongnu.org; Mon, 03 Aug 2015 10:46:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35929) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZMH0i-0006jg-2Y for qemu-devel@nongnu.org; Mon, 03 Aug 2015 10:46:40 -0400 Date: Mon, 3 Aug 2015 15:46:37 +0100 From: "Richard W.M. Jones" Message-ID: <20150803144637.GQ29283@redhat.com> References: <1438609948-3744-1-git-send-email-lvivier@redhat.com> <55BF7A7E.1050303@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55BF7A7E.1050303@redhat.com> Subject: Re: [Qemu-devel] [PATCH] i6300esb: correctly convert watchdog clock ticks into nanoseconds List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier Cc: Paolo Bonzini , qemu-devel@nongnu.org, David Gibson 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 > > --- > > 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