[PATCH v3] PCI: imx6: Fix PERST# start-up sequence

[PATCH v3] PCI: imx6: Fix PERST# start-up sequence

[Francesco Dolcini]

According to the PCIe standard the PERST# signal (reset-gpio in
fsl,imx* compatible dts) should be kept asserted for at least 100 usec
before the PCIe refclock is stable, should be kept asserted for at
least 100 msec after the power rails are stable and the host should wait
at least 100 msec after it is de-asserted before accessing the
configuration space of any attached device.

From PCIe CEM r2.0, sec 2.6.2

  T-PVPERL: Power stable to PERST# inactive - 100 msec
  T-PERST-CLK: REFCLK stable before PERST# inactive - 100 usec.

From PCIe r5.0, sec 6.6.1

  With a Downstream Port that does not support Link speeds greater than
  5.0 GT/s, software must wait a minimum of 100 ms before sending a
  Configuration Request to the device immediately below that Port.

Failure to do so could prevent PCIe devices to be working correctly,
and this was experienced with real devices.

Move reset assert to imx6_pcie_assert_core_reset(), this way we ensure
that PERST# is asserted before enabling any clock, move de-assert to the
end of imx6_pcie_deassert_core_reset() after the clock is enabled and
deemed stable and add a new delay of 100 msec just afterward.

Link: https://lore.kernel.org/all/20220211152550.286821-1-francesco.dolcini@toradex.com
Fixes: bb38919ec56e ("PCI: imx6: Add support for i.MX6 PCIe controller")
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
Acked-by: Richard Zhu <hongxing.zhu@nxp.com>

---
v3: Add Acked-by: Richard Zhu
v2: Add complete reference to the PCIe standards, s/PCI-E/PCIe/g
---
 drivers/pci/controller/dwc/pci-imx6.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 6619e3caffe2..7a285fb0f619 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -408,6 +408,11 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 			dev_err(dev, "failed to disable vpcie regulator: %d\n",
 				ret);
 	}
+
+	/* Some boards don't have PCIe reset GPIO. */
+	if (gpio_is_valid(imx6_pcie->reset_gpio))
+		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
+					imx6_pcie->gpio_active_high);
 }
 
 static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
@@ -540,15 +545,6 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	/* allow the clocks to stabilize */
 	usleep_range(200, 500);
 
-	/* Some boards don't have PCIe reset GPIO. */
-	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
-		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
-					imx6_pcie->gpio_active_high);
-		msleep(100);
-		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
-					!imx6_pcie->gpio_active_high);
-	}
-
 	switch (imx6_pcie->drvdata->variant) {
 	case IMX8MQ:
 		reset_control_deassert(imx6_pcie->pciephy_reset);
@@ -595,6 +591,15 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 		break;
 	}
 
