Re: [PATCH RFC 0/2] PCI: rockchip-host: Retry training and hot-reset a JMB585 on RK3399

[Shawn Lin <shawn.lin@rock-chips.com>]

[-- Attachment #1: Type: text/plain, Size: 3171 bytes --]

Hi Igor,

在 2026/10/24 星期六 9:02, Igor Velkov via B4 Relay 写道:
> How should the host recover an endpoint that trains the link but does
> not answer config requests? On RK3399 that read ends in an external
> abort (an SError at probe, a synchronous abort on resume), and the
> kernel oopses instead of seeing an absent device.
> 
> This happens on a Kobol Helios64 (RK3399, JMicron JMB585 SATA) with
> v7.3-rc7 and the mainline DT, in two forms:
> 
> - link training times out on resume from s2idle (patch 1);
> - the link trains, but a config read of the JMB585 aborts in
>    pci_power_up() during resume, and during the bus scan at probe on
>    1 of 4 boots (patch 2).
> 
> Patch 1 trains again after a timeout. Patch 2 hot-resets the endpoint
> after training and waits the 100 ms required after a Conventional
> Reset, so every RK3399 probe and resume takes ~105 ms longer.
> 
> s2idle with both patches:
> 
>    sleep    patch 1 only    both patches
>    ~40 s    2 aborts / 30   0 / 100
>    ~25 s    1 abort / 11    0 / 40, then 2 aborts in the next 3
> 
> In the last of those aborts the link came back 25 ms after the hot
> reset instead of 2.8-2.9 ms, and the endpoint did not answer. We could
> not reproduce that state on demand.
> 
> host_init_port() already pulses PERST# and waits 100 ms before link
> training, so config reads start as soon as the link is up. Waiting 1 s
> after link-up before the bus scan did not help: 2 of 2 rebinds aborted,
> against 2 of 25 without the wait. That test ran without PERST# (the DT
> had no ep-gpios).
> 
> The data is from the code as posted, except that patch 2 now also fails
> probe and resume when the hot reset itself fails; that path was not hit.
> 
> Questions:
> 1. Is an unconditional hot reset after training acceptable, or should
>     it depend on the board?

Probably it depends on the board.

> 2. Can the host tell before a config read that the endpoint will not
>     answer?

The host cannot distinguish this case. If the endpoint has not prepared
to respond to a configuration read TLP, the controller should return all 
ones to the AXI bus instead of triggering a SError. Unfortunately, this 
is a limitation of the RK3399.

> 3. Is PERST# or a power cycle of the slot the expected recovery from a
>     slow link after the hot reset?

You've tried PERST# with the patch 1 applied. So perhaps it's doesn't
help.

> 4. Should the reset be a Secondary Bus Reset through the Root Port
>     rather than PCIE_CLIENT_HOT_RESET_CTRL? We did not try it.
> 

They are identical from the endpoint's point of view.

I've attached a vendor's debug patch, perhaps you could try it without
your patch.

> ---
> Igor Velkov (2):
>        PCI: rockchip-host: Retry link training that times out
>        PCI: rockchip-host: Hot-reset the endpoint before using the link
> 
>   drivers/pci/controller/pcie-rockchip-host.c | 88 ++++++++++++++++++++++++++++-
>   drivers/pci/controller/pcie-rockchip.h      |  4 ++
>   2 files changed, 89 insertions(+), 3 deletions(-)
> ---
> base-commit: 732c4f7d2c04f8c175d85d9243213783ee024ebc
> change-id: 20261018-rockchip-pcie-resume-2c4dfe23341e
> 
> Best regards,

