diff mbox

Implement the global timer present in ARM MPCore chips.

Message ID 20110706200435.GA2168@harvey-pc.matrox.com
State New
Headers show

Commit Message

Christopher Harvey July 6, 2011, 8:04 p.m. UTC 
I just rebased this from a REALLY old version and made some
changes. Before I go ahead and clean it up and properly test I wanted
to run it by this list to make sure it wont get rejected after all
that work. 

You can read about the hardware here:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0407f/BEJHAGEE.html

Reminder: 
There are probably bugs in this code, do not commit :P

I will be happy to answer any questions. 

Signed-off-by: Christopher Harvey <charvey@matrox.com>
---
 hw/mpcore.c |  188 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 177 insertions(+), 11 deletions(-)

Comments

Peter Maydell July 6, 2011, 9:58 p.m. UTC | #1 
On 6 July 2011 21:04, Christopher Harvey <charvey@matrox.com> wrote:
> [a patch for A9 global timer]

Cool, thanks.

> Reminder:
> There are probably bugs in this code, do not commit :P

(You can flag that by saying "[RFC]" in the subject rather than
"[PATCH]".)

The first thing to note is that hw/mpcore.c is shared between
the ARM11MPCore and the Cortex-A9MP, implementing the "private memory
region" peripherals. In fact the two cores don't have identical
sets of peripherals, and in particular the global timer is not
present in the 11MPCore. (The SCUs are different too, although
we have such a minimal implementation of the SCU it scarcely
matters.) You need to sort out some reasonably clean way of
distinguishing the two...


Below are some random comments noticed on a first-pass.

> +/* Global timer data */
> +static uint64_t global_timer_count = 0;
> +static uint64_t global_timer_this_inc = 0;
> +  /* Only for prescale and enable bits */
> +static uint32_t global_timer_control = 0;

Static globals look like the wrong thing to me.

scripts/checkpatch.pl will help you catch random nits like wrong
indentation and qemu's brace-placement shibboleths (though it is
not infallible by a long shot).

Avoid random unconnected whitespace changes like:
-    case 12: /* Interrupt status.  */
+    case 12: /* Interrupt status. */

Do implement the counter low/high read and write :-)

This constant looks a bit fishy, doesn't it need a ULL suffix?
+        s->compare &= 0xffffffff00000000;

+    if(s->control & 0xB)/*test for auto-inc bit, comp and timer enable bits*/

You could define some constants for the bits rather than
using a comment.

I don't know enough about qemu's timer infrastructure to
comment on your use of it.

-- PMM
Christopher Harvey July 7, 2011, 5:15 p.m. UTC | #2 
On Wed, Jul 06, 2011 at 10:58:09PM +0100, Peter Maydell wrote:
> On 6 July 2011 21:04, Christopher Harvey <charvey@matrox.com> wrote:
> 
> Below are some random comments noticed on a first-pass.
> 
> > +/* Global timer data */
> > +static uint64_t global_timer_count = 0;
> > +static uint64_t global_timer_this_inc = 0;
> > + ??/* Only for prescale and enable bits */
> > +static uint32_t global_timer_control = 0;
> 
> Static globals look like the wrong thing to me.
> 

My C code design is pretty bad. I generally just try to copy the style
around what I'm editing. These variables are to be used exclusively
inside hw/mpcore.c and are referenced in many of the functions. How
would you change this code?

thanks,
-Chris
diff mbox

Patch

diff --git a/hw/mpcore.c b/hw/mpcore.c
index 379065a..a66cb0c 100644
--- a/hw/mpcore.c
+++ b/hw/mpcore.c
@@ -20,6 +20,12 @@  gic_get_current_cpu(void)
 
 #include "arm_gic.c"
 
