On 28/06/2011 19:49, Scott Wood wrote:
> On Tue, 28 Jun 2011 15:35:00 +0200
> Fabien Chouteau <chout...@adacore.com> wrote:
>
>> On 27/06/2011 22:28, Scott Wood wrote:
>>> On Mon, 27 Jun 2011 18:14:06 +0200
>>> Fabien Chouteau <chout...@adacore.com> 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
