[Qemu-devel] [PULL 0/8] arm-devs queue

[Qemu-devel] [PULL 0/8] arm-devs queue

[Peter Maydell]

arm-devs pullreq; nothing particularly earthshattering.
As with the target-arm pullreq, I'm trying to create a signed
pullreq here, so let me know if I've messed it up.
The signed tag itself is 'pull-arm-devs-20130625'; arm-devs.for-upstream
is the branch name, as usual. 


The following changes since commit baf8673ca802cb3ea2cdbe94813441d23bde223b:

  Merge remote-tracking branch 'stefanha/block' into staging (2013-06-24 14:33:17 -0500)

are available in the git repository at:


  git://git.linaro.org/people/pmaydell/qemu-arm.git arm-devs.for-upstream

for you to fetch changes up to 7426aa72c36c908a7d0eae3e38568bb0a70de479:

  nand: Don't inherit from Sysbus (2013-06-25 19:15:46 +0100)

----------------------------------------------------------------
arm-devs queue

----------------------------------------------------------------
Jean-Christophe DUBOIS (2):
      i.MX: Implement a more complete version of the GPT timer.
      i.MX: Rework functions/types name and use new style initialization

John Rigby (1):
      ARM: Allow dumping of device tree

Peter Crosthwaite (3):
      block/nand: QOM casting sweep
      block/nand: Convert Sysbus::init to Device::realize
      nand: Don't inherit from Sysbus

Peter Maydell (1):
      arm/boot: Free dtb blob memory after use

Stefan Weil (1):
      i.MX31: Fix PRCS bit test

 hw/arm/boot.c      |   21 +-
 hw/block/nand.c    |   48 +++--
 hw/misc/imx_ccm.c  |    2 +-
 hw/timer/imx_gpt.c |  558 ++++++++++++++++++++++++++++++----------------------
 4 files changed, 369 insertions(+), 260 deletions(-)

[Qemu-devel] [PULL 1/8] ARM: Allow dumping of device tree

[Peter Maydell]

From: John Rigby <john.rigby@linaro.org>

By calling qemu_devtree_dumpdtb near the end of load_dtb.

Signed-off-by: John Rigby <john.rigby@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index defcf15..797c691 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -303,6 +303,7 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
             fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");
         }
     }
+    qemu_devtree_dumpdtb(fdt, size);
 
     cpu_physical_memory_write(addr, fdt, size);
 
-- 
1.7.9.5

[Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[Peter Maydell]

From: Jean-Christophe DUBOIS <jcd@tribudubois.net>

* implement compare 1 2 and 3 registers
* simplify Debug printf

Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
Message-id: 1369898943-1993-2-git-send-email-jcd@tribudubois.net
Reviewed-by: Peter Chubb <peter.chubb@nicta.com.au>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/imx_gpt.c |  443 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 269 insertions(+), 174 deletions(-)

diff --git a/hw/timer/imx_gpt.c b/hw/timer/imx_gpt.c
index d8c4f0b..ecf6e53 100644
--- a/hw/timer/imx_gpt.c
+++ b/hw/timer/imx_gpt.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2011 NICTA Pty Ltd
  * Originally written by Hans Jiang
  * Updated by Peter Chubb
+ * Updated by Jean-Christophe Dubois
  *
  * This code is licensed under GPL version 2 or later.  See
  * the COPYING file in the top-level directory.
@@ -18,10 +19,44 @@
 #include "hw/sysbus.h"
 #include "hw/arm/imx.h"
 
-//#define DEBUG_TIMER 1
-#ifdef DEBUG_TIMER
+#define TYPE_IMX_GPT "imx.gpt"
+
+/*
+ * Define to 1 for debug messages
+ */
+#define DEBUG_TIMER 0
+#if DEBUG_TIMER
+
+static char const *imx_timerg_reg_name(uint32_t reg)
+{
+    switch (reg) {
+    case 0:
+        return "CR";
+    case 1:
+        return "PR";
+    case 2:
+        return "SR";
+    case 3:
+        return "IR";
+    case 4:
+        return "OCR1";
+    case 5:
+        return "OCR2";
+    case 6:
+        return "OCR3";
+    case 7:
+        return "ICR1";
+    case 8:
+        return "ICR2";
+    case 9:
+        return "CNT";
+    default:
+        return "[?]";
+    }
+}
+
 #  define DPRINTF(fmt, args...) \
-      do { printf("imx_timer: " fmt , ##args); } while (0)
+          do { printf("%s: " fmt , __func__, ##args); } while (0)
 #else
 #  define DPRINTF(fmt, args...) do {} while (0)
 #endif
@@ -33,7 +68,7 @@
 #define DEBUG_IMPLEMENTATION 1
 #if DEBUG_IMPLEMENTATION
 #  define IPRINTF(fmt, args...)                                         \
-    do  { fprintf(stderr, "imx_timer: " fmt, ##args); } while (0)
+          do { fprintf(stderr, "%s: " fmt, __func__, ##args); } while (0)
 #else
 #  define IPRINTF(fmt, args...) do {} while (0)
 #endif
@@ -43,16 +78,7 @@
  *
  * This timer counts up continuously while it is enabled, resetting itself
  * to 0 when it reaches TIMER_MAX (in freerun mode) or when it
- * reaches the value of ocr1 (in periodic mode).  WE simulate this using a
- * QEMU ptimer counting down from ocr1 and reloading from ocr1 in
- * periodic mode, or counting from ocr1 to zero, then TIMER_MAX - ocr1.
- * waiting_rov is set when counting from TIMER_MAX.
- *
- * In the real hardware, there are three comparison registers that can
- * trigger interrupts, and compare channel 1 can be used to
- * force-reset the timer. However, this is a `bare-bones'
- * implementation: only what Linux 3.x uses has been implemented
- * (free-running timer from 0 to OCR1 or TIMER_MAX) .
+ * reaches the value of one of the ocrX (in periodic mode).
  */
 
 #define TIMER_MAX  0XFFFFFFFFUL
@@ -79,9 +105,13 @@
 #define GPT_CR_FO3    (1 << 31) /* Force Output Compare Channel 3 */
 
 #define GPT_SR_OF1  (1 << 0)
+#define GPT_SR_OF2  (1 << 1)
+#define GPT_SR_OF3  (1 << 2)
 #define GPT_SR_ROV  (1 << 5)
 
 #define GPT_IR_OF1IE  (1 << 0)
+#define GPT_IR_OF2IE  (1 << 1)
+#define GPT_IR_OF3IE  (1 << 2)
 #define GPT_IR_ROVIE  (1 << 5)
 
 typedef struct {
@@ -101,15 +131,19 @@ typedef struct {
     uint32_t icr2;
     uint32_t cnt;
 
-    uint32_t waiting_rov;
+    uint32_t next_timeout;
+    uint32_t next_int;
+
+    uint32_t freq;
+
     qemu_irq irq;
 } IMXTimerGState;
 
 static const VMStateDescription vmstate_imx_timerg = {
-    .name = "imx-timerg",
-    .version_id = 2,
-    .minimum_version_id = 2,
-    .minimum_version_id_old = 2,
+    .name = TYPE_IMX_GPT,
+    .version_id = 3,
+    .minimum_version_id = 3,
+    .minimum_version_id_old = 3,
     .fields      = (VMStateField[]) {
         VMSTATE_UINT32(cr, IMXTimerGState),
         VMSTATE_UINT32(pr, IMXTimerGState),
@@ -121,7 +155,9 @@ static const VMStateDescription vmstate_imx_timerg = {
         VMSTATE_UINT32(icr1, IMXTimerGState),
         VMSTATE_UINT32(icr2, IMXTimerGState),
         VMSTATE_UINT32(cnt, IMXTimerGState),
-        VMSTATE_UINT32(waiting_rov, IMXTimerGState),
+        VMSTATE_UINT32(next_timeout, IMXTimerGState),
+        VMSTATE_UINT32(next_int, IMXTimerGState),
+        VMSTATE_UINT32(freq, IMXTimerGState),
         VMSTATE_PTIMER(timer, IMXTimerGState),
         VMSTATE_END_OF_LIST()
     }
@@ -138,16 +174,14 @@ static const IMXClk imx_timerg_clocks[] = {
     NOCLK,    /* 111 not defined */
 };
 
-
 static void imx_timerg_set_freq(IMXTimerGState *s)
 {
-    int clksrc;
-    uint32_t freq;
-
-    clksrc = (s->cr >> GPT_CR_CLKSRC_SHIFT) & GPT_CR_CLKSRC_MASK;
-    freq = imx_clock_frequency(s->ccm, imx_timerg_clocks[clksrc]) / (1 + s->pr);
+    uint32_t clksrc = extract32(s->cr, GPT_CR_CLKSRC_SHIFT, 3);
+    uint32_t freq = imx_clock_frequency(s->ccm, imx_timerg_clocks[clksrc])
+                                                / (1 + s->pr);
+    s->freq = freq;
 
-    DPRINTF("Setting gtimer clksrc %d to frequency %d\n", clksrc, freq);
+    DPRINTF("Setting clksrc %d to frequency %d\n", clksrc, freq);
 
     if (freq) {
         ptimer_set_freq(s->timer, freq);
@@ -156,111 +190,176 @@ static void imx_timerg_set_freq(IMXTimerGState *s)
 
 static void imx_timerg_update(IMXTimerGState *s)
 {
-    uint32_t flags = s->sr & s->ir & (GPT_SR_OF1 | GPT_SR_ROV);
-
-    DPRINTF("g-timer SR: %s %s IR=%s %s, %s\n",
-            s->sr & GPT_SR_OF1 ? "OF1" : "",
-            s->sr & GPT_SR_ROV ? "ROV" : "",
-            s->ir & GPT_SR_OF1 ? "OF1" : "",
-            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);
+    if ((s->sr & s->ir) && (s->cr & GPT_CR_EN)) {
+        qemu_irq_raise(s->irq);
+    } else {
+        qemu_irq_lower(s->irq);
+    }
 }
 
 static uint32_t imx_timerg_update_counts(IMXTimerGState *s)
 {
-    uint64_t target = s->waiting_rov ? TIMER_MAX : s->ocr1;
-    uint64_t cnt = ptimer_get_count(s->timer);
-    s->cnt = target - cnt;
+    s->cnt = s->next_timeout - (uint32_t)ptimer_get_count(s->timer);
+
     return s->cnt;
 }
 
-static void imx_timerg_reload(IMXTimerGState *s, uint32_t timeout)
+static inline uint32_t imx_timerg_find_limit(uint32_t count, uint32_t reg,
+                                             uint32_t timeout)
 {
-    uint64_t diff_cnt;
+    if ((count < reg) && (timeout > reg)) {
+        timeout = reg;
+    }
+
+    return timeout;
+}
 
-    if (!(s->cr & GPT_CR_FRR)) {
-        IPRINTF("IMX_timerg_reload --- called in reset-mode\n");
+static void imx_timerg_compute_next_timeout(IMXTimerGState *s, bool event)
+{
+    uint32_t timeout = TIMER_MAX;
+    uint32_t count = 0;
+    long long limit;
+
+    if (!(s->cr & GPT_CR_EN)) {
+        /* if not enabled just return */
         return;
     }
 
-    /*
-     * For small timeouts, qemu sometimes runs too slow.
-     * Better deliver a late interrupt than none.
-     *
-     * In Reset mode (FRR bit clear)
-     * the ptimer reloads itself from OCR1;
-     * in free-running mode we need to fake
-     * running from 0 to ocr1 to TIMER_MAX
-     */
-    if (timeout > s->cnt) {
-        diff_cnt = timeout - s->cnt;
+    if (event) {
+        /* This is a timer event  */
+
+        if ((s->cr & GPT_CR_FRR)  && (s->next_timeout != TIMER_MAX)) {
+            /*
+             * if we are in free running mode and we have not reached
+             * the TIMER_MAX limit, then update the count
+             */
+            count = imx_timerg_update_counts(s);
+        }
     } else {
-        diff_cnt = 0;
+        /* not a timer event, then just update the count */
+
+        count = imx_timerg_update_counts(s);
+    }
+
+    /* now, find the next timeout related to count */
+
+    if (s->ir & GPT_IR_OF1IE) {
+        timeout = imx_timerg_find_limit(count, s->ocr1, timeout);
+    }
+    if (s->ir & GPT_IR_OF2IE) {
+        timeout = imx_timerg_find_limit(count, s->ocr2, timeout);
+    }
+    if (s->ir & GPT_IR_OF3IE) {
+        timeout = imx_timerg_find_limit(count, s->ocr3, timeout);
+    }
+
+    /* find the next set of interrupts to raise for next timer event */
+
+    s->next_int = 0;
+    if ((s->ir & GPT_IR_OF1IE) && (timeout == s->ocr1)) {
+        s->next_int |= GPT_SR_OF1;
+    }
+    if ((s->ir & GPT_IR_OF2IE) && (timeout == s->ocr2)) {
+        s->next_int |= GPT_SR_OF2;
+    }
+    if ((s->ir & GPT_IR_OF3IE) && (timeout == s->ocr3)) {
+        s->next_int |= GPT_SR_OF3;
+    }
+    if ((s->ir & GPT_IR_ROVIE) && (timeout == TIMER_MAX)) {
+        s->next_int |= GPT_SR_ROV;
+    }
+
+    /* the new range to count down from */
+    limit = timeout - imx_timerg_update_counts(s);
+
+    if (limit < 0) {
+        /*
+         * if we reach here, then QEMU is running too slow and we pass the
+         * timeout limit while computing it. Let's deliver the interrupt
+         * and compute a new limit.
+         */
+        s->sr |= s->next_int;
+
+        imx_timerg_compute_next_timeout(s, event);
+
+        imx_timerg_update(s);
+    } else {
+        /* New timeout value */
+        s->next_timeout = timeout;
+
+        /* reset the limit to the computed range */
+        ptimer_set_limit(s->timer, limit, 1);
     }
-    ptimer_set_count(s->timer, diff_cnt);
 }
 
 static uint64_t imx_timerg_read(void *opaque, hwaddr offset,
                                 unsigned size)
 {
     IMXTimerGState *s = (IMXTimerGState *)opaque;
+    uint32_t reg_value = 0;
+    uint32_t reg = offset >> 2;
 
-    DPRINTF("g-read(offset=%x)", (unsigned int)(offset >> 2));
-    switch (offset >> 2) {
+    switch (reg) {
     case 0: /* Control Register */
-        DPRINTF(" cr = %x\n", s->cr);
-        return s->cr;
+        reg_value = s->cr;
+        break;
 
     case 1: /* prescaler */
-        DPRINTF(" pr = %x\n", s->pr);
-        return s->pr;
+        reg_value = s->pr;
+        break;
 
     case 2: /* Status Register */
-        DPRINTF(" sr = %x\n", s->sr);
-        return s->sr;
+        reg_value = s->sr;
+        break;
 
     case 3: /* Interrupt Register */
-        DPRINTF(" ir = %x\n", s->ir);
-        return s->ir;
+        reg_value = s->ir;
+        break;
 
     case 4: /* Output Compare Register 1 */
-        DPRINTF(" ocr1 = %x\n", s->ocr1);
-        return s->ocr1;
+        reg_value = s->ocr1;
+        break;
 
     case 5: /* Output Compare Register 2 */
-        DPRINTF(" ocr2 = %x\n", s->ocr2);
-        return s->ocr2;
+        reg_value = s->ocr2;
+        break;
 
     case 6: /* Output Compare Register 3 */
-        DPRINTF(" ocr3 = %x\n", s->ocr3);
-        return s->ocr3;
+        reg_value = s->ocr3;
+        break;
 
     case 7: /* input Capture Register 1 */
-        DPRINTF(" icr1 = %x\n", s->icr1);
-        return s->icr1;
+        qemu_log_mask(LOG_UNIMP, "icr1 feature is not implemented\n");
+        reg_value = s->icr1;
+        break;
 
     case 8: /* input Capture Register 2 */
-        DPRINTF(" icr2 = %x\n", s->icr2);
-        return s->icr2;
+        qemu_log_mask(LOG_UNIMP, "icr2 feature is not implemented\n");
+        reg_value = s->icr2;
+        break;
 
     case 9: /* cnt */
         imx_timerg_update_counts(s);
-        DPRINTF(" cnt = %x\n", s->cnt);
-        return s->cnt;
+        reg_value = s->cnt;
+        break;
+
+    default:
+        IPRINTF("Bad offset %x\n", reg);
+        break;
     }
 
-    IPRINTF("imx_timerg_read: Bad offset %x\n",
-            (int)offset >> 2);
+    DPRINTF("(%s) = 0x%08x\n", imx_timerg_reg_name(reg), reg_value);
 
-    return 0;
+    return reg_value;
 }
 
 static void imx_timerg_reset(DeviceState *dev)
 {
     IMXTimerGState *s = container_of(dev, IMXTimerGState, busdev.qdev);
 
+    /* stop timer */
+    ptimer_stop(s->timer);
+
     /*
      * Soft reset doesn't touch some bits; hard reset clears them
      */
@@ -275,89 +374,110 @@ static void imx_timerg_reset(DeviceState *dev)
     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);
+
+    s->next_timeout = TIMER_MAX;
+    s->next_int = 0;
+
+    /* compute new freq */
     imx_timerg_set_freq(s);
+
+    /* reset the limit to TIMER_MAX */
+    ptimer_set_limit(s->timer, TIMER_MAX, 1);
+
+    /* if the timer is still enabled, restart it */
+    if (s->freq && (s->cr & GPT_CR_EN)) {
+        ptimer_run(s->timer, 1);
+    }
 }
 
 static void imx_timerg_write(void *opaque, hwaddr offset,
                              uint64_t value, unsigned size)
 {
     IMXTimerGState *s = (IMXTimerGState *)opaque;
-    DPRINTF("g-write(offset=%x, value = 0x%x)\n", (unsigned int)offset >> 2,
-            (unsigned int)value);
-
-    switch (offset >> 2) {
-    case 0: {
-        uint32_t oldcr = s->cr;
-        /* CR */
-        if (value & GPT_CR_SWR) { /* force reset */
-            value &= ~GPT_CR_SWR;
-            imx_timerg_reset(&s->busdev.qdev);
-            imx_timerg_update(s);
-        }
-
-        s->cr = value & ~0x7c00;
-        imx_timerg_set_freq(s);
-        if ((oldcr ^ value) & GPT_CR_EN) {
-            if (value & GPT_CR_EN) {
-                if (value & GPT_CR_ENMOD) {
-                    ptimer_set_count(s->timer, s->ocr1);
-                    s->cnt = 0;
+    uint32_t oldreg;
+    uint32_t reg = offset >> 2;
+
+    DPRINTF("(%s, value = 0x%08x)\n", imx_timerg_reg_name(reg),
+            (uint32_t)value);
+
+    switch (reg) {
+    case 0:
+        oldreg = s->cr;
+        s->cr = value & ~0x7c14;
+        if (s->cr & GPT_CR_SWR) { /* force reset */
+            /* handle the reset */
+            imx_timerg_reset(DEVICE(s));
+        } else {
+            /* set our freq, as the source might have changed */
+            imx_timerg_set_freq(s);
+
+            if ((oldreg ^ s->cr) & GPT_CR_EN) {
+                if (s->cr & GPT_CR_EN) {
+                    if (s->cr & GPT_CR_ENMOD) {
+                        s->next_timeout = TIMER_MAX;
+                        ptimer_set_count(s->timer, TIMER_MAX);
+                        imx_timerg_compute_next_timeout(s, false);
+                    }
+                    ptimer_run(s->timer, 1);
+                } else {
+                    /* stop timer */
+                    ptimer_stop(s->timer);
                 }
-                ptimer_run(s->timer,
-                           (value & GPT_CR_FRR) && (s->ocr1 != TIMER_MAX));
-            } else {
-                ptimer_stop(s->timer);
-            };
+            }
         }
-        return;
-    }
+        break;
 
     case 1: /* Prescaler */
         s->pr = value & 0xfff;
         imx_timerg_set_freq(s);
-        return;
+        break;
 
     case 2: /* SR */
-        /*
-         * No point in implementing the status register bits to do with
-         * external interrupt sources.
-         */
-        value &= GPT_SR_OF1 | GPT_SR_ROV;
-        s->sr &= ~value;
+        s->sr &= ~(value & 0x3f);
         imx_timerg_update(s);
-        return;
+        break;
 
     case 3: /* IR -- interrupt register */
         s->ir = value & 0x3f;
         imx_timerg_update(s);
-        return;
+
+        imx_timerg_compute_next_timeout(s, false);
+
+        break;
 
     case 4: /* OCR1 -- output compare register */
+        s->ocr1 = value;
+
         /* In non-freerun mode, reset count when this register is written */
         if (!(s->cr & GPT_CR_FRR)) {
-            s->waiting_rov = 0;
-            ptimer_set_limit(s->timer, value, 1);
-        } else {
-            imx_timerg_update_counts(s);
-            if (value > s->cnt) {
-                s->waiting_rov = 0;
-                imx_timerg_reload(s, value);
-            } else {
-                s->waiting_rov = 1;
-                imx_timerg_reload(s, TIMER_MAX - s->cnt);
-            }
+            s->next_timeout = TIMER_MAX;
+            ptimer_set_limit(s->timer, TIMER_MAX, 1);
         }
-        s->ocr1 = value;
-        return;
+
+        /* compute the new timeout */
+        imx_timerg_compute_next_timeout(s, false);
+
+        break;
 
     case 5: /* OCR2 -- output compare register */
+        s->ocr2 = value;
+
+        /* compute the new timeout */
+        imx_timerg_compute_next_timeout(s, false);
+
+        break;
+
     case 6: /* OCR3 -- output compare register */
+        s->ocr3 = value;
+
+        /* compute the new timeout */
+        imx_timerg_compute_next_timeout(s, false);
+
+        break;
+
     default:
-        IPRINTF("imx_timerg_write: Bad offset %x\n",
-                (int)offset >> 2);
+        IPRINTF("Bad offset %x\n", reg);
+        break;
     }
 }
 
@@ -365,41 +485,18 @@ static void imx_timerg_timeout(void *opaque)
 {
     IMXTimerGState *s = (IMXTimerGState *)opaque;
 
-    DPRINTF("imx_timerg_timeout, waiting rov=%d\n", s->waiting_rov);
-    if (s->cr & GPT_CR_FRR) {
-        /*
-         * Free running timer from 0 -> TIMERMAX
-         * Generates interrupt at TIMER_MAX and at cnt==ocr1
-         * If ocr1 == TIMER_MAX, then no need to reload timer.
-         */
-        if (s->ocr1 == TIMER_MAX) {
-            DPRINTF("s->ocr1 == TIMER_MAX, FRR\n");
-            s->sr |= GPT_SR_OF1 | GPT_SR_ROV;
-            imx_timerg_update(s);
-            return;
-        }
+    DPRINTF("\n");
 
-        if (s->waiting_rov) {
-            /*
-             * We were waiting for cnt==TIMER_MAX
-             */
-            s->sr |= GPT_SR_ROV;
-            s->waiting_rov = 0;
-            s->cnt = 0;
-            imx_timerg_reload(s, s->ocr1);
-        } else {
-            /* Must have got a cnt==ocr1 timeout. */
-            s->sr |= GPT_SR_OF1;
-            s->cnt = s->ocr1;
-            s->waiting_rov = 1;
-            imx_timerg_reload(s, TIMER_MAX);
-        }
-        imx_timerg_update(s);
-        return;
-    }
+    s->sr |= s->next_int;
+    s->next_int = 0;
+
+    imx_timerg_compute_next_timeout(s, true);
 
-    s->sr |= GPT_SR_OF1;
     imx_timerg_update(s);
+
+    if (s->freq && (s->cr & GPT_CR_EN)) {
+        ptimer_run(s->timer, 1);
+    }
 }
 
 static const MemoryRegionOps imx_timerg_ops = {
@@ -416,7 +513,7 @@ static int imx_timerg_init(SysBusDevice *dev)
 
     sysbus_init_irq(dev, &s->irq);
     memory_region_init_io(&s->iomem, &imx_timerg_ops,
-                          s, "imxg-timer",
+                          s, TYPE_IMX_GPT,
                           0x00001000);
     sysbus_init_mmio(dev, &s->iomem);
 
@@ -428,14 +525,12 @@ static int imx_timerg_init(SysBusDevice *dev)
     return 0;
 }
 
-void imx_timerg_create(const hwaddr addr,
-                              qemu_irq irq,
-                              DeviceState *ccm)
+void imx_timerg_create(const hwaddr addr, qemu_irq irq, DeviceState *ccm)
 {
     IMXTimerGState *pp;
     DeviceState *dev;
 
-    dev = sysbus_create_simple("imx_timerg", addr, irq);
+    dev = sysbus_create_simple(TYPE_IMX_GPT, addr, irq);
     pp = container_of(dev, IMXTimerGState, busdev.qdev);
     pp->ccm = ccm;
 }
@@ -451,7 +546,7 @@ static void imx_timerg_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo imx_timerg_info = {
-    .name = "imx_timerg",
+    .name = TYPE_IMX_GPT,
     .parent = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(IMXTimerGState),
     .class_init = imx_timerg_class_init,
-- 
1.7.9.5

Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[Paolo Bonzini]

Il 25/06/2013 20:21, Peter Maydell ha scritto:
> @@ -416,7 +513,7 @@ static int imx_timerg_init(SysBusDevice *dev)
>  
>      sysbus_init_irq(dev, &s->irq);
>      memory_region_init_io(&s->iomem, &imx_timerg_ops,
> -                          s, "imxg-timer",
> +                          s, TYPE_IMX_GPT,
>                            0x00001000);
>      sysbus_init_mmio(dev, &s->iomem);
>  

There was some agreement that this is not a good change.

Paolo

Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[Peter Maydell]

On 25 June 2013 19:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 25/06/2013 20:21, Peter Maydell ha scritto:
>> @@ -416,7 +513,7 @@ static int imx_timerg_init(SysBusDevice *dev)
>>
>>      sysbus_init_irq(dev, &s->irq);
>>      memory_region_init_io(&s->iomem, &imx_timerg_ops,
>> -                          s, "imxg-timer",
>> +                          s, TYPE_IMX_GPT,
>>                            0x00001000);
>>      sysbus_init_mmio(dev, &s->iomem);
>>
>
> There was some agreement that this is not a good change.

I agree (and more so regarding the use of the macro in the
vmstate name), but nobody actually posted any comment to
that effect against any of the versions of this patch that
got sent out for review...

thanks
-- PMM

Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[Paolo Bonzini]

Il 25/06/2013 22:53, Peter Maydell ha scritto:
> On 25 June 2013 19:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 25/06/2013 20:21, Peter Maydell ha scritto:
>>> @@ -416,7 +513,7 @@ static int imx_timerg_init(SysBusDevice *dev)
>>>
>>>      sysbus_init_irq(dev, &s->irq);
>>>      memory_region_init_io(&s->iomem, &imx_timerg_ops,
>>> -                          s, "imxg-timer",
>>> +                          s, TYPE_IMX_GPT,
>>>                            0x00001000);
>>>      sysbus_init_mmio(dev, &s->iomem);
>>>
>>
>> There was some agreement that this is not a good change.
> 
> I agree (and more so regarding the use of the macro in the
> vmstate name), but nobody actually posted any comment to
> that effect against any of the versions of this patch that
> got sent out for review...

Yeah, the timing was bad... Can you post a revert, though?

Paolo

Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[jcd]

[-- Attachment #1: Type: text/plain, Size: 944 bytes --]

For my own education, could you elaborate on the reason this is not a good change? 

Thanks. 

JC 

----- Mail original -----

> Il 25/06/2013 22:53, Peter Maydell ha scritto:
> > On 25 June 2013 19:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> Il 25/06/2013 20:21, Peter Maydell ha scritto:
> >>> @@ -416,7 +513,7 @@ static int imx_timerg_init(SysBusDevice *dev)
> >>>
> >>> sysbus_init_irq(dev, &s->irq);
> >>> memory_region_init_io(&s->iomem, &imx_timerg_ops,
> >>> - s, "imxg-timer",
> >>> + s, TYPE_IMX_GPT,
> >>> 0x00001000);
> >>> sysbus_init_mmio(dev, &s->iomem);
> >>>
> >>
> >> There was some agreement that this is not a good change.
> >
> > I agree (and more so regarding the use of the macro in the
> > vmstate name), but nobody actually posted any comment to
> > that effect against any of the versions of this patch that
> > got sent out for review...

> Yeah, the timing was bad... Can you post a revert, though?

> Paolo

[-- Attachment #2: Type: text/html, Size: 1929 bytes --]

Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[Paolo Bonzini]

Il 26/06/2013 09:08, jcd@tribudubois.net ha scritto:
> For my own education, could you elaborate on the reason this is not a
> good change?

Quoting from http://article.gmane.org/gmane.comp.emulators.qemu/218078:

> The MemoryRegion name is free text that can be changed as desired and
> doesn't need to match the type name.
> 
> The TYPE_* constant serves to keep consistency between usages of the
> string literal but does not prohibit changing its value.
> 
> The VMStateDescription should thus not use the TYPE_* constant because
> it must not ever change for live migration. Renaming a type is a valid
> thing to do if there is no command line compatibility to keep (think
> board-level "xlnx.foo" -> "xlnx,foo").

Paolo

Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[jcd]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

Thanks for the info. 

Is this consolidated into some "developper documentation" somewhere (a wiki, ...)? 

JC 

----- Mail original -----

> Il 26/06/2013 09:08, jcd@tribudubois.net ha scritto:
> > For my own education, could you elaborate on the reason this is not a
> > good change?

> Quoting from http://article.gmane.org/gmane.comp.emulators.qemu/218078:

> > The MemoryRegion name is free text that can be changed as desired and
> > doesn't need to match the type name.
> >
> > The TYPE_* constant serves to keep consistency between usages of the
> > string literal but does not prohibit changing its value.
> >
> > The VMStateDescription should thus not use the TYPE_* constant because
> > it must not ever change for live migration. Renaming a type is a valid
> > thing to do if there is no command line compatibility to keep (think
> > board-level "xlnx.foo" -> "xlnx,foo").

> Paolo

[-- Attachment #2: Type: text/html, Size: 1648 bytes --]

Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[Peter Crosthwaite]

Hi

On Wed, Jun 26, 2013 at 4:56 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 25/06/2013 22:53, Peter Maydell ha scritto:
>> On 25 June 2013 19:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 25/06/2013 20:21, Peter Maydell ha scritto:
>>>> @@ -416,7 +513,7 @@ static int imx_timerg_init(SysBusDevice *dev)
>>>>
>>>>      sysbus_init_irq(dev, &s->irq);
>>>>      memory_region_init_io(&s->iomem, &imx_timerg_ops,
>>>> -                          s, "imxg-timer",
>>>> +                          s, TYPE_IMX_GPT,
>>>>                            0x00001000);
>>>>      sysbus_init_mmio(dev, &s->iomem);
>>>>
>>>
>>> There was some agreement that this is not a good change.
>>
>> I agree (and more so regarding the use of the macro in the
>> vmstate name), but nobody actually posted any comment to
>> that effect against any of the versions of this patch that
>> got sent out for review...
>
> Yeah, the timing was bad... Can you post a revert, though?
>

The original string being replaced was a poor choice as well. IIUC the
consensus was string field of the memory regions is supposed to
indicate the purpose of the memory region for the device. Good
examples would be "Control regs" or "MMIO". Naming the memory region
after the device type is a redundancy as that info will come via
memory region owners.

So rather than revert could you just choose something more descriptive?

Regards,
Peter

> Paolo
>

Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[Jean-Christophe DUBOIS]

On 06/26/2013 09:21 AM, Peter Crosthwaite wrote:
> Hi
>
> On Wed, Jun 26, 2013 at 4:56 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 25/06/2013 22:53, Peter Maydell ha scritto:
>>> On 25 June 2013 19:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> Il 25/06/2013 20:21, Peter Maydell ha scritto:
>>>>> @@ -416,7 +513,7 @@ static int imx_timerg_init(SysBusDevice *dev)
>>>>>
>>>>>       sysbus_init_irq(dev, &s->irq);
>>>>>       memory_region_init_io(&s->iomem, &imx_timerg_ops,
>>>>> -                          s, "imxg-timer",
>>>>> +                          s, TYPE_IMX_GPT,
>>>>>                             0x00001000);
>>>>>       sysbus_init_mmio(dev, &s->iomem);
>>>>>
>>>> There was some agreement that this is not a good change.
>>> I agree (and more so regarding the use of the macro in the
>>> vmstate name), but nobody actually posted any comment to
>>> that effect against any of the versions of this patch that
>>> got sent out for review...
>> Yeah, the timing was bad... Can you post a revert, though?
>>
> The original string being replaced was a poor choice as well. IIUC the
> consensus was string field of the memory regions is supposed to
> indicate the purpose of the memory region for the device. Good
> examples would be "Control regs" or "MMIO". Naming the memory region
> after the device type is a redundancy as that info will come via
> memory region owners.
>
> So rather than revert could you just choose something more descriptive?
Peter (Maydell),

How do you want to work this out?

Do you revert it and we start over?

Or should I provide a patch on top of the actual file to change the 
"wrong name"?

Please advise.

JC

>
> Regards,
> Peter
>
>> Paolo
>>
>

Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[Jean-Christophe DUBOIS]

On 06/26/2013 08:58 PM, Jean-Christophe DUBOIS wrote:
> On 06/26/2013 09:21 AM, Peter Crosthwaite wrote:
>> Hi
>>
>> On Wed, Jun 26, 2013 at 4:56 PM, Paolo Bonzini <pbonzini@redhat.com> 
>> wrote:
>>> Il 25/06/2013 22:53, Peter Maydell ha scritto:
>>>> On 25 June 2013 19:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>> Il 25/06/2013 20:21, Peter Maydell ha scritto:
>>>>>> @@ -416,7 +513,7 @@ static int imx_timerg_init(SysBusDevice *dev)
>>>>>>
>>>>>>       sysbus_init_irq(dev, &s->irq);
>>>>>>       memory_region_init_io(&s->iomem, &imx_timerg_ops,
>>>>>> -                          s, "imxg-timer",
>>>>>> +                          s, TYPE_IMX_GPT,
>>>>>>                             0x00001000);
>>>>>>       sysbus_init_mmio(dev, &s->iomem);
>>>>>>
>>>>> There was some agreement that this is not a good change.
>>>> I agree (and more so regarding the use of the macro in the
>>>> vmstate name), but nobody actually posted any comment to
>>>> that effect against any of the versions of this patch that
>>>> got sent out for review...
>>> Yeah, the timing was bad... Can you post a revert, though?
>>>
>> The original string being replaced was a poor choice as well. IIUC the
>> consensus was string field of the memory regions is supposed to
>> indicate the purpose of the memory region for the device. Good
>> examples would be "Control regs" or "MMIO". Naming the memory region
>> after the device type is a redundancy as that info will come via
>> memory region owners.
>>
>> So rather than revert could you just choose something more descriptive?
> Peter (Maydell),
>
> How do you want to work this out?
>
> Do you revert it and we start over?
>
> Or should I provide a patch on top of the actual file to change the 
> "wrong name"?
>
> Please advise.

I browsed through the various timer implementations in the hw/timer 
directory and I was not able to spot one that would follow the 
convention you indicated.

Could you point me to a "good citizen" example?

JC

>
> JC
>
>>
>> Regards,
>> Peter
>>
>>> Paolo
>>>
>>
>
>
>

Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[Paolo Bonzini]

Il 26/06/2013 23:13, Jean-Christophe DUBOIS ha scritto:
> On 06/26/2013 08:58 PM, Jean-Christophe DUBOIS wrote:
>> On 06/26/2013 09:21 AM, Peter Crosthwaite wrote:
>>> Hi
>>>
>>> On Wed, Jun 26, 2013 at 4:56 PM, Paolo Bonzini <pbonzini@redhat.com>
>>> wrote:
>>>> Il 25/06/2013 22:53, Peter Maydell ha scritto:
>>>>> On 25 June 2013 19:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>>> Il 25/06/2013 20:21, Peter Maydell ha scritto:
>>>>>>> @@ -416,7 +513,7 @@ static int imx_timerg_init(SysBusDevice *dev)
>>>>>>>
>>>>>>>       sysbus_init_irq(dev, &s->irq);
>>>>>>>       memory_region_init_io(&s->iomem, &imx_timerg_ops,
>>>>>>> -                          s, "imxg-timer",
>>>>>>> +                          s, TYPE_IMX_GPT,
>>>>>>>                             0x00001000);
>>>>>>>       sysbus_init_mmio(dev, &s->iomem);
>>>>>>>
>>>>>> There was some agreement that this is not a good change.
>>>>> I agree (and more so regarding the use of the macro in the
>>>>> vmstate name), but nobody actually posted any comment to
>>>>> that effect against any of the versions of this patch that
>>>>> got sent out for review...
>>>> Yeah, the timing was bad... Can you post a revert, though?
>>>>
>>> The original string being replaced was a poor choice as well. IIUC the
>>> consensus was string field of the memory regions is supposed to
>>> indicate the purpose of the memory region for the device. Good
>>> examples would be "Control regs" or "MMIO". Naming the memory region
>>> after the device type is a redundancy as that info will come via
>>> memory region owners.
>>>
>>> So rather than revert could you just choose something more descriptive?
>> Peter (Maydell),
>>
>> How do you want to work this out?
>>
>> Do you revert it and we start over?
>>
>> Or should I provide a patch on top of the actual file to change the
>> "wrong name"?
>>
>> Please advise.
> 
> I browsed through the various timer implementations in the hw/timer
> directory and I was not able to spot one that would follow the
> convention you indicated.
> 
> Could you point me to a "good citizen" example?

Here is one example (hw/pci-host/piix.c):

    memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s,
                          "pci-conf-idx", 4);
    sysbus_add_io(dev, 0xcf8, &s->conf_mem);
    sysbus_init_ioports(&s->busdev, 0xcf8, 4);

    memory_region_init_io(&s->data_mem, &pci_host_data_le_ops, s,
                          "pci-conf-data", 4);
    sysbus_add_io(dev, 0xcfc, &s->data_mem);
    sysbus_init_ioports(&s->busdev, 0xcfc, 4);

Just reverting the changes to memory regions and vmstate names is enough
for now.

Paolo

Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[Jean-Christophe DUBOIS]

[-- Attachment #1: Type: text/plain, Size: 5245 bytes --]

On 06/26/2013 11:18 PM, Paolo Bonzini wrote:
> Il 26/06/2013 23:13, Jean-Christophe DUBOIS ha scritto:
>> On 06/26/2013 08:58 PM, Jean-Christophe DUBOIS wrote:
>>> On 06/26/2013 09:21 AM, Peter Crosthwaite wrote:
>>>> Hi
>>>>
>>>> On Wed, Jun 26, 2013 at 4:56 PM, Paolo Bonzini <pbonzini@redhat.com>
>>>> wrote:
>>>>> Il 25/06/2013 22:53, Peter Maydell ha scritto:
>>>>>> On 25 June 2013 19:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>>>> Il 25/06/2013 20:21, Peter Maydell ha scritto:
>>>>>>>> @@ -416,7 +513,7 @@ static int imx_timerg_init(SysBusDevice *dev)
>>>>>>>>
>>>>>>>>        sysbus_init_irq(dev, &s->irq);
>>>>>>>>        memory_region_init_io(&s->iomem, &imx_timerg_ops,
>>>>>>>> -                          s, "imxg-timer",
>>>>>>>> +                          s, TYPE_IMX_GPT,
>>>>>>>>                              0x00001000);
>>>>>>>>        sysbus_init_mmio(dev, &s->iomem);
>>>>>>>>
>>>>>>> There was some agreement that this is not a good change.
>>>>>> I agree (and more so regarding the use of the macro in the
>>>>>> vmstate name), but nobody actually posted any comment to
>>>>>> that effect against any of the versions of this patch that
>>>>>> got sent out for review...
>>>>> Yeah, the timing was bad... Can you post a revert, though?
>>>>>
>>>> The original string being replaced was a poor choice as well. IIUC the
>>>> consensus was string field of the memory regions is supposed to
>>>> indicate the purpose of the memory region for the device. Good
>>>> examples would be "Control regs" or "MMIO". Naming the memory region
>>>> after the device type is a redundancy as that info will come via
>>>> memory region owners.
>>>>
>>>> So rather than revert could you just choose something more descriptive?
>>> Peter (Maydell),
>>>
>>> How do you want to work this out?
>>>
>>> Do you revert it and we start over?
>>>
>>> Or should I provide a patch on top of the actual file to change the
>>> "wrong name"?
>>>
>>> Please advise.
>> I browsed through the various timer implementations in the hw/timer
>> directory and I was not able to spot one that would follow the
>> convention you indicated.
>>
>> Could you point me to a "good citizen" example?
> Here is one example (hw/pci-host/piix.c):
>
>      memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s,
>                            "pci-conf-idx", 4);
>      sysbus_add_io(dev, 0xcf8, &s->conf_mem);
>      sysbus_init_ioports(&s->busdev, 0xcf8, 4);
>
>      memory_region_init_io(&s->data_mem, &pci_host_data_le_ops, s,
>                            "pci-conf-data", 4);
>      sysbus_add_io(dev, 0xcfc, &s->data_mem);
>      sysbus_init_ioports(&s->busdev, 0xcfc, 4);
>
> Just reverting the changes to memory regions and vmstate names is enough
> for now.

I tried to change the memory region name for the timer registers to 
something not prepended wit the device name (I choose "regs") and here 
is the result in the qemu console (I changes both EPIT and GPT timers). 
We went from:

    (qemu) info mtree
    memory
    0000000000000000-7ffffffffffffffe (prio 0, RW): system
       000000001fffc000-000000001fffffff (prio 0, RW): kzm.sram
       0000000043f90000-0000000043f90fff (prio 0, RW): imx-serial
       0000000043f94000-0000000043f94fff (prio 0, RW): imx-serial
       0000000053f80000-0000000053f80fff (prio 0, RW): imx_ccm
       0000000053f90000-0000000053f90fff (prio 0, RW): imx.gpt
       0000000053f94000-0000000053f94fff (prio 0, RW): imx.epit
       0000000053f98000-0000000053f98fff (prio 0, RW): imx.epit
       0000000068000000-0000000068000fff (prio 0, RW): imx_avic
       0000000080000000-0000000083ffffff (prio 0, RW): kzm.ram
       0000000088000000-000000008bffffff (prio 0, RW): alias ram.alias
    @kzm.ram 0000000000000000-0000000003ffffff
       00000000b6000000-00000000b60000ff (prio 0, RW): lan9118-mmio
    I/O
    0000000000000000-000000000000ffff (prio 0, RW): io
    aliases
    kzm.ram
    0000000080000000-0000000083ffffff (prio 0, RW): kzm.ram
    (qemu)

to:

    (qemu) info mtree
    memory
    0000000000000000-7ffffffffffffffe (prio 0, RW): system
       000000001fffc000-000000001fffffff (prio 0, RW): kzm.sram
       0000000043f90000-0000000043f90fff (prio 0, RW): imx-serial
       0000000043f94000-0000000043f94fff (prio 0, RW): imx-serial
       0000000053f80000-0000000053f80fff (prio 0, RW): imx_ccm
       0000000053f90000-0000000053f90fff (prio 0, RW): regs
       0000000053f94000-0000000053f94fff (prio 0, RW): regs
       0000000053f98000-0000000053f98fff (prio 0, RW): regs
       0000000068000000-0000000068000fff (prio 0, RW): imx_avic
       0000000080000000-0000000083ffffff (prio 0, RW): kzm.ram
       0000000088000000-000000008bffffff (prio 0, RW): alias ram.alias
    @kzm.ram 0000000000000000-0000000003ffffff
       00000000b6000000-00000000b60000ff (prio 0, RW): lan9118-mmio
    I/O
    0000000000000000-000000000000ffff (prio 0, RW): io
    aliases
    kzm.ram
    0000000080000000-0000000083ffffff (prio 0, RW): kzm.ram
    (qemu)


Names don't feel really useful in the second case as they are 
indistinguishable. Is the consensus around "generic" names (like MMIO or 
"Ctrl regs") without adding reference to the device a good one for all 
platforms?

JC


>
> Paolo
>
>


[-- Attachment #2: Type: text/html, Size: 7004 bytes --]

Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[Peter Maydell]

On 26 June 2013 22:57, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
> Names don't feel really useful in the second case as they are
> indistinguishable. Is the consensus around "generic" names (like MMIO or
> "Ctrl regs") without adding reference to the device a good one for all
> platforms?

Just leave the memory region and vmstate names as they
were before your patches.

(Personally I think it's entirely fine to include the device
name in the memory region name. These strings are for debugging,
so if I print region->name in my debugger it's much more helpful
if it says "imx-serial" than if it just says "regs".)

-- PMM

Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[Jean-Christophe DUBOIS]

On 06/27/2013 12:15 AM, Peter Maydell wrote:
> On 26 June 2013 22:57, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
>> Names don't feel really useful in the second case as they are
>> indistinguishable. Is the consensus around "generic" names (like MMIO or
>> "Ctrl regs") without adding reference to the device a good one for all
>> platforms?
> Just leave the memory region and vmstate names as they
> were before your patches.

OK, I will.

Now, please notice that the EPIT timer has been changed recently (on 
main line) with the exact same code model as the one proposed in this 
patch. So it is not on par either with any consensus that might have 
been made on VMState or memory region naming.

JC

>
> (Personally I think it's entirely fine to include the device
> name in the memory region name. These strings are for debugging,
> so if I print region->name in my debugger it's much more helpful
> if it says "imx-serial" than if it just says "regs".)
>
> -- PMM
>

Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[Peter Maydell]

On 26 June 2013 23:32, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
> On 06/27/2013 12:15 AM, Peter Maydell wrote:
>> Just leave the memory region and vmstate names as they
>> were before your patches.

> OK, I will.
>
> Now, please notice that the EPIT timer has been changed recently (on main
> line) with the exact same code model as the one proposed in this patch. So
> it is not on par either with any consensus that might have been made on
> VMState or memory region naming.

Since Anthony's now applied this pullreq, I'll just send
out a fresh patch which corrects these (and also which
fixes some other devices which were also using type name
macros in vmstate).

thanks
-- PMM

Re: [Qemu-devel] [PULL 2/8] i.MX: Implement a more complete version of the GPT timer.

[Andreas Färber]

Am 26.06.2013 23:57, schrieb Jean-Christophe DUBOIS:
> On 06/26/2013 11:18 PM, Paolo Bonzini wrote:
>> Il 26/06/2013 23:13, Jean-Christophe DUBOIS ha scritto:
>>> On 06/26/2013 08:58 PM, Jean-Christophe DUBOIS wrote:
>>>> On 06/26/2013 09:21 AM, Peter Crosthwaite wrote:
>>>>> Hi
>>>>>
>>>>> On Wed, Jun 26, 2013 at 4:56 PM, Paolo Bonzini <pbonzini@redhat.com>
>>>>> wrote:
>>>>>> Il 25/06/2013 22:53, Peter Maydell ha scritto:
>>>>>>> On 25 June 2013 19:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>>>>> Il 25/06/2013 20:21, Peter Maydell ha scritto:
>>>>>>>>> @@ -416,7 +513,7 @@ static int imx_timerg_init(SysBusDevice *dev)
>>>>>>>>>
>>>>>>>>>       sysbus_init_irq(dev, &s->irq);
>>>>>>>>>       memory_region_init_io(&s->iomem, &imx_timerg_ops,
>>>>>>>>> -                          s, "imxg-timer",
>>>>>>>>> +                          s, TYPE_IMX_GPT,
>>>>>>>>>                             0x00001000);
>>>>>>>>>       sysbus_init_mmio(dev, &s->iomem);
>>>>>>>>>
>>>>>>>> There was some agreement that this is not a good change.
>>>>>>> I agree (and more so regarding the use of the macro in the
>>>>>>> vmstate name), but nobody actually posted any comment to
>>>>>>> that effect against any of the versions of this patch that
>>>>>>> got sent out for review...
>>>>>> Yeah, the timing was bad... Can you post a revert, though?
>>>>>>
>>>>> The original string being replaced was a poor choice as well. IIUC the
>>>>> consensus was string field of the memory regions is supposed to
>>>>> indicate the purpose of the memory region for the device. Good
>>>>> examples would be "Control regs" or "MMIO". Naming the memory region
>>>>> after the device type is a redundancy as that info will come via
>>>>> memory region owners.
>>>>>
>>>>> So rather than revert could you just choose something more descriptive?
>>>> Peter (Maydell),
>>>>
>>>> How do you want to work this out?
>>>>
>>>> Do you revert it and we start over?
>>>>
>>>> Or should I provide a patch on top of the actual file to change the
>>>> "wrong name"?
>>>>
>>>> Please advise.
>>> I browsed through the various timer implementations in the hw/timer
>>> directory and I was not able to spot one that would follow the
>>> convention you indicated.
>>>
>>> Could you point me to a "good citizen" example?
>> Here is one example (hw/pci-host/piix.c):
>>
>>     memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s,
>>                           "pci-conf-idx", 4);
>>     sysbus_add_io(dev, 0xcf8, &s->conf_mem);
>>     sysbus_init_ioports(&s->busdev, 0xcf8, 4);
>>
>>     memory_region_init_io(&s->data_mem, &pci_host_data_le_ops, s,
>>                           "pci-conf-data", 4);
>>     sysbus_add_io(dev, 0xcfc, &s->data_mem);
>>     sysbus_init_ioports(&s->busdev, 0xcfc, 4);
>>
>> Just reverting the changes to memory regions and vmstate names is enough
>> for now.
> 
> I tried to change the memory region name for the timer registers to
> something not prepended wit the device name (I choose "regs") and here
> is the result in the qemu console (I changes both EPIT and GPT timers).
> We went from:
> 
>     (qemu) info mtree
>     memory
>     0000000000000000-7ffffffffffffffe (prio 0, RW): system
>       000000001fffc000-000000001fffffff (prio 0, RW): kzm.sram
>       0000000043f90000-0000000043f90fff (prio 0, RW): imx-serial
>       0000000043f94000-0000000043f94fff (prio 0, RW): imx-serial
>       0000000053f80000-0000000053f80fff (prio 0, RW): imx_ccm
>       0000000053f90000-0000000053f90fff (prio 0, RW): imx.gpt
>       0000000053f94000-0000000053f94fff (prio 0, RW): imx.epit
>       0000000053f98000-0000000053f98fff (prio 0, RW): imx.epit
>       0000000068000000-0000000068000fff (prio 0, RW): imx_avic
>       0000000080000000-0000000083ffffff (prio 0, RW): kzm.ram
>       0000000088000000-000000008bffffff (prio 0, RW): alias ram.alias
>     @kzm.ram 0000000000000000-0000000003ffffff
>       00000000b6000000-00000000b60000ff (prio 0, RW): lan9118-mmio
>     I/O
>     0000000000000000-000000000000ffff (prio 0, RW): io
>     aliases
>     kzm.ram
>     0000000080000000-0000000083ffffff (prio 0, RW): kzm.ram
>     (qemu)
> 
> to:
> 
>     (qemu) info mtree
>     memory
>     0000000000000000-7ffffffffffffffe (prio 0, RW): system
>       000000001fffc000-000000001fffffff (prio 0, RW): kzm.sram
>       0000000043f90000-0000000043f90fff (prio 0, RW): imx-serial
>       0000000043f94000-0000000043f94fff (prio 0, RW): imx-serial
>       0000000053f80000-0000000053f80fff (prio 0, RW): imx_ccm
>       0000000053f90000-0000000053f90fff (prio 0, RW): regs
>       0000000053f94000-0000000053f94fff (prio 0, RW): regs
>       0000000053f98000-0000000053f98fff (prio 0, RW): regs
>       0000000068000000-0000000068000fff (prio 0, RW): imx_avic
>       0000000080000000-0000000083ffffff (prio 0, RW): kzm.ram
>       0000000088000000-000000008bffffff (prio 0, RW): alias ram.alias
>     @kzm.ram 0000000000000000-0000000003ffffff
>       00000000b6000000-00000000b60000ff (prio 0, RW): lan9118-mmio
>     I/O
>     0000000000000000-000000000000ffff (prio 0, RW): io
>     aliases
>     kzm.ram
>     0000000080000000-0000000083ffffff (prio 0, RW): kzm.ram
>     (qemu)
> 
> 
> Names don't feel really useful in the second case as they are
> indistinguishable. Is the consensus around "generic" names (like MMIO or
> "Ctrl regs") without adding reference to the device a good one for all
> platforms?

Paolo's series adds the MemoryRegion owner but hasn't been merged yet.
Just revert the names so that Paolo's series applies cleanly. Any name
changes can then still be done as follow-ups.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PULL 3/8] i.MX: Rework functions/types name and use new style initialization

[Peter Maydell]

From: Jean-Christophe DUBOIS <jcd@tribudubois.net>

* use dynamic cast whenever possible
* Change function names to some more meaningful prefix
* Change type names to a more meaningful one
* use new style device initialization

Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
Message-id: 1369898943-1993-3-git-send-email-jcd@tribudubois.net
Reviewed-by: Peter Chubb <peter.chubb@nicta.com.au>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/imx_gpt.c |  171 ++++++++++++++++++++++++++--------------------------
 1 file changed, 84 insertions(+), 87 deletions(-)

diff --git a/hw/timer/imx_gpt.c b/hw/timer/imx_gpt.c
index ecf6e53..de53b13 100644
--- a/hw/timer/imx_gpt.c
+++ b/hw/timer/imx_gpt.c
@@ -27,7 +27,7 @@
 #define DEBUG_TIMER 0
 #if DEBUG_TIMER
 
-static char const *imx_timerg_reg_name(uint32_t reg)
+static char const *imx_gpt_reg_name(uint32_t reg)
 {
     switch (reg) {
     case 0:
@@ -67,12 +67,14 @@ static char const *imx_timerg_reg_name(uint32_t reg)
  */
 #define DEBUG_IMPLEMENTATION 1
 #if DEBUG_IMPLEMENTATION
-#  define IPRINTF(fmt, args...)                                         \
+#  define IPRINTF(fmt, args...) \
           do { fprintf(stderr, "%s: " fmt, __func__, ##args); } while (0)
 #else
 #  define IPRINTF(fmt, args...) do {} while (0)
 #endif
 
+#define IMX_GPT(obj) \
+        OBJECT_CHECK(IMXGPTState, (obj), TYPE_IMX_GPT)
 /*
  * GPT : General purpose timer
  *
@@ -137,33 +139,33 @@ typedef struct {
     uint32_t freq;
 
     qemu_irq irq;
-} IMXTimerGState;
+} IMXGPTState;
 
-static const VMStateDescription vmstate_imx_timerg = {
+static const VMStateDescription vmstate_imx_timer_gpt = {
     .name = TYPE_IMX_GPT,
     .version_id = 3,
     .minimum_version_id = 3,
     .minimum_version_id_old = 3,
     .fields      = (VMStateField[]) {
-        VMSTATE_UINT32(cr, IMXTimerGState),
-        VMSTATE_UINT32(pr, IMXTimerGState),
-        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(next_timeout, IMXTimerGState),
-        VMSTATE_UINT32(next_int, IMXTimerGState),
-        VMSTATE_UINT32(freq, IMXTimerGState),
-        VMSTATE_PTIMER(timer, IMXTimerGState),
+        VMSTATE_UINT32(cr, IMXGPTState),
+        VMSTATE_UINT32(pr, IMXGPTState),
+        VMSTATE_UINT32(sr, IMXGPTState),
+        VMSTATE_UINT32(ir, IMXGPTState),
+        VMSTATE_UINT32(ocr1, IMXGPTState),
+        VMSTATE_UINT32(ocr2, IMXGPTState),
+        VMSTATE_UINT32(ocr3, IMXGPTState),
+        VMSTATE_UINT32(icr1, IMXGPTState),
+        VMSTATE_UINT32(icr2, IMXGPTState),
+        VMSTATE_UINT32(cnt, IMXGPTState),
+        VMSTATE_UINT32(next_timeout, IMXGPTState),
+        VMSTATE_UINT32(next_int, IMXGPTState),
+        VMSTATE_UINT32(freq, IMXGPTState),
+        VMSTATE_PTIMER(timer, IMXGPTState),
         VMSTATE_END_OF_LIST()
     }
 };
 
-static const IMXClk imx_timerg_clocks[] = {
+static const IMXClk imx_gpt_clocks[] = {
     NOCLK,    /* 000 No clock source */
     IPG,      /* 001 ipg_clk, 532MHz*/
     IPG,      /* 010 ipg_clk_highfreq */
@@ -174,10 +176,10 @@ static const IMXClk imx_timerg_clocks[] = {
     NOCLK,    /* 111 not defined */
 };
 
-static void imx_timerg_set_freq(IMXTimerGState *s)
+static void imx_gpt_set_freq(IMXGPTState *s)
 {
     uint32_t clksrc = extract32(s->cr, GPT_CR_CLKSRC_SHIFT, 3);
-    uint32_t freq = imx_clock_frequency(s->ccm, imx_timerg_clocks[clksrc])
+    uint32_t freq = imx_clock_frequency(s->ccm, imx_gpt_clocks[clksrc])
                                                 / (1 + s->pr);
     s->freq = freq;
 
@@ -188,7 +190,7 @@ static void imx_timerg_set_freq(IMXTimerGState *s)
     }
 }
 
-static void imx_timerg_update(IMXTimerGState *s)
+static void imx_gpt_update_int(IMXGPTState *s)
 {
     if ((s->sr & s->ir) && (s->cr & GPT_CR_EN)) {
         qemu_irq_raise(s->irq);
@@ -197,14 +199,14 @@ static void imx_timerg_update(IMXTimerGState *s)
     }
 }
 
-static uint32_t imx_timerg_update_counts(IMXTimerGState *s)
+static uint32_t imx_gpt_update_count(IMXGPTState *s)
 {
     s->cnt = s->next_timeout - (uint32_t)ptimer_get_count(s->timer);
 
     return s->cnt;
 }
 
-static inline uint32_t imx_timerg_find_limit(uint32_t count, uint32_t reg,
+static inline uint32_t imx_gpt_find_limit(uint32_t count, uint32_t reg,
                                              uint32_t timeout)
 {
     if ((count < reg) && (timeout > reg)) {
@@ -214,7 +216,7 @@ static inline uint32_t imx_timerg_find_limit(uint32_t count, uint32_t reg,
     return timeout;
 }
 
-static void imx_timerg_compute_next_timeout(IMXTimerGState *s, bool event)
+static void imx_gpt_compute_next_timeout(IMXGPTState *s, bool event)
 {
     uint32_t timeout = TIMER_MAX;
     uint32_t count = 0;
@@ -233,24 +235,24 @@ static void imx_timerg_compute_next_timeout(IMXTimerGState *s, bool event)
              * if we are in free running mode and we have not reached
              * the TIMER_MAX limit, then update the count
              */
-            count = imx_timerg_update_counts(s);
+            count = imx_gpt_update_count(s);
         }
     } else {
         /* not a timer event, then just update the count */
 
-        count = imx_timerg_update_counts(s);
+        count = imx_gpt_update_count(s);
     }
 
     /* now, find the next timeout related to count */
 
     if (s->ir & GPT_IR_OF1IE) {
-        timeout = imx_timerg_find_limit(count, s->ocr1, timeout);
+        timeout = imx_gpt_find_limit(count, s->ocr1, timeout);
     }
     if (s->ir & GPT_IR_OF2IE) {
-        timeout = imx_timerg_find_limit(count, s->ocr2, timeout);
+        timeout = imx_gpt_find_limit(count, s->ocr2, timeout);
     }
     if (s->ir & GPT_IR_OF3IE) {
-        timeout = imx_timerg_find_limit(count, s->ocr3, timeout);
+        timeout = imx_gpt_find_limit(count, s->ocr3, timeout);
     }
 
     /* find the next set of interrupts to raise for next timer event */
@@ -270,7 +272,7 @@ static void imx_timerg_compute_next_timeout(IMXTimerGState *s, bool event)
     }
 
     /* the new range to count down from */
-    limit = timeout - imx_timerg_update_counts(s);
+    limit = timeout - imx_gpt_update_count(s);
 
     if (limit < 0) {
         /*
@@ -280,9 +282,9 @@ static void imx_timerg_compute_next_timeout(IMXTimerGState *s, bool event)
          */
         s->sr |= s->next_int;
 
-        imx_timerg_compute_next_timeout(s, event);
+        imx_gpt_compute_next_timeout(s, event);
 
-        imx_timerg_update(s);
+        imx_gpt_update_int(s);
     } else {
         /* New timeout value */
         s->next_timeout = timeout;
@@ -292,10 +294,9 @@ static void imx_timerg_compute_next_timeout(IMXTimerGState *s, bool event)
     }
 }
 
-static uint64_t imx_timerg_read(void *opaque, hwaddr offset,
-                                unsigned size)
+static uint64_t imx_gpt_read(void *opaque, hwaddr offset, unsigned size)
 {
-    IMXTimerGState *s = (IMXTimerGState *)opaque;
+    IMXGPTState *s = IMX_GPT(opaque);
     uint32_t reg_value = 0;
     uint32_t reg = offset >> 2;
 
@@ -339,7 +340,7 @@ static uint64_t imx_timerg_read(void *opaque, hwaddr offset,
         break;
 
     case 9: /* cnt */
-        imx_timerg_update_counts(s);
+        imx_gpt_update_count(s);
         reg_value = s->cnt;
         break;
 
@@ -348,14 +349,14 @@ static uint64_t imx_timerg_read(void *opaque, hwaddr offset,
         break;
     }
 
-    DPRINTF("(%s) = 0x%08x\n", imx_timerg_reg_name(reg), reg_value);
+    DPRINTF("(%s) = 0x%08x\n", imx_gpt_reg_name(reg), reg_value);
 
     return reg_value;
 }
 
-static void imx_timerg_reset(DeviceState *dev)
+static void imx_gpt_reset(DeviceState *dev)
 {
-    IMXTimerGState *s = container_of(dev, IMXTimerGState, busdev.qdev);
+    IMXGPTState *s = IMX_GPT(dev);
 
     /* stop timer */
     ptimer_stop(s->timer);
@@ -379,7 +380,7 @@ static void imx_timerg_reset(DeviceState *dev)
     s->next_int = 0;
 
     /* compute new freq */
-    imx_timerg_set_freq(s);
+    imx_gpt_set_freq(s);
 
     /* reset the limit to TIMER_MAX */
     ptimer_set_limit(s->timer, TIMER_MAX, 1);
@@ -390,14 +391,14 @@ static void imx_timerg_reset(DeviceState *dev)
     }
 }
 
-static void imx_timerg_write(void *opaque, hwaddr offset,
-                             uint64_t value, unsigned size)
+static void imx_gpt_write(void *opaque, hwaddr offset, uint64_t value,
+                          unsigned size)
 {
-    IMXTimerGState *s = (IMXTimerGState *)opaque;
+    IMXGPTState *s = IMX_GPT(opaque);
     uint32_t oldreg;
     uint32_t reg = offset >> 2;
 
-    DPRINTF("(%s, value = 0x%08x)\n", imx_timerg_reg_name(reg),
+    DPRINTF("(%s, value = 0x%08x)\n", imx_gpt_reg_name(reg),
             (uint32_t)value);
 
     switch (reg) {
@@ -406,17 +407,17 @@ static void imx_timerg_write(void *opaque, hwaddr offset,
         s->cr = value & ~0x7c14;
         if (s->cr & GPT_CR_SWR) { /* force reset */
             /* handle the reset */
-            imx_timerg_reset(DEVICE(s));
+            imx_gpt_reset(DEVICE(s));
         } else {
             /* set our freq, as the source might have changed */
-            imx_timerg_set_freq(s);
+            imx_gpt_set_freq(s);
 
             if ((oldreg ^ s->cr) & GPT_CR_EN) {
                 if (s->cr & GPT_CR_EN) {
                     if (s->cr & GPT_CR_ENMOD) {
                         s->next_timeout = TIMER_MAX;
                         ptimer_set_count(s->timer, TIMER_MAX);
-                        imx_timerg_compute_next_timeout(s, false);
+                        imx_gpt_compute_next_timeout(s, false);
                     }
                     ptimer_run(s->timer, 1);
                 } else {
@@ -429,19 +430,19 @@ static void imx_timerg_write(void *opaque, hwaddr offset,
 
     case 1: /* Prescaler */
         s->pr = value & 0xfff;
-        imx_timerg_set_freq(s);
+        imx_gpt_set_freq(s);
         break;
 
     case 2: /* SR */
         s->sr &= ~(value & 0x3f);
-        imx_timerg_update(s);
+        imx_gpt_update_int(s);
         break;
 
     case 3: /* IR -- interrupt register */
         s->ir = value & 0x3f;
-        imx_timerg_update(s);
+        imx_gpt_update_int(s);
 
-        imx_timerg_compute_next_timeout(s, false);
+        imx_gpt_compute_next_timeout(s, false);
 
         break;
 
@@ -455,7 +456,7 @@ static void imx_timerg_write(void *opaque, hwaddr offset,
         }
 
         /* compute the new timeout */
-        imx_timerg_compute_next_timeout(s, false);
+        imx_gpt_compute_next_timeout(s, false);
 
         break;
 
@@ -463,7 +464,7 @@ static void imx_timerg_write(void *opaque, hwaddr offset,
         s->ocr2 = value;
 
         /* compute the new timeout */
-        imx_timerg_compute_next_timeout(s, false);
+        imx_gpt_compute_next_timeout(s, false);
 
         break;
 
@@ -471,7 +472,7 @@ static void imx_timerg_write(void *opaque, hwaddr offset,
         s->ocr3 = value;
 
         /* compute the new timeout */
-        imx_timerg_compute_next_timeout(s, false);
+        imx_gpt_compute_next_timeout(s, false);
 
         break;
 
@@ -481,80 +482,76 @@ static void imx_timerg_write(void *opaque, hwaddr offset,
     }
 }
 
-static void imx_timerg_timeout(void *opaque)
+static void imx_gpt_timeout(void *opaque)
 {
-    IMXTimerGState *s = (IMXTimerGState *)opaque;
+    IMXGPTState *s = IMX_GPT(opaque);
 
     DPRINTF("\n");
 
     s->sr |= s->next_int;
     s->next_int = 0;
 
-    imx_timerg_compute_next_timeout(s, true);
+    imx_gpt_compute_next_timeout(s, true);
 
-    imx_timerg_update(s);
+    imx_gpt_update_int(s);
 
     if (s->freq && (s->cr & GPT_CR_EN)) {
         ptimer_run(s->timer, 1);
     }
 }
 
-static const MemoryRegionOps imx_timerg_ops = {
-    .read = imx_timerg_read,
-    .write = imx_timerg_write,
+static const MemoryRegionOps imx_gpt_ops = {
+    .read = imx_gpt_read,
+    .write = imx_gpt_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
 
-static int imx_timerg_init(SysBusDevice *dev)
+static void imx_gpt_realize(DeviceState *dev, Error **errp)
 {
-    IMXTimerGState *s = FROM_SYSBUS(IMXTimerGState, dev);
+    IMXGPTState *s = IMX_GPT(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     QEMUBH *bh;
 
-    sysbus_init_irq(dev, &s->irq);
-    memory_region_init_io(&s->iomem, &imx_timerg_ops,
-                          s, TYPE_IMX_GPT,
+    sysbus_init_irq(sbd, &s->irq);
+    memory_region_init_io(&s->iomem, &imx_gpt_ops, s, TYPE_IMX_GPT,
                           0x00001000);
-    sysbus_init_mmio(dev, &s->iomem);
+    sysbus_init_mmio(sbd, &s->iomem);
 
-    bh = qemu_bh_new(imx_timerg_timeout, s);
+    bh = qemu_bh_new(imx_gpt_timeout, s);
     s->timer = ptimer_init(bh);
-
-    /* Hard reset resets extra bits in CR */
-    s->cr = 0;
-    return 0;
 }
 
 void imx_timerg_create(const hwaddr addr, qemu_irq irq, DeviceState *ccm)
 {
-    IMXTimerGState *pp;
+    IMXGPTState *pp;
     DeviceState *dev;
 
     dev = sysbus_create_simple(TYPE_IMX_GPT, addr, irq);
-    pp = container_of(dev, IMXTimerGState, busdev.qdev);
+    pp = IMX_GPT(dev);
     pp->ccm = ccm;
 }
 
-static void imx_timerg_class_init(ObjectClass *klass, void *data)
+static void imx_gpt_class_init(ObjectClass *klass, void *data)
 {
-    DeviceClass *dc  = DEVICE_CLASS(klass);
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
-    k->init = imx_timerg_init;
-    dc->vmsd = &vmstate_imx_timerg;
-    dc->reset = imx_timerg_reset;
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = imx_gpt_realize;
+    dc->reset = imx_gpt_reset;
+    dc->vmsd = &vmstate_imx_timer_gpt;
     dc->desc = "i.MX general timer";
 }
 
-static const TypeInfo imx_timerg_info = {
+static const TypeInfo imx_gpt_info = {
     .name = TYPE_IMX_GPT,
     .parent = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(IMXTimerGState),
-    .class_init = imx_timerg_class_init,
+    .instance_size = sizeof(IMXGPTState),
+    .class_init = imx_gpt_class_init,
 };
 
-static void imx_timer_register_types(void)
+static void imx_gpt_register_types(void)
 {
-    type_register_static(&imx_timerg_info);
+    type_register_static(&imx_gpt_info);
 }
 
-type_init(imx_timer_register_types)
+type_init(imx_gpt_register_types)
-- 
1.7.9.5

[Qemu-devel] [PULL 4/8] arm/boot: Free dtb blob memory after use

[Peter Maydell]

The dtb blob returned by load_device_tree() is in memory allocated
with g_malloc(). Free it accordingly once we have copied its
contents into the guest memory. To make this easy, we need also to
clean up the error handling in load_dtb() so that we consistently
handle errors in the same way (by printing a message and then
returning -1, rather than either plowing on or exiting immediately).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Message-id: 1371209256-11408-1-git-send-email-peter.maydell@linaro.org
---
 hw/arm/boot.c |   20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 797c691..7c0090f 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -237,14 +237,14 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, binfo->dtb_filename);
     if (!filename) {
         fprintf(stderr, "Couldn't open dtb file %s\n", binfo->dtb_filename);
-        return -1;
+        goto fail;
     }
 
     fdt = load_device_tree(filename, &size);
     if (!fdt) {
         fprintf(stderr, "Couldn't open dtb file %s\n", filename);
         g_free(filename);
-        return -1;
+        goto fail;
     }
     g_free(filename);
 
@@ -252,7 +252,7 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
     scells = qemu_devtree_getprop_cell(fdt, "/", "#size-cells");
     if (acells == 0 || scells == 0) {
         fprintf(stderr, "dtb file invalid (#address-cells or #size-cells 0)\n");
-        return -1;
+        goto fail;
     }
 
     mem_reg_propsize = acells + scells;
@@ -264,7 +264,7 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
     } else if (hival != 0) {
         fprintf(stderr, "qemu: dtb file not compatible with "
                 "RAM start address > 4GB\n");
-        exit(1);
+        goto fail;
     }
     mem_reg_property[acells + scells - 1] = cpu_to_be32(binfo->ram_size);
     hival = cpu_to_be32(binfo->ram_size >> 32);
@@ -273,13 +273,14 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
     } else if (hival != 0) {
         fprintf(stderr, "qemu: dtb file not compatible with "
                 "RAM size > 4GB\n");
-        exit(1);
+        goto fail;
     }
 
     rc = qemu_devtree_setprop(fdt, "/memory", "reg", mem_reg_property,
                               mem_reg_propsize * sizeof(uint32_t));
     if (rc < 0) {
         fprintf(stderr, "couldn't set /memory/reg\n");
+        goto fail;
     }
 
     if (binfo->kernel_cmdline && *binfo->kernel_cmdline) {
@@ -287,6 +288,7 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
                                           binfo->kernel_cmdline);
         if (rc < 0) {
             fprintf(stderr, "couldn't set /chosen/bootargs\n");
+            goto fail;
         }
     }
 
@@ -295,19 +297,27 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
                 binfo->initrd_start);
         if (rc < 0) {
             fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
+            goto fail;
         }
 
         rc = qemu_devtree_setprop_cell(fdt, "/chosen", "linux,initrd-end",
                     binfo->initrd_start + binfo->initrd_size);
         if (rc < 0) {
             fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");
+            goto fail;
         }
     }
     qemu_devtree_dumpdtb(fdt, size);
 
     cpu_physical_memory_write(addr, fdt, size);
 
+    g_free(fdt);
+
     return 0;
+
+fail:
+    g_free(fdt);
+    return -1;
 }
 
 static void do_cpu_reset(void *opaque)
-- 
1.7.9.5

[Qemu-devel] [PULL 5/8] i.MX31: Fix PRCS bit test

[Peter Maydell]

From: Stefan Weil <sw@weilnetz.de>

cppcheck detected a condition which was always false.

According to the MCIMX31 Reference Manual, the PRCS bits have to be 01
to select the Frequency Pre-Multiplier (FPM). PRCS uses bits 1 and 2,
so we have to test for 2.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Peter Chubb <peter.chubb@nicta.com.au>
Message-id: 1370810662-32320-1-git-send-email-sw@weilnetz.de
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/misc/imx_ccm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/imx_ccm.c b/hw/misc/imx_ccm.c
index c153a24..427ce5c 100644
--- a/hw/misc/imx_ccm.c
+++ b/hw/misc/imx_ccm.c
@@ -153,7 +153,7 @@ static void update_clocks(IMXCCMState *s)
      * approach
      */
 
-    if ((s->ccmr & CCMR_PRCS) == 1) {
+    if ((s->ccmr & CCMR_PRCS) == 2) {
         s->pll_refclk_freq = CKIL_FREQ * 1024;
     } else {
         s->pll_refclk_freq = CKIH_FREQ;
-- 
1.7.9.5

[Qemu-devel] [PULL 6/8] block/nand: QOM casting sweep

[Peter Maydell]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Define and use standard QOM cast macro. Remove usages of DO_UPCAST and
direct -> style casting.

Cc: afaerber@suse.de

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/block/nand.c |   25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index 43401a0..861e893 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -82,6 +82,11 @@ struct NANDFlashState {
     uint32_t ioaddr_vmstate;
 };
 
+#define TYPE_NAND "nand"
+
+#define NAND(obj) \
+    OBJECT_CHECK(NANDFlashState, (obj), TYPE_NAND)
+
 static void mem_and(uint8_t *dest, const uint8_t *src, size_t n)
 {
     /* Like memcpy() but we logical-AND the data into the destination */
@@ -224,7 +229,7 @@ static const struct {
 
 static void nand_reset(DeviceState *dev)
 {
-    NANDFlashState *s = FROM_SYSBUS(NANDFlashState, SYS_BUS_DEVICE(dev));
+    NANDFlashState *s = NAND(dev);
     s->cmd = NAND_CMD_READ0;
     s->addr = 0;
     s->addrlen = 0;
@@ -279,7 +284,7 @@ static void nand_command(NANDFlashState *s)
         break;
 
     case NAND_CMD_RESET:
-        nand_reset(&s->busdev.qdev);
+        nand_reset(DEVICE(s));
         break;
 
     case NAND_CMD_PAGEPROGRAM1:
@@ -319,14 +324,14 @@ static void nand_command(NANDFlashState *s)
 
 static void nand_pre_save(void *opaque)
 {
-    NANDFlashState *s = opaque;
+    NANDFlashState *s = NAND(opaque);
 
     s->ioaddr_vmstate = s->ioaddr - s->io;
 }
 
 static int nand_post_load(void *opaque, int version_id)
 {
-    NANDFlashState *s = opaque;
+    NANDFlashState *s = NAND(opaque);
 
     if (s->ioaddr_vmstate > sizeof(s->io)) {
         return -EINVAL;
@@ -365,7 +370,7 @@ static const VMStateDescription vmstate_nand = {
 static int nand_device_init(SysBusDevice *dev)
 {
     int pagesize;
-    NANDFlashState *s = FROM_SYSBUS(NANDFlashState, dev);
+    NANDFlashState *s = NAND(dev);
 
     s->buswidth = nand_flash_ids[s->chip_id].width >> 3;
     s->size = nand_flash_ids[s->chip_id].size << 20;
@@ -436,7 +441,7 @@ static void nand_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo nand_info = {
-    .name          = "nand",
+    .name          = TYPE_NAND,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(NANDFlashState),
     .class_init    = nand_class_init,
@@ -456,7 +461,8 @@ static void nand_register_types(void)
 void nand_setpins(DeviceState *dev, uint8_t cle, uint8_t ale,
                   uint8_t ce, uint8_t wp, uint8_t gnd)
 {
-    NANDFlashState *s = (NANDFlashState *) dev;
+    NANDFlashState *s = NAND(dev);
+
     s->cle = cle;
     s->ale = ale;
     s->ce = ce;
@@ -477,7 +483,8 @@ void nand_getpins(DeviceState *dev, int *rb)
 void nand_setio(DeviceState *dev, uint32_t value)
 {
     int i;
-    NANDFlashState *s = (NANDFlashState *) dev;
+    NANDFlashState *s = NAND(dev);
+
     if (!s->ce && s->cle) {
         if (nand_flash_ids[s->chip_id].options & NAND_SAMSUNG_LP) {
             if (s->cmd == NAND_CMD_READ0 && value == NAND_CMD_LPREAD2)
@@ -581,7 +588,7 @@ uint32_t nand_getio(DeviceState *dev)
 {
     int offset;
     uint32_t x = 0;
-    NANDFlashState *s = (NANDFlashState *) dev;
+    NANDFlashState *s = NAND(dev);
 
     /* Allow sequential reading */
     if (!s->iolen && s->cmd == NAND_CMD_READ0) {
-- 
1.7.9.5

[Qemu-devel] [PULL 7/8] block/nand: Convert Sysbus::init to Device::realize

[Peter Maydell]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

The prescribed transition from Sysbus::init function to a
Device::realize.

Cc: afaerber@suse.de

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/block/nand.c |   16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index 861e893..a0232d1 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -367,7 +367,7 @@ static const VMStateDescription vmstate_nand = {
     }
 };
 
-static int nand_device_init(SysBusDevice *dev)
+static void nand_realize(DeviceState *dev, Error **errp)
 {
     int pagesize;
     NANDFlashState *s = NAND(dev);
@@ -393,16 +393,17 @@ static int nand_device_init(SysBusDevice *dev)
         nand_init_2048(s);
         break;
     default:
-        error_report("Unsupported NAND block size");
-        return -1;
+        error_setg(errp, "Unsupported NAND block size %#x\n",
+                   1 << s->page_shift);
+        return;
     }
 
     pagesize = 1 << s->oob_shift;
     s->mem_oob = 1;
     if (s->bdrv) {
         if (bdrv_is_read_only(s->bdrv)) {
-            error_report("Can't use a read-only drive");
-            return -1;
+            error_setg(errp, "Can't use a read-only drive");
+            return;
         }
         if (bdrv_getlength(s->bdrv) >=
                 (s->pages << s->page_shift) + (s->pages << s->oob_shift)) {
@@ -418,8 +419,6 @@ static int nand_device_init(SysBusDevice *dev)
     }
     /* Give s->ioaddr a sane value in case we save state before it is used. */
     s->ioaddr = s->io;
-
-    return 0;
 }
 
 static Property nand_properties[] = {
@@ -432,9 +431,8 @@ static Property nand_properties[] = {
 static void nand_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
-    k->init = nand_device_init;
+    dc->realize = nand_realize;
     dc->reset = nand_reset;
     dc->vmsd = &vmstate_nand;
     dc->props = nand_properties;
-- 
1.7.9.5

[Qemu-devel] [PULL 8/8] nand: Don't inherit from Sysbus

[Peter Maydell]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Nand chips are not sysbus devices - they do not have any sense of MMIO,
nor interrupts. Re-parent to TYPE_DEVICE accordingly.

Cc: afaerber@suse.de

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/block/nand.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index a0232d1..a871ce0 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -21,7 +21,7 @@
 # include "hw/hw.h"
 # include "hw/block/flash.h"
 # include "sysemu/blockdev.h"
-# include "hw/sysbus.h"
+#include "hw/qdev.h"
 #include "qemu/error-report.h"
 
 # define NAND_CMD_READ0		0x00
@@ -54,7 +54,8 @@
 
 typedef struct NANDFlashState NANDFlashState;
 struct NANDFlashState {
-    SysBusDevice busdev;
+    DeviceState parent_obj;
+
     uint8_t manf_id, chip_id;
     uint8_t buswidth; /* in BYTES */
     int size, pages;
@@ -440,7 +441,7 @@ static void nand_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo nand_info = {
     .name          = TYPE_NAND,
-    .parent        = TYPE_SYS_BUS_DEVICE,
+    .parent        = TYPE_DEVICE,
     .instance_size = sizeof(NANDFlashState),
     .class_init    = nand_class_init,
 };
-- 
1.7.9.5

Re: [Qemu-devel] [PULL 0/8] arm-devs queue

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> arm-devs pullreq; nothing particularly earthshattering.
> As with the target-arm pullreq, I'm trying to create a signed
> pullreq here, so let me know if I've messed it up.
> The signed tag itself is 'pull-arm-devs-20130625'; arm-devs.for-upstream
> is the branch name, as usual. 
>
>
> The following changes since commit baf8673ca802cb3ea2cdbe94813441d23bde223b:
>
>   Merge remote-tracking branch 'stefanha/block' into staging (2013-06-24 14:33:17 -0500)
>
> are available in the git repository at:
>
>
>   git://git.linaro.org/people/pmaydell/qemu-arm.git
>   arm-devs.for-upstream

No need to resubmit, but you'll want to use the tag name instead of the
branch name here in the future.

Regards,

Anthony Liguori

>
> for you to fetch changes up to 7426aa72c36c908a7d0eae3e38568bb0a70de479:
>
>   nand: Don't inherit from Sysbus (2013-06-25 19:15:46 +0100)
>
> ----------------------------------------------------------------
> arm-devs queue
>
> ----------------------------------------------------------------
> Jean-Christophe DUBOIS (2):
>       i.MX: Implement a more complete version of the GPT timer.
>       i.MX: Rework functions/types name and use new style initialization
>
> John Rigby (1):
>       ARM: Allow dumping of device tree
>
> Peter Crosthwaite (3):
>       block/nand: QOM casting sweep
>       block/nand: Convert Sysbus::init to Device::realize
>       nand: Don't inherit from Sysbus
>
> Peter Maydell (1):
>       arm/boot: Free dtb blob memory after use
>
> Stefan Weil (1):
>       i.MX31: Fix PRCS bit test
>
>  hw/arm/boot.c      |   21 +-
>  hw/block/nand.c    |   48 +++--
>  hw/misc/imx_ccm.c  |    2 +-
>  hw/timer/imx_gpt.c |  558 ++++++++++++++++++++++++++++++----------------------
>  4 files changed, 369 insertions(+), 260 deletions(-)

Re: [Qemu-devel] [PULL 0/8] arm-devs queue

[Peter Maydell]

On 25 June 2013 20:07, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> arm-devs pullreq; nothing particularly earthshattering.
>> As with the target-arm pullreq, I'm trying to create a signed
>> pullreq here, so let me know if I've messed it up.
>> The signed tag itself is 'pull-arm-devs-20130625'; arm-devs.for-upstream
>> is the branch name, as usual.
>>
>>
>> The following changes since commit baf8673ca802cb3ea2cdbe94813441d23bde223b:
>>
>>   Merge remote-tracking branch 'stefanha/block' into staging (2013-06-24 14:33:17 -0500)
>>
>> are available in the git repository at:
>>
>>
>>   git://git.linaro.org/people/pmaydell/qemu-arm.git
>>   arm-devs.for-upstream
>
> No need to resubmit, but you'll want to use the tag name instead of the
> branch name here in the future.

Yeah, I'd wondered if that might be the case.

Oh, do you want the actual tag to have a signed-off-by:
line, and (more generically) do you care if the tag message
actually has any useful content? I notice mst does both.

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/8] arm-devs queue

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 25 June 2013 20:07, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> arm-devs pullreq; nothing particularly earthshattering.
>>> As with the target-arm pullreq, I'm trying to create a signed
>>> pullreq here, so let me know if I've messed it up.
>>> The signed tag itself is 'pull-arm-devs-20130625'; arm-devs.for-upstream
>>> is the branch name, as usual.
>>>
>>>
>>> The following changes since commit baf8673ca802cb3ea2cdbe94813441d23bde223b:
>>>
>>>   Merge remote-tracking branch 'stefanha/block' into staging (2013-06-24 14:33:17 -0500)
>>>
>>> are available in the git repository at:
>>>
>>>
>>>   git://git.linaro.org/people/pmaydell/qemu-arm.git
>>>   arm-devs.for-upstream
>>
>> No need to resubmit, but you'll want to use the tag name instead of the
>> branch name here in the future.
>
> Yeah, I'd wondered if that might be the case.
>
> Oh, do you want the actual tag to have a signed-off-by:
> line,

I don't see any practical reason to add a SoB to a tag.  A tag is a
symbolic reference to an existing commit.  There is never any code
change.

A merge may involve code change so a SoB makes sense.  I now SoB all
merges.

Of course, if you want to include a SoB, more power to you but I am not
requiring it.

> and (more generically) do you care if the tag message
> actually has any useful content? I notice mst does both.

Note the tags don't show up in qemu.git git history.  I don't push them
as there's no point in pushing them there.  When I do a merge, I'm
merging the commit that the tag points to, not the tag itself.  The tag
only exists for me to be able to validate you are who you say you are
sending what you said you've sent.

So I really don't care if there's any content in the tag.  It's mostly
for your own personal benefit.

Regards,

Anthony Liguori

>
> thanks
> -- PMM

Re: [Qemu-devel] [PULL 0/8] arm-devs queue

[Peter Maydell]

On 25 June 2013 22:45, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> and (more generically) do you care if the tag message
>> actually has any useful content? I notice mst does both.
>
> Note the tags don't show up in qemu.git git history.

The tags don't, but the message does, at least according to
https://www.kernel.org/pub/software/scm/git/docs/howto/using-signed-tag-in-pull-request.html
which says a signed tag always results in a merge commit
which includes the message that came with the tag.

For most of these pull requests I'm not sure the message
would actually add much useful info though, which is why
I left it at just "arm-devs queue".

thanks
-- PMM