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

Shawn Lin
2025-11-04T00:58:27Z

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
>