+/* Global timer data */
+static uint64_t global_timer_count = 0;
+static uint64_t global_timer_this_inc = 0;
+  /* Only for prescale and enable bits */
+static uint32_t global_timer_control = 0;
+
 /* MPCore private memory region.  */
 
 typedef struct {
@@ -34,11 +40,30 @@  typedef struct {
     int id; /* Encodes both timer/watchdog and CPU.  */
 } mpcore_timer_state;
 
+typedef struct {
+    uint64_t compare;
+    uint32_t inc;
+    uint32_t status;
+    /* Only used for Auto-inc, IRQ enable and comp-enable (banked bits) */
+    /* Prescaler and enable bits are in the global control variable. */
+    uint32_t control; 
+
+    /* 
+     We actually need one of these for each core even for the global timer
+     because we don't simulate every tick, just the interrupt intervals, 
+     which can be different for each core.
+    */
+    QEMUTimer *timer;
+    int id;  /* Encodes only CPU id. */
+    struct mpcore_priv_state *mpcore;
+} mpcore_global_timer_state;
+
 typedef struct mpcore_priv_state {
     gic_state gic;
     uint32_t scu_control;
     int iomemtype;
-    mpcore_timer_state timer[8];
+    mpcore_timer_state timer[2*NCPU];
+    mpcore_global_timer_state g_timers[NCPU];
     uint32_t num_cpu;
 } mpcore_priv_state;
 
@@ -46,16 +71,41 @@  typedef struct mpcore_priv_state {
 
 static inline void mpcore_timer_update_irq(mpcore_timer_state *s)
 {
-    if (s->status & ~s->old_status) {
+    if (s->status & ~s->old_status) { /* status == 1 && old_status == 0 */
         gic_set_pending_private(&s->mpcore->gic, s->id >> 1, 29 + (s->id & 1));
     }
     s->old_status = s->status;
 }
 
 /* Return conversion factor from mpcore timer ticks to qemu timer ticks.  */
-static inline uint32_t mpcore_timer_scale(mpcore_timer_state *s)
+static inline uint32_t mpcore_timer_scale(uint32_t control)
+{   
+    /* (Prescaler + 1) * 10 */
+    return (((control >> 8) & 0xff) + 1) * 10;
+}
+
+static uint64_t get_instantaneous_count(void) 
+{
+    uint64_t ret;
+    ret = qemu_get_clock_ns(vm_clock) - global_timer_count;
+    ret /= mpcore_timer_scale(global_timer_control);
+    ret += global_timer_count;
+    return ret;
+}
+
+static void set_next_global_tick(mpcore_global_timer_state *s) 
 {
-    return (((s->control >> 8) & 0xff) + 1) * 10;
+    int64_t to_wait;
+    if(s->compare < get_instantaneous_count())
+        return; /* Cortex-A9 r2p0 and later fire even when count >= compare, 
+		   not only on equality. Worth simulating? */
+
+    if(global_timer_control & 1) { /* Only set next tick if enabled. */
+       to_wait = qemu_get_clock_ns(vm_clock); 
+       to_wait += (s->compare - global_timer_count) * mpcore_timer_scale(global_timer_control);
+       global_timer_this_inc = s->compare - global_timer_count;
+       qemu_mod_timer(s->timer, to_wait);
+    }
 }
 
 static void mpcore_timer_reload(mpcore_timer_state *s, int restart)
@@ -64,10 +114,27 @@  static void mpcore_timer_reload(mpcore_timer_state *s, int restart)
         return;
     if (restart)
         s->tick = qemu_get_clock_ns(vm_clock);
-    s->tick += (int64_t)s->count * mpcore_timer_scale(s);
+    s->tick += (int64_t)s->count * mpcore_timer_scale(s->control);
     qemu_mod_timer(s->timer, s->tick);
 }
 
+static void mpcore_global_timer_tick(void *opaque) 
+{
+    mpcore_global_timer_state *s = (mpcore_global_timer_state *)opaque;
+    global_timer_count += global_timer_this_inc;
+    if(s->control & 0xB)/*test for auto-inc bit, comp and timer enable bits*/
+        s->compare += s->inc;
+
+    if(s->control & 7) { /* test for IRQ enable bit, compare enable and,
+			    timer enable */
+        if(!s->status) {
+            s->status = 1;
+            gic_set_pending_private(&s->mpcore->gic, s->id >> 1, 27);
+	}
+    }
+    set_next_global_tick(s);
+}
+
 static void mpcore_timer_tick(void *opaque)
 {
     mpcore_timer_state *s = (mpcore_timer_state *)opaque;
@@ -93,7 +160,7 @@  static uint32_t mpcore_timer_read(mpcore_timer_state *s, int offset)
             return 0;
         /* Slow and ugly, but hopefully won't happen too often.  */
         val = s->tick - qemu_get_clock_ns(vm_clock);
-        val /= mpcore_timer_scale(s);
+        val /= mpcore_timer_scale(s->control);
         if (val < 0)
             val = 0;
         return val;
@@ -106,6 +173,39 @@  static uint32_t mpcore_timer_read(mpcore_timer_state *s, int offset)
     }
 }
 
+static uint32_t mpcore_global_timer_read(mpcore_global_timer_state *s, int offset) 
+{
+    uint32_t ret;
+    offset &= 0xff; 
+    switch(offset) {
+    case 0:    /* Counter lower bits */
+    case 0x04: /* Counter high bits */
+
+        break;
+    case 0x08: /* Control */
+        ret = (s->control              & 0xE)     |    /* Bits [3:1]  */
+	      (global_timer_control    & 0xFF01);      /* Bits [15:8] and 0 */
+	break;
+    case 0x0C: /* Status */
+        ret = s->status & 1;
+	break;
+    case 0x10: /* Compare lower */
+        ret = s->compare & 0xffffffff;
+	break;
+    case 0x14: /* Compare upper  */
+        ret = s->compare >> 32;
+	break;
+    case 0x18: /* Auto increment amount */
+        ret = s->inc;
+	break;
+    default:
+        hw_error("mpcore_priv_read: Bad offset %x\n", (int)offset);
+	break;
+    }
+    return ret;
+}
+
+
 static void mpcore_timer_write(mpcore_timer_state *s, int offset,
                                uint32_t value)
 {
@@ -116,7 +216,7 @@  static void mpcore_timer_write(mpcore_timer_state *s, int offset,
         /* Fall through.  */
     case 4: /* Counter.  */
         if ((s->control & 1) && s->count) {
-            /* Cancel the previous timer.  */
+            /* Cancel the previous timer. */
             qemu_del_timer(s->timer);
         }
         s->count = value;
@@ -133,13 +233,49 @@  static void mpcore_timer_write(mpcore_timer_state *s, int offset,
             mpcore_timer_reload(s, 1);
         }
         break;
-    case 12: /* Interrupt status.  */
+    case 12: /* Interrupt status. */
         s->status &= ~value;
         mpcore_timer_update_irq(s);
         break;
     }
 }
 
+static void mpcore_global_timer_write(mpcore_global_timer_state *s, int offset,
+				      uint32_t value) 
+{
+    offset &= 0xff;
+    switch (offset) {
+    case 0:     /* lower 32 bits. */
+    case 0x04:  /* upper 32 bits. */
+        /* TODO: implement counter modify */
+        break;
+    case 0x08:
+        /* Control */
+        s->control = value & 0xE;
+	global_timer_control = value & 0xFF01;
+	set_next_global_tick(s);
+	break;
+    case 0x0C:
+        if (value & 1)
+	    s->status = 0;
+	break;
+    case 0x10:
+        /* Compare registers */
+        s->compare &= 0xffffffff00000000;
+	s->compare |= value;
+	set_next_global_tick(s);
+	break;
+    case 0x14:
+        s->compare = (s->compare & 0xffffffff ) |
+	             ((uint64_t)value << 32);
+	set_next_global_tick(s);
+	break;
+    case 0x18:
+        s->inc = value;
+	break;
+    }
+}
+
 static void mpcore_timer_init(mpcore_priv_state *mpcore,
                               mpcore_timer_state *s, int id)
 {
@@ -148,6 +284,18 @@  static void mpcore_timer_init(mpcore_priv_state *mpcore,
     s->timer = qemu_new_timer_ns(vm_clock, mpcore_timer_tick, s);
 }
 
+static void mpcore_global_timer_init(mpcore_priv_state *mpcore,
+				     mpcore_global_timer_state *s, int id) 
+{
+    s->status = 0; /* 0 is the correct power-on/reset value. */
+    s->inc = 0;
+    s->compare = 0;
+    s->control = 0;
+    s->id = id;
+    s->mpcore = mpcore;
+    s->timer = qemu_new_timer_ns(vm_clock, mpcore_global_timer_tick, s);
+}
+
 
 /* Per-CPU private memory mapped IO.  */
 
@@ -171,7 +319,7 @@  static uint32_t mpcore_priv_read(void *opaque, target_phys_addr_t offset)
         default:
             goto bad_reg;
         }
-    } else if (offset < 0x600) {
+    } else if (offset < 0x200) {
         /* Interrupt controller.  */
         if (offset < 0x200) {
             id = gic_get_current_cpu();
@@ -182,6 +330,13 @@  static uint32_t mpcore_priv_read(void *opaque, target_phys_addr_t offset)
             }
         }
         return gic_cpu_read(&s->gic, id, offset & 0xff);
