Re: [PATCH qemu.git v2 9/9] hw/timer/imx_epit: fix compare timer handling
Peter, If you're correcting behaviour of the timer use here, you should start by fixing the way the timers are currently created with PTIMER_POLICY_LEGACY. That setting is basically "bug-for-bug-compatibility with very old QEMU, for devices where nobody really knows what the hardware behaviour should be". Where we do know what the hardware's supposed to do and we have some way of testing we're not breaking guest code, the right thing is to set the correct policy flags for the desired behaviour. These are documented in a comment near the top of include/hw/ptimer.h. I would prefer to postpone changing PTIMER_POLICY_LEGACY to a separate patchset, which is on top of the current one, as this seems not to be an issue at the moment. Fixing the general isses on access and ensure the flags are correct seem more pressing, and this seem unrelated to the timer policy. It is modestly harmful because the sequence counter = ptimer_get_count(s->timer_reload); ... ptimer_set_count(s->timer_cmp, counter); will cause the counter to lose or gain time. This happens because when you call "get" the ptimer code will look at the current exact time in nanoseconds and tell you the counter value at that point. That is probably somewhere in the middle of a timer-clock period (which runs at whatever frequency you tell the ptimer to use): for argument's sake, suppose the timer-clock counts every 1000ns. Suppose at the point of the 'get' the next tick will be in 300ns time. When you do a "set" that is assumed to be the result of a guest register write of some kind, and will effectively start a new timer-clock period. This means the next tick will not be for a full 1000ns, and we just lost 300ns (or gained 700ns perhaps). So it's better to avoid this kind of "get-and-then-set" code. I see you point. The "get-and-then-set" was already in the code, I did not really change this. I have tried to find a better way to implement this, but could not come up with something so far. Any suggestions here that is non trivial? Othereise I would prefer to look into this in a new patch-set, together with replacing the PTIMER_POLICY_LEGACY. Axel
Re: [PATCH qemu.git v2 9/9] hw/timer/imx_epit: fix compare timer handling
On Mon, 7 Nov 2022 at 16:42, ~axelheider wrote:
>
> From: Axel Heider
>
> - fix #1263
> - rework compare time handling
> - The compare timer has to run even if CR.OCIEN is not set,
> as SR.OCIF must be updated.
> - The compare timer fires exactly once when the
> compare value is less than the current value, but the
> reload values is less than the compare value.
> - The compare timer will never fire if the reload value is
> less than the compare value. Disable it in this case.
If you're correcting behaviour of the timer use here,
you should start by fixing the way the timers are currently
created with PTIMER_POLICY_LEGACY. That setting is basically
"bug-for-bug-compatibility with very old QEMU, for devices
where nobody really knows what the hardware behaviour should
be". Where we do know what the hardware's supposed to do and
we have some way of testing we're not breaking guest code,
the right thing is to set the correct policy flags for
the desired behaviour. These are documented in a comment
near the top of include/hw/ptimer.h.
> Signed-off-by: Axel Heider
> ---
> hw/timer/imx_epit.c | 188 +---
> 1 file changed, 123 insertions(+), 65 deletions(-)
>
> diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
> index 77bd2b0a2b..cb2880cabc 100644
> --- a/hw/timer/imx_epit.c
> +++ b/hw/timer/imx_epit.c
> @@ -6,6 +6,7 @@
> * Originally written by Hans Jiang
> * Updated by Peter Chubb
> * Updated by Jean-Christophe Dubois
> + * Updated by Axel Heider
> *
> * This code is licensed under GPL version 2 or later. See
> * the COPYING file in the top-level directory.
> @@ -110,33 +111,84 @@ static uint64_t imx_epit_read(void *opaque, hwaddr
> offset, unsigned size)
> return reg_value;
> }
>
> -/* Must be called from ptimer_transaction_begin/commit block for
> s->timer_cmp */
> -static void imx_epit_reload_compare_timer(IMXEPITState *s)
> +/*
> + * Must be called from a ptimer_transaction_begin/commit block for
> + * s->timer_cmp, but outside of a transaction block of s->timer_reload,
> + * so the proper counter value is read.
> + */
> +static void imx_epit_update_compare_timer(IMXEPITState *s)
> {
> -if ((s->cr & (CR_EN | CR_OCIEN)) == (CR_EN | CR_OCIEN)) {
> -/* if the compare feature is on and timers are running */
> -uint32_t tmp = ptimer_get_count(s->timer_reload);
> -uint64_t next;
> -if (tmp > s->cmp) {
> -/* It'll fire in this round of the timer */
> -next = tmp - s->cmp;
> -} else { /* catch it next time around */
> -next = tmp - s->cmp + ((s->cr & CR_RLD) ? EPIT_TIMER_MAX :
> s->lr);
> +uint64_t counter = 0;
> +bool is_oneshot = false;
> +/* The compare timer only has to run if the timer peripheral is active
> + * and there is an input clock, Otherwise it can be switched off.
> + */
QEMU coding style wants the "/*" on a line of its own in multiline comments.
> +bool is_active = (s->cr & CR_EN) && imx_epit_get_freq(s);
> +if (is_active)
> +{
Brace goes on same line as "if".
> +/*
> + * Calculate next timeout for compare timer. Reading the reload
> + * counter returns proper results only if pending transactions
> + * on it are committed here. Otherwise stale values are be read.
> + */
> +counter = ptimer_get_count(s->timer_reload);
> +uint64_t limit = ptimer_get_limit(s->timer_cmp);
> +/* The compare timer is a periodic timer if the limit is at least
> + * the compare value. Otherwise it may fire at most once in the
> + * current round.
> + */
> +bool is_oneshot = (limit >= s->cmp);
> +if (counter >= s->cmp) {
> +/* The compare timer fires in the current round. */
> +counter -= s->cmp;
> +} else if (!is_oneshot) {
> +/*
> + * The compare timer fires after a reload, as it below the
> + * compare value already in this round. Note that the counter
> + * value calculated below can be above the 32-bit limit, which
> + * is legal here because the compare timer is an internal
> + * helper ptimer only.
> + */
> +counter += limit - s->cmp;
> +} else {
> +/*
> + * The compare timer wont fire in this round, and the limit is
"won't"
> + * set to a value below the compare value. This practically means
> + * it will never fire, so it can be switched off.
> + */
> +is_active = false;
> }
> -ptimer_set_count(s->timer_cmp, next);
> }
> +
> +/*
> + * Set the compare timer and let it run, or stop it. This is agnostic
> + * of CR.OCIEN bit, as this only matters for interrupt generation. The
> + * compare timer needs to run in any case, as the SR.OCIF bit must be
> + *
[PATCH qemu.git v2 9/9] hw/timer/imx_epit: fix compare timer handling
From: Axel Heider
- fix #1263
- rework compare time handling
- The compare timer has to run even if CR.OCIEN is not set,
as SR.OCIF must be updated.
- The compare timer fires exactly once when the
compare value is less than the current value, but the
reload values is less than the compare value.
- The compare timer will never fire if the reload value is
less than the compare value. Disable it in this case.
Signed-off-by: Axel Heider
---
hw/timer/imx_epit.c | 188 +---
1 file changed, 123 insertions(+), 65 deletions(-)
diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index 77bd2b0a2b..cb2880cabc 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -6,6 +6,7 @@
* Originally written by Hans Jiang
* Updated by Peter Chubb
* Updated by Jean-Christophe Dubois
+ * Updated by Axel Heider
*
* This code is licensed under GPL version 2 or later. See
* the COPYING file in the top-level directory.
@@ -110,33 +111,84 @@ static uint64_t imx_epit_read(void *opaque, hwaddr
offset, unsigned size)
return reg_value;
}
-/* Must be called from ptimer_transaction_begin/commit block for s->timer_cmp
*/
-static void imx_epit_reload_compare_timer(IMXEPITState *s)
+/*
+ * Must be called from a ptimer_transaction_begin/commit block for
+ * s->timer_cmp, but outside of a transaction block of s->timer_reload,
+ * so the proper counter value is read.
+ */
+static void imx_epit_update_compare_timer(IMXEPITState *s)
{
-if ((s->cr & (CR_EN | CR_OCIEN)) == (CR_EN | CR_OCIEN)) {
-/* if the compare feature is on and timers are running */
-uint32_t tmp = ptimer_get_count(s->timer_reload);
-uint64_t next;
-if (tmp > s->cmp) {
-/* It'll fire in this round of the timer */
-next = tmp - s->cmp;
-} else { /* catch it next time around */
-next = tmp - s->cmp + ((s->cr & CR_RLD) ? EPIT_TIMER_MAX : s->lr);
+uint64_t counter = 0;
+bool is_oneshot = false;
+/* The compare timer only has to run if the timer peripheral is active
+ * and there is an input clock, Otherwise it can be switched off.
+ */
+bool is_active = (s->cr & CR_EN) && imx_epit_get_freq(s);
+if (is_active)
+{
+/*
+ * Calculate next timeout for compare timer. Reading the reload
+ * counter returns proper results only if pending transactions
+ * on it are committed here. Otherwise stale values are be read.
+ */
+counter = ptimer_get_count(s->timer_reload);
+uint64_t limit = ptimer_get_limit(s->timer_cmp);
+/* The compare timer is a periodic timer if the limit is at least
+ * the compare value. Otherwise it may fire at most once in the
+ * current round.
+ */
+bool is_oneshot = (limit >= s->cmp);
+if (counter >= s->cmp) {
+/* The compare timer fires in the current round. */
+counter -= s->cmp;
+} else if (!is_oneshot) {
+/*
+ * The compare timer fires after a reload, as it below the
+ * compare value already in this round. Note that the counter
+ * value calculated below can be above the 32-bit limit, which
+ * is legal here because the compare timer is an internal
+ * helper ptimer only.
+ */
+counter += limit - s->cmp;
+} else {
+/*
+ * The compare timer wont fire in this round, and the limit is
+ * set to a value below the compare value. This practically means
+ * it will never fire, so it can be switched off.
+ */
+is_active = false;
}
-ptimer_set_count(s->timer_cmp, next);
}
+
+/*
+ * Set the compare timer and let it run, or stop it. This is agnostic
+ * of CR.OCIEN bit, as this only matters for interrupt generation. The
+ * compare timer needs to run in any case, as the SR.OCIF bit must be
+ * updated even if no interrupt in generated.
+ * Note that the timer might already be stopped or be running with
+ * counter values. However, finding out when an update is needed and
+ * when not is not trivial. It's much easier applying the setting again,
+ * as this does not harm either and the overhead is negligible.
+ */
+if (is_active) {
+ptimer_set_count(s->timer_cmp, counter);
+ptimer_run(s->timer_cmp, is_oneshot ? 1 : 0);
+} else {
+ptimer_stop(s->timer_cmp);
+}
+
}
static void imx_epit_write_cr(IMXEPITState *s, uint32_t value)
{
uint32_t freq = 0;
-uint32_t oldcr = s->cr;
+bool set_limit = false;
+bool set_counter = false;
/* SWR bit is never persisted, it clears itself once reset is done */
+uint32_t old_cr = s->cr;
s->cr = (value & ~CR_SWR) & 0x03ff;
-
-ptimer_transaction_begin(s->timer_cmp);
-
