Backporting Renesas IA55 IRQC driver

Backporting Renesas IA55 IRQC driver

[Claudiu Beznea]

Hi, CIP maintainers,

I'm trying to backport the IA55 IRQC driver [1] to v5.10.209-cip44.

The controller is connected to GIC and pin controller as follows:

                                          ┌──────────┐          ┌──────────┐
                                          │          │ SPIX     │          │
                                          │          ├─────────►│          │
                                          │          │          │          │
                                          │          │          │          │
                  ┌────────┐IRQ0-7        │  IA55    │          │  GIC     │
     Pin0 ───────►│        ├─────────────►│          │          │          │
                  │        │              │          │ PPIY     │          │
     ...          │  GPIO  │              │          ├─────────►│          │
                  │        │GPIOINT0-127  │          │          │          │
     PinN ───────►│        ├─────────────►│          │          │          │
                  └────────┘              └──────────┘          └──────────┘

While working on mainline driver, my colleague, Prabhakar, found that there
are issues with this kind of hierarchy due to
platform_get_resource(pdev, IORESOURCE_IRQ, ...) which relies on static
allocation of IRQ resources in DT core code causing issues
when using hierarchical interrupt domains with "interrupts" property
in the node as this bypasses the hierarchical setup and messes up the
irq chaining.

As a result he run a cleanup campaign in mainline kernel and changed
drivers from using platform_get_resource(pdev, IORESOURCE_IRQ, ...)
to platform_get_irq(pdev, ...) (see list here [2]) and at the submitting [3].

Now we want to backport the IA55 driver to v5.10.209-cip44 (in the first instance).

At the moment we have 2 working approaches:
1/ backport all the work that Prabhakar did (I counted ~80 commits)
2/ change the mainline IA55 driver to not use the interrupts bindings
   to describe the interrupts (see [4] for the mainline approach)
   but another DT property (we have a working version that uses
   interrupt-map and interrupt-map-mask) and adapt the
   rzg2l_irqc_parse_interrupts() to do the proper mapping

The issue we see with 1 is that it affects multiple drivers but not only Renesas ones.
The issue we see with 2 is that the DT bindings are changed and we don't know
if we should keep the DTB as ABI: should a CIP DTB be able to work with a mainline
kernel? Should a mainline DTB be able to work with a CIP kernel?

Could you please share your knowledge on this?
What solution would you prefer?

Thank you,
Claudiu Beznea

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-renesas-rzg2l.c
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=Use+platform_get_irq%28%29+to+get+the+interrupt
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/of/platform.c?id=a1a2b7125e1079cfcc13a116aa3af3df2f9e002b
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml#n196
________________________________

Renesas Electronics Europe GmbH
Registered Office: Arcadiastrasse 10
DE-40472 Duesseldorf
Commercial Registry: Duesseldorf, HRB 3708
Managing Director: Carsten Jauch
VAT-No.: DE 14978647
Tax-ID-No: 105/5839/1793

Legal Disclaimer: This e-mail communication (and any attachment/s) is confidential and contains proprietary information, some or all of which may be legally privileged. It is intended solely for the use of the individual or entity to which it is addressed. Access to this email by anyone else is unauthorized. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited and may be unlawful.

Re: Backporting Renesas IA55 IRQC driver

[Jan Kiszka]

On 15.03.24 16:18, Claudiu Beznea wrote:
> Hi, CIP maintainers,
> 
> I'm trying to backport the IA55 IRQC driver [1] to v5.10.209-cip44.
> 
> The controller is connected to GIC and pin controller as follows:
> 
>                                           ┌──────────┐          ┌──────────┐
>                                           │          │ SPIX     │          │
>                                           │          ├─────────►│          │
>                                           │          │          │          │
>                                           │          │          │          │
>                   ┌────────┐IRQ0-7        │  IA55    │          │  GIC     │
>      Pin0 ───────►│        ├─────────────►│          │          │          │
>                   │        │              │          │ PPIY     │          │
>      ...          │  GPIO  │              │          ├─────────►│          │
>                   │        │GPIOINT0-127  │          │          │          │
>      PinN ───────►│        ├─────────────►│          │          │          │
>                   └────────┘              └──────────┘          └──────────┘
> 
> While working on mainline driver, my colleague, Prabhakar, found that there
> are issues with this kind of hierarchy due to
> platform_get_resource(pdev, IORESOURCE_IRQ, ...) which relies on static
> allocation of IRQ resources in DT core code causing issues
> when using hierarchical interrupt domains with "interrupts" property
> in the node as this bypasses the hierarchical setup and messes up the
> irq chaining.
> 
> As a result he run a cleanup campaign in mainline kernel and changed
> drivers from using platform_get_resource(pdev, IORESOURCE_IRQ, ...)
> to platform_get_irq(pdev, ...) (see list here [2]) and at the submitting [3].
> 
> Now we want to backport the IA55 driver to v5.10.209-cip44 (in the first instance).
> 
> At the moment we have 2 working approaches:
> 1/ backport all the work that Prabhakar did (I counted ~80 commits)
> 2/ change the mainline IA55 driver to not use the interrupts bindings
>    to describe the interrupts (see [4] for the mainline approach)
>    but another DT property (we have a working version that uses
>    interrupt-map and interrupt-map-mask) and adapt the
>    rzg2l_irqc_parse_interrupts() to do the proper mapping
> 
> The issue we see with 1 is that it affects multiple drivers but not only Renesas ones.

Right, and that will raise the question how to test all the others (a
rhetorical question).

> The issue we see with 2 is that the DT bindings are changed and we don't know
> if we should keep the DTB as ABI: should a CIP DTB be able to work with a mainline
> kernel? Should a mainline DTB be able to work with a CIP kernel?

Upstream DTB means upstream approved bindings. CIP is upstream in this
regard as well. It's already a major PITA when moving from permature
vendor bindings to the finally approved ones in upstream as it may
involve both firmware and kernel.

> 
> Could you please share your knowledge on this?
> What solution would you prefer?

I would prefer none of them right now, but I'm neither the final
authority nor do I have a much better idea yet. Well... maybe there is a
way to provide both existing semantics of 5.10 for the existing drivers
and the new own for the backported one, but then under a differently
named interface (of_device_alloc_nextgen)?

Jan

-- 
Siemens AG, Technology
Linux Expert Center

Re: Backporting Renesas IA55 IRQC driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3714 bytes --]

Hi!

> I'm trying to backport the IA55 IRQC driver [1] to v5.10.209-cip44.
> 
> The controller is connected to GIC and pin controller as follows:
> 
>                                           ┌──────────┐          ┌──────────┐
>                                           │          │ SPIX     │          │
>                                           │          ├─────────►│          │
>                                           │          │          │          │
>                                           │          │          │          │
>                   ┌────────┐IRQ0-7        │  IA55    │          │  GIC     │
>      Pin0 ───────►│        ├─────────────►│          │          │          │
>                   │        │              │          │ PPIY     │          │
>      ...          │  GPIO  │              │          ├─────────►│          │
>                   │        │GPIOINT0-127  │          │          │          │
>      PinN ───────►│        ├─────────────►│          │          │          │
>                   └────────┘              └──────────┘          └──────────┘
> 
> While working on mainline driver, my colleague, Prabhakar, found that there
> are issues with this kind of hierarchy due to
> platform_get_resource(pdev, IORESOURCE_IRQ, ...) which relies on static
> allocation of IRQ resources in DT core code causing issues
> when using hierarchical interrupt domains with "interrupts" property
> in the node as this bypasses the hierarchical setup and messes up the
> irq chaining.
> 
> As a result he run a cleanup campaign in mainline kernel and changed
> drivers from using platform_get_resource(pdev, IORESOURCE_IRQ, ...)
> to platform_get_irq(pdev, ...) (see list here [2]) and at the submitting [3].

Ok.

> Now we want to backport the IA55 driver to v5.10.209-cip44 (in the first instance).
> 
> At the moment we have 2 working approaches:
> 1/ backport all the work that Prabhakar did (I counted ~80 commits)

No.

> 2/ change the mainline IA55 driver to not use the interrupts bindings
>    to describe the interrupts (see [4] for the mainline approach)
>    but another DT property (we have a working version that uses
>    interrupt-map and interrupt-map-mask) and adapt the
>    rzg2l_irqc_parse_interrupts() to do the proper mapping

We'd rather not.

So... you are converting 80 drivers to newer/better/cleaner API, but
surely there's a way not to do the cleanup and only convert "your"
drivers affected by this?

It may mean passing flag down the callchain saying "hey, I'm new
user", but I believe this should be explored before going with 1/ or
2/.

Please avoid these:

> Legal Disclaimer: This e-mail communication (and any attachment/s) is confidential and contains proprietary information, some or all of which may be legally privileged. It is intended solely for the use of the individual or entity to which it is addressed. Access to this email by anyone else is unauthorized. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited and may be unlawful.

Best regards,
								Pavel
-- 
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Backporting Renesas IA55 IRQC driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 4334 bytes --]

Hi!

> I'm trying to backport the IA55 IRQC driver [1] to v5.10.209-cip44.
> 
> The controller is connected to GIC and pin controller as follows:
> 
>                                           ┌──────────┐          ┌──────────┐
>                                           │          │ SPIX     │          │
>                                           │          ├─────────►│          │
>                                           │          │          │          │
>                                           │          │          │          │
>                   ┌────────┐IRQ0-7        │  IA55    │          │  GIC     │
>      Pin0 ───────►│        ├─────────────►│          │          │          │
>                   │        │              │          │ PPIY     │          │
>      ...          │  GPIO  │              │          ├─────────►│          │
>                   │        │GPIOINT0-127  │          │          │          │
>      PinN ───────►│        ├─────────────►│          │          │          │
>                   └────────┘              └──────────┘          └──────────┘
> 
> While working on mainline driver, my colleague, Prabhakar, found that there
> are issues with this kind of hierarchy due to
> platform_get_resource(pdev, IORESOURCE_IRQ, ...) which relies on static
> allocation of IRQ resources in DT core code causing issues
> when using hierarchical interrupt domains with "interrupts" property
> in the node as this bypasses the hierarchical setup and messes up the
> irq chaining.
> 
> As a result he run a cleanup campaign in mainline kernel and changed
> drivers from using platform_get_resource(pdev, IORESOURCE_IRQ, ...)
> to platform_get_irq(pdev, ...) (see list here [2]) and at the submitting [3].
> 
> Now we want to backport the IA55 driver to v5.10.209-cip44 (in the first instance).
> 
> At the moment we have 2 working approaches:
> 1/ backport all the work that Prabhakar did (I counted ~80 commits)
> 2/ change the mainline IA55 driver to not use the interrupts bindings
>    to describe the interrupts (see [4] for the mainline approach)
>    but another DT property (we have a working version that uses
>    interrupt-map and interrupt-map-mask) and adapt the
>    rzg2l_irqc_parse_interrupts() to do the proper mapping

So I'm suggesting 3/:

Replace a1a2b7125e1079cfcc13a116aa3af3df2f9e002b with something like

and then arrange new_style_board to be set on machines you converted
but nowhere else. Best would be passing that information down the
callchain somehow, but you use have global variable or even config
option...

Best regards,
								Pavel

