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

[Manivannan Sadhasivam]

On Mon, Jul 20, 2026 at 08:32:31AM +0000, Hongxing Zhu (OSS) wrote:
> > -----Original Message-----
> > From: Hongxing Zhu (OSS) <hongxing.zhu@xxxxxxxxxxx>
> > Sent: Friday, July 17, 2026 4:57 PM
> > To: Manivannan Sadhasivam <mani@xxxxxxxxxx>; 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
> > 
> > > -----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.
> 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

-- 
மணிவண்ணன் சதாசிவம்