Re: [PATCH v2 3/4] clock: Add clock_ns_to_ticks() function

[Luc Michel]

On 13:20 Tue 09 Feb     , Peter Maydell wrote:
> Add a clock_ns_to_ticks() function which does the opposite of
> clock_ticks_to_ns(): given a duration in nanoseconds, it returns the
> number of clock ticks that would happen in that time.  This is useful
> for devices that have a free running counter register whose value can
> be calculated when it is read.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Luc Michel <luc@lmichel.fr>

> ---
> I have made the overflow behaviour here be "wrap", with justification
> as per the comment; but I'm not 100% set on this.
> ---
>  docs/devel/clocks.rst | 12 ++++++++++++
>  include/hw/clock.h    | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
> index f0391e76b4f..956bd147ea0 100644
> --- a/docs/devel/clocks.rst
> +++ b/docs/devel/clocks.rst
> @@ -360,6 +360,18 @@ rather than simply passing it to a QEMUTimer function 
> like
>  ``timer_mod_ns()`` then you should be careful to avoid overflow
>  in those calculations, of course.)
>  
> +Obtaining tick counts
> +---------------------
> +
> +For calculations where you need to know the number of ticks in
> +a given duration, use ``clock_ns_to_ticks()``. This function handles
> +possible non-whole-number-of-nanoseconds periods and avoids
> +potential rounding errors. It will return '0' if the clock is stopped
> +(i.e. it has period zero). If the inputs imply a tick count that
> +overflows a 64-bit value (a very long duration for a clock with a
> +very short period) the output value is truncated, so effectively
> +the 64-bit output wraps around.
> +
>  Changing a clock period
>  -----------------------
>  
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> index d7a6673c29e..79c3b7ebe40 100644
> --- a/include/hw/clock.h
> +++ b/include/hw/clock.h
> @@ -286,6 +286,47 @@ static inline uint64_t clock_ticks_to_ns(const Clock 
> *clk, uint64_t ticks)
>      return ns_low >> 32 | ns_high << 32;
>  }
>  
> +/**
> + * clock_ns_to_ticks:
> + * @clk: the clock to query
> + * @ns: duration in nanoseconds
> + *
> + * Returns the number of ticks this clock would make in the given
> + * number of nanoseconds. Because a clock can have a period which
> + * is not a whole number of nanoseconds, it is important to use this
> + * function rather than attempting to obtain a "period in nanoseconds"
> + * value and then dividing the duration by that value.
> + *
> + * If the clock is stopped (ie it has period zero), returns 0.
> + *
> + * For some inputs the result could overflow a 64-bit value (because
> + * the clock's period is short and the duration is long). In these
> + * cases we truncate the result to a 64-bit value. This is on the
> + * assumption that generally the result is going to be used to report
> + * a 32-bit or 64-bit guest register value, so wrapping either cannot
> + * happen or is the desired behaviour.
> + */
> +static inline uint64_t clock_ns_to_ticks(const Clock *clk, uint64_t ns)
> +{
> +    /*
> +     * ticks = duration_in_ns / period_in_ns
> +     *       = ns / (period / 2^32)
> +     *       = (ns * 2^32) / period
> +     * The hi, lo inputs to divu128() are (ns << 32) as a 128 bit value.
> +     */
> +    uint64_t lo = ns << 32;
> +    uint64_t hi = ns >> 32;
> +    if (clk->period == 0) {
> +        return 0;
> +    }
> +    /*
> +     * Ignore divu128() return value as we've caught div-by-zero and don't
> +     * need different behaviour for overflow.
> +     */
> +    divu128(&lo, &hi, clk->period);
> +    return lo;
> +}
> +
>  /**
>   * clock_is_enabled:
>   * @clk: a clock
> -- 
> 2.20.1
> 

--