2021-11-22 10:30:49

by Marc Zyngier

Subject: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

Since 041284181226 ("of/irq: Allow matching of an interrupt-map local
to an interrupt controller"), a handful of interrupt controllers have
stopped working correctly. This is due to the DT exposing a non-sensical
interrupt-map property, and their drivers relying on the kernel ignoring
this property.

Since we cannot realistically fix this terrible behaviour, add a quirk
for the limited set of devices that have implemented this monster,
and document that this is a pretty bad practice.

Cc: Rob Herring <[email protected]>
Cc: John Crispin <[email protected]>
Cc: Biwen Li <[email protected]>
Cc: Chris Brandt <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/of/irq.c | 37 +++++++++++++++++++++++++++++++++++--
1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index b10f015b2e37..27a5173c813c 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -76,6 +76,36 @@ struct device_node *of_irq_find_parent(struct device_node *child)
}
EXPORT_SYMBOL_GPL(of_irq_find_parent);

+/*
+ * These interrupt controllers abuse interrupt-map for unspeakable
+ * reasons and rely on the core code to *ignore* it (the drivers do
+ * their own parsing of the property).
+ *
+ * If you think of adding to the list for something *new*, think
+ * again. There is a high chance that you will be sent back to the
+ * drawing board.
+ */
+static const char * const of_irq_imap_abusers[] = {
+ "CBEA,platform-spider-pic",
+ "sti,platform-spider-pic",
+ "realtek,rtl-intc",
+ "fsl,ls1021a-extirq",
+ "fsl,ls1043a-extirq",
+ "fsl,ls1088a-extirq",
+ "renesas,rza1-irqc",
+};
+
+static bool of_irq_abuses_interrupt_map(struct device_node *np)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(of_irq_imap_abusers); i++)
+ if (of_device_is_compatible(np, of_irq_imap_abusers[i]))
+ return true;
+
+ return false;
+}
+
/**
* of_irq_parse_raw - Low level interrupt tree parsing
* @addr: address specifier (start of "reg" property of the device) in be32 format
@@ -159,12 +189,15 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
/*
* Now check if cursor is an interrupt-controller and
* if it is then we are done, unless there is an
- * interrupt-map which takes precedence.
+ * interrupt-map which takes precedence if we're not
+ * in presence of once of these broken platform that
+ * want to parse interrupt-map themselves for $reason.
*/
bool intc = of_property_read_bool(ipar, "interrupt-controller");

imap = of_get_property(ipar, "interrupt-map", &imaplen);
- if (imap == NULL && intc) {
+ if (intc && (imap == NULL ||
+ (imap && of_irq_abuses_interrupt_map(ipar)))) {
pr_debug(" -> got it !\n");
return 0;
}
--
2.30.2

2021-11-22 13:10:48

by Geert Uytterhoeven

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

Hi Marc,

On Mon, Nov 22, 2021 at 11:30 AM Marc Zyngier <[email protected]> wrote:
> Since 041284181226 ("of/irq: Allow matching of an interrupt-map local
> to an interrupt controller"), a handful of interrupt controllers have
> stopped working correctly. This is due to the DT exposing a non-sensical
> interrupt-map property, and their drivers relying on the kernel ignoring
> this property.
>
> Since we cannot realistically fix this terrible behaviour, add a quirk
> for the limited set of devices that have implemented this monster,
> and document that this is a pretty bad practice.
>
> Cc: Rob Herring <[email protected]>
> Cc: John Crispin <[email protected]>
> Cc: Biwen Li <[email protected]>
> Cc: Chris Brandt <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Signed-off-by: Marc Zyngier <[email protected]>

Thanks for your patch!

> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -76,6 +76,36 @@ struct device_node *of_irq_find_parent(struct device_node *child)
> }
> EXPORT_SYMBOL_GPL(of_irq_find_parent);
>
> +/*
> + * These interrupt controllers abuse interrupt-map for unspeakable
> + * reasons and rely on the core code to *ignore* it (the drivers do
> + * their own parsing of the property).
> + *
> + * If you think of adding to the list for something *new*, think
> + * again. There is a high chance that you will be sent back to the
> + * drawing board.
> + */
> +static const char * const of_irq_imap_abusers[] = {
> + "CBEA,platform-spider-pic",
> + "sti,platform-spider-pic",
> + "realtek,rtl-intc",
> + "fsl,ls1021a-extirq",
> + "fsl,ls1043a-extirq",
> + "fsl,ls1088a-extirq",
> + "renesas,rza1-irqc",
> +};

Are you sure "renesas,rza1-irqc" handles this wrong? How should it
be handled instead? I read the other thread[1], but didn't became
any wiser: interrupts are mapped one-to-one with the RZ/A1 IRQC.

In both v5.15 and v5.16-rc1, interrupts seem to work fine on RSK+RZA1
and RZA2MEVB, both with gpio-keys and when used as a wake-up interrupt.

With this patch applied, I see double keypresses with evtest: when
pressing a key, I get a key-down event, immediately followed by a
key-up event. When releasing the key, I again get two events.

Good (v5.15 or v5.16-rc1):

Event: time 1637585631.288990, type 1 (EV_KEY), code 2 (KEY_1), value 1
Event: time 1637585631.288990, -------------- SYN_REPORT ------------
Event: time 1637585631.499924, type 1 (EV_KEY), code 2 (KEY_1), value 0
Event: time 1637585631.499924, -------------- SYN_REPORT ------------

Bad (v5.16-rc1 + this patch):

Event: time 1637585341.946647, type 1 (EV_KEY), code 2 (KEY_1), value 1
Event: time 1637585341.946647, -------------- SYN_REPORT ------------
Event: time 1637585341.960256, type 1 (EV_KEY), code 2 (KEY_1), value 0
Event: time 1637585341.960256, -------------- SYN_REPORT ------------
Event: time 1637585342.146775, type 1 (EV_KEY), code 2 (KEY_1), value 1
Event: time 1637585342.146775, -------------- SYN_REPORT ------------
Event: time 1637585342.160092, type 1 (EV_KEY), code 2 (KEY_1), value 0
Event: time 1637585342.160092, -------------- SYN_REPORT ------------

Thanks!

[1] https://lore.kernel.org/all/[email protected]/

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-11-22 13:54:27

by Marc Zyngier

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Mon, 22 Nov 2021 13:10:32 +0000,
Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Marc,
>
> On Mon, Nov 22, 2021 at 11:30 AM Marc Zyngier <[email protected]> wrote:
> > Since 041284181226 ("of/irq: Allow matching of an interrupt-map local
> > to an interrupt controller"), a handful of interrupt controllers have
> > stopped working correctly. This is due to the DT exposing a non-sensical
> > interrupt-map property, and their drivers relying on the kernel ignoring
> > this property.
> >
> > Since we cannot realistically fix this terrible behaviour, add a quirk
> > for the limited set of devices that have implemented this monster,
> > and document that this is a pretty bad practice.
> >
> > Cc: Rob Herring <[email protected]>
> > Cc: John Crispin <[email protected]>
> > Cc: Biwen Li <[email protected]>
> > Cc: Chris Brandt <[email protected]>
> > Cc: Geert Uytterhoeven <[email protected]>
> > Signed-off-by: Marc Zyngier <[email protected]>
>
> Thanks for your patch!
>
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -76,6 +76,36 @@ struct device_node *of_irq_find_parent(struct device_node *child)
> > }
> > EXPORT_SYMBOL_GPL(of_irq_find_parent);
> >
> > +/*
> > + * These interrupt controllers abuse interrupt-map for unspeakable
> > + * reasons and rely on the core code to *ignore* it (the drivers do
> > + * their own parsing of the property).
> > + *
> > + * If you think of adding to the list for something *new*, think
> > + * again. There is a high chance that you will be sent back to the
> > + * drawing board.
> > + */
> > +static const char * const of_irq_imap_abusers[] = {
> > + "CBEA,platform-spider-pic",
> > + "sti,platform-spider-pic",
> > + "realtek,rtl-intc",
> > + "fsl,ls1021a-extirq",
> > + "fsl,ls1043a-extirq",
> > + "fsl,ls1088a-extirq",
> > + "renesas,rza1-irqc",
> > +};
>
> Are you sure "renesas,rza1-irqc" handles this wrong? How should it
> be handled instead? I read the other thread[1], but didn't became
> any wiser: interrupts are mapped one-to-one with the RZ/A1 IRQC.
>
> In both v5.15 and v5.16-rc1, interrupts seem to work fine on RSK+RZA1
> and RZA2MEVB, both with gpio-keys and when used as a wake-up interrupt.

This is odd. 5.16-rc1 should actively breaks the behaviour, as each
interrupt is directly routed to the GIC. Here's an extract of the DT
for r7s9210:

interrupt-map = <0 0 &gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
<1 0 &gic GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
<2 0 &gic GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
<3 0 &gic GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
<4 0 &gic GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
<5 0 &gic GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
<6 0 &gic GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
<7 0 &gic GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;

I expect v5.16-rc1 to honour the routing described here and not
involve rza1-irqc, because that's what the DT says.

> With this patch applied, I see double keypresses with evtest: when
> pressing a key, I get a key-down event, immediately followed by a
> key-up event. When releasing the key, I again get two events.
>
> Good (v5.15 or v5.16-rc1):
>
> Event: time 1637585631.288990, type 1 (EV_KEY), code 2 (KEY_1), value 1
> Event: time 1637585631.288990, -------------- SYN_REPORT ------------
> Event: time 1637585631.499924, type 1 (EV_KEY), code 2 (KEY_1), value 0
> Event: time 1637585631.499924, -------------- SYN_REPORT ------------
>
> Bad (v5.16-rc1 + this patch):
>
> Event: time 1637585341.946647, type 1 (EV_KEY), code 2 (KEY_1), value 1
> Event: time 1637585341.946647, -------------- SYN_REPORT ------------
> Event: time 1637585341.960256, type 1 (EV_KEY), code 2 (KEY_1), value 0
> Event: time 1637585341.960256, -------------- SYN_REPORT ------------
> Event: time 1637585342.146775, type 1 (EV_KEY), code 2 (KEY_1), value 1
> Event: time 1637585342.146775, -------------- SYN_REPORT ------------
> Event: time 1637585342.160092, type 1 (EV_KEY), code 2 (KEY_1), value 0
> Event: time 1637585342.160092, -------------- SYN_REPORT ------------

Is there any chance you could trace whether rza1-irqc gets called at
all when setting up and handling the interrupt?

Thanks,

M.

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

2021-11-22 16:58:43

by Geert Uytterhoeven

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

Hi Marc,

On Mon, Nov 22, 2021 at 2:54 PM Marc Zyngier <[email protected]> wrote:
> On Mon, 22 Nov 2021 13:10:32 +0000,
> Geert Uytterhoeven <[email protected]> wrote:
> > On Mon, Nov 22, 2021 at 11:30 AM Marc Zyngier <[email protected]> wrote:
> > > Since 041284181226 ("of/irq: Allow matching of an interrupt-map local
> > > to an interrupt controller"), a handful of interrupt controllers have
> > > stopped working correctly. This is due to the DT exposing a non-sensical
> > > interrupt-map property, and their drivers relying on the kernel ignoring
> > > this property.
> > >
> > > Since we cannot realistically fix this terrible behaviour, add a quirk
> > > for the limited set of devices that have implemented this monster,
> > > and document that this is a pretty bad practice.

> > > --- a/drivers/of/irq.c
> > > +++ b/drivers/of/irq.c
> > > @@ -76,6 +76,36 @@ struct device_node *of_irq_find_parent(struct device_node *child)
> > > }
> > > EXPORT_SYMBOL_GPL(of_irq_find_parent);
> > >
> > > +/*
> > > + * These interrupt controllers abuse interrupt-map for unspeakable
> > > + * reasons and rely on the core code to *ignore* it (the drivers do
> > > + * their own parsing of the property).
> > > + *
> > > + * If you think of adding to the list for something *new*, think
> > > + * again. There is a high chance that you will be sent back to the
> > > + * drawing board.
> > > + */
> > > +static const char * const of_irq_imap_abusers[] = {
> > > + "CBEA,platform-spider-pic",
> > > + "sti,platform-spider-pic",
> > > + "realtek,rtl-intc",
> > > + "fsl,ls1021a-extirq",
> > > + "fsl,ls1043a-extirq",
> > > + "fsl,ls1088a-extirq",
> > > + "renesas,rza1-irqc",
> > > +};
> >
> > Are you sure "renesas,rza1-irqc" handles this wrong? How should it
> > be handled instead? I read the other thread[1], but didn't became
> > any wiser: interrupts are mapped one-to-one with the RZ/A1 IRQC.
> >
> > In both v5.15 and v5.16-rc1, interrupts seem to work fine on RSK+RZA1
> > and RZA2MEVB, both with gpio-keys and when used as a wake-up interrupt.

Oops, it turned out my "v5.15" tree was not plain v5.15, but v5.15 with
some parts of next, including an older version of commit 041284181226.

> This is odd. 5.16-rc1 should actively breaks the behaviour, as each
> interrupt is directly routed to the GIC. Here's an extract of the DT
> for r7s9210:
>
> interrupt-map = <0 0 &gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> <1 0 &gic GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> <2 0 &gic GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> <3 0 &gic GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> <4 0 &gic GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> <5 0 &gic GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> <6 0 &gic GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> <7 0 &gic GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
>
> I expect v5.16-rc1 to honour the routing described here and not
> involve rza1-irqc, because that's what the DT says.
>
> > With this patch applied, I see double keypresses with evtest: when
> > pressing a key, I get a key-down event, immediately followed by a
> > key-up event. When releasing the key, I again get two events.
> >
> > Good (v5.15 or v5.16-rc1):
> >
> > Event: time 1637585631.288990, type 1 (EV_KEY), code 2 (KEY_1), value 1
> > Event: time 1637585631.288990, -------------- SYN_REPORT ------------
> > Event: time 1637585631.499924, type 1 (EV_KEY), code 2 (KEY_1), value 0
> > Event: time 1637585631.499924, -------------- SYN_REPORT ------------
> >
> > Bad (v5.16-rc1 + this patch):
> >
> > Event: time 1637585341.946647, type 1 (EV_KEY), code 2 (KEY_1), value 1
> > Event: time 1637585341.946647, -------------- SYN_REPORT ------------
> > Event: time 1637585341.960256, type 1 (EV_KEY), code 2 (KEY_1), value 0
> > Event: time 1637585341.960256, -------------- SYN_REPORT ------------
> > Event: time 1637585342.146775, type 1 (EV_KEY), code 2 (KEY_1), value 1
> > Event: time 1637585342.146775, -------------- SYN_REPORT ------------
> > Event: time 1637585342.160092, type 1 (EV_KEY), code 2 (KEY_1), value 0
> > Event: time 1637585342.160092, -------------- SYN_REPORT ------------
>
> Is there any chance you could trace whether rza1-irqc gets called at
> all when setting up and handling the interrupt?

I reran my tests ([A] pristine v5.15, [B] my current tree based on v5.16-rc1,
[C] my tree plus your patch).

[A] and [B] behave the same:

Boot:

rza1_irqc_translate:152: domain :soc:interrupt-controller@fcfef800
hwirq 3 type 3
rza1_irqc_alloc:115: domain :soc:interrupt-controller@fcfef800
virq 41 nr_irqs 1
rza1_irqc_alloc:127: param[0] = 0
rza1_irqc_alloc:127: param[1] = 3
rza1_irqc_alloc:127: param[2] = 4
rza1_irqc_translate:152: domain :soc:interrupt-controller@fcfef800
hwirq 2 type 3
rza1_irqc_alloc:115: domain :soc:interrupt-controller@fcfef800
virq 42 nr_irqs 1
rza1_irqc_alloc:127: param[0] = 0
rza1_irqc_alloc:127: param[1] = 2
rza1_irqc_alloc:127: param[2] = 4
rza1_irqc_translate:152: domain :soc:interrupt-controller@fcfef800
hwirq 5 type 3
rza1_irqc_alloc:115: domain :soc:interrupt-controller@fcfef800
virq 43 nr_irqs 1
rza1_irqc_alloc:127: param[0] = 0
rza1_irqc_alloc:127: param[1] = 5
rza1_irqc_alloc:127: param[2] = 4
rza1_irqc_set_type:76: hwirq 3 type 3
rza1_irqc_set_type:76: hwirq 2 type 3
rza1_irqc_set_type:76: hwirq 5 type 3

Pressing all 3 keys on RSK+RZA1:

rza1_irqc_eoi:62: hw_irq 3 IRQRR 0x8
rza1_irqc_eoi:62: hw_irq 3 IRQRR 0x8
rza1_irqc_eoi:62: hw_irq 2 IRQRR 0x4
rza1_irqc_eoi:62: hw_irq 2 IRQRR 0x4
rza1_irqc_eoi:62: hw_irq 5 IRQRR 0x20
rza1_irqc_eoi:62: hw_irq 5 IRQRR 0x20

/proc/interrupts:

41: 2 rza1-irqc 3 Edge SW1
42: 2 rza1-irqc 2 Edge SW2
43: 2 rza1-irqc 5 Edge SW3

evtest:

Event: time 1637597938.224621, type 1 (EV_KEY), code 2 (KEY_1), value 1
Event: time 1637597938.224621, -------------- SYN_REPORT ------------
Event: time 1637597938.232198, type 1 (EV_KEY), code 2 (KEY_1), value 0
Event: time 1637597938.232198, -------------- SYN_REPORT ------------
Event: time 1637597938.532939, type 1 (EV_KEY), code 2 (KEY_1), value 1
Event: time 1637597938.532939, -------------- SYN_REPORT ------------
Event: time 1637597938.542304, type 1 (EV_KEY), code 2 (KEY_1), value 0
Event: time 1637597938.542304, -------------- SYN_REPORT ------------
Event: time 1637597941.772467, type 1 (EV_KEY), code 3 (KEY_2), value 1
Event: time 1637597941.772467, -------------- SYN_REPORT ------------
Event: time 1637597941.782309, type 1 (EV_KEY), code 3 (KEY_2), value 0
Event: time 1637597941.782309, -------------- SYN_REPORT ------------
Event: time 1637597942.110321, type 1 (EV_KEY), code 3 (KEY_2), value 1
Event: time 1637597942.110321, -------------- SYN_REPORT ------------
Event: time 1637597942.122303, type 1 (EV_KEY), code 3 (KEY_2), value 0
Event: time 1637597942.122303, -------------- SYN_REPORT ------------
Event: time 1637597945.256109, type 1 (EV_KEY), code 4 (KEY_3), value 1
Event: time 1637597945.256109, -------------- SYN_REPORT ------------
Event: time 1637597945.262132, type 1 (EV_KEY), code 4 (KEY_3), value 0
Event: time 1637597945.262132, -------------- SYN_REPORT ------------
Event: time 1637597945.630469, type 1 (EV_KEY), code 4 (KEY_3), value 1
Event: time 1637597945.630469, -------------- SYN_REPORT ------------
Event: time 1637597945.642299, type 1 (EV_KEY), code 4 (KEY_3), value 0
Event: time 1637597945.642299, -------------- SYN_REPORT ------------

So despite seeing only 2 interrupts per key, gpio-keys generates
4 events per key.

With my v5.16-rc1-based tree, rza1_irqc_translate(), rza1_irqc_alloc(),
rza1_irqc_set_type(), and rza1_irqc_eoi() are indeed not called.

/proc/interrupts:

41: 242419 GIC-0 35 Level SW1
42: 142771 GIC-0 34 Level SW2
43: 136355 GIC-0 37 Level SW3
^^^^^^
Oops

evtest:

Event: time 1637598499.076306, type 1 (EV_KEY), code 2 (KEY_1), value 1
Event: time 1637598499.076306, -------------- SYN_REPORT ------------
Event: time 1637598499.350985, type 1 (EV_KEY), code 2 (KEY_1), value 0
Event: time 1637598499.350985, -------------- SYN_REPORT ------------
Event: time 1637598501.979770, type 1 (EV_KEY), code 3 (KEY_2), value 1
Event: time 1637598501.979770, -------------- SYN_REPORT ------------
Event: time 1637598502.370948, type 1 (EV_KEY), code 3 (KEY_2), value 0
Event: time 1637598502.370948, -------------- SYN_REPORT ------------
Event: time 1637598504.660146, type 1 (EV_KEY), code 4 (KEY_3), value 1
Event: time 1637598504.660146, -------------- SYN_REPORT ------------
Event: time 1637598505.030947, type 1 (EV_KEY), code 4 (KEY_3), value 0
Event: time 1637598505.030947, -------------- SYN_REPORT ------------

So despite receiving an interrupt storm, gpio-keys behaves as expected.

I will retest tomorrow with an old kernel, as I do not remember seeing such
behavior when I wrote the rza1-irqc driver.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-11-23 07:58:07

by Geert Uytterhoeven

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

Hi Marc,

On Mon, Nov 22, 2021 at 5:58 PM Geert Uytterhoeven <[email protected]> wrote:
> On Mon, Nov 22, 2021 at 2:54 PM Marc Zyngier <[email protected]> wrote:
> > On Mon, 22 Nov 2021 13:10:32 +0000,
> > Geert Uytterhoeven <[email protected]> wrote:
> > > On Mon, Nov 22, 2021 at 11:30 AM Marc Zyngier <[email protected]> wrote:
> > > > Since 041284181226 ("of/irq: Allow matching of an interrupt-map local
> > > > to an interrupt controller"), a handful of interrupt controllers have
> > > > stopped working correctly. This is due to the DT exposing a non-sensical
> > > > interrupt-map property, and their drivers relying on the kernel ignoring
> > > > this property.
> > > >
> > > > Since we cannot realistically fix this terrible behaviour, add a quirk
> > > > for the limited set of devices that have implemented this monster,
> > > > and document that this is a pretty bad practice.
>
> > > > --- a/drivers/of/irq.c
> > > > +++ b/drivers/of/irq.c
> > > > @@ -76,6 +76,36 @@ struct device_node *of_irq_find_parent(struct device_node *child)
> > > > }
> > > > EXPORT_SYMBOL_GPL(of_irq_find_parent);
> > > >
> > > > +/*
> > > > + * These interrupt controllers abuse interrupt-map for unspeakable
> > > > + * reasons and rely on the core code to *ignore* it (the drivers do
> > > > + * their own parsing of the property).
> > > > + *
> > > > + * If you think of adding to the list for something *new*, think
> > > > + * again. There is a high chance that you will be sent back to the
> > > > + * drawing board.
> > > > + */
> > > > +static const char * const of_irq_imap_abusers[] = {
> > > > + "CBEA,platform-spider-pic",
> > > > + "sti,platform-spider-pic",
> > > > + "realtek,rtl-intc",
> > > > + "fsl,ls1021a-extirq",
> > > > + "fsl,ls1043a-extirq",
> > > > + "fsl,ls1088a-extirq",
> > > > + "renesas,rza1-irqc",
> > > > +};
> > >
> > > Are you sure "renesas,rza1-irqc" handles this wrong? How should it
> > > be handled instead? I read the other thread[1], but didn't became
> > > any wiser: interrupts are mapped one-to-one with the RZ/A1 IRQC.
> > >
> > > In both v5.15 and v5.16-rc1, interrupts seem to work fine on RSK+RZA1
> > > and RZA2MEVB, both with gpio-keys and when used as a wake-up interrupt.
>
> Oops, it turned out my "v5.15" tree was not plain v5.15, but v5.15 with
> some parts of next, including an older version of commit 041284181226.
>
> > This is odd. 5.16-rc1 should actively breaks the behaviour, as each
> > interrupt is directly routed to the GIC. Here's an extract of the DT
> > for r7s9210:
> >
> > interrupt-map = <0 0 &gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > <1 0 &gic GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > <2 0 &gic GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > <3 0 &gic GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > <4 0 &gic GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > <5 0 &gic GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > <6 0 &gic GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> > <7 0 &gic GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> >
> > I expect v5.16-rc1 to honour the routing described here and not
> > involve rza1-irqc, because that's what the DT says.
> >
> > > With this patch applied, I see double keypresses with evtest: when
> > > pressing a key, I get a key-down event, immediately followed by a
> > > key-up event. When releasing the key, I again get two events.
> > >
> > > Good (v5.15 or v5.16-rc1):
> > >
> > > Event: time 1637585631.288990, type 1 (EV_KEY), code 2 (KEY_1), value 1
> > > Event: time 1637585631.288990, -------------- SYN_REPORT ------------
> > > Event: time 1637585631.499924, type 1 (EV_KEY), code 2 (KEY_1), value 0
> > > Event: time 1637585631.499924, -------------- SYN_REPORT ------------
> > >
> > > Bad (v5.16-rc1 + this patch):
> > >
> > > Event: time 1637585341.946647, type 1 (EV_KEY), code 2 (KEY_1), value 1
> > > Event: time 1637585341.946647, -------------- SYN_REPORT ------------
> > > Event: time 1637585341.960256, type 1 (EV_KEY), code 2 (KEY_1), value 0
> > > Event: time 1637585341.960256, -------------- SYN_REPORT ------------
> > > Event: time 1637585342.146775, type 1 (EV_KEY), code 2 (KEY_1), value 1
> > > Event: time 1637585342.146775, -------------- SYN_REPORT ------------
> > > Event: time 1637585342.160092, type 1 (EV_KEY), code 2 (KEY_1), value 0
> > > Event: time 1637585342.160092, -------------- SYN_REPORT ------------
> >
> > Is there any chance you could trace whether rza1-irqc gets called at
> > all when setting up and handling the interrupt?
>
> I reran my tests ([A] pristine v5.15, [B] my current tree based on v5.16-rc1,
> [C] my tree plus your patch).
>
> [A] and [B] behave the same:

Sorry, [A] and [C]:

>
> Boot:
>
> rza1_irqc_translate:152: domain :soc:interrupt-controller@fcfef800
> hwirq 3 type 3
> rza1_irqc_alloc:115: domain :soc:interrupt-controller@fcfef800
> virq 41 nr_irqs 1
> rza1_irqc_alloc:127: param[0] = 0
> rza1_irqc_alloc:127: param[1] = 3
> rza1_irqc_alloc:127: param[2] = 4
> rza1_irqc_translate:152: domain :soc:interrupt-controller@fcfef800
> hwirq 2 type 3
> rza1_irqc_alloc:115: domain :soc:interrupt-controller@fcfef800
> virq 42 nr_irqs 1
> rza1_irqc_alloc:127: param[0] = 0
> rza1_irqc_alloc:127: param[1] = 2
> rza1_irqc_alloc:127: param[2] = 4
> rza1_irqc_translate:152: domain :soc:interrupt-controller@fcfef800
> hwirq 5 type 3
> rza1_irqc_alloc:115: domain :soc:interrupt-controller@fcfef800
> virq 43 nr_irqs 1
> rza1_irqc_alloc:127: param[0] = 0
> rza1_irqc_alloc:127: param[1] = 5
> rza1_irqc_alloc:127: param[2] = 4
> rza1_irqc_set_type:76: hwirq 3 type 3
> rza1_irqc_set_type:76: hwirq 2 type 3
> rza1_irqc_set_type:76: hwirq 5 type 3
>
> Pressing all 3 keys on RSK+RZA1:
>
> rza1_irqc_eoi:62: hw_irq 3 IRQRR 0x8
> rza1_irqc_eoi:62: hw_irq 3 IRQRR 0x8
> rza1_irqc_eoi:62: hw_irq 2 IRQRR 0x4
> rza1_irqc_eoi:62: hw_irq 2 IRQRR 0x4
> rza1_irqc_eoi:62: hw_irq 5 IRQRR 0x20
> rza1_irqc_eoi:62: hw_irq 5 IRQRR 0x20
>
> /proc/interrupts:
>
> 41: 2 rza1-irqc 3 Edge SW1
> 42: 2 rza1-irqc 2 Edge SW2
> 43: 2 rza1-irqc 5 Edge SW3
>
> evtest:
>
> Event: time 1637597938.224621, type 1 (EV_KEY), code 2 (KEY_1), value 1
> Event: time 1637597938.224621, -------------- SYN_REPORT ------------
> Event: time 1637597938.232198, type 1 (EV_KEY), code 2 (KEY_1), value 0
> Event: time 1637597938.232198, -------------- SYN_REPORT ------------
> Event: time 1637597938.532939, type 1 (EV_KEY), code 2 (KEY_1), value 1
> Event: time 1637597938.532939, -------------- SYN_REPORT ------------
> Event: time 1637597938.542304, type 1 (EV_KEY), code 2 (KEY_1), value 0
> Event: time 1637597938.542304, -------------- SYN_REPORT ------------
> Event: time 1637597941.772467, type 1 (EV_KEY), code 3 (KEY_2), value 1
> Event: time 1637597941.772467, -------------- SYN_REPORT ------------
> Event: time 1637597941.782309, type 1 (EV_KEY), code 3 (KEY_2), value 0
> Event: time 1637597941.782309, -------------- SYN_REPORT ------------
> Event: time 1637597942.110321, type 1 (EV_KEY), code 3 (KEY_2), value 1
> Event: time 1637597942.110321, -------------- SYN_REPORT ------------
> Event: time 1637597942.122303, type 1 (EV_KEY), code 3 (KEY_2), value 0
> Event: time 1637597942.122303, -------------- SYN_REPORT ------------
> Event: time 1637597945.256109, type 1 (EV_KEY), code 4 (KEY_3), value 1
> Event: time 1637597945.256109, -------------- SYN_REPORT ------------
> Event: time 1637597945.262132, type 1 (EV_KEY), code 4 (KEY_3), value 0
> Event: time 1637597945.262132, -------------- SYN_REPORT ------------
> Event: time 1637597945.630469, type 1 (EV_KEY), code 4 (KEY_3), value 1
> Event: time 1637597945.630469, -------------- SYN_REPORT ------------
> Event: time 1637597945.642299, type 1 (EV_KEY), code 4 (KEY_3), value 0
> Event: time 1637597945.642299, -------------- SYN_REPORT ------------
>
> So despite seeing only 2 interrupts per key, gpio-keys generates
> 4 events per key.
>
> With my v5.16-rc1-based tree, rza1_irqc_translate(), rza1_irqc_alloc(),
> rza1_irqc_set_type(), and rza1_irqc_eoi() are indeed not called.

Hence this is [B], i.e. after the bad commit:

>
> /proc/interrupts:
>
> 41: 242419 GIC-0 35 Level SW1
> 42: 142771 GIC-0 34 Level SW2
> 43: 136355 GIC-0 37 Level SW3
> ^^^^^^
> Oops
>
> evtest:
>
> Event: time 1637598499.076306, type 1 (EV_KEY), code 2 (KEY_1), value 1
> Event: time 1637598499.076306, -------------- SYN_REPORT ------------
> Event: time 1637598499.350985, type 1 (EV_KEY), code 2 (KEY_1), value 0
> Event: time 1637598499.350985, -------------- SYN_REPORT ------------
> Event: time 1637598501.979770, type 1 (EV_KEY), code 3 (KEY_2), value 1
> Event: time 1637598501.979770, -------------- SYN_REPORT ------------
> Event: time 1637598502.370948, type 1 (EV_KEY), code 3 (KEY_2), value 0
> Event: time 1637598502.370948, -------------- SYN_REPORT ------------
> Event: time 1637598504.660146, type 1 (EV_KEY), code 4 (KEY_3), value 1
> Event: time 1637598504.660146, -------------- SYN_REPORT ------------
> Event: time 1637598505.030947, type 1 (EV_KEY), code 4 (KEY_3), value 0
> Event: time 1637598505.030947, -------------- SYN_REPORT ------------
>
> So despite receiving an interrupt storm, gpio-keys behaves as expected.
>
> I will retest tomorrow with an old kernel, as I do not remember seeing such
> behavior when I wrote the rza1-irqc driver.

Summarized:
- Before the bad commit, and after your fix, irqc-rza1 is invoked,
and the number of interrupts seen is correct, but input events
are doubled.
- After the bad commit, irqc-rza1 is not invoked, and there is an
interrupt storm, but input events are OK.

Sorry for the confusion.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-11-23 08:33:16

by Marc Zyngier

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Tue, 23 Nov 2021 07:57:48 +0000,
Geert Uytterhoeven <[email protected]> wrote:

Hi Geert,

[...]

> Summarized:
> - Before the bad commit, and after your fix, irqc-rza1 is invoked,
> and the number of interrupts seen is correct, but input events
> are doubled.
> - After the bad commit, irqc-rza1 is not invoked, and there is an
> interrupt storm, but input events are OK.

OK, that's reassuring, even if the "twice the events" stuff isn't what
you'd expect. We at least know this is a separate issue, and that this
patch on top of -rc1 brings you back to the 5.15 behaviour.

I'd expect it to be the case for the other platforms as well.

Thanks,

M.

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

2021-11-23 08:40:36

by Sander Vanheule

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

Hi Marc,

On Mon, 2021-11-22 at 10:30 +0000, Marc Zyngier wrote:
> Since 041284181226 ("of/irq: Allow matching of an interrupt-map local
> to an interrupt controller"), a handful of interrupt controllers have
> stopped working correctly. This is due to the DT exposing a non-sensical
> interrupt-map property, and their drivers relying on the kernel ignoring
> this property.
>
> Since we cannot realistically fix this terrible behaviour, add a quirk
> for the limited set of devices that have implemented this monster,
> and document that this is a pretty bad practice.
>
> Cc: Rob Herring <[email protected]>
> Cc: John Crispin <[email protected]>
> Cc: Biwen Li <[email protected]>
> Cc: Chris Brandt <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Signed-off-by: Marc Zyngier <[email protected]>

Thanks for the patch. As far as I can tell, this restores the old behaviour from
5.15 for "realtek,rtl-intc" and things work as expected.

Best,
Sander

2021-11-23 08:44:35

by Geert Uytterhoeven

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

Hi Marc,

On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <[email protected]> wrote:
> On Tue, 23 Nov 2021 07:57:48 +0000,
> Geert Uytterhoeven <[email protected]> wrote:
> > Summarized:
> > - Before the bad commit, and after your fix, irqc-rza1 is invoked,
> > and the number of interrupts seen is correct, but input events
> > are doubled.
> > - After the bad commit, irqc-rza1 is not invoked, and there is an
> > interrupt storm, but input events are OK.
>
> OK, that's reassuring, even if the "twice the events" stuff isn't what
> you'd expect. We at least know this is a separate issue, and that this
> patch on top of -rc1 brings you back to the 5.15 behaviour.
>
> I'd expect it to be the case for the other platforms as well.

OK.

BTW, what would have been the correct way to do this for irqc-rza1?
I think we're about to make the same mistake with RZ/G2L IRQC
support[1]?

Thanks!

[1] "[RFC PATCH v3 0/7] Renesas RZ/G2L IRQC support"
https://lore.kernel.org/all/[email protected]/

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-11-23 09:11:16

by Marc Zyngier

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Tue, 23 Nov 2021 08:44:19 +0000,
Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Marc,
>
> On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <[email protected]> wrote:
> > On Tue, 23 Nov 2021 07:57:48 +0000,
> > Geert Uytterhoeven <[email protected]> wrote:
> > > Summarized:
> > > - Before the bad commit, and after your fix, irqc-rza1 is invoked,
> > > and the number of interrupts seen is correct, but input events
> > > are doubled.
> > > - After the bad commit, irqc-rza1 is not invoked, and there is an
> > > interrupt storm, but input events are OK.
> >
> > OK, that's reassuring, even if the "twice the events" stuff isn't what
> > you'd expect. We at least know this is a separate issue, and that this
> > patch on top of -rc1 brings you back to the 5.15 behaviour.
> >
> > I'd expect it to be the case for the other platforms as well.
>
> OK.
>
> BTW, what would have been the correct way to do this for irqc-rza1?
> I think we're about to make the same mistake with RZ/G2L IRQC
> support[1]?

Indeed, and I was about to look into it.

There are multiple ways to skin this cat, including renaming
'interrupt-map' to 'my-own-private-interrupt-map'. Or use something
akin the new 'msi-range' (which we could call interrupt-range), and
replace:

interrupt-map = <0 0 &gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
<1 0 &gic GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
<2 0 &gic GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
<3 0 &gic GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
<4 0 &gic GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
<5 0 &gic GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
<6 0 &gic GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
<7 0 &gic GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;

with:

interrupt-range = <&gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH 0 8>;

which reads as "base interrupt spec", "start pin", "count". This
gives you almost the same level of information, and doesn't interfere
with the rest of the DT properties. Parsing it is also much simpler.
But that's up to you, really.

M.

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

2021-11-23 09:12:08

by Marc Zyngier

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Tue, 23 Nov 2021 08:40:29 +0000,
Sander Vanheule <[email protected]> wrote:
>
> Hi Marc,
>
> On Mon, 2021-11-22 at 10:30 +0000, Marc Zyngier wrote:
> > Since 041284181226 ("of/irq: Allow matching of an interrupt-map local
> > to an interrupt controller"), a handful of interrupt controllers have
> > stopped working correctly. This is due to the DT exposing a non-sensical
> > interrupt-map property, and their drivers relying on the kernel ignoring
> > this property.
> >
> > Since we cannot realistically fix this terrible behaviour, add a quirk
> > for the limited set of devices that have implemented this monster,
> > and document that this is a pretty bad practice.
> >
> > Cc: Rob Herring <[email protected]>
> > Cc: John Crispin <[email protected]>
> > Cc: Biwen Li <[email protected]>
> > Cc: Chris Brandt <[email protected]>
> > Cc: Geert Uytterhoeven <[email protected]>
> > Signed-off-by: Marc Zyngier <[email protected]>
>
> Thanks for the patch. As far as I can tell, this restores the old
> behaviour from 5.15 for "realtek,rtl-intc" and things work as
> expected.

Thanks for testing. Let's see what Rob says about this approach (I bet
he won't be thrilled...).

M.

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

2021-11-23 11:02:52

by Geert Uytterhoeven

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

Hi Marc,

On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <[email protected]> wrote:
> On Tue, 23 Nov 2021 07:57:48 +0000,
> Geert Uytterhoeven <[email protected]> wrote:
> > Summarized:
> > - Before the bad commit, and after your fix, irqc-rza1 is invoked,
> > and the number of interrupts seen is correct, but input events
> > are doubled.
> > - After the bad commit, irqc-rza1 is not invoked, and there is an
> > interrupt storm, but input events are OK.
>
> OK, that's reassuring, even if the "twice the events" stuff isn't what
> you'd expect. We at least know this is a separate issue, and that this
> patch on top of -rc1 brings you back to the 5.15 behaviour.

So the "twice the events" stuff did happen before, and is caused by
gpio-keys always fabricating timer-based auto-"up" events when using
"interrupts" instead of "gpios".

arch/arm/boot/dts/r7s72100-rskrza1.dts has IRQ_TYPE_EDGE_BOTH to
detect the real "up", which becomes a second set of "down"/"up" events.
Using IRQ_TYPE_EDGE_FALLING gets rid of the dupe by only detecting
the real "down" event. Similar for IRQ_TYPE_LEVEL_LOW, but then
there's a temporary interrupt storm until the key is released.

Seems like gpio-keys needs to be fixed for IRQ_TYPE_EDGE_BOTH.
When using "gpios" instead of "interrupts", it does pass
IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, and handles that case
correctly.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-11-24 07:55:04

by Geert Uytterhoeven

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

Hi Marc,

On Tue, Nov 23, 2021 at 10:11 AM Marc Zyngier <[email protected]> wrote:
> On Tue, 23 Nov 2021 08:44:19 +0000,
> Geert Uytterhoeven <[email protected]> wrote:
> > On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <[email protected]> wrote:
> > > On Tue, 23 Nov 2021 07:57:48 +0000,
> > > Geert Uytterhoeven <[email protected]> wrote:
> > > > Summarized:
> > > > - Before the bad commit, and after your fix, irqc-rza1 is invoked,
> > > > and the number of interrupts seen is correct, but input events
> > > > are doubled.
> > > > - After the bad commit, irqc-rza1 is not invoked, and there is an
> > > > interrupt storm, but input events are OK.
> > >
> > > OK, that's reassuring, even if the "twice the events" stuff isn't what
> > > you'd expect. We at least know this is a separate issue, and that this
> > > patch on top of -rc1 brings you back to the 5.15 behaviour.
> > >
> > > I'd expect it to be the case for the other platforms as well.
> >
> > OK.
> >
> > BTW, what would have been the correct way to do this for irqc-rza1?
> > I think we're about to make the same mistake with RZ/G2L IRQC
> > support[1]?
>
> Indeed, and I was about to look into it.
>
> There are multiple ways to skin this cat, including renaming
> 'interrupt-map' to 'my-own-private-interrupt-map'. Or use something
> akin the new 'msi-range' (which we could call interrupt-range), and
> replace:

"interrupt-ranges" (with trailing "S"), cfr. "msi-ranges"?

> interrupt-map = <0 0 &gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> <1 0 &gic GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> <2 0 &gic GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> <3 0 &gic GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> <4 0 &gic GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> <5 0 &gic GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> <6 0 &gic GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> <7 0 &gic GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
>
> with:
>
> interrupt-range = <&gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH 0 8>;
>
> which reads as "base interrupt spec", "start pin", "count". This
> gives you almost the same level of information, and doesn't interfere
> with the rest of the DT properties. Parsing it is also much simpler.

And in the non-consecutive case, you need multiple ranges, right?

> But that's up to you, really.

Chris: do you think we can still do this for RZ/A, or do we have too
many users in the wild using the upstream code?

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-11-24 11:14:37

by Marc Zyngier

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Wed, 24 Nov 2021 07:54:48 +0000,
Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Marc,
>
> On Tue, Nov 23, 2021 at 10:11 AM Marc Zyngier <[email protected]> wrote:
> > On Tue, 23 Nov 2021 08:44:19 +0000,
> > Geert Uytterhoeven <[email protected]> wrote:
> > > On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <[email protected]> wrote:
> > > > On Tue, 23 Nov 2021 07:57:48 +0000,
> > > > Geert Uytterhoeven <[email protected]> wrote:
> > > > > Summarized:
> > > > > - Before the bad commit, and after your fix, irqc-rza1 is invoked,
> > > > > and the number of interrupts seen is correct, but input events
> > > > > are doubled.
> > > > > - After the bad commit, irqc-rza1 is not invoked, and there is an
> > > > > interrupt storm, but input events are OK.
> > > >
> > > > OK, that's reassuring, even if the "twice the events" stuff isn't what
> > > > you'd expect. We at least know this is a separate issue, and that this
> > > > patch on top of -rc1 brings you back to the 5.15 behaviour.
> > > >
> > > > I'd expect it to be the case for the other platforms as well.
> > >
> > > OK.
> > >
> > > BTW, what would have been the correct way to do this for irqc-rza1?
> > > I think we're about to make the same mistake with RZ/G2L IRQC
> > > support[1]?
> >
> > Indeed, and I was about to look into it.
> >
> > There are multiple ways to skin this cat, including renaming
> > 'interrupt-map' to 'my-own-private-interrupt-map'. Or use something
> > akin the new 'msi-range' (which we could call interrupt-range), and
> > replace:
>
> "interrupt-ranges" (with trailing "S"), cfr. "msi-ranges"?

Yes, absolutely. I keep thinking of it in the singular form, but it
absolutely needs to be plural.

> > interrupt-map = <0 0 &gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > <1 0 &gic GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > <2 0 &gic GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > <3 0 &gic GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > <4 0 &gic GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > <5 0 &gic GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > <6 0 &gic GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> > <7 0 &gic GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> >
> > with:
> >
> > interrupt-range = <&gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH 0 8>;
> >
> > which reads as "base interrupt spec", "start pin", "count". This
> > gives you almost the same level of information, and doesn't interfere
> > with the rest of the DT properties. Parsing it is also much simpler.
>
> And in the non-consecutive case, you need multiple ranges, right?

That's the idea. The nice part about this is that you can grab each
range with of_parse_phandle_with_args() + two read_u32_index. The bad
part is that you have to know what part of the intspec needs to be
hacked to provide the range, but you already have this built-in in any
hierarchical interrupt controller driver.

> > But that's up to you, really.
>
> Chris: do you think we can still do this for RZ/A, or do we have too
> many users in the wild using the upstream code?

Honestly, I don't think it is worth it. There are a number of other
irqchips in the same boat, and nobody will ever update them (the fsl
stuff, for example). I'd rather you focus on the new stuff to make it
right.

Thanks,

M.

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

2021-11-27 00:44:58

by Prabhakar Mahadev Lad

Subject: RE: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <[email protected]>
> Sent: 23 November 2021 09:11
> To: Geert Uytterhoeven <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; Rob Herring
> <[email protected]>; John Crispin <[email protected]>; Biwen Li <[email protected]>; Chris Brandt
> <[email protected]>; [email protected]; Prabhakar Mahadev Lad
> <[email protected]>
> Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map
>
> On Tue, 23 Nov 2021 08:44:19 +0000,
> Geert Uytterhoeven <[email protected]> wrote:
> >
> > Hi Marc,
> >
> > On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <[email protected]> wrote:
> > > On Tue, 23 Nov 2021 07:57:48 +0000,
> > > Geert Uytterhoeven <[email protected]> wrote:
> > > > Summarized:
> > > > - Before the bad commit, and after your fix, irqc-rza1 is invoked,
> > > > and the number of interrupts seen is correct, but input events
> > > > are doubled.
> > > > - After the bad commit, irqc-rza1 is not invoked, and there is an
> > > > interrupt storm, but input events are OK.
> > >
> > > OK, that's reassuring, even if the "twice the events" stuff isn't
> > > what you'd expect. We at least know this is a separate issue, and
> > > that this patch on top of -rc1 brings you back to the 5.15 behaviour.
> > >
> > > I'd expect it to be the case for the other platforms as well.
> >
> > OK.
> >
> > BTW, what would have been the correct way to do this for irqc-rza1?
> > I think we're about to make the same mistake with RZ/G2L IRQC
> > support[1]?
>
> Indeed, and I was about to look into it.
>
> There are multiple ways to skin this cat, including renaming 'interrupt-map' to 'my-own-private-
> interrupt-map'. Or use something akin the new 'msi-range' (which we could call interrupt-range), and
> replace:
>
> interrupt-map = <0 0 &gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> <1 0 &gic GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> <2 0 &gic GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> <3 0 &gic GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> <4 0 &gic GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> <5 0 &gic GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> <6 0 &gic GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> <7 0 &gic GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
>
> with:
>
> interrupt-range = <&gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH 0 8>;
>
Just to clarify, do you suggest to add interrupt-range as a generic DT property or SoC/company specific property? If you meant to add generic property where would you suggest to document this property?

Maybe to chime in with "msi-ranges" we could name it to "interrupt-ranges"?

Cheers,
Prabhakar

> which reads as "base interrupt spec", "start pin", "count". This gives you almost the same level of
> information, and doesn't interfere with the rest of the DT properties. Parsing it is also much
> simpler.
> But that's up to you, really.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.

2021-11-29 11:19:16

by Marc Zyngier

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Sat, 27 Nov 2021 00:42:49 +0000,
Prabhakar Mahadev Lad <[email protected]> wrote:
>
> Hi Marc,
>
> > -----Original Message-----
> > From: Marc Zyngier <[email protected]>
> > Sent: 23 November 2021 09:11
> > To: Geert Uytterhoeven <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected]; Rob Herring
> > <[email protected]>; John Crispin <[email protected]>; Biwen Li <[email protected]>; Chris Brandt
> > <[email protected]>; [email protected]; Prabhakar Mahadev Lad
> > <[email protected]>
> > Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map
> >
> > On Tue, 23 Nov 2021 08:44:19 +0000,
> > Geert Uytterhoeven <[email protected]> wrote:
> > >
> > > Hi Marc,
> > >
> > > On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <[email protected]> wrote:
> > > > On Tue, 23 Nov 2021 07:57:48 +0000,
> > > > Geert Uytterhoeven <[email protected]> wrote:
> > > > > Summarized:
> > > > > - Before the bad commit, and after your fix, irqc-rza1 is invoked,
> > > > > and the number of interrupts seen is correct, but input events
> > > > > are doubled.
> > > > > - After the bad commit, irqc-rza1 is not invoked, and there is an
> > > > > interrupt storm, but input events are OK.
> > > >
> > > > OK, that's reassuring, even if the "twice the events" stuff isn't
> > > > what you'd expect. We at least know this is a separate issue, and
> > > > that this patch on top of -rc1 brings you back to the 5.15 behaviour.
> > > >
> > > > I'd expect it to be the case for the other platforms as well.
> > >
> > > OK.
> > >
> > > BTW, what would have been the correct way to do this for irqc-rza1?
> > > I think we're about to make the same mistake with RZ/G2L IRQC
> > > support[1]?
> >
> > Indeed, and I was about to look into it.
> >
> > There are multiple ways to skin this cat, including renaming 'interrupt-map' to 'my-own-private-
> > interrupt-map'. Or use something akin the new 'msi-range' (which we could call interrupt-range), and
> > replace:
> >
> > interrupt-map = <0 0 &gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > <1 0 &gic GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > <2 0 &gic GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > <3 0 &gic GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > <4 0 &gic GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > <5 0 &gic GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > <6 0 &gic GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> > <7 0 &gic GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> >
> > with:
> >
> > interrupt-range = <&gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH 0 8>;
> >
> Just to clarify, do you suggest to add interrupt-range as a generic
> DT property or SoC/company specific property?

As a generic one. I have no interest in SoC-specific stuff (though you
are free to invent your own and run it by Rob).

> If you meant to add generic property where would you suggest to
> document this property?

Ideally collocated with the rest of the interrupt properties.

Thanks,

M.

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

2021-11-29 13:35:21

by Lad, Prabhakar

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Mon, Nov 29, 2021 at 10:34 AM Marc Zyngier <[email protected]> wrote:
>
> On Sat, 27 Nov 2021 00:42:49 +0000,
> Prabhakar Mahadev Lad <[email protected]> wrote:
> >
> > Hi Marc,
> >
> > > -----Original Message-----
> > > From: Marc Zyngier <[email protected]>
> > > Sent: 23 November 2021 09:11
> > > To: Geert Uytterhoeven <[email protected]>
> > > Cc: [email protected]; [email protected]; [email protected]; Rob Herring
> > > <[email protected]>; John Crispin <[email protected]>; Biwen Li <[email protected]>; Chris Brandt
> > > <[email protected]>; [email protected]; Prabhakar Mahadev Lad
> > > <[email protected]>
> > > Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map
> > >
> > > On Tue, 23 Nov 2021 08:44:19 +0000,
> > > Geert Uytterhoeven <[email protected]> wrote:
> > > >
> > > > Hi Marc,
> > > >
> > > > On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <[email protected]> wrote:
> > > > > On Tue, 23 Nov 2021 07:57:48 +0000,
> > > > > Geert Uytterhoeven <[email protected]> wrote:
> > > > > > Summarized:
> > > > > > - Before the bad commit, and after your fix, irqc-rza1 is invoked,
> > > > > > and the number of interrupts seen is correct, but input events
> > > > > > are doubled.
> > > > > > - After the bad commit, irqc-rza1 is not invoked, and there is an
> > > > > > interrupt storm, but input events are OK.
> > > > >
> > > > > OK, that's reassuring, even if the "twice the events" stuff isn't
> > > > > what you'd expect. We at least know this is a separate issue, and
> > > > > that this patch on top of -rc1 brings you back to the 5.15 behaviour.
> > > > >
> > > > > I'd expect it to be the case for the other platforms as well.
> > > >
> > > > OK.
> > > >
> > > > BTW, what would have been the correct way to do this for irqc-rza1?
> > > > I think we're about to make the same mistake with RZ/G2L IRQC
> > > > support[1]?
> > >
> > > Indeed, and I was about to look into it.
> > >
> > > There are multiple ways to skin this cat, including renaming 'interrupt-map' to 'my-own-private-
> > > interrupt-map'. Or use something akin the new 'msi-range' (which we could call interrupt-range), and
> > > replace:
> > >
> > > interrupt-map = <0 0 &gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > > <1 0 &gic GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > > <2 0 &gic GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > > <3 0 &gic GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > > <4 0 &gic GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > > <5 0 &gic GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > > <6 0 &gic GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> > > <7 0 &gic GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> > >
> > > with:
> > >
> > > interrupt-range = <&gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH 0 8>;
> > >
> > Just to clarify, do you suggest to add interrupt-range as a generic
> > DT property or SoC/company specific property?
>
> As a generic one. I have no interest in SoC-specific stuff (though you
> are free to invent your own and run it by Rob).
>
OK will go with a generic one.

> > If you meant to add generic property where would you suggest to
> > document this property?
>
> Ideally collocated with the rest of the interrupt properties.
>
OK, I will go with interrupts.txt for now. Is that OK with you Rob?
(the reason I ask because interrupt-map/mask haven't been documented).

Cheers,
Prabhakar

> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.

2021-11-29 19:17:46

by Rob Herring (Arm)

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Mon, Nov 22, 2021 at 4:30 AM Marc Zyngier <[email protected]> wrote:
>
> Since 041284181226 ("of/irq: Allow matching of an interrupt-map local
> to an interrupt controller"), a handful of interrupt controllers have
> stopped working correctly. This is due to the DT exposing a non-sensical
> interrupt-map property, and their drivers relying on the kernel ignoring
> this property.
>
> Since we cannot realistically fix this terrible behaviour, add a quirk
> for the limited set of devices that have implemented this monster,
> and document that this is a pretty bad practice.
>
> Cc: Rob Herring <[email protected]>
> Cc: John Crispin <[email protected]>
> Cc: Biwen Li <[email protected]>
> Cc: Chris Brandt <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> drivers/of/irq.c | 37 +++++++++++++++++++++++++++++++++++--
> 1 file changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index b10f015b2e37..27a5173c813c 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -76,6 +76,36 @@ struct device_node *of_irq_find_parent(struct device_node *child)
> }
> EXPORT_SYMBOL_GPL(of_irq_find_parent);
>
> +/*
> + * These interrupt controllers abuse interrupt-map for unspeakable
> + * reasons and rely on the core code to *ignore* it (the drivers do
> + * their own parsing of the property).
> + *
> + * If you think of adding to the list for something *new*, think
> + * again. There is a high chance that you will be sent back to the
> + * drawing board.
> + */
> +static const char * const of_irq_imap_abusers[] = {
> + "CBEA,platform-spider-pic",
> + "sti,platform-spider-pic",
> + "realtek,rtl-intc",
> + "fsl,ls1021a-extirq",
> + "fsl,ls1043a-extirq",
> + "fsl,ls1088a-extirq",
> + "renesas,rza1-irqc",
> +};

I guess this list was obtained by with a: git grep '"interrupt-map"'

I suppose that should be sufficient to find all the cases. I'd like to
be able to identify this case just from a DT file, but it's not really
clear

Perhaps a simpler solution to all this is only handle interrupt-map
with interrupt-controller if it points to its own node. That works for
Apple and I don't see a need beyond that case.


> +static bool of_irq_abuses_interrupt_map(struct device_node *np)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(of_irq_imap_abusers); i++)
> + if (of_device_is_compatible(np, of_irq_imap_abusers[i]))
> + return true;
> +
> + return false;

With a NULL terminated list, you can use of_device_compatible_match() instead .

Rob

2021-11-29 19:42:49

by Rob Herring (Arm)

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Mon, Nov 29, 2021 at 1:31 PM Marc Zyngier <[email protected]> wrote:
>
> On Mon, 29 Nov 2021 19:15:27 +0000,
> Rob Herring <[email protected]> wrote:
> >
> > On Mon, Nov 22, 2021 at 4:30 AM Marc Zyngier <[email protected]> wrote:
> > >
> > > Since 041284181226 ("of/irq: Allow matching of an interrupt-map local
> > > to an interrupt controller"), a handful of interrupt controllers have
> > > stopped working correctly. This is due to the DT exposing a non-sensical
> > > interrupt-map property, and their drivers relying on the kernel ignoring
> > > this property.
> > >
> > > Since we cannot realistically fix this terrible behaviour, add a quirk
> > > for the limited set of devices that have implemented this monster,
> > > and document that this is a pretty bad practice.
> > >
> > > Cc: Rob Herring <[email protected]>
> > > Cc: John Crispin <[email protected]>
> > > Cc: Biwen Li <[email protected]>
> > > Cc: Chris Brandt <[email protected]>
> > > Cc: Geert Uytterhoeven <[email protected]>
> > > Signed-off-by: Marc Zyngier <[email protected]>
> > > ---
> > > drivers/of/irq.c | 37 +++++++++++++++++++++++++++++++++++--
> > > 1 file changed, 35 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > > index b10f015b2e37..27a5173c813c 100644
> > > --- a/drivers/of/irq.c
> > > +++ b/drivers/of/irq.c
> > > @@ -76,6 +76,36 @@ struct device_node *of_irq_find_parent(struct device_node *child)
> > > }
> > > EXPORT_SYMBOL_GPL(of_irq_find_parent);
> > >
> > > +/*
> > > + * These interrupt controllers abuse interrupt-map for unspeakable
> > > + * reasons and rely on the core code to *ignore* it (the drivers do
> > > + * their own parsing of the property).
> > > + *
> > > + * If you think of adding to the list for something *new*, think
> > > + * again. There is a high chance that you will be sent back to the
> > > + * drawing board.
> > > + */
> > > +static const char * const of_irq_imap_abusers[] = {
> > > + "CBEA,platform-spider-pic",
> > > + "sti,platform-spider-pic",
> > > + "realtek,rtl-intc",
> > > + "fsl,ls1021a-extirq",
> > > + "fsl,ls1043a-extirq",
> > > + "fsl,ls1088a-extirq",
> > > + "renesas,rza1-irqc",
> > > +};
> >
> > I guess this list was obtained by with a: git grep '"interrupt-map"'
>
> Yes. Anyone having its own interrupt-map parser is likely to have the
> same problem.
>
> > I suppose that should be sufficient to find all the cases. I'd like to
> > be able to identify this case just from a DT file, but it's not really
> > clear
>
> Indeed. Not to mention that the PPC stuff doesn't has its DT hidden in
> some firmware.
>
> > Perhaps a simpler solution to all this is only handle interrupt-map
> > with interrupt-controller if it points to its own node. That works for
> > Apple and I don't see a need beyond that case.
>
> The problem is that interrupt-map can point to more than a single
> controller. What if the map points to a both a local interrupt and a a
> remote one?

Seems like a theoretical problem...

> It feels weird to standardise on a behaviour that seems to contradict
> the spec and to single out the one that (IMO) matches the expected
> behaviour. At the end of the day, I'll implement whichever solution
> you prefer.

Let's keep the public shaming list I guess. If it grows I may change my mind...

> > > +static bool of_irq_abuses_interrupt_map(struct device_node *np)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(of_irq_imap_abusers); i++)
> > > + if (of_device_is_compatible(np, of_irq_imap_abusers[i]))
> > > + return true;
> > > +
> > > + return false;
> >
> > With a NULL terminated list, you can use of_device_compatible_match() instead .
>
> Ah, neat.
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.

2021-11-29 22:02:32

by Marc Zyngier

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Mon, 29 Nov 2021 19:15:27 +0000,
Rob Herring <[email protected]> wrote:
>
> On Mon, Nov 22, 2021 at 4:30 AM Marc Zyngier <[email protected]> wrote:
> >
> > Since 041284181226 ("of/irq: Allow matching of an interrupt-map local
> > to an interrupt controller"), a handful of interrupt controllers have
> > stopped working correctly. This is due to the DT exposing a non-sensical
> > interrupt-map property, and their drivers relying on the kernel ignoring
> > this property.
> >
> > Since we cannot realistically fix this terrible behaviour, add a quirk
> > for the limited set of devices that have implemented this monster,
> > and document that this is a pretty bad practice.
> >
> > Cc: Rob Herring <[email protected]>
> > Cc: John Crispin <[email protected]>
> > Cc: Biwen Li <[email protected]>
> > Cc: Chris Brandt <[email protected]>
> > Cc: Geert Uytterhoeven <[email protected]>
> > Signed-off-by: Marc Zyngier <[email protected]>
> > ---
> > drivers/of/irq.c | 37 +++++++++++++++++++++++++++++++++++--
> > 1 file changed, 35 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index b10f015b2e37..27a5173c813c 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -76,6 +76,36 @@ struct device_node *of_irq_find_parent(struct device_node *child)
> > }
> > EXPORT_SYMBOL_GPL(of_irq_find_parent);
> >
> > +/*
> > + * These interrupt controllers abuse interrupt-map for unspeakable
> > + * reasons and rely on the core code to *ignore* it (the drivers do
> > + * their own parsing of the property).
> > + *
> > + * If you think of adding to the list for something *new*, think
> > + * again. There is a high chance that you will be sent back to the
> > + * drawing board.
> > + */
> > +static const char * const of_irq_imap_abusers[] = {
> > + "CBEA,platform-spider-pic",
> > + "sti,platform-spider-pic",
> > + "realtek,rtl-intc",
> > + "fsl,ls1021a-extirq",
> > + "fsl,ls1043a-extirq",
> > + "fsl,ls1088a-extirq",
> > + "renesas,rza1-irqc",
> > +};
>
> I guess this list was obtained by with a: git grep '"interrupt-map"'

Yes. Anyone having its own interrupt-map parser is likely to have the
same problem.

> I suppose that should be sufficient to find all the cases. I'd like to
> be able to identify this case just from a DT file, but it's not really
> clear

Indeed. Not to mention that the PPC stuff doesn't has its DT hidden in
some firmware.

> Perhaps a simpler solution to all this is only handle interrupt-map
> with interrupt-controller if it points to its own node. That works for
> Apple and I don't see a need beyond that case.

The problem is that interrupt-map can point to more than a single
controller. What if the map points to a both a local interrupt and a a
remote one?

It feels weird to standardise on a behaviour that seems to contradict
the spec and to single out the one that (IMO) matches the expected
behaviour. At the end of the day, I'll implement whichever solution
you prefer.

> > +static bool of_irq_abuses_interrupt_map(struct device_node *np)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(of_irq_imap_abusers); i++)
> > + if (of_device_is_compatible(np, of_irq_imap_abusers[i]))
> > + return true;
> > +
> > + return false;
>
> With a NULL terminated list, you can use of_device_compatible_match() instead .

