| Message ID | 20240413091942.316054-1-apatel@ventanamicro.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] of: property: Add fw_devlink support for interrupt-map property | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
On Sat, Apr 13, 2024 at 02:49:42PM +0530, Anup Patel 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. This 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 affects every platform 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> > --- > Changes since v1: > - Updated commit description based on Rob's suggestion > - Use of_irq_parse_raw() for parsing interrupt-map DT property > --- > drivers/of/property.c | 58 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 58 insertions(+) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index a6358ee99b74..67be66384dac 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -1311,6 +1311,63 @@ 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) > +{ > + const __be32 *imap, *imap_end, *addr; > + struct of_phandle_args sup_args; > + struct device_node *tn, *ipar; > + u32 addrcells, intcells; > + int i, j, 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); No need for this loop. We've only gotten here if 'interrupt-map' is present in the node and '#interrupt-cells' is required if 'interrupt-map' is present. > + 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; > + > + sup_args.np = NULL; > + for (i = 0; i <= index && imap < imap_end; i++) { > + if (sup_args.np) { > + of_node_put(sup_args.np); > + sup_args.np = NULL; > + } > + > + addr = imap; > + imap += addrcells; > + > + sup_args.np = np; > + sup_args.args_count = intcells; > + for (j = 0; j < intcells; j++) > + sup_args.args[j] = be32_to_cpu(imap[j]); > + imap += intcells; > + > + if (of_irq_parse_raw(addr, &sup_args)) > + return NULL; > + imap += sup_args.args_count + 1; > + } Doesn't this leak a ref on the last time the function is invoked? For example, if we have 2 entries and index is 2. We'll get index=1, but then exit because imap==imap_end. We need a put on index==1 node. Look at my next branch where I've converted things to use __free() cleanups. I don't see it helping here as-is, but maybe when it is correct. Rob
On Tue, Apr 16, 2024 at 7:11 AM Rob Herring <robh@kernel.org> wrote: > > On Sat, Apr 13, 2024 at 02:49:42PM +0530, Anup Patel 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. This the only case where > > the interrupts are not described in DT. > > > > Currently, there is no fw_devlink created based on "interrupt-map" > > DT property parse_interrupts() calls of_irq_parse_one() that in turn calls of_irq_parse_one() that does the "interrupts-map" handling. In fact of_irq_parse_pci() also calls of_irq_parse_one() if the PCI device has a DT node. So I don't think any new functionality needs to be added. If I'm not mistaken we just need parse_interrupts to not ignore interrupts-map? A one line change? Why do we need all of this code you wrote below? -Saravana > > so interrupt controller is not guaranteed to be probed > > before PCI host controller. This affects every platform 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> > > --- > > Changes since v1: > > - Updated commit description based on Rob's suggestion > > - Use of_irq_parse_raw() for parsing interrupt-map DT property > > --- > > drivers/of/property.c | 58 +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 58 insertions(+) > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > index a6358ee99b74..67be66384dac 100644 > > --- a/drivers/of/property.c > > +++ b/drivers/of/property.c > > @@ -1311,6 +1311,63 @@ 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) > > +{ > > + const __be32 *imap, *imap_end, *addr; > > + struct of_phandle_args sup_args; > > + struct device_node *tn, *ipar; > > + u32 addrcells, intcells; > > + int i, j, 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); > > No need for this loop. We've only gotten here if 'interrupt-map' is > present in the node and '#interrupt-cells' is required if > 'interrupt-map' is present. > > > + 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; > > + > > + sup_args.np = NULL; > > + for (i = 0; i <= index && imap < imap_end; i++) { > > + if (sup_args.np) { > > + of_node_put(sup_args.np); > > + sup_args.np = NULL; > > + } > > + > > + addr = imap; > > + imap += addrcells; > > + > > + sup_args.np = np; > > + sup_args.args_count = intcells; > > + for (j = 0; j < intcells; j++) > > + sup_args.args[j] = be32_to_cpu(imap[j]); > > + imap += intcells; > > + > > + if (of_irq_parse_raw(addr, &sup_args)) > > + return NULL; > > + imap += sup_args.args_count + 1; > > + } > > Doesn't this leak a ref on the last time the function is invoked? For > example, if we have 2 entries and index is 2. We'll get index=1, but > then exit because imap==imap_end. We need a put on index==1 node. > > Look at my next branch where I've converted things to use __free() > cleanups. I don't see it helping here as-is, but maybe when it is > correct. > > Rob
On Wed, Apr 17, 2024 at 6:34 AM Saravana Kannan <saravanak@google.com> wrote: > > On Tue, Apr 16, 2024 at 7:11 AM Rob Herring <robh@kernel.org> wrote: > > > > On Sat, Apr 13, 2024 at 02:49:42PM +0530, Anup Patel 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. This the only case where > > > the interrupts are not described in DT. > > > > > > Currently, there is no fw_devlink created based on "interrupt-map" > > > DT property > > parse_interrupts() calls of_irq_parse_one() that in turn calls > of_irq_parse_one() that does the "interrupts-map" handling. In fact > of_irq_parse_pci() also calls of_irq_parse_one() if the PCI device has > a DT node. So I don't think any new functionality needs to be added. > If I'm not mistaken we just need parse_interrupts to not ignore > interrupts-map? A one line change? > > Why do we need all of this code you wrote below? The of_irq_parse_one() calls of_irq_parse_raw() only if the DT node has "interrupts" or "interrupts-extended" DT property. This means for most PCI host controller DT nodes, the of_irq_parse_one() will not return any interrupts. Here's an example PCI host DT node from the RISC-V world (but this also applies to other architectures): pci@30000000 { compatible = "pci-host-ecam-generic"; device_type = "pci"; #address-cells = <0x03>; #size-cells = <0x02>; #interrupt-cells = <0x01>; interrupt-map-mask = <0x0 0x00 0x00 0x07>; interrupt-map = <0x00 0x00 0x00 0x01 &aplic_slevel 28 0x04>, <0x00 0x00 0x00 0x02 &aplic_slevel 29 0x04>, <0x00 0x00 0x00 0x03 &aplic_slevel 30 0x04>, <0x00 0x00 0x00 0x04 &aplic_slevel 31 0x04>; reg = <0x0 0x30000000 0x0 0x10000000>; ranges = <0x01000000 0x0 0x0 0x0 0x1fff0000 0x00000000 0x10000>, <0x02000000 0x0 0x40000000 0x0 0x40000000 0x0 0x40000000>, <0x03000000 0x100 0x0 0x100 0x0 0x4 0x0>; interrupt-parent = <&aplic_slevel>; msi-parent = <&imsic_slevel>; bus-range = <0x00 0xff>; linux,pci-domain = <0x00>; }; In the above example, the "interrupt-map" DT property maps a set of PCIe endpoints to APLIC interrupt sources but this only applies if the corresponding PCIe endpoints are actually present. Also, which entry of "interrupt-map" DT property is used for a PCIe endpoint also depends on the PCI requester ID (bus-device-func) assigned to the PCIe endpoint. Regards, Anup > > -Saravana > > > > so interrupt controller is not guaranteed to be probed > > > before PCI host controller. This affects every platform 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> > > > --- > > > Changes since v1: > > > - Updated commit description based on Rob's suggestion > > > - Use of_irq_parse_raw() for parsing interrupt-map DT property > > > --- > > > drivers/of/property.c | 58 +++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 58 insertions(+) > > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > > index a6358ee99b74..67be66384dac 100644 > > > --- a/drivers/of/property.c > > > +++ b/drivers/of/property.c > > > @@ -1311,6 +1311,63 @@ 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) > > > +{ > > > + const __be32 *imap, *imap_end, *addr; > > > + struct of_phandle_args sup_args; > > > + struct device_node *tn, *ipar; > > > + u32 addrcells, intcells; > > > + int i, j, 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); > > > > No need for this loop. We've only gotten here if 'interrupt-map' is > > present in the node and '#interrupt-cells' is required if > > 'interrupt-map' is present. > > > > > + 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; > > > + > > > + sup_args.np = NULL; > > > + for (i = 0; i <= index && imap < imap_end; i++) { > > > + if (sup_args.np) { > > > + of_node_put(sup_args.np); > > > + sup_args.np = NULL; > > > + } > > > + > > > + addr = imap; > > > + imap += addrcells; > > > + > > > + sup_args.np = np; > > > + sup_args.args_count = intcells; > > > + for (j = 0; j < intcells; j++) > > > + sup_args.args[j] = be32_to_cpu(imap[j]); > > > + imap += intcells; > > > + > > > + if (of_irq_parse_raw(addr, &sup_args)) > > > + return NULL; > > > + imap += sup_args.args_count + 1; > > > + } > > > > Doesn't this leak a ref on the last time the function is invoked? For > > example, if we have 2 entries and index is 2. We'll get index=1, but > > then exit because imap==imap_end. We need a put on index==1 node. > > > > Look at my next branch where I've converted things to use __free() > > cleanups. I don't see it helping here as-is, but maybe when it is > > correct. > > > > Rob
On Tue, Apr 16, 2024 at 7:41 PM Rob Herring <robh@kernel.org> wrote: > > On Sat, Apr 13, 2024 at 02:49:42PM +0530, Anup Patel 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. This 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 affects every platform 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> > > --- > > Changes since v1: > > - Updated commit description based on Rob's suggestion > > - Use of_irq_parse_raw() for parsing interrupt-map DT property > > --- > > drivers/of/property.c | 58 +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 58 insertions(+) > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > index a6358ee99b74..67be66384dac 100644 > > --- a/drivers/of/property.c > > +++ b/drivers/of/property.c > > @@ -1311,6 +1311,63 @@ 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) > > +{ > > + const __be32 *imap, *imap_end, *addr; > > + struct of_phandle_args sup_args; > > + struct device_node *tn, *ipar; > > + u32 addrcells, intcells; > > + int i, j, 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); > > No need for this loop. We've only gotten here if 'interrupt-map' is > present in the node and '#interrupt-cells' is required if > 'interrupt-map' is present. Ahh, okay. I will update. > > > + 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; > > + > > + sup_args.np = NULL; > > + for (i = 0; i <= index && imap < imap_end; i++) { > > + if (sup_args.np) { > > + of_node_put(sup_args.np); > > + sup_args.np = NULL; > > + } > > + > > + addr = imap; > > + imap += addrcells; > > + > > + sup_args.np = np; > > + sup_args.args_count = intcells; > > + for (j = 0; j < intcells; j++) > > + sup_args.args[j] = be32_to_cpu(imap[j]); > > + imap += intcells; > > + > > + if (of_irq_parse_raw(addr, &sup_args)) > > + return NULL; > > + imap += sup_args.args_count + 1; > > + } > > Doesn't this leak a ref on the last time the function is invoked? For > example, if we have 2 entries and index is 2. We'll get index=1, but > then exit because imap==imap_end. We need a put on index==1 node. Okay, I will update. > > Look at my next branch where I've converted things to use __free() > cleanups. I don't see it helping here as-is, but maybe when it is > correct. > Okay, I will check your next branch. Thanks, Anup
diff --git a/drivers/of/property.c b/drivers/of/property.c index a6358ee99b74..67be66384dac 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -1311,6 +1311,63 @@ 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) +{ + const __be32 *imap, *imap_end, *addr; + struct of_phandle_args sup_args; + struct device_node *tn, *ipar; + u32 addrcells, intcells; + int i, j, 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; + + sup_args.np = NULL; + for (i = 0; i <= index && imap < imap_end; i++) { + if (sup_args.np) { + of_node_put(sup_args.np); + sup_args.np = NULL; + } + + addr = imap; + imap += addrcells; + + sup_args.np = np; + sup_args.args_count = intcells; + for (j = 0; j < intcells; j++) + sup_args.args[j] = be32_to_cpu(imap[j]); + imap += intcells; + + if (of_irq_parse_raw(addr, &sup_args)) + return NULL; + imap += sup_args.args_count + 1; + } + + return sup_args.np; +} + static struct device_node *parse_remote_endpoint(struct device_node *np, const char *prop_name, int index) @@ -1359,6 +1416,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. This 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 affects every platform 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> --- Changes since v1: - Updated commit description based on Rob's suggestion - Use of_irq_parse_raw() for parsing interrupt-map DT property --- drivers/of/property.c | 58 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)