[PATCH 0/2] PCI/ASPM: Allow quirks to avoid L0s and L1

[PATCH 0/2] PCI/ASPM: Allow quirks to avoid L0s and L1

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

We enabled ASPM too aggressively in v6.18-rc1.  f3ac2ff14834 ("PCI/ASPM:
Enable all ClockPM and ASPM states for devicetree platforms") enabled ASPM
L0s, L1, and (if advertised) L1 PM Substates.

L1 PM Substates and Clock PM in particular are a problem because they
depend on CLKREQ# and sometimes device-specific configuration, and none of
this is discoverable in a generic way.

df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
(v6.18-rc3) backed off and omitted Clock PM and L1 Substates.

L0s and L1 are generically discoverable, but some devices advertise them
even though they don't work correctly.  This series is a way to avoid L0s
and L1 in that case.

Bjorn Helgaas (2):
  PCI/ASPM: Cache Link Capabilities so quirks can override them
  PCI/ASPM: Avoid L0s and L1 on Freescale Root Ports

 drivers/pci/pcie/aspm.c | 42 ++++++++++++++++++++---------------------
 drivers/pci/probe.c     |  5 ++---
 drivers/pci/quirks.c    | 12 ++++++++++++
 include/linux/pci.h     |  1 +
 4 files changed, 36 insertions(+), 24 deletions(-)

-- 
2.43.0

[PATCH 1/2] PCI/ASPM: Cache Link Capabilities so quirks can override them

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Cache the PCIe Link Capabilities register in struct pci_dev so quirks can
remove features to avoid hardware defects.  The idea is:

  - set_pcie_port_type() reads PCIe Link Capabilities and caches it in
    dev->lnkcap

  - HEADER quirks can update the cached dev->lnkcap to remove advertised
    features that don't work correctly

  - pcie_aspm_cap_init() relies on dev->lnkcap and ignores any features not
    advertised there

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 42 ++++++++++++++++++++---------------------
 drivers/pci/probe.c     |  5 ++---
 include/linux/pci.h     |  1 +
 3 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 7cc8281e7011..07536891f1f6 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -391,15 +391,13 @@ static void pcie_clkpm_override_default_link_state(struct pcie_link_state *link,
 static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	int capable = 1, enabled = 1;
-	u32 reg32;
 	u16 reg16;
 	struct pci_dev *child;
 	struct pci_bus *linkbus = link->pdev->subordinate;
 
 	/* All functions should have the same cap and state, take the worst */
 	list_for_each_entry(child, &linkbus->devices, bus_list) {
-		pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &reg32);
-		if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
+		if (!(child->lnkcap & PCI_EXP_LNKCAP_CLKPM)) {
 			capable = 0;
 			enabled = 0;
 			break;
@@ -581,7 +579,7 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
 
 static void pcie_aspm_check_latency(struct pci_dev *endpoint)
 {
-	u32 latency, encoding, lnkcap_up, lnkcap_dw;
+	u32 latency, encoding;
 	u32 l1_switch_latency = 0, latency_up_l0s;
 	u32 latency_up_l1, latency_dw_l0s, latency_dw_l1;
 	u32 acceptable_l0s, acceptable_l1;
@@ -606,14 +604,10 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
 		struct pci_dev *dev = pci_function_0(link->pdev->subordinate);
 
 		/* Read direction exit latencies */
-		pcie_capability_read_dword(link->pdev, PCI_EXP_LNKCAP,
-					   &lnkcap_up);
-		pcie_capability_read_dword(dev, PCI_EXP_LNKCAP,
-					   &lnkcap_dw);
-		latency_up_l0s = calc_l0s_latency(lnkcap_up);
-		latency_up_l1 = calc_l1_latency(lnkcap_up);
-		latency_dw_l0s = calc_l0s_latency(lnkcap_dw);
-		latency_dw_l1 = calc_l1_latency(lnkcap_dw);
+		latency_up_l0s = calc_l0s_latency(link->pdev->lnkcap);
+		latency_up_l1 = calc_l1_latency(link->pdev->lnkcap);
+		latency_dw_l0s = calc_l0s_latency(dev->lnkcap);
+		latency_dw_l1 = calc_l1_latency(dev->lnkcap);
 
 		/* Check upstream direction L0s latency */
 		if ((link->aspm_capable & PCIE_LINK_STATE_L0S_UP) &&
@@ -830,7 +824,7 @@ static void pcie_aspm_override_default_link_state(struct pcie_link_state *link)
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
-	u32 parent_lnkcap, child_lnkcap;
+	u32 lnkcap;
 	u16 parent_lnkctl, child_lnkctl;
 	struct pci_bus *linkbus = parent->subordinate;
 
@@ -845,9 +839,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	 * If ASPM not supported, don't mess with the clocks and link,
 	 * bail out now.
 	 */
-	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &parent_lnkcap);
-	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &child_lnkcap);
-	if (!(parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPMS))
+	if (!(parent->lnkcap & child->lnkcap & PCI_EXP_LNKCAP_ASPMS))
 		return;
 
 	/* Configure common clock before checking latencies */
@@ -857,10 +849,18 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	 * Re-read upstream/downstream components' register state after
 	 * clock configuration.  L0s & L1 exit latencies in the otherwise
 	 * read-only Link Capabilities may change depending on common clock
-	 * configuration (PCIe r5.0, sec 7.5.3.6).
+	 * configuration (PCIe r5.0, sec 7.5.3.6).  Update only the exit
+	 * latencies in the cached dev->lnkcap because quirks may have
+	 * removed broken features advertised by the device.
 	 */
-	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &parent_lnkcap);
-	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &child_lnkcap);
+	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &lnkcap);
+	parent->lnkcap &= ~(PCI_EXP_LNKCAP_L0SEL | PCI_EXP_LNKCAP_L1EL);
+	parent->lnkcap |= lnkcap & (PCI_EXP_LNKCAP_L0SEL | PCI_EXP_LNKCAP_L1EL);
+
+	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &lnkcap);
+	child->lnkcap &= ~(PCI_EXP_LNKCAP_L0SEL | PCI_EXP_LNKCAP_L1EL);
+	child->lnkcap |= lnkcap & (PCI_EXP_LNKCAP_L0SEL | PCI_EXP_LNKCAP_L1EL);
+
 	pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &parent_lnkctl);
 	pcie_capability_read_word(child, PCI_EXP_LNKCTL, &child_lnkctl);
 
@@ -880,7 +880,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	 * given link unless components on both sides of the link each
 	 * support L0s.
 	 */
-	if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
+	if (parent->lnkcap & child->lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
 		link->aspm_support |= PCIE_LINK_STATE_L0S;
 
 	if (child_lnkctl & PCI_EXP_LNKCTL_ASPM_L0S)
@@ -889,7 +889,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 		link->aspm_enabled |= PCIE_LINK_STATE_L0S_DW;
 
 	/* Setup L1 state */
-	if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L1)
+	if (parent->lnkcap & child->lnkcap & PCI_EXP_LNKCAP_ASPM_L1)
 		link->aspm_support |= PCIE_LINK_STATE_L1;
 
 	if (parent_lnkctl & child_lnkctl & PCI_EXP_LNKCTL_ASPM_L1)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index c83e75a0ec12..db4635b1ec47 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1640,7 +1640,6 @@ void set_pcie_port_type(struct pci_dev *pdev)
 {
 	int pos;
 	u16 reg16;
-	u32 reg32;
 	int type;
 	struct pci_dev *parent;
 
@@ -1659,8 +1658,8 @@ void set_pcie_port_type(struct pci_dev *pdev)
 	pci_read_config_dword(pdev, pos + PCI_EXP_DEVCAP, &pdev->devcap);
 	pdev->pcie_mpss = FIELD_GET(PCI_EXP_DEVCAP_PAYLOAD, pdev->devcap);
 
-	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &reg32);
-	if (reg32 & PCI_EXP_LNKCAP_DLLLARC)
+	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &pdev->lnkcap);
+	if (pdev->lnkcap & PCI_EXP_LNKCAP_DLLLARC)
 		pdev->link_active_reporting = 1;
 
 	parent = pci_upstream_bridge(pdev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d1fdf81fbe1e..ec4133ae9cae 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -361,6 +361,7 @@ struct pci_dev {
 	struct pci_dev  *rcec;          /* Associated RCEC device */
 #endif
 	u32		devcap;		/* PCIe Device Capabilities */
+	u32		lnkcap;		/* PCIe Link Capabilities */
 	u16		rebar_cap;	/* Resizable BAR capability offset */
 	u8		pcie_cap;	/* PCIe capability offset */
 	u8		msi_cap;	/* MSI capability offset */
-- 
2.43.0

[PATCH 2/2] PCI/ASPM: Avoid L0s and L1 on Freescale Root Ports

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Christian reported that f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and
ASPM states for devicetree platforms") broke booting on the A-EON X5000.

Fixes: f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for devicetree platforms")
Fixes: df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms"
)
Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
Link: https://lore.kernel.org/r/db5c95a1-cf3e-46f9-8045-a1b04908051a@xenosoft.de
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/quirks.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 214ed060ca1b..44e780718953 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2525,6 +2525,18 @@ static void quirk_disable_aspm_l0s_l1(struct pci_dev *dev)
  */
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ASMEDIA, 0x1080, quirk_disable_aspm_l0s_l1);
 
+/*
+ * Remove ASPM L0s and L1 support from cached copy of Link Capabilities so
+ * aspm.c won't try to enable them.
+ */
+static void quirk_disable_aspm_l0s_l1_cap(struct pci_dev *dev)
+{
+	dev->lnkcap &= ~PCI_EXP_LNKCAP_ASPM_L0S;
+	dev->lnkcap &= ~PCI_EXP_LNKCAP_ASPM_L1;
+	pci_info(dev, "ASPM: L0s L1 removed from Link Capabilities to work around device defect\n");
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, 0x0451, quirk_disable_aspm_l0s_l1_cap);
+
 /*
  * Some Pericom PCIe-to-PCI bridges in reverse mode need the PCIe Retrain
  * Link bit cleared after starting the link retrain process to allow this
-- 
2.43.0

Re: [PATCH 0/2] PCI/ASPM: Allow quirks to avoid L0s and L1

[Bjorn Helgaas]

On Thu, Nov 06, 2025 at 12:36:37PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> We enabled ASPM too aggressively in v6.18-rc1.  f3ac2ff14834 ("PCI/ASPM:
> Enable all ClockPM and ASPM states for devicetree platforms") enabled ASPM
> L0s, L1, and (if advertised) L1 PM Substates.
> 
> L1 PM Substates and Clock PM in particular are a problem because they
> depend on CLKREQ# and sometimes device-specific configuration, and none of
> this is discoverable in a generic way.
> 
> df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> (v6.18-rc3) backed off and omitted Clock PM and L1 Substates.
> 
> L0s and L1 are generically discoverable, but some devices advertise them
> even though they don't work correctly.  This series is a way to avoid L0s
> and L1 in that case.
> 
> Bjorn Helgaas (2):
>   PCI/ASPM: Cache Link Capabilities so quirks can override them
>   PCI/ASPM: Avoid L0s and L1 on Freescale Root Ports
> 
>  drivers/pci/pcie/aspm.c | 42 ++++++++++++++++++++---------------------
>  drivers/pci/probe.c     |  5 ++---
>  drivers/pci/quirks.c    | 12 ++++++++++++
>  include/linux/pci.h     |  1 +
>  4 files changed, 36 insertions(+), 24 deletions(-)

I put these on for-linus, hopefully for v6.18.  I would like to have
some review and testing before asking Linus to pull them, especially
since the first one is not completely trivial and is a change (but
shouldn't be a functional change) for all platforms.

Re: [PATCH 1/2] PCI/ASPM: Cache Link Capabilities so quirks can override them

[Shawn Lin]

在 2025/11/07 星期五 2:36, Bjorn Helgaas 写道:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Cache the PCIe Link Capabilities register in struct pci_dev so quirks can
> remove features to avoid hardware defects.  The idea is:
> 
>    - set_pcie_port_type() reads PCIe Link Capabilities and caches it in
>      dev->lnkcap
> 
>    - HEADER quirks can update the cached dev->lnkcap to remove advertised
>      features that don't work correctly
> 
>    - pcie_aspm_cap_init() relies on dev->lnkcap and ignores any features not
>      advertised there
> 

Quick test with a NVMe shows it works.

Before this patch,  lspci -vvv dumps:

  LnkCap: Port #0, Speed 16GT/s, Width x4, ASPM L1, Exit Latency L1 <64us
          ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp+
  LnkCtl: ASPM L1 Enabled; RCB 64 bytes, LnkDisable- CommClk+


Capabilities: [21c v1] L1 PM Substates
          L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ 
L1_PM_Substates+
                    PortCommonModeRestoreTime=10us PortTPowerOnTime=10us
          L1SubCtl1: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
                     T_CommonMode=0us LTR1.2_Threshold=26016ns

After this patch + a local quirk patch like your patch 2, it shows:

  LnkCap: Port #0, Speed 16GT/s, Width x4, ASPM L1, Exit Latency L1 <64us
          ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp+
  LnkCtl: ASPM Disabled; RCB 64 bytes, LnkDisable- CommClk-

Capabilities: [21c v1] L1 PM Substates
           L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ 
L1_PM_Substates+
                     PortCommonModeRestoreTime=10us PortTPowerOnTime=10us
           L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1-
                      T_CommonMode=0us LTR1.2_Threshold=0ns



One things I noticed is CommClk in LnkCtl is changed. Per the spec,

"A value of 0b indicates that this component and the component at the
opposite end of this Link are operating with asynchronous reference
clock.

Components utilize this Common Clock Configuration information to report
the correct L0s and L1 Exit Latencies. After changing the value in this
bit in both components on a Link, software must trigger the Link to
retrain by writing a 1b to the Retrain Link bit of the Downstream Port."

Obviously my NVMe and RC are operating with common reference clk. So
CommClk- looks wrong to me. And should we also perform Retrain Link if
we really wants to change it?


> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   drivers/pci/pcie/aspm.c | 42 ++++++++++++++++++++---------------------
>   drivers/pci/probe.c     |  5 ++---
>   include/linux/pci.h     |  1 +
>   3 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 7cc8281e7011..07536891f1f6 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -391,15 +391,13 @@ static void pcie_clkpm_override_default_link_state(struct pcie_link_state *link,
>   static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
>   {
>   	int capable = 1, enabled = 1;
> -	u32 reg32;
>   	u16 reg16;
>   	struct pci_dev *child;
>   	struct pci_bus *linkbus = link->pdev->subordinate;
>   
>   	/* All functions should have the same cap and state, take the worst */
>   	list_for_each_entry(child, &linkbus->devices, bus_list) {
> -		pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &reg32);
> -		if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
> +		if (!(child->lnkcap & PCI_EXP_LNKCAP_CLKPM)) {
>   			capable = 0;
>   			enabled = 0;
>   			break;
> @@ -581,7 +579,7 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
>   
>   static void pcie_aspm_check_latency(struct pci_dev *endpoint)
>   {
> -	u32 latency, encoding, lnkcap_up, lnkcap_dw;
> +	u32 latency, encoding;
>   	u32 l1_switch_latency = 0, latency_up_l0s;
>   	u32 latency_up_l1, latency_dw_l0s, latency_dw_l1;
>   	u32 acceptable_l0s, acceptable_l1;
> @@ -606,14 +604,10 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
>   		struct pci_dev *dev = pci_function_0(link->pdev->subordinate);
>   
>   		/* Read direction exit latencies */
> -		pcie_capability_read_dword(link->pdev, PCI_EXP_LNKCAP,
> -					   &lnkcap_up);
> -		pcie_capability_read_dword(dev, PCI_EXP_LNKCAP,
> -					   &lnkcap_dw);
> -		latency_up_l0s = calc_l0s_latency(lnkcap_up);
> -		latency_up_l1 = calc_l1_latency(lnkcap_up);
> -		latency_dw_l0s = calc_l0s_latency(lnkcap_dw);
> -		latency_dw_l1 = calc_l1_latency(lnkcap_dw);
> +		latency_up_l0s = calc_l0s_latency(link->pdev->lnkcap);
> +		latency_up_l1 = calc_l1_latency(link->pdev->lnkcap);
> +		latency_dw_l0s = calc_l0s_latency(dev->lnkcap);
> +		latency_dw_l1 = calc_l1_latency(dev->lnkcap);
>   
>   		/* Check upstream direction L0s latency */
>   		if ((link->aspm_capable & PCIE_LINK_STATE_L0S_UP) &&
> @@ -830,7 +824,7 @@ static void pcie_aspm_override_default_link_state(struct pcie_link_state *link)
>   static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>   {
>   	struct pci_dev *child = link->downstream, *parent = link->pdev;
> -	u32 parent_lnkcap, child_lnkcap;
> +	u32 lnkcap;
>   	u16 parent_lnkctl, child_lnkctl;
>   	struct pci_bus *linkbus = parent->subordinate;
>   
> @@ -845,9 +839,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>   	 * If ASPM not supported, don't mess with the clocks and link,
>   	 * bail out now.
>   	 */
> -	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &parent_lnkcap);
> -	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &child_lnkcap);
> -	if (!(parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPMS))
> +	if (!(parent->lnkcap & child->lnkcap & PCI_EXP_LNKCAP_ASPMS))
>   		return;
>   
>   	/* Configure common clock before checking latencies */
> @@ -857,10 +849,18 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>   	 * Re-read upstream/downstream components' register state after
>   	 * clock configuration.  L0s & L1 exit latencies in the otherwise
>   	 * read-only Link Capabilities may change depending on common clock
> -	 * configuration (PCIe r5.0, sec 7.5.3.6).
> +	 * configuration (PCIe r5.0, sec 7.5.3.6).  Update only the exit
> +	 * latencies in the cached dev->lnkcap because quirks may have
> +	 * removed broken features advertised by the device.
>   	 */
> -	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &parent_lnkcap);
> -	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &child_lnkcap);
> +	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &lnkcap);
> +	parent->lnkcap &= ~(PCI_EXP_LNKCAP_L0SEL | PCI_EXP_LNKCAP_L1EL);
> +	parent->lnkcap |= lnkcap & (PCI_EXP_LNKCAP_L0SEL | PCI_EXP_LNKCAP_L1EL);
> +
> +	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &lnkcap);
> +	child->lnkcap &= ~(PCI_EXP_LNKCAP_L0SEL | PCI_EXP_LNKCAP_L1EL);
> +	child->lnkcap |= lnkcap & (PCI_EXP_LNKCAP_L0SEL | PCI_EXP_LNKCAP_L1EL);
> +
>   	pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &parent_lnkctl);
>   	pcie_capability_read_word(child, PCI_EXP_LNKCTL, &child_lnkctl);
>   
> @@ -880,7 +880,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>   	 * given link unless components on both sides of the link each
>   	 * support L0s.
>   	 */
> -	if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
> +	if (parent->lnkcap & child->lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
>   		link->aspm_support |= PCIE_LINK_STATE_L0S;
>   
>   	if (child_lnkctl & PCI_EXP_LNKCTL_ASPM_L0S)
> @@ -889,7 +889,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>   		link->aspm_enabled |= PCIE_LINK_STATE_L0S_DW;
>   
>   	/* Setup L1 state */
> -	if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L1)
> +	if (parent->lnkcap & child->lnkcap & PCI_EXP_LNKCAP_ASPM_L1)
>   		link->aspm_support |= PCIE_LINK_STATE_L1;
>   
>   	if (parent_lnkctl & child_lnkctl & PCI_EXP_LNKCTL_ASPM_L1)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index c83e75a0ec12..db4635b1ec47 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1640,7 +1640,6 @@ void set_pcie_port_type(struct pci_dev *pdev)
>   {
>   	int pos;
>   	u16 reg16;
> -	u32 reg32;
>   	int type;
>   	struct pci_dev *parent;
>   
> @@ -1659,8 +1658,8 @@ void set_pcie_port_type(struct pci_dev *pdev)
>   	pci_read_config_dword(pdev, pos + PCI_EXP_DEVCAP, &pdev->devcap);
>   	pdev->pcie_mpss = FIELD_GET(PCI_EXP_DEVCAP_PAYLOAD, pdev->devcap);
>   
> -	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &reg32);
> -	if (reg32 & PCI_EXP_LNKCAP_DLLLARC)
> +	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &pdev->lnkcap);
> +	if (pdev->lnkcap & PCI_EXP_LNKCAP_DLLLARC)
>   		pdev->link_active_reporting = 1;
>   
>   	parent = pci_upstream_bridge(pdev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d1fdf81fbe1e..ec4133ae9cae 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -361,6 +361,7 @@ struct pci_dev {
>   	struct pci_dev  *rcec;          /* Associated RCEC device */
>   #endif
>   	u32		devcap;		/* PCIe Device Capabilities */
> +	u32		lnkcap;		/* PCIe Link Capabilities */
>   	u16		rebar_cap;	/* Resizable BAR capability offset */
>   	u8		pcie_cap;	/* PCIe capability offset */
>   	u8		msi_cap;	/* MSI capability offset */

RE: [PATCH 0/2] PCI/ASPM: Allow quirks to avoid L0s and L1

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2025年11月7日 2:37
> To: linux-pci@vger.kernel.org
> Cc: Christian Zigotzky <chzigotzky@xenosoft.de>; Manivannan Sadhasivam
> <mani@kernel.org>; mad skateman <madskateman@gmail.com>; R . T .
> Dickinson <rtd2@xtra.co.nz>; Darren Stevens <darren@stevens-zone.net>;
> John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>; Lukas Wunner
> <lukas@wunner.de>; luigi burdo <intermediadc@hotmail.com>; Al
> <al@datazap.net>; Roland <rol7and@gmx.com>; Hongxing Zhu
> <hongxing.zhu@nxp.com>; hypexed@yahoo.com.au;
> linuxppc-dev@lists.ozlabs.org; debian-powerpc@lists.debian.org;
> linux-kernel@vger.kernel.org; Bjorn Helgaas <bhelgaas@google.com>
> Subject: [PATCH 0/2] PCI/ASPM: Allow quirks to avoid L0s and L1
> 
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> We enabled ASPM too aggressively in v6.18-rc1.  f3ac2ff14834 ("PCI/ASPM:
> Enable all ClockPM and ASPM states for devicetree platforms") enabled
> ASPM L0s, L1, and (if advertised) L1 PM Substates.
> 
> L1 PM Substates and Clock PM in particular are a problem because they
> depend on CLKREQ# and sometimes device-specific configuration, and none
> of this is discoverable in a generic way.
> 
> df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> (v6.18-rc3) backed off and omitted Clock PM and L1 Substates.
> 
> L0s and L1 are generically discoverable, but some devices advertise them
> even though they don't work correctly.  This series is a way to avoid L0s and
> L1 in that case.
> 
> Bjorn Helgaas (2):
>   PCI/ASPM: Cache Link Capabilities so quirks can override them
>   PCI/ASPM: Avoid L0s and L1 on Freescale Root Ports

Tested-by: Richard Zhu <hongxing.zhu@nxp.com>

Best Regards
Richard Zhu

> 
>  drivers/pci/pcie/aspm.c | 42 ++++++++++++++++++++---------------------
>  drivers/pci/probe.c     |  5 ++---
>  drivers/pci/quirks.c    | 12 ++++++++++++
>  include/linux/pci.h     |  1 +
>  4 files changed, 36 insertions(+), 24 deletions(-)
> 
> --
> 2.43.0

Re: [PATCH 1/2] PCI/ASPM: Cache Link Capabilities so quirks can override them

[Lukas Wunner]

On Thu, Nov 06, 2025 at 12:36:38PM -0600, Bjorn Helgaas wrote:
> Cache the PCIe Link Capabilities register in struct pci_dev so quirks can
> remove features to avoid hardware defects.  The idea is:
> 
>   - set_pcie_port_type() reads PCIe Link Capabilities and caches it in
>     dev->lnkcap
> 
>   - HEADER quirks can update the cached dev->lnkcap to remove advertised
>     features that don't work correctly
> 
>   - pcie_aspm_cap_init() relies on dev->lnkcap and ignores any features not
>     advertised there

I realize that memory is cheap, but it still feels a bit wasteful
to cache the entire 32-bit register wholesale.  It contains
reserved bits as of PCIe r7.0, various uninteresting bits and
portions of it are already cached elsewhere and thus now duplicated.
I'm wondering if it would make sense to instead only cache the ASPM bits
that are relevant here?  That's the approach we've followed so far.

You're initializing the link_active_reporting bit from the newly
cached lnkcap register, I'd prefer having a static inline instead
which extracts the bit from the cached register on demand,
thus obviating the need to have a duplicate cached copy of the bit.

pci_set_bus_speed() caches bus->max_bus_speed from the Link
Capabilities register and isn't converted by this patch to use
the cached register.  There are various others, e.g.
get_port_device_capability() in drivers/pci/pcie/portdrv.c
could also get PCI_EXP_LNKCAP_LBNC from the cached lnkcap
register.  Same for pcie_get_supported_speeds().  If the
intention is to convert these in a separate step in v6.19,
it would be good to mention that in the changelog.

Thanks,

Lukas

Re: [PATCH 2/2] PCI/ASPM: Avoid L0s and L1 on Freescale Root Ports

[Lukas Wunner]

On Thu, Nov 06, 2025 at 12:36:39PM -0600, Bjorn Helgaas wrote:
> +++ b/drivers/pci/quirks.c
> @@ -2525,6 +2525,18 @@ static void quirk_disable_aspm_l0s_l1(struct pci_dev *dev)
>   */
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ASMEDIA, 0x1080, quirk_disable_aspm_l0s_l1);
>  
> +/*
> + * Remove ASPM L0s and L1 support from cached copy of Link Capabilities so
> + * aspm.c won't try to enable them.
> + */
> +static void quirk_disable_aspm_l0s_l1_cap(struct pci_dev *dev)
> +{
> +	dev->lnkcap &= ~PCI_EXP_LNKCAP_ASPM_L0S;
> +	dev->lnkcap &= ~PCI_EXP_LNKCAP_ASPM_L1;
> +	pci_info(dev, "ASPM: L0s L1 removed from Link Capabilities to work around device defect\n");
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, 0x0451, quirk_disable_aspm_l0s_l1_cap);
> +

Hm, I liked the nice generic pcie_aspm_disable_cap() helper that
you had in this earlier version:

https://lore.kernel.org/all/20251105220925.GA1926619@bhelgaas/

Thanks,

Lukas

Re: [PATCH 0/2] PCI/ASPM: Allow quirks to avoid L0s and L1

[Manivannan Sadhasivam]

On Thu, Nov 06, 2025 at 12:36:37PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> We enabled ASPM too aggressively in v6.18-rc1.  f3ac2ff14834 ("PCI/ASPM:
> Enable all ClockPM and ASPM states for devicetree platforms") enabled ASPM
> L0s, L1, and (if advertised) L1 PM Substates.
> 
> L1 PM Substates and Clock PM in particular are a problem because they
> depend on CLKREQ# and sometimes device-specific configuration, and none of
> this is discoverable in a generic way.
> 
> df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> (v6.18-rc3) backed off and omitted Clock PM and L1 Substates.
> 
> L0s and L1 are generically discoverable, but some devices advertise them
> even though they don't work correctly.  This series is a way to avoid L0s
> and L1 in that case.
> 

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Tested-by: Manivannan Sadhasivam <mani@kernel.org> # T14s

- Mani

> Bjorn Helgaas (2):
>   PCI/ASPM: Cache Link Capabilities so quirks can override them
>   PCI/ASPM: Avoid L0s and L1 on Freescale Root Ports
> 
>  drivers/pci/pcie/aspm.c | 42 ++++++++++++++++++++---------------------
>  drivers/pci/probe.c     |  5 ++---
>  drivers/pci/quirks.c    | 12 ++++++++++++
>  include/linux/pci.h     |  1 +
>  4 files changed, 36 insertions(+), 24 deletions(-)
> 
> -- 
> 2.43.0
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/2] PCI/ASPM: Cache Link Capabilities so quirks can override them

[Manivannan Sadhasivam]

On Fri, Nov 07, 2025 at 09:17:09AM +0800, Shawn Lin wrote:
> 在 2025/11/07 星期五 2:36, Bjorn Helgaas 写道:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Cache the PCIe Link Capabilities register in struct pci_dev so quirks can
> > remove features to avoid hardware defects.  The idea is:
> > 
> >    - set_pcie_port_type() reads PCIe Link Capabilities and caches it in
> >      dev->lnkcap
> > 
> >    - HEADER quirks can update the cached dev->lnkcap to remove advertised
> >      features that don't work correctly
> > 
> >    - pcie_aspm_cap_init() relies on dev->lnkcap and ignores any features not
> >      advertised there
> > 
> 
> Quick test with a NVMe shows it works.
> 
> Before this patch,  lspci -vvv dumps:
> 
>  LnkCap: Port #0, Speed 16GT/s, Width x4, ASPM L1, Exit Latency L1 <64us
>          ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp+
>  LnkCtl: ASPM L1 Enabled; RCB 64 bytes, LnkDisable- CommClk+
> 
> 
> Capabilities: [21c v1] L1 PM Substates
>          L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
> L1_PM_Substates+
>                    PortCommonModeRestoreTime=10us PortTPowerOnTime=10us
>          L1SubCtl1: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
>                     T_CommonMode=0us LTR1.2_Threshold=26016ns
> 
> After this patch + a local quirk patch like your patch 2, it shows:
> 
>  LnkCap: Port #0, Speed 16GT/s, Width x4, ASPM L1, Exit Latency L1 <64us
>          ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp+
>  LnkCtl: ASPM Disabled; RCB 64 bytes, LnkDisable- CommClk-
> 
> Capabilities: [21c v1] L1 PM Substates
>           L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
> L1_PM_Substates+
>                     PortCommonModeRestoreTime=10us PortTPowerOnTime=10us
>           L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1-
>                      T_CommonMode=0us LTR1.2_Threshold=0ns
> 
> 
> 
> One things I noticed is CommClk in LnkCtl is changed.

That's not because of this series, but because of your quirk that disables L0s
and L1. Common Clock Configuration happens only when ASPM is enabled, if it is
disabled, PCI core will not configure it (the value remains untouched). That's
why it was enabled before your quirk and disabled afterwards.

This bit is also only used to report the L0s and L1 Exit latencies by the
devices.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 2/2] PCI/ASPM: Avoid L0s and L1 on Freescale Root Ports

[Manivannan Sadhasivam]

On Thu, Nov 06, 2025 at 12:36:39PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Christian reported that f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and
> ASPM states for devicetree platforms") broke booting on the A-EON X5000.
> 
> Fixes: f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for devicetree platforms")
> Fixes: df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms"
> )
> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
> Link: https://lore.kernel.org/r/db5c95a1-cf3e-46f9-8045-a1b04908051a@xenosoft.de
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/quirks.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 214ed060ca1b..44e780718953 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2525,6 +2525,18 @@ static void quirk_disable_aspm_l0s_l1(struct pci_dev *dev)
>   */
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ASMEDIA, 0x1080, quirk_disable_aspm_l0s_l1);
>  
> +/*
> + * Remove ASPM L0s and L1 support from cached copy of Link Capabilities so
> + * aspm.c won't try to enable them.
> + */
> +static void quirk_disable_aspm_l0s_l1_cap(struct pci_dev *dev)
> +{
> +	dev->lnkcap &= ~PCI_EXP_LNKCAP_ASPM_L0S;
> +	dev->lnkcap &= ~PCI_EXP_LNKCAP_ASPM_L1;
> +	pci_info(dev, "ASPM: L0s L1 removed from Link Capabilities to work around device defect\n");
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, 0x0451, quirk_disable_aspm_l0s_l1_cap);

From the commit message of the earlier version [1] you shared:

    Removing advertised features prevents aspm.c from enabling them, even if
    users try to enable them via sysfs or by building the kernel with
    CONFIG_PCIEASPM_POWERSAVE or CONFIG_PCIEASPM_POWER_SUPERSAVE.

Going by this reasoning, shouldn't we be doing this for the other quirks
(quirk_disable_aspm_l0s_l1/quirk_disable_aspm_l0s) as well?

- Mani

[1] https://lore.kernel.org/linux-pci/20251105220925.GA1926619@bhelgaas

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/2] PCI/ASPM: Cache Link Capabilities so quirks can override them

[Shawn Lin]

在 2025/11/07 星期五 14:03, Manivannan Sadhasivam 写道:
> On Fri, Nov 07, 2025 at 09:17:09AM +0800, Shawn Lin wrote:
>> 在 2025/11/07 星期五 2:36, Bjorn Helgaas 写道:
>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> Cache the PCIe Link Capabilities register in struct pci_dev so quirks can
>>> remove features to avoid hardware defects.  The idea is:
>>>
>>>     - set_pcie_port_type() reads PCIe Link Capabilities and caches it in
>>>       dev->lnkcap
>>>
>>>     - HEADER quirks can update the cached dev->lnkcap to remove advertised
>>>       features that don't work correctly
>>>
>>>     - pcie_aspm_cap_init() relies on dev->lnkcap and ignores any features not
>>>       advertised there
>>>
>>
>> Quick test with a NVMe shows it works.
>>
>> Before this patch,  lspci -vvv dumps:
>>
>>   LnkCap: Port #0, Speed 16GT/s, Width x4, ASPM L1, Exit Latency L1 <64us
>>           ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp+
>>   LnkCtl: ASPM L1 Enabled; RCB 64 bytes, LnkDisable- CommClk+
>>
>>
>> Capabilities: [21c v1] L1 PM Substates
>>           L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
>> L1_PM_Substates+
>>                     PortCommonModeRestoreTime=10us PortTPowerOnTime=10us
>>           L1SubCtl1: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
>>                      T_CommonMode=0us LTR1.2_Threshold=26016ns
>>
>> After this patch + a local quirk patch like your patch 2, it shows:
>>
>>   LnkCap: Port #0, Speed 16GT/s, Width x4, ASPM L1, Exit Latency L1 <64us
>>           ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp+
>>   LnkCtl: ASPM Disabled; RCB 64 bytes, LnkDisable- CommClk-
>>
>> Capabilities: [21c v1] L1 PM Substates
>>            L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
>> L1_PM_Substates+
>>                      PortCommonModeRestoreTime=10us PortTPowerOnTime=10us
>>            L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1-
>>                       T_CommonMode=0us LTR1.2_Threshold=0ns
>>
>>
>>
>> One things I noticed is CommClk in LnkCtl is changed.
> 
> That's not because of this series, but because of your quirk that disables L0s
> and L1. Common Clock Configuration happens only when ASPM is enabled, if it is
> disabled, PCI core will not configure it (the value remains untouched). That's
> why it was enabled before your quirk and disabled afterwards.
> 

Thanks for the detailed explanation, I have no more questions now.

> This bit is also only used to report the L0s and L1 Exit latencies by the
> devices.
> 
> - Mani
> 

Re: [PATCH 0/2] PCI/ASPM: Allow quirks to avoid L0s and L1

[Lukas Wunner]

On Thu, Nov 06, 2025 at 12:36:37PM -0600, Bjorn Helgaas wrote:
> L1 PM Substates and Clock PM in particular are a problem because they
> depend on CLKREQ# and sometimes device-specific configuration, and none of
> this is discoverable in a generic way.

According to PCIe r7.0 sec 7.5.3.7, the "Enable Clock Power Management"
bit is "applicable only for Upstream Ports and with form factors that
support a Clock Request (CLKREQ#) mechanism".

Thus, if BIOS has set the "Enable Clock Power Management" bit
on a Downstream Port, we can infer that CLKREQ# is supported.

Thanks,

Lukas

Re: [PATCH 1/2] PCI/ASPM: Cache Link Capabilities so quirks can override them

[Bjorn Helgaas]

On Fri, Nov 07, 2025 at 06:32:14AM +0100, Lukas Wunner wrote:
> On Thu, Nov 06, 2025 at 12:36:38PM -0600, Bjorn Helgaas wrote:
> > Cache the PCIe Link Capabilities register in struct pci_dev so quirks can
> > remove features to avoid hardware defects.  The idea is:
> > 
> >   - set_pcie_port_type() reads PCIe Link Capabilities and caches it in
> >     dev->lnkcap
> > 
> >   - HEADER quirks can update the cached dev->lnkcap to remove advertised
> >     features that don't work correctly
> > 
> >   - pcie_aspm_cap_init() relies on dev->lnkcap and ignores any features not
> >     advertised there
> 
> I realize that memory is cheap, but it still feels a bit wasteful
> to cache the entire 32-bit register wholesale.  It contains
> reserved bits as of PCIe r7.0, various uninteresting bits and
> portions of it are already cached elsewhere and thus now duplicated.
> I'm wondering if it would make sense to instead only cache the ASPM bits
> that are relevant here?  That's the approach we've followed so far.

My first try (which I didn't post) cached only the two bits we need
for this.  It's not awful, and the aspm.c patch was smaller, so maybe
it's the right approach, at least for v6.18.

One thing I didn't like about pci_disable_aspm_cap() (which I know you
said you *did* like :)) is that it adds a layer of indirection.  I
like having PCI_EXP_LNKCAP_ASPM_L0S in the quirk because it's more
directly connected to the spec and the hardware register, and grep
works better for code readers.

But if we only cache the ASPM cap bits, we would need
pci_disable_aspm_cap() to manage converting PCI_EXP_LNKCAP_ASPM_L0S or
PCIE_LINK_STATE_L0S to the right place.

(A bit of a tangent, but I've never liked the PCIE_LINK_STATE_* bits
because they look like they ought to be register bits, but they're
not.  I think the code would be improved overall if we could remove
them.)

> You're initializing the link_active_reporting bit from the newly
> cached lnkcap register, I'd prefer having a static inline instead
> which extracts the bit from the cached register on demand,
> thus obviating the need to have a duplicate cached copy of the bit.
> 
> pci_set_bus_speed() caches bus->max_bus_speed from the Link
> Capabilities register and isn't converted by this patch to use
> the cached register.  There are various others, e.g.
> get_port_device_capability() in drivers/pci/pcie/portdrv.c
> could also get PCI_EXP_LNKCAP_LBNC from the cached lnkcap
> register.  Same for pcie_get_supported_speeds().  If the
> intention is to convert these in a separate step in v6.19,
> it would be good to mention that in the changelog.

I agree with all of that, and there are several other PCI_EXP_LNKCAP
reads that could be replaced, but that would have to be for v6.19.

Bjorn

Re: [PATCH 2/2] PCI/ASPM: Avoid L0s and L1 on Freescale Root Ports

