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@xxxxxxxxxxxxxx>
> ---
>
> 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
