Add global timer group A to open-pic.  This patch is still somewhat
dubious because I'm not sure how to determine what QEMU wants for the
timer frequency.  Suggestions solicited.

Signed-off-by: Aaron Larson <alarson@ddci.com>
---
 hw/intc/openpic.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 116 insertions(+), 18 deletions(-)

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 4349e45..769a31a 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -45,6 +45,7 @@
 #include "qemu/bitops.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/log.h"
+#include "qemu/timer.h"
 
 //#define DEBUG_OPENPIC
 
@@ -54,8 +55,10 @@ static const int debug_openpic = 1;
 static const int debug_openpic = 0;
 #endif
 
+static int get_current_cpu(void);
 #define DPRINTF(fmt, ...) do { \
         if (debug_openpic) { \
+            printf("Core%d: ", get_current_cpu()); \
             printf(fmt , ## __VA_ARGS__); \
         } \
     } while (0)
@@ -246,9 +249,24 @@ typedef struct IRQSource {
 #define IDR_EP      0x80000000  /* external pin */
 #define IDR_CI      0x40000000  /* critical interrupt */
 
+/* Conversion between ticks and nanosecs: TODO: need better way for this.
+ Assume a 100mhz clock, divided by 8, or 25mhz
+ 25,000,000 ticks/sec, 25,000/ms, 25/us, 1 tick/40ns
+*/
+#define CONV_FACTOR 40LL
+static inline uint64_t ns_to_ticks(uint64_t ns)   { return ns   / CONV_FACTOR; }
+static inline uint64_t ticks_to_ns(uint64_t tick) { return tick * CONV_FACTOR; }
+
 typedef struct OpenPICTimer {
     uint32_t tccr;  /* Global timer current count register */
     uint32_t tbcr;  /* Global timer base count register */
+    int                   n_IRQ;
+    bool                  qemu_timer_active; /* Is the qemu_timer is running? */
+    struct QEMUTimer     *qemu_timer;   /* May be 0 if not created. */
+    struct OpenPICState  *opp;          /* device timer is part of. */
+    /* the time corresponding to the last current_count written or read,
+       only defined if qemu_timer_active. */
+    uint64_t              originTime;
 } OpenPICTimer;
 
 typedef struct OpenPICMSI {
@@ -795,37 +813,102 @@ static uint64_t openpic_gbl_read(void *opaque, hwaddr addr, unsigned len)
     return retval;
 }
 
+static void openpic_tmr_set_tmr(OpenPICTimer *tmr, uint32_t val, bool enabled);
+
+static void qemu_timer_cb(void *opaque)
+{
+    OpenPICTimer *tmr = opaque;
+    OpenPICState *opp = tmr->opp;
+    uint32_t      n_IRQ = tmr->n_IRQ;
+    uint32_t val =   tmr->tbcr & ~TBCR_CI;
+    uint32_t tog = ((tmr->tccr & TCCR_TOG) ^ TCCR_TOG);  /* invert toggle. */
+
+    DPRINTF("%s n_IRQ=%d\n", __func__, n_IRQ);
+    /* reload current count from base count and setup timer. */
+    tmr->tccr = val | tog;
+    openpic_tmr_set_tmr(tmr, val, /*enabled=*/true);
+    /* raise the interrupt. */
+    opp->src[n_IRQ].destmask = read_IRQreg_idr(opp, n_IRQ);
+    openpic_set_irq(opp, n_IRQ, 1);
+    openpic_set_irq(opp, n_IRQ, 0);
+}
+
+/* If enabled is true, arranges for an interrupt to be raised val clocks into
+   the future, if enabled is false cancels the timer. */
+static void openpic_tmr_set_tmr(OpenPICTimer *tmr, uint32_t val, bool enabled)
+{
+    /* if timer doesn't exist, create it. */
+    if (tmr->qemu_timer == 0) {
+        tmr->qemu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &qemu_timer_cb, tmr);
+        DPRINTF("Created timer for n_IRQ %d\n", tmr->n_IRQ);
+    }
+    uint64_t ns = ticks_to_ns(val & ~TCCR_TOG);
+    /* A count of zero causes a timer to be set to expire immediately.  This
+       effectively stops the simulation so we don't honor that configuration.
+       On real hardware, this would generate an interrupt on every clock cycle
+       if the interrupt was unmasked. */
+    if ((ns == 0) || !enabled) {
+        tmr->qemu_timer_active = false;
+        tmr->tccr = tmr->tccr & TCCR_TOG;
+        timer_del(tmr->qemu_timer); /* set timer to never expire. */
+    } else {
+        tmr->qemu_timer_active = true;
+        uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        tmr->originTime = now;
+        timer_mod(tmr->qemu_timer, now + ns);     /* set timer expiration. */
+    }
+}
+
+/* Returns the currrent tccr value, i.e., timer value (in clocks) with
+   appropriate TOG. */
+static uint64_t openpic_tmr_get_timer(OpenPICTimer *tmr)
+{
+    uint64_t retval;
+    if (!tmr->qemu_timer_active) {
+        retval = tmr->tccr;
+    } else {
+        uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        uint64_t used = now - tmr->originTime;  /* nsecs */
+        uint32_t used_ticks = (uint32_t)ns_to_ticks(used);
+        uint32_t count = (tmr->tccr & ~TCCR_TOG) - used_ticks;
+        retval = (uint32_t)((tmr->tccr & TCCR_TOG) | (count & ~TCCR_TOG));
+    }
+    return retval;
+}
+
 static void openpic_tmr_write(void *opaque, hwaddr addr, uint64_t val,
-                                unsigned len)
+                              unsigned len)
 {
     OpenPICState *opp = opaque;
     int idx;
 
-    addr += 0x10f0;
-
     DPRINTF("%s: addr %#" HWADDR_PRIx " <= %08" PRIx64 "\n",
-            __func__, addr, val);
+            __func__, (addr + 0x10f0), val);
     if (addr & 0xF) {
         return;
     }
 