Ah, neat.

Thanks,

M.

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

2021-11-29 22:30:14

by Rob Herring (Arm)

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Mon, Nov 29, 2021 at 6:13 AM Lad, Prabhakar
<[email protected]> wrote:
>

I timed my vacation well...

> On Mon, Nov 29, 2021 at 10:34 AM Marc Zyngier <[email protected]> wrote:
> >
> > On Sat, 27 Nov 2021 00:42:49 +0000,
> > Prabhakar Mahadev Lad <[email protected]> wrote:
> > >
> > > Hi Marc,
> > >
> > > > -----Original Message-----
> > > > From: Marc Zyngier <[email protected]>
> > > > Sent: 23 November 2021 09:11
> > > > To: Geert Uytterhoeven <[email protected]>
> > > > Cc: [email protected]; [email protected]; [email protected]; Rob Herring
> > > > <[email protected]>; John Crispin <[email protected]>; Biwen Li <[email protected]>; Chris Brandt
> > > > <[email protected]>; [email protected]; Prabhakar Mahadev Lad
> > > > <[email protected]>
> > > > Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map
> > > >
> > > > On Tue, 23 Nov 2021 08:44:19 +0000,
> > > > Geert Uytterhoeven <[email protected]> wrote:
> > > > >
> > > > > Hi Marc,
> > > > >
> > > > > On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <[email protected]> wrote:
> > > > > > On Tue, 23 Nov 2021 07:57:48 +0000,
> > > > > > Geert Uytterhoeven <[email protected]> wrote:
> > > > > > > Summarized:
> > > > > > > - Before the bad commit, and after your fix, irqc-rza1 is invoked,
> > > > > > > and the number of interrupts seen is correct, but input events
> > > > > > > are doubled.
> > > > > > > - After the bad commit, irqc-rza1 is not invoked, and there is an
> > > > > > > interrupt storm, but input events are OK.
> > > > > >
> > > > > > OK, that's reassuring, even if the "twice the events" stuff isn't
> > > > > > what you'd expect. We at least know this is a separate issue, and
> > > > > > that this patch on top of -rc1 brings you back to the 5.15 behaviour.
> > > > > >
> > > > > > I'd expect it to be the case for the other platforms as well.
> > > > >
> > > > > OK.
> > > > >
> > > > > BTW, what would have been the correct way to do this for irqc-rza1?
> > > > > I think we're about to make the same mistake with RZ/G2L IRQC
> > > > > support[1]?
> > > >
> > > > Indeed, and I was about to look into it.
> > > >
> > > > There are multiple ways to skin this cat, including renaming 'interrupt-map' to 'my-own-private-
> > > > interrupt-map'. Or use something akin the new 'msi-range' (which we could call interrupt-range), and
> > > > replace:
> > > >
> > > > interrupt-map = <0 0 &gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > > > <1 0 &gic GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > > > <2 0 &gic GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > > > <3 0 &gic GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > > > <4 0 &gic GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > > > <5 0 &gic GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > > > <6 0 &gic GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> > > > <7 0 &gic GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> > > >
> > > > with:
> > > >
> > > > interrupt-range = <&gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH 0 8>;

