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

[Christian Hewitt <christianshewitt@gmail.com>]

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