+++ b/drivers/of/platform.c
@@ -114,35 +114,31 @@ struct platform_device *of_device_alloc(struct device_node *np,
 				  struct device *parent)
 {
 	struct platform_device *dev;
	int rc, i, num_reg = 0, num_irq;
 	struct resource *res, temp_res;
 
 	dev = platform_device_alloc("", PLATFORM_DEVID_NONE);
 	if (!dev)
 		return NULL;
 
	/* count the io and irq resources */
 	while (of_address_to_resource(np, num_reg, &temp_res) == 0)
 		num_reg++;
	num_irq = of_irq_count(np);
	if (new_style_board)
	   num_irq = 0;
 
 	/* Populate the resource table */
	if (num_irq || num_reg) {
		res = kcalloc(num_irq + num_reg, sizeof(*res), GFP_KERNEL);
 		if (!res) {
 			platform_device_put(dev);
 			return NULL;
 		}
 
		dev->num_resources = num_reg + num_irq;
 		dev->resource = res;
 		for (i = 0; i < num_reg; i++, res++) {
 			rc = of_address_to_resource(np, i, res);
 			WARN_ON(rc);
 		}
+		if (num_irq)		
		    if (of_irq_to_resource_table(np, res, num_irq) != num_irq)
			pr_debug("not all legacy IRQ resources mapped for %pOFn\n",
				 np);
 	}
 
 	dev->dev.of_node = of_node_get(np);

Best regards,
								Pavel
-- 
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Backporting Renesas IA55 IRQC driver

[Claudiu Beznea]

Hi, Pavel, all,

Thank you for your input!

> -----Original Message-----
> From: Pavel Machek <pavel@denx.de>
> Sent: Monday, March 18, 2024 2:12 PM
> To: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> Cc: Pavel Machek <pavel@denx.de>; Nobuhiro Iwamatsu
> <nobuhiro1.iwamatsu@toshiba.co.jp>; cip-dev@lists.cip-project.org;
> Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>; Biju Das
> <biju.das.jz@bp.renesas.com>
> Subject: Re: Backporting Renesas IA55 IRQC driver
>
> Hi!
>
> > I'm trying to backport the IA55 IRQC driver [1] to v5.10.209-cip44.
> >
> > The controller is connected to GIC and pin controller as follows:
> >
> >                                           ┌──────────┐
> ┌──────────┐
> >                                           │          │ SPIX     │
> │
> >                                           │          ├─────────►│
> │
> >                                           │          │          │
> │
> >                                           │          │          │
> │
> >                   ┌────────┐IRQ0-7        │  IA55    │          │  GIC
> │
> >      Pin0 ───────►│        ├─────────────►│          │          │
> │
> >                   │        │              │          │ PPIY     │
> │
> >      ...          │  GPIO  │              │          ├─────────►│
> │
> >                   │        │GPIOINT0-127  │          │          │
> │
> >      PinN ───────►│        ├─────────────►│          │          │
> │
> >                   └────────┘              └──────────┘
> └──────────┘
> >
> > While working on mainline driver, my colleague, Prabhakar, found that
> > there are issues with this kind of hierarchy due to
> > platform_get_resource(pdev, IORESOURCE_IRQ, ...) which relies on
> > static allocation of IRQ resources in DT core code causing issues when
> > using hierarchical interrupt domains with "interrupts" property in the
> > node as this bypasses the hierarchical setup and messes up the irq
> > chaining.
> >
> > As a result he run a cleanup campaign in mainline kernel and changed
> > drivers from using platform_get_resource(pdev, IORESOURCE_IRQ, ...) to
> > platform_get_irq(pdev, ...) (see list here [2]) and at the submitting
> [3].
> >
> > Now we want to backport the IA55 driver to v5.10.209-cip44 (in the first
> instance).
> >
> > At the moment we have 2 working approaches:
> > 1/ backport all the work that Prabhakar did (I counted ~80 commits) 2/
> > change the mainline IA55 driver to not use the interrupts bindings
> >    to describe the interrupts (see [4] for the mainline approach)
> >    but another DT property (we have a working version that uses
> >    interrupt-map and interrupt-map-mask) and adapt the
> >    rzg2l_irqc_parse_interrupts() to do the proper mapping
>
> So I'm suggesting 3/:
>
> Replace a1a2b7125e1079cfcc13a116aa3af3df2f9e002b with something like
>

Ok!

> and then arrange new_style_board to be set on machines you converted but
> nowhere else. Best would be passing that information down the callchain
> somehow, but you use have global variable or even config option...

IIUC the proposal here: the of_device_alloc() is called in the
initialization phase, this being the call stack:

[    0.057410] Call trace:
[    0.057438]  dump_backtrace+0x0/0x1c0
[    0.057453]  show_stack+0x18/0x40
[    0.057471]  dump_stack+0xe8/0x124
[    0.057489]  of_device_alloc+0x54/0x1dc
[    0.057501]  of_platform_device_create_pdata.part.0+0x24/0xc4
[    0.057520]  of_platform_device_create_pdata+0x74/0xa0
[    0.057533]  of_platform_bus_create+0x180/0x3e0
[    0.057545]  of_platform_populate+0x58/0xfc
[    0.057563]  of_platform_default_populate_init+0xb8/0xdc
[    0.057576]  do_one_initcall+0x50/0x1c0
[    0.057596]  kernel_init_freeable+0x230/0x298
[    0.057609]  kernel_init+0x14/0x118
[    0.057620]  ret_from_fork+0x10/0x38

while the IRQ driver is probed and used later, so, I cannot pass
information from driver to of_device_alloc().

Based on your proposal, what about the following diff (or something
similar)? Would it be acceptable for CIP:

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 43748c6480c8..ddbd46043d46 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -135,7 +135,8 @@ struct platform_device *of_device_alloc(struct device_node *np,
                        rc = of_address_to_resource(np, i, res);
                        WARN_ON(rc);
                }
-               if (of_irq_to_resource_table(np, res, num_irq) != num_irq)
+               if (!of_device_is_compatible(np, "renesas,rzg2l-irqc") &&
+                   of_irq_to_resource_table(np, res, num_irq) != num_irq)
                        pr_debug("not all legacy IRQ resources mapped for %pOFn\n",
                                 np);
        }

where "renesas,rzg2l-irqc" is the compatible for IA55 IRQ driver.

Thank you,
Claudiu Beznea

