Subject: Re: [Qemu-devel] [PATCH] [PowerPC][RFC] booke timers
To: Scott Wood <scottw...@freescale.com>
Cc: qemu-devel@nongnu.org
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 <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?
>
>> +/* 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
