[PATCH] Revert "phy: qcom: qmp-pcie: Skip PHY reset if already up"

[PATCH] Revert "phy: qcom: qmp-pcie: Skip PHY reset if already up"

[Michael Burch]

This reverts commit 910b828b22b7b91054b3bd4be676a017444b0e00.

That commit skips the no-csr reset assert/deassert (and the 200-300us
settle) whenever the bootloader already brought the PCIe QMP PHY up:

  skip_reset = qmp->skip_init &&
               !qphy_checkbits(pcs, cfg->regs[QPHY_PCS_STATUS],
                               cfg->phy_status);

and states its own assumption explicitly: "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."

That assumption does not hold on the Dell XPS 13 9345 (X1E80100).  UEFI
boots from the NVMe behind pcie6a, so the PHY is always already started
when Linux takes over, skip_init is true, and the reset is skipped.  The
link then never retrains and the root port comes up with no endpoint:

  qcom-pcie 1bf8000.pcie: Device found, but not active
  pci 0006:00:00.0: [17cb:0111] type 01 class 0x060400 PCIe Root Port
  (no 0006:01:00.0, no nvme at all)

"Device found, but not active" is dw_pcie_wait_for_link() reporting LTSSM
stuck in POLL.Active/POLL.Compliance -- receiver detected, training never
completes -- so the machine cannot find its root filesystem and drops to
a dracut emergency shell.  v7.2 is unaffected because it always performed
the reset.

The commit was applied on its own as patch 1/4 of "PCI: qcom: Add link
retention support"; 2/4-4/4 have not landed. Without them,
qcom_pcie_host_init() still asserts PERST# and resets the controller
before qcom_pcie_phy_power_on(), so the link is down while
QPHY_PCS_STATUS still reports the bootloader's state, and the skip's
precondition never holds. Revert it until the rest of the series lands.

Fixes: 910b828b22b7 ("phy: qcom: qmp-pcie: Skip PHY reset if already up")
Signed-off-by: Michael Burch <me@michaelburch.net>
---
Reproduced today with Fedora's stock 7.3.0-0.rc7.58 kernel: the NVMe
never enumerates and boot stops in dracut. It was first seen here on
the stock 7.3.0-0.rc2 kernel. Every 7.3 build I have made with this
revert finds the NVMe (Gen4 x4 link up); the latest are v7.3-rc6 based.
Applying 2/4-4/4 of the original series on top of -rc2 also boots, but
only with clk_ignore_unused here. This applies to v7.3-rc7 and to
linux-phy fixes.

Qiang Yu's "PCI: qcom: Align PHY init sequence with HPG" carries the
same skip into the new phy_ops::reset. I'm replying there separately.

