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

[Fabien Chouteau]

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?

>
>> +/* 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) {
>       ...
> }

OK, I will use this kind of declaration:

#define TCR_WP_SHIFT  30        /* Watchdog Timer Period */
#define TCR_WP_MASK   (0x3 << TCR_WP_SHIFT)

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

I will try to fix this for v2.

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

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


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

Fixed.

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

That's right, I will fix this for v2.

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

Can you please explain, I don't see where I'm not compliant with booke's
decrementer.

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

It's just a mask to keep only the defined bits.

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

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

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

Nothing...

Thanks for your review.

-- 
Fabien Chouteau