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: Mon, 02 Mar 2015 12:56:49 +0000 Message-ID: <54F45E11.8070305@linaro.org> References: <1424359395.30924.89.camel@citrix.com> <1424359443-21584-1-git-send-email-ian.campbell@citrix.com> <54F23D65.8000602@linaro.org> <1425294756.1886.33.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1425294756.1886.33.camel@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 Cc: stefano.stabellini@eu.citrix.com, tim@xen.org, Pranavkumar Sawargaonkar , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org Hi Ian, On 02/03/15 11:12, Ian Campbell wrote: > 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 >>> 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. > > You mean the gic spec? I think the DT allows for both. I haven't been able to find the paragraph in the spec speaking about it. The device tree bindings clearly say the high-to-low edge triggered and active low level-sensitive is invalid for SPIs (see Documentation/devicetree/bindings/arm/gic.txt) > >> 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. Linux seems to set edge (resp. level) bit for any edge (resp. level) type interrupts. > 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? It doesn't seem to be clearly written on the spec. Although, the GICv3 spec has 2 diagrams to explain SPIs handling: 4.3.3 and 4.3.4 > > Ian. > > 8<------- > > From 852f6e3fe49f7fab801f2857b8b505922556d746 Mon Sep 17 00:00:00 2001 > From: Ian Campbell > 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 Given the usage of desc->arch.type in Xen, I think this is the right solution: Reviewed-by: Julien Grall Regards, -- Julien Grall