Re: [PATCH qemu.git v3 8/8] hw/timer/imx_epit: fix compare timer handling

[Peter Maydell]

On Thu, 1 Dec 2022 at 15:42, ~axelheider <axelheider@git.sr.ht> wrote:
>
> From: Axel Heider <axel.heider@hensoldt.net>
>
> - fix #1263 for CR writes
> - 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 <axel.heider@hensoldt.net>

There's a couple of minor code-style issues here (block comment
format, variable declarations in the middle of a block); rather
than asking you to re-roll the series I'll just squash in the
fixes for those:

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index 663907f9cf9..f63d3a20830 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -161,7 +161,8 @@ static void imx_epit_update_compare_timer(IMXEPITState *s)
 {
     uint64_t counter = 0;
     bool is_oneshot = false;
-    /* The compare timer only has to run if the timer peripheral is active
+    /*
+     * 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);
@@ -233,19 +234,22 @@ static void imx_epit_write_cr(IMXEPITState *s,
uint32_t value)
          */
         imx_epit_reset(s, false);
     } else {
-        ptimer_transaction_begin(s->timer_cmp);
-        ptimer_transaction_begin(s->timer_reload);
-        uint32_t freq = imx_epit_get_freq(s);
-        if (freq) {
-            ptimer_set_freq(s->timer_reload, freq);
-            ptimer_set_freq(s->timer_cmp, freq);
-        }
+        uint32_t freq;
         uint32_t toggled_cr_bits = oldcr ^ s->cr;
         /* re-initialize the limits if CR.RLD has changed */
         bool set_limit = toggled_cr_bits & CR_RLD;
         /* set the counter if the timer got just enabled and CR.ENMOD is set */
         bool is_switched_on = (toggled_cr_bits & s->cr) & CR_EN;
         bool set_counter = is_switched_on && (s->cr & CR_ENMOD);
+
+        ptimer_transaction_begin(s->timer_cmp);
+        ptimer_transaction_begin(s->timer_reload);
+        freq = imx_epit_get_freq(s);
+        if (freq) {
+            ptimer_set_freq(s->timer_reload, freq);
+            ptimer_set_freq(s->timer_cmp, freq);
+        }
+
         if (set_limit || set_counter) {
             uint64_t limit = (s->cr & CR_RLD) ? s->lr : EPIT_TIMER_MAX;
             ptimer_set_limit(s->timer_reload, limit, set_counter ? 1 : 0);

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM