qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] [PowerPC][RFC] booke timers


From: Scott Wood
Subject: Re: [Qemu-devel] [PATCH] [PowerPC][RFC] booke timers
Date: Tue, 28 Jun 2011 12:49:10 -0500

On Tue, 28 Jun 2011 15:35:00 +0200
Fabien Chouteau <address@hidden> wrote:

> Subject:   Re: [Qemu-devel] [PATCH] [PowerPC][RFC] booke timers
> To:        Scott Wood <address@hidden>
> Cc:        address@hidden
> Bcc:
> -=-=-=-=-=-=-=-=-=# Don't remove this line #=-=-=-=-=-=-=-=-=-w 
> On 27/06/2011 22:28, Scott Wood wrote:
> > On Mon, 27 Jun 2011 18:14:06 +0200
> > Fabien Chouteau <address@hidden> wrote:
> >
> >> While working on the emulation of the freescale p2010 (e500v2) I realized 
> >> that
> >> there's no implementation of booke's timers features. Currently mpc8544 
> >> uses
> >> ppc_emb (ppc_emb_timers_init) which is close but not exactly like booke 
> >> (for
> >> example booke uses different SPR).
> >
> > ppc_emb_timers_init should be renamed something less generic, then.
> 
> Agreed, can you help me to find a better name?

What chips are covered by it?  40x?

> Maybe I can do something like:
> 
> static uint64_t booke_get_fit_target(CPUState *env)
> {
>     uint32_t fp = (env->spr[SPR_BOOKE_TCR] & TCR_FP_MASK) >> TCR_FP_SHIFT;
> 
>     /* Only for e500 */
>     if (/* CPU is e500 */) {
>         uint32_t fpext = (env->spr[SPR_BOOKE_TCR] & TCR_E500_FPEXT_MASK)
>             >> TCR_E500_FPEXT_SHIFT;
>         fp |= fpext << 2;
>     } else {
>         fp = env->fit_period[fp];
>     }
> 
>     return 1 << fp;
> }

Looks good -- or just have a CPU-specific function pointer that extracts
the period from TCR.

> > I think some changes in the decrementer code are needed to provide booke
> > semantics -- no raising the interrupt based on the high bit of decr, and
> > stop counting when reach zero.
> 
> Can you please explain, I don't see where I'm not compliant with booke's
> decrementer.

It's not an issue with this code specifically, but existing behavior in the
decrementer code that isn't appropriate for booke.

On classic/server powerpc, when decrementer hits zero, it wraps around, and
the upper bit of DECR is used to signal the interrupt.  On booke, when
decrementer hits zero, it stops, and the upper bit of DECR is not special.

> >> +void store_booke_tsr (CPUState *env, target_ulong val)
> >> +{
> >> +    ppc_tb_t *tb_env = env->tb_env;
> >> +    booke_timer_t *booke_timer;
> >> +
> >> +    booke_timer = tb_env->opaque;
> >> +
> >> +    env->spr[SPR_BOOKE_TSR] &= ~(val & 0xFC000000);
> >
> > Do we really need the "& 0xFC000000"?  Likewise in TCR.
> 
> It's just a mask to keep only the defined bits.

Just seems unnecessary, and potentially harmful if CPU-specific code wants
to interpret implementation-defined bits, or if new bits are architected
in the future.

> >> +    if (val & TSR_DIS) {
> >> +        ppc_set_irq(env, booke_timer->decr_excp, 0);
> >> +    }
> >> +
> >> +    if (val & TSR_FIS) {
> >> +        ppc_set_irq(env, booke_timer->fit_excp, 0);
> >> +    }
> >> +
> >> +    if (val & TSR_WIS) {
> >> +        ppc_set_irq(env, booke_timer->wdt_excp, 0);
> >> +    }
> >> +}
> >
> > It looks like ppc_hw_interrupt() is currently treating these as
> > edge-triggered, deasserting upon delivery.  It should be level for booke.
> 
> This is beyond the scope of this patch.

It's part of correctly implementing booke timers.

> >> +void store_booke_tcr (CPUState *env, target_ulong val)
> >> +{
> >> +    ppc_tb_t *tb_env = env->tb_env;
> >> +    booke_timer_t *booke_timer = tb_env->opaque;
> >> +
> >> +    tb_env = env->tb_env;
> >> +    env->spr[SPR_BOOKE_TCR] = val & 0xFFC00000;
> >> +
> >> +    booke_update_fixed_timer(env,
> >> +                             booke_get_fit_target(env),
> >> +                             &booke_timer->fit_next,
> >> +                             booke_timer->fit_timer);
> >> +
> >> +    booke_update_fixed_timer(env,
> >> +                             booke_get_wdt_target(env),
> >> +                             &booke_timer->wdt_next,
> >> +                             booke_timer->wdt_timer);
> >> +}
> >
> > Check for FIS/DIS/WIS -- the corresponding enable bit might have just been
> > set.
> 
> Can you explain, I don't see the problem.

If a decrementer fires with TCR[DIE] unset, it won't be delivered, but
TSR[DIS] will be set.

If a guest subsequenly sets TCR[DIE], without having first cleared TSR[DIS],
the interrupt should fire immediately -- but that will only happen if you
check for it here.

-Scott




reply via email to

[Prev in Thread] Current Thread [Next in Thread]