| Message ID | 20241118054447.2470345-1-hongxing.zhu@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] PCI: dwc: Clean up some unnecessary codes in dw_pcie_suspend_noirq() | expand |
On 11/18/2024 11:14 AM, 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. > > Also, when the endpoint is connected and PME_TURN_OFF is sent, do not return > error if the link doesn't enter L2. Just print a warning and continue with the > suspend as the link will be recovered in dw_pcie_resume_noirq(). > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > --- > v3 changes: > - Refine the commit message refer to Manivannan's comments. > - Regarding Frank's comments, avoid 10ms wait when no link up. > v2 changes: > - Don't remove L2 poll check. > - Add one 1us delay after L2 entry. > - No error return when L2 entry is timeout > - Don't print message when no link up. > --- > .../pci/controller/dwc/pcie-designware-host.c | 40 ++++++++++--------- > drivers/pci/controller/dwc/pcie-designware.h | 1 + > 2 files changed, 23 insertions(+), 18 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 24e89b66b772..68fbc16199e8 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -927,24 +927,28 @@ 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 && (val > DW_PCIE_LTSSM_DETECT_WAIT)) > + /* Only dump message when ltssm_stat isn't in DETECT and POLL */ > + dev_warn(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val); we need to return ret here in case we fail to enter L2 when the endpoint is connected. - Krishna Chaitanya. > + else > + /* > + * 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) > @@ -952,7 +956,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci) > > pci->suspended = true; > > - return ret; > + return 0; > } > EXPORT_SYMBOL_GPL(dw_pcie_suspend_noirq); > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 347ab74ac35a..bf036e66717e 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, >
> -----Original Message----- > From: Krishna Chaitanya Chundru <quic_krichai@quicinc.com> > Sent: 2024年11月18日 14:57 > 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> > Cc: imx@lists.linux.dev; kernel@pengutronix.de; linux-pci@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH v3] PCI: dwc: Clean up some unnecessary codes in > dw_pcie_suspend_noirq() > > > > On 11/18/2024 11:14 AM, 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. > > > > Also, when the endpoint is connected and PME_TURN_OFF is sent, do not > > return error if the link doesn't enter L2. Just print a warning and > > continue with the suspend as the link will be recovered in > dw_pcie_resume_noirq(). > > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > --- > > v3 changes: > > - Refine the commit message refer to Manivannan's comments. > > - Regarding Frank's comments, avoid 10ms wait when no link up. > > v2 changes: > > - Don't remove L2 poll check. > > - Add one 1us delay after L2 entry. > > - No error return when L2 entry is timeout > > - Don't print message when no link up. > > --- > > .../pci/controller/dwc/pcie-designware-host.c | 40 ++++++++++--------- > > drivers/pci/controller/dwc/pcie-designware.h | 1 + > > 2 files changed, 23 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c > > b/drivers/pci/controller/dwc/pcie-designware-host.c > > index 24e89b66b772..68fbc16199e8 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > @@ -927,24 +927,28 @@ 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 && (val > DW_PCIE_LTSSM_DETECT_WAIT)) > > + /* Only dump message when ltssm_stat isn't in DETECT and POLL */ > > + dev_warn(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", > > +val); > we need to return ret here in case we fail to enter L2 when the endpoint is > connected. > Hi Krishna: I used encounter the following error, when some NVME devices are used. For example, the "Sandisk SN720 256G NVME SSD disk". "imx6q-pcie 4c300000.pcie: Timeout waiting for L2 entry! LTSSM: 0x19" LTSSM:0x19 means S_DISABLED. Is this an error actually or something else? BTW, without the error return. The NVME disk can be functional again after resume back. Otherwise, system is hang in suspend. Logs with error return when LTSSM is 0x19(v4 patch). rtcwakeup.out: wakeup from "mem" using rtc0 at Fri Jan 2 00:01:02 1970 [ 31.014728] PM: suspend entry (deep) ... [ 31.636729] imx6q-pcie 4c300000.pcie: Timeout waiting for L2 entry! LTSSM: 0x19 [ 31.644191] imx6q-pcie 4c300000.pcie: PM: dpm_run_callback(): genpd_suspend_noirq returns -110 [ 31.652936] imx6q-pcie 4c300000.pcie: PM: failed to suspend noirq: error -110 Logs without error return when LTSSM is 0x19(this v3 patch). rtcwakeup.out: wakeup from "mem" using rtc0 at Fri Jan 2 00:01:02 1970 [ 31.079868] PM: suspend entry (deep) ... [ 31.732253] imx6q-pcie 4c300000.pcie: Timeout waiting for L2 entry! LTSSM: 0x19 [ 31.758051] Disabling non-boot CPUs ... ... [ 31.799148] psci: CPU1 killed (polled 4 ms) [ 31.806229] Enabling non-boot CPUs ... [ 31.810690] Detected VIPT I-cache on CPU1 [ 31.810766] GICv3: CPU1: found redistributor 100 region 0:0x0000000048080000 [ 31.810844] CPU1: Booted secondary processor 0x0000000100 [0x412fd050] [ 31.812365] CPU1 is up ... Best Regards Richard Zhu > - Krishna Chaitanya. > > + else > > + /* > > + * 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) > > @@ -952,7 +956,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci) > > > > pci->suspended = true; > > > > - return ret; > > + return 0; > > } > > EXPORT_SYMBOL_GPL(dw_pcie_suspend_noirq); > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h > > b/drivers/pci/controller/dwc/pcie-designware.h > > index 347ab74ac35a..bf036e66717e 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, > >
On Mon, Nov 18, 2024 at 01:44:47PM +0800, 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. > > Also, when the endpoint is connected and PME_TURN_OFF is sent, do not return > error if the link doesn't enter L2. Just print a warning and continue with the > suspend as the link will be recovered in dw_pcie_resume_noirq(). > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > --- > v3 changes: > - Refine the commit message refer to Manivannan's comments. > - Regarding Frank's comments, avoid 10ms wait when no link up. > v2 changes: > - Don't remove L2 poll check. > - Add one 1us delay after L2 entry. > - No error return when L2 entry is timeout > - Don't print message when no link up. > --- > .../pci/controller/dwc/pcie-designware-host.c | 40 ++++++++++--------- > drivers/pci/controller/dwc/pcie-designware.h | 1 + > 2 files changed, 23 insertions(+), 18 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 24e89b66b772..68fbc16199e8 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -927,24 +927,28 @@ 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 && (val > DW_PCIE_LTSSM_DETECT_WAIT)) if true of (val <= DW_PCIE_LTSSM_DETECT_WAIT), which means not device attached, 'ret' should be 0. when ret is not 0, means, link up and not in L2 idle status. So check '(val > DW_PCIE_LTSSM_DETECT_WAIT)' is redundant. NOT(val == DW_PCIE_LTSSM_L2_IDLE || val <= DW_PCIE_LTSSM_DETECT_WAIT) equal (val != DW_PCIE_LTSSM_L2_IDLE) && (val > DW_PCIE_LTSSM_DETECT_WAIT) Frank > + /* Only dump message when ltssm_stat isn't in DETECT and POLL */ > + dev_warn(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val); > + else > + /* > + * 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) > @@ -952,7 +956,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci) > > pci->suspended = true; > > - return ret; > + return 0; > } > EXPORT_SYMBOL_GPL(dw_pcie_suspend_noirq); > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 347ab74ac35a..bf036e66717e 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 >
> -----Original Message----- > From: Frank Li <frank.li@nxp.com> > Sent: 2024年11月20日 2:10 > To: Hongxing Zhu <hongxing.zhu@nxp.com> > Cc: jingoohan1@gmail.com; bhelgaas@google.com; lpieralisi@kernel.org; > kw@linux.com; manivannan.sadhasivam@linaro.org; robh@kernel.org; > imx@lists.linux.dev; kernel@pengutronix.de; linux-pci@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH v3] PCI: dwc: Clean up some unnecessary codes in > dw_pcie_suspend_noirq() > > On Mon, Nov 18, 2024 at 01:44:47PM +0800, 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. > > > > Also, when the endpoint is connected and PME_TURN_OFF is sent, do not > > return error if the link doesn't enter L2. Just print a warning and > > continue with the suspend as the link will be recovered in > dw_pcie_resume_noirq(). > > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > --- > > v3 changes: > > - Refine the commit message refer to Manivannan's comments. > > - Regarding Frank's comments, avoid 10ms wait when no link up. > > v2 changes: > > - Don't remove L2 poll check. > > - Add one 1us delay after L2 entry. > > - No error return when L2 entry is timeout > > - Don't print message when no link up. > > --- > > .../pci/controller/dwc/pcie-designware-host.c | 40 > > ++++++++++--------- drivers/pci/controller/dwc/pcie-designware.h | > > 1 + > > 2 files changed, 23 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c > > b/drivers/pci/controller/dwc/pcie-designware-host.c > > index 24e89b66b772..68fbc16199e8 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > @@ -927,24 +927,28 @@ 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 && (val > DW_PCIE_LTSSM_DETECT_WAIT)) > > if true of (val <= DW_PCIE_LTSSM_DETECT_WAIT), which means not device > attached, 'ret' should be 0. > > when ret is not 0, means, link up and not in L2 idle status. So check '(val > > DW_PCIE_LTSSM_DETECT_WAIT)' is redundant. > > NOT(val == DW_PCIE_LTSSM_L2_IDLE || val <= > DW_PCIE_LTSSM_DETECT_WAIT) equal (val != DW_PCIE_LTSSM_L2_IDLE) && > (val > DW_PCIE_LTSSM_DETECT_WAIT) I see. So only do the ret value check "if (ret)" is enough. When ret is not 0, link is up, something is wrong in L2 entry, a warning would be printed. If ret is 0, that means L2 entry is done, or no link up at all. Thanks. Best Regards Richard Zhu > > Frank > > > + /* Only dump message when ltssm_stat isn't in DETECT and POLL */ > > + dev_warn(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", > val); > > + else > > + /* > > + * 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) > > @@ -952,7 +956,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci) > > > > pci->suspended = true; > > > > - return ret; > > + return 0; > > } > > EXPORT_SYMBOL_GPL(dw_pcie_suspend_noirq); > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h > > b/drivers/pci/controller/dwc/pcie-designware.h > > index 347ab74ac35a..bf036e66717e 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 > >
On 11/19/2024 2:18 PM, Hongxing Zhu wrote: >> -----Original Message----- >> From: Krishna Chaitanya Chundru <quic_krichai@quicinc.com> >> Sent: 2024年11月18日 14:57 >> 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> >> Cc: imx@lists.linux.dev; kernel@pengutronix.de; linux-pci@vger.kernel.org; >> linux-kernel@vger.kernel.org >> Subject: Re: [PATCH v3] PCI: dwc: Clean up some unnecessary codes in >> dw_pcie_suspend_noirq() >> >> >> >> On 11/18/2024 11:14 AM, 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. >>> >>> Also, when the endpoint is connected and PME_TURN_OFF is sent, do not >>> return error if the link doesn't enter L2. Just print a warning and >>> continue with the suspend as the link will be recovered in >> dw_pcie_resume_noirq(). >>> >>> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> >>> --- >>> v3 changes: >>> - Refine the commit message refer to Manivannan's comments. >>> - Regarding Frank's comments, avoid 10ms wait when no link up. >>> v2 changes: >>> - Don't remove L2 poll check. >>> - Add one 1us delay after L2 entry. >>> - No error return when L2 entry is timeout >>> - Don't print message when no link up. >>> --- >>> .../pci/controller/dwc/pcie-designware-host.c | 40 ++++++++++--------- >>> drivers/pci/controller/dwc/pcie-designware.h | 1 + >>> 2 files changed, 23 insertions(+), 18 deletions(-) >>> >>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c >>> b/drivers/pci/controller/dwc/pcie-designware-host.c >>> index 24e89b66b772..68fbc16199e8 100644 >>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c >>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c >>> @@ -927,24 +927,28 @@ 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 && (val > DW_PCIE_LTSSM_DETECT_WAIT)) >>> + /* Only dump message when ltssm_stat isn't in DETECT and POLL */ >>> + dev_warn(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", >>> +val); >> we need to return ret here in case we fail to enter L2 when the endpoint is >> connected. >> > Hi Krishna: > I used encounter the following error, when some NVME devices are used. > For example, the "Sandisk SN720 256G NVME SSD disk". > "imx6q-pcie 4c300000.pcie: Timeout waiting for L2 entry! LTSSM: 0x19" > LTSSM:0x19 means S_DISABLED. Is this an error actually or something else? > BTW, without the error return. The NVME disk can be functional again after > resume back. Otherwise, system is hang in suspend. > To my knowledge I know two cases which might have happen here. LTSSM state can also enter disabled state if the disable bit in the link control register is set but I don't think that is the case here. Other case might be if DPC is enabled and hardware detects any error in the link can you check if DPC is enabled. IF it is enabled you can try if disabling helps here or not. - Krishna Chaitanya. > Logs with error return when LTSSM is 0x19(v4 patch). > rtcwakeup.out: wakeup from "mem" using rtc0 at Fri Jan 2 00:01:02 1970 > [ 31.014728] PM: suspend entry (deep) > ... > [ 31.636729] imx6q-pcie 4c300000.pcie: Timeout waiting for L2 entry! LTSSM: 0x19 > [ 31.644191] imx6q-pcie 4c300000.pcie: PM: dpm_run_callback(): genpd_suspend_noirq returns -110 > [ 31.652936] imx6q-pcie 4c300000.pcie: PM: failed to suspend noirq: error -110 > > Logs without error return when LTSSM is 0x19(this v3 patch). > rtcwakeup.out: wakeup from "mem" using rtc0 at Fri Jan 2 00:01:02 1970 > [ 31.079868] PM: suspend entry (deep) > ... > [ 31.732253] imx6q-pcie 4c300000.pcie: Timeout waiting for L2 entry! LTSSM: 0x19 > [ 31.758051] Disabling non-boot CPUs ... > ... > [ 31.799148] psci: CPU1 killed (polled 4 ms) > [ 31.806229] Enabling non-boot CPUs ... > [ 31.810690] Detected VIPT I-cache on CPU1 > [ 31.810766] GICv3: CPU1: found redistributor 100 region 0:0x0000000048080000 > [ 31.810844] CPU1: Booted secondary processor 0x0000000100 [0x412fd050] > [ 31.812365] CPU1 is up > ... > > Best Regards > Richard Zhu > >> - Krishna Chaitanya. >>> + else >>> + /* >>> + * 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) >>> @@ -952,7 +956,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci) >>> >>> pci->suspended = true; >>> >>> - return ret; >>> + return 0; >>> } >>> EXPORT_SYMBOL_GPL(dw_pcie_suspend_noirq); >>> >>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h >>> b/drivers/pci/controller/dwc/pcie-designware.h >>> index 347ab74ac35a..bf036e66717e 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, >>>
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 24e89b66b772..68fbc16199e8 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -927,24 +927,28 @@ 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 && (val > DW_PCIE_LTSSM_DETECT_WAIT)) + /* Only dump message when ltssm_stat isn't in DETECT and POLL */ + dev_warn(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n", val); + else + /* + * 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) @@ -952,7 +956,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci) pci->suspended = true; - return ret; + return 0; } EXPORT_SYMBOL_GPL(dw_pcie_suspend_noirq); diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 347ab74ac35a..bf036e66717e 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,
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. Also, when the endpoint is connected and PME_TURN_OFF is sent, do not return error if the link doesn't enter L2. Just print a warning and continue with the suspend as the link will be recovered in dw_pcie_resume_noirq(). Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> --- v3 changes: - Refine the commit message refer to Manivannan's comments. - Regarding Frank's comments, avoid 10ms wait when no link up. v2 changes: - Don't remove L2 poll check. - Add one 1us delay after L2 entry. - No error return when L2 entry is timeout - Don't print message when no link up. --- .../pci/controller/dwc/pcie-designware-host.c | 40 ++++++++++--------- drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 23 insertions(+), 18 deletions(-)