| Message ID | 20240804072109.2330880-2-avri.altman@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: ufs: Add host capabilities sysfs group | expand |
Hi Avri, > Prepare so we'll be able to read various other HCI registers. > While at it, fix the HCPID & HCMID register names to stand for what they > really are. Also replace the pm_runtime_{get/put}_sync() calls in > auto_hibern8_show to ufshcd_rpm_{get/put}_sync() as any host controller > register reads should. > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > --- > drivers/ufs/core/ufs-sysfs.c | 38 +++++++++++++++++++++--------------- > include/ufs/ufshci.h | 5 +++-- > 2 files changed, 25 insertions(+), 18 deletions(-) > > diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c > index e80a32421a8c..dec7746c98e0 100644 > --- a/drivers/ufs/core/ufs-sysfs.c > +++ b/drivers/ufs/core/ufs-sysfs.c > @@ -198,6 +198,24 @@ static u32 ufshcd_us_to_ahit(unsigned int timer) > FIELD_PREP(UFSHCI_AHIBERN8_SCALE_MASK, scale); > } > > +static int ufshcd_read_hci_reg(struct ufs_hba *hba, u32 *val, unsigned int reg) > +{ > + down(&hba->host_sem); > + if (!ufshcd_is_user_access_allowed(hba)) { > + up(&hba->host_sem); > + return -EBUSY; > + } > + > + ufshcd_rpm_get_sync(hba); > + ufshcd_hold(hba); > + *val = ufshcd_readl(hba, reg); > + ufshcd_release(hba); > + ufshcd_rpm_put_sync(hba); > + > + up(&hba->host_sem); > + return 0; > +} > + > static ssize_t auto_hibern8_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > @@ -208,23 +226,11 @@ static ssize_t auto_hibern8_show(struct device *dev, > if (!ufshcd_is_auto_hibern8_supported(hba)) > return -EOPNOTSUPP; > > - down(&hba->host_sem); > - if (!ufshcd_is_user_access_allowed(hba)) { > - ret = -EBUSY; > - goto out; > - } > - > - pm_runtime_get_sync(hba->dev); > - ufshcd_hold(hba); > - ahit = ufshcd_readl(hba, REG_AUTO_HIBERNATE_IDLE_TIMER); > - ufshcd_release(hba); > - pm_runtime_put_sync(hba->dev); > - > - ret = sysfs_emit(buf, "%d\n", ufshcd_ahit_to_us(ahit)); > + ret = ufshcd_read_hci_reg(hba, &ahit, REG_AUTO_HIBERNATE_IDLE_TIMER); > + if (ret) > + return ret; > > -out: > - up(&hba->host_sem); > - return ret; > + return sysfs_emit(buf, "%d\n", ufshcd_ahit_to_us(ahit)); > } > > static ssize_t auto_hibern8_store(struct device *dev, > diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h > index 38fe97971a65..194e3655902e 100644 > --- a/include/ufs/ufshci.h > +++ b/include/ufs/ufshci.h > @@ -25,8 +25,9 @@ enum { > REG_CONTROLLER_CAPABILITIES = 0x00, > REG_MCQCAP = 0x04, > REG_UFS_VERSION = 0x08, > - REG_CONTROLLER_DEV_ID = 0x10, > - REG_CONTROLLER_PROD_ID = 0x14, > + REG_EXT_CONTROLLER_CAPABILITIES = 0x0C, > + REG_CONTROLLER_PID = 0x10, > + REG_CONTROLLER_MID = 0x14, > REG_AUTO_HIBERNATE_IDLE_TIMER = 0x18, > REG_INTERRUPT_STATUS = 0x20, > REG_INTERRUPT_ENABLE = 0x24, > -- > 2.25.1 Looks good to me. Reviewed-by: Keoseong Park <keosung.park@samsung.com> Best Regards, Keoseong
On 8/4/24 12:21 AM, Avri Altman wrote: > - up(&hba->host_sem); > - return ret; > + return sysfs_emit(buf, "%d\n", ufshcd_ahit_to_us(ahit)); > } All ufshcd_read_hci_reg() callers call sysfs_emit(). How about renaming ufshcd_read_hci_reg() into ufshcd_show_hci_reg(), adding an argument that indicates how the result should be formatted and moving the sysfs_emit() call into ufshcd_show_hci_reg()? Thanks, Bart.
> On 8/4/24 12:21 AM, Avri Altman wrote: > > - up(&hba->host_sem); > > - return ret; > > + return sysfs_emit(buf, "%d\n", ufshcd_ahit_to_us(ahit)); > > } > > All ufshcd_read_hci_reg() callers call sysfs_emit(). How about renaming > ufshcd_read_hci_reg() into ufshcd_show_hci_reg(), adding an argument that > indicates how the result should be formatted and moving the > sysfs_emit() call into ufshcd_show_hci_reg()? Yes, but with the cost of: - complication - You would need to attend the extra processing e.g. if ufs4.0 or as in hibern8 ahit_to_us(), - readability - read_hci_reg does just that (reading), and nothing more - breaks the _show _store convention that one would expect from a sysfs entry Wouldn't keep it simple be better? Thanks, Avri > > Thanks, > > Bart.
On 8/4/24 12:21 AM, Avri Altman wrote: > Prepare so we'll be able to read various other HCI registers. > While at it, fix the HCPID & HCMID register names to stand for what they > really are. Also replace the pm_runtime_{get/put}_sync() calls in > auto_hibern8_show to ufshcd_rpm_{get/put}_sync() as any host controller > register reads should. Reviewed-by: Bart Van Assche <bvanassche@acm.org>
diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c index e80a32421a8c..dec7746c98e0 100644 --- a/drivers/ufs/core/ufs-sysfs.c +++ b/drivers/ufs/core/ufs-sysfs.c @@ -198,6 +198,24 @@ static u32 ufshcd_us_to_ahit(unsigned int timer) FIELD_PREP(UFSHCI_AHIBERN8_SCALE_MASK, scale); } +static int ufshcd_read_hci_reg(struct ufs_hba *hba, u32 *val, unsigned int reg) +{ + down(&hba->host_sem); + if (!ufshcd_is_user_access_allowed(hba)) { + up(&hba->host_sem); + return -EBUSY; + } + + ufshcd_rpm_get_sync(hba); + ufshcd_hold(hba); + *val = ufshcd_readl(hba, reg); + ufshcd_release(hba); + ufshcd_rpm_put_sync(hba); + + up(&hba->host_sem); + return 0; +} + static ssize_t auto_hibern8_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -208,23 +226,11 @@ static ssize_t auto_hibern8_show(struct device *dev, if (!ufshcd_is_auto_hibern8_supported(hba)) return -EOPNOTSUPP; - down(&hba->host_sem); - if (!ufshcd_is_user_access_allowed(hba)) { - ret = -EBUSY; - goto out; - } - - pm_runtime_get_sync(hba->dev); - ufshcd_hold(hba); - ahit = ufshcd_readl(hba, REG_AUTO_HIBERNATE_IDLE_TIMER); - ufshcd_release(hba); - pm_runtime_put_sync(hba->dev); - - ret = sysfs_emit(buf, "%d\n", ufshcd_ahit_to_us(ahit)); + ret = ufshcd_read_hci_reg(hba, &ahit, REG_AUTO_HIBERNATE_IDLE_TIMER); + if (ret) + return ret; -out: - up(&hba->host_sem); - return ret; + return sysfs_emit(buf, "%d\n", ufshcd_ahit_to_us(ahit)); } static ssize_t auto_hibern8_store(struct device *dev, diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h index 38fe97971a65..194e3655902e 100644 --- a/include/ufs/ufshci.h +++ b/include/ufs/ufshci.h @@ -25,8 +25,9 @@ enum { REG_CONTROLLER_CAPABILITIES = 0x00, REG_MCQCAP = 0x04, REG_UFS_VERSION = 0x08, - REG_CONTROLLER_DEV_ID = 0x10, - REG_CONTROLLER_PROD_ID = 0x14, + REG_EXT_CONTROLLER_CAPABILITIES = 0x0C, + REG_CONTROLLER_PID = 0x10, + REG_CONTROLLER_MID = 0x14, REG_AUTO_HIBERNATE_IDLE_TIMER = 0x18, REG_INTERRUPT_STATUS = 0x20, REG_INTERRUPT_ENABLE = 0x24,
Prepare so we'll be able to read various other HCI registers. While at it, fix the HCPID & HCMID register names to stand for what they really are. Also replace the pm_runtime_{get/put}_sync() calls in auto_hibern8_show to ufshcd_rpm_{get/put}_sync() as any host controller register reads should. Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/ufs/core/ufs-sysfs.c | 38 +++++++++++++++++++++--------------- include/ufs/ufshci.h | 5 +++-- 2 files changed, 25 insertions(+), 18 deletions(-)