Question about SPIs' interrupt trigger type restrictions

[richard clark]

On Thu, May 26, 2022 at 3:14 AM Robin Murphy <robin.murphy at arm.com> wrote:
>
> On 2022-05-25 11:01, richard clark wrote:
> > Hi Marc,
> >
> > For below code snippet about SPI interrupt trigger type:
> >
> > static int gic_set_type(struct irq_data *d, unsigned int type)
> > {
> >          ...
> >          /* SPIs have restrictions on the supported types */
> >          if ((range == SPI_RANGE || range == ESPI_RANGE) &&
> >              type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> >                  return -EINVAL;
> >          ...
> > }
> >
> > We have a device at hand whose interrupt type is SPI, Falling edge
> > will trigger the interrupt. But the request_irq(50, handler,
> > IRQ_TYPE_EDGE_FALLING, ...) will return -EINVAL.
> >
> > The question is, why must the SPI interrupt use IRQ_TYPE_EDGE_RISING
> > instead of IRQ_TYPE_EDGE_FALLING?
>
> Because that's what the GIC architecture[1] says. From section 1.2.1
> "Interrupt Types":
>
> "An interrupt that is edge-triggered has the following property:
>         • It is asserted on detection of a rising edge of an interrupt signal

This rising edge detection is not true, it's also asserted by falling
edge, just like the GICD_ICFGR register says:
Changing the interrupt configuration between level-sensitive and
*edge-triggered (in either direction)* at a time when there is a
pending interrupt ..., which
has been confirmed by GIC-500 on my platform.

> and then, regardless of the state of the signal, remains asserted until
> the interrupt is acknowledged by software."
>
> External signals with the wrong polarity may need external logic to

IMO, it's not wrong polarity for a device to interrupt the processor
with a falling edge, it's normal. Actually, the GIC supports
edge-trigger type:
'0b10 Corresponding interrupt is edge-triggered', the
IRQ_TYPE_EDGE_RISING check in gic_set_type(...) is just a sanity check
from this point of view.
I would more like to have below changes applied:

--- a/linux/drivers/irqchip/irq-gic-v3.c
+++ b/linux/drivers/irqchip/irq-gic-v3.c

@@ -560,8 +560,7 @@ static int gic_set_type(struct irq_data *d,
unsigned int type)
                return type != IRQ_TYPE_EDGE_RISING ? -EINVAL : 0;
        /* SPIs have restrictions on the supported types */
-       if ((range == SPI_RANGE || range == ESPI_RANGE) &&
-           type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
+       if ((range == SPI_RANGE || range == ESPI_RANGE) && !(type & 0xf))
                return -EINVAL;

 I believe irq-gic.c has the same issue, but can't confirm now.


> invert them (which might even be offered by the GIC implementation
> itself, e.g. [2]), but the programmer's model neither knows nor cares
> about such details, it only knows notions of "edge-triggered" and
> "level-sensitive", where from its point of view the asserted states are
> rising and high respectively.
>
> Robin.
>
> [1] https://developer.arm.com/documentation/ihi0069/latest
> [2]
> https://developer.arm.com/documentation/100336/0106/components-and-configuration/spi-collator/spi-collator-wires?lang=en