From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH 1/2] xen: arm: log warning for interrupt configuration mismatch Date: Sat, 28 Feb 2015 22:12:53 +0000 Message-ID: <54F23D65.8000602@linaro.org> References: <1424359395.30924.89.camel@citrix.com> <1424359443-21584-1-git-send-email-ian.campbell@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1424359443-21584-1-git-send-email-ian.campbell@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , xen-devel@lists.xen.org Cc: tim@xen.org, Pranavkumar Sawargaonkar , stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org 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 > Cc: Pranavkumar Sawargaonkar > --- > 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. SPIs can only be low-to-high edge triggered and high-level sensitive. > + } > + > /* Set target CPU mask (RAZ/WI on uniprocessor) */ > writeb_gicd(mask, GICD_ITARGETSR + irq); > /* Set priority */ > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index 47452ca..339b0cd 100644 > --- a/xen/arch/arm/gic-v3.c > +++ b/xen/arch/arm/gic-v3.c > @@ -465,7 +465,7 @@ static void gicv3_set_irq_properties(struct irq_desc *desc, > const cpumask_t *cpu_mask, > unsigned int priority) > { > - uint32_t cfg, edgebit; > + uint32_t cfg, actual, edgebit; > uint64_t affinity; > void __iomem *base; > unsigned int cpu = gicv3_get_cpu_from_mask(cpu_mask); > @@ -492,6 +492,20 @@ static void gicv3_set_irq_properties(struct irq_desc *desc, > > writel_relaxed(cfg, base); > > + actual = readl_relaxed(base); > + if ( ( cfg & edgebit ) ^ ( actual & edgebit ) ) > + { > + printk(XENLOG_WARNING "GICv3: 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; GICv3 bindings only support edge rising (4) and level high (1). Although the GICv3 seems to allow the other possibilities (edge falling and level low) for PPIs. Sorry I haven't spot those errors until now. Regards, -- Julien Grall