[PATCH] of: property: Add device link support for interrupt-parent, dmas and -gpio(s)

[PATCH] of: property: Add device link support for interrupt-parent, dmas and -gpio(s)

[Saravana Kannan]

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)
+DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
 DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
+DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
+DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
 
 static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_clocks, },
@@ -1196,7 +1200,11 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_iommus, },
 	{ .parse_prop = parse_mboxes, },
 	{ .parse_prop = parse_io_channels, },
+	{ .parse_prop = parse_interrupt_parent, },
+	{ .parse_prop = parse_dmas, },
 	{ .parse_prop = parse_regulators, },
+	{ .parse_prop = parse_gpio, },
+	{ .parse_prop = parse_gpios, },
 	{}
 };
 
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH] of: property: Add device link support for interrupt-parent, dmas and -gpio(s)

[Saravana Kannan]

On Tue, Nov 19, 2019 at 11:13 PM 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)
> +DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
>  DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> +DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> +DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
>
>  static const struct supplier_bindings of_supplier_bindings[] = {
>         { .parse_prop = parse_clocks, },
> @@ -1196,7 +1200,11 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>         { .parse_prop = parse_iommus, },
>         { .parse_prop = parse_mboxes, },
>         { .parse_prop = parse_io_channels, },
> +       { .parse_prop = parse_interrupt_parent, },
> +       { .parse_prop = parse_dmas, },
>         { .parse_prop = parse_regulators, },
> +       { .parse_prop = parse_gpio, },
> +       { .parse_prop = parse_gpios, },
>         {}
>  };
>

Rob,

Any chance I get an Ack/Review please?

-Saravana

Re: [PATCH] of: property: Add device link support for interrupt-parent, dmas and -gpio(s)

[Linus Walleij]

On Wed, Nov 20, 2019 at 8: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>

This looks to me like doing the right thing and making sure that
the GPIO drivers get probed before their consumers and thus
speed up boot.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I guess I should get on with adding device links at runtime
as well, both for GPIO and pin control so that things work
with runtime-added devices and boardfiles and ACPI, if I
understand correctly it's fine to add the same link twice, it
will just be ignored?

Yours,
Linus Walleij

Re: [PATCH] of: property: Add device link support for interrupt-parent, dmas and -gpio(s)

[Rob Herring]

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. You could make it work
by specifying 'interrupt-parent' in every node, but that's not a
pattern I want to encourage. 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.

> +DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
>  DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> +DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> +DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
>
>  static const struct supplier_bindings of_supplier_bindings[] = {
>         { .parse_prop = parse_clocks, },
> @@ -1196,7 +1200,11 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>         { .parse_prop = parse_iommus, },
>         { .parse_prop = parse_mboxes, },
>         { .parse_prop = parse_io_channels, },
> +       { .parse_prop = parse_interrupt_parent, },
> +       { .parse_prop = parse_dmas, },
>         { .parse_prop = parse_regulators, },
> +       { .parse_prop = parse_gpio, },
> +       { .parse_prop = parse_gpios, },
>         {}
>  };
>
> --
> 2.24.0.432.g9d3f5f5b63-goog
>

Re: [PATCH] of: property: Add device link support for interrupt-parent, dmas and -gpio(s)

[Saravana Kannan]

On Thu, Nov 21, 2019 at 11:38 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Nov 20, 2019 at 8: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>
>
> This looks to me like doing the right thing and making sure that
> the GPIO drivers get probed before their consumers and thus
> speed up boot.
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks!

>
> I guess I should get on with adding device links at runtime
> as well, both for GPIO and pin control so that things work
> with runtime-added devices and boardfiles and ACPI, if I
> understand correctly it's fine to add the same link twice, it
> will just be ignored?

It's actually ref counted. So do check that the device_link_add()
succeeds before trying to release it later on [1].

-Saravana
[1] - https://lore.kernel.org/lkml/20191115000438.45970-1-saravanak@google.com/

Re: [PATCH] of: property: Add device link support for interrupt-parent, dmas and -gpio(s)

[Saravana Kannan]

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.

> 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".

-Saravana

>
> > +DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
> >  DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> > +DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> > +DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
> >
> >  static const struct supplier_bindings of_supplier_bindings[] = {
> >         { .parse_prop = parse_clocks, },
> > @@ -1196,7 +1200,11 @@ static const struct supplier_bindings of_supplier_bindings[] = {
> >         { .parse_prop = parse_iommus, },
> >         { .parse_prop = parse_mboxes, },
> >         { .parse_prop = parse_io_channels, },
> > +       { .parse_prop = parse_interrupt_parent, },
> > +       { .parse_prop = parse_dmas, },
> >         { .parse_prop = parse_regulators, },
> > +       { .parse_prop = parse_gpio, },
> > +       { .parse_prop = parse_gpios, },
> >         {}
> >  };
> >
> > --
> > 2.24.0.432.g9d3f5f5b63-goog
> >

Re: [PATCH] of: property: Add device link support for interrupt-parent, dmas and -gpio(s)

[Rob Herring]

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