Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <300cf1ee-3fee-edf9-555a-11440c589ddb@quicinc.com>
Date:   Wed, 24 Aug 2022 09:11:02 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v5 3/3] PCI: qcom: Add retry logic for link to be stable
 in L1ss
Content-Language: en-US
To:     Matthias Kaehlcke <mka@chromium.org>
CC:     <helgaas@kernel.org>, <linux-pci@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <quic_vbadigan@quicinc.com>, <quic_hemantk@quicinc.com>,
        <quic_nitegupt@quicinc.com>, <quic_skananth@quicinc.com>,
        <quic_ramkri@quicinc.com>, <manivannan.sadhasivam@linaro.org>,
        <swboyd@chromium.org>, <dmitry.baryshkov@linaro.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        "Stanimir Varbanov" <svarbanov@mm-sol.com>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Rob Herring <robh@kernel.org>,
        =?UTF-8?Q?Krzysztof_Wilczy=c5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>
References: <1659526134-22978-1-git-send-email-quic_krichai@quicinc.com>
 <1659526134-22978-4-git-send-email-quic_krichai@quicinc.com>
 <Yuw7HyoFX4jk6eOE@google.com>
From:   Krishna Chaitanya Chundru <quic_krichai@quicinc.com>
In-Reply-To: <Yuw7HyoFX4jk6eOE@google.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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.