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

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

[Remi Pommarel]

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).

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

Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
 drivers/pci/controller/pci-aardvark.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index a30ae7cf8e7e..70a1023d0ef1 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -177,6 +177,9 @@
 
 #define PIO_TIMEOUT_MS			1
 
+/* Endpoint can take up to 100ms to be ready after a reset */
+#define ENDPOINT_RST_MS			100
+
 #define LINK_WAIT_MAX_RETRIES		10
 #define LINK_WAIT_USLEEP_MIN		90000
 #define LINK_WAIT_USLEEP_MAX		100000
@@ -242,8 +245,10 @@ static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
 	return -ETIMEDOUT;
 }
 
-static void advk_pcie_setup_hw(struct advk_pcie *pcie)
+static void
+advk_pcie_setup_hw(struct advk_pcie *pcie, unsigned long ep_rdy_time)
 {
+	unsigned long now;
 	u32 reg;
 
 	/* Set to Direct mode */
@@ -327,9 +332,12 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	reg |= PIO_CTRL_ADDR_WIN_DISABLE;
 	advk_writel(pcie, reg, PIO_CTRL);
 
-	/* Start link training */
+	/* Wait for endpoint to exit reset state and start link training */
 	reg = advk_readl(pcie, PCIE_CORE_LINK_CTRL_STAT_REG);
 	reg |= PCIE_CORE_LINK_TRAINING;
+	now = jiffies;
+	if (time_before(now, ep_rdy_time))
+		msleep(jiffies_to_msecs(ep_rdy_time - now));
 	advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
 
 	advk_pcie_wait_for_link(pcie);
@@ -993,8 +1001,11 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	struct advk_pcie *pcie;
 	struct resource *res;
 	struct pci_host_bridge *bridge;
+	unsigned long ep_rdy_time;
 	int ret, irq;
 
+	ep_rdy_time = jiffies + msecs_to_jiffies(ENDPOINT_RST_MS);
+
 	bridge = devm_pci_alloc_host_bridge(dev, sizeof(struct advk_pcie));
 	if (!bridge)
 		return -ENOMEM;
@@ -1022,7 +1033,7 @@ static int advk_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	advk_pcie_setup_hw(pcie);
+	advk_pcie_setup_hw(pcie, ep_rdy_time);
 
 	advk_sw_pci_bridge_init(pcie);
 
-- 
2.20.1

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

[Lorenzo Pieralisi]

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).
> 
> [1] "PCI Express Base Specification", REV. 2.1
>         PCI Express, March 4 2009, 6.6.1 Conventional Reset
> 
> Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> ---
>  drivers/pci/controller/pci-aardvark.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index a30ae7cf8e7e..70a1023d0ef1 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -177,6 +177,9 @@
>  
>  #define PIO_TIMEOUT_MS			1
>  
> +/* Endpoint can take up to 100ms to be ready after a reset */
> +#define ENDPOINT_RST_MS			100
> +
>  #define LINK_WAIT_MAX_RETRIES		10
>  #define LINK_WAIT_USLEEP_MIN		90000
>  #define LINK_WAIT_USLEEP_MAX		100000
> @@ -242,8 +245,10 @@ static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
>  	return -ETIMEDOUT;
>  }
>  
> -static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> +static void
> +advk_pcie_setup_hw(struct advk_pcie *pcie, unsigned long ep_rdy_time)

Nit: I prefer the prototype to be in one line, I wrap it for you.

I am wondering why you need to pass in ep_rdy_time parameter when you
can easily compute it in the function itself.

Regardless, I need Thomas's ACK to proceed, I can make these changes
myself.

Thanks,
Lorenzo

