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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Mon, 2015-03-02 at 12:56 +0000, Julien Grall wrote:
> 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 <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.
> 
> 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)

Right, but the DT bindings are not binding (pun intended) on the h/w
designers.

Regardless, it seems like its the best we've got to go on.

> >> 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.

Right, sorry I wasn't clear. We should obviously do this too when
writing to ICFG (I believe we do), the case I was talking about was the
one relating to the quoted code above: when we read back ICFG and find
that the setting hasn't taken. In that case we want to update
desc->arch.type to somehow reflect "reality", which requires us to
fabricate a polarity for the interrupt (rising/falling or low/high).

> 
> > 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 <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>
> 
> Given the usage of desc->arch.type in Xen, I think this is the right
> solution:
> 
> Reviewed-by: Julien Grall <julien.grall@xxxxxxxxxx>

Thanks.


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