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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] PCIe PHY remains powered up on probe error paths
--

commit a734b2214ed3f695bc7968ca2438d6c116ff54b7
Author: Richard Zhu <hongxing.zhu@nxp.com>

PCI: imx6: Fix i.MX6Q/DL boot hang by separating PHY power and reference clock control

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.

[ ... ]

> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 98e1db751132a..ab74f03f70bc8 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);

[ ... ]

> -	} 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;
>  }

[Severity: Medium]
When TEST_PD is no longer asserted here, does this cause a power leak
during probe failure or driver removal?

Because imx_pcie_clk_disable() indirectly calls this function with
enable=false, it used to power down the PHY by asserting TEST_PD. After
this change, error paths only disable the clock but leave the PHY powered up:

drivers/pci/controller/dwc/pci-imx6.c:imx_pcie_host_init() {
    ...
    imx_pcie_deassert_core_reset(imx_pcie);
    ...
    if (imx_pcie->drvdata->wait_pll_lock) {
        ret = imx_pcie->drvdata->wait_pll_lock(imx_pcie);
        if (ret < 0)
            goto err_phy_off;
    }
    ...
err_clk_disable:
    imx_pcie_clk_disable(imx_pcie);
    ...
}

The same applies to imx_pcie_host_exit() which calls imx_pcie_clk_disable()
during teardown.

Should imx_pcie_assert_core_reset() be explicitly called in
imx_pcie_host_init()'s error path (and in imx_pcie_host_exit()) to ensure
the PHY is properly powered down and reset?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706030629.3264320-1-hongxing.zhu@oss.nxp.com?part=1