Patchwork [v2] i.MX: implement a more correct version of EPIT timer.

login
register
mail settings
Submitter Jean-Christophe DUBOIS
Date April 10, 2013, 8:02 p.m.
Message ID <1365624169-15451-1-git-send-email-jcd@tribudubois.net>
Download mbox | patch
Permalink /patch/235463/
State New
Headers show

Comments

Jean-Christophe DUBOIS - April 10, 2013, 8:02 p.m.
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.

Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>

Change from v1:
  - bump up the version number on VMSTATE struct
  - fix comment
---
 hw/timer/imx_timer.c |  253 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 193 insertions(+), 60 deletions(-)
Peter Maydell - April 10, 2013, 8:07 p.m.
On 10 April 2013 21:02, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
>  static const VMStateDescription vmstate_imx_timerp = {
>      .name = "imx-timerp",
> -    .version_id = 1,
> +    .version_id = 2,
>      .minimum_version_id = 1,
>      .minimum_version_id_old = 1,

I said "all the version id fields" but you've only changed one.
(I'm assuming you don't want to try to support old-to-new
migration, which is the case where the minimum and current IDs
differ; in that case you need to annotate fields with the
version they appear in and you can't delete or rename them.)

thanks
-- PMM
Jean-Christophe DUBOIS - April 10, 2013, 8:19 p.m.
On 04/10/2013 10:07 PM, Peter Maydell wrote:
> On 10 April 2013 21:02, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
>>   static const VMStateDescription vmstate_imx_timerp = {
>>       .name = "imx-timerp",
>> -    .version_id = 1,
>> +    .version_id = 2,
>>       .minimum_version_id = 1,
>>       .minimum_version_id_old = 1,
> I said "all the version id fields" but you've only changed one.
> (I'm assuming you don't want to try to support old-to-new
> migration, which is the case where the minimum and current IDs
> differ; in that case you need to annotate fields with the
> version they appear in and you can't delete or rename them.)
To be honest with you Peter, I don't even know how these state 
information are used.

I never practiced VM migration with Qemu for now.

Actually if you had a few pointers to a decent explanation, I would be 
interested.

JC
>
> thanks
> -- PMM
>
Peter Maydell - April 10, 2013, 8:25 p.m.
On 10 April 2013 21:19, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
> To be honest with you Peter, I don't even know how these state information
> are used.
>
> I never practiced VM migration with Qemu for now.
>
> Actually if you had a few pointers to a decent explanation, I would be
> interested.

There's docs/migration.txt, though I agree that we could do better
on the documentation front (some of the details are a bit subtle.

Migration proper is only really useful for KVM I think; save/restore
support (ie VM snapshots) has wider application -- I've used it for
TCG debugging sessions a few times (snapshot the system just before
it gets to the bit that falls over so you don't have to wait 5 minutes
and a lot of runtime logging to retry your failing test case).

-- PMM
Peter Chubb - April 28, 2013, 9:29 p.m.
>>>>> "Jean-Christophe" == Jean-Christophe DUBOIS <jcd@tribudubois.net> writes:

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

Looks good.  You can add my

Reviewed-by: Peter Chubb <peter.chubb@nicta.com.au>

line.


Peter C
--
Dr Peter Chubb				        peter.chubb AT nicta.com.au
http://www.ssrg.nicta.com.au          Software Systems Research Group/NICTA
Jean-Christophe DUBOIS - April 29, 2013, 4:39 p.m.
On 04/28/2013 11:29 PM, Peter Chubb wrote:
>>>>>> "Jean-Christophe" == Jean-Christophe DUBOIS <jcd@tribudubois.net> writes:
> Jean-Christophe> This patch is providing a complete version of the
> Jean-Christophe> EPIT timer.  Note, however that the GPT timer in the
> Jean-Christophe> same file is still not complete.
>
> Looks good.  You can add my
>
> Reviewed-by: Peter Chubb <peter.chubb@nicta.com.au>
>
> line.
Peter (Maydel), will you pick it up as is (actually the v3  version) or 
do I need to send a new version of the patch (a v4) ?

JC
>
> Peter C
> --
> Dr Peter Chubb				        peter.chubb AT nicta.com.au
> http://www.ssrg.nicta.com.au          Software Systems Research Group/NICTA
>
Peter Maydell - April 29, 2013, 4:49 p.m.
On 29 April 2013 17:39, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
> Peter (Maydel), will you pick it up as is (actually the v3  version) or do I
> need to send a new version of the patch (a v4) ?

The last on-list version still has the wrong vmstate version
fields, I think?

thanks
-- PMM
Jean-Christophe DUBOIS - April 29, 2013, 7:39 p.m.
On 04/29/2013 06:49 PM, Peter Maydell wrote:
> On 29 April 2013 17:39, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
>> Peter (Maydel), will you pick it up as is (actually the v3  version) or do I
>> need to send a new version of the patch (a v4) ?
> The last on-list version still has the wrong vmstate version
> fields, I think?

The last on list (v3) has all VMSTATE version numbers set to 2 (as 
opposed to 1 previously).

Am I missing something?

JC

>
> thanks
> -- PMM
>
David Croswell - April 30, 2013, 12:44 a.m.
Can you remove 'philipo@ok-labs.com<mailto:philipo@ok-labs.com>' from this mailing list / thread, as he no longer reads this email (and it's forward to me).

Thanks,
David
---
David Croswell | IT Manager
General Dynamics Cyber Solutions | Open Kernel Labs
t +61 2 8003 9942 | m +61 466 719 764
e davidc@ok-labs.com<mailto:davidc@ok-labs.com>

On 30-Apr-2013, at 5:39 AM, Jean-Christophe DUBOIS <jcd@tribudubois.net<mailto:jcd@tribudubois.net>> wrote:

On 04/29/2013 06:49 PM, Peter Maydell wrote:
On 29 April 2013 17:39, Jean-Christophe DUBOIS <jcd@tribudubois.net<mailto:jcd@tribudubois.net>> wrote:
Peter (Maydel), will you pick it up as is (actually the v3  version) or do I
need to send a new version of the patch (a v4) ?
The last on-list version still has the wrong vmstate version
fields, I think?

The last on list (v3) has all VMSTATE version numbers set to 2 (as
opposed to 1 previously).

Am I missing something?

JC


thanks
-- PMM

Patch

diff --git a/hw/timer/imx_timer.c b/hw/timer/imx_timer.c
index 03197e3..514a907 100644
--- a/hw/timer/imx_timer.c
+++ b/hw/timer/imx_timer.c
@@ -95,6 +95,10 @@  typedef struct {
     uint32_t sr;
     uint32_t ir;
     uint32_t ocr1;
+    uint32_t ocr2;
+    uint32_t ocr3;
+    uint32_t icr1;
+    uint32_t icr2;
     uint32_t cnt;
 
     uint32_t waiting_rov;
@@ -103,7 +107,7 @@  typedef struct {
 
 static const VMStateDescription vmstate_imx_timerg = {
     .name = "imx-timerg",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .fields      = (VMStateField[]) {
@@ -112,6 +116,10 @@  static const VMStateDescription vmstate_imx_timerg = {
         VMSTATE_UINT32(sr, IMXTimerGState),
         VMSTATE_UINT32(ir, IMXTimerGState),
         VMSTATE_UINT32(ocr1, IMXTimerGState),
+        VMSTATE_UINT32(ocr2, IMXTimerGState),
+        VMSTATE_UINT32(ocr3, IMXTimerGState),
+        VMSTATE_UINT32(icr1, IMXTimerGState),
+        VMSTATE_UINT32(icr2, IMXTimerGState),
         VMSTATE_UINT32(cnt, IMXTimerGState),
         VMSTATE_UINT32(waiting_rov, IMXTimerGState),
         VMSTATE_PTIMER(timer, IMXTimerGState),
@@ -156,7 +164,6 @@  static void imx_timerg_update(IMXTimerGState *s)
             s->ir & GPT_SR_ROV ? "ROV" : "",
             s->cr & GPT_CR_EN ? "CR_EN" : "Not Enabled");
 
-
     qemu_set_irq(s->irq, (s->cr & GPT_CR_EN) && flags);
 }
 
@@ -221,6 +228,21 @@  static uint64_t imx_timerg_read(void *opaque, hwaddr offset,
         DPRINTF(" ocr1 = %x\n", s->ocr1);
         return s->ocr1;
 
+    case 5: /* Output Compare Register 2 */
+        DPRINTF(" ocr2 = %x\n", s->ocr2);
+        return s->ocr2;
+
+    case 6: /* Output Compare Register 3 */
+        DPRINTF(" ocr3 = %x\n", s->ocr3);
+        return s->ocr3;
+
+    case 7: /* input Capture Register 1 */
+        DPRINTF(" icr1 = %x\n", s->icr1);
+        return s->icr1;
+
+    case 8: /* input Capture Register 2 */
+        DPRINTF(" icr2 = %x\n", s->icr2);
+        return s->icr2;
 
     case 9: /* cnt */
         imx_timerg_update_counts(s);
@@ -230,6 +252,7 @@  static uint64_t imx_timerg_read(void *opaque, hwaddr offset,
 
     IPRINTF("imx_timerg_read: Bad offset %x\n",
             (int)offset >> 2);
+
     return 0;
 }
 
@@ -240,14 +263,19 @@  static void imx_timerg_reset(DeviceState *dev)
     /*
      * Soft reset doesn't touch some bits; hard reset clears them
      */
-    s->cr &= ~(GPT_CR_EN|GPT_CR_DOZEN|GPT_CR_WAITEN|GPT_CR_DBGEN);
+    s->cr &= ~(GPT_CR_EN|GPT_CR_ENMOD|GPT_CR_STOPEN|GPT_CR_DOZEN|GPT_CR_WAITEN|GPT_CR_DBGEN);
     s->sr = 0;
     s->pr = 0;
     s->ir = 0;
     s->cnt = 0;
     s->ocr1 = TIMER_MAX;
+    s->ocr2 = TIMER_MAX;
+    s->ocr3 = TIMER_MAX;
+    s->icr1 = 0;
+    s->icr2 = 0;
     ptimer_stop(s->timer);
     ptimer_set_limit(s->timer, TIMER_MAX, 1);
+    ptimer_set_count(s->timer, TIMER_MAX);
     imx_timerg_set_freq(s);
 }
 
@@ -323,6 +351,8 @@  static void imx_timerg_write(void *opaque, hwaddr offset,
         s->ocr1 = value;
         return;
 
+    case 5: /* OCR2 -- output compare register */
+    case 6: /* OCR3 -- output compare register */
     default:
         IPRINTF("imx_timerg_write: Bad offset %x\n",
                 (int)offset >> 2);
@@ -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)
 #define CR_DOZEN    (1 << 20)
 #define CR_STOPEN   (1 << 21)
 #define CR_CLKSRC_SHIFT (24)
@@ -423,24 +453,26 @@  static int imx_timerg_init(SysBusDevice *dev)
  * These are typical.
  */
 static const IMXClk imx_timerp_clocks[] =  {
-    0,        /* disabled */
-    IPG, /* ipg_clk, ~532MHz */
-    IPG, /* ipg_clk_highfreq */
-    CLK_32k,    /* ipg_clk_32k -- ~32kHz */
+    0,        /* 00 disabled */
+    IPG,      /* 01 ipg_clk, ~532MHz */
+    IPG,      /* 10 ipg_clk_highfreq */
+    CLK_32k,  /* 11 ipg_clk_32k -- ~32kHz */
 };
 
 typedef struct {
     SysBusDevice busdev;
-    ptimer_state *timer;
+    ptimer_state *timer_reload;
+    ptimer_state *timer_cmp;
     MemoryRegion iomem;
     DeviceState *ccm;
 
     uint32_t cr;
+    uint32_t sr;
     uint32_t lr;
     uint32_t cmp;
+    uint32_t cnt;
 
     uint32_t freq;
-    int int_level;
     qemu_irq irq;
 } IMXTimerPState;
 
@@ -449,23 +481,63 @@  typedef struct {
  */
 static void imx_timerp_update(IMXTimerPState *s)
 {
-    if (s->int_level && (s->cr & CR_OCIEN)) {
+    if (s->sr && (s->cr & CR_OCIEN)) {
         qemu_irq_raise(s->irq);
     } else {
         qemu_irq_lower(s->irq);
     }
 }
 
+static void set_timerp_freq(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_reload, freq);
+        ptimer_set_freq(s->timer_cmp, freq);
+    }
+}
+
 static void imx_timerp_reset(DeviceState *dev)
 {
     IMXTimerPState *s = container_of(dev, IMXTimerPState, busdev.qdev);
 
-    s->cr = 0;
+    /*
+     * 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 = TIMER_MAX;
-    s->int_level = 0;
     s->cmp = 0;
-    ptimer_stop(s->timer);
-    ptimer_set_count(s->timer, TIMER_MAX);
+    s->cnt = 0;
+    /* stop both timers */
+    ptimer_stop(s->timer_cmp);
+    ptimer_stop(s->timer_reload);
+    /* compute new frequency */
+    set_timerp_freq(s);
+    /* init both timers to TIMER_MAX */
+    ptimer_set_limit(s->timer_cmp, TIMER_MAX, 1);
+    ptimer_set_limit(s->timer_reload, TIMER_MAX, 1);
+    if (s->freq && (s->cr & CR_EN)) {
+        /* if the timer is still enabled, restart it */
+        ptimer_run(s->timer_reload, 1);
+    }
+}
+
+static uint32_t imx_timerp_update_counts(IMXTimerPState *s)
+{
+     s->cnt = ptimer_get_count(s->timer_reload);
+
+     return s->cnt;
 }
 
 static uint64_t imx_timerp_read(void *opaque, hwaddr offset,
@@ -480,8 +552,8 @@  static uint64_t imx_timerp_read(void *opaque, hwaddr offset,
         return s->cr;
 
     case 1: /* Status Register */
-        DPRINTF("int_level %x\n", s->int_level);
-        return s->int_level;
+        DPRINTF("sr %x\n", s->sr);
+        return s->sr;
 
     case 2: /* LR - ticks*/
         DPRINTF("lr %x\n", s->lr);
@@ -492,28 +564,29 @@  static uint64_t imx_timerp_read(void *opaque, hwaddr offset,
         return s->cmp;
 
     case 4: /* CNT */
-        return ptimer_get_count(s->timer);
+        imx_timerp_update_counts(s);
+        DPRINTF(" cnt = %x\n", s->cnt);
+        return s->cnt;
     }
+
     IPRINTF("imx_timerp_read: Bad offset %x\n",
             (int)offset >> 2);
     return 0;
 }
 
-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) {
+        /* if the compare feature is on */
+        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,62 @@  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);
 
         if (s->freq && (s->cr & CR_EN)) {
-            if (!(s->cr & CR_ENMOD)) {
-                ptimer_set_count(s->timer, s->lr);
+            if (s->cr & CR_ENMOD) {
+                if (s->cr & CR_RLD) {
+                    ptimer_set_limit(s->timer_reload, s->lr, 1);
+                } else {
+                    ptimer_set_limit(s->timer_reload, TIMER_MAX, 1);
+                }
             }
-            ptimer_run(s->timer, 0);
+
+            imx_reload_compare_timer(s);
+
+            ptimer_run(s->timer_reload, 1);
         } else {
-            ptimer_stop(s->timer);
+            /* stop both timers */
+            ptimer_stop(s->timer_reload);
+            ptimer_stop(s->timer_cmp);
         }
         break;
 
     case 1: /* SR - ACK*/
-        s->int_level = 0;
-        imx_timerp_update(s);
+        /* writing 1 to OCIF clear the OCIF bit */
+        if (value & 0x01) {
+            s->sr = 0;
+            imx_timerp_update(s);
+        }
         break;
 
     case 2: /* LR - set ticks */
         s->lr = value;
-        ptimer_set_limit(s->timer, value, !!(s->cr & CR_IOVW));
+
+        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);
+        } else if (s->cr & CR_IOVW) {
+            /* If IOVW bit is set then set the timer value */
+            ptimer_set_count(s->timer_reload, s->lr);
+        }
+
+        imx_reload_compare_timer(s);
+
         break;
 
     case 3: /* CMP */
         s->cmp = value;
-        if (value) {
-            IPRINTF(
-                "Values for EPIT comparison other than zero not supported\n"
-            );
-        }
+
+        imx_reload_compare_timer(s);
+
         break;
 
     default:
@@ -568,16 +663,50 @@  static void imx_timerp_write(void *opaque, hwaddr offset,
     }
 }
 
-static void imx_timerp_tick(void *opaque)
+static void imx_timerp_reload(void *opaque)
 {
     IMXTimerPState *s = (IMXTimerPState *)opaque;
 
-   DPRINTF("imxp tick\n");
-    if (!(s->cr & CR_RLD)) {
-        ptimer_set_count(s->timer, TIMER_MAX);
+    DPRINTF("imxp reload\n");
+
+    if (!(s->cr & CR_EN)) {
+        return;
+    }
+
+    if (s->cr & CR_RLD) {
+        ptimer_set_limit(s->timer_reload, s->lr, 1);
+    } else {
+        ptimer_set_limit(s->timer_reload, TIMER_MAX, 1);
+    }
+
+    if (s->cr & CR_OCIEN) {
+        /* if compare register is 0 then we handle the interrupt here */
+        if (s->cmp == 0) {
+            s->sr = 1;
+            imx_timerp_update(s);
+        } else if (s->cmp <= s->lr) {
+            /* We should launch the compare register */
+            ptimer_set_count(s->timer_cmp, s->lr - s->cmp);
+            ptimer_run(s->timer_cmp, 0);
+        } else {
+            IPRINTF("imxp reload: s->lr < s->cmp\n");
+        }
+    }
+}
+
+static void imx_timerp_cmp(void *opaque)
+{
+    IMXTimerPState *s = (IMXTimerPState *)opaque;
+
+    DPRINTF("imxp compare\n");
+
+    ptimer_stop(s->timer_cmp);
+
+    /* compare register is not 0 */
+    if (s->cmp) {
+        s->sr = 1;
+        imx_timerp_update(s);
     }
-    s->int_level = 1;
-    imx_timerp_update(s);
 }
 
 void imx_timerp_create(const hwaddr addr,
@@ -600,16 +729,18 @@  static const MemoryRegionOps imx_timerp_ops = {
 
 static const VMStateDescription vmstate_imx_timerp = {
     .name = "imx-timerp",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .fields      = (VMStateField[]) {
         VMSTATE_UINT32(cr, IMXTimerPState),
+        VMSTATE_UINT32(sr, IMXTimerPState),
         VMSTATE_UINT32(lr, IMXTimerPState),
         VMSTATE_UINT32(cmp, IMXTimerPState),
+        VMSTATE_UINT32(cnt, IMXTimerPState),
         VMSTATE_UINT32(freq, IMXTimerPState),
-        VMSTATE_INT32(int_level, IMXTimerPState),
-        VMSTATE_PTIMER(timer, IMXTimerPState),
+        VMSTATE_PTIMER(timer_reload, IMXTimerPState),
+        VMSTATE_PTIMER(timer_cmp, IMXTimerPState),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -620,15 +751,17 @@  static int imx_timerp_init(SysBusDevice *dev)
     QEMUBH *bh;
 
     DPRINTF("imx_timerp_init\n");
-
     sysbus_init_irq(dev, &s->irq);
     memory_region_init_io(&s->iomem, &imx_timerp_ops,
                           s, "imxp-timer",
                           0x00001000);
     sysbus_init_mmio(dev, &s->iomem);
 
-    bh = qemu_bh_new(imx_timerp_tick, s);
-    s->timer = ptimer_init(bh);
+    bh = qemu_bh_new(imx_timerp_reload, s);
+    s->timer_reload = ptimer_init(bh);
+
+    bh = qemu_bh_new(imx_timerp_cmp, s);
+    s->timer_cmp = ptimer_init(bh);
 
     return 0;
 }