Previous QEMU open-pic implemented the 4 open-pic timers including all
timer registers, but the timers did not "count" or generate any
interrupts.  The patch makes the timers both count and generate
interrupts.  The timer clock frequency is fixed at 100MHZ.

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

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 4349e45..e0556f1 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,25 @@ typedef struct IRQSource {
 #define IDR_EP      0x80000000  /* external pin */
 #define IDR_CI      0x40000000  /* critical interrupt */
 
+/* Conversion between openpic clock ticks and nanosecs.  Ideally this clock
+   frequency would follow the openpic spec, for now hard code to 100mz.
+   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 NULL if not created. */
+    struct OpenPICState  *opp;          /* Device timer is part of. */
+    /* The QEMU_CLOCK_VIRTUAL time (in ns) corresponding to the last
+       current_count written or read, only defined if qemu_timer_active. */
+    uint64_t              originTime;
 } OpenPICTimer;
 
 typedef struct OpenPICMSI {
@@ -795,37 +814,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 == NULL) {
+        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 +928,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 +1223,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 +1431,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 = false;
+        }
     }
     /* Go out of RESET state */
     opp->gcr = 0;
@@ -1393,6 +1485,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 = false;
+        opp->timers[i].qemu_timer = NULL;
+        opp->timers[i].opp = opp;
+    }
 }
 
 static void map_list(OpenPICState *opp, const MemReg *list, int *count)
-- 
2.7.4

On Tue, May 02, 2017 at 07:57:22PM -0700, Aaron Larson wrote:
> 
> Previous QEMU open-pic implemented the 4 open-pic timers including all
> timer registers, but the timers did not "count" or generate any
> interrupts.  The patch makes the timers both count and generate
> interrupts.  The timer clock frequency is fixed at 100MHZ.
> 
> Signed-off-by: Aaron Larson <alarson@ddci.com>

Looks sound in concept AFAICT not knowing the openpic hardware.

> ---
>  hw/intc/openpic.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 117 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index 4349e45..e0556f1 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,25 @@ typedef struct IRQSource {
>  #define IDR_EP      0x80000000  /* external pin */
>  #define IDR_CI      0x40000000  /* critical interrupt */
>  
> +/* Conversion between openpic clock ticks and nanosecs.  Ideally this clock
> +   frequency would follow the openpic spec, for now hard code to 100mz.
> +   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; }

This is a little hard to follow.  Where does the divide by 8 come
from?  Also 100MHz / 8 is 12.5 MHz, not 25MHz..

I'd prefer logic that comes from an explicit clock frequency
value, even if that's a constant 100000000 for now.

>  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 NULL if not created. */
> +    struct OpenPICState  *opp;          /* Device timer is part of. */
> +    /* The QEMU_CLOCK_VIRTUAL time (in ns) corresponding to the last
> +       current_count written or read, only defined if qemu_timer_active. */
> +    uint64_t              originTime;

qemu doesn't generally use camelCase for structure fields.  I'd
consider an exception if the name 'originTime' appears exactly like
that in the documentation, otherwise not.

>  } OpenPICTimer;
>  
>  typedef struct OpenPICMSI {
> @@ -795,37 +814,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 == NULL) {
> +        tmr->qemu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &qemu_timer_cb, tmr);
> +        DPRINTF("Created timer for n_IRQ %d\n", tmr->n_IRQ);

Is there a reason to lazily create the timer, rather than always
creating it at init time and just activating it when the timer is set?

> +    }
> +    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. */

Could you also jam up if the count is non-zero but a too-small value
to make forward progress?  It's probably worth doing an error_report()
in this case too, so the user has some idea what's wrong.

