[RFC PATCH 1/2] PCI: rockchip-host: Retry link training on failure without PERST#

[RFC PATCH 1/2] PCI: rockchip-host: Retry link training on failure without PERST#

[Geraldo Nascimento]

Hi,

After almost 30 days of battling with RK3399 buggy PCIe on my Rock Pi
N10 through trial-and-error debugging, I finally got positive results
with enumeration on the PCI bus for both a Realtek 8111E NIC and a
Samsung PM981a SSD.

The NIC was connected to a M.2->PCIe x4 riser card and it would get
stuck on Polling.Compliance, without breaking electrical idle on the
Host RX side. The Samsung PM981a SSD is directly connected to M.2
connector and that SSD is known to be quirky (OEM... no support)
and non-functional on the RK3399 platform.

The Samsung SSD was even worse than the NIC - it would get stuck on
Detect.Active like a bricked card, even though it was fully functional
via USB adapter.

It seems both devices benefit from retrying Link Training if - big if
here - PERST# is not toggled during retry. This patch does exactly that.
I find the patch to be ugly as hell but it works - every time.

I added Hugh Cole-Baker and Christian Hewitt to Cc: as both are
experienced on RK3399 and hopefully at least one of them has faced
the non-working SSD experience on RK3399 and will be able to test this.

---
 drivers/pci/controller/pcie-rockchip-host.c | 141 ++++++++++++--------
 1 file changed, 87 insertions(+), 54 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index 6a46be17aa91..6a465f45a09c 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -284,6 +284,53 @@ static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
 	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DCR);
 }
 
+static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
+{
+	struct device *dev = rockchip->dev;
+	int err;
+
+	if (!IS_ERR(rockchip->vpcie12v)) {
+		err = regulator_enable(rockchip->vpcie12v);
+		if (err) {
+			dev_err(dev, "fail to enable vpcie12v regulator\n");
+			goto err_out;
+		}
+	}
+
+	if (!IS_ERR(rockchip->vpcie3v3)) {
+		err = regulator_enable(rockchip->vpcie3v3);
+		if (err) {
+			dev_err(dev, "fail to enable vpcie3v3 regulator\n");
+			goto err_disable_12v;
+		}
+	}
+
+	err = regulator_enable(rockchip->vpcie1v8);
+	if (err) {
+		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
+		goto err_disable_3v3;
+	}
+
+	err = regulator_enable(rockchip->vpcie0v9);
+	if (err) {
+		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
+		goto err_disable_1v8;
+	}
+
+	return 0;
+
+err_disable_1v8:
+	regulator_disable(rockchip->vpcie1v8);
+err_disable_3v3:
+	if (!IS_ERR(rockchip->vpcie3v3))
+		regulator_disable(rockchip->vpcie3v3);
+err_disable_12v:
+	if (!IS_ERR(rockchip->vpcie12v))
+		regulator_disable(rockchip->vpcie12v);
+err_out:
+	return err;
+}
+
 /**
  * rockchip_pcie_host_init_port - Initialize hardware
  * @rockchip: PCIe port information
@@ -291,11 +338,14 @@ static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
 static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
 {
 	struct device *dev = rockchip->dev;
-	int err, i = MAX_LANE_NUM;
+	int err, i = MAX_LANE_NUM, is_reinit = 0;
 	u32 status;
 
-	gpiod_set_value_cansleep(rockchip->perst_gpio, 0);
+	if (!is_reinit) {
+		gpiod_set_value_cansleep(rockchip->perst_gpio, 0);
+	}
 
+reinit:
 	err = rockchip_pcie_init_port(rockchip);
 	if (err)
 		return err;
@@ -322,16 +372,46 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
 	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
 			    PCIE_CLIENT_CONFIG);
 
-	msleep(PCIE_T_PVPERL_MS);
-	gpiod_set_value_cansleep(rockchip->perst_gpio, 1);
-
-	msleep(PCIE_T_RRS_READY_MS);
+	if (!is_reinit) {
+		msleep(PCIE_T_PVPERL_MS);
+		gpiod_set_value_cansleep(rockchip->perst_gpio, 1);
+		msleep(PCIE_T_RRS_READY_MS);
+	}
 
 	/* 500ms timeout value should be enough for Gen1/2 training */
 	err = readl_poll_timeout(rockchip->apb_base + PCIE_CLIENT_BASIC_STATUS1,
 				 status, PCIE_LINK_UP(status), 20,
 				 500 * USEC_PER_MSEC);
-	if (err) {
+
+	if (err && !is_reinit) {
+		while (i--)
+			phy_power_off(rockchip->phys[i]);
+		i = MAX_LANE_NUM;
+		while (i--)
+			phy_exit(rockchip->phys[i]);
+		i = MAX_LANE_NUM;
+		is_reinit = 1;
+		dev_dbg(dev, "Will reinit PCIe without toggling PERST#");
+		if (!IS_ERR(rockchip->vpcie12v))
+			regulator_disable(rockchip->vpcie12v);
+		if (!IS_ERR(rockchip->vpcie3v3))
+			regulator_disable(rockchip->vpcie3v3);
+		regulator_disable(rockchip->vpcie1v8);
+		regulator_disable(rockchip->vpcie0v9);
+		rockchip_pcie_disable_clocks(rockchip);
+		err = rockchip_pcie_enable_clocks(rockchip);
+		if (err)
+			return err;
+		err = rockchip_pcie_set_vpcie(rockchip);
+		if (err) {
+			dev_err(dev, "failed to set vpcie regulator\n");
+			rockchip_pcie_disable_clocks(rockchip);
+			return err;
+		}
+		goto reinit;
+	}
+
+	else if (err) {
 		dev_err(dev, "PCIe link training gen1 timeout!\n");
 		goto err_power_off_phy;
 	}
@@ -613,53 +693,6 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
 	return 0;
 }
 
