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

[Laurent Vivier <lvivier@redhat.com>]

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