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

[Scott Wood]

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.

> +/* Timer Control Register */
> +
> +#define TCR_WP_MASK   0x3       /* Watchdog Timer Period */
> +#define TCR_WP_SHIFT  30
> +#define TCR_WRC_MASK  0x3       /* Watchdog Timer Reset Control */
> +#define TCR_WRC_SHIFT 28

Usually such MASK defines are before shifting right:

#define TCR_WP_MASK   0xc0000000
#define TCR_WP_SHIFT  30
#define TCR_WRC_MASK  0x30000000
#define TCR_WRC_SHIFT 28

That way you can do things like

if (tcr & TCR_WRC_MASK) {
        ...
}

> +/* Return the time base value at which the FIT will raise an interrupt */
> +static uint64_t booke_get_fit_target(CPUState *env)
> +{
> +    uint32_t fp = (env->spr[SPR_BOOKE_TCR] >> TCR_FP_SHIFT) & TCR_FP_MASK;
> +
> +    /* Only for e500 */
> +    if (env->insns_flags2 & PPC2_BOOKE206) {
> +        uint32_t fpext = (env->spr[SPR_BOOKE_TCR] >> TCR_E500_FPEXT_SHIFT)
> +            & TCR_E500_FPEXT_MASK;
> +        fp |= fpext << 2;
> +    }

BOOKE206 does not mean e500.  FPEXT does not exist in Power ISA V2.06 Book
III-E.

> +
> +    return 1 << fp;
> +}

The particular bits selected by the possible values of FP are
implementation-dependent.  e500 uses fpext to make all values possible, but
on 440 the four values of fp select from 2^13, 2^17, 2^21, and 2^25.

> +/* Return the time base value at which the WDT will raise an interrupt */
> +static uint64_t booke_get_wdt_target(CPUState *env)
> +{
> +    uint32_t fp = (env->spr[SPR_BOOKE_TCR] >> TCR_WP_SHIFT) & TCR_WP_MASK;
> +
> +    /* Only for e500 */
> +    if (env->insns_flags2 & PPC2_BOOKE206) {
> +        uint32_t fpext = (env->spr[SPR_BOOKE_TCR] >> TCR_E500_WPEXT_SHIFT)
> +            & TCR_E500_WPEXT_MASK;
> +        fp |= fpext << 2;
> +    }
> +
> +    return 1 << fp;
> +}

s/fp/wp/

Avoiding the confusion is especially important on 440, since a different
interval is selected by a given value in FP versus WP.

> +static void booke_update_fixed_timer(CPUState         *env,
> +                                     uint64_t          tb_target,
> +                                     uint64_t          *next,
> +                                     struct QEMUTimer *timer)
> +{
> +    ppc_tb_t *tb_env = env->tb_env;
> +    uint64_t lapse;
> +    uint64_t tb;
> +    uint64_t now;
> +
> +    now = qemu_get_clock_ns(vm_clock);
> +    tb  = cpu_ppc_get_tb(tb_env, now, tb_env->tb_offset);
> +
> +    if (tb_target < tb) {
> +        qemu_del_timer(timer);

You're treating the target as the timebase value that has only the selected
bit and nothing else -- you want to expire the next time that bit
transitions from zero to one, regardless of the other bits.

The timer should never be outright disabled.

> +static void booke_decr_cb (void *opaque)
> +{
> +    CPUState *env;
> +    ppc_tb_t *tb_env;
> +    booke_timer_t *booke_timer;
> +
> +    env = opaque;
> +    tb_env = env->tb_env;
> +    booke_timer = tb_env->opaque;
> +    env->spr[SPR_BOOKE_TSR] |= TSR_DIS;
> +    if (env->spr[SPR_BOOKE_TCR] & TCR_DIE) {
> +        ppc_set_irq(env, booke_timer->decr_excp, 1);
> +    }
> +
> +    if (env->spr[SPR_BOOKE_TCR] & TCR_ARE) {
> +        /* Auto Reload */
> +        cpu_ppc_store_decr(env, env->spr[SPR_BOOKE_DECAR]);
> +    }
> +}

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.

> +static void booke_wdt_cb (void *opaque)
> +{
> +    CPUState *env;
> +    ppc_tb_t *tb_env;
> +    booke_timer_t *booke_timer;
> +
> +    env = opaque;
> +    tb_env = env->tb_env;
> +    booke_timer = tb_env->opaque;
> +
> +    /* TODO: There's lots of complicated stuff to do here */
> +    abort();
> +
> +    booke_update_fixed_timer(env,
> +                             booke_get_wdt_target(env),
> +                             &booke_timer->wdt_next,
> +                             booke_timer->wdt_timer);
> +}

Might want to avoid arming this one until that abort() is fixed...

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

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

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

> +    booke_timer->decr_excp = PPC_INTERRUPT_DECR;
> +    booke_timer->fit_excp = PPC_INTERRUPT_FIT;
> +    booke_timer->wdt_excp = PPC_INTERRUPT_WDT;

What do we gain by using this instead of PPC_INTERRUPT_foo directly?

-SCott