[PATCH v1 0/3] PCI: imx6: reset link after suspend/resume

[PATCH v1 0/3] PCI: imx6: reset link after suspend/resume

[Stefan Eichenberger]

On the i.MX6Quad (not QuadPlus), the PCIe link does not work after a
suspend/resume cycle. Worse, the PCIe memory mapped I/O isn't accessible
at all, so the system freezes when a PCIe driver tries to access its I/O
space. The only way to get resume working again is to reset the PCIe
link, similar to what is done on devices that support suspend/resume.
Through trial and error, we found that something about the PCIe
reference clock does not work as expected after a resume. We could not
figure out if it is disabled (even though the registers still say it is
enabled), or if it is somehow unstable or has some hiccups. With the
workaround introduced in this patch series, we were able to fully resume
a Compex WLE900VX (ath10k) miniPCIe Wifi module and an Intel AX200 M.2
Wifi module. If there is a better way or other ideas on how to fix this
problem, please let us know. We are aware that resetting the link should
not be necessary, but we could not find a better solution. More
interestingly, even the SoCs that support suspend/resume according to
the i.MX erratas seem to reset the link on resume in
imx6_pcie_host_init, so we hope this might be a valid workaround.

Stefan Eichenberger (3):
  PCI: imx6: Add a function to deassert the reset gpio
  PCI: imx6: move the wait for clock stabilization to enable ref clk
  PCI: imx6: reset link on resume

 drivers/pci/controller/dwc/pci-imx6.c | 69 +++++++++++++++++++++++----
 1 file changed, 59 insertions(+), 10 deletions(-)

-- 
2.43.0

[PATCH v1 1/3] PCI: imx6: Add a function to deassert the reset gpio

[Stefan Eichenberger]

From: Stefan Eichenberger <stefan.eichenberger@toradex.com>

To avoid code duplication, move the code to disable the reset GPIO to a
separate function.

Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 964d67756eb2b..fda704d82431f 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -722,6 +722,17 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 	gpiod_set_value_cansleep(imx6_pcie->reset_gpiod, 1);
 }
 
+static void imx6_pcie_deassert_reset_gpio(struct imx6_pcie *imx6_pcie)
+{
+	/* Some boards don't have PCIe reset GPIO. */
+	if (imx6_pcie->reset_gpiod) {
+		msleep(100);
+		gpiod_set_value_cansleep(imx6_pcie->reset_gpiod, 0);
+		/* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
+		msleep(100);
+	}
+}
+
 static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
@@ -766,13 +777,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 		break;
 	}
 
-	/* Some boards don't have PCIe reset GPIO. */
-	if (imx6_pcie->reset_gpiod) {
-		msleep(100);
-		gpiod_set_value_cansleep(imx6_pcie->reset_gpiod, 0);
-		/* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
-		msleep(100);
-	}
+	imx6_pcie_deassert_reset_gpio(imx6_pcie);
 
 	return 0;
 }
-- 
2.43.0

[PATCH v1 2/3] PCI: imx6: move the wait for clock stabilization to enable ref clk

[Stefan Eichenberger]

From: Stefan Eichenberger <stefan.eichenberger@toradex.com>

After enabling the ref clock, we should wait for the clock to stabilize.
To eliminate the need for code duplication in the future, move the
usleep to the enable_ref_clk function.

Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index fda704d82431f..f17561791e35a 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -632,6 +632,9 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
 		break;
 	}
 
+	/* allow the clocks to stabilize */
+	usleep_range(200, 500);
+
 	return ret;
 }
 
@@ -672,8 +675,6 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
 		goto err_ref_clk;
 	}
 
-	/* allow the clocks to stabilize */
-	usleep_range(200, 500);
 	return 0;
 
 err_ref_clk:
-- 
2.43.0

[PATCH v1 3/3] PCI: imx6: reset link on resume

[Stefan Eichenberger]

From: Stefan Eichenberger <stefan.eichenberger@toradex.com>

According to the https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf errata,
the i.MX6Q PCIe controller does not support suspend/resume. So suspend
and resume was omitted. However, this does not seem to work because it
looks like the PCIe link is still expecting a reset. If we do not reset
the link, we end up with a frozen system after resume. The last message
we see is:
ath10k_pci 0000:01:00.0: Unable to change power state from D3hot to D0,
device inaccessible

Besides resetting the link, we also need to enable msi again, otherwise
DMA access will not work and we can still end up with a frozen system.
With these changes we can suspend and resume the system properly with a
PCIe device attached. This was tested with a Compex WLE900VX miniPCIe
Wifi module.

Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 45 ++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index f17561791e35a..751243f4c519e 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1213,14 +1213,57 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
 	return 0;
 }
 
