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

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

On Mon, Jan 29, 2024 at 06:56:24PM +0530, Ajay Agarwal wrote:
> On Mon, Jan 29, 2024 at 01:42:54PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Jan 29, 2024 at 01:34:52PM +0530, Ajay Agarwal wrote:
> > > On Mon, Jan 29, 2024 at 12:40:25PM +0530, Manivannan Sadhasivam wrote:
> > > > On Mon, Jan 29, 2024 at 12:21:51PM +0530, Ajay Agarwal wrote:
> > > > > 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?
> > > > > 
> > > > 
> > > > If that's the case with your driver, when are you starting the link training?
> > > > 
> > > The link training starts later based on a userspace/debugfs trigger.
> > > 
> > 
> > Why does it happen as such? What's the problem in starting the link during
> > probe? Keep it in mind that if you rely on the userspace for starting the link
> > based on a platform (like Android), then if the same SoC or peripheral instance
> > get reused in other platform (non-android), the it won't be a seamless user
> > experience.
> > 
> > If there are any other usecases, please state them.
> > 
> > - Mani
> >
> This SoC is targeted for an android phone usecase and the endpoints
> being enumerated need to go through an appropriate and device specific
> power sequence which gets triggered only when the userspace is up. The
> PCIe probe cannot assume that the EPs have been powered up already and
> hence the link-up is not attempted.

Still, I do not see the necessity to not call start_link() during probe. If you
add PROBE_PREFER_ASYNCHRONOUS to your controller driver, this delay would become
negligible. The reason why I'm against not calling start_link() is due to below
reasons:

1. If the same SoC gets reused for other platforms, even other android phones
that powers up the endpoints during boot, then it creates a dependency with
userspace to always start the link even though the devices were available.
That's why we should never fix the behavior of the controller drivers based on a
single platform.

2. This will create fragmentation among the DWC glue drivers w.r.t the behavior
and will become a maintenance nightmare (there are enough already).

So, I'd suggest you to use the asynchronous probe mentioned above so that other
drivers may probe in parallel thus avoiding the delay that you are worried about.

- Mani

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