diff mbox series

[qemu.git,11/11] hw/timer/imx_epit: rework CR write handling

Message ID 166718254546.5893.5075929684621857903-11@git.sr.ht
State New
Headers show
Series improve hw/timer/imx_epit | expand

Commit Message

~axelheider Oct. 31, 2022, 12:28 a.m. UTC
From: Axel Heider <axel.heider@hensoldt.net>

- simplify code, improve comments
- fix https://gitlab.com/qemu-project/qemu/-/issues/1263

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

Comments

Philippe Mathieu-Daudé Oct. 31, 2022, 8:25 a.m. UTC | #1
On 31/10/22 01:28, ~axelheider wrote:
> From: Axel Heider <axel.heider@hensoldt.net>
> 
> - simplify code, improve comments
> - fix https://gitlab.com/qemu-project/qemu/-/issues/1263

This doesn't match GitLab issues closing pattern:
https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#default-closing-pattern

> 
> Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
> ---
>   hw/timer/imx_epit.c | 139 +++++++++++++++++++++-----------------------
>   1 file changed, 65 insertions(+), 74 deletions(-)
Axel Heider Nov. 1, 2022, 9:19 p.m. UTC | #2
-------- Original Message --------
> From: Philippe Mathieu-Daudé [mailto:philmd@linaro.org]
>
>> - simplify code, improve comments
>> - fix https://gitlab.com/qemu-project/qemu/-/issues/1263
>
> This doesn't match GitLab issues closing pattern:
> https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#default-closing-pattern

I will change this to use "fix #1263" then. But the issue git closed already
from an earlier patch that got merged a few days ago.

Axel
diff mbox series

Patch

diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index 7dd9f54c9c..4a6326b1de 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -69,43 +69,6 @@  static uint32_t imx_epit_get_freq(IMXEPITState *s)
     return f_in / prescaler;
 }
 
-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);
-    /* stop both timers */
-    ptimer_stop(s->timer_cmp);
-    ptimer_stop(s->timer_reload);
-    /* compute new frequency */
-    uint32_t freq = imx_epit_get_freq(s);
-    DPRINTF("Setting ptimer frequency to %u\n", freq);
-    if (freq) {
-        ptimer_set_freq(s->timer_reload, freq);
-        ptimer_set_freq(s->timer_cmp, freq);
-    }
-    /* 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 (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 uint64_t imx_epit_read(void *opaque, hwaddr offset, unsigned size)
 {
     IMXEPITState *s = IMX_EPIT(opaque);
@@ -192,56 +155,75 @@  static void imx_epit_update_compare_timer(IMXEPITState *s)
 
 static void imx_epit_write_cr(IMXEPITState *s, uint32_t value)
 {
-    uint32_t freq = 0;
+    ptimer_transaction_begin(s->timer_cmp);
+    ptimer_transaction_begin(s->timer_reload);
 
-    /* SWR bit is never persisted, it clears itself once reset is done */
     uint32_t oldcr = s->cr;
     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.
+         * Soft reset doesn't touch some bits, just a hard reset clears all
+         * of them. Clearing CLKSRC disables the input clock, which will
+         * happen when we re-init of the timer frequency below.
+         */
+        s->cr &= (CR_EN|CR_ENMOD|CR_STOPEN|CR_DOZEN|CR_WAITEN|CR_DBGEN);
+        /*
+         * We have applied the new CR value and then cleared most bits,
+         * thus some bits from the write request are now lost. The TRM
+         * is not clear about the behavior, maybe these bits are to be
+         * applied after the reset (e.g. for selecting a new clock
+         * source). However, it seem this is undefined behavior and a
+         * it's assumed a reset does not try to do anything else.
          */
+        s->sr = 0;
+        s->lr = EPIT_TIMER_MAX;
+        s->cmp = 0;
+        /* turn interrupt off since SR and the OCIEN bit is cleared */
+        qemu_irq_lower(s->irq);
+        /* reset timer limits, set timer values to the limits */
+        ptimer_set_limit(s->timer_cmp, EPIT_TIMER_MAX, 1);
+        ptimer_set_limit(s->timer_reload, EPIT_TIMER_MAX, 1);
     }
 
-    ptimer_transaction_begin(s->timer_cmp);
-    ptimer_transaction_begin(s->timer_reload);
-
-    if (!(value & CR_SWR)) {
-        uint32_t freq = imx_epit_get_freq(s);
+    /* re-initialize frequency, or turn off timers if input clock is off */
+    uint32_t freq = imx_epit_get_freq(s);
+    if (freq) {
         DPRINTF("Setting ptimer frequency to %u\n", freq);
-        if (freq) {
-            ptimer_set_freq(s->timer_reload, freq);
-            ptimer_set_freq(s->timer_cmp, 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) {
-            uint64_t limit = (s->cr & CR_RLD) ? s->lr : EPIT_TIMER_MAX;
-            /* set new limit and also set timer to this value right now */
-            ptimer_set_limit(s->timer_reload, limit, 1);
-            ptimer_set_limit(s->timer_cmp, limit, 1);
-        }
-        ptimer_run(s->timer_reload, 0);
-        imx_epit_update_compare_timer(s);
-    } else if (!(s->cr & CR_EN)) {
-        /* stop both timers */
-        ptimer_stop(s->timer_reload);
+    if (!freq || !(s->cr & CR_EN)) {
+        /*
+         * The EPIT timer is effectively disabled if it is not enabled or
+         * the input clock is off. In this case we can stop the ptimers.
+         */
         ptimer_stop(s->timer_cmp);
-    } else if (s->cr & CR_OCIEN) {
-        if (!(oldcr & CR_OCIEN)) {
-            imx_epit_update_compare_timer(s);
-        }
+        ptimer_stop(s->timer_reload);
     } else {
-        ptimer_stop(s->timer_cmp);
+        /* The EPIT timer is active. */
+        if (!(oldcr & CR_EN)) {
+            /* The EPI timer has just been enabled, initialize and start it. */
+            if (s->cr & CR_ENMOD) {
+                uint64_t limit = (s->cr & CR_RLD) ? s->lr : EPIT_TIMER_MAX;
+                /* set new limit and also set timer to this value right now */
+                ptimer_set_limit(s->timer_reload, limit, 1);
+                ptimer_set_limit(s->timer_cmp, limit, 1);
+            }
+            ptimer_run(s->timer_reload, 0);
+        }
     }
+    /*
+     * Commit the change to s->timer_reload, so it can propagate and the
+     * updated value will be read in imx_epit_update_compare_timer(),
+     * Otherwise a stale value will be seen and the compare interrupt is
+     * set up wrongly.
+     */
+    ptimer_transaction_commit(s->timer_reload);
+    imx_epit_update_compare_timer(s);
 
     ptimer_transaction_commit(s->timer_cmp);
-    ptimer_transaction_commit(s->timer_reload);
 }
 
 static void imx_epit_write_sr(IMXEPITState *s, uint32_t value)
@@ -269,10 +251,10 @@  static void imx_epit_write_lr(IMXEPITState *s, uint32_t 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_update_compare_timer(), which reads
-     * s->timer_reload internally again.
+     * Commit the change to s->timer_reload, so it can propagate and the
+     * updated value will be read in imx_epit_update_compare_timer(),
+     * Otherwise a stale value will be seen and the compare interrupt is
+     * set up wrongly.
      */
     ptimer_transaction_commit(s->timer_reload);
     imx_epit_update_compare_timer(s);
@@ -390,6 +372,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);