>  {
> +	unsigned long now;
>  	u32 reg;
>  
>  	/* Set to Direct mode */
> @@ -327,9 +332,12 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  	reg |= PIO_CTRL_ADDR_WIN_DISABLE;
>  	advk_writel(pcie, reg, PIO_CTRL);
>  
> -	/* Start link training */
> +	/* Wait for endpoint to exit reset state and start link training */
>  	reg = advk_readl(pcie, PCIE_CORE_LINK_CTRL_STAT_REG);
>  	reg |= PCIE_CORE_LINK_TRAINING;
> +	now = jiffies;
> +	if (time_before(now, ep_rdy_time))
> +		msleep(jiffies_to_msecs(ep_rdy_time - now));
>  	advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
>  
>  	advk_pcie_wait_for_link(pcie);
> @@ -993,8 +1001,11 @@ static int advk_pcie_probe(struct platform_device *pdev)
>  	struct advk_pcie *pcie;
>  	struct resource *res;
>  	struct pci_host_bridge *bridge;
> +	unsigned long ep_rdy_time;
>  	int ret, irq;
>  
> +	ep_rdy_time = jiffies + msecs_to_jiffies(ENDPOINT_RST_MS);
> +
>  	bridge = devm_pci_alloc_host_bridge(dev, sizeof(struct advk_pcie));
>  	if (!bridge)
>  		return -ENOMEM;
> @@ -1022,7 +1033,7 @@ static int advk_pcie_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	advk_pcie_setup_hw(pcie);
> +	advk_pcie_setup_hw(pcie, ep_rdy_time);
>  
>  	advk_sw_pci_bridge_init(pcie);
>  
> -- 
> 2.20.1
> 

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

[Remi Pommarel]

Hi,

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).
> > 
> > [1] "PCI Express Base Specification", REV. 2.1
> >         PCI Express, March 4 2009, 6.6.1 Conventional Reset
> > 
> > Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index a30ae7cf8e7e..70a1023d0ef1 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -177,6 +177,9 @@
> >  
> >  #define PIO_TIMEOUT_MS			1
> >  
> > +/* Endpoint can take up to 100ms to be ready after a reset */
> > +#define ENDPOINT_RST_MS			100
> > +
> >  #define LINK_WAIT_MAX_RETRIES		10
> >  #define LINK_WAIT_USLEEP_MIN		90000
> >  #define LINK_WAIT_USLEEP_MAX		100000
> > @@ -242,8 +245,10 @@ static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
> >  	return -ETIMEDOUT;
> >  }
> >  
> > -static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> > +static void
> > +advk_pcie_setup_hw(struct advk_pcie *pcie, unsigned long ep_rdy_time)
> 
> Nit: I prefer the prototype to be in one line, I wrap it for you.
> 
> I am wondering why you need to pass in ep_rdy_time parameter when you
> can easily compute it in the function itself.
> 

The only reason for that is because the sooner I get the jiffies the
lower the delay has to be. I was trying to reduce the impact of this
delay to a minimum, but maybe the improvement is not worth it.

-- 
Remi

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

[Lorenzo Pieralisi]

On Wed, Apr 24, 2019 at 12:29:18AM +0200, Remi Pommarel wrote:
> Hi,
> 
> 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 ?

Please elaborate.

> > > 
> > > [1] "PCI Express Base Specification", REV. 2.1
> > >         PCI Express, March 4 2009, 6.6.1 Conventional Reset
> > > 
> > > Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> > > ---
> > >  drivers/pci/controller/pci-aardvark.c | 17 ++++++++++++++---
> > >  1 file changed, 14 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > index a30ae7cf8e7e..70a1023d0ef1 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -177,6 +177,9 @@
> > >  
> > >  #define PIO_TIMEOUT_MS			1
> > >  
> > > +/* Endpoint can take up to 100ms to be ready after a reset */
> > > +#define ENDPOINT_RST_MS			100
> > > +
> > >  #define LINK_WAIT_MAX_RETRIES		10
> > >  #define LINK_WAIT_USLEEP_MIN		90000
> > >  #define LINK_WAIT_USLEEP_MAX		100000
> > > @@ -242,8 +245,10 @@ static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
> > >  	return -ETIMEDOUT;
> > >  }
> > >  
> > > -static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> > > +static void
> > > +advk_pcie_setup_hw(struct advk_pcie *pcie, unsigned long ep_rdy_time)
> > 
> > Nit: I prefer the prototype to be in one line, I wrap it for you.
> > 
> > I am wondering why you need to pass in ep_rdy_time parameter when you
> > can easily compute it in the function itself.
> > 
> 
> The only reason for that is because the sooner I get the jiffies the
> lower the delay has to be. I was trying to reduce the impact of this
> delay to a minimum, but maybe the improvement is not worth it.

That should just be (roughly) some microseconds unless there is
something I am missing. Try to measure it :)

More importantly, I would ask you to elaborate a bit more
about the logic behind this patch, see above because I need
to understand the logic behind pinctrl reset and the probe()
hook execution ordering.

Thanks,
Lorenzo

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

[Remi Pommarel]

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:
> > Hi,
> > 
> > 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 ?
> 
> Please elaborate.
> 

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();
		...
	}
	...
}

