[PATCH] PCI: qcom: Handle mixed PERST#/PHY DT configuration

[PATCH] PCI: qcom: Handle mixed PERST#/PHY DT configuration

[Qiang Yu]

The driver currently supports two PERST# and PHY DT configuration. In one
case, PHY, PERST#, are described in the RC node. In the other case, they
are described in the RP node.

A mixed setup is not supported. One common example is PHY on the RP node
while PERST# remains on the RC node. In that case the driver goes through
the RP parse path, does not find PERST# on RP, and does not report an
error because PERST# is optional. Probe can then succeed silently while
PERST# is left uncontrolled, and PCIe endpoints fails to work later. This
silent probe success makes debugging difficult.

Handle this mixed case in the RP parse path by checking whether PERST# is
present on RC and, if so, using the RC PERST# GPIO for RP ports while
keeping RP parsing for PHY. Emit a warning to indicate mixed DT content so
it can be fixed.

This keeps mixed systems functional and makes the configuration issue
visible instead of failing later at endpoint bring-up.

Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
Signed-off-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 9fdfc88ac151..8235961d692f 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -282,6 +282,7 @@ struct qcom_pcie {
 	const struct qcom_pcie_cfg *cfg;
 	struct dentry *debugfs;
 	struct list_head ports;
+	struct gpio_desc *reset;
 	bool suspended;
 	bool use_pm_opp;
 };
