MIME-Version: 1.0
References: <20211209001056.29774-1-prabhakar.mahadev-lad.rj@bp.renesas.com> <CAL_JsqJUS0-ZNus__7nJJ-BaJBqQcS0NZ8a4o5QheLt4g8oK+Q@mail.gmail.com>
In-Reply-To: <CAL_JsqJUS0-ZNus__7nJJ-BaJBqQcS0NZ8a4o5QheLt4g8oK+Q@mail.gmail.com>
From:   "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Date:   Thu, 9 Dec 2021 09:48:54 +0000
Message-ID: <CA+V-a8s5TpsXCFLFxzzsWj0ho3Hu+pS93mqXdHsCCh2LdzSQuQ@mail.gmail.com>
Subject: Re: [RFC PATCH] of: platform: Skip mapping of interrupts in of_device_alloc()
To:     Rob Herring <robh+dt@kernel.org>
Cc:     Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
        Frank Rowand <frowand.list@gmail.com>,
        Marc Zyngier <maz@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "open list:MEDIA DRIVERS FOR RENESAS - FCP" 
        <linux-renesas-soc@vger.kernel.org>,
        Biju Das <biju.das.jz@bp.renesas.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Rob,

Thank you for the review.

On Thu, Dec 9, 2021 at 3:08 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Wed, Dec 8, 2021 at 6:11 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> 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@bp.renesas.com>
> > ---
> > 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@kernel.org/
> >     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