[v2,0/2] scsi: ufs: Add host capabilities sysfs group

Hi Martin,

This patch series add sysfs entries for the host capabilities registers.
This platform info is otherwise not available. Please consider this
patch series for the next merge window.

Thanks,
Avri

---

Changes in v2:
 - Add sysfs doc
 - replace the pm_runtime_xx by ufshcd_rpm_xx for hci register read

---

Avri Altman (2):
  scsi: ufs: Prepare to add HCI capabilities sysfs
  scsi: ufs: Add HCI capabilities sysfs group

 Documentation/ABI/testing/sysfs-driver-ufs |  48 ++++++++
 drivers/ufs/core/ufs-sysfs.c               | 133 ++++++++++++++++++---
 include/ufs/ufshci.h                       |   5 +-
 3 files changed, 168 insertions(+), 18 deletions(-)

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.