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

[Damien Hedde]

On 8/17/21 5:59 PM, Peter Maydell wrote:
> On Tue, 17 Aug 2021 at 16:55, Damien Hedde  wrote:
>>
>>
>>
>> On 8/12/21 11:33 AM, Peter Maydell wrote:
>> 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.
> 
> I guess we should do something, yes, though pretty much anything
> we do will not really provide the guest with sensible data...
> I suppose masking out the higher bits is no worse than anything else.
> 
> -- PMM
> 


Then, with the masking.
Reviewed-by: Damien Hedde 

--
Damien

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

[Peter Maydell]

On Tue, 17 Aug 2021 at 16:55, Damien Hedde  wrote:
>
>
>
> On 8/12/21 11:33 AM, Peter Maydell wrote:
> 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.

I guess we should do something, yes, though pretty much anything
we do will not really provide the guest with sensible data...
I suppose masking out the higher bits is no worse than anything else.

-- PMM

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 
> ---
>  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 = 1;
> +/*
> + * 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

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

[Peter Maydell]

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 
---
 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 = 1;
+/*
+ * 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;
+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;
 default:
 val = 0;
@@ -115,6 +139,11 @@ static MemTxResult systick_write(void *opaque, hwaddr addr,
 {
 uint32_t oldval;
 
+if (!clock_has_source(s->refclk)) {
+/* This bit is always 1 if there is no external refclk */
+value |= SYSTICK_CLKSOURCE;
+}
+
 ptimer_transaction_begin(s->ptimer);
 oldval = s->control;
 s->control &= 0xfff8;
@@ -122,19 +151,14 @@ static MemTxResult systick_write(void *opaque, hwaddr 
addr,
 
 if ((oldval ^ value) & SYSTICK_ENABLE) {
 if (value & SYSTICK_ENABLE) {
-/*
- * Always reload the period in case board code has
- * changed system_clock_scale. If we ever replace that
- * global with a more sensible API then we might be able
- * to set the period only when it actually changes.
- */
-ptimer_set_period(s->ptimer, systick_scale(s));
 ptimer_run(s->ptimer, 0);
 } else {
 ptimer_stop(s->ptimer);
 }
-} else if ((oldval ^ value) & SYSTICK_CLKSOURCE) {
-ptimer_set_period(s->ptimer, systick_scale(s));
+}
+
+if ((oldval ^ value) & SYSTICK_CLKSOURCE) {
+systick_set_period_from_clock(s);
 }
 ptimer_transaction_commit(s->ptimer);
 break;
@@ -177,20 +201,42 @@ static void systick_reset(DeviceState *dev)
 {
 SysTickState *s = SYSTICK(dev);
 
-/*
- * Forgetting to set system_clock_scale is always a board code
- * bug. We can't check this earlier because for some boards
- * (like stellaris) it is not yet configured at the point where
- * the systick device is realized.
- */
-assert(system_clock_scale != 0);
-
 ptimer_transaction_begin(s->ptimer);
 s->control = 0;
+if (!clock_has_source(s->refclk)) {
+/* This bit is always 1 if there is no e