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

[Ajay Agarwal <ajayagarwal@google.com>]

On Tue, Jan 30, 2024 at 12:15:55PM +0530, Manivannan Sadhasivam wrote:
> 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
I am not adding any logic/condition around calling the start_link()
itself. I am only avoiding the wait for the link to be up if the
controller driver has not defined start_link().

> 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.
I wonder how the behavior is changing with this patch. Do you have an
example of a platform which does not have start_link() defined but would
like to still wait for a second for the link to come up?

For example, consider the intel-gw driver. The 1 sec wait time in its
probe path is also a waste because it explicitly starts link training
later in time.

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