[PATCH qemu.git v2 0/9] hw/timer/imx_epit: imprive and fix compare timer handling

[PATCH qemu.git v2 0/9] hw/timer/imx_epit: imprive and fix compare timer handling

[~axelheider]

This patch set improves the i.MX EPIT emulation:
- fix #1263 for writes to CR also
- do not persist CR.SWR bit
- remove explicit fields cnt and freq
- software reset clears the interrupt
- general code and documentation improvements

Axel Heider (9):
  hw/timer/imx_epit: improve comments
  hw/timer/imx_epit: cleanup CR defines
  hw/timer/imx_epit: simplify interrupt logic
  hw/timer/imx_epit: software reset clears the interrupt
  hw/timer/imx_epit: do not persist CR.SWR bit
  hw/timer/imx_epit: remove explicit fields cnt and freq
  hw/timer/imx_epit: factor out register write handlers
  hw/timer/imx_epit: change reset handling
  hw/timer/imx_epit: fix compare timer handling

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

-- 
2.34.5

[PATCH qemu.git v2 6/9] 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.

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

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index 6af460946f..b0ef727efb 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -61,27 +61,16 @@ static const IMXClk imx_epit_clocks[] =  {
     CLK_32k,       /* 11 ipg_clk_32k -- ~32kHz */
 };
 
-/*
- * 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;
 }
 
 static void imx_epit_reset(DeviceState *dev)
@@ -93,36 +82,26 @@ static void imx_epit_reset(DeviceState *dev)
     s->sr = 0;
     s->lr = EPIT_TIMER_MAX;
     s->cmp = 0;
-    s->cnt = 0;
-
     /* clear the interrupt */
     qemu_irq_lower(s->irq);
 
     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(0 == imx_epit_get_freq(s));
     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);
@@ -146,8 +125,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:
@@ -166,7 +144,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 */
@@ -182,6 +160,7 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
                            unsigned size)
 {
     IMXEPITState *s = IMX_EPIT(opaque);
+    uint64_t freq = 0;
     uint64_t oldcr;
 
     DPRINTF("(%s, value = 0x%08x)\n", imx_epit_reg_name(offset >> 2),
@@ -205,12 +184,19 @@ 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. */
+        /*
+         * Update the frequency. In case of a reset the input clock was
+         * switched off, so this can be skipped.
+         */
         if (!(value & 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);
@@ -324,15 +310,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 e2cb96229b..f6d41be7e1 100644
--- a/include/hw/timer/imx_epit.h
+++ b/include/hw/timer/imx_epit.h
@@ -72,9 +72,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 v2 6/9] hw/timer/imx_epit: remove explicit fields cnt and freq

[Peter Maydell]

On Mon, 7 Nov 2022 at 16: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.

This patch bumps the vmstate version ID for the device, which
is a migration compatibility break. For this device/SoC,
that's fine, but we generally prefer to note the break
explicitly in the commit message (eg see commit 759ae1b47e7
or commit 23f6e3b11be for examples).

> Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
> ---
>  hw/timer/imx_epit.c         | 76 +++++++++++++++----------------------
>  include/hw/timer/imx_epit.h |  2 -
>  2 files changed, 30 insertions(+), 48 deletions(-)
>
> diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
> index 6af460946f..b0ef727efb 100644
> --- a/hw/timer/imx_epit.c
> +++ b/hw/timer/imx_epit.c
> @@ -61,27 +61,16 @@ static const IMXClk imx_epit_clocks[] =  {
>      CLK_32k,       /* 11 ipg_clk_32k -- ~32kHz */
>  };
>
> -/*
> - * 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);

These lines have been reformatted but that doesn't have anything
to do with the change to switch from s->freq to a local variable.
If you want to make formatting-type changes can you keep those
separate from other patches, please? It makes things a lot easier
to review.

> +    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;
>  }
>
>  static void imx_epit_reset(DeviceState *dev)
> @@ -93,36 +82,26 @@ static void imx_epit_reset(DeviceState *dev)
>      s->sr = 0;
>      s->lr = EPIT_TIMER_MAX;
>      s->cmp = 0;
> -    s->cnt = 0;
> -
>      /* clear the interrupt */
>      qemu_irq_lower(s->irq);
>
>      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(0 == imx_epit_get_freq(s));

Don't use yoda conditionals, please. "imx_epit_get_freq(s) == 0" is the
QEMU standard way to write this.

>      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);
>  }

