[Qemu-devel] Re: [PATCH 2/4] powerpc: Make the decr interrupt type overridable

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Mon, Sep 20, 2010 at 12:42:41PM +0200, Alexander Graf wrote:
> Edgar E. Iglesias wrote:
> > Make it possible for boards to override the kind of interrupt
> > to be signaled when the decr timer hits. The 405's signal PIT
> > interrupts while the 440's signal DECR.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > ---
> >  hw/ppc.c |   22 ++++++++++++++++++++--
> >  hw/ppc.h |    2 ++
> >  2 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/ppc.c b/hw/ppc.c
> > index 55e3808..3df7801 100644
> > --- a/hw/ppc.c
> > +++ b/hw/ppc.c
> > @@ -769,6 +769,9 @@ struct ppcemb_timer_t {
> >      struct QEMUTimer *fit_timer;
> >      uint64_t wdt_next;    /* Tick for next WDT interrupt  */
> >      struct QEMUTimer *wdt_timer;
> > +
> > +    /* 405 have the PIT, 440 have a DECR.  */
> > +    unsigned int decr_excp;
> >  };
> >  
> >  /* Fixed interval timer */
> > @@ -851,7 +854,7 @@ static void cpu_4xx_pit_cb (void *opaque)
> >      ppcemb_timer = tb_env->opaque;
> >      env->spr[SPR_40x_TSR] |= 1 << 27;
> >      if ((env->spr[SPR_40x_TCR] >> 26) & 0x1)
> >   
> 
> Do these registers also apply to 440? If so, they should probably be
> renamed to 4xx. Also while you're at it - I'd love to have readable
> #define's for magic numbers :).

Sure, but it sounds to me like follow-up patches :)
The code is already full with mixed use of 4xx and 40x.

> > -        ppc_set_irq(env, PPC_INTERRUPT_PIT, 1);
> > +        ppc_set_irq(env, ppcemb_timer->decr_excp, 1);
> >      start_stop_pit(env, tb_env, 1);
> >      LOG_TB("%s: ar %d ir %d TCR " TARGET_FMT_lx " TSR " TARGET_FMT_lx " "
> >             "%016" PRIx64 "\n", __func__,
> > @@ -948,10 +951,15 @@ target_ulong load_40x_pit (CPUState *env)
> >  
> >  void store_booke_tsr (CPUState *env, target_ulong val)
> >  {
> > +    ppc_tb_t *tb_env = env->tb_env;
> > +    ppcemb_timer_t *ppcemb_timer;
> > +
> > +    ppcemb_timer = tb_env->opaque;
> > +
> >      LOG_TB("%s: val " TARGET_FMT_lx "\n", __func__, val);
> >      env->spr[SPR_40x_TSR] &= ~(val & 0xFC000000);
> >      if (val & 0x80000000)
> > -        ppc_set_irq(env, PPC_INTERRUPT_PIT, 0);
> > +        ppc_set_irq(env, ppcemb_timer->decr_excp, 0);
> >  }
> >  
> >  void store_booke_tcr (CPUState *env, target_ulong val)
> > @@ -977,6 +985,15 @@ static void ppc_emb_set_tb_clk (void *opaque, uint32_t freq)
> >      /* XXX: we should also update all timers */
> >  }
> >  
> > +void ppc_emb_timers_set_decr_excp(CPUState *env, unsigned int excp)
> > +{
> > +    ppc_tb_t *tb_env = env->tb_env;
> > +    ppcemb_timer_t *ppcemb_timer;
> > +
> > +    ppcemb_timer = tb_env->opaque;
> > +    ppcemb_timer->decr_excp = excp;
> >   
> 
> Why do you need this? Shouldn't the decrementor type be set by the CPU core?

Not the way things are modelled today. These blocks are indirectly
instantiated by the boards. But lets make the decr_excp ...

> 
> > +}
> > +
> >  clk_setup_cb ppc_emb_timers_init (CPUState *env, uint32_t freq)
> >  {
> >      ppc_tb_t *tb_env;
> > @@ -996,6 +1013,7 @@ clk_setup_cb ppc_emb_timers_init (CPUState *env, uint32_t freq)
> >              qemu_new_timer(vm_clock, &cpu_4xx_fit_cb, env);
> >          ppcemb_timer->wdt_timer =
> >              qemu_new_timer(vm_clock, &cpu_4xx_wdt_cb, env);
> > +        ppcemb_timer->decr_excp = PPC_INTERRUPT_PIT;
> >   
> 
> If anything, it should be a parameter here.

... an argument to ppc_emb_timers_init and ppc_emb_timers_set_decr_excp
can go away. Does it sound good enough?

Thanks