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