Re: [PATCH for-6.2 21/25] hw/timer/armv7m_systick: Use clock inputs instead of system_clock_scale

[Damien Hedde]

On 8/12/21 11:33 AM, Peter Maydell wrote:
> Now that all users of the systick devices wire up the clock inputs,
> use those instead of the system_clock_scale and the hardwired 1MHz
> value for the reference clock.
> 
> This will fix various board models where we were incorrectly
> providing a 1MHz reference clock instead of some other value or
> instead of providing no reference clock at all.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/timer/armv7m_systick.c | 110 ++++++++++++++++++++++++++++----------
>  1 file changed, 82 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/timer/armv7m_systick.c b/hw/timer/armv7m_systick.c
> index e43f74114e8..39cca206cfd 100644
> --- a/hw/timer/armv7m_systick.c
> +++ b/hw/timer/armv7m_systick.c
> @@ -18,25 +18,29 @@
>  #include "qemu/timer.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> +#include "qapi/error.h"
>  #include "trace.h"
>  
> -/* qemu timers run at 1GHz.   We want something closer to 1MHz.  */
> -#define SYSTICK_SCALE 1000ULL
> -
>  #define SYSTICK_ENABLE    (1 << 0)
>  #define SYSTICK_TICKINT   (1 << 1)
>  #define SYSTICK_CLKSOURCE (1 << 2)
>  #define SYSTICK_COUNTFLAG (1 << 16)
>  
> +#define SYSCALIB_NOREF (1U << 31)
> +#define SYSCALIB_SKEW (1U << 30)
> +
>  int system_clock_scale;
>  
> -/* Conversion factor from qemu timer to SysTick frequencies.  */
> -static inline int64_t systick_scale(SysTickState *s)
> +static void systick_set_period_from_clock(SysTickState *s)
>  {
> +    /*
> +     * Set the ptimer period from whichever clock is selected.
> +     * Must be called from within a ptimer transaction block.
> +     */
>      if (s->control & SYSTICK_CLKSOURCE) {
> -        return system_clock_scale;
> +        ptimer_set_period_from_clock(s->ptimer, s->cpuclk, 1);
>      } else {
> -        return 1000;
> +        ptimer_set_period_from_clock(s->ptimer, s->refclk, 1);
>      }
>  }
>  
> @@ -83,7 +87,27 @@ static MemTxResult systick_read(void *opaque, hwaddr addr, 
> uint64_t *data,
>          val = ptimer_get_count(s->ptimer);
>          break;
>      case 0xc: /* SysTick Calibration Value.  */
> -        val = 10000;
> +        /*
> +         * In real hardware it is possible to make this register report
> +         * a different value from what the reference clock is actually
> +         * running at. We don't model that (which usually happens due
> +         * to integration errors in the real hardware) and instead always
> +         * report the theoretical correct value as described in the
> +         * knowledgebase article at
> +         * https://developer.arm.com/documentation/ka001325/latest
> +         * If necessary, we could implement an extra QOM property on this
> +         * device to force the STCALIB value to something different from
> +         * the "correct" value.
> +         */
> +        if (!clock_has_source(s->refclk)) {
> +            val = SYSCALIB_NOREF;
> +            break;
> +        }
> +        val = clock_ns_to_ticks(s->refclk, 10 * SCALE_MS) - 1;

According to
https://developer.arm.com/documentation/ddi0403/d/System-Level-Architecture/System-Address-Map/The-system-timer--SysTick/SysTick-Calibration-value-Register--SYST-CALIB
, the field is 24bits wide.

Should we prevent an overflow into the reserved bits and other fields ?
by doing something like this:
           val &= SYSCALIB_TENMS;
with the following #define with the other ones, above.
    #define SYSCALIB_TENMS ((1U << 24) - 1)

Note, the overflow would happen around ~1.68GHz refclk frequency, it is
probably a config that will never happen. I'm not sure if we should care
or do something if this happens because it is probably an error
somewhere else.

> +        if (clock_ticks_to_ns(s->refclk, val + 1) != 10 * SCALE_MS) {
> +            /* report that tick count does not yield exactly 10ms */
> +            val |= SYSCALIB_SKEW;
> +        }
>          break;

Otherwise the patch looks great.

Thanks,
Damien