| Message ID | 20230914224211.2184828-2-komlodi@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/sse-timer: Add CNTFRQ reset property | expand |
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
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;
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(-)