| Message ID | 1663669347-29308-3-git-send-email-quic_krichai@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: qcom: Add system suspend & resume support | expand |
On 9/20/2022 3:22 AM, Krishna chaitanya chundru wrote: > When link is in L1ss(L1.1 or L1.2), all the clocks will gate off and there > will be no activity on the link. At that point clocks and phy > can be turned off. If clocks got disabled before link enters > L1ss the PCIe link goes down. > > Few endpoints are taking time more time to settle the link in L1 substates. "time more time" does not parse > When we check the traffic in protocol analyzer, we see some DLLP packets > going on still. So Wait for max time of 200ms for the link to be stable in s/Wait/wait/ > L1 substates. > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > --- > changes since v6: > - updated comments. > --- > drivers/pci/controller/dwc/pcie-qcom.c | 46 ++++++++++++++++++++++++++-------- > 1 file changed, 35 insertions(+), 11 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 3f5424a..7a6f69e 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -1809,23 +1809,47 @@ static int qcom_pcie_probe(struct platform_device *pdev) > static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie) > { > u32 val; > + ktime_t timeout, start; > struct dw_pcie *pci = pcie->pci; > struct device *dev = pci->dev; > > - /* if the link is not active turn off clocks */ > - if (!dw_pcie_link_up(pci)) { > - dev_dbg(dev, "Link is not active\n"); > - goto suspend; > - } > + /* > + * When link is in L1ss, all the clocks will gate off and > + * there will be no activity on the link. At that point clocks > + * and phy can be turned off. If clocks got disabled before > + * link enters L1ss the PCIe link goes down. > + * > + * Few endpoints are taking time more time to settle the link "time more time" does not parse > + * in L1ss. Wait for max of 200ms for the link to be stable in > + * L1ss. > + */ > + start = ktime_get(); > + /* Wait max 200 ms */ > + timeout = ktime_add_ms(start, 200); > + > + while (1) { > + /* if the liink is not active turn off clocks */ > + if (!dw_pcie_link_up(pci)) { > + dev_dbg(dev, "Link is not active\n"); > + break; > + } > > - /* if the link is not in l1ss don't turn off clocks */ > - val = readl(pcie->parf + PCIE20_PARF_PM_STTS); > - if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) { > - dev_warn(dev, "Link is not in L1ss\n"); > - return 0; > + /* if the link is not in l1ss don't turn off clocks */ > + val = readl(pcie->parf + PCIE20_PARF_PM_STTS); > + if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) { > + dev_dbg(dev, "Link enters L1ss after %lld ms\n", > + ktime_to_ms(ktime_get() - start)); > + break; > + } > + > + if (ktime_after(ktime_get(), timeout)) { > + dev_warn(dev, "Link is not in L1ss\n"); > + return 0; > + } > + > + udelay(1000); > } > > -suspend: > if (pcie->cfg->ops->suspend) > pcie->cfg->ops->suspend(pcie); >
On 20-09-22, 15:52, Krishna chaitanya chundru wrote: > When link is in L1ss(L1.1 or L1.2), all the clocks will gate off and there > will be no activity on the link. At that point clocks and phy > can be turned off. If clocks got disabled before link enters > L1ss the PCIe link goes down. > > Few endpoints are taking time more time to settle the link in L1 substates. > When we check the traffic in protocol analyzer, we see some DLLP packets > going on still. So Wait for max time of 200ms for the link to be stable in > L1 substates. > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > --- > changes since v6: > - updated comments. > --- > drivers/pci/controller/dwc/pcie-qcom.c | 46 ++++++++++++++++++++++++++-------- > 1 file changed, 35 insertions(+), 11 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 3f5424a..7a6f69e 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -1809,23 +1809,47 @@ static int qcom_pcie_probe(struct platform_device *pdev) > static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie) > { > u32 val; > + ktime_t timeout, start; > struct dw_pcie *pci = pcie->pci; > struct device *dev = pci->dev; > > - /* if the link is not active turn off clocks */ > - if (!dw_pcie_link_up(pci)) { > - dev_dbg(dev, "Link is not active\n"); > - goto suspend; > - } > + /* > + * When link is in L1ss, all the clocks will gate off and > + * there will be no activity on the link. At that point clocks > + * and phy can be turned off. If clocks got disabled before > + * link enters L1ss the PCIe link goes down. > + * > + * Few endpoints are taking time more time to settle the link > + * in L1ss. Wait for max of 200ms for the link to be stable in > + * L1ss. > + */ > + start = ktime_get(); > + /* Wait max 200 ms */ > + timeout = ktime_add_ms(start, 200); > + > + while (1) { > + /* if the liink is not active turn off clocks */ > + if (!dw_pcie_link_up(pci)) { > + dev_dbg(dev, "Link is not active\n"); > + break; > + } > > - /* if the link is not in l1ss don't turn off clocks */ > - val = readl(pcie->parf + PCIE20_PARF_PM_STTS); > - if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) { > - dev_warn(dev, "Link is not in L1ss\n"); > - return 0; > + /* if the link is not in l1ss don't turn off clocks */ > + val = readl(pcie->parf + PCIE20_PARF_PM_STTS); > + if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) { > + dev_dbg(dev, "Link enters L1ss after %lld ms\n", > + ktime_to_ms(ktime_get() - start)); > + break; > + } > + > + if (ktime_after(ktime_get(), timeout)) { > + dev_warn(dev, "Link is not in L1ss\n"); > + return 0; > + } > + ugh, why not use readl_poll_timeout()? > + udelay(1000); > } > > -suspend: > if (pcie->cfg->ops->suspend) > pcie->cfg->ops->suspend(pcie); > > -- > 2.7.4
On 9/24/2022 11:35 AM, Vinod Koul wrote: > On 20-09-22, 15:52, Krishna chaitanya chundru wrote: >> When link is in L1ss(L1.1 or L1.2), all the clocks will gate off and there >> will be no activity on the link. At that point clocks and phy >> can be turned off. If clocks got disabled before link enters >> L1ss the PCIe link goes down. >> >> Few endpoints are taking time more time to settle the link in L1 substates. >> When we check the traffic in protocol analyzer, we see some DLLP packets >> going on still. So Wait for max time of 200ms for the link to be stable in >> L1 substates. >> >> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >> --- >> changes since v6: >> - updated comments. >> --- >> drivers/pci/controller/dwc/pcie-qcom.c | 46 ++++++++++++++++++++++++++-------- >> 1 file changed, 35 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c >> index 3f5424a..7a6f69e 100644 >> --- a/drivers/pci/controller/dwc/pcie-qcom.c >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c >> @@ -1809,23 +1809,47 @@ static int qcom_pcie_probe(struct platform_device *pdev) >> static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie) >> { >> u32 val; >> + ktime_t timeout, start; >> struct dw_pcie *pci = pcie->pci; >> struct device *dev = pci->dev; >> >> - /* if the link is not active turn off clocks */ >> - if (!dw_pcie_link_up(pci)) { >> - dev_dbg(dev, "Link is not active\n"); >> - goto suspend; >> - } >> + /* >> + * When link is in L1ss, all the clocks will gate off and >> + * there will be no activity on the link. At that point clocks >> + * and phy can be turned off. If clocks got disabled before >> + * link enters L1ss the PCIe link goes down. >> + * >> + * Few endpoints are taking time more time to settle the link >> + * in L1ss. Wait for max of 200ms for the link to be stable in >> + * L1ss. >> + */ >> + start = ktime_get(); >> + /* Wait max 200 ms */ >> + timeout = ktime_add_ms(start, 200); >> + >> + while (1) { >> + /* if the liink is not active turn off clocks */ >> + if (!dw_pcie_link_up(pci)) { >> + dev_dbg(dev, "Link is not active\n"); >> + break; >> + } >> >> - /* if the link is not in l1ss don't turn off clocks */ >> - val = readl(pcie->parf + PCIE20_PARF_PM_STTS); >> - if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) { >> - dev_warn(dev, "Link is not in L1ss\n"); >> - return 0; >> + /* if the link is not in l1ss don't turn off clocks */ >> + val = readl(pcie->parf + PCIE20_PARF_PM_STTS); >> + if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) { >> + dev_dbg(dev, "Link enters L1ss after %lld ms\n", >> + ktime_to_ms(ktime_get() - start)); >> + break; >> + } >> + >> + if (ktime_after(ktime_get(), timeout)) { >> + dev_warn(dev, "Link is not in L1ss\n"); >> + return 0; >> + } >> + > ugh, why not use readl_poll_timeout()? As this is called from the syscore ops, all the interrupts will be disabled by the time the execution reaches here. readl_poll_timeout uses interrupt internally and cause some issues. So we are using this method instead of readl_poll_timeout. > >> + udelay(1000); >> } >> >> -suspend: >> if (pcie->cfg->ops->suspend) >> pcie->cfg->ops->suspend(pcie); >> >> -- >> 2.7.4
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index 3f5424a..7a6f69e 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -1809,23 +1809,47 @@ static int qcom_pcie_probe(struct platform_device *pdev) static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie) { u32 val; + ktime_t timeout, start; struct dw_pcie *pci = pcie->pci; struct device *dev = pci->dev; - /* if the link is not active turn off clocks */ - if (!dw_pcie_link_up(pci)) { - dev_dbg(dev, "Link is not active\n"); - goto suspend; - } + /* + * When link is in L1ss, all the clocks will gate off and + * there will be no activity on the link. At that point clocks + * and phy can be turned off. If clocks got disabled before + * link enters L1ss the PCIe link goes down. + * + * Few endpoints are taking time more time to settle the link + * in L1ss. Wait for max of 200ms for the link to be stable in + * L1ss. + */ + start = ktime_get(); + /* Wait max 200 ms */ + timeout = ktime_add_ms(start, 200); + + while (1) { + /* if the liink is not active turn off clocks */ + if (!dw_pcie_link_up(pci)) { + dev_dbg(dev, "Link is not active\n"); + break; + } - /* if the link is not in l1ss don't turn off clocks */ - val = readl(pcie->parf + PCIE20_PARF_PM_STTS); - if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) { - dev_warn(dev, "Link is not in L1ss\n"); - return 0; + /* if the link is not in l1ss don't turn off clocks */ + val = readl(pcie->parf + PCIE20_PARF_PM_STTS); + if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) { + dev_dbg(dev, "Link enters L1ss after %lld ms\n", + ktime_to_ms(ktime_get() - start)); + break; + } + + if (ktime_after(ktime_get(), timeout)) { + dev_warn(dev, "Link is not in L1ss\n"); + return 0; + } + + udelay(1000); } -suspend: if (pcie->cfg->ops->suspend) pcie->cfg->ops->suspend(pcie);
When link is in L1ss(L1.1 or L1.2), all the clocks will gate off and there will be no activity on the link. At that point clocks and phy can be turned off. If clocks got disabled before link enters L1ss the PCIe link goes down. Few endpoints are taking time more time to settle the link in L1 substates. When we check the traffic in protocol analyzer, we see some DLLP packets going on still. So Wait for max time of 200ms for the link to be stable in L1 substates. Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> --- changes since v6: - updated comments. --- drivers/pci/controller/dwc/pcie-qcom.c | 46 ++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 11 deletions(-)