Message-ID: <CAMuHMdV03D_3b_JA2vzW4tE_QbkkT1bN1dm+zLLLX24oDHMj0Q@mail.gmail.com>
Date: Tue, 21 Oct 2025 15:05:35 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: "Herve Codina (Schneider Electric)" <herve.codina@...tlin.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Wolfram Sang <wsa+renesas@...g-engineering.com>, 
	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>, 
	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 v5 7/8] soc: renesas: Add support for Renesas RZ/N1 GPIO
 Interrupt Multiplexer

Hi Hervé,

On Mon, 20 Oct 2025 at 10:08, Herve Codina (Schneider Electric)
<herve.codina@...tlin.com> wrote:
> On the Renesas RZ/N1 SoC, GPIOs can generate interruptions. Those
> interruption lines are multiplexed by the GPIO Interrupt Multiplexer in
> order to map 32 * 3 GPIO interrupt lines to 8 GIC interrupt lines.
>
> The GPIO interrupt multiplexer IP does nothing but select 8 GPIO
> IRQ lines out of the 96 available to wire them to the GIC input lines.
>
> Signed-off-by: Herve Codina (Schneider Electric) <herve.codina@...tlin.com>

Thanks for your patch!

> --- a/drivers/soc/renesas/Makefile
> +++ b/drivers/soc/renesas/Makefile
> @@ -14,4 +14,5 @@ obj-$(CONFIG_SYS_R9A09G057)   += r9a09g057-sys.o
>  # Family
>  obj-$(CONFIG_PWC_RZV2M)                += pwc-rzv2m.o
>  obj-$(CONFIG_RST_RCAR)         += rcar-rst.o
> +obj-$(CONFIG_RZN1_IRQMUX)              += rzn1_irqmux.o

One TAB too much.

> --- /dev/null
> +++ b/drivers/soc/renesas/rzn1_irqmux.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * RZ/N1 GPIO Interrupt Multiplexer
> + *
> + * Copyright 2025 Schneider Electric
> + * Author: Herve Codina <herve.codina@...tlin.com>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/build_bug.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +/*
> + * The array index is the output line index, the value at the index is the
> + * GIC SPI interrupt number the output line is connected to.
> + */
> +static const u32 rzn1_irqmux_output_lines[] = {
> +       103, 104, 105, 106, 107, 108, 109, 110
> +};

I did read the discussion with Wolfram, but the flexibility (and
overhead) provided by this array sounds a bit overkill to me.

What about just defining:

    #define RZN1_IRQMUX_SPI_BASE    103
    #define RZN1_IRQMUX_NUM_IRQS    8

?

If/when a new SoC with a similar setup ever arrives, you can probably
just replace the constants above by variables inside SoC-specific
match data.  And if the new mapping would be non-contiguous, you can
still revive this array ;-)

More about this below...

> +
> +static int rzn1_irqmux_parent_args_to_line_index(struct device *dev,
> +                                                const struct of_phandle_args *parent_args,
> +                                                const u32 output_lines[],
> +                                                unsigned int output_lines_count)
> +{
> +       unsigned int i;
> +
> +       /*
> +        * The parent interrupt should be one of the GIC controller.
> +        * Three arguments must be provided.
> +        *  - args[0]: GIC_SPI
> +        *  - args[1]: The GIC interrupt number
> +        *  - args[2]: The interrupt flags
> +        *
> +        * We retrieve the line index based on the GIC interrupt number
> +        * provided and rzn1_irqmux_output_line[] mapping array.
> +        */
> +
> +       if (parent_args->args_count != 3 ||
> +           parent_args->args[0] != GIC_SPI) {
> +               dev_err(dev, "Invalid interrupt-map item\n");
> +               return -EINVAL;
> +       }
> +
> +       for (i = 0; i < output_lines_count; i++) {
> +               if (parent_args->args[1] == output_lines[i])
> +                       return i;
> +       }

... then this loop can be replaced by two simple comparisons.

> +
> +       dev_err(dev, "Invalid GIC interrupt %u\n", parent_args->args[1]);
> +       return -EINVAL;
> +}
> +
> +static int rzn1_irqmux_setup(struct device *dev, struct device_node *np, u32 __iomem *regs)
> +{
> +       struct of_imap_parser imap_parser;
> +       struct of_imap_item imap_item;
> +       unsigned long index_done = 0;

Perhaps "DECLARE_BITMAP(index_done, RZN1_IRQMUX_NUM_IRQS)",
so the BITS_PER_LONG limit can be lifted, without any cost?

> +       int index;
> +       int ret;
> +       u32 tmp;
> +
> +       /* We support only #interrupt-cells = <1> and #address-cells = <0> */
> +       ret = of_property_read_u32(np, "#interrupt-cells", &tmp);
> +       if (ret)
> +               return ret;
> +       if (tmp != 1)
> +               return -EINVAL;
> +
> +       ret = of_property_read_u32(np, "#address-cells", &tmp);
> +       if (ret)
> +               return ret;
> +       if (tmp != 0)
> +               return -EINVAL;
> +
> +       ret = of_imap_parser_init(&imap_parser, np, &imap_item);
> +       if (ret)
> +               return ret;
> +
> +       /* 8 output lines are available */
> +       BUILD_BUG_ON(ARRAY_SIZE(rzn1_irqmux_output_lines) != 8);

... then this check can be removed, too.

> +
> +       /*
> +        * index_done is an unsigned long integer. Be sure that no buffer
> +        * overflow can occur.
> +        */
> +       BUILD_BUG_ON(ARRAY_SIZE(rzn1_irqmux_output_lines) > BITS_PER_LONG);

Currently this is less strict than the check above, so a bit useless?

> +
> +       for_each_of_imap_item(&imap_parser, &imap_item) {
> +               index = rzn1_irqmux_parent_args_to_line_index(dev,
> +                                                             &imap_item.parent_args,
> +                                                             rzn1_irqmux_output_lines,
> +                                                             ARRAY_SIZE(rzn1_irqmux_output_lines));
> +               if (index < 0) {
> +                       of_node_put(imap_item.parent_args.np);
> +                       return index;
> +               }
> +
> +               if (test_and_set_bit(index, &index_done)) {
> +                       of_node_put(imap_item.parent_args.np);
> +                       dev_err(dev, "Mux output line already defined\n");
> +                       return -EINVAL;
> +               }
> +
> +               /*
> +                * The child #address-cells is 0 (already checked). The first
> +                * value in imap item is the src hwirq.
> +                */
> +               writel(imap_item.child_imap[0], regs + index);
> +       }
> +
> +       return 0;
> +}

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