Otherwise the patch looks good.

thanks
-- PMM

[PATCH qemu.git v2 3/9] hw/timer/imx_epit: simplify interrupt logic

[~axelheider]

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

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

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index 8ec770f674..2e9dae0bc8 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -61,18 +61,6 @@ static const IMXClk imx_epit_clocks[] =  {
     CLK_32k,       /* 11 ipg_clk_32k -- ~32kHz */
 };
 
-/*
- * Update interrupt status
- */
-static void imx_epit_update_int(IMXEPITState *s)
-{
-    if (s->sr && (s->cr & CR_OCIEN) && (s->cr & CR_EN)) {
-        qemu_irq_raise(s->irq);
-    } else {
-        qemu_irq_lower(s->irq);
-    }
-}
-
 /*
  * Must be called from within a ptimer_transaction_begin/commit block
  * for both s->timer_cmp and s->timer_reload.
@@ -253,10 +241,10 @@ 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 */
+        /* writing 1 to OCIF clears the OCIF bit and the interrupt */
         if (value & 0x01) {
             s->sr = 0;
-            imx_epit_update_int(s);
+            qemu_irq_lower(s->irq);
         }
         break;
 
@@ -305,10 +293,17 @@ static void imx_epit_cmp(void *opaque)
 {
     IMXEPITState *s = IMX_EPIT(opaque);
 
+    /* Set the interrupt status flag to signaled. */
     DPRINTF("sr was %d\n", s->sr);
-
     s->sr = 1;
-    imx_epit_update_int(s);
+
+    /*
+     * An actual interrupt is generated only if the peripheral is enabled
+     * and the interrupt generation is enabled.
+     */
+    if ((s->cr & (CR_EN | CR_OCIEN)) == (CR_EN | CR_OCIEN)) {
+        qemu_irq_raise(s->irq);
+    }
 }
 
 static void imx_epit_reload(void *opaque)
-- 
2.34.5

Re: [PATCH qemu.git v2 3/9] hw/timer/imx_epit: simplify interrupt logic

[Peter Maydell]

On Mon, 7 Nov 2022 at 16: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 | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
> index 8ec770f674..2e9dae0bc8 100644
> --- a/hw/timer/imx_epit.c
> +++ b/hw/timer/imx_epit.c
> @@ -61,18 +61,6 @@ static const IMXClk imx_epit_clocks[] =  {
>      CLK_32k,       /* 11 ipg_clk_32k -- ~32kHz */
>  };
>
> -/*
> - * Update interrupt status
> - */
> -static void imx_epit_update_int(IMXEPITState *s)
> -{
> -    if (s->sr && (s->cr & CR_OCIEN) && (s->cr & CR_EN)) {
> -        qemu_irq_raise(s->irq);
> -    } else {
> -        qemu_irq_lower(s->irq);
> -    }
> -}
> -
>  /*
>   * Must be called from within a ptimer_transaction_begin/commit block
>   * for both s->timer_cmp and s->timer_reload.
> @@ -253,10 +241,10 @@ 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 */
> +        /* writing 1 to OCIF clears the OCIF bit and the interrupt */
>          if (value & 0x01) {
>              s->sr = 0;
> -            imx_epit_update_int(s);
> +            qemu_irq_lower(s->irq);
>          }
>          break;
>
> @@ -305,10 +293,17 @@ static void imx_epit_cmp(void *opaque)
>  {
>      IMXEPITState *s = IMX_EPIT(opaque);
>
> +    /* Set the interrupt status flag to signaled. */
>      DPRINTF("sr was %d\n", s->sr);
> -
>      s->sr = 1;
> -    imx_epit_update_int(s);
> +
> +    /*
> +     * An actual interrupt is generated only if the peripheral is enabled
> +     * and the interrupt generation is enabled.
> +     */
> +    if ((s->cr & (CR_EN | CR_OCIEN)) == (CR_EN | CR_OCIEN)) {
> +        qemu_irq_raise(s->irq);
> +    }
>  }

