| Message ID | 1309191246-26224-1-git-send-email-chouteau@adacore.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 27 Jun 2011 18:14:06 +0200 Fabien Chouteau <chouteau@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
Subject: Re: [Qemu-devel] [PATCH] [PowerPC][RFC] booke timers To: Scott Wood <scottwood@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 <chouteau@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.
On Tue, 28 Jun 2011 15:35:00 +0200 Fabien Chouteau <chouteau@adacore.com> wrote: > Subject: Re: [Qemu-devel] [PATCH] [PowerPC][RFC] booke timers > To: Scott Wood <scottwood@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 <chouteau@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? > 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; > } Looks good -- or just have a CPU-specific function pointer that extracts the period from TCR. > > 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. > >> +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. > >> + 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. It's part of correctly implementing booke timers. > >> +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. -Scott
On 28/06/2011 19:49, Scott Wood wrote: > On Tue, 28 Jun 2011 15:35:00 +0200 > Fabien Chouteau <chouteau@adacore.com> wrote: > >> On 27/06/2011 22:28, Scott Wood wrote: >>> On Mon, 27 Jun 2011 18:14:06 +0200 >>> Fabien Chouteau <chouteau@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.
On Thu, 30 Jun 2011 17:51:10 +0200 Fabien Chouteau <chouteau@adacore.com> wrote: > On 28/06/2011 19:49, Scott Wood wrote: > > On Tue, 28 Jun 2011 15:35:00 +0200 > > Fabien Chouteau <chouteau@adacore.com> wrote: > > > >> On 27/06/2011 22:28, Scott Wood wrote: > >>> On Mon, 27 Jun 2011 18:14:06 +0200 > >>> Fabien Chouteau <chouteau@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... Doesn't 440 have normal booke timers? > >>> 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. booke_decr_cb() doesn't control what happens when DECR is read after an expiration, nor does it control whether an interrupt is raised if software writes DECR with the high bit set. > >> 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. I don't think there's any such requirement. -Scott
On 30/06/2011 21:26, Scott Wood wrote: > On Thu, 30 Jun 2011 17:51:10 +0200 > Fabien Chouteau <chouteau@adacore.com> wrote: > >> On 28/06/2011 19:49, Scott Wood wrote: >>> On Tue, 28 Jun 2011 15:35:00 +0200 >>> Fabien Chouteau <chouteau@adacore.com> wrote: >>> >>>> On 27/06/2011 22:28, Scott Wood wrote: >>>>> On Mon, 27 Jun 2011 18:14:06 +0200 >>>>> Fabien Chouteau <chouteau@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... > > Doesn't 440 have normal booke timers? > Actually ppc_emb_timers_init should be renamed ppc40x_timers_init, and Xilinx's ppc440 should use the new implementation of booke timers. >>>>> 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. > > booke_decr_cb() doesn't control what happens when DECR is read after an > expiration, nor does it control whether an interrupt is raised if software > writes DECR with the high bit set. > OK, I will add a condition to avoid this on booke processors. >>>> 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. > > I don't think there's any such requirement. My bad ;) "The handling of reserved bits in System Registers (e.g. Integer Exception Register, Floating-Point Status and Control Register) is implementation-dependent. Software is permitted to write any value to such a bit with no visible effect on processors that implement this version of Book E. A subsequent reading of the bit returns a 0 if the last value written to the bit was 0 and returns an undefined value (0 or 1) otherwise."
diff --git a/Makefile.target b/Makefile.target index 38afdb8..f0d4062 100644 --- a/Makefile.target +++ b/Makefile.target @@ -242,7 +242,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o # shared objects -obj-ppc-y = ppc.o +obj-ppc-y = ppc.o ppc_booke.o obj-ppc-y += vga.o # PREP target obj-ppc-y += i8259.o mc146818rtc.o diff --git a/hw/ppc.c b/hw/ppc.c index 9157719..9932136 100644 --- a/hw/ppc.c +++ b/hw/ppc.c @@ -50,7 +50,7 @@ static void cpu_ppc_tb_stop (CPUState *env); static void cpu_ppc_tb_start (CPUState *env); -static void ppc_set_irq (CPUState *env, int n_IRQ, int level) +void ppc_set_irq (CPUState *env, int n_IRQ, int level) { unsigned int old_pending = env->pending_interrupts; @@ -423,25 +423,8 @@ void ppce500_irq_init (CPUState *env) } /*****************************************************************************/ /* PowerPC time base and decrementer emulation */ -struct ppc_tb_t { - /* Time base management */ - int64_t tb_offset; /* Compensation */ - int64_t atb_offset; /* Compensation */ - uint32_t tb_freq; /* TB frequency */ - /* Decrementer management */ - uint64_t decr_next; /* Tick for next decr interrupt */ - uint32_t decr_freq; /* decrementer frequency */ - struct QEMUTimer *decr_timer; - /* Hypervisor decrementer management */ - uint64_t hdecr_next; /* Tick for next hdecr interrupt */ - struct QEMUTimer *hdecr_timer; - uint64_t purr_load; - uint64_t purr_start; - void *opaque; -}; -static inline uint64_t cpu_ppc_get_tb(ppc_tb_t *tb_env, uint64_t vmclk, - int64_t tb_offset) +uint64_t cpu_ppc_get_tb(ppc_tb_t *tb_env, uint64_t vmclk, int64_t tb_offset) { /* TB time in tb periods */ return muldiv64(vmclk, tb_env->tb_freq, get_ticks_per_sec()) + tb_offset; @@ -996,30 +979,6 @@ target_ulong load_40x_pit (CPUState *env) return cpu_ppc_load_decr(env); } -void store_booke_tsr (CPUState *env, target_ulong val) -{ - ppc_tb_t *tb_env = env->tb_env; - ppcemb_timer_t *ppcemb_timer; - - ppcemb_timer = tb_env->opaque; - - LOG_TB("%s: val " TARGET_FMT_lx "\n", __func__, val); - env->spr[SPR_40x_TSR] &= ~(val & 0xFC000000); - if (val & 0x80000000) - ppc_set_irq(env, ppcemb_timer->decr_excp, 0); -} - -void store_booke_tcr (CPUState *env, target_ulong val) -{ - ppc_tb_t *tb_env; - - tb_env = env->tb_env; - LOG_TB("%s: val " TARGET_FMT_lx "\n", __func__, val); - env->spr[SPR_40x_TCR] = val & 0xFFC00000; - start_stop_pit(env, tb_env, 1); - cpu_4xx_wdt_cb(env); -} - static void ppc_emb_set_tb_clk (void *opaque, uint32_t freq) { CPUState *env = opaque; diff --git a/hw/ppc.h b/hw/ppc.h index 3ccf134..633c53e 100644 --- a/hw/ppc.h +++ b/hw/ppc.h @@ -1,3 +1,5 @@ +void ppc_set_irq (CPUState *env, int n_IRQ, int level); + /* PowerPC hardware exceptions management helpers */ typedef void (*clk_setup_cb)(void *opaque, uint32_t freq); typedef struct clk_setup_t clk_setup_t; @@ -11,6 +13,24 @@ static inline void clk_setup (clk_setup_t *clk, uint32_t freq) (*clk->cb)(clk->opaque, freq); } +struct ppc_tb_t { + /* Time base management */ + int64_t tb_offset; /* Compensation */ + int64_t atb_offset; /* Compensation */ + uint32_t tb_freq; /* TB frequency */ + /* Decrementer management */ + uint64_t decr_next; /* Tick for next decr interrupt */ + uint32_t decr_freq; /* decrementer frequency */ + struct QEMUTimer *decr_timer; + /* Hypervisor decrementer management */ + uint64_t hdecr_next; /* Tick for next hdecr interrupt */ + struct QEMUTimer *hdecr_timer; + uint64_t purr_load; + uint64_t purr_start; + void *opaque; +}; + +uint64_t cpu_ppc_get_tb(ppc_tb_t *tb_env, uint64_t vmclk, int64_t tb_offset); clk_setup_cb cpu_ppc_tb_init (CPUState *env, uint32_t freq); /* Embedded PowerPC DCR management */ typedef uint32_t (*dcr_read_cb)(void *opaque, int dcrn); @@ -55,3 +75,6 @@ enum { #define FW_CFG_PPC_KVM_PID (FW_CFG_ARCH_LOCAL + 0x07) #define PPC_SERIAL_MM_BAUDBASE 399193 + +/* ppc_booke.c */ +void ppc_booke_timers_init (CPUState *env, uint32_t freq); diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c new file mode 100644 index 0000000..71e13d2 --- /dev/null +++ b/hw/ppc_booke.c @@ -0,0 +1,254 @@ +/* + * QEMU PowerPC Booke hardware System Emulator + * + * Copyright (c) 2011 AdaCore + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +#include "hw.h" +#include "ppc.h" +#include "qemu-timer.h" +#include "sysemu.h" +#include "nvram.h" +#include "qemu-log.h" +#include "loader.h" + + +/* 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 +#define TCR_WIE (1 << 27) /* Watchdog Timer Interrupt Enable */ +#define TCR_DIE (1 << 26) /* Decrementer Interrupt Enable */ +#define TCR_FP_MASK 0x3 /* Fixed-Interval Timer Period */ +#define TCR_FP_SHIFT 24 +#define TCR_FIE (1 << 23) /* Fixed-Interval Timer Interrupt Enable */ +#define TCR_ARE (1 << 22) /* Auto-Reload Enable */ + +/* Timer Control Register (e500 specific fields) */ + +#define TCR_E500_FPEXT_MASK 0xf /* Fixed-Interval Timer Period Extension */ +#define TCR_E500_FPEXT_SHIFT 13 +#define TCR_E500_WPEXT_MASK 0xf /* Watchdog Timer Period Extension */ +#define TCR_E500_WPEXT_SHIFT 17 + +/* Timer Status Register */ + +#define TSR_FIS (1 << 26) /* Fixed-Interval Timer Interrupt Status */ +#define TSR_DIS (1 << 27) /* Decrementer Interrupt Status */ +#define TSR_WRS_MASK 0x3 /* Watchdog Timer Reset Status */ +#define TSR_WRS_SHIFT 28 +#define TSR_WIS (1 << 30) /* Watchdog Timer Interrupt Status */ +#define TSR_ENW (1 << 31) /* Enable Next Watchdog Timer */ + +typedef struct booke_timer_t booke_timer_t; +struct booke_timer_t { + int decr_excp; + + int fit_excp; + uint64_t fit_next; + struct QEMUTimer *fit_timer; + + int wdt_excp; + uint64_t wdt_next; + struct QEMUTimer *wdt_timer; +}; + +/* 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; + } + + 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; +} + +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); + } else { + lapse = tb_target - tb; + *next = now + muldiv64(lapse, get_ticks_per_sec(), tb_env->tb_freq); + + if (*next == now) + (*next)++; + + qemu_mod_timer(timer, *next); + } +} + +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]); + } +} + +static void booke_fit_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_FIS; + if (env->spr[SPR_BOOKE_TCR] & TCR_FIE) { + ppc_set_irq(env, booke_timer->fit_excp, 1); + } + + booke_update_fixed_timer(env, + booke_get_fit_target(env), + &booke_timer->fit_next, + booke_timer->fit_timer); +} + +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); +} + +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); + + 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); + } +} + +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); +} + +void ppc_booke_timers_init (CPUState *env, uint32_t freq) +{ + ppc_tb_t *tb_env; + booke_timer_t *booke_timer; + + tb_env = qemu_mallocz(sizeof(ppc_tb_t)); + booke_timer = qemu_mallocz(sizeof(booke_timer_t)); + + env->tb_env = tb_env; + + tb_env->tb_freq = freq; + tb_env->decr_freq = freq; + tb_env->opaque = booke_timer; + tb_env->decr_timer = qemu_new_timer_ns(vm_clock, &booke_decr_cb, env); + + booke_timer->decr_excp = PPC_INTERRUPT_DECR; + booke_timer->fit_excp = PPC_INTERRUPT_FIT; + booke_timer->wdt_excp = PPC_INTERRUPT_WDT; + + booke_timer->fit_timer = + qemu_new_timer_ns(vm_clock, &booke_fit_cb, env); + booke_timer->wdt_timer = + qemu_new_timer_ns(vm_clock, &booke_wdt_cb, env); +} diff --git a/hw/ppce500_mpc8544ds.c b/hw/ppce500_mpc8544ds.c index 6b57fbf..83672c9 100644 --- a/hw/ppce500_mpc8544ds.c +++ b/hw/ppce500_mpc8544ds.c @@ -237,9 +237,7 @@ static void mpc8544ds_init(ram_addr_t ram_size, exit(1); } - /* XXX register timer? */ - ppc_emb_timers_init(env, 400000000, PPC_INTERRUPT_DECR); - ppc_dcr_init(env, NULL, NULL); + ppc_booke_timers_init(env, 400000000); /* Register reset handler */ qemu_register_reset(mpc8544ds_cpu_reset, env);
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). This is a first attempt for a distinct and clean implementation of booke's timers. Please feel free to comment. Signed-off-by: Fabien Chouteau <chouteau@adacore.com> --- Makefile.target | 2 +- hw/ppc.c | 45 +-------- hw/ppc.h | 23 +++++ hw/ppc_booke.c | 254 ++++++++++++++++++++++++++++++++++++++++++++++++ hw/ppce500_mpc8544ds.c | 4 +- 5 files changed, 281 insertions(+), 47 deletions(-) create mode 100644 hw/ppc_booke.c