Re: Question about SPIs' interrupt trigger type restrictions

[Robin Murphy <robin.murphy@arm.com>]

On 2022-05-26 07:54, Marc Zyngier wrote:
> On Thu, 26 May 2022 04:44:41 +0100,
> richard clark <richard.xnu.clark@gmail.com> wrote:
>>
>> 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
>> ...,
> 
> Let me finish the sentence for you:
> 
> <quote>
> ... will leave the interrupt in an UNKNOWN pending state.
> </quote>
> 
> and the direction here is about the configuration bit, not the edge
> direction.

Indeed it's clearly referring to either direction of *the change*, i.e. 
from edge to level and from level to edge.

>> 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.
> 
> 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.

I think the important concept to grasp here is that what we describe in 
DT is not properties of the device in isolation, but properties of its 
integration into the system as a whole. Consider the "reg" property, 
which in 99% of cases has nothing to do with the actual device it 
belongs to, but is instead describing a property of the interconnect, 
namely how its address map decodes to a particular interface, to which 
the given device happens to be attached.

At the HDL level, the device block may very well have an output signal 
which idles at logic-high, and pulses low to indicate an event, however 
it only becomes an *interrupt* if it is wired up to an interrupt 
controller; on its own it's just some output signal. What the DT 
interrupt specifier describes is that wiring, *from the interrupt 
controller's point of view*. If a pulsed signal is fed into an Arm GIC 
SPI input then as an interrupt it *is* IRQ_TYPE_EDGE_RISING, because 
that's how the GIC hardware will treat it. The integration as a whole 
takes care of the details and makes that happen, so what the logic 
levels at some arbitrary HDL boundary in the middle might be is simply 
not meaningful.

Thanks,
Robin.