Re: [PATCH v4 1/2] PCI: dwc: Always stop link in the dw_pcie_suspend_noirq

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Dec 10, 2024 at 04:15:56PM +0800, Richard Zhu wrote:
> On i.MX8QM, PCIe link can't be re-established again in
> dw_pcie_resume_noirq(), if the LTSSM_EN bit is not cleared properly in
> dw_pcie_suspend_noirq().
>
> Add dw_pcie_stop_link() into dw_pcie_suspend_noirq() to fix this issue and
> keep symmetric in suspend/resume function since there is
> dw_pcie_start_link() in dw_pcie_resume_noirq().
> 
> Fixes: 4774faf854f5 ("PCI: dwc: Implement generic suspend/resume functionality")
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index f882b11fd7b94..f56cb7b9e6f99 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -1001,6 +1001,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
>  		return ret;
>  	}
>  
> +	dw_pcie_stop_link(pci);

We should try to avoid changes to the generic DWC path just to
accommodate one controller.  Since other DWC-based controllers
apparently don't need dw_pcie_stop_link() here, this seems like it
might be the wrong place for this change.

If doing dw_pcie_stop_link() here is really helpful for all DWC
controllers, this would be fine, but the commit log should then explain
why it helps everybody, not why one particular controller benefits.

If it's only needed for i.MX8QM, we do already have a
controller-specific hook (.deinit()) just below; maybe that could be
used?

>  	if (pci->pp.ops->deinit)
>  		pci->pp.ops->deinit(&pci->pp);
>  
> -- 
> 2.37.1
>