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

[Niklas Cassel <cassel@kernel.org>]

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?


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.


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
>