@@ -1703,6 +1704,11 @@ static int qcom_pcie_parse_perst(struct qcom_pcie *pcie,
 	struct gpio_desc *reset;
 	int ret;
 
+	if (pcie->reset) {
+		reset = pcie->reset;
+		goto skip_perst_parsing;
+	}
+
 	if (!of_find_property(np, "reset-gpios", NULL))
 		goto parse_child_node;
 
@@ -1721,6 +1727,7 @@ static int qcom_pcie_parse_perst(struct qcom_pcie *pcie,
 		return PTR_ERR(reset);
 	}
 
+skip_perst_parsing:
 	perst = devm_kzalloc(dev, sizeof(*perst), GFP_KERNEL);
 	if (!perst)
 		return -ENOMEM;
@@ -1778,6 +1785,14 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 	struct device *dev = pcie->pci->dev;
 	int ret = -ENODEV;
 
+	if (of_find_property(dev->of_node, "perst-gpios", NULL)) {
+		pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
+		if (IS_ERR(pcie->reset))
+			return PTR_ERR(pcie->reset);
+
+		dev_warn(dev, "Reusing PERST# from Root Complex node. DT needs to be fixed!\n");
+	}
+
 	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
 		if (!of_node_is_type(of_port, "pci"))
 			continue;

---
base-commit: 33a76fc3c3e61386524479b99f35423bd3d9a895
change-id: 20260508-mix_perst_phy_dts-b741b840833b

Best regards,
--  
Qiang Yu <qiang.yu@oss.qualcomm.com>

Re: [PATCH] PCI: qcom: Handle mixed PERST#/PHY DT configuration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Probing fails for legacy device trees due to a GPIO request conflict (-EBUSY).
- [High] Unconditional recursion in `qcom_pcie_parse_perst()` duplicates the `pcie->reset` GPIO descriptor for every descendant node.
- [High] Missing `phy_exit()` cleanup on error path in `qcom_pcie_parse_port()` causes hardware state and resource leak.
--

commit 1a0ea25971de08bb67bf6aea3574504602a6d020
Author: Qiang Yu <qiang.yu@oss.qualcomm.com>

PCI: qcom: Handle mixed PERST#/PHY DT configuration

The driver currently supports PERST# and PHY DT configuration on either the
RC or RP node. This commit adds support for mixed setups where PHY is on
the RP node but PERST# remains on the RC node by reusing the RC PERST#
GPIO and emitting a warning.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 9fdfc88ac1512..8235961d692f2 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c

[ ... ]

> @@ -1703,6 +1704,11 @@ static int qcom_pcie_parse_perst(struct qcom_pcie *pcie,
>  	struct gpio_desc *reset;
>  	int ret;
>  
> +	if (pcie->reset) {
> +		reset = pcie->reset;
> +		goto skip_perst_parsing;
> +	}
> +
>  	if (!of_find_property(np, "reset-gpios", NULL))
>  		goto parse_child_node;
>  
> @@ -1721,6 +1727,7 @@ static int qcom_pcie_parse_perst(struct qcom_pcie *pcie,
>  		return PTR_ERR(reset);
>  	}
>  
> +skip_perst_parsing:
>  	perst = devm_kzalloc(dev, sizeof(*perst), GFP_KERNEL);

When pcie->reset is configured, this jumps to skip_perst_parsing, adds
pcie->reset to the port's list, and then falls through to parse_child_node:

parse_child_node:
	for_each_available_child_of_node_scoped(np, child) {
		ret = qcom_pcie_parse_perst(pcie, port, child);
		if (ret)
			return ret;
	}

Since pcie->reset remains set globally, does this recursively call itself
for all descendant nodes, skip property parsing again, and append the exact
same GPIO descriptor to the port's list for every descendant?

Will this redundantly toggle the same GPIO multiple times and unintentionally
ignore any endpoint-specific reset-gpios properties defined in child nodes?

[ ... ]

> @@ -1778,6 +1785,14 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>  	struct device *dev = pcie->pci->dev;
>  	int ret = -ENODEV;
>  
> +	if (of_find_property(dev->of_node, "perst-gpios", NULL)) {
> +		pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> +		if (IS_ERR(pcie->reset))
> +			return PTR_ERR(pcie->reset);
> +
> +		dev_warn(dev, "Reusing PERST# from Root Complex node. DT needs to be fixed!\n");
> +	}
> +
>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {

If there are no child "pci" nodes, this function returns -ENODEV. Does the
caller then fall back to qcom_pcie_parse_legacy_binding() which will also
attempt to request the exact same "perst" GPIO?

Since devm_gpiod_get_optional() persists for the lifetime of the probe and
GPIOLIB enforces exclusive access by default, will the second request fail
with -EBUSY and break the probe path for legacy platforms?

>  		if (!of_node_is_type(of_port, "pci"))
>  			continue;
>  		ret = qcom_pcie_parse_port(pcie, of_port);
>  		if (ret)
>  			goto err_port_del;
>  	}

This issue wasn't introduced by this patch, but if qcom_pcie_parse_perst()
returns an error (such as -EPROBE_DEFER or -ENOMEM) from within
qcom_pcie_parse_port(), is phy_init() successfully called before the error
is encountered?

If qcom_pcie_parse_port() returns early without calling phy_exit(), and the
failing port has not yet been added to the pcie->ports list, does the
err_port_del cleanup loop skip this port?

err_port_del:
	list_for_each_entry_safe(port, tmp_port, &pcie->ports, list) {
		...
		phy_exit(port->phy);

Could this leave the PHY active and unbalance regulator/clock enable counts
across probe retries?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-mix_perst_phy_dts-v1-1-9eff6ee9b51a@oss.qualcomm.com?part=1

Re: [PATCH] PCI: qcom: Handle mixed PERST#/PHY DT configuration

[Konrad Dybcio]

On 5/8/26 11:54 AM, Qiang Yu wrote:
> The driver currently supports two PERST# and PHY DT configuration. In one
> case, PHY, PERST#, are described in the RC node. In the other case, they
> are described in the RP node.
> 
> A mixed setup is not supported. One common example is PHY on the RP node
> while PERST# remains on the RC node. In that case the driver goes through
> the RP parse path, does not find PERST# on RP, and does not report an
> error because PERST# is optional. Probe can then succeed silently while
> PERST# is left uncontrolled, and PCIe endpoints fails to work later. This
> silent probe success makes debugging difficult.
> 
> Handle this mixed case in the RP parse path by checking whether PERST# is
> present on RC and, if so, using the RC PERST# GPIO for RP ports while
> keeping RP parsing for PHY. Emit a warning to indicate mixed DT content so
> it can be fixed.
> 
> This keeps mixed systems functional and makes the configuration issue
> visible instead of failing later at endpoint bring-up.
> 
> Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> Signed-off-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 9fdfc88ac151..8235961d692f 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -282,6 +282,7 @@ struct qcom_pcie {
>  	const struct qcom_pcie_cfg *cfg;
>  	struct dentry *debugfs;
>  	struct list_head ports;
> +	struct gpio_desc *reset;

I'd rename this to make it more obvious it's at the RC node.. but
rc_reset doesn't really sound great..

otherwise:

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad