Peter, > -----Original Message----- > From: Peter Maydell [mailto:peter.mayd...@linaro.org] > Sent: Monday, October 24, 2016 5:26 PM > To: Krzeminski, Marcin (Nokia - PL/Wroclaw) > <marcin.krzemin...@nokia.com> > Cc: QEMU Developers <qemu-devel@nongnu.org>; qemu-arm <qemu- > a...@nongnu.org>; rfsw-patc...@mlist.nokia.com > Subject: Re: [v2] nvic: set pending status for not active interrupts > > On 18 October 2016 at 07:14, <marcin.krzemin...@nokia.com> wrote: > > From: Marcin Krzeminski <marcin.krzemin...@nokia.com> > > > > According to ARM DUI 0552A 4.2.10. NVIC set pending status also for > > disabled interrupts. This patch adds possibility to emulate this in > > Qemu. > > > > Signed-off-by: Marcin Krzeminski <marcin.krzemin...@nokia.com> > > Thanks for rolling a v2. Unfortunately I think we don't have the logic quite > right yet... > Yeap, I separated it but not update to work only with NVIC. > > --- > > Changes for v2: > > - add a dedicated unction for nvic > > - update complete_irq > > hw/intc/arm_gic.c | 27 +++++++++++++++++++++++++-- > > 1 file changed, 25 insertions(+), 2 deletions(-) > > > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index > > b30cc91..72e4c01 100644 > > --- a/hw/intc/arm_gic.c > > +++ b/hw/intc/arm_gic.c > > @@ -156,6 +156,21 @@ static void gic_set_irq_11mpcore(GICState *s, int > irq, int level, > > } > > } > > > > +static void gic_set_irq_nvic(GICState *s, int irq, int level, > > + int cm, int target) { > > + if (level) { > > + GIC_SET_LEVEL(irq, cm); > > + if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm) > > + || !GIC_TEST_ACTIVE(irq, cm)) { > > + DPRINTF("Set %d pending mask %x\n", irq, target); > > + GIC_SET_PENDING(irq, target); > > + } > > Why is GIC_TEST_ENABLED() in this condition, when the idea is that we don't > care about the enabled status for whether we can pend the interrupt? > > I think this should actually just always do > GIC_SET_PENDING(irq, target) > whenever level is high > > because: > (1) pending status can be set for disabled interrupts > (2) edge-triggered IRQs definitely always re-pend on rising edge > (3) I think level-triggered IRQs do too (it's a bit > less clear in the docs, but DUI0552A 4.2.9 says we pend on > a rising edge and doesn't say that applies only to edge > triggered IRQs) > > I don't have any real hardware to hand to test against, though. > Yes, it works exactly as you're saying (checked on HW), if level is still high after exit interrupt handler, interrupt is re-pend. > > + } else { > > + GIC_CLEAR_LEVEL(irq, cm); > > + } > > +} > > + > > static void gic_set_irq_generic(GICState *s, int irq, int level, > > int cm, int target) { @@ -201,8 > > +216,10 @@ static void gic_set_irq(void *opaque, int irq, int level) > > return; > > } > > > > - if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) { > > + if (s->revision == REV_11MPCORE) { > > gic_set_irq_11mpcore(s, irq, level, cm, target); > > + } else if (s->revision == REV_NVIC) { > > + gic_set_irq_nvic(s, irq, level, cm, target); > > } else { > > gic_set_irq_generic(s, irq, level, cm, target); > > } > > @@ -568,7 +585,7 @@ void gic_complete_irq(GICState *s, int cpu, int irq, > MemTxAttrs attrs) > > return; /* No active IRQ. */ > > } > > > > - if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) { > > + if (s->revision == REV_11MPCORE) { > > /* Mark level triggered interrupts as pending if they are still > > raised. */ > > if (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm) > > @@ -576,6 +593,12 @@ void gic_complete_irq(GICState *s, int cpu, int irq, > MemTxAttrs attrs) > > DPRINTF("Set %d pending mask %x\n", irq, cm); > > GIC_SET_PENDING(irq, cm); > > } > > + } else if (s->revision == REV_NVIC) { > > + if (GIC_TEST_ENABLED(irq, cm) && GIC_TEST_LEVEL(irq, cm) > > + && (GIC_TARGET(irq) & cm) != 0) { > > + DPRINTF("Set nvic %d pending mask %x\n", irq, cm); > > + GIC_SET_PENDING(irq, cm); > > + } > > Similarly, I think this should just be > if (GIC_TEST_LEVEL(irq, cm) { > GIC_SET_PENDING(irq, cm); > } > > because: > (1) for the NVIC there is only one CPU and GIC_TARGET(irq) always indicates > that IRQs target it > (2) we don't care whether the interrupt is enabled when deciding whether > it should be pended > (3) we don't care whether the interrupt is level-triggered or edge-triggered > for deciding whether to re-pend it (see statement R_XVWM in the v8M > ARM ARM DDI0553A.c) > Same as above. I'll send v3.
Thanks, Marcin > > } > > > > group = gic_has_groups(s) && GIC_TEST_GROUP(irq, cm); > > -- > > 2.7.4 > > thanks > -- PMM