| Message ID | 20240412112931.285507-1-apatel@ventanamicro.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | of: property: Add fw_devlink support for interrupt-map property | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
On Fri, Apr 12, 2024 at 6:29 AM Anup Patel <apatel@ventanamicro.com> wrote: > > Some of the PCI controllers (such as generic PCI host controller) > use "interrupt-map" DT property to describe the mapping between > PCI endpoints and PCI interrupt pins. I would go as far as saying that's the only case as that's the only case where the interrupts are not described in DT. > Currently, there is no fw_devlink created based on "interrupt-map" > DT property so interrupt controller is not guaranteed to be probed > before PCI host controller. This mainly affects RISC-V platforms > where both PCI host controller and interrupt controllers are probed > as regular platform devices. That's *every* system with PCI really. > This creates fw_devlink between consumers (PCI host controller) and > supplier (interrupt controller) based on "interrupt-map" DT property. > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > --- > drivers/of/property.c | 53 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index a6358ee99b74..ccbbb651a89a 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -1311,6 +1311,58 @@ static struct device_node *parse_interrupts(struct device_node *np, > return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np; > } > > +static struct device_node *parse_interrupt_map(struct device_node *np, > + const char *prop_name, int index) > +{ > + struct device_node *tn, *ipar, *supnp = NULL; > + u32 addrcells, intcells, cells; > + const __be32 *imap, *imap_end; > + int i, imaplen; > + > + if (!IS_ENABLED(CONFIG_OF_IRQ)) > + return NULL; > + > + if (strcmp(prop_name, "interrupt-map")) > + return NULL; > + > + ipar = of_node_get(np); > + do { > + if (!of_property_read_u32(ipar, "#interrupt-cells", &intcells)) > + break; > + tn = ipar; > + ipar = of_irq_find_parent(ipar); > + of_node_put(tn); > + } while (ipar); > + if (!ipar) > + return NULL; > + addrcells = of_bus_n_addr_cells(ipar); > + of_node_put(ipar); > + > + imap = of_get_property(np, "interrupt-map", &imaplen); > + if (!imap || imaplen <= (addrcells + intcells)) > + return NULL; > + imap_end = imap + imaplen; > + > + for (i = 0; i <= index && imap < imap_end; i++) { > + if (supnp) > + of_node_put(supnp); > + > + imap += addrcells; > + imap += intcells; > + > + supnp = of_find_node_by_phandle(be32_to_cpu(imap[0])); > + if (!supnp) > + return NULL; > + imap += 1; > + > + if (of_property_read_u32(supnp, "#interrupt-cells", &cells)) > + return NULL; > + imap += cells; This is wrong. Technically, you can have #address-cells too. The bigger problem I have is this creates 2 sets of 'interrupt-map' parsing code. Your version skips a lot of things like whether the interrupt controller is available and there's the list of 'interrupt-map' abusers to think about. Rob
On Fri, Apr 12, 2024 at 6:07 PM Rob Herring <robh@kernel.org> wrote: > > On Fri, Apr 12, 2024 at 6:29 AM Anup Patel <apatel@ventanamicro.com> wrote: > > > > Some of the PCI controllers (such as generic PCI host controller) > > use "interrupt-map" DT property to describe the mapping between > > PCI endpoints and PCI interrupt pins. > > I would go as far as saying that's the only case as that's the only > case where the interrupts are not described in DT. Sure, I will update the text. > > > Currently, there is no fw_devlink created based on "interrupt-map" > > DT property so interrupt controller is not guaranteed to be probed > > before PCI host controller. This mainly affects RISC-V platforms > > where both PCI host controller and interrupt controllers are probed > > as regular platform devices. > > That's *every* system with PCI really. Okay, I will re-word. > > > This creates fw_devlink between consumers (PCI host controller) and > > supplier (interrupt controller) based on "interrupt-map" DT property. > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > --- > > drivers/of/property.c | 53 +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 53 insertions(+) > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > index a6358ee99b74..ccbbb651a89a 100644 > > --- a/drivers/of/property.c > > +++ b/drivers/of/property.c > > @@ -1311,6 +1311,58 @@ static struct device_node *parse_interrupts(struct device_node *np, > > return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np; > > } > > > > +static struct device_node *parse_interrupt_map(struct device_node *np, > > + const char *prop_name, int index) > > +{ > > + struct device_node *tn, *ipar, *supnp = NULL; > > + u32 addrcells, intcells, cells; > > + const __be32 *imap, *imap_end; > > + int i, imaplen; > > + > > + if (!IS_ENABLED(CONFIG_OF_IRQ)) > > + return NULL; > > + > > + if (strcmp(prop_name, "interrupt-map")) > > + return NULL; > > + > > + ipar = of_node_get(np); > > + do { > > + if (!of_property_read_u32(ipar, "#interrupt-cells", &intcells)) > > + break; > > + tn = ipar; > > + ipar = of_irq_find_parent(ipar); > > + of_node_put(tn); > > + } while (ipar); > > + if (!ipar) > > + return NULL; > > + addrcells = of_bus_n_addr_cells(ipar); > > + of_node_put(ipar); > > + > > + imap = of_get_property(np, "interrupt-map", &imaplen); > > + if (!imap || imaplen <= (addrcells + intcells)) > > + return NULL; > > + imap_end = imap + imaplen; > > + > > + for (i = 0; i <= index && imap < imap_end; i++) { > > + if (supnp) > > + of_node_put(supnp); > > + > > + imap += addrcells; > > + imap += intcells; > > + > > + supnp = of_find_node_by_phandle(be32_to_cpu(imap[0])); > > + if (!supnp) > > + return NULL; > > + imap += 1; > > + > > + if (of_property_read_u32(supnp, "#interrupt-cells", &cells)) > > + return NULL; > > + imap += cells; > > This is wrong. Technically, you can have #address-cells too. > > The bigger problem I have is this creates 2 sets of 'interrupt-map' > parsing code. Your version skips a lot of things like whether the > interrupt controller is available and there's the list of > 'interrupt-map' abusers to think about. > In my first attempt, I was trying to keep it simple but I guess I over-simplified it. I have an alternate approach to use of_irq_parse_raw() over here (similar to of_irq_parse_pci() in drivers/pci/of.c). Are you okay with this approach ? Regards, Anup
diff --git a/drivers/of/property.c b/drivers/of/property.c index a6358ee99b74..ccbbb651a89a 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -1311,6 +1311,58 @@ static struct device_node *parse_interrupts(struct device_node *np, return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np; } +static struct device_node *parse_interrupt_map(struct device_node *np, + const char *prop_name, int index) +{ + struct device_node *tn, *ipar, *supnp = NULL; + u32 addrcells, intcells, cells; + const __be32 *imap, *imap_end; + int i, imaplen; + + if (!IS_ENABLED(CONFIG_OF_IRQ)) + return NULL; + + if (strcmp(prop_name, "interrupt-map")) + return NULL; + + ipar = of_node_get(np); + do { + if (!of_property_read_u32(ipar, "#interrupt-cells", &intcells)) + break; + tn = ipar; + ipar = of_irq_find_parent(ipar); + of_node_put(tn); + } while (ipar); + if (!ipar) + return NULL; + addrcells = of_bus_n_addr_cells(ipar); + of_node_put(ipar); + + imap = of_get_property(np, "interrupt-map", &imaplen); + if (!imap || imaplen <= (addrcells + intcells)) + return NULL; + imap_end = imap + imaplen; + + for (i = 0; i <= index && imap < imap_end; i++) { + if (supnp) + of_node_put(supnp); + + imap += addrcells; + imap += intcells; + + supnp = of_find_node_by_phandle(be32_to_cpu(imap[0])); + if (!supnp) + return NULL; + imap += 1; + + if (of_property_read_u32(supnp, "#interrupt-cells", &cells)) + return NULL; + imap += cells; + } + + return supnp; +} + static struct device_node *parse_remote_endpoint(struct device_node *np, const char *prop_name, int index) @@ -1359,6 +1411,7 @@ static const struct supplier_bindings of_supplier_bindings[] = { { .parse_prop = parse_msi_parent, }, { .parse_prop = parse_gpio_compat, }, { .parse_prop = parse_interrupts, }, + { .parse_prop = parse_interrupt_map, }, { .parse_prop = parse_regulators, }, { .parse_prop = parse_gpio, }, { .parse_prop = parse_gpios, },
Some of the PCI controllers (such as generic PCI host controller) use "interrupt-map" DT property to describe the mapping between PCI endpoints and PCI interrupt pins. Currently, there is no fw_devlink created based on "interrupt-map" DT property so interrupt controller is not guaranteed to be probed before PCI host controller. This mainly affects RISC-V platforms where both PCI host controller and interrupt controllers are probed as regular platform devices. This creates fw_devlink between consumers (PCI host controller) and supplier (interrupt controller) based on "interrupt-map" DT property. Signed-off-by: Anup Patel <apatel@ventanamicro.com> --- drivers/of/property.c | 53 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)