| Message ID | b141cfcd7998b8933635828b56fbb64f8ad4d175.1598661071.git.nguyenb@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 499f7a9660926c9176563bf8da22abf585b65e65 |
| 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-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. > > 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 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
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!
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 --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);