interrupts would work just fine here:

interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;

We don't need a different solution for N:1 interrupts from N:M. Sure,
that could become unweldy if there are a lot of interrupts (just like
interrupt-map), but is that an immediate problem?

> > > >
> > > Just to clarify, do you suggest to add interrupt-range as a generic
> > > DT property or SoC/company specific property?
> >
> > As a generic one. I have no interest in SoC-specific stuff (though you
> > are free to invent your own and run it by Rob).
> >
> OK will go with a generic one.
>
> > > If you meant to add generic property where would you suggest to
> > > document this property?
> >
> > Ideally collocated with the rest of the interrupt properties.
> >
> OK, I will go with interrupts.txt for now. Is that OK with you Rob?

Nope. As the rest of interrupts are documented in DT Spec it goes
there and there needs to be a schema (in dtschema).

> (the reason I ask because interrupt-map/mask haven't been documented).

Yes, they are. Only for the last 20+ years.

Rob

2021-11-30 12:52:57

by Lad, Prabhakar

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <[email protected]> wrote:
>
> On Mon, Nov 29, 2021 at 6:13 AM Lad, Prabhakar
> <[email protected]> wrote:
> >
>
> I timed my vacation well...
>
> > On Mon, Nov 29, 2021 at 10:34 AM Marc Zyngier <[email protected]> wrote:
> > >
> > > On Sat, 27 Nov 2021 00:42:49 +0000,
> > > Prabhakar Mahadev Lad <[email protected]> wrote:
> > > >
> > > > Hi Marc,
> > > >
> > > > > -----Original Message-----
> > > > > From: Marc Zyngier <[email protected]>
> > > > > Sent: 23 November 2021 09:11
> > > > > To: Geert Uytterhoeven <[email protected]>
> > > > > Cc: [email protected]; [email protected]; [email protected]; Rob Herring
> > > > > <[email protected]>; John Crispin <[email protected]>; Biwen Li <[email protected]>; Chris Brandt
> > > > > <[email protected]>; [email protected]; Prabhakar Mahadev Lad
> > > > > <[email protected]>
> > > > > Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map
> > > > >
> > > > > On Tue, 23 Nov 2021 08:44:19 +0000,
> > > > > Geert Uytterhoeven <[email protected]> wrote:
> > > > > >
> > > > > > Hi Marc,
> > > > > >
> > > > > > On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <[email protected]> wrote:
> > > > > > > On Tue, 23 Nov 2021 07:57:48 +0000,
> > > > > > > Geert Uytterhoeven <[email protected]> wrote:
> > > > > > > > Summarized:
> > > > > > > > - Before the bad commit, and after your fix, irqc-rza1 is invoked,
> > > > > > > > and the number of interrupts seen is correct, but input events
> > > > > > > > are doubled.
> > > > > > > > - After the bad commit, irqc-rza1 is not invoked, and there is an
> > > > > > > > interrupt storm, but input events are OK.
> > > > > > >
> > > > > > > OK, that's reassuring, even if the "twice the events" stuff isn't
> > > > > > > what you'd expect. We at least know this is a separate issue, and
> > > > > > > that this patch on top of -rc1 brings you back to the 5.15 behaviour.
> > > > > > >
> > > > > > > I'd expect it to be the case for the other platforms as well.
> > > > > >
> > > > > > OK.
> > > > > >
> > > > > > BTW, what would have been the correct way to do this for irqc-rza1?
> > > > > > I think we're about to make the same mistake with RZ/G2L IRQC
> > > > > > support[1]?
> > > > >
> > > > > Indeed, and I was about to look into it.
> > > > >
> > > > > There are multiple ways to skin this cat, including renaming 'interrupt-map' to 'my-own-private-
> > > > > interrupt-map'. Or use something akin the new 'msi-range' (which we could call interrupt-range), and
> > > > > replace:
> > > > >
> > > > > interrupt-map = <0 0 &gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <1 0 &gic GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <2 0 &gic GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <3 0 &gic GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <4 0 &gic GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <5 0 &gic GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <6 0 &gic GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <7 0 &gic GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> > > > >
> > > > > with:
> > > > >
> > > > > interrupt-range = <&gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH 0 8>;
>
> interrupts would work just fine here:
>
> interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
>
> We don't need a different solution for N:1 interrupts from N:M. Sure,
> that could become unweldy if there are a lot of interrupts (just like
> interrupt-map), but is that an immediate problem?
>
It's just that with this approach the driver will have to index the
interrupts instead of reading from DT.

Marc - is it OK with the above approach?

> > > > >
> > > > Just to clarify, do you suggest to add interrupt-range as a generic
> > > > DT property or SoC/company specific property?
> > >
> > > As a generic one. I have no interest in SoC-specific stuff (though you
> > > are free to invent your own and run it by Rob).
> > >
> > OK will go with a generic one.
> >
> > > > If you meant to add generic property where would you suggest to
> > > > document this property?
> > >
> > > Ideally collocated with the rest of the interrupt properties.
> > >
> > OK, I will go with interrupts.txt for now. Is that OK with you Rob?
>
> Nope. As the rest of interrupts are documented in DT Spec it goes
> there and there needs to be a schema (in dtschema).
>
Agreed.

> > (the reason I ask because interrupt-map/mask haven't been documented).
>
> Yes, they are. Only for the last 20+ years.
>
My bad I was referring in
Documentation/devicetree/bindings/interrupt-controller/

Cheers,
Prabhakar

> Rob

2021-11-30 18:37:16

by Marc Zyngier

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Tue, 30 Nov 2021 12:52:21 +0000,
"Lad, Prabhakar" <[email protected]> wrote:
>
> On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <[email protected]> wrote:
> >
> > interrupts would work just fine here:
> >
> > interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> >
> > We don't need a different solution for N:1 interrupts from N:M. Sure,
> > that could become unweldy if there are a lot of interrupts (just like
> > interrupt-map), but is that an immediate problem?
> >
> It's just that with this approach the driver will have to index the
> interrupts instead of reading from DT.
>
> Marc - is it OK with the above approach?

Anything that uses standard properties in a standard way works for me.

M.

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

2021-12-01 13:37:23

by Lad, Prabhakar

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

Hi Marc/Rob,

On Tue, Nov 30, 2021 at 6:37 PM Marc Zyngier <[email protected]> wrote:
>
> On Tue, 30 Nov 2021 12:52:21 +0000,
> "Lad, Prabhakar" <[email protected]> wrote:
> >
> > On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <[email protected]> wrote:
> > >
> > > interrupts would work just fine here:
> > >
> > > interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> > >
> > > We don't need a different solution for N:1 interrupts from N:M. Sure,
> > > that could become unweldy if there are a lot of interrupts (just like
> > > interrupt-map), but is that an immediate problem?
> > >
> > It's just that with this approach the driver will have to index the
> > interrupts instead of reading from DT.
> >
> > Marc - is it OK with the above approach?
>
> Anything that uses standard properties in a standard way works for me.
>
I added interrupts property now instead of interrupt-map as below:

irqc: interrupt-controller@110a0000 {
compatible = "renesas,r9a07g044-irqc", "renesas,rzg2l-irqc";
#address-cells = <0>;
interrupt-parent = <&gic>;
interrupt-controller;
reg = <0 0x110a0000 0 0x10000>;
interrupts =
<GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 444 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 445 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 446 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 447 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 450 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 451 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 452 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 453 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 454 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 455 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 457 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 459 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 460 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 462 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 463 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 470 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 474 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 475 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&cpg CPG_MOD R9A07G044_IA55_CLK>,
<&cpg CPG_MOD R9A07G044_IA55_PCLK>;
clock-names = "clk", "pclk";
power-domains = <&cpg>;
resets = <&cpg R9A07G044_IA55_RESETN>;
};


In the hierarchal interrupt code its parsed as below:
on probe fetch the details:
range = of_get_property(np, "interrupts", &len);
if (!range)
return -EINVAL;

for (len /= sizeof(*range), j = 0; len >= 3; len -= 3) {
if (j >= IRQC_NUM_IRQ)
return -EINVAL;

priv->map[j].args[0] = be32_to_cpu(*range++);
priv->map[j].args[1] = be32_to_cpu(*range++);
priv->map[j].args[2] = be32_to_cpu(*range++);
priv->map[j].args_count = 3;
j++;
}

On alloc callback:
spec.fwnode = domain->parent->fwnode;
spec.param_count = priv->map[hwirq].args_count;
for (i = 0; i < spec.param_count; i++)
spec.param[i] = priv->map[hwirq].args[i];

return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &spec);

In the board DTS (map interrupt 3 (index 3 is used as hwirq) to GIC):
&eth0 {
pinctrl-0 = <&eth0_pins>;
...
phy0: ethernet-phy@7 {
compatible = "ethernet-phy-id0022.1640", "ethernet-phy-ieee802.3-c22";
reg = <7>;
interrupt-parent = <&irqc>;
interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
rxc-skew-psec = <2400>;
....
};
};

With this approach I see the kernel hangs. It looks like using
"interrupts" property makes the core to parse it and is used somewhere
(I have not looked where exactly it's done or causes this issue) I say
this because if I rename the "interrupts" property to something else I
no longer see any kernel hangs.

Is there something else I'm missing here?

Cheers,
Prabhakar



> M.
>
> --
> Without deviation from the norm, progress is not possible.

2021-12-01 14:40:59

by Rob Herring (Arm)

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Wed, Dec 1, 2021 at 7:37 AM Lad, Prabhakar
<[email protected]> wrote:
>
> Hi Marc/Rob,
>
> On Tue, Nov 30, 2021 at 6:37 PM Marc Zyngier <[email protected]> wrote:
> >
> > On Tue, 30 Nov 2021 12:52:21 +0000,
> > "Lad, Prabhakar" <[email protected]> wrote:
> > >
> > > On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <[email protected]> wrote:
> > > >
> > > > interrupts would work just fine here:
> > > >
> > > > interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> > > >
> > > > We don't need a different solution for N:1 interrupts from N:M. Sure,
> > > > that could become unweldy if there are a lot of interrupts (just like
> > > > interrupt-map), but is that an immediate problem?
> > > >
> > > It's just that with this approach the driver will have to index the
> > > interrupts instead of reading from DT.
> > >
> > > Marc - is it OK with the above approach?
> >
> > Anything that uses standard properties in a standard way works for me.
> >
> I added interrupts property now instead of interrupt-map as below:
>
> irqc: interrupt-controller@110a0000 {
> compatible = "renesas,r9a07g044-irqc", "renesas,rzg2l-irqc";
> #address-cells = <0>;
> interrupt-parent = <&gic>;
> interrupt-controller;
> reg = <0 0x110a0000 0 0x10000>;
> interrupts =
> <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 444 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 445 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 446 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 447 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 450 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 451 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 452 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 453 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 454 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 455 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 457 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 459 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 460 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 462 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 463 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 470 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 474 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 475 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&cpg CPG_MOD R9A07G044_IA55_CLK>,
> <&cpg CPG_MOD R9A07G044_IA55_PCLK>;
> clock-names = "clk", "pclk";
> power-domains = <&cpg>;
> resets = <&cpg R9A07G044_IA55_RESETN>;
> };
>
>
> In the hierarchal interrupt code its parsed as below:
> on probe fetch the details:
> range = of_get_property(np, "interrupts", &len);
> if (!range)
> return -EINVAL;
>
> for (len /= sizeof(*range), j = 0; len >= 3; len -= 3) {
> if (j >= IRQC_NUM_IRQ)
> return -EINVAL;
>
> priv->map[j].args[0] = be32_to_cpu(*range++);
> priv->map[j].args[1] = be32_to_cpu(*range++);
> priv->map[j].args[2] = be32_to_cpu(*range++);
> priv->map[j].args_count = 3;
> j++;

Not sure what's wrong, but you shouldn't be doing your own parsing.
The setup shouldn't look much different than a GPIO controller
interrupts except you have multiple parent interrupts.

Rob

2021-12-01 16:17:05

by Lad, Prabhakar

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Wed, Dec 1, 2021 at 2:36 PM Rob Herring <[email protected]> wrote:
>
> On Wed, Dec 1, 2021 at 7:37 AM Lad, Prabhakar
> <[email protected]> wrote:
> >
> > Hi Marc/Rob,
> >
> > On Tue, Nov 30, 2021 at 6:37 PM Marc Zyngier <[email protected]> wrote:
> > >
> > > On Tue, 30 Nov 2021 12:52:21 +0000,
> > > "Lad, Prabhakar" <[email protected]> wrote:
> > > >
> > > > On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <[email protected]> wrote:
> > > > >
> > > > > interrupts would work just fine here:
> > > > >
> > > > > interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> > > > >
> > > > > We don't need a different solution for N:1 interrupts from N:M. Sure,
> > > > > that could become unweldy if there are a lot of interrupts (just like
> > > > > interrupt-map), but is that an immediate problem?
> > > > >
> > > > It's just that with this approach the driver will have to index the
> > > > interrupts instead of reading from DT.
> > > >
> > > > Marc - is it OK with the above approach?
> > >
> > > Anything that uses standard properties in a standard way works for me.
> > >
> > I added interrupts property now instead of interrupt-map as below:
> >
> > irqc: interrupt-controller@110a0000 {
> > compatible = "renesas,r9a07g044-irqc", "renesas,rzg2l-irqc";
> > #address-cells = <0>;
> > interrupt-parent = <&gic>;
> > interrupt-controller;
> > reg = <0 0x110a0000 0 0x10000>;
> > interrupts =
> > <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 444 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 445 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 446 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 447 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 450 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 451 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 452 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 453 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 454 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 455 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 457 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 459 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 460 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 462 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 463 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 470 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 474 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 475 IRQ_TYPE_LEVEL_HIGH>;
> > clocks = <&cpg CPG_MOD R9A07G044_IA55_CLK>,
> > <&cpg CPG_MOD R9A07G044_IA55_PCLK>;
> > clock-names = "clk", "pclk";
> > power-domains = <&cpg>;
> > resets = <&cpg R9A07G044_IA55_RESETN>;
> > };
> >
> >
> > In the hierarchal interrupt code its parsed as below:
> > on probe fetch the details:
> > range = of_get_property(np, "interrupts", &len);
> > if (!range)
> > return -EINVAL;
> >
> > for (len /= sizeof(*range), j = 0; len >= 3; len -= 3) {
> > if (j >= IRQC_NUM_IRQ)
> > return -EINVAL;
> >
> > priv->map[j].args[0] = be32_to_cpu(*range++);
> > priv->map[j].args[1] = be32_to_cpu(*range++);
> > priv->map[j].args[2] = be32_to_cpu(*range++);
> > priv->map[j].args_count = 3;
> > j++;
>
> Not sure what's wrong, but you shouldn't be doing your own parsing.
> The setup shouldn't look much different than a GPIO controller
> interrupts except you have multiple parent interrupts.
>
Sorry does that mean the IRQ domain should be chained handler and not
hierarchical? Or is it I have miss-understood.

If the IRQ domain has to be hierarchical how do we map to the parent?
(based on the previous reviews Marc had suggested to implement as
hierarchical [1])

[1] https://lore.kernel.org/lkml/[email protected]/T/

Cheers,
Prabhakar

2021-12-05 22:28:05

by Lad, Prabhakar

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Wed, Dec 1, 2021 at 4:16 PM Lad, Prabhakar
<[email protected]> wrote:
>
> On Wed, Dec 1, 2021 at 2:36 PM Rob Herring <[email protected]> wrote:
> >
> > On Wed, Dec 1, 2021 at 7:37 AM Lad, Prabhakar
> > <[email protected]> wrote:
> > >
> > > Hi Marc/Rob,
> > >
> > > On Tue, Nov 30, 2021 at 6:37 PM Marc Zyngier <[email protected]> wrote:
> > > >
> > > > On Tue, 30 Nov 2021 12:52:21 +0000,
> > > > "Lad, Prabhakar" <[email protected]> wrote:
> > > > >
> > > > > On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <[email protected]> wrote:
> > > > > >
> > > > > > interrupts would work just fine here:
> > > > > >
> > > > > > interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> > > > > >
> > > > > > We don't need a different solution for N:1 interrupts from N:M. Sure,
> > > > > > that could become unweldy if there are a lot of interrupts (just like
> > > > > > interrupt-map), but is that an immediate problem?
> > > > > >
> > > > > It's just that with this approach the driver will have to index the
> > > > > interrupts instead of reading from DT.
> > > > >
> > > > > Marc - is it OK with the above approach?
> > > >
> > > > Anything that uses standard properties in a standard way works for me.
> > > >
> > > I added interrupts property now instead of interrupt-map as below:
> > >
> > > irqc: interrupt-controller@110a0000 {
> > > compatible = "renesas,r9a07g044-irqc", "renesas,rzg2l-irqc";
> > > #address-cells = <0>;
> > > interrupt-parent = <&gic>;
> > > interrupt-controller;
> > > reg = <0 0x110a0000 0 0x10000>;
> > > interrupts =
> > > <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 444 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 445 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 446 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 447 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 450 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 451 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 452 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 453 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 454 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 455 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 457 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 459 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 460 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 462 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 463 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 470 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 474 IRQ_TYPE_LEVEL_HIGH>,
> > > <GIC_SPI 475 IRQ_TYPE_LEVEL_HIGH>;
> > > clocks = <&cpg CPG_MOD R9A07G044_IA55_CLK>,
> > > <&cpg CPG_MOD R9A07G044_IA55_PCLK>;
> > > clock-names = "clk", "pclk";
> > > power-domains = <&cpg>;
> > > resets = <&cpg R9A07G044_IA55_RESETN>;
> > > };
> > >
> > >
> > > In the hierarchal interrupt code its parsed as below:
> > > on probe fetch the details:
> > > range = of_get_property(np, "interrupts", &len);
> > > if (!range)
> > > return -EINVAL;
> > >
> > > for (len /= sizeof(*range), j = 0; len >= 3; len -= 3) {
> > > if (j >= IRQC_NUM_IRQ)
> > > return -EINVAL;
> > >
> > > priv->map[j].args[0] = be32_to_cpu(*range++);
> > > priv->map[j].args[1] = be32_to_cpu(*range++);
> > > priv->map[j].args[2] = be32_to_cpu(*range++);
> > > priv->map[j].args_count = 3;
> > > j++;
> >
> > Not sure what's wrong, but you shouldn't be doing your own parsing.
> > The setup shouldn't look much different than a GPIO controller
> > interrupts except you have multiple parent interrupts.
> >
> Sorry does that mean the IRQ domain should be chained handler and not
> hierarchical? Or is it I have miss-understood.
>
> If the IRQ domain has to be hierarchical how do we map to the parent?
> (based on the previous reviews Marc had suggested to implement as
> hierarchical [1])
>
Gentle ping.

> [1] https://lore.kernel.org/lkml/[email protected]/T/
>
> Cheers,
> Prabhakar

2021-12-06 10:26:45

by Marc Zyngier

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Sun, 05 Dec 2021 22:27:35 +0000,
"Lad, Prabhakar" <[email protected]> wrote:
>
> On Wed, Dec 1, 2021 at 4:16 PM Lad, Prabhakar
> <[email protected]> wrote:
> >
> > On Wed, Dec 1, 2021 at 2:36 PM Rob Herring <[email protected]> wrote:
> > >
> > > On Wed, Dec 1, 2021 at 7:37 AM Lad, Prabhakar
> > > <[email protected]> wrote:
> > > >
> > > > Hi Marc/Rob,
> > > >
> > > > On Tue, Nov 30, 2021 at 6:37 PM Marc Zyngier <[email protected]> wrote:
> > > > >
> > > > > On Tue, 30 Nov 2021 12:52:21 +0000,
> > > > > "Lad, Prabhakar" <[email protected]> wrote:
> > > > > >
> > > > > > On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <[email protected]> wrote:
> > > > > > >
> > > > > > > interrupts would work just fine here:
> > > > > > >
> > > > > > > interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > >
> > > > > > > We don't need a different solution for N:1 interrupts from N:M. Sure,
> > > > > > > that could become unweldy if there are a lot of interrupts (just like
> > > > > > > interrupt-map), but is that an immediate problem?
> > > > > > >
> > > > > > It's just that with this approach the driver will have to index the
> > > > > > interrupts instead of reading from DT.
> > > > > >
> > > > > > Marc - is it OK with the above approach?
> > > > >
> > > > > Anything that uses standard properties in a standard way works for me.
> > > > >
> > > > I added interrupts property now instead of interrupt-map as below:
> > > >
> > > > irqc: interrupt-controller@110a0000 {
> > > > compatible = "renesas,r9a07g044-irqc", "renesas,rzg2l-irqc";
> > > > #address-cells = <0>;
> > > > interrupt-parent = <&gic>;
> > > > interrupt-controller;
> > > > reg = <0 0x110a0000 0 0x10000>;
> > > > interrupts =
> > > > <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 444 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 445 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 446 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 447 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 450 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 451 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 452 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 453 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 454 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 455 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 457 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 459 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 460 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 462 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 463 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 470 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 474 IRQ_TYPE_LEVEL_HIGH>,
> > > > <GIC_SPI 475 IRQ_TYPE_LEVEL_HIGH>;
> > > > clocks = <&cpg CPG_MOD R9A07G044_IA55_CLK>,
> > > > <&cpg CPG_MOD R9A07G044_IA55_PCLK>;
> > > > clock-names = "clk", "pclk";
> > > > power-domains = <&cpg>;
> > > > resets = <&cpg R9A07G044_IA55_RESETN>;
> > > > };
> > > >
> > > >
> > > > In the hierarchal interrupt code its parsed as below:
> > > > on probe fetch the details:
> > > > range = of_get_property(np, "interrupts", &len);
> > > > if (!range)
> > > > return -EINVAL;
> > > >
> > > > for (len /= sizeof(*range), j = 0; len >= 3; len -= 3) {
> > > > if (j >= IRQC_NUM_IRQ)
> > > > return -EINVAL;
> > > >
> > > > priv->map[j].args[0] = be32_to_cpu(*range++);
> > > > priv->map[j].args[1] = be32_to_cpu(*range++);
> > > > priv->map[j].args[2] = be32_to_cpu(*range++);
> > > > priv->map[j].args_count = 3;
> > > > j++;
> > >
> > > Not sure what's wrong, but you shouldn't be doing your own parsing.
> > > The setup shouldn't look much different than a GPIO controller
> > > interrupts except you have multiple parent interrupts.
> > >
> > Sorry does that mean the IRQ domain should be chained handler and not
> > hierarchical? Or is it I have miss-understood.

I guess the core DT code allocates the interrupts itself, as if the
interrupt controller was the interrupt producer itself (which isn't
the case here), bypassing the hierarchical setup altogether.

We solved it on the MSI side by not using 'interrupts'. Either we
adopt a similar solution for wired interrupts, or we fix the core DT
code.

> >
> > If the IRQ domain has to be hierarchical how do we map to the parent?
> > (based on the previous reviews Marc had suggested to implement as
> > hierarchical [1])
> >
> Gentle ping.

Please move this discussion to the relevant thread.

M.

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

2021-12-06 17:05:26

by Lad, Prabhakar

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Mon, Dec 6, 2021 at 10:26 AM Marc Zyngier <[email protected]> wrote:
>
> On Sun, 05 Dec 2021 22:27:35 +0000,
> "Lad, Prabhakar" <[email protected]> wrote:
> >
> > On Wed, Dec 1, 2021 at 4:16 PM Lad, Prabhakar
> > <[email protected]> wrote:
> > >
> > > On Wed, Dec 1, 2021 at 2:36 PM Rob Herring <[email protected]> wrote:
> > > >
> > > > On Wed, Dec 1, 2021 at 7:37 AM Lad, Prabhakar
> > > > <[email protected]> wrote:
> > > > >
> > > > > Hi Marc/Rob,
> > > > >
> > > > > On Tue, Nov 30, 2021 at 6:37 PM Marc Zyngier <[email protected]> wrote:
> > > > > >
> > > > > > On Tue, 30 Nov 2021 12:52:21 +0000,
> > > > > > "Lad, Prabhakar" <[email protected]> wrote:
> > > > > > >
> > > > > > > On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <[email protected]> wrote:
> > > > > > > >
> > > > > > > > interrupts would work just fine here:
> > > > > > > >
> > > > > > > > interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > >
> > > > > > > > We don't need a different solution for N:1 interrupts from N:M. Sure,
> > > > > > > > that could become unweldy if there are a lot of interrupts (just like
> > > > > > > > interrupt-map), but is that an immediate problem?
> > > > > > > >
> > > > > > > It's just that with this approach the driver will have to index the
> > > > > > > interrupts instead of reading from DT.
> > > > > > >
> > > > > > > Marc - is it OK with the above approach?
> > > > > >
> > > > > > Anything that uses standard properties in a standard way works for me.
> > > > > >
> > > > > I added interrupts property now instead of interrupt-map as below:
> > > > >
> > > > > irqc: interrupt-controller@110a0000 {
> > > > > compatible = "renesas,r9a07g044-irqc", "renesas,rzg2l-irqc";
> > > > > #address-cells = <0>;
> > > > > interrupt-parent = <&gic>;
> > > > > interrupt-controller;
> > > > > reg = <0 0x110a0000 0 0x10000>;
> > > > > interrupts =
> > > > > <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 444 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 445 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 446 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 447 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 450 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 451 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 452 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 453 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 454 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 455 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 457 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 459 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 460 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 462 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 463 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 470 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 474 IRQ_TYPE_LEVEL_HIGH>,
> > > > > <GIC_SPI 475 IRQ_TYPE_LEVEL_HIGH>;
> > > > > clocks = <&cpg CPG_MOD R9A07G044_IA55_CLK>,
> > > > > <&cpg CPG_MOD R9A07G044_IA55_PCLK>;
> > > > > clock-names = "clk", "pclk";
> > > > > power-domains = <&cpg>;
> > > > > resets = <&cpg R9A07G044_IA55_RESETN>;
> > > > > };
> > > > >
> > > > >
> > > > > In the hierarchal interrupt code its parsed as below:
> > > > > on probe fetch the details:
> > > > > range = of_get_property(np, "interrupts", &len);
> > > > > if (!range)
> > > > > return -EINVAL;
> > > > >
> > > > > for (len /= sizeof(*range), j = 0; len >= 3; len -= 3) {
> > > > > if (j >= IRQC_NUM_IRQ)
> > > > > return -EINVAL;
> > > > >
> > > > > priv->map[j].args[0] = be32_to_cpu(*range++);
> > > > > priv->map[j].args[1] = be32_to_cpu(*range++);
> > > > > priv->map[j].args[2] = be32_to_cpu(*range++);
> > > > > priv->map[j].args_count = 3;
> > > > > j++;
> > > >
> > > > Not sure what's wrong, but you shouldn't be doing your own parsing.
> > > > The setup shouldn't look much different than a GPIO controller
> > > > interrupts except you have multiple parent interrupts.
> > > >
> > > Sorry does that mean the IRQ domain should be chained handler and not
> > > hierarchical? Or is it I have miss-understood.
>
> I guess the core DT code allocates the interrupts itself, as if the
> interrupt controller was the interrupt producer itself (which isn't
> the case here), bypassing the hierarchical setup altogether.
>
> We solved it on the MSI side by not using 'interrupts'. Either we
> adopt a similar solution for wired interrupts, or we fix the core DT
> code.
>
So maybe for now we go with your earlier suggestion of using
"interrupt-range"? (And address the core DT in near future)

Rob, is that OK with you?

Cheers,
Prabhakar

> > >
> > > If the IRQ domain has to be hierarchical how do we map to the parent?
> > > (based on the previous reviews Marc had suggested to implement as
> > > hierarchical [1])
> > >
> > Gentle ping.
>
> Please move this discussion to the relevant thread.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.

2021-12-06 19:00:07

by Rob Herring (Arm)

Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map

On Mon, Dec 6, 2021 at 10:55 AM Lad, Prabhakar
<[email protected]> wrote:
>
> On Mon, Dec 6, 2021 at 10:26 AM Marc Zyngier <[email protected]> wrote:
> >
> > On Sun, 05 Dec 2021 22:27:35 +0000,
> > "Lad, Prabhakar" <[email protected]> wrote:
> > >
> > > On Wed, Dec 1, 2021 at 4:16 PM Lad, Prabhakar
> > > <[email protected]> wrote:
> > > >
> > > > On Wed, Dec 1, 2021 at 2:36 PM Rob Herring <[email protected]> wrote:
> > > > >
> > > > > On Wed, Dec 1, 2021 at 7:37 AM Lad, Prabhakar
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > Hi Marc/Rob,
> > > > > >
> > > > > > On Tue, Nov 30, 2021 at 6:37 PM Marc Zyngier <[email protected]> wrote:
> > > > > > >
> > > > > > > On Tue, 30 Nov 2021 12:52:21 +0000,
> > > > > > > "Lad, Prabhakar" <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > interrupts would work just fine here:
> > > > > > > > >
> > > > > > > > > interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > > <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > > <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > > <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > > <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > > <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > > <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > > <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > > >
> > > > > > > > > We don't need a different solution for N:1 interrupts from N:M. Sure,
> > > > > > > > > that could become unweldy if there are a lot of interrupts (just like
> > > > > > > > > interrupt-map), but is that an immediate problem?
> > > > > > > > >
> > > > > > > > It's just that with this approach the driver will have to index the
> > > > > > > > interrupts instead of reading from DT.
> > > > > > > >
> > > > > > > > Marc - is it OK with the above approach?
> > > > > > >
> > > > > > > Anything that uses standard properties in a standard way works for me.
> > > > > > >
> > > > > > I added interrupts property now instead of interrupt-map as below:
> > > > > >
> > > > > > irqc: interrupt-controller@110a0000 {
> > > > > > compatible = "renesas,r9a07g044-irqc", "renesas,rzg2l-irqc";
> > > > > > #address-cells = <0>;
> > > > > > interrupt-parent = <&gic>;
> > > > > > interrupt-controller;
> > > > > > reg = <0 0x110a0000 0 0x10000>;
> > > > > > interrupts =
> > > > > > <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 444 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 445 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 446 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 447 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 450 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 451 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 452 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 453 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 454 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 455 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 457 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 459 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 460 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 462 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 463 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 470 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 474 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 475 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > clocks = <&cpg CPG_MOD R9A07G044_IA55_CLK>,
> > > > > > <&cpg CPG_MOD R9A07G044_IA55_PCLK>;
> > > > > > clock-names = "clk", "pclk";
> > > > > > power-domains = <&cpg>;
> > > > > > resets = <&cpg R9A07G044_IA55_RESETN>;
> > > > > > };
> > > > > >
> > > > > >
> > > > > > In the hierarchal interrupt code its parsed as below:
> > > > > > on probe fetch the details:
> > > > > > range = of_get_property(np, "interrupts", &len);
> > > > > > if (!range)
> > > > > > return -EINVAL;
> > > > > >
> > > > > > for (len /= sizeof(*range), j = 0; len >= 3; len -= 3) {
> > > > > > if (j >= IRQC_NUM_IRQ)
> > > > > > return -EINVAL;
> > > > > >
> > > > > > priv->map[j].args[0] = be32_to_cpu(*range++);
> > > > > > priv->map[j].args[1] = be32_to_cpu(*range++);
> > > > > > priv->map[j].args[2] = be32_to_cpu(*range++);
> > > > > > priv->map[j].args_count = 3;
> > > > > > j++;
> > > > >
> > > > > Not sure what's wrong, but you shouldn't be doing your own parsing.
> > > > > The setup shouldn't look much different than a GPIO controller
> > > > > interrupts except you have multiple parent interrupts.
> > > > >
> > > > Sorry does that mean the IRQ domain should be chained handler and not
> > > > hierarchical? Or is it I have miss-understood.
> >
> > I guess the core DT code allocates the interrupts itself, as if the
> > interrupt controller was the interrupt producer itself (which isn't
> > the case here), bypassing the hierarchical setup altogether.
> >
> > We solved it on the MSI side by not using 'interrupts'. Either we
> > adopt a similar solution for wired interrupts, or we fix the core DT
> > code.
> >
> So maybe for now we go with your earlier suggestion of using
> "interrupt-range"? (And address the core DT in near future)
>
> Rob, is that OK with you?

No. The existing bindings are sufficient for describing what you need
to describe. If the kernel can't handle that, that's no reason for a
new binding.

The core code needs to handle all this whether it's 'interrupts' or
'interrupt-range' you are parsing. Sorry, but I really don't
understand the hierarchical stuff to provide better guidance.

Rob