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

[Sherry Sun]

> Subject: Re: [PATCH V14 02/12] PCI: host-generic: Add common helpers for
> parsing Root Port properties
> 
> On Wed, Apr 22, 2026 at 05:35:39PM +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 list of PERST# GPIO descriptors) 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.
> > ...
> 
> > +static int pci_host_common_parse_port(struct device *dev,
> > +				      struct pci_host_bridge *bridge,
> > +				      struct device_node *node)
> > +{
> > +	struct pci_host_port *port;
> > +	int ret;
> > +
> > +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > +	if (!port)
> > +		return -ENOMEM;
> > +
> > +	INIT_LIST_HEAD(&port->perst);
> > +
> > +	ret = pci_host_common_parse_perst(dev, port, node);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * 1. PERST# found in RP or its child nodes - list is not empty, continue
> > +	 * 2. PERST# not found in RP/children, but found in RC node - return -
> ENODEV
> > +	 *    to fallback legacy binding
> > +	 * 3. PERST# not found anywhere - list is empty, continue (optional
> PERST#)
> > +	 */
> > +	if (list_empty(&port->perst)) {
> > +		if (of_property_present(dev->of_node, "reset-gpios") ||
> > +		    of_property_present(dev->of_node, "reset-gpio"))
> > +			return -ENODEV;
> 
> This doesn't seem right to me.  The parser of per-Root Port properties should
> not be responsible for deciding whether legacy methods are valid, i.e.,
> whether a property is in the Root Complex node.  I think it's up to the caller
> to decide whether it needs to look elsewhere.
> 
> I don't think this even needs to return a "success/failure" value because there
> may be more properties in the future, and not all will be required.  This
> function can't tell which properties a specific driver requires and which are
> optional.
> 
> The caller can check whether we found what it needs and fall back to a legacy
> method as needed.

Hi Bjorn,
The code here was suggested by Mani, https://lore.kernel.org/all/lnzprzrdwra7pn7d6m3sbj5pvjy64blwpjl6i3lmlnfbyho63b@czpyhpgz5vum/.
I think your suggestion here is reasonable, the per-Root Port parser shouldn't
check the RC-level binding. That's a policy decision that belongs to the caller.

Hi Mani, if you also agree, I'll rework this so that:
1. pci_host_common_parse_port() only parses properties from the Root Port
    (and its children) without checking the RC node.
2. The function won't return failure for "property not found" - it will only return
     errors for real failures (e.g., -ENOMEM, GPIO acquisition errors).
3. The legacy fallback logic will be moved to the caller, which can inspect the
     parsed result and decide whether to fall back to the legacy binding.

> 
> > +	}
> > +
> > +	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 GPIOs).
> > + *
> > + * Returns: 0 on success, -ENODEV if no ports found or PERST# found
> > +in RC node
> > + * (legacy binding should be used), Other negative error codes on failure.
> > + */
> > +int pci_host_common_parse_ports(struct device *dev, struct
> > +pci_host_bridge *bridge) {
> > +	int ret = -ENODEV;
> > +
> > +	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)
> > +			goto err_cleanup;
> > +	}
> 
> I think we should export pci_host_common_parse_port() itself and drop this
> so we deal with a single Root Port, and drivers that support multiple RPs
> should include their own loop similar to this.  That way the driver can do
> several things at once in each iteration of that loop, e.g., get resources, power
> up, configure, etc.
> 
> I see that would require some rework of the devm_add_action_or_reset()
> cleanup.

Thanks for the suggestion, make sense.
I'll export pci_host_common_parse_port() to deal with a single Root Port
and drop pci_host_common_parse_ports().

> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	return devm_add_action_or_reset(dev,
> pci_host_common_delete_ports,
> > +					&bridge->ports);
> > +
> > +err_cleanup:
> > +	pci_host_common_delete_ports(&bridge->ports);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_host_common_parse_ports);
> > ...
> 
> > + * struct pci_host_perst - PERST# GPIO descriptor
> > + * @list: List node for linking multiple PERST# GPIOs
> > + * @desc: GPIO descriptor for PERST# signal
> > + *
> > + * This structure holds a single PERST# GPIO descriptor.
> > + */
> > +struct pci_host_perst {
> > +	struct list_head	list;
> > +	struct gpio_desc	*desc;
> > +};
> 
> How do we associate an element of this list with something?
> 
> Based on the imx6 changes, I guess we don't; we don't even associate the
> pci_host_port with an RP.  We just assert/deassert PERST# for every RP at
> once, and we do it for every GPIO associated with each RP.
> 
> There's no way to assert PERST# for a single RP.  I guess we don't need that?

You are right. In the current design, we assert/deassert PERST# for all GPIOs across
all Root Ports simultaneously - there is no per-RP independent PERST# control.
And I think we don't need it for now, for most platforms, multiple RPs typically share
a single PERST# signal or require synchronized reset sequencing during initialization.
I am not aware of a practical use case where independent per-RP PERST# control is
required at the host controller driver level during probe/remove.

The per-port structure is a natural result of parsing per-RP DT nodes (each Root Port
child node maps to one pci_host_port, with PERST# GPIOs collected from the RP and
its downstream nodes). Even if such a requirement does arise in the future, since
the controller driver now owns the multi-RP loop, the driver that needs per-RP PERST#
control could associate each parsed port with its own per-RP context and operate on
specific ports independently.

For the i.MX case (single Root Port), there's no need for per-RP PERST# control -- we
just assert/deassert all PERST# GPIOs at once.

> 
> > +/**
> > + * struct pci_host_port - Generic Root Port properties
> > + * @list: List node for linking multiple ports
> > + * @perst: List of PERST# GPIO descriptors for this port and its
> > +children
> > + *
> > + * This structure contains common properties that can be parsed from
> > + * Root Port device tree nodes.
> > + */
> > +struct pci_host_port {
> 
> "host_port" is not really a standard term.  And despite the comments above
> and below, I don't think the list is restricted to Root Ports because we traverse
> the whole hierarchy below the RP.

Ok. How about struct pci_root_port_info to better reflect its purpose, I can also
highlight in the struct comment that it holds common properties parsed from a
Root Port device tree node and all PCIe bridge nodes under this Root Port (currently
only PERST# GPIOs). Or any other suggestions?

Best Regards
Sherry

> 
> > +	struct list_head	list;
> > +	struct list_head	perst;
> > +};
> 
> > +	struct list_head ports;		/* Root Port list (pci_host_port) */