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

[Julien Grall <julien.grall@linaro.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 <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.

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