Re: [PATCH] PCI: dw-rockchip: Enable L0S capability

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

在 2025/04/10 星期四 16:43, Niklas Cassel 写道:
> Hello Shawn,
> 
> On Thu, Apr 10, 2025 at 09:36:21AM +0800, Shawn Lin wrote:
>> L0S capability isn't enabled on all SoCs by default, so enabling it
>> in order to make ASPM L0S work on Rockchip platforms. We have been
>> testing it for quite a long time and the default FTS number provided
>> by DWC core is broken since it fits only for DWC PHY IP but not for
>> other types of PHY IP from other vendors.
> 
> If we take the rk3588 SoC for example,
> some PCIe controllers use PHY:
> drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> some PCIe controllers use PHY:
> drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
> 
> This second PHY is obviously a Synopsys PHY.
> 
> So, I think the commit message is a bit confusing/misleading,
> since IMO the second PHY driver is for a DWC PHY IP.
> 
> Is this N_FTS value correct for both of these PHYs?

yes, the vaule is PHY depends, but we don't know which PHY is
used currently. So the safest one is 255 which is the max defined
in spec, seems work fine for both under massive test.

> 
> 
> Having this code in ->start_link() looks incorrect.
> 
> rockchip_pcie_configure_rc(), calls dw_pcie_host_init().
> 
> dw_pcie_host_init() first calls dw_pcie_setup_rc(), which calls dw_pcie_setup().
> dw_pcie_setup() will write pci->n_fts[0] / pci->n_fts[1].
> 
> dw_pcie_host_init() then calls dw_pcie_start_link()
> (which calls ->start_link()).
> 
> So, setting pci->n_fts[0] / pci->n_fts[1] in ->start_link() is thus too late.
> 

Oops, I did move n_fts a bit afterward after testing, in order to look
more reasonable until we enable L0S capability. Let me see how to fix
it.

Thannks for the review.

> 
> Kind regards,
> Niklas
> 
> 
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>>   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> index 21dc99c..56acfea 100644
>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> @@ -185,6 +185,20 @@ static int rockchip_pcie_link_up(struct dw_pcie *pci)
>>   static int rockchip_pcie_start_link(struct dw_pcie *pci)
>>   {
>>   	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
>> +	u32 cap, lnkcap;
>> +
>> +	/* Enable L0S capability for all SoCs */
>> +	cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>> +	if (cap) {
>> +		/* Default fts number(210) is broken, override it */
>> +		pci->n_fts[0] = 255; /* Gen1 */
>> +		pci->n_fts[1] = 255; /* Gen2+ */
>> +		lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
>> +		lnkcap |= PCI_EXP_LNKCAP_ASPM_L0S;
>> +		dw_pcie_dbi_ro_wr_en(pci);
>> +		dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, lnkcap);
>> +		dw_pcie_dbi_ro_wr_dis(pci);
>> +	}
>>   
>>   	/* Reset device */
>>   	gpiod_set_value_cansleep(rockchip->rst_gpio, 0);
>> -- 
>> 2.7.4
>>
>