diff mbox series

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

Message ID b141cfcd7998b8933635828b56fbb64f8ad4d175.1598661071.git.nguyenb@codeaurora.org (mailing list archive)
State Accepted
Headers show
Series [v1,1/1] scsi: ufshcd: Allow zero value setting to Auto-Hibernate Timer | expand

Commit Message

Bao D. Nguyen Aug. 29, 2020, 1:05 a.m. UTC
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(-)

Comments

Bart Van Assche Aug. 29, 2020, 3:13 a.m. UTC | #1
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.
Avri Altman Aug. 29, 2020, 7:32 a.m. UTC | #2
> 
> 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
Bao D. Nguyen Aug. 31, 2020, 5:38 p.m. UTC | #3
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.
Bao D. Nguyen Aug. 31, 2020, 6:07 p.m. UTC | #4
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
Avri Altman Sept. 2, 2020, 5:10 a.m. UTC | #5
> 
> 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
Stanley Chu Sept. 4, 2020, 1:39 a.m. UTC | #6
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
Martin K. Petersen Sept. 9, 2020, 2:03 a.m. UTC | #7
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 Sept. 15, 2020, 8:16 p.m. UTC | #8
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
diff mbox series

Patch

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);