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?
Thanks,
On Thu, 26 May 2022 13:09:32 +0100,
richard clark <[email protected]> wrote:
>
> CC'ing some nxp guys for the S32G274A SOC...
>
> On Thu, May 26, 2022 at 2:54 PM Marc Zyngier <[email protected]> wrote:
> >
> > On Thu, 26 May 2022 04:44:41 +0100,
> > richard clark <[email protected]> wrote:
> > >
> > > On Thu, May 26, 2022 at 3:14 AM Robin Murphy <[email protected]> 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).
This is thus driven by an external piece of HW, which, I expect, would
perform the signal conversion.
>
> 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...
If you can prove that the GIC itself (and not some piece of HW on the
signal path) latches on a falling edge, then that would be a huge
bug. I would encourage you (or NXP) to report it to ARM so that it
would be fixed.
Now, given that GIC500 has been with us for over 8 years, such a bug
would have been witnessed on tons of existing systems (all the
SPI-based MSIs would trigger twice, for example). Since there has been
(to my knowledge) no report of such an issue, I seriously doubt what
you are seeing is a GIC misbehaviour.
M.
--
Without deviation from the norm, progress is not possible.
On 2022-05-26 07:54, Marc Zyngier wrote:
> On Thu, 26 May 2022 04:44:41 +0100,
> richard clark <[email protected]> wrote:
>>
>> On Thu, May 26, 2022 at 3:14 AM Robin Murphy <[email protected]> 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.
On Thu, 26 May 2022 04:44:41 +0100,
richard clark <[email protected]> wrote:
>
> On Thu, May 26, 2022 at 3:14 AM Robin Murphy <[email protected]> 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.
> 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.
>
> > 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.
On Thu, 26 May 2022 13:30:35 +0100,
richard clark <[email protected]> wrote:
>
> On Thu, May 26, 2022 at 4:41 PM Robin Murphy <[email protected]> wrote:
> >
> > On 2022-05-26 07:54, Marc Zyngier wrote:
> > > On Thu, 26 May 2022 04:44:41 +0100,
> > > richard clark <[email protected]> wrote:
> > >>
> > >> On Thu, May 26, 2022 at 3:14 AM Robin Murphy <[email protected]> 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.
>
> 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.
>
> >
> > 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
>
> 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.
--
Without deviation from the norm, progress is not possible.
On Thu, May 26, 2022 at 4:41 PM Robin Murphy <[email protected]> wrote:
>
> On 2022-05-26 07:54, Marc Zyngier wrote:
> > On Thu, 26 May 2022 04:44:41 +0100,
> > richard clark <[email protected]> wrote:
> >>
> >> On Thu, May 26, 2022 at 3:14 AM Robin Murphy <[email protected]> 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.
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 ;-)
>
> 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
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?
I believe that the drivers still work if the trigger type sanity check
in the GIC driver is removed.
Thanks
> 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.
CC'ing some nxp guys for the S32G274A SOC...
On Thu, May 26, 2022 at 2:54 PM Marc Zyngier <[email protected]> wrote:
>
> On Thu, 26 May 2022 04:44:41 +0100,
> richard clark <[email protected]> wrote:
> >
> > On Thu, May 26, 2022 at 3:14 AM Robin Murphy <[email protected]> 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.
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 <[email protected]> wrote:
> > richard clark <[email protected]> wrote:
> > > On Thu, May 26, 2022 at 3:14 AM Robin Murphy <[email protected]> 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.
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).
Daniel.
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?
On Mon, May 30, 2022 at 4:40 PM Daniel Thompson
<[email protected]> 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 <[email protected]> wrote:
> > > richard clark <[email protected]> wrote:
> > > > On Thu, May 26, 2022 at 3:14 AM Robin Murphy <[email protected]> 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
>
> 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.
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)
>
> Daniel.
>
>
> 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
On Sun, Jun 05, 2022 at 08:03:02PM +0800, richard clark wrote:
> On Mon, May 30, 2022 at 4:40 PM Daniel Thompson
> <[email protected]> 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 <[email protected]> wrote:
> > > > richard clark <[email protected]> wrote:
> > > > > On Thu, May 26, 2022 at 3:14 AM Robin Murphy <[email protected]> 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.
> > 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.
> 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. 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).
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.
> > 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.
On Mon, Jun 6, 2022 at 6:08 PM Daniel Thompson
<[email protected]> 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
> > <[email protected]> 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 <[email protected]> wrote:
> > > > > richard clark <[email protected]> wrote:
> > > > > > On Thu, May 26, 2022 at 3:14 AM Robin Murphy <[email protected]> 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.
On Tue, Jun 07, 2022 at 09:29:06AM +0800, richard clark wrote:
> On Mon, Jun 6, 2022 at 6:08 PM Daniel Thompson
> <[email protected]> wrote:
> > 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
See below.
> > 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.
Yes there is.
The WKPU is an interrupt controller and it sits between the SoC pin and
the GIC. To avoid weird code then you need a WKPU driver. Drivers for
interrupt controllers normally belong in drivers/irqchip.
I believe, in your case, this driver is either missing or broken. This
belief is based on evidence that you provided. You code appears to
attach an ISR directly to a GIC SPI irqno: if an irqchip driver for
the WKPU was present and working then I think you would be unable to
do this (because it would be busy).
> For me, the most important thing is WHY the GIC only support the
> rising edge just as its document said.
I can't help you with that. Sure, it might be interesting to know more
but I'm a pragmatist: I simply not that invested in revisiting a fifteen
year old design decision that can no longer be changed.
Daniel.