[03/15] openpic: fix sense and priority bits

Previously, the sense and priority bits were masked off when writing
to IVPR, and all interrupts were treated as edge-triggered (despite
the existence of code for handling level-triggered interrupts).

Polarity is implemented only as storage.  We don't simulate the
bad effects that you'd get on real hardware if you set this incorrectly,
but at least the guest sees the right thing when it reads back the register.

Sense now controls level/edge on FSL external interrupts (and all
interrupts on non-FSL MPIC).  FSL internal interrupts do not have a sense
bit (reads as zero), but are level.  FSL timers and IPIs do not have
sense or polarity bits (read as zero), and are edge-triggered.  To
accommodate FSL internal interrupts, QEMU's internal notion of whether an
interrupt is level-triggered is separated from the IVPR bit.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 hw/openpic.c |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 55 insertions(+), 6 deletions(-)

On 22.12.2012, at 03:15, Scott Wood wrote:

> Previously, the sense and priority bits were masked off when writing
> to IVPR, and all interrupts were treated as edge-triggered (despite
> the existence of code for handling level-triggered interrupts).
> 
> Polarity is implemented only as storage.  We don't simulate the
> bad effects that you'd get on real hardware if you set this incorrectly,
> but at least the guest sees the right thing when it reads back the register.
> 
> Sense now controls level/edge on FSL external interrupts (and all
> interrupts on non-FSL MPIC).  FSL internal interrupts do not have a sense
> bit (reads as zero), but are level.  FSL timers and IPIs do not have
> sense or polarity bits (read as zero), and are edge-triggered.  To
> accommodate FSL internal interrupts, QEMU's internal notion of whether an
> interrupt is level-triggered is separated from the IVPR bit.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
> hw/openpic.c |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/openpic.c b/hw/openpic.c
> index 02f793b..34449a7 100644
> --- a/hw/openpic.c
> +++ b/hw/openpic.c
> @@ -189,6 +189,9 @@ typedef struct IRQ_src_t {
>     uint32_t ide;   /* IRQ destination register */
>     int last_cpu;
>     int pending;    /* TRUE if IRQ is pending */
> +    bool level;     /* level-triggered */
> +    bool fslint;    /* FSL internal interrupt -- level only */
> +    bool fslspecial; /* FSL timer/IPI interrupt, edge, no polarity */

This really looks more like an "irqtype" enum, no?

enum irqtype {
    IRQ_TYPE_NORMAL = 0,
    IRQ_TYPE_FSLINT,
    IRQ_TYPE_FSLSPECIAL,
}


Alex

> } IRQ_src_t;
> 
> #define IPVP_MASK_SHIFT       31
> @@ -427,7 +430,7 @@ static void openpic_set_irq(void *opaque, int n_IRQ, int level)
>     src = &opp->src[n_IRQ];
>     DPRINTF("openpic: set irq %d = %d ipvp=0x%08x\n",
>             n_IRQ, level, src->ipvp);
> -    if (src->ipvp & IPVP_SENSE_MASK) {
> +    if (src->level) {
>         /* level-sensitive irq */
>         src->pending = level;
>         if (!level) {
> @@ -459,6 +462,14 @@ static void openpic_reset(DeviceState *d)
>     for (i = 0; i < opp->max_irq; i++) {
>         opp->src[i].ipvp = opp->ipvp_reset;
>         opp->src[i].ide  = opp->ide_reset;
> +
> +        if (opp->src[i].fslint) {
> +            opp->src[i].ipvp |= IPVP_POLARITY_MASK;
> +        }
> +
> +        if (!opp->src[i].fslint && !opp->src[i].fslspecial) {
> +            opp->src[i].level = !!(opp->ipvp_reset & IPVP_SENSE_MASK);
> +        }
>     }
>     /* Initialise IRQ destinations */
>     for (i = 0; i < MAX_CPU; i++) {
> @@ -499,10 +510,30 @@ static inline void write_IRQreg_ide(OpenPICState *opp, int n_IRQ, uint32_t val)
> 
> static inline void write_IRQreg_ipvp(OpenPICState *opp, int n_IRQ, uint32_t val)
> {
> -    /* NOTE: not fully accurate for special IRQs, but simple and sufficient */
> +    uint32_t mask;
> +
> +    /* NOTE when implementing newer FSL MPIC models: starting with v4.0,
> +     * the polarity bit is read-only on internal interrupts.
> +     */
> +    mask = IPVP_MASK_MASK | IPVP_PRIORITY_MASK | IPVP_SENSE_MASK |
> +           IPVP_POLARITY_MASK | opp->vector_mask;
> +
>     /* ACTIVITY bit is read-only */
> -    opp->src[n_IRQ].ipvp = (opp->src[n_IRQ].ipvp & IPVP_ACTIVITY_MASK) |
> -        (val & (IPVP_MASK_MASK | IPVP_PRIORITY_MASK | opp->vector_mask));
> +    opp->src[n_IRQ].ipvp =
> +        (opp->src[n_IRQ].ipvp & IPVP_ACTIVITY_MASK) | (val & mask);
> +
> +    /* For FSL internal interrupts, The sense bit is reserved and zero,
> +     * and the interrupt is always level-triggered.  Timers and IPIs
> +     * have no sense or polarity bits, and are edge-triggered.
> +     */
> +    if (opp->src[n_IRQ].fslint) {
> +        opp->src[n_IRQ].ipvp &= ~IPVP_SENSE_MASK;
> +    } else if (opp->src[n_IRQ].fslspecial) {
> +        opp->src[n_IRQ].ipvp &= ~(IPVP_POLARITY_MASK | IPVP_SENSE_MASK);
> +    } else {
> +        opp->src[n_IRQ].level = !!(opp->src[n_IRQ].ipvp & IPVP_SENSE_MASK);
> +    }
> +
>     openpic_update_irq(opp, n_IRQ);
>     DPRINTF("Set IPVP %d to 0x%08x -> 0x%08x\n", n_IRQ, val,
>             opp->src[n_IRQ].ipvp);
> @@ -934,7 +965,7 @@ static uint32_t openpic_cpu_read_internal(void *opaque, hwaddr addr,
>             }
>             IRQ_resetbit(&dst->raised, n_IRQ);
>             dst->raised.next = -1;
> -            if (!(src->ipvp & IPVP_SENSE_MASK)) {
> +            if (!src->level) {
>                 /* edge-sensitive IRQ */
>                 src->ipvp &= ~IPVP_ACTIVITY_MASK;
>                 src->pending = 0;
> @@ -942,7 +973,7 @@ static uint32_t openpic_cpu_read_internal(void *opaque, hwaddr addr,
> 
>             if ((n_IRQ >= opp->irq_ipi0) &&  (n_IRQ < (opp->irq_ipi0 + MAX_IPI))) {
>                 src->ide &= ~(1 << idx);
> -                if (src->ide && !(src->ipvp & IPVP_SENSE_MASK)) {
> +                if (src->ide && !src->level) {
>                     /* trigger on CPUs that didn't know about it yet */
>                     openpic_set_irq(opp, n_IRQ, 1);
>                     openpic_set_irq(opp, n_IRQ, 0);
> @@ -1226,7 +1257,25 @@ static int openpic_init(SysBusDevice *dev)
>         opp->brr1 = FSL_BRR1_IPID | FSL_BRR1_IPMJ | FSL_BRR1_IPMN;
>         msi_supported = true;
>         list = list_be;
> +
> +        for (i = 0; i < FSL_MPIC_20_MAX_EXT; i++) {
> +            opp->src[i].level = false;
> +        }
> +
> +        /* Internal interrupts, including message and MSI */
> +        for (i = 16; i < MAX_SRC; i++) {
> +            opp->src[i].fslint = true;
> +            opp->src[i].level = true;
> +        }
> +
> +        /* timers and IPIs */
> +        for (i = MAX_SRC; i < MAX_IRQ; i++) {
> +            opp->src[i].fslspecial = true;
> +            opp->src[i].level = false;
> +        }
> +
>         break;
> +
>     case OPENPIC_MODEL_RAVEN:
>         opp->nb_irqs = RAVEN_MAX_EXT;
>         opp->vid = VID_REVISION_1_3;
> -- 
> 1.7.9.5
> 
>

On 01/03/2013 11:51:56 AM, Alexander Graf wrote:
> 
> On 22.12.2012, at 03:15, Scott Wood wrote:
> 
> > Previously, the sense and priority bits were masked off when writing
> > to IVPR, and all interrupts were treated as edge-triggered (despite
> > the existence of code for handling level-triggered interrupts).
> >
> > Polarity is implemented only as storage.  We don't simulate the
> > bad effects that you'd get on real hardware if you set this  
> incorrectly,
> > but at least the guest sees the right thing when it reads back the  
> register.
> >
> > Sense now controls level/edge on FSL external interrupts (and all
> > interrupts on non-FSL MPIC).  FSL internal interrupts do not have a  
> sense
> > bit (reads as zero), but are level.  FSL timers and IPIs do not have
> > sense or polarity bits (read as zero), and are edge-triggered.  To
> > accommodate FSL internal interrupts, QEMU's internal notion of  
> whether an
> > interrupt is level-triggered is separated from the IVPR bit.
> >
> > Signed-off-by: Scott Wood <scottwood@freescale.com>
> > ---
> > hw/openpic.c |   61  
> ++++++++++++++++++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 55 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/openpic.c b/hw/openpic.c
> > index 02f793b..34449a7 100644
> > --- a/hw/openpic.c
> > +++ b/hw/openpic.c
> > @@ -189,6 +189,9 @@ typedef struct IRQ_src_t {
> >     uint32_t ide;   /* IRQ destination register */
> >     int last_cpu;
> >     int pending;    /* TRUE if IRQ is pending */
> > +    bool level;     /* level-triggered */
> > +    bool fslint;    /* FSL internal interrupt -- level only */
> > +    bool fslspecial; /* FSL timer/IPI interrupt, edge, no polarity  
> */
> 
> This really looks more like an "irqtype" enum, no?
> 
> enum irqtype {
>     IRQ_TYPE_NORMAL = 0,
>     IRQ_TYPE_FSLINT,
>     IRQ_TYPE_FSLSPECIAL,
> }

OK.  At one point they could both be set, before I looked more closely  
at how the special interrupts are defined in hardware.

-Scott