Re: Question about SPIs' interrupt trigger type restrictions

[richard clark]

On Mon, Jun 6, 2022 at 6:08 PM Daniel Thompson
<daniel.thompson@xxxxxxxxxx> wrote:
>
> On Sun, Jun 05, 2022 at 08:03:02PM +0800, richard clark wrote:
> > On Mon, May 30, 2022 at 4:40 PM Daniel Thompson
> > <daniel.thompson@xxxxxxxxxx> wrote:
> > >
> > > On Thu, May 26, 2022 at 08:09:32PM +0800, richard clark wrote:
> > > > CC'ing some nxp guys for the S32G274A SOC...
> > > >
> > > > On Thu, May 26, 2022 at 2:54 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > > > > richard clark <richard.xnu.clark@xxxxxxxxx> wrote:
> > > > > > On Thu, May 26, 2022 at 3:14 AM Robin Murphy <robin.murphy@xxxxxxx> wrote:
> > > > > > > On 2022-05-25 11:01, richard clark wrote:
> > > > > 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...
> > >
> > > Are you really describing the GIC behaviour here? It sounds like you are
> > > describing the behaviour of the Wakeup Unit.
> >
> > Definitely it's behavior of GIC, not WKPU's
>
> I don't understand what evidence you are basing this on. Everything you
> say below contradicts this assertion.

Ah, I think we're not on the same page here before

>
>
> > > The SPI that goes to the GIC is the *output* of the WKPU. However the
> > > registers you mention above all control edge detection at the input to
> > > the WKPU. If so, the kernel would need an WKPU irqchip driver in order
> > > to manage the trigger mode registers above (and to clear them).
> >
> > external wakeup signal has a transition from High to Low to the SOC,
> > then output of WIREER (rising detect) or WIFEER(falling detect) to
> > generate INTID to the GIC, you have to enable WIFEER to generate the
> > IRQ signal to the GIC which is also an evidence that the external
> > wakup is falling edge.
>
> That's what I mean.
>
> How can you reason about the behaviour of the GIC when every
> observation you make is based on the behaviour of a second level
> interrupt controller (the WKPU)?
>
> I have no doubt you are observing a falling edge being delivered to the
> SoC... but that falling edge is *not* delivered to the GIC; it is
> delivered to the WKPU.
>
> The WKPU then delivers an active-high signal to the GIC.

but now I understand and agree with you that the output from the WKPU
to the GIC is different with the external wakeup of the SOC, but
please note that even with the
IRQ_TYPE_EDGE_RISING to pass the sanity check, the final value
programmed into the trigger mode register(GICD_ICFGR) of GIC is still
the edge-triggered, you can't tell it's falling or rising because that
register is just distinct as *edge* or *level*, definitely it's not
active-high. Since the SOC likes a blackbox we can't tell it's a
falling or rising edge, the only clue is come from the WKPU interrupt
generation block diagram from the NXP S32G274A RM, I reason that the
final signal output from the WKPU to the GIC is still same as the
external one.
>
> > With this clear *falling edge*, I have to
> > write the below irq_request code as:
> >
> >   request_irq(50, wkup12_interrupt, IRQ_TYPE_EDGE_RISING...)
> >
> > IMO, this is very weird because the wakeup signal is falling edge from
> > the point of SOC/GIC side, but I have to name it as
> > *IRQ_TYPE_EDGE_RISING*, but it works just to pass the sanity
> > check(although I think which is not necessary as the fact shows)
>
> It is indeed very weird.
>
> However it is not the falling edge from the SoC/GIC side that makes it
> weird! In fact there is *not* a falling edge from the SoC *and* the GIC
> because they are not the same thing.

'Not the same thing' doesn't mean it has to be different

> There is a secondary interrupt
> controller between the SoC pin and the GIC which inverts the logic (and
> also obviates any need to deploy the GIC's edge detection features at
> all... IMHO the GIC trigger mode should be active high).
I don't think there's a interrupt controller between SOC WKPU and GIC,
all the WKPU interrupt related does is to *detect* the external
signal(falling/rising) and route to the GIC
>
> Instead, I think the reason your code is weird is because the irqchip
> driver for the WKPU is missing or broken. A secondary interrupt
> controller requires an irqchip driver or you will end up with pieces of
> the interrupt controller management code (e.g. weird pokes to the WKPU
> to acknowledge things) appearing in all manner of inappropriate places.
>
Again, no so-called second irq-chip except the GIC within the SOC.
For me, the most important thing is WHY the GIC only support the
rising edge just as its document said.
>
> > > PS I can't find any sign of a WKPU driver in the mainline kernel and
> > >    AFAICT this would make wake up sources unusable. What kernel have
> > >    you been running your experiments on?
> >
> > 5.10.44- BSP code from NXP:
> > https://source.codeaurora.org/external/autobsps32/linux
>
> Given you are running a vendor kernel I think you need to discuss this
> with that vendor's support channels. To stop your code being weird then
> you need to obtain (or implement) an irqchip driver for the WKPU.
>
>
> Daniel.