>
> Best regards,
>                                                               Pavel
>
> +++ b/drivers/of/platform.c
> @@ -114,35 +114,31 @@ struct platform_device *of_device_alloc(struct
> device_node *np,
>                                 struct device *parent)
>  {
>       struct platform_device *dev;
>       int rc, i, num_reg = 0, num_irq;
>       struct resource *res, temp_res;
>
>       dev = platform_device_alloc("", PLATFORM_DEVID_NONE);
>       if (!dev)
>               return NULL;
>
>       /* count the io and irq resources */
>       while (of_address_to_resource(np, num_reg, &temp_res) == 0)
>               num_reg++;
>       num_irq = of_irq_count(np);
>       if (new_style_board)
>          num_irq = 0;
>
>       /* Populate the resource table */
>       if (num_irq || num_reg) {
>               res = kcalloc(num_irq + num_reg, sizeof(*res), GFP_KERNEL);
>               if (!res) {
>                       platform_device_put(dev);
>                       return NULL;
>               }
>
>               dev->num_resources = num_reg + num_irq;
>               dev->resource = res;
>               for (i = 0; i < num_reg; i++, res++) {
>                       rc = of_address_to_resource(np, i, res);
>                       WARN_ON(rc);
>               }
> +             if (num_irq)
>                   if (of_irq_to_resource_table(np, res, num_irq) != num_irq)
>                       pr_debug("not all legacy IRQ resources mapped for
> %pOFn\n",
>                                np);
>       }
>
>       dev->dev.of_node = of_node_get(np);
>
> Best regards,
>                                                               Pavel
> --
> DENX Software Engineering GmbH,        Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
________________________________

Renesas Electronics Europe GmbH
Registered Office: Arcadiastrasse 10
DE-40472 Duesseldorf
Commercial Registry: Duesseldorf, HRB 3708
Managing Director: Carsten Jauch
VAT-No.: DE 14978647
Tax-ID-No: 105/5839/1793

Legal Disclaimer: This e-mail communication (and any attachment/s) is confidential and contains proprietary information, some or all of which may be legally privileged. It is intended solely for the use of the individual or entity to which it is addressed. Access to this email by anyone else is unauthorized. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited and may be unlawful.

Re: Backporting Renesas IA55 IRQC driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2498 bytes --]

Hi!

> > and then arrange new_style_board to be set on machines you converted but
> > nowhere else. Best would be passing that information down the callchain
> > somehow, but you use have global variable or even config option...
> 
> IIUC the proposal here: the of_device_alloc() is called in the
> initialization phase, this being the call stack:
> 
> [    0.057410] Call trace:
> [    0.057438]  dump_backtrace+0x0/0x1c0
> [    0.057453]  show_stack+0x18/0x40
> [    0.057471]  dump_stack+0xe8/0x124
> [    0.057489]  of_device_alloc+0x54/0x1dc
> [    0.057501]  of_platform_device_create_pdata.part.0+0x24/0xc4
> [    0.057520]  of_platform_device_create_pdata+0x74/0xa0
> [    0.057533]  of_platform_bus_create+0x180/0x3e0
> [    0.057545]  of_platform_populate+0x58/0xfc
> [    0.057563]  of_platform_default_populate_init+0xb8/0xdc
> [    0.057576]  do_one_initcall+0x50/0x1c0
> [    0.057596]  kernel_init_freeable+0x230/0x298
> [    0.057609]  kernel_init+0x14/0x118
> [    0.057620]  ret_from_fork+0x10/0x38
> 
> while the IRQ driver is probed and used later, so, I cannot pass
> information from driver to of_device_alloc().
> 
> Based on your proposal, what about the following diff (or something
> similar)? Would it be acceptable for CIP:

This is just a warning workarond, AFAICT, so I guess more will be
needed and I'm not the only stakeholder. We'll need to see whole
series. But this sounds better than 1/ or 2/ alternatives proposed.

> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 43748c6480c8..ddbd46043d46 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -135,7 +135,8 @@ struct platform_device *of_device_alloc(struct device_node *np,
>                         rc = of_address_to_resource(np, i, res);
>                         WARN_ON(rc);
>                 }
> -               if (of_irq_to_resource_table(np, res, num_irq) != num_irq)
> +               if (!of_device_is_compatible(np, "renesas,rzg2l-irqc") &&
> +                   of_irq_to_resource_table(np, res, num_irq) != num_irq)
>                         pr_debug("not all legacy IRQ resources mapped for %pOFn\n",
>                                  np);
>         }
> 
> where "renesas,rzg2l-irqc" is the compatible for IA55 IRQ driver.

Best regards,
								Pavel

-- 
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]