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

[Julien Grall <julien.grall@linaro.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 <ian.campbell@citrix.com>
>>> Cc: Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
>>> ---
>>>   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 <ian.campbell@citrix.com>
> 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@citrix.com>

Given the usage of desc->arch.type in Xen, I think this is the right
solution:

Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall