[PATCH V11 02/12] PCI: host-generic: Add common helpers for parsing Root Port properties
Sherry Sun
sherry.sun at nxp.com
Tue Apr 7 23:34:02 PDT 2026
> On Tue, Apr 07, 2026 at 06:41:44PM +0800, Sherry Sun wrote:
> > Introduce generic helper functions to parse Root Port device tree
> > nodes and extract common properties like reset GPIOs. This allows
> > multiple PCI host controller drivers to share the same parsing logic.
> >
> > Define struct pci_host_port to hold common Root Port properties
> > (currently only reset GPIO descriptor) and add
> > pci_host_common_parse_ports() to parse Root Port nodes from device
> tree.
> >
> > Also add the 'ports' list to struct pci_host_bridge for better
> > maintain parsed Root Port information.
> >
> > Signed-off-by: Sherry Sun <sherry.sun at nxp.com>
> > ---
> > drivers/pci/controller/pci-host-common.c | 77
> > ++++++++++++++++++++++++ drivers/pci/controller/pci-host-common.h |
> 16 +++++
> > drivers/pci/probe.c | 1 +
> > include/linux/pci.h | 1 +
> > 4 files changed, 95 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pci-host-common.c
> > b/drivers/pci/controller/pci-host-common.c
> > index d6258c1cffe5..0fb6991dde7b 100644
> > --- a/drivers/pci/controller/pci-host-common.c
> > +++ b/drivers/pci/controller/pci-host-common.c
> > @@ -9,6 +9,7 @@
> >
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > +#include <linux/gpio/consumer.h>
> > #include <linux/of.h>
> > #include <linux/of_address.h>
> > #include <linux/of_pci.h>
> > @@ -17,6 +18,82 @@
> >
> > #include "pci-host-common.h"
> >
> > +/**
> > + * pci_host_common_delete_ports - Cleanup function for port list
> > + * @data: Pointer to the port list head */ void
> > +pci_host_common_delete_ports(void *data) {
> > + struct list_head *ports = data;
> > + struct pci_host_port *port, *tmp;
> > +
> > + list_for_each_entry_safe(port, tmp, ports, list)
> > + list_del(&port->list);
> > +}
> > +EXPORT_SYMBOL_GPL(pci_host_common_delete_ports);
> > +
> > +/**
> > + * pci_host_common_parse_port - Parse a single Root Port node
> > + * @dev: Device pointer
> > + * @bridge: PCI host bridge
> > + * @node: Device tree node of the Root Port
> > + *
> > + * Returns: 0 on success, negative error code on failure */ static
> > +int pci_host_common_parse_port(struct device *dev,
> > + struct pci_host_bridge *bridge,
> > + struct device_node *node)
> > +{
> > + struct pci_host_port *port;
> > + struct gpio_desc *reset;
> > +
> > + reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
> > + "reset", GPIOD_ASIS, "PERST#");
>
> Sorry, I missed this earlier.
>
> Since PERST# is optional, you cannot reliably detect whether the Root Port
> binding intentionally skipped the PERST# GPIO or legacy binding is used, just
> by checking for PERST# in Root Port node.
>
> So this helper should do 3 things:
>
> 1. If PERST# is found in Root Port node, use it.
> 2. If not, check the RC node and if present, return -ENOENT to fallback to the
> legacy binding.
> 3. If not found in both nodes, assume that the PERST# is not present in the
> design, and proceed with parsing Root Port binding further.
Hi Mani, understand, does the following code looks ok for above three cases?
/* Check if PERST# is present in Root Port node */
reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
"reset", GPIOD_ASIS, "PERST#");
if (IS_ERR(reset)) {
/* If error is not -ENOENT, it's a real error */
if (PTR_ERR(reset) != -ENOENT)
return PTR_ERR(reset);
/* PERST# not found in Root Port node, check RC node */
rc_has_reset = of_property_read_bool(dev->of_node, "reset-gpios") ||
of_property_read_bool(dev->of_node, "reset-gpio");
if (rc_has_reset)
return -ENOENT;
/* No PERST# in either node, assume not present in design */
reset = NULL;
}
port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
if (!port)
return -ENOMEM;
...
>
> But there is one more important limitation here. Right now, this API only
> handles PERST#. But if another vendor tries to use it and if they need other
> properties such as PHY, clocks etc... those resources should be fetched
> optionally only by this helper. But if the controller has a hard dependency on
> those resources, the driver will fail to operate.
>
> I don't think we can fix this limitation though and those platforms should
> ensure that the resource dependency is correctly modeled in DT binding and
> the DTS is validated properly. It'd be good to mention this in the kernel doc of
> this API.
Ok, I will add a NOTE for this in this API description.
>
> > + if (IS_ERR(reset))
> > + return PTR_ERR(reset);
> > +
> > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > + if (!port)
> > + return -ENOMEM;
> > +
> > + port->reset = reset;
> > + INIT_LIST_HEAD(&port->list);
> > + list_add_tail(&port->list, &bridge->ports);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * pci_host_common_parse_ports - Parse Root Port nodes from device
> > +tree
> > + * @dev: Device pointer
> > + * @bridge: PCI host bridge
> > + *
> > + * This function iterates through child nodes of the host bridge and
> > +parses
> > + * Root Port properties (currently only reset GPIO).
> > + *
> > + * Returns: 0 on success, -ENOENT if no ports found, other negative
> > +error codes
> > + * on failure
> > + */
> > +int pci_host_common_parse_ports(struct device *dev, struct
> > +pci_host_bridge *bridge) {
> > + int ret = -ENOENT;
> > +
> > + for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> > + if (!of_node_is_type(of_port, "pci"))
> > + continue;
> > + ret = pci_host_common_parse_port(dev, bridge, of_port);
> > + if (ret)
> > + return ret;
>
> As Sashiko flagged, you need to make sure that devm_add_action_or_reset()
> is added even during the error path:
Yes, it needs to be fixed. We can handle it with the following two methods, I am not sure which method is better or more preferable?
#1: register cleanup action after first successful port parse and use cleanup_registered flag to avoid duplicate register.
int ret = -ENOENT;
bool cleanup_registered = false;
for_each_available_child_of_node_scoped(dev->of_node, of_port) {
if (!of_node_is_type(of_port, "pci"))
continue;
ret = pci_host_common_parse_port(dev, bridge, of_port);
if (ret)
return ret;
/* Register cleanup action after first successful port parse */
if (!cleanup_registered) {
ret = devm_add_action_or_reset(dev,
pci_host_common_delete_ports,
&bridge->ports);
if (ret)
return ret;
cleanup_registered = true;
}
}
#2: call devm_add_action to register cleanup action before the loop begins.
ret = devm_add_action(dev, pci_host_common_delete_ports,
&bridge->ports);
if (ret)
return ret;
ret = -ENOENT;
for_each_available_child_of_node_scoped(dev->of_node, of_port) {
if (!of_node_is_type(of_port, "pci"))
continue;
ret = pci_host_common_parse_port(dev, bridge, of_port);
if (ret)
return ret;
}
Best Regards
Sherry
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsashiko
> .dev%2F%23%2Fpatchset%2F20260407104154.2842132-1-
> sherry.sun%2540nxp.com%3Fpart%3D2&data=05%7C02%7Csherry.sun%40nx
> p.com%7Ca19d6997cb63454afd7808de94a961fe%7C686ea1d3bc2b4c6fa92cd
> 99c5c301635%7C0%7C0%7C639111652420710900%7CUnknown%7CTWFpbG
> Zsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiI
> sIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=P4Vz%2B7kH
> i07bzBnR1w4smYzRWDKPbzQsEcJXqEGyzP4%3D&reserved=0
>
> - Mani
>
> > + }
> > +
> > + if (ret)
> > + return ret;
> > +
> > + return devm_add_action_or_reset(dev,
> pci_host_common_delete_ports,
> > + &bridge->ports);
> > +}
> > +EXPORT_SYMBOL_GPL(pci_host_common_parse_ports);
> > +
> > static void gen_pci_unmap_cfg(void *ptr) {
> > pci_ecam_free((struct pci_config_window *)ptr); diff --git
> > a/drivers/pci/controller/pci-host-common.h
> > b/drivers/pci/controller/pci-host-common.h
> > index b5075d4bd7eb..37714bedb625 100644
> > --- a/drivers/pci/controller/pci-host-common.h
> > +++ b/drivers/pci/controller/pci-host-common.h
> > @@ -12,6 +12,22 @@
> >
> > struct pci_ecam_ops;
> >
> > +/**
> > + * struct pci_host_port - Generic Root Port properties
> > + * @list: List node for linking multiple ports
> > + * @reset: GPIO descriptor for PERST# signal
> > + *
> > + * This structure contains common properties that can be parsed from
> > + * Root Port device tree nodes.
> > + */
> > +struct pci_host_port {
> > + struct list_head list;
> > + struct gpio_desc *reset;
> > +};
> > +
> > +void pci_host_common_delete_ports(void *data); int
> > +pci_host_common_parse_ports(struct device *dev, struct
> > +pci_host_bridge *bridge);
> > +
> > int pci_host_common_probe(struct platform_device *pdev); int
> > pci_host_common_init(struct platform_device *pdev,
> > struct pci_host_bridge *bridge,
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index
> > eaa4a3d662e8..629ae08b7d35 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -677,6 +677,7 @@ static void pci_init_host_bridge(struct
> > pci_host_bridge *bridge) {
> > INIT_LIST_HEAD(&bridge->windows);
> > INIT_LIST_HEAD(&bridge->dma_ranges);
> > + INIT_LIST_HEAD(&bridge->ports);
> >
> > /*
> > * We assume we can manage these PCIe features. Some systems
> may
> > diff --git a/include/linux/pci.h b/include/linux/pci.h index
> > 8f63de38f2d2..a73ea81ce88f 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -636,6 +636,7 @@ struct pci_host_bridge {
> > int domain_nr;
> > struct list_head windows; /* resource_entry */
> > struct list_head dma_ranges; /* dma ranges resource list */
> > + struct list_head ports; /* Root Port list (pci_host_port) */
> > #ifdef CONFIG_PCI_IDE
> > u16 nr_ide_streams; /* Max streams possibly active in
> @ide_stream_ida */
> > struct ida ide_stream_ida;
> > --
> > 2.37.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்
More information about the linux-arm-kernel
mailing list