Message-ID:
 <TYRPR01MB15619BBF26CCC5EC9BB46BF148582A@TYRPR01MB15619.jpnprd01.prod.outlook.com>
Date: Fri, 9 Jan 2026 12:26:21 +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: Friday, January 9, 2026 1:38 PM
> 
> Hi Cosmin,
> 
> On Thu, 8 Jan 2026 at 19:56, Cosmin-Gabriel Tanislav
> <cosmin-gabriel.tanislav.xa@...esas.com> wrote:
> > > From: Geert Uytterhoeven <geert@...ux-m68k.org>
> > > 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>
> 
> > > > --- 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.
> 
> Thanks, I had forgotten about that.
> 
> > 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.
> 
> Np, there are never direct users of this in DT, right?
> 

I think there won't be direct users. Peripherals internal to the SoC use the
GIC directly, while external ones must go through pinctrl.

I will submit patches later to fix the description of #interrupt-cells anyway.

> > > > + * 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?
> 
> I think that would be helpful for the casual reviewer.
> 

Ack.

> > > > + * 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.
> 
> OK.
> 
> > > >+ 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?
> 
> OK, please leave it as is.
> 
> > > >  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.
> 
> OK, looks like I misread a check in of_irq_find_parent().
> 
> > 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?
> 
> Yeah, this is a bit moot. If you decide to have a check, I think
> 'of_property_present(np, "interrupt-controller")' is the right one.
> 

Okay, I will submit V3 with this modification and the adjusted comment.

> 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