| Message ID | 1744248981-43371-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: dw-rockchip: Enable L0S capability | expand |
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 >
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. I also think it makes sense to split this to two patches. Modifying N_FTS is completely different from enabling L0S, so please have a 1/2 patch that overrides N_FTS and a 2/2 patch that enables L0S. Kind regards, Niklas
在 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 >> >
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);
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. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/pci/controller/dwc/pcie-dw-rockchip.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)