Re: [PATCH v8 0/4] arm: KGDB NMI/FIQ support

[Harro Haan <hrhaan@gmail.com>]

On 15 July 2014 19:08, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> On 15/07/14 16:59, Harro Haan wrote:
>> On 15 July 2014 16:52, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>>> On 15/07/14 14:04, Harro Haan wrote:
>>>>> Makes sense.
>>>>>
>>>>> Avoiding this problem on GICv2 is easy (thanks to the aliased intack
>>>>> register) but on iMX.6 we have only a GICv1.
>>>>>
>>>>>
>>>>>> I can reduce the number of occurrences (not prevent it) by adding the
>>>>>> following hack to irq-gic.c
>>>>>> @@ -297,10 +309,12 @@ static asmlinkage void __exception_irq_entry
>>>>>> gic_handle_irq(struct pt_regs *regs
>>>>>>   u32 irqstat, irqnr;
>>>>>>   struct gic_chip_data *gic = &gic_data[0];
>>>>>>   void __iomem *cpu_base = gic_data_cpu_base(gic);
>>>>>>
>>>>>>   do {
>>>>>> + while(readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_PENDING_SET)
>>>>>> & (1 << 30))
>>>>>> +   printk(KERN_ERR "TEMP: gic_handle_irq: wait for FIQ exception\n");
>>>>>>   irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
>>>>>>   irqnr = irqstat & ~0x1c00;
>>>>>
>>>>> I've made a more complete attempt to fix this. Could you test the
>>>>> following? (and be prepared to fuzz the line numbers)
>>>>
>>>> Thanks Daniel, I have tested it, see the comments below.
>>>>
>>>>>
>>>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>>>> index 73ae896..309bf2c 100644
>>>>> --- a/drivers/irqchip/irq-gic.c
>>>>> +++ b/drivers/irqchip/irq-gic.c
>>>>> @@ -303,6 +303,28 @@ static int gic_set_wake(struct irq_data *d,
>>>>> unsigned int on)
>>>>>  #define gic_set_wake   NULL
>>>>>  #endif
>>>>>
>>>>> +/* Check for group 0 interrupt spuriously acked as a normal IRQ. This
>>>>> + * workaround will only work for level triggered interrupts (and in
>>>>> + * its current form is actively harmful on systems that don't support
>>>>> + * FIQ).
>>>>> + */
>>>>> +static u32 gic_handle_spurious_group_0(struct gic_chip_data *gic, u32
>>>>> irqstat)
>>>>> +{
>>>>> +       u32 irqnr = irqstat & GICC_IAR_INT_ID_MASK;
>>>>> +
>>>>> +       if (!gic_data_fiq_enable(gic) || irqnr >= 1021)
>>>>> +               return irqnr;
>>>>> +
>>>>> +       if (readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IGROUP +
>>>>> +                         (irqnr / 32 * 4)) &
>>>>> +           BIT(irqnr % 32))
>>>>> +               return irqnr;
>>>>> +
>>>>> +       /* this interrupt was spurious (needs to be handled as FIQ) */
>>>>> +       writel_relaxed(irqstat, gic_data_cpu_base(gic) + GIC_CPU_EOI);
>>>>
>>>> This will NOT work, because of the note I mentioned above:
>>>> "The FIQ exception will not occur anymore after gic_handle_irq
>>>> read/acknowledges the FIQ by reading the GIC_CPU_INTACK register"
>>>> So with this code you will say End Of Interrupt at the GIC level,
>>>> without actually handling the interrupt, so you are missing an
>>>> interrupt.
>>>> I did the following test to confirm the missing interrupt:
>>>> I have changed the periodic timer interrupt by an one-shot timer
>>>> interrupt. The one-shot timer interrupt is programmed by the FIQ
>>>> handler for the next FIQ interrupt. As expected: When the problem
>>>> occurs, no more FIQ interrupts are generated.
>>>
>>> Can you confirm whether your timer interrupts are configured level or
>>> edge triggered? Or whether EOIing the GIC causes them to be cleared by
>>> some other means.
>>
>> From page 48 of DDI0407I_cortex_a9_mpcore_r4p1_trm.pdf:
>> Watchdog timers, PPI(3)
>> Each Cortex-A9 processor has its own watchdog timers that can generate
>> interrupts, using ID30.
>>
>> From page 56:
>> PPI[0], [2],and[3]:b11
>> interrupt is rising-edge sensitive.
>
> Thanks. This is clear.
>
>
>>> If we can't get level triggering to work then we have to:
>>>
>>> 1. Write code to jump correctly into FIQ mode.
>>>
>>> 2. Modify the gic's ack_fiq() callback to automatically avoid reading
>>>    intack when the workaround is deployed.
>>>
>>> The above is why I wanted to see if we can make do with level triggering
>>> (and automatic re-triggering for interrupts such as SGIs that are
>>> cleared by EOI).
>>
>> But the re-triggering introduces extra latencies and a lot of use
>> cases of FIQ's try to avoid that.
>
> I'm not really clear why retriggering should be significantly more
> expensive than the dance required to fake up entry the FIQ handler.
>
> On the other hand retriggering allows us to avoid hacks in the FIQ
> handler to stop it acknowledging the interrupt. Given hacks like that
> won't be needed on A7/A15 this seems like a good outcome.
>
> Anyhow I've put together a new version of my earlier patch that I think
> will retrigger all interrupts except SGIs (I'll look at SGIs and
> compatibility with non-Freescale parts only if this improved approach
> works).
>
> Reported-by: Harro Haan <hrhaan@gmail.com>
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
>  drivers/irqchip/irq-gic.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 73ae896..88f92e6 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -303,6 +303,33 @@ static int gic_set_wake(struct irq_data *d,
> unsigned int on)
>  #define gic_set_wake   NULL
>  #endif
>
> +/* This is a software emulation of the Aliased Interrupt Acknowledge
> Register
> + * (GIC_AIAR) found in GICv2+.
> + */
> +static u32 gic_handle_spurious_group_0(struct gic_chip_data *gic, u32
> irqstat)
> +{
> +       u32 irqnr = irqstat & GICC_IAR_INT_ID_MASK;
> +       void __iomem *dist_base = gic_data_dist_base(gic);
> +       u32 offset, mask;
> +
> +       if (!gic_data_fiq_enable(gic) || irqnr >= 1021)
> +               return irqnr;
> +
> +       offset = irqnr / 32 * 4;
> +       mask = 1 << (irqnr % 32);
> +       if (readl_relaxed(dist_base + GIC_DIST_IGROUP + offset) & mask)
> +               return irqnr;
> +
> +       /* this interrupt must be taken as a FIQ so put it back into the
> +        * pending state and end our own servicing of it.
> +        */
> +       writel_relaxed(mask, dist_base + GIC_DIST_PENDING_SET + offset);
> +       readl_relaxed(dist_base + GIC_DIST_PENDING_SET + offset);
> +       writel_relaxed(irqstat, gic_data_cpu_base(gic) + GIC_CPU_EOI);
> +
> +       return 1023;
> +}
> +
>  static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
>  {
>         u32 irqstat, irqnr;
> @@ -310,8 +337,10 @@ static void __exception_irq_entry
> gic_handle_irq(struct pt_regs *regs)
>         void __iomem *cpu_base = gic_data_cpu_base(gic);
>
>         do {
> +               local_fiq_disable();
>                 irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
> -               irqnr = irqstat & GICC_IAR_INT_ID_MASK;
> +               irqnr = gic_handle_spurious_group_0(gic, irqstat);
> +               local_fiq_enable();
>
>                 if (likely(irqnr > 15 && irqnr < 1021)) {
>                         irqnr = irq_find_mapping(gic->domain, irqnr);
> --
> 1.9.3
>
>
>

I just tested the above code. This approach also works as expected for
edge sensitive interrupts.