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.

It should be noted that if more than one bridge node has the same GPIO for
PERST# (shared PERST#), the driver will error out. This is due to the
limitation in the GPIOLIB subsystem that allows only exclusive (non-shared)
access to GPIOs from consumers. But this is soon going to get fixed. Once
that happens, it will get incorporated in this driver.

So for now, PERST# sharing is not supported.

Tested-by: Chen-Yu Tsai <wenst@chromium.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 102 +++++++++++++++++++++++++++------
 1 file changed, 85 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 7b92e7a1c0d9..73032449d289 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -267,10 +267,15 @@ struct qcom_pcie_cfg {
 	bool no_l0s;
 };
 
+struct qcom_pcie_perst {
+	struct list_head list;
+	struct gpio_desc *desc;
+};
+
 struct qcom_pcie_port {
 	struct list_head list;
-	struct gpio_desc *reset;
 	struct phy *phy;
+	struct list_head perst;
 };
 
 struct qcom_pcie {
@@ -291,11 +296,14 @@ struct qcom_pcie {
 
 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);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node)
+{
+	struct device *dev = pcie->pci->dev;
+	struct qcom_pcie_port *port;
+	struct phy *phy;
+	int ret;
 
 	phy = devm_of_phy_get(dev, node, NULL);
 	if (IS_ERR(phy))
@@ -1727,7 +1775,12 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
 	if (ret)
 		return ret;
 
-	port->reset = reset;
+	INIT_LIST_HEAD(&port->perst);
+
+	ret = qcom_pcie_parse_perst(pcie, port, node);
+	if (ret)
+		return ret;
+
 	port->phy = phy;
 	INIT_LIST_HEAD(&port->list);
 	list_add_tail(&port->list, &pcie->ports);
@@ -1737,9 +1790,10 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
 
 static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 {
+	struct qcom_pcie_perst *perst, *tmp_perst;
+	struct qcom_pcie_port *port, *tmp_port;
 	struct device *dev = pcie->pci->dev;
-	struct qcom_pcie_port *port, *tmp;
-	int ret = -ENOENT;
+	int ret = -ENODEV;
 
 	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
 		if (!of_node_is_type(of_port, "pci"))
@@ -1752,7 +1806,9 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 	return ret;
 
 err_port_del:
-	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+	list_for_each_entry_safe(port, tmp_port, &pcie->ports, list) {
+		list_for_each_entry_safe(perst, tmp_perst, &port->perst, list)
+			list_del(&perst->list);
 		phy_exit(port->phy);
 		list_del(&port->list);
 	}
@@ -1763,6 +1819,7 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
 {
 	struct device *dev = pcie->pci->dev;
+	struct qcom_pcie_perst *perst;
 	struct qcom_pcie_port *port;
 	struct gpio_desc *reset;
 	struct phy *phy;
@@ -1784,19 +1841,28 @@ static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
 	if (!port)
 		return -ENOMEM;
 
-	port->reset = reset;
+	perst = devm_kzalloc(dev, sizeof(*perst), GFP_KERNEL);
+	if (!perst)
+		return -ENOMEM;
+
 	port->phy = phy;
 	INIT_LIST_HEAD(&port->list);
 	list_add_tail(&port->list, &pcie->ports);
 
+	perst->desc = reset;
+	INIT_LIST_HEAD(&port->perst);
+	INIT_LIST_HEAD(&perst->list);
+	list_add_tail(&perst->list, &port->perst);
+
 	return 0;
 }
 
 static int qcom_pcie_probe(struct platform_device *pdev)
 {
+	struct qcom_pcie_perst *perst, *tmp_perst;
+	struct qcom_pcie_port *port, *tmp_port;
 	const struct qcom_pcie_cfg *pcie_cfg;
 	unsigned long max_freq = ULONG_MAX;
-	struct qcom_pcie_port *port, *tmp;
 	struct device *dev = &pdev->dev;
 	struct dev_pm_opp *opp;
 	struct qcom_pcie *pcie;
@@ -1937,7 +2003,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	ret = qcom_pcie_parse_ports(pcie);
 	if (ret) {
-		if (ret != -ENOENT) {
+		if (ret != -ENODEV) {
 			dev_err_probe(pci->dev, ret,
 				      "Failed to parse Root Port: %d\n", ret);
 			goto err_pm_runtime_put;
@@ -1996,7 +2062,9 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 err_host_deinit:
 	dw_pcie_host_deinit(pp);
 err_phy_exit:
-	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+	list_for_each_entry_safe(port, tmp_port, &pcie->ports, list) {
+		list_for_each_entry_safe(perst, tmp_perst, &port->perst, list)
+			list_del(&perst->list);
 		phy_exit(port->phy);
 		list_del(&port->list);
 	}

-- 
2.48.1

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

On Fri, Dec 26, 2025 at 05:24:58PM -0600, Bjorn Helgaas wrote:
> 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?
> 

Yes.

> 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 did't give much importance to the PERST# ordering since it doesn't matter,
atleast per base/electromechanical specs.

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

No, that plan has been dropped for good. For now, PERST# will be handled
entirely by the controller drivers. Sharing the PERST# handling with pwrctrl
proved to be a pain and it looks more clean (after the API introduction) to
handle PERST# in controller drivers.

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

Yeah, it is a bit confusing now due to the introduction of the
'dw_pcie_ops::assert_perst' callback. But it will go away at the end of the
pwrctrl rework series and I'll fix those names by then.

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

Yes, ofc. This patch allows the controller driver to parse and assert/deassert
PERST# from all PCI nodes, not just from the Root Port node.

- Mani

-- 
மணிவண்ணன் சதாசிவம்