Re: [Qemu-arm] [PATCH] Added periodic IRQ support for bcm2836_control local timer

[bzt]

Hi,

Thanks for your answers. If I don't clear the INTENABLE flag, then the
IRQ would keep firing constantly. This is not how the real hardware
works: it triggers the IRQ once, and then it inhibits. The timer won't
trigger the IRQ again until you acknowledge it by writing the INTFLAG
into the ack register. My solution emulates this behaviour. That's
what the triggered flag was for in my original patch. Should I bring
that flag back or is the current solution acceptable knowing this?

About keeping the INTFLAG on control write that's to avoid an instant
IRQ, but that's OK. I'll modify that.

Thanks,
bzt


On 3/7/19, Peter Maydell <address@hidden> wrote:
> On Thu, 7 Mar 2019 at 15:57, bzt <address@hidden> wrote:
>>
>> Nope. I meant the second patch, sent on 4th Mar, which had all the
>> things fixed you listed in your review.
>>
>> But here's the modification for your latest query. This one controls
>> the timer depending on ENABLE bit. It sets the INTFLAG even if
>> INTENABLE is not set, and only raises the IRQ if both are set.
>
> Thanks. I've listed a couple of minor things below
> which I think are not quite handling the flags right,
> but the rest looks good.
>
>> @@ -78,6 +94,17 @@ static void bcm2836_control_update(BCM2836ControlState
>> *s)
>>          s->fiqsrc[s->route_gpu_fiq] |= (uint32_t)1 << IRQ_GPU;
>>      }
>>
>> +    /* handle THE local timer interrupt for one of the cores' IRQ/FIQ */
>> +    if ((s->local_timer_control & LOCALTIMER_INTENABLE) &&
>> +        (s->local_timer_control & LOCALTIMER_INTFLAG)) {
>> +        if (s->route_localtimer & 4) {
>> +            s->fiqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 <<
>> IRQ_TIMER;
>> +        } else {
>> +            s->irqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 <<
>> IRQ_TIMER;
>> +        }
>> +        s->local_timer_control &= ~LOCALTIMER_INTENABLE;
>
> This shouldn't clear the INTENABLE flag.
>
>> +    }
>> +
>>      for (i = 0; i < BCM2836_NCORES; i++) {
>>          /* handle local timer interrupts for this core */
>>          if (s->timerirqs[i]) {
>> @@ -162,6 +189,55 @@ static void bcm2836_control_set_gpu_fiq(void
>> *opaque, int irq, int level)
>>      bcm2836_control_update(s);
>>  }
>
>> +static void bcm2836_control_local_timer_control(void *opaque, uint32_t
>> val)
>> +{
>> +    BCM2836ControlState *s = opaque;
>> +
>> +    s->local_timer_control = val & ~LOCALTIMER_INTFLAG;
>
> This will clear the LOCALTIMER_INTFLAG if it was already
> set -- you want to preserve it, something like
>    s->local_timer_control = (s->local_timer_control & LOCALTIMER_INTFLAG) |
>      (val & ~LOCALTIMER_INTFLAG);
>
>> +    if (val & LOCALTIMER_ENABLE) {
>> +        bcm2836_control_local_timer_set_next(s);
>> +    } else {
>> +        timer_del(&s->timer);
>> +    }
>> +}
>> +
>> +static void bcm2836_control_local_timer_ack(void *opaque, uint32_t val)
>> +{
>> +    BCM2836ControlState *s = opaque;
>> +
>> +    if (val & LOCALTIMER_INTFLAG) {
>> +        s->local_timer_control |= LOCALTIMER_INTENABLE;
>> +        s->local_timer_control &= ~LOCALTIMER_INTFLAG;
>
> This should just clear the INTFLAG, it doesn't affect INTENABLE.
>
>> +    }
>> +    if ((val & LOCALTIMER_RELOAD) &&
>> +        (s->local_timer_control & LOCALTIMER_ENABLE)) {
>> +            bcm2836_control_local_timer_set_next(s);
>> +    }
>> +}
>
> thanks
> -- PMM
>