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

[Fabien Chouteau]

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).

Regards,

-- 
Fabien Chouteau