The L1 substates support requires additional steps to work, namely: -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: stable@vger.kernel.org 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 shawn.lin@rock-chips.com Signed-off-by: Niklas Cassel cassel@kernel.org --- Changes since v2: -Improve commit message (Bjorn)
drivers/pci/controller/dwc/pcie-dw-rockchip.c | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c index 3e2752c7dd09..84f882abbca5 100644 --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c @@ -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);
On Fri, Oct 17, 2025 at 06:32:53PM +0200, Niklas Cassel wrote:
The L1 substates support requires additional steps to work, namely: -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.
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.
Cc: stable@vger.kernel.org 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 shawn.lin@rock-chips.com Signed-off-by: Niklas Cassel cassel@kernel.org
Changes since v2: -Improve commit message (Bjorn)
drivers/pci/controller/dwc/pcie-dw-rockchip.c | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c index 3e2752c7dd09..84f882abbca5 100644 --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c @@ -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);
2.51.0
On 17 October 2025 18:45:58 CEST, Bjorn Helgaas helgaas@kernel.org wrote:
(snip)
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/
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.
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.
I agree.
Kind regards, Niklas
On Sat, Oct 18, 2025 at 07:07:35AM +0200, Niklas Cassel wrote:
On 17 October 2025 18:45:58 CEST, Bjorn Helgaas helgaas@kernel.org wrote:
(snip)
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/
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 default ASPM issue will be fixed by a separate patch which will be generic for all platforms. I'll apply this patch for v6.19.
- Mani
Hello Mani,
On Tue, Oct 21, 2025 at 08:02:01AM +0530, Manivannan Sadhasivam wrote:
On Sat, Oct 18, 2025 at 07:07:35AM +0200, Niklas Cassel wrote:
On 17 October 2025 18:45:58 CEST, Bjorn Helgaas helgaas@kernel.org wrote:
(snip)
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/
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.
I don't agree that "strictly 6.18 material" means "only fixes for bugs introduced in the 6.18 merge window".
Normally, for a strict bug fix, the fix is merged during the current kernel release cycle (rather than waiting for the next merge window), even if the bug was introduced in an earlier kernel version.
E.g. a fix for a bug introduced during the 6.17 release cycle is definitely 6.18 material, IMO.
Sure, if the bug is extremely old, that probably means that it is okay to wait until the next merge window.
In this case, the Fixes tag is a commit first introduced in kernel v5.15, released Sun Oct 31 13:53:10 2021.
Having that said, I'm perfectly fine with delaying.
Kind regards, Niklas
Hi Niklas,
Thank you for your work.
On 10/18/25 01:32, Niklas Cassel wrote:
The L1 substates support requires additional steps to work, namely: -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: stable@vger.kernel.org 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 shawn.lin@rock-chips.com Signed-off-by: Niklas Cassel cassel@kernel.org
I've confirmed this patch resolves the issue in v6.18-rc1 using the following configuration:
ROCK 5A & M.2 RTL8852BE ROCK 5B & M.2 MT7921E, NVMe SSD ROCK 5T & on-board AX210, NVMe SSD x2 ROCK 5 ITX+ & M.2 MT7922E, NVMe SSD x2
Therefore,
Tested-by: FUKAUMI Naoki naoki@radxa.com
Best regards,
(P.S. I couldn't test on the ROCK 5B & M.2 RTL8852BE, ROCK 5B+ & on-board RTL8852BE, and ROCK 5C & ASM2806 due to separate issues.)
-- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd.
Changes since v2: -Improve commit message (Bjorn)
drivers/pci/controller/dwc/pcie-dw-rockchip.c | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c index 3e2752c7dd09..84f882abbca5 100644 --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c @@ -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);
On Fri, 17 Oct 2025 18:32:53 +0200, Niklas Cassel wrote:
The L1 substates support requires additional steps to work, namely: -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.)
[...]
Applied, thanks!
[1/1] PCI: dw-rockchip: Prevent advertising L1 Substates support commit: 40331c63e7901a2cc75ce6b5d24d50601efb833d
Best regards,
[+cc Daire, Karthikeyan, Hou]
On Fri, Oct 17, 2025 at 06:32:53PM +0200, Niklas Cassel wrote:
The L1 substates support requires additional steps to work, namely: -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: stable@vger.kernel.org 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 shawn.lin@rock-chips.com Signed-off-by: Niklas Cassel cassel@kernel.org
Changes since v2: -Improve commit message (Bjorn)
drivers/pci/controller/dwc/pcie-dw-rockchip.c | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c index 3e2752c7dd09..84f882abbca5 100644 --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c @@ -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);- }
+}
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.
Bjorn
On Tue, Oct 28, 2025 at 02:02:18PM -0500, Bjorn Helgaas wrote:
On Fri, Oct 17, 2025 at 06:32:53PM +0200, Niklas Cassel wrote:
The L1 substates support requires additional steps to work, namely: -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.)
...
+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.
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/20251017163252.598812-2-cassel@kernel.org
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 20c9333bcb1c..83b5330c9e45 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -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
Hi Bjorn,
在 2025/11/04 星期二 5:32, Bjorn Helgaas 写道:
On Tue, Oct 28, 2025 at 02:02:18PM -0500, Bjorn Helgaas wrote:
On Fri, Oct 17, 2025 at 06:32:53PM +0200, Niklas Cassel wrote:
The L1 substates support requires additional steps to work, namely: -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.)
...
+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.
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?
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/20251017163252.598812-2-cassel@kernel.orgdiff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 20c9333bcb1c..83b5330c9e45 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -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
On Tue, Nov 04, 2025 at 08:58:02AM +0800, Shawn Lin wrote:
在 2025/11/04 星期二 5:32, Bjorn Helgaas 写道:
On Tue, Oct 28, 2025 at 02:02:18PM -0500, Bjorn Helgaas wrote:
On Fri, Oct 17, 2025 at 06:32:53PM +0200, Niklas Cassel wrote:
The L1 substates support requires additional steps to work, namely: -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.)
...
+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.
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.
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.
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/20251017163252.598812-2-cassel@kernel.orgdiff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 20c9333bcb1c..83b5330c9e45 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -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
On Tue, Nov 04, 2025 at 04:17:24PM -0600, Bjorn Helgaas wrote:
On Tue, Nov 04, 2025 at 08:58:02AM +0800, Shawn Lin wrote:
在 2025/11/04 星期二 5:32, Bjorn Helgaas 写道:
On Tue, Oct 28, 2025 at 02:02:18PM -0500, Bjorn Helgaas wrote:
On Fri, Oct 17, 2025 at 06:32:53PM +0200, Niklas Cassel wrote:
The L1 substates support requires additional steps to work, namely: -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.)
...
+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.
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.
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.
Otherwise, we may have to introduce a flag for the controller drivers to opt-out of this disablement. Like, dw_pcie_rp::native_clkreq and bailing out early if this flag is set.
- Mani
Hello Bjorn,
On Mon, Nov 03, 2025 at 03:32:06PM -0600, Bjorn Helgaas wrote:
On Tue, Oct 28, 2025 at 02:02:18PM -0500, Bjorn Helgaas wrote:
On Fri, Oct 17, 2025 at 06:32:53PM +0200, Niklas Cassel wrote:
The L1 substates support requires additional steps to work, namely: -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.)
...
+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.
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 SubstatesL1 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/20251017163252.598812-2-cassel@kernel.org
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 20c9333bcb1c..83b5330c9e45 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -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
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.
Kind regards, Niklas
On Tue, Nov 04, 2025 at 01:53:24PM +0100, Niklas Cassel wrote:
On Mon, Nov 03, 2025 at 03:32:06PM -0600, Bjorn Helgaas wrote:
On Tue, Oct 28, 2025 at 02:02:18PM -0500, Bjorn Helgaas wrote:
On Fri, Oct 17, 2025 at 06:32:53PM +0200, Niklas Cassel wrote:
The L1 substates support requires additional steps to work, namely: -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.)
...
+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.
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 SubstatesL1 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/20251017163252.598812-2-cassel@kernel.org
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 20c9333bcb1c..83b5330c9e45 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -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
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.
I'm not opposed to doing it for endpoints as well if the endpoint mode driver needs to configure L1SS things before it can work.
I don't think an endpoint should be enabling L1SS on the host, so enabling L1SS probably has to be done by the host, and the host driver knows the topology and whether CLKREQ# is supported (e.g., via DT) and whether the RP is configured for 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.
Yes, we would need to ensure tegra, qcom, and any others that use L1SS don't regress. That's why I said "something like this" and didn't include a signed-off-by. I wanted to have exactly this conversation.
Bjorn
linux-stable-mirror@lists.linaro.org
-
Bjorn Helgaas -
FUKAUMI Naoki -
Manivannan Sadhasivam -
Niklas Cassel -
Shawn Lin