On Fri, 15 Aug 2025 at 10:01, Corvin Köhne <corvin.koe...@gmail.com> wrote: > > From: YannickV <y.vos...@beckhoff.com> > > The a9 global timer and arm mp timers rely on the PERIPHCLK as > their clock source. The current implementation does not take > that into account. That causes problems for applications assuming > other frequencies than 1 GHz. > > We can now configure frequencies for the a9 global timer and > arm mp timer. By allowing these values to be set according to > the application's needs, we ensure that the timers behave > consistently with the expected system configuration. > > The frequency can also be set via the command line, for example > for the a9 global timer: > -global driver=arm.cortex-a9-global-timer, > property=cpu-freq,value=1000000000
-global is a rare thing for special cases, you don't need to mention it here. The standard case should be "the SoC configures the device correctly". > Information can be found in the Zynq 7000 SoC Technical > Reference Manual under Timers. > https://docs.amd.com/r/en-US/ug585-zynq-7000-SoC-TRM > > Signed-off-by: Yannick Voßen <y.vos...@beckhoff.com> > --- > hw/timer/a9gtimer.c | 8 +++++--- > hw/timer/arm_mptimer.c | 15 +++++++++++---- > include/hw/timer/a9gtimer.h | 1 + > include/hw/timer/arm_mptimer.h | 2 ++ > 4 files changed, 19 insertions(+), 7 deletions(-) > > diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c > index 9835c35483..a1f5540e75 100644 > --- a/hw/timer/a9gtimer.c > +++ b/hw/timer/a9gtimer.c > @@ -63,9 +63,9 @@ static inline int a9_gtimer_get_current_cpu(A9GTimerState > *s) > static inline uint64_t a9_gtimer_get_conv(A9GTimerState *s) > { > uint64_t prescale = extract32(s->control, R_CONTROL_PRESCALER_SHIFT, > - R_CONTROL_PRESCALER_LEN); > - > - return (prescale + 1) * 10; > + R_CONTROL_PRESCALER_LEN) + 1; > + uint64_t ret = NANOSECONDS_PER_SECOND * prescale * 10; > + return (uint32_t) (ret / s->cpu_clk_freq_hz); Why the cast to uint32_t here ? If you are doing an "x * NANOSECONDS_PER_SECOND / frequency" calculation, use muldiv64(). (This does the calculation with a 96 bit intermediate result.) > } > > static A9GTimerUpdate a9_gtimer_get_update(A9GTimerState *s) > @@ -374,6 +374,8 @@ static const VMStateDescription vmstate_a9_gtimer = { > }; > > static const Property a9_gtimer_properties[] = { > + DEFINE_PROP_UINT64("cpu-freq", A9GTimerState, cpu_clk_freq_hz, > + NANOSECONDS_PER_SECOND), You could have a comment mentioning that this default is 1GHz. The modern way to do this is to have a Clock property, but we have enough existing timer devices that take an integer frequency that I don't object to another one. > DEFINE_PROP_UINT32("num-cpu", A9GTimerState, num_cpu, 0), > }; > > diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c > index 803dad1e8a..a748b6ab1a 100644 > --- a/hw/timer/arm_mptimer.c > +++ b/hw/timer/arm_mptimer.c > @@ -59,9 +59,11 @@ static inline void timerblock_update_irq(TimerBlock *tb) > } > > /* Return conversion factor from mpcore timer ticks to qemu timer ticks. */ > -static inline uint32_t timerblock_scale(uint32_t control) > +static inline uint32_t timerblock_scale(TimerBlock *tb, uint32_t control) > { > - return (((control >> 8) & 0xff) + 1) * 10; > + uint64_t prescale = (((control >> 8) & 0xff) + 1); > + uint64_t ret = NANOSECONDS_PER_SECOND * prescale * 10; > + return (uint32_t) (ret / tb->freq_hz); > } > > /* Must be called within a ptimer transaction block */ > @@ -155,7 +157,7 @@ static void timerblock_write(void *opaque, hwaddr addr, > ptimer_stop(tb->timer); > } > if ((control & 0xff00) != (value & 0xff00)) { > - ptimer_set_period(tb->timer, timerblock_scale(value)); > + ptimer_set_period(tb->timer, timerblock_scale(tb, value)); > } > if (value & 1) { > uint64_t count = ptimer_get_count(tb->timer); > @@ -222,7 +224,8 @@ static void timerblock_reset(TimerBlock *tb) > ptimer_transaction_begin(tb->timer); > ptimer_stop(tb->timer); > ptimer_set_limit(tb->timer, 0, 1); > - ptimer_set_period(tb->timer, timerblock_scale(0)); > + ptimer_set_period(tb->timer, > + timerblock_scale(tb, tb->control)); > ptimer_transaction_commit(tb->timer); > } > } > @@ -269,6 +272,7 @@ static void arm_mptimer_realize(DeviceState *dev, Error > **errp) > */ > for (i = 0; i < s->num_cpu; i++) { > TimerBlock *tb = &s->timerblock[i]; > + tb->freq_hz = s->clk_freq_hz; > tb->timer = ptimer_init(timerblock_tick, tb, PTIMER_POLICY); > sysbus_init_irq(sbd, &tb->irq); > memory_region_init_io(&tb->iomem, OBJECT(s), &timerblock_ops, tb, > @@ -283,6 +287,7 @@ static const VMStateDescription vmstate_timerblock = { > .minimum_version_id = 3, > .fields = (const VMStateField[]) { > VMSTATE_UINT32(control, TimerBlock), > + VMSTATE_UINT64(freq_hz, TimerBlock), > VMSTATE_UINT32(status, TimerBlock), > VMSTATE_PTIMER(timer, TimerBlock), > VMSTATE_END_OF_LIST() You cannot add fields to a VMStateDescription without doing something to handle migration compatibility. Fortunately, in this case you don't need to add the field at all -- freq_hz is a property value that cannot be updated by the guest at runtime, so it doesn't need to be saved for migration. > @@ -301,6 +306,8 @@ static const VMStateDescription vmstate_arm_mptimer = { > }; > > static const Property arm_mptimer_properties[] = { > + DEFINE_PROP_UINT64("clk-freq", ARMMPTimerState, clk_freq_hz, > + NANOSECONDS_PER_SECOND), Can we be consistent about the property name and state struct field that we use, please? The one that seems to be most used in hw/timer/ is a property name "clock-frequency" and a field name freq_hz. thanks -- PMM
