[PATCH v3 0/2] scsi: ufs: Add host capabilities sysfs group

[PATCH v3 0/2] scsi: ufs: Add host capabilities sysfs group

[Avri Altman]

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 v3:
 - Fix path in sysfs doc
 - Fix spelling mistake

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 |  42 +++++++
 drivers/ufs/core/ufs-sysfs.c               | 133 ++++++++++++++++++---
 include/ufs/ufshci.h                       |   5 +-
 3 files changed, 162 insertions(+), 18 deletions(-)

-- 
2.25.1

[PATCH v3 1/2] scsi: ufs: Prepare to add HCI capabilities sysfs

[Avri Altman]

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: Keoseong Park <keosung.park@samsung.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
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

[PATCH v3 2/2] scsi: ufs: Add HCI capabilities sysfs group

[Avri Altman]

The standard register map of UFSHCI is comprised of several groups.  The
first group (starting from offset 0x00), is the host capabilities group.
It contains some interesting information, that otherwise is not
available, e.g. the UFS version of the platform etc.

Reviewed-by: Keoseong Park <keosung.park@samsung.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 Documentation/ABI/testing/sysfs-driver-ufs | 42 ++++++++++
 drivers/ufs/core/ufs-sysfs.c               | 95 ++++++++++++++++++++++
 2 files changed, 137 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index fe943ce76c60..b6e0c3b806fd 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -1532,3 +1532,45 @@ Contact:	Bean Huo <beanhuo@micron.com>
 Description:
 		rtc_update_ms indicates how often the host should synchronize or update the
 		UFS RTC. If set to 0, this will disable UFS RTC periodic update.
+
+What:		/sys/devices/platform/.../ufshci_capabilities/capabilities
+Date:		August 2024
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:
+		Host Capabilities register group: host controller capabilities register.
+		Symbol - CAP.  Offset: 0x00 - 0x03.
+
+What:		/sys/devices/platform/.../ufshci_capabilities/mcq_cap
+Date:		August 2024
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:
+		Host Capabilities register group: multi-circular queue capability register.
+		Symbol - MCQCAP.  Offset: 0x04 - 0x07.
+
+What:		/sys/devices/platform/.../ufshci_capabilities/version
+Date:		August 2024
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:
+		Host Capabilities register group: UFS version register.
+		Symbol - VER.  Offset: 0x08 - 0x0B.
+
+What:		/sys/devices/platform/.../ufshci_capabilities/ext_capabilities
+Date:		August 2024
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:
+		Host Capabilities register group: extended controller capabilities register.
+		Symbol - EXT_CAP.  Offset: 0x0C - 0x0F.
+
+What:		/sys/devices/platform/.../ufshci_capabilities/product_id
+Date:		August 2024
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:
+		Host Capabilities register group: product ID register.
+		Symbol - HCPID.  Offset: 0x10 - 0x13.
+
+What:		/sys/devices/platform/.../ufshci_capabilities/man_id
+Date:		August 2024
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:
+		Host Capabilities register group: manufacturer ID register.
+		Symbol - HCMID.  Offset: 0x14 - 0x17.
diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index dec7746c98e0..751d5ff406da 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -525,6 +525,100 @@ static const struct attribute_group ufs_sysfs_capabilities_group = {
 	.attrs = ufs_sysfs_capabilities_attrs,
 };
 
