linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: Remi Pommarel <repk@triplefau.lt>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Miquel Raynal <miquel.raynal@bootlin.com>
Subject: Re: [PATCH] pci: aardvark: Wait for endpoint to be ready before training link
Date: Fri, 26 Apr 2019 11:10:50 -0500	[thread overview]
Message-ID: <20190426161050.GA189964@google.com> (raw)
In-Reply-To: <20190425210826.GQ2754@voidbox.localdomain>

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

  reply	other threads:[~2019-04-26 16:10 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-13 21:37 [PATCH] pci: aardvark: Wait for endpoint to be ready before training link Remi Pommarel
2019-04-23 16:32 ` Lorenzo Pieralisi
2019-04-23 22:29   ` Remi Pommarel
2019-04-24 16:50     ` Lorenzo Pieralisi
2019-04-25 21:08       ` Remi Pommarel
2019-04-26 16:10         ` Bjorn Helgaas [this message]
2019-04-26 17:16           ` Lorenzo Pieralisi
2019-04-25 16:30 ` Bjorn Helgaas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190426161050.GA189964@google.com \
    --to=helgaas@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=miquel.raynal@bootlin.com \
    --cc=repk@triplefau.lt \
    --cc=thomas.petazzoni@bootlin.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).