From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:52995) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QcJWQ-0007Oq-Sk for qemu-devel@nongnu.org; Thu, 30 Jun 2011 11:51:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QcJWO-0007SN-LD for qemu-devel@nongnu.org; Thu, 30 Jun 2011 11:51:18 -0400 Received: from mel.act-europe.fr ([194.98.77.210]:49466) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QcJWO-0007SI-7O for qemu-devel@nongnu.org; Thu, 30 Jun 2011 11:51:16 -0400 Message-ID: <4E0C9B6E.3000100@adacore.com> Date: Thu, 30 Jun 2011 17:51:10 +0200 From: Fabien Chouteau MIME-Version: 1.0 References: <1309191246-26224-1-git-send-email-chouteau@adacore.com> <20110627152851.2ad8d560@schlenkerla.am.freescale.net> <4E09D884.8090700@adacore.com> <20110628124910.04228660@schlenkerla.am.freescale.net> In-Reply-To: <20110628124910.04228660@schlenkerla.am.freescale.net> Content-Type: text/plain; charset=ISO-8859-1 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: Scott Wood Cc: qemu-devel@nongnu.org On 28/06/2011 19:49, Scott Wood wrote: > On Tue, 28 Jun 2011 15:35:00 +0200 > Fabien Chouteau wrote: > >> On 27/06/2011 22:28, Scott Wood wrote: >>> 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. >> >> 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