+static ssize_t capabilities_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "0x%x\n", hba->capabilities);
+}
+
+static ssize_t mcq_cap_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	if (hba->ufs_version < ufshci_version(4, 0))
+		return -EOPNOTSUPP;
+
+	return sysfs_emit(buf, "0x%x\n", hba->mcq_capabilities);
+}
+
+static ssize_t version_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "0x%x\n", hba->ufs_version);
+}
+
+static ssize_t ext_capabilities_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	int ret;
+	u32 val;
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	if (hba->ufs_version < ufshci_version(4, 0))
+		return -EOPNOTSUPP;
+
+	ret = ufshcd_read_hci_reg(hba, &val, REG_EXT_CONTROLLER_CAPABILITIES);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "0x%x\n", val);
+}
+
+static ssize_t product_id_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	int ret;
+	u32 val;
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	ret = ufshcd_read_hci_reg(hba, &val, REG_CONTROLLER_PID);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "0x%x\n", val);
+}
+
+static ssize_t man_id_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	int ret;
+	u32 val;
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	ret = ufshcd_read_hci_reg(hba, &val, REG_CONTROLLER_MID);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "0x%x\n", val);
+}
+
+static DEVICE_ATTR_RO(capabilities);
+static DEVICE_ATTR_RO(mcq_cap);
+static DEVICE_ATTR_RO(version);
+static DEVICE_ATTR_RO(ext_capabilities);
+static DEVICE_ATTR_RO(product_id);
+static DEVICE_ATTR_RO(man_id);
+
+static struct attribute *ufs_sysfs_ufshci_cap_attrs[] = {
+	&dev_attr_capabilities.attr,
+	&dev_attr_mcq_cap.attr,
+	&dev_attr_version.attr,
+	&dev_attr_ext_capabilities.attr,
+	&dev_attr_product_id.attr,
+	&dev_attr_man_id.attr,
+	NULL
+};
+
+static const struct attribute_group ufs_sysfs_ufshci_group = {
+	.name = "ufshci_capabilities",
+	.attrs = ufs_sysfs_ufshci_cap_attrs,
+};
+
 static ssize_t monitor_enable_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
