* Re: [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control
2026-07-08 3:59 ` [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control hongxing.zhu
@ 2026-07-08 15:28 ` Frank Li
2026-07-14 12:51 ` Francesco Dolcini
` (2 subsequent siblings)
3 siblings, 0 replies; 20+ messages in thread
From: Frank Li @ 2026-07-08 15:28 UTC (permalink / raw)
To: hongxing.zhu
Cc: frank.li, l.stach, lpieralisi, kwilczynski, mani, robh, bhelgaas,
s.hauer, kernel, festevam, linux-pci, linux-arm-kernel, imx,
linux-kernel, Richard Zhu
On Wed, Jul 08, 2026 at 11:59:27AM +0800, hongxing.zhu@oss.nxp.com wrote:
> 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 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.
>
> 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.
>
> Fixes: 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling regulators")
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
look more symmetry than before.
Reviewed-by: Frank Li <Frank.Li@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
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control
2026-07-08 3:59 ` [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control hongxing.zhu
2026-07-08 15:28 ` Frank Li
@ 2026-07-14 12:51 ` Francesco Dolcini
2026-07-15 1:38 ` Hongxing Zhu (OSS)
2026-07-16 14:43 ` Leonardo Costa
2026-07-16 16:35 ` Manivannan Sadhasivam
3 siblings, 1 reply; 20+ messages in thread
From: Francesco Dolcini @ 2026-07-14 12:51 UTC (permalink / raw)
To: bhelgaas, hongxing.zhu, linux-pci, Richard Zhu
Cc: frank.li, l.stach, lpieralisi, kwilczynski, mani, robh, s.hauer,
kernel, festevam, linux-arm-kernel, imx, linux-kernel,
regressions
Hello,
On Wed, Jul 08, 2026 at 11:59:27AM +0800, hongxing.zhu@oss.nxp.com wrote:
> 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 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.
>
> 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.
>
> Fixes: 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling regulators")
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
What's the plan with this? 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling regulators")
introduces a severe regression with boards not booting at all.
As of now we are not able to test anything, and our CI is completely
blind on any other issue that linux 7.2 might have introduced.
Is the plan to merge this, or to revert the commit? Any timeline?
Francesco
^ permalink raw reply [flat|nested] 20+ messages in thread* RE: [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control
2026-07-14 12:51 ` Francesco Dolcini
@ 2026-07-15 1:38 ` Hongxing Zhu (OSS)
0 siblings, 0 replies; 20+ messages in thread
From: Hongxing Zhu (OSS) @ 2026-07-15 1:38 UTC (permalink / raw)
To: Francesco Dolcini, bhelgaas@google.com, Hongxing Zhu (OSS),
linux-pci@vger.kernel.org, Hongxing Zhu
Cc: Frank Li, l.stach@pengutronix.de, lpieralisi@kernel.org,
kwilczynski@kernel.org, mani@kernel.org, robh@kernel.org,
s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com,
linux-arm-kernel@lists.infradead.org, imx@lists.linux.dev,
linux-kernel@vger.kernel.org, regressions@lists.linux.dev
> -----Original Message-----
> From: Francesco Dolcini <francesco@dolcini.it>
> Sent: Tuesday, July 14, 2026 8:51 PM
> To: bhelgaas@google.com; Hongxing Zhu (OSS) <hongxing.zhu@oss.nxp.com>;
> linux-pci@vger.kernel.org; Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: Frank Li <frank.li@nxp.com>; l.stach@pengutronix.de; lpieralisi@kernel.org;
> kwilczynski@kernel.org; mani@kernel.org; robh@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com; linux-
> arm-kernel@lists.infradead.org; imx@lists.linux.dev; linux-
> kernel@vger.kernel.org; regressions@lists.linux.dev
> Subject: Re: [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY
> power and reference clock control
>
> [You don't often get email from francesco@dolcini.it. Learn why this is important
> at https://aka.ms/LearnAboutSenderIdentification ]
>
> Hello,
>
> On Wed, Jul 08, 2026 at 11:59:27AM +0800, hongxing.zhu@oss.nxp.com wrote:
> > 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 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.
> >
> > 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.
> >
> > Fixes: 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling
> > regulators")
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
>
> What's the plan with this? 610fa91d9863 ("PCI: imx6: Assert PERST# before
> enabling regulators") introduces a severe regression with boards not booting at
> all.
>
> As of now we are not able to test anything, and our CI is completely blind on any
> other issue that linux 7.2 might have introduced.
>
> Is the plan to merge this, or to revert the commit? Any timeline?[]
Hi Francesco:
Sorry for this severe regression.
I prefer to merge this fix as soon as possible. Would you be able to test this
patch on your affected boards? Your testing would help expedite the merge
process and ensure the fix resolves the boot issue you're experiencing.
Thank you for your assistance.
Best Regards
Richard Zhu
>
> Francesco
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control
2026-07-08 3:59 ` [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control hongxing.zhu
2026-07-08 15:28 ` Frank Li
2026-07-14 12:51 ` Francesco Dolcini
@ 2026-07-16 14:43 ` Leonardo Costa
2026-07-17 6:10 ` Francesco Dolcini
2026-07-16 16:35 ` Manivannan Sadhasivam
3 siblings, 1 reply; 20+ messages in thread
From: Leonardo Costa @ 2026-07-16 14:43 UTC (permalink / raw)
To: hongxing.zhu
Cc: frank.li, l.stach, lpieralisi, kwilczynski, mani, robh, bhelgaas,
s.hauer, kernel, festevam, linux-pci, linux-arm-kernel, imx,
linux-kernel, Richard Zhu, leonardo.costa
On Wed, Jul 08, 2026 at 11:59:27AM +0800, hongxing.zhu@oss.nxp.com wrote:
> 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 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.
>
> 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.
>
> Fixes: 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling regulators")
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
Tested-by: Leonardo Costa <leonardo.costa@toradex.com>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control
2026-07-16 14:43 ` Leonardo Costa
@ 2026-07-17 6:10 ` Francesco Dolcini
0 siblings, 0 replies; 20+ messages in thread
From: Francesco Dolcini @ 2026-07-17 6:10 UTC (permalink / raw)
To: Leonardo Costa, bhelgaas
Cc: hongxing.zhu, frank.li, l.stach, lpieralisi, kwilczynski, mani,
robh, s.hauer, kernel, festevam, linux-pci, linux-arm-kernel, imx,
linux-kernel, Richard Zhu, leonardo.costa
On Thu, Jul 16, 2026 at 11:43:31AM -0300, Leonardo Costa wrote:
> On Wed, Jul 08, 2026 at 11:59:27AM +0800, hongxing.zhu@oss.nxp.com wrote:
> > 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 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.
> >
> > 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.
> >
> > Fixes: 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling regulators")
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
>
> Tested-by: Leonardo Costa <leonardo.costa@toradex.com>
Reported-by: Leonardo Costa <leoreis.costa@gmail.com>
Closes: https://lore.kernel.org/lkml/20260629143439.361560-1-leoreis.costa@gmail.com/
Bjorn: this should solve the concerns your questions from https://lore.kernel.org/lkml/20260716172858.GA111215@bhelgaas/
Francesco
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control
2026-07-08 3:59 ` [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control hongxing.zhu
` (2 preceding siblings ...)
2026-07-16 14:43 ` Leonardo Costa
@ 2026-07-16 16:35 ` Manivannan Sadhasivam
2026-07-17 8:57 ` Hongxing Zhu (OSS)
3 siblings, 1 reply; 20+ messages in thread
From: Manivannan Sadhasivam @ 2026-07-16 16:35 UTC (permalink / raw)
To: hongxing.zhu
Cc: frank.li, l.stach, lpieralisi, kwilczynski, robh, bhelgaas,
s.hauer, kernel, festevam, linux-pci, linux-arm-kernel, imx,
linux-kernel, Richard Zhu
On Wed, Jul 08, 2026 at 11:59:27AM +0800, hongxing.zhu@oss.nxp.com wrote:
> 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 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?
> 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?
- Mani
> Fixes: 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling regulators")
> Signed-off-by: Richard Zhu <hongxing.zhu@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
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 20+ messages in thread* RE: [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control
2026-07-16 16:35 ` Manivannan Sadhasivam
@ 2026-07-17 8:57 ` Hongxing Zhu (OSS)
2026-07-17 23:14 ` Bjorn Helgaas
2026-07-20 8:32 ` Hongxing Zhu (OSS)
0 siblings, 2 replies; 20+ messages in thread
From: Hongxing Zhu (OSS) @ 2026-07-17 8:57 UTC (permalink / raw)
To: Manivannan Sadhasivam, Hongxing Zhu (OSS)
Cc: Frank Li, l.stach@pengutronix.de, lpieralisi@kernel.org,
kwilczynski@kernel.org, robh@kernel.org, bhelgaas@google.com,
s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com,
linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
imx@lists.linux.dev, linux-kernel@vger.kernel.org, Hongxing Zhu
> -----Original Message-----
> From: Manivannan Sadhasivam <mani@kernel.org>
> Sent: Friday, July 17, 2026 12:35 AM
> To: Hongxing Zhu (OSS) <hongxing.zhu@oss.nxp.com>
> Cc: Frank Li <frank.li@nxp.com>; l.stach@pengutronix.de; lpieralisi@kernel.org;
> kwilczynski@kernel.org; robh@kernel.org; bhelgaas@google.com;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com; linux-
> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; imx@lists.linux.dev;
> linux-kernel@vger.kernel.org; Hongxing Zhu <hongxing.zhu@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@oss.nxp.com wrote:
> > 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 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@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
> >
>
> --
> மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control
2026-07-17 8:57 ` Hongxing Zhu (OSS)
@ 2026-07-17 23:14 ` Bjorn Helgaas
2026-07-20 6:39 ` Hongxing Zhu
2026-07-20 8:32 ` Hongxing Zhu (OSS)
1 sibling, 1 reply; 20+ messages in thread
From: Bjorn Helgaas @ 2026-07-17 23:14 UTC (permalink / raw)
To: Hongxing Zhu (OSS)
Cc: Manivannan Sadhasivam, Frank Li, l.stach@pengutronix.de,
lpieralisi@kernel.org, kwilczynski@kernel.org, robh@kernel.org,
bhelgaas@google.com, s.hauer@pengutronix.de,
kernel@pengutronix.de, festevam@gmail.com,
linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
imx@lists.linux.dev, linux-kernel@vger.kernel.org, Hongxing Zhu
On Fri, Jul 17, 2026 at 08:57:04AM +0000, Hongxing Zhu (OSS) wrote:
> > -----Original Message-----
> > From: Manivannan Sadhasivam <mani@kernel.org>
> ...
> > On Wed, Jul 08, 2026 at 11:59:27AM +0800, hongxing.zhu@oss.nxp.com wrote:
> > > 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 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.
> > 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.
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.
> > > Fixes: 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling
> > > regulators")
> > > Signed-off-by: Richard Zhu <hongxing.zhu@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
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 20+ messages in thread* RE: [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control
2026-07-17 23:14 ` Bjorn Helgaas
@ 2026-07-20 6:39 ` Hongxing Zhu
0 siblings, 0 replies; 20+ messages in thread
From: Hongxing Zhu @ 2026-07-20 6:39 UTC (permalink / raw)
To: Bjorn Helgaas, Hongxing Zhu (OSS)
Cc: Manivannan Sadhasivam, Frank Li, l.stach@pengutronix.de,
lpieralisi@kernel.org, kwilczynski@kernel.org, robh@kernel.org,
bhelgaas@google.com, s.hauer@pengutronix.de,
kernel@pengutronix.de, festevam@gmail.com,
linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
imx@lists.linux.dev, linux-kernel@vger.kernel.org
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Saturday, July 18, 2026 7:14 AM
> To: Hongxing Zhu (OSS) <hongxing.zhu@oss.nxp.com>
> Cc: Manivannan Sadhasivam <mani@kernel.org>; Frank Li <frank.li@nxp.com>;
> l.stach@pengutronix.de; lpieralisi@kernel.org; kwilczynski@kernel.org;
> robh@kernel.org; bhelgaas@google.com; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com; linux-pci@vger.kernel.org; linux-
> arm-kernel@lists.infradead.org; imx@lists.linux.dev; linux-
> kernel@vger.kernel.org; Hongxing Zhu <hongxing.zhu@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@kernel.org>
> > ...
> > > On Wed, Jul 08, 2026 at 11:59:27AM +0800, hongxing.zhu@oss.nxp.com
> wrote:
> > > > 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
> > > > 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@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
> > > >
> > >
> > > --
> > > மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control
2026-07-17 8:57 ` Hongxing Zhu (OSS)
2026-07-17 23:14 ` Bjorn Helgaas
@ 2026-07-20 8:32 ` Hongxing Zhu (OSS)
2026-07-21 6:16 ` Manivannan Sadhasivam
1 sibling, 1 reply; 20+ messages in thread
From: Hongxing Zhu (OSS) @ 2026-07-20 8:32 UTC (permalink / raw)
To: Hongxing Zhu (OSS), Manivannan Sadhasivam, Hongxing Zhu (OSS)
Cc: Frank Li, l.stach@pengutronix.de, lpieralisi@kernel.org,
kwilczynski@kernel.org, robh@kernel.org, bhelgaas@google.com,
s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com,
linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
imx@lists.linux.dev, linux-kernel@vger.kernel.org, Hongxing Zhu
> -----Original Message-----
> From: Hongxing Zhu (OSS) <hongxing.zhu@oss.nxp.com>
> Sent: Friday, July 17, 2026 4:57 PM
> To: Manivannan Sadhasivam <mani@kernel.org>; Hongxing Zhu (OSS)
> <hongxing.zhu@oss.nxp.com>
> Cc: Frank Li <frank.li@nxp.com>; l.stach@pengutronix.de; lpieralisi@kernel.org;
> kwilczynski@kernel.org; robh@kernel.org; bhelgaas@google.com;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com; linux-
> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; imx@lists.linux.dev;
> linux-kernel@vger.kernel.org; Hongxing Zhu <hongxing.zhu@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@kernel.org>
> > Sent: Friday, July 17, 2026 12:35 AM
> > To: Hongxing Zhu (OSS) <hongxing.zhu@oss.nxp.com>
> > Cc: Frank Li <frank.li@nxp.com>; l.stach@pengutronix.de;
> > lpieralisi@kernel.org; kwilczynski@kernel.org; robh@kernel.org;
> > bhelgaas@google.com; s.hauer@pengutronix.de; kernel@pengutronix.de;
> > festevam@gmail.com; linux- pci@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org; imx@lists.linux.dev;
> > linux-kernel@vger.kernel.org; Hongxing Zhu <hongxing.zhu@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@oss.nxp.com wrote:
> > > 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
> > > 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@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
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2] PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control
2026-07-20 8:32 ` Hongxing Zhu (OSS)
@ 2026-07-21 6:16 ` Manivannan Sadhasivam
0 siblings, 0 replies; 20+ messages in thread
From: Manivannan Sadhasivam @ 2026-07-21 6:16 UTC (permalink / raw)
To: Hongxing Zhu (OSS)
Cc: Frank Li, l.stach@pengutronix.de, lpieralisi@kernel.org,
kwilczynski@kernel.org, robh@kernel.org, bhelgaas@google.com,
s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com,
linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
imx@lists.linux.dev, linux-kernel@vger.kernel.org, Hongxing Zhu
On Mon, Jul 20, 2026 at 08:32:31AM +0000, Hongxing Zhu (OSS) wrote:
> > -----Original Message-----
> > From: Hongxing Zhu (OSS) <hongxing.zhu@oss.nxp.com>
> > Sent: Friday, July 17, 2026 4:57 PM
> > To: Manivannan Sadhasivam <mani@kernel.org>; Hongxing Zhu (OSS)
> > <hongxing.zhu@oss.nxp.com>
> > Cc: Frank Li <frank.li@nxp.com>; l.stach@pengutronix.de; lpieralisi@kernel.org;
> > kwilczynski@kernel.org; robh@kernel.org; bhelgaas@google.com;
> > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com; linux-
> > pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; imx@lists.linux.dev;
> > linux-kernel@vger.kernel.org; Hongxing Zhu <hongxing.zhu@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@kernel.org>
> > > Sent: Friday, July 17, 2026 12:35 AM
> > > To: Hongxing Zhu (OSS) <hongxing.zhu@oss.nxp.com>
> > > Cc: Frank Li <frank.li@nxp.com>; l.stach@pengutronix.de;
> > > lpieralisi@kernel.org; kwilczynski@kernel.org; robh@kernel.org;
> > > bhelgaas@google.com; s.hauer@pengutronix.de; kernel@pengutronix.de;
> > > festevam@gmail.com; linux- pci@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org; imx@lists.linux.dev;
> > > linux-kernel@vger.kernel.org; Hongxing Zhu <hongxing.zhu@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@oss.nxp.com wrote:
> > > > 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
> > > > 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.
>
Hmm, then you should combine both in a single patch. But the commit message
should be accurate and properly describe what was causing issue, and why both
changes are necessary.
You should justify the delay removal also.
- Mani
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 20+ messages in thread