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

[PATCH v2 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 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(-)

-- 
2.25.1

[PATCH v2 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.

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 v2 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.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 Documentation/ABI/testing/sysfs-driver-ufs | 48 +++++++++++
 drivers/ufs/core/ufs-sysfs.c               | 95 ++++++++++++++++++++++
 2 files changed, 143 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index fe943ce76c60..6c6cf12d25ca 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -1532,3 +1532,51 @@ 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/bus/platform/drivers/ufshcd/ufshci_capabilities/capabilities
+What:		/sys/bus/platform/devices/*.ufs/ufshci_capabilities/capabilities
+Date:		August 2024
+Contact:	Avri Altman <avri.altman@wdc.com>
+Description:
+		Host Capabilities register group: host controller capabiities register.
+		Symbol - CAP.  Offset: 0x00 - 0x03.
+
+What:		/sys/bus/platform/drivers/ufshcd/ufshci_capabilities/mcq_cap
+What:		/sys/bus/platform/devices/*.ufs/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/bus/platform/drivers/ufshcd/ufshci_capabilities/version
+What:		/sys/bus/platform/devices/*.ufs/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/bus/platform/drivers/ufshcd/ufshci_capabilities/ext_capabilities
+What:		/sys/bus/platform/devices/*.ufs/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/bus/platform/drivers/ufshcd/ufshci_capabilities/product_id
+What:		/sys/bus/platform/devices/*.ufs/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/bus/platform/drivers/ufshcd/ufshci_capabilities/man_id
+What:		/sys/bus/platform/devices/*.ufs/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 v2 1/2] scsi: ufs: Prepare to add HCI capabilities sysfs

[Keoseong Park]

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

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

[Keoseong Park]

Hi Avri,

> 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.
> 
> Signed-off-by: Avri Altman <avri.altman@wdc.com>
> ---
>  Documentation/ABI/testing/sysfs-driver-ufs | 48 +++++++++++
>  drivers/ufs/core/ufs-sysfs.c               | 95 ++++++++++++++++++++++
>  2 files changed, 143 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
> index fe943ce76c60..6c6cf12d25ca 100644
> --- a/Documentation/ABI/testing/sysfs-driver-ufs
> +++ b/Documentation/ABI/testing/sysfs-driver-ufs
> @@ -1532,3 +1532,51 @@ 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/bus/platform/drivers/ufshcd/ufshci_capabilities/capabilities
> +What:		/sys/bus/platform/devices/*.ufs/ufshci_capabilities/capabilities
> +Date:		August 2024
> +Contact:	Avri Altman <avri.altman@wdc.com>
> +Description:
> +		Host Capabilities register group: host controller capabiities register.

A nit: s/capabiities/capabilities

With above addressed,

Reviewed-by: Keoseong Park <keosung.park@samsung.com>

Best Regards,
Keoseong

> +		Symbol - CAP.  Offset: 0x00 - 0x03.
> +
> +What:		/sys/bus/platform/drivers/ufshcd/ufshci_capabilities/mcq_cap
> +What:		/sys/bus/platform/devices/*.ufs/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/bus/platform/drivers/ufshcd/ufshci_capabilities/version
> +What:		/sys/bus/platform/devices/*.ufs/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/bus/platform/drivers/ufshcd/ufshci_capabilities/ext_capabilities
> +What:		/sys/bus/platform/devices/*.ufs/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/bus/platform/drivers/ufshcd/ufshci_capabilities/product_id
> +What:		/sys/bus/platform/devices/*.ufs/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/bus/platform/drivers/ufshcd/ufshci_capabilities/man_id
> +What:		/sys/bus/platform/devices/*.ufs/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 v2 1/2] scsi: ufs: Prepare to add HCI capabilities sysfs

[Bart Van Assche]

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.

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

[Avri Altman]

> 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.

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

[Bart Van Assche]

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>

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

[Bart Van Assche]

On 8/4/24 12:21 AM, Avri Altman wrote:
> +What:		/sys/bus/platform/drivers/ufshcd/ufshci_capabilities/capabilities

That path seems wrong to me. I think that "ufshcd" should be changed
into something like ${host_driver_name}/${ufshci_instance_name}. An 
example from a Pixel 8 device:

$ adb shell ls /sys/bus/platform/drivers/*ufs*
/sys/bus/platform/drivers/exynos-ufs:
13200000.ufs
module
uevent

/sys/bus/platform/drivers/ufshcd-hisi:
bind
uevent
unbind

> +What:		/sys/bus/platform/devices/*.ufs/ufshci_capabilities/capabilities
> +Date:		August 2024
> +Contact:	Avri Altman <avri.altman@wdc.com>
> +Description:
> +		Host Capabilities register group: host controller capabiities register.
> +		Symbol - CAP.  Offset: 0x00 - 0x03.

Please fix the spelling error that was already reported by Keoseong
Park. Otherwise this patch looks good to me.

Thanks,

Bart.

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

[Avri Altman]

> 
> On 8/4/24 12:21 AM, Avri Altman wrote:
> > +What:
> /sys/bus/platform/drivers/ufshcd/ufshci_capabilities/capabilities
> 
> That path seems wrong to me. I think that "ufshcd" should be changed into
> something like ${host_driver_name}/${ufshci_instance_name}. An example from
> a Pixel 8 device:
> 
> $ adb shell ls /sys/bus/platform/drivers/*ufs*
> /sys/bus/platform/drivers/exynos-ufs:
> 13200000.ufs
> module
> uevent
> 
> /sys/bus/platform/drivers/ufshcd-hisi:
> bind
> uevent
> unbind
Yeah - nor on my rb5.  Here it is in: /sys/devices/platform/soc@0/1d84000.ufshc/ufshci_capabilities
So I am wondering if having something like the below is acceptable?
/sys/devices/platform/<platform-specific-path>/ufshci_capabilities

Thanks,
Avri 

> 
> > +What:
> /sys/bus/platform/devices/*.ufs/ufshci_capabilities/capabilities
> > +Date:                August 2024
> > +Contact:     Avri Altman <avri.altman@wdc.com>
> > +Description:
> > +             Host Capabilities register group: host controller capabiities register.
> > +             Symbol - CAP.  Offset: 0x00 - 0x03.
> 
> Please fix the spelling error that was already reported by Keoseong Park.
> Otherwise this patch looks good to me.
> 
> Thanks,
> 
> Bart.

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

[Bart Van Assche]

On 8/8/24 4:32 AM, Avri Altman wrote:
> So I am wondering if having something like the below is acceptable?
> /sys/devices/platform/<platform-specific-path>/ufshci_capabilities

This is also fine with me:

/sys/devices/platform/.../ufshci_capabilities

Thanks,

Bart.