[Bjorn Helgaas]

On Fri, Nov 07, 2025 at 11:39:50AM +0530, Manivannan Sadhasivam wrote:
> On Thu, Nov 06, 2025 at 12:36:39PM -0600, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Christian reported that f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and
> > ASPM states for devicetree platforms") broke booting on the A-EON X5000.
> > 
> > Fixes: f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for devicetree platforms")
> > Fixes: df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms"
> > )
> > Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
> > Link: https://lore.kernel.org/r/db5c95a1-cf3e-46f9-8045-a1b04908051a@xenosoft.de
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/quirks.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 214ed060ca1b..44e780718953 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -2525,6 +2525,18 @@ static void quirk_disable_aspm_l0s_l1(struct pci_dev *dev)
> >   */
> >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ASMEDIA, 0x1080, quirk_disable_aspm_l0s_l1);
> >  
> > +/*
> > + * Remove ASPM L0s and L1 support from cached copy of Link Capabilities so
> > + * aspm.c won't try to enable them.
> > + */
> > +static void quirk_disable_aspm_l0s_l1_cap(struct pci_dev *dev)
> > +{
> > +	dev->lnkcap &= ~PCI_EXP_LNKCAP_ASPM_L0S;
> > +	dev->lnkcap &= ~PCI_EXP_LNKCAP_ASPM_L1;
> > +	pci_info(dev, "ASPM: L0s L1 removed from Link Capabilities to work around device defect\n");
> > +}
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, 0x0451, quirk_disable_aspm_l0s_l1_cap);
> 
> From the commit message of the earlier version [1] you shared:
> 
>     Removing advertised features prevents aspm.c from enabling them, even if
>     users try to enable them via sysfs or by building the kernel with
>     CONFIG_PCIEASPM_POWERSAVE or CONFIG_PCIEASPM_POWER_SUPERSAVE.
> 
> Going by this reasoning, shouldn't we be doing this for the other
> quirks (quirk_disable_aspm_l0s_l1/quirk_disable_aspm_l0s) as well?

