Re: [PATCH v2 1/2] PCI/ASPM: Override the ASPM and Clock PM states set by BIOS for devicetree platforms

[Shawn Lin <shawn.lin@rock-chips.com>]

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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip