Messages in this thread Patch in this message |  | | From | richard clark <> | Date | Thu, 26 May 2022 11:44:41 +0800 | Subject | Re: Question about SPIs' interrupt trigger type restrictions |
| |
On Thu, May 26, 2022 at 3:14 AM Robin Murphy <robin.murphy@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
|  |