Re: [PATCH v2 0/9] PCI: rockchip: Fix RK3399 PCIe endpoint controller driver
From: Damien Le Moal
Date: Tue Mar 14 2023 - 20:01:02 EST
On 3/15/23 07:54, Damien Le Moal wrote:
> On 3/14/23 23:53, Rick Wertenbroek wrote:
>> Hello Damien,
>> I also noticed random issues I suspect to be related to link status or power
>> state, in my case it sometimes happens that the BARs (0-6) in the config
>> space get reset to 0. This is not due to the driver because the driver never
>> ever accesses these registers (@0xfd80'0010 to 0xfd80'0024 TRM
>> 17.6.4.1.5-17.6.4.1.10).
>> I don't think the host rewrites them because lspci shows the BARs as
>> "[virtual]" which means they have been assigned by host but have 0
>> value in the endpoint device (when lspci rereads the PCI config header).
>> See https://github.com/pciutils/pciutils/blob/master/lspci.c#L422
>>
>> So I suspect the controller detects something related to link status or
>> power state and internally (in hardware) resets those registers. It's not
>> the kernel code, it never accesses these regs. The problem occurs
>> very randomly, sometimes in a few seconds, sometimes I cannot see
>> it for a whole day.
>>
>> Is this similar to what you are experiencing ?
>
> Yes. I sometimes get NMIs after starting the function driver, when my function
> driver starts probing the bar registers after seeing the host changing one
> register. And the link also comes up with 4 lanes or 2 lanes, random.
>
>> Do you have any idea as to what could make these registers to be reset
>> (I could not find anything in the TRM, also nothing in the driver seems to
>> cause it).
>
> My thinking is that since we do not have a linkup notifier, the function driver
> starts setting things up without the link established (e.g. when the host is
> still powered down). Once the host start booting and pic link is established,
> things may be reset in the hardware... That is the only thing I can think of.
>
> And yes, there are definitely something going on with the power states too I
> think: if I let things idle for a few minutes, everything stops working: no
> activity seen on the endpoint over the BARs. I tried enabling the sys and client
> interrupts to see if I can see power state changes, or if clearing the
> interrupts helps (they are masked by default), but no change. And booting the
> host with pci_aspm=off does not help either. Also tried to change all the
> capabilities related to link & power states to "off" (not supported), and no
> change either. So currently, I am out of ideas regarding that one.
>
> I am trying to make progress on my endpoint driver (nvme function) to be sure it
> is not a bug there that breaks things. I may still have something bad because
> when I enable the BIOS native NVMe driver on the host, either the host does not
> boot, or grub crashes with memory corruptions. Overall, not yet very stable and
> still trying to sort out the root cause of that.
By the way, enabling the interrupts to see the error notifications, I do see a
lot of retry timeout and other recoverable errors. So the issues I am seeing
could be due to my PCI cable setup that is not ideal (bad signal, ground loops,
... ?). Not sure. I do not have a PCI analyzer handy :)
I attached the patches I used to enable the EP interrupts. Enabling debug prints
will tell you what is going on. That may give you some hints on your setup ?
--
Damien Le Moal
Western Digital Research
From fde667e889548a064009c45ea4891b78977962b9 Mon Sep 17 00:00:00 2001
From: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
Date: Tue, 28 Feb 2023 17:00:32 +0900
Subject: [PATCH] arm: rk3399-rockpro64: Add interrupts to PCI endpoint mode DT
node
Add subsystem and client interrupts definitions to the rockchip PCI
eendpoint node.
Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
---
arch/arm64/boot/dts/rockchip/rk3399.dtsi | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index c3489be3852d..92eb541fde9d 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -274,6 +274,9 @@ pcie0_ep: pcie-ep@f8000000 {
<&cru PCLK_PCIE>, <&cru SCLK_PCIE_PM>;
clock-names = "aclk", "aclk-perf",
"hclk", "pm";
+ interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH 0>,
+ <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH 0>;
+ interrupt-names = "sys", "client";
max-functions = /bits/ 8 <8>;
num-lanes = <4>;
reg = <0x0 0xfd000000 0x0 0x1000000>, <0x0 0xfa000000 0x0 0x2000000>;
--
2.39.2
From 9f236b9ac02e52407ea9d88920ada2887cec872c Mon Sep 17 00:00:00 2001
From: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
Date: Tue, 28 Feb 2023 16:57:01 +0900
Subject: [PATCH] PCI: rockchip: Add client and subsys interrupts for endpoint
mode
Add an interrupt handler for the client and subsystem interrupts to
the rockchip endpoint driver. Clearing these interrupts is necesssary
to avoid seeing the controller getting stuck on non fatal errors.
Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
---
drivers/pci/controller/pcie-rockchip-ep.c | 176 ++++++++++++++++++++++
1 file changed, 176 insertions(+)
diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 12db9a9d92af..c1d39d3b45da 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -50,6 +50,176 @@ struct rockchip_pcie_ep {
u8 irq_pending;
};
+static void rockchip_pcie_ep_handle_local_int(struct rockchip_pcie *rockchip)
+{
+ struct device *dev = rockchip->dev;
+ u32 reg;
+
+ dev_dbg(dev, "local interrupt received\n");
+
+ reg = rockchip_pcie_read(rockchip, PCIE_CORE_INT_STATUS);
+ if (reg & PCIE_CORE_INT_PRFPE)
+ dev_dbg(dev, "parity error detected while reading from the PNP receive FIFO RAM\n");
+
+ if (reg & PCIE_CORE_INT_CRFPE)
+ dev_dbg(dev, "parity error detected while reading from the Completion Receive FIFO RAM\n");
+
+ if (reg & PCIE_CORE_INT_RRPE)
+ dev_dbg(dev, "parity error detected while reading from replay buffer RAM\n");
+
+ if (reg & PCIE_CORE_INT_PRFO)
+ dev_dbg(dev, "overflow occurred in the PNP receive FIFO\n");
+
+ if (reg & PCIE_CORE_INT_CRFO)
+ dev_dbg(dev, "overflow occurred in the completion receive FIFO\n");
+
+ if (reg & PCIE_CORE_INT_RT)
+ dev_dbg(dev, "replay timer timed out\n");
+
+ if (reg & PCIE_CORE_INT_RTR)
+ dev_dbg(dev, "replay timer rolled over after 4 transmissions of the same TLP\n");
+
+ if (reg & PCIE_CORE_INT_PE)
+ dev_dbg(dev, "phy error detected on receive side\n");
+
+ if (reg & PCIE_CORE_INT_MTR)
+ dev_dbg(dev, "malformed TLP received from the link\n");
+
+ if (reg & PCIE_CORE_INT_UCR)
+ dev_dbg(dev, "malformed TLP received from the link\n");
+
+ if (reg & PCIE_CORE_INT_FCE)
+ dev_dbg(dev, "an error was observed in the flow control advertisements from the other side\n");
+
+ if (reg & PCIE_CORE_INT_CT)
+ dev_dbg(dev, "a request timed out waiting for completion\n");
+
+ if (reg & PCIE_CORE_INT_UTC)
+ dev_dbg(dev, "unmapped TC error\n");
+
+ if (reg & PCIE_CORE_INT_MMVC)
+ dev_dbg(dev, "MSI mask register changes\n");
+
+ rockchip_pcie_write(rockchip, reg, PCIE_CORE_INT_STATUS);
+}
+
+static irqreturn_t rockchip_pcie_ep_irq_handler(int irq, void *arg)
+{
+ struct rockchip_pcie *rockchip = arg;
+ struct device *dev = rockchip->dev;
+ u32 reg, mask = 0;
+
+ reg = rockchip_pcie_read(rockchip, PCIE_CLIENT_INT_STATUS);
+
+ if (reg & PCIE_CLIENT_INT_LEGACY_DONE) {
+ dev_dbg(dev, "legacy done interrupt received\n");
+ mask |= PCIE_CLIENT_INT_LEGACY_DONE;
+ }
+
+ if (reg & PCIE_CLIENT_INT_MSG) {
+ dev_dbg(dev, "message done interrupt received\n");
+ mask |= PCIE_CLIENT_INT_MSG;
+ }
+
+ if (reg & PCIE_CLIENT_INT_HOT_RST) {
+ dev_dbg(dev, "hot reset interrupt received\n");
+ mask |= PCIE_CLIENT_INT_HOT_RST;
+ }
+
+ if (reg & PCIE_CLIENT_INT_DPA) {
+ dev_dbg(dev, "dpa interrupt received\n");
+ mask |= PCIE_CLIENT_INT_DPA;
+ }
+
+ if (reg & PCIE_CLIENT_INT_FATAL_ERR) {
+ dev_dbg(dev, "fatal error interrupt received\n");
+ mask |= PCIE_CLIENT_INT_FATAL_ERR;
+ }
+
+ if (reg & PCIE_CLIENT_INT_NFATAL_ERR) {
+ dev_dbg(dev, "no fatal error interrupt received\n");
+ mask |= PCIE_CLIENT_INT_NFATAL_ERR;
+ }
+
+ if (reg & PCIE_CLIENT_INT_CORR_ERR) {
+ dev_dbg(dev, "correctable error interrupt received\n");
+ mask |= PCIE_CLIENT_INT_CORR_ERR;
+ }
+
+ if (reg & PCIE_CLIENT_INT_LOCAL) {
+ rockchip_pcie_ep_handle_local_int(rockchip);
+ mask |= PCIE_CLIENT_INT_LOCAL;
+ }
+
+ if (reg & PCIE_CLIENT_INT_UDMA) {
+ dev_dbg(dev, "uDMA interrupt received\n");
+ mask |= PCIE_CLIENT_INT_UDMA;
+ }
+
+ if (reg & PCIE_CLIENT_INT_PHY) {
+ dev_dbg(dev, "phy interrupt received\n");
+ mask |= PCIE_CLIENT_INT_PHY;
+ }
+
+ if (reg & PCIE_CLIENT_INT_PWR_STCG) {
+ dev_dbg(dev, "Power state change interrupt received\n");
+ mask |= PCIE_CLIENT_INT_PWR_STCG;
+ }
+
+ rockchip_pcie_write(rockchip, reg & mask, PCIE_CLIENT_INT_STATUS);
+
+ return IRQ_HANDLED;
+}
+
+static int rockchip_pcie_ep_setup_irq(struct rockchip_pcie *rockchip)
+{
+ int irq, err;
+ struct device *dev = rockchip->dev;
+ struct platform_device *pdev = to_platform_device(dev);
+
+ irq = platform_get_irq_byname(pdev, "sys");
+ if (irq < 0)
+ return irq;
+
+ /* PCIe subsystem interrupt */
+ err = devm_request_irq(dev, irq, rockchip_pcie_ep_irq_handler,
+ IRQF_SHARED, "pcie-ep-sys", rockchip);
+ if (err) {
+ dev_err(dev, "Request PCIe subsystem IRQ failed\n");
+ return err;
+ }
+
+ /* PCIe client interrupt */
+ irq = platform_get_irq_byname(pdev, "client");
+ if (irq < 0)
+ return irq;
+
+ err = devm_request_irq(dev, irq, rockchip_pcie_ep_irq_handler,
+ IRQF_SHARED, "pcie-ep-client", rockchip);
+ if (err) {
+ dev_err(dev, "Request PCIe client IRQ failed\n");
+ return err;
+ }
+
+ return 0;
+}
+
+static void rockchip_pcie_ep_enable_interrupts(struct rockchip_pcie *rockchip)
+{
+ u32 client_mask = PCIE_CLIENT_INT_LEGACY_DONE |
+ PCIE_CLIENT_INT_MSG | PCIE_CLIENT_INT_DPA |
+ PCIE_CLIENT_INT_FATAL_ERR | PCIE_CLIENT_INT_NFATAL_ERR |
+ PCIE_CLIENT_INT_CORR_ERR |
+ PCIE_CLIENT_INT_LOCAL | PCIE_CLIENT_INT_UDMA |
+ PCIE_CLIENT_INT_PWR_STCG;
+
+ rockchip_pcie_write(rockchip, (client_mask << 16) & ~client_mask,
+ PCIE_CLIENT_INT_MASK);
+
+ rockchip_pcie_write(rockchip, (u32)(~PCIE_CORE_INT),
+ PCIE_CORE_INT_MASK);
+}
+
static inline int rockchip_ob_region(u64 phys_addr)
{
return (phys_addr >> ilog2(SZ_1M)) & 0x1f;
@@ -637,6 +807,12 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
rockchip_pcie_write(rockchip, PCIE_CLIENT_CONF_ENABLE, PCIE_CLIENT_CONFIG);
+ err = rockchip_pcie_ep_setup_irq(rockchip);
+ if (err)
+ goto err_epc_mem_exit;
+
+ rockchip_pcie_ep_enable_interrupts(rockchip);
+
return 0;
err_epc_mem_exit:
pci_epc_mem_exit(epc);
--
2.39.2