> +    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) {

I don't really understand how this change fits in with the rest - it
appears to be changing existing unrelated behaviour.

>          /* 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 +928,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 +1223,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 +1431,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 = false;
> +        }
>      }
>      /* Go out of RESET state */
>      opp->gcr = 0;
> @@ -1393,6 +1485,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 = false;
> +        opp->timers[i].qemu_timer = NULL;
> +        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 05/12/2017 01:52:04 
AM:

> 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: 05/12/2017 01:52 AM
> Subject: Re: [PATCH v2] target-ppc: Enable open-pic timers to count and 
generate interrupts
> 
> On Tue, May 02, 2017 at 07:57:22PM -0700, Aaron Larson wrote:
> > 
> > Previous QEMU open-pic implemented the 4 open-pic timers including all
> > timer registers, but the timers did not "count" or generate any
> > interrupts.  The patch makes the timers both count and generate
> > interrupts.  The timer clock frequency is fixed at 100MHZ.
> > 
> > Signed-off-by: Aaron Larson <alarson@ddci.com>
> 
> Looks sound in concept AFAICT not knowing the openpic hardware.

Thanks again for your review.  I will provide an updated patch to
address all of the comments, but I need a bit more guidance on some.

> > ---
> >  hw/intc/openpic.c | 135 
++++++++++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 117 insertions(+), 18 deletions(-)
> > 

> > +/* 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 == NULL) {
> > +        tmr->qemu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, 
&qemu_timer_cb, tmr);
> > +        DPRINTF("Created timer for n_IRQ %d\n", tmr->n_IRQ);
> 
> Is there a reason to lazily create the timer, rather than always
> creating it at init time and just activating it when the timer is set?

I'm not familiar with the QEMU timer code so guidance is appreciated,
but a quick check indicates there wouldn't be much overhead to do as
you suggest.  I will change to that approach.

> > +    }
> > +    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. */
> 
> Could you also jam up if the count is non-zero but a too-small value
> to make forward progress?  ...

The case I'm concerned about is where a transient value is programmed
to the timer and the interrupt is masked.  In that case QEMU will fire
the timer on (potentially) every other clock, which will slow the
emulation down until a more sane value is programmed.  I could add
code to inhibit the timer while the associated interrupt is masked,
but that is messy, and this seems like an unlikely corner case.

Let me know how you'd like this handled.

> >  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) {
> 
> I don't really understand how this change fits in with the rest - it
> appears to be changing existing unrelated behaviour.

I debated on changing this.  I needed to make changes to both the
timer read and write code, and the existing code was inconsistent on
the treatment of the offset.  The open-pic has a standard memory map
and the timer block starts at 0x10f0 from the BAR.  Of course the
region in QEMU for the timer is setup such that the timer is at offset
zero in the QEMU timer memory region.  The write code added the offset
to match the hardware, the read code did not, and consequently the
code I added for timer read and write needed to be gratuitously
different because of that.  I chose to update the write to match the
read.  I can undo the change, if you'd like, but it does make the
resulting code harder to understand (IMHO).

On Fri, May 12, 2017 at 09:34:33AM -0500, alarson@ddci.com wrote:
> David Gibson <david@gibson.dropbear.id.au> wrote on 05/12/2017 01:52:04 
> AM:
> 
> > 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: 05/12/2017 01:52 AM
> > Subject: Re: [PATCH v2] target-ppc: Enable open-pic timers to count and 
> generate interrupts
> > 
> > On Tue, May 02, 2017 at 07:57:22PM -0700, Aaron Larson wrote:
> > > 
> > > Previous QEMU open-pic implemented the 4 open-pic timers including all
> > > timer registers, but the timers did not "count" or generate any
> > > interrupts.  The patch makes the timers both count and generate
> > > interrupts.  The timer clock frequency is fixed at 100MHZ.
> > > 
> > > Signed-off-by: Aaron Larson <alarson@ddci.com>
> > 
> > Looks sound in concept AFAICT not knowing the openpic hardware.
> 
> Thanks again for your review.  I will provide an updated patch to
> address all of the comments, but I need a bit more guidance on some.
> 
> > > ---
> > >  hw/intc/openpic.c | 135 
> ++++++++++++++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 117 insertions(+), 18 deletions(-)
> > > 
> 
> > > +/* 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 == NULL) {
> > > +        tmr->qemu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, 
> &qemu_timer_cb, tmr);
> > > +        DPRINTF("Created timer for n_IRQ %d\n", tmr->n_IRQ);
> > 
> > Is there a reason to lazily create the timer, rather than always
> > creating it at init time and just activating it when the timer is set?
> 
> I'm not familiar with the QEMU timer code so guidance is appreciated,
> but a quick check indicates there wouldn't be much overhead to do as
> you suggest.  I will change to that approach.

Ok.  I'm not an expert on the timers either, so I'll trust your
judgement.  In general I'd suggest simplicity unless there's an
observed performance problem, which would also suggest the early
initialization.

> > > +    }
> > > +    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. */
> > 
> > Could you also jam up if the count is non-zero but a too-small value
> > to make forward progress?  ...
> 
> The case I'm concerned about is where a transient value is programmed
> to the timer and the interrupt is masked.  In that case QEMU will fire
> the timer on (potentially) every other clock,

Erm.. TCG doesn't really have "clocks" as such, so I'm not entirely
sure what you mean here.

> which will slow the
> emulation down until a more sane value is programmed.  I could add
> code to inhibit the timer while the associated interrupt is masked,
> but that is messy, and this seems like an unlikely corner case.

I concur.

> Let me know how you'd like this handled.
> 
> > >  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) {
> > 
> > I don't really understand how this change fits in with the rest - it
> > appears to be changing existing unrelated behaviour.
> 
> I debated on changing this.  I needed to make changes to both the
> timer read and write code, and the existing code was inconsistent on
> the treatment of the offset.  The open-pic has a standard memory map
> and the timer block starts at 0x10f0 from the BAR.  Of course the
> region in QEMU for the timer is setup such that the timer is at offset
> zero in the QEMU timer memory region.  The write code added the offset
> to match the hardware, the read code did not, and consequently the
> code I added for timer read and write needed to be gratuitously
> different because of that.  I chose to update the write to match the
> read.  I can undo the change, if you'd like, but it does make the
> resulting code harder to understand (IMHO).

So, making the read/write functions use consistent addressing sounds
like a good idea.  But I think it would be clearer to do this as a
preliminary patch, rather than folded in with adding the timers
implementation.

-- 
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 05/13/2017 02:59:16 
AM:

> From: David Gibson <david@gibson.dropbear.id.au>
> On Fri, May 12, 2017 at 09:34:33AM -0500, alarson@ddci.com wrote:
> > David Gibson <david@gibson.dropbear.id.au> wrote on 05/12/2017 
01:52:04 

Sorry for the long delay getting back to you.  I was out of town for a
while.

> > > > +    }
> > > > +    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. */
> > > 
> > > Could you also jam up if the count is non-zero but a too-small value
> > > to make forward progress?  ...
> > 
> > The case I'm concerned about is where a transient value is programmed
> > to the timer and the interrupt is masked.  In that case QEMU will fire
> > the timer on (potentially) every other clock,
> 
> Erm.. TCG doesn't really have "clocks" as such, so I'm not entirely
> sure what you mean here.

