Patchwork [06/15] openpic: rework critical interrupt support

login
register
mail settings
Submitter Scott Wood
Date Dec. 22, 2012, 2:15 a.m.
Message ID <1356142552-13453-7-git-send-email-scottwood@freescale.com>
Download mbox | patch
Permalink /patch/207912/
State New
Headers show

Comments

Scott Wood - Dec. 22, 2012, 2:15 a.m.
Critical interrupts on FSL MPIC are not supposed to pay
attention to priority, IACK, EOI, etc.  On the currently modeled
version it's not supposed to pay attention to the mask bit either.

Also reorganize to make it easier to implement newer FSL MPIC models,
which encode interrupt level information differently and support
mcheck as well as crit, and to reduce problems for later patches
in this set.

Still missing is the ability to lower the CINT signal to the core,
as IACK/EOI is not used.  This will come with general IRQ-source-driven
lowering in the next patch.

New state is added which is not serialized, but instead is recomputed
in openpic_load() by calling the appropriate write_IRQreg function.
This should have the side effect of causing the IRQ outputs to be
raised appropriately on load, which was missing.

The serialization format is altered by swapping ivpr and idr (we'd like
IDR to be restored before we run the IVPR logic), and moving interrupts
to the end (so that other state has been restored by the time we run the
IDR/IVPR logic.  Serialization for this driver is not yet in a state
where backwards compatibility is reasonable (assuming it works at all),
and the current serialization format was not built for extensibility.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 hw/openpic.c |  110 ++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 76 insertions(+), 34 deletions(-)
Alexander Graf - Jan. 3, 2013, 6:31 p.m.
On 22.12.2012, at 03:15, Scott Wood wrote:

> Critical interrupts on FSL MPIC are not supposed to pay
> attention to priority, IACK, EOI, etc.  On the currently modeled
> version it's not supposed to pay attention to the mask bit either.
> 
> Also reorganize to make it easier to implement newer FSL MPIC models,
> which encode interrupt level information differently and support
> mcheck as well as crit, and to reduce problems for later patches
> in this set.
> 
> Still missing is the ability to lower the CINT signal to the core,
> as IACK/EOI is not used.  This will come with general IRQ-source-driven
> lowering in the next patch.
> 
> New state is added which is not serialized, but instead is recomputed
> in openpic_load() by calling the appropriate write_IRQreg function.
> This should have the side effect of causing the IRQ outputs to be
> raised appropriately on load, which was missing.
> 
> The serialization format is altered by swapping ivpr and idr (we'd like
> IDR to be restored before we run the IVPR logic), and moving interrupts
> to the end (so that other state has been restored by the time we run the
> IDR/IVPR logic.  Serialization for this driver is not yet in a state
> where backwards compatibility is reasonable (assuming it works at all),
> and the current serialization format was not built for extensibility.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>

Thanks, applied to ppc-next (with adjustments).

Alex
Scott Wood - Jan. 3, 2013, 11:07 p.m.
On 01/03/2013 12:31:47 PM, Alexander Graf wrote:
> 
> On 22.12.2012, at 03:15, Scott Wood wrote:
> 
> > Critical interrupts on FSL MPIC are not supposed to pay
> > attention to priority, IACK, EOI, etc.  On the currently modeled
> > version it's not supposed to pay attention to the mask bit either.
> >
> > Also reorganize to make it easier to implement newer FSL MPIC  
> models,
> > which encode interrupt level information differently and support
> > mcheck as well as crit, and to reduce problems for later patches
> > in this set.
> >
> > Still missing is the ability to lower the CINT signal to the core,
> > as IACK/EOI is not used.  This will come with general  
> IRQ-source-driven
> > lowering in the next patch.
> >
> > New state is added which is not serialized, but instead is  
> recomputed
> > in openpic_load() by calling the appropriate write_IRQreg function.
> > This should have the side effect of causing the IRQ outputs to be
> > raised appropriately on load, which was missing.
> >
> > The serialization format is altered by swapping ivpr and idr (we'd  
> like
> > IDR to be restored before we run the IVPR logic), and moving  
> interrupts
> > to the end (so that other state has been restored by the time we  
> run the
> > IDR/IVPR logic.  Serialization for this driver is not yet in a state
> > where backwards compatibility is reasonable (assuming it works at  
> all),
> > and the current serialization format was not built for  
> extensibility.
> >
> > Signed-off-by: Scott Wood <scottwood@freescale.com>
> 
> Thanks, applied to ppc-next (with adjustments).

It looks like one of the adjustments was wrapping an error string --  
does QEMU not follow the Linux rule of not wrapping error strings?  If  
not, maybe that should change.

QEMU's checkpatch appears to retain the line-length exception, but the  
set of log functions hasn't been updated (not even to include printf)...

-Scott
Alexander Graf - Jan. 4, 2013, 8:04 a.m.
On 04.01.2013, at 00:07, Scott Wood wrote:

> On 01/03/2013 12:31:47 PM, Alexander Graf wrote:
>> On 22.12.2012, at 03:15, Scott Wood wrote:
>> > Critical interrupts on FSL MPIC are not supposed to pay
>> > attention to priority, IACK, EOI, etc.  On the currently modeled
>> > version it's not supposed to pay attention to the mask bit either.
>> >
>> > Also reorganize to make it easier to implement newer FSL MPIC models,
>> > which encode interrupt level information differently and support
>> > mcheck as well as crit, and to reduce problems for later patches
>> > in this set.
>> >
>> > Still missing is the ability to lower the CINT signal to the core,
>> > as IACK/EOI is not used.  This will come with general IRQ-source-driven
>> > lowering in the next patch.
>> >
>> > New state is added which is not serialized, but instead is recomputed
>> > in openpic_load() by calling the appropriate write_IRQreg function.
>> > This should have the side effect of causing the IRQ outputs to be
>> > raised appropriately on load, which was missing.
>> >
>> > The serialization format is altered by swapping ivpr and idr (we'd like
>> > IDR to be restored before we run the IVPR logic), and moving interrupts
>> > to the end (so that other state has been restored by the time we run the
>> > IDR/IVPR logic.  Serialization for this driver is not yet in a state
>> > where backwards compatibility is reasonable (assuming it works at all),
>> > and the current serialization format was not built for extensibility.
>> >
>> > Signed-off-by: Scott Wood <scottwood@freescale.com>
>> Thanks, applied to ppc-next (with adjustments).
> 
> It looks like one of the adjustments was wrapping an error string -- does QEMU not follow the Linux rule of not wrapping error strings?  If not, maybe that should change.

At least not that I'm aware of :).

> QEMU's checkpatch appears to retain the line-length exception, but the set of log functions hasn't been updated (not even to include printf)...

Yeah, checkpatch was simply copied from Linux. I suppose nobody adjusted that bit.


Alex
Blue Swirl - Jan. 4, 2013, 8:46 p.m.
On Thu, Jan 3, 2013 at 11:07 PM, Scott Wood <scottwood@freescale.com> wrote:
> On 01/03/2013 12:31:47 PM, Alexander Graf wrote:
>>
>>
>> On 22.12.2012, at 03:15, Scott Wood wrote:
>>
>> > Critical interrupts on FSL MPIC are not supposed to pay
>> > attention to priority, IACK, EOI, etc.  On the currently modeled
>> > version it's not supposed to pay attention to the mask bit either.
>> >
>> > Also reorganize to make it easier to implement newer FSL MPIC models,
>> > which encode interrupt level information differently and support
>> > mcheck as well as crit, and to reduce problems for later patches
>> > in this set.
>> >
>> > Still missing is the ability to lower the CINT signal to the core,
>> > as IACK/EOI is not used.  This will come with general IRQ-source-driven
>> > lowering in the next patch.
>> >
>> > New state is added which is not serialized, but instead is recomputed
>> > in openpic_load() by calling the appropriate write_IRQreg function.
>> > This should have the side effect of causing the IRQ outputs to be
>> > raised appropriately on load, which was missing.
>> >
>> > The serialization format is altered by swapping ivpr and idr (we'd like
>> > IDR to be restored before we run the IVPR logic), and moving interrupts
>> > to the end (so that other state has been restored by the time we run the
>> > IDR/IVPR logic.  Serialization for this driver is not yet in a state
>> > where backwards compatibility is reasonable (assuming it works at all),
>> > and the current serialization format was not built for extensibility.
>> >
>> > Signed-off-by: Scott Wood <scottwood@freescale.com>
>>
>> Thanks, applied to ppc-next (with adjustments).
>
>
> It looks like one of the adjustments was wrapping an error string -- does
> QEMU not follow the Linux rule of not wrapping error strings?  If not, maybe
> that should change.

There was some discussion earlier and IIRC the outcome was that error
strings should contain unique IDs to make them greppable.

>
> QEMU's checkpatch appears to retain the line-length exception, but the set
> of log functions hasn't been updated (not even to include printf)...

The exception should be removed.

>
> -Scott
>
Scott Wood - Jan. 4, 2013, 8:49 p.m.
On 01/04/2013 02:46:00 PM, Blue Swirl wrote:
> On Thu, Jan 3, 2013 at 11:07 PM, Scott Wood <scottwood@freescale.com>  
> wrote:
> > It looks like one of the adjustments was wrapping an error string  
> -- does
> > QEMU not follow the Linux rule of not wrapping error strings?  If  
> not, maybe
> > that should change.
> 
> There was some discussion earlier and IIRC the outcome was that error
> strings should contain unique IDs to make them greppable.

Sigh.  Are there any examples of this being done?  How is uniqueness  
ensured?

-Scott
Blue Swirl - Jan. 4, 2013, 9:17 p.m.
On Fri, Jan 4, 2013 at 8:49 PM, Scott Wood <scottwood@freescale.com> wrote:
> On 01/04/2013 02:46:00 PM, Blue Swirl wrote:
>>
>> On Thu, Jan 3, 2013 at 11:07 PM, Scott Wood <scottwood@freescale.com>
>> wrote:
>> > It looks like one of the adjustments was wrapping an error string --
>> > does
>> > QEMU not follow the Linux rule of not wrapping error strings?  If not,
>> > maybe
>> > that should change.
>>
>> There was some discussion earlier and IIRC the outcome was that error
>> strings should contain unique IDs to make them greppable.
>
>
> Sigh.  Are there any examples of this being done?  How is uniqueness
> ensured?

Here's one example:
http://h71000.www7.hp.com/doc/73final/6023/6023pro_001.html

:-)

>
> -Scott
Scott Wood - Jan. 4, 2013, 9:25 p.m.
On 01/04/2013 03:17:21 PM, Blue Swirl wrote:
> On Fri, Jan 4, 2013 at 8:49 PM, Scott Wood <scottwood@freescale.com>  
> wrote:
> > On 01/04/2013 02:46:00 PM, Blue Swirl wrote:
> >>
> >> On Thu, Jan 3, 2013 at 11:07 PM, Scott Wood  
> <scottwood@freescale.com>
> >> wrote:
> >> > It looks like one of the adjustments was wrapping an error  
> string --
> >> > does
> >> > QEMU not follow the Linux rule of not wrapping error strings?   
> If not,
> >> > maybe
> >> > that should change.
> >>
> >> There was some discussion earlier and IIRC the outcome was that  
> error
> >> strings should contain unique IDs to make them greppable.
> >
> >
> > Sigh.  Are there any examples of this being done?  How is uniqueness
> > ensured?
> 
> Here's one example:
> http://h71000.www7.hp.com/doc/73final/6023/6023pro_001.html
> 
> :-)

I meant in QEMU. :-P

-Scott

Patch

diff --git a/hw/openpic.c b/hw/openpic.c
index 7647368..94a7807 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -187,7 +187,9 @@  typedef struct IRQ_queue_t {
 typedef struct IRQ_src_t {
     uint32_t ivpr;  /* IRQ vector/priority register */
     uint32_t idr;   /* IRQ destination register */
+    uint32_t destmask; /* bitmap of CPU destinations */
     int last_cpu;
+    int output;     /* IRQ level, e.g. OPENPIC_OUTPUT_INT */
     int pending;    /* TRUE if IRQ is pending */
     bool level;     /* level-triggered */
     bool fslint;    /* FSL internal interrupt -- level only */
@@ -265,8 +267,6 @@  typedef struct OpenPICState {
     uint32_t irq_msi;
 } OpenPICState;
 
-static void openpic_irq_raise(OpenPICState *opp, int n_CPU, IRQ_src_t *src);
-
 static inline void IRQ_setbit(IRQ_queue_t *q, int n_IRQ)
 {
     q->pending++;
@@ -331,6 +331,19 @@  static void IRQ_local_pipe(OpenPICState *opp, int n_CPU, int n_IRQ)
 
     dst = &opp->dst[n_CPU];
     src = &opp->src[n_IRQ];
+
+    if (src->output != OPENPIC_OUTPUT_INT) {
+        /* On Freescale MPIC, critical interrupts ignore priority,
+         * IACK, EOI, etc.  Before MPIC v4.1 they also ignore
+         * masking.
+         */
+        src->ivpr |= IVPR_ACTIVITY_MASK;
+        DPRINTF("%s: Raise OpenPIC output %d cpu %d irq %d\n",
+                __func__, src->output, n_CPU, n_IRQ);
+        qemu_irq_raise(opp->dst[n_CPU].irqs[src->output]);
+        return;
+    }
+
     priority = IVPR_PRIORITY(src->ivpr);
     if (priority <= dst->ctpr) {
         /* Too low priority */
@@ -361,7 +374,7 @@  static void IRQ_local_pipe(OpenPICState *opp, int n_CPU, int n_IRQ)
         return;
     }
     DPRINTF("Raise OpenPIC INT output cpu %d irq %d\n", n_CPU, n_IRQ);
-    openpic_irq_raise(opp, n_CPU, src);
+    qemu_irq_raise(opp->dst[n_CPU].irqs[OPENPIC_OUTPUT_INT]);
 }
 
 /* update pic state because registers for n_IRQ have changed value */
@@ -404,7 +417,7 @@  static void openpic_update_irq(OpenPICState *opp, int n_IRQ)
     } else if (!(src->ivpr & IVPR_MODE_MASK)) {
         /* Directed delivery mode */
         for (i = 0; i < opp->nb_cpus; i++) {
-            if (src->idr & (1 << i)) {
+            if (src->destmask & (1 << i)) {
                 IRQ_local_pipe(opp, i, n_IRQ);
             }
         }
@@ -413,7 +426,7 @@  static void openpic_update_irq(OpenPICState *opp, int n_IRQ)
         for (i = src->last_cpu + 1; i != src->last_cpu; i++) {
             if (i == opp->nb_cpus)
                 i = 0;
-            if (src->idr & (1 << i)) {
+            if (src->destmask & (1 << i)) {
                 IRQ_local_pipe(opp, i, n_IRQ);
                 src->last_cpu = i;
                 break;
@@ -500,12 +513,45 @@  static inline uint32_t read_IRQreg_ivpr(OpenPICState *opp, int n_IRQ)
 
 static inline void write_IRQreg_idr(OpenPICState *opp, int n_IRQ, uint32_t val)
 {
-    uint32_t tmp;
+    IRQ_src_t *src = &opp->src[n_IRQ];
+    uint32_t normal_mask = (1UL << opp->nb_cpus) - 1;
+    uint32_t crit_mask = 0;
+    uint32_t mask = normal_mask;
+    int crit_shift = IDR_EP_SHIFT - opp->nb_cpus;
+    int i;
+
+    if (opp->flags & OPENPIC_FLAG_IDR_CRIT) {
+        crit_mask = mask << crit_shift;
+        mask |= crit_mask | IDR_EP;
+    }
+
+    src->idr = val & mask;
+    DPRINTF("Set IDE %d to 0x%08x\n", n_IRQ, src->idr);
+
+    if (opp->flags & OPENPIC_FLAG_IDR_CRIT) {
+        if (src->idr & crit_mask) {
+            if (src->idr & normal_mask) {
+                DPRINTF("%s: IRQ configured for multiple output types, using critical\n",
+                        __func__);
+            }
+
+            src->output = OPENPIC_OUTPUT_CINT;
+            src->destmask = 0;
+
+            for (i = 0; i < opp->nb_cpus; i++) {
+                int n_ci = IDR_CI0_SHIFT - i;
 
-    tmp = val & (IDR_EP | IDR_CI);
-    tmp |= val & ((1ULL << MAX_CPU) - 1);
-    opp->src[n_IRQ].idr = tmp;
-    DPRINTF("Set IDE %d to 0x%08x\n", n_IRQ, opp->src[n_IRQ].idr);
+                if (src->idr & (1UL << n_ci)) {
+                    src->destmask |= 1UL << i;
+                }
+            }
+        } else {
+            src->output = OPENPIC_OUTPUT_INT;
+            src->destmask = src->idr & normal_mask;
+        }
+    } else {
+        src->destmask = src->idr;
+    }
 }
 
 static inline void write_IRQreg_ivpr(OpenPICState *opp, int n_IRQ, uint32_t val)
@@ -899,7 +945,7 @@  static void openpic_cpu_write_internal(void *opaque, hwaddr addr,
              IVPR_PRIORITY(src->ivpr) > dst->servicing.priority)) {
             DPRINTF("Raise OpenPIC INT output cpu %d irq %d\n",
                     idx, n_IRQ);
-            openpic_irq_raise(opp, idx, src);
+            qemu_irq_raise(opp->dst[idx].irqs[OPENPIC_OUTPUT_INT]);
         }
         break;
     default:
@@ -1121,13 +1167,6 @@  static void openpic_save(QEMUFile* f, void *opaque)
     qemu_put_be32s(f, &opp->spve);
     qemu_put_be32s(f, &opp->tfrr);
 
-    for (i = 0; i < opp->max_irq; i++) {
-        qemu_put_be32s(f, &opp->src[i].ivpr);
-        qemu_put_be32s(f, &opp->src[i].idr);
-        qemu_put_sbe32s(f, &opp->src[i].last_cpu);
-        qemu_put_sbe32s(f, &opp->src[i].pending);
-    }
-
     qemu_put_be32s(f, &opp->nb_cpus);
 
     for (i = 0; i < opp->nb_cpus; i++) {
@@ -1140,6 +1179,13 @@  static void openpic_save(QEMUFile* f, void *opaque)
         qemu_put_be32s(f, &opp->timers[i].tccr);
         qemu_put_be32s(f, &opp->timers[i].tbcr);
     }
+
+    for (i = 0; i < opp->max_irq; i++) {
+        qemu_put_be32s(f, &opp->src[i].ivpr);
+        qemu_put_be32s(f, &opp->src[i].idr);
+        qemu_put_sbe32s(f, &opp->src[i].last_cpu);
+        qemu_put_sbe32s(f, &opp->src[i].pending);
+    }
 }
 
 static void openpic_load_IRQ_queue(QEMUFile* f, IRQ_queue_t *q)
@@ -1167,13 +1213,6 @@  static int openpic_load(QEMUFile* f, void *opaque, int version_id)
     qemu_get_be32s(f, &opp->spve);
     qemu_get_be32s(f, &opp->tfrr);
 
-    for (i = 0; i < opp->max_irq; i++) {
-        qemu_get_be32s(f, &opp->src[i].ivpr);
-        qemu_get_be32s(f, &opp->src[i].idr);
-        qemu_get_sbe32s(f, &opp->src[i].last_cpu);
-        qemu_get_sbe32s(f, &opp->src[i].pending);
-    }
-
     qemu_get_be32s(f, &opp->nb_cpus);
 
     for (i = 0; i < opp->nb_cpus; i++) {
@@ -1187,18 +1226,21 @@  static int openpic_load(QEMUFile* f, void *opaque, int version_id)
         qemu_get_be32s(f, &opp->timers[i].tbcr);
     }
 
-    return 0;
-}
+    for (i = 0; i < opp->max_irq; i++) {
+        uint32_t val;
 
-static void openpic_irq_raise(OpenPICState *opp, int n_CPU, IRQ_src_t *src)
-{
-    int n_ci = IDR_CI0_SHIFT - n_CPU;
+        val = qemu_get_be32(f);
+        write_IRQreg_idr(opp, i, val);
+        val = qemu_get_be32(f);
+        write_IRQreg_ivpr(opp, i, val);
 
-    if ((opp->flags & OPENPIC_FLAG_IDR_CRIT) && (src->idr & (1 << n_ci))) {
-        qemu_irq_raise(opp->dst[n_CPU].irqs[OPENPIC_OUTPUT_CINT]);
-    } else {
-        qemu_irq_raise(opp->dst[n_CPU].irqs[OPENPIC_OUTPUT_INT]);
+        qemu_get_be32s(f, &opp->src[i].ivpr);
+        qemu_get_be32s(f, &opp->src[i].idr);
+        qemu_get_sbe32s(f, &opp->src[i].last_cpu);
+        qemu_get_sbe32s(f, &opp->src[i].pending);
     }
+
+    return 0;
 }
 
 struct memreg {