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 <[email protected]>
Signed-off-by: Asutosh Das <[email protected]>
Signed-off-by: Can Guo <[email protected]>
---
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
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.
>
> 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
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
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.
>
> 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
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 <[email protected]>
>
> Thanks,
> Avri
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
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