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

[Laurent Vivier]

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 ?
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);
>  
>