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 -- மணிவண்ணன் சதாசிவம்