#regzbot introduced: 910b828b22b7

 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 31 ++++++++----------------
 1 file changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index da83358c97ea..17c53ae62fac 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -5677,7 +5677,6 @@ 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;
 
 	/*
@@ -5693,9 +5692,6 @@ 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;
@@ -5719,16 +5715,14 @@ static int qmp_pcie_init(struct phy *phy)
 		}
 	}
 
-	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);
+	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);
+
 	if (!qmp->skip_init) {
 		ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets);
 		if (ret) {
@@ -5777,11 +5771,8 @@ 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.
@@ -5805,12 +5796,10 @@ static int qmp_pcie_power_on(struct phy *phy)
 	if (ret)
 		return ret;
 
-	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;
-		}
+	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)

base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.56.0

Re: [PATCH] Revert "phy: qcom: qmp-pcie: Skip PHY reset if already up"

[Thorsten Leemhuis]

On 10/13/26 16:22, Michael Burch wrote:
> This reverts commit 910b828b22b7b91054b3bd4be676a017444b0e00.

What happened to this regression fix? Is this still needed? And if it
is: is this on the way to 7.4-rc1? Looks like it fell through the
cracks, but well, it's easy to miss something from the outside.

Ciao, Thorsten
> That commit skips the no-csr reset assert/deassert (and the 200-300us
> settle) whenever the bootloader already brought the PCIe QMP PHY up:
> 
>   skip_reset = qmp->skip_init &&
>                !qphy_checkbits(pcs, cfg->regs[QPHY_PCS_STATUS],
>                                cfg->phy_status);
> 
> and states its own assumption explicitly: "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."
> 
> That assumption does not hold on the Dell XPS 13 9345 (X1E80100).  UEFI
> boots from the NVMe behind pcie6a, so the PHY is always already started
> when Linux takes over, skip_init is true, and the reset is skipped.  The
> link then never retrains and the root port comes up with no endpoint:
> 
>   qcom-pcie 1bf8000.pcie: Device found, but not active
>   pci 0006:00:00.0: [17cb:0111] type 01 class 0x060400 PCIe Root Port
>   (no 0006:01:00.0, no nvme at all)
> 
> "Device found, but not active" is dw_pcie_wait_for_link() reporting LTSSM
> stuck in POLL.Active/POLL.Compliance -- receiver detected, training never
> completes -- so the machine cannot find its root filesystem and drops to
> a dracut emergency shell.  v7.2 is unaffected because it always performed
> the reset.
> 
> The commit was applied on its own as patch 1/4 of "PCI: qcom: Add link
> retention support"; 2/4-4/4 have not landed. Without them,
> qcom_pcie_host_init() still asserts PERST# and resets the controller
> before qcom_pcie_phy_power_on(), so the link is down while
> QPHY_PCS_STATUS still reports the bootloader's state, and the skip's
> precondition never holds. Revert it until the rest of the series lands.
> 
> Fixes: 910b828b22b7 ("phy: qcom: qmp-pcie: Skip PHY reset if already up")
> Signed-off-by: Michael Burch <me@michaelburch.net>
> ---
> Reproduced today with Fedora's stock 7.3.0-0.rc7.58 kernel: the NVMe
> never enumerates and boot stops in dracut. It was first seen here on
> the stock 7.3.0-0.rc2 kernel. Every 7.3 build I have made with this
> revert finds the NVMe (Gen4 x4 link up); the latest are v7.3-rc6 based.
> Applying 2/4-4/4 of the original series on top of -rc2 also boots, but
> only with clk_ignore_unused here. This applies to v7.3-rc7 and to
> linux-phy fixes.
> 
> Qiang Yu's "PCI: qcom: Align PHY init sequence with HPG" carries the
> same skip into the new phy_ops::reset. I'm replying there separately.
> 
> #regzbot introduced: 910b828b22b7
> 
>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 31 ++++++++----------------
>  1 file changed, 10 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index da83358c97ea..17c53ae62fac 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -5677,7 +5677,6 @@ 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;
>  
>  	/*
> @@ -5693,9 +5692,6 @@ 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;
> @@ -5719,16 +5715,14 @@ static int qmp_pcie_init(struct phy *phy)
>  		}
>  	}
>  
> -	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);
> +	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);
> +
>  	if (!qmp->skip_init) {
>  		ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets);
>  		if (ret) {
> @@ -5777,11 +5771,8 @@ 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.
> @@ -5805,12 +5796,10 @@ static int qmp_pcie_power_on(struct phy *phy)
>  	if (ret)
>  		return ret;
>  
> -	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;
> -		}
> +	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)
> 
> base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907

Re: [PATCH] Revert "phy: qcom: qmp-pcie: Skip PHY reset if already up"

[Michael Burch]

Yes, still needed. I retested today with unpatched upstream 7.3.0 on
the Dell XPS 13 9345 (X1E80100). It still reports "Device found, but not
active" for 1bf8000.pcie, enumerates only root port 0006:00:00.0, and
drops to dracut with no NVMe endpoint or /dev/nvme* devices. My patched
7.3.0 kernel with the revert boots successfully.


Thanks,
Michael