Hopefully its just terminology.  QEMU has QEMU_CLOCK_VIRTUAL which
drives a timer.  My understanding is that the top level loop in QEMU
is effectively:

  while 1
    if timer expired then run the handler
    else execute some simulated code

If a QEMU timer handler reprograms the timer to expire at "now+0ns",
then no code is ever executed because the VIRTUAL CLOCK timer is
always pending.  This is not exactly the same as what would happen on
real hardware because on hardware the interrupt would be asserted, but
if it was masked it would have no effect.  In the simulation such a
situation effectively results in a "hang".

> > > >  static void openpic_tmr_write(void *opaque, hwaddr addr, uint64_t 

> > > > ...
> > > > -    if (addr == 0x10f0) {
> > > > +    if (addr == 0) {
> > > 
> > > I don't really understand how this change fits in with the rest - it
> > > appears to be changing existing unrelated behaviour.
> > 
> > I debated on changing this.  I needed to make changes to both the
> > timer read and write code, and the existing code was inconsistent on
> > the treatment of the offset.  The open-pic has a standard memory map
> > and the timer block starts at 0x10f0 from the BAR.  Of course the
> > region in QEMU for the timer is setup such that the timer is at offset
> > zero in the QEMU timer memory region.  The write code added the offset
> > to match the hardware, the read code did not, and consequently the
> > code I added for timer read and write needed to be gratuitously
> > different because of that.  I chose to update the write to match the
> > read.  I can undo the change, if you'd like, but it does make the
> > resulting code harder to understand (IMHO).
> 
> So, making the read/write functions use consistent addressing sounds
> like a good idea.  But I think it would be clearer to do this as a
> preliminary patch, rather than folded in with adding the timers
> implementation.

Will do.  Patch to follow shortly.  I assume that once the preliminary
patch is approved, I should submit a follow up patch with the addition
of the timer.  BTW, I forgot to mention that the previous timer read
code was not only inconsistent, but was in error.  Bad enough in error
that I doubt anyone could have been using the code.