[PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control
Hongxing Zhu
hongxing.zhu at nxp.com
Sun Jul 19 23:39:57 PDT 2026
> -----Original Message-----
> From: Bjorn Helgaas <helgaas at kernel.org>
> Sent: Saturday, July 18, 2026 7:14 AM
> To: Hongxing Zhu (OSS) <hongxing.zhu at oss.nxp.com>
> Cc: Manivannan Sadhasivam <mani at kernel.org>; Frank Li <frank.li at nxp.com>;
> l.stach at pengutronix.de; lpieralisi at kernel.org; kwilczynski at kernel.org;
> robh at kernel.org; bhelgaas at google.com; s.hauer at pengutronix.de;
> kernel at pengutronix.de; festevam at gmail.com; linux-pci at vger.kernel.org; linux-
> arm-kernel at lists.infradead.org; imx at lists.linux.dev; linux-
> kernel at vger.kernel.org; Hongxing Zhu <hongxing.zhu at nxp.com>
> Subject: Re: [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY
> power and reference clock control
>
> On Fri, Jul 17, 2026 at 08:57:04AM +0000, Hongxing Zhu (OSS) wrote:
> > > -----Original Message-----
> > > From: Manivannan Sadhasivam <mani at kernel.org>
> > ...
> > > On Wed, Jul 08, 2026 at 11:59:27AM +0800, hongxing.zhu at oss.nxp.com
> wrote:
> > > > From: Richard Zhu <hongxing.zhu at nxp.com>
> > > >
> > > > Commit 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling
> > > > regulators") introduced a boot hang on i.MX6Q/DL variants by
> > > > changing the initialization sequence.
> > > >
> > > > The issue stems from coupling PHY power (TEST_PD) and reference
> > > > clock
> > > > (REF_CLK_EN) control in imx6q_pcie_enable_ref_clk(). When these
> > > > are managed together, the timing between PHY power-up and
> > > > reference clock enablement cannot be properly controlled, leading
> > > > to initialization failures.
>
> This is kind of a hand-wavy description that doesn't explain exactly what
> 610fa91d9863 changed that broke the boot.
>
> I don't understand what you're saying about timing between PHY power-up and
> REFCLK enable because it looks like you enable REFCLK *first*, then power up the
> PHY. There's a 200us delay in
> imx_pcie_clk_enable() after enabling REFCLK, but that was already there in
> 610fa91d9863.
Hi Bjorn:
You're right that my initial description was unclear. Let me explain exactly
what commit 610fa91d9863 changed that caused the boot hang.
Before commit 610fa91d9863:
The function call order was:
imx_pcie_assert_core_reset() - Asserts TEST_PD and clears REF_CLK_EN
imx_pcie_clk_enable() - Clears TEST_PD and asserts REF_CLK_EN
Link training starts with TEST_PD properly cleared ✓
After commit 610fa91d9863:
The function call order changed to:
imx_pcie_clk_enable() - Clears TEST_PD and asserts REF_CLK_EN
imx_pcie_assert_core_reset() - Re-asserts TEST_PD and asserts REF_CLK_EN again
imx_pcie_deassert_core_reset() - Does NOT clear TEST_PD
Link training starts with TEST_PD still asserted ✗
Root cause: The reordering means TEST_PD gets cleared early in
imx_pcie_clk_enable(), but then gets re-asserted by
imx_pcie_assert_core_reset() and is never cleared again before link training
begins. This causes the boot hang.
This fix ensures TEST_PD is cleared at the appropriate time regardless of the
function call order.
>
> > > What is the timing requirement here?
> >
> > The timing requirement is that TEST_PD must be deasserted (cleared)
> > before link training starts.
>
> Is there any delay required between deasserting TEST_PD and link training?
>
> Prior to this patch, imx_pcie_deassert_core_reset() didn't touch TEST_PD on
> imx6qp, but it did delay 200us in imx6qp_pcie_core_reset().
> Now it will clear TEST_PD and still delay 200us.
>
Yes, there is a delay requirement (~ 120us) between TEST_PD de-assertion and
link training start.
This delay is already satisfied by the PERST# toggling sequence in
imx_pcie_assert_perst(), which is called after imx_pcie_deassert_core_reset().
The PERST# assertion time is much longer than 120us, so it provides sufficient
delay.
Regarding the 200us delay in imx6qp_pcie_core_reset(): this delay was
originally intended to satisfy the TEST_PD timing requirement. Since TEST_PD
is now properly cleared in imx_pcie_deassert_core_reset() and the timing is
covered by the subsequent PERST# sequence, the 200us delay in
imx6qp_pcie_core_reset() is redundant and could be removed in a follow-up
patch.
> On imx6q, it didn't touch TEST_PD or delay. Now it will clear TEST_PD but still
> won't delay.
>
> I don't see any other delay enforced between PHY power up (in
> imx_pcie_deassert_core_reset()) and link training. So after this patch, it looks like
> the chipset-specific behavior in
> imx_pcie_deassert_core_reset() is:
>
> imx6sx: clear TEST_POWERDOWN, no delay
> imx6q: clear TEST_PD, no delay
> imx6qp: clear TEST_PD, usleep(200)
> imx7d: wait for PHY PLL lock
> imx95: nothing
>
> Here's the path I see after this patch is applied:
>
> imx_pcie_probe
> dw_pcie_host_init
> imx_pcie_host_init
> imx_pcie_clk_enable
> imx6q_pcie_enable_ref_clk(enable=true)
> regmap_set_bits(IMX6Q_GPR1_PCIE_REF_CLK_EN) # REFCLK enable
> usleep(200) # <-- delay
> imx_pcie_assert_core_reset
> imx6q_pcie_core_reset(assert=true)
> regmap_set_bits(IMX6Q_GPR1_PCIE_TEST_PD) # PHY power off
> imx_pcie_ltssm_disable
> imx_pcie_deassert_core_reset
>
> imx6q_pcie_core_reset(assert=false)
> regmap_clear_bits(IMX6Q_GPR1_PCIE_TEST_PD) # PHY power on
> -- or --
> imx6qp_pcie_core_reset(assert=false)
> regmap_clear_bits(IMX6Q_GPR1_PCIE_TEST_PD) # PHY power on
> regmap_update_bits(IMX6Q_GPR1_PCIE_SW_RST)
> usleep(200) # <-- delay
>
> dw_pcie_start_link
> imx_pcie_start_link
>
> > Before commit 610fa91d9863:
> > - imx_pcie_assert_core_reset(): Assert TEST_PD and REF_CLK_EN
> > - imx_pcie_clk_enable(): Deassert TEST_PD and assert REF_CLK_EN
> > - Link training starts with TEST_PD properly cleared
> >
> > After commit 610fa91d9863:
> > - imx_pcie_clk_enable(): Deassert TEST_PD and assert REF_CLK_EN
> > - imx_pcie_assert_core_reset(): Assert TEST_PD and assert REF_CLK_EN
> > again
> > - Link training starts with TEST_PD still asserted (never cleared
> > again)
> >
> > This commit corrects the sequence, and makes sure the TEST_PD is
> > cleared before link training starts.
>
>
> > > > Fix this by separating the two concerns:
> > > >
> > > > - Move PHY power control (TEST_PD) to imx6q_pcie_core_reset() where it
> > > > logically belongs with reset operations. This ensures PHY power state
> > > > is managed as part of the core reset sequence.
> > > >
> > > > - Update imx6qp_pcie_core_reset() to call imx6q_pcie_core_reset() for
> > > > shared PHY power management, avoiding code duplication.
> > > >
> > > > - Make imx6q_pcie_enable_ref_clk() responsible only for reference clock
> > > > (REF_CLK_EN) control, simplifying its purpose.
> > > >
> > > > - Remove the 10us delay workaround from imx6q_pcie_enable_ref_clk() as
> > > > proper sequencing is now handled by the core_reset functions.
> > > >
> > > > This refactoring ensures PHY power is controlled during reset
> > > > operations, fixing the boot hang while improving code maintainability.
> > > >
> > >
> > > This patch does too many things at once. Can't you split it and keep
> > > the minimal fix in one patch?
> >
> > Okay, I'll split this into a patch series in v3.
>
> The "invoke imx_pcie_assert_core_reset() explicitly in error path of
> imx_pcie_host_init() and imx_pcie_host_exit()" part seems unrelated to the boot
> hang.
The changes to the error path and exit function are related to this fix.
Previously, imx_pcie_clk_disable() would assert TEST_PD for i.MX6Q/i.MX6QP as
a side effect. However, with this patch, TEST_PD manipulation is moved out of
the clock enable/disable functions and into the core reset functions where it
logically belongs.
This means we need to explicitly call imx_pcie_assert_core_reset() in the
error path of imx_pcie_host_init() and in imx_pcie_host_exit() to ensure
TEST_PD is properly asserted during shutdown/cleanup. Without this, we would
have a power leak issue, which is why Sashiko suggested this change in the
previous review.
Thanks for your kindly review.
Best Regards
Richard Zhu
>
> > > > Fixes: 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling
> > > > regulators")
> > > > Signed-off-by: Richard Zhu <hongxing.zhu at nxp.com>
> > > > ---
> > > > Changes in v2:
> > > > Regarding sashiko's reivew, 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 | 45
> > > > ++++++++++++---------------
> > > > 1 file changed, 20 insertions(+), 25 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > > index 9406bba36953f..53f3da6ab30d5 100644
> > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > @@ -680,21 +680,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;
> > > > }
> > > > @@ -823,23 +814,25 @@ static int imx6sx_pcie_core_reset(struct
> > > > imx_pcie
> > > *imx_pcie, bool assert)
> > > > return 0;
> > > > }
> > > >
> > > > -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)
> > > > {
> > > > - regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1,
> > > IMX6Q_GPR1_PCIE_SW_RST,
> > > > - assert ? IMX6Q_GPR1_PCIE_SW_RST : 0);
> > > > - if (!assert)
> > > > - usleep_range(200, 500);
> > > > + 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);
> > > >
> > > > return 0;
> > > > }
> > > >
> > > > -static int imx6q_pcie_core_reset(struct imx_pcie *imx_pcie, bool
> > > > assert)
> > > > +static int imx6qp_pcie_core_reset(struct imx_pcie *imx_pcie, bool
> > > > +assert)
> > > > {
> > > > + imx6q_pcie_core_reset(imx_pcie, assert);
> > > > + regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR1,
> > > IMX6Q_GPR1_PCIE_SW_RST,
> > > > + assert ? IMX6Q_GPR1_PCIE_SW_RST : 0);
> > > > if (!assert)
> > > > - return 0;
> > > > -
> > > > - 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);
> > > > + usleep_range(200, 500);
> > > >
> > > > return 0;
> > > > }
> > > > @@ -1445,6 +1438,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);
> > > > @@ -1471,6 +1465,7 @@ static void imx_pcie_host_exit(struct
> > > > dw_pcie_rp
> > > *pp)
> > > > dev_err(pci->dev, "unable to power off PHY\n");
> > > > phy_exit(imx_pcie->phy);
> > > > }
> > > > + imx_pcie_assert_core_reset(imx_pcie);
> > > > imx_pcie_clk_disable(imx_pcie);
> > > >
> > > > pci_pwrctrl_power_off_devices(pci->dev);
> > > > --
> > > > 2.34.1
> > > >
> > >
> > > --
> > > மணிவண்ணன் சதாசிவம்
More information about the linux-arm-kernel
mailing list