[PATCH v3 0/3] Bug fixes when dwc generic suspend/resume callbacks are used

[PATCH v3 0/3] Bug fixes when dwc generic suspend/resume callbacks are used

[Richard Zhu]

The patch #1 is issued before, but it's not applied yet refer to [1].
Combine these two bug fixes into one series and send here.

[1] https://patchwork.kernel.org/project/linux-pci/patch/1721628913-1449-1-git-send-email-hongxing.zhu@nxp.com/

Resend this bug fixes patch-set, with one more codes clean up patch.
Here is the discussion [2] and final solution [3] of the codes clean up commit.
[2] https://patchwork.kernel.org/project/linux-pci/patch/1721628913-1449-1-git-send-email-hongxing.zhu@nxp.com/
[3] https://patchwork.kernel.org/project/linux-pci/patch/20241126073909.4058733-1-hongxing.zhu@nxp.com/

v3 changes:
Regarding the discussion listed above[2].
Resend the patch-set after adding one more codes clean up patch together.

v2 changes:
Thanks for Manivannan's review.
- Refine the subject of second patch and add
  "Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>"
- In first patch, update the commit message and move one comment into
  proper place.
  BTW, Manivannan found another potential issue that suspend is entry but
  the link is in L1SS stat in v1 review. This is a new story. And it's better
  to be verified and fixed by another commit later.
v2: https://patchwork.kernel.org/project/linux-pci/cover/1728539269-1861-1-git-send-email-hongxing.zhu@nxp.com/

[PATCH v3 1/3] PCI: dwc: Fix resume failure if no EP is connected on
[PATCH v3 2/3] PCI: dwc: Always stop link in the
[PATCH v3 3/3] PCI: dwc: Clean up some unnecessary codes in

drivers/pci/controller/dwc/pcie-designware-host.c | 17 ++++++++++++-----
drivers/pci/controller/dwc/pcie-designware.h      |  1 +
2 files changed, 13 insertions(+), 5 deletions(-)

[PATCH v3 1/3] PCI: dwc: Fix resume failure if no EP is connected on some platforms

[Richard Zhu]

The dw_pcie_suspend_noirq() function currently returns success directly
if no endpoint (EP) device is connected. However, on some platforms,
power loss occurs during suspend, causing dw_resume() to do nothing in
this case. This results in a system halt because the DWC controller is
not initialized after power-on during resume.

Call deinit() in suspend and init() at resume regardless of whether
there are EP device connections or not. It is not harmful to perform
deinit() and init() again for the no power-off case, and it keeps the
code simple and consistent in logic.

Fixes: 4774faf854f5 ("PCI: dwc: Implement generic suspend/resume functionality")
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 .../pci/controller/dwc/pcie-designware-host.c | 30 +++++++++----------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index f882b11fd7b94..11563402c571b 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -982,23 +982,23 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
 	if (dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKCTL) & PCI_EXP_LNKCTL_ASPM_L1)
 		return 0;
 
