[PATCH 0/1] hw/arm/sse-timer: Add CNTFRQ reset property

[PATCH 0/1] hw/arm/sse-timer: Add CNTFRQ reset property

[Joe Komlodi]

Hi all,

This patch just adds an object property to initialize the reset value of
CNTFRQ.
We noticed that Linux would complain that CNTFRQ would have a mismatch
compared to an expected value, and this was because TF-A was assuming
that CNTFRQ was initialized to a different value out of reset.

Since it's valid for CNTFRQ to have a non-zero reset value, we just
added an object property so people can set it.

Thanks!
Joe

Joe Komlodi (1):
  hw/timer/sse-timer: Add CNTFRQ reset property

 hw/timer/sse-timer.c         | 4 +++-
 include/hw/timer/sse-timer.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

-- 
2.42.0.459.ge4e396fd5e-goog

[PATCH 1/1] hw/timer/sse-timer: Add CNTFRQ reset property

[Joe Komlodi]

This can have a non-zero reset value, and cause the kernel to complain
about a CNTFRQ mismatch if TF-A (or firmware in general) does not
initialize it (because it expects the value to be non-zero out of
reset).

To fix this, we'll just add an object property that people can use to
initialize the CNTFRQ reset value.

Signed-off-by: Joe Komlodi <komlodi@google.com>
---
 hw/timer/sse-timer.c         | 4 +++-
 include/hw/timer/sse-timer.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/timer/sse-timer.c b/hw/timer/sse-timer.c
index e92e83747d..a727f05bac 100644
--- a/hw/timer/sse-timer.c
+++ b/hw/timer/sse-timer.c
@@ -376,7 +376,7 @@ static void sse_timer_reset(DeviceState *dev)
     trace_sse_timer_reset();
 
     timer_del(&s->timer);
-    s->cntfrq = 0;
+    s->cntfrq = s->cntfrq_reset;
     s->cntp_ctl = 0;
     s->cntp_cval = 0;
     s->cntp_aival = 0;
@@ -430,6 +430,7 @@ static const VMStateDescription sse_timer_vmstate = {
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
         VMSTATE_TIMER(timer, SSETimer),
+        VMSTATE_UINT32(cntfrq_reset, SSETimer),
         VMSTATE_UINT32(cntfrq, SSETimer),
         VMSTATE_UINT32(cntp_ctl, SSETimer),
         VMSTATE_UINT64(cntp_cval, SSETimer),
@@ -442,6 +443,7 @@ static const VMStateDescription sse_timer_vmstate = {
 
 static Property sse_timer_properties[] = {
     DEFINE_PROP_LINK("counter", SSETimer, counter, TYPE_SSE_COUNTER, SSECounter *),
+    DEFINE_PROP_UINT32("cntfrq-reset", SSETimer, cntfrq_reset, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/timer/sse-timer.h b/include/hw/timer/sse-timer.h
index 265ad32400..ad84c24940 100644
--- a/include/hw/timer/sse-timer.h
+++ b/include/hw/timer/sse-timer.h
@@ -43,6 +43,8 @@ struct SSETimer {
     QEMUTimer timer;
     Notifier counter_notifier;
 
+    uint32_t cntfrq_reset;
+
     uint32_t cntfrq;
     uint32_t cntp_ctl;
     uint64_t cntp_cval;
-- 
2.42.0.459.ge4e396fd5e-goog

Re: [PATCH 1/1] hw/timer/sse-timer: Add CNTFRQ reset property

[Peter Maydell]

On Thu, 14 Sept 2023 at 23:42, Joe Komlodi <komlodi@google.com> wrote:
>
> This can have a non-zero reset value, and cause the kernel to complain
> about a CNTFRQ mismatch if TF-A (or firmware in general) does not
> initialize it (because it expects the value to be non-zero out of
> reset).
>
> To fix this, we'll just add an object property that people can use to
> initialize the CNTFRQ reset value.

I'm a bit confused -- you talk about TF-A, but as far as I'm
aware the only use of this timer device is in the ARMSSE,
which is used only on M-profile boards.

Also, since this device is a sysbus device, there's not much
point adding the property without also adding code in the
ARMSSE etc that sets the property appropriately. On its
own this patch will do nothing.

Plus, it is firmware's job to set the value of this register:
in real hardware it is always zero on reset. For the A-profile
in-core CNTFRQ we set a non-zero reset value because in a
lot of use cases QEMU is effectively emulating firmware
and then directly booting Linux. I'm not so sure those cases
apply for M-profile boards?

If we do want to follow a similar approach to the A-profile
CPU timer, rather than having a separate property that must
be set, it would be neater for the timer to ask the counter
for the frequency -- the SSE timer is run from the SSE counter,
and the counter has a Clock input and can call clock_get_hz()
to get its frequency.

thanks
-- PMM