Yes, probably so.  I was thinking that could be done later to limit
the scope of v6.18 changes, but since we're enabling L0s/L1 when we
didn't before, we should probably update those quirks too.

I was hesitant because quirk_disable_aspm_l0s_l1_cap() isn't quite the
same as quirk_disable_aspm_l0s_l1() because pci_disable_link_state()
turns off states that are currently enabled and also prevents them
from being enabled in the future, but quirk_disable_aspm_l0s_l1_cap()
essentially just clears Link Capability bits.

But if we clear a Link Capability bit for a state that was already
enabled by firmware:

  - If POLICY_DEFAULT or POLICY_PERFORMANCE, I think we'll disable the
    state during enumeration:

      pci_scan_slot
	pcie_aspm_init_link_state
	  pcie_aspm_init_link_state
	    if (POLICY_DEFAULT || POLICY_PERFORMANCE)
	      pcie_config_aspm_path

  - If POLICY_POWERSAVE or POLICY_POWER_SUPERSAVE, I think we'll
    disable it in pci_enable_device():

      pci_enable_device
	do_pci_enable_device
	  pcie_aspm_powersave_config_link
	    if (POLICY_POWERSAVE || POLICY_POWER_SUPERSAVE)
	      pcie_config_aspm_path

If firmware enabled the state, it must at least be safe enough to
boot, and we should eventually disable it regardless of how the kernel
was built.

Bjorn