From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6CC3720AF65; Tue, 14 Jan 2025 20:26:25 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736886385; cv=none; b=gr51Cp1Zvek9QVl0jFqzxqIhqOM9AHSW+wwTyPCWPVSG5uE8ezmritjFKSjXhtG+y+PMLZnjG/Tp5Luyb8Siq79gj5g+tmbSRLF2aUauiY9AlUMvV7Glpi55sshkPK04FgsFke6nN8TkHodoyFJ+AJvzZjR5wpRQo5WFq0U2RHo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736886385; c=relaxed/simple; bh=1IgRfw629cmZrFu0qFbSHjpnPx0gzmnM39OIlUYVCHY=; h=Date:From:To:Cc:Subject:Message-ID:MIME-Version:Content-Type: Content-Disposition:In-Reply-To; b=IEXLIlfiFli7F0WUdY0yFbDw8oGPqQ2iOvedY1YnU0ygZ4EuBOxdvt9gY1HfrIhXqGdL/YSAVf5jWJZp6DXsswCrPmDrmPvU/omCJiasdEqwcPgqEKUW2RdnlRXxXXHqa3Se6GcO1mj0yZtNNhgUc3kqONSJZLhoLWdcc90hygU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=Qek0qVuN; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="Qek0qVuN" Received: by smtp.kernel.org (Postfix) with ESMTPSA id D1530C4CEE0; Tue, 14 Jan 2025 20:26:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1736886385; bh=1IgRfw629cmZrFu0qFbSHjpnPx0gzmnM39OIlUYVCHY=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=Qek0qVuNhlWGFbwYGSKClZeZ/yWh5m00JJLLCPFkEr5CtllYfucr1Dimv1oH1j1fh gxsI8dog3/AWh/V+ymvKf3NxfN+N8STzU5qjFW9Bcua8CceMxUeSJpNZrV0RgKbuAG 6IhAva51jinHP5Vx7EaMuRgTxgoMrbgBjCk6CDnYmOif5JLczexWPgabuUtfqdc4sp dFpUs4e88Z73DlSDtTkLL+XxxRFwqLi/kBahSiAiijRKP7k2vJKH5rLPDVBzVmNaX4 pD6wgAEIjYjMW9hRq2PPS3RHistOP/YiVBgnNBYty6rtfv0nyLL/ae2fDXCVFaxrfZ 2pu/GlARHWiow== Date: Tue, 14 Jan 2025 14:26:22 -0600 From: Bjorn Helgaas To: Frank Li Cc: Richard Zhu , dlemoal@kernel.org, jingoohan1@gmail.com, bhelgaas@google.com, lpieralisi@kernel.org, kw@linux.com, manivannan.sadhasivam@linaro.org, robh@kernel.org, quic_krichai@quicinc.com, imx@lists.linux.dev, kernel@pengutronix.de, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4 1/2] PCI: dwc: Always stop link in the dw_pcie_suspend_noirq Message-ID: <20250114202622.GA483044@bhelgaas> Precedence: bulk X-Mailing-List: linux-pci@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Tue, Jan 14, 2025 at 02:32:38PM -0500, Frank Li wrote: > On Tue, Jan 14, 2025 at 12:15:18PM -0600, Bjorn Helgaas wrote: > > 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 > > > Reviewed-by: Manivannan Sadhasivam > > > --- > > > 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. > > It should be for all dwc controllers although find such problem at i.MX8QM > platfrom. It should keep symmetric between suspend/resume function. > > So far only layerscape and i.MX platform use these common functions. Other > dwc platform still have not switched to this common function yet. I see that layerscape uses dw_pcie_suspend_noirq(): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-layerscape.c?id=v6.13-rc7#n379 But I don't see where imx6 does: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-imx6.c?id=v6.13-rc7#n1236 We don't currently have anything queued that touches pci-imx6.c; am I missing a patch that converts pci-imx6.c to use dw_pcie_suspend_noirq()? This doesn't feel urgent yet since the commit log talks about i.MX8QM, but I can't make a connection between i.MX8QM and this patch. Bjorn