[PATCH qemu.git v3 0/8] hw/timer/imx_epit: improve and fix EPIT compare timer

[PATCH qemu.git v3 0/8] hw/timer/imx_epit: improve and fix EPIT compare timer

[~axelheider]

This patch set improves the i.MX EPIT emulation:
- fix #1263 for writes to CR
- ensure SR_OCIF is set properly even if CR_OCIEN is off
- hardware reset initialized CR to 0
- interrupt state update on CR writes (e.g. software reset)
- remove explicit fields cnt and freq (they are redundant)
- general code and documentation improvements

v3 addresses the comments from the previous iterations, but still keeps
the scope of this patchset limited to addressing the obvious bugs in the
behavior. It does not try to improve the timer accuracy, thus the
following remarks remain to be fixed in a future patch after this one is
merged:
- don't use PTIMER_POLICY_LEGACY. Fine tuning this requires more
  time and currently this is not a major concern, because the timer is
  working reasonably well.
- replace the modestly harmful sequence
        counter = ptimer_get_count(s->timer_reload);
        ...
        ptimer_set_count(s->timer_cmp, counter);
  by something better that does not lose or gain time. The current
  patchset does not introduce this sequence, it has been there
  before already. Again,  the current lack of accuracy here is not a
  major concern because the timer is working reasonably well.

Axel Heider (8):
  hw/timer/imx_epit: improve comments
  hw/timer/imx_epit: cleanup CR defines
  hw/timer/imx_epit: define SR_OCIF
  hw/timer/imx_epit: update interrupt state on CR write access
  hw/timer/imx_epit: hard reset initializes CR with 0
  hw/timer/imx_epit: factor out register write handlers
  hw/timer/imx_epit: remove explicit fields cnt and freq
  hw/timer/imx_epit: fix compare timer handling

 hw/timer/imx_epit.c         | 370 +++++++++++++++++++++---------------
 include/hw/timer/imx_epit.h |   8 +-
 2 files changed, 222 insertions(+), 156 deletions(-)

-- 
2.34.5

[PATCH qemu.git v3 7/8] hw/timer/imx_epit: remove explicit fields cnt and freq

[~axelheider]

From: Axel Heider <axel.heider@hensoldt.net>

The CNT register is a read-only register. There is no need to
store it's value, it can be calculated on demand.
The calculated frequency is needed temporarily only.

Note that this is a migration compatibility break for all boards
types that use the EPIT peripheral.

Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
---
 hw/timer/imx_epit.c         | 73 ++++++++++++++-----------------------
 include/hw/timer/imx_epit.h |  2 -
 2 files changed, 28 insertions(+), 47 deletions(-)

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index e04427542f..cf13496165 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -73,27 +73,14 @@ static void imx_epit_update_int(IMXEPITState *s)
     }
 }
 
-/*
- * Must be called from within a ptimer_transaction_begin/commit block
- * for both s->timer_cmp and s->timer_reload.
- */
-static void imx_epit_set_freq(IMXEPITState *s)
+static uint32_t imx_epit_get_freq(IMXEPITState *s)
 {
-    uint32_t clksrc;
-    uint32_t prescaler;
-
-    clksrc = extract32(s->cr, CR_CLKSRC_SHIFT, CR_CLKSRC_BITS);
-    prescaler = 1 + extract32(s->cr, CR_PRESCALE_SHIFT, CR_PRESCALE_BITS);
-
-    s->freq = imx_ccm_get_clock_frequency(s->ccm,
-                                imx_epit_clocks[clksrc]) / prescaler;
-
-    DPRINTF("Setting ptimer frequency to %u\n", s->freq);
-
-    if (s->freq) {
-        ptimer_set_freq(s->timer_reload, s->freq);
-        ptimer_set_freq(s->timer_cmp, s->freq);
-    }
+    uint32_t clksrc = extract32(s->cr, CR_CLKSRC_SHIFT, CR_CLKSRC_BITS);
+    uint32_t prescaler = 1 + extract32(s->cr, CR_PRESCALE_SHIFT, CR_PRESCALE_BITS);
+    uint32_t f_in = imx_ccm_get_clock_frequency(s->ccm, imx_epit_clocks[clksrc]);
+    uint32_t freq = f_in / prescaler;
+    DPRINTF("ptimer frequency is %u\n", freq);
+    return freq;
 }
 
 /*
@@ -110,32 +97,23 @@ static void imx_epit_reset(IMXEPITState *s, bool is_hard_reset)
     s->sr = 0;
     s->lr = EPIT_TIMER_MAX;
     s->cmp = 0;
-    s->cnt = 0;
     ptimer_transaction_begin(s->timer_cmp);
     ptimer_transaction_begin(s->timer_reload);
-    /* stop both timers */
+
+    /*
+     * The reset switches off the input clock, so even if the CR.EN is still
+     * set, the timers are no longer running.
+     */
+    assert(imx_epit_get_freq(s) == 0);
     ptimer_stop(s->timer_cmp);
     ptimer_stop(s->timer_reload);
-    /* compute new frequency */
-    imx_epit_set_freq(s);
     /* init both timers to EPIT_TIMER_MAX */
     ptimer_set_limit(s->timer_cmp, EPIT_TIMER_MAX, 1);
     ptimer_set_limit(s->timer_reload, EPIT_TIMER_MAX, 1);
