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

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(), leading to a kernel crash if hardware initialization fails.
--

--- Patch [1]: [PATCH v1] PCI: imx6: Move clock enable after core reset assertion ---
commit 6d9f9eb5750531f447a5eaee6a038ee84ba72010
Author: Richard Zhu <[email protected]>

PCI: imx6: Move clock enable after core reset assertion

This patch reverts a sequence change by moving clock enablement after
asserting core reset and configuring the controller type. This restores
correct initialization on i.MX6Q Apalis platforms where PCI device
initialization was failing.

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 is a pre-existing issue, but is it safe for imx_add_pcie_ep() to ignore
the return value of imx_pcie_host_init()?

Looking at imx_add_pcie_ep(), it calls imx_pcie_host_init() and immediately
proceeds without checking if it succeeded:

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

If imx_pcie_host_init() fails here (for example, if imx_pcie_clk_enable()
returns an error), it cleans up its resources and returns an error code with
clocks disabled.

Execution would then continue into dw_pcie_ep_init(), which attempts
hardware register accesses. On i.MX hardware, accessing PCIe registers with
disabled clocks triggers a synchronous bus error (SError), resulting in a
kernel panic.

Could imx_add_pcie_ep() be updated to check the return value of
imx_pcie_host_init() and handle the failure gracefully?

>  	if (imx_pcie->phy) {
>  		ret = phy_init(imx_pcie->phy);
>  		if (ret) {

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