Linux kernel and device drivers for NXP i.MX platforms
 help / color / mirror / Atom feed
* [PATCH v2] PCI: imx6: Enable ASPM L1 only iff CLKREQ# is supported
@ 2026-07-31 13:44 Michal Gorlas
  2026-07-31 13:54 ` sashiko-bot
  0 siblings, 1 reply; 3+ messages in thread
From: Michal Gorlas @ 2026-07-31 13:44 UTC (permalink / raw)
  To: Richard Zhu, Lucas Stach, Lorenzo Pieralisi,
	Krzysztof Wilczyński, Manivannan Sadhasivam, Rob Herring,
	Bjorn Helgaas, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam
  Cc: linux-pci, linux-arm-kernel, imx, linux-kernel

Since commit f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM
states for devicetree platforms"), ASPM states are force enabled
on all devicetree platforms. This causes PCI devices on some i.MX SoCs
(for e.g. i.MX 8M Plus Kontron SMARC eval carrier) to enter L1 even
though CLKREQ# is not supported (and consequently supports-clkreq is
not set in pcie controller node of their DT), which causes the device to
never wake up from L1.

Hence, disable L1 capability in the LNKCAP register if supports-clkreq
is not set in the DT explicitly. For devices that set
IMX_PCIE_FLAG_BROKEN_SUSPEND, disable L1 on resume as well, as these
skip imx_pcie_host_init().

Note, the logic here is very similar to 613f3255a35a, only difference
is that leaving L0s enabled does not cause issues (at least
I never encountered them myself when testing on the device) as its
not dependent on CLKREQ# being physically there.
Since there is already a way of specifying its presence in the DT,
the driver should be aware and disable L1.

Fixes: f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for devicetree platforms")
Signed-off-by: Michal Gorlas <michal.gorlas@9elements.com>
Tested-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
---
Changes in v2:
- disable L1 unconditionally on resume for platforms advertising
IMX_PCIE_FLAG_BROKEN_SUSPEND (Sashiko)
- typos in the commit message
- Link to v1: https://patch.msgid.link/20260719-l1-conditional-imx-v1-1-a55a57915c68@9elements.com
---
 drivers/pci/controller/dwc/pci-imx6.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 98e1db751132..1fdcbcd2422e 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1318,6 +1318,22 @@ static void imx_pcie_assert_perst(struct imx_pcie *imx_pcie, bool assert)
 	}
 }
 
+static void imx_pcie_disable_l1(struct dw_pcie *pci)
+{
+	u32 val;
+	u8 offset;
+
+	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+
+	dw_pcie_dbi_ro_wr_en(pci);
+
+	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
+	val &= ~PCI_EXP_LNKCAP_ASPM_L1;
+	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, val);
+
+	dw_pcie_dbi_ro_wr_dis(pci);
+}
+
 static int imx_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -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);
 
 		/*
 		 * Using PCIE_TEST_PD seems to disable MSI and powers down the

---
base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
change-id: 20260713-l1-conditional-imx-c2bfd20ae085

Best regards,
--  
Michal Gorlas <michal.gorlas@9elements.com>


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] PCI: imx6: Enable ASPM L1 only iff CLKREQ# is supported
  2026-07-31 13:44 [PATCH v2] PCI: imx6: Enable ASPM L1 only iff CLKREQ# is supported Michal Gorlas
@ 2026-07-31 13:54 ` sashiko-bot
  2026-07-31 15:27   ` Michal Gorlas
  0 siblings, 1 reply; 3+ messages in thread
From: sashiko-bot @ 2026-07-31 13:54 UTC (permalink / raw)
  To: Michal Gorlas; +Cc: linux-pci, imx, Frank.Li

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 <michal.gorlas@9elements.com>

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/20260719-l1-conditional-imx-v1-1-a55a57915c68@9elements.com

> 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/20260731-l1-conditional-imx-v2-1-6016ee9148b2@9elements.com?part=1

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] PCI: imx6: Enable ASPM L1 only iff CLKREQ# is supported
  2026-07-31 13:54 ` sashiko-bot
@ 2026-07-31 15:27   ` Michal Gorlas
  0 siblings, 0 replies; 3+ messages in thread
From: Michal Gorlas @ 2026-07-31 15:27 UTC (permalink / raw)
  To: sashiko-reviews, Michal Gorlas; +Cc: linux-pci, imx, Frank.Li

On Fri Jul 31, 2026 at 3:54 PM CEST, sashiko-bot wrote:
> 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 <michal.gorlas@9elements.com>
>
> 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/20260719-l1-conditional-imx-v1-1-a55a57915c68@9elements.com
>
>> 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.

Platforms that set IMX_PCIE_FLAG_BROKEN_SUSPEND (i.MX6Q) do not
set supports-clkreq in their DTs. So no, such scenario won't occur.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-07-31 15:27 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-31 13:44 [PATCH v2] PCI: imx6: Enable ASPM L1 only iff CLKREQ# is supported Michal Gorlas
2026-07-31 13:54 ` sashiko-bot
2026-07-31 15:27   ` Michal Gorlas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox