| Message ID | 20241209073924.2155933-4-hongxing.zhu@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bug fixes when dwc generic suspend/resume callbacks are used | expand |
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, >
> -----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
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,