Re: [Qemu-devel] [PATCH 3/6] kvm/powerpc: Add irq support for E500 core

[Aurelien Jarno]

On Thu, Jan 22, 2009 at 06:14:13PM +0800, Liu Yu wrote:
> Signed-off-by: Liu Yu <address@hidden>
> ---
>  hw/ppc.c                    |   89 
> +++++++++++++++++++++++++++++++++++++++++++
>  hw/ppc.h                    |    1 +
>  target-ppc/cpu.h            |   10 +++++
>  target-ppc/translate_init.c |    6 ++-
>  4 files changed, 104 insertions(+), 2 deletions(-)

The same comment for indentation as in the previous patch also applies
here, though it concerns very few lines here. I also have a few minor
comments below, but otherwise the patch is ok.

> diff --git a/hw/ppc.c b/hw/ppc.c
> index 05e787f..7a44951 100644
> --- a/hw/ppc.c
> +++ b/hw/ppc.c
> @@ -314,6 +314,95 @@ void ppc40x_irq_init (CPUState *env)
>                                                    env, PPC40x_INPUT_NB);
>  }
>  
> +/* PowerPC E500 internal IRQ controller */
> +static void ppce500_set_irq (void *opaque, int pin, int level)
> +{
> +    CPUState *env = opaque;
> +    int cur_level;
> +
> +#if defined(PPC_DEBUG_IRQ)
> +    if (loglevel & CPU_LOG_INT) {
> +        fprintf(logfile, "%s: env %p pin %d level %d\n", __func__,
> +                env, pin, level);
> +    }
> +#endif
> +    cur_level = (env->irq_input_state >> pin) & 1;
> +    /* Don't generate spurious events */
> +    if ((cur_level == 1 && level == 0) || (cur_level == 0 && level != 0)) {
> +        switch (pin) {
> +        case PPCE500_INPUT_MCK:
> +            if (level) {
> +#if defined(PPC_DEBUG_IRQ)
> +                if (loglevel & CPU_LOG_INT) {
> +                    fprintf(logfile, "%s: reset the PowerPC system\n",
> +                            __func__);
> +                }
> +#endif

You should probably use LOG_IRQ() instead here which does exactly the
same as the code between the #if and #endif.

> +             fprintf(stderr,"PowerPC E500 reset core\n");
> +             qemu_system_reset_request();
> +            }
> +            break;
> +        case PPCE500_INPUT_RESET_CORE:
> +            if (level) {
> +#if defined(PPC_DEBUG_IRQ)
> +                if (loglevel & CPU_LOG_INT) {
> +                    fprintf(logfile, "%s: reset the PowerPC core\n", 
> __func__);
> +                }
> +#endif
> +             ppc_set_irq(env, PPC_INTERRUPT_MCK, level);
> +            }
> +            break;
> +        case PPCE500_INPUT_CINT:
> +            /* Level sensitive - active high */
> +#if defined(PPC_DEBUG_IRQ)
> +            if (loglevel & CPU_LOG_INT) {
> +                fprintf(logfile, "%s: set the critical IRQ state to %d\n",
> +                        __func__, level);
> +            }
> +#endif

Same here.

> +            ppc_set_irq(env, PPC_INTERRUPT_CEXT, level);
> +            break;
> +        case PPCE500_INPUT_INT:
> +            /* Level sensitive - active high */
> +#if defined(PPC_DEBUG_IRQ)
> +            if (loglevel & CPU_LOG_INT) {
> +                fprintf(logfile, "%s: set the core IRQ state to %d\n",
> +                        __func__, level);
> +            }
> +#endif

Same here.

> +            ppc_set_irq(env, PPC_INTERRUPT_EXT, level);
> +            break;
> +        case PPCE500_INPUT_DEBUG:
> +            /* Level sensitive - active high */
> +#if defined(PPC_DEBUG_IRQ)
> +            if (loglevel & CPU_LOG_INT) {
> +                fprintf(logfile, "%s: set the debug pin state to %d\n",
> +                        __func__, level);
> +            }
> +#endif

Same here.

> +            ppc_set_irq(env, PPC_INTERRUPT_DEBUG, level);
> +            break;
> +        default:
> +            /* Unknown pin - do nothing */
> +#if defined(PPC_DEBUG_IRQ)
> +            if (loglevel & CPU_LOG_INT) {
> +                fprintf(logfile, "%s: unknown IRQ pin %d\n", __func__, pin);
> +            }
> +#endif

Same here.

> +            return;
> +        }
> +        if (level)
> +            env->irq_input_state |= 1 << pin;
> +        else
> +            env->irq_input_state &= ~(1 << pin);
> +    }
> +}
> +
> +void ppce500_irq_init (CPUState *env)
> +{
> +    env->irq_inputs = (void **)qemu_allocate_irqs(&ppce500_set_irq,
> +                                     env, PPCE500_INPUT_NB);
> +}
>  
> /*****************************************************************************/
>  /* PowerPC time base and decrementer emulation */
>  struct ppc_tb_t {
> diff --git a/hw/ppc.h b/hw/ppc.h
> index 75eb11a..2ec4680 100644
> --- a/hw/ppc.h
> +++ b/hw/ppc.h
> @@ -31,6 +31,7 @@ extern CPUReadMemoryFunc *PPC_io_read[];
>  void PPC_debug_write (void *opaque, uint32_t addr, uint32_t val);
>  
>  void ppc40x_irq_init (CPUState *env);
> +void ppce500_irq_init (CPUState *env);
>  void ppc6xx_irq_init (CPUState *env);
>  void ppc970_irq_init (CPUState *env);
>  
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index f7a12da..0eb794f 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1352,6 +1352,16 @@ enum {
>  };
>  
>  enum {
> +    /* PowerPC E500 input pins */
> +    PPCE500_INPUT_RESET_CORE = 0,
> +    PPCE500_INPUT_MCK        = 1,
> +    PPCE500_INPUT_CINT       = 3,
> +    PPCE500_INPUT_INT        = 4,
> +    PPCE500_INPUT_DEBUG      = 6,
> +    PPCE500_INPUT_NB,
> +};
> +
> +enum {
>      /* PowerPC 40x input pins */
>      PPC40x_INPUT_RESET_CORE = 0,
>      PPC40x_INPUT_RESET_CHIP = 1,
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 5008a3a..7953c69 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -4154,7 +4154,8 @@ static void init_proc_e300 (CPUPPCState *env)
>                                POWERPC_FLAG_BUS_CLK)
>  #define check_pow_e500       check_pow_hid0
>  
> -__attribute__ (( unused ))
> +extern void ppce500_irq_init (CPUState *env);
> +

You should use PPC_IRQ_INIT_FN(e500) instead, see at the beginning of
translate_init.c.

>  static void init_proc_e500 (CPUPPCState *env)
>  {
>      /* Time base */
> @@ -4256,7 +4257,8 @@ static void init_proc_e500 (CPUPPCState *env)
>      init_excp_e200(env);
>      env->dcache_line_size = 32;
>      env->icache_line_size = 32;
> -    /* XXX: TODO: allocate internal IRQ controller */
> +    /* Allocate hardware IRQ controller */
> +    ppce500_irq_init(env);
>  }
>  
>  /* Non-embedded PowerPC                                                      
> */
> -- 
> 1.5.4
> 
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net