RE: [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control

[Hongxing Zhu (OSS)]

> -----Original Message-----
> From: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> Sent: Friday, July 17, 2026 12:35 AM
> To: Hongxing Zhu (OSS) <hongxing.zhu@xxxxxxxxxxx>
> Cc: Frank Li <frank.li@xxxxxxx>; l.stach@xxxxxxxxxxxxxx; lpieralisi@xxxxxxxxxx;
> kwilczynski@xxxxxxxxxx; robh@xxxxxxxxxx; bhelgaas@xxxxxxxxxx;
> s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; linux-
> pci@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Hongxing Zhu <hongxing.zhu@xxxxxxx>
> 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@xxxxxxxxxxx wrote:
> > From: Richard Zhu <hongxing.zhu@xxxxxxx>
> >
> > 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.

Best Regards
Richard Zhu
> 
> - Mani
> 
> > Fixes: 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling
> > regulators")
> > Signed-off-by: Richard Zhu <hongxing.zhu@xxxxxxx>
> > ---
> > 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
> >
> 
> --
> மணிவண்ணன் சதாசிவம்