Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 26 May 2022 14:00:05 +0100
Message-ID: <87bkvkmotm.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     richard clark <richard.xnu.clark@gmail.com>
Cc:     Robin Murphy <robin.murphy@arm.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: Question about SPIs' interrupt trigger type restrictions
In-Reply-To: <CAJNi4rOtsfzJWsgx7CABczbf3DqqsigErf1YFw6hKOhWnACnHQ@mail.gmail.com>
References: <CAJNi4rNwPQf747UM_hiYYwL=HDxg8QnPpfFPv1PfrtN9ZP1y1g@mail.gmail.com>
        <35f95ba3-8a7b-7918-0f9d-e14274a5ffe9@arm.com>
        <CAJNi4rMbBbLP2Tsv-wnnJKt4Y0moOE5-sNaZN1fCty908pwAKw@mail.gmail.com>
        <87ee0gn5rq.wl-maz@kernel.org>
        <5a8d5c51-ae02-a335-6768-2bedf809ab63@arm.com>
        <CAJNi4rOtsfzJWsgx7CABczbf3DqqsigErf1YFw6hKOhWnACnHQ@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, 26 May 2022 13:30:35 +0100,
richard clark <richard.xnu.clark@gmail.com> wrote:
>=20
> On Thu, May 26, 2022 at 4:41 PM Robin Murphy <robin.murphy@arm.com> wrote:
> >
> > 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 =3D=3D SPI_RANGE || range =3D=3D ESPI_RANGE) =
&&
> > >>>>               type !=3D IRQ_TYPE_LEVEL_HIGH && type !=3D 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_RISI=
NG
> > >>>> 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:
> > >>>          =E2=80=A2 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.
>=20
> I don't care about the DT at all... The essential is- does the GIC
> only support rising edge detection really just as the document says,
> I'm doubtful about that ;-)

Doubt as much as you want. The architecture and implementations are
crystal clear on the subject.

>=20
> >
> > 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
>=20
> EDGE_RISING can leave its mark in the GIC, that's the *how*, but why
> EDGE_FALLING can't, any reasons to justify this behavior?

Err, because there is no bit that allows such a configuration, maybe?

And that if someone has a falling edge signal, they can drop an
inverter on the path and be done with it?

Anyway, if you have an issue with the current behaviour of either
implementations or architecture, I suggest you take up with ARM.

> I believe that the drivers still work if the trigger type sanity check
> in the GIC driver is removed.

The driver would then be misrepresenting the architecture, and that
would be a bug. There are enough bugs in that area that we don't need
to add an extra one.

	M.

--=20
Without deviation from the norm, progress is not possible.