Re: [Qemu-devel] [PATCH pic32 v3 03/16] pic32: add support for external interrupt controller mode (EIC)

[Aurelien Jarno]

On 2015-07-05 23:14, Serge Vakulenko wrote:
> EIC is required for pic32 microcontroller.
> 
> Signed-off-by: Serge Vakulenko <address@hidden>
> ---
>  hw/mips/cputimer.c   | 17 +++++++++++++++--
>  hw/mips/mips_int.c   |  8 +++++++-
>  target-mips/cpu.h    |  8 +++++++-
>  target-mips/helper.c | 18 ++++++++++++------
>  4 files changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/mips/cputimer.c b/hw/mips/cputimer.c
> index 58707ed..dab532d 100644
> --- a/hw/mips/cputimer.c
> +++ b/hw/mips/cputimer.c
> @@ -63,7 +63,13 @@ static void cpu_mips_timer_expire(CPUMIPSState *env)
>      if (env->insn_flags & ISA_MIPS32R2) {
>          env->CP0_Cause |= 1 << CP0Ca_TI;
>      }
> -    qemu_irq_raise(env->irq[(env->CP0_IntCtl >> CP0IntCtl_IPTI) & 0x7]);
> +    if (env->CP0_Config3 & (1 << CP0C3_VEIC)) {
> +        /* External interrupt controller mode. */
> +        qemu_irq_raise(env->eic_timer_irq);
> +    } else {
> +        /* Legacy or vectored interrupt mode. */
> +        qemu_irq_raise(env->irq[(env->CP0_IntCtl >> CP0IntCtl_IPTI) & 0x7]);
> +    }
>  }
>  
>  uint32_t cpu_mips_get_count (CPUMIPSState *env)
> @@ -111,7 +117,14 @@ void cpu_mips_store_compare (CPUMIPSState *env, uint32_t 
> value)
>          cpu_mips_timer_update(env);
>      if (env->insn_flags & ISA_MIPS32R2)
>          env->CP0_Cause &= ~(1 << CP0Ca_TI);
> -    qemu_irq_lower(env->irq[(env->CP0_IntCtl >> CP0IntCtl_IPTI) & 0x7]);
> +
> +    if (env->CP0_Config3 & (1 << CP0C3_VEIC)) {
> +        /* External interrupt controller mode. */
> +        qemu_irq_lower(env->eic_timer_irq);
> +    } else {
> +        /* Legacy or vectored interrupt mode. */
> +        qemu_irq_lower(env->irq[(env->CP0_IntCtl >> CP0IntCtl_IPTI) & 0x7]);
> +    }
>  }
>  
>  void cpu_mips_start_count(CPUMIPSState *env)
> diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
> index d740046..eb92439 100644
> --- a/hw/mips/mips_int.c
> +++ b/hw/mips/mips_int.c
> @@ -74,5 +74,11 @@ void cpu_mips_soft_irq(CPUMIPSState *env, int irq, int 
> level)
>          return;
>      }
>  
> -    qemu_set_irq(env->irq[irq], level);
> +    if (env->CP0_Config3 & (1 << CP0C3_VEIC)) {
> +        /* External interrupt controller mode. */
> +        qemu_set_irq(env->eic_soft_irq[irq], level);
> +    } else {
> +        /* Legacy or vectored interrupt mode. */
> +        qemu_set_irq(env->irq[irq], level);
> +    }
>  }
> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index c476166..9513f02 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -596,6 +596,10 @@ struct CPUMIPSState {
>      void *irq[8];
>      QEMUTimer *timer; /* Internal timer */
>      unsigned count_freq; /* rate of Count register */
> +
> +    /* Fields for external interrupt controller. */
> +    void *eic_timer_irq;
> +    void *eic_soft_irq[2];
>  };
>  
>  #include "cpu-qom.h"
> @@ -664,7 +668,9 @@ static inline int 
> cpu_mips_hw_interrupts_pending(CPUMIPSState *env)
>      if (env->CP0_Config3 & (1 << CP0C3_VEIC)) {
>          /* A MIPS configured with a vectorizing external interrupt controller
>             will feed a vector into the Cause pending lines. The core treats
> -           the status lines as a vector level, not as indiviual masks.  */
> +           the status lines as a vector level, not as individual masks. */
> +        pending >>= CP0Ca_IP + 2;
> +        status >>= CP0Ca_IP + 2;

As said in the other email, it's probably better to add a CP0Ca_RIPL_mask
and used a different masking in the VEIC and non VEIC case. This way we
avoid the hardcoded + 2.

Otherwise this patch looks fine.

>          r = pending > status;
>      } else {
>          /* A MIPS configured with compatibility or VInt (Vectored Interrupts)
> diff --git a/target-mips/helper.c b/target-mips/helper.c
> index 8e3204a..6fe5212 100644
> --- a/target-mips/helper.c
> +++ b/target-mips/helper.c
> @@ -574,23 +574,29 @@ void mips_cpu_do_interrupt(CPUState *cs)
>              unsigned int vector;
>              unsigned int pending = (env->CP0_Cause & CP0Ca_IP_mask) >> 8;
>  
> -            pending &= env->CP0_Status >> 8;
>              /* Compute the Vector Spacing.  */
>              spacing = (env->CP0_IntCtl >> CP0IntCtl_VS) & ((1 << 6) - 1);
>              spacing <<= 5;
>  
> -            if (env->CP0_Config3 & (1 << CP0C3_VInt)) {
> +            if (env->CP0_Config3 & (1 << CP0C3_VEIC)) {
> +                /* For VEIC mode, the external interrupt controller feeds the
> +                 * vector through the CP0Cause IP lines. */
> +                vector = pending;
> +
> +                /* Architecturally, this is chip-specific behavior.
> +                 * Some processors, like PIC32, have a separate
> +                 * bit INTCON.MVEC to explicitly enable vectored mode,
> +                 * disabled by default. */
> +                spacing = 0;
> +            } else {
>                  /* For VInt mode, the MIPS computes the vector internally.  
> */
> +                pending &= env->CP0_Status >> 8;
>                  for (vector = 7; vector > 0; vector--) {
>                      if (pending & (1 << vector)) {
>                          /* Found it.  */
>                          break;
>                      }
>                  }
> -            } else {
> -                /* For VEIC mode, the external interrupt controller feeds the
> -                   vector through the CP0Cause IP lines.  */
> -                vector = pending;
>              }
>              offset = 0x200 + vector * spacing;
>          }
> -- 
> 2.2.2
> 
> 

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
address@hidden                 http://www.aurel32.net