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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The return value of `imx_pcie_host_init()` is ignored in `imx_add_pcie_ep()`.
- [High] Missing PERST# assertion in error and exit paths leaks power into the PCIe slot when regulators are disabled.
--

--- Patch [1]: [PATCH v5] PCI: imx6: Fix i.MX6Q/DL boot hang caused by improper PHY power sequencing ---
commit 5815e6ed3898888ef7843d167117ab98b00ad077
Author: Richard Zhu <[email protected]>

PCI: imx6: Fix i.MX6Q/DL boot hang caused by improper PHY power sequencing

This commit fixes a boot hang on i.MX6Q/DL boards by adjusting the power
sequencing of the PCIe PHY. It separates TEST_PD and REF_CLK_EN controls,
adds a delay for link training, and explicitly calls core reset assertions
in the error and exit paths.

Link: https://lore.kernel.org/lkml/[email protected]/

> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c

[ ... ]

> @@ -1445,6 +1446,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);

[Severity: High]
This is a pre-existing issue, but is it possible we are missing a PERST#
assertion here?

If initialization fails (e.g., due to a PLL lock timeout), the error path
disables the slot power via pci_pwrctrl_power_off_devices() and
regulator_disable(). However, since PERST# was deasserted earlier, it
remains driven to its inactive state.

Can this back-power the PCIe endpoint device through its reset pin while
the main slot power is off? 

Should we call imx_pcie_assert_perst(imx_pcie, true) before powering off
the slot?

[ ... ]

> @@ -1466,6 +1468,7 @@ static void imx_pcie_host_exit(struct dw_pcie_rp *pp)
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct imx_pcie *imx_pcie = to_imx_pcie(pci);
>  
> +	imx_pcie_assert_core_reset(imx_pcie);
>  	if (imx_pcie->phy) {
>  		if (phy_power_off(imx_pcie->phy))
>  			dev_err(pci->dev, "unable to power off PHY\n");

[Severity: High]
This is a pre-existing issue, but similar to the error path above, when
disabling the slot power during teardown in imx_pcie_host_exit(), do we
also need to assert PERST# first to prevent back-powering the endpoint?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the PCIe
initialization, I noticed a potential issue in imx_add_pcie_ep() in the
same file:

drivers/pci/controller/dwc/pci-imx6.c:imx_add_pcie_ep() {
    ...
    imx_pcie_host_init(pp);
    ...
}

Does this code safely handle initialization failures?

imx_pcie_host_init() can fail and return an error code (for instance, if
regulators or clocks fail to enable), which correctly cleans up and
powers off the controller. But imx_add_pcie_ep() ignores this return
value and continues execution.

Later on, dw_pcie_ep_init_registers() unconditionally accesses the endpoint
hardware registers. If the controller was powered off due to the previous
failure, could reading these unclocked registers cause a synchronous
external abort (kernel panic)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1