Re: [PATCH v5 3/3] PCI: qcom: Add retry logic for link to be stable in L1ss

[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@xxxxxxxxxxx>
> > ---
> >   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
>
> > +	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;
> 		}
>
> > +
> > +		/* 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.
>
> > +			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;
> 	}

Hi Matthias,

In the v6 patch series we tried to include this logic, but as we are 
going with syscore ops

all the interrupts will be disabled by that time. and this timeout logic 
is enabling interrupts

and this causes the suspend to fail. So going with the previous logic.

Thanks & Regards,

Krishna chaitanya.