[PATCH v3 10/12] PCI: rockchip-ep: Improve link training

[Manivannan Sadhasivam]

On Fri, Oct 11, 2024 at 05:55:25PM +0900, Damien Le Moal wrote:
> On 10/10/24 19:35, Manivannan Sadhasivam wrote:
> >> +static void rockchip_pcie_ep_link_training(struct work_struct *work)
> >> +{
> >> +	struct rockchip_pcie_ep *ep =
> >> +		container_of(work, struct rockchip_pcie_ep, link_training.work);
> >> +	struct rockchip_pcie *rockchip = &ep->rockchip;
> >> +	struct device *dev = rockchip->dev;
> >> +	u32 val;
> >> +	int ret;
> >> +
> >> +	/* Enable Gen1 training and wait for its completion */
> >> +	ret = readl_poll_timeout(rockchip->apb_base + PCIE_CORE_CTRL,
> >> +				 val, PCIE_LINK_TRAINING_DONE(val), 50,
> >> +				 LINK_TRAIN_TIMEOUT);
> >> +	if (ret)
> >> +		goto again;
> >> +
> >> +	/* Make sure that the link is up */
> >> +	ret = readl_poll_timeout(rockchip->apb_base + PCIE_CLIENT_BASIC_STATUS1,
> >> +				 val, PCIE_LINK_UP(val), 50,
> >> +				 LINK_TRAIN_TIMEOUT);
> >> +	if (ret)
> >> +		goto again;
> >> +
> >> +	/* Check the current speed */
> >> +	val = rockchip_pcie_read(rockchip, PCIE_CORE_CTRL);
> >> +	if (!PCIE_LINK_IS_GEN2(val) && rockchip->link_gen == 2) {
> > 
> > PCIE_LINK_IS_GEN2()?
> 
> This is defined in drivers/pci/controller/pcie-rockchip.h. What is it exactly
> you would like to know about this ?
> 

!PCIE_LINK_IS_GEN2 means check is for non-Gen2 mode, isn't it? I guess the check
should be 'if (PCIE_LINK_IS_GEN2...)

> > 
> >> +		/* Enable retrain for gen2 */
> >> +		rockchip_pcie_ep_retrain_link(rockchip);
> >> +		readl_poll_timeout(rockchip->apb_base + PCIE_CORE_CTRL,
> >> +				   val, PCIE_LINK_IS_GEN2(val), 50,
> >> +				   LINK_TRAIN_TIMEOUT);
> >> +	}
> >> +
> >> +	/* Check again that the link is up */
> >> +	if (!rockchip_pcie_ep_link_up(rockchip))
> >> +		goto again;
> > 
> > TRM doesn't mention this check. Is this really necessary?
> 
> I think so, to check the result of the second training for gen2.
> Even though the TRM does not say so, I prefer checking that the result is what
> we expect: the link is up.
> 

Ok.

- Mani

-- 
மணிவண்ணன் சதாசிவம்