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

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 7 Mar 2019 at 15:57, bzt <bztemail@gmail.com> 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