[PATCH v5] PCI: imx6: Fix i.MX6Q/DL boot hang caused by improper PHY power sequencing

[PATCH v5] PCI: imx6: Fix i.MX6Q/DL boot hang caused by improper PHY power sequencing

[hongxing.zhu]

From: Richard Zhu <hongxing.zhu@nxp.com>

commit 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling regulators")
introduced a boot hang on i.MX6Q/DL variants by reordering
imx_pcie_host_init() to call imx6q_pcie_enable_ref_clk() (which powered up
the PHY) before imx6q_pcie_core_reset() (which powered it back down).

Before 610fa91d9863, the sequence was:

  1. imx_pcie_assert_core_reset() - power down PHY (set TEST_PD), set
     REF_CLK_EN
  2. imx_pcie_clk_enable() - power up PHY (clear TEST_PD), set REF_CLK_EN
  3. Link training starts with PHY powered up (TEST_PD cleared)
  4. Link training succeeds

After 610fa91d9863, the sequence became:

  1. imx_pcie_clk_enable() - power up PHY (clear TEST_PD), set REF_CLK_EN
  2. imx_pcie_assert_core_reset() - power down PHY (set TEST_PD), set
     REF_CLK_EN
  3. imx_pcie_deassert_core_reset() - does nothing
  4. Link training starts with PHY powered down (TEST_PD set)
  5. Link training fails and boot hangs

To fix this:

  - Remove TEST_PD PHY power control from imx6q_pcie_enable_ref_clk()
  - Remove REF_CLK_EN control from imx6q_pcie_core_reset()
  - Add TEST_PD PHY power control to imx6qp_pcie_core_reset(), which
    previously relied on imx6q_pcie_enable_ref_clk() to power up the PHY by
    clearing TEST_PD
  - Clear TEST_PD to power on PHY in imx_pcie_deassert_core_reset()

These changes together ensure the correct sequence:

  1. REF_CLK_EN set in clk_enable() (TEST_PD untouched)
  2. TEST_PD set in assert_core_reset() (PHY power off)
  3. TEST_PD cleared in deassert_core_reset() (PHY power on)
  4. Link training starts with proper PHY state

The i.MX6Q/DL PCIe PHY requires approximately 120us between TEST_PD
de-assertion and link training start. Add usleep_range(200, 500) in
imx6q_pcie_core_reset() after clearing TEST_PD to satisfy this requirement.

Add explicit imx_pcie_assert_core_reset() calls in error paths and
host_exit() to ensure no power leak.

Fixes: 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling regulators")
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reported-by: Leonardo Costa <leoreis.costa@gmail.com>
Closes: https://lore.kernel.org/lkml/20260629143439.361560-1-leoreis.costa@gmail.com/
Tested-by: Leonardo Costa <leonardo.costa@toradex.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
Changes in v5:
Update the commit log suggested by Bjorn.

Changes in v4:
Refer to Sashiko' reivew of v3 patch.
- Adjust imx_pcie_assert_core_reset() in imx_pcie_host_exit().
- Add the delay explicitly after TEST_PD is cleared, since the PERST# GPIO
might be optional.

Changes in v3:
Update the commit descriptions to address the following items.
- Clarify the root cause of this regresssion.
- Describe why both changes are mandatory required to fix this
  regression.
- Justify the delay removal also.

Changes in v2:
Per Sashiko's review, invoke imx_pcie_assert_core_reset() explicitly
in error path of imx_pcie_host_init() and imx_pcie_host_exit().
---
 drivers/pci/controller/dwc/pci-imx6.c | 41 ++++++++++++++-------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index e25f938eefe23..c94b5e991aecd 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -681,21 +681,12 @@ static int imx_pcie_attach_pd(struct device *dev)
 
 static int imx6q_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool enable)
 {
-	if (enable) {
-		/* power up core phy and enable ref clock */
-		regmap_clear_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1, IMX6Q_GPR1_PCIE_TEST_PD);
-		/*
-		 * The async reset input need ref clock to sync internally,
-		 * when the ref clock comes after reset, internal synced
-		 * reset time is too short, cannot meet the requirement.
-		 * Add a ~10us delay here.
-		 */
-		usleep_range(10, 100);
-		regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1, IMX6Q_GPR1_PCIE_REF_CLK_EN);
-	} else {
-		regmap_clear_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1, IMX6Q_GPR1_PCIE_REF_CLK_EN);
-		regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1, IMX6Q_GPR1_PCIE_TEST_PD);
-	}
+	if (enable)
+		regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1,
+				IMX6Q_GPR1_PCIE_REF_CLK_EN);
+	else
+		regmap_clear_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1,
+				  IMX6Q_GPR1_PCIE_REF_CLK_EN);
 
 	return 0;
 }
@@ -826,6 +817,12 @@ static int imx6sx_pcie_core_reset(struct imx_pcie *imx_pcie, bool assert)
 
 static int imx6qp_pcie_core_reset(struct imx_pcie *imx_pcie, bool assert)
 {
+	if (assert)
+		regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1,
+				IMX6Q_GPR1_PCIE_TEST_PD);
+	else
+		regmap_clear_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1,
+				  IMX6Q_GPR1_PCIE_TEST_PD);
 	regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1, IMX6Q_GPR1_PCIE_SW_RST,
 			   assert ? IMX6Q_GPR1_PCIE_SW_RST : 0);
 	if (!assert)
@@ -836,11 +833,15 @@ static int imx6qp_pcie_core_reset(struct imx_pcie *imx_pcie, bool assert)
 
 static int imx6q_pcie_core_reset(struct imx_pcie *imx_pcie, bool assert)
 {
-	if (!assert)
-		return 0;
+	if (assert)
+		regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1,
+				IMX6Q_GPR1_PCIE_TEST_PD);
+	else
+		regmap_clear_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1,
+				  IMX6Q_GPR1_PCIE_TEST_PD);
 
-	regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1, IMX6Q_GPR1_PCIE_TEST_PD);
-	regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1, IMX6Q_GPR1_PCIE_REF_CLK_EN);
+	if (!assert)
+		usleep_range(200, 500);
 
 	return 0;
 }
@@ -1451,6 +1452,7 @@ static int imx_pcie_host_init(struct dw_pcie_rp *pp)
 	return 0;
 
 err_phy_off:
+	imx_pcie_assert_core_reset(imx_pcie);
 	phy_power_off(imx_pcie->phy);
 err_phy_exit:
 	phy_exit(imx_pcie->phy);
@@ -1470,6 +1472,7 @@ static void imx_pcie_host_exit(struct dw_pcie_rp *pp)
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct imx_pcie *imx_pcie = to_imx_pcie(pci);
 
+	imx_pcie_assert_core_reset(imx_pcie);
 	if (imx_pcie->phy) {
 		if (phy_power_off(imx_pcie->phy))
 			dev_err(pci->dev, "unable to power off PHY\n");
-- 
2.34.1

Re: [PATCH v5] PCI: imx6: Fix i.MX6Q/DL boot hang caused by improper PHY power sequencing

[sashiko-bot]

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

Pre-existing issues:
- [High] The return value of `imx_pcie_host_init()` is ignored in `imx_add_pcie_ep()`.
- [High] Missing PERST# assertion in error and exit paths leaks power into the PCIe slot when regulators are disabled.
--

--- Patch [1]: [PATCH v5] PCI: imx6: Fix i.MX6Q/DL boot hang caused by improper PHY power sequencing ---
commit 5815e6ed3898888ef7843d167117ab98b00ad077
Author: Richard Zhu <hongxing.zhu@nxp.com>

PCI: imx6: Fix i.MX6Q/DL boot hang caused by improper PHY power sequencing

This commit fixes a boot hang on i.MX6Q/DL boards by adjusting the power
sequencing of the PCIe PHY. It separates TEST_PD and REF_CLK_EN controls,
adds a delay for link training, and explicitly calls core reset assertions
in the error and exit paths.

Link: https://lore.kernel.org/lkml/20260629143439.361560-1-leoreis.costa@gmail.com/

> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c

[ ... ]

> @@ -1445,6 +1446,7 @@ static int imx_pcie_host_init(struct dw_pcie_rp *pp)
>  	return 0;
>  
>  err_phy_off:
> +	imx_pcie_assert_core_reset(imx_pcie);
>  	phy_power_off(imx_pcie->phy);
>  err_phy_exit:
>  	phy_exit(imx_pcie->phy);

[Severity: High]
This is a pre-existing issue, but is it possible we are missing a PERST#
assertion here?

If initialization fails (e.g., due to a PLL lock timeout), the error path
disables the slot power via pci_pwrctrl_power_off_devices() and
regulator_disable(). However, since PERST# was deasserted earlier, it
remains driven to its inactive state.

Can this back-power the PCIe endpoint device through its reset pin while
the main slot power is off? 

Should we call imx_pcie_assert_perst(imx_pcie, true) before powering off
the slot?

[ ... ]

> @@ -1466,6 +1468,7 @@ static void imx_pcie_host_exit(struct dw_pcie_rp *pp)
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct imx_pcie *imx_pcie = to_imx_pcie(pci);
>  
> +	imx_pcie_assert_core_reset(imx_pcie);
>  	if (imx_pcie->phy) {
>  		if (phy_power_off(imx_pcie->phy))
>  			dev_err(pci->dev, "unable to power off PHY\n");

[Severity: High]
This is a pre-existing issue, but similar to the error path above, when
disabling the slot power during teardown in imx_pcie_host_exit(), do we
also need to assert PERST# first to prevent back-powering the endpoint?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the PCIe
initialization, I noticed a potential issue in imx_add_pcie_ep() in the
same file:

drivers/pci/controller/dwc/pci-imx6.c:imx_add_pcie_ep() {
    ...
    imx_pcie_host_init(pp);
    ...
}

Does this code safely handle initialization failures?

imx_pcie_host_init() can fail and return an error code (for instance, if
regulators or clocks fail to enable), which correctly cleans up and
powers off the controller. But imx_add_pcie_ep() ignores this return
value and continues execution.

Later on, dw_pcie_ep_init_registers() unconditionally accesses the endpoint
hardware registers. If the controller was powered off due to the previous
failure, could reading these unclocked registers cause a synchronous
external abort (kernel panic)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728035159.2702021-1-hongxing.zhu@oss.nxp.com?part=1