> > > > 
> > > > [1] "PCI Express Base Specification", REV. 2.1
> > > >         PCI Express, March 4 2009, 6.6.1 Conventional Reset
> > > > 
> > > > Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> > > > ---
> > > >  drivers/pci/controller/pci-aardvark.c | 17 ++++++++++++++---
> > > >  1 file changed, 14 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > index a30ae7cf8e7e..70a1023d0ef1 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -177,6 +177,9 @@
> > > >  
> > > >  #define PIO_TIMEOUT_MS			1
> > > >  
> > > > +/* Endpoint can take up to 100ms to be ready after a reset */
> > > > +#define ENDPOINT_RST_MS			100
> > > > +
> > > >  #define LINK_WAIT_MAX_RETRIES		10
> > > >  #define LINK_WAIT_USLEEP_MIN		90000
> > > >  #define LINK_WAIT_USLEEP_MAX		100000
> > > > @@ -242,8 +245,10 @@ static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
> > > >  	return -ETIMEDOUT;
> > > >  }
> > > >  
> > > > -static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> > > > +static void
> > > > +advk_pcie_setup_hw(struct advk_pcie *pcie, unsigned long ep_rdy_time)
> > > 
> > > Nit: I prefer the prototype to be in one line, I wrap it for you.
> > > 
> > > I am wondering why you need to pass in ep_rdy_time parameter when you
> > > can easily compute it in the function itself.
> > > 
> > 
> > The only reason for that is because the sooner I get the jiffies the
> > lower the delay has to be. I was trying to reduce the impact of this
> > delay to a minimum, but maybe the improvement is not worth it.
> 
> That should just be (roughly) some microseconds unless there is
> something I am missing. Try to measure it :)

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.

-- 
Remi

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

[Bjorn Helgaas]

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

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

[Lorenzo Pieralisi]

On Fri, Apr 26, 2019 at 11:10:50AM -0500, Bjorn Helgaas wrote:
> 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 :)

Yes, it is horrible. Can't this be managed explicitly through
the reset core code (drivers/reset/) ? I really do not like
this implicit reset going on behind the scenes, I will have
a look to understand how other controllers handle this.

Lorenzo

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

[Bjorn Helgaas]

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).
> 
> [1] "PCI Express Base Specification", REV. 2.1
>         PCI Express, March 4 2009, 6.6.1 Conventional Reset

It would be nice to use a current reference, e.g., PCIe r4.0, sec
6.6.1.

> Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> ---
>  drivers/pci/controller/pci-aardvark.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index a30ae7cf8e7e..70a1023d0ef1 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -177,6 +177,9 @@
>  
>  #define PIO_TIMEOUT_MS			1
>  
> +/* Endpoint can take up to 100ms to be ready after a reset */
> +#define ENDPOINT_RST_MS			100
> +
>  #define LINK_WAIT_MAX_RETRIES		10
>  #define LINK_WAIT_USLEEP_MIN		90000
>  #define LINK_WAIT_USLEEP_MAX		100000
> @@ -242,8 +245,10 @@ static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
>  	return -ETIMEDOUT;
>  }
>  
> -static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> +static void
> +advk_pcie_setup_hw(struct advk_pcie *pcie, unsigned long ep_rdy_time)
>  {
> +	unsigned long now;
>  	u32 reg;
>  
>  	/* Set to Direct mode */
> @@ -327,9 +332,12 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  	reg |= PIO_CTRL_ADDR_WIN_DISABLE;
>  	advk_writel(pcie, reg, PIO_CTRL);
>  
> -	/* Start link training */
> +	/* Wait for endpoint to exit reset state and start link training */
>  	reg = advk_readl(pcie, PCIE_CORE_LINK_CTRL_STAT_REG);
>  	reg |= PCIE_CORE_LINK_TRAINING;
> +	now = jiffies;
> +	if (time_before(now, ep_rdy_time))
> +		msleep(jiffies_to_msecs(ep_rdy_time - now));
>  	advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
>  
>  	advk_pcie_wait_for_link(pcie);
> @@ -993,8 +1001,11 @@ static int advk_pcie_probe(struct platform_device *pdev)
>  	struct advk_pcie *pcie;
>  	struct resource *res;
>  	struct pci_host_bridge *bridge;
> +	unsigned long ep_rdy_time;
>  	int ret, irq;
>  
> +	ep_rdy_time = jiffies + msecs_to_jiffies(ENDPOINT_RST_MS);

I think this is partly what Lorenzo is getting at, so at the risk of
being repetitious, there should be some connection between the point
where PERST# is deasserted and the point where you sample "jiffies".
Ideally you would sample "jiffies" immediately after the function call
that deasserts PERST#.

I see that you mention this connection in the commit log, but I don't
see a way to easily deduce it from the code.  This looks like the very
first executable statement in the advk driver, so presumably there's
some implicit connection and ordering with the pinctrl driver.

>  	bridge = devm_pci_alloc_host_bridge(dev, sizeof(struct advk_pcie));
>  	if (!bridge)
>  		return -ENOMEM;
> @@ -1022,7 +1033,7 @@ static int advk_pcie_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	advk_pcie_setup_hw(pcie);
> +	advk_pcie_setup_hw(pcie, ep_rdy_time);
>  
>  	advk_sw_pci_bridge_init(pcie);
>  
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel