RE: [PATCH] PCI: imx6: enable ASPM L1 only iff CLKREQ# is supported

[Hongxing Zhu]

Best Regards
Richard Zhu

> -----Original Message-----
> From: Michal Gorlas <michal.gorlas@xxxxxxxxxxxxx>
> Sent: Thursday, July 30, 2026 8:14 PM
> To: Manivannan Sadhasivam <mani@xxxxxxxxxx>; Hongxing Zhu
> <hongxing.zhu@xxxxxxx>; Frank Li <frank.li@xxxxxxx>; Michal Gorlas
> <michal.gorlas@xxxxxxxxxxxxx>
> Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>; Lorenzo Pieralisi
> <lpieralisi@xxxxxxxxxx>; Krzysztof Wilczyński <kwilczynski@xxxxxxxxxx>; Rob
> Herring <robh@xxxxxxxxxx>; Bjorn Helgaas <bhelgaas@xxxxxxxxxx>; Sascha
> Hauer <s.hauer@xxxxxxxxxxxxxx>; Pengutronix Kernel Team
> <kernel@xxxxxxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>; linux-
> pci@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] PCI: imx6: enable ASPM L1 only iff CLKREQ# is supported
> 
> [You don't often get email from michal.gorlas@xxxxxxxxxxxxx. Learn why this is
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Wed Jul 29, 2026 at 7:01 PM CEST, Manivannan Sadhasivam wrote:
> > On Sun, Jul 19, 2026 at 02:47:54AM +0200, Michal Gorlas wrote:
> >> 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 iMX SoCs
> >> (for e.g. iMX 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 LINKCAP register if
> >> supports-clkreq is not set in the DT explicitly.
> >>
> >> 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@xxxxxxxxxxxxx>
> >> ---
> >>  drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++++++++++++++
> >>  1 file changed, 19 insertions(+)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> >> b/drivers/pci/controller/dwc/pci-imx6.c
> >> index 98e1db751132..7471501fbf04 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);
> >
> > Sashiko raised a valid issue here:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > hwork.kernel.org%2Fproject%2Flinux-pci%2Fpatch%2F20260719-l1-condition
> > al-imx-v1-1-
> a55a57915c68%409elements.com%2F%2327076171&data=05%7C02%7C
> >
> hongxing.zhu%40nxp.com%7Cd76ee1c9a654445357a808deee341bb5%7C686ea
> 1d3bc
> >
> 2b4c6fa92cd99c5c301635%7C0%7C0%7C639210104755924878%7CUnknown%
> 7CTWFpbG
> >
> Zsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIk
> FO
> >
> IjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=VV0FrH0p73aStM7N
> TFfSwS
> > C2zJywJ3qG3Hp9PcjLuq0%3D&reserved=0
> >
> > IMX_PCIE_FLAG_BROKEN_SUSPEND is only set for i.MX6Q, but none of its
> > DTs have 'supports-clkreq' property. So this patch will impact them
> > also and since they do not reinitialize the hardware fully in
> > resume(), ASPM settings will reset to hardware default.
> 
> Right, for i.MX6Q, imx_pcie_disable_l1() could be called unconditionally in
> imx_pcie_resume_noirq() after the imx_check_flag(imx_pcie,
> IMX_PCIE_FLAG_BROKEN_SUSPEND) block, something like:
> 
> @@ -1767,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
> 
> Though I do not have imx6q hw to test this, so it would be nice if NXP folks could
> give it a test.
With this change suggested by Sashiko.
Tested on i.MX6Q Sabresd board. The ASPM L1 capability in the LNKCAP of i.MX6Q
PCIe is disabled as expected.

Tested-by: Richard Zhu <hongxing.zhu@xxxxxxx>
Reviewed-by: Richard Zhu <hongxing.zhu@xxxxxxx>
> 
> Best,
> Michal