Quality Improvements for Rockchip-IP PCIe
Subsystems (5)
v3 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.
---
V2 -> V3: correctly clean-up with standard PCIe defines as per Bjorn's suggestion
V1 -> V2: use standard PCIe defines as suggested by Bjorn
Geraldo Nascimento (5):
PCI: rockchip-host: Use standard PCIe defines
PCI: rockchip: Drop unused custom registers and bitfields
PCI: rockchip-host: Set Target Link Speed before retraining
phy: rockchip-pcie: Enable all four lanes
phy: rockchip-pcie: Adjust read mask and write
drivers/pci/controller/pcie-rockchip-host.c | 48 +++++++++++---------- drivers/pci/controller/pcie-rockchip.h | 11 +---- drivers/phy/rockchip/phy-rockchip-pcie.c | 16 ++++--- 3 files changed, 36 insertions(+), 39 deletions(-)
Signature · 1 line
2.49.0
RFC PATCH v3 1/5 PCI: rockchip-host: Use standard PCIe defines
Current code uses custom-defined register offsets and bitfields for standard PCIe registers. Change to using standard PCIe defines.
--- drivers/pci/controller/pcie-rockchip-host.c | 44 ++++++++++----------- 1 file changed, 22 insertions(+), 22 deletions(-)
drivers/pci/controller/pcie-rockchip-host.c +22 -22
@@ -40,18 +40,18 @@ static void rockchip_pcie_enable_bw_int(struct rockchip_pcie *rockchip) {u32 status; - status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_LCS ); + status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL ); status |= (PCI_EXP_LNKCTL_LBMIE |PCI_EXP_LNKCTL_LABIE ); -rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_LCS ); +rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL ); }static void rockchip_pcie_clr_bw_int (struct rockchip_pcie *rockchip) {u32 status; - status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_LCS ); + status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL ); status |= (PCI_EXP_LNKSTA_LBMS |PCI_EXP_LNKSTA_LABS ) <<16 ; -rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_LCS ); +rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL ); }static void rockchip_pcie_update_txcredit_mui (struct rockchip_pcie *rockchip)
@@ -269,7 +269,7 @@ static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip) scale =3 ;/* 0.001x */ curr = curr /1000 ;/* convert to mA */ power = (curr *3300 ) /1000 ;/* milliwatt */ -while (power >PCIE_RC_CONFIG_DCR_CSPL_LIMIT ) { +while (power >FIELD_MAX (PCI_EXP_DEVCAP_PWR_VAL )) {if (!scale) {dev_warn (rockchip->dev,"invalid power supply\n" );return ;
@@ -278,10 +278,10 @@ static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip) power = power /10 ; } - status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_DCR ); - status |= (power <<PCIE_RC_CONFIG_DCR_CSPL_SHIFT ) | - (scale <<PCIE_RC_CONFIG_DCR_CPLS_SHIFT ); -rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_DCR ); + status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_DEVCAP ); + status |=FIELD_PREP (PCI_EXP_DEVCAP_PWR_VAL , power); + status |=FIELD_PREP (PCI_EXP_DEVCAP_PWR_SCL , scale); +rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_CR +PCI_EXP_DEVCAP ); }/**
@@ -309,14 +309,14 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)rockchip_pcie_set_power_limit (rockchip);/* Set RC's clock architecture as common clock */ - status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_LCS ); + status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL ); status |=PCI_EXP_LNKSTA_SLC <<16 ; -rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_LCS ); +rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL );/* Set RC's RCB to 128 */ - status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_LCS ); + status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL ); status |=PCI_EXP_LNKCTL_RCB ; -rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_LCS ); +rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL );/* Enable Gen1 training */ rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
@@ -341,9 +341,9 @@ 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 ); + status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL ); status |=PCI_EXP_LNKCTL_RL ; -rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_LCS ); +rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL ); err = readl_poll_timeout(rockchip->apb_base + PCIE_CORE_CTRL, status, PCIE_LINK_IS_GEN2(status), 20,
@@ -380,15 +380,15 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)/* Clear L0s from RC's link cap */ if (of_property_read_bool (dev->of_node,"aspm-no-l0s" )) { - status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_LINK_CAP ); - status &= ~PCIE_RC_CONFIG_LINK_CAP_L0S ; -rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_LINK_CAP ); + status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCAP ); + status &= ~PCI_EXP_LNKCAP_ASPM_L0S ; +rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCAP ); } - status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_DCSR ); - status &= ~PCIE_RC_CONFIG_DCSR_MPS_MASK ; - status |=PCIE_RC_CONFIG_DCSR_MPS_256 ; -rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_DCSR ); + status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_DEVCTL ); + status &= ~PCI_EXP_DEVCTL_PAYLOAD ; + status |=PCI_EXP_DEVCTL_PAYLOAD_256B ; +rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_CR +PCI_EXP_DEVCTL );return 0 ; err_power_off_phy:
Signature · 1 line
2.49.0
kernel test robot
build botRe: [RFC PATCH v3 1/5] PCI: rockchip-host: Use standard PCIe defines
kernel test robot noticed the following build errors:
Hi Geraldo,
[This is a private test report for your RFC patch.] kernel test robot noticed the following build errors:
[auto build test ERROR on rockchip/for-next]
[also build test ERROR on pci/next pci/for-linus mani-mhi/mhi-next linus/master v6.16-rc1 next-20250613] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Geraldo-Nascimento/PCI-rockchip-host-Use-standard-PCIe-defines/20250613-132302 base: https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git for-next patch link: https://lore.kernel.org/r/fc88993a561fcc9a9960c038bbc8b0b717924888.1749791474.git.geraldogabriel%40gmail.com patch subject: [RFC PATCH v3 1/5] PCI: rockchip-host: Use standard PCIe defines config: s390-randconfig-002-20250614 (https://download.01.org/0day-ci/archive/20250614/[email protected]/config) compiler: s390-linux-gcc (GCC) 10.5.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250614/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
drivers/pci/controller/pcie-rockchip-host.c: In function 'rockchip_pcie_enable_bw_int':
drivers/pci/controller/pcie-rockchip-host.c:43:40: error: 'PCIE_RC_CONFIG_CR' undeclared (first use in this function); did you mean 'PCIE_RC_CONFIG_DCR'?
43 | status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
| ^~~~~~~~~~~~~~~~~
| PCIE_RC_CONFIG_DCR
drivers/pci/controller/pcie-rockchip-host.c:43:40: note: each undeclared identifier is reported only once for each function it appears in
drivers/pci/controller/pcie-rockchip-host.c: In function 'rockchip_pcie_clr_bw_int':
drivers/pci/controller/pcie-rockchip-host.c:52:40: error: 'PCIE_RC_CONFIG_CR' undeclared (first use in this function); did you mean 'PCIE_RC_CONFIG_DCR'?
52 | status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
| ^~~~~~~~~~~~~~~~~
| PCIE_RC_CONFIG_DCR
drivers/pci/controller/pcie-rockchip-host.c: In function 'rockchip_pcie_set_power_limit':
>> drivers/pci/controller/pcie-rockchip-host.c:272:17: error: implicit declaration of function 'FIELD_MAX' [-Werror=implicit-function-declaration]
272 | while (power > FIELD_MAX(PCI_EXP_DEVCAP_PWR_VAL)) {
| ^~~~~~~~~
drivers/pci/controller/pcie-rockchip-host.c:281:40: error: 'PCIE_RC_CONFIG_CR' undeclared (first use in this function); did you mean 'PCIE_RC_CONFIG_DCR'?
281 | status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCAP);
| ^~~~~~~~~~~~~~~~~
| PCIE_RC_CONFIG_DCR
>> drivers/pci/controller/pcie-rockchip-host.c:282:12: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration]
282 | status |= FIELD_PREP(PCI_EXP_DEVCAP_PWR_VAL, power);
| ^~~~~~~~~~
drivers/pci/controller/pcie-rockchip-host.c: In function 'rockchip_pcie_host_init_port':
drivers/pci/controller/pcie-rockchip-host.c:312:40: error: 'PCIE_RC_CONFIG_CR' undeclared (first use in this function); did you mean 'PCIE_RC_CONFIG_DCR'?
312 | status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
| ^~~~~~~~~~~~~~~~~
| PCIE_RC_CONFIG_DCRcc1: some warnings being treated as errors
vim +/FIELD_MAX +272 drivers/pci/controller/pcie-rockchip-host.c
250
251 static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
252 {
253 int curr;
254 u32 status, scale, power;
255
256 if (IS_ERR(rockchip->vpcie3v3))
257 return;
258
259 /*
260 * Set RC's captured slot power limit and scale if
261 * vpcie3v3 available. The default values are both zero
262 * which means the software should set these two according
263 * to the actual power supply.
264 */
265 curr = regulator_get_current_limit(rockchip->vpcie3v3);
266 if (curr <= 0)
267 return;
268
269 scale = 3; /* 0.001x */
270 curr = curr / 1000; /* convert to mA */
271 power = (curr * 3300) / 1000; /* milliwatt */
> 272 while (power > FIELD_MAX(PCI_EXP_DEVCAP_PWR_VAL)) {
273 if (!scale) {
274 dev_warn(rockchip->dev, "invalid power supply\n");
275 return;
276 }
277 scale--;
278 power = power / 10;
279 }
280
281 status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCAP);
> 282 status |= FIELD_PREP(PCI_EXP_DEVCAP_PWR_VAL, power); 283 status |= FIELD_PREP(PCI_EXP_DEVCAP_PWR_SCL, scale);
284 rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCAP);
285 }
286
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
RFC PATCH v3 2/5 PCI: rockchip: Drop unused custom registers and bitfields
In reply to Geraldo NascimentoSince we are now using standard PCIe defines, drop unused custom-defined ones and instead reference them by offset from Capabilities Register.
--- drivers/pci/controller/pcie-rockchip.h | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)
drivers/pci/controller/pcie-rockchip.h +1 -10
@@ -155,16 +155,7 @@#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_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_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_L0S BIT(10) -#define PCIE_RC_CONFIG_LCS (PCIE_RC_CONFIG_BASE + 0xd0) +#define PCIE_RC_CONFIG_CR (PCIE_RC_CONFIG_BASE + 0xc0)#define PCIE_EP_CONFIG_LCS (PCIE_EP_CONFIG_BASE + 0xd0)#define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE + 0x90c)#define PCIE_RC_CONFIG_THP_CAP (PCIE_RC_CONFIG_BASE + 0x274)
Signature · 1 line
2.49.0
RFC PATCH v3 3/5 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_CR +PCI_EXP_LNKCTL2 ); + status |=FIELD_PREP (PCI_EXP_LNKCTL2_TLS ,PCI_EXP_LNKCTL2_TLS_5_0GT ); +rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL2 ); + status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL ); status |=PCI_EXP_LNKCTL_RL ;rockchip_pcie_write (rockchip, status,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL );
Signature · 1 line
2.49.0
Re: [RFC PATCH v3 3/5] PCI: rockchip-host: Set Target Link Speed before retraining
Quoted 14 lines · Geraldo Nascimento
Current 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.
Signed-off-by Geraldo Nascimento <[email protected]>--- drivers/pci/controller/pcie-rockchip-host.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/pci/controller/pcie-rockchip-host.c +0
-0
drivers/pci/controller/pcie-rockchip-host.c static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip) · line 345
Quoted 5 lines
drivers/pci/controller/pcie-rockchip-host.c +2
-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_CR +PCI_EXP_LNKCTL2 ); + status |=FIELD_PREP (PCI_EXP_LNKCTL2_TLS ,PCI_EXP_LNKCTL2_TLS_5_0GT );
Although this incidentally "works" for facilitating Gen2 training because mask clears 2.5 GT/s, this will shift the value 0x2 by the mask I think. This is clearly wrong, and instead we should use AND clearing plus OR'ing the 5.0GT/s value.
But I wait until at least Bjorn's review to send v4 with correction.
Geraldo Nascimento
RFC PATCH v3 4/5 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.
--- 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 v3 5/5 phy: rockchip-pcie: Adjust read mask and write
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