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