Re: [PATCH v5] PCI: dwc: Wait for link up only if link is started

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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

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