Re: [PATCH v2] PCI: imx6: Enable ASPM L1 only iff CLKREQ# is supported

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ASPM L1 is conditionally disabled at boot but unconditionally disabled upon resume, causing state inconsistency across power cycles.
--

--- Patch [1]: [PATCH v2] PCI: imx6: Enable ASPM L1 only iff CLKREQ# is supported ---
commit 7a899eb7e9f8116a0334832c7490e52f71ae751c
Author: Michal Gorlas <[email protected]>

PCI: imx6: Enable ASPM L1 only iff CLKREQ# is supported

This commit conditionally disables ASPM L1 capabilities in the LNKCAP
register if supports-clkreq is not set in the device tree. It also disables
L1 during resume for platforms advertising IMX_PCIE_FLAG_BROKEN_SUSPEND.

Link: https://patch.msgid.link/[email protected]

> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 98e1db751132a..1fdcbcd2422e5 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
[ ... ]
> @@ -1442,6 +1458,9 @@ static int imx_pcie_host_init(struct dw_pcie_rp *pp)
>  
>  	imx_setup_phy_mpll(imx_pcie);
>  
> +	if (!imx_pcie->supports_clkreq)
> +		imx_pcie_disable_l1(pci);
> +
>  	return 0;
>  
>  err_phy_off:
[ ... ]
> @@ -1748,6 +1767,7 @@ static int imx_pcie_resume_noirq(struct device *dev)
>  			return ret;
>  		imx_pcie_deassert_core_reset(imx_pcie);
>  		imx_pcie_assert_perst(imx_pcie, false);
> +		imx_pcie_disable_l1(imx_pcie->pci);

[Severity: Medium]
Will this cause an inconsistent ASPM L1 state across power cycles?

During boot in imx_pcie_host_init(), imx_pcie_disable_l1() is only called if
imx_pcie->supports_clkreq is false. However, during resume here in
imx_pcie_resume_noirq(), the L1 capability is disabled unconditionally for
platforms with IMX_PCIE_FLAG_BROKEN_SUSPEND.

If a platform has supports_clkreq set to true, it would retain the L1
capability at boot, but lose it unconditionally after a suspend/resume cycle.

Modifying the read-only Link Capabilities asymmetrically across power cycles
can confuse the OS ASPM subsystem, which caches LNKCAP during enumeration.
Should this also check for !imx_pcie->supports_clkreq before disabling L1
during resume?

>  
>  		/*
>  		 * Using PCIE_TEST_PD seems to disable MSI and powers down the
[ ... ]

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