[RFC PATCH] of/platform: Drop static setup of IRQ resource from DT core

[RFC PATCH] of/platform: Drop static setup of IRQ resource from DT core

[Lad Prabhakar]

Now that all the DT drivers have switched to platform_get_irq() we can now
safely drop the static setup of IRQ resource from DT core code.

With the above change hierarchical setup of irq domains is no longer
bypassed and thus allowing hierarchical interrupt domains to describe
interrupts using "interrupts" DT property.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
Hi All,

Sending this as RFC as couple of more drivers need to hit -rc yet with
the platform_get_irq() change while that is in progress I wanted to get
some feedback on this patch.

Cheers,
Prabhakar
---
 drivers/of/platform.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 793350028906..6890f7fe556f 100644
--- a/drivers/of/platform.c
+++ 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;
+	int rc, i, num_reg = 0;
 	struct resource *res, temp_res;
 
 	dev = platform_device_alloc("", PLATFORM_DEVID_NONE);
 	if (!dev)
 		return NULL;
 
-	/* count the io and irq resources */
+	/* count the io resources */
 	while (of_address_to_resource(np, num_reg, &temp_res) == 0)
 		num_reg++;
-	num_irq = of_irq_count(np);
 
 	/* Populate the resource table */
-	if (num_irq || num_reg) {
-		res = kcalloc(num_irq + num_reg, sizeof(*res), GFP_KERNEL);
+	if (num_reg) {
+		res = kcalloc(num_reg, sizeof(*res), GFP_KERNEL);
 		if (!res) {
 			platform_device_put(dev);
 			return NULL;
 		}
 
-		dev->num_resources = num_reg + num_irq;
+		dev->num_resources = num_reg;
 		dev->resource = res;
 		for (i = 0; i < num_reg; i++, res++) {
 			rc = of_address_to_resource(np, i, res);
 			WARN_ON(rc);
 		}
-		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);
-- 
2.17.1

Re: [RFC PATCH] of/platform: Drop static setup of IRQ resource from DT core

[Marc Zyngier]

On Wed, 16 Mar 2022 20:06:33 +0000,
Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> 
> Now that all the DT drivers have switched to platform_get_irq() we can now
> safely drop the static setup of IRQ resource from DT core code.
> 
> With the above change hierarchical setup of irq domains is no longer
> bypassed and thus allowing hierarchical interrupt domains to describe
> interrupts using "interrupts" DT property.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> Hi All,
> 
> Sending this as RFC as couple of more drivers need to hit -rc yet with
> the platform_get_irq() change while that is in progress I wanted to get
> some feedback on this patch.
> 
> Cheers,
> Prabhakar
> ---
>  drivers/of/platform.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 793350028906..6890f7fe556f 100644
> --- a/drivers/of/platform.c
> +++ 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;
> +	int rc, i, num_reg = 0;
>  	struct resource *res, temp_res;
>  
>  	dev = platform_device_alloc("", PLATFORM_DEVID_NONE);
>  	if (!dev)
>  		return NULL;
>  
> -	/* count the io and irq resources */
> +	/* count the io resources */
>  	while (of_address_to_resource(np, num_reg, &temp_res) == 0)
>  		num_reg++;
> -	num_irq = of_irq_count(np);
>  
>  	/* Populate the resource table */
> -	if (num_irq || num_reg) {
> -		res = kcalloc(num_irq + num_reg, sizeof(*res), GFP_KERNEL);
> +	if (num_reg) {
> +		res = kcalloc(num_reg, sizeof(*res), GFP_KERNEL);
>  		if (!res) {
>  			platform_device_put(dev);
>  			return NULL;
>  		}
>  
> -		dev->num_resources = num_reg + num_irq;
> +		dev->num_resources = num_reg;
>  		dev->resource = res;
>  		for (i = 0; i < num_reg; i++, res++) {
>  			rc = of_address_to_resource(np, i, res);
>  			WARN_ON(rc);
>  		}
> -		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);

I think this definitely goes in the right direction by not eagerly
populating resources without a driver actually needing it. If anything
breaks, that should be seen as an opportunity to fix the users of this
misfeature. I booted a couple of boxes with this patch, and nothing
caught fire, so:

Acked-by: Marc Zyngier <maz@kernel.org>
Tested-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [RFC PATCH] of/platform: Drop static setup of IRQ resource from DT core

[Rob Herring]

On Wed, Mar 16, 2022 at 08:06:33PM +0000, Lad Prabhakar wrote:
> Now that all the DT drivers have switched to platform_get_irq() we can now
> safely drop the static setup of IRQ resource from DT core code.
> 
> With the above change hierarchical setup of irq domains is no longer
> bypassed and thus allowing hierarchical interrupt domains to describe
> interrupts using "interrupts" DT property.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> Hi All,
> 
> Sending this as RFC as couple of more drivers need to hit -rc yet with
> the platform_get_irq() change while that is in progress I wanted to get
> some feedback on this patch.

I just applied this on top of current master and pushed to my 
for-kernelci branch. It should show up in kernelCI in a bit. I did this 
before all the fixes too and there were definitely a couple of test 
regressions.

Rob

Re: [RFC PATCH] of/platform: Drop static setup of IRQ resource from DT core

[Lad, Prabhakar]

Hi Rob,

On Thu, Mar 31, 2022 at 10:02 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Mar 16, 2022 at 08:06:33PM +0000, Lad Prabhakar wrote:
> > Now that all the DT drivers have switched to platform_get_irq() we can now
> > safely drop the static setup of IRQ resource from DT core code.
> >
> > With the above change hierarchical setup of irq domains is no longer
> > bypassed and thus allowing hierarchical interrupt domains to describe
> > interrupts using "interrupts" DT property.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > Hi All,
> >
> > Sending this as RFC as couple of more drivers need to hit -rc yet with
> > the platform_get_irq() change while that is in progress I wanted to get
> > some feedback on this patch.
>
> I just applied this on top of current master and pushed to my
> for-kernelci branch. It should show up in kernelCI in a bit. I did this
> before all the fixes too and there were definitely a couple of test
> regressions.
>
Any chance you can share the regressions or maybe the CI script so
that I can reproduce it locally.

Cheers,
Prabhakar

Re: [RFC PATCH] of/platform: Drop static setup of IRQ resource from DT core

[Rob Herring]

On Fri, Apr 1, 2022 at 2:42 AM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
>
> Hi Rob,
>
> On Thu, Mar 31, 2022 at 10:02 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Wed, Mar 16, 2022 at 08:06:33PM +0000, Lad Prabhakar wrote:
> > > Now that all the DT drivers have switched to platform_get_irq() we can now
> > > safely drop the static setup of IRQ resource from DT core code.
> > >
> > > With the above change hierarchical setup of irq domains is no longer
> > > bypassed and thus allowing hierarchical interrupt domains to describe
> > > interrupts using "interrupts" DT property.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > Hi All,
> > >
> > > Sending this as RFC as couple of more drivers need to hit -rc yet with
> > > the platform_get_irq() change while that is in progress I wanted to get
> > > some feedback on this patch.
> >
> > I just applied this on top of current master and pushed to my
> > for-kernelci branch. It should show up in kernelCI in a bit. I did this
> > before all the fixes too and there were definitely a couple of test
> > regressions.
> >
> Any chance you can share the regressions or maybe the CI script so
> that I can reproduce it locally.

It will show up here[1] as 'robh', but it still hasn't built yet which
is strange.

Rob

[1] https://linux.kernelci.org/job/

Re: [RFC PATCH] of/platform: Drop static setup of IRQ resource from DT core

[Rob Herring]

On Wed, Mar 16, 2022 at 08:06:33PM +0000, Lad Prabhakar wrote:
> Now that all the DT drivers have switched to platform_get_irq() we can now
> safely drop the static setup of IRQ resource from DT core code.
> 
> With the above change hierarchical setup of irq domains is no longer
> bypassed and thus allowing hierarchical interrupt domains to describe
> interrupts using "interrupts" DT property.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> Hi All,
> 
> Sending this as RFC as couple of more drivers need to hit -rc yet with
> the platform_get_irq() change while that is in progress I wanted to get
> some feedback on this patch.

I've applied this now and it is in today's linux-next. Keep an eye out 
for any regression reports. There's one for i.MX8 in kernel-ci, but I 
don't think it is related.

Rob

Re: [RFC PATCH] of/platform: Drop static setup of IRQ resource from DT core

[Lad, Prabhakar]

On Wed, Apr 6, 2022 at 2:57 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Mar 16, 2022 at 08:06:33PM +0000, Lad Prabhakar wrote:
> > Now that all the DT drivers have switched to platform_get_irq() we can now
> > safely drop the static setup of IRQ resource from DT core code.
> >
> > With the above change hierarchical setup of irq domains is no longer
> > bypassed and thus allowing hierarchical interrupt domains to describe
> > interrupts using "interrupts" DT property.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > Hi All,
> >
> > Sending this as RFC as couple of more drivers need to hit -rc yet with
> > the platform_get_irq() change while that is in progress I wanted to get
> > some feedback on this patch.
>
> I've applied this now and it is in today's linux-next. Keep an eye out
> for any regression reports. There's one for i.MX8 in kernel-ci, but I
> don't think it is related.
>
Thanks for the heads up, sure I will keep an eye on it.

Cheers,
Prabhakar

Re: [RFC PATCH] of/platform: Drop static setup of IRQ resource from DT core

[Yongqin Liu]

Hi, Lad

# sorry for the confusion if you have received it before with the
non-plain-text mode

In this change you said "all the DT drivers have switched to
platform_get_irq()",
could you please help share with me one example about the above change
as reference?
We have one hikey960 android build with some out of tree changes,
which could not boot
successfully with some errors on surfaceflinger(I am not sure it's a
problem with the gpu or display),
but could boot if I have this change reverted.

I guess it needs some changes on the gpu/display dts or driver side to
have it work
with this change, not sure if you could give some suggestions on the fix.

And here are two out of tree changes might be related listed here just
for reference in case:
https://android-review.linaro.org/c/kernel/common/+/21680
https://android-review.linaro.org/c/kernel/common/+/21682

Thanks in advance!

On Thu, 17 Mar 2022 at 04:07, Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
>
> Now that all the DT drivers have switched to platform_get_irq() we can now
> safely drop the static setup of IRQ resource from DT core code.
>
> With the above change hierarchical setup of irq domains is no longer
> bypassed and thus allowing hierarchical interrupt domains to describe
> interrupts using "interrupts" DT property.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> Hi All,
>
> Sending this as RFC as couple of more drivers need to hit -rc yet with
> the platform_get_irq() change while that is in progress I wanted to get
> some feedback on this patch.
>
> Cheers,
> Prabhakar
> ---
>  drivers/of/platform.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 793350028906..6890f7fe556f 100644
> --- a/drivers/of/platform.c
> +++ 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;
> +       int rc, i, num_reg = 0;
>         struct resource *res, temp_res;
>
>         dev = platform_device_alloc("", PLATFORM_DEVID_NONE);
>         if (!dev)
>                 return NULL;
>
> -       /* count the io and irq resources */
> +       /* count the io resources */
>         while (of_address_to_resource(np, num_reg, &temp_res) == 0)
>                 num_reg++;
> -       num_irq = of_irq_count(np);
>
>         /* Populate the resource table */
> -       if (num_irq || num_reg) {
> -               res = kcalloc(num_irq + num_reg, sizeof(*res), GFP_KERNEL);
> +       if (num_reg) {
> +               res = kcalloc(num_reg, sizeof(*res), GFP_KERNEL);
>                 if (!res) {
>                         platform_device_put(dev);
>                         return NULL;
>                 }
>
> -               dev->num_resources = num_reg + num_irq;
> +               dev->num_resources = num_reg;
>                 dev->resource = res;
>                 for (i = 0; i < num_reg; i++, res++) {
>                         rc = of_address_to_resource(np, i, res);
>                         WARN_ON(rc);
>                 }
> -               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);
> --
> 2.17.1
>


-- 
Best Regards,
Yongqin Liu
---------------------------------------------------------------
#mailing list
linaro-android@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-android

Re: [RFC PATCH] of/platform: Drop static setup of IRQ resource from DT core

[Lad, Prabhakar]

Hi Yongqin,

On Sat, Jun 11, 2022 at 6:28 AM Yongqin Liu <yongqin.liu@linaro.org> wrote:
>
> Hi, Lad
>
> # sorry for the confusion if you have received it before with the
> non-plain-text mode
>
> In this change you said "all the DT drivers have switched to
> platform_get_irq()",
> could you please help share with me one example about the above change
> as reference?
The change is we just switch to using platform_get_irq() [0] for
fetching IRQ numbers.

> We have one hikey960 android build with some out of tree changes,
> which could not boot
> successfully with some errors on surfaceflinger(I am not sure it's a
> problem with the gpu or display),
> but could boot if I have this change reverted.
>
> I guess it needs some changes on the gpu/display dts or driver side to
> have it work
Just the changes to the driver is needed.

> with this change, not sure if you could give some suggestions on the fix.
>
> And here are two out of tree changes might be related listed here just
> for reference in case:
> https://android-review.linaro.org/c/kernel/common/+/21680
> https://android-review.linaro.org/c/kernel/common/+/21682
>

[0] https://lore.kernel.org/lkml/20211221213547.1553-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar

Re: [RFC PATCH] of/platform: Drop static setup of IRQ resource from DT core

[Yongqin Liu]

Hi, Lad

Thanks a lot for the links and suggestions!
Finally I resolved the problem with the call of
platform_get_irq_byname and irq_get_trigger_type.

Btw, I just have a question about  the of_irq_to_resource function.
At the beginning I tried to use platform_get_irq and of_irq_to_resource
to get the irq name and flags information, but it seems of_irq_to_resource
does not work as expected, maybe I called incorrectly somewhere,
here I just want to ask, do you think that if of_irq_to_resource still
could be used to
get the resource with the irq returned from platform_get_irq?


Thanks,
Yongqin Liu

On Sat, 11 Jun 2022 at 16:01, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> Hi Yongqin,
>
> On Sat, Jun 11, 2022 at 6:28 AM Yongqin Liu <yongqin.liu@linaro.org> wrote:
> >
> > Hi, Lad
> >
> > # sorry for the confusion if you have received it before with the
> > non-plain-text mode
> >
> > In this change you said "all the DT drivers have switched to
> > platform_get_irq()",
> > could you please help share with me one example about the above change
> > as reference?
> The change is we just switch to using platform_get_irq() [0] for
> fetching IRQ numbers.
>
> > We have one hikey960 android build with some out of tree changes,
> > which could not boot
> > successfully with some errors on surfaceflinger(I am not sure it's a
> > problem with the gpu or display),
> > but could boot if I have this change reverted.
> >
> > I guess it needs some changes on the gpu/display dts or driver side to
> > have it work
> Just the changes to the driver is needed.
>
> > with this change, not sure if you could give some suggestions on the fix.
> >
> > And here are two out of tree changes might be related listed here just
> > for reference in case:
> > https://android-review.linaro.org/c/kernel/common/+/21680
> > https://android-review.linaro.org/c/kernel/common/+/21682
> >
>
> [0] https://lore.kernel.org/lkml/20211221213547.1553-1-prabhakar.mahadev-lad.rj@bp.renesas.com/
>
> Cheers,
> Prabhakar



-- 
Best Regards,
Yongqin Liu
---------------------------------------------------------------
#mailing list
linaro-android@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-android

Re: [RFC PATCH] of/platform: Drop static setup of IRQ resource from DT core

[Lad, Prabhakar]

On Mon, Jun 13, 2022 at 1:41 PM Yongqin Liu <yongqin.liu@linaro.org> wrote:
>
> Hi, Lad
>
> Thanks a lot for the links and suggestions!
> Finally I resolved the problem with the call of
> platform_get_irq_byname and irq_get_trigger_type.
>
> Btw, I just have a question about  the of_irq_to_resource function.
> At the beginning I tried to use platform_get_irq and of_irq_to_resource
> to get the irq name and flags information, but it seems of_irq_to_resource
> does not work as expected, maybe I called incorrectly somewhere,
> here I just want to ask, do you think that if of_irq_to_resource still
> could be used to
> get the resource with the irq returned from platform_get_irq?
>
>
Yes of_irq_to_resource() can be used to get the irq number.

Cheers,
Prabhakar