diff mbox series

[v2,4/9] irqchip: irq-renesas-rzg2l: Add support for RZ/G2UL SoC

Message ID 20221221000242.340202-5-prabhakar.mahadev-lad.rj@bp.renesas.com
State New
Headers show
Series Add IRQC support to RZ/G2UL SoC | expand

Commit Message

Lad, Prabhakar Dec. 21, 2022, 12:02 a.m. UTC 
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

The IRQC block on RZ/G2UL SoC is almost identical to one found on the
RZ/G2L SoC the only difference being it can support BUS_ERR_INT for
which it has additional registers.

This patch adds a new entry for "renesas,rzg2ul-irqc" compatible string
and now that we have interrupt-names property the driver code parses the
interrupts based on names and for backward compatibility we fallback to
parse interrupts based on index.

For now we will be using rzg2l_irqc_init() as a callback for RZ/G2UL SoC
too and in future when the interrupt handler will be registered for
BUS_ERR_INT we will have to implement a new callback.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1 -> v2
* New patch
---
 drivers/irqchip/irq-renesas-rzg2l.c | 80 ++++++++++++++++++++++++++---
 1 file changed, 74 insertions(+), 6 deletions(-)

Comments

Marc Zyngier Dec. 21, 2022, 10:20 a.m. UTC | #1 
On Wed, 21 Dec 2022 00:02:37 +0000,
Prabhakar <prabhakar.csengg@gmail.com> wrote:
> 
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> The IRQC block on RZ/G2UL SoC is almost identical to one found on the
> RZ/G2L SoC the only difference being it can support BUS_ERR_INT for
> which it has additional registers.
> 
> This patch adds a new entry for "renesas,rzg2ul-irqc" compatible string
> and now that we have interrupt-names property the driver code parses the
> interrupts based on names and for backward compatibility we fallback to
> parse interrupts based on index.
> 
> For now we will be using rzg2l_irqc_init() as a callback for RZ/G2UL SoC
> too and in future when the interrupt handler will be registered for
> BUS_ERR_INT we will have to implement a new callback.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Since you're posting from a different address, please add a second SoB
with your gmail address.

