Revert "PCI: rockchip: dw: remove .link_up() hook from struct dw_pcie…

[RadxaStephen]

There is one PCIe to SATA controller ASM1164 on ROCK 5 ITX.
This helps resolve the issue where SATA SSDs are sometimes not recognized on ROCK 5 ITX.

AR-2434

[amazingfate]

nice work!

[igorpecovnik]

I was just getting on this ;) Thanks.

[rpardini]

I've been facing some recently-introduced pci/nvme trouble with vendor (6.1-rkr3) kernel; none with 5.10-rkr6.

As far as I can tell, if PCI is enumerated/NVMe scanned during u-boot (and works in u-boot!), then vendor kernel can't see the NVMe.
If I force u-boot to skip the pci/nvme enumeration, then NVMe works when kernel booted (from eMMC).

The vendor u-boot scans NVMe by default if it's enabled in the defconfig and has the correct nodes in DT.

I've the impression this changed between the first drop of 6.1-rkr3 and now, and a quick visual scan reveals this commit shows up as the only PCI-related change in the vendor kernel since the rkr3 drop; I've not investigated u-boot changes yet.

Before I go reverting this locally to test, any opinions? This is on Blade3 btw.

[amazingfate]

If this revert breaks other boards, I think it's better to make it rock5-itx only by introducing a devicetree property.

[Joshua-Riek]

I had a few bug reports related to PCIe devices such as the dual NIC not working on the Orange Pi 5 Plus with this commit.

Ref: Joshua-Riek/ubuntu-rockchip#1011

[amazingfate]

The revert commit is from rockchip and from commit msg:

Revert this commit as it wasn't reliably work as expected by massive test.
The problem is clear now that cxpl_debug_info from DWC core is missing
rdlh_link_up. So reading PCIE_PORT_DEBUG1 and check smlh_link_up isn't enough.

When ((val & (RDLH_LINKUP | SMLH_LINKUP)) == 0x30000), link_up will return 1.
@rpardini we may add log print to see what's the value of (val & (RDLH_LINKUP | SMLH_LINKUP), and see if we can fix the issue by relaxing the condition.

[amazingfate]

Here is the mainline driver code: https://github.com/torvalds/linux/blob/v6.10/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L158-L160, which is the same as

if ((val & (RDLH_LINKUP | SMLH_LINKUP)) == 0x30000 &&
    (val & PCIE_LTSSM_STATUS_MASK) == PCIE_L0S_ENTRY)
        return 1;

While the vendor driver code has

if ((val & (RDLH_LINKUP | SMLH_LINKUP)) == 0x30000)
    return 1;

Mainline code has extra check (val & PCIE_LTSSM_STATUS_MASK) == PCIE_L0S_ENTRY), while both PCIE_LTSSM_STATUS_MASK and PCIE_L0S_ENTRY are not defined in vendor driver.

[amazingfate]

Here is a patch showing result of rk_pcie_readl_apb when rk_pcie_link_up is going to return 1:

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 94b221b5b614..03c79f0fcd5f 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -281,7 +281,10 @@ static int rk_pcie_link_up(struct dw_pcie *pci)

        val = rk_pcie_readl_apb(rk_pcie, PCIE_CLIENT_LTSSM_STATUS);
        if ((val & (RDLH_LINKUP | SMLH_LINKUP)) == 0x30000)
+       {
+               dev_info(pci->dev, "%s: val & (RDLH_LINKUP | SMLH_LINKUP) is 0x%lx, val & GENMASK(5, 0) is 0x%lx, going to return 1\n", __func__, (val & (RDLH_LINKUP | SMLH_LINKUP)), (val & GENMASK(5, 0)));
                return 1;
+       }

        return 0;
 }

On rock5b I can see

rk-pcie fe150000.pcie: rk_pcie_link_up: val & (RDLH_LINKUP | SMLH_LINKUP) is 0x30000, val & GENMASK(5, 0) is 0x11, going to return 1

while nvme ssd is still booting.

[amazingfate]

@rpardini you can see if this patch can make things better: amazingfate@6b67a8d
This patch will do the same like mainline's driver.

[rpardini]

Super thanks. I've added the dev_info debug as suggested, but it spews a lot and I'm still parsing to make a comparison between u-boot enumerating and not.

[rpardini]

@rpardini you can see if this patch can make things better: amazingfate@6b67a8d

This makes the NVMe work (5 out of 5 reboots). That is with u-boot enumerating NVMe (as is standard).

Unfortunately, it also makes the RTL8169's not work at all (similar to the report Joshua sent).

For now, the only setup I can arrange that makes both NVMe work in kernel when scanned in u-boot and the ethernets work is fully reverting this PR.