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

Frank Li
2024-08-19T14:39:57Z

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
>