linux-kernel - Re: [PATCH v3 7/8] soc: renesas: Add support for Renesas RZ/N1 GPIO Interrupt Multiplexer

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250919151448.14f8719a@bootlin.com>
Date: Fri, 19 Sep 2025 15:14:48 +0200
From: Herve Codina <herve.codina@...tlin.com>
To: Wolfram Sang <wsa+renesas@...g-engineering.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Hoan Tran
 <hoan@...amperecomputing.com>, Linus Walleij <linus.walleij@...aro.org>,
 Bartosz Golaszewski <brgl@...ev.pl>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Geert Uytterhoeven <geert+renesas@...der.be>, Magnus
 Damm <magnus.damm@...il.com>, Saravana Kannan <saravanak@...gle.com>, Serge
 Semin <fancer.lancer@...il.com>, Phil Edworthy <phil.edworthy@...esas.com>,
 linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org, Pascal
 Eberhard <pascal.eberhard@...com>, Miquel Raynal
 <miquel.raynal@...tlin.com>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v3 7/8] soc: renesas: Add support for Renesas RZ/N1 GPIO
 Interrupt Multiplexer

Hi Wolfram,

On Fri, 19 Sep 2025 11:41:39 +0200
Wolfram Sang <wsa+renesas@...g-engineering.com> wrote:

> Hi Herve,
> 
> > +#define IRQMUX_MAX_IRQS 8
> > +
> > +static int irqmux_setup(struct device *dev, struct device_node *np, u32 __iomem *regs)  
> 
> The whole driver would benefit from a 'rzn1_irqmux' instead of 'irqmux'
> prefix, I'd say.

Agree, I will used the 'rzn1_irqmux' prefix.

> 
> > +	for_each_of_imap_item(&imap_parser, &imap_item) {
> > +		/*
> > +		 * The child #address-cells is 0 (already checked). The first
> > +		 * value in imap item is the src hwirq.
> > +		 *
> > +		 * imap items matches 1:1 the interrupt lines that could
> > +		 * be configured by registers (same order, same number).
> > +		 * Configure the related register with the src hwirq retrieved
> > +		 * from the interrupt-map.
> > +		 */  
> 
> I haven't looked into the above for_each_of_imap_item-helper. But
> wouldn't it be possibleto retrieve the GIC_SPI number as well and use
> the correct register based on that? That would remove the need of an
> already sorted interrupt-map.

Hum, this give the knowledge of the GIC interrupt number in the driver itself.

Not sure that the mapping between the output interrupt line number N (handled
by register index N) and the GIC interrupt number X should be hardcoded in
the driver.

In my v1 series iteration, I used the 'interrupts' property to provide this
missing information:
   interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>, /* line 0 (reg index 0) route to GIC 103 */
                <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>, /* line 1 (reg index 1) route to GIC 104 */
                <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>, /* line 2 (reg index 2) route to GIC 105 */
                ...

Base on the interrupts table and the interrupt-map, I deduce the reg index:
  - From interrupt-map, got a GIC interrupt number
  - From interrupts table and the GIC interrupt number, got the line/reg index.

Rob asked to use only interrupt-map and use directly the interrupt-map index as
the hardware index:
  https://lore.kernel.org/lkml/20250801111753.382f52ac@bootlin.com/

> 
> > +		if (index > IRQMUX_MAX_IRQS) {
> > +			of_node_put(imap_item.parent_args.np);
> > +			dev_err(dev, "too much items in interrupt-map\n");
> > +			return -EINVAL;  
> 
> -E2BIG? With such a unique errno, we could even drop the dev_err.

Yes sure.

> 
> > +		}
> > +
> > +		writel(imap_item.child_imap[0], regs + index);
> > +		index++;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int irqmux_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	u32 __iomem *regs;
> > +	int nr_irqs;
> > +	int ret;
> > +
> > +	regs = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(regs))
> > +		return PTR_ERR(regs);
> > +
> > +	nr_irqs = of_irq_count(np);
> > +	if (nr_irqs < 0)
> > +		return nr_irqs;
> > +
> > +	if (nr_irqs > IRQMUX_MAX_IRQS) {
> > +		dev_err(dev, "too many output interrupts\n");
> > +		return -ENOENT;  
> 
> -E2BIG? Wait, isn't this the same check twice?

This is not the same check but this one should not be there.

Indeed of_irq_count() counts the number of items available in the
'interrupts' property. This is not used anymore.

I missed to remove it from v1 to v2 updates (and also from v2 to v3).

The of_irq_count() call and related checks will be remove in the next
iteration.

Best regards,
Hervé