Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191120071302.227777-1-saravanak@google.com> <CAL_Jsq+f1+xRv36z0o--u4SskTG-WxUdssJ-CP32RUZbtVuQ3w@mail.gmail.com>
 <CAGETcx-GV1kTAVbqcCLGVPoN16RpSrDw4gcxSgAVqWCb1NOzXA@mail.gmail.com>
In-Reply-To: <CAGETcx-GV1kTAVbqcCLGVPoN16RpSrDw4gcxSgAVqWCb1NOzXA@mail.gmail.com>
From:   Rob Herring <robh+dt@kernel.org>
Date:   Fri, 22 Nov 2019 14:42:27 -0600
Message-ID: <CAL_JsqLHGvSWsUrDJObLqCnnwe=pwWJpeXtToMj_sHgLQGev7A@mail.gmail.com>
Subject: Re: [PATCH] of: property: Add device link support for
 interrupt-parent, dmas and -gpio(s)
To:     Saravana Kannan <saravanak@google.com>
Cc:     Frank Rowand <frowand.list@gmail.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Vinod Koul <vkoul@kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Android Kernel Team <kernel-team@android.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Nov 22, 2019 at 1:34 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Fri, Nov 22, 2019 at 5:35 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Wed, Nov 20, 2019 at 1:13 AM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > Add support for creating device links out of more DT properties.
> > >
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: Vinod Koul <vkoul@kernel.org>
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/of/property.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index 0fa04692e3cc..dedbf82da838 100644
> > > --- a/drivers/of/property.c
> > > +++ b/drivers/of/property.c
> > > @@ -1188,7 +1188,11 @@ DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells")
> > >  DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
> > >  DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
> > >  DEFINE_SIMPLE_PROP(io_channels, "io-channel", "#io-channel-cells")
> > > +DEFINE_SIMPLE_PROP(interrupt_parent, "interrupt-parent", NULL)
> >
> > This one is not going to work most of the time (ignoring the fact that
> > the primary controller doesn't have a struct device) because the
> > interrupt-parent is typically in a parent node.
>
> Just to make sure, I'm not parsing this property incorrectly, right?
>
> Are you saying it's listed at the parent of a bunch of devices and the
> interrupt-parent is inherited and won't really create device links for
> those child devices?
> I mainly added this to make sure IRQ controllers are probed in the
> right order. Also, if this delays the parent device probe, by the time
> the child devices are added, the interrupt parent most likely would
> already be probed.
>
> > You could make it work
> > by specifying 'interrupt-parent' in every node, but that's not a
> > pattern I want to encourage.
>
> I'm trying to take care of the basic per-device properties first.
> Adding support for inherited properties isn't too difficult, I just
> need to get to those at some point. Also, for inherited properties, we
> can't really block probing because the child device might not depend
> on that resource. Inherited properties are mainly relevant only for
> sync_state() callbacks.

You're looking at the wrong property. You need to look for
'interrupts' and 'interrupts-extended'. Pretty good chance if the
device has interrupts, then it needs them. The latter is easy. It's
just like the others you have (phandle + cells). If you have
'interrupts' then you need to walk the interrupt tree the same way the
OF IRQ code already does.

> > There's also all the other ways the
> > parent can be determined. Any parent node with 'interrupt-controller'
> > or 'interrupt-map' property is the parent. And there's
> > 'interrupts-extended' too.
>
> Now I'm confused. Not sure if you are referring to actual device
> parent now or if you are talking about an "interrupt supplier".

Yes. both. The interrupt tree may or may not mirror the device node
tree. Or you could have a mixture. You've got to handle both cases.
But really, you just need to use the code that's already there to do
all that and deal with all these conditions. Essentially, you need to
copy of_irq_get(), but get the struct device from the irq_domain.

Rob