Re: [PATCH v2 1/5] PCI: qcom: Parse PERST# from all PCIe bridge nodes

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Dec 16, 2025 at 06:21:43PM +0530, Manivannan Sadhasivam wrote:
> Devicetree schema allows the PERST# GPIO to be present in all PCIe bridge
> nodes, not just in Root Port node. But the current logic parses PERST# only
> from the Root Port nodes. Though it is not causing any issue on the current
> platforms, the upcoming platforms will have PERST# in PCIe switch
> downstream ports also. So this requires parsing all the PCIe bridge nodes
> for the PERST# GPIO.
> 
> Hence, rework the parsing logic to extend to all PCIe bridge nodes starting
> from the Root Port node. If the 'reset-gpios' property is found for a PCI
> bridge node, the GPIO descriptor will be stored in qcom_pcie_perst::desc
> and added to the qcom_pcie_port::perst list.

>  static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
>  {
> +	struct qcom_pcie_perst *perst;
>  	struct qcom_pcie_port *port;
>  	int val = assert ? 1 : 0;
>  
> -	list_for_each_entry(port, &pcie->ports, list)
> -		gpiod_set_value_cansleep(port->reset, val);
> +	list_for_each_entry(port, &pcie->ports, list) {
> +		list_for_each_entry(perst, &port->perst, list)
> +			gpiod_set_value_cansleep(perst->desc, val);
> +	}
>  
>  	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
>  }
> @@ -1702,18 +1710,58 @@ static const struct pci_ecam_ops pci_qcom_ecam_ops = {
>  	}
>  };
>  
> -static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node)
> +/* Parse PERST# from all nodes in depth first manner starting from @np */
> +static int qcom_pcie_parse_perst(struct qcom_pcie *pcie,
> +				 struct qcom_pcie_port *port,
> +				 struct device_node *np)
>  {
>  	struct device *dev = pcie->pci->dev;
> -	struct qcom_pcie_port *port;
> +	struct qcom_pcie_perst *perst;
>  	struct gpio_desc *reset;
> -	struct phy *phy;
>  	int ret;
>  
> -	reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
> -				      "reset", GPIOD_OUT_HIGH, "PERST#");
> -	if (IS_ERR(reset))
> +	if (!of_find_property(np, "reset-gpios", NULL))
> +		goto parse_child_node;
> +
> +	reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(np), "reset",
> +				      GPIOD_OUT_HIGH, "PERST#");
> +	if (IS_ERR(reset)) {
> +		/*
> +		 * FIXME: GPIOLIB currently supports exclusive GPIO access only.
> +		 * Non exclusive access is broken. But shared PERST# requires
> +		 * non-exclusive access. So once GPIOLIB properly supports it,
> +		 * implement it here.
> +		 */
> +		if (PTR_ERR(reset) == -EBUSY)
> +			dev_err(dev, "Shared PERST# is not supported\n");
> +
>  		return PTR_ERR(reset);
> +	}
> +
> +	perst = devm_kzalloc(dev, sizeof(*perst), GFP_KERNEL);
> +	if (!perst)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&perst->list);
> +	perst->desc = reset;
> +	list_add_tail(&perst->list, &port->perst);
> +
> +parse_child_node:
> +	for_each_available_child_of_node_scoped(np, child) {
> +		ret = qcom_pcie_parse_perst(pcie, port, child);

It looks like the perst->list will be ordered by distance from the
root, i.e., a Root Port first, followed by downstream devices?

And qcom_perst_assert() will assert/deassert PERST# in that same
order?  Intuitively I would have expected that if there are multiple
PERST# signals, we would assert them bottom-up, and deassert them
top-down.  Does the order matter?

I suppose maybe you plan to enhance pwrctrl so it can assert/deassert
individual PERST# in the hierarchy?

I'm confused about qcom_perst_assert() because it's only called from
qcom_ep_reset_assert() and qcom_ep_reset_deassert(), which are only
called from qcom_pcie_assert_perst().  Seems like a mix of host and
endpoint situation.  I assumed pwrctrl would be used on the host.
Maybe the "_ep_" names are not quite right?  Or more likely I'm just
misunderstanding the plan.

I notice you'd only applied this patch (1/5) so far on
pci/controller/dwc-qcom.  Is this patch useful by itself?

> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}