@@ -1508,6 +1602,7 @@ static const struct attribute_group ufs_sysfs_attributes_group = {
 static const struct attribute_group *ufs_sysfs_groups[] = {
 	&ufs_sysfs_default_group,
 	&ufs_sysfs_capabilities_group,
+	&ufs_sysfs_ufshci_group,
 	&ufs_sysfs_monitor_group,
 	&ufs_sysfs_power_info_group,
 	&ufs_sysfs_device_descriptor_group,
-- 
2.25.1

Re: [PATCH v3 2/2] scsi: ufs: Add HCI capabilities sysfs group

[Manivannan Sadhasivam]

On Fri, Aug 09, 2024 at 10:23:31AM +0300, Avri Altman wrote:
> The standard register map of UFSHCI is comprised of several groups.  The
> first group (starting from offset 0x00), is the host capabilities group.
> It contains some interesting information, that otherwise is not
> available, e.g. the UFS version of the platform etc.
> 
> Reviewed-by: Keoseong Park <keosung.park@samsung.com>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: Avri Altman <avri.altman@wdc.com>
> ---
>  Documentation/ABI/testing/sysfs-driver-ufs | 42 ++++++++++
>  drivers/ufs/core/ufs-sysfs.c               | 95 ++++++++++++++++++++++
>  2 files changed, 137 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
> index fe943ce76c60..b6e0c3b806fd 100644
> --- a/Documentation/ABI/testing/sysfs-driver-ufs
> +++ b/Documentation/ABI/testing/sysfs-driver-ufs
> @@ -1532,3 +1532,45 @@ Contact:	Bean Huo <beanhuo@micron.com>
>  Description:
>  		rtc_update_ms indicates how often the host should synchronize or update the
>  		UFS RTC. If set to 0, this will disable UFS RTC periodic update.
> +
> +What:		/sys/devices/platform/.../ufshci_capabilities/capabilities
> +Date:		August 2024
> +Contact:	Avri Altman <avri.altman@wdc.com>
> +Description:
> +		Host Capabilities register group: host controller capabilities register.
> +		Symbol - CAP.  Offset: 0x00 - 0x03.

This doesn't look like an ABI description. You are merely specifying the
register name and offset that gets accessed while reading this attribute.

Also, I'm not sure if we really want to expose HCI/MCQ capabilities as sysfs
ABI. This just prints the hex value without even telling users how to interpret
it. 

> +
> +What:		/sys/devices/platform/.../ufshci_capabilities/mcq_cap
> +Date:		August 2024
> +Contact:	Avri Altman <avri.altman@wdc.com>
> +Description:
> +		Host Capabilities register group: multi-circular queue capability register.
> +		Symbol - MCQCAP.  Offset: 0x04 - 0x07.
> +
> +What:		/sys/devices/platform/.../ufshci_capabilities/version
> +Date:		August 2024
> +Contact:	Avri Altman <avri.altman@wdc.com>
> +Description:
> +		Host Capabilities register group: UFS version register.
> +		Symbol - VER.  Offset: 0x08 - 0x0B.

This and below attributes are fine. But the description should be changed. No
need to put register name and offset here, that is not relevant to the ABI.
Description should clearly state what is the purpose of the attribute, how to
interpret (if necessary) and read/write capability. Like,

```
	This file shows the UFSHCD version.

	The file is read only.
```

Applies to other attributes below.

- Mani

> +
> +What:		/sys/devices/platform/.../ufshci_capabilities/ext_capabilities
> +Date:		August 2024
> +Contact:	Avri Altman <avri.altman@wdc.com>
> +Description:
> +		Host Capabilities register group: extended controller capabilities register.
> +		Symbol - EXT_CAP.  Offset: 0x0C - 0x0F.
> +
> +What:		/sys/devices/platform/.../ufshci_capabilities/product_id
> +Date:		August 2024
> +Contact:	Avri Altman <avri.altman@wdc.com>
> +Description:
> +		Host Capabilities register group: product ID register.
> +		Symbol - HCPID.  Offset: 0x10 - 0x13.
> +
> +What:		/sys/devices/platform/.../ufshci_capabilities/man_id
> +Date:		August 2024
> +Contact:	Avri Altman <avri.altman@wdc.com>
> +Description:
> +		Host Capabilities register group: manufacturer ID register.
> +		Symbol - HCMID.  Offset: 0x14 - 0x17.
> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
> index dec7746c98e0..751d5ff406da 100644
> --- a/drivers/ufs/core/ufs-sysfs.c
> +++ b/drivers/ufs/core/ufs-sysfs.c
> @@ -525,6 +525,100 @@ static const struct attribute_group ufs_sysfs_capabilities_group = {
>  	.attrs = ufs_sysfs_capabilities_attrs,
>  };
>  
> +static ssize_t capabilities_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "0x%x\n", hba->capabilities);
> +}
> +
> +static ssize_t mcq_cap_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> +	if (hba->ufs_version < ufshci_version(4, 0))
> +		return -EOPNOTSUPP;
> +
> +	return sysfs_emit(buf, "0x%x\n", hba->mcq_capabilities);
> +}
> +
> +static ssize_t version_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "0x%x\n", hba->ufs_version);
> +}
> +
> +static ssize_t ext_capabilities_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	int ret;
> +	u32 val;
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> +	if (hba->ufs_version < ufshci_version(4, 0))
> +		return -EOPNOTSUPP;
> +
> +	ret = ufshcd_read_hci_reg(hba, &val, REG_EXT_CONTROLLER_CAPABILITIES);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "0x%x\n", val);
> +}
> +
> +static ssize_t product_id_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	int ret;
> +	u32 val;
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> +	ret = ufshcd_read_hci_reg(hba, &val, REG_CONTROLLER_PID);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "0x%x\n", val);
> +}
> +
> +static ssize_t man_id_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	int ret;
> +	u32 val;
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> +	ret = ufshcd_read_hci_reg(hba, &val, REG_CONTROLLER_MID);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "0x%x\n", val);
> +}
> +
> +static DEVICE_ATTR_RO(capabilities);
> +static DEVICE_ATTR_RO(mcq_cap);
> +static DEVICE_ATTR_RO(version);
> +static DEVICE_ATTR_RO(ext_capabilities);
> +static DEVICE_ATTR_RO(product_id);
> +static DEVICE_ATTR_RO(man_id);
> +
> +static struct attribute *ufs_sysfs_ufshci_cap_attrs[] = {
> +	&dev_attr_capabilities.attr,
> +	&dev_attr_mcq_cap.attr,
> +	&dev_attr_version.attr,
> +	&dev_attr_ext_capabilities.attr,
> +	&dev_attr_product_id.attr,
> +	&dev_attr_man_id.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group ufs_sysfs_ufshci_group = {
> +	.name = "ufshci_capabilities",
> +	.attrs = ufs_sysfs_ufshci_cap_attrs,
> +};
> +
>  static ssize_t monitor_enable_show(struct device *dev,
>  				   struct device_attribute *attr, char *buf)
>  {
> @@ -1508,6 +1602,7 @@ static const struct attribute_group ufs_sysfs_attributes_group = {
>  static const struct attribute_group *ufs_sysfs_groups[] = {
>  	&ufs_sysfs_default_group,
>  	&ufs_sysfs_capabilities_group,
> +	&ufs_sysfs_ufshci_group,
>  	&ufs_sysfs_monitor_group,
>  	&ufs_sysfs_power_info_group,
>  	&ufs_sysfs_device_descriptor_group,
> -- 
> 2.25.1
> 
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 1/2] scsi: ufs: Prepare to add HCI capabilities sysfs

[Bean Huo]

On Fri, 2024-08-09 at 10:23 +0300, 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: Keoseong Park <keosung.park@samsung.com>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>

Re: [PATCH v3 2/2] scsi: ufs: Add HCI capabilities sysfs group

[Bean Huo]

On Fri, 2024-08-09 at 10:23 +0300, Avri Altman wrote:
> The standard register map of UFSHCI is comprised of several groups. 
> The
> first group (starting from offset 0x00), is the host capabilities
> group.
> It contains some interesting information, that otherwise is not
> available, e.g. the UFS version of the platform etc.
> 
> Reviewed-by: Keoseong Park <keosung.park@samsung.com>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>

RE: [PATCH v3 2/2] scsi: ufs: Add HCI capabilities sysfs group

[Avri Altman]

> On Fri, Aug 09, 2024 at 10:23:31AM +0300, Avri Altman wrote:
> > The standard register map of UFSHCI is comprised of several groups.
> > The first group (starting from offset 0x00), is the host capabilities group.
> > It contains some interesting information, that otherwise is not
> > available, e.g. the UFS version of the platform etc.
> >
> > Reviewed-by: Keoseong Park <keosung.park@samsung.com>
> > Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> > Signed-off-by: Avri Altman <avri.altman@wdc.com>
> > ---
> >  Documentation/ABI/testing/sysfs-driver-ufs | 42 ++++++++++
> >  drivers/ufs/core/ufs-sysfs.c               | 95 ++++++++++++++++++++++
> >  2 files changed, 137 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-driver-ufs
> > b/Documentation/ABI/testing/sysfs-driver-ufs
> > index fe943ce76c60..b6e0c3b806fd 100644
> > --- a/Documentation/ABI/testing/sysfs-driver-ufs
> > +++ b/Documentation/ABI/testing/sysfs-driver-ufs
> > @@ -1532,3 +1532,45 @@ Contact:       Bean Huo <beanhuo@micron.com>
> >  Description:
> >               rtc_update_ms indicates how often the host should synchronize or
> update the
> >               UFS RTC. If set to 0, this will disable UFS RTC periodic update.
> > +
> > +What:                /sys/devices/platform/.../ufshci_capabilities/capabilities
> > +Date:                August 2024
> > +Contact:     Avri Altman <avri.altman@wdc.com>
> > +Description:
> > +             Host Capabilities register group: host controller capabilities register.
> > +             Symbol - CAP.  Offset: 0x00 - 0x03.
> 
> This doesn't look like an ABI description. You are merely specifying the register
> name and offset that gets accessed while reading this attribute.
> 
> Also, I'm not sure if we really want to expose HCI/MCQ capabilities as sysfs ABI.
Why not?
Like the cover letter say, this info is otherwise unavailable.

> This just prints the hex value without even telling users how to interpret it.
Each register typically carries several parts of info encoded withing its 32 bits.
I am not sure that copying the spec into the documentation is useful.

> 
> > +
> > +What:                /sys/devices/platform/.../ufshci_capabilities/mcq_cap
> > +Date:                August 2024
> > +Contact:     Avri Altman <avri.altman@wdc.com>
> > +Description:
> > +             Host Capabilities register group: multi-circular queue capability
> register.
> > +             Symbol - MCQCAP.  Offset: 0x04 - 0x07.
> > +
> > +What:                /sys/devices/platform/.../ufshci_capabilities/version
> > +Date:                August 2024
> > +Contact:     Avri Altman <avri.altman@wdc.com>
> > +Description:
> > +             Host Capabilities register group: UFS version register.
> > +             Symbol - VER.  Offset: 0x08 - 0x0B.
> 
> This and below attributes are fine.
Still fail to see the difference.

>But the description should be changed. No
> need to put register name and offset here, that is not relevant to the ABI.
> Description should clearly state what is the purpose of the attribute, how to
> interpret (if necessary) and read/write capability. Like,
> 
> ```
>         This file shows the UFSHCD version.
> 
>         The file is read only.
> ```
> 
> Applies to other attributes below.
OK.  Thanks.

Avri

> 
> - Mani
> 
> > +
> > +What:                /sys/devices/platform/.../ufshci_capabilities/ext_capabilities
> > +Date:                August 2024
> > +Contact:     Avri Altman <avri.altman@wdc.com>
> > +Description:
> > +             Host Capabilities register group: extended controller capabilities
> register.
> > +             Symbol - EXT_CAP.  Offset: 0x0C - 0x0F.
> > +
> > +What:                /sys/devices/platform/.../ufshci_capabilities/product_id
> > +Date:                August 2024
> > +Contact:     Avri Altman <avri.altman@wdc.com>
> > +Description:
> > +             Host Capabilities register group: product ID register.
> > +             Symbol - HCPID.  Offset: 0x10 - 0x13.
> > +
> > +What:                /sys/devices/platform/.../ufshci_capabilities/man_id
> > +Date:                August 2024
> > +Contact:     Avri Altman <avri.altman@wdc.com>
> > +Description:
> > +             Host Capabilities register group: manufacturer ID register.
> > +             Symbol - HCMID.  Offset: 0x14 - 0x17.
> > diff --git a/drivers/ufs/core/ufs-sysfs.c
> > b/drivers/ufs/core/ufs-sysfs.c index dec7746c98e0..751d5ff406da 100644
> > --- a/drivers/ufs/core/ufs-sysfs.c
> > +++ b/drivers/ufs/core/ufs-sysfs.c
> > @@ -525,6 +525,100 @@ static const struct attribute_group
> ufs_sysfs_capabilities_group = {
> >       .attrs = ufs_sysfs_capabilities_attrs,  };
> >
> > +static ssize_t capabilities_show(struct device *dev,
> > +             struct device_attribute *attr, char *buf) {
> > +     struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > +     return sysfs_emit(buf, "0x%x\n", hba->capabilities); }
> > +
> > +static ssize_t mcq_cap_show(struct device *dev,
> > +             struct device_attribute *attr, char *buf) {
> > +     struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > +     if (hba->ufs_version < ufshci_version(4, 0))
> > +             return -EOPNOTSUPP;
> > +
> > +     return sysfs_emit(buf, "0x%x\n", hba->mcq_capabilities); }
> > +
> > +static ssize_t version_show(struct device *dev,
> > +             struct device_attribute *attr, char *buf) {
> > +     struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > +     return sysfs_emit(buf, "0x%x\n", hba->ufs_version); }
> > +
> > +static ssize_t ext_capabilities_show(struct device *dev,
> > +             struct device_attribute *attr, char *buf) {
> > +     int ret;
> > +     u32 val;
> > +     struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > +     if (hba->ufs_version < ufshci_version(4, 0))
> > +             return -EOPNOTSUPP;
> > +
> > +     ret = ufshcd_read_hci_reg(hba, &val,
> REG_EXT_CONTROLLER_CAPABILITIES);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return sysfs_emit(buf, "0x%x\n", val); }
> > +
> > +static ssize_t product_id_show(struct device *dev,
> > +             struct device_attribute *attr, char *buf) {
> > +     int ret;
> > +     u32 val;
> > +     struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > +     ret = ufshcd_read_hci_reg(hba, &val, REG_CONTROLLER_PID);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return sysfs_emit(buf, "0x%x\n", val); }
> > +
> > +static ssize_t man_id_show(struct device *dev,
> > +             struct device_attribute *attr, char *buf) {
> > +     int ret;
> > +     u32 val;
> > +     struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > +     ret = ufshcd_read_hci_reg(hba, &val, REG_CONTROLLER_MID);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return sysfs_emit(buf, "0x%x\n", val); }
> > +
> > +static DEVICE_ATTR_RO(capabilities);
> > +static DEVICE_ATTR_RO(mcq_cap);
> > +static DEVICE_ATTR_RO(version);
> > +static DEVICE_ATTR_RO(ext_capabilities); static
> > +DEVICE_ATTR_RO(product_id); static DEVICE_ATTR_RO(man_id);
> > +
> > +static struct attribute *ufs_sysfs_ufshci_cap_attrs[] = {
> > +     &dev_attr_capabilities.attr,
> > +     &dev_attr_mcq_cap.attr,
> > +     &dev_attr_version.attr,
> > +     &dev_attr_ext_capabilities.attr,
> > +     &dev_attr_product_id.attr,
> > +     &dev_attr_man_id.attr,
> > +     NULL
> > +};
> > +
> > +static const struct attribute_group ufs_sysfs_ufshci_group = {
> > +     .name = "ufshci_capabilities",
> > +     .attrs = ufs_sysfs_ufshci_cap_attrs, };
> > +
> >  static ssize_t monitor_enable_show(struct device *dev,
> >                                  struct device_attribute *attr, char
> > *buf)  { @@ -1508,6 +1602,7 @@ static const struct attribute_group
> > ufs_sysfs_attributes_group = {  static const struct attribute_group
> > *ufs_sysfs_groups[] = {
> >       &ufs_sysfs_default_group,
> >       &ufs_sysfs_capabilities_group,
> > +     &ufs_sysfs_ufshci_group,
> >       &ufs_sysfs_monitor_group,
> >       &ufs_sysfs_power_info_group,
> >       &ufs_sysfs_device_descriptor_group,
> > --
> > 2.25.1
> >
> >
> 
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 2/2] scsi: ufs: Add HCI capabilities sysfs group

[Manivannan Sadhasivam]

On Sun, Aug 11, 2024 at 06:19:49AM +0000, Avri Altman wrote:
> > On Fri, Aug 09, 2024 at 10:23:31AM +0300, Avri Altman wrote:
> > > The standard register map of UFSHCI is comprised of several groups.
> > > The first group (starting from offset 0x00), is the host capabilities group.
> > > It contains some interesting information, that otherwise is not
> > > available, e.g. the UFS version of the platform etc.
> > >
> > > Reviewed-by: Keoseong Park <keosung.park@samsung.com>
> > > Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> > > Signed-off-by: Avri Altman <avri.altman@wdc.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-driver-ufs | 42 ++++++++++
> > >  drivers/ufs/core/ufs-sysfs.c               | 95 ++++++++++++++++++++++
> > >  2 files changed, 137 insertions(+)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-driver-ufs
> > > b/Documentation/ABI/testing/sysfs-driver-ufs
> > > index fe943ce76c60..b6e0c3b806fd 100644
> > > --- a/Documentation/ABI/testing/sysfs-driver-ufs
> > > +++ b/Documentation/ABI/testing/sysfs-driver-ufs
> > > @@ -1532,3 +1532,45 @@ Contact:       Bean Huo <beanhuo@micron.com>
> > >  Description:
> > >               rtc_update_ms indicates how often the host should synchronize or
> > update the
> > >               UFS RTC. If set to 0, this will disable UFS RTC periodic update.
> > > +
> > > +What:                /sys/devices/platform/.../ufshci_capabilities/capabilities
> > > +Date:                August 2024
> > > +Contact:     Avri Altman <avri.altman@wdc.com>
> > > +Description:
> > > +             Host Capabilities register group: host controller capabilities register.
> > > +             Symbol - CAP.  Offset: 0x00 - 0x03.
> > 
> > This doesn't look like an ABI description. You are merely specifying the register
> > name and offset that gets accessed while reading this attribute.
> > 
> > Also, I'm not sure if we really want to expose HCI/MCQ capabilities as sysfs ABI.
> Why not?

sysfs is an userspace ABI. The information that is exposed is supposed to be
consumed by the sysadmins/userspace applications.

Looking at the contents of the capabilities, I don't think anyone could
interpret what is being exposed unless they have access to the spec (which is
not free).

> Like the cover letter say, this info is otherwise unavailable.

There are other ways to expose this information too, like debugfs. But what
exactly do you want users to do with it?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

RE: [PATCH v3 2/2] scsi: ufs: Add HCI capabilities sysfs group

[Avri Altman]

> On Sun, Aug 11, 2024 at 06:19:49AM +0000, Avri Altman wrote:
> > > On Fri, Aug 09, 2024 at 10:23:31AM +0300, Avri Altman wrote:
> > > > The standard register map of UFSHCI is comprised of several groups.
> > > > The first group (starting from offset 0x00), is the host capabilities group.
> > > > It contains some interesting information, that otherwise is not
> > > > available, e.g. the UFS version of the platform etc.
> > > >
> > > > Reviewed-by: Keoseong Park <keosung.park@samsung.com>
> > > > Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> > > > Signed-off-by: Avri Altman <avri.altman@wdc.com>
> > > > ---
> > > >  Documentation/ABI/testing/sysfs-driver-ufs | 42 ++++++++++
> > > >  drivers/ufs/core/ufs-sysfs.c               | 95 ++++++++++++++++++++++
> > > >  2 files changed, 137 insertions(+)
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-driver-ufs
> > > > b/Documentation/ABI/testing/sysfs-driver-ufs
> > > > index fe943ce76c60..b6e0c3b806fd 100644
> > > > --- a/Documentation/ABI/testing/sysfs-driver-ufs
> > > > +++ b/Documentation/ABI/testing/sysfs-driver-ufs
> > > > @@ -1532,3 +1532,45 @@ Contact:       Bean Huo <beanhuo@micron.com>
> > > >  Description:
> > > >               rtc_update_ms indicates how often the host should
> > > > synchronize or
> > > update the
> > > >               UFS RTC. If set to 0, this will disable UFS RTC periodic update.
> > > > +
> > > > +What:                /sys/devices/platform/.../ufshci_capabilities/capabilities
> > > > +Date:                August 2024
> > > > +Contact:     Avri Altman <avri.altman@wdc.com>
> > > > +Description:
> > > > +             Host Capabilities register group: host controller capabilities register.
> > > > +             Symbol - CAP.  Offset: 0x00 - 0x03.
> > >
> > > This doesn't look like an ABI description. You are merely specifying
> > > the register name and offset that gets accessed while reading this attribute.
> > >
> > > Also, I'm not sure if we really want to expose HCI/MCQ capabilities as sysfs
> ABI.
> > Why not?
> 
> sysfs is an userspace ABI. The information that is exposed is supposed to be
> consumed by the sysadmins/userspace applications.
> 
> Looking at the contents of the capabilities, I don't think anyone could interpret
> what is being exposed unless they have access to the spec (which is not free).
> 
> > Like the cover letter say, this info is otherwise unavailable.
> 
> There are other ways to expose this information too, like debugfs. But what
> exactly do you want users to do with it?
OK.  Will drop those.

Thanks,
Avri

> 
> - Mani
> 
> --
> மணிவண்ணன் சதாசிவம்