Re: [PATCH 01/25] ptimer: Add new ptimer_set_period_from_clock() function

[Luc Michel]

On 19:05 Thu 21 Jan     , Peter Maydell wrote:
> The ptimer API currently provides two methods for setting the period:
> ptimer_set_period(), which takes a period in nanoseconds, and
> ptimer_set_freq(), which takes a frequency in Hz.  Neither of these
> lines up nicely with the Clock API, because although both the Clock
> and the ptimer track the frequency using a representation of whole
> and fractional nanoseconds, conversion via either period-in-ns or
> frequency-in-Hz will introduce a rounding error.
> 
> Add a new function ptimer_set_period_from_clock() which takes the
> Clock object directly to avoid the rounding issues.  This includes a
> facility for the user to specify that there is a frequency divider
> between the Clock proper and the timer, as some timer devices like
> the CMSDK APB dualtimer need this.
> 
> To avoid having to drag in clock.h from ptimer.h we add the Clock
> type to typedefs.h.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
> Side note, I forget why we didn't go for 64.32 fixedpoint for the
> Clock too.  I kinda feel we might run into the "clocks can't handle
> periods greater than 4 seconds" limit some day.  Hopefully we can
> backwards-compatibly expand it if that day ever comes...
> 
> The 'divisor' functionality seemed like the simplest way to get
> to what I needed for the dualtimer; perhaps we should think about
> whether we can have generic lightweight support for clock
> frequency divider/multipliers...
> ---
>  include/hw/ptimer.h     | 22 ++++++++++++++++++++++
>  include/qemu/typedefs.h |  1 +
>  hw/core/ptimer.c        | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/include/hw/ptimer.h b/include/hw/ptimer.h
> index 412763fffb2..c443218475b 100644
> --- a/include/hw/ptimer.h
> +++ b/include/hw/ptimer.h
> @@ -165,6 +165,28 @@ void ptimer_transaction_commit(ptimer_state *s);
>   */
>  void ptimer_set_period(ptimer_state *s, int64_t period);
>  
> +/**
> + * ptimer_set_period_from_clock - Set counter increment from a Clock
> + * @s: ptimer to configure
> + * @clk: pointer to Clock object to take period from
> + * @divisor: value to scale the clock frequency down by
> + *
> + * If the ptimer is being driven from a Clock, this is the preferred
> + * way to tell the ptimer about the period, because it avoids any
> + * possible rounding errors that might happen if the internal
> + * representation of the Clock period was converted to either a period
> + * in ns or a frequency in Hz.
> + *
> + * If the ptimer should run at the same frequency as the clock,
> + * pass 1 as the @divisor; if the ptimer should run at half the
> + * frequency, pass 2, and so on.
> + *
> + * This function will assert if it is called outside a
> + * ptimer_transaction_begin/commit block.
> + */
> +void ptimer_set_period_from_clock(ptimer_state *s, const Clock *clock,
> +                                  unsigned int divisor);
> +
>  /**
>   * ptimer_set_freq - Set counter frequency in Hz
>   * @s: ptimer to configure
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 976b529dfb5..68deb74ef6f 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -34,6 +34,7 @@ typedef struct BlockDriverState BlockDriverState;
>  typedef struct BusClass BusClass;
>  typedef struct BusState BusState;
>  typedef struct Chardev Chardev;
> +typedef struct Clock Clock;
>  typedef struct CompatProperty CompatProperty;
>  typedef struct CoMutex CoMutex;
>  typedef struct CPUAddressSpace CPUAddressSpace;
> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index 2aa97cb665c..6ba19fd9658 100644
> --- a/hw/core/ptimer.c
> +++ b/hw/core/ptimer.c
> @@ -15,6 +15,7 @@
>  #include "sysemu/qtest.h"
>  #include "block/aio.h"
>  #include "sysemu/cpus.h"
> +#include "hw/clock.h"
>  
>  #define DELTA_ADJUST     1
>  #define DELTA_NO_ADJUST -1
> @@ -348,6 +349,39 @@ void ptimer_set_period(ptimer_state *s, int64_t period)
>      }
>  }
>  
> +/* Set counter increment interval from a Clock */
> +void ptimer_set_period_from_clock(ptimer_state *s, const Clock *clk,
> +                                  unsigned int divisor)
> +{
> +    /*
> +     * The raw clock period is a 64-bit value in units of 2^-32 ns;
> +     * put another way it's a 32.32 fixed-point ns value. Our internal
> +     * representation of the period is 64.32 fixed point ns, so
> +     * the conversion is simple.
> +     */
> +    uint64_t raw_period = clock_get(clk);
> +    uint64_t period_frac;
> +
> +    assert(s->in_transaction);
> +    s->delta = ptimer_get_count(s);
> +    s->period = extract64(raw_period, 32, 32);
> +    period_frac = extract64(raw_period, 0, 32);
> +    /*
> +     * divisor specifies a possible frequency divisor between the
> +     * clock and the timer, so it is a multiplier on the period.
> +     * We do the multiply after splitting the raw period out into
> +     * period and frac to avoid having to do a 32*64->96 multiply.
> +     */
> +    s->period *= divisor;
> +    period_frac *= divisor;
> +    s->period += extract64(period_frac, 32, 32);
> +    s->period_frac = (uint32_t)period_frac;
> +
> +    if (s->enabled) {
> +        s->need_reload = true;
> +    }
> +}
> +
>  /* Set counter frequency in Hz.  */
>  void ptimer_set_freq(ptimer_state *s, uint32_t freq)
>  {
> -- 
> 2.20.1
> 

--