[Qemu-devel] [PATCH v3] target-ppc: Enable open-pic timers to count and generate interrupts

[Qemu-devel] [PATCH v3] target-ppc: Enable open-pic timers to count and generate interrupts

[Aaron Larson]

Previously 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 25MHZ.

--

Responding to V2 patch comments.
- Simplify clock frequency logic and commentary.
- Remove camelCase variables.
- Timer objects now created at init rather than lazily.

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

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index f966d06..0dec51c 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,31 @@ typedef struct IRQSource {
 #define IDR_EP      0x80000000  /* external pin */
 #define IDR_CI      0x40000000  /* critical interrupt */
 
+/* Convert between openpic clock ticks and nanosecs.  In the hardware the clock
+   frequency is driven by board inputs to the PIC which the PIC would then
+   divide by 4 or 8.  For now hard code to 25MZ.
+*/
+#define OPENPIC_TIMER_FREQ_MHZ 25
+#define OPENPIC_TIMER_NS_PER_TICK (1000 / OPENPIC_TIMER_FREQ_MHZ)
+static inline uint64_t ns_to_ticks(uint64_t ns)
+{
+    return ns    / OPENPIC_TIMER_NS_PER_TICK;
+}
+static inline uint64_t ticks_to_ns(uint64_t ticks)
+{
+    return ticks * OPENPIC_TIMER_NS_PER_TICK;
+}
+
 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;
+    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              origin_time;
 } OpenPICTimer;
 
 typedef struct OpenPICMSI {
@@ -795,6 +820,65 @@ 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)
+{
+    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 since the timer is constantly expiring
+       which prevents guest code execution, so we don't honor that
+       configuration.  On real hardware, this situation 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->origin_time = 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->origin_time;  /* 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)
 {
@@ -819,10 +903,15 @@ static void openpic_tmr_write(void *opaque, hwaddr addr, uint64_t val,
     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;
@@ -854,7 +943,7 @@ static uint64_t openpic_tmr_read(void *opaque, hwaddr addr, unsigned len)
     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;
@@ -1136,7 +1225,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 */
@@ -1341,6 +1433,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;
@@ -1391,6 +1487,15 @@ 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 = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                                 &qemu_timer_cb,
+                                                 &opp->timers[i]);
+        opp->timers[i].opp = opp;
+    }
 }
 
 static void map_list(OpenPICState *opp, const MemReg *list, int *count)
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3] target-ppc: Enable open-pic timers to count and generate interrupts

[alarson]

Aaron Larson <alarson@ddci.com> wrote on 06/05/2017 12:22:53 PM:

> From: Aaron Larson <alarson@ddci.com>
> To: agraf@suse.de, alarson@ddci.com, david@gibson.dropbear.id.au, 
qemu-devel@nongnu.org, qemu-ppc@nongnu.org
> Date: 06/05/2017 12:22 PM
> Subject: [PATCH v3] target-ppc: Enable open-pic timers to count and 
generate interrupts
> 
> Previously 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 25MHZ.
> 
> --
> 
> Responding to V2 patch comments.
> - Simplify clock frequency logic and commentary.
> - Remove camelCase variables.
> - Timer objects now created at init rather than lazily.
> 
> Signed-off-by: Aaron Larson <alarson@ddci.com>


I haven't received any feedback on this patch, and I don't see a response 
on the mailing list archive.  Did it get lost?

Re: [Qemu-devel] [PATCH v3] target-ppc: Enable open-pic timers to count and generate interrupts

[David Gibson]

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

On Fri, Jun 16, 2017 at 11:31:02AM -0500, alarson@ddci.com wrote:
> Aaron Larson <alarson@ddci.com> wrote on 06/05/2017 12:22:53 PM:
> 
> > From: Aaron Larson <alarson@ddci.com>
> > To: agraf@suse.de, alarson@ddci.com, david@gibson.dropbear.id.au, 
> qemu-devel@nongnu.org, qemu-ppc@nongnu.org
> > Date: 06/05/2017 12:22 PM
> > Subject: [PATCH v3] target-ppc: Enable open-pic timers to count and 
> generate interrupts
> > 
> > Previously 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 25MHZ.
> 
> 
> I haven't received any feedback on this patch, and I don't see a response 
> on the mailing list archive.  Did it get lost?

No, I've just been busy and then sick.  I'll get to it eventually..

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v3] target-ppc: Enable open-pic timers to count and generate interrupts

[David Gibson]

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

On Mon, Jun 05, 2017 at 10:22:53AM -0700, Aaron Larson wrote:
> Previously 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 25MHZ.

Applied to ppc-for-2.10.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v3] target-ppc: Enable open-pic timers to count and generate interrupts

[G 3]

