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

[Niklas Cassel <cassel@kernel.org>]

Hello Shawn,

On Fri, Apr 11, 2025 at 08:48:01AM +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 found the default FTS number
> provided by DWC core doesn't work stable and make LTSSM switch between
> L0S and Recovery, leading to long exit latency, even fail to link sometimes.
> So override it to the max 255 which seems work fine under test for both PHYs
> used by Rockchip platforms.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
> Changes in v2:
> - Move n_fts to probe function
> - rewrite the commit message
> 
>  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..c397f3a 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -185,6 +185,17 @@ 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) {
> +		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);
> +	}

I think it looks wrong to have this in _start_link().
Why not put this code in a new "_enable_l0s() function, and call it from:
rockchip_pcie_ep_init() and rockchip_pcie_host_init().

When looking at this, we probably should also move the
rockchip_pcie_ep_hide_broken_ats_cap_rk3588() call from _pre_init() to _init(),
because if there is a core reset, _init() is called again, but _pre_init() is
not.


>  
>  	/* Reset device */
>  	gpiod_set_value_cansleep(rockchip->rst_gpio, 0);
> @@ -598,6 +609,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  	rockchip->pci.dev = dev;
>  	rockchip->pci.ops = &dw_pcie_ops;
>  	rockchip->data = data;

Perhaps add a newline here.

> +	/* Default fts number(210) is broken, override it to 255 */

Perhaps add a space between "number" and "(".
Perhaps write it as N_FTS instead of fts.
Perhaps use "value" instead of "number".

ie.

/* Default N_FTS value (210) is broken, ... */



Kind regards,
Niklas