Messages in this thread | | | Date | Wed, 24 Aug 2022 09:11:02 +0530 | Subject | Re: [PATCH v5 3/3] PCI: qcom: Add retry logic for link to be stable in L1ss | From | Krishna Chaitanya Chundru <> |
| |
On 8/5/2022 3:03 AM, Matthias Kaehlcke wrote: > On Wed, Aug 03, 2022 at 04:58:54PM +0530, Krishna chaitanya chundru wrote: >> Some specific devices are taking time to settle the link in L1ss. >> So added a retry logic before returning from the suspend op. >> >> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >> --- >> drivers/pci/controller/dwc/pcie-qcom.c | 25 ++++++++++++++++++++----- >> 1 file changed, 20 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c >> index f7dd5dc..f3201bd 100644 >> --- a/drivers/pci/controller/dwc/pcie-qcom.c >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c >> @@ -1829,15 +1829,30 @@ static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev) >> { >> struct qcom_pcie *pcie = dev_get_drvdata(dev); >> u32 val; >> + ktime_t timeout, start; >> >> if (!pcie->cfg->supports_system_suspend) >> 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_warn(dev, "Link is not in L1ss\n"); >> - return 0; >> + start = ktime_get(); >> + /* Wait max 100 ms */ >> + timeout = ktime_add_ms(start, 100); > In my tests 100 ms is ample margin for most NVMe models (it's often 0 and > generally < 10), however with one model I saw delays of up to 150 ms, so > this should probably be 200 ms or so (it's a long time, but most of the > time the actual delay is significantly lower ok I will increase the time to 200. > >> + while (1) { >> + bool timedout = ktime_after(ktime_get(), timeout); > 'timedout' looks very similar to the other local variable 'timeout' > in this function. Actually why not just do without the new variable and > put this after reading the register. > > if (ktime_after(ktime_get(), timeout)) { > dev_warn(dev, "Link is not in L1ss\n"); > return 0; > } ok sure will update in the next patch. >> + >> + /* 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_info(dev, "Link enters L1ss after %d ms\n", >> + ktime_to_ms(ktime_get() - start)); > > Probably this should be dev_dbg() to avoid cluttering the kernel log that > isn't relevant most of the time. ok sure will update in next patch. > >> + break; >> + } >> + >> + if (timedout) { >> + dev_warn(dev, "Link is not in L1ss\n"); >> + return 0; >> + } >> + usleep_range(1000, 1200); > You could use fsleep() instead of specifying a range. > > Based on my testing I think a slightly higher delay like 5ms wouldn't hurt. > That would result in less 'busy looping' for slower NVMes and would still > be reasonable fast for those that need 10 ms or so. > > Actually you could replace the entire loop with something like this: > > if (readl_poll_timeout(pcie->parf + PCIE20_PARF_PM_STTS, val, > val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB, 5000, 200000) { > dev_warn(dev, "Link is not in L1ss\n"); > return 0; > }
Ok we will look in to this option and will update the patch if needed.
| |