From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:43883) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QbIQr-00019L-18 for qemu-devel@nongnu.org; Mon, 27 Jun 2011 16:29:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QbIQp-0000Pp-9w for qemu-devel@nongnu.org; Mon, 27 Jun 2011 16:29:20 -0400 Received: from ch1ehsobe003.messaging.microsoft.com ([216.32.181.183]:6134 helo=CH1EHSOBE003.bigfish.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QbIQo-0000Pc-O1 for qemu-devel@nongnu.org; Mon, 27 Jun 2011 16:29:18 -0400 Date: Mon, 27 Jun 2011 15:28:51 -0500 From: Scott Wood Message-ID: <20110627152851.2ad8d560@schlenkerla.am.freescale.net> In-Reply-To: <1309191246-26224-1-git-send-email-chouteau@adacore.com> References: <1309191246-26224-1-git-send-email-chouteau@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] [PowerPC][RFC] booke timers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fabien Chouteau Cc: qemu-devel@nongnu.org On Mon, 27 Jun 2011 18:14:06 +0200 Fabien Chouteau 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