Quality Improvements for Rockchip-IP PCIe
Subsystems (5)
v2 cover Quality Improvements for Rockchip-IP PCIe
During a 30-day debugging-run fighting quirky PCIe devices on RK3399 some quality improvements began to take form and this is my attempt at upstreaming it. It will ensure maximum chance of retraining to Gen2 5.0GT/s, on all four lanes and plus if anybody is debugging the PHY they'll now get real values from TEST_I[3:0] for every TEST_ADDR[4:0] without risk of locking up kernel like with present broken async strobe TEST_WRITE.
---
V1 -> V2: use standard PCIe defines as suggested by Bjorn
Geraldo Nascimento (4):
PCI: pcie-rockchip: add Link Control and Status Register 2
PCI: rockchip-host: Set Target Link Speed before retraining
phy: rockchip-pcie: enable all four lanes
phy: rockchip-pcie: adjust read mask and write strobe disable
drivers/pci/controller/pcie-rockchip-host.c | 4 ++++ drivers/pci/controller/pcie-rockchip.h | 10 ++++++---- drivers/phy/rockchip/phy-rockchip-pcie.c | 16 +++++++++------- 3 files changed, 19 insertions(+), 11 deletions(-)
Signature · 1 line
2.49.0
RFC PATCH v2 1/4 PCI: pcie-rockchip: add Link Control and Status Register 2
Link Control and Status Register 2 is not present in current pcie-rockchip.h definitions. Add it in preparation for setting it before Gen2 retraining.
While at it, also reference other registers from offset at Capabilities Register through standard PCI definitions. Only RC registers have been touched, although in principle there's no functional change.
--- drivers/pci/controller/pcie-rockchip.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/pci/controller/pcie-rockchip.h +6 -4
@@ -155,17 +155,19 @@#define PCIE_EP_CONFIG_DID_VID (PCIE_EP_CONFIG_BASE + 0x00)#define PCIE_EP_CONFIG_LCS (PCIE_EP_CONFIG_BASE + 0xd0)#define PCIE_RC_CONFIG_RID_CCR (PCIE_RC_CONFIG_BASE + 0x08) -#define PCIE_RC_CONFIG_DCR (PCIE_RC_CONFIG_BASE + 0xc4) +#define PCIE_RC_CONFIG_CR (PCIE_RC_CONFIG_BASE + 0xc0) +#define PCIE_RC_CONFIG_DCR (PCIE_RC_CONFIG_CR + PCI_EXP_DEVCAP)#define PCIE_RC_CONFIG_DCR_CSPL_SHIFT 18#define PCIE_RC_CONFIG_DCR_CSPL_LIMIT 0xff#define PCIE_RC_CONFIG_DCR_CPLS_SHIFT 26 -#define PCIE_RC_CONFIG_DCSR (PCIE_RC_CONFIG_BASE + 0xc8) +#define PCIE_RC_CONFIG_DCSR (PCIE_RC_CONFIG_CR + PCI_EXP_DEVCTL)#define PCIE_RC_CONFIG_DCSR_MPS_MASK GENMASK(7, 5)#define PCIE_RC_CONFIG_DCSR_MPS_256 (0x1 << 5) -#define PCIE_RC_CONFIG_LINK_CAP (PCIE_RC_CONFIG_BASE + 0xcc) +#define PCIE_RC_CONFIG_LINK_CAP (PCIE_RC_CONFIG_CR + PCI_EXP_LNKCAP)#define PCIE_RC_CONFIG_LINK_CAP_L0S BIT(10) -#define PCIE_RC_CONFIG_LCS (PCIE_RC_CONFIG_BASE + 0xd0) +#define PCIE_RC_CONFIG_LCS (PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL)#define PCIE_EP_CONFIG_LCS (PCIE_EP_CONFIG_BASE + 0xd0) +#define PCIE_RC_CONFIG_LCS_2 (PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL2)#define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE + 0x90c)#define PCIE_RC_CONFIG_THP_CAP (PCIE_RC_CONFIG_BASE + 0x274)#define PCIE_RC_CONFIG_THP_CAP_NEXT_MASK GENMASK(31, 20)
Signature · 1 line
2.49.0
Re: [RFC PATCH v2 1/4] PCI: pcie-rockchip: add Link Control and Status Register 2
Quoted 19 lines · Geraldo Nascimento
Link Control and Status Register 2 is not present in current
pcie-rockchip.hdefinitions. Add it in preparation for setting it before Gen2 retraining.While at it, also reference other registers from offset at Capabilities Register through standard PCI definitions. Only RC registers have been touched, although in principle there's no functional change.
Signed-off-by Geraldo Nascimento <[email protected]>--- drivers/pci/controller/pcie-rockchip.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)drivers/pci/controller/pcie-rockchip.h +0
-0
drivers/pci/controller/pcie-rockchip.h line 159
Quoted 6 lines
drivers/pci/controller/pcie-rockchip.h +2
-1@@ -155,17 +155,19 @@#define PCIE_EP_CONFIG_DID_VID (PCIE_EP_CONFIG_BASE + 0x00)#define PCIE_EP_CONFIG_LCS (PCIE_EP_CONFIG_BASE + 0xd0)#define PCIE_RC_CONFIG_RID_CCR (PCIE_RC_CONFIG_BASE + 0x08) -#define PCIE_RC_CONFIG_DCR (PCIE_RC_CONFIG_BASE + 0xc4) +#define PCIE_RC_CONFIG_CR (PCIE_RC_CONFIG_BASE + 0xc0) +#define PCIE_RC_CONFIG_DCR (PCIE_RC_CONFIG_CR + PCI_EXP_DEVCAP)
I would really like to see PCI_EXP_DEVCAP referenced in the source where we currently use PCIE_RC_CONFIG_DCR. That way, cscope/tags/grep will find the actual uses of PCI_EXP_DEVCAP, not just this #define of PCIE_RC_CONFIG_DCR.
Something like this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pci-mvebu.c?id=v6.15#n265
drivers/pci/controller/pcie-rockchip.h line 162
Quoted 3 lines
drivers/pci/controller/pcie-rockchip.h +0
-0#define PCIE_RC_CONFIG_DCR_CSPL_SHIFT 18#define PCIE_RC_CONFIG_DCR_CSPL_LIMIT 0xff#define PCIE_RC_CONFIG_DCR_CPLS_SHIFT 26
Also use PCI_EXP_DEVCAP_PWR_VAL and PCI_EXP_DEVCAP_PWR_SCL here if possible. And FIELD_GET()/FIELD_PREP(), which avoid the need to define _SHIFT values.
I would do a pure conversion patch of the existing #defines. Then I suspect you wouldn't need a patch to add the Link 2 registers at all because you could just use the #defines from pci_regs.h.
Quoted 17 lines
drivers/pci/controller/pcie-rockchip.h +4
-3-#define PCIE_RC_CONFIG_DCSR (PCIE_RC_CONFIG_BASE + 0xc8) +#define PCIE_RC_CONFIG_DCSR (PCIE_RC_CONFIG_CR + PCI_EXP_DEVCTL)#define PCIE_RC_CONFIG_DCSR_MPS_MASK GENMASK(7, 5)#define PCIE_RC_CONFIG_DCSR_MPS_256 (0x1 << 5) -#define PCIE_RC_CONFIG_LINK_CAP (PCIE_RC_CONFIG_BASE + 0xcc) +#define PCIE_RC_CONFIG_LINK_CAP (PCIE_RC_CONFIG_CR + PCI_EXP_LNKCAP)#define PCIE_RC_CONFIG_LINK_CAP_L0S BIT(10) -#define PCIE_RC_CONFIG_LCS (PCIE_RC_CONFIG_BASE + 0xd0) +#define PCIE_RC_CONFIG_LCS (PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL)#define PCIE_EP_CONFIG_LCS (PCIE_EP_CONFIG_BASE + 0xd0) +#define PCIE_RC_CONFIG_LCS_2 (PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL2)#define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE + 0x90c)#define PCIE_RC_CONFIG_THP_CAP (PCIE_RC_CONFIG_BASE + 0x274)#define PCIE_RC_CONFIG_THP_CAP_NEXT_MASK GENMASK(31, 20)Signature · 1 line
2.49.0
Re: [RFC PATCH v2 1/4] PCI: pcie-rockchip: add Link Control and Status Register 2
Quoted 11 lines · Bjorn Helgaas
Quoted 1 line · Geraldo Nascimento
+#define
PCIE_RC_CONFIG_DCR(PCIE_RC_CONFIG_CR+PCI_EXP_DEVCAP)I would really like to see
PCI_EXP_DEVCAPreferenced in the source where we currently usePCIE_RC_CONFIG_DCR. That way, cscope/tags/grep will find the actual uses ofPCI_EXP_DEVCAP, not just this #define ofPCIE_RC_CONFIG_DCR.Something like this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pci-mvebu.c?id=v6.15#n265
Hi Bjorn,
Yes, thank you for the code snippet, it makes things more clear.
Quoted 4 lines
Quoted 3 lines
#define PCIE_RC_CONFIG_DCR_CSPL_SHIFT 18 #define PCIE_RC_CONFIG_DCR_CSPL_LIMIT 0xff #define PCIE_RC_CONFIG_DCR_CPLS_SHIFT 26
Quoted 8 lines
Also use
PCI_EXP_DEVCAP_PWR_VALandPCI_EXP_DEVCAP_PWR_SCLhere if possible. And FIELD_GET()/FIELD_PREP(), which avoid the need to define_SHIFTvalues.I would do a pure conversion patch of the existing #defines. Then I suspect you wouldn't need a patch to add the Link 2 registers at all because you could just use the #defines from
pci_regs.h.
Got it!
Thanks!
Geraldo Nascimento
Re: [RFC PATCH v2 1/4] PCI: pcie-rockchip: add Link Control and Status Register 2
In reply to Bjorn HelgaasQuoted 3 lines · Bjorn Helgaas
I would do a pure conversion patch of the existing #defines. Then I suspect you wouldn't need a patch to add the Link 2 registers at all because you could just use the #defines from
pci_regs.h.
Hi Bjorn,
I've hit roadblock, maybe you can help?
PCIE_RC_CONFIG_DCR_CSPL_LIMIT is defined as 0xff...
I'd like to kill that define too, since it will be orphaned.
But hardcoding 0xff seems like illegible solution.
Perhaps there is another standard define that
maps to 0xff that I can use? Anyone comes
to your mind?
Thanks!
Geraldo Nascimento
Re: [RFC PATCH v2 1/4] PCI: pcie-rockchip: add Link Control and Status Register 2
Quoted 11 lines · Geraldo Nascimento
Quoted 3 lines · Bjorn Helgaas
I would do a pure conversion patch of the existing #defines. Then I suspect you wouldn't need a patch to add the Link 2 registers at all because you could just use the #defines from
pci_regs.h.Hi Bjorn,
I've hit roadblock, maybe you can help?
PCIE_RC_CONFIG_DCR_CSPL_LIMITis defined as0xff...
Quoted 8 lines
I'd like to kill that define too, since it will be orphaned.
But hardcoding
0xffseems like illegible solution.Perhaps there is another standard define that
maps to0xffthat I can use? Anyone comes
to your mind?
Maybe FIELD_MAX(PCI_EXP_DEVCAP_PWR_VAL)?
RFC PATCH v2 2/4 PCI: rockchip-host: Set Target Link Speed before retraining
In reply to Geraldo NascimentoCurrent code may fail Gen2 retraining if Target Link Speed is set to 2.5 GT/s in Link Control and Status Register 2. Set it to 5.0 GT/s accordingly.
--- drivers/pci/controller/pcie-rockchip-host.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/pci/controller/pcie-rockchip-host.c +4 -0
@@ -341,6 +341,10 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)* Enable retrain for gen2. This should be configured only after * gen1 finished. */ + status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_LCS_2 ); + status &= ~PCI_EXP_LNKCTL2_TLS_2_5GT ; + status |=PCI_EXP_LNKCTL2_TLS_5_0GT ; +rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_LCS_2 ); status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_LCS ); status |=PCI_EXP_LNKCTL_RL ;rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_LCS );
Signature · 1 line
2.49.0
RFC PATCH v2 3/4 phy: rockchip-pcie: enable all four lanes
In reply to Geraldo NascimentoCurrent code enables only Lane 0 because pwr_cnt will be incremented on first call to the function. Use for-loop to enable all 4 lanes through GRF register.
--- drivers/phy/rockchip/phy-rockchip-pcie.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/phy/rockchip/phy-rockchip-pcie.c +7 -5
@@ -176,11 +176,13 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)PHY_CFG_ADDR_MASK , PHY_CFG_ADDR_SHIFT)); -regmap_write (rk_phy->reg_base, - rk_phy->phy_data->pcie_laneoff, -HIWORD_UPDATE (!PHY_LANE_IDLE_OFF , -PHY_LANE_IDLE_MASK , -PHY_LANE_IDLE_A_SHIFT + inst->index)); +for (int i=0 ; i <PHY_MAX_LANE_NUM ; i++) { +regmap_write (rk_phy->reg_base, + rk_phy->phy_data->pcie_laneoff, +HIWORD_UPDATE (!PHY_LANE_IDLE_OFF , +PHY_LANE_IDLE_MASK , +PHY_LANE_IDLE_A_SHIFT + i)); + }/* * No documented timeout value for phy operation below,
Signature · 1 line
2.49.0
RFC PATCH v2 4/4 phy: rockchip-pcie: adjust read mask and write strobe disable
In reply to Geraldo NascimentoSection 17.6.10 of the RK3399 TRM "PCIe PIPE PHY registers Description" defines asynchronous strobe TEST_WRITE which should be enabled then disabled and seems to have been copy-pasted as of current. Adjust it. While at it, adjust read mask which should be the same as write mask.
--- drivers/phy/rockchip/phy-rockchip-pcie.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/phy/rockchip/phy-rockchip-pcie.c +2 -2
@@ -30,9 +30,9 @@#define PHY_CFG_ADDR_SHIFT 1#define PHY_CFG_DATA_MASK 0xf#define PHY_CFG_ADDR_MASK 0x3f -#define PHY_CFG_RD_MASK 0x3ff +#define PHY_CFG_RD_MASK 0x3f#define PHY_CFG_WR_ENABLE 1 -#define PHY_CFG_WR_DISABLE 1 +#define PHY_CFG_WR_DISABLE 0#define PHY_CFG_WR_SHIFT 0#define PHY_CFG_WR_MASK 1#define PHY_CFG_PLL_LOCK 0x10
Signature · 1 line
2.49.0