[PATCH v2 0/4] PCI: qcom: Add link retention support

[PATCH v2 0/4] PCI: qcom: Add link retention support

[Krishna Chaitanya Chundru]

This patch series introduces support for retaining the PCIe link across
bootloader and kernel handoff on Qualcomm platforms, specifically
X1E80100. The goal is to reduce boot time and avoid unnecessary link
reinitialization  when the link is already up.

We are not enabling link retantion support for all the targets, as there
is no guarantee that the bootloader on all targets has initialized the
PCIe link in max supported speed. So we are enabling for hamoa & glymur
target only for now based on the config flag.

If the link is up and has link_retain is set to true in the
ithe driver config data then enable retain logic in the controller.

In phy as we already have skip init logic, the phy patch uses same
assumption that if there is phy no csr and bootloader has done the init
then driver can skip resetting the phy when phy status indicates it is
up.

Problem:-
1) As part of late init calls of clock the framework is disabling all the
unvoted resources by that time and also there is no sync state to keep
them enabled till the probe is completed. Due to dependencies with
regulators and phy, qcom pcie probe is happening after late init which is
causing the resources(clocks) to be off which causes the link to go down.
To avoid this we need to use this kernel command line argument
(clk_ignore_unused) to skip disabling clocks as part of late init for
initial version. Once it is resolved we can avoid those kernel command
line argument.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
Changes in v2:
- Rebased with latest changes
- GENPD/power domains are not longer getting turned off with the latest
  kernel, only issue is with the clocks.
- Removed the patch [2/5] PCI: dwc: Add support for retaining link during host init
  as we are not seeing much difference with this or without this (Bjorn).
- couple of nits in commit & prints (Mani).
- Remove skip_reset for the long term (Dmitry).
- Link to v1: https://lore.kernel.org/r/20260109-link_retain-v1-0-7e6782230f4b@oss.qualcomm.com

---
Krishna Chaitanya Chundru (4):
      phy: qcom: qmp-pcie: Skip PHY reset if already up
      PCI: qcom: Keep PERST# GPIO state as-is during probe
      PCI: qcom: Add link retention support
      PCI: qcom: enable Link retain logic for Hamoa

 drivers/pci/controller/dwc/pcie-designware.h |  1 +
 drivers/pci/controller/dwc/pcie-qcom.c       | 74 +++++++++++++++++++++++++---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c     | 31 ++++++++----
 3 files changed, 88 insertions(+), 18 deletions(-)
---
base-commit: 8bc67e4db64aa72732c474b44ea8622062c903f0
change-id: 20251001-link_retain-f181307947e4

Best regards,
--  
Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 1/4] phy: qcom: qmp-pcie: Skip PHY reset if already up

[Krishna Chaitanya Chundru]

If the bootloader has already powered up the PCIe PHY, performing a
full reset and waiting for the PHY to come up again adds unnecessary
delay during boot.

Extend the existing skip_init handling by introducing a skip_reset
condition. When skip_init is active and the PHY status indicates that
the PHY is already operational, skip asserting and deasserting the
no-csr reset while still enabling the required resources during
power-on.

This allows reusing the bootloader-initialized PHY state and avoids
redundant PHY reinitialization and PCIe link retraining, which can
add hundred's of milliseconds of delay.

This relies on the assumption that when skip_init is enabled and the
PHY is reported as up, the bootloader has already configured the PHY
correctly and the link is in a usable state.

Reviewed-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Tested-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index fed2fc9bb311..1458ac1478c7 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -4729,6 +4729,7 @@ static int qmp_pcie_init(struct phy *phy)
 	struct qmp_pcie *qmp = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
 	void __iomem *pcs = qmp->pcs;
