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

[Fabien Chouteau]

On 28/06/2011 19:49, Scott Wood wrote:
> On Tue, 28 Jun 2011 15:35:00 +0200
> Fabien Chouteau <address@hidden> wrote:
> 
>> 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?

The function is used by ppc4xx_init (ppc4xx_devs.c) and ppc440_init_xilinx
(virtex_ml507.c), so I guess ppc_4xx_timers_int will be fine...

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

And that's why I implemented the booke_decr_cb function that will emulate this
behavior.

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

On the other hand, undefined bit should always be read as zeros.

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

I see, I will fix this.

-- 
Fabien Chouteau