Re: [Qemu-devel] [RESEND][PATCH] booke timers

[Alexander Graf]

On 09.09.2011, at 16:22, Fabien Chouteau wrote:

> On 09/09/2011 15:46, Alexander Graf wrote:
>> 
>> On 09.09.2011, at 15:27, Fabien Chouteau wrote:
>> 
>>> On 09/09/2011 12:55, Alexander Graf wrote:
>>>> 
>>>> On 09.09.2011, at 12:36, Fabien Chouteau wrote:
>>>>>>> 
>>>>>>>> 
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    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, PPC_INTERRUPT_DECR, 1);
>>>>>>>>> +    }
>>>>>>>> 
>>>>>>>> You will need this more often - also for the TCR write case - so 
>>>>>>>> please put
>>>>>>>> the 3 lines above into a separate function and just call that here :)
>>>>>>> 
>>>>>>> Actually the checks are never exactly the same, here we test DIE in 
>>>>>>> TCR...
>>>>>> 
>>>>>> Sure, we have 3 different tests for the 3 different bits we can 
>>>>>> potentially set in TCR. The check always ends up being the same though:
>>>>>> 
>>>>>> if (TSR & bit && TCR & bit)
>>>>>>  set_irq(irq_for_bit);
>>>>>> 
>>>>>> Most device emulators have a simple function for this called 
>>>>>> "update_irq" that checks for all the bits and sets the respective irq 
>>>>>> lines.
>>>>>> 
>>>>> 
>>>>> I know but we have two cases:
>>>>> 
>>>>> - Timer hit: we check DIE in TCR
>>>>> - Rising edge of DIE in TCR (from user): check if DIS is set
>>>>> 
>>>>> I don't think we can have a good generic function for this, and I don't
>>>>> forecast any improvement in code readability.
>>>> 
>>>> update_decr_irq() {
>>>> if (TSR.DIS && TCR.DIE) {
>>>>   set_irq(DECR);
>>>> } else {
>>>>   unset_irq(DECR);
>>>> }
>>>> }
>>>> 
>>>> Timer hit:
>>>> 
>>>> TSR |= DIS;
>>>> update_decr_irq();
>>>> 
>>>> Setting TCR:
>>>> 
>>>> TCR |= DIE;
>>>> update_decr_irq();
>>>> 
>>>> Or am I misunderstanding the conditions under which the irq actually
>>>> triggers? TCR.DIE is only the interrupt enabled flag - the timer can still
>>>> hit nevertheless. The level of the interrupt is determined by TSR.DIR which
>>>> is what the timer sets when it hits. Unless I completely misread the spec, 
>>>> an
>>>> interrupt occurs when both of them are true. So all we need to do is have
>>>> that check and run it every time we change a value in TSR or TCR.
>>> 
>>> Well OK, this can work to trigger the interrupts, not to clear them though.
>>> And it will call ppc_set_irq when it's not required.
>> 
>> Calling ppc_set_irq shouldn't be an issue - at the end of the day it only
>> does a simple OR operation. Unsetting should be pretty obvious too though,
>> no? When either TSR.DIS or TCR.DIE are not set, the interrupt line is low.
>> 
> 
> if the interrupt is already set and you clear TCR.DIE, the interrupt has to
> remain set. The only way to unset an interrupt is to clear the corresponding
> bit in TSR (currently in store_booke_tsr).

Are you sure? I see several things in the 2.06 spec:

TSR.DIS:

0       A Decrementer event has not occurred.
1       A Decrementer event has occurred.

when MSR.EE=1 and TCR.DIE=1, a Decrementer interrupt is taken.

TCR.DIE:

0       Disable Decrementer interrupt
1       Enable Decrementer interrupt

Decrement to one and stop on zero

If TCR.ARE=0, TSR.DIS is set to 1, the value 0x0000_0000 is then placed into 
the DEC, and the Decrementer stops decrementing.
A Decrementer interrupt occurs when no higher priority interrupt exists, a 
Decrementer exception exists, and the exception is enabled.
The interrupt is enabled by TCR.DIE=1 and MSR.EE=1.

See     Section 7.6.12, “Decrementer Interrupt” on page 1021 for details of 
register behavior caused by the Decrementer inter- rupt.

7.6.12  Decrementer Interrupt
A Decrementer interrupt occurs when no higher priority interrupt exists (see 
Section 7.9 on page 1037), a Decrementer exception exists (TSR.DIS=1), and the 
exception is enabled. The interrupt is enabled by TCR.DIE=1 and MSR.EE=1. See 
Section 9.3 on page 1047.

Software is responsible for clearing the Decre- menter exception status prior 
to re-enabling the MSR.EE bit in order to avoid another redundant Decrementer 
interrupt. To clear the Decrementer exception, the interrupt handling routine 
must clear TSR.DIS. Clearing is done by writing a word to TSR using mtspr with 
a 1 in any bit position that is to be cleared and 0 in all other bit positions. 
The write-data to the TSR is not direct data, but a mask. A 1 causes the bit to 
be cleared, and a 0 has no effect.


To me that sounds as if the decrementer interrupt gets injected only when 
TSR.DIS=1, TCR.DIE=1 and MSR.EE=1. Unsetting any of these bits stops the 
interrupt from being delivered.

Scott, can you please check up with the hardware guys if this is correct?


Alex