-static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
-{
-	struct device *dev = rockchip->dev;
-	int err;
-
-	if (!IS_ERR(rockchip->vpcie12v)) {
-		err = regulator_enable(rockchip->vpcie12v);
-		if (err) {
-			dev_err(dev, "fail to enable vpcie12v regulator\n");
-			goto err_out;
-		}
-	}
-
-	if (!IS_ERR(rockchip->vpcie3v3)) {
-		err = regulator_enable(rockchip->vpcie3v3);
-		if (err) {
-			dev_err(dev, "fail to enable vpcie3v3 regulator\n");
-			goto err_disable_12v;
-		}
-	}
-
-	err = regulator_enable(rockchip->vpcie1v8);
-	if (err) {
-		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
-		goto err_disable_3v3;
-	}
-
-	err = regulator_enable(rockchip->vpcie0v9);
-	if (err) {
-		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
-		goto err_disable_1v8;
-	}
-
-	return 0;
-
-err_disable_1v8:
-	regulator_disable(rockchip->vpcie1v8);
-err_disable_3v3:
-	if (!IS_ERR(rockchip->vpcie3v3))
-		regulator_disable(rockchip->vpcie3v3);
-err_disable_12v:
-	if (!IS_ERR(rockchip->vpcie12v))
-		regulator_disable(rockchip->vpcie12v);
-err_out:
-	return err;
-}
-
 static void rockchip_pcie_enable_interrupts(struct rockchip_pcie *rockchip)
 {
 	rockchip_pcie_write(rockchip, (PCIE_CLIENT_INT_CLI << 16) &
-- 
2.49.0

Re: [RFC PATCH 1/2] PCI: rockchip-host: Retry link training on failure without PERST#

[Christian Hewitt]

> On 4 Jun 2025, at 1:14 pm, Geraldo Nascimento <geraldogabriel@gmail.com> wrote:
> 
> Hi,
> 
> After almost 30 days of battling with RK3399 buggy PCIe on my Rock Pi
> N10 through trial-and-error debugging, I finally got positive results
> with enumeration on the PCI bus for both a Realtek 8111E NIC and a
> Samsung PM981a SSD.
> 
> The NIC was connected to a M.2->PCIe x4 riser card and it would get
> stuck on Polling.Compliance, without breaking electrical idle on the
> Host RX side. The Samsung PM981a SSD is directly connected to M.2
> connector and that SSD is known to be quirky (OEM... no support)
> and non-functional on the RK3399 platform.
> 
> The Samsung SSD was even worse than the NIC - it would get stuck on
> Detect.Active like a bricked card, even though it was fully functional
> via USB adapter.
> 
> It seems both devices benefit from retrying Link Training if - big if
> here - PERST# is not toggled during retry. This patch does exactly that.
> I find the patch to be ugly as hell but it works - every time.
> 
> I added Hugh Cole-Baker and Christian Hewitt to Cc: as both are
> experienced on RK3399 and hopefully at least one of them has faced
> the non-working SSD experience on RK3399 and will be able to test this.

I think you have me confused with another Christian (or auto-complete
scored a new victim). Sadly I have no clue about PCI and not a lot of
knowledge on RK3399 matters :)

CH.

> ---
> drivers/pci/controller/pcie-rockchip-host.c | 141 ++++++++++++--------
> 1 file changed, 87 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index 6a46be17aa91..6a465f45a09c 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -284,6 +284,53 @@ static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
> rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DCR);
> }
> 
> +static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
> +{
> + struct device *dev = rockchip->dev;
> + int err;
> +
> + if (!IS_ERR(rockchip->vpcie12v)) {
> + err = regulator_enable(rockchip->vpcie12v);
> + if (err) {
> + dev_err(dev, "fail to enable vpcie12v regulator\n");
> + goto err_out;
> + }
> + }
> +
> + if (!IS_ERR(rockchip->vpcie3v3)) {
> + err = regulator_enable(rockchip->vpcie3v3);
> + if (err) {
> + dev_err(dev, "fail to enable vpcie3v3 regulator\n");
> + goto err_disable_12v;
> + }
> + }
> +
> + err = regulator_enable(rockchip->vpcie1v8);
> + if (err) {
> + dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> + goto err_disable_3v3;
> + }
> +
> + err = regulator_enable(rockchip->vpcie0v9);
> + if (err) {
> + dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> + goto err_disable_1v8;
> + }
> +
> + return 0;
> +
> +err_disable_1v8:
> + regulator_disable(rockchip->vpcie1v8);
> +err_disable_3v3:
> + if (!IS_ERR(rockchip->vpcie3v3))
> + regulator_disable(rockchip->vpcie3v3);
> +err_disable_12v:
> + if (!IS_ERR(rockchip->vpcie12v))
> + regulator_disable(rockchip->vpcie12v);
> +err_out:
> + return err;
> +}
> +
> /**
>  * rockchip_pcie_host_init_port - Initialize hardware
>  * @rockchip: PCIe port information
> @@ -291,11 +338,14 @@ static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
> static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
> {
> struct device *dev = rockchip->dev;
> - int err, i = MAX_LANE_NUM;
> + int err, i = MAX_LANE_NUM, is_reinit = 0;
> u32 status;
> 
> - gpiod_set_value_cansleep(rockchip->perst_gpio, 0);
> + if (!is_reinit) {
> + gpiod_set_value_cansleep(rockchip->perst_gpio, 0);
> + }
> 
> +reinit:
> err = rockchip_pcie_init_port(rockchip);
> if (err)
> return err;
> @@ -322,16 +372,46 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
> rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
>    PCIE_CLIENT_CONFIG);
> 
> - msleep(PCIE_T_PVPERL_MS);
> - gpiod_set_value_cansleep(rockchip->perst_gpio, 1);
> -
> - msleep(PCIE_T_RRS_READY_MS);
> + if (!is_reinit) {
> + msleep(PCIE_T_PVPERL_MS);
> + gpiod_set_value_cansleep(rockchip->perst_gpio, 1);
> + msleep(PCIE_T_RRS_READY_MS);
> + }
> 
> /* 500ms timeout value should be enough for Gen1/2 training */
> err = readl_poll_timeout(rockchip->apb_base + PCIE_CLIENT_BASIC_STATUS1,
> status, PCIE_LINK_UP(status), 20,
> 500 * USEC_PER_MSEC);
> - if (err) {
> +
> + if (err && !is_reinit) {
> + while (i--)
> + phy_power_off(rockchip->phys[i]);
> + i = MAX_LANE_NUM;
> + while (i--)
> + phy_exit(rockchip->phys[i]);
> + i = MAX_LANE_NUM;
> + is_reinit = 1;
> + dev_dbg(dev, "Will reinit PCIe without toggling PERST#");
> + if (!IS_ERR(rockchip->vpcie12v))
> + regulator_disable(rockchip->vpcie12v);
> + if (!IS_ERR(rockchip->vpcie3v3))
> + regulator_disable(rockchip->vpcie3v3);
> + regulator_disable(rockchip->vpcie1v8);
> + regulator_disable(rockchip->vpcie0v9);
> + rockchip_pcie_disable_clocks(rockchip);
> + err = rockchip_pcie_enable_clocks(rockchip);
> + if (err)
> + return err;
> + err = rockchip_pcie_set_vpcie(rockchip);
> + if (err) {
> + dev_err(dev, "failed to set vpcie regulator\n");
> + rockchip_pcie_disable_clocks(rockchip);
> + return err;
> + }
> + goto reinit;
> + }
> +
> + else if (err) {
> dev_err(dev, "PCIe link training gen1 timeout!\n");
> goto err_power_off_phy;
> }
> @@ -613,53 +693,6 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
> return 0;
> }
> 
> -static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
> -{
> - struct device *dev = rockchip->dev;
> - int err;
> -
> - if (!IS_ERR(rockchip->vpcie12v)) {
> - err = regulator_enable(rockchip->vpcie12v);
> - if (err) {
> - dev_err(dev, "fail to enable vpcie12v regulator\n");
> - goto err_out;
> - }
> - }
> -
> - if (!IS_ERR(rockchip->vpcie3v3)) {
> - err = regulator_enable(rockchip->vpcie3v3);
> - if (err) {
> - dev_err(dev, "fail to enable vpcie3v3 regulator\n");
> - goto err_disable_12v;
> - }
> - }
> -
> - err = regulator_enable(rockchip->vpcie1v8);
> - if (err) {
> - dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> - goto err_disable_3v3;
> - }
> -
> - err = regulator_enable(rockchip->vpcie0v9);
> - if (err) {
> - dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> - goto err_disable_1v8;
> - }
> -
> - return 0;
> -
> -err_disable_1v8:
> - regulator_disable(rockchip->vpcie1v8);
> -err_disable_3v3:
> - if (!IS_ERR(rockchip->vpcie3v3))
> - regulator_disable(rockchip->vpcie3v3);
> -err_disable_12v:
> - if (!IS_ERR(rockchip->vpcie12v))
> - regulator_disable(rockchip->vpcie12v);
> -err_out:
> - return err;
> -}
> -
> static void rockchip_pcie_enable_interrupts(struct rockchip_pcie *rockchip)
> {
> rockchip_pcie_write(rockchip, (PCIE_CLIENT_INT_CLI << 16) &
> -- 
> 2.49.0
> 

Re: [RFC PATCH 1/2] PCI: rockchip-host: Retry link training on failure without PERST#

[Geraldo Nascimento]

On Wed, Jun 04, 2025 at 02:20:20PM +0400, Christian Hewitt wrote:
> I think you have me confused with another Christian (or auto-complete
> scored a new victim). Sadly I have no clue about PCI and not a lot of
> knowledge on RK3399 matters :)
> 
> CH.

Hi Christian,

I did not confuse you. I thought your work with LibreElec partially
involved getting the ol' hand dirty with RK3399 patches. But if it's not
the case, I'll drop you from future Cc: on this subject.

Thanks!
Geraldo Nascimento