RE: [PATCH V11 02/12] PCI: host-generic: Add common helpers for parsing Root Port properties

[Sherry Sun]

> 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@xxxxxxx>
> > ---
> >  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
> >
> 
> --
> மணிவண்ணன் சதாசிவம்