-    if (addr == 0x10f0) {
+    if (addr == 0) {
         /* TFRR */
         opp->tfrr = val;
         return;
     }
-
+    addr -= 0x10;  /* correct for TFRR */
     idx = (addr >> 6) & 0x3;
-    addr = addr & 0x30;
 
     switch (addr & 0x30) {
     case 0x00: /* TCCR */
         break;
     case 0x10: /* TBCR */
-        if ((opp->timers[idx].tccr & TCCR_TOG) != 0 &&
-            (val & TBCR_CI) == 0 &&
-            (opp->timers[idx].tbcr & TBCR_CI) != 0) {
-            opp->timers[idx].tccr &= ~TCCR_TOG;
+        /* Did the enable status change? */
+        if ((opp->timers[idx].tbcr & TBCR_CI) != (val & TBCR_CI)) {
+            /* Did "Count Inhibit" transition from 1 to 0? */
+            if ((val & TBCR_CI) == 0) {
+                opp->timers[idx].tccr = val & ~TCCR_TOG;
+            }
+            openpic_tmr_set_tmr(&opp->timers[idx],
+                                (val & ~TBCR_CI),
+                                /*enabled=*/((val & TBCR_CI) == 0));
         }
         opp->timers[idx].tbcr = val;
         break;
@@ -844,27 +927,28 @@ static uint64_t openpic_tmr_read(void *opaque, hwaddr addr, unsigned len)
     uint32_t retval = -1;
     int idx;
 
-    DPRINTF("%s: addr %#" HWADDR_PRIx "\n", __func__, addr);
+    DPRINTF("%s: addr %#" HWADDR_PRIx "\n", __func__, addr + 0x10f0);
     if (addr & 0xF) {
         goto out;
     }
-    idx = (addr >> 6) & 0x3;
-    if (addr == 0x0) {
+    if (addr == 0) {
         /* TFRR */
         retval = opp->tfrr;
         goto out;
     }
+    addr -= 0x10;  /* correct for TFRR */
+    idx = (addr >> 6) & 0x3;
     switch (addr & 0x30) {
     case 0x00: /* TCCR */
-        retval = opp->timers[idx].tccr;
+        retval = openpic_tmr_get_timer(&opp->timers[idx]);
         break;
     case 0x10: /* TBCR */
         retval = opp->timers[idx].tbcr;
         break;
-    case 0x20: /* TIPV */
+    case 0x20: /* TVPR */
         retval = read_IRQreg_ivpr(opp, opp->irq_tim0 + idx);
         break;
-    case 0x30: /* TIDE (TIDR) */
+    case 0x30: /* TDR */
         retval = read_IRQreg_idr(opp, opp->irq_tim0 + idx);
         break;
     }
@@ -1138,7 +1222,10 @@ static uint32_t openpic_iack(OpenPICState *opp, IRQDest *dst, int cpu)
         IRQ_resetbit(&dst->raised, irq);
     }
 
-    if ((irq >= opp->irq_ipi0) &&  (irq < (opp->irq_ipi0 + OPENPIC_MAX_IPI))) {
+    /* Timers and IPIs support multicast. */
+    if (((irq >= opp->irq_ipi0) && (irq < (opp->irq_ipi0 + OPENPIC_MAX_IPI))) ||
+        ((irq >= opp->irq_tim0) && (irq < (opp->irq_tim0 + OPENPIC_MAX_TMR)))) {
+        DPRINTF("irq is IPI or TMR\n");
         src->destmask &= ~(1 << cpu);
         if (src->destmask && !src->level) {
             /* trigger on CPUs that didn't know about it yet */
@@ -1343,6 +1430,10 @@ static void openpic_reset(DeviceState *d)
     for (i = 0; i < OPENPIC_MAX_TMR; i++) {
         opp->timers[i].tccr = 0;
         opp->timers[i].tbcr = TBCR_CI;
+        if (opp->timers[i].qemu_timer_active) {
+            timer_del(opp->timers[i].qemu_timer);  /* Inhibit timer */
+            opp->timers[i].qemu_timer_active = 0;
+        }
     }
     /* Go out of RESET state */
     opp->gcr = 0;
@@ -1393,6 +1484,13 @@ static void fsl_common_init(OpenPICState *opp)
         opp->src[i].type = IRQ_TYPE_FSLSPECIAL;
         opp->src[i].level = false;
     }
+
+    for (i = 0; i < OPENPIC_MAX_TMR; i++) {
+        opp->timers[i].n_IRQ = opp->irq_tim0 + i;
+        opp->timers[i].qemu_timer_active = 0;
+        opp->timers[i].qemu_timer = 0;
+        opp->timers[i].opp = opp;
+    }
 }
 
 static void map_list(OpenPICState *opp, const MemReg *list, int *count)
-- 
2.7.4

On Fri, Apr 21, 2017 at 02:58:23PM -0700, Aaron Larson wrote:
> Add global timer group A to open-pic.  This patch is still somewhat
> dubious because I'm not sure how to determine what QEMU wants for the
> timer frequency.  Suggestions solicited.

This commit message really needs some more context.  What's a "global
time group A" a and why do we want it?

> 
> Signed-off-by: Aaron Larson <alarson@ddci.com>
> ---
>  hw/intc/openpic.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 116 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index 4349e45..769a31a 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -45,6 +45,7 @@
>  #include "qemu/bitops.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qemu/log.h"
> +#include "qemu/timer.h"
>  
>  //#define DEBUG_OPENPIC
>  
> @@ -54,8 +55,10 @@ static const int debug_openpic = 1;
>  static const int debug_openpic = 0;
>  #endif
>  
> +static int get_current_cpu(void);
>  #define DPRINTF(fmt, ...) do { \
>          if (debug_openpic) { \
> +            printf("Core%d: ", get_current_cpu()); \
>              printf(fmt , ## __VA_ARGS__); \
>          } \
>      } while (0)
> @@ -246,9 +249,24 @@ typedef struct IRQSource {
>  #define IDR_EP      0x80000000  /* external pin */
>  #define IDR_CI      0x40000000  /* critical interrupt */
>  
> +/* Conversion between ticks and nanosecs: TODO: need better way for this.
> + Assume a 100mhz clock, divided by 8, or 25mhz
> + 25,000,000 ticks/sec, 25,000/ms, 25/us, 1 tick/40ns
> +*/
> +#define CONV_FACTOR 40LL
> +static inline uint64_t ns_to_ticks(uint64_t ns)   { return ns   / CONV_FACTOR; }
> +static inline uint64_t ticks_to_ns(uint64_t tick) { return tick * CONV_FACTOR; }
> +
>  typedef struct OpenPICTimer {
>      uint32_t tccr;  /* Global timer current count register */
>      uint32_t tbcr;  /* Global timer base count register */
> +    int                   n_IRQ;
> +    bool                  qemu_timer_active; /* Is the qemu_timer is running? */
> +    struct QEMUTimer     *qemu_timer;   /* May be 0 if not created. */
> +    struct OpenPICState  *opp;          /* device timer is part of. */
> +    /* the time corresponding to the last current_count written or read,
> +       only defined if qemu_timer_active. */
> +    uint64_t              originTime;
>  } OpenPICTimer;
>  
>  typedef struct OpenPICMSI {
> @@ -795,37 +813,102 @@ static uint64_t openpic_gbl_read(void *opaque, hwaddr addr, unsigned len)
>      return retval;
>  }
>  
> +static void openpic_tmr_set_tmr(OpenPICTimer *tmr, uint32_t val, bool enabled);
> +
> +static void qemu_timer_cb(void *opaque)
> +{
> +    OpenPICTimer *tmr = opaque;
> +    OpenPICState *opp = tmr->opp;
> +    uint32_t      n_IRQ = tmr->n_IRQ;
> +    uint32_t val =   tmr->tbcr & ~TBCR_CI;
> +    uint32_t tog = ((tmr->tccr & TCCR_TOG) ^ TCCR_TOG);  /* invert toggle. */
> +
> +    DPRINTF("%s n_IRQ=%d\n", __func__, n_IRQ);
> +    /* reload current count from base count and setup timer. */
> +    tmr->tccr = val | tog;
> +    openpic_tmr_set_tmr(tmr, val, /*enabled=*/true);
> +    /* raise the interrupt. */
> +    opp->src[n_IRQ].destmask = read_IRQreg_idr(opp, n_IRQ);
> +    openpic_set_irq(opp, n_IRQ, 1);
> +    openpic_set_irq(opp, n_IRQ, 0);
> +}
> +
> +/* If enabled is true, arranges for an interrupt to be raised val clocks into
> +   the future, if enabled is false cancels the timer. */
> +static void openpic_tmr_set_tmr(OpenPICTimer *tmr, uint32_t val, bool enabled)
> +{
> +    /* if timer doesn't exist, create it. */
> +    if (tmr->qemu_timer == 0) {
> +        tmr->qemu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &qemu_timer_cb, tmr);
> +        DPRINTF("Created timer for n_IRQ %d\n", tmr->n_IRQ);
> +    }
> +    uint64_t ns = ticks_to_ns(val & ~TCCR_TOG);
> +    /* A count of zero causes a timer to be set to expire immediately.  This
> +       effectively stops the simulation so we don't honor that configuration.
> +       On real hardware, this would generate an interrupt on every clock cycle
> +       if the interrupt was unmasked. */
> +    if ((ns == 0) || !enabled) {
> +        tmr->qemu_timer_active = false;
> +        tmr->tccr = tmr->tccr & TCCR_TOG;
> +        timer_del(tmr->qemu_timer); /* set timer to never expire. */
> +    } else {
> +        tmr->qemu_timer_active = true;
> +        uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        tmr->originTime = now;
> +        timer_mod(tmr->qemu_timer, now + ns);     /* set timer expiration. */
> +    }
> +}
> +
> +/* Returns the currrent tccr value, i.e., timer value (in clocks) with
> +   appropriate TOG. */
> +static uint64_t openpic_tmr_get_timer(OpenPICTimer *tmr)
> +{
> +    uint64_t retval;
> +    if (!tmr->qemu_timer_active) {
> +        retval = tmr->tccr;
> +    } else {
> +        uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        uint64_t used = now - tmr->originTime;  /* nsecs */
> +        uint32_t used_ticks = (uint32_t)ns_to_ticks(used);
> +        uint32_t count = (tmr->tccr & ~TCCR_TOG) - used_ticks;
> +        retval = (uint32_t)((tmr->tccr & TCCR_TOG) | (count & ~TCCR_TOG));
> +    }
> +    return retval;
> +}
> +
>  static void openpic_tmr_write(void *opaque, hwaddr addr, uint64_t val,
> -                                unsigned len)
> +                              unsigned len)
>  {
>      OpenPICState *opp = opaque;
>      int idx;
>  
> -    addr += 0x10f0;
> -
>      DPRINTF("%s: addr %#" HWADDR_PRIx " <= %08" PRIx64 "\n",
> -            __func__, addr, val);
> +            __func__, (addr + 0x10f0), val);
>      if (addr & 0xF) {
>          return;
>      }
>  
> -    if (addr == 0x10f0) {
> +    if (addr == 0) {
>          /* TFRR */
>          opp->tfrr = val;
>          return;
>      }
> -
> +    addr -= 0x10;  /* correct for TFRR */
>      idx = (addr >> 6) & 0x3;
> -    addr = addr & 0x30;
>  
>      switch (addr & 0x30) {
>      case 0x00: /* TCCR */
>          break;
>      case 0x10: /* TBCR */
> -        if ((opp->timers[idx].tccr & TCCR_TOG) != 0 &&
> -            (val & TBCR_CI) == 0 &&
> -            (opp->timers[idx].tbcr & TBCR_CI) != 0) {
> -            opp->timers[idx].tccr &= ~TCCR_TOG;
> +        /* Did the enable status change? */
> +        if ((opp->timers[idx].tbcr & TBCR_CI) != (val & TBCR_CI)) {
> +            /* Did "Count Inhibit" transition from 1 to 0? */
> +            if ((val & TBCR_CI) == 0) {
> +                opp->timers[idx].tccr = val & ~TCCR_TOG;
> +            }
> +            openpic_tmr_set_tmr(&opp->timers[idx],
> +                                (val & ~TBCR_CI),
> +                                /*enabled=*/((val & TBCR_CI) == 0));
>          }
>          opp->timers[idx].tbcr = val;
>          break;
> @@ -844,27 +927,28 @@ static uint64_t openpic_tmr_read(void *opaque, hwaddr addr, unsigned len)
>      uint32_t retval = -1;
>      int idx;
>  
> -    DPRINTF("%s: addr %#" HWADDR_PRIx "\n", __func__, addr);
> +    DPRINTF("%s: addr %#" HWADDR_PRIx "\n", __func__, addr + 0x10f0);
>      if (addr & 0xF) {
>          goto out;
>      }
> -    idx = (addr >> 6) & 0x3;
> -    if (addr == 0x0) {
> +    if (addr == 0) {
>          /* TFRR */
>          retval = opp->tfrr;
>          goto out;
>      }
> +    addr -= 0x10;  /* correct for TFRR */
> +    idx = (addr >> 6) & 0x3;
>      switch (addr & 0x30) {
>      case 0x00: /* TCCR */
> -        retval = opp->timers[idx].tccr;
> +        retval = openpic_tmr_get_timer(&opp->timers[idx]);
>          break;
>      case 0x10: /* TBCR */
>          retval = opp->timers[idx].tbcr;
>          break;
> -    case 0x20: /* TIPV */
> +    case 0x20: /* TVPR */
>          retval = read_IRQreg_ivpr(opp, opp->irq_tim0 + idx);
>          break;
> -    case 0x30: /* TIDE (TIDR) */
> +    case 0x30: /* TDR */
>          retval = read_IRQreg_idr(opp, opp->irq_tim0 + idx);
>          break;
>      }
> @@ -1138,7 +1222,10 @@ static uint32_t openpic_iack(OpenPICState *opp, IRQDest *dst, int cpu)
>          IRQ_resetbit(&dst->raised, irq);
>      }
>  
> -    if ((irq >= opp->irq_ipi0) &&  (irq < (opp->irq_ipi0 + OPENPIC_MAX_IPI))) {
> +    /* Timers and IPIs support multicast. */
> +    if (((irq >= opp->irq_ipi0) && (irq < (opp->irq_ipi0 + OPENPIC_MAX_IPI))) ||
> +        ((irq >= opp->irq_tim0) && (irq < (opp->irq_tim0 + OPENPIC_MAX_TMR)))) {
> +        DPRINTF("irq is IPI or TMR\n");
>          src->destmask &= ~(1 << cpu);
>          if (src->destmask && !src->level) {
>              /* trigger on CPUs that didn't know about it yet */
> @@ -1343,6 +1430,10 @@ static void openpic_reset(DeviceState *d)
>      for (i = 0; i < OPENPIC_MAX_TMR; i++) {
>          opp->timers[i].tccr = 0;
>          opp->timers[i].tbcr = TBCR_CI;
> +        if (opp->timers[i].qemu_timer_active) {
> +            timer_del(opp->timers[i].qemu_timer);  /* Inhibit timer */
> +            opp->timers[i].qemu_timer_active = 0;
> +        }
>      }
>      /* Go out of RESET state */
>      opp->gcr = 0;
> @@ -1393,6 +1484,13 @@ static void fsl_common_init(OpenPICState *opp)
>          opp->src[i].type = IRQ_TYPE_FSLSPECIAL;
>          opp->src[i].level = false;
>      }
> +
> +    for (i = 0; i < OPENPIC_MAX_TMR; i++) {
> +        opp->timers[i].n_IRQ = opp->irq_tim0 + i;
> +        opp->timers[i].qemu_timer_active = 0;
> +        opp->timers[i].qemu_timer = 0;
> +        opp->timers[i].opp = opp;
> +    }
>  }
>  
>  static void map_list(OpenPICState *opp, const MemReg *list, int *count)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

David Gibson <david@gibson.dropbear.id.au> wrote on 04/23/2017 06:17:22 
PM:

> From: David Gibson <david@gibson.dropbear.id.au>
> To: Aaron Larson <alarson@ddci.com>
> Cc: agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org
> Date: 04/23/2017 06:54 PM
> Subject: Re: Subject: [PATCH] target-ppc: Add global timer group A to 
open-pic.
> 
> On Fri, Apr 21, 2017 at 02:58:23PM -0700, Aaron Larson wrote:
> > Add global timer group A to open-pic.  This patch is still somewhat
> > dubious because I'm not sure how to determine what QEMU wants for the
> > timer frequency.  Suggestions solicited.
> 
> This commit message really needs some more context.  What's a "global
> time group A" a and why do we want it?

The open-pic spec includes two sets/groups of timers, groups A and B,
4 timers in each group.  Previously in QEMU the timer group A
registers were implemented but they did not "count" or generate any
interrupts.  The patch makes the timers to do both (count and generate
interrupts).

About a year ago I mentioned that we had implemented this and offered
to submit a patch, which seemed to be acceptable.  Sadly, when I
reviewed the implementation it had several egregious errors that I
didn't know how to fix until recently.

Quite frankly I didn't expect the patch to be accepted in its current
form for the reason mentioned in the above commit log and was hoping
for some guidance.  If this is no longer a desirable patch, I can
certainly continue to maintain it locally for our use.\

Aaron.

On Sun, Apr 23, 2017 at 07:37:55PM -0500, alarson@ddci.com wrote:
> David Gibson <david@gibson.dropbear.id.au> wrote on 04/23/2017 06:17:22 
> PM:
> 
> > From: David Gibson <david@gibson.dropbear.id.au>
> > To: Aaron Larson <alarson@ddci.com>
> > Cc: agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org
> > Date: 04/23/2017 06:54 PM
> > Subject: Re: Subject: [PATCH] target-ppc: Add global timer group A to 
> open-pic.
> > 
> > On Fri, Apr 21, 2017 at 02:58:23PM -0700, Aaron Larson wrote:
> > > Add global timer group A to open-pic.  This patch is still somewhat
> > > dubious because I'm not sure how to determine what QEMU wants for the
> > > timer frequency.  Suggestions solicited.
> > 
> > This commit message really needs some more context.  What's a "global
> > time group A" a and why do we want it?
> 
> The open-pic spec includes two sets/groups of timers, groups A and B,
> 4 timers in each group.  Previously in QEMU the timer group A
> registers were implemented but they did not "count" or generate any
> interrupts.  The patch makes the timers to do both (count and generate
> interrupts).

Ok, sounds good, but this is exactly the sort of thing that needs to
go in the commit message.

> About a year ago I mentioned that we had implemented this and offered
> to submit a patch, which seemed to be acceptable.  Sadly, when I
> reviewed the implementation it had several egregious errors that I
> didn't know how to fix until recently.

Ok.  Now that you mention that, it rings a vague bell.

> Quite frankly I didn't expect the patch to be accepted in its current
> form for the reason mentioned in the above commit log and was hoping
> for some guidance.  If this is no longer a desirable patch, I can
> certainly continue to maintain it locally for our use.

That's fine - but I need context of what the patch is attempting to
do, and why that's desirable in order to properly review it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

On Fri, Apr 21, 2017 at 02:58:23PM -0700, Aaron Larson wrote:
> Add global timer group A to open-pic.  This patch is still somewhat
> dubious because I'm not sure how to determine what QEMU wants for the
> timer frequency.  Suggestions solicited.
> 
> Signed-off-by: Aaron Larson <alarson@ddci.com>
> ---
>  hw/intc/openpic.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 116 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index 4349e45..769a31a 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -45,6 +45,7 @@
>  #include "qemu/bitops.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qemu/log.h"
> +#include "qemu/timer.h"
>  
>  //#define DEBUG_OPENPIC
>  
> @@ -54,8 +55,10 @@ static const int debug_openpic = 1;
>  static const int debug_openpic = 0;
>  #endif
>  
> +static int get_current_cpu(void);
>  #define DPRINTF(fmt, ...) do { \
>          if (debug_openpic) { \
> +            printf("Core%d: ", get_current_cpu()); \
>              printf(fmt , ## __VA_ARGS__); \
>          } \
>      } while (0)
> @@ -246,9 +249,24 @@ typedef struct IRQSource {
>  #define IDR_EP      0x80000000  /* external pin */
>  #define IDR_CI      0x40000000  /* critical interrupt */
>  
> +/* Conversion between ticks and nanosecs: TODO: need better way for this.
> + Assume a 100mhz clock, divided by 8, or 25mhz
> + 25,000,000 ticks/sec, 25,000/ms, 25/us, 1 tick/40ns
> +*/

I'm not sure what you mean by "ticks" here.  Ticks of the openpic
clock itself?  Or something internal to qemu?

> +#define CONV_FACTOR 40LL
> +static inline uint64_t ns_to_ticks(uint64_t ns)   { return ns   / CONV_FACTOR; }
> +static inline uint64_t ticks_to_ns(uint64_t tick) { return tick * CONV_FACTOR; }
> +
>  typedef struct OpenPICTimer {
>      uint32_t tccr;  /* Global timer current count register */
>      uint32_t tbcr;  /* Global timer base count register */
> +    int                   n_IRQ;
> +    bool                  qemu_timer_active; /* Is the qemu_timer is running? */
> +    struct QEMUTimer     *qemu_timer;   /* May be 0 if not created. */

s/0/NULL/

> +    struct OpenPICState  *opp;          /* device timer is part of. */
> +    /* the time corresponding to the last current_count written or read,
> +       only defined if qemu_timer_active. */
> +    uint64_t              originTime;

time by what measure?

>  } OpenPICTimer;
>  
>  typedef struct OpenPICMSI {
> @@ -795,37 +813,102 @@ static uint64_t openpic_gbl_read(void *opaque, hwaddr addr, unsigned len)
>      return retval;
>  }
>  
> +static void openpic_tmr_set_tmr(OpenPICTimer *tmr, uint32_t val, bool enabled);
> +
> +static void qemu_timer_cb(void *opaque)
> +{
> +    OpenPICTimer *tmr = opaque;
> +    OpenPICState *opp = tmr->opp;
> +    uint32_t      n_IRQ = tmr->n_IRQ;
> +    uint32_t val =   tmr->tbcr & ~TBCR_CI;
> +    uint32_t tog = ((tmr->tccr & TCCR_TOG) ^ TCCR_TOG);  /* invert toggle. */
> +
> +    DPRINTF("%s n_IRQ=%d\n", __func__, n_IRQ);
> +    /* reload current count from base count and setup timer. */
> +    tmr->tccr = val | tog;
> +    openpic_tmr_set_tmr(tmr, val, /*enabled=*/true);
> +    /* raise the interrupt. */
> +    opp->src[n_IRQ].destmask = read_IRQreg_idr(opp, n_IRQ);
> +    openpic_set_irq(opp, n_IRQ, 1);
> +    openpic_set_irq(opp, n_IRQ, 0);
> +}
> +
> +/* If enabled is true, arranges for an interrupt to be raised val clocks into
> +   the future, if enabled is false cancels the timer. */
> +static void openpic_tmr_set_tmr(OpenPICTimer *tmr, uint32_t val, bool enabled)
> +{
> +    /* if timer doesn't exist, create it. */
> +    if (tmr->qemu_timer == 0) {

Please use == NULL, or just !tmr->qemu_timer

> +        tmr->qemu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &qemu_timer_cb, tmr);
> +        DPRINTF("Created timer for n_IRQ %d\n", tmr->n_IRQ);
> +    }
> +    uint64_t ns = ticks_to_ns(val & ~TCCR_TOG);
> +    /* A count of zero causes a timer to be set to expire immediately.  This
> +       effectively stops the simulation so we don't honor that configuration.
> +       On real hardware, this would generate an interrupt on every clock cycle
> +       if the interrupt was unmasked. */
> +    if ((ns == 0) || !enabled) {
> +        tmr->qemu_timer_active = false;
> +        tmr->tccr = tmr->tccr & TCCR_TOG;
> +        timer_del(tmr->qemu_timer); /* set timer to never expire. */
> +    } else {
> +        tmr->qemu_timer_active = true;
> +        uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        tmr->originTime = now;
> +        timer_mod(tmr->qemu_timer, now + ns);     /* set timer expiration. */
> +    }
> +}
> +
> +/* Returns the currrent tccr value, i.e., timer value (in clocks) with
> +   appropriate TOG. */
> +static uint64_t openpic_tmr_get_timer(OpenPICTimer *tmr)
> +{
> +    uint64_t retval;
> +    if (!tmr->qemu_timer_active) {
> +        retval = tmr->tccr;
> +    } else {
> +        uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        uint64_t used = now - tmr->originTime;  /* nsecs */
> +        uint32_t used_ticks = (uint32_t)ns_to_ticks(used);
> +        uint32_t count = (tmr->tccr & ~TCCR_TOG) - used_ticks;
> +        retval = (uint32_t)((tmr->tccr & TCCR_TOG) | (count & ~TCCR_TOG));
> +    }
> +    return retval;
> +}
> +
>  static void openpic_tmr_write(void *opaque, hwaddr addr, uint64_t val,
> -                                unsigned len)
> +                              unsigned len)
>  {
>      OpenPICState *opp = opaque;
>      int idx;
>  
> -    addr += 0x10f0;
> -
>      DPRINTF("%s: addr %#" HWADDR_PRIx " <= %08" PRIx64 "\n",
> -            __func__, addr, val);
> +            __func__, (addr + 0x10f0), val);
>      if (addr & 0xF) {
>          return;
>      }
>  
> -    if (addr == 0x10f0) {
> +    if (addr == 0) {
>          /* TFRR */
>          opp->tfrr = val;
>          return;
>      }
> -
> +    addr -= 0x10;  /* correct for TFRR */
>      idx = (addr >> 6) & 0x3;
> -    addr = addr & 0x30;
>  
>      switch (addr & 0x30) {
>      case 0x00: /* TCCR */
>          break;
>      case 0x10: /* TBCR */
> -        if ((opp->timers[idx].tccr & TCCR_TOG) != 0 &&
> -            (val & TBCR_CI) == 0 &&
> -            (opp->timers[idx].tbcr & TBCR_CI) != 0) {
> -            opp->timers[idx].tccr &= ~TCCR_TOG;
> +        /* Did the enable status change? */
> +        if ((opp->timers[idx].tbcr & TBCR_CI) != (val & TBCR_CI)) {
> +            /* Did "Count Inhibit" transition from 1 to 0? */
> +            if ((val & TBCR_CI) == 0) {
> +                opp->timers[idx].tccr = val & ~TCCR_TOG;
> +            }
> +            openpic_tmr_set_tmr(&opp->timers[idx],
> +                                (val & ~TBCR_CI),
> +                                /*enabled=*/((val & TBCR_CI) == 0));
>          }
>          opp->timers[idx].tbcr = val;
>          break;
> @@ -844,27 +927,28 @@ static uint64_t openpic_tmr_read(void *opaque, hwaddr addr, unsigned len)
>      uint32_t retval = -1;
>      int idx;
>  
> -    DPRINTF("%s: addr %#" HWADDR_PRIx "\n", __func__, addr);
> +    DPRINTF("%s: addr %#" HWADDR_PRIx "\n", __func__, addr + 0x10f0);
>      if (addr & 0xF) {
>          goto out;
>      }
> -    idx = (addr >> 6) & 0x3;
> -    if (addr == 0x0) {
> +    if (addr == 0) {
>          /* TFRR */
>          retval = opp->tfrr;
>          goto out;
>      }
> +    addr -= 0x10;  /* correct for TFRR */
> +    idx = (addr >> 6) & 0x3;
>      switch (addr & 0x30) {
>      case 0x00: /* TCCR */
> -        retval = opp->timers[idx].tccr;
> +        retval = openpic_tmr_get_timer(&opp->timers[idx]);
>          break;
>      case 0x10: /* TBCR */
>          retval = opp->timers[idx].tbcr;
>          break;
> -    case 0x20: /* TIPV */
> +    case 0x20: /* TVPR */
>          retval = read_IRQreg_ivpr(opp, opp->irq_tim0 + idx);
>          break;
> -    case 0x30: /* TIDE (TIDR) */
> +    case 0x30: /* TDR */
>          retval = read_IRQreg_idr(opp, opp->irq_tim0 + idx);
>          break;
>      }
> @@ -1138,7 +1222,10 @@ static uint32_t openpic_iack(OpenPICState *opp, IRQDest *dst, int cpu)
>          IRQ_resetbit(&dst->raised, irq);
>      }
>  
> -    if ((irq >= opp->irq_ipi0) &&  (irq < (opp->irq_ipi0 + OPENPIC_MAX_IPI))) {
> +    /* Timers and IPIs support multicast. */
> +    if (((irq >= opp->irq_ipi0) && (irq < (opp->irq_ipi0 + OPENPIC_MAX_IPI))) ||
> +        ((irq >= opp->irq_tim0) && (irq < (opp->irq_tim0 + OPENPIC_MAX_TMR)))) {
> +        DPRINTF("irq is IPI or TMR\n");
>          src->destmask &= ~(1 << cpu);
>          if (src->destmask && !src->level) {
>              /* trigger on CPUs that didn't know about it yet */
> @@ -1343,6 +1430,10 @@ static void openpic_reset(DeviceState *d)
>      for (i = 0; i < OPENPIC_MAX_TMR; i++) {
>          opp->timers[i].tccr = 0;
>          opp->timers[i].tbcr = TBCR_CI;
> +        if (opp->timers[i].qemu_timer_active) {
> +            timer_del(opp->timers[i].qemu_timer);  /* Inhibit timer */
> +            opp->timers[i].qemu_timer_active = 0;
> +        }
>      }
>      /* Go out of RESET state */
>      opp->gcr = 0;
> @@ -1393,6 +1484,13 @@ static void fsl_common_init(OpenPICState *opp)
>          opp->src[i].type = IRQ_TYPE_FSLSPECIAL;
>          opp->src[i].level = false;
>      }
> +
> +    for (i = 0; i < OPENPIC_MAX_TMR; i++) {
> +        opp->timers[i].n_IRQ = opp->irq_tim0 + i;
> +        opp->timers[i].qemu_timer_active = 0;
> +        opp->timers[i].qemu_timer = 0;
> +        opp->timers[i].opp = opp;
> +    }
>  }
>  
>  static void map_list(OpenPICState *opp, const MemReg *list, int *count)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

David Gibson <david@gibson.dropbear.id.au> wrote on 04/25/2017 11:34:41 
PM:

Thanks for the review David.  I'll send an updated patch soon with all
your comments addressed and all other s/0/NULL or false/ as
appropriate and a better commit message.

Just to clarify my concern; the patch causes the QEMU openpic timer to
be hard coded at 100MHZ.  This is common, but the openpic spec has
numerous ways the clock could be driven and the actual frequency
depends on target board configuration.  Given that no other users are
apparently using the QEMU openpic timer, it is probably ok, but I did
want to point out the deficiency.

On Tue, May 02, 2017 at 08:37:09PM -0500, alarson@ddci.com wrote:
> David Gibson <david@gibson.dropbear.id.au> wrote on 04/25/2017 11:34:41 
> PM:
> 
> Thanks for the review David.  I'll send an updated patch soon with all
> your comments addressed and all other s/0/NULL or false/ as
> appropriate and a better commit message.
> 
> Just to clarify my concern; the patch causes the QEMU openpic timer to
> be hard coded at 100MHZ.  This is common, but the openpic spec has
> numerous ways the clock could be driven and the actual frequency
> depends on target board configuration.  Given that no other users are
> apparently using the QEMU openpic timer, it is probably ok, but I did
> want to point out the deficiency.

Ah, ok.  The usual way to handle this is to have the frequency as a
property on the device.  The board code which constructs the openPIC
value can then set it to the appropriate value.

Having an interim step where its hardcoded to 100 MHz is perfectly ok
though.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson