Re: [Qemu-devel] [PATCH] i.MX: implement a more correct version of EPIT timer.

[Peter Chubb <peter.chubb@nicta.com.au>]

> This patch is providing a complete version of the EPIT timer.
> Note, however that the GPT timer in the same file is still not
> complete.

Thanks!

Comments in=line below.

> @@ -411,7 +441,7 @@ static int imx_timerg_init(SysBusDevice *dev)
>  #define CR_SWR      (1 << 16)
>  #define CR_IOVW     (1 << 17)
>  #define CR_DBGEN    (1 << 18)
> -#define CR_EPIT     (1 << 19)
> +#define CR_WAITEN   (1 << 19)
>
In the docs this bit is called EPIT.  Its function is to enable the
timer in wait-mode. So is conformance with the docs or the description
of the function more important?  If you *do* rename it, there should
be a comment to say it's EPIT in the i.mx31 documentation.
 
 
>-static void set_timerp_freq(IMXTimerPState *s)
>+static void imx_reload_compare_timer(IMXTimerPState *s)
> {
>-    int clksrc;
>-    unsigned prescaler;
>-    uint32_t freq;
>-
>-    clksrc = (s->cr & CR_CLKSRC_MASK) >> CR_CLKSRC_SHIFT;
>-    prescaler = 1 + ((s->cr >> CR_PRESCALE_SHIFT) & CR_PRESCALE_MASK);
>-    freq = imx_clock_frequency(s->ccm, imx_timerp_clocks[clksrc]) / prescaler;
>-
>-    s->freq = freq;
>-    DPRINTF("Setting ptimer frequency to %u\n", freq);
>-
>-    if (freq) {
>-        ptimer_set_freq(s->timer, freq);
>+    if ((s->cr & CR_OCIEN) && s->cmp) {
>+        /* it the compare feature is on */]

s/it/if/

+        uint32_t tmp = imx_timerp_update_counts(s);
+        if (tmp > s->cmp) {
+            /* reinit the cmp timer if required */
+            ptimer_set_count(s->timer_cmp, tmp - s->cmp);
+            if ((s->cr & CR_EN)) {
+                /* Restart the cmp timer if required */
+                ptimer_run(s->timer_cmp, 0);
+            }
+        }
     }
 }
 
>@@ -526,40 +599,63 @@ static void imx_timerp_write(void *opaque, hwaddr offset,
> 
>     switch (offset >> 2) {
>     case 0: /* CR */
>-        if (value & CR_SWR) {
>+        s->cr = value & 0x03ffffff;
>+        if (s->cr & CR_SWR) {
>+            /* handle the reset */
>             imx_timerp_reset(&s->busdev.qdev);
>-            value &= ~CR_SWR;
>+        } else {
>+            set_timerp_freq(s);
>         }
>-        s->cr = value & 0x03ffffff;
>-        set_timerp_freq(s);
>+        value &= s->cr;

You're letting the reset function clear the SWR.  It's unclear to me
from the docs whether when the SWR bit is set, assignment to other
fields in the CR happens before or after the reset.  I punted on
`after' (because it seemted to work), this changes to `before'.

The reset looks good --- you seem to understand the ptimer interface
better than I did when I updated this code.

Peter c
--
Dr Peter Chubb				        peter.chubb AT nicta.com.au
http://www.ssrg.nicta.com.au          Software Systems Research Group/NICTA