| Message ID | 1438609948-3744-1-git-send-email-lvivier@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <lvivier@redhat.com> > --- > 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 ? 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) Any comment ? > > i6300esb_debug("stage %d, timeout %" PRIi64 "\n", d->stage, timeout); > >
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 <lvivier@redhat.com> > > --- > > 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.
On 03/08/2015 16:46, Richard W.M. Jones wrote: >> > 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 found that the spec allows for any speed up to 33333333 Hz (30 ns cycle), so both are okay. However, at least hw/net/rtl8139.c assumes it's 33000000 Hz, so it's nice to be consistent. Paolo
On 03/08/2015 16:46, Richard W.M. Jones wrote: > 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 <lvivier@redhat.com> >>> --- >>> 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. So we can multiply by 30 :) > >> 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. I've seen that, but it seems it always overflows and the maximum value seems to be 256 seconds. The second parameters of muldiv64() must be uint32 whereas "timeout" is int64_t (real size is 35 bit long): static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c) Laurent
On 03/08/2015 17:02, Paolo Bonzini wrote: > > > On 03/08/2015 16:46, Richard W.M. Jones wrote: >>>> 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 found that the spec allows for any speed up to 33333333 Hz (30 ns > cycle), so both are okay. However, at least hw/net/rtl8139.c assumes > it's 33000000 Hz, so it's nice to be consistent. So, do you want I resend a patch with "1000 / 33" instead ? We can also update hw/net/rtl8139.c to replace "muldiv64(X, get_ticks_per_sec(), 33000000)" by "X * 30" ? Laurent
On 03/08/2015 17:13, Laurent Vivier wrote: >>> >> it says "33 MHz clock (30 ns clock ticks)" which is contradictory. >> > >> > I found that the spec allows for any speed up to 33333333 Hz (30 ns >> > cycle), so both are okay. However, at least hw/net/rtl8139.c assumes >> > it's 33000000 Hz, so it's nice to be consistent. > So, do you want I resend a patch with "1000 / 33" instead ? > > We can also update hw/net/rtl8139.c to replace "muldiv64(X, > get_ticks_per_sec(), 33000000)" by "X * 30" ? Either would do. If you modify hw/net/rtl8139.c you have to modify CLK in tests/test-rtl8139.c as well. Paolo
On 03/08/2015 17:18, Paolo Bonzini wrote: > > > On 03/08/2015 17:13, Laurent Vivier wrote: >>>>>> it says "33 MHz clock (30 ns clock ticks)" which is contradictory. >>>> >>>> I found that the spec allows for any speed up to 33333333 Hz (30 ns >>>> cycle), so both are okay. However, at least hw/net/rtl8139.c assumes >>>> it's 33000000 Hz, so it's nice to be consistent. >> So, do you want I resend a patch with "1000 / 33" instead ? >> >> We can also update hw/net/rtl8139.c to replace "muldiv64(X, >> get_ticks_per_sec(), 33000000)" by "X * 30" ? > > Either would do. If you modify hw/net/rtl8139.c you have to modify CLK > in tests/test-rtl8139.c as well. I like the idea. I will. I guess you mean tests/rtl8139-test.c What is the "in_Timer()" function ? Laurent
On 03/08/2015 17:35, Laurent Vivier wrote: > > I guess you mean tests/rtl8139-test.c > > What is the "in_Timer()" function ? There's some awful macro magic: #define PORT(name, len, val) \ static unsigned __attribute__((unused)) in_##name(void) \ { \ unsigned res = qpci_io_read##len(dev, dev_base+(val)); \ g_test_message("*%s -> %x\n", #name, res); \ return res; \ } \ static void out_##name(unsigned v) \ { \ g_test_message("%x -> *%s\n", v, #name); \ qpci_io_write##len(dev, dev_base+(val), v); \ } PORT(Timer, l, 0x48) PORT(IntrMask, w, 0x3c) PORT(IntrStatus, w, 0x3E) PORT(TimerInt, l, 0x54) Pa##olo
On Mon, Aug 03, 2015 at 03:52:28PM +0200, Laurent Vivier wrote: > + /* 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 see that you've just posted a v2 of this patch. However here are the results of testing the above version. I used the attached test script which automates things, mostly. All times are in seconds. Requested timeout Observed timeout 60 58 120 120 250 249 270 271 500 501 [note 1] 520 522 1010 1016 1030 1035 2046 2058 2500 ioctl: WDIOC_SETTIMEOUT: error setting timeout: Invalid argument [note 2] [note 1] I'm not worried about the timeout being off by a few seconds, as that could easily be caused by inaccuracies in the test framework. [note 2] Maximum setting for i6300esb Linux driver is 2046, see linux.git/drivers/watchdog/i6300esb.c but note that the printed messages in the driver relating to the range of the timeout are not accurate. This patch looks good to me. I will test the v2 patch shortly. Rich.
On 04/08/2015 12:13, Richard W.M. Jones wrote: > On Mon, Aug 03, 2015 at 03:52:28PM +0200, Laurent Vivier wrote: >> + /* 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 see that you've just posted a v2 of this patch. However here are > the results of testing the above version. I used the attached test > script which automates things, mostly. > > All times are in seconds. > > Requested timeout Observed timeout > 60 58 > 120 120 > 250 249 > 270 271 > 500 501 [note 1] > 520 522 > 1010 1016 > 1030 1035 > 2046 2058 > 2500 ioctl: WDIOC_SETTIMEOUT: error setting timeout: Invalid argument > [note 2] Thank you Rich. > [note 1] I'm not worried about the timeout being off by a few seconds, > as that could easily be caused by inaccuracies in the test framework. The driver put 1024 in the counter for 1 second. So for 520 seconds, we have ((520 * 1024) << 15) * 30 ns = 523 seconds 2036 seconds ((2046 * 1024) << 15) * 30 ns = 2059 seconds So the emulation seems correct. > [note 2] Maximum setting for i6300esb Linux driver is 2046, see > linux.git/drivers/watchdog/i6300esb.c but note that the printed > messages in the driver relating to the range of the timeout are not > accurate. > This patch looks good to me. I will test the v2 patch shortly. I have no preference of which patch to include in QEMU. This one is nicer but request the same kind of modification in other devices, the V2 is a trivial bug fix (currently timer overflow at 256 seconds). I just want to have the bug fixed... Laurent
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; i6300esb_debug("stage %d, timeout %" PRIi64 "\n", d->stage, timeout);
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 <lvivier@redhat.com> --- hw/watchdog/wdt_i6300esb.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-)