Re: [RFC PATCH] of: platform: Skip mapping of interrupts in of_device_alloc()

[Lad, Prabhakar]

Hi Rob,

Thank you for the review.

On Thu, Dec 9, 2021 at 3:08 AM Rob Herring <robh+dt@xxxxxxxxxx> wrote:
>
> On Wed, Dec 8, 2021 at 6:11 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> wrote:
> >
> > of_device_alloc() in early boot stage creates a interrupt mapping if
> > there exists a "interrupts" property in the node.
> >
> > For hierarchical interrupt domains using "interrupts" property in the node
> > bypassed the hierarchical setup and messed up the irq setup.
> >
> > This patch adds a check in of_device_alloc() to skip interrupt mapping if
> > "not-interrupt-producer" property is present in the node. This allows
> > nodes to describe the interrupts using "interrupts" property.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > Hi All,
> >
> > Spawning from discussion [1], here is simple patch (not the ideal probably
> > welcome for suggestions) from stopping the OF code from creating a map for
> > the interrupts when using "interrupts" property.
> >
> > [1] https://lore.kernel.org/lkml/87pmqrck2m.wl-maz@xxxxxxxxxx/
> >     T/#mbd1e47c1981082aded4b32a52e2c04291e515508
> >
> > Cheers,
> > Prabhakar
> > ---
> >  drivers/of/platform.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index b3faf89744aa..629776ca1721 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -114,7 +114,7 @@ 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, num_irq = 0;
> >         struct resource *res, temp_res;
> >
> >         dev = platform_device_alloc("", PLATFORM_DEVID_NONE);
> > @@ -124,7 +124,14 @@ struct platform_device *of_device_alloc(struct device_node *np,
> >         /* 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);
> > +
> > +       /*
> > +        * we don't want to map the interrupts of hierarchical interrupt domain
> > +        * into the parent domain yet. This will be the job of the hierarchical
> > +        * interrupt driver code to map the interrupts as and when needed.
> > +        */
> > +       if (!of_property_read_bool(np, "not-interrupt-producer"))
> > +               num_irq = of_irq_count(np);
>
> The property won't fly for sure. A compatible match table could work
> here, but I don't really want another temporary solution.
>
Agreed.

> >         /* Populate the resource table */
> >         if (num_irq || num_reg) {
> > @@ -140,7 +147,7 @@ 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 (num_irq && of_irq_to_resource_table(np, res, num_irq) != num_irq)
>
> You might want to look at commit 9ec36cafe43b ("of/irq: do irq
> resolution in platform_get_irq"). The intent was to remove this code,
> but looks like the cleanup has a ways to go 7 years on. Primarily,
> it's convert any platform_get_resource(pdev, IORESOURCE_IRQ, n) call
> to platform_get_irq(). There's ~169 of those.
>
That's a good point converting all the drivers to use
platform_get_irq() so that the resource allocation happens on demand,
and the above code can be dropped.

> There are probably some open coded accesses to pdev->resources too,
> but I didn't spot any.
>
I'll have a closer look.

Cheers,
Prabhakar

> Rob