PCI: dw-rockchip: Prevent advertising L1 Substates support

The L1 substates support requires additional steps to work, namely:

(quoted diff) +0 -2

-Proper handling of the CLKREQ# sideband signal. (It is mostly handled by
 hardware, but software still needs to set the clkreq fields in the
 PCIE_CLIENT_POWER_CON register to match the hardware implementation.)
-Program the frequency of the aux clock into the
 DSP_PCIE_PL_AUX_CLK_FREQ_OFF register. (During L1 substates the core_clk
 is turned off and the aux_clk is used instead.)

These steps are currently missing from the driver.

For more details, see section '18.6.6.4 L1 Substate' in the RK3658 TRM 1.1 Part 2, or section '11.6.6.4 L1 Substate' in the RK3588 TRM 1.0 Part2.

While this has always been a problem when using e.g. CONFIG_PCIEASPM_POWER_SUPERSAVE=y, or when modifying /sys/bus/pci/devices/.../link/l1_2_aspm, the lacking driver support for L1 substates became more apparent after commit f3ac2ff14834 ("PCI/ASPM:
Enable all ClockPM and ASPM states for devicetree platforms"), which enabled ASPM also for CONFIG_PCIEASPM_DEFAULT=y.

When using e.g. an NVMe drive connected to the PCIe controller, the
problem will be seen as:
nvme nvme0: controller is down; will reset: CSTS=0xffffffff, PCI_STATUS=0x10
nvme nvme0: Does your device have a faulty power saving mode enabled?
nvme nvme0: Try "nvme_core.default_ps_max_latency_us=0 pcie_aspm=off pcie_port_pm=off" and report a bug

Thus, prevent advertising L1 Substates support until proper driver support is added.

Cc [email protected]

Fixes 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")

Fixes f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for devicetree platforms")

Acked-by Shawn Lin <[email protected]>

Signed-off-by Niklas Cassel <[email protected]>

---
Changes since v2:

(quoted diff) +0 -1

-Improve commit message (Bjorn)
 
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

drivers/pci/controller/dwc/pcie-dw-rockchip.c +21 -0

index 3e2752c7dd09..84f882abbca5 100644
@@ -200,6 +200,25 @@ static bool rockchip_pcie_link_up(struct dw_pcie *pci)
 	return FIELD_GET(PCIE_LINKUP_MASK, val) == PCIE_LINKUP;
 }
 
+/*
+ * See e.g. section '11.6.6.4 L1 Substate' in the RK3588 TRM V1.0 for the steps
+ * needed to support L1 substates. Currently, not a single rockchip platform
+ * performs these steps, so disable L1 substates until there is proper support.
+ */
+static void rockchip_pcie_disable_l1sub(struct dw_pcie *pci)
+{
+	u32 cap, l1subcap;
+
+	cap = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_L1SS);
+	if (cap) {
+		l1subcap = dw_pcie_readl_dbi(pci, cap + PCI_L1SS_CAP);
+		l1subcap &= ~(PCI_L1SS_CAP_L1_PM_SS | PCI_L1SS_CAP_ASPM_L1_1 |
+			      PCI_L1SS_CAP_ASPM_L1_2 | PCI_L1SS_CAP_PCIPM_L1_1 |
+			      PCI_L1SS_CAP_PCIPM_L1_2);
+		dw_pcie_writel_dbi(pci, cap + PCI_L1SS_CAP, l1subcap);
+	}
+}
+
 static void rockchip_pcie_enable_l0s(struct dw_pcie *pci)
 {
 	u32 cap, lnkcap;

@@ -264,6 +283,7 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
 	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
 					 rockchip);
 
+	rockchip_pcie_disable_l1sub(pci);
 	rockchip_pcie_enable_l0s(pci);
 
 	return 0;

@@ -301,6 +321,7 @@ static void rockchip_pcie_ep_init(struct dw_pcie_ep *ep)
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 	enum pci_barno bar;
 
+	rockchip_pcie_disable_l1sub(pci);
 	rockchip_pcie_enable_l0s(pci);
 	rockchip_pcie_ep_hide_broken_ats_cap_rk3588(ep);
 

Signature · 1 line

2.51.0

Quoted 3 lines

Thus, prevent advertising L1 Substates support until proper driver support is added.

I think Mani is planning a change so we don't try to enable L1 Substates by default, which should avoid the regression even without a patch like this.

That will still leave the existing CONFIG_PCIEASPM_POWER_SUPERSAVE=y and sysfs l1_1_aspm problems.

And we'll need to figure out a way to allow L1.x to be enabled based on 'supports-clkreq' and possibly other info. That would likely be v6.19 material since it's new functionality.

On 17 October 2025 18:45:58 CEST, Bjorn Helgaas <[email protected]> wrote:

(snip)

Quoted 7 lines

Quoted 3 lines

Thus, prevent advertising L1 Substates support until proper driver support is added.

I think Mani is planning a change so we don't try to enable L1 Substates by default, which should avoid the regression even without a patch like this.

Sounds good, I suggested the same:
https://lore.kernel.org/linux-pci/aO9tWjgHnkATroNa@ryzen/

Quoted 1 line

