From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:60911) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h2QOV-0001yM-KX for qemu-devel@nongnu.org; Fri, 08 Mar 2019 20:03:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1h2QOU-0006oe-Ox for qemu-devel@nongnu.org; Fri, 08 Mar 2019 20:03:19 -0500 MIME-Version: 1.0 In-Reply-To: References: From: bzt Date: Sat, 9 Mar 2019 02:03:13 +0100 Message-ID: Content-Type: text/plain; charset="UTF-8" Subject: Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Andrew Baumann , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= , qemu-arm , QEMU Developers 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 wrote: > On Thu, 7 Mar 2019 at 15:57, bzt 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 >