+static int imx6_pcie_reset_link(struct imx6_pcie *imx6_pcie)
+{
+	int ret;
+
+	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+			   IMX6Q_GPR1_PCIE_TEST_PD, 1 << 18);
+	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+			   IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
+
+	/* Reset the PCIe device */
+	gpiod_set_value_cansleep(imx6_pcie->reset_gpiod, 1);
+
+	ret = imx6_pcie_enable_ref_clk(imx6_pcie);
+	if (ret) {
+		dev_err(imx6_pcie->pci->dev, "unable to enable pcie ref clock\n");
+		return ret;
+	}
+
+	imx6_pcie_deassert_reset_gpio(imx6_pcie);
+
+	/*
+	 * Setup the root complex again and enable msi. Without this PCIe will
+	 * not work in msi mode and drivers will crash if they try to access
+	 * the device memory area
+	 */
+	dw_pcie_setup_rc(&imx6_pcie->pci->pp);
+	if (pci_msi_enabled()) {
+		u32 val;
+		u8 offset = dw_pcie_find_capability(imx6_pcie->pci, PCI_CAP_ID_MSI);
+
+		val = dw_pcie_readw_dbi(imx6_pcie->pci, offset + PCI_MSI_FLAGS);
+		val |= PCI_MSI_FLAGS_ENABLE;
+		dw_pcie_writew_dbi(imx6_pcie->pci, offset + PCI_MSI_FLAGS, val);
+	}
+
+	return 0;
+}
+
 static int imx6_pcie_resume_noirq(struct device *dev)
 {
 	int ret;
 	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
 	struct dw_pcie_rp *pp = &imx6_pcie->pci->pp;
 
+	/*
+	 * Even though the i.MX6Q does not support suspend/resume, we need to
+	 * reset the link after resume or the memory mapped PCIe I/O space will
+	 * be inaccessible. This will cause the system to freeze.
+	 */
 	if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_SUPPORTS_SUSPEND))
-		return 0;
+		return imx6_pcie_reset_link(imx6_pcie);
 
 	ret = imx6_pcie_host_init(pp);
 	if (ret)
-- 
2.43.0

Re: [PATCH v1 0/3] PCI: imx6: reset link after suspend/resume

[Frank Li]

On Mon, Aug 19, 2024 at 11:03:16AM +0200, Stefan Eichenberger wrote:
> On the i.MX6Quad (not QuadPlus), the PCIe link does not work after a
> suspend/resume cycle. Worse, the PCIe memory mapped I/O isn't accessible
> at all, so the system freezes when a PCIe driver tries to access its I/O
> space. The only way to get resume working again is to reset the PCIe
> link, similar to what is done on devices that support suspend/resume.
> Through trial and error, we found that something about the PCIe
> reference clock does not work as expected after a resume. We could not
> figure out if it is disabled (even though the registers still say it is
> enabled), or if it is somehow unstable or has some hiccups. With the
> workaround introduced in this patch series, we were able to fully resume
> a Compex WLE900VX (ath10k) miniPCIe Wifi module and an Intel AX200 M.2
> Wifi module. If there is a better way or other ideas on how to fix this
> problem, please let us know. We are aware that resetting the link should
> not be necessary, but we could not find a better solution. More
> interestingly, even the SoCs that support suspend/resume according to
> the i.MX erratas seem to reset the link on resume in
> imx6_pcie_host_init, so we hope this might be a valid workaround.
>
> Stefan Eichenberger (3):
>   PCI: imx6: Add a function to deassert the reset gpio
>   PCI: imx6: move the wait for clock stabilization to enable ref clk
>   PCI: imx6: reset link on resume

Thanks you for your patch, but it may have conflict with
https://lore.kernel.org/linux-pci/Zr4XG6r+HnbIlu8S@lizhi-Precision-Tower-5810/T/#t


>
>  drivers/pci/controller/dwc/pci-imx6.c | 69 +++++++++++++++++++++++----
>  1 file changed, 59 insertions(+), 10 deletions(-)
>
> --
> 2.43.0
>

