Re: [PATCH] of: property: Add device link support for interrupts

[Rob Herring]

On Wed, Jan 6, 2021 at 11:53 AM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> On Sat, Jan 2, 2021 at 3:37 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> >
> > On Thu, 31 Dec 2020 21:12:40 +0000,
> > Rob Herring <robh@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Dec 21, 2020 at 09:30:45AM +0000, Marc Zyngier wrote:
> > > > On 2020-12-18 21:07, Saravana Kannan wrote:
> > > > > Add support for creating device links out of interrupts property.
> > > > >
> > > > > Cc: Marc Zyngier <maz@xxxxxxxxxx>
> > > > > Cc: Kevin Hilman <khilman@xxxxxxxxxxxx>
> > > > > Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> > > > > ---
> > > > > Rob/Greg,
> > > > >
> > > > > This might need to go into driver-core to avoid conflict
> > > > > due to fw_devlink refactor series that merged there.
> > > > >
> > > > > Thanks,
> > > > > Saravana
> > > > >
> > > > >
> > > > >  drivers/of/property.c | 17 +++++++++++++++++
> > > > >  1 file changed, 17 insertions(+)
> > > > >
> > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > > > index 5f9eed79a8aa..e56a5eae0a0b 100644
> > > > > --- a/drivers/of/property.c
> > > > > +++ b/drivers/of/property.c
> > > > > @@ -1271,6 +1271,22 @@ static struct device_node
> > > > > *parse_iommu_maps(struct device_node *np,
> > > > >   return of_parse_phandle(np, prop_name, (index * 4) + 1);
> > > > >  }
> > > > >
> > > > > +static struct device_node *parse_interrupts(struct device_node *np,
> > > > > +                                     const char *prop_name, int index)
> > > > > +{
> > > > > + struct device_node *sup;
> > > > > +
> > > > > + if (strcmp(prop_name, "interrupts") || index)
> > > > > +         return NULL;
> > > > > +
> > > > > + of_node_get(np);
> > > > > + while (np && !(sup = of_parse_phandle(np, "interrupt-parent", 0)))
> > > > > +         np = of_get_next_parent(np);
> > > > > + of_node_put(np);
> > > > > +
> > > > > + return sup;
> > > > > +}
> > > > > +
> > > > >  static const struct supplier_bindings of_supplier_bindings[] = {
> > > > >   { .parse_prop = parse_clocks, },
> > > > >   { .parse_prop = parse_interconnects, },
> > > > > @@ -1296,6 +1312,7 @@ static const struct supplier_bindings
> > > > > of_supplier_bindings[] = {
> > > > >   { .parse_prop = parse_pinctrl6, },
> > > > >   { .parse_prop = parse_pinctrl7, },
> > > > >   { .parse_prop = parse_pinctrl8, },
> > > > > + { .parse_prop = parse_interrupts, },
> > > > >   { .parse_prop = parse_regulators, },
> > > > >   { .parse_prop = parse_gpio, },
> > > > >   { .parse_prop = parse_gpios, },
> > > >
> > > > You don't really describe what this is for so I'm only guessing
> > > > from the context. If you want to follow the interrupt hierarchy,
> > > > "interrupt-parent" isn't enough. You also need to track
> > > > things like interrupt-map, or anything that carries a phandle
> > > > to an interrupt controller.
> > >
> > > We don't need to follow the hierarchy, we just need the immediate
> > > dependencies.
> >
> > Indeed. I also wonder why this isn't just a irq_find_parent() call, TBH.
>
> Thanks Rob for explaining it.
>
> Marc, I wasn't sure if Rob would be okay with including of_irq.h here.
> Also, I'm trying to keep of/property.c independent of the framework
> code for now. The long term goal is to see if I can move out most of
> this into the frameworks. But I want to do that after I sort of some
> of the larger problems (like getting fw_devlink=on to work on all
> devices  first). Let me know if you have a strong preference for right
> now, if not, I'd rather keep property.c independent for now.
>
> I wasn't aware of interrupt-map until a few weeks ago and didn't know
> it carried phandles. I can add support for that too. There's no reason
> for all of them to go in one patch though.
>
> >
> > > But you are right that 'interrupt-map' also needs to be tracked.
> >
> > And 'interrupts-extended', while we're at it.
>
> This is already handled.
>
> > >
> > > I also noticed that we define 'interrupt-parent' as a dependency to
> > > parse, but that's wrong. The dependency is where 'interrupts' appears
> > > and where 'interrupt-parent' appears is irrelevant.
>
> No, the interrupt-parent parsing is correct and it's needed for
> interrupt controllers to probe in the right order. But
> interrupt-parent is also needs to be looked at for parsing
> "interrupts".

If you parse 'interrupts' for interrupt controllers (which in turn
will use 'interrupt-parent'), then you aren't going to need to track
'interrupt-parent' by itself.

To look at it another way, 'interrupt-parent' can appear in any
ancestor node. Which node the dts author arbitrarily decided to put it
in does not matter at all. It could be at the DT root or duplicated in
every single node with 'interrupts'. Those are logically the same. The
node(s) with the dependency are the ones with 'interrupts'.

Rob