* [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map
@ 2021-11-22 10:30 Marc Zyngier
2021-11-22 13:10 ` Geert Uytterhoeven
` (2 more replies)
0 siblings, 3 replies; 29+ messages in thread
From: Marc Zyngier @ 2021-11-22 10:30 UTC (permalink / raw)
To: linux-kernel, devicetree
Cc: kernel-team, Rob Herring, John Crispin, Biwen Li, Chris Brandt,
Geert Uytterhoeven
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 <robh@kernel.org>
Cc: John Crispin <john@phrozen.org>
Cc: Biwen Li <biwen.li@nxp.com>
Cc: Chris Brandt <chris.brandt@renesas.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-22 10:30 [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map Marc Zyngier @ 2021-11-22 13:10 ` Geert Uytterhoeven 2021-11-22 13:54 ` Marc Zyngier 2021-11-23 8:40 ` Sander Vanheule 2021-11-29 19:15 ` Rob Herring 2 siblings, 1 reply; 29+ messages in thread From: Geert Uytterhoeven @ 2021-11-22 13:10 UTC (permalink / raw) To: Marc Zyngier Cc: linux-kernel, devicetree, kernel-team, Rob Herring, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc Hi Marc, On Mon, Nov 22, 2021 at 11:30 AM Marc Zyngier <maz@kernel.org> 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 <robh@kernel.org> > Cc: John Crispin <john@phrozen.org> > Cc: Biwen Li <biwen.li@nxp.com> > Cc: Chris Brandt <chris.brandt@renesas.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Marc Zyngier <maz@kernel.org> 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/bbe5506a2458b2d6049bd22a5fda77ae6175ddec.camel@svanheule.net/ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-22 13:10 ` Geert Uytterhoeven @ 2021-11-22 13:54 ` Marc Zyngier 2021-11-22 16:58 ` Geert Uytterhoeven 0 siblings, 1 reply; 29+ messages in thread From: Marc Zyngier @ 2021-11-22 13:54 UTC (permalink / raw) To: Geert Uytterhoeven Cc: linux-kernel, devicetree, kernel-team, Rob Herring, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc On Mon, 22 Nov 2021 13:10:32 +0000, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Marc, > > On Mon, Nov 22, 2021 at 11:30 AM Marc Zyngier <maz@kernel.org> 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 <robh@kernel.org> > > Cc: John Crispin <john@phrozen.org> > > Cc: Biwen Li <biwen.li@nxp.com> > > Cc: Chris Brandt <chris.brandt@renesas.com> > > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-22 13:54 ` Marc Zyngier @ 2021-11-22 16:58 ` Geert Uytterhoeven 2021-11-23 7:57 ` Geert Uytterhoeven 0 siblings, 1 reply; 29+ messages in thread From: Geert Uytterhoeven @ 2021-11-22 16:58 UTC (permalink / raw) To: Marc Zyngier Cc: linux-kernel, devicetree, kernel-team, Rob Herring, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc Hi Marc, On Mon, Nov 22, 2021 at 2:54 PM Marc Zyngier <maz@kernel.org> wrote: > On Mon, 22 Nov 2021 13:10:32 +0000, > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, Nov 22, 2021 at 11:30 AM Marc Zyngier <maz@kernel.org> 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 -- geert@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-22 16:58 ` Geert Uytterhoeven @ 2021-11-23 7:57 ` Geert Uytterhoeven 2021-11-23 8:33 ` Marc Zyngier 0 siblings, 1 reply; 29+ messages in thread From: Geert Uytterhoeven @ 2021-11-23 7:57 UTC (permalink / raw) To: Marc Zyngier Cc: linux-kernel, devicetree, kernel-team, Rob Herring, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc Hi Marc, On Mon, Nov 22, 2021 at 5:58 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, Nov 22, 2021 at 2:54 PM Marc Zyngier <maz@kernel.org> wrote: > > On Mon, 22 Nov 2021 13:10:32 +0000, > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Mon, Nov 22, 2021 at 11:30 AM Marc Zyngier <maz@kernel.org> 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 -- geert@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-23 7:57 ` Geert Uytterhoeven @ 2021-11-23 8:33 ` Marc Zyngier 2021-11-23 8:44 ` Geert Uytterhoeven 2021-11-23 11:02 ` Geert Uytterhoeven 0 siblings, 2 replies; 29+ messages in thread From: Marc Zyngier @ 2021-11-23 8:33 UTC (permalink / raw) To: Geert Uytterhoeven Cc: linux-kernel, devicetree, kernel-team, Rob Herring, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc On Tue, 23 Nov 2021 07:57:48 +0000, Geert Uytterhoeven <geert@linux-m68k.org> 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-23 8:33 ` Marc Zyngier @ 2021-11-23 8:44 ` Geert Uytterhoeven 2021-11-23 9:11 ` Marc Zyngier 2021-11-23 11:02 ` Geert Uytterhoeven 1 sibling, 1 reply; 29+ messages in thread From: Geert Uytterhoeven @ 2021-11-23 8:44 UTC (permalink / raw) To: Marc Zyngier Cc: linux-kernel, devicetree, kernel-team, Rob Herring, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc, Lad Prabhakar Hi Marc, On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <maz@kernel.org> wrote: > On Tue, 23 Nov 2021 07:57:48 +0000, > Geert Uytterhoeven <geert@linux-m68k.org> 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/20211110225808.16388-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-23 8:44 ` Geert Uytterhoeven @ 2021-11-23 9:11 ` Marc Zyngier 2021-11-24 7:54 ` Geert Uytterhoeven 2021-11-27 0:42 ` Prabhakar Mahadev Lad 0 siblings, 2 replies; 29+ messages in thread From: Marc Zyngier @ 2021-11-23 9:11 UTC (permalink / raw) To: Geert Uytterhoeven Cc: linux-kernel, devicetree, kernel-team, Rob Herring, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc, Lad Prabhakar On Tue, 23 Nov 2021 08:44:19 +0000, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Marc, > > On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 23 Nov 2021 07:57:48 +0000, > > Geert Uytterhoeven <geert@linux-m68k.org> 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-23 9:11 ` Marc Zyngier @ 2021-11-24 7:54 ` Geert Uytterhoeven 2021-11-24 11:14 ` Marc Zyngier 2021-11-27 0:42 ` Prabhakar Mahadev Lad 1 sibling, 1 reply; 29+ messages in thread From: Geert Uytterhoeven @ 2021-11-24 7:54 UTC (permalink / raw) To: Marc Zyngier, Chris Brandt Cc: Linux Kernel Mailing List, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Android Kernel Team, Rob Herring, John Crispin, Biwen Li, Linux-Renesas, Lad Prabhakar Hi Marc, On Tue, Nov 23, 2021 at 10:11 AM Marc Zyngier <maz@kernel.org> wrote: > On Tue, 23 Nov 2021 08:44:19 +0000, > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <maz@kernel.org> wrote: > > > On Tue, 23 Nov 2021 07:57:48 +0000, > > > Geert Uytterhoeven <geert@linux-m68k.org> 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 -- geert@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-24 7:54 ` Geert Uytterhoeven @ 2021-11-24 11:14 ` Marc Zyngier 0 siblings, 0 replies; 29+ messages in thread From: Marc Zyngier @ 2021-11-24 11:14 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Chris Brandt, Linux Kernel Mailing List, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Android Kernel Team, Rob Herring, John Crispin, Biwen Li, Linux-Renesas, Lad Prabhakar On Wed, 24 Nov 2021 07:54:48 +0000, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Marc, > > On Tue, Nov 23, 2021 at 10:11 AM Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 23 Nov 2021 08:44:19 +0000, > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Tue, 23 Nov 2021 07:57:48 +0000, > > > > Geert Uytterhoeven <geert@linux-m68k.org> 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* RE: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-23 9:11 ` Marc Zyngier 2021-11-24 7:54 ` Geert Uytterhoeven @ 2021-11-27 0:42 ` Prabhakar Mahadev Lad 2021-11-29 10:34 ` Marc Zyngier 1 sibling, 1 reply; 29+ messages in thread From: Prabhakar Mahadev Lad @ 2021-11-27 0:42 UTC (permalink / raw) To: Marc Zyngier, Geert Uytterhoeven, Rob Herring Cc: linux-kernel, devicetree, kernel-team, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc, Lad, Prabhakar Hi Marc, > -----Original Message----- > From: Marc Zyngier <maz@kernel.org> > Sent: 23 November 2021 09:11 > To: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; kernel-team@android.com; Rob Herring > <robh@kernel.org>; John Crispin <john@phrozen.org>; Biwen Li <biwen.li@nxp.com>; Chris Brandt > <Chris.Brandt@renesas.com>; linux-renesas-soc@vger.kernel.org; Prabhakar Mahadev Lad > <prabhakar.mahadev-lad.rj@bp.renesas.com> > 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 <geert@linux-m68k.org> wrote: > > > > Hi Marc, > > > > On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <maz@kernel.org> wrote: > > > On Tue, 23 Nov 2021 07:57:48 +0000, > > > Geert Uytterhoeven <geert@linux-m68k.org> 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-27 0:42 ` Prabhakar Mahadev Lad @ 2021-11-29 10:34 ` Marc Zyngier 2021-11-29 12:13 ` Lad, Prabhakar 0 siblings, 1 reply; 29+ messages in thread From: Marc Zyngier @ 2021-11-29 10:34 UTC (permalink / raw) To: Prabhakar Mahadev Lad Cc: Geert Uytterhoeven, Rob Herring, linux-kernel, devicetree, kernel-team, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc, Lad, Prabhakar On Sat, 27 Nov 2021 00:42:49 +0000, Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Hi Marc, > > > -----Original Message----- > > From: Marc Zyngier <maz@kernel.org> > > Sent: 23 November 2021 09:11 > > To: Geert Uytterhoeven <geert@linux-m68k.org> > > Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; kernel-team@android.com; Rob Herring > > <robh@kernel.org>; John Crispin <john@phrozen.org>; Biwen Li <biwen.li@nxp.com>; Chris Brandt > > <Chris.Brandt@renesas.com>; linux-renesas-soc@vger.kernel.org; Prabhakar Mahadev Lad > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > 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 <geert@linux-m68k.org> wrote: > > > > > > Hi Marc, > > > > > > On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Tue, 23 Nov 2021 07:57:48 +0000, > > > > Geert Uytterhoeven <geert@linux-m68k.org> 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-29 10:34 ` Marc Zyngier @ 2021-11-29 12:13 ` Lad, Prabhakar 2021-11-29 18:33 ` Rob Herring 0 siblings, 1 reply; 29+ messages in thread From: Lad, Prabhakar @ 2021-11-29 12:13 UTC (permalink / raw) To: Marc Zyngier, Rob Herring Cc: Prabhakar Mahadev Lad, Geert Uytterhoeven, linux-kernel, devicetree, kernel-team, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc On Mon, Nov 29, 2021 at 10:34 AM Marc Zyngier <maz@kernel.org> wrote: > > On Sat, 27 Nov 2021 00:42:49 +0000, > Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > Hi Marc, > > > > > -----Original Message----- > > > From: Marc Zyngier <maz@kernel.org> > > > Sent: 23 November 2021 09:11 > > > To: Geert Uytterhoeven <geert@linux-m68k.org> > > > Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; kernel-team@android.com; Rob Herring > > > <robh@kernel.org>; John Crispin <john@phrozen.org>; Biwen Li <biwen.li@nxp.com>; Chris Brandt > > > <Chris.Brandt@renesas.com>; linux-renesas-soc@vger.kernel.org; Prabhakar Mahadev Lad > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > 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 <geert@linux-m68k.org> wrote: > > > > > > > > Hi Marc, > > > > > > > > On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > On Tue, 23 Nov 2021 07:57:48 +0000, > > > > > Geert Uytterhoeven <geert@linux-m68k.org> 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-29 12:13 ` Lad, Prabhakar @ 2021-11-29 18:33 ` Rob Herring 2021-11-30 12:52 ` Lad, Prabhakar 0 siblings, 1 reply; 29+ messages in thread From: Rob Herring @ 2021-11-29 18:33 UTC (permalink / raw) To: Lad, Prabhakar, Marc Zyngier Cc: Prabhakar Mahadev Lad, Geert Uytterhoeven, linux-kernel, devicetree, kernel-team, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc On Mon, Nov 29, 2021 at 6:13 AM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > I timed my vacation well... > On Mon, Nov 29, 2021 at 10:34 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Sat, 27 Nov 2021 00:42:49 +0000, > > Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > > Hi Marc, > > > > > > > -----Original Message----- > > > > From: Marc Zyngier <maz@kernel.org> > > > > Sent: 23 November 2021 09:11 > > > > To: Geert Uytterhoeven <geert@linux-m68k.org> > > > > Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; kernel-team@android.com; Rob Herring > > > > <robh@kernel.org>; John Crispin <john@phrozen.org>; Biwen Li <biwen.li@nxp.com>; Chris Brandt > > > > <Chris.Brandt@renesas.com>; linux-renesas-soc@vger.kernel.org; Prabhakar Mahadev Lad > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > 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 <geert@linux-m68k.org> wrote: > > > > > > > > > > Hi Marc, > > > > > > > > > > On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Tue, 23 Nov 2021 07:57:48 +0000, > > > > > > Geert Uytterhoeven <geert@linux-m68k.org> 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-29 18:33 ` Rob Herring @ 2021-11-30 12:52 ` Lad, Prabhakar 2021-11-30 18:36 ` Marc Zyngier 0 siblings, 1 reply; 29+ messages in thread From: Lad, Prabhakar @ 2021-11-30 12:52 UTC (permalink / raw) To: Rob Herring, Marc Zyngier Cc: Prabhakar Mahadev Lad, Geert Uytterhoeven, linux-kernel, devicetree, kernel-team, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <robh@kernel.org> wrote: > > On Mon, Nov 29, 2021 at 6:13 AM Lad, Prabhakar > <prabhakar.csengg@gmail.com> wrote: > > > > I timed my vacation well... > > > On Mon, Nov 29, 2021 at 10:34 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Sat, 27 Nov 2021 00:42:49 +0000, > > > Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > > > > Hi Marc, > > > > > > > > > -----Original Message----- > > > > > From: Marc Zyngier <maz@kernel.org> > > > > > Sent: 23 November 2021 09:11 > > > > > To: Geert Uytterhoeven <geert@linux-m68k.org> > > > > > Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; kernel-team@android.com; Rob Herring > > > > > <robh@kernel.org>; John Crispin <john@phrozen.org>; Biwen Li <biwen.li@nxp.com>; Chris Brandt > > > > > <Chris.Brandt@renesas.com>; linux-renesas-soc@vger.kernel.org; Prabhakar Mahadev Lad > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > 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 <geert@linux-m68k.org> wrote: > > > > > > > > > > > > Hi Marc, > > > > > > > > > > > > On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > On Tue, 23 Nov 2021 07:57:48 +0000, > > > > > > > Geert Uytterhoeven <geert@linux-m68k.org> 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-30 12:52 ` Lad, Prabhakar @ 2021-11-30 18:36 ` Marc Zyngier 2021-12-01 13:36 ` Lad, Prabhakar 0 siblings, 1 reply; 29+ messages in thread From: Marc Zyngier @ 2021-11-30 18:36 UTC (permalink / raw) To: Lad, Prabhakar Cc: Rob Herring, Prabhakar Mahadev Lad, Geert Uytterhoeven, linux-kernel, devicetree, kernel-team, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc On Tue, 30 Nov 2021 12:52:21 +0000, "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <robh@kernel.org> 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-30 18:36 ` Marc Zyngier @ 2021-12-01 13:36 ` Lad, Prabhakar 2021-12-01 14:35 ` Rob Herring 0 siblings, 1 reply; 29+ messages in thread From: Lad, Prabhakar @ 2021-12-01 13:36 UTC (permalink / raw) To: Marc Zyngier, Rob Herring Cc: Geert Uytterhoeven, Prabhakar Mahadev Lad, linux-kernel, devicetree, kernel-team, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc Hi Marc/Rob, On Tue, Nov 30, 2021 at 6:37 PM Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 30 Nov 2021 12:52:21 +0000, > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <robh@kernel.org> 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): ð0 { pinctrl-0 = <ð0_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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-12-01 13:36 ` Lad, Prabhakar @ 2021-12-01 14:35 ` Rob Herring 2021-12-01 16:16 ` Lad, Prabhakar 0 siblings, 1 reply; 29+ messages in thread From: Rob Herring @ 2021-12-01 14:35 UTC (permalink / raw) To: Lad, Prabhakar Cc: Marc Zyngier, Geert Uytterhoeven, Prabhakar Mahadev Lad, linux-kernel, devicetree, kernel-team, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc On Wed, Dec 1, 2021 at 7:37 AM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > > Hi Marc/Rob, > > On Tue, Nov 30, 2021 at 6:37 PM Marc Zyngier <maz@kernel.org> wrote: > > > > On Tue, 30 Nov 2021 12:52:21 +0000, > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > > > On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <robh@kernel.org> 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-12-01 14:35 ` Rob Herring @ 2021-12-01 16:16 ` Lad, Prabhakar 2021-12-05 22:27 ` Lad, Prabhakar 0 siblings, 1 reply; 29+ messages in thread From: Lad, Prabhakar @ 2021-12-01 16:16 UTC (permalink / raw) To: Rob Herring, Marc Zyngier Cc: Geert Uytterhoeven, Prabhakar Mahadev Lad, linux-kernel, devicetree, kernel-team, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc On Wed, Dec 1, 2021 at 2:36 PM Rob Herring <robh@kernel.org> wrote: > > On Wed, Dec 1, 2021 at 7:37 AM Lad, Prabhakar > <prabhakar.csengg@gmail.com> wrote: > > > > Hi Marc/Rob, > > > > On Tue, Nov 30, 2021 at 6:37 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Tue, 30 Nov 2021 12:52:21 +0000, > > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > > > > > On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <robh@kernel.org> 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/20211110225808.16388-1-prabhakar.mahadev-lad.rj@bp.renesas.com/T/ Cheers, Prabhakar ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-12-01 16:16 ` Lad, Prabhakar @ 2021-12-05 22:27 ` Lad, Prabhakar 2021-12-06 10:26 ` Marc Zyngier 0 siblings, 1 reply; 29+ messages in thread From: Lad, Prabhakar @ 2021-12-05 22:27 UTC (permalink / raw) To: Marc Zyngier, Rob Herring Cc: Geert Uytterhoeven, Prabhakar Mahadev Lad, linux-kernel, devicetree, kernel-team, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc On Wed, Dec 1, 2021 at 4:16 PM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > > On Wed, Dec 1, 2021 at 2:36 PM Rob Herring <robh@kernel.org> wrote: > > > > On Wed, Dec 1, 2021 at 7:37 AM Lad, Prabhakar > > <prabhakar.csengg@gmail.com> wrote: > > > > > > Hi Marc/Rob, > > > > > > On Tue, Nov 30, 2021 at 6:37 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > On Tue, 30 Nov 2021 12:52:21 +0000, > > > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > > > > > > > On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <robh@kernel.org> 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/20211110225808.16388-1-prabhakar.mahadev-lad.rj@bp.renesas.com/T/ > > Cheers, > Prabhakar ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-12-05 22:27 ` Lad, Prabhakar @ 2021-12-06 10:26 ` Marc Zyngier 2021-12-06 16:55 ` Lad, Prabhakar 0 siblings, 1 reply; 29+ messages in thread From: Marc Zyngier @ 2021-12-06 10:26 UTC (permalink / raw) To: Lad, Prabhakar Cc: Rob Herring, Geert Uytterhoeven, Prabhakar Mahadev Lad, linux-kernel, devicetree, kernel-team, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc On Sun, 05 Dec 2021 22:27:35 +0000, "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > On Wed, Dec 1, 2021 at 4:16 PM Lad, Prabhakar > <prabhakar.csengg@gmail.com> wrote: > > > > On Wed, Dec 1, 2021 at 2:36 PM Rob Herring <robh@kernel.org> wrote: > > > > > > On Wed, Dec 1, 2021 at 7:37 AM Lad, Prabhakar > > > <prabhakar.csengg@gmail.com> wrote: > > > > > > > > Hi Marc/Rob, > > > > > > > > On Tue, Nov 30, 2021 at 6:37 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > On Tue, 30 Nov 2021 12:52:21 +0000, > > > > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > > > > > > > > > On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <robh@kernel.org> 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-12-06 10:26 ` Marc Zyngier @ 2021-12-06 16:55 ` Lad, Prabhakar 2021-12-06 18:59 ` Rob Herring 0 siblings, 1 reply; 29+ messages in thread From: Lad, Prabhakar @ 2021-12-06 16:55 UTC (permalink / raw) To: Marc Zyngier, Rob Herring Cc: Geert Uytterhoeven, Prabhakar Mahadev Lad, linux-kernel, devicetree, kernel-team, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc On Mon, Dec 6, 2021 at 10:26 AM Marc Zyngier <maz@kernel.org> wrote: > > On Sun, 05 Dec 2021 22:27:35 +0000, > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > On Wed, Dec 1, 2021 at 4:16 PM Lad, Prabhakar > > <prabhakar.csengg@gmail.com> wrote: > > > > > > On Wed, Dec 1, 2021 at 2:36 PM Rob Herring <robh@kernel.org> wrote: > > > > > > > > On Wed, Dec 1, 2021 at 7:37 AM Lad, Prabhakar > > > > <prabhakar.csengg@gmail.com> wrote: > > > > > > > > > > Hi Marc/Rob, > > > > > > > > > > On Tue, Nov 30, 2021 at 6:37 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > > > On Tue, 30 Nov 2021 12:52:21 +0000, > > > > > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > > > > > > > > > > > On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <robh@kernel.org> 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-12-06 16:55 ` Lad, Prabhakar @ 2021-12-06 18:59 ` Rob Herring 0 siblings, 0 replies; 29+ messages in thread From: Rob Herring @ 2021-12-06 18:59 UTC (permalink / raw) To: Lad, Prabhakar Cc: Marc Zyngier, Geert Uytterhoeven, Prabhakar Mahadev Lad, linux-kernel, devicetree, kernel-team, John Crispin, Biwen Li, Chris Brandt, linux-renesas-soc On Mon, Dec 6, 2021 at 10:55 AM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > > On Mon, Dec 6, 2021 at 10:26 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Sun, 05 Dec 2021 22:27:35 +0000, > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > > > On Wed, Dec 1, 2021 at 4:16 PM Lad, Prabhakar > > > <prabhakar.csengg@gmail.com> wrote: > > > > > > > > On Wed, Dec 1, 2021 at 2:36 PM Rob Herring <robh@kernel.org> wrote: > > > > > > > > > > On Wed, Dec 1, 2021 at 7:37 AM Lad, Prabhakar > > > > > <prabhakar.csengg@gmail.com> wrote: > > > > > > > > > > > > Hi Marc/Rob, > > > > > > > > > > > > On Tue, Nov 30, 2021 at 6:37 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > > > > > On Tue, 30 Nov 2021 12:52:21 +0000, > > > > > > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > > > > > > > > > > > > > On Mon, Nov 29, 2021 at 6:33 PM Rob Herring <robh@kernel.org> 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-23 8:33 ` Marc Zyngier 2021-11-23 8:44 ` Geert Uytterhoeven @ 2021-11-23 11:02 ` Geert Uytterhoeven 1 sibling, 0 replies; 29+ messages in thread From: Geert Uytterhoeven @ 2021-11-23 11:02 UTC (permalink / raw) To: Marc Zyngier Cc: Linux Kernel Mailing List, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Android Kernel Team, Rob Herring, John Crispin, Biwen Li, Chris Brandt, Linux-Renesas Hi Marc, On Tue, Nov 23, 2021 at 9:33 AM Marc Zyngier <maz@kernel.org> wrote: > On Tue, 23 Nov 2021 07:57:48 +0000, > Geert Uytterhoeven <geert@linux-m68k.org> 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 -- geert@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-22 10:30 [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map Marc Zyngier 2021-11-22 13:10 ` Geert Uytterhoeven @ 2021-11-23 8:40 ` Sander Vanheule 2021-11-23 9:12 ` Marc Zyngier 2021-11-29 19:15 ` Rob Herring 2 siblings, 1 reply; 29+ messages in thread From: Sander Vanheule @ 2021-11-23 8:40 UTC (permalink / raw) To: Marc Zyngier, linux-kernel, devicetree Cc: kernel-team, Rob Herring, John Crispin, Biwen Li, Chris Brandt, Geert Uytterhoeven 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 <robh@kernel.org> > Cc: John Crispin <john@phrozen.org> > Cc: Biwen Li <biwen.li@nxp.com> > Cc: Chris Brandt <chris.brandt@renesas.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Marc Zyngier <maz@kernel.org> 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-23 8:40 ` Sander Vanheule @ 2021-11-23 9:12 ` Marc Zyngier 0 siblings, 0 replies; 29+ messages in thread From: Marc Zyngier @ 2021-11-23 9:12 UTC (permalink / raw) To: Sander Vanheule Cc: linux-kernel, devicetree, kernel-team, Rob Herring, John Crispin, Biwen Li, Chris Brandt, Geert Uytterhoeven On Tue, 23 Nov 2021 08:40:29 +0000, Sander Vanheule <sander@svanheule.net> 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 <robh@kernel.org> > > Cc: John Crispin <john@phrozen.org> > > Cc: Biwen Li <biwen.li@nxp.com> > > Cc: Chris Brandt <chris.brandt@renesas.com> > > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-22 10:30 [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map Marc Zyngier 2021-11-22 13:10 ` Geert Uytterhoeven 2021-11-23 8:40 ` Sander Vanheule @ 2021-11-29 19:15 ` Rob Herring 2021-11-29 19:31 ` Marc Zyngier 2 siblings, 1 reply; 29+ messages in thread From: Rob Herring @ 2021-11-29 19:15 UTC (permalink / raw) To: Marc Zyngier Cc: linux-kernel, devicetree, Android Kernel Team, John Crispin, Biwen Li, Chris Brandt, Geert Uytterhoeven On Mon, Nov 22, 2021 at 4:30 AM Marc Zyngier <maz@kernel.org> 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 <robh@kernel.org> > Cc: John Crispin <john@phrozen.org> > Cc: Biwen Li <biwen.li@nxp.com> > Cc: Chris Brandt <chris.brandt@renesas.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-29 19:15 ` Rob Herring @ 2021-11-29 19:31 ` Marc Zyngier 2021-11-29 19:40 ` Rob Herring 0 siblings, 1 reply; 29+ messages in thread From: Marc Zyngier @ 2021-11-29 19:31 UTC (permalink / raw) To: Rob Herring Cc: linux-kernel, devicetree, Android Kernel Team, John Crispin, Biwen Li, Chris Brandt, Geert Uytterhoeven On Mon, 29 Nov 2021 19:15:27 +0000, Rob Herring <robh@kernel.org> wrote: > > On Mon, Nov 22, 2021 at 4:30 AM Marc Zyngier <maz@kernel.org> 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 <robh@kernel.org> > > Cc: John Crispin <john@phrozen.org> > > Cc: Biwen Li <biwen.li@nxp.com> > > Cc: Chris Brandt <chris.brandt@renesas.com> > > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map 2021-11-29 19:31 ` Marc Zyngier @ 2021-11-29 19:40 ` Rob Herring 0 siblings, 0 replies; 29+ messages in thread From: Rob Herring @ 2021-11-29 19:40 UTC (permalink / raw) To: Marc Zyngier Cc: linux-kernel, devicetree, Android Kernel Team, John Crispin, Biwen Li, Chris Brandt, Geert Uytterhoeven On Mon, Nov 29, 2021 at 1:31 PM Marc Zyngier <maz@kernel.org> wrote: > > On Mon, 29 Nov 2021 19:15:27 +0000, > Rob Herring <robh@kernel.org> wrote: > > > > On Mon, Nov 22, 2021 at 4:30 AM Marc Zyngier <maz@kernel.org> 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 <robh@kernel.org> > > > Cc: John Crispin <john@phrozen.org> > > > Cc: Biwen Li <biwen.li@nxp.com> > > > Cc: Chris Brandt <chris.brandt@renesas.com> > > > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > > --- > > > 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2021-12-06 19:00 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-22 10:30 [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map Marc Zyngier 2021-11-22 13:10 ` Geert Uytterhoeven 2021-11-22 13:54 ` Marc Zyngier 2021-11-22 16:58 ` Geert Uytterhoeven 2021-11-23 7:57 ` Geert Uytterhoeven 2021-11-23 8:33 ` Marc Zyngier 2021-11-23 8:44 ` Geert Uytterhoeven 2021-11-23 9:11 ` Marc Zyngier 2021-11-24 7:54 ` Geert Uytterhoeven 2021-11-24 11:14 ` Marc Zyngier 2021-11-27 0:42 ` Prabhakar Mahadev Lad 2021-11-29 10:34 ` Marc Zyngier 2021-11-29 12:13 ` Lad, Prabhakar 2021-11-29 18:33 ` Rob Herring 2021-11-30 12:52 ` Lad, Prabhakar 2021-11-30 18:36 ` Marc Zyngier 2021-12-01 13:36 ` Lad, Prabhakar 2021-12-01 14:35 ` Rob Herring 2021-12-01 16:16 ` Lad, Prabhakar 2021-12-05 22:27 ` Lad, Prabhakar 2021-12-06 10:26 ` Marc Zyngier 2021-12-06 16:55 ` Lad, Prabhakar 2021-12-06 18:59 ` Rob Herring 2021-11-23 11:02 ` Geert Uytterhoeven 2021-11-23 8:40 ` Sander Vanheule 2021-11-23 9:12 ` Marc Zyngier 2021-11-29 19:15 ` Rob Herring 2021-11-29 19:31 ` Marc Zyngier 2021-11-29 19:40 ` Rob Herring
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.