qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Aurelien Jarno <aurelien@aurel32.net>
To: Serge Vakulenko <serge.vakulenko@gmail.com>
Cc: Leon Alrae <leon.alrae@imgtec.com>, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH pic32 v2 3/5] Added support for external interrupt controller (EIC) mode.
Date: Wed, 1 Jul 2015 13:07:09 +0200	[thread overview]
Message-ID: <20150701110709.GC11361@aurel32.net> (raw)
In-Reply-To: <ae51ccd90e91eb22b84a88291cc23b535b965e21.1435723168.git.serge.vakulenko@gmail.com>

On 2015-06-30 21:12, Serge Vakulenko wrote:
> Signed-off-by: Serge Vakulenko <serge.vakulenko@gmail.com>
> ---
>  hw/mips/cputimer.c   | 17 +++++++++++++++--
>  hw/mips/mips_int.c   | 12 ++++++++++--
>  target-mips/cpu.h    |  9 ++++++++-
>  target-mips/helper.c | 20 ++++++++++++++------
>  4 files changed, 47 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/mips/cputimer.c b/hw/mips/cputimer.c
> index 94a29df..849d62c 100644
> --- a/hw/mips/cputimer.c
> +++ b/hw/mips/cputimer.c
> @@ -54,7 +54,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. */
> +        env->eic_timer_irq(env, 1);
> +    } 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)
> @@ -102,7 +108,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. */
> +        env->eic_timer_irq(env, 0);
> +    } 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..3452866 100644
> --- a/hw/mips/mips_int.c
> +++ b/hw/mips/mips_int.c
> @@ -32,7 +32,7 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level)
>      CPUMIPSState *env = &cpu->env;
>      CPUState *cs = CPU(cpu);
>  
> -    if (irq < 0 || irq > 7)
> +    if (irq < 0 || irq > 7 || (env->CP0_Config3 & (1 << CP0C3_VEIC)))
>          return;

In which case you go through these code path? The internal interrupt
controller should not be used with an external one, so this should not
be reached. Actually machines with an external interrupt controller
should not even call cpu_mips_irq_init_cpu().

>      if (level) {
> @@ -74,5 +74,13 @@ 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. */
> +        if (level > 0) {
> +            env->eic_soft_irq(env, irq);

It means software interrupts can be asserted, but not deasserted with an
external vector interrupt. I don't think that is correct.

> +        }
> +    } 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..ab830ee 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -596,6 +596,11 @@ struct CPUMIPSState {
>      void *irq[8];
>      QEMUTimer *timer; /* Internal timer */
>      unsigned count_freq; /* rate of Count register */
> +
> +    /* Fields for external interrupt controller. */
> +    void *eic_context;
> +    void (*eic_timer_irq)(CPUMIPSState *env, int raise);
> +    void (*eic_soft_irq)(CPUMIPSState *env, int num);

I don't think this is the way to go. You should just define variable
like eic_timer_irq and eic_soft_irq similar to the irq[8] field above,
and allocate them with qemu_allocate_irqs(). That way you can pass the
context of the external interrupt controller.

>  };
>  
>  #include "cpu-qom.h"
> @@ -664,7 +669,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;
>          r = pending > status;

I don't think it's needed. As the pending and status field have been
masked above, we don't need to shift them to do the comparison.

>      } else {
>          /* A MIPS configured with compatibility or VInt (Vectored Interrupts)
> diff --git a/target-mips/helper.c b/target-mips/helper.c
> index 8e3204a..7e25998 100644
> --- a/target-mips/helper.c
> +++ b/target-mips/helper.c
> @@ -574,23 +574,31 @@ 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.
> +                 * TODO: some processors, like PIC32MZ,
> +                 * provide vector in a different way.
> +                 * 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;
>              }

The changes looks correct, but I do wonder why you swap the test on
CP0_Config3. It would be less changes to just move the pending masking
with CP0_Status inside the VInt mode.

>              offset = 0x200 + vector * spacing;
>          }
> -- 
> 1.9.1
> 
> 
> 

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

  reply	other threads:[~2015-07-01 11:07 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-01  4:12 [Qemu-devel] [PATCH pic32 v2 0/5] Support for Microchip pic32mx7 and pic32mz microcontrollers Serge Vakulenko
     [not found] ` <cover.1435723168.git.serge.vakulenko@gmail.com>
2015-07-01  4:12   ` [Qemu-devel] [PATCH pic32 v2 1/5] Speed of MIPS CPU timer made configurable per platform Serge Vakulenko
2015-07-01 10:02     ` Aurelien Jarno
2015-07-05 23:25       ` Serge Vakulenko
2015-07-06  8:31         ` Aurelien Jarno
2015-07-01  4:12   ` [Qemu-devel] [PATCH pic32 v2 2/5] Fixed random index generation for TLBWR instruction. It was not quite random and did not skip Wired entries Serge Vakulenko
2015-07-01 10:11     ` Aurelien Jarno
2015-07-03 21:39       ` Maciej W. Rozycki
2015-07-06  0:16         ` Serge Vakulenko
2015-07-06  0:03       ` Serge Vakulenko
2015-07-06  8:32         ` Aurelien Jarno
2015-07-02  7:52     ` Antony Pavlov
2015-07-06  0:06       ` Serge Vakulenko
2015-07-01  4:12   ` [Qemu-devel] [PATCH pic32 v2 3/5] Added support for external interrupt controller (EIC) mode Serge Vakulenko
2015-07-01 11:07     ` Aurelien Jarno [this message]
2015-07-06  3:05       ` Serge Vakulenko
2015-07-06  3:31         ` Serge Vakulenko
2015-07-06  9:31           ` Aurelien Jarno
2015-07-06  9:28         ` Aurelien Jarno
2015-07-01  4:12   ` [Qemu-devel] [PATCH pic32 v2 4/5] Two new processor variants: M4K and microAptivP Serge Vakulenko
2015-07-01 13:37     ` Aurelien Jarno
2015-07-03 22:04       ` Maciej W. Rozycki
2015-07-06  4:15         ` Serge Vakulenko
2015-07-06  3:48       ` Serge Vakulenko
2015-07-06  8:40         ` Aurelien Jarno
2015-07-01  4:12   ` [Qemu-devel] [PATCH pic32 v2 5/5] Two new machine platforms: pic32mz7 and pic32mz Serge Vakulenko
2015-07-01 13:41     ` Aurelien Jarno
2015-07-06  4:18       ` Serge Vakulenko
2015-07-06  7:33         ` Antony Pavlov
2015-07-06 18:58           ` Serge Vakulenko
2015-07-06 21:43             ` Peter Crosthwaite
2015-07-07  7:30             ` Antony Pavlov
2015-07-07 14:08               ` Aurelien Jarno
2015-07-02  5:56     ` Antony Pavlov
2015-07-06  4:27       ` Serge Vakulenko
2015-07-06  7:55         ` Antony Pavlov
2015-07-02  5:31 ` [Qemu-devel] [PATCH pic32 v2 0/5] Support for Microchip pic32mx7 and pic32mz microcontrollers Antony Pavlov
2015-07-06  0:39   ` Serge Vakulenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150701110709.GC11361@aurel32.net \
    --to=aurelien@aurel32.net \
    --cc=leon.alrae@imgtec.com \
    --cc=qemu-devel@nongnu.org \
    --cc=serge.vakulenko@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).