Having an "update interrupt" function is the more common
convention in QEMU device models -- it means you have one
function you can call from any point where you've updated
any of the state that affects whether an interrupt is generated or not.

For instance there's currently I think a bug where when the
guest writes to the CR register and changes the value of
the CR_OCIEN bit we aren't updating the state of the IRQ line.
If you keep the imx_epit_update_int() function then fixing
that bug is fairly straightforward: you just need to call it
in the appropriate place. Without the function then the
logic of "what is the IRQ line state given the current
device register state" ends up dispersed across the device
model code and potentially duplicated.

thanks
-- PMM

Re: [PATCH qemu.git v2 3/9] hw/timer/imx_epit: simplify interrupt logic

[Axel Heider]

> Having an "update interrupt" function is the more common
> convention in QEMU device models -- it means you have one
> function you can call from any point where you've updated
> any of the state that affects whether an interrupt is generated or not.

Ok, will keept it.

> For instance there's currently I think a bug where when the
> guest writes to the CR register and changes the value of
> the CR_OCIEN bit we aren't updating the state of the IRQ line.
> If you keep the imx_epit_update_int() function then fixing
> that bug is fairly straightforward: you just need to call it
> in the appropriate place. Without the function then the
> logic of "what is the IRQ line state given the current
> device register state" ends up dispersed across the device
> model code and potentially duplicated.

That bug is fixed for the next iteration of this patch set.

Axel

[PATCH qemu.git v2 1/9] 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 | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index ec0fa440d7..4af730593f 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -100,9 +100,7 @@ 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 +212,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 +253,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 +351,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 v2 1/9] hw/timer/imx_epit: improve comments

[Peter Maydell]

On Mon, 7 Nov 2022 at 16: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 v2 4/9] hw/timer/imx_epit: software reset clears the interrupt

[~axelheider]

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

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

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index 2e9dae0bc8..5315d9633e 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -94,6 +94,10 @@ static void imx_epit_reset(DeviceState *dev)
     s->lr = EPIT_TIMER_MAX;
     s->cmp = 0;
     s->cnt = 0;
+
+    /* clear the interrupt */
+    qemu_irq_lower(s->irq);
+
     ptimer_transaction_begin(s->timer_cmp);
     ptimer_transaction_begin(s->timer_reload);
     /* stop both timers */
-- 
2.34.5

Re: [PATCH qemu.git v2 4/9] hw/timer/imx_epit: software reset clears the interrupt

[Peter Maydell]

On Mon, 7 Nov 2022 at 16: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 | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
> index 2e9dae0bc8..5315d9633e 100644
> --- a/hw/timer/imx_epit.c
> +++ b/hw/timer/imx_epit.c
> @@ -94,6 +94,10 @@ static void imx_epit_reset(DeviceState *dev)
>      s->lr = EPIT_TIMER_MAX;
>      s->cmp = 0;
>      s->cnt = 0;
> +
> +    /* clear the interrupt */
> +    qemu_irq_lower(s->irq);
> +
>      ptimer_transaction_begin(s->timer_cmp);
>      ptimer_transaction_begin(s->timer_reload);
>      /* stop both timers */
> --