[-- Attachment #2: 0001-arm64-pci-rockchip-Add-a-SError-handler-hook-for-the.patch --]
[-- Type: text/plain, Size: 4551 bytes --]

From 9b145593a514d0b5cd4e710129d60c54f2368e2e Mon Sep 17 00:00:00 2001
From: Shawn Lin <shawn.lin@rock-chips.com>
Date: Mon, 26 Oct 2026 16:55:32 +0800
Subject: [PATCH] arm64: pci: rockchip: Add a SError handler hook for the PCIe
 host

Some RK3399 boards with a PCIe switch behind the Rockchip host
controller see a config read end in a synchronous external abort, which
do_serror() treats as fatal and panics the kernel.

Allow a platform driver to install a handler for SError. The handler
runs before the default handling and, when it returns 0, the error is
considered handled and the kernel keeps running. Give it the faulting
address from FAR_EL1 and the ESR value, so the handler can decide
whether the error belongs to its device.

The Rockchip PCIe host driver registers such a handler once the link is
up and just logs the address and ESR for now.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
 arch/arm64/include/asm/system_misc.h        |  3 +++
 arch/arm64/kernel/traps.c                   | 18 ++++++++++++++++++
 drivers/pci/controller/pcie-rockchip-host.c | 17 +++++++++++++++++
 drivers/pci/controller/pcie-rockchip.h      |  5 +++++
 4 files changed, 43 insertions(+)

diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
index d316a804eb38..299839f517eb 100644
--- a/arch/arm64/include/asm/system_misc.h
+++ b/arch/arm64/include/asm/system_misc.h
@@ -25,6 +25,9 @@ void arm64_notify_die(const char *str, struct pt_regs *regs,
 		      int signo, int sicode, unsigned long far,
 		      unsigned long err);
 
+void *hook_serror_handler(int (*fn)(unsigned long, unsigned long,
+				    struct pt_regs *));
+
 struct mm_struct;
 extern void __show_regs(struct pt_regs *);
 
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index d6391ab20db3..d87bd1bd0ee8 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -946,6 +946,9 @@ void __noreturn arm64_serror_panic(struct pt_regs *regs, unsigned long esr)
 	cpu_park_loop();
 }
 
+static int (*serror_handler)(unsigned long, unsigned long,
+			     struct pt_regs *) __ro_after_init;
+
 bool arm64_is_fatal_ras_serror(struct pt_regs *regs, unsigned long esr)
 {
 	unsigned long aet = arm64_ras_serror_get_severity(esr);
@@ -980,11 +983,26 @@ bool arm64_is_fatal_ras_serror(struct pt_regs *regs, unsigned long esr)
 
 void do_serror(struct pt_regs *regs, unsigned long esr)
 {
+	unsigned long addr = read_sysreg(far_el1);
+
+	if (serror_handler && !serror_handler(addr, esr, regs))
+		return;
+
 	/* non-RAS errors are not containable */
 	if (!arm64_is_ras_serror(esr) || arm64_is_fatal_ras_serror(regs, esr))
 		arm64_serror_panic(regs, esr);
 }
 
+void *hook_serror_handler(int (*fn)(unsigned long, unsigned long,
+				    struct pt_regs *))
+{
+	void *ret = serror_handler;
+
+	serror_handler = fn;
+
+	return ret;
+}
+
 /* GENERIC_BUG traps */
 #ifdef CONFIG_GENERIC_BUG
 int is_valid_bugaddr(unsigned long addr)
diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index 5b071bdc4f90..4640adee127a 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -904,6 +904,19 @@ static int rockchip_pcie_resume_noirq(struct device *dev)
 	return err;
 }
 
+#ifdef CONFIG_ARM64
+static int (*serror_chain)(unsigned long addr, unsigned long esr,
+			   struct pt_regs *regs);
+
+static int do_rockchip_pcie_serror(unsigned long addr, unsigned long esr,
+				   struct pt_regs *regs)
+{
+	pr_info("rockchip-pcie: SError handled, addr %#lx esr %#lx\n",
+		addr, esr);
+	return 0;
+}
+#endif
+
 static int rockchip_pcie_probe(struct platform_device *pdev)
 {
 	struct rockchip_pcie *rockchip;
@@ -969,6 +982,10 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 	if (err < 0)
 		goto err_remove_irq_domain;
 
+#ifdef CONFIG_ARM64
+	serror_chain = hook_serror_handler(do_rockchip_pcie_serror);
+#endif
+
 	return 0;
 
 err_remove_irq_domain:
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index b5da15601b58..291befcdf6c3 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -18,6 +18,11 @@
 #include <linux/pci-ecam.h>
 #include <linux/reset.h>
 
+#ifdef CONFIG_ARM64
+#include <asm/signal.h>
+#include <asm/system_misc.h>
+#endif
+
 /*
  * The upper 16 bits of PCIE_CLIENT_CONFIG are a write mask for the lower 16
  * bits.  This allows atomic updates of the register without locking.
-- 
2.25.1