Question about SPIs' interrupt trigger type restrictions

[richard clark]

CC'ing some nxp guys for the S32G274A SOC...

On Thu, May 26, 2022 at 2:54 PM Marc Zyngier <maz at kernel.org> wrote:
>
> On Thu, 26 May 2022 04:44:41 +0100,
> richard clark <richard.xnu.clark at gmail.com> wrote:
> >
> > 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
> > ...,
>
> Let me finish the sentence for you:
>
> <quote>
> ... will leave the interrupt in an UNKNOWN pending state.
> </quote>

Context sensitive(register-update leaves UNKNOWN pending) and

>
> and the direction here is about the configuration bit, not the edge
> direction.

with this(configuration bit: either level-sensitive or
edge-triggered), but it doesn't matter.

>
> > which has been confirmed by GIC-500 on my platform.
>
> From the GIC500 r1p1 TRM, page 2-8:
>
> <quote>
> SPIs are generated either by wire inputs or by writes to the AXI4
> slave programming interface.  The GIC-500 can support up to 960 SPIs
> corresponding to the external spi[991:32] signal. The number of SPIs
> available depends on the implemented configuration. The permitted
> values are 32-960, in steps of 32. The first SPI has an ID number of
> 32. You can configure whether each SPI is triggered on a rising edge
> or is active-HIGH level-sensitive.
> </quote>
>
> So I have no idea what you are talking about, but you definitely have
> the wrong end of the stick. Both the architecture and the
> implementations are aligned with what the GIC drivers do.

What I am talking about is - The SPI is triggered on a rising edge
only, while the falling edge is not as the document says. But I've
observed the falling edge does trigger the SPI interrupt on my
platform (the SOC is NXP S32G274A, an external wakeup signal with high
to low transition to wake up the SOC - 'Wakeup/Interrupt Rising-Edge
Event Enable Register (WIREER)' and 'Wakeup/Interrupt Falling-Edge
Event Enable Register (WIFEER)', WIFEER set 1 and WIREER  set 0
works).

I don't know why the GIC has such a behavior and what the subtle
rationale is behind this, so just mark this as a record...

Thanks

>
> If your system behaves differently, this is because something is
> inverting the signal, which is extremely common. Just describe this in
> your device tree, or lie to the kernel, whichever way you want.
>
> >
> > > 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.
>
> No, this is an architectural requirement, and the driver caters for
> the architecture (and only that).
>
> > 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;
> >
>
> Not under my watch.
>
> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.