+    } else if (offset < 0x300) {
+      /* Global timer */
+      id = gic_get_current_cpu();
+      return mpcore_global_timer_read(&s->g_timers[id], offset);
+    } else if (offset < 0x600) {
+      /* Non-existent */
+      goto bad_reg;
     } else if (offset < 0xb00) {
         /* Timers.  */
         if (offset < 0x700) {
@@ -203,7 +358,7 @@  bad_reg:
 }
 
 static void mpcore_priv_write(void *opaque, target_phys_addr_t offset,
-                          uint32_t value)
+			      uint32_t value)
 {
     mpcore_priv_state *s = (mpcore_priv_state *)opaque;
     int id;
@@ -220,7 +375,7 @@  static void mpcore_priv_write(void *opaque, target_phys_addr_t offset,
         default:
             goto bad_reg;
         }
-    } else if (offset < 0x600) {
+    } else if (offset < 0x200) {
         /* Interrupt controller.  */
         if (offset < 0x200) {
             id = gic_get_current_cpu();
@@ -230,6 +385,13 @@  static void mpcore_priv_write(void *opaque, target_phys_addr_t offset,
         if (id < s->num_cpu) {
             gic_cpu_write(&s->gic, id, offset & 0xff, value);
         }
+    } else if (offset < 0x300) {
+        /* Global timer */
+        id = gic_get_current_cpu();
+        mpcore_global_timer_write(&s->g_timers[id], offset, value);
+    } else if (offset < 0x600) {
+        /* Non-existent */
+        goto bad_reg;
     } else if (offset < 0xb00) {
         /* Timers.  */
         if (offset < 0x700) {
@@ -282,5 +444,9 @@  static int mpcore_priv_init(SysBusDevice *dev)
     for (i = 0; i < s->num_cpu * 2; i++) {
         mpcore_timer_init(s, &s->timer[i], i);
     }
+    /* Assuming that num_cpu is 4 or less */
+    for (i = 0; i < s->num_cpu; i++) {
+        mpcore_global_timer_init(s, &s->g_timers[i], i);
+    }
     return 0;
 }