+	/* Some boards don't have PCIe reset GPIO. */
+	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
+		msleep(100);
+		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
+					!imx6_pcie->gpio_active_high);
+		/* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
+		msleep(100);
+	}
+
 	return;
 
 err_ref_clk:
-- 
2.25.1

Re: [PATCH v3] PCI: imx6: Fix PERST# start-up sequence

[Lucas Stach]

Am Montag, dem 04.04.2022 um 10:15 +0200 schrieb Francesco Dolcini:
> According to the PCIe standard the PERST# signal (reset-gpio in
> fsl,imx* compatible dts) should be kept asserted for at least 100 usec
> before the PCIe refclock is stable, should be kept asserted for at
> least 100 msec after the power rails are stable and the host should wait
> at least 100 msec after it is de-asserted before accessing the
> configuration space of any attached device.
> 
> From PCIe CEM r2.0, sec 2.6.2
> 
>   T-PVPERL: Power stable to PERST# inactive - 100 msec
>   T-PERST-CLK: REFCLK stable before PERST# inactive - 100 usec.
> 
> From PCIe r5.0, sec 6.6.1
> 
>   With a Downstream Port that does not support Link speeds greater than
>   5.0 GT/s, software must wait a minimum of 100 ms before sending a
>   Configuration Request to the device immediately below that Port.
> 
> Failure to do so could prevent PCIe devices to be working correctly,
> and this was experienced with real devices.
> 
> Move reset assert to imx6_pcie_assert_core_reset(), this way we ensure
> that PERST# is asserted before enabling any clock, move de-assert to the
> end of imx6_pcie_deassert_core_reset() after the clock is enabled and
> deemed stable and add a new delay of 100 msec just afterward.
> 
> Link: https://lore.kernel.org/all/20220211152550.286821-1-francesco.dolcini@toradex.com
> Fixes: bb38919ec56e ("PCI: imx6: Add support for i.MX6 PCIe controller")
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> Acked-by: Richard Zhu <hongxing.zhu@nxp.com>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> 
> ---
> v3: Add Acked-by: Richard Zhu
> v2: Add complete reference to the PCIe standards, s/PCI-E/PCIe/g
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 6619e3caffe2..7a285fb0f619 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -408,6 +408,11 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>  			dev_err(dev, "failed to disable vpcie regulator: %d\n",
>  				ret);
>  	}
> +
> +	/* Some boards don't have PCIe reset GPIO. */
> +	if (gpio_is_valid(imx6_pcie->reset_gpio))
> +		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> +					imx6_pcie->gpio_active_high);
>  }
>  
>  static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
> @@ -540,15 +545,6 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  	/* allow the clocks to stabilize */
>  	usleep_range(200, 500);
>  
> -	/* Some boards don't have PCIe reset GPIO. */
> -	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> -		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> -					imx6_pcie->gpio_active_high);
> -		msleep(100);
> -		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> -					!imx6_pcie->gpio_active_high);
> -	}
> -
>  	switch (imx6_pcie->drvdata->variant) {
>  	case IMX8MQ:
>  		reset_control_deassert(imx6_pcie->pciephy_reset);
> @@ -595,6 +591,15 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  		break;
>  	}
>  
> +	/* Some boards don't have PCIe reset GPIO. */
> +	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> +		msleep(100);
> +		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> +					!imx6_pcie->gpio_active_high);
> +		/* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
> +		msleep(100);
> +	}
> +
>  	return;
>  
>  err_ref_clk:

Re: [PATCH v3] PCI: imx6: Fix PERST# start-up sequence

[Lorenzo Pieralisi]

[CC'ed Pali, who is working on PERST consolidation]

On Mon, Apr 04, 2022 at 10:15:09AM +0200, Francesco Dolcini wrote:
> According to the PCIe standard the PERST# signal (reset-gpio in
> fsl,imx* compatible dts) should be kept asserted for at least 100 usec
> before the PCIe refclock is stable, should be kept asserted for at
> least 100 msec after the power rails are stable and the host should wait
> at least 100 msec after it is de-asserted before accessing the
> configuration space of any attached device.
> 
> From PCIe CEM r2.0, sec 2.6.2
> 
>   T-PVPERL: Power stable to PERST# inactive - 100 msec
>   T-PERST-CLK: REFCLK stable before PERST# inactive - 100 usec.
> 
> From PCIe r5.0, sec 6.6.1
> 
>   With a Downstream Port that does not support Link speeds greater than
>   5.0 GT/s, software must wait a minimum of 100 ms before sending a
>   Configuration Request to the device immediately below that Port.
> 
> Failure to do so could prevent PCIe devices to be working correctly,
> and this was experienced with real devices.
> 
> Move reset assert to imx6_pcie_assert_core_reset(), this way we ensure
> that PERST# is asserted before enabling any clock, move de-assert to the
> end of imx6_pcie_deassert_core_reset() after the clock is enabled and
> deemed stable and add a new delay of 100 msec just afterward.
> 
> Link: https://lore.kernel.org/all/20220211152550.286821-1-francesco.dolcini@toradex.com
> Fixes: bb38919ec56e ("PCI: imx6: Add support for i.MX6 PCIe controller")
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
> 
> ---
> v3: Add Acked-by: Richard Zhu
> v2: Add complete reference to the PCIe standards, s/PCI-E/PCIe/g
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 6619e3caffe2..7a285fb0f619 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -408,6 +408,11 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>  			dev_err(dev, "failed to disable vpcie regulator: %d\n",
>  				ret);
>  	}
> +
> +	/* Some boards don't have PCIe reset GPIO. */
> +	if (gpio_is_valid(imx6_pcie->reset_gpio))
> +		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> +					imx6_pcie->gpio_active_high);
>  }
>  
>  static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
> @@ -540,15 +545,6 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  	/* allow the clocks to stabilize */
>  	usleep_range(200, 500);
>  
> -	/* Some boards don't have PCIe reset GPIO. */
> -	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> -		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> -					imx6_pcie->gpio_active_high);
> -		msleep(100);
> -		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> -					!imx6_pcie->gpio_active_high);
> -	}
> -
>  	switch (imx6_pcie->drvdata->variant) {
>  	case IMX8MQ:
>  		reset_control_deassert(imx6_pcie->pciephy_reset);
> @@ -595,6 +591,15 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  		break;
>  	}
>  
> +	/* Some boards don't have PCIe reset GPIO. */
> +	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> +		msleep(100);
> +		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> +					!imx6_pcie->gpio_active_high);
> +		/* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
> +		msleep(100);
> +	}
> +
>  	return;
>  
>  err_ref_clk:
> -- 
> 2.25.1
> 

Re: [PATCH v3] PCI: imx6: Fix PERST# start-up sequence

[Francesco Dolcini]

Hello Lorenzo,

On Mon, Apr 11, 2022 at 05:50:41PM +0100, Lorenzo Pieralisi wrote:
> [CC'ed Pali, who is working on PERST consolidation]
> 
> On Mon, Apr 04, 2022 at 10:15:09AM +0200, Francesco Dolcini wrote:
> > Failure to do so could prevent PCIe devices to be working correctly,
> > and this was experienced with real devices.

Just a gentle ping, any concern on the patch?

Francesco

Re: [PATCH v3] PCI: imx6: Fix PERST# start-up sequence

[Lorenzo Pieralisi]

On Mon, May 09, 2022 at 04:09:19PM +0200, Francesco Dolcini wrote:
> Hello Lorenzo,
> 
> On Mon, Apr 11, 2022 at 05:50:41PM +0100, Lorenzo Pieralisi wrote:
> > [CC'ed Pali, who is working on PERST consolidation]
> > 
> > On Mon, Apr 04, 2022 at 10:15:09AM +0200, Francesco Dolcini wrote:
> > > Failure to do so could prevent PCIe devices to be working correctly,
> > > and this was experienced with real devices.
> 
> Just a gentle ping, any concern on the patch?

I was waiting to see if Pali has any comments on it, given that he
is working on consolidating PERST management.

Lorenzo

Re: [PATCH v3] PCI: imx6: Fix PERST# start-up sequence

[Francesco Dolcini]

On Tue, May 10, 2022 at 06:49:12PM +0100, Lorenzo Pieralisi wrote:
> On Mon, May 09, 2022 at 04:09:19PM +0200, Francesco Dolcini wrote:
> > Hello Lorenzo,
> > 
> > On Mon, Apr 11, 2022 at 05:50:41PM +0100, Lorenzo Pieralisi wrote:
> > > [CC'ed Pali, who is working on PERST consolidation]
> > > 
> > > On Mon, Apr 04, 2022 at 10:15:09AM +0200, Francesco Dolcini wrote:
> > > > Failure to do so could prevent PCIe devices to be working correctly,
> > > > and this was experienced with real devices.
> > 
> > Just a gentle ping, any concern on the patch?
> 
> I was waiting to see if Pali has any comments on it, given that he
> is working on consolidating PERST management.

Hello Pali,
any concern on merging this patch?

Thanks a lot
Francesco

Re: [PATCH v3] PCI: imx6: Fix PERST# start-up sequence

[Pali Rohár]

On Wednesday 11 May 2022 09:13:50 Francesco Dolcini wrote:
> On Tue, May 10, 2022 at 06:49:12PM +0100, Lorenzo Pieralisi wrote:
> > On Mon, May 09, 2022 at 04:09:19PM +0200, Francesco Dolcini wrote:
> > > Hello Lorenzo,
> > > 
> > > On Mon, Apr 11, 2022 at 05:50:41PM +0100, Lorenzo Pieralisi wrote:
> > > > [CC'ed Pali, who is working on PERST consolidation]
> > > > 
> > > > On Mon, Apr 04, 2022 at 10:15:09AM +0200, Francesco Dolcini wrote:
> > > > > Failure to do so could prevent PCIe devices to be working correctly,
> > > > > and this was experienced with real devices.
> > > 
> > > Just a gentle ping, any concern on the patch?
> > 
> > I was waiting to see if Pali has any comments on it, given that he
> > is working on consolidating PERST management.
> 
> Hello Pali,
> any concern on merging this patch?
> 
> Thanks a lot
> Francesco

Hello Francesco! I have no objections for your patch, it is fine to merge it.

Btw, I lost interest in consolidating PERST# management as now I know that it would not be accepted.

Re: [PATCH v3] PCI: imx6: Fix PERST# start-up sequence

[Lorenzo Pieralisi]

On Mon, 4 Apr 2022 10:15:09 +0200, Francesco Dolcini wrote:
> According to the PCIe standard the PERST# signal (reset-gpio in
> fsl,imx* compatible dts) should be kept asserted for at least 100 usec
> before the PCIe refclock is stable, should be kept asserted for at
> least 100 msec after the power rails are stable and the host should wait
> at least 100 msec after it is de-asserted before accessing the
> configuration space of any attached device.
> 
> [...]

Applied to pci/imx6, thanks!

[1/1] PCI: imx6: Fix PERST# start-up sequence
      https://git.kernel.org/lpieralisi/pci/c/a6809941c1

Thanks,
Lorenzo