Quoted 2 lines

That will still leave the existing CONFIG_PCIEASPM_POWER_SUPERSAVE=y and sysfs l1_1_aspm problems.

Indeed, which is why I think that this patch is v6.18 material.

Quoted 17 lines · Niklas Cassel

On 17 October 2025 18:45:58 CEST, Bjorn Helgaas <[email protected]> wrote:

(snip)

Quoted 7 lines

Quoted 3 lines

Thus, prevent advertising L1 Substates support until proper driver support is added.

I think Mani is planning a change so we don't try to enable L1 Substates by default, which should avoid the regression even without a patch like this.

Sounds good, I suggested the same:
https://lore.kernel.org/linux-pci/aO9tWjgHnkATroNa@ryzen/

Quoted 1 line

Quoted 5 lines

Quoted 2 lines

That will still leave the existing CONFIG_PCIEASPM_POWER_SUPERSAVE=y and sysfs l1_1_aspm problems.

Indeed, which is why I think that this patch is v6.18 material.

Not strictly a 6.18 material as the Kconfig/sysfs/cmdline issues existed even before we enabled the ASPM states by default in v6.18-rc1.

The L1 substates support requires additional steps to work, namely:

(quoted diff) +0 -2

-Proper handling of the CLKREQ# sideband signal. (It is mostly handled by
  hardware, but software still needs to set the clkreq fields in the
  PCIE_CLIENT_POWER_CON register to match the hardware implementation.)
-Program the frequency of the aux clock into the
  DSP_PCIE_PL_AUX_CLK_FREQ_OFF register. (During L1 substates the core_clk
  is turned off and the aux_clk is used instead.)

These steps are currently missing from the driver.

For more details, see section '18.6.6.4 L1 Substate' in the RK3658 TRM 1.1 Part 2, or section '11.6.6.4 L1 Substate' in the RK3588 TRM 1.0 Part2.

While this has always been a problem when using e.g. CONFIG_PCIEASPM_POWER_SUPERSAVE=y, or when modifying /sys/bus/pci/devices/.../link/l1_2_aspm, the lacking driver support for L1 substates became more apparent after commit f3ac2ff14834 ("PCI/ASPM:
Enable all ClockPM and ASPM states for devicetree platforms"), which enabled ASPM also for CONFIG_PCIEASPM_DEFAULT=y.

When using e.g. an NVMe drive connected to the PCIe controller, the
problem will be seen as:
nvme nvme0: controller is down; will reset: CSTS=0xffffffff, PCI_STATUS=0x10
nvme nvme0: Does your device have a faulty power saving mode enabled?
nvme nvme0: Try "nvme_core.default_ps_max_latency_us=0 pcie_aspm=off pcie_port_pm=off" and report a bug

Thus, prevent advertising L1 Substates support until proper driver support is added.

Cc [email protected]

Fixes 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")

Fixes f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for devicetree platforms")

Acked-by Shawn Lin <[email protected]>

Signed-off-by Niklas Cassel <[email protected]>

---
Changes since v2:

(quoted diff) +0 -1

-Improve commit message (Bjorn)
 
  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 21 +++++++++++++++++++
  1 file changed, 21 insertions(+)

drivers/pci/controller/dwc/pcie-dw-rockchip.c +21 -0

index 3e2752c7dd09..84f882abbca5 100644
@@ -200,6 +200,25 @@ static bool rockchip_pcie_link_up(struct dw_pcie *pci)
  	return FIELD_GET(PCIE_LINKUP_MASK, val) == PCIE_LINKUP;
  }
 
+/*
+ * See e.g. section '11.6.6.4 L1 Substate' in the RK3588 TRM V1.0 for the steps
+ * needed to support L1 substates. Currently, not a single rockchip platform
+ * performs these steps, so disable L1 substates until there is proper support.
+ */
+static void rockchip_pcie_disable_l1sub(struct dw_pcie *pci)
+{
+	u32 cap, l1subcap;
+
+	cap = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_L1SS);
+	if (cap) {
+		l1subcap = dw_pcie_readl_dbi(pci, cap + PCI_L1SS_CAP);
+		l1subcap &= ~(PCI_L1SS_CAP_L1_PM_SS | PCI_L1SS_CAP_ASPM_L1_1 |
+			      PCI_L1SS_CAP_ASPM_L1_2 | PCI_L1SS_CAP_PCIPM_L1_1 |
+			      PCI_L1SS_CAP_PCIPM_L1_2);
+		dw_pcie_writel_dbi(pci, cap + PCI_L1SS_CAP, l1subcap);
+	}
+}
+
  static void rockchip_pcie_enable_l0s(struct dw_pcie *pci)
  {
  	u32 cap, lnkcap;

@@ -264,6 +283,7 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
  	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
  					 rockchip);
 
+	rockchip_pcie_disable_l1sub(pci);
  	rockchip_pcie_enable_l0s(pci);
 
  	return 0;

@@ -301,6 +321,7 @@ static void rockchip_pcie_ep_init(struct dw_pcie_ep *ep)
  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
  	enum pci_barno bar;
 
+	rockchip_pcie_disable_l1sub(pci);
  	rockchip_pcie_enable_l0s(pci);
  	rockchip_pcie_ep_hide_broken_ats_cap_rk3588(ep);
 

Quoted 8 lines · Niklas Cassel

The L1 substates support requires additional steps to work, namely:

(quoted diff) +0 -2

-Proper handling of the CLKREQ# sideband signal. (It is mostly handled by
 hardware, but software still needs to set the clkreq fields in the
 PCIE_CLIENT_POWER_CON register to match the hardware implementation.)
-Program the frequency of the aux clock into the
 DSP_PCIE_PL_AUX_CLK_FREQ_OFF register. (During L1 substates the core_clk
 is turned off and the aux_clk is used instead.)

Quoted 1 line

[...]

Applied, thanks!

[1/1] PCI: dw-rockchip: Prevent advertising L1 Substates support

      commit: 40331c63e7901a2cc75ce6b5d24d50601efb833d

Hello Mani,

Quoted 10 lines · Manivannan Sadhasivam

Quoted 8 lines · Niklas Cassel

The L1 substates support requires additional steps to work, namely:

(quoted diff) +0 -2

-Proper handling of the CLKREQ# sideband signal. (It is mostly handled by
  hardware, but software still needs to set the clkreq fields in the
  PCIE_CLIENT_POWER_CON register to match the hardware implementation.)
-Program the frequency of the aux clock into the
  DSP_PCIE_PL_AUX_CLK_FREQ_OFF register. (During L1 substates the core_clk
  is turned off and the aux_clk is used instead.)

Quoted 6 lines

Quoted 1 line

[...]

Applied, thanks!

[1/1] PCI: dw-rockchip: Prevent advertising L1 Substates support

       commit: 40331c63e7901a2cc75ce6b5d24d50601efb833d

Last update in this thread was "Applied, thanks!"

and the patch was applied to pci/controller/dw-rockchip

since then it seems to have been demoted to pci/controller/dw-rockchip-pend

I'm simply curious, what is the plan for this patch?

I know that Shawn was working on a series that adds support for L1ss for this driver, but it seems to have stagnated, so it seems far from certain

that it will be ready in time to make it for the v6.19 merge window.

Right now, pci/next branch seems to merge pci/controller/dw-rockchip rather than pci/controller/dw-rockchip-pend.

If the L1ss does not make it in time, then this patch will not have had any time in linux-next, which might not make Linus happy.

Kind regards,
Niklas

Quoted 9 lines · Manivannan Sadhasivam

Quoted 8 lines · Niklas Cassel

The L1 substates support requires additional steps to work, namely:

(quoted diff) +0 -2

-Proper handling of the CLKREQ# sideband signal. (It is mostly handled by
 hardware, but software still needs to set the clkreq fields in the
 PCIE_CLIENT_POWER_CON register to match the hardware implementation.)
-Program the frequency of the aux clock into the
 DSP_PCIE_PL_AUX_CLK_FREQ_OFF register. (During L1 substates the core_clk
 is turned off and the aux_clk is used instead.)

Quoted 6 lines

Quoted 1 line

[...]

Applied, thanks!

[1/1] PCI: dw-rockchip: Prevent advertising L1 Substates support

      commit: 40331c63e7901a2cc75ce6b5d24d50601efb833d

Last update in this thread was "Applied, thanks!"

and the patch was applied to pci/controller/dw-rockchip

since then it seems to have been demoted to
pci/controller/dw-rockchip-pend

Quoted 21 lines · Niklas Cassel

Quoted 9 lines · Manivannan Sadhasivam

Quoted 8 lines · Niklas Cassel

The L1 substates support requires additional steps to work, namely:

(quoted diff) +0 -2

-Proper handling of the CLKREQ# sideband signal. (It is mostly handled by
 hardware, but software still needs to set the clkreq fields in the
 PCIE_CLIENT_POWER_CON register to match the hardware implementation.)
-Program the frequency of the aux clock into the
 DSP_PCIE_PL_AUX_CLK_FREQ_OFF register. (During L1 substates the core_clk
 is turned off and the aux_clk is used instead.)

Quoted 6 lines

Quoted 1 line

[...]

Applied, thanks!

[1/1] PCI: dw-rockchip: Prevent advertising L1 Substates support

      commit: 40331c63e7901a2cc75ce6b5d24d50601efb833d

Last update in this thread was "Applied, thanks!"

and the patch was applied to pci/controller/dw-rockchip

Quoted 2 lines

since then it seems to have been demoted to
pci/controller/dw-rockchip-pend

That was Oct 20, and we had some conversation about a more generic approach after that, so I moved it to dw-rockchip-pend while we worked that out.

I fleshed it out the generic approach and will post it soon.

The L1 substates support requires additional steps to work, namely:

(quoted diff) +0 -2

-Proper handling of the CLKREQ# sideband signal. (It is mostly handled by
 hardware, but software still needs to set the clkreq fields in the
 PCIE_CLIENT_POWER_CON register to match the hardware implementation.)
-Program the frequency of the aux clock into the
 DSP_PCIE_PL_AUX_CLK_FREQ_OFF register. (During L1 substates the core_clk
 is turned off and the aux_clk is used instead.)

These steps are currently missing from the driver.

For more details, see section '18.6.6.4 L1 Substate' in the RK3658 TRM 1.1 Part 2, or section '11.6.6.4 L1 Substate' in the RK3588 TRM 1.0 Part2.

While this has always been a problem when using e.g. CONFIG_PCIEASPM_POWER_SUPERSAVE=y, or when modifying /sys/bus/pci/devices/.../link/l1_2_aspm, the lacking driver support for L1 substates became more apparent after commit f3ac2ff14834 ("PCI/ASPM:
Enable all ClockPM and ASPM states for devicetree platforms"), which enabled ASPM also for CONFIG_PCIEASPM_DEFAULT=y.

When using e.g. an NVMe drive connected to the PCIe controller, the
problem will be seen as:
nvme nvme0: controller is down; will reset: CSTS=0xffffffff, PCI_STATUS=0x10
nvme nvme0: Does your device have a faulty power saving mode enabled?
nvme nvme0: Try "nvme_core.default_ps_max_latency_us=0 pcie_aspm=off pcie_port_pm=off" and report a bug

Thus, prevent advertising L1 Substates support until proper driver support is added.

Cc [email protected]

Fixes 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")

Fixes f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for devicetree platforms")

Acked-by Shawn Lin <[email protected]>

Signed-off-by Niklas Cassel <[email protected]>

---
Changes since v2:

(quoted diff) +0 -1

-Improve commit message (Bjorn)
 
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

drivers/pci/controller/dwc/pcie-dw-rockchip.c +8 -0

index 3e2752c7dd09..84f882abbca5 100644
@@ -200,6 +200,25 @@ static bool rockchip_pcie_link_up(struct dw_pcie *pci)
 	return FIELD_GET(PCIE_LINKUP_MASK, val) == PCIE_LINKUP;
 }
 
+/*
+ * See e.g. section '11.6.6.4 L1 Substate' in the RK3588 TRM V1.0 for the steps
+ * needed to support L1 substates. Currently, not a single rockchip platform
+ * performs these steps, so disable L1 substates until there is proper support.
+ */
+static void rockchip_pcie_disable_l1sub(struct dw_pcie *pci)
+{
+	u32 cap, l1subcap;

Quoted 7 lines · Niklas Cassel

The L1 substates support requires additional steps to work, namely:

(quoted diff) +0 -2

-Proper handling of the CLKREQ# sideband signal. (It is mostly handled by
 hardware, but software still needs to set the clkreq fields in the
 PCIE_CLIENT_POWER_CON register to match the hardware implementation.)
-Program the frequency of the aux clock into the
 DSP_PCIE_PL_AUX_CLK_FREQ_OFF register. (During L1 substates the core_clk
 is turned off and the aux_clk is used instead.)

...

Review

Quoted 13 lines

(quoted diff) +13 -0

+static void rockchip_pcie_disable_l1sub(struct dw_pcie *pci)
+{
+	u32 cap, l1subcap;
+
+	cap = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_L1SS);
+	if (cap) {
+		l1subcap = dw_pcie_readl_dbi(pci, cap + PCI_L1SS_CAP);
+		l1subcap &= ~(PCI_L1SS_CAP_L1_PM_SS | PCI_L1SS_CAP_ASPM_L1_1 |
+			      PCI_L1SS_CAP_ASPM_L1_2 | PCI_L1SS_CAP_PCIPM_L1_1 |
+			      PCI_L1SS_CAP_PCIPM_L1_2);
+		dw_pcie_writel_dbi(pci, cap + PCI_L1SS_CAP, l1subcap);
+	}
+}

I like this. But why should we do it just for dw-rockchip? Is there something special about dw-rockchip that makes this a problem? Maybe we should consider doing this in the dwc, cadence, mobiveil, and plda cores instead of trying to do it for every driver individually?

Advertising L1SS support via PCI_EXT_CAP_ID_L1SS means users can enable L1SS via CONFIG_PCIEASPM_POWER_SUPERSAVE=y or sysfs, and that seems likely to cause problems unless CLKREQ# is supported.

Quoted 29 lines · Bjorn Helgaas

Quoted 7 lines · Niklas Cassel

The L1 substates support requires additional steps to work, namely:

(quoted diff) +0 -2

-Proper handling of the CLKREQ# sideband signal. (It is mostly handled by
  hardware, but software still needs to set the clkreq fields in the
  PCIE_CLIENT_POWER_CON register to match the hardware implementation.)
-Program the frequency of the aux clock into the
  DSP_PCIE_PL_AUX_CLK_FREQ_OFF register. (During L1 substates the core_clk
  is turned off and the aux_clk is used instead.)

...

Review

Quoted 13 lines

(quoted diff) +13 -0

+static void rockchip_pcie_disable_l1sub(struct dw_pcie *pci)
+{
+	u32 cap, l1subcap;
+
+	cap = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_L1SS);
+	if (cap) {
+		l1subcap = dw_pcie_readl_dbi(pci, cap + PCI_L1SS_CAP);
+		l1subcap &= ~(PCI_L1SS_CAP_L1_PM_SS | PCI_L1SS_CAP_ASPM_L1_1 |
+			      PCI_L1SS_CAP_ASPM_L1_2 | PCI_L1SS_CAP_PCIPM_L1_1 |
+			      PCI_L1SS_CAP_PCIPM_L1_2);
+		dw_pcie_writel_dbi(pci, cap + PCI_L1SS_CAP, l1subcap);
+	}
+}

I like this. But why should we do it just for dw-rockchip? Is there something special about dw-rockchip that makes this a problem? Maybe we should consider doing this in the dwc, cadence, mobiveil, and plda cores instead of trying to do it for every driver individually?

Quoted 3 lines

Advertising L1SS support via PCI_EXT_CAP_ID_L1SS means users can enable L1SS via CONFIG_PCIEASPM_POWER_SUPERSAVE=y or sysfs, and that seems likely to cause problems unless CLKREQ# is supported.

Any thoughts on this? There's nothing rockchip-specific in this patch. What I'm proposing is something like this:

PCI: dwc: Prevent advertising L1 PM Substates

L1 PM Substates require the CLKREF# signal and driver-specific support. If CLKREF# is not supported or the driver support is lacking, enabling L1.1 or L1.2 may cause errors when accessing devices, e.g.,

nvme nvme0: controller is down; will reset: CSTS=0xffffffff, PCI_STATUS=0x10

If both ends of a link advertise support for L1 PM Substates, and the kernel is built with CONFIG_PCIEASPM_POWER_SUPERSAVE=y or users enable L1.x via sysfs, Linux tries to enable them.

To prevent errors when L1.x may not work, disable advertising the L1 PM Substates. Drivers can enable advertising them if they know CLKREF# is present and the Root Port is configured correctly.

Based on Niklas's patch from
https://patch.msgid.link/[email protected]

drivers/pci/controller/dwc/pcie-designware-host.c +22 -0

index 20c9333bcb1c..83b5330c9e45 100644
@@ -950,6 +950,27 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
  	return 0;
  }
 
+static void dw_pcie_clear_l1ss_advert(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	u16 l1ss;
+	u32 l1ss_cap;
+
+	l1ss = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_L1SS);
+	if (!l1ss)
+		return;
+
+	/*
+	 * By default, don't advertise L1 PM Substates because they require
+	 * CLKREF# and other driver-specific support.
+	 */
+	l1ss_cap = dw_pcie_readl_dbi(pci, l1ss + PCI_L1SS_CAP);
+	l1ss_cap &= ~(PCI_L1SS_CAP_PCIPM_L1_1 | PCI_L1SS_CAP_ASPM_L1_1 |
+		      PCI_L1SS_CAP_PCIPM_L1_2 | PCI_L1SS_CAP_ASPM_L1_2 |
+		      PCI_L1SS_CAP_L1_PM_SS);
+	dw_pcie_writel_dbi(pci, l1ss + PCI_L1SS_CAP, l1ss_cap);
+}
+
  static void dw_pcie_program_presets(struct dw_pcie_rp *pp, enum pci_bus_speed speed)
  {
  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

@@ -1060,6 +1081,7 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
  		PCI_COMMAND_MASTER | PCI_COMMAND_SERR;
  	dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
 
+	dw_pcie_clear_l1ss_advert(pp);
  	dw_pcie_config_presets(pp);
  	/*
  	 * If the platform provides its own child bus config accesses, it means

Quoted 34 lines · Bjorn Helgaas

Quoted 32 lines · Bjorn Helgaas

Quoted 7 lines · Niklas Cassel

The L1 substates support requires additional steps to work, namely:

(quoted diff) +0 -2

-Proper handling of the CLKREQ# sideband signal. (It is mostly handled by
  hardware, but software still needs to set the clkreq fields in the
  PCIE_CLIENT_POWER_CON register to match the hardware implementation.)
-Program the frequency of the aux clock into the
  DSP_PCIE_PL_AUX_CLK_FREQ_OFF register. (During L1 substates the core_clk
  is turned off and the aux_clk is used instead.)

...

Review

Quoted 13 lines

(quoted diff) +13 -0

+static void rockchip_pcie_disable_l1sub(struct dw_pcie *pci)
+{
+	u32 cap, l1subcap;
+
+	cap = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_L1SS);
+	if (cap) {
+		l1subcap = dw_pcie_readl_dbi(pci, cap + PCI_L1SS_CAP);
+		l1subcap &= ~(PCI_L1SS_CAP_L1_PM_SS | PCI_L1SS_CAP_ASPM_L1_1 |
+			      PCI_L1SS_CAP_ASPM_L1_2 | PCI_L1SS_CAP_PCIPM_L1_1 |
+			      PCI_L1SS_CAP_PCIPM_L1_2);
+		dw_pcie_writel_dbi(pci, cap + PCI_L1SS_CAP, l1subcap);
+	}
+}

I like this. But why should we do it just for dw-rockchip? Is there something special about dw-rockchip that makes this a problem? Maybe we should consider doing this in the dwc, cadence, mobiveil, and plda cores instead of trying to do it for every driver individually?

Advertising L1SS support via PCI_EXT_CAP_ID_L1SS means users can enable L1SS via CONFIG_PCIEASPM_POWER_SUPERSAVE=y or sysfs, and that seems likely to cause problems unless CLKREQ# is supported.

Quoted 2 lines

Any thoughts on this? There's nothing rockchip-specific in this patch. What I'm proposing is something like this:

I like your idea, though. But could it be another form of regression that we may breaks the platform which have already support L1SS properly? It's even harder to detect because a functional break is easier to notice than increased power consumption.

Or maybe we could
just export dw_pcie_clear_l1ss_advert() in dwc for host drivers to call it?

Quoted 53 lines

PCI: dwc: Prevent advertising L1 PM Substates
L1 PM Substates require the CLKREF# signal and driver-specific support. If CLKREF# is not supported or the driver support is lacking, enabling L1.1 or L1.2 may cause errors when accessing devices, e.g.,
nvme nvme0: controller is down; will reset: CSTS=0xffffffff, PCI_STATUS=0x10
If both ends of a link advertise support for L1 PM Substates, and the kernel is built with CONFIG_PCIEASPM_POWER_SUPERSAVE=y or users enable L1.x via sysfs, Linux tries to enable them.
To prevent errors when L1.x may not work, disable advertising the L1 PM Substates. Drivers can enable advertising them if they know CLKREF# is present and the Root Port is configured correctly.
Based on Niklas's patch from
https://patch.msgid.link/[email protected]

drivers/pci/controller/dwc/pcie-designware-host.c +22 -0

index 20c9333bcb1c..83b5330c9e45 100644
@@ -950,6 +950,27 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
  	return 0;
  }
+static void dw_pcie_clear_l1ss_advert(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	u16 l1ss;
+	u32 l1ss_cap;
+
+	l1ss = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_L1SS);
+	if (!l1ss)
+		return;
+
+	/*
+	 * By default, don't advertise L1 PM Substates because they require
+	 * CLKREF# and other driver-specific support.
+	 */
+	l1ss_cap = dw_pcie_readl_dbi(pci, l1ss + PCI_L1SS_CAP);
+	l1ss_cap &= ~(PCI_L1SS_CAP_PCIPM_L1_1 | PCI_L1SS_CAP_ASPM_L1_1 |
+		      PCI_L1SS_CAP_PCIPM_L1_2 | PCI_L1SS_CAP_ASPM_L1_2 |
+		      PCI_L1SS_CAP_L1_PM_SS);
+	dw_pcie_writel_dbi(pci, l1ss + PCI_L1SS_CAP, l1ss_cap);
+}
+
  static void dw_pcie_program_presets(struct dw_pcie_rp *pp, enum pci_bus_speed speed)
  {
  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

@@ -1060,6 +1081,7 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
  		PCI_COMMAND_MASTER | PCI_COMMAND_SERR;
  	dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
+	dw_pcie_clear_l1ss_advert(pp);
  	dw_pcie_config_presets(pp);
  	/*
  	 * If the platform provides its own child bus config accesses, it means
 

Quoted 35 lines · Shawn Lin

Quoted 34 lines · Bjorn Helgaas

Quoted 32 lines · Bjorn Helgaas

Quoted 7 lines · Niklas Cassel

The L1 substates support requires additional steps to work, namely:

(quoted diff) +0 -2

-Proper handling of the CLKREQ# sideband signal. (It is mostly handled by
  hardware, but software still needs to set the clkreq fields in the
  PCIE_CLIENT_POWER_CON register to match the hardware implementation.)
-Program the frequency of the aux clock into the
  DSP_PCIE_PL_AUX_CLK_FREQ_OFF register. (During L1 substates the core_clk
  is turned off and the aux_clk is used instead.)

...

Review

Quoted 13 lines

(quoted diff) +13 -0

+static void rockchip_pcie_disable_l1sub(struct dw_pcie *pci)
+{
+	u32 cap, l1subcap;
+
+	cap = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_L1SS);
+	if (cap) {
+		l1subcap = dw_pcie_readl_dbi(pci, cap + PCI_L1SS_CAP);
+		l1subcap &= ~(PCI_L1SS_CAP_L1_PM_SS | PCI_L1SS_CAP_ASPM_L1_1 |
+			      PCI_L1SS_CAP_ASPM_L1_2 | PCI_L1SS_CAP_PCIPM_L1_1 |
+			      PCI_L1SS_CAP_PCIPM_L1_2);
+		dw_pcie_writel_dbi(pci, cap + PCI_L1SS_CAP, l1subcap);
+	}
+}

I like this. But why should we do it just for dw-rockchip? Is there something special about dw-rockchip that makes this a problem? Maybe we should consider doing this in the dwc, cadence, mobiveil, and plda cores instead of trying to do it for every driver individually?

Advertising L1SS support via PCI_EXT_CAP_ID_L1SS means users can enable L1SS via CONFIG_PCIEASPM_POWER_SUPERSAVE=y or sysfs, and that seems likely to cause problems unless CLKREQ# is supported.

Quoted 7 lines

Quoted 2 lines

Any thoughts on this? There's nothing rockchip-specific in this patch. What I'm proposing is something like this:

I like your idea, though. But could it be another form of regression that we may breaks the platform which have already support L1SS properly? It's even harder to detect because a functional break is easier to notice than increased power consumption.

True, but I think it's unlikely because the PCI core never enabled L1SS (except for CONFIG_PCIEASPM_POWER_SUPERSAVE=y or sysfs, which I doubt anybody really uses).

Devicetree platforms that use L1SS should have explicit code to enable it, like qcom does, so we should be able to find them and make sure they do what's needed to prevent the regression.

Quoted 3 lines

Or maybe we could
just export dw_pcie_clear_l1ss_advert() in dwc for host drivers to call it?

I don't like the idea of host drivers having to opt in for this because that requires changes to all of them, not just changes to drivers that have done the work to actually support L1SS.

Quoted 29 lines · Bjorn Helgaas

Quoted 7 lines · Niklas Cassel

The L1 substates support requires additional steps to work, namely:

(quoted diff) +0 -2

-Proper handling of the CLKREQ# sideband signal. (It is mostly handled by
 hardware, but software still needs to set the clkreq fields in the
 PCIE_CLIENT_POWER_CON register to match the hardware implementation.)
-Program the frequency of the aux clock into the
 DSP_PCIE_PL_AUX_CLK_FREQ_OFF register. (During L1 substates the core_clk
 is turned off and the aux_clk is used instead.)

...

Review

Quoted 13 lines

(quoted diff) +13 -0

+static void rockchip_pcie_disable_l1sub(struct dw_pcie *pci)
+{
+	u32 cap, l1subcap;
+
+	cap = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_L1SS);
+	if (cap) {
+		l1subcap = dw_pcie_readl_dbi(pci, cap + PCI_L1SS_CAP);
+		l1subcap &= ~(PCI_L1SS_CAP_L1_PM_SS | PCI_L1SS_CAP_ASPM_L1_1 |
+			      PCI_L1SS_CAP_ASPM_L1_2 | PCI_L1SS_CAP_PCIPM_L1_1 |
+			      PCI_L1SS_CAP_PCIPM_L1_2);
+		dw_pcie_writel_dbi(pci, cap + PCI_L1SS_CAP, l1subcap);
+	}
+}

I like this. But why should we do it just for dw-rockchip? Is there something special about dw-rockchip that makes this a problem? Maybe we should consider doing this in the dwc, cadence, mobiveil, and plda cores instead of trying to do it for every driver individually?

Quoted 3 lines

Advertising L1SS support via PCI_EXT_CAP_ID_L1SS means users can enable L1SS via CONFIG_PCIEASPM_POWER_SUPERSAVE=y or sysfs, and that seems likely to cause problems unless CLKREQ# is supported.

Any thoughts on this? There's nothing rockchip-specific in this patch. What I'm proposing is something like this:

PCI: dwc: Prevent advertising L1 PM Substates

L1 PM Substates require the CLKREF# signal and driver-specific support. If CLKREF# is not supported or the driver support is lacking, enabling L1.1 or L1.2 may cause errors when accessing devices, e.g.,

nvme nvme0: controller is down; will reset: CSTS=0xffffffff, PCI_STATUS=0x10

If both ends of a link advertise support for L1 PM Substates, and the kernel is built with CONFIG_PCIEASPM_POWER_SUPERSAVE=y or users enable L1.x via sysfs, Linux tries to enable them.

To prevent errors when L1.x may not work, disable advertising the L1 PM Substates. Drivers can enable advertising them if they know CLKREF# is present and the Root Port is configured correctly.

Based on Niklas's patch from
https://patch.msgid.link/[email protected]

drivers/pci/controller/dwc/pcie-designware-host.c +21 -0

index 20c9333bcb1c..83b5330c9e45 100644
@@ -950,6 +950,27 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 	return 0;
 }
 
+static void dw_pcie_clear_l1ss_advert(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	u16 l1ss;
+	u32 l1ss_cap;
+
+	l1ss = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_L1SS);
+	if (!l1ss)
+		return;
+
+	/*
+	 * By default, don't advertise L1 PM Substates because they require
+	 * CLKREF# and other driver-specific support.
+	 */
+	l1ss_cap = dw_pcie_readl_dbi(pci, l1ss + PCI_L1SS_CAP);
+	l1ss_cap &= ~(PCI_L1SS_CAP_PCIPM_L1_1 | PCI_L1SS_CAP_ASPM_L1_1 |
+		      PCI_L1SS_CAP_PCIPM_L1_2 | PCI_L1SS_CAP_ASPM_L1_2 |
+		      PCI_L1SS_CAP_L1_PM_SS);
+	dw_pcie_writel_dbi(pci, l1ss + PCI_L1SS_CAP, l1ss_cap);
+}
+
 static void dw_pcie_program_presets(struct dw_pcie_rp *pp, enum pci_bus_speed speed)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

Quoted 89 lines · Bjorn Helgaas

Quoted 29 lines · Bjorn Helgaas

Quoted 7 lines · Niklas Cassel

The L1 substates support requires additional steps to work, namely:

(quoted diff) +0 -2

-Proper handling of the CLKREQ# sideband signal. (It is mostly handled by
 hardware, but software still needs to set the clkreq fields in the
 PCIE_CLIENT_POWER_CON register to match the hardware implementation.)
-Program the frequency of the aux clock into the
 DSP_PCIE_PL_AUX_CLK_FREQ_OFF register. (During L1 substates the core_clk
 is turned off and the aux_clk is used instead.)

...

Review

Quoted 13 lines

(quoted diff) +13 -0

+static void rockchip_pcie_disable_l1sub(struct dw_pcie *pci)
+{
+	u32 cap, l1subcap;
+
+	cap = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_L1SS);
+	if (cap) {
+		l1subcap = dw_pcie_readl_dbi(pci, cap + PCI_L1SS_CAP);
+		l1subcap &= ~(PCI_L1SS_CAP_L1_PM_SS | PCI_L1SS_CAP_ASPM_L1_1 |
+			      PCI_L1SS_CAP_ASPM_L1_2 | PCI_L1SS_CAP_PCIPM_L1_1 |
+			      PCI_L1SS_CAP_PCIPM_L1_2);
+		dw_pcie_writel_dbi(pci, cap + PCI_L1SS_CAP, l1subcap);
+	}
+}

I like this. But why should we do it just for dw-rockchip? Is there something special about dw-rockchip that makes this a problem? Maybe we should consider doing this in the dwc, cadence, mobiveil, and plda cores instead of trying to do it for every driver individually?

Quoted 3 lines

Advertising L1SS support via PCI_EXT_CAP_ID_L1SS means users can enable L1SS via CONFIG_PCIEASPM_POWER_SUPERSAVE=y or sysfs, and that seems likely to cause problems unless CLKREQ# is supported.

Any thoughts on this? There's nothing rockchip-specific in this patch. What I'm proposing is something like this:

PCI: dwc: Prevent advertising L1 PM Substates

L1 PM Substates require the CLKREF# signal and driver-specific support. If CLKREF# is not supported or the driver support is lacking, enabling L1.1 or L1.2 may cause errors when accessing devices, e.g.,

nvme nvme0: controller is down; will reset: CSTS=0xffffffff, PCI_STATUS=0x10

If both ends of a link advertise support for L1 PM Substates, and the kernel is built with CONFIG_PCIEASPM_POWER_SUPERSAVE=y or users enable L1.x via sysfs, Linux tries to enable them.

To prevent errors when L1.x may not work, disable advertising the L1 PM Substates. Drivers can enable advertising them if they know CLKREF# is present and the Root Port is configured correctly.

Based on Niklas's patch from
https://patch.msgid.link/[email protected]

drivers/pci/controller/dwc/pcie-designware-host.c +21 -0

index 20c9333bcb1c..83b5330c9e45 100644
@@ -950,6 +950,27 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 	return 0;
 }
 
+static void dw_pcie_clear_l1ss_advert(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	u16 l1ss;
+	u32 l1ss_cap;
+
+	l1ss = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_L1SS);
+	if (!l1ss)
+		return;
+
+	/*
+	 * By default, don't advertise L1 PM Substates because they require
+	 * CLKREF# and other driver-specific support.
+	 */
+	l1ss_cap = dw_pcie_readl_dbi(pci, l1ss + PCI_L1SS_CAP);
+	l1ss_cap &= ~(PCI_L1SS_CAP_PCIPM_L1_1 | PCI_L1SS_CAP_ASPM_L1_1 |
+		      PCI_L1SS_CAP_PCIPM_L1_2 | PCI_L1SS_CAP_ASPM_L1_2 |
+		      PCI_L1SS_CAP_L1_PM_SS);
+	dw_pcie_writel_dbi(pci, l1ss + PCI_L1SS_CAP, l1ss_cap);
+}
+
 static void dw_pcie_program_presets(struct dw_pcie_rp *pp, enum pci_bus_speed speed)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

drivers/pci/controller/dwc/pcie-designware-host.c int dw_pcie_setup_rc(struct dw_pcie_rp *pp) · line 1087

Quoted 7 lines

drivers/pci/controller/dwc/pcie-designware-host.c +1 -0

@@ -1060,6 +1081,7 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
 		PCI_COMMAND_MASTER | PCI_COMMAND_SERR;
 	dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
 
+	dw_pcie_clear_l1ss_advert(pp);
 	dw_pcie_config_presets(pp);
 	/*
 	 * If the platform provides its own child bus config accesses, it means

My patch disables L1 substates when running the controller in both root complex mode and endpoint mode.

Your patch above only disables L1 substates when running the the controller in root complex mode.

I think this code has to be in pcie-designware.c and then e.g. dw_pcie_setup_rc() (pcie-designware-host.c) and dw_pcie_ep_init_registers() (pcie-designware-ep.c) can both use it.

And like Shawn mentions. Disabling it by default for all DWC based platforms could introduce regressions where L1 substates already is working fine.

Sure, the only driver that checks for the DT property 'supports-clkreq' is drivers/pci/controller/dwc/pcie-tegra194.c. However Mani claimed that the qcom driver also has support for L1 substates already, even though it does not check the 'supports-clkreq' DT property.