From: YannickV <Y.Vossen@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
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.vossen@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);
}
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),
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()
@@ -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),
DEFINE_PROP_UINT32("num-cpu", ARMMPTimerState, num_cpu, 0),
};
diff --git a/include/hw/timer/a9gtimer.h b/include/hw/timer/a9gtimer.h
index 6ae9122e4b..8ce507a793 100644
--- a/include/hw/timer/a9gtimer.h
+++ b/include/hw/timer/a9gtimer.h
@@ -76,6 +76,7 @@ struct A9GTimerState {
MemoryRegion iomem;
/* static props */
+ uint64_t cpu_clk_freq_hz;
uint32_t num_cpu;
QEMUTimer *timer;
diff --git a/include/hw/timer/arm_mptimer.h b/include/hw/timer/arm_mptimer.h
index 65a96e2a0d..8b936cceac 100644
--- a/include/hw/timer/arm_mptimer.h
+++ b/include/hw/timer/arm_mptimer.h
@@ -31,6 +31,7 @@ typedef struct {
uint32_t control;
uint32_t status;
struct ptimer_state *timer;
+ uint64_t freq_hz;
qemu_irq irq;
MemoryRegion iomem;
} TimerBlock;
@@ -43,6 +44,7 @@ struct ARMMPTimerState {
SysBusDevice parent_obj;
/*< public >*/
+ uint64_t clk_freq_hz;
uint32_t num_cpu;
TimerBlock timerblock[ARM_MPTIMER_MAX_CPUS];
MemoryRegion iomem;
--
2.50.1
On Fri, 15 Aug 2025 at 10:01, Corvin Köhne <corvin.koehne@gmail.com> wrote: > > From: YannickV <Y.Vossen@beckhoff.com> > Signed-off-by: Yannick Voßen <y.vossen@beckhoff.com> Is it intentional that the names in the From and the Signed-off-by: are different ? thanks -- PMM
On Fri, 15 Aug 2025 at 10:01, Corvin Köhne <corvin.koehne@gmail.com> wrote:
>
> From: YannickV <Y.Vossen@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.vossen@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
© 2016 - 2025 Red Hat, Inc.