-	if (dw_pcie_get_ltssm(pci) <= DW_PCIE_LTSSM_DETECT_ACT)
-		return 0;
-
-	if (pci->pp.ops->pme_turn_off)
-		pci->pp.ops->pme_turn_off(&pci->pp);
-	else
-		ret = dw_pcie_pme_turn_off(pci);
+	/* Only send out PME_TURN_OFF when PCIE link is up */
+	if (dw_pcie_get_ltssm(pci) > DW_PCIE_LTSSM_DETECT_ACT) {
+		if (pci->pp.ops->pme_turn_off)
+			pci->pp.ops->pme_turn_off(&pci->pp);
+		else
+			ret = dw_pcie_pme_turn_off(pci);
 
-	if (ret)
-		return ret;
+		if (ret)
+			return ret;
 
-	ret = read_poll_timeout(dw_pcie_get_ltssm, val, val == DW_PCIE_LTSSM_L2_IDLE,
-				PCIE_PME_TO_L2_TIMEOUT_US/10,
-				PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
-	if (ret) {
-		dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val);
-		return ret;
+		ret = read_poll_timeout(dw_pcie_get_ltssm, val, val == DW_PCIE_LTSSM_L2_IDLE,
+					PCIE_PME_TO_L2_TIMEOUT_US/10,
+					PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
+		if (ret) {
+			dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val);
+			return ret;
+		}
 	}
 
 	if (pci->pp.ops->deinit)
-- 
2.37.1

Re: [PATCH v3 1/3] PCI: dwc: Fix resume failure if no EP is connected on some platforms

[Damien Le Moal]

On 12/9/24 16:39, Richard Zhu wrote:
> The dw_pcie_suspend_noirq() function currently returns success directly
> if no endpoint (EP) device is connected. However, on some platforms,
> power loss occurs during suspend, causing dw_resume() to do nothing in
> this case. This results in a system halt because the DWC controller is
> not initialized after power-on during resume.
> 
> Call deinit() in suspend and init() at resume regardless of whether
> there are EP device connections or not. It is not harmful to perform
> deinit() and init() again for the no power-off case, and it keeps the
> code simple and consistent in logic.
> 
> Fixes: 4774faf854f5 ("PCI: dwc: Implement generic suspend/resume functionality")
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
>  .../pci/controller/dwc/pcie-designware-host.c | 30 +++++++++----------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index f882b11fd7b94..11563402c571b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -982,23 +982,23 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
>  	if (dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKCTL) & PCI_EXP_LNKCTL_ASPM_L1)
>  		return 0;
>  
> -	if (dw_pcie_get_ltssm(pci) <= DW_PCIE_LTSSM_DETECT_ACT)
> -		return 0;
> -
> -	if (pci->pp.ops->pme_turn_off)
> -		pci->pp.ops->pme_turn_off(&pci->pp);
> -	else
> -		ret = dw_pcie_pme_turn_off(pci);
> +	/* Only send out PME_TURN_OFF when PCIE link is up */
> +	if (dw_pcie_get_ltssm(pci) > DW_PCIE_LTSSM_DETECT_ACT) {
> +		if (pci->pp.ops->pme_turn_off)
> +			pci->pp.ops->pme_turn_off(&pci->pp);
> +		else
> +			ret = dw_pcie_pme_turn_off(pci);
>  
> -	if (ret)
> -		return ret;
> +		if (ret)
> +			return ret;

Same comment as for patch 3. This "if (ret) return ret;" can go inside the else.
It is harmless, but there is also no point in having it here.

>  
> -	ret = read_poll_timeout(dw_pcie_get_ltssm, val, val == DW_PCIE_LTSSM_L2_IDLE,
> -				PCIE_PME_TO_L2_TIMEOUT_US/10,
> -				PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> -	if (ret) {
> -		dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val);
> -		return ret;
> +		ret = read_poll_timeout(dw_pcie_get_ltssm, val, val == DW_PCIE_LTSSM_L2_IDLE,
> +					PCIE_PME_TO_L2_TIMEOUT_US/10,
> +					PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> +		if (ret) {
> +			dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val);
> +			return ret;
> +		}
>  	}
>  
>  	if (pci->pp.ops->deinit)


-- 
Damien Le Moal
Western Digital Research

RE: [PATCH v3 1/3] PCI: dwc: Fix resume failure if no EP is connected on some platforms

[Hongxing Zhu]

Best Regards
Richard Zhu

> -----Original Message-----
> From: Damien Le Moal <dlemoal@kernel.org>
> Sent: 2024年12月9日 16:50
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; jingoohan1@gmail.com;
> bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
> manivannan.sadhasivam@linaro.org; robh@kernel.org; Frank Li
> <frank.li@nxp.com>; quic_krichai@quicinc.com
> Cc: imx@lists.linux.dev; kernel@pengutronix.de; linux-pci@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 1/3] PCI: dwc: Fix resume failure if no EP is connected on
> some platforms
> 
> On 12/9/24 16:39, Richard Zhu wrote:
> > The dw_pcie_suspend_noirq() function currently returns success
> > directly if no endpoint (EP) device is connected. However, on some
> > platforms, power loss occurs during suspend, causing dw_resume() to do
> > nothing in this case. This results in a system halt because the DWC
> > controller is not initialized after power-on during resume.
> >
> > Call deinit() in suspend and init() at resume regardless of whether
> > there are EP device connections or not. It is not harmful to perform
> > deinit() and init() again for the no power-off case, and it keeps the
> > code simple and consistent in logic.
> >
> > Fixes: 4774faf854f5 ("PCI: dwc: Implement generic suspend/resume
> > functionality")
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  .../pci/controller/dwc/pcie-designware-host.c | 30
> > +++++++++----------
> >  1 file changed, 15 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index f882b11fd7b94..11563402c571b 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -982,23 +982,23 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> >  	if (dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKCTL) &
> PCI_EXP_LNKCTL_ASPM_L1)
> >  		return 0;
> >
> > -	if (dw_pcie_get_ltssm(pci) <= DW_PCIE_LTSSM_DETECT_ACT)
> > -		return 0;
> > -
> > -	if (pci->pp.ops->pme_turn_off)
> > -		pci->pp.ops->pme_turn_off(&pci->pp);
> > -	else
> > -		ret = dw_pcie_pme_turn_off(pci);
> > +	/* Only send out PME_TURN_OFF when PCIE link is up */
> > +	if (dw_pcie_get_ltssm(pci) > DW_PCIE_LTSSM_DETECT_ACT) {
> > +		if (pci->pp.ops->pme_turn_off)
> > +			pci->pp.ops->pme_turn_off(&pci->pp);
> > +		else
> > +			ret = dw_pcie_pme_turn_off(pci);
> >
> > -	if (ret)
> > -		return ret;
> > +		if (ret)
> > +			return ret;
> 
> Same comment as for patch 3. This "if (ret) return ret;" can go inside the else.
> It is harmless, but there is also no point in having it here.
> 
Hi Damien:
Okay, thanks.
BTW, I'm considering that the use-case of #1 patch had been covered by #3
 commit already.
Since, the PME_TURN_OFF would be kicked off without LTSSM stat check in #3.
To be simple, how about drop the #1 patch, re-format the codes, and add one
 Fixes tag into #3?

Best Regards
Richard Zhu
> >
> > -	ret = read_poll_timeout(dw_pcie_get_ltssm, val, val ==
> DW_PCIE_LTSSM_L2_IDLE,
> > -				PCIE_PME_TO_L2_TIMEOUT_US/10,
> > -				PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> > -	if (ret) {
> > -		dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val);
> > -		return ret;
> > +		ret = read_poll_timeout(dw_pcie_get_ltssm, val, val ==
> DW_PCIE_LTSSM_L2_IDLE,
> > +					PCIE_PME_TO_L2_TIMEOUT_US/10,
> > +					PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> > +		if (ret) {
> > +			dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n",
> val);
> > +			return ret;
> > +		}
> >  	}
> >
> >  	if (pci->pp.ops->deinit)
> 
> 
> --
> Damien Le Moal
> Western Digital Research

[PATCH v3 2/3] PCI: dwc: Always stop link in the dw_pcie_suspend_noirq

[Richard Zhu]

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 11563402c571b..14e95c2952bbe 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)
 		}
 	}
 
