| Message ID | b141cfcd7998b8933635828b56fbb64f8ad4d175.1598661071.git.nguyenb@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] scsi: ufshcd: Allow zero value setting to Auto-Hibernate Timer | expand |
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. Yeah, I don't think another caps is required, Maybe just an ack from Stanley. Thanks, Avri
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
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);