Re: [Xen-devel] [PATCH 1/2] xen: arm: log warning for interrupt configuration mismatch

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Sat, 2015-02-28 at 22:12 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 19/02/2015 15:24, Ian Campbell wrote:
> > The ICFGR register is not necessarily writeable, in particular it is
> > IMPLEMENTATION DEFINED for a PPI if the configuration register is
> > writeable. Log a warning if the hardware has ignored our write and
> > update the actual type in the irq descriptor so subsequent code can do
> > the right thing.
> >
> > This most likely implies a buggy firmware description (e.g.
> > device-tree).
> >
> > The issue is observed for example on the APM Mustang board where the
> > device tree (as shipped by Linux) describes all 3 timer interrupts as
> > rising edge but the PPI is hard-coded to level triggered (as makes
> > sense for an arch timer interrupt).
> 
> BTW the cavium device tree also use edge-triggered. I guess this is an 
> error in the device tree?
> 
> >
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > Cc: Pranavkumar Sawargaonkar <psawargaonkar@xxxxxxx>
> > ---
> >   xen/arch/arm/gic-v2.c |   16 +++++++++++++++-
> >   xen/arch/arm/gic-v3.c |   16 +++++++++++++++-
> >   2 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index 31fb81a..6836ab6 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -211,7 +211,7 @@ static void gicv2_set_irq_properties(struct irq_desc 
> > *desc,
> >                                      const cpumask_t *cpu_mask,
> >                                      unsigned int priority)
> >   {
> > -    uint32_t cfg, edgebit;
> > +    uint32_t cfg, actual, edgebit;
> >       unsigned int mask = gicv2_cpu_mask(cpu_mask);
> >       unsigned int irq = desc->irq;
> >       unsigned int type = desc->arch.type;
> > @@ -229,6 +229,20 @@ static void gicv2_set_irq_properties(struct irq_desc 
> > *desc,
> >           cfg |= edgebit;
> >       writel_gicd(cfg, GICD_ICFGR + (irq / 16) * 4);
> >
> > +    actual = readl_gicd(GICD_ICFGR + (irq / 16) * 4);
> > +    if ( ( cfg & edgebit ) ^ ( actual & edgebit ) )
> > +    {
> > +        printk(XENLOG_WARNING "GICv2: WARNING: "
> > +               "CPU%d: Failed to configure IRQ%u as %s-triggered. "
> > +               "H/w forces to %s-triggered.\n",
> > +               smp_processor_id(), desc->irq,
> > +               cfg & edgebit ? "Edge" : "Level",
> > +               actual & edgebit ? "Edge" : "Level");
> > +        desc->arch.type = actual & edgebit ?
> > +            DT_IRQ_TYPE_EDGE_RISING :
> > +            DT_IRQ_TYPE_LEVEL_LOW;
> 
> I got some error with the interrupts configuration on FreeBSD and after 
> reading the spec and the device tree bindings, level low is invalid for 
> SPIs.

You mean the gic spec? I think the DT allows for both.

> SPIs can only be low-to-high edge triggered and high-level sensitive.

I even reread the spec before sending and reached the same conclusion,
but apparently managed to fail to change the actual code!

Question is -- what to do about PPIs, since the CFG register cannot
express the polarity of the edge or level. I suppose we may as well just
assume the one that is compatible with SPIs, since we have nothing
better to go on.

Making that assumption results in the following patch. Do you still have
the spec(s) open to the right page, in which case can you tell me the
section numbers or do I need to go find them again?

Ian.

8<-------

From 852f6e3fe49f7fab801f2857b8b505922556d746 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@xxxxxxxxxx>
Date: Mon, 2 Mar 2015 11:09:35 +0000
Subject: [PATCH] xen: arm: Assume level triggered means high, not low.

When reading back the ICFG register we cannot know the polarity of the
configuration, just that it is level or edge.

Since falling edge and low level are invalid for SPIs we should assume
rising edge and high level (we have no better information for PPIs, so
it'll have to do).

We already assumed rising edge, switch to high level as well.

Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
---
 xen/arch/arm/gic-v2.c |    2 +-
 xen/arch/arm/gic-v3.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index c05b64a..d88c6a9 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -240,7 +240,7 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
                actual & edgebit ? "Edge" : "Level");
         desc->arch.type = actual & edgebit ?
             DT_IRQ_TYPE_EDGE_RISING :
-            DT_IRQ_TYPE_LEVEL_LOW;
+            DT_IRQ_TYPE_LEVEL_HIGH;
     }
 
     /* Set target CPU mask (RAZ/WI on uniprocessor) */
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 1558e17..e999cd9 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -504,7 +504,7 @@ static void gicv3_set_irq_properties(struct irq_desc *desc,
                actual & edgebit ? "Edge" : "Level");
         desc->arch.type = actual & edgebit ?
             DT_IRQ_TYPE_EDGE_RISING :
-            DT_IRQ_TYPE_LEVEL_LOW;
+            DT_IRQ_TYPE_LEVEL_HIGH;
     }
 
     affinity = gicv3_mpidr_to_affinity(cpu);
-- 
1.7.10.4






_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel