| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7df0bf91-8ab1-4e76-83fa-841a4059c634@rock-chips.com>
Date: Wed, 15 Oct 2025 21:00:41 +0800
From: Shawn Lin <shawn.lin@...k-chips.com>
To: Niklas Cassel <cassel@...nel.org>, Manivannan Sadhasivam <mani@...nel.org>
Cc: shawn.lin@...k-chips.com, Bjorn Helgaas <helgaas@...nel.org>,
manivannan.sadhasivam@....qualcomm.com, Bjorn Helgaas <bhelgaas@...gle.com>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kwilczynski@...nel.org>,
Rob Herring <robh@...nel.org>, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
"David E. Box" <david.e.box@...ux.intel.com>,
Kai-Heng Feng <kai.heng.feng@...onical.com>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Heiner Kallweit <hkallweit1@...il.com>,
Chia-Lin Kao <acelan.kao@...onical.com>, Dragan Simic <dsimic@...jaro.org>,
linux-rockchip@...ts.infradead.org, regressions@...ts.linux.dev,
FUKAUMI Naoki <naoki@...xa.com>
Subject: Re: [PATCH v2 1/2] PCI/ASPM: Override the ASPM and Clock PM states
set by BIOS for devicetree platforms
在 2025/10/15 星期三 20:17, Niklas Cassel 写道:
> On Wed, Oct 15, 2025 at 04:03:53PM +0530, Manivannan Sadhasivam wrote:
>> On Wed, Oct 15, 2025 at 11:46:02AM +0200, Niklas Cassel wrote:
>>> Hello Shawn,
>>>
>>> On Wed, Oct 15, 2025 at 05:11:39PM +0800, Shawn Lin wrote:
>>>>>
>>>>> Thanks! Could you please try the below diff with f3ac2ff14834 applied?
>>>>>
>>>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>>>>> index 214ed060ca1b..0069d06c282d 100644
>>>>> --- a/drivers/pci/quirks.c
>>>>> +++ b/drivers/pci/quirks.c
>>>>> @@ -2525,6 +2525,15 @@ static void quirk_disable_aspm_l0s_l1(struct pci_dev *dev)
>>>>> */
>>>>> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ASMEDIA, 0x1080, quirk_disable_aspm_l0s_l1);
>>>>>
>>>>> +
>>>>> +static void quirk_disable_aspm_all(struct pci_dev *dev)
>>>>> +{
>>>>> + pci_info(dev, "Disabling ASPM\n");
>>>>> + pci_disable_link_state(dev, PCIE_LINK_STATE_ALL);
>>>>> +}
>>>>> +
>>>>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ROCKCHIP, 0x3588, quirk_disable_aspm_all);
>>>>
>>>> That's not true from my POV. Rockchip platform supports all ASPM policy
>>>> after mass production verification. I also verified current upstream
>>>> code this morning with RK3588-EVB and can check L0s/L1/L1ss work fine.
>>>>
>>>> The log and lspci output could be found here:
>>>> https://pastebin.com/qizeYED7
>>>>
>>>> Moreover, I disscussed this issue with FUKAUMI today off-list and his
>>>> board seems to work when only disable L1ss by patching:
>>>>
>>>> --- a/drivers/pci/pcie/aspm.c
>>>> +++ b/drivers/pci/pcie/aspm.c
>>>> @@ -813,7 +813,7 @@ static void pcie_aspm_override_default_link_state(struct
>>>> pcie_link_state *link)
>>>>
>>>> /* For devicetree platforms, enable all ASPM states by default */
>>>> if (of_have_populated_dt()) {
>>>> - link->aspm_default = PCIE_LINK_STATE_ASPM_ALL;
>>>> + link->aspm_default = PCIE_LINK_STATE_L0S |
>>>> PCIE_LINK_STATE_L1;
>>>> override = link->aspm_default & ~link->aspm_enabled;
>>>> if (override)
>>>> pci_info(pdev, "ASPM: DT platform,
>>>>
>>>>
>>>> So, is there a proper way to just disable this feature for spec boards
>>>> instead of this Soc?
>>>
>>> This fix seems do the trick, without needing to patch common code (aspm.c):
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> index 3e2752c7dd09..f5e1aaa97719 100644
>>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> @@ -200,6 +200,19 @@ static bool rockchip_pcie_link_up(struct dw_pcie *pci)
>>> return FIELD_GET(PCIE_LINKUP_MASK, val) == PCIE_LINKUP;
>>> }
>>>
>>> +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_ASPM_L1_1 | PCI_L1SS_CAP_ASPM_L1_2 | PCI_L1SS_CAP_L1_PM_SS);
>>> + dw_pcie_writel_dbi(pci, cap + PCI_L1SS_CAP, l1subcap);
>>> + l1subcap = dw_pcie_readl_dbi(pci, cap + PCI_L1SS_CAP);
>>> + }
>>> +}
>>> +
>>> static void rockchip_pcie_enable_l0s(struct dw_pcie *pci)
>>> {
>>> u32 cap, lnkcap;
>>> @@ -264,6 +277,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);
For now, this is a acceptable option if default ASPM policy enable L1ss
w/o checking if the HW could supports it... But how about adding
supports-clkreq stuff to upstream host driver directly? That would help
folks enable L1ss if the HW is ready and they just need adding property
to the DT.
>>> rockchip_pcie_enable_l0s(pci);
>>>
>>> return 0;
>>> @@ -301,6 +315,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);
>>>
>>
>> But this patch removes the L1SS CAP for all boards, isn't it?
>
> Yes, all boards supported by pcie-dw-rockchip.c, which matches what their
> downstream driver does.
>
> (Their downstream driver disables L1 substates for all boards that have
> not defined 'supports-clkreq', and a grep through their downstream tree,
> for all their all their different branches, shows that not a since rockchip
> DTS has this property specified.)
The L1ss support is quite strict and need several steps to check, so we
didn't add supports-clkreq for them unless the HW is ready to go...
For adding supports of L1ss,
[1] the HW should support CLKREQ#, expecially for PCIe3.0 case on
Rockchip SoCs , since both CLKREQ# of RC and EP should connect to the
100MHz crystal generator's enable pin, as L1.2 need to disable refclk as
well. If the enable pin is high active, the HW even need a invertor....
[2] define proper clkreq iomux to pinctrl of pcie node
[3] make sure the devices work fine with L1ss.(It's hard to check the
slot case with random devices in the wild )
[4] add supports-clkreq to the DT and enable
CONFIG_PCIEASPM_POWER_SUPERSAVE
>
> So, let me submit a real patch with the above.
>
>
> Kind regards,
> Niklas
>
Powered by blists - more mailing lists