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