It's not valid to call qemu_irq_set/qemu_irq_lower from a legacy
reset function (because whether it has an effect or not
depends on whether the device on the other end of the line gets
reset before or after this one, and there is no guaranteed
order for devices being reset). The convention is that if the
post-reset state of the IRQ line is 0, then you don't do anything.
The device on the other end will reset into a state corresponding
to "the input line is 0".

thanks
-- PMM

[PATCH qemu.git v2 7/9] 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 | 211 ++++++++++++++++++++++++--------------------
 1 file changed, 115 insertions(+), 96 deletions(-)

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index b0ef727efb..30280a9ac1 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -156,130 +156,149 @@ 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 freq = 0;
-    uint64_t oldcr;
-
-    DPRINTF("(%s, value = 0x%08x)\n", imx_epit_reg_name(offset >> 2),
-            (uint32_t)value);
+    uint32_t freq = 0;
+    uint32_t oldcr = s->cr;
 
-    switch (offset >> 2) {
-    case 0: /* CR */
-
-        oldcr = s->cr;
-        /* SWR bit is never persisted, it clears itself once reset is done */
-        s->cr = (value & ~CR_SWR) & 0x03ffffff;
-        if (value & 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.
-             */
-        }
-
-        ptimer_transaction_begin(s->timer_cmp);
-        ptimer_transaction_begin(s->timer_reload);
+    /* SWR bit is never persisted, it clears itself once reset is done */
+    s->cr = (value & ~CR_SWR) & 0x03ffffff;
 
+    if (value & CR_SWR) {
+        /* handle the reset */
+        imx_epit_reset(DEVICE(s));
         /*
-         * Update the frequency. In case of a reset the input clock was
-         * switched off, so this can be skipped.
+         * TODO: could we 'break' here? following operations appear
+         * to duplicate the work imx_epit_reset() already did.
          */
-        if (!(value & CR_SWR)) {
-            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);
-                }
-            }
+    ptimer_transaction_begin(s->timer_cmp);
+    ptimer_transaction_begin(s->timer_reload);
 
-            imx_epit_reload_compare_timer(s);
-            ptimer_run(s->timer_reload, 0);
-            if (s->cr & CR_OCIEN) {
-                ptimer_run(s->timer_cmp, 0);
+    /*
+     * Update the frequency. In case of a reset the input clock was
+     * switched off, so this can be skipped.
+     */
+    if (!(value & CR_SWR)) {
+        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_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);
+}
+
+static void imx_epit_write_sr(IMXEPITState *s, uint32_t value)
+{
+    /* writing 1 to OCIF clears the OCIF bit and the interrupt */
+    if (value & 0x01) {
+        s->sr = 0;
+        qemu_irq_lower(s->irq);
+    }
+}
+
+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);
 
-        ptimer_transaction_commit(s->timer_cmp);
-        ptimer_transaction_commit(s->timer_reload);
+    switch (offset >> 2) {
+    case 0: /* CR */
+        imx_epit_write_cr(s, (uint32_t)value);
         break;
 
-    case 1: /* SR - ACK*/
-        /* writing 1 to OCIF clears the OCIF bit and the interrupt */
-        if (value & 0x01) {
-            s->sr = 0;
-            qemu_irq_lower(s->irq);
-        }
+    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 v2 7/9] hw/timer/imx_epit: factor out register write handlers

[Peter Maydell]

On Mon, 7 Nov 2022 at 16: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 | 211 ++++++++++++++++++++++++--------------------
>  1 file changed, 115 insertions(+), 96 deletions(-)

Good idea (unfortunate that git diff has not shown the change
in a very clear way, but it does that sometimes). I won't
review this one for now because changes I've suggested for
earlier patches are going to affect the detail on this one.

(If you don't already, you might try setting
   git config --global diff.algorithm histogram
which I think often produces nicer patches. I don't know
whether it will help in this particular case, though.)

thanks
-- PMM