-    if (s->freq && (s->cr & CR_EN)) {
-        /* if the timer is still enabled, restart it */
-        ptimer_run(s->timer_reload, 0);
-    }
     ptimer_transaction_commit(s->timer_cmp);
     ptimer_transaction_commit(s->timer_reload);
 }
 
-static uint32_t imx_epit_update_count(IMXEPITState *s)
-{
-    s->cnt = ptimer_get_count(s->timer_reload);
-
-    return s->cnt;
-}
-
 static uint64_t imx_epit_read(void *opaque, hwaddr offset, unsigned size)
 {
     IMXEPITState *s = IMX_EPIT(opaque);
@@ -159,8 +137,7 @@ static uint64_t imx_epit_read(void *opaque, hwaddr offset, unsigned size)
         break;
 
     case 4: /* CNT */
-        imx_epit_update_count(s);
-        reg_value = s->cnt;
+        reg_value = ptimer_get_count(s->timer_reload);
         break;
 
     default:
@@ -179,7 +156,7 @@ static void imx_epit_reload_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 = imx_epit_update_count(s);
+        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 */
@@ -193,6 +170,7 @@ static void imx_epit_reload_compare_timer(IMXEPITState *s)
 
 static void imx_epit_write_cr(IMXEPITState *s, uint32_t value)
 {
+    uint32_t freq = 0;
     uint32_t oldcr = s->cr;
 
     s->cr = value & 0x03ffffff;
@@ -217,12 +195,19 @@ static void imx_epit_write_cr(IMXEPITState *s, uint32_t value)
     ptimer_transaction_begin(s->timer_cmp);
     ptimer_transaction_begin(s->timer_reload);
 
-    /* Update the frequency. Has been done already in case of a reset. */
+    /*
+     * Update the frequency. In case of a reset the input clock was
+     * switched off, so this can be skipped.
+     */
     if (!(s->cr & CR_SWR)) {
-        imx_epit_set_freq(s);
+        freq = imx_epit_get_freq(s);
+        if (freq) {
+            ptimer_set_freq(s->timer_reload, freq);
+            ptimer_set_freq(s->timer_cmp, freq);
+        }
     }
 
-    if (s->freq && (s->cr & CR_EN) && !(oldcr & CR_EN)) {
+    if (freq && (s->cr & CR_EN) && !(oldcr & CR_EN)) {
         if (s->cr & CR_ENMOD) {
             if (s->cr & CR_RLD) {
                 ptimer_set_limit(s->timer_reload, s->lr, 1);
@@ -356,15 +341,13 @@ static const MemoryRegionOps imx_epit_ops = {
 
 static const VMStateDescription vmstate_imx_timer_epit = {
     .name = TYPE_IMX_EPIT,
-    .version_id = 2,
-    .minimum_version_id = 2,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(cr, IMXEPITState),
         VMSTATE_UINT32(sr, IMXEPITState),
         VMSTATE_UINT32(lr, IMXEPITState),
         VMSTATE_UINT32(cmp, IMXEPITState),
-        VMSTATE_UINT32(cnt, IMXEPITState),
-        VMSTATE_UINT32(freq, IMXEPITState),
         VMSTATE_PTIMER(timer_reload, IMXEPITState),
         VMSTATE_PTIMER(timer_cmp, IMXEPITState),
         VMSTATE_END_OF_LIST()
diff --git a/include/hw/timer/imx_epit.h b/include/hw/timer/imx_epit.h
index 783eaf0c3a..79aff0cec2 100644
--- a/include/hw/timer/imx_epit.h
+++ b/include/hw/timer/imx_epit.h
@@ -74,9 +74,7 @@ struct IMXEPITState {
     uint32_t sr;
     uint32_t lr;
     uint32_t cmp;
-    uint32_t cnt;
 
-    uint32_t freq;
     qemu_irq irq;
 };
 
-- 
2.34.5

Re: [PATCH qemu.git v3 7/8] hw/timer/imx_epit: remove explicit fields cnt and freq

[Peter Maydell]

On Thu, 1 Dec 2022 at 15:42, ~axelheider <axelheider@git.sr.ht> wrote:
>
> From: Axel Heider <axel.heider@hensoldt.net>
>
> The CNT register is a read-only register. There is no need to
> store it's value, it can be calculated on demand.
> The calculated frequency is needed temporarily only.
>
> Note that this is a migration compatibility break for all boards
> types that use the EPIT peripheral.

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

thanks
-- PMM

[PATCH qemu.git v3 1/8] hw/timer/imx_epit: improve comments

[~axelheider]

From: Axel Heider <axel.heider@hensoldt.net>

Fix typos, add background information

Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
---
 hw/timer/imx_epit.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index ec0fa440d7..2841fbaa1c 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -96,13 +96,14 @@ static void imx_epit_set_freq(IMXEPITState *s)
     }
 }
 
+/*
+ * This is called both on hardware (device) reset and software reset.
+ */
 static void imx_epit_reset(DeviceState *dev)
 {
     IMXEPITState *s = IMX_EPIT(dev);
 
-    /*
-     * Soft reset doesn't touch some bits; hard reset clears them
-     */
+    /* Soft reset doesn't touch some bits; hard reset clears them */
     s->cr &= (CR_EN|CR_ENMOD|CR_STOPEN|CR_DOZEN|CR_WAITEN|CR_DBGEN);
     s->sr = 0;
     s->lr = EPIT_TIMER_MAX;
@@ -214,6 +215,7 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
         ptimer_transaction_begin(s->timer_cmp);
         ptimer_transaction_begin(s->timer_reload);
 
+        /* Update the frequency. Has been done already in case of a reset. */
         if (!(s->cr & CR_SWR)) {
             imx_epit_set_freq(s);
         }
@@ -254,7 +256,7 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
         break;
 
     case 1: /* SR - ACK*/
-        /* writing 1 to OCIF clear the OCIF bit */
+        /* writing 1 to OCIF clears the OCIF bit */
         if (value & 0x01) {
             s->sr = 0;
             imx_epit_update_int(s);
@@ -352,8 +354,18 @@ static void imx_epit_realize(DeviceState *dev, Error **errp)
                           0x00001000);
     sysbus_init_mmio(sbd, &s->iomem);
 
+    /*
+     * The reload timer keeps running when the peripheral is enabled. It is a
+     * kind of wall clock that does not generate any interrupts. The callback
+     * needs to be provided, but it does nothing as the ptimer already supports
+     * all necessary reloading functionality.
+     */
     s->timer_reload = ptimer_init(imx_epit_reload, s, PTIMER_POLICY_LEGACY);
 
+    /*
+     * The compare timer is running only when the peripheral configuration is
+     * in a state that will generate compare interrupts.
+     */
     s->timer_cmp = ptimer_init(imx_epit_cmp, s, PTIMER_POLICY_LEGACY);
 }
 
-- 
2.34.5

Re: [PATCH qemu.git v3 1/8] hw/timer/imx_epit: improve comments

[Peter Maydell]

On Thu, 1 Dec 2022 at 15:42, ~axelheider <axelheider@git.sr.ht> wrote:
>
> From: Axel Heider <axel.heider@hensoldt.net>
>
> Fix typos, add background information
>
> Signed-off-by: Axel Heider <axel.heider@hensoldt.net>

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

thanks
-- PMM

[PATCH qemu.git v3 4/8] hw/timer/imx_epit: update interrupt state on CR write access

[~axelheider]

From: Axel Heider <axel.heider@hensoldt.net>

The interrupt state can change due to:
- reset clears both SR.OCIF and CR.OCIE
- write to CR.EN or CR.OCIE

Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
---
 hw/timer/imx_epit.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index f148868b8c..7af3a8b10e 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -206,12 +206,20 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
         if (s->cr & CR_SWR) {
             /* handle the reset */
             imx_epit_reset(DEVICE(s));
-            /*
-             * TODO: could we 'break' here? following operations appear
-             * to duplicate the work imx_epit_reset() already did.
-             */
         }
 
+        /*
+         * The interrupt state can change due to:
+         * - reset clears both SR.OCIF and CR.OCIE
+         * - write to CR.EN or CR.OCIE
+         */
+        imx_epit_update_int(s);
+
+        /*
+         * TODO: could we 'break' here for reset? following operations appear
+         * to duplicate the work imx_epit_reset() already did.
+         */
+
         ptimer_transaction_begin(s->timer_cmp);
         ptimer_transaction_begin(s->timer_reload);
 
-- 
2.34.5

Re: [PATCH qemu.git v3 4/8] hw/timer/imx_epit: update interrupt state on CR write access

[Peter Maydell]

On Thu, 1 Dec 2022 at 15:42, ~axelheider <axelheider@git.sr.ht> wrote:
>
> From: Axel Heider <axel.heider@hensoldt.net>
>
> The interrupt state can change due to:
> - reset clears both SR.OCIF and CR.OCIE
> - write to CR.EN or CR.OCIE
>
> Signed-off-by: Axel Heider <axel.heider@hensoldt.net>

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

thanks
-- PMM

[PATCH qemu.git v3 6/8] hw/timer/imx_epit: factor out register write handlers

[~axelheider]

From: Axel Heider <axel.heider@hensoldt.net>

Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
---
 hw/timer/imx_epit.c | 207 ++++++++++++++++++++++++--------------------
 1 file changed, 113 insertions(+), 94 deletions(-)

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index 39f47222d0..e04427542f 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -191,129 +191,148 @@ static void imx_epit_reload_compare_timer(IMXEPITState *s)
     }
 }
 
-static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
-                           unsigned size)
+static void imx_epit_write_cr(IMXEPITState *s, uint32_t value)
 {
-    IMXEPITState *s = IMX_EPIT(opaque);
-    uint64_t oldcr;
-
-    DPRINTF("(%s, value = 0x%08x)\n", imx_epit_reg_name(offset >> 2),
-            (uint32_t)value);
+    uint32_t oldcr = s->cr;
 
-    switch (offset >> 2) {
-    case 0: /* CR */
-
-        oldcr = s->cr;
-        s->cr = value & 0x03ffffff;
-        if (s->cr & CR_SWR) {
-            /* handle the reset */
-            imx_epit_reset(s, false);
-        }
+    s->cr = value & 0x03ffffff;
 
-        /*
-         * The interrupt state can change due to:
-         * - reset clears both SR.OCIF and CR.OCIE
-         * - write to CR.EN or CR.OCIE
-         */
-        imx_epit_update_int(s);
+    if (s->cr & CR_SWR) {
+        /* handle the reset */
+        imx_epit_reset(s, false);
+    }
 
-        /*
-         * TODO: could we 'break' here for reset? following operations appear
-         * to duplicate the work imx_epit_reset() already did.
-         */
+    /*
+     * The interrupt state can change due to:
+     * - reset clears both SR.OCIF and CR.OCIE
+     * - write to CR.EN or CR.OCIE
+     */
+    imx_epit_update_int(s);
 
-        ptimer_transaction_begin(s->timer_cmp);
-        ptimer_transaction_begin(s->timer_reload);
+    /*
+     * TODO: could we 'break' here for reset? following operations appear
+     * to duplicate the work imx_epit_reset() already did.
+     */
 
-        /* Update the frequency. Has been done already in case of a reset. */
-        if (!(s->cr & CR_SWR)) {
-            imx_epit_set_freq(s);
-        }
+    ptimer_transaction_begin(s->timer_cmp);
+    ptimer_transaction_begin(s->timer_reload);
 
-        if (s->freq && (s->cr & CR_EN) && !(oldcr & CR_EN)) {
-            if (s->cr & CR_ENMOD) {
-                if (s->cr & CR_RLD) {
-                    ptimer_set_limit(s->timer_reload, s->lr, 1);
-                    ptimer_set_limit(s->timer_cmp, s->lr, 1);
-                } else {
-                    ptimer_set_limit(s->timer_reload, EPIT_TIMER_MAX, 1);
-                    ptimer_set_limit(s->timer_cmp, EPIT_TIMER_MAX, 1);
-                }
-            }
+    /* Update the frequency. Has been done already in case of a reset. */
+    if (!(s->cr & CR_SWR)) {
+        imx_epit_set_freq(s);
+    }
 
-            imx_epit_reload_compare_timer(s);
-            ptimer_run(s->timer_reload, 0);
-            if (s->cr & CR_OCIEN) {
-                ptimer_run(s->timer_cmp, 0);
+    if (s->freq && (s->cr & CR_EN) && !(oldcr & CR_EN)) {
+        if (s->cr & CR_ENMOD) {
+            if (s->cr & CR_RLD) {
+                ptimer_set_limit(s->timer_reload, s->lr, 1);
+                ptimer_set_limit(s->timer_cmp, s->lr, 1);
             } else {
-                ptimer_stop(s->timer_cmp);
-            }
-        } else if (!(s->cr & CR_EN)) {
-            /* stop both timers */
-            ptimer_stop(s->timer_reload);
-            ptimer_stop(s->timer_cmp);
-        } else  if (s->cr & CR_OCIEN) {
-            if (!(oldcr & CR_OCIEN)) {
-                imx_epit_reload_compare_timer(s);
-                ptimer_run(s->timer_cmp, 0);
+                ptimer_set_limit(s->timer_reload, EPIT_TIMER_MAX, 1);
+                ptimer_set_limit(s->timer_cmp, EPIT_TIMER_MAX, 1);
             }
+        }
+
+        imx_epit_reload_compare_timer(s);
+        ptimer_run(s->timer_reload, 0);
+        if (s->cr & CR_OCIEN) {
+            ptimer_run(s->timer_cmp, 0);
         } else {
             ptimer_stop(s->timer_cmp);
         }
+    } else if (!(s->cr & CR_EN)) {
+        /* stop both timers */
+        ptimer_stop(s->timer_reload);
+        ptimer_stop(s->timer_cmp);
+    } else  if (s->cr & CR_OCIEN) {
+        if (!(oldcr & CR_OCIEN)) {
+            imx_epit_reload_compare_timer(s);
+            ptimer_run(s->timer_cmp, 0);
+        }
+    } else {
+        ptimer_stop(s->timer_cmp);
+    }
 
-        ptimer_transaction_commit(s->timer_cmp);
-        ptimer_transaction_commit(s->timer_reload);
+    ptimer_transaction_commit(s->timer_cmp);
+    ptimer_transaction_commit(s->timer_reload);
+}
+
+static void imx_epit_write_sr(IMXEPITState *s, uint32_t value)
+{
+    /* writing 1 to SR.OCIF clears this bit and turns the interrupt off */
+    if (value & SR_OCIF) {
+        s->sr = 0; /* SR.OCIF is the only bit in this register anyway */
+        imx_epit_update_int(s);
+    }
+}
+
+static void imx_epit_write_lr(IMXEPITState *s, uint32_t value)
+{
+    s->lr = value;
+
+    ptimer_transaction_begin(s->timer_cmp);
+    ptimer_transaction_begin(s->timer_reload);
+    if (s->cr & CR_RLD) {
+        /* Also set the limit if the LRD bit is set */
+        /* If IOVW bit is set then set the timer value */
+        ptimer_set_limit(s->timer_reload, s->lr, s->cr & CR_IOVW);
+        ptimer_set_limit(s->timer_cmp, s->lr, 0);
+    } else if (s->cr & CR_IOVW) {
+        /* If IOVW bit is set then set the timer value */
+        ptimer_set_count(s->timer_reload, s->lr);
+    }
+    /*
+     * Commit the change to s->timer_reload, so it can propagate. Otherwise
+     * the timer interrupt may not fire properly. The commit must happen
+     * before calling imx_epit_reload_compare_timer(), which reads
+     * s->timer_reload internally again.
+     */
+    ptimer_transaction_commit(s->timer_reload);
+    imx_epit_reload_compare_timer(s);
+    ptimer_transaction_commit(s->timer_cmp);
+}
+
+static void imx_epit_write_cmp(IMXEPITState *s, uint32_t value)
+{
+    s->cmp = value;
+
+    ptimer_transaction_begin(s->timer_cmp);
+    imx_epit_reload_compare_timer(s);
+    ptimer_transaction_commit(s->timer_cmp);
+}
+
+static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
+                           unsigned size)
+{
+    IMXEPITState *s = IMX_EPIT(opaque);
+
+    DPRINTF("(%s, value = 0x%08x)\n", imx_epit_reg_name(offset >> 2),
+            (uint32_t)value);
+
+    switch (offset >> 2) {
+    case 0: /* CR */
+        imx_epit_write_cr(s, (uint32_t)value);
         break;
 
-    case 1: /* SR - ACK*/
-        /* writing 1 to SR.OCIF clears this bit and turns the interrupt off */
-        if (value & SR_OCIF) {
-            s->sr = 0; /* SR.OCIF is the only bit in this register anyway */
-            imx_epit_update_int(s);
-        }
+    case 1: /* SR */
+        imx_epit_write_sr(s, (uint32_t)value);
         break;
 
-    case 2: /* LR - set ticks */
-        s->lr = value;
-
-        ptimer_transaction_begin(s->timer_cmp);
-        ptimer_transaction_begin(s->timer_reload);
-        if (s->cr & CR_RLD) {
-            /* Also set the limit if the LRD bit is set */
-            /* If IOVW bit is set then set the timer value */
-            ptimer_set_limit(s->timer_reload, s->lr, s->cr & CR_IOVW);
-            ptimer_set_limit(s->timer_cmp, s->lr, 0);
-        } else if (s->cr & CR_IOVW) {
-            /* If IOVW bit is set then set the timer value */
-            ptimer_set_count(s->timer_reload, s->lr);
-        }
-        /*
-         * Commit the change to s->timer_reload, so it can propagate. Otherwise
-         * the timer interrupt may not fire properly. The commit must happen
-         * before calling imx_epit_reload_compare_timer(), which reads
-         * s->timer_reload internally again.
-         */
-        ptimer_transaction_commit(s->timer_reload);
-        imx_epit_reload_compare_timer(s);
-        ptimer_transaction_commit(s->timer_cmp);
+    case 2: /* LR */
+        imx_epit_write_lr(s, (uint32_t)value);
         break;
 
     case 3: /* CMP */
-        s->cmp = value;
-
-        ptimer_transaction_begin(s->timer_cmp);
-        imx_epit_reload_compare_timer(s);
-        ptimer_transaction_commit(s->timer_cmp);
-
+        imx_epit_write_cmp(s, (uint32_t)value);
         break;
 
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
                       HWADDR_PRIx "\n", TYPE_IMX_EPIT, __func__, offset);
-
         break;
     }
 }
+
 static void imx_epit_cmp(void *opaque)
 {
     IMXEPITState *s = IMX_EPIT(opaque);
-- 
2.34.5

Re: [PATCH qemu.git v3 6/8] hw/timer/imx_epit: factor out register write handlers

[Peter Maydell]

On Thu, 1 Dec 2022 at 15:42, ~axelheider <axelheider@git.sr.ht> wrote:
>
> From: Axel Heider <axel.heider@hensoldt.net>
>
> Signed-off-by: Axel Heider <axel.heider@hensoldt.net>

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

thanks
-- PMM

[PATCH qemu.git v3 2/8] hw/timer/imx_epit: cleanup CR defines

[~axelheider]

From: Axel Heider <axel.heider@hensoldt.net>

remove unused defines, add needed defines

Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
---
 hw/timer/imx_epit.c         | 4 ++--
 include/hw/timer/imx_epit.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index 2841fbaa1c..661e9158e3 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -82,8 +82,8 @@ static void imx_epit_set_freq(IMXEPITState *s)
     uint32_t clksrc;
     uint32_t prescaler;
 
-    clksrc = extract32(s->cr, CR_CLKSRC_SHIFT, 2);
-    prescaler = 1 + extract32(s->cr, CR_PRESCALE_SHIFT, 12);
+    clksrc = extract32(s->cr, CR_CLKSRC_SHIFT, CR_CLKSRC_BITS);
+    prescaler = 1 + extract32(s->cr, CR_PRESCALE_SHIFT, CR_PRESCALE_BITS);
 
     s->freq = imx_ccm_get_clock_frequency(s->ccm,
                                 imx_epit_clocks[clksrc]) / prescaler;
diff --git a/include/hw/timer/imx_epit.h b/include/hw/timer/imx_epit.h
index 2acc41e982..e2cb96229b 100644
--- a/include/hw/timer/imx_epit.h
+++ b/include/hw/timer/imx_epit.h
@@ -43,7 +43,7 @@
 #define CR_OCIEN    (1 << 2)
 #define CR_RLD      (1 << 3)
 #define CR_PRESCALE_SHIFT (4)
-#define CR_PRESCALE_MASK  (0xfff)
+#define CR_PRESCALE_BITS  (12)
 #define CR_SWR      (1 << 16)
 #define CR_IOVW     (1 << 17)
 #define CR_DBGEN    (1 << 18)
@@ -51,7 +51,7 @@
 #define CR_DOZEN    (1 << 20)
 #define CR_STOPEN   (1 << 21)
 #define CR_CLKSRC_SHIFT (24)
-#define CR_CLKSRC_MASK  (0x3 << CR_CLKSRC_SHIFT)
+#define CR_CLKSRC_BITS  (2)
 
 #define EPIT_TIMER_MAX  0XFFFFFFFFUL
 
-- 
2.34.5

Re: [PATCH qemu.git v3 2/8] hw/timer/imx_epit: cleanup CR defines

[Peter Maydell]

On Thu, 1 Dec 2022 at 15:42, ~axelheider <axelheider@git.sr.ht> wrote:
>
> From: Axel Heider <axel.heider@hensoldt.net>
>
> remove unused defines, add needed defines
>
> Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
> ---


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

thanks
-- PMM

[PATCH qemu.git v3 3/8] hw/timer/imx_epit: define SR_OCIF

[~axelheider]

From: Axel Heider <axel.heider@hensoldt.net>

---
 hw/timer/imx_epit.c         | 12 ++++++------
 include/hw/timer/imx_epit.h |  2 ++
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index 661e9158e3..f148868b8c 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -66,7 +66,7 @@ static const IMXClk imx_epit_clocks[] =  {
  */
 static void imx_epit_update_int(IMXEPITState *s)
 {
-    if (s->sr && (s->cr & CR_OCIEN) && (s->cr & CR_EN)) {
+    if ((s->sr & SR_OCIF) && (s->cr & CR_OCIEN) && (s->cr & CR_EN)) {
         qemu_irq_raise(s->irq);
     } else {
         qemu_irq_lower(s->irq);
@@ -256,9 +256,9 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
         break;
 
     case 1: /* SR - ACK*/
-        /* writing 1 to OCIF clears the OCIF bit */
-        if (value & 0x01) {
-            s->sr = 0;
+        /* writing 1 to SR.OCIF clears this bit and turns the interrupt off */
+        if (value & SR_OCIF) {
+            s->sr = 0; /* SR.OCIF is the only bit in this register anyway */
             imx_epit_update_int(s);
         }
         break;
@@ -309,8 +309,8 @@ static void imx_epit_cmp(void *opaque)
     IMXEPITState *s = IMX_EPIT(opaque);
 
     DPRINTF("sr was %d\n", s->sr);
-
-    s->sr = 1;
+    /* Set interrupt status bit SR.OCIF and update the interrupt state */
+    s->sr |= SR_OCIF;
     imx_epit_update_int(s);
 }
 
diff --git a/include/hw/timer/imx_epit.h b/include/hw/timer/imx_epit.h
index e2cb96229b..783eaf0c3a 100644
--- a/include/hw/timer/imx_epit.h
+++ b/include/hw/timer/imx_epit.h
@@ -53,6 +53,8 @@
 #define CR_CLKSRC_SHIFT (24)
 #define CR_CLKSRC_BITS  (2)
 
+#define SR_OCIF     (1 << 0)
+
 #define EPIT_TIMER_MAX  0XFFFFFFFFUL
 
 #define TYPE_IMX_EPIT "imx.epit"
-- 
2.34.5

Re: [PATCH qemu.git v3 3/8] hw/timer/imx_epit: define SR_OCIF

[Peter Maydell]

On Thu, 1 Dec 2022 at 15:42, ~axelheider <axelheider@git.sr.ht> wrote:
>
> From: Axel Heider <axel.heider@hensoldt.net>
>
> ---

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

thanks
-- PMM

[PATCH qemu.git v3 5/8] hw/timer/imx_epit: hard reset initializes CR with 0

[~axelheider]

From: Axel Heider <axel.heider@hensoldt.net>

Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
---
 hw/timer/imx_epit.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index 7af3a8b10e..39f47222d0 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -99,12 +99,14 @@ static void imx_epit_set_freq(IMXEPITState *s)
 /*
  * This is called both on hardware (device) reset and software reset.
  */
-static void imx_epit_reset(DeviceState *dev)
+static void imx_epit_reset(IMXEPITState *s, bool is_hard_reset)
 {
-    IMXEPITState *s = IMX_EPIT(dev);
-
     /* Soft reset doesn't touch some bits; hard reset clears them */
-    s->cr &= (CR_EN|CR_ENMOD|CR_STOPEN|CR_DOZEN|CR_WAITEN|CR_DBGEN);
+    if (is_hard_reset) {
+        s->cr = 0;
+    } else {
+        s->cr &= (CR_EN|CR_ENMOD|CR_STOPEN|CR_DOZEN|CR_WAITEN|CR_DBGEN);
+    }
     s->sr = 0;
     s->lr = EPIT_TIMER_MAX;
     s->cmp = 0;
@@ -205,7 +207,7 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
         s->cr = value & 0x03ffffff;
         if (s->cr & CR_SWR) {
             /* handle the reset */
-            imx_epit_reset(DEVICE(s));
+            imx_epit_reset(s, false);
         }
 
         /*
@@ -377,12 +379,18 @@ static void imx_epit_realize(DeviceState *dev, Error **errp)
     s->timer_cmp = ptimer_init(imx_epit_cmp, s, PTIMER_POLICY_LEGACY);
 }
 
+static void imx_epit_dev_reset(DeviceState *dev)
+{
+    IMXEPITState *s = IMX_EPIT(dev);
+    imx_epit_reset(s, true);
+}
+
 static void imx_epit_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc  = DEVICE_CLASS(klass);
 
     dc->realize = imx_epit_realize;
-    dc->reset = imx_epit_reset;
+    dc->reset = imx_epit_dev_reset;
     dc->vmsd = &vmstate_imx_timer_epit;
     dc->desc = "i.MX periodic timer";
 }
-- 
2.34.5

Re: [PATCH qemu.git v3 5/8] hw/timer/imx_epit: hard reset initializes CR with 0

[Peter Maydell]

On Thu, 1 Dec 2022 at 15:42, ~axelheider <axelheider@git.sr.ht> wrote:
>
> From: Axel Heider <axel.heider@hensoldt.net>
>
> Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
> ---
>  hw/timer/imx_epit.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)

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

thanks
-- PMM

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

[~axelheider]

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>
---
 hw/timer/imx_epit.c | 184 ++++++++++++++++++++++++++------------------
 1 file changed, 110 insertions(+), 74 deletions(-)

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index cf13496165..663907f9cf 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 <jcd@tribudubois.net>
+ * Updated by Axel Heider
  *
  * This code is licensed under GPL version 2 or later.  See
  * the COPYING file in the top-level directory.
@@ -151,95 +152,130 @@ 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 won't 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 bit affects interrupt generation only. The
+     * compare timer needs to run even if no interrupts are to be generated,
+     * because the SR.OCIF bit must be updated also.
+     * 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;
 
     s->cr = value & 0x03ffffff;
 
     if (s->cr & CR_SWR) {
-        /* handle the reset */
+        /*
+         * Reset clears CR.SWR again. It does not touch CR.EN, but the timers
+         * are still stopped because the input clock is disabled.
+         */
         imx_epit_reset(s, false);
-    }
-
-    /*
-     * The interrupt state can change due to:
-     * - reset clears both SR.OCIF and CR.OCIE
-     * - write to CR.EN or CR.OCIE
-     */
-    imx_epit_update_int(s);
-
-    /*
-     * TODO: could we 'break' here for reset? following operations appear
-     * to duplicate the work imx_epit_reset() already did.
-     */
-
-    ptimer_transaction_begin(s->timer_cmp);
-    ptimer_transaction_begin(s->timer_reload);
-
-    /*
-     * Update the frequency. In case of a reset the input clock was
-     * switched off, so this can be skipped.
-     */
-    if (!(s->cr & CR_SWR)) {
-        freq = imx_epit_get_freq(s);
+    } 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);
         }
-    }
-
-    if (freq && (s->cr & CR_EN) && !(oldcr & CR_EN)) {
-        if (s->cr & CR_ENMOD) {
-            if (s->cr & CR_RLD) {
-                ptimer_set_limit(s->timer_reload, s->lr, 1);
-                ptimer_set_limit(s->timer_cmp, s->lr, 1);
-            } else {
-                ptimer_set_limit(s->timer_reload, EPIT_TIMER_MAX, 1);
-                ptimer_set_limit(s->timer_cmp, EPIT_TIMER_MAX, 1);
+        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);
+        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);
+            if (set_limit) {
+                ptimer_set_limit(s->timer_cmp, limit, 0);
             }
         }
-
-        imx_epit_reload_compare_timer(s);
-        ptimer_run(s->timer_reload, 0);
-        if (s->cr & CR_OCIEN) {
-            ptimer_run(s->timer_cmp, 0);
+        /*
+         * If there is an input clock and the peripheral is enabled, then
+         * ensure the wall clock timer is ticking. Otherwise stop the timers.
+         * The compare timer will be updated later.
+         */
+        if (freq && (s->cr & CR_EN)) {
+            ptimer_run(s->timer_reload, 0);
         } else {
-            ptimer_stop(s->timer_cmp);
-        }
-    } else if (!(s->cr & CR_EN)) {
-        /* stop both timers */
-        ptimer_stop(s->timer_reload);
-        ptimer_stop(s->timer_cmp);
-    } else  if (s->cr & CR_OCIEN) {
-        if (!(oldcr & CR_OCIEN)) {
-            imx_epit_reload_compare_timer(s);
-            ptimer_run(s->timer_cmp, 0);
+            ptimer_stop(s->timer_reload);
         }
-    } else {
-        ptimer_stop(s->timer_cmp);
+        /* Commit changes to reload timer, so they can propagate. */
+        ptimer_transaction_commit(s->timer_reload);
+        /* Update compare timer based on the committed reload timer value. */
+        imx_epit_update_compare_timer(s);
+        ptimer_transaction_commit(s->timer_cmp);
     }
 
-    ptimer_transaction_commit(s->timer_cmp);
-    ptimer_transaction_commit(s->timer_reload);
+    /*
+     * The interrupt state can change due to:
+     * - reset clears both SR.OCIF and CR.OCIE
+     * - write to CR.EN or CR.OCIE
+     */
+    imx_epit_update_int(s);
 }
 
 static void imx_epit_write_sr(IMXEPITState *s, uint32_t value)
@@ -266,14 +302,10 @@ static void imx_epit_write_lr(IMXEPITState *s, uint32_t value)
         /* If IOVW bit is set then set the timer value */
         ptimer_set_count(s->timer_reload, s->lr);
     }
-    /*
-     * Commit the change to s->timer_reload, so it can propagate. Otherwise
-     * the timer interrupt may not fire properly. The commit must happen
-     * before calling imx_epit_reload_compare_timer(), which reads
-     * s->timer_reload internally again.
-     */
+    /* Commit the changes to s->timer_reload, so they can propagate. */
     ptimer_transaction_commit(s->timer_reload);
-    imx_epit_reload_compare_timer(s);
+    /* Update the compare timer based on the committed reload timer value. */
+    imx_epit_update_compare_timer(s);
     ptimer_transaction_commit(s->timer_cmp);
 }
 
@@ -281,8 +313,9 @@ static void imx_epit_write_cmp(IMXEPITState *s, uint32_t value)
 {
     s->cmp = value;
 
+    /* Update the compare timer based on the committed reload timer value. */
     ptimer_transaction_begin(s->timer_cmp);
-    imx_epit_reload_compare_timer(s);
+    imx_epit_update_compare_timer(s);
     ptimer_transaction_commit(s->timer_cmp);
 }
 
@@ -322,6 +355,9 @@ static void imx_epit_cmp(void *opaque)
 {
     IMXEPITState *s = IMX_EPIT(opaque);
 
+    /* The cmp ptimer can't be running when the peripheral is disabled */
+    assert(s->cr & CR_EN);
+
     DPRINTF("sr was %d\n", s->sr);
     /* Set interrupt status bit SR.OCIF and update the interrupt state */
     s->sr |= SR_OCIF;
-- 
2.34.5

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

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

[Axel Heider]

Peter,

> 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:
> [...]

Thanks, that makes things easier. Seems these still unfortunately
slipped in my last patch iteration. Your changes look good to me.

Axel

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>

Hi; Coverity has just noticed an issue with this patch:


> -/* 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;

Here we declare the is_oneshot variable...

> +    /* 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);

...but here we declare another is_oneshot, which shadows the first
declaration...

> +        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 won't 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 bit affects interrupt generation only. The
> +     * compare timer needs to run even if no interrupts are to be generated,
> +     * because the SR.OCIF bit must be updated also.
> +     * 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);

...so here when the inner variable is no longer in scope, the
value of the outer is_oneshot variable must always be 'false',
because there's never any assignment to it.

> +    } else {
> +        ptimer_stop(s->timer_cmp);
> +    }
> +
>  }

What was the intention here? My guess is that there should only
have been one 'is_oneshot', not two.

There's also been this bug report:
https://gitlab.com/qemu-project/qemu/-/issues/1491
which suggests that the condition for setting is_oneshot
should be "(limit <= s->cmp)" rather than ">=".

What do you think ?

thanks
-- PMM

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

[Axel Heider]

Peter,

> Hi; Coverity has just noticed an issue with this patch:
> [...]
> Here we declare the is_oneshot variable...
> [...]
> ...but here we declare another is_oneshot, which shadows the first
> declaration...
> ...so here when the inner variable is no longer in scope, the
> value of the outer is_oneshot variable must always be 'false',
> because there's never any assignment to it.
> What was the intention here? My guess is that there should only
> have been one 'is_oneshot', not two.

The shadowing is not intended, as this does not make any sense. There
is only one instance of this variable, it is FALSE by default and can
become TRUE.

> [...]
> There's also been this bug report:
> https://gitlab.com/qemu-project/qemu/-/issues/1491
> which suggests that the condition for setting is_oneshot
> should be "(limit <= s->cmp)" rather than ">=".
> What do you think ?

The Bug report is right, that the check should be
"(limit <= s->cmp)", as it's about the on-shot characteristic and not
the periodic characteristic (which earler version of the patch had).


I will provide a patch to fix this.


Axel

Re: [PATCH qemu.git v3 0/8] hw/timer/imx_epit: improve and fix EPIT compare timer

[Peter Maydell]

On Thu, 1 Dec 2022 at 15:42, ~axelheider <axelheider@git.sr.ht> wrote:
>
> This patch set improves the i.MX EPIT emulation:
> - fix #1263 for writes to CR
> - ensure SR_OCIF is set properly even if CR_OCIEN is off
> - hardware reset initialized CR to 0
> - interrupt state update on CR writes (e.g. software reset)
> - remove explicit fields cnt and freq (they are redundant)
> - general code and documentation improvements
>
> v3 addresses the comments from the previous iterations, but still keeps
> the scope of this patchset limited to addressing the obvious bugs in the
> behavior. It does not try to improve the timer accuracy, thus the
> following remarks remain to be fixed in a future patch after this one is
> merged:
> - don't use PTIMER_POLICY_LEGACY. Fine tuning this requires more
>   time and currently this is not a major concern, because the timer is
>   working reasonably well.
> - replace the modestly harmful sequence
>         counter = ptimer_get_count(s->timer_reload);
>         ...
>         ptimer_set_count(s->timer_cmp, counter);
>   by something better that does not lose or gain time. The current
>   patchset does not introduce this sequence, it has been there
>   before already. Again,  the current lack of accuracy here is not a
>   major concern because the timer is working reasonably well.

Applied to target-arm.next, thanks. Sorry it took me so long to
get to this.

-- PMM

Re: [PATCH qemu.git v3 0/8] hw/timer/imx_epit: improve and fix EPIT compare timer

[Axel Heider]

Peter,

> Applied to target-arm.next, thanks. Sorry it took me so long to> get to this.

No worries. Thanks for picking up the changes, and I really appreciate
all the review feedback.

Axel