Re: [PATCH 02/11] PCI: rcar-gen4: Drop the APP-based link_up check

[Marek Vasut <marek.vasut@mailbox.org>]

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@valinux.co.jp>
> ---
>   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