| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<TYRPR01MB15619ED191A00BDE5087CF1DC8585A@TYRPR01MB15619.jpnprd01.prod.outlook.com>
Date: Thu, 8 Jan 2026 18:56:39 +0000
From: Cosmin-Gabriel Tanislav <cosmin-gabriel.tanislav.xa@...esas.com>
To: geert <geert@...ux-m68k.org>
CC: Linus Walleij <linusw@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
magnus.damm <magnus.damm@...il.com>, Prabhakar Mahadev Lad
<prabhakar.mahadev-lad.rj@...renesas.com>,
"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 4/8] pinctrl: renesas: rzt2h: add GPIO IRQ chip to
handle interrupts
> From: Geert Uytterhoeven <geert@...ux-m68k.org>
> Sent: Thursday, January 8, 2026 7:31 PM
>
> Hi Cosmin,
>
> On Fri, 5 Dec 2025 at 16:03, Cosmin Tanislav
> <cosmin-gabriel.tanislav.xa@...esas.com> wrote:
> > The Renesas RZ/T2H (R9A09G077) and Renesas RZ/N2H (R9A09G087) SoCs have
> > IRQ-capable pins handled by the ICU, which forwards them to the GIC.
> >
> > The ICU supports 16 IRQ lines, the pins map to these lines arbitrarily,
> > and the mapping is not configurable.
> >
> > Add a GPIO IRQ chip to the pin controller that can be used to configure
> > these pins as IRQ lines.
> >
> > The pin controller places the requested pins into IRQ function,
> > disabling GPIO mode. A hierarchical IRQ domain is used to forward other
> > functionality to the parent IRQ domain, the ICU. The ICU does level
> > translation and then forwards other functionality to the GIC.
> >
> > Wakeup capability is implemented by placing the entire pin controller on
> > the wakeup path if any pins are requested to be wakeup-capable.
> >
> > Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@...esas.com>
>
> Thanks for your patch!
>
> > --- a/drivers/pinctrl/renesas/pinctrl-rzt2h.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzt2h.c
>
> > @@ -644,14 +650,194 @@ static const char * const rzt2h_gpio_names[] = {
> > "P35_0", "P35_1", "P35_2", "P35_3", "P35_4", "P35_5", "P35_6", "P35_7",
> > };
> >
> > +/*
> > + * Interrupts 0-15 are for INTCPUn, which are not exposed externally.
> > + * Interrupts 16-31 are for IRQn. SEI is 32.
>
> Isn't SEI 406, and DMAC0_INT0 32?
>
406 is the SPI number, but the IRQ parent of the pinctrl chip is the ICU,
not the GIC.
The ICU has the SPI interrupts inside the interrupts array in device tree,
so this index is an index into that array, as that's how translation is
done in the ICU driver. See rzt2h_icu_parse_interrupts().
SEI particularly works just fine as it was tested using the user switch.
I think the #interrupt-cells descriptions inside the
renesas,r9a09g077-icu.yaml and renesas,rzv2h-icu.yaml files need adjusting.
My bad that I didn't catch those mistakes.
> > + * This table matches the information found in User Manual's Table 17.2,
> > + * List of multiplexed pin configurations (5 of 51).
>
> 3-6 of 51
>
> However, the mapping is much easier to derive from the "Interrupt" rows
> in the various tables in Section 17.5 ("Multiplexed Pin Configurations"),
> as these match the order in rzt2h_gpio_irq_map[].
>
Do you want me to change the comment?
> > + * RZ/N2H has the same GPIO to IRQ mapping, except for the pins which
> > + * are not present.
> > + */
> > +static const u8 rzt2h_gpio_irq_map[] = {
> > + 32, 16, 17, 18, 19, 0, 20, 21,
>
> s/32/406/, but that doesn't fit in u8, and would overflow .used_irqs[],
> so you probably should translate it in the code instead.
>
See above.
> > + 22, 0, 0, 0, 0, 0, 0, 0,
> > + 23, 24, 25, 26, 27, 0, 0, 0,
> > + 0, 0, 28, 29, 30, 31, 0, 0,
> > + 0, 0, 0, 0, 0, 32, 16, 17,
> > + 18, 19, 20, 21, 22, 0, 0, 0,
> > + 0, 0, 24, 25, 26, 27, 0, 28,
> > + 29, 30, 31, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 0, 0, 24, 32, 16,
> > + 0, 0, 0, 0, 0, 0, 0, 0,
> > + 20, 23, 17, 18, 19, 0, 16, 25,
> > + 29, 20, 21, 22, 23, 0, 0, 0,
> > + 0, 0, 0, 0, 17, 0, 0, 18,
> > + 0, 0, 19, 0, 0, 20, 0, 30,
> > + 21, 0, 0, 22, 0, 0, 24, 25,
> > + 0, 0, 0, 0, 0, 16, 17, 0,
> > + 18, 0, 0, 26, 27, 0, 0, 0,
> > + 28, 29, 30, 31, 0, 0, 0, 0,
> > + 23, 31, 32, 16, 17, 18, 19, 20,
> > + 0, 0, 0, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 0, 0, 0, 0, 0,
> > + 27, 0, 0, 21, 22, 23, 24, 25,
> > + 26, 0, 0, 0, 0, 0, 0, 0,
> > + 27, 28, 29, 30, 31, 0, 0, 0,
> > + 0, 0, 0, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 0, 0, 28, 32, 16,
> > + 17, 18, 19, 0, 0, 0, 0, 20,
> > + 21, 22, 23, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 0, 24, 25, 0, 0,
> > + 0, 0, 26, 27, 0, 0, 0, 30,
> > + 0, 29, 0, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 28, 29, 30, 31, 0,
> > + 0, 0, 0, 0, 0, 0, 0, 30,
> > + 0, 0, 0, 0, 0, 0, 0, 0,
> > +};
>
> > +static int rzt2h_pinctrl_suspend_noirq(struct device *dev)
> > +{
> > + struct rzt2h_pinctrl *pctrl = dev_get_drvdata(dev);
> > +
> > + if (atomic_read(&pctrl->wakeup_path))
> > + device_set_wakeup_path(dev);
> > +
> > + return 0;
> > +}
>
> Please move this function closer to the location where it is used,
> i.e. just above rzt2h_pinctrl_pm_ops.
>
Ack.
> >+ static int rzt2h_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
> >+ unsigned int child,
> >+ unsigned int child_type,
> >+ unsigned int *parent,
> >+ unsigned int *parent_type)
> >+ {
> >+ struct rzt2h_pinctrl *pctrl = gpiochip_get_data(gc);
> >+ u8 port = RZT2H_PIN_ID_TO_PORT(child);
> >+ u8 pin = RZT2H_PIN_ID_TO_PIN(child);
> >+ u8 parent_irq;
> >+
> >+ parent_irq = rzt2h_gpio_irq_map[child];
> >+ if (parent_irq < RZT2H_INTERRUPTS_START)
>
> Or !parent_irq, cfr. the check in rzt2h_gpio_init_irq_valid_mask()?
>
This was done to make sure that the parent_irq - RZT2H_INTERRUPTS_START
operation below cannot possibly underflow. The same check makes less
sense in the context of rzt2h_gpio_init_irq_valid_mask(). With this
extra information, what would you like me to change?
> >+ return -EINVAL;
> >+
> >+ if (test_and_set_bit(parent_irq - RZT2H_INTERRUPTS_START,
> >+ pctrl->used_irqs))
> >+ return -EBUSY;
> >+
> >+ rzt2h_pinctrl_set_pfc_mode(pctrl, port, pin, PFC_FUNC_INTERRUPT);
> >+
> >+ *parent = parent_irq;
> >+ *parent_type = child_type;
> >+
> >+ return 0;
> >+ }
>
> > +
> > static int rzt2h_gpio_register(struct rzt2h_pinctrl *pctrl)
> > {
> > struct pinctrl_gpio_range *range = &pctrl->gpio_range;
> > struct gpio_chip *chip = &pctrl->gpio_chip;
> > + struct device_node *np = pctrl->dev->of_node;
> > + struct irq_domain *parent_domain;
> > struct device *dev = pctrl->dev;
> > struct of_phandle_args of_args;
> > + struct device_node *parent_np;
> > + struct gpio_irq_chip *girq;
> > int ret;
> >
> > + parent_np = of_irq_find_parent(np);
> > + if (!parent_np)
> > + return -ENXIO;
>
> Despite your claim that the interrupts properties are optional,
> I think this will abort probing when they are actually missing?
>
It won't, it will just use whatever parent interrupt controller
it can find (GIC in this case). I tested it.
interrupt-parent is implicitly allowed on any node so it's not
useful to add it to the bindings. And we cannot mark it as
required because it breaks compatibility.
But, without ICU being set as the interrupt-parent, the parent
returned here will depend on the structure of the device tree,
as interrupt-parent can be set at any level.
With the current device tree structure, it will return the GIC
if the interrupt-parent is missing on the pinctrl node.
Without #interrupt-cells, no one will be able to reference the
interrupt controller created by the pinctrl. And if
#interrupt-cells is present then surely interrupt-parent will be
there too.
I think we can leave the parent domain finding as-is but we should
guard GPIO's irqchip initialization using
if (of_property_present(np, "interrupt-parent") {...}
so that an IRQ chip with the wrong parent is not created, even if
it cannot be referenced in any way.
What do you think?
> > +
> > + parent_domain = irq_find_host(parent_np);
> > + of_node_put(parent_np);
> > + if (!parent_domain)
> > + return -EPROBE_DEFER;
> > +
> > ret = of_parse_phandle_with_fixed_args(dev->of_node, "gpio-ranges", 3, 0, &of_args);
> > if (ret)
> > return dev_err_probe(dev, ret, "Unable to parse gpio-ranges\n");
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-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
Powered by blists - more mailing lists