+	dw_pcie_stop_link(pci);
 	if (pci->pp.ops->deinit)
 		pci->pp.ops->deinit(&pci->pp);
 
-- 
2.37.1

[PATCH v3 3/3] PCI: dwc: Clean up some unnecessary codes in dw_pcie_suspend_noirq()

[Richard Zhu]

Before sending PME_TURN_OFF, don't test the LTSSM state. Since it's safe
to send PME_TURN_OFF message regardless of whether the link is up or
down. So, there would be no need to test the LTSSM state before sending
PME_TURN_OFF message.

Only print the message when ltssm_stat is not in DETECT and POLL.
In the other words, there isn't an error message when no endpoint is
connected at all.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 .../pci/controller/dwc/pcie-designware-host.c | 38 +++++++++++--------
 drivers/pci/controller/dwc/pcie-designware.h  |  1 +
 2 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 14e95c2952bbe..02e0e8c255c70 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -982,25 +982,31 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
 	if (dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKCTL) & PCI_EXP_LNKCTL_ASPM_L1)
 		return 0;
 
-	/* Only send out PME_TURN_OFF when PCIE link is up */
-	if (dw_pcie_get_ltssm(pci) > DW_PCIE_LTSSM_DETECT_ACT) {
-		if (pci->pp.ops->pme_turn_off)
-			pci->pp.ops->pme_turn_off(&pci->pp);
-		else
-			ret = dw_pcie_pme_turn_off(pci);
-
-		if (ret)
-			return ret;
+	if (pci->pp.ops->pme_turn_off)
+		pci->pp.ops->pme_turn_off(&pci->pp);
+	else
+		ret = dw_pcie_pme_turn_off(pci);
+	if (ret)
+		return ret;
 
-		ret = read_poll_timeout(dw_pcie_get_ltssm, val, val == DW_PCIE_LTSSM_L2_IDLE,
-					PCIE_PME_TO_L2_TIMEOUT_US/10,
-					PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
-		if (ret) {
-			dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val);
-			return ret;
-		}
+	ret = read_poll_timeout(dw_pcie_get_ltssm, val,
+				val == DW_PCIE_LTSSM_L2_IDLE ||
+				val <= DW_PCIE_LTSSM_DETECT_WAIT,
+				PCIE_PME_TO_L2_TIMEOUT_US/10,
+				PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
+	if (ret) {
+		/* Only dump message when ltssm_stat isn't in DETECT and POLL */
+		dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val);
+		return ret;
 	}
 
+	/*
+	 * Refer to r6.0, sec 5.3.3.2.1, software should wait at least
+	 * 100ns after L2/L3 Ready before turning off refclock and
+	 * main power. It's harmless too when no endpoint connected.
+	 */
+	udelay(1);
+
 	dw_pcie_stop_link(pci);
 	if (pci->pp.ops->deinit)
 		pci->pp.ops->deinit(&pci->pp);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 5c14ed2cb91ed..7efcb4af66da3 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -330,6 +330,7 @@ enum dw_pcie_ltssm {
 	/* Need to align with PCIE_PORT_DEBUG0 bits 0:5 */
 	DW_PCIE_LTSSM_DETECT_QUIET = 0x0,
 	DW_PCIE_LTSSM_DETECT_ACT = 0x1,
+	DW_PCIE_LTSSM_DETECT_WAIT = 0x6,
 	DW_PCIE_LTSSM_L0 = 0x11,
 	DW_PCIE_LTSSM_L2_IDLE = 0x15,
 
-- 
2.37.1

Re: [PATCH v3 3/3] PCI: dwc: Clean up some unnecessary codes in dw_pcie_suspend_noirq()

[Damien Le Moal]

On 12/9/24 16:39, Richard Zhu wrote:
> Before sending PME_TURN_OFF, don't test the LTSSM state. Since it's safe
> to send PME_TURN_OFF message regardless of whether the link is up or
> down. So, there would be no need to test the LTSSM state before sending
> PME_TURN_OFF message.
> 
> Only print the message when ltssm_stat is not in DETECT and POLL.
> In the other words, there isn't an error message when no endpoint is
> connected at all.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  .../pci/controller/dwc/pcie-designware-host.c | 38 +++++++++++--------
>  drivers/pci/controller/dwc/pcie-designware.h  |  1 +
>  2 files changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 14e95c2952bbe..02e0e8c255c70 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -982,25 +982,31 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
>  	if (dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKCTL) & PCI_EXP_LNKCTL_ASPM_L1)
>  		return 0;
>  
> -	/* Only send out PME_TURN_OFF when PCIE link is up */
> -	if (dw_pcie_get_ltssm(pci) > DW_PCIE_LTSSM_DETECT_ACT) {
> -		if (pci->pp.ops->pme_turn_off)
> -			pci->pp.ops->pme_turn_off(&pci->pp);
> -		else
> -			ret = dw_pcie_pme_turn_off(pci);
> -
> -		if (ret)
> -			return ret;
> +	if (pci->pp.ops->pme_turn_off)
> +		pci->pp.ops->pme_turn_off(&pci->pp);
> +	else
> +		ret = dw_pcie_pme_turn_off(pci);
> +	if (ret)
> +		return ret;

ret is always 0 for the "if (pci->pp.ops->pme_turn_off)" case. So this test of
"if (ret) return ret" should really go inside the "else", and the initialization
of ret to 0 on declaration can be removed too.

>  
> -		ret = read_poll_timeout(dw_pcie_get_ltssm, val, val == DW_PCIE_LTSSM_L2_IDLE,
> -					PCIE_PME_TO_L2_TIMEOUT_US/10,
> -					PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> -		if (ret) {
> -			dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val);
> -			return ret;
> -		}
> +	ret = read_poll_timeout(dw_pcie_get_ltssm, val,
> +				val == DW_PCIE_LTSSM_L2_IDLE ||
> +				val <= DW_PCIE_LTSSM_DETECT_WAIT,
> +				PCIE_PME_TO_L2_TIMEOUT_US/10,
> +				PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> +	if (ret) {
> +		/* Only dump message when ltssm_stat isn't in DETECT and POLL */
> +		dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val);
> +		return ret;
>  	}
>  
> +	/*
> +	 * Refer to r6.0, sec 5.3.3.2.1, software should wait at least
> +	 * 100ns after L2/L3 Ready before turning off refclock and
> +	 * main power. It's harmless too when no endpoint connected.
> +	 */
> +	udelay(1);
> +
>  	dw_pcie_stop_link(pci);
>  	if (pci->pp.ops->deinit)
>  		pci->pp.ops->deinit(&pci->pp);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 5c14ed2cb91ed..7efcb4af66da3 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -330,6 +330,7 @@ enum dw_pcie_ltssm {
>  	/* Need to align with PCIE_PORT_DEBUG0 bits 0:5 */
>  	DW_PCIE_LTSSM_DETECT_QUIET = 0x0,
>  	DW_PCIE_LTSSM_DETECT_ACT = 0x1,
> +	DW_PCIE_LTSSM_DETECT_WAIT = 0x6,
>  	DW_PCIE_LTSSM_L0 = 0x11,
>  	DW_PCIE_LTSSM_L2_IDLE = 0x15,
>  


-- 
Damien Le Moal
Western Digital Research

RE: [PATCH v3 3/3] PCI: dwc: Clean up some unnecessary codes in dw_pcie_suspend_noirq()

[Hongxing Zhu]

> -----Original Message-----
> From: Damien Le Moal <dlemoal@kernel.org>
> Sent: 2024年12月9日 16:48
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; jingoohan1@gmail.com;
> bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
> manivannan.sadhasivam@linaro.org; robh@kernel.org; Frank Li
> <frank.li@nxp.com>; quic_krichai@quicinc.com
> Cc: imx@lists.linux.dev; kernel@pengutronix.de; linux-pci@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 3/3] PCI: dwc: Clean up some unnecessary codes in
> dw_pcie_suspend_noirq()
> 
> On 12/9/24 16:39, Richard Zhu wrote:
> > Before sending PME_TURN_OFF, don't test the LTSSM state. Since it's
> > safe to send PME_TURN_OFF message regardless of whether the link is up
> > or down. So, there would be no need to test the LTSSM state before
> > sending PME_TURN_OFF message.
> >
> > Only print the message when ltssm_stat is not in DETECT and POLL.
> > In the other words, there isn't an error message when no endpoint is
> > connected at all.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  .../pci/controller/dwc/pcie-designware-host.c | 38
> > +++++++++++--------  drivers/pci/controller/dwc/pcie-designware.h  |
> > 1 +
> >  2 files changed, 23 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 14e95c2952bbe..02e0e8c255c70 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -982,25 +982,31 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> >  	if (dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKCTL) &
> PCI_EXP_LNKCTL_ASPM_L1)
> >  		return 0;
> >
> > -	/* Only send out PME_TURN_OFF when PCIE link is up */
> > -	if (dw_pcie_get_ltssm(pci) > DW_PCIE_LTSSM_DETECT_ACT) {
> > -		if (pci->pp.ops->pme_turn_off)
> > -			pci->pp.ops->pme_turn_off(&pci->pp);
> > -		else
> > -			ret = dw_pcie_pme_turn_off(pci);
> > -
> > -		if (ret)
> > -			return ret;
> > +	if (pci->pp.ops->pme_turn_off)
> > +		pci->pp.ops->pme_turn_off(&pci->pp);
> > +	else
> > +		ret = dw_pcie_pme_turn_off(pci);
> > +	if (ret)
> > +		return ret;
> 
> ret is always 0 for the "if (pci->pp.ops->pme_turn_off)" case. So this test of "if (ret)
> return ret" should really go inside the "else", and the initialization of ret to 0 on
> declaration can be removed too.
Hi Damien:
Okay, thanks.
BTW, I'm considering that the use-case of #1 patch had been covered by #3
 commit already.
To be simple, how about drop the #1 patch, re-format the codes, and add one
 Fixes tag into #3?

Best Regards
Richard Zhu
> 
> >
> > -		ret = read_poll_timeout(dw_pcie_get_ltssm, val, val ==
> DW_PCIE_LTSSM_L2_IDLE,
> > -					PCIE_PME_TO_L2_TIMEOUT_US/10,
> > -					PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> > -		if (ret) {
> > -			dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n",
> val);
> > -			return ret;
> > -		}
> > +	ret = read_poll_timeout(dw_pcie_get_ltssm, val,
> > +				val == DW_PCIE_LTSSM_L2_IDLE ||
> > +				val <= DW_PCIE_LTSSM_DETECT_WAIT,
> > +				PCIE_PME_TO_L2_TIMEOUT_US/10,
> > +				PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> > +	if (ret) {
> > +		/* Only dump message when ltssm_stat isn't in DETECT and POLL */
> > +		dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val);
> > +		return ret;
> >  	}
> >
> > +	/*
> > +	 * Refer to r6.0, sec 5.3.3.2.1, software should wait at least
> > +	 * 100ns after L2/L3 Ready before turning off refclock and
> > +	 * main power. It's harmless too when no endpoint connected.
> > +	 */
> > +	udelay(1);
> > +
> >  	dw_pcie_stop_link(pci);
> >  	if (pci->pp.ops->deinit)
> >  		pci->pp.ops->deinit(&pci->pp);
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> > b/drivers/pci/controller/dwc/pcie-designware.h
> > index 5c14ed2cb91ed..7efcb4af66da3 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -330,6 +330,7 @@ enum dw_pcie_ltssm {
> >  	/* Need to align with PCIE_PORT_DEBUG0 bits 0:5 */
> >  	DW_PCIE_LTSSM_DETECT_QUIET = 0x0,
> >  	DW_PCIE_LTSSM_DETECT_ACT = 0x1,
> > +	DW_PCIE_LTSSM_DETECT_WAIT = 0x6,
> >  	DW_PCIE_LTSSM_L0 = 0x11,
> >  	DW_PCIE_LTSSM_L2_IDLE = 0x15,
> >
> 
> 
> --
> Damien Le Moal
> Western Digital Research