| Message ID | 20240819090428.17349-4-eichest@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: imx6: reset link after suspend/resume | expand |
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 >
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 > >
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)