> ---
> v1 -> v2
> * New patch
> ---
>  drivers/irqchip/irq-renesas-rzg2l.c | 80 ++++++++++++++++++++++++++---
>  1 file changed, 74 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
> index 7918fe201218..5bdf0106ef51 100644
> --- a/drivers/irqchip/irq-renesas-rzg2l.c
> +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> @@ -299,19 +299,86 @@ static const struct irq_domain_ops rzg2l_irqc_domain_ops = {
>  	.translate = irq_domain_translate_twocell,
>  };
>  
> -static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
> -				       struct device_node *np)
> +static int rzg2l_irqc_parse_interrupt_to_fwspec(struct rzg2l_irqc_priv *priv,
> +						struct device_node *np,
> +						unsigned int index,
> +						unsigned int fwspec_index)
>  {
>  	struct of_phandle_args map;
> +	int ret;
> +
> +	ret = of_irq_parse_one(np, index, &map);
> +	if (ret)
> +		return ret;
> +
> +	of_phandle_args_to_fwspec(np, map.args, map.args_count,
> +				  &priv->fwspec[fwspec_index]);
> +
> +	return 0;
> +}
> +
> +static int rzg2l_irqc_parse_interrupt_by_name_to_fwspec(struct rzg2l_irqc_priv *priv,
> +							struct device_node *np,
> +							char *irq_name,
> +							unsigned int fwspec_index)
> +{
> +	int index;
> +
> +	index = of_property_match_string(np, "interrupt-names", irq_name);
> +	if (index < 0)
> +		return index;
> +
> +	return rzg2l_irqc_parse_interrupt_to_fwspec(priv, np, index, fwspec_index);
> +}
> +
> +/* Parse hierarchy domain interrupts ie only IRQ0-7 and TINT0-31 */
> +static int rzg2l_irqc_parse_hierarchy_interrupts(struct rzg2l_irqc_priv *priv,
> +						 struct device_node *np)
> +{
> +	struct property *pp;
>  	unsigned int i;
>  	int ret;
>  
> +	/*
> +	 * first check if interrupt-names property exists if so parse them by name
> +	 * or else parse them by index for backward compatibility.
> +	 */
> +	pp = of_find_property(np, "interrupt-names", NULL);
> +	if (pp) {
> +		char *irq_name;
> +
> +		/* parse IRQ0-7 */
> +		for (i = 0; i < IRQC_IRQ_COUNT; i++) {
> +			irq_name = kasprintf(GFP_KERNEL, "irq%d", i);
> +			if (!irq_name)
> +				return -ENOMEM;
> +
> +			ret = rzg2l_irqc_parse_interrupt_by_name_to_fwspec(priv, np, irq_name, i);

Am I the only one that find it rather odd to construct a name from an
index, only to get another index back?

In any case, the string stuff could be moved into
rzg2l_irqc_parse_interrupt_by_name_to_fwspec(). Which could really do
with a name shortening)... rzg2l_irqc_name_to_fwspec? Same thing for
the other function (rzg2l_irqc_index_to_fwspec).

	M.
Geert Uytterhoeven Dec. 21, 2022, 12:18 p.m. UTC | #2 
On Wed, Dec 21, 2022 at 11:20 AM Marc Zyngier <maz@kernel.org> wrote:
> On Wed, 21 Dec 2022 00:02:37 +0000,
> Prabhakar <prabhakar.csengg@gmail.com> wrote:
> >
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > The IRQC block on RZ/G2UL SoC is almost identical to one found on the
> > RZ/G2L SoC the only difference being it can support BUS_ERR_INT for
> > which it has additional registers.
> >
> > This patch adds a new entry for "renesas,rzg2ul-irqc" compatible string
> > and now that we have interrupt-names property the driver code parses the
> > interrupts based on names and for backward compatibility we fallback to
> > parse interrupts based on index.
> >
> > For now we will be using rzg2l_irqc_init() as a callback for RZ/G2UL SoC
> > too and in future when the interrupt handler will be registered for
> > BUS_ERR_INT we will have to implement a new callback.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> > +/* Parse hierarchy domain interrupts ie only IRQ0-7 and TINT0-31 */
> > +static int rzg2l_irqc_parse_hierarchy_interrupts(struct rzg2l_irqc_priv *priv,
> > +                                              struct device_node *np)
> > +{
> > +     struct property *pp;
> >       unsigned int i;
> >       int ret;
> >
> > +     /*
> > +      * first check if interrupt-names property exists if so parse them by name
> > +      * or else parse them by index for backward compatibility.
> > +      */
> > +     pp = of_find_property(np, "interrupt-names", NULL);
> > +     if (pp) {
> > +             char *irq_name;
> > +
> > +             /* parse IRQ0-7 */
> > +             for (i = 0; i < IRQC_IRQ_COUNT; i++) {
> > +                     irq_name = kasprintf(GFP_KERNEL, "irq%d", i);

%u

> > +                     if (!irq_name)
> > +                             return -ENOMEM;
> > +
> > +                     ret = rzg2l_irqc_parse_interrupt_by_name_to_fwspec(priv, np, irq_name, i);
>
> Am I the only one that find it rather odd to construct a name from an
> index, only to get another index back?

The issue is that there are two number ranges ("irq%u" and "tint%u"),
stored in a single interrupts property.

An alternative solution would be to get rid of the "interrupt-names",
and use two separate prefixed interrupts properties instead, like is
common for e.g. gpios: "irq-interrupts" and "tint-interrupts".

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
Lad, Prabhakar Dec. 22, 2022, 11:49 a.m. UTC | #3 
Hi Geert,

On Wed, Dec 21, 2022 at 12:18 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> On Wed, Dec 21, 2022 at 11:20 AM Marc Zyngier <maz@kernel.org> wrote:
> > On Wed, 21 Dec 2022 00:02:37 +0000,
> > Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > >
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > The IRQC block on RZ/G2UL SoC is almost identical to one found on the
> > > RZ/G2L SoC the only difference being it can support BUS_ERR_INT for
> > > which it has additional registers.
> > >
> > > This patch adds a new entry for "renesas,rzg2ul-irqc" compatible string
> > > and now that we have interrupt-names property the driver code parses the
> > > interrupts based on names and for backward compatibility we fallback to
> > > parse interrupts based on index.
> > >
> > > For now we will be using rzg2l_irqc_init() as a callback for RZ/G2UL SoC
> > > too and in future when the interrupt handler will be registered for
> > > BUS_ERR_INT we will have to implement a new callback.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> > > +/* Parse hierarchy domain interrupts ie only IRQ0-7 and TINT0-31 */
> > > +static int rzg2l_irqc_parse_hierarchy_interrupts(struct rzg2l_irqc_priv *priv,
> > > +                                              struct device_node *np)
> > > +{
> > > +     struct property *pp;
> > >       unsigned int i;
> > >       int ret;
> > >
> > > +     /*
> > > +      * first check if interrupt-names property exists if so parse them by name
> > > +      * or else parse them by index for backward compatibility.
> > > +      */
> > > +     pp = of_find_property(np, "interrupt-names", NULL);
> > > +     if (pp) {
> > > +             char *irq_name;
> > > +
> > > +             /* parse IRQ0-7 */
> > > +             for (i = 0; i < IRQC_IRQ_COUNT; i++) {
> > > +                     irq_name = kasprintf(GFP_KERNEL, "irq%d", i);
>
> %u
>
Ok.

> > > +                     if (!irq_name)
> > > +                             return -ENOMEM;
> > > +
> > > +                     ret = rzg2l_irqc_parse_interrupt_by_name_to_fwspec(priv, np, irq_name, i);
> >
> > Am I the only one that find it rather odd to construct a name from an
> > index, only to get another index back?
>
> The issue is that there are two number ranges ("irq%u" and "tint%u"),
> stored in a single interrupts property.
>
> An alternative solution would be to get rid of the "interrupt-names",
> and use two separate prefixed interrupts properties instead, like is
> common for e.g. gpios: "irq-interrupts" and "tint-interrupts".
>
Maybe I will read all the interrupts based on index only for all the
SoCs and we still add interrupt-names in dt bindings with the
dt_binding check we can make sure all the interrupts for each SoC
exist in the DT and the driver still reads them based on index. Does
that sound good?

Cheers,
Prabhakar
Geert Uytterhoeven Dec. 22, 2022, 12:51 p.m. UTC | #4 
Hi Prabhakar,

On Thu, Dec 22, 2022 at 12:50 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Wed, Dec 21, 2022 at 12:18 PM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Wed, Dec 21, 2022 at 11:20 AM Marc Zyngier <maz@kernel.org> wrote:
> > > On Wed, 21 Dec 2022 00:02:37 +0000,
> > > Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > >
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > The IRQC block on RZ/G2UL SoC is almost identical to one found on the
> > > > RZ/G2L SoC the only difference being it can support BUS_ERR_INT for
> > > > which it has additional registers.
> > > >
> > > > This patch adds a new entry for "renesas,rzg2ul-irqc" compatible string
> > > > and now that we have interrupt-names property the driver code parses the
> > > > interrupts based on names and for backward compatibility we fallback to
> > > > parse interrupts based on index.
> > > >
> > > > For now we will be using rzg2l_irqc_init() as a callback for RZ/G2UL SoC
> > > > too and in future when the interrupt handler will be registered for
> > > > BUS_ERR_INT we will have to implement a new callback.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > > > +/* Parse hierarchy domain interrupts ie only IRQ0-7 and TINT0-31 */
> > > > +static int rzg2l_irqc_parse_hierarchy_interrupts(struct rzg2l_irqc_priv *priv,
> > > > +                                              struct device_node *np)
> > > > +{
> > > > +     struct property *pp;
> > > >       unsigned int i;
> > > >       int ret;
> > > >
> > > > +     /*
> > > > +      * first check if interrupt-names property exists if so parse them by name
> > > > +      * or else parse them by index for backward compatibility.
> > > > +      */
> > > > +     pp = of_find_property(np, "interrupt-names", NULL);
> > > > +     if (pp) {
> > > > +             char *irq_name;
> > > > +
> > > > +             /* parse IRQ0-7 */
> > > > +             for (i = 0; i < IRQC_IRQ_COUNT; i++) {
> > > > +                     irq_name = kasprintf(GFP_KERNEL, "irq%d", i);
> >
> > %u
> >
> Ok.
>
> > > > +                     if (!irq_name)
> > > > +                             return -ENOMEM;
> > > > +
> > > > +                     ret = rzg2l_irqc_parse_interrupt_by_name_to_fwspec(priv, np, irq_name, i);
> > >
> > > Am I the only one that find it rather odd to construct a name from an
> > > index, only to get another index back?
> >
> > The issue is that there are two number ranges ("irq%u" and "tint%u"),
> > stored in a single interrupts property.
> >
> > An alternative solution would be to get rid of the "interrupt-names",
> > and use two separate prefixed interrupts properties instead, like is
> > common for e.g. gpios: "irq-interrupts" and "tint-interrupts".
> >
> Maybe I will read all the interrupts based on index only for all the
> SoCs and we still add interrupt-names in dt bindings with the
> dt_binding check we can make sure all the interrupts for each SoC
> exist in the DT and the driver still reads them based on index. Does
> that sound good?

Sure, sounds fine.

You can postpone parsing interrupt-names in the driver (until a new
SoC arrives that uses a different number of IRQ or TINT interrupts).

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
diff mbox series

Patch

diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index 7918fe201218..5bdf0106ef51 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -299,19 +299,86 @@  static const struct irq_domain_ops rzg2l_irqc_domain_ops = {
 	.translate = irq_domain_translate_twocell,
 };
 
-static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
-				       struct device_node *np)
+static int rzg2l_irqc_parse_interrupt_to_fwspec(struct rzg2l_irqc_priv *priv,
+						struct device_node *np,
+						unsigned int index,
+						unsigned int fwspec_index)
 {
 	struct of_phandle_args map;
+	int ret;
+
+	ret = of_irq_parse_one(np, index, &map);
+	if (ret)
+		return ret;
+
+	of_phandle_args_to_fwspec(np, map.args, map.args_count,
+				  &priv->fwspec[fwspec_index]);
+
+	return 0;
+}
+
+static int rzg2l_irqc_parse_interrupt_by_name_to_fwspec(struct rzg2l_irqc_priv *priv,
+							struct device_node *np,
+							char *irq_name,
+							unsigned int fwspec_index)
+{
+	int index;
+
+	index = of_property_match_string(np, "interrupt-names", irq_name);
+	if (index < 0)
+		return index;
+
+	return rzg2l_irqc_parse_interrupt_to_fwspec(priv, np, index, fwspec_index);
+}
+
+/* Parse hierarchy domain interrupts ie only IRQ0-7 and TINT0-31 */
+static int rzg2l_irqc_parse_hierarchy_interrupts(struct rzg2l_irqc_priv *priv,
+						 struct device_node *np)
+{
+	struct property *pp;
 	unsigned int i;
 	int ret;
 
+	/*
+	 * first check if interrupt-names property exists if so parse them by name
+	 * or else parse them by index for backward compatibility.
+	 */
+	pp = of_find_property(np, "interrupt-names", NULL);
+	if (pp) {
+		char *irq_name;
+
+		/* parse IRQ0-7 */
+		for (i = 0; i < IRQC_IRQ_COUNT; i++) {
+			irq_name = kasprintf(GFP_KERNEL, "irq%d", i);
+			if (!irq_name)
+				return -ENOMEM;
+
+			ret = rzg2l_irqc_parse_interrupt_by_name_to_fwspec(priv, np, irq_name, i);
+			kfree(irq_name);
+			if (ret)
+				return ret;
+		}
+
+		/* parse TINT0-31 */
+		for (i = 0; i < IRQC_TINT_COUNT; i++) {
+			irq_name = kasprintf(GFP_KERNEL, "tint%d", i);
+			if (!irq_name)
+				return -ENOMEM;
+
+			ret = rzg2l_irqc_parse_interrupt_by_name_to_fwspec(priv, np, irq_name,
+									   i + IRQC_IRQ_COUNT);
+			kfree(irq_name);
+			if (ret)
+				return ret;
+		}
+
+		return 0;
+	}
+
 	for (i = 1; i <= IRQC_NUM_HIERARCHY_IRQ; i++) {
-		ret = of_irq_parse_one(np, i, &map);
+		ret = rzg2l_irqc_parse_interrupt_to_fwspec(priv, np, i, i - 1);
 		if (ret)
 			return ret;
-		of_phandle_args_to_fwspec(np, map.args, map.args_count,
-					  &priv->fwspec[i - 1]);
 	}
 
 	return 0;
@@ -343,7 +410,7 @@  static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
 	if (IS_ERR(priv->base))
 		return PTR_ERR(priv->base);
 
-	ret = rzg2l_irqc_parse_interrupts(priv, node);
+	ret = rzg2l_irqc_parse_hierarchy_interrupts(priv, node);
 	if (ret) {
 		dev_err(&pdev->dev, "cannot parse interrupts: %d\n", ret);
 		return ret;
@@ -389,6 +456,7 @@  static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
 
 IRQCHIP_PLATFORM_DRIVER_BEGIN(rzg2l_irqc)
 IRQCHIP_MATCH("renesas,rzg2l-irqc", rzg2l_irqc_init)
+IRQCHIP_MATCH("renesas,rzg2ul-irqc", rzg2l_irqc_init)
 IRQCHIP_PLATFORM_DRIVER_END(rzg2l_irqc)
 MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>");
 MODULE_DESCRIPTION("Renesas RZ/G2L IRQC Driver");