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

[Frank Li <Frank.li@nxp.com>]

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
>