On Jun 18, 2017, at 6:50 AM, qemu-devel-request@nongnu.org wrote:
> On Fri, Jun 16, 2017 at 11:31:02AM -0500, alarson@ddci.com wrote:
>> Aaron Larson <alarson@ddci.com> wrote on 06/05/2017 12:22:53 PM:
>>
>>> From: Aaron Larson <alarson@ddci.com>
>>> To: agraf@suse.de, alarson@ddci.com, david@gibson.dropbear.id.au,
>> qemu-devel@nongnu.org, qemu-ppc@nongnu.org
>>> Date: 06/05/2017 12:22 PM
>>> Subject: [PATCH v3] target-ppc: Enable open-pic timers to count and
>> generate interrupts
>>>
>>> Previously 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 25MHZ.
>>
>>
>> I haven't received any feedback on this patch, and I don't see a  
>> response
>> on the mailing list archive.  Did it get lost?

Does QEMU behave differently after this patch is implemented? 

Re: [Qemu-devel] [PATCH v3] target-ppc: Enable open-pic timers to count and generate interrupts

[alarson]

G 3 <programmingkidx@gmail.com> wrote on 06/18/2017 09:45:25 AM:

> >>> From: Aaron Larson <alarson@ddci.com>
> >>> To: agraf@suse.de, alarson@ddci.com, david@gibson.dropbear.id.au,
> >> qemu-devel@nongnu.org, qemu-ppc@nongnu.org
> >>> Date: 06/05/2017 12:22 PM
> >>> Subject: [PATCH v3] target-ppc: Enable open-pic timers to count and
> >> generate interrupts
> >>>
> >>> Previously 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 25MHZ.

G3> Does QEMU behave differently after this patch is implemented? 

If the open-pic timers are programmed, then QEMU behaves
differently, otherwise there should be no difference.

Re: [Qemu-devel] [PATCH v3] target-ppc: Enable open-pic timers to count and generate interrupts

[G 3]

On Jun 18, 2017, at 4:36 PM, alarson@ddci.com wrote:

> G 3 <programmingkidx@gmail.com> wrote on 06/18/2017 09:45:25 AM:
>
>>>>> From: Aaron Larson <alarson@ddci.com>
>>>>> To: agraf@suse.de, alarson@ddci.com, david@gibson.dropbear.id.au,
>>>> qemu-devel@nongnu.org, qemu-ppc@nongnu.org
>>>>> Date: 06/05/2017 12:22 PM
>>>>> Subject: [PATCH v3] target-ppc: Enable open-pic timers to count  
>>>>> and
>>>> generate interrupts
>>>>>
>>>>> Previously 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 25MHZ.
>
> G3> Does QEMU behave differently after this patch is implemented?
>
> If the open-pic timers are programmed, then QEMU behaves
> differently, otherwise there should be no difference.

I guess I should have asked what differences did you see in QEMU's  
behavior?

Re: [Qemu-devel] [PATCH v3] target-ppc: Enable open-pic timers to count and generate interrupts

[alarson]

G 3 <programmingkidx@gmail.com> wrote on 06/18/2017 03:38:29 PM:
> >>>>> From: Aaron Larson <alarson@ddci.com>
> >>>>> Date: 06/05/2017 12:22 PM
> >>>>> Subject: [PATCH v3] target-ppc: Enable open-pic timers to count 
> >>>>> and generate interrupts
> >>>>>
> >>>>> Previously 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 25MHZ.
> >
> > G3> Does QEMU behave differently after this patch is implemented?
> >
> > If the open-pic timers are programmed, then QEMU behaves
> > differently, otherwise there should be no difference.
> 
> I guess I should have asked what differences did you see in QEMU's 
> behavior?

In the past, the open-pic timers were, effectively, unimplemented.
They allowed a write to the timer registers, e.g., to enable a
counter, but the counter never "counted", instead just returning
whatever was most recently programmed.

Now the timers are, well, timers.  Given the previous implementation,
it was highly unlikely any software running on QEMU actually used the
timers, so in general I would expect there would be no difference at
all.

Re: [Qemu-devel] [PATCH v3] target-ppc: Enable open-pic timers to count and generate interrupts

[alarson]

David Gibson <david@gibson.dropbear.id.au> wrote on 06/18/2017 04:04:48 
AM:
> On Fri, Jun 16, 2017 at 11:31:02AM -0500, alarson@ddci.com wrote:

> > I haven't received any feedback on this patch, ...  Did it get lost?
> 
> No, I've just been busy and then sick.  I'll get to it eventually..

As always, thanks for your efforts.  You have been so responsive in the
past I figured something was up.  I hope you're feeling better.

If karma is any consolation, I was just going through my QEMU "todo" list
and found that I neglected to submit a follow up patch to an email
conversation from last July.  Ouch.  I'll get that in the mail ASAP.

http://lists.nongnu.org/archive/html/qemu-ppc/2016-07/msg00916.html