[PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Niklas Cassel]

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);
 
-- 
2.51.0

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Bjorn Helgaas]

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
> 

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Niklas Cassel]

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

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Manivannan Sadhasivam]

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

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

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Niklas Cassel]

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

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[FUKAUMI Naoki]

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);
>   

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Manivannan Sadhasivam]

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,
-- 
Manivannan Sadhasivam <mani@kernel.org>

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Niklas Cassel]

Hello Mani,

On Tue, Oct 21, 2025 at 08:05:17AM +0530, Manivannan Sadhasivam wrote:
> 
> 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

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

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Shawn Lin]

在 2025/11/11 星期二 21:08, Niklas Cassel 写道:
> Hello Mani,
> 
> On Tue, Oct 21, 2025 at 08:05:17AM +0530, Manivannan Sadhasivam wrote:
>>
>> 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
> 
> 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

Yes, that's because Bjorn prefers to disable it in dwc core intially and
sent a patch which we discussed but didn't come to a conclusion. So I 
still be waiting..:）

> 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
> 

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Bjorn Helgaas]

On Tue, Nov 11, 2025 at 02:08:36PM +0100, Niklas Cassel wrote:
> On Tue, Oct 21, 2025 at 08:05:17AM +0530, Manivannan Sadhasivam wrote:
> > 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
> 
> 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

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.

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Niklas Cassel]

On Tue, Nov 11, 2025 at 03:43:36PM -0600, Bjorn Helgaas wrote:
> On Tue, Nov 11, 2025 at 02:08:36PM +0100, Niklas Cassel wrote:
> > On Tue, Oct 21, 2025 at 08:05:17AM +0530, Manivannan Sadhasivam wrote:
> > > 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
> > 
> > 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
> 
> 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.

Ok, thank you!


Kind regards,
Niklas

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Bjorn Helgaas]

[+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

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[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:

    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

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Shawn Lin]

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.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
> 

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Bjorn Helgaas]

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.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
> > 
> 

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Manivannan Sadhasivam]

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

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

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Niklas Cassel]

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 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

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

Re: [PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Bjorn Helgaas]

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 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
> 
> 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