[PATCH qemu.git v2 2/9] 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 4af730593f..8ec770f674 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 v2 2/9] hw/timer/imx_epit: cleanup CR defines

[Peter Maydell]

On Mon, 7 Nov 2022 at 16: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 v2 5/9] hw/timer/imx_epit: do not persist CR.SWR bit

[~axelheider]

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

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

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index 5315d9633e..6af460946f 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -191,8 +191,9 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
     case 0: /* CR */
 
         oldcr = s->cr;
-        s->cr = value & 0x03ffffff;
-        if (s->cr & CR_SWR) {
+        /* SWR bit is never persisted, it clears itself once reset is done */
+        s->cr = (value & ~CR_SWR) & 0x03ffffff;
+        if (value & CR_SWR) {
             /* handle the reset */
             imx_epit_reset(DEVICE(s));
             /*
@@ -205,7 +206,7 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
         ptimer_transaction_begin(s->timer_reload);
 
         /* Update the frequency. Has been done already in case of a reset. */
-        if (!(s->cr & CR_SWR)) {
+        if (!(value & CR_SWR)) {
             imx_epit_set_freq(s);
         }
 
-- 
2.34.5

Re: [PATCH qemu.git v2 5/9] hw/timer/imx_epit: do not persist CR.SWR bit

[Peter Maydell]

On Mon, 7 Nov 2022 at 16: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 | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
> index 5315d9633e..6af460946f 100644
> --- a/hw/timer/imx_epit.c
> +++ b/hw/timer/imx_epit.c
> @@ -191,8 +191,9 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
>      case 0: /* CR */
>
>          oldcr = s->cr;
> -        s->cr = value & 0x03ffffff;
> -        if (s->cr & CR_SWR) {
> +        /* SWR bit is never persisted, it clears itself once reset is done */
> +        s->cr = (value & ~CR_SWR) & 0x03ffffff;
> +        if (value & CR_SWR) {
>              /* handle the reset */
>              imx_epit_reset(DEVICE(s));
>              /*

There's a comment just below here that says "can we just 'break'
in this case?". That's there because last time we had to touch
this device we didn't have enough specific knowledge of the hardware
or test cases so we made a refactor that left the code with the same
probably-incorrect behaviour it had before the refactor. But, since
you're working on this device anyway: can we simply add the "break"
after imx_epit_reset() ?

If we can just say "if CR_SWR is set then the device resets like
a hardware reset", then this all simplifies out a lot and this
patch isn't necessary at all. (imx_epit_reset() clears the CR_SWR bit.)
I'm fairly sure we ought to be able to do that, and the missing 'break'
was just a bug...

> @@ -205,7 +206,7 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
>          ptimer_transaction_begin(s->timer_reload);
>
>          /* Update the frequency. Has been done already in case of a reset. */
> -        if (!(s->cr & CR_SWR)) {
> +        if (!(value & CR_SWR)) {
>              imx_epit_set_freq(s);
>          }

thanks
-- PMM

Re: [PATCH qemu.git v2 5/9] hw/timer/imx_epit: do not persist CR.SWR bit

[Axel Heider]

>>
>> From: Axel Heider <axel.heider@hensoldt.net>
>> Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
>> ---
>>   hw/timer/imx_epit.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
>> index 5315d9633e..6af460946f 100644
>> --- a/hw/timer/imx_epit.c
>> +++ b/hw/timer/imx_epit.c
>> @@ -191,8 +191,9 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
>>       case 0: /* CR */
>>
>>           oldcr = s->cr;
>> -        s->cr = value & 0x03ffffff;
>> -        if (s->cr & CR_SWR) {
>> +        /* SWR bit is never persisted, it clears itself once reset is done */
>> +        s->cr = (value & ~CR_SWR) & 0x03ffffff;
>> +        if (value & CR_SWR) {
>>               /* handle the reset */
>>               imx_epit_reset(DEVICE(s));
>>               /*
>>
>
> There's a comment just below here that says "can we just 'break'
> in this case?". That's there because last time we had to touch
> this device we didn't have enough specific knowledge of the hardware
> or test cases so we made a refactor that left the code with the same
> probably-incorrect behaviour it had before the refactor. But, since
> you're working on this device anyway: can we simply add the "break"
> after imx_epit_reset() ?
>
> If we can just say "if CR_SWR is set then the device resets like
> a hardware reset", then this all simplifies out a lot and this
> patch isn't necessary at all. (imx_epit_reset() clears the CR_SWR bit.)
> I'm fairly sure we ought to be able to do that, and the missing 'break'
> was just a bug...


You are right, this patch is not needed. Actually, the refactoring in
the other patches in this series makes this quite obvious then anyway.
And you comment fully holds, eventually all the setup is skipped in
case of a reset.

I will drop this patch from the set. Thanks for the review.

Axel

[PATCH qemu.git v2 9/9] hw/timer/imx_epit: fix compare timer handling

[~axelheider]

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

- 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 <axel.heider@hensoldt.net>
---
 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 <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.
@@ -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) & 0x03ffffff;
-
-    ptimer_transaction_begin(s->timer_cmp);
-    ptimer_transaction_begin(s->timer_reload);
+    uint32_t toggled_cr = old_cr ^ s->cr;
 
     if (value & CR_SWR) {
         /* Soft reset doesn't touch some bits; only a hard reset clears them */
@@ -149,49 +201,52 @@ static void imx_epit_write_cr(IMXEPITState *s, uint32_t value)
         /* turn interrupt off since SR and the OCIEN bit got cleared */
         qemu_irq_lower(s->irq);
         /* reset timer limits, set timer values to these limits */
-        ptimer_set_limit(s->timer_cmp, EPIT_TIMER_MAX, 1);
-        ptimer_set_limit(s->timer_reload, EPIT_TIMER_MAX, 1);
+        set_limit = true;
+        set_counter = true;
     } else {
-        freq = imx_epit_get_freq(s);
-        if (freq) {
-            ptimer_set_freq(s->timer_reload, freq);
-            ptimer_set_freq(s->timer_cmp, freq);
-        }
+        /* re-initialize the limits if CR.RLD has changed */
+        set_limit = toggled_cr & CR_RLD;
+        /* set the counter if the timer got just enabled and CR.ENMOD is set */
+        set_counter = ((toggled_cr & s->cr) & CR_EN) && (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 (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);
-            }
+    if (set_limit || set_counter) {
+        uint64_t limit = (s->cr & CR_RLD) ? s->lr : EPIT_TIMER_MAX;
+        if (set_limit) {
+            ptimer_set_limit(s->timer_cmp, limit, 0);
         }
+        ptimer_set_limit(s->timer_reload, limit, set_counter ? 1 : 0);
+    }
 
-        imx_epit_reload_compare_timer(s);
+    /*
+     * 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);
-        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_reload);
+        /* Stop the compare timer also. This just plays safe, the call to
+         * imx_epit_update_compare_timer() below should also so this. */
         ptimer_stop(s->timer_cmp);
     }
 
-    ptimer_transaction_commit(s->timer_cmp);
+    /* Commit the changes to s->timer_reload, so they can propagate. */
     ptimer_transaction_commit(s->timer_reload);
+
+    /* Update the compare timer based on the committed reload timer value. */
+    imx_epit_update_compare_timer(s);
+    ptimer_transaction_commit(s->timer_cmp);
 }
 
 static void imx_epit_write_sr(IMXEPITState *s, uint32_t value)
@@ -218,14 +273,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);
 }
 
@@ -233,8 +284,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);
 }
 
@@ -274,16 +326,22 @@ static void imx_epit_cmp(void *opaque)
 {
     IMXEPITState *s = IMX_EPIT(opaque);
 
-    /* Set the interrupt status flag to signaled. */
-    DPRINTF("sr was %d\n", s->sr);
-    s->sr = 1;
+    if (s->cr & CR_EN) {
+        /* Set the interrupt status flag to signaled. */
+        DPRINTF("sr was %d\n", s->sr);
+        s->sr = 1;
 
-    /*
-     * An actual interrupt is generated only if the peripheral is enabled
-     * and the interrupt generation is enabled.
-     */
-    if ((s->cr & (CR_EN | CR_OCIEN)) == (CR_EN | CR_OCIEN)) {
-        qemu_irq_raise(s->irq);
+        /* If CR,OCIEN is set, an actual interrupt is generated */
+        if (s->cr & CR_OCIEN) {
+            qemu_irq_raise(s->irq);
+        }
+    } else {
+        /*
+         * The cmp ptimer is not supposed to be running when the
+         * peripheral is not enabled. Ignore this. However, it's
+         * worth investigating why this happened.
+         */
+        DPRINTF("compare trigger when timer not enabled\n");
     }
 }
 
-- 
2.34.5

Re: [PATCH qemu.git v2 9/9] hw/timer/imx_epit: fix compare timer handling

[Peter Maydell]

On Mon, 7 Nov 2022 at 16:42, ~axelheider <axelheider@git.sr.ht> wrote:
>
> From: Axel Heider <axel.heider@hensoldt.net>
>
> - 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 <axel.heider@hensoldt.net>
> ---
>  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 <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.
> @@ -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
> +     * updated even if no interrupt in generated.

"is 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.
> +     */

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.

> +    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);
> +    }
> +
>  }

> @@ -274,16 +326,22 @@ static void imx_epit_cmp(void *opaque)
>  {
>      IMXEPITState *s = IMX_EPIT(opaque);
>
> -    /* Set the interrupt status flag to signaled. */
> -    DPRINTF("sr was %d\n", s->sr);
> -    s->sr = 1;
> +    if (s->cr & CR_EN) {
> +        /* Set the interrupt status flag to signaled. */
> +        DPRINTF("sr was %d\n", s->sr);
> +        s->sr = 1;
>
> -    /*
> -     * An actual interrupt is generated only if the peripheral is enabled
> -     * and the interrupt generation is enabled.
> -     */
> -    if ((s->cr & (CR_EN | CR_OCIEN)) == (CR_EN | CR_OCIEN)) {
> -        qemu_irq_raise(s->irq);
> +        /* If CR,OCIEN is set, an actual interrupt is generated */
> +        if (s->cr & CR_OCIEN) {
> +            qemu_irq_raise(s->irq);
> +        }
> +    } else {
> +        /*
> +         * The cmp ptimer is not supposed to be running when the
> +         * peripheral is not enabled. Ignore this. However, it's
> +         * worth investigating why this happened.
> +         */
> +        DPRINTF("compare trigger when timer not enabled\n");

Is this a "can't happen, it would be a bug in this code"? If so,
use assert(). If it's a "guest code can program the timer in
silly ways" situation then either do what the hardware does
or (if it's not clear what that is) do something plausible.
You can use qemu_log_mask(LOG_GUEST_ERROR, ...) to log things
which are "guest has done something silly" if you like.

More generally, please don't introduce new uses of the DPRINTF
macro. For cases which are "this can be useful to the user to
log for debugging either the driver or their guest code" we
have a trace-events facility, where you put a line into
hw/timer/trace-events that specifies the prototype and format
string for the trace event, and then call a corresponding
trace_whatever() function in the code. Some of the other timer
devices do this, if you want to look at how it works.
Older device models like this one still use debug-print macros,
but they're not good practice in new code.

thanks
-- PMM

Re: [PATCH qemu.git v2 9/9] hw/timer/imx_epit: fix compare timer handling

[Axel Heider]

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

[PATCH qemu.git v2 8/9] hw/timer/imx_epit: change reset handling

[~axelheider]

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

- inline software reset
- make hardware reset invoke software reset
- simplify code flow

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

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index 30280a9ac1..77bd2b0a2b 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -73,35 +73,6 @@ static uint32_t imx_epit_get_freq(IMXEPITState *s)
     return freq;
 }
 
-static void imx_epit_reset(DeviceState *dev)
-{
-    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);
-    s->sr = 0;
-    s->lr = EPIT_TIMER_MAX;
-    s->cmp = 0;
-    /* clear the interrupt */
-    qemu_irq_lower(s->irq);
-
-    ptimer_transaction_begin(s->timer_cmp);
-    ptimer_transaction_begin(s->timer_reload);
-
-    /*
-     * The reset switches off the input clock, so even if the CR.EN is still
-     * set, the timers are no longer running.
-     */
-    assert(0 == imx_epit_get_freq(s));
-    ptimer_stop(s->timer_cmp);
-    ptimer_stop(s->timer_reload);
-    /* 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);
-    ptimer_transaction_commit(s->timer_cmp);
-    ptimer_transaction_commit(s->timer_reload);
-}
-
 static uint64_t imx_epit_read(void *opaque, hwaddr offset, unsigned size)
 {
     IMXEPITState *s = IMX_EPIT(opaque);
@@ -164,23 +135,23 @@ static void imx_epit_write_cr(IMXEPITState *s, uint32_t value)
     /* SWR bit is never persisted, it clears itself once reset is done */
     s->cr = (value & ~CR_SWR) & 0x03ffffff;
 
-    if (value & 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.
-         */
-    }
-
     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 (!(value & CR_SWR)) {
+    if (value & CR_SWR) {
+        /* Soft reset doesn't touch some bits; only a 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;
+        s->cmp = 0;
+        /* reset is supposed to disable the input clock */
+        assert(0 == imx_epit_get_freq(s));
+        /* turn interrupt off since SR and the OCIEN bit got cleared */
+        qemu_irq_lower(s->irq);
+        /* reset timer limits, set timer values to these limits */
+        ptimer_set_limit(s->timer_cmp, EPIT_TIMER_MAX, 1);
+        ptimer_set_limit(s->timer_reload, EPIT_TIMER_MAX, 1);
+    } else {
         freq = imx_epit_get_freq(s);
         if (freq) {
             ptimer_set_freq(s->timer_reload, freq);
@@ -369,6 +340,15 @@ 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_reset(DeviceState *dev)
+{
+    IMXEPITState *s = IMX_EPIT(dev);
+
+    /* initialize CR and perform a software reset */
+    s->cr = 0;
+    imx_epit_write_cr(s, CR_SWR);
+}
+
 static void imx_epit_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc  = DEVICE_CLASS(klass);
-- 
2.34.5

Re: [PATCH qemu.git v2 8/9] hw/timer/imx_epit: change reset handling

[Peter Maydell]

On Mon, 7 Nov 2022 at 16:42, ~axelheider <axelheider@git.sr.ht> wrote:
>
> From: Axel Heider <axel.heider@hensoldt.net>
>
> - inline software reset
> - make hardware reset invoke software reset
> - simplify code flow

I think this patch is fixing a bug, right? We weren't
previously clearing CR for the hardware reset. If so,
that's worth noting in the commit message.

> Signed-off-by: Axel Heider <axel.heider@hensoldt.net>


> +static void imx_epit_reset(DeviceState *dev)
> +{
> +    IMXEPITState *s = IMX_EPIT(dev);
> +
> +    /* initialize CR and perform a software reset */
> +    s->cr = 0;
> +    imx_epit_write_cr(s, CR_SWR);
> +}

Generally we prefer not to do this for the hardware reset
function, as it makes it harder to see what the reset
is doing (eg to confirm that it isn't changing qemu IRQ
state). You can have a common helper function to do the
work of the reset though if that helps.

thanks
-- PMM