Re: [PATCH v1 3/3] PCI: imx6: reset link on resume

[Frank Li]

On Mon, Aug 19, 2024 at 11:03:19AM +0200, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
>
> According to the https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf errata,

Can you show errata number here?

> the i.MX6Q PCIe controller does not support suspend/resume. So suspend
> and resume was omitted. However, this does not seem to work because it
> looks like the PCIe link is still expecting a reset. If we do not reset
> the link, we end up with a frozen system after resume. The last message
> we see is:
> ath10k_pci 0000:01:00.0: Unable to change power state from D3hot to D0,
> device inaccessible
>
> Besides resetting the link, we also need to enable msi again, otherwise
> DMA access will not work and we can still end up with a frozen system.
> With these changes we can suspend and resume the system properly with a
> PCIe device attached. This was tested with a Compex WLE900VX miniPCIe
> Wifi module.
>
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 45 ++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index f17561791e35a..751243f4c519e 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1213,14 +1213,57 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
>  	return 0;
>  }
>
> +static int imx6_pcie_reset_link(struct imx6_pcie *imx6_pcie)
> +{
> +	int ret;
> +
> +	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> +			   IMX6Q_GPR1_PCIE_TEST_PD, 1 << 18);
> +	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> +			   IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
> +
> +	/* Reset the PCIe device */
> +	gpiod_set_value_cansleep(imx6_pcie->reset_gpiod, 1);
> +
> +	ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> +	if (ret) {
> +		dev_err(imx6_pcie->pci->dev, "unable to enable pcie ref clock\n");
> +		return ret;
> +	}
> +
> +	imx6_pcie_deassert_reset_gpio(imx6_pcie);

In my patch https://lore.kernel.org/linux-pci/Zr4XG6r+HnbIlu8S@lizhi-Precision-Tower-5810/T/#mc5f38934b6cef95eca90f1a6a63b3193e45179de

imx6qp_pcie_core_reset() and imx6q_pcie_core_reset() is not symatic for
assert/desert() to match origin code. I plan fix it after above patch
merged.

Does it work if make above code symatic?

> +
> +	/*
> +	 * Setup the root complex again and enable msi. Without this PCIe will
> +	 * not work in msi mode and drivers will crash if they try to access
> +	 * the device memory area
> +	 */
> +	dw_pcie_setup_rc(&imx6_pcie->pci->pp);
> +	if (pci_msi_enabled()) {
> +		u32 val;
> +		u8 offset = dw_pcie_find_capability(imx6_pcie->pci, PCI_CAP_ID_MSI);
> +
> +		val = dw_pcie_readw_dbi(imx6_pcie->pci, offset + PCI_MSI_FLAGS);
> +		val |= PCI_MSI_FLAGS_ENABLE;
> +		dw_pcie_writew_dbi(imx6_pcie->pci, offset + PCI_MSI_FLAGS, val);
> +	}

there are already have imx6_pcie_msi_save_restore(imx6_pcie, true); in
suspend/resume, why need addtional one here?

> +
> +	return 0;
> +}
> +
>  static int imx6_pcie_resume_noirq(struct device *dev)
>  {
>  	int ret;
>  	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
>  	struct dw_pcie_rp *pp = &imx6_pcie->pci->pp;
>
> +	/*
> +	 * Even though the i.MX6Q does not support suspend/resume, we need to
> +	 * reset the link after resume or the memory mapped PCIe I/O space will
> +	 * be inaccessible. This will cause the system to freeze.
> +	 */
>  	if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_SUPPORTS_SUSPEND))
> -		return 0;
> +		return imx6_pcie_reset_link(imx6_pcie);

If reset everything, I supposed we can add IMX6_PCIE_FLAG_SUPPORTS_SUSPEND
at driver data.

>
>  	ret = imx6_pcie_host_init(pp);
>  	if (ret)
> --
> 2.43.0
>

Re: [PATCH v1 2/3] PCI: imx6: move the wait for clock stabilization to enable ref clk

[Frank Li]

On Mon, Aug 19, 2024 at 11:03:18AM +0200, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
>
> After enabling the ref clock, we should wait for the clock to stabilize.
> To eliminate the need for code duplication in the future, move the
> usleep to the enable_ref_clk function.

Logically, it's better wait in imx6_pcie_clk_enable(). But not sure why
it can reduce duplication.

Frank

>
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index fda704d82431f..f17561791e35a 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -632,6 +632,9 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
>  		break;
>  	}
>
> +	/* allow the clocks to stabilize */
> +	usleep_range(200, 500);
> +
>  	return ret;
>  }
>
> @@ -672,8 +675,6 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
>  		goto err_ref_clk;
>  	}
>
> -	/* allow the clocks to stabilize */
> -	usleep_range(200, 500);
>  	return 0;
>
>  err_ref_clk:
> --
> 2.43.0
>

Re: [PATCH v1 1/3] PCI: imx6: Add a function to deassert the reset gpio

[Frank Li]

On Mon, Aug 19, 2024 at 11:03:17AM +0200, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
>
> To avoid code duplication, move the code to disable the reset GPIO to a
> separate function.

Add help function imx6_pcie_deassert_reset_gpio() to handle reset GPIO.

>
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 964d67756eb2b..fda704d82431f 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -722,6 +722,17 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>  	gpiod_set_value_cansleep(imx6_pcie->reset_gpiod, 1);
>  }
>
> +static void imx6_pcie_deassert_reset_gpio(struct imx6_pcie *imx6_pcie)
> +{
> +	/* Some boards don't have PCIe reset GPIO. */
> +	if (imx6_pcie->reset_gpiod) {
> +		msleep(100);
> +		gpiod_set_value_cansleep(imx6_pcie->reset_gpiod, 0);
> +		/* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
> +		msleep(100);
> +	}
> +}
> +
>  static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  {
>  	struct dw_pcie *pci = imx6_pcie->pci;
> @@ -766,13 +777,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  		break;
>  	}
>
> -	/* Some boards don't have PCIe reset GPIO. */
> -	if (imx6_pcie->reset_gpiod) {
> -		msleep(100);
> -		gpiod_set_value_cansleep(imx6_pcie->reset_gpiod, 0);
> -		/* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
> -		msleep(100);
> -	}
> +	imx6_pcie_deassert_reset_gpio(imx6_pcie);
>
>  	return 0;
>  }
> --
> 2.43.0
>

Re: [PATCH v1 0/3] PCI: imx6: reset link after suspend/resume

[Stefan Eichenberger]

On Mon, Aug 19, 2024 at 10:30:38AM -0400, Frank Li wrote:
> On Mon, Aug 19, 2024 at 11:03:16AM +0200, Stefan Eichenberger wrote:
> > On the i.MX6Quad (not QuadPlus), the PCIe link does not work after a
> > suspend/resume cycle. Worse, the PCIe memory mapped I/O isn't accessible
> > at all, so the system freezes when a PCIe driver tries to access its I/O
> > space. The only way to get resume working again is to reset the PCIe
> > link, similar to what is done on devices that support suspend/resume.
> > Through trial and error, we found that something about the PCIe
> > reference clock does not work as expected after a resume. We could not
> > figure out if it is disabled (even though the registers still say it is
> > enabled), or if it is somehow unstable or has some hiccups. With the
> > workaround introduced in this patch series, we were able to fully resume
> > a Compex WLE900VX (ath10k) miniPCIe Wifi module and an Intel AX200 M.2
> > Wifi module. If there is a better way or other ideas on how to fix this
> > problem, please let us know. We are aware that resetting the link should
> > not be necessary, but we could not find a better solution. More
> > interestingly, even the SoCs that support suspend/resume according to
> > the i.MX erratas seem to reset the link on resume in
> > imx6_pcie_host_init, so we hope this might be a valid workaround.
> >
> > Stefan Eichenberger (3):
> >   PCI: imx6: Add a function to deassert the reset gpio
> >   PCI: imx6: move the wait for clock stabilization to enable ref clk
> >   PCI: imx6: reset link on resume
> 
> Thanks you for your patch, but it may have conflict with
> https://lore.kernel.org/linux-pci/Zr4XG6r+HnbIlu8S@lizhi-Precision-Tower-5810/T/#t
> 

Thanks a lot for the hint. I will have a look at the series and see if I
can adapt my changes including your suggestions.

Regards,
Stefan

Re: [PATCH v1 3/3] PCI: imx6: reset link on resume

[Stefan Eichenberger]

On Mon, Aug 19, 2024 at 10:39:45AM -0400, Frank Li wrote:
> On Mon, Aug 19, 2024 at 11:03:19AM +0200, Stefan Eichenberger wrote:
> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> >
> > According to the https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf errata,
> 
> Can you show errata number here?
> 

I will include it in the next version of the patch. If I understand it
correct it is ERR005723.

> > the i.MX6Q PCIe controller does not support suspend/resume. So suspend
> > and resume was omitted. However, this does not seem to work because it
> > looks like the PCIe link is still expecting a reset. If we do not reset
> > the link, we end up with a frozen system after resume. The last message
> > we see is:
> > ath10k_pci 0000:01:00.0: Unable to change power state from D3hot to D0,
> > device inaccessible
> >
> > Besides resetting the link, we also need to enable msi again, otherwise
> > DMA access will not work and we can still end up with a frozen system.
> > With these changes we can suspend and resume the system properly with a
> > PCIe device attached. This was tested with a Compex WLE900VX miniPCIe
> > Wifi module.
> >
> > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 45 ++++++++++++++++++++++++++-
> >  1 file changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index f17561791e35a..751243f4c519e 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -1213,14 +1213,57 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> >  	return 0;
> >  }
> >
> > +static int imx6_pcie_reset_link(struct imx6_pcie *imx6_pcie)
> > +{
> > +	int ret;
> > +
> > +	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> > +			   IMX6Q_GPR1_PCIE_TEST_PD, 1 << 18);
> > +	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> > +			   IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
> > +
> > +	/* Reset the PCIe device */
> > +	gpiod_set_value_cansleep(imx6_pcie->reset_gpiod, 1);
> > +
> > +	ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> > +	if (ret) {
> > +		dev_err(imx6_pcie->pci->dev, "unable to enable pcie ref clock\n");
> > +		return ret;
> > +	}
> > +
> > +	imx6_pcie_deassert_reset_gpio(imx6_pcie);
> 
> In my patch https://lore.kernel.org/linux-pci/Zr4XG6r+HnbIlu8S@lizhi-Precision-Tower-5810/T/#mc5f38934b6cef95eca90f1a6a63b3193e45179de
> 
> imx6qp_pcie_core_reset() and imx6q_pcie_core_reset() is not symatic for
> assert/desert() to match origin code. I plan fix it after above patch
> merged.
> 
> Does it work if make above code symatic?
> 

I will give it a try with your patches applied and let you know.

> > +
> > +	/*
> > +	 * Setup the root complex again and enable msi. Without this PCIe will
> > +	 * not work in msi mode and drivers will crash if they try to access
> > +	 * the device memory area
> > +	 */
> > +	dw_pcie_setup_rc(&imx6_pcie->pci->pp);
> > +	if (pci_msi_enabled()) {
> > +		u32 val;
> > +		u8 offset = dw_pcie_find_capability(imx6_pcie->pci, PCI_CAP_ID_MSI);
> > +
> > +		val = dw_pcie_readw_dbi(imx6_pcie->pci, offset + PCI_MSI_FLAGS);
> > +		val |= PCI_MSI_FLAGS_ENABLE;
> > +		dw_pcie_writew_dbi(imx6_pcie->pci, offset + PCI_MSI_FLAGS, val);
> > +	}
> 
> there are already have imx6_pcie_msi_save_restore(imx6_pcie, true); in
> suspend/resume, why need addtional one here?
> 

I took the part from the probe function and added it here. I will see if
I can rework that part.

> > +
> > +	return 0;
> > +}
> > +
> >  static int imx6_pcie_resume_noirq(struct device *dev)
> >  {
> >  	int ret;
> >  	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> >  	struct dw_pcie_rp *pp = &imx6_pcie->pci->pp;
> >
> > +	/*
> > +	 * Even though the i.MX6Q does not support suspend/resume, we need to
> > +	 * reset the link after resume or the memory mapped PCIe I/O space will
> > +	 * be inaccessible. This will cause the system to freeze.
> > +	 */
> >  	if (!(imx6_pcie->drvdata->flags & IMX6_PCIE_FLAG_SUPPORTS_SUSPEND))
> > -		return 0;
> > +		return imx6_pcie_reset_link(imx6_pcie);
> 
> If reset everything, I supposed we can add IMX6_PCIE_FLAG_SUPPORTS_SUSPEND
> at driver data.
> 

This didn't work for the current version. It seems we do too much in the
suspend function and therefore it is still not working. However, maybe
it is really better to just add the flag and try to make suspend/resume
work by using the function pointers you introduced. I will have a look
at it, thanks.

> >
> >  	ret = imx6_pcie_host_init(pp);
> >  	if (ret)
> > --
> > 2.43.0
> >

Re: [PATCH v1 2/3] PCI: imx6: move the wait for clock stabilization to enable ref clk

[Stefan Eichenberger]

On Mon, Aug 19, 2024 at 10:45:13AM -0400, Frank Li wrote:
> On Mon, Aug 19, 2024 at 11:03:18AM +0200, Stefan Eichenberger wrote:
> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> >
> > After enabling the ref clock, we should wait for the clock to stabilize.
> > To eliminate the need for code duplication in the future, move the
> > usleep to the enable_ref_clk function.
> 
> Logically, it's better wait in imx6_pcie_clk_enable(). But not sure why
> it can reduce duplication.

I'm using the imx6_pcie_clk_enable in the if statement on resume for the
i.MX6Q. If the sleep is not in there I have to add it there as well. I
will check if this is still necessary with your changes and if so would
update the commit message.

Re: [PATCH v1 1/3] PCI: imx6: Add a function to deassert the reset gpio

[Stefan Eichenberger]

On Mon, Aug 19, 2024 at 10:49:14AM -0400, Frank Li wrote:
> On Mon, Aug 19, 2024 at 11:03:17AM +0200, Stefan Eichenberger wrote:
> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> >
> > To avoid code duplication, move the code to disable the reset GPIO to a
> > separate function.
> 
> Add help function imx6_pcie_deassert_reset_gpio() to handle reset GPIO.
> 

Sounds good, thanks for the suggestion.

Re: [PATCH v1 0/3] PCI: imx6: reset link after suspend/resume

[Stefan Eichenberger]

Hi Frank,

On Tue, Aug 20, 2024 at 08:52:41AM +0200, Stefan Eichenberger wrote:
> On Mon, Aug 19, 2024 at 10:30:38AM -0400, Frank Li wrote:
> > On Mon, Aug 19, 2024 at 11:03:16AM +0200, Stefan Eichenberger wrote:
> > > On the i.MX6Quad (not QuadPlus), the PCIe link does not work after a
> > > suspend/resume cycle. Worse, the PCIe memory mapped I/O isn't accessible
> > > at all, so the system freezes when a PCIe driver tries to access its I/O
> > > space. The only way to get resume working again is to reset the PCIe
> > > link, similar to what is done on devices that support suspend/resume.
> > > Through trial and error, we found that something about the PCIe
> > > reference clock does not work as expected after a resume. We could not
> > > figure out if it is disabled (even though the registers still say it is
> > > enabled), or if it is somehow unstable or has some hiccups. With the
> > > workaround introduced in this patch series, we were able to fully resume
> > > a Compex WLE900VX (ath10k) miniPCIe Wifi module and an Intel AX200 M.2
> > > Wifi module. If there is a better way or other ideas on how to fix this
> > > problem, please let us know. We are aware that resetting the link should
> > > not be necessary, but we could not find a better solution. More
> > > interestingly, even the SoCs that support suspend/resume according to
> > > the i.MX erratas seem to reset the link on resume in
> > > imx6_pcie_host_init, so we hope this might be a valid workaround.
> > >
> > > Stefan Eichenberger (3):
> > >   PCI: imx6: Add a function to deassert the reset gpio
> > >   PCI: imx6: move the wait for clock stabilization to enable ref clk
> > >   PCI: imx6: reset link on resume
> > 
> > Thanks you for your patch, but it may have conflict with
> > https://lore.kernel.org/linux-pci/Zr4XG6r+HnbIlu8S@lizhi-Precision-Tower-5810/T/#t
> > 
> 
> Thanks a lot for the hint. I will have a look at the series and see if I
> can adapt my changes including your suggestions.

I did some more tests with your series applied. Everything works as
expected on an i.MX8M Plus. However, the i.MX6Quad PCIe link still does
not work after a resume. I could trace the issues back to the following
functions, besides that we could use the same suspend/resume function as
for the other i.MX SoCs.

In suspend the follwoing function is causing issues:
imx_pcie_stop_link(imx_pcie->pci);

In resume the following functions are causing issues:
imx_pcie->drvdata->init_phy(imx_pcie); // in imx_pcie_host_init
imx_pcie_start_link(imx_pcie->pci);

I think the second one makes sense because I could not stop the link I
should not start it again. But why is also the init_phy function
failing? Are they required to setup the link? 

The messages I get when the system resumes are:
[   50.176212] Enabling non-boot CPUs ...
[   50.194446] CPU1 is up
[   50.198087] CPU2 is up
[   50.201746] CPU3 is up
[   50.563710] imx6q-pcie 1ffc000.pcie: Read DBI address failed

After the last message the system hangs. It seems this happens because
the PCIe I/O mem is not accessible anymore.

Do you have an idea what could cause imx_pcie_stop_link to break the
link on resume? Without calling them the link is working fine after
resuming and the drivers can access the PCIe I/O mem.

Thanks,
Stefan

Re: [PATCH v1 0/3] PCI: imx6: reset link after suspend/resume

[Frank Li]

On Wed, Aug 21, 2024 at 05:41:00PM +0200, Stefan Eichenberger wrote:
> Hi Frank,
>
> On Tue, Aug 20, 2024 at 08:52:41AM +0200, Stefan Eichenberger wrote:
> > On Mon, Aug 19, 2024 at 10:30:38AM -0400, Frank Li wrote:
> > > On Mon, Aug 19, 2024 at 11:03:16AM +0200, Stefan Eichenberger wrote:
> > > > On the i.MX6Quad (not QuadPlus), the PCIe link does not work after a
> > > > suspend/resume cycle. Worse, the PCIe memory mapped I/O isn't accessible
> > > > at all, so the system freezes when a PCIe driver tries to access its I/O
> > > > space. The only way to get resume working again is to reset the PCIe
> > > > link, similar to what is done on devices that support suspend/resume.
> > > > Through trial and error, we found that something about the PCIe
> > > > reference clock does not work as expected after a resume. We could not
> > > > figure out if it is disabled (even though the registers still say it is
> > > > enabled), or if it is somehow unstable or has some hiccups. With the
> > > > workaround introduced in this patch series, we were able to fully resume
> > > > a Compex WLE900VX (ath10k) miniPCIe Wifi module and an Intel AX200 M.2
> > > > Wifi module. If there is a better way or other ideas on how to fix this
> > > > problem, please let us know. We are aware that resetting the link should
> > > > not be necessary, but we could not find a better solution. More
> > > > interestingly, even the SoCs that support suspend/resume according to
> > > > the i.MX erratas seem to reset the link on resume in
> > > > imx6_pcie_host_init, so we hope this might be a valid workaround.
> > > >
> > > > Stefan Eichenberger (3):
> > > >   PCI: imx6: Add a function to deassert the reset gpio
> > > >   PCI: imx6: move the wait for clock stabilization to enable ref clk
> > > >   PCI: imx6: reset link on resume
> > >
> > > Thanks you for your patch, but it may have conflict with
> > > https://lore.kernel.org/linux-pci/Zr4XG6r+HnbIlu8S@lizhi-Precision-Tower-5810/T/#t
> > >
> >
> > Thanks a lot for the hint. I will have a look at the series and see if I
> > can adapt my changes including your suggestions.
>
> I did some more tests with your series applied. Everything works as
> expected on an i.MX8M Plus. However, the i.MX6Quad PCIe link still does
> not work after a resume. I could trace the issues back to the following
> functions, besides that we could use the same suspend/resume function as
> for the other i.MX SoCs.

Upstream 6q pci don't support suspend/resume.

 [IMX6Q] = {
                .variant = IMX6Q,
                .flags = IMX_PCIE_FLAG_IMX_PHY |
                         IMX_PCIE_FLAG_IMX_SPEED_CHANGE,

If you add some code, can you post your patch(mark as RFC) then let me to
check.

Frank

>
> In suspend the follwoing function is causing issues:
> imx_pcie_stop_link(imx_pcie->pci);
>
> In resume the following functions are causing issues:
> imx_pcie->drvdata->init_phy(imx_pcie); // in imx_pcie_host_init
> imx_pcie_start_link(imx_pcie->pci);
>
> I think the second one makes sense because I could not stop the link I
> should not start it again. But why is also the init_phy function
> failing? Are they required to setup the link?
>
> The messages I get when the system resumes are:
> [   50.176212] Enabling non-boot CPUs ...
> [   50.194446] CPU1 is up
> [   50.198087] CPU2 is up
> [   50.201746] CPU3 is up
> [   50.563710] imx6q-pcie 1ffc000.pcie: Read DBI address failed
>
> After the last message the system hangs. It seems this happens because
> the PCIe I/O mem is not accessible anymore.
>
> Do you have an idea what could cause imx_pcie_stop_link to break the
> link on resume? Without calling them the link is working fine after
> resuming and the drivers can access the PCIe I/O mem.
>
> Thanks,
> Stefan

Re: [PATCH v1 0/3] PCI: imx6: reset link after suspend/resume

[Stefan Eichenberger]

On Wed, Aug 21, 2024 at 12:51:12PM -0400, Frank Li wrote:
> On Wed, Aug 21, 2024 at 05:41:00PM +0200, Stefan Eichenberger wrote:
> > Hi Frank,
> >
> > On Tue, Aug 20, 2024 at 08:52:41AM +0200, Stefan Eichenberger wrote:
> > > On Mon, Aug 19, 2024 at 10:30:38AM -0400, Frank Li wrote:
> > > > On Mon, Aug 19, 2024 at 11:03:16AM +0200, Stefan Eichenberger wrote:
> > > > > On the i.MX6Quad (not QuadPlus), the PCIe link does not work after a
> > > > > suspend/resume cycle. Worse, the PCIe memory mapped I/O isn't accessible
> > > > > at all, so the system freezes when a PCIe driver tries to access its I/O
> > > > > space. The only way to get resume working again is to reset the PCIe
> > > > > link, similar to what is done on devices that support suspend/resume.
> > > > > Through trial and error, we found that something about the PCIe
> > > > > reference clock does not work as expected after a resume. We could not
> > > > > figure out if it is disabled (even though the registers still say it is
> > > > > enabled), or if it is somehow unstable or has some hiccups. With the
> > > > > workaround introduced in this patch series, we were able to fully resume
> > > > > a Compex WLE900VX (ath10k) miniPCIe Wifi module and an Intel AX200 M.2
> > > > > Wifi module. If there is a better way or other ideas on how to fix this
> > > > > problem, please let us know. We are aware that resetting the link should
> > > > > not be necessary, but we could not find a better solution. More
> > > > > interestingly, even the SoCs that support suspend/resume according to
> > > > > the i.MX erratas seem to reset the link on resume in
> > > > > imx6_pcie_host_init, so we hope this might be a valid workaround.
> > > > >
> > > > > Stefan Eichenberger (3):
> > > > >   PCI: imx6: Add a function to deassert the reset gpio
> > > > >   PCI: imx6: move the wait for clock stabilization to enable ref clk
> > > > >   PCI: imx6: reset link on resume
> > > >
> > > > Thanks you for your patch, but it may have conflict with
> > > > https://lore.kernel.org/linux-pci/Zr4XG6r+HnbIlu8S@lizhi-Precision-Tower-5810/T/#t
> > > >
> > >
> > > Thanks a lot for the hint. I will have a look at the series and see if I
> > > can adapt my changes including your suggestions.
> >
> > I did some more tests with your series applied. Everything works as
> > expected on an i.MX8M Plus. However, the i.MX6Quad PCIe link still does
> > not work after a resume. I could trace the issues back to the following
> > functions, besides that we could use the same suspend/resume function as
> > for the other i.MX SoCs.
> 
> Upstream 6q pci don't support suspend/resume.
> 
>  [IMX6Q] = {
>                 .variant = IMX6Q,
>                 .flags = IMX_PCIE_FLAG_IMX_PHY |
>                          IMX_PCIE_FLAG_IMX_SPEED_CHANGE,
> 
> If you add some code, can you post your patch(mark as RFC) then let me to
> check.

Sure, I will wait until your series is merged and then try to come up
with an RFC for suspend/resume on i.MX6Quad. Maybe we could use the same
mechanism as for the downstream kernel where they just toggle the
IMX6Q_GPR1_PCIE_TEST_PD bit?
https://github.com/nxp-imx/linux-imx/commit/4e92355e1f79d225ea842511fcfd42b343b32995