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

[Hongxing Zhu (OSS)]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Tuesday, July 28, 2026 5:40 AM
> To: Hongxing Zhu (OSS) <hongxing.zhu@xxxxxxxxxxx>
> Cc: Frank Li <frank.li@xxxxxxx>; l.stach@xxxxxxxxxxxxxx; lpieralisi@xxxxxxxxxx;
> kwilczynski@xxxxxxxxxx; mani@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>; Leonardo Costa
> <leoreis.costa@xxxxxxxxx>; Leonardo Costa <leonardo.costa@xxxxxxxxxxx>
> Subject: Re: [PATCH v4] PCI: imx6: Fix i.MX6Q/DL boot hang caused by improper
> PHY power sequencing
> 
> [You don't often get email from helgaas@xxxxxxxxxx. Learn why this is important
> at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Wed, Jul 22, 2026 at 03:10:30PM +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
> > reordering the initialization sequence, which exposed a critical PHY power
> control issue.
> >
> > Root cause:
> > Before commit 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling
> > regulators"), the sequence was:
> >   1. imx_pcie_assert_core_reset() - asserts TEST_PD, asserts REF_CLK_EN
> >   2. imx_pcie_clk_enable() - clears TEST_PD, asserts REF_CLK_EN
> >   3. Link training starts with TEST_PD properly cleared ✓
> >
> > After commit 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling
> > regulators"), the sequence became:
> >   1. imx_pcie_clk_enable() - clears TEST_PD, asserts REF_CLK_EN
> >   2. imx_pcie_assert_core_reset() - re-asserts TEST_PD, asserts REF_CLK_EN
> >   3. imx_pcie_deassert_core_reset() - does NOT clear TEST_PD
> >   4. Link training starts with TEST_PD still asserted ✗
> >
> > The reordering caused TEST_PD to be cleared prematurely in
> > clk_enable(), then re-asserted by assert_core_reset(), and never
> > cleared again before link training, resulting in the boot hang.
> >
> > The fix requires two interdependent changes that cannot be split:
> > 1. Move TEST_PD control to imx6q_pcie_core_reset() where it belongs
> >    logically with reset operations
> > 2. Remove TEST_PD manipulation from imx6q_pcie_enable_ref_clk() to
> >    prevent premature clearing
> >
> > Applying only change #1 results in device detection failure because
> > TEST_PD gets cleared too early in clk_enable(), then re-asserted in
> > assert_core_reset(), then cleared again in deassert_core_reset(). This
> > premature clearing disrupts the proper PHY power-up sequence.
> >
> > Both changes together ensure the correct sequence:
> >   1. REF_CLK_EN asserted in clk_enable() (TEST_PD untouched)
> >   2. TEST_PD asserted in assert_core_reset()
> >   3. TEST_PD cleared in deassert_core_reset()
> >   4. Link training starts with proper PHY state ✓
> >
> > The previous delay in imx6q_pcie_enable_ref_clk() was a workaround for
> > async reset synchronization when ref clock and PHY power were coupled.
> > With proper sequencing, this delay is no longer needed.
> >
> > The i.MX6Q/DL PCIe PHY requires approximately 120us between TEST_PD
> > de-assertion and link training start. Add usleep_range(200, 500) in
> > imx6q_pcie_core_reset() after clearing TEST_PD to satisfy this
> > requirement.
> >
> > Additional changes:
> > Add explicit imx_pcie_assert_core_reset() calls in error paths and
> > host_exit() to ensure no power leak.
> >
> > Fixes: 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling
> > regulators")
> > Signed-off-by: Richard Zhu <hongxing.zhu@xxxxxxx>
> > Reported-by: Leonardo Costa <leoreis.costa@xxxxxxxxx>
> > Closes:
> > https://lore.kernel.org/lkml/20260629143439.361560-1-leoreis.costa@gma
> > il.com/
> > Tested-by: Leonardo Costa <leonardo.costa@xxxxxxxxxxx>
> > Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
> > ---
> > Changes in v4:
> > Refer to Sashiko' reivew of v3 patch.
> > - Adjust imx_pcie_assert_core_reset() in imx_pcie_host_exit().
> > - Add the delay explicitly after TEST_PD is cleared, since the PERST#
> > GPIO might be optional.
> >
> > Changes in v3:
> > Update the commit descriptions to address the following items.
> > - Clarify the root cause of this regresssion.
> > - Describe why both changes are mandatory required to fix this
> >   regression.
> > - Justify the delay removal also.
> >
> > Changes in v2:
> > Per Sashiko's review, invoke imx_pcie_assert_core_reset() explicitly
> > in error path of imx_pcie_host_init() and imx_pcie_host_exit().
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 41
> > ++++++++++++++-------------
> >  1 file changed, 22 insertions(+), 19 deletions(-)
> 
> I'd propose the following commit log to focus on the effects of TEST_PD (setting
> TEST_PD powers down the PHY, clearing it powers up the PHY) and to match the
> regmap_set/clear language in the patch
> itself:
> 
> 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling regulators") introduced
> a boot hang on i.MX6Q/DL variants by reordering
> imx_pcie_host_init() to call imx6q_pcie_enable_ref_clk() (which powered up the
> PHY) before imx6q_pcie_core_reset() (which powered it back down).
> 
> Before 610fa91d9863, the sequence was:
> 
>   1. imx_pcie_assert_core_reset() - power down PHY (set TEST_PD), set
>      REF_CLK_EN
>   2. imx_pcie_clk_enable() - power up PHY (clear TEST_PD), set REF_CLK_EN
>   3. Link training starts with PHY powered up (TEST_PD cleared)
>   4. Link training succeeds
> 
> After 610fa91d9863, the sequence became:
> 
>   1. imx_pcie_clk_enable() - power up PHY (clear TEST_PD), set REF_CLK_EN
>   2. imx_pcie_assert_core_reset() - power down PHY (set TEST_PD), set
>      REF_CLK_EN
>   3. imx_pcie_deassert_core_reset() - does nothing
>   4. Link training starts with PHY powered down (TEST_PD set)
>   5. Link training fails and boot hangs
> 
> To fix this:
> 
>   - Remove TEST_PD PHY power control from imx6q_pcie_enable_ref_clk()
> 
>   - Remove REF_CLK_EN control from imx6q_pcie_core_reset()
> 
>   - Add TEST_PD PHY power control to imx6qp_pcie_core_reset(), which
>     previously relied on imx6q_pcie_enable_ref_clk() to power up the PHY by
>     clearing TEST_PD
> 
>   - Clear TEST_PD to power on PHY in imx_pcie_deassert_core_reset()
> 
> These changes together ensure the correct sequence:
> 
>   1. REF_CLK_EN set in clk_enable() (TEST_PD untouched)
>   2. TEST_PD set in assert_core_reset() (PHY power off)
>   3. TEST_PD cleared in deassert_core_reset() (PHY power on)
>   4. Link training starts with proper PHY state
> 
> The i.MX6Q/DL PCIe PHY requires approximately 120us between TEST_PD de-
> assertion and link training start. Add usleep_range(200, 500) in
> imx6q_pcie_core_reset() after clearing TEST_PD to satisfy this requirement.
> 
> Add explicit imx_pcie_assert_core_reset() calls in error paths and
> host_exit() to ensure no power leak.
Hi Bjorn:
Thanks for your kindly help.
Your new proposed commit message is totally fine for me.

Best Regards
Richard Zhu