[PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control
Hongxing Zhu (OSS)
hongxing.zhu at oss.nxp.com
Mon Jul 20 01:32:31 PDT 2026
> -----Original Message-----
> From: Hongxing Zhu (OSS) <hongxing.zhu at oss.nxp.com>
> Sent: Friday, July 17, 2026 4:57 PM
> To: Manivannan Sadhasivam <mani at kernel.org>; Hongxing Zhu (OSS)
> <hongxing.zhu at oss.nxp.com>
> Cc: 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
>
> > -----Original Message-----
> > From: Manivannan Sadhasivam <mani at kernel.org>
> > Sent: Friday, July 17, 2026 12:35 AM
> > To: Hongxing Zhu (OSS) <hongxing.zhu at oss.nxp.com>
> > Cc: 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 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.
> > >
> >
> > What is the timing requirement here?
> The timing requirement is that TEST_PD must be deasserted (cleared) before link
> training starts.
>
> 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.
> Thanks.
Hi Mani:
I've attempted to split the changes as below:
1. Set/Clear TEST_PD in assert_core_reset()/deassert_core_reset()
2. Clean up imx6q_pcie_enable_ref_clk() to only manipulate the REF_CLK_EN bit
However, I found that patch 1 alone doesn't work correctly. Here's what happens:
With only patch 1 applied:
- The board boots successfully, but fails to detect the remote endpoint device
- Problem sequence:
Begin (TEST_PD asserted by default)
→ TEST_PD cleared + REF_CLK_EN asserted in clk_enable()
→ TEST_PD asserted again in assert_core_reset()
→ TEST_PD cleared in deassert_core_reset()
With both patches applied:
- The board boots and detects the remote endpoint device successfully
- Correct sequence:
Begin (TEST_PD asserted by default)
→ REF_CLK_EN asserted in clk_enable() (TEST_PD remains untouched)
→ TEST_PD asserted in assert_core_reset()
→ TEST_PD cleared in deassert_core_reset()
The issue is that patch 1 relies on patch 2 to avoid prematurely clearing TEST_PD
in clk_enable(). Both changes are mandatory for the fix to work.
Given this dependency, would you prefer:
- A two-patch series with the dependency clearly documented, or
- A single combined patch since they cannot function independently.
Thanks.
Best Regards
Richard Zhu
>
> Best Regards
> Richard Zhu
> >
> > - Mani
> >
> > > 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