We will work on the updated version to remove the duplication. Chuanhua From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> Sent: Thursday, February 1, 2024 3:32 PM To: Bjorn Helgaas <helgaas@xxxxxxxxxx> Cc: Ajay Agarwal <ajayagarwal@xxxxxxxxxx>; Jingoo Han <jingoohan1@xxxxxxxxx>; Johan Hovold <johan+linaro@xxxxxxxxxx>; Jon Hunter <jonathanh@xxxxxxxxxx>; Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx>; Krzysztof Wilczyński <kw@xxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Bjorn Helgaas <bhelgaas@xxxxxxxxxx>; Manu Gautam <manugautam@xxxxxxxxxx>; Doug Zobel <zobel@xxxxxxxxxx>; William McVicker <willmcvicker@xxxxxxxxxx>; Serge Semin <fancer.lancer@xxxxxxxxx>; Robin Murphy <robin.murphy@xxxxxxx>; linux-pci@xxxxxxxxxxxxxxx <linux-pci@xxxxxxxxxxxxxxx>; Lei Chuan Hua <lchuanhua@xxxxxxxxxxxxx> Subject: Re: [PATCH v5] PCI: dwc: Wait for link up only if link is started This email was sent from outside of MaxLinear. On Wed, Jan 31, 2024 at 09:14:13PM -0600, Bjorn Helgaas wrote: > [+cc Chuanhua Lei, intel-gw maintainer, sorry I forgot this!] > > On Wed, Jan 31, 2024 at 05:48:17PM -0600, Bjorn Helgaas 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. > > > > > > 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. > > > > What fixup do you have in mind for intel-gw? > > > > It looks a little strange to me because it duplicates > > dw_pcie_setup_rc() and dw_pcie_wait_for_link(): dw_pcie_host_init() > > calls them first via pp->ops->init(), and then calls them a second > > time directly: > > > > struct dw_pcie_host_ops intel_pcie_dw_ops = { > > .init = intel_pcie_rc_init > > } > > > > intel_pcie_probe > > pp->ops = &intel_pcie_dw_ops > > dw_pcie_host_init(pp) > > if (pp->ops->init) > > pp->ops->init > > intel_pcie_rc_init > > intel_pcie_host_setup > > dw_pcie_setup_rc # <-- > > dw_pcie_wait_for_link # <-- > > dw_pcie_setup_rc # <-- > > dw_pcie_wait_for_link # <-- > > > > Is that what you're thinking? > > Right. There is no need of this driver duplicating dw_pcie_setup_rc() and dw_pcie_wait_for_link(). Perhaps those functions were added to dw_pcie_host_init() after this driver got upstreamed and the author failed to take this driver into account. But my point was, the new drivers _should_not_ take inspiration from this driver to not define start_link() callback at the first place (unless there is a real requirement). - Mani -- மணிவண்ணன் சதாசிவம்