* [PATCH] of: property: Add device link support for interrupts @ 2020-12-18 21:07 Saravana Kannan 2020-12-21 9:30 ` Marc Zyngier 2021-01-20 9:53 ` Geert Uytterhoeven 0 siblings, 2 replies; 20+ messages in thread From: Saravana Kannan @ 2020-12-18 21:07 UTC (permalink / raw) To: Greg Kroah-Hartman, Rob Herring, Frank Rowand Cc: Saravana Kannan, Marc Zyngier, Kevin Hilman, kernel-team, devicetree, linux-kernel Add support for creating device links out of interrupts property. Cc: Marc Zyngier <maz@kernel.org> Cc: Kevin Hilman <khilman@baylibre.com> Signed-off-by: Saravana Kannan <saravanak@google.com> --- 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, }, -- 2.29.2.684.gfbc64c5ab5-goog ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2020-12-18 21:07 [PATCH] of: property: Add device link support for interrupts Saravana Kannan @ 2020-12-21 9:30 ` Marc Zyngier 2020-12-31 21:12 ` Rob Herring 2021-01-20 9:53 ` Geert Uytterhoeven 1 sibling, 1 reply; 20+ messages in thread From: Marc Zyngier @ 2020-12-21 9:30 UTC (permalink / raw) To: Saravana Kannan Cc: Greg Kroah-Hartman, Rob Herring, Frank Rowand, Kevin Hilman, kernel-team, devicetree, linux-kernel On 2020-12-18 21:07, Saravana Kannan wrote: > Add support for creating device links out of interrupts property. > > Cc: Marc Zyngier <maz@kernel.org> > Cc: Kevin Hilman <khilman@baylibre.com> > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > 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. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2020-12-21 9:30 ` Marc Zyngier @ 2020-12-31 21:12 ` Rob Herring 2021-01-02 11:36 ` Marc Zyngier 0 siblings, 1 reply; 20+ messages in thread From: Rob Herring @ 2020-12-31 21:12 UTC (permalink / raw) To: Marc Zyngier Cc: Saravana Kannan, Greg Kroah-Hartman, Frank Rowand, Kevin Hilman, kernel-team, devicetree, linux-kernel 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@kernel.org> > > Cc: Kevin Hilman <khilman@baylibre.com> > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > --- > > 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. But you are right that 'interrupt-map' also needs to be tracked. 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. Rob ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2020-12-31 21:12 ` Rob Herring @ 2021-01-02 11:36 ` Marc Zyngier 2021-01-06 18:52 ` Saravana Kannan 0 siblings, 1 reply; 20+ messages in thread From: Marc Zyngier @ 2021-01-02 11:36 UTC (permalink / raw) To: Rob Herring Cc: Saravana Kannan, Greg Kroah-Hartman, Frank Rowand, Kevin Hilman, kernel-team, devicetree, linux-kernel On Thu, 31 Dec 2020 21:12:40 +0000, Rob Herring <robh@kernel.org> 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@kernel.org> > > > Cc: Kevin Hilman <khilman@baylibre.com> > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > --- > > > 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. > But you are right that 'interrupt-map' also needs to be tracked. And 'interrupts-extended', while we're at it. > > 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. Agreed. Though you need the object the dependency is on, I guess, if you want to be able to have the dependency edge between the device and the interrupt controller. But since the commit message doesn't say much about what this is trying to achieve, I'm only guessing the purpose of this patch. M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2021-01-02 11:36 ` Marc Zyngier @ 2021-01-06 18:52 ` Saravana Kannan 2021-01-07 1:25 ` Saravana Kannan 2021-01-07 18:39 ` Rob Herring 0 siblings, 2 replies; 20+ messages in thread From: Saravana Kannan @ 2021-01-06 18:52 UTC (permalink / raw) To: Marc Zyngier Cc: Rob Herring, Greg Kroah-Hartman, Frank Rowand, Kevin Hilman, Android Kernel Team, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML On Sat, Jan 2, 2021 at 3:37 AM Marc Zyngier <maz@kernel.org> wrote: > > On Thu, 31 Dec 2020 21:12:40 +0000, > Rob Herring <robh@kernel.org> 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@kernel.org> > > > > Cc: Kevin Hilman <khilman@baylibre.com> > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > --- > > > > 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". -Saravana > Agreed. Though you need the object the dependency is on, I guess, if > you want to be able to have the dependency edge between the device and > the interrupt controller. But since the commit message doesn't say > much about what this is trying to achieve, I'm only guessing the > purpose of this patch. > > M. > > -- > Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2021-01-06 18:52 ` Saravana Kannan @ 2021-01-07 1:25 ` Saravana Kannan 2021-01-07 16:47 ` Rob Herring 2021-01-07 18:39 ` Rob Herring 1 sibling, 1 reply; 20+ messages in thread From: Saravana Kannan @ 2021-01-07 1:25 UTC (permalink / raw) To: Marc Zyngier Cc: Rob Herring, Greg Kroah-Hartman, Frank Rowand, Kevin Hilman, Android Kernel Team, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML On Wed, Jan 6, 2021 at 10:52 AM Saravana Kannan <saravanak@google.com> wrote: > > On Sat, Jan 2, 2021 at 3:37 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Thu, 31 Dec 2020 21:12:40 +0000, > > Rob Herring <robh@kernel.org> 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@kernel.org> > > > > > Cc: Kevin Hilman <khilman@baylibre.com> > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > --- > > > > > 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. Hmm... I looked at Documentation/devicetree/bindings/interrupt-controller/interrupts.txt and it has no documentation for interrupt-map. There's a bunch of references to it in device specific DT binding documentation but I don't want to rely on those for correctness. Marc, can you explain the interrupt-map format here or add documentation for it please? -Saravana ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2021-01-07 1:25 ` Saravana Kannan @ 2021-01-07 16:47 ` Rob Herring 2021-01-07 17:08 ` Saravana Kannan 0 siblings, 1 reply; 20+ messages in thread From: Rob Herring @ 2021-01-07 16:47 UTC (permalink / raw) To: Saravana Kannan Cc: Marc Zyngier, Greg Kroah-Hartman, Frank Rowand, Kevin Hilman, Android Kernel Team, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML On Wed, Jan 6, 2021 at 6:26 PM Saravana Kannan <saravanak@google.com> wrote: > > On Wed, Jan 6, 2021 at 10:52 AM Saravana Kannan <saravanak@google.com> wrote: > > > > On Sat, Jan 2, 2021 at 3:37 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Thu, 31 Dec 2020 21:12:40 +0000, > > > Rob Herring <robh@kernel.org> 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@kernel.org> > > > > > > Cc: Kevin Hilman <khilman@baylibre.com> > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > > --- > > > > > > 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. > > Hmm... I looked at > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > and it has no documentation for interrupt-map. There's a bunch of > references to it in device specific DT binding documentation but I > don't want to rely on those for correctness. See the DT spec and there's also details on elinux.org. It's only existed since the 1990s. Rob ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2021-01-07 16:47 ` Rob Herring @ 2021-01-07 17:08 ` Saravana Kannan 2021-01-07 17:17 ` Marc Zyngier 0 siblings, 1 reply; 20+ messages in thread From: Saravana Kannan @ 2021-01-07 17:08 UTC (permalink / raw) To: Rob Herring Cc: Marc Zyngier, Greg Kroah-Hartman, Frank Rowand, Kevin Hilman, Android Kernel Team, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML On Thu, Jan 7, 2021 at 8:48 AM Rob Herring <robh@kernel.org> wrote: > > On Wed, Jan 6, 2021 at 6:26 PM Saravana Kannan <saravanak@google.com> wrote: > > > > On Wed, Jan 6, 2021 at 10:52 AM Saravana Kannan <saravanak@google.com> wrote: > > > > > > On Sat, Jan 2, 2021 at 3:37 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > On Thu, 31 Dec 2020 21:12:40 +0000, > > > > Rob Herring <robh@kernel.org> 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@kernel.org> > > > > > > > Cc: Kevin Hilman <khilman@baylibre.com> > > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > > > --- > > > > > > > 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. > > > > Hmm... I looked at > > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > > and it has no documentation for interrupt-map. There's a bunch of > > references to it in device specific DT binding documentation but I > > don't want to rely on those for correctness. > > See the DT spec and there's also details on elinux.org. It's only > existed since the 1990s. Thanks :) Will try to find it there. -Saravana ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2021-01-07 17:08 ` Saravana Kannan @ 2021-01-07 17:17 ` Marc Zyngier 0 siblings, 0 replies; 20+ messages in thread From: Marc Zyngier @ 2021-01-07 17:17 UTC (permalink / raw) To: Saravana Kannan Cc: Rob Herring, Greg Kroah-Hartman, Frank Rowand, Kevin Hilman, Android Kernel Team, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML On 2021-01-07 17:08, Saravana Kannan wrote: > On Thu, Jan 7, 2021 at 8:48 AM Rob Herring <robh@kernel.org> wrote: >> >> On Wed, Jan 6, 2021 at 6:26 PM Saravana Kannan <saravanak@google.com> >> wrote: >> > >> > On Wed, Jan 6, 2021 at 10:52 AM Saravana Kannan <saravanak@google.com> wrote: [...] >> > > 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. >> > >> > Hmm... I looked at >> > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt >> > and it has no documentation for interrupt-map. There's a bunch of >> > references to it in device specific DT binding documentation but I >> > don't want to rely on those for correctness. >> >> See the DT spec and there's also details on elinux.org. It's only >> existed since the 1990s. > > Thanks :) Will try to find it there. My document of reference is [1]. Although interrupt-map appears there in the context of PCI, it is in no way specific to a specific bus. HTH, M. [1] https://www.openbios.org/data/docs/rec.intmap.d09.pdf -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2021-01-06 18:52 ` Saravana Kannan 2021-01-07 1:25 ` Saravana Kannan @ 2021-01-07 18:39 ` Rob Herring 2021-01-07 19:09 ` Saravana Kannan 1 sibling, 1 reply; 20+ messages in thread From: Rob Herring @ 2021-01-07 18:39 UTC (permalink / raw) To: Saravana Kannan Cc: Marc Zyngier, Greg Kroah-Hartman, Frank Rowand, Kevin Hilman, Android Kernel Team, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML On Wed, Jan 6, 2021 at 11:53 AM Saravana Kannan <saravanak@google.com> wrote: > > On Sat, Jan 2, 2021 at 3:37 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Thu, 31 Dec 2020 21:12:40 +0000, > > Rob Herring <robh@kernel.org> 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@kernel.org> > > > > > Cc: Kevin Hilman <khilman@baylibre.com> > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > --- > > > > > 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2021-01-07 18:39 ` Rob Herring @ 2021-01-07 19:09 ` Saravana Kannan 2021-01-07 19:33 ` Rob Herring 0 siblings, 1 reply; 20+ messages in thread From: Saravana Kannan @ 2021-01-07 19:09 UTC (permalink / raw) To: Rob Herring Cc: Marc Zyngier, Greg Kroah-Hartman, Frank Rowand, Kevin Hilman, Android Kernel Team, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML On Thu, Jan 7, 2021 at 10:39 AM Rob Herring <robh@kernel.org> wrote: > > On Wed, Jan 6, 2021 at 11:53 AM Saravana Kannan <saravanak@google.com> wrote: > > > > On Sat, Jan 2, 2021 at 3:37 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Thu, 31 Dec 2020 21:12:40 +0000, > > > Rob Herring <robh@kernel.org> 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@kernel.org> > > > > > > Cc: Kevin Hilman <khilman@baylibre.com> > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > > --- > > > > > > 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. Do all interrupt controllers (that are not the root interrupt controller) need to have "interrupts" property? If yes, then yeah, that makes sense. But I vaguely remember that this wasn't the case for some DT I saw. Ah, here's one I found. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/mt2701.dtsi#n209 -Saravana ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2021-01-07 19:09 ` Saravana Kannan @ 2021-01-07 19:33 ` Rob Herring 2021-01-07 23:13 ` Saravana Kannan 0 siblings, 1 reply; 20+ messages in thread From: Rob Herring @ 2021-01-07 19:33 UTC (permalink / raw) To: Saravana Kannan Cc: Marc Zyngier, Greg Kroah-Hartman, Frank Rowand, Kevin Hilman, Android Kernel Team, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML On Thu, Jan 7, 2021 at 12:09 PM Saravana Kannan <saravanak@google.com> wrote: > > On Thu, Jan 7, 2021 at 10:39 AM Rob Herring <robh@kernel.org> wrote: > > > > On Wed, Jan 6, 2021 at 11:53 AM Saravana Kannan <saravanak@google.com> wrote: > > > > > > On Sat, Jan 2, 2021 at 3:37 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > On Thu, 31 Dec 2020 21:12:40 +0000, > > > > Rob Herring <robh@kernel.org> 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@kernel.org> > > > > > > > Cc: Kevin Hilman <khilman@baylibre.com> > > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > > > --- > > > > > > > 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. > > Do all interrupt controllers (that are not the root interrupt > controller) need to have "interrupts" property? If yes, then yeah, > that makes sense. But I vaguely remember that this wasn't the case for > some DT I saw. There are some cases of stacked controllers where it's implicit. > > Ah, here's one I found. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/mt2701.dtsi#n209 Right, so this is one of several cases of custom interrupt mapping properties (mediatek,ext-irq-range). Really, 'interrupts' or 'interrupt-map' should have been used here, but 'interrupt-map' doesn't really scale well if you have large ranges of interrupts. To handle the dependency with just 'interrupt-parent', you need to find nodes that are themselves an 'interrupt-parent' and then find their 'interrupt-parent'. Rob ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2021-01-07 19:33 ` Rob Herring @ 2021-01-07 23:13 ` Saravana Kannan 2021-01-08 3:35 ` Rob Herring 0 siblings, 1 reply; 20+ messages in thread From: Saravana Kannan @ 2021-01-07 23:13 UTC (permalink / raw) To: Rob Herring Cc: Marc Zyngier, Greg Kroah-Hartman, Frank Rowand, Kevin Hilman, Android Kernel Team, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML On Thu, Jan 7, 2021 at 11:33 AM Rob Herring <robh@kernel.org> wrote: > > On Thu, Jan 7, 2021 at 12:09 PM Saravana Kannan <saravanak@google.com> wrote: > > > > On Thu, Jan 7, 2021 at 10:39 AM Rob Herring <robh@kernel.org> wrote: > > > > > > On Wed, Jan 6, 2021 at 11:53 AM Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > On Sat, Jan 2, 2021 at 3:37 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > On Thu, 31 Dec 2020 21:12:40 +0000, > > > > > Rob Herring <robh@kernel.org> 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@kernel.org> > > > > > > > > Cc: Kevin Hilman <khilman@baylibre.com> > > > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > > > > --- > > > > > > > > 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. > > > > Do all interrupt controllers (that are not the root interrupt > > controller) need to have "interrupts" property? If yes, then yeah, > > that makes sense. But I vaguely remember that this wasn't the case for > > some DT I saw. > > There are some cases of stacked controllers where it's implicit. In that case, I think it's good to track interrupt-parent explicitly. Doesn't really hurt anything. We already protect for stuff like making sure a parent doesn't depend on its child, etc. > > > > Ah, here's one I found. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/mt2701.dtsi#n209 > > Right, so this is one of several cases of custom interrupt mapping > properties (mediatek,ext-irq-range). Really, 'interrupts' or > 'interrupt-map' should have been used here, but 'interrupt-map' > doesn't really scale well if you have large ranges of interrupts. > > To handle the dependency with just 'interrupt-parent', you need to > find nodes that are themselves an 'interrupt-parent' and then find > their 'interrupt-parent'. Not sure I understand this. On a side note, if I'm adding device links between a device and the "interrupt-parent" it's pointing to, at worst, I'm having it depend on an interrupt controller its child devices would depend on. This combined with the fact that "weird" links like "parent depending on child", "non-device node having interrupt-parent", etc are already ignored, seems safe to leave in "interrupt-parent" to catch these cases where interrupt controllers don't specify "interrupts" or "interrupt-map"? I don't mind removing it, but maybe we can wait till we get fw_devlink=on and then remove it to see if anything breaks? If nothing breaks, we can remove explicit interrupt-parent parsing? Going back to interrupt-map, I understand the syntax now. I'm trying to see if I can break up of_irq_parse_raw() into smaller pieces and reuse (call into) some of that code. While doing that I see that when "address-cells" isn't present, the "addrsize" is initialized to 2 [1] but when "address-cells" isn't present, the "newaddrsize" is initialized to 0 [1]. Why is the default value of #address-cells two different numbers? Thanks, Saravana [1] - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/irq.c#n141 [2] - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/irq.c#n226 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2021-01-07 23:13 ` Saravana Kannan @ 2021-01-08 3:35 ` Rob Herring 2021-01-08 4:34 ` Saravana Kannan 0 siblings, 1 reply; 20+ messages in thread From: Rob Herring @ 2021-01-08 3:35 UTC (permalink / raw) To: Saravana Kannan Cc: Marc Zyngier, Greg Kroah-Hartman, Frank Rowand, Kevin Hilman, Android Kernel Team, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML On Thu, Jan 7, 2021 at 4:13 PM Saravana Kannan <saravanak@google.com> wrote: > > On Thu, Jan 7, 2021 at 11:33 AM Rob Herring <robh@kernel.org> wrote: > > > > On Thu, Jan 7, 2021 at 12:09 PM Saravana Kannan <saravanak@google.com> wrote: > > > > > > On Thu, Jan 7, 2021 at 10:39 AM Rob Herring <robh@kernel.org> wrote: > > > > > > > > On Wed, Jan 6, 2021 at 11:53 AM Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > > > On Sat, Jan 2, 2021 at 3:37 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > > > On Thu, 31 Dec 2020 21:12:40 +0000, > > > > > > Rob Herring <robh@kernel.org> 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@kernel.org> > > > > > > > > > Cc: Kevin Hilman <khilman@baylibre.com> > > > > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > > > > > --- > > > > > > > > > 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. > > > > > > Do all interrupt controllers (that are not the root interrupt > > > controller) need to have "interrupts" property? If yes, then yeah, > > > that makes sense. But I vaguely remember that this wasn't the case for > > > some DT I saw. > > > > There are some cases of stacked controllers where it's implicit. > > In that case, I think it's good to track interrupt-parent explicitly. > Doesn't really hurt anything. We already protect for stuff like making > sure a parent doesn't depend on its child, etc. Given the interrupt controller is probably always going to be there, it probably doesn't really matter. But the dependency is wrong. Does this work: bus { interrupt-parent = <&root_int>; root_int: interrupt-controller { }; device { interrupts = <1>; }; }; The only dependency here should be /bus/device -> root_int. > > > > > > Ah, here's one I found. > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/mt2701.dtsi#n209 > > > > Right, so this is one of several cases of custom interrupt mapping > > properties (mediatek,ext-irq-range). Really, 'interrupts' or > > 'interrupt-map' should have been used here, but 'interrupt-map' > > doesn't really scale well if you have large ranges of interrupts. > > > > To handle the dependency with just 'interrupt-parent', you need to > > find nodes that are themselves an 'interrupt-parent' and then find > > their 'interrupt-parent'. > > Not sure I understand this. > > On a side note, if I'm adding device links between a device and the > "interrupt-parent" it's pointing to, at worst, I'm having it depend on > an interrupt controller its child devices would depend on. This > combined with the fact that "weird" links like "parent depending on > child", The above would be this case if you treat root_int as a dependency of bus. It's not, but I guess that plus the child dependency on parent makes interrupt dependencies mostly work. > "non-device node having interrupt-parent", Yes, that's probably 95% of interrupt-parent cases. It's either at the root or bus node. > etc are already > ignored, seems safe to leave in "interrupt-parent" to catch these > cases where interrupt controllers don't specify "interrupts" or > "interrupt-map"? I don't mind removing it, but maybe we can wait till > we get fw_devlink=on and then remove it to see if anything breaks? If > nothing breaks, we can remove explicit interrupt-parent parsing? Okay, as long as we're breaking platforms once potentially. > Going back to interrupt-map, I understand the syntax now. I'm trying > to see if I can break up of_irq_parse_raw() into smaller pieces and > reuse (call into) some of that code. While doing that I see that when > "address-cells" isn't present, the "addrsize" is initialized to 2 [1] > but when "address-cells" isn't present, the "newaddrsize" is > initialized to 0 [1]. Why is the default value of #address-cells two > different numbers? It's some ancient code that needs to handle quirks of ancient systems. The first one is the #address-cells for the device's bus. The second is #address-cells in the interrupt-controller/map. It's complicated because interrupt hierarchies used to follow the bus hierarchies. Generally in modern systems #address-cells used for interrupt-map purposes (the 2nd case) will always be 0. Note that there's generic nexus code to support gpio-map. It works similarly though without the address-cells part. Any refactoring should take that into account. Rob ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2021-01-08 3:35 ` Rob Herring @ 2021-01-08 4:34 ` Saravana Kannan 0 siblings, 0 replies; 20+ messages in thread From: Saravana Kannan @ 2021-01-08 4:34 UTC (permalink / raw) To: Rob Herring Cc: Marc Zyngier, Greg Kroah-Hartman, Frank Rowand, Kevin Hilman, Android Kernel Team, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML On Thu, Jan 7, 2021 at 7:36 PM Rob Herring <robh@kernel.org> wrote: > > On Thu, Jan 7, 2021 at 4:13 PM Saravana Kannan <saravanak@google.com> wrote: > > > > On Thu, Jan 7, 2021 at 11:33 AM Rob Herring <robh@kernel.org> wrote: > > > > > > On Thu, Jan 7, 2021 at 12:09 PM Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > On Thu, Jan 7, 2021 at 10:39 AM Rob Herring <robh@kernel.org> wrote: > > > > > > > > > > On Wed, Jan 6, 2021 at 11:53 AM Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > > > > > On Sat, Jan 2, 2021 at 3:37 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > > > > > On Thu, 31 Dec 2020 21:12:40 +0000, > > > > > > > Rob Herring <robh@kernel.org> 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@kernel.org> > > > > > > > > > > Cc: Kevin Hilman <khilman@baylibre.com> > > > > > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > > > > > > --- > > > > > > > > > > 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. > > > > > > > > Do all interrupt controllers (that are not the root interrupt > > > > controller) need to have "interrupts" property? If yes, then yeah, > > > > that makes sense. But I vaguely remember that this wasn't the case for > > > > some DT I saw. > > > > > > There are some cases of stacked controllers where it's implicit. > > > > In that case, I think it's good to track interrupt-parent explicitly. > > Doesn't really hurt anything. We already protect for stuff like making > > sure a parent doesn't depend on its child, etc. > > Given the interrupt controller is probably always going to be there, > it probably doesn't really matter. But the dependency is wrong. > > Does this work: > > bus { > interrupt-parent = <&root_int>; > > root_int: interrupt-controller { > }; > > device { > interrupts = <1>; > }; > }; > > The only dependency here should be /bus/device -> root_int. Right, and that's the only one that'll get created because a parent (bus) can't depend on a child (root_int). There are other similar things that'll effectively make the interrupt-parent handling harmless and also cover the few odd cases like the one I pointed out. > > > > > > > > Ah, here's one I found. > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/mt2701.dtsi#n209 > > > > > > Right, so this is one of several cases of custom interrupt mapping > > > properties (mediatek,ext-irq-range). Really, 'interrupts' or > > > 'interrupt-map' should have been used here, but 'interrupt-map' > > > doesn't really scale well if you have large ranges of interrupts. > > > > > > To handle the dependency with just 'interrupt-parent', you need to > > > find nodes that are themselves an 'interrupt-parent' and then find > > > their 'interrupt-parent'. > > > > Not sure I understand this. > > > > On a side note, if I'm adding device links between a device and the > > "interrupt-parent" it's pointing to, at worst, I'm having it depend on > > an interrupt controller its child devices would depend on. This > > combined with the fact that "weird" links like "parent depending on > > child", > > The above would be this case if you treat root_int as a dependency of > bus. It's not, but I guess that plus the child dependency on parent > makes interrupt dependencies mostly work. > > > "non-device node having interrupt-parent", > > Yes, that's probably 95% of interrupt-parent cases. It's either at the > root or bus node. > > > etc are already > > ignored, seems safe to leave in "interrupt-parent" to catch these > > cases where interrupt controllers don't specify "interrupts" or > > "interrupt-map"? I don't mind removing it, but maybe we can wait till > > we get fw_devlink=on and then remove it to see if anything breaks? If > > nothing breaks, we can remove explicit interrupt-parent parsing? > > Okay, as long as we're breaking platforms once potentially. I'm not sure if the sentence needs a negative somewhere in there, but I believe we are in agreement. > > Going back to interrupt-map, I understand the syntax now. I'm trying > > to see if I can break up of_irq_parse_raw() into smaller pieces and > > reuse (call into) some of that code. While doing that I see that when > > "address-cells" isn't present, the "addrsize" is initialized to 2 [1] > > but when "address-cells" isn't present, the "newaddrsize" is > > initialized to 0 [1]. Why is the default value of #address-cells two > > different numbers? > > It's some ancient code that needs to handle quirks of ancient systems. > > The first one is the #address-cells for the device's bus. The second > is #address-cells in the interrupt-controller/map. It's complicated > because interrupt hierarchies used to follow the bus hierarchies. > Generally in modern systems #address-cells used for interrupt-map > purposes (the 2nd case) will always be 0. > > Note that there's generic nexus code to support gpio-map. It works > similarly though without the address-cells part. Any refactoring > should take that into account. Ok, thanks for the info. I'll refactor out the common code and try to use that to implement interrupt-map support. -Saravana ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2020-12-18 21:07 [PATCH] of: property: Add device link support for interrupts Saravana Kannan 2020-12-21 9:30 ` Marc Zyngier @ 2021-01-20 9:53 ` Geert Uytterhoeven 2021-01-20 10:04 ` Marc Zyngier 2021-01-20 14:28 ` Geert Uytterhoeven 1 sibling, 2 replies; 20+ messages in thread From: Geert Uytterhoeven @ 2021-01-20 9:53 UTC (permalink / raw) To: Saravana Kannan Cc: Greg Kroah-Hartman, Rob Herring, Frank Rowand, Marc Zyngier, Kevin Hilman, Android Kernel Team, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List Hi Saravana, On Fri, Dec 18, 2020 at 10:11 PM Saravana Kannan <saravanak@google.com> wrote: > Add support for creating device links out of interrupts property. > > Cc: Marc Zyngier <maz@kernel.org> > Cc: Kevin Hilman <khilman@baylibre.com> > Signed-off-by: Saravana Kannan <saravanak@google.com> Thanks for your patch! This does not seem to add all links. I see links being created to the secondary interrupt controller (e61c0000 "renesas,irqc"), but not to the primary interrupt controller (GIC) Which is good, as the GIC driver is not a platform_driver, and thus creating links would break everything ;-) BTW, I'd _love_ the GIC driver to be a platform_driver, as the GIC is part of a power/and or clock domain on Renesas SoCs... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2021-01-20 9:53 ` Geert Uytterhoeven @ 2021-01-20 10:04 ` Marc Zyngier 2021-01-20 14:28 ` Geert Uytterhoeven 1 sibling, 0 replies; 20+ messages in thread From: Marc Zyngier @ 2021-01-20 10:04 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Saravana Kannan, Greg Kroah-Hartman, Rob Herring, Frank Rowand, Kevin Hilman, Android Kernel Team, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List On 2021-01-20 09:53, Geert Uytterhoeven wrote: > Hi Saravana, > > On Fri, Dec 18, 2020 at 10:11 PM Saravana Kannan <saravanak@google.com> > wrote: >> Add support for creating device links out of interrupts property. >> >> Cc: Marc Zyngier <maz@kernel.org> >> Cc: Kevin Hilman <khilman@baylibre.com> >> Signed-off-by: Saravana Kannan <saravanak@google.com> > > Thanks for your patch! > > This does not seem to add all links. I see links being created to the > secondary interrupt controller (e61c0000 "renesas,irqc"), but not to > the primary interrupt controller (GIC) > > Which is good, as the GIC driver is not a platform_driver, and thus > creating links would break everything ;-) > > BTW, I'd _love_ the GIC driver to be a platform_driver, as the GIC is > part of a power/and or clock domain on Renesas SoCs... The trouble is that we need the GIC much earlier than the device model is available, because the timer needs to be available super early. This isn't specific to the GIC, by the way, but also to all root interrupt controllers that end-up controlling the per-CPU interrupts. If you try to relax this constraint, you'll end up observing some of the very weird dependencies between sysfs, sched, and the device model (I went there a few years back, wasted a week on it, did a reset --hard on the branch, and swore never to look at this again! ;-) But for a start, I'd like the ITS code to be turned into a platform driver, as this would potentially allow for the various domains to be instantiated dynamically. One day. M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2021-01-20 9:53 ` Geert Uytterhoeven 2021-01-20 10:04 ` Marc Zyngier @ 2021-01-20 14:28 ` Geert Uytterhoeven 2021-01-20 17:15 ` Saravana Kannan 1 sibling, 1 reply; 20+ messages in thread From: Geert Uytterhoeven @ 2021-01-20 14:28 UTC (permalink / raw) To: Saravana Kannan Cc: Greg Kroah-Hartman, Rob Herring, Frank Rowand, Marc Zyngier, Kevin Hilman, Android Kernel Team, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List Hi Saravana, On Wed, Jan 20, 2021 at 10:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Fri, Dec 18, 2020 at 10:11 PM Saravana Kannan <saravanak@google.com> wrote: > > Add support for creating device links out of interrupts property. > > > > Cc: Marc Zyngier <maz@kernel.org> > > Cc: Kevin Hilman <khilman@baylibre.com> > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > Thanks for your patch! > > This does not seem to add all links. I see links being created to the > secondary interrupt controller (e61c0000 "renesas,irqc"), but not to > the primary interrupt controller (GIC) > > Which is good, as the GIC driver is not a platform_driver, and thus > creating links would break everything ;-) of_link_to_phandle() ignores device nodes where OF_POPULATED is set, and of_irq_init() sets that flag. Hence the GIC is ignored. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts 2021-01-20 14:28 ` Geert Uytterhoeven @ 2021-01-20 17:15 ` Saravana Kannan 0 siblings, 0 replies; 20+ messages in thread From: Saravana Kannan @ 2021-01-20 17:15 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Greg Kroah-Hartman, Rob Herring, Frank Rowand, Marc Zyngier, Kevin Hilman, Android Kernel Team, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List On Wed, Jan 20, 2021 at 6:28 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Saravana, > > On Wed, Jan 20, 2021 at 10:53 AM Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > > On Fri, Dec 18, 2020 at 10:11 PM Saravana Kannan <saravanak@google.com> wrote: > > > Add support for creating device links out of interrupts property. > > > > > > Cc: Marc Zyngier <maz@kernel.org> > > > Cc: Kevin Hilman <khilman@baylibre.com> > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > Thanks for your patch! > > > > This does not seem to add all links. I see links being created to the > > secondary interrupt controller (e61c0000 "renesas,irqc"), but not to > > the primary interrupt controller (GIC) > > > > Which is good, as the GIC driver is not a platform_driver, and thus > > creating links would break everything ;-) > > of_link_to_phandle() ignores device nodes where OF_POPULATED > is set, and of_irq_init() sets that flag. Hence the GIC is ignored. Geert, Yes, I know :) I wrote it that way for GIC and some of the early devices that can't use the driver core. And as Marc said in the other email, it looks like GIC is going to be stuck as a non-device for a while. But this patch and others that'll follow are all set up for allowing non-root interrupt controllers to be implemented as platform devices (or <anybus> device). -Saravana ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] of: property: Add device link support for interrupts @ 2020-12-19 6:19 kernel test robot 0 siblings, 0 replies; 20+ messages in thread From: kernel test robot @ 2020-12-19 6:19 UTC (permalink / raw) To: kbuild [-- Attachment #1: Type: text/plain, Size: 2948 bytes --] CC: kbuild-all(a)lists.01.org In-Reply-To: <20201218210750.3455872-1-saravanak@google.com> References: <20201218210750.3455872-1-saravanak@google.com> TO: Saravana Kannan <saravanak@google.com> TO: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> TO: Rob Herring <robh+dt@kernel.org> TO: Frank Rowand <frowand.list@gmail.com> CC: Saravana Kannan <saravanak@google.com> CC: Marc Zyngier <maz@kernel.org> CC: Kevin Hilman <khilman@baylibre.com> CC: kernel-team(a)android.com CC: devicetree(a)vger.kernel.org CC: linux-kernel(a)vger.kernel.org Hi Saravana, I love your patch! Perhaps something to improve: [auto build test WARNING on robh/for-next] [also build test WARNING on v5.10 next-20201218] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Saravana-Kannan/of-property-Add-device-link-support-for-interrupts/20201219-051015 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next :::::: branch date: 9 hours ago :::::: commit date: 9 hours ago config: i386-randconfig-m021-20201217 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> smatch warnings: drivers/of/property.c:1335 parse_interrupts() error: uninitialized symbol 'sup'. vim +/sup +1335 drivers/of/property.c e149573b2f84d05 Will Deacon 2019-11-20 1321 12bb2bcf0f092e2 Saravana Kannan 2020-12-18 1322 static struct device_node *parse_interrupts(struct device_node *np, 12bb2bcf0f092e2 Saravana Kannan 2020-12-18 1323 const char *prop_name, int index) 12bb2bcf0f092e2 Saravana Kannan 2020-12-18 1324 { 12bb2bcf0f092e2 Saravana Kannan 2020-12-18 1325 struct device_node *sup; 12bb2bcf0f092e2 Saravana Kannan 2020-12-18 1326 12bb2bcf0f092e2 Saravana Kannan 2020-12-18 1327 if (strcmp(prop_name, "interrupts") || index) 12bb2bcf0f092e2 Saravana Kannan 2020-12-18 1328 return NULL; 12bb2bcf0f092e2 Saravana Kannan 2020-12-18 1329 12bb2bcf0f092e2 Saravana Kannan 2020-12-18 1330 of_node_get(np); 12bb2bcf0f092e2 Saravana Kannan 2020-12-18 1331 while (np && !(sup = of_parse_phandle(np, "interrupt-parent", 0))) 12bb2bcf0f092e2 Saravana Kannan 2020-12-18 1332 np = of_get_next_parent(np); 12bb2bcf0f092e2 Saravana Kannan 2020-12-18 1333 of_node_put(np); 12bb2bcf0f092e2 Saravana Kannan 2020-12-18 1334 12bb2bcf0f092e2 Saravana Kannan 2020-12-18 @1335 return sup; 12bb2bcf0f092e2 Saravana Kannan 2020-12-18 1336 } 12bb2bcf0f092e2 Saravana Kannan 2020-12-18 1337 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 32980 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2021-01-20 17:17 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-18 21:07 [PATCH] of: property: Add device link support for interrupts Saravana Kannan 2020-12-21 9:30 ` Marc Zyngier 2020-12-31 21:12 ` Rob Herring 2021-01-02 11:36 ` Marc Zyngier 2021-01-06 18:52 ` Saravana Kannan 2021-01-07 1:25 ` Saravana Kannan 2021-01-07 16:47 ` Rob Herring 2021-01-07 17:08 ` Saravana Kannan 2021-01-07 17:17 ` Marc Zyngier 2021-01-07 18:39 ` Rob Herring 2021-01-07 19:09 ` Saravana Kannan 2021-01-07 19:33 ` Rob Herring 2021-01-07 23:13 ` Saravana Kannan 2021-01-08 3:35 ` Rob Herring 2021-01-08 4:34 ` Saravana Kannan 2021-01-20 9:53 ` Geert Uytterhoeven 2021-01-20 10:04 ` Marc Zyngier 2021-01-20 14:28 ` Geert Uytterhoeven 2021-01-20 17:15 ` Saravana Kannan 2020-12-19 6:19 kernel test robot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.