[PATCH v2] PCI: imx6: Move clock enable after core reset assertion

[PATCH v2] PCI: imx6: Move clock enable after core reset assertion

[hongxing.zhu]

From: Richard Zhu <hongxing.zhu@nxp.com>

Commit 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling regulators")
inadvertently moved clock enablement before core reset assertion, breaking
PCI device initialization on i.MX6Q Apalis platforms with
ASM1061/ASM1062 SATA controllers connected.

NOTE: It is not 100% clear if the issue is specific to the ASM1061/ASM1062
device or on the specific power-up sequence (reset vs cold-power-on).

Logs:
root@apalis-imx6-11486042:~# dmesg | grep pcie
imx6q-pcie 1ffc000.pcie: host bridge /soc/pcie@1ffc000 ranges:
imx6q-pcie 1ffc000.pcie:       IO 0x0001f80000..0x0001f8ffff -> 0x0000000000
imx6q-pcie 1ffc000.pcie:      MEM 0x0001000000..0x0001efffff -> 0x0001000000
imx6q-pcie 1ffc000.pcie: config reg[1] 0x01f00000 == cpu 0x01f00000
imx6q-pcie 1ffc000.pcie: iATU: unroll F, 4 ob, 4 ib, align 64K, limit 4G
imx6q-pcie 1ffc000.pcie: Link: Only Gen1 is enabled
imx6q-pcie 1ffc000.pcie: Link failed to come up. LTSSM: POLL_CONFIG
imx6q-pcie 1ffc000.pcie: probe with driver imx6q-pcie failed with error -110

To fix this regression, restore the original sequence where clocks are
enabled after asserting core reset and configuring the controller type.

Fixes: 610fa91d9863 ("PCI: imx6: Assert PERST# before enabling regulators")
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reported-by: Leonardo Costa <leoreis.costa@gmail.com>
Closes: https://lore.kernel.org/all/bl7i3obu2clzsgeoct2a4mtfhv6typcjdqmgneropf3hpgwve6@n2m5uhlduw57/T/#u
Reported-by: Franz Schnyder <fra.schnyder@gmail.com>
Closes: https://lore.kernel.org/all/t65y5d54axtksbfs7r4olcefqhwm6m4dz3njgnrnf7fcotj74i@o7avoznlafbj/
Cc: stable@vger.kernel.org # 7.2+
---
Changes in v2:
Update the commit message refer to Francesco' suggestions.
---
 drivers/pci/controller/dwc/pci-imx6.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 39790e66b98dc..f7a2eb257c169 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1394,12 +1394,6 @@ static int imx_pcie_host_init(struct dw_pcie_rp *pp)
 		}
 	}
 
-	ret = imx_pcie_clk_enable(imx_pcie);
-	if (ret) {
-		dev_err(dev, "unable to enable pcie clocks: %d\n", ret);
-		goto err_pwrctrl_power_off;
-	}
-
 	if (pp->bridge && imx_check_flag(imx_pcie, IMX_PCIE_FLAG_HAS_LUT)) {
 		pp->bridge->enable_device = imx_pcie_enable_device;
 		pp->bridge->disable_device = imx_pcie_disable_device;
@@ -1415,6 +1409,12 @@ static int imx_pcie_host_init(struct dw_pcie_rp *pp)
 
 	imx_pcie_configure_type(imx_pcie);
 
+	ret = imx_pcie_clk_enable(imx_pcie);
+	if (ret) {
+		dev_err(dev, "unable to enable pcie clocks: %d\n", ret);
+		goto err_pwrctrl_power_off;
+	}
+
 	if (imx_pcie->phy) {
 		ret = phy_init(imx_pcie->phy);
 		if (ret) {

base-commit: 28d012efb4327f9c75d5e042a7c91e9a542efa98
-- 
2.34.1

Re: [PATCH v2] PCI: imx6: Move clock enable after core reset assertion

[sashiko-bot]

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

Pre-existing issues:
- [High] The return value of `imx_pcie_host_init()` is ignored in `imx_add_pcie_ep()`, allowing initialization to proceed even if critical hardware resources failed to enable.
--

--- Patch [1]: [PATCH v2] PCI: imx6: Move clock enable after core reset assertion ---
commit e96724a712688ba6f907245675d12f392129c5de
Author: Richard Zhu <hongxing.zhu@nxp.com>

PCI: imx6: Move clock enable after core reset assertion

Restores the sequence where clocks are enabled after asserting core reset
and configuring the controller type. Fixes a regression on i.MX6Q Apalis
platforms with ASM1061/ASM1062 SATA controllers connected.

Link: https://lore.kernel.org/all/bl7i3obu2clzsgeoct2a4mtfhv6typcjdqmgneropf3hpgwve6@n2m5uhlduw57/T/#u
Link: https://lore.kernel.org/all/t65y5d54axtksbfs7r4olcefqhwm6m4dz3njgnrnf7fcotj74i@o7avoznlafbj/

> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 39790e66b98dc..f7a2eb257c169 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
[ ... ]
> @@ -1415,6 +1409,12 @@ static int imx_pcie_host_init(struct dw_pcie_rp *pp)
>  
>  	imx_pcie_configure_type(imx_pcie);
>  
> +	ret = imx_pcie_clk_enable(imx_pcie);
> +	if (ret) {
> +		dev_err(dev, "unable to enable pcie clocks: %d\n", ret);
> +		goto err_pwrctrl_power_off;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to ignore the
return value of imx_pcie_host_init() in imx_add_pcie_ep()?

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

If imx_pcie_host_init() fails (for example, if imx_pcie_clk_enable() fails
here), the error is discarded and the endpoint initialization proceeds.

Could this lead to a kernel panic or synchronous external abort when
dw_pcie_ep_init() subsequently attempts to access unclocked or unpowered
PCIe registers?

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