diff mbox

nvic: allow to set pending status for not active interrupts

Message ID 1475833331-26882-1-git-send-email-marcin.krzeminski@nokia.com (mailing list archive)
State New, archived
Headers show

Commit Message

marcin.krzeminski@nokia.com Oct. 7, 2016, 9:42 a.m. UTC
From: Marcin Krzeminski <marcin.krzeminski@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.krzeminski@nokia.com>
---
 hw/intc/arm_gic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Peter Maydell Oct. 13, 2016, 4:59 p.m. UTC | #1
On 7 October 2016 at 10:42,  <marcin.krzeminski@nokia.com> wrote:
> From: Marcin Krzeminski <marcin.krzeminski@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.krzeminski@nokia.com>
> ---
>  hw/intc/arm_gic.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index b30cc91..85be6e4 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -147,7 +147,8 @@ static void gic_set_irq_11mpcore(GICState *s, int irq, int level,
>  {
>      if (level) {
>          GIC_SET_LEVEL(irq, cm);
> -        if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) {
> +        if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)
> +                || (!GIC_TEST_ACTIVE(irq, cm) &&  s->revision == REV_NVIC)) {
>              DPRINTF("Set %d pending mask %x\n", irq, target);
>              GIC_SET_PENDING(irq, target);
>          }
> --

Thanks for this patch. I agree that the current behaviour isn't
correct.

I think it would be cleaner to define a new gic_set_irq_nvic() which
has the NVIC specific behaviour, rather than sticking an "if this
is really an NVIC" check into this function.

You probably also want to check whether the logic for re-pending
level triggered interrupts in gic_complete_irq() also needs a
similar change:

    if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
        /* Mark level triggered interrupts as pending if they are still
           raised.  */
        if (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm)
            && GIC_TEST_LEVEL(irq, cm) && (GIC_TARGET(irq) & cm) != 0) {
            DPRINTF("Set %d pending mask %x\n", irq, cm);
            GIC_SET_PENDING(irq, cm);
        }
    }

thanks
-- PMM
marcin.krzeminski@nokia.com Oct. 17, 2016, 2:33 p.m. UTC | #2
Hi Peter

> -----Original Message-----

> From: Peter Maydell [mailto:peter.maydell@linaro.org]

> Sent: Thursday, October 13, 2016 6:59 PM

> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)

> <marcin.krzeminski@nokia.com>

> Cc: QEMU Developers <qemu-devel@nongnu.org>; qemu-arm <qemu-

> arm@nongnu.org>; rfsw-patches@mlist.nokia.com

> Subject: Re: [PATCH] nvic: allow to set pending status for not active

> interrupts

> 

> On 7 October 2016 at 10:42,  <marcin.krzeminski@nokia.com> wrote:

> > From: Marcin Krzeminski <marcin.krzeminski@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.krzeminski@nokia.com>

> > ---

> >  hw/intc/arm_gic.c | 3 ++-

> >  1 file changed, 2 insertions(+), 1 deletion(-)

> >

> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index

> > b30cc91..85be6e4 100644

> > --- a/hw/intc/arm_gic.c

> > +++ b/hw/intc/arm_gic.c

> > @@ -147,7 +147,8 @@ static void gic_set_irq_11mpcore(GICState *s, int

> > irq, int level,  {

> >      if (level) {

> >          GIC_SET_LEVEL(irq, cm);

> > -        if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) {

> > +        if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)

> > +                || (!GIC_TEST_ACTIVE(irq, cm) &&  s->revision ==

> > + REV_NVIC)) {

> >              DPRINTF("Set %d pending mask %x\n", irq, target);

> >              GIC_SET_PENDING(irq, target);

> >          }

> > --

> 

> Thanks for this patch. I agree that the current behaviour isn't correct.

> 

> I think it would be cleaner to define a new gic_set_irq_nvic() which has the

> NVIC specific behaviour, rather than sticking an "if this is really an NVIC"

> check into this function.

>

Sure.

> You probably also want to check whether the logic for re-pending level

> triggered interrupts in gic_complete_irq() also needs a similar change:

> 

>     if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {

>         /* Mark level triggered interrupts as pending if they are still

>            raised.  */

>         if (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm)

>             && GIC_TEST_LEVEL(irq, cm) && (GIC_TARGET(irq) & cm) != 0) {

>             DPRINTF("Set %d pending mask %x\n", irq, cm);

>             GIC_SET_PENDING(irq, cm);

>         }

>     }

It seem thet level triggered interrupt could be removed from fi for NVIC.
I do not have guest to model that, but I will at least check if this
do not brake anything.

Thanks,
Marcin 
> 

> thanks

> -- PMM
diff mbox

Patch

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index b30cc91..85be6e4 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -147,7 +147,8 @@  static void gic_set_irq_11mpcore(GICState *s, int irq, int level,
 {
     if (level) {
         GIC_SET_LEVEL(irq, cm);
-        if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) {
+        if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)
+                || (!GIC_TEST_ACTIVE(irq, cm) &&  s->revision == REV_NVIC)) {
             DPRINTF("Set %d pending mask %x\n", irq, target);
             GIC_SET_PENDING(irq, target);
         }