RE: [Qemu-devel] [PATCH 3/6] kvm/powerpc: Add irq support for E500core

[Liu Yu-B13201]

> -----Original Message-----
> From: Aurelien Jarno [mailto:address@hidden 
> Sent: Sunday, January 25, 2009 12:32 AM
> To: Liu Yu-B13201
> Cc: address@hidden; address@hidden; address@hidden
> Subject: Re: [Qemu-devel] [PATCH 3/6] kvm/powerpc: Add irq 
> support for E500core
> 
> 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.

Fixed.

> 
> > 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.

Fixed.

> 
> > +           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.

Fixed.

> 
> > +            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.

Fixed.

> 
> > +            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.

Fixed.

> 
> > +            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.

Fixed.

> 
> > +            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.

Fixed.

> 
> >  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
> 
>