Re: [PATCH] pci: aardvark: Wait for endpoint to be ready before training link

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Apr 25, 2019 at 11:08:27PM +0200, Remi Pommarel wrote:
> On Wed, Apr 24, 2019 at 05:50:02PM +0100, Lorenzo Pieralisi wrote:
> > On Wed, Apr 24, 2019 at 12:29:18AM +0200, Remi Pommarel wrote:
> > > On Tue, Apr 23, 2019 at 05:32:15PM +0100, Lorenzo Pieralisi wrote:
> > > > On Wed, Mar 13, 2019 at 10:37:52PM +0100, Remi Pommarel wrote:
> > > > > When configuring pcie reset pin from gpio (e.g. initially set by
> > > > > u-boot) to pcie function this pin goes low for a brief moment
> > > > > asserting the PERST# signal. Thus connected device enters fundamental
> > > > > reset process and link configuration can only begin after a minimal
> > > > > 100ms delay (see [1]).
> > > > > 
> > > > > This makes sure that link is configured after at least 100ms from
> > > > > beginning of probe() callback (shortly after the reset pin function
> > > > > configuration switch through pinctrl subsytem).
> > 
> > I am a bit lost, what's the connection between the probe() callback
> > and the reset pin function configuration ?
> 
> So currently u-boot configures the reset pin as a GPIO set to high. The
> espressobin devicetree defines a default pinctrl to configure this pin
> as a PCIe reset function.
> 
> As you can see in drivers/base/dd.c, driver_probe_device() calls
> really_probe() which first calls pinctrl_bind_pins() then shortly after
> drv->probe() callback. The pinctrl_bind_pins() function applies the
> default state. So here, just before drv->probe() gets called our reset
> pin goes from GPIO function to PCIe reset one making it going low for a
> short time during this transition.
> 
> Because the pin goes low then gets back to high, PERST# signal is
> asserted then deasserted and device enters fundamental reset process
> just before drv->probe() is called. So in order to reduce the waiting
> time to a minimum I sample jiffies at the very beginning of the probe
> function, which is the closer spot from where PERST# is deasserted.
> 
> To sum up:
> 
> driver_probe_device() {
> 	...
> 	really_probe() {
> 		...
> 		pinctrl_bind_pins(); /* Here PERST# is asserted because pin configuration changes */
> 		...
> 		drv->probe();

Ah, I see.  Hmmm.  This definitely warrants a comment in
advk_pcie_probe() about the connection.

I appreciate that ab78029ecc34 ("drivers/pinctrl: grab default handles
from device core") saves some boilerplate from drivers, but ... at the
same time, it makes for some non-obvious implicit connections like
this.  I'm not sure whether having the boilerplate or the comment is
worse.  But I'm pretty sure the "no boilerplate, no comment" option is
the worst of the three :)

> > > > > [1] "PCI Express Base Specification", REV. 2.1
> > > > >         PCI Express, March 4 2009, 6.6.1 Conventional Reset

> > > > > +/* Endpoint can take up to 100ms to be ready after a reset */
> > > > > +#define ENDPOINT_RST_MS			100

> So doing that I do a msleep() of around 75-80ms instead of 100ms. So,
> yes, are 20ms enough to justify that, or should we just go with a plain
> msleep(100) to improve legibility.

I vote for 100ms so it's easily tied to the spec.  We *should* have a
PCI core #define for that to make it even easier.

PCI_PM_D3COLD_WAIT might be close, but we might need some cleanup in
this area.  There are a whole bunch of "msleep(100)" calls in
drivers/pci that probably should use the same #define.  Somebody
should look through pci_af_flr(), pcie_flr(), pci_pm_reset(),
pcie_wait_for_link(), to see if we can use a common constant.

Bjorn