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

[Bjorn Helgaas <helgaas@kernel.org>]

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