Re: [PATCH 02/11] PCI: rcar-gen4: Drop the APP-based link_up check
From: Marek Vasut
Date: Tue Sep 22 2026 - 17:03:05 EST
Hello Den-san,
I apologize for my late reply.
On 9/18/26 5:20 AM, Koichiro Den wrote:
rcar_gen4_pcie_link_up() checks link state using SMLH_LINK_UP and
RDLH_LINK_UP in PCIEINTSTS0. However, these bits do not reflect the live
link state. On an R-Car S4, after taking down the endpoint, a link-down
interrupt saw PCIEINTSTS0 = 0x20a000c5 with both bits still set. Even
after resetting the controller with the LTSSM back in Polling, they read
0xa000c5, still set.
As a result, dw_pcie_link_up() keeps reporting the link as up after it
has gone down. That defeats the check in dw_pcie_other_conf_map_bus(),
which is supposed to stop config accesses to downstream devices while
the link is down, so such accesses go out on the dead link and stall the
host. It also makes the callback useless for the link-down recovery
added later, which has to wait for the link to actually come back after
resetting the controller.
Drop the callback and let the DesignWare core use its PORT_DEBUG1 check
instead, which correctly detects the downed link.
Fixes: 0d0c551011df ("PCI: rcar-gen4: Add R-Car Gen4 PCIe controller support for host mode")
Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
---
drivers/pci/controller/dwc/pcie-rcar-gen4.c | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index 5a076aa3f490..fe1f1940e809 100644
--- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
+++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
@@ -44,8 +44,6 @@
/* PCIe Interrupt Status 0 Enable */
#define PCIEINTSTS0EN 0x0310
#define MSI_CTRL_INT BIT(26)
-#define SMLH_LINK_UP BIT(7)
-#define RDLH_LINK_UP BIT(6)
/* PCIe DMA Interrupt Status Enable */
#define PCIEDMAINTSTSEN 0x0314
@@ -102,17 +100,6 @@ struct rcar_gen4_pcie {
#define to_rcar_gen4_pcie(_dw) container_of(_dw, struct rcar_gen4_pcie, dw)
/* Common */
-static bool rcar_gen4_pcie_link_up(struct dw_pcie *dw)
-{
- struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
- u32 val, mask;
-
- val = readl(rcar->base + PCIEINTSTS0);
- mask = RDLH_LINK_UP | SMLH_LINK_UP;
-
- return (val & mask) == mask;
-}
-
/*
* Manually initiate the speed change. Return 0 if change succeeded; otherwise
* -ETIMEDOUT.
@@ -298,7 +285,6 @@ static int rcar_gen4_pcie_get_resources(struct rcar_gen4_pcie *rcar)
static const struct dw_pcie_ops dw_pcie_ops = {
.start_link = rcar_gen4_pcie_start_link,
.stop_link = rcar_gen4_pcie_stop_link,
- .link_up = rcar_gen4_pcie_link_up,
};
static struct rcar_gen4_pcie *rcar_gen4_pcie_alloc(struct platform_device *pdev)
Can we include some form of the draft patch below, so the S4 Reference Manual rev.1.40 , page 1564 , Figure 104.5 Initial Setting of PCIEC , bottommost diamond in the figure (smlh_link_up and rdlh_link_up = 1 test), would still be fulfilled, and the initialization code in the driver would not diverge from the initialization sequence listed in the reference manual ? What do you think ?
The PCIEINTSTS0CLR should clear the link state bits before the link gets started, so the initialization code should be able to sample those bits after the link came up and confirm they were set during the link up.
The PCIEINTSTS0CLR usage however won't solve the case where the DWC PCIe core code has to sample PCIe link state at arbitrary time, this is what this patch does solve correctly.
"
diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index e5833f36625d2..9f29055f0bed7 100644
--- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
+++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
@@ -203,17 +203,28 @@ static int rcar_gen5_pcie_speed_control(struct rcar_gen4_pcie *rcar)
* Enable LTSSM of this controller and manually initiate the speed change.
* Always return 0.
*/
+#define PCIEINTSTS0CLR 0x0340
static int rcar_gen4_pcie_start_link(struct dw_pcie *dw)
{
struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
+ u32 val, mask;
int ret;
+ /* Clear RDLH/SMLH link state */
+ writel(RDLH_LINK_UP | SMLH_LINK_UP, rcar->base + PCIEINTSTS0CLR);
+
if (rcar->drvdata->ltssm_control) {
ret = rcar->drvdata->ltssm_control(rcar, true);
if (ret)
return ret;
}
+ ret = rcar->drvdata->speed_control(rcar);
+ if (ret)
+ return ret;
+
+ val = readl(rcar->base + PCIEINTSTS0);
+ mask = RDLH_LINK_UP | SMLH_LINK_UP;
+ return ((val & mask) == mask)
}
@@ -223,6 +234,9 @@ static void rcar_gen4_pcie_stop_link(struct dw_pcie *dw)
if (rcar->drvdata->ltssm_control)
rcar->drvdata->ltssm_control(rcar, false);
+
+ /* Clear RDLH/SMLH link state */
+ writel(RDLH_LINK_UP | SMLH_LINK_UP, rcar->base + PCIEINTSTS0CLR);
}
static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
"
--
Best regards,
Marek Vasut