+	bool skip_reset;
 	int ret;
 
 	/*
@@ -4744,6 +4745,9 @@ static int qmp_pcie_init(struct phy *phy)
 		qphy_checkbits(pcs, cfg->regs[QPHY_START_CTRL], SERDES_START | PCS_START) &&
 		qphy_checkbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], cfg->pwrdn_ctrl);
 
+	skip_reset = qmp->skip_init && !qphy_checkbits(pcs, cfg->regs[QPHY_PCS_STATUS],
+							    cfg->phy_status);
+
 	if (!qmp->skip_init && !cfg->tbls.serdes_num) {
 		dev_err(qmp->dev, "Init sequence not available\n");
 		return -ENODATA;
@@ -4767,13 +4771,15 @@ static int qmp_pcie_init(struct phy *phy)
 		}
 	}
 
-	ret = reset_control_assert(qmp->nocsr_reset);
-	if (ret) {
-		dev_err(qmp->dev, "no-csr reset assert failed\n");
-		goto err_assert_reset;
-	}
+	if (!skip_reset) {
+		ret = reset_control_assert(qmp->nocsr_reset);
+		if (ret) {
+			dev_err(qmp->dev, "no-csr reset assert failed\n");
+			goto err_assert_reset;
+		}
 
-	usleep_range(200, 300);
+		usleep_range(200, 300);
+	}
 
 	if (!qmp->skip_init) {
 		ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets);
@@ -4823,8 +4829,11 @@ static int qmp_pcie_power_on(struct phy *phy)
 	void __iomem *pcs = qmp->pcs;
 	void __iomem *status;
 	unsigned int mask, val;
+	bool skip_reset;
 	int ret;
 
+	skip_reset = qmp->skip_init && !qphy_checkbits(pcs, cfg->regs[QPHY_PCS_STATUS],
+							    cfg->phy_status);
 	/*
 	 * Write CSR register for PHY that doesn't support no_csr reset or has not
 	 * been initialized.
@@ -4848,10 +4857,12 @@ static int qmp_pcie_power_on(struct phy *phy)
 	if (ret)
 		return ret;
 
-	ret = reset_control_deassert(qmp->nocsr_reset);
-	if (ret) {
-		dev_err(qmp->dev, "no-csr reset deassert failed\n");
-		goto err_disable_pipe_clk;
+	if (!skip_reset) {
+		ret = reset_control_deassert(qmp->nocsr_reset);
+		if (ret) {
+			dev_err(qmp->dev, "no-csr reset deassert failed\n");
+			goto err_disable_pipe_clk;
+		}
 	}
 
 	if (qmp->skip_init)

-- 
2.34.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 2/4] PCI: qcom: Keep PERST# GPIO state as-is during probe

[Krishna Chaitanya Chundru]

The PERST# signal is used to reset PCIe devices. Requesting the GPIO with
GPIOD_OUT_HIGH forces the line high during probe, which can unintentionally
assert reset on devices already out of reset and break proper link
sequencing.

Change the request to use GPIOD_ASIS so the driver preserves the existing
GPIO state configured by the bootloader or firmware. This allows platforms
that manage PERST# externally to maintain correct reset sequencing. PERST#
is asserted explicitly later during qcom_pcie_host_init(), so forcing it
high at probe time is unnecessary.

Tested-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index af6bf5cce65b..bfe873cbf44f 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1707,7 +1707,7 @@ static int qcom_pcie_parse_perst(struct qcom_pcie *pcie,
 		goto parse_child_node;
 
 	reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(np), "reset",
-				      GPIOD_OUT_HIGH, "PERST#");
+				      GPIOD_ASIS, "PERST#");
 	if (IS_ERR(reset)) {
 		/*
 		 * FIXME: GPIOLIB currently supports exclusive GPIO access only.
@@ -1812,7 +1812,7 @@ static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
 	if (IS_ERR(phy))
 		return PTR_ERR(phy);
 
-	reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
+	reset = devm_gpiod_get_optional(dev, "perst", GPIOD_ASIS);
 	if (IS_ERR(reset))
 		return PTR_ERR(reset);
 

-- 
2.34.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 3/4] PCI: qcom: Add link retention support

[Krishna Chaitanya Chundru]

Some platforms keep the PCIe link active across bootloader and kernel
handoff. Reinitializing the controller and toggling PERST# in such cases is
unnecessary when the driver does not need to retrain the link.

Introduce link_retain in both qcom_pcie_cfg and qcom_pcie to indicate when
link retention is supported. During initialization, check the LTSSM state;
if the link is already in L0 or L1 idle and LTSSM is enabled, set
link_retain and skip controller reset, PERST# toggling, and other post-
init steps.

If the current link speed or lane width does not satisfy the constraints
specified by max-link-speed or num-lanes in the device tree, fall back to
normal initialization and retrain the link instead of retaining it.

Configure the DBI and ATU base addresses in the retention path, since the
bootloader may use different base addresses than those provided by the
device tree.

Also fix the -EPROBE_DEFER error handling path to return 0 instead of
propagating the error, avoiding unnecessary cleanup when probe deferral is
requested.

Tested-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-designware.h |  1 +
 drivers/pci/controller/dwc/pcie-qcom.c       | 62 +++++++++++++++++++++++++---
 2 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 3e69ef60165b..be6c4abf31e8 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -450,6 +450,7 @@ struct dw_pcie_rp {
 	bool			ecam_enabled;
 	bool			native_ecam;
 	bool                    skip_l23_ready;
+	bool			link_retain;
 };
 
 struct dw_pcie_ep_ops {
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index bfe873cbf44f..b061eaa227b3 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -253,12 +253,14 @@ struct qcom_pcie_ops {
   * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable cache
   * snooping
   * @firmware_managed: Set if the Root Complex is firmware managed
+  * @link_retain: Set if controller supports retaining link from bootloader
   */
 struct qcom_pcie_cfg {
 	const struct qcom_pcie_ops *ops;
 	bool override_no_snoop;
 	bool firmware_managed;
 	bool no_l0s;
+	bool link_retain;
 };
 
 struct qcom_pcie_perst {
@@ -960,6 +962,42 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
 	return 0;
 }
 
+/*
+ * Determine whether the link established by the bootloader can be reused.
+ *
+ * Reuse the existing link only if its current speed and lane count match
+ * the max-link-speed and num-lanes specified in Device Tree; otherwise,
+ * retrain the link.
+ */
+static bool qcom_pcie_check_link_retain(struct qcom_pcie *pcie)
+{
+	u32 cap, speed, val, ltssm, width;
+	struct dw_pcie *pci = pcie->pci;
+	u8 offset;
+
+	val = readl(pcie->parf + PARF_LTSSM);
+	ltssm = val & 0x1f;
+	if ((val & LTSSM_EN) &&
+	    (ltssm == DW_PCIE_LTSSM_L0 || ltssm == DW_PCIE_LTSSM_L1_IDLE)) {
+		qcom_pcie_configure_dbi_atu_base(pcie);
+
+		offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+		cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
+		speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, cap);
+		width = dw_pcie_link_get_max_link_width(pci);
+
+		if (pci->max_link_speed > 0 && speed > pci->max_link_speed)
+			return false;
+
+		if (pci->num_lanes > 0 && width > pci->num_lanes)
+			return false;
+
+		return true;
+	}
+
+	return false;
+}
+
 static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
@@ -978,6 +1016,14 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 	if (ret < 0)
 		goto err_disable_regulators;
 
+	if (pcie->cfg->link_retain) {
+		pci->pp.link_retain = qcom_pcie_check_link_retain(pcie);
+		if (pci->pp.link_retain) {
+			dev_info(dev, "Retaining PCIe link\n");
+			return 0;
+		}
+	}
+
 	ret = reset_control_assert(res->rst);
 	if (ret) {
 		dev_err(dev, "reset assert failed (%d)\n", ret);
@@ -1038,6 +1084,9 @@ static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
 {
 	const struct qcom_pcie_cfg *pcie_cfg = pcie->cfg;
 
+	if (pcie->pci->pp.link_retain)
+		return 0;
+
 	if (pcie_cfg->override_no_snoop)
 		writel(WR_NO_SNOOP_OVERRIDE_EN | RD_NO_SNOOP_OVERRIDE_EN,
 				pcie->parf + PARF_NO_SNOOP_OVERRIDE);
@@ -1294,12 +1343,13 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
 	int ret;
 
-	qcom_pcie_perst_assert(pcie);
-
 	ret = pcie->cfg->ops->init(pcie);
 	if (ret)
 		return ret;
 
+	if (!pp->link_retain)
+		qcom_pcie_perst_assert(pcie);
+
 	ret = qcom_pcie_phy_power_on(pcie);
 	if (ret)
 		goto err_deinit;
@@ -1322,7 +1372,8 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 	dw_pcie_remove_capability(pcie->pci, PCI_CAP_ID_MSIX);
 	dw_pcie_remove_ext_capability(pcie->pci, PCI_EXT_CAP_ID_DPC);
 
-	qcom_pcie_perst_deassert(pcie);
+	if (!pp->link_retain)
+		qcom_pcie_perst_deassert(pcie);
 
 	if (pcie->cfg->ops->config_sid) {
 		ret = pcie->cfg->ops->config_sid(pcie);
@@ -1337,8 +1388,9 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 err_pwrctrl_power_off:
 	pci_pwrctrl_power_off_devices(pci->dev);
 err_pwrctrl_destroy:
-	if (ret != -EPROBE_DEFER)
-		pci_pwrctrl_destroy_devices(pci->dev);
+	if (pp->link_retain && ret == -EPROBE_DEFER)
+		return 0;
+	pci_pwrctrl_destroy_devices(pci->dev);
 err_disable_phy:
 	qcom_pcie_phy_power_off(pcie);
 err_deinit:

-- 
2.34.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 4/4] PCI: qcom: enable Link retain logic for Hamoa

[Krishna Chaitanya Chundru]

The Hamoa platform supports keeping the PCIe link active across
bootloader and kernel handoff. To take advantage of this, introduce a
specific configuration (cfg_x1e80100) with link_retain = true and
update the device match table to use it.

Tested-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index b061eaa227b3..8496eb87debb 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1557,6 +1557,12 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
 	.no_l0s = true,
 };
 
+static const struct qcom_pcie_cfg cfg_x1e80100 = {
+	.ops = &ops_1_21_0,
+	.no_l0s = true,
+	.link_retain = true,
+};
+
 static const struct qcom_pcie_cfg cfg_fw_managed = {
 	.firmware_managed = true,
 };
@@ -2199,7 +2205,7 @@ static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-sm8450-pcie0", .data = &cfg_1_9_0 },
 	{ .compatible = "qcom,pcie-sm8450-pcie1", .data = &cfg_1_9_0 },
 	{ .compatible = "qcom,pcie-sm8550", .data = &cfg_1_9_0 },
-	{ .compatible = "qcom,pcie-x1e80100", .data = &cfg_sc8280xp },
+	{ .compatible = "qcom,pcie-x1e80100", .data = &cfg_x1e80100 },
 	{ }
 };
 

-- 
2.34.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/4] phy: qcom: qmp-pcie: Skip PHY reset if already up

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `skip_reset` condition is evaluated twice using a live hardware register, risking unbalanced reset operations if the PHY status changes between init and power_on.
--

commit 7deef03fab352fea4d882c35e253738b3fad392c
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

phy: qcom: qmp-pcie: Skip PHY reset if already up

This patch skips the nocsr reset during initialization if the bootloader
has already powered up the PCIe PHY and its status indicates it is up.
This avoids redundant initialization and link retraining delays during
boot.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index fed2fc9bb3110..1458ac1478c7e 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c

[ ... ]

> @@ -4744,6 +4745,9 @@ static int qmp_pcie_init(struct phy *phy)
>  		qphy_checkbits(pcs, cfg->regs[QPHY_START_CTRL], SERDES_START | PCS_START) &&
>  		qphy_checkbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], cfg->pwrdn_ctrl);
>  
> +	skip_reset = qmp->skip_init && !qphy_checkbits(pcs, cfg->regs[QPHY_PCS_STATUS],
> +							    cfg->phy_status);
> +
>  	if (!qmp->skip_init && !cfg->tbls.serdes_num) {
>  		dev_err(qmp->dev, "Init sequence not available\n");
>  		return -ENODATA;

[ ... ]

> @@ -4823,8 +4829,11 @@ static int qmp_pcie_power_on(struct phy *phy)
>  	void __iomem *pcs = qmp->pcs;
>  	void __iomem *status;
>  	unsigned int mask, val;
> +	bool skip_reset;
>  	int ret;
>  
> +	skip_reset = qmp->skip_init && !qphy_checkbits(pcs, cfg->regs[QPHY_PCS_STATUS],
> +							    cfg->phy_status);

[Severity: High]
Since skip_reset is evaluated dynamically in both qmp_pcie_init() and
qmp_pcie_power_on() by reading the live QPHY_PCS_STATUS hardware register,
could this lead to an unbalanced reset?

If qmp->skip_init is true but the PHY status bit indicates initialization is
not complete, skip_reset will be false in qmp_pcie_init(), causing
nocsr_reset to be asserted.

Once the reset is asserted, the hardware state changes. When
qmp_pcie_power_on() later re-evaluates skip_reset, reading the status
register of a PHY held in reset might return 0. This would make skip_reset
evaluate to true here, skipping the reset_control_deassert() call and leaving
the PHY permanently stuck in reset.

Should the decision to skip the reset be evaluated exactly once and cached in
the device context instead of reading the live register twice?

>  	/*
>  	 * Write CSR register for PHY that doesn't support no_csr reset or has not
>  	 * been initialized.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-link_retain-v2-0-08ed448b081c@oss.qualcomm.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/4] PCI: qcom: Keep PERST# GPIO state as-is during probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Silent hardware control failure due to incorrect GPIO direction when using `GPIOD_ASIS`.
--

commit b4b7937282f9b2dc2dff52265ec3936b1024af9d
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

PCI: qcom: Keep PERST# GPIO state as-is during probe

This commit changes the PERST# GPIO request to use GPIOD_ASIS instead of
GPIOD_OUT_HIGH. This preserves the existing GPIO state configured by the
bootloader or firmware to avoid unintentionally asserting reset during
probe.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index af6bf5cce65be..bfe873cbf44f6 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1707,7 +1707,7 @@ static int qcom_pcie_parse_perst(struct qcom_pcie *pcie,
>  		goto parse_child_node;
>  
>  	reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(np), "reset",
> -				      GPIOD_OUT_HIGH, "PERST#");
> +				      GPIOD_ASIS, "PERST#");

[Severity: High]
If the bootloader leaves the PERST# pin floating or as an input, will
this cause gpiod_set_value_cansleep() to silently fail later?

When using GPIOD_ASIS, if the pin was not previously configured as an
output, GPIOD_FLAG_IS_OUT will not be set in the GPIO descriptor.

Later in qcom_pcie_host_init(), the driver attempts to toggle the reset
pin by calling gpiod_set_value_cansleep() via __qcom_pcie_perst_assert().

Because gpiod_set_value_cansleep() enforces directionality, it will
return -EPERM if the pin is not configured as an output. Since
__qcom_pcie_perst_assert() ignores the return value of
gpiod_set_value_cansleep(), the reset line might never actually be driven.

Could this break PCIe link initialization on platforms where the
bootloader hasn't set the GPIO direction to output?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-link_retain-v2-0-08ed448b081c@oss.qualcomm.com?part=2

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 3/4] PCI: qcom: Add link retention support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Checking hardware link capabilities instead of negotiated link status to determine current link speed and width.
- [High] Returning 0 instead of -EPROBE_DEFER defeats the probe deferral mechanism and leaves the controller partially initialized.
- [High] Early returns during link retention bypass critical controller software state initialization and hardware capabilities.
--

commit 6dd7e9bfe9f104689a018777f9dcaa31b6e736f8
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
PCI: qcom: Add link retention support

This patch introduces link retention support for Qualcomm PCIe controllers,
skipping reinitialization and PERST# toggling if the bootloader left the
link active in L0 or L1 idle. It falls back to normal initialization and
link retraining if the current link does not meet device tree constraints.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index bfe873cbf44f6..b061eaa227b39 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c

[ ... ]

> @@ -960,6 +962,42 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>  	return 0;
>  }
>  
> +/*
> + * Determine whether the link established by the bootloader can be reused.
> + *
> + * Reuse the existing link only if its current speed and lane count match
> + * the max-link-speed and num-lanes specified in Device Tree; otherwise,
> + * retrain the link.
> + */
> +static bool qcom_pcie_check_link_retain(struct qcom_pcie *pcie)
> +{
> +	u32 cap, speed, val, ltssm, width;
> +	struct dw_pcie *pci = pcie->pci;
> +	u8 offset;
> +
> +	val = readl(pcie->parf + PARF_LTSSM);
> +	ltssm = val & 0x1f;
> +	if ((val & LTSSM_EN) &&
> +	    (ltssm == DW_PCIE_LTSSM_L0 || ltssm == DW_PCIE_LTSSM_L1_IDLE)) {
> +		qcom_pcie_configure_dbi_atu_base(pcie);
> +
> +		offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +		cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> +		speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, cap);
> +		width = dw_pcie_link_get_max_link_width(pci);

[Severity: High]
Does this check the maximum hardware capabilities instead of the currently
negotiated link status?

Reading PCI_EXP_LNKCAP (and using dw_pcie_link_get_max_link_width) retrieves
the hardware capabilities rather than the current link speed and width from
PCI_EXP_LNKSTA.

If the bootloader brought up the link at a degraded speed (e.g., Gen1x1 on a
port capable of Gen3x2), could this incorrectly pass the constraint checks
and leave the system with severely degraded PCIe performance?

> +
> +		if (pci->max_link_speed > 0 && speed > pci->max_link_speed)
> +			return false;
> +
> +		if (pci->num_lanes > 0 && width > pci->num_lanes)
> +			return false;
> +
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> @@ -978,6 +1016,14 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
>  	if (ret < 0)
>  		goto err_disable_regulators;
>  
> +	if (pcie->cfg->link_retain) {
> +		pci->pp.link_retain = qcom_pcie_check_link_retain(pcie);
> +		if (pci->pp.link_retain) {
> +			dev_info(dev, "Retaining PCIe link\n");
> +			return 0;
> +		}
> +	}

[Severity: High]
Does this early return bypass critical software state initialization?

Returning early here skips the assignment of pci->l1ss_support = true later
in this function. Because struct dw_pcie is zero-initialized, the downstream
DesignWare core might falsely assume ASPM L1 Substates are unsupported and
fail to configure them correctly.

> +
>  	ret = reset_control_assert(res->rst);
>  	if (ret) {
>  		dev_err(dev, "reset assert failed (%d)\n", ret);
> @@ -1038,6 +1084,9 @@ static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
>  {
>  	const struct qcom_pcie_cfg *pcie_cfg = pcie->cfg;
>  
> +	if (pcie->pci->pp.link_retain)
> +		return 0;
> +

[Severity: High]
Could this early return skip necessary hot-plug configurations?

By returning early, this skips the call to qcom_pcie_set_slot_nccs() located
further down. Missing the No Command Completed Support (NCCS) configuration
could break PCIe hot-plug command completions and lead to timeouts during
hot-plug operations.

>  	if (pcie_cfg->override_no_snoop)
>  		writel(WR_NO_SNOOP_OVERRIDE_EN | RD_NO_SNOOP_OVERRIDE_EN,
>  				pcie->parf + PARF_NO_SNOOP_OVERRIDE);

[ ... ]

> @@ -1337,8 +1388,9 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>  err_pwrctrl_power_off:
>  	pci_pwrctrl_power_off_devices(pci->dev);
>  err_pwrctrl_destroy:
> -	if (ret != -EPROBE_DEFER)
> -		pci_pwrctrl_destroy_devices(pci->dev);
> +	if (pp->link_retain && ret == -EPROBE_DEFER)
> +		return 0;

[Severity: High]
Does returning 0 here defeat the probe deferral mechanism?

If pci_pwrctrl_power_on_devices() earlier in this function returns
-EPROBE_DEFER, returning 0 instead of propagating the error code causes
the driver core to treat the probe as successful. This means the driver
will not be re-probed when resources become available.

Additionally, returning early from this error cleanup path leaves the rest
of the host initialization incomplete (such as SMMU setup and removing
MSIX/DPC capabilities), potentially leaving the controller in an unusable
state while the kernel believes the probe succeeded.

> +	pci_pwrctrl_destroy_devices(pci->dev);
>  err_disable_phy:
>  	qcom_pcie_phy_power_off(pcie);
>  err_deinit:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-link_retain-v2-0-08ed448b081c@oss.qualcomm.com?part=3

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 4/4] PCI: qcom: enable Link retain logic for Hamoa

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Setting `link_retain = true` in `cfg_x1e80100` exposes a flawed error handling path in `qcom_pcie_host_init()` that silently ignores `-EPROBE_DEFER` and bypasses critical host configuration.
--

commit 868652002bbdb8222b28dbec2b0655af97d3ceb8
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

    PCI: qcom: enable Link retain logic for Hamoa

    The Hamoa platform supports keeping the PCIe link active across
    bootloader and kernel handoff. To take advantage of this, introduce a
    specific configuration (cfg_x1e80100) with link_retain = true and
    update the device match table to use it.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index b061eaa227b39..8496eb87debbc 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1557,6 +1557,12 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
>  	.no_l0s = true,
>  };
>  
> +static const struct qcom_pcie_cfg cfg_x1e80100 = {
> +	.ops = &ops_1_21_0,
> +	.no_l0s = true,
> +	.link_retain = true,
> +};
> +

[Severity: High]
This isn't a bug introduced by this patch, but setting link_retain to true
here exposes a pre-existing flawed error handling path in qcom_pcie_host_init()
that silently ignores -EPROBE_DEFER.

If pci_pwrctrl_power_on_devices() returns -EPROBE_DEFER when the power control
driver hasn't bound yet, the error handler intercepts the deferral:

qcom_pcie_host_init() {
    ...
	ret = pci_pwrctrl_power_on_devices(pci->dev);
	if (ret)
		goto err_pwrctrl_destroy;
    ...
err_pwrctrl_destroy:
	if (pp->link_retain && ret == -EPROBE_DEFER)
		return 0;
    ...
}

Will returning 0 here trick the DWC core (dw_pcie_host_init()) into assuming
host initialization completed successfully?

Since the probe completes without retrying, could this leave the PCIe slot
regulators unmanaged by the kernel, causing them to be disabled later when
regulator_init_complete() runs at late initcall?

Additionally, does returning early skip mandatory initialization steps like
qcom_pcie_clear_aspm_l0s() (which is required by cfg_x1e80100.no_l0s = true),
hiding MSI-X/DPC capabilities, and performing SID configuration? Skipping these
could result in link instability, AER errors, and hardware malfunction.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-link_retain-v2-0-08ed448b081c@oss.qualcomm.com?part=4

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy