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

[Bjorn Helgaas <helgaas@kernel.org>]

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.

> +	}
> +
> +	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.

> +	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?

> +/**
> + * 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.

> +	struct list_head	list;
> +	struct list_head	perst;
> +};

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