PCI: imx6: reset link after suspend/resume

From: Stefan Eichenberger <[email protected]>

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

Signed-off-by Stefan Eichenberger <[email protected]>

---
 drivers/pci/controller/dwc/pci-imx6.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

drivers/pci/controller/dwc/pci-imx6.c +12 -8

index 964d67756eb2b..fda704d82431f 100644
@@ -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

Quoted 4 lines · Stefan Eichenberger

From: Stefan Eichenberger <[email protected]>

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.

From: Stefan Eichenberger <[email protected]>

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 <[email protected]>

---
 drivers/pci/controller/dwc/pci-imx6.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/pci/controller/dwc/pci-imx6.c +3 -3

index fda704d82431f..f17561791e35a 100644
@@ -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

Quoted 5 lines · Stefan Eichenberger

From: Stefan Eichenberger <[email protected]>

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.

From: Stefan Eichenberger <[email protected]>

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 <[email protected]>

---
 drivers/pci/controller/dwc/pci-imx6.c | 45 ++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

drivers/pci/controller/dwc/pci-imx6.c +9 -0

index f17561791e35a..751243f4c519e 100644
@@ -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);
+

drivers/pci/controller/dwc/pci-imx6.c +5 -0

+
+	/*
+	 * 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

drivers/pci/controller/dwc/pci-imx6.c +4 -0

+
+	return 0;
+}
+
 static int imx6_pcie_resume_noirq(struct device *dev)
 {
 	int ret;
 	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);

drivers/pci/controller/dwc/pci-imx6.c +0 -1

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

2.43.0

Quoted 3 lines · Stefan Eichenberger

From: Stefan Eichenberger <[email protected]>

According to the https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf errata,

Can you show errata number here?

Quoted 36 lines

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 <[email protected]>

---
 drivers/pci/controller/dwc/pci-imx6.c | 45 ++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

drivers/pci/controller/dwc/pci-imx6.c +9 -0

index f17561791e35a..751243f4c519e 100644
@@ -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);
+

drivers/pci/controller/dwc/pci-imx6.c static int imx6_pcie_suspend_noirq(struct device *dev) · line 1234

Quoted 10 lines

drivers/pci/controller/dwc/pci-imx6.c +10 -0

+	/* 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?

Quoted 10 lines

drivers/pci/controller/dwc/pci-imx6.c +10 -0

+
+	/*
+	 * 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);

drivers/pci/controller/dwc/pci-imx6.c static int imx6_pcie_suspend_noirq(struct device *dev) · line 1249

Quoted 5 lines

drivers/pci/controller/dwc/pci-imx6.c +5 -0

+
+		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?

Quoted 10 lines

drivers/pci/controller/dwc/pci-imx6.c +4 -0

+
+	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;
 

drivers/pci/controller/dwc/pci-imx6.c static int imx6_pcie_suspend_noirq(struct device *dev) · line 1266

Quoted 8 lines

drivers/pci/controller/dwc/pci-imx6.c +6 -1

+	/*
+	 * 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.

Quoted 6 lines

drivers/pci/controller/dwc/pci-imx6.c +0 -1

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

2.43.0

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

Quoted 18 lines · 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.

Quoted 4 lines

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

Quoted 24 lines · Frank Li

Quoted 22 lines · 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

Quoted 3 lines

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.

Hi Frank,

Quoted 29 lines · Stefan Eichenberger

Quoted 27 lines · Frank Li

Quoted 18 lines · 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.

Quoted 4 lines

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

Quoted 2 lines

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

Quoted 32 lines · Stefan Eichenberger

Hi Frank,

Quoted 29 lines · Stefan Eichenberger

Quoted 27 lines · Frank Li

Quoted 18 lines · 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.

Quoted 4 lines

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

Quoted 8 lines

Quoted 2 lines

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.