[PATCH v1 1/1] scsi: ufshcd: Allow zero value setting to Auto-Hibernate Timer

[PATCH v1 1/1] scsi: ufshcd: Allow zero value setting to Auto-Hibernate Timer

[Bao D. Nguyen]

The zero value Auto-Hibernate Timer is a valid setting, and it
indicates the Auto-Hibernate feature being disabled. Correctly
support this setting. In addition, when this value is queried
from sysfs, read from the host controller's register and return
that value instead of using the RAM value.

Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufs-sysfs.c | 9 ++++++++-
 drivers/scsi/ufs/ufshcd.c    | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 02d379f00..bdcd27f 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -146,12 +146,19 @@ static u32 ufshcd_us_to_ahit(unsigned int timer)
 static ssize_t auto_hibern8_show(struct device *dev,
 				 struct device_attribute *attr, char *buf)
 {
+	u32 ahit;
 	struct ufs_hba *hba = dev_get_drvdata(dev);
 
 	if (!ufshcd_is_auto_hibern8_supported(hba))
 		return -EOPNOTSUPP;
 
-	return snprintf(buf, PAGE_SIZE, "%d\n", ufshcd_ahit_to_us(hba->ahit));
+	pm_runtime_get_sync(hba->dev);
+	ufshcd_hold(hba, false);
+	ahit = ufshcd_readl(hba, REG_AUTO_HIBERNATE_IDLE_TIMER);
+	ufshcd_release(hba);
+	pm_runtime_put_sync(hba->dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", ufshcd_ahit_to_us(ahit));
 }
 
 static ssize_t auto_hibern8_store(struct device *dev,
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 06e2439..ea5cc33 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3975,7 +3975,7 @@ void ufshcd_auto_hibern8_enable(struct ufs_hba *hba)
 {
 	unsigned long flags;
 
-	if (!ufshcd_is_auto_hibern8_supported(hba) || !hba->ahit)
+	if (!ufshcd_is_auto_hibern8_supported(hba))
 		return;
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 1/1] scsi: ufshcd: Allow zero value setting to Auto-Hibernate Timer

[Bart Van Assche]

On 2020-08-28 18:05, Bao D. Nguyen wrote:
>  static ssize_t auto_hibern8_show(struct device *dev,
>  				 struct device_attribute *attr, char *buf)
>  {
> +	u32 ahit;
>  	struct ufs_hba *hba = dev_get_drvdata(dev);

Although not strictly required for SCSI code, how about following the "reverse
christmas tree" convention and adding "u32 ahit" below the "hba" declaration?

>  	if (!ufshcd_is_auto_hibern8_supported(hba))
>  		return -EOPNOTSUPP;
>  
> -	return snprintf(buf, PAGE_SIZE, "%d\n", ufshcd_ahit_to_us(hba->ahit));
> +	pm_runtime_get_sync(hba->dev);
> +	ufshcd_hold(hba, false);
> +	ahit = ufshcd_readl(hba, REG_AUTO_HIBERNATE_IDLE_TIMER);
> +	ufshcd_release(hba);
> +	pm_runtime_put_sync(hba->dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", ufshcd_ahit_to_us(ahit));
>  }

Why the pm_runtime_get_sync()/pm_runtime_put_sync() and
ufshcd_hold()/ufshcd_release() calls? I don't think these are necessary here.

Thanks,

Bart.

RE: [PATCH v1 1/1] scsi: ufshcd: Allow zero value setting to Auto-Hibernate Timer

[Avri Altman]

 
> 
> The zero value Auto-Hibernate Timer is a valid setting, and it
> indicates the Auto-Hibernate feature being disabled. Correctly
Right. So " ufshcd_auto_hibern8_enable" is no longer an appropriate name.
Maybe ufshcd_auto_hibern8_set instead?

Also, did you verified that no other platform relies on its non-zero value?

Thanks,
Avri

Re: [PATCH v1 1/1] scsi: ufshcd: Allow zero value setting to Auto-Hibernate Timer

[nguyenb]

On 2020-08-28 20:13, Bart Van Assche wrote:
> On 2020-08-28 18:05, Bao D. Nguyen wrote:
>>  static ssize_t auto_hibern8_show(struct device *dev,
>>  				 struct device_attribute *attr, char *buf)
>>  {
>> +	u32 ahit;
>>  	struct ufs_hba *hba = dev_get_drvdata(dev);
> 
> Although not strictly required for SCSI code, how about following the 
> "reverse
> christmas tree" convention and adding "u32 ahit" below the "hba" 
> declaration?
Thanks for your comment. I will change it.
>>  	if (!ufshcd_is_auto_hibern8_supported(hba))
>>  		return -EOPNOTSUPP;
>> 
>> -	return snprintf(buf, PAGE_SIZE, "%d\n", 
>> ufshcd_ahit_to_us(hba->ahit));
>> +	pm_runtime_get_sync(hba->dev);
>> +	ufshcd_hold(hba, false);
>> +	ahit = ufshcd_readl(hba, REG_AUTO_HIBERNATE_IDLE_TIMER);
>> +	ufshcd_release(hba);
>> +	pm_runtime_put_sync(hba->dev);
>> +
>> +	return scnprintf(buf, PAGE_SIZE, "%d\n", ufshcd_ahit_to_us(ahit));
>>  }
> 
> Why the pm_runtime_get_sync()/pm_runtime_put_sync() and
> ufshcd_hold()/ufshcd_release() calls? I don't think these are necessary 
> here.
We may try to access the hardware register during runtime suspend or UFS 
clock is gated.
UFS clock gating can happen even during runtime resume. Here we are 
trying to prevent NoC error
due to unclocked access.
> Thanks,
> 
> Bart.

Re: [PATCH v1 1/1] scsi: ufshcd: Allow zero value setting to Auto-Hibernate Timer

[nguyenb]

On 2020-08-29 00:32, Avri Altman wrote:
>> 
>> The zero value Auto-Hibernate Timer is a valid setting, and it
>> indicates the Auto-Hibernate feature being disabled. Correctly
> Right. So " ufshcd_auto_hibern8_enable" is no longer an appropriate 
> name.
> Maybe ufshcd_auto_hibern8_set instead?
Thanks for your comment. I am ok with the name change suggestion.
> 
> Also, did you verified that no other platform relies on its non-zero 
> value?
I only tested the change on Qualcomm's platform. I do not have other 
platforms to do the test.
The UFS host controller spec JESD220E, Section 5.2.5 says
"Software writes “0” to disable Auto-Hibernate Idle Timer". So the spec 
supports this zero value.
Some options:
- We could add a hba->caps so that we only apply the change for 
Qualcomm's platforms.
This is not preferred because it is following the spec implementations.
- Or other platforms that do not support the zero value needs a caps.
> 
> Thanks,
> Avri

RE: [PATCH v1 1/1] scsi: ufshcd: Allow zero value setting to Auto-Hibernate Timer

[Avri Altman]

 
> 
> On 2020-08-29 00:32, Avri Altman wrote:
> >>
> >> The zero value Auto-Hibernate Timer is a valid setting, and it
> >> indicates the Auto-Hibernate feature being disabled. Correctly
> > Right. So " ufshcd_auto_hibern8_enable" is no longer an appropriate
> > name.
> > Maybe ufshcd_auto_hibern8_set instead?
> Thanks for your comment. I am ok with the name change suggestion.
> >
> > Also, did you verified that no other platform relies on its non-zero
> > value?
> I only tested the change on Qualcomm's platform. I do not have other
> platforms to do the test.
> The UFS host controller spec JESD220E, Section 5.2.5 says
> "Software writes “0” to disable Auto-Hibernate Idle Timer". So the spec
> supports this zero value.
> Some options:
> - We could add a hba->caps so that we only apply the change for
> Qualcomm's platforms.
> This is not preferred because it is following the spec implementations.
> - Or other platforms that do not support the zero value needs a caps.
Yeah, I don't think another caps is required,
Maybe just an ack from Stanley.

Thanks,
Avri

RE: [PATCH v1 1/1] scsi: ufshcd: Allow zero value setting to Auto-Hibernate Timer

[Stanley Chu]

On Wed, 2020-09-02 at 05:10 +0000, Avri Altman wrote:
> > 
> > On 2020-08-29 00:32, Avri Altman wrote:
> > >>
> > >> The zero value Auto-Hibernate Timer is a valid setting, and it
> > >> indicates the Auto-Hibernate feature being disabled. Correctly
> > > Right. So " ufshcd_auto_hibern8_enable" is no longer an appropriate
> > > name.
> > > Maybe ufshcd_auto_hibern8_set instead?
> > Thanks for your comment. I am ok with the name change suggestion.
> > >
> > > Also, did you verified that no other platform relies on its non-zero
> > > value?
> > I only tested the change on Qualcomm's platform. I do not have other
> > platforms to do the test.
> > The UFS host controller spec JESD220E, Section 5.2.5 says
> > "Software writes “0” to disable Auto-Hibernate Idle Timer". So the spec
> > supports this zero value.
> > Some options:
> > - We could add a hba->caps so that we only apply the change for
> > Qualcomm's platforms.
> > This is not preferred because it is following the spec implementations.
> > - Or other platforms that do not support the zero value needs a caps.
> Yeah, I don't think another caps is required,
> Maybe just an ack from Stanley.

Looks good to me.

Acked-by: Stanley Chu <stanley.chu@mediatek.com>

> 
> Thanks,
> Avri

Re: [PATCH v1 1/1] scsi: ufshcd: Allow zero value setting to Auto-Hibernate Timer

[Martin K. Petersen]

Bao,

> The zero value Auto-Hibernate Timer is a valid setting, and it
> indicates the Auto-Hibernate feature being disabled. Correctly support
> this setting. In addition, when this value is queried from sysfs, read
> from the host controller's register and return that value instead of
> using the RAM value.

Applied to my 5.10 staging tree. Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v1 1/1] scsi: ufshcd: Allow zero value setting to Auto-Hibernate Timer

[Martin K. Petersen]

On Fri, 28 Aug 2020 18:05:13 -0700, Bao D. Nguyen wrote:

> The zero value Auto-Hibernate Timer is a valid setting, and it
> indicates the Auto-Hibernate feature being disabled. Correctly
> support this setting. In addition, when this value is queried
> from sysfs, read from the host controller's register and return
> that value instead of using the RAM value.

Applied to 5.10/scsi-queue, thanks!

[1/1] scsi: ufshcd: Allow specifying an Auto-Hibernate Timer value of zero
      https://git.kernel.org/mkp/scsi/c/499f7a966092

-- 
Martin K. Petersen	Oracle Linux Engineering