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

[Koichiro Den]

On Tue, Sep 22, 2026 at 10:56:02PM +0200, Marek Vasut wrote:
> 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 ?

If always relying on the PORT_DEBUG1 check instead of the SMLH/RDLH check does
not introduce any regressions, I'd personally prefer to keep patch 2 as-is
because it keeps the code simpler. We could just add a comment noting that this
link-up check diverges from Figure 104.5.

That said, I agree that in general we should follow the R-Car reference manual
where possible, and your draft makes sense for that purpose. If we go that way,
I have one question: would we need a polling loop with a timeout
(PCIE_LINK_WAIT_MAX_RETRIES * PCIE_LINK_WAIT_SLEEP_MS) in
rcar_gen4_pcie_start_link(), similar to dw_pcie_wait_for_link()?

P.S. I'll rebase v2 onto the latest pci/controller/dwc-rcar-gen4.

Thanks for the review.

Best regards,
Koichiro

> 
> 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