Re: [PATCH v4 6/6] PCI: dwc: rcar-gen4: Add support for R-Car X5H PCIe4

[Marek Vasut]

On 9/21/26 6:03 PM, Manivannan Sadhasivam wrote:

Hello Manivannan,

> > +static int rcar_gen5_pcie_speed_control(struct rcar_gen4_pcie *rcar)
> > +{
> > +	struct dw_pcie *dw = &rcar->dw;
> > +	u32 lnkcap = dw_pcie_readl_dbi(dw, EXPCAP(PCI_EXP_LNKCAP));
> > +	u32 lnksta = dw_pcie_readw_dbi(dw, EXPCAP(PCI_EXP_LNKSTA));
> > +	u32 val, retries;
> > +
> > +	if ((lnksta & PCI_EXP_LNKSTA_CLS) == (lnkcap & PCI_EXP_LNKCAP_SLS))
> > +		return 0;
> > +
> > +	/* Retrain link */
> > +	val = dw_pcie_readw_dbi(dw, EXPCAP(PCI_EXP_LNKCTL));
> > +	val |= PCI_EXP_LNKCTL_RL;
> > +	dw_pcie_writew_dbi(dw, EXPCAP(PCI_EXP_LNKCTL), val);
> > +
> > +	/* Wait for link retrain */
> > +	for (retries = 0; retries <= 10; retries++) {
>
> So this loop executes 11 times. Is that intented?

No, I don't think this is correct, or in fact sufficient. The training 
should poll for at least 100ms.

> > +		lnksta = dw_pcie_readw_dbi(dw, EXPCAP(PCI_EXP_LNKSTA));
> > +
> > +		/* Check retrain flag */
> > +		if (!(lnksta & PCI_EXP_LNKSTA_LT))
> > +			break;
> > +
> > +		usleep_range(1000, 1100);
>
> What is the expected behavior if the Link retrain is not successfull? If it is
> safe to continue, shouldn't the users be warned atleast?

They should, I will be replacing this with this simpler option, which 
returns -ETIMEDOUT in case the training failed:

diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c 
b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index a5feb73864c7f..7f4bda6a492ab 100644
--- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
+++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
@@ -183,7 +183,7 @@ static int rcar_gen5_pcie_speed_control(struct 
rcar_gen4_pcie *rcar)
        struct dw_pcie *dw = &rcar->dw;
        u32 lnkcap = dw_pcie_readl_dbi(dw, EXPCAP(PCI_EXP_LNKCAP));
        u32 lnksta = dw_pcie_readw_dbi(dw, EXPCAP(PCI_EXP_LNKSTA));
-       u32 val, retries;
+       u32 val;

        if ((lnksta & PCI_EXP_LNKSTA_CLS) == (lnkcap & PCI_EXP_LNKCAP_SLS))
                return 0;
@@ -193,18 +193,10 @@ static int rcar_gen5_pcie_speed_control(struct 
rcar_gen4_pcie *rcar)
        val |= PCI_EXP_LNKCTL_RL;
        dw_pcie_writew_dbi(dw, EXPCAP(PCI_EXP_LNKCTL), val);

-       /* Wait for link retrain */
-       for (retries = 0; retries <= 10; retries++) {
-               lnksta = dw_pcie_readw_dbi(dw, EXPCAP(PCI_EXP_LNKSTA));
-
-               /* Check retrain flag */
-               if (!(lnksta & PCI_EXP_LNKSTA_LT))
-                       break;
-
-               usleep_range(1000, 1100);
-       }
-
-       return 0;
+       /* Wait for link retrain, 500ms must be enough for all link 
rates. */
+       return read_poll_timeout(dw_pcie_readw_dbi, lnksta, !(lnksta & 
PCI_EXP_LNKSTA_LT),
+                                1000, 5 * PCIE_RESET_CONFIG_WAIT_MS * 
USEC_PER_MSEC,
+                                false, dw, EXPCAP(PCI_EXP_LNKSTA));
 }

 /*

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >   /*
> >    * Enable LTSSM of this controller and manually initiate the speed change.
> >    * Always return 0.
> > @@ -285,6 +322,49 @@ static int rcar_gen4_v4h_v4m_pcie_init(struct rcar_gen4_pcie *rcar)
> >   	return 0;
> >   }
> >   

[...]

> > +static int rcar_gen5_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
> > +{
> > +	u32 val;
> > +
> > +	val = readl(rcar->base + PCIERSTCTRL1);
> > +	if (enable) {
> > +		val |= APP_LTSSM_ENABLE;
> > +		val &= ~APP_HOLD_PHY_RST;
> > +	} else {
> > +		val &= ~APP_LTSSM_ENABLE;
> > +		val |= APP_HOLD_PHY_RST;
> > +	}
> > +	writel(val, rcar->base + PCIERSTCTRL1);
> > +
> > +	if (enable)
> > +		phy_power_on(rcar->phy);
> > +	else
> > +		phy_power_off(rcar->phy);
>
> It is quite unusual to handle PHY power on/off in the start_link() callback.
> Should this be moved to init/deinit callbacks?

This really has to be here, else the controller programming order does 
not follow the sequence in the SoC datasheet.