Re: [Qemu-devel] [PATCH V2] [PowerPC][RFC] booke timers

2011-07-05 Thread Scott Wood 
On Tue, 5 Jul 2011 18:41:13 +0200
Fabien Chouteau  wrote:

> On 05/07/2011 18:02, Scott Wood wrote:
> > On Mon, 4 Jul 2011 17:06:54 +0200
> > Fabien Chouteau  wrote:
> >> Do you mean "lapse = period - ((tb - (1 << target_bit)) & (period - 1));" ?
> > 
> > Yes.
> > 
> > Or more simply:
> > 
> > lapse = period - ((tb - period) & (period - 1));
> > 
> 
> Are you sure? Note that period != (1 << target_bit).

Ah, right -- misread that.

-Scott

Re: [Qemu-devel] [PATCH V2] [PowerPC][RFC] booke timers

2011-07-05 Thread Fabien Chouteau 
On 05/07/2011 18:02, Scott Wood wrote:
> On Mon, 4 Jul 2011 17:06:54 +0200
> Fabien Chouteau  wrote:
> 
>> On 01/07/2011 22:22, Scott Wood wrote:
>>> On Fri, 1 Jul 2011 16:13:41 +0200
>>> Fabien Chouteau  wrote:
 +static void booke_update_fixed_timer(CPUState *env,
 + uint8_t   target_bit,
 + uint64_t  *next,
 + struct QEMUTimer *timer)
 +{
 +ppc_tb_t *tb_env = env->tb_env;
 +uint64_t lapse;
 +uint64_t tb;
 +uint64_t period = 1 << (target_bit + 1);
 +uint64_t now;
 +
 +now = qemu_get_clock_ns(vm_clock);
 +tb  = cpu_ppc_get_tb(tb_env, now, tb_env->tb_offset);
 +
 +if (tb <= (1 << target_bit)) {
 +lapse = (1 << target_bit) - tb;
 +} else {
 +lapse = period - ((tb - (1 << target_bit)) % period);
>>>
>>> We know period is a power of two, so just do "& (period - 1)".
>>>
>>> That should let you get rid of the special case for
>>> "tb <= (1 << target_bit)" as well.
>>>
>>
>> Do you mean "lapse = period - ((tb - (1 << target_bit)) & (period - 1));" ?
> 
> Yes.
> 
> Or more simply:
> 
> lapse = period - ((tb - period) & (period - 1));
> 

Are you sure? Note that period != (1 << target_bit).

>> I don't see how this solves the "tb <= (1 << target_bit)" case.
> 
> Actually, since everything is unsigned the special case shouldn't be needed
> regardless.

You're right about this one, it's tricky though :)

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH V2] [PowerPC][RFC] booke timers

2011-07-05 Thread Scott Wood 
On Mon, 4 Jul 2011 17:06:54 +0200
Fabien Chouteau  wrote:

> On 01/07/2011 22:22, Scott Wood wrote:
> > On Fri, 1 Jul 2011 16:13:41 +0200
> > Fabien Chouteau  wrote:
> >> +static void booke_update_fixed_timer(CPUState *env,
> >> + uint8_t   target_bit,
> >> + uint64_t  *next,
> >> + struct QEMUTimer *timer)
> >> +{
> >> +ppc_tb_t *tb_env = env->tb_env;
> >> +uint64_t lapse;
> >> +uint64_t tb;
> >> +uint64_t period = 1 << (target_bit + 1);
> >> +uint64_t now;
> >> +
> >> +now = qemu_get_clock_ns(vm_clock);
> >> +tb  = cpu_ppc_get_tb(tb_env, now, tb_env->tb_offset);
> >> +
> >> +if (tb <= (1 << target_bit)) {
> >> +lapse = (1 << target_bit) - tb;
> >> +} else {
> >> +lapse = period - ((tb - (1 << target_bit)) % period);
> > 
> > We know period is a power of two, so just do "& (period - 1)".
> > 
> > That should let you get rid of the special case for
> > "tb <= (1 << target_bit)" as well.
> > 
> 
> Do you mean "lapse = period - ((tb - (1 << target_bit)) & (period - 1));" ?

Yes.

Or more simply:

lapse = period - ((tb - period) & (period - 1));

> I don't see how this solves the "tb <= (1 << target_bit)" case.

Actually, since everything is unsigned the special case shouldn't be needed
regardless.

-Scott

Re: [Qemu-devel] [PATCH V2] [PowerPC][RFC] booke timers

2011-07-04 Thread Fabien Chouteau 
On 01/07/2011 22:22, Scott Wood wrote:
> On Fri, 1 Jul 2011 16:13:41 +0200
> Fabien Chouteau  wrote:
>> +static void booke_update_fixed_timer(CPUState *env,
>> + uint8_t   target_bit,
>> + uint64_t  *next,
>> + struct QEMUTimer *timer)
>> +{
>> +ppc_tb_t *tb_env = env->tb_env;
>> +uint64_t lapse;
>> +uint64_t tb;
>> +uint64_t period = 1 << (target_bit + 1);
>> +uint64_t now;
>> +
>> +now = qemu_get_clock_ns(vm_clock);
>> +tb  = cpu_ppc_get_tb(tb_env, now, tb_env->tb_offset);
>> +
>> +if (tb <= (1 << target_bit)) {
>> +lapse = (1 << target_bit) - tb;
>> +} else {
>> +lapse = period - ((tb - (1 << target_bit)) % period);
> 
> We know period is a power of two, so just do "& (period - 1)".
> 
> That should let you get rid of the special case for
> "tb <= (1 << target_bit)" as well.
> 

Do you mean "lapse = period - ((tb - (1 << target_bit)) & (period - 1));" ?

I don't see how this solves the "tb <= (1 << target_bit)" case.

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH V2] [PowerPC][RFC] booke timers

2011-07-01 Thread Scott Wood 
On Fri, 1 Jul 2011 16:13:41 +0200
Fabien Chouteau  wrote:

> +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;
> @@ -678,18 +661,23 @@ static void __cpu_ppc_store_decr (CPUState *env, 
> uint64_t *nextp,
>  decr, value);
>  now = qemu_get_clock_ns(vm_clock);
>  next = now + muldiv64(value, get_ticks_per_sec(), tb_env->decr_freq);
> -if (is_excp)
> +if (is_excp) {
>  next += *nextp - now;
> -if (next == now)
> +}
> +if (next == now) {
>  next++;
> +}
>  *nextp = next;
>  /* Adjust timer */
>  qemu_mod_timer(timer, next);
>  /* If we set a negative value and the decrementer was positive,
> - * raise an exception.
> + * raise an exception (not for booke).
>   */
> -if ((value & 0x8000) && !(decr & 0x8000))
> +if (! (env->insns_flags & PPC_BOOKE)
> +&& (value & 0x8000)
> +&& !(decr & 0x8000)) {
>  (*raise_excp)(env);
> +}
>  }
>  

Also, load_decr should go from this:

if (diff >= 0)
decr = muldiv64(diff, tb_env->decr_freq, get_ticks_per_sec());
else
decr = -muldiv64(-diff, tb_env->decr_freq, get_ticks_per_sec());

to something like this:

if (diff >= 0) {
decr = muldiv64(diff, tb_env->decr_freq, get_ticks_per_sec());
} else if (env->insns_flags & PPC_BOOKE) {
decr = 0;
} else {
decr = -muldiv64(-diff, tb_env->decr_freq, get_ticks_per_sec());
}

> +/* Return the location of the bit of time base at which the FIT will raise an
> +   interrupt */
> +static uint8_t booke_get_fit_target(CPUState *env)
> +{
> +uint8_t fp = (env->spr[SPR_BOOKE_TCR] & TCR_FP_MASK) >> TCR_FP_SHIFT;
> +
> +/* Only for e500 */
> +if (env->insns_flags2 & PPC2_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 fp;
> +}
> +
> +/* Return the location of the bit of time base at which the WDT will raise an
> +   interrupt */
> +static uint8_t booke_get_wdt_target(CPUState *env)
> +{
> +uint8_t wp = (env->spr[SPR_BOOKE_TCR] & TCR_WP_MASK) >> TCR_WP_SHIFT;
> +
> +/* Only for e500 */
> +if (env->insns_flags2 & PPC2_E500) {
> +uint32_t wpext = (env->spr[SPR_BOOKE_TCR] & TCR_E500_WPEXT_MASK)
> +>> TCR_E500_WPEXT_SHIFT;
> +wp |= wpext << 2;
> +} else {
> +wp = env->wdt_period[wp];
> +}
> +
> +return wp;
> +}

e500 fp/wp is expressed as bits from the MSB side of TB, so we need to
subtract from 63 to get something sane -- and document that fit/wdt_period
is from the LSB side.

> +static void booke_update_fixed_timer(CPUState *env,
> + uint8_t   target_bit,
> + uint64_t  *next,
> + struct QEMUTimer *timer)
> +{
> +ppc_tb_t *tb_env = env->tb_env;
> +uint64_t lapse;
> +uint64_t tb;
> +uint64_t period = 1 << (target_bit + 1);
> +uint64_t now;
> +
> +now = qemu_get_clock_ns(vm_clock);
> +tb  = cpu_ppc_get_tb(tb_env, now, tb_env->tb_offset);
> +
> +if (tb <= (1 << target_bit)) {
> +lapse = (1 << target_bit) - tb;
> +} else {
> +lapse = period - ((tb - (1 << target_bit)) % period);

We know period is a power of two, so just do "& (period - 1)".

That should let you get rid of the special case for
"tb <= (1 << target_bit)" as well.

-Scott

[Qemu-devel] [PATCH V2] [PowerPC][RFC] booke timers

2011-07-01 Thread Fabien Chouteau 
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.

V2:
  - Fix fixed timer, now trigger each time the selected
bit switch from 0 to 1.
  - Fix e500 criterion.
  - Trigger an interrupt when user set DIE/FIE/WIE while
DIS/FIS/WIS is already set.
  - Minor fixes (mask definition, variable name...).
  - rename ppc_emb to ppc_40x

Signed-off-by: Fabien Chouteau 
---
 Makefile.target |2 +-
 hw/ppc.c|  126 +++-
 hw/ppc.h|   25 -
 hw/ppc4xx_devs.c|2 +-
 hw/ppc_booke.c  |  266 +++
 hw/ppce500_mpc8544ds.c  |4 +-
 hw/virtex_ml507.c   |   11 +--
 target-ppc/cpu.h|   22 
 target-ppc/helper.c |   12 ++-
 target-ppc/translate_init.c |   43 +++-
 10 files changed, 412 insertions(+), 101 deletions(-)
 create mode 100644 hw/ppc_booke.c

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..f13f4ea 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;
@@ -678,18 +661,23 @@ static void __cpu_ppc_store_decr (CPUState *env, uint64_t 
*nextp,
 decr, value);
 now = qemu_get_clock_ns(vm_clock);
 next = now + muldiv64(value, get_ticks_per_sec(), tb_env->decr_freq);
-if (is_excp)
+if (is_excp) {
 next += *nextp - now;
-if (next == now)
+}
+if (next == now) {
 next++;
+}
 *nextp = next;
 /* Adjust timer */
 qemu_mod_timer(timer, next);
 /* If we set a negative value and the decrementer was positive,
- * raise an exception.
+ * raise an exception (not for booke).
  */
-if ((value & 0x8000) && !(decr & 0x8000))
+if (! (env->insns_flags & PPC_BOOKE)
+&& (value & 0x8000)
+&& !(decr & 0x8000)) {
 (*raise_excp)(env);
+}
 }
 
 static inline void _cpu_ppc_store_decr(CPUState *env, uint32_t decr,
@@ -806,11 +794,11 @@ uint32_t cpu_ppc601_load_rtcl (CPUState *env)
 }
 
 /*/
-/* Embedded PowerPC timers */
+/* PowerPC 40x timers */
 
 /* PIT, FIT & WDT */
-typedef struct ppcemb_timer_t ppcemb_timer_t;
-struct ppcemb_timer_t {
+typedef struct ppc40x_timer_t ppc40x_timer_t;
+struct ppc40x_timer_t {
 uint64_t pit_reload;  /* PIT auto-reload value*/
 uint64_t fit_next;/* Tick for next FIT interrupt  */
 struct QEMUTimer *fit_timer;
@@ -826,12 +814,12 @@ static void cpu_4xx_fit_cb (void *opaque)
 {
 CPUState *env;
 ppc_tb_t *tb_env;
-ppcemb_timer_t *ppcemb_timer;
+ppc40x_timer_t *ppc40x_timer;
 uint64_t now, next;
 
 env = opaque;
 tb_env = env->tb_env;
-ppcemb_timer = tb_env->opaque;
+ppc40x_timer = tb_env->opaque;
 now = qemu_get_clock_ns(vm_cloc