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

[Jean-Christophe DUBOIS]

On 04/10/2013 01:27 AM, Peter Chubb wrote:
> > 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.

Well, in the i.MX25 or the i.MX6 manual, this bit is named WAITEN (like 
in the GPT).

I believe it is a typo in the i.MX31 documentation (where it is referred 
as WAITEN in the SWR bit description).

>   
>   
> > -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/
right
> +        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'.

Well the documentation states that the SWR bit is reset to 0 at the end 
of reset. So this should be the case even when we don't call it 
explicitly in the driver (when it is called directly by Qemu).

> 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