On Sat, Jan 20, 2024 at 08:04:34PM +0530, Manivannan Sadhasivam wrote: > On Fri, Jan 19, 2024 at 11:29:22PM +0530, Ajay Agarwal wrote: > > On Fri, Jan 19, 2024 at 01:22:19PM +0530, Manivannan Sadhasivam wrote: > > > On Fri, Jan 12, 2024 at 03:00:06PM +0530, Ajay Agarwal wrote: > > > > In dw_pcie_host_init() regardless of whether the link has been > > > > started or not, the code waits for the link to come up. Even in > > > > cases where start_link() is not defined the code ends up spinning > > > > in a loop for 1 second. Since in some systems dw_pcie_host_init() > > > > gets called during probe, this one second loop for each pcie > > > > interface instance ends up extending the boot time. > > > > > > > > > > Which platform you are working on? Is that upstreamed? You should mention the > > > specific platform where you are observing the issue. > > > > > This is for the Pixel phone platform. The platform driver for the same > > is not upstreamed yet. It is in the process. > > > > Then you should submit this patch at the time of the driver submission. Right > now, you are trying to fix a problem which is not present in upstream. One can > argue that it is a problem for designware-plat driver, but honestly I do not > know how it works. > > - Mani > Yes Mani, this can be a problem for the designware-plat driver. To me, the problem of a second being wasted in the probe path seems pretty obvious. We will wait for the link to be up even though we are not starting the link training. Can this patch be accepted considering the problem in the dw-plat driver then? Additionally, I have made this patch a part of a series [1] to keep the functional and refactoring changes separate. Please let me know if you see a concern with that. [1] https://lore.kernel.org/linux-pci/20240124092533.1267836-1-ajayagarwal@xxxxxxxxxx/ > > > Right now, intel-gw and designware-plat are the only drivers not defining that > > > callback. First one definitely needs a fixup and I do not know how the latter > > > works. > > > > > > - Mani > > > > > > > Wait for the link up in only if the start_link() is defined. > > > > > > > > Signed-off-by: Ajay Agarwal <ajayagarwal@xxxxxxxxxx> > > > > --- > > > > v4 was applied, but then reverted. The reason being v4 added a > > > > regression on some products which expect the link to not come up as a > > > > part of the probe. Since v4 returned error from dw_pcie_wait_for_link > > > > check, the probe function of these products started to fail. > > > > > > > > Changelog since v4: > > > > - Do not return error from dw_pcie_wait_for_link check > > > > > > > > .../pci/controller/dwc/pcie-designware-host.c | 12 +++++++---- > > > > drivers/pci/controller/dwc/pcie-designware.c | 20 ++++++++++++------- > > > > drivers/pci/controller/dwc/pcie-designware.h | 1 + > > > > 3 files changed, 22 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > > > > index 7991f0e179b2..e53132663d1d 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > > > @@ -487,14 +487,18 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp) > > > > if (ret) > > > > goto err_remove_edma; > > > > > > > > - if (!dw_pcie_link_up(pci)) { > > > > + if (dw_pcie_link_up(pci)) { > > > > + dw_pcie_print_link_status(pci); > > > > + } else { > > > > ret = dw_pcie_start_link(pci); > > > > if (ret) > > > > goto err_remove_edma; > > > > - } > > > > > > > > - /* Ignore errors, the link may come up later */ > > > > - dw_pcie_wait_for_link(pci); > > > > + if (pci->ops && pci->ops->start_link) { > > > > + /* Ignore errors, the link may come up later */ > > > > + dw_pcie_wait_for_link(pci); > > > > + } > > > > + } > > > > > > > > bridge->sysdata = pp; > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > > index 250cf7f40b85..c067d2e960cf 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > > @@ -645,9 +645,20 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index) > > > > dw_pcie_writel_atu(pci, dir, index, PCIE_ATU_REGION_CTRL2, 0); > > > > } > > > > > > > > -int dw_pcie_wait_for_link(struct dw_pcie *pci) > > > > +void dw_pcie_print_link_status(struct dw_pcie *pci) > > > > { > > > > u32 offset, val; > > > > + > > > > + offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > > > > + val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA); > > > > + > > > > + dev_info(pci->dev, "PCIe Gen.%u x%u link up\n", > > > > + FIELD_GET(PCI_EXP_LNKSTA_CLS, val), > > > > + FIELD_GET(PCI_EXP_LNKSTA_NLW, val)); > > > > +} > > > > + > > > > +int dw_pcie_wait_for_link(struct dw_pcie *pci) > > > > +{ > > > > int retries; > > > > > > > > /* Check if the link is up or not */ > > > > @@ -663,12 +674,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci) > > > > return -ETIMEDOUT; > > > > } > > > > > > > > - offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > > > > - val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA); > > > > - > > > > - dev_info(pci->dev, "PCIe Gen.%u x%u link up\n", > > > > - FIELD_GET(PCI_EXP_LNKSTA_CLS, val), > > > > - FIELD_GET(PCI_EXP_LNKSTA_NLW, val)); > > > > + dw_pcie_print_link_status(pci); > > > > > > > > return 0; > > > > } > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > > > > index 55ff76e3d384..164214a7219a 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-designware.h > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h > > > > @@ -447,6 +447,7 @@ void dw_pcie_setup(struct dw_pcie *pci); > > > > void dw_pcie_iatu_detect(struct dw_pcie *pci); > > > > int dw_pcie_edma_detect(struct dw_pcie *pci); > > > > void dw_pcie_edma_remove(struct dw_pcie *pci); > > > > +void dw_pcie_print_link_status(struct dw_pcie *pci); > > > > > > > > int dw_pcie_suspend_noirq(struct dw_pcie *pci); > > > > int dw_pcie_resume_noirq(struct dw_pcie *pci); > > > > -- > > > > 2.43.0.381.gb435a96ce8-goog > > > > > > > > > > -- > > > மணிவண்ணன் சதாசிவம் > > -- > மணிவண்ணன் சதாசிவம்
