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