[PATCH 0/2] soc: qcom: socinfo: Add fields in sysfs custom attributes

[PATCH 0/2] soc: qcom: socinfo: Add fields in sysfs custom attributes

[Naman Jain]

This series adds support to have SoC info fields available in sysfs
to enable the use of these nodes in userland scripts and test scripts.
This is to provide the interface to these scripts to find the details of
parts present in the SoC and decide to execute a set of shell commands,
that are supported/required for these parts. The decision to extend sysfs
interface is taken as debugfs is not mounted by default and the use cases
for this information are not essentially for debug.

The patches add the following changes:
1. Restructure the code to make the scope of socinfo variable, from
   function to file. Also, make the socinfo variable name more descriptive.
2. Extend the sysfs custom attributes to incorporate fields introduced in
   socinfo format version 2 to 6. Add name mappings for hw_platform field
   to make the sysfs information more descriptive.

Support for versions 7 and above will be added in future patchsets.

Also, patch 2 depends on patch 1 in the series.

Naman Jain (2):
  soc: qcom: socinfo: Change socinfo variable name and scope
  soc: qcom: socinfo: Add sysfs attributes for fields in v2-v6

 drivers/soc/qcom/socinfo.c | 261 +++++++++++++++++++++++++++++++------
 1 file changed, 223 insertions(+), 38 deletions(-)

-- 
2.17.1

[PATCH 1/2] soc: qcom: socinfo: Change socinfo variable name and scope

[Naman Jain]

Change socinfo structure variable scope from function to file
to make it easy to support custom attributes for sysfs. Also,
change variable name to make it more descriptive.

Signed-off-by: Naman Jain <quic_namajain@quicinc.com>
---
 drivers/soc/qcom/socinfo.c | 80 ++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 38 deletions(-)

diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
index 10efdbcfdf05..251c0fd94962 100644
--- a/drivers/soc/qcom/socinfo.c
+++ b/drivers/soc/qcom/socinfo.c
@@ -175,6 +175,7 @@ struct socinfo {
 	__le32  npartnamemap_offset;
 	__le32  nnum_partname_mapping;
 };
+static struct socinfo *soc_info;
 
 #ifdef CONFIG_DEBUG_FS
 struct socinfo_params {
@@ -502,7 +503,7 @@ DEFINE_IMAGE_OPS(variant);
 DEFINE_IMAGE_OPS(oem);
 
 static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo,
-				 struct socinfo *info, size_t info_size)
+				 size_t info_size)
 {
 	struct smem_image_version *versions;
 	struct dentry *dentry;
@@ -513,15 +514,15 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo,
 
 	qcom_socinfo->dbg_root = debugfs_create_dir("qcom_socinfo", NULL);
 
-	qcom_socinfo->info.fmt = __le32_to_cpu(info->fmt);
+	qcom_socinfo->info.fmt = __le32_to_cpu(soc_info->fmt);
 
 	debugfs_create_x32("info_fmt", 0444, qcom_socinfo->dbg_root,
 			   &qcom_socinfo->info.fmt);
 
 	switch (qcom_socinfo->info.fmt) {
 	case SOCINFO_VERSION(0, 16):
-		qcom_socinfo->info.feature_code = __le32_to_cpu(info->feature_code);
-		qcom_socinfo->info.pcode = __le32_to_cpu(info->pcode);
+		qcom_socinfo->info.feature_code = __le32_to_cpu(soc_info->feature_code);
+		qcom_socinfo->info.pcode = __le32_to_cpu(soc_info->pcode);
 
 		debugfs_create_u32("feature_code", 0444, qcom_socinfo->dbg_root,
 				   &qcom_socinfo->info.feature_code);
@@ -529,16 +530,20 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo,
 				   &qcom_socinfo->info.pcode);
 		fallthrough;
 	case SOCINFO_VERSION(0, 15):
-		qcom_socinfo->info.nmodem_supported = __le32_to_cpu(info->nmodem_supported);
+		qcom_socinfo->info.nmodem_supported = __le32_to_cpu(soc_info->nmodem_supported);
 
 		debugfs_create_u32("nmodem_supported", 0444, qcom_socinfo->dbg_root,
 				   &qcom_socinfo->info.nmodem_supported);
 		fallthrough;
 	case SOCINFO_VERSION(0, 14):
-		qcom_socinfo->info.num_clusters = __le32_to_cpu(info->num_clusters);
-		qcom_socinfo->info.ncluster_array_offset = __le32_to_cpu(info->ncluster_array_offset);
-		qcom_socinfo->info.num_defective_parts = __le32_to_cpu(info->num_defective_parts);
-		qcom_socinfo->info.ndefective_parts_array_offset = __le32_to_cpu(info->ndefective_parts_array_offset);
+		qcom_socinfo->info.num_clusters =
+			__le32_to_cpu(soc_info->num_clusters);
+		qcom_socinfo->info.ncluster_array_offset =
+			__le32_to_cpu(soc_info->ncluster_array_offset);
+		qcom_socinfo->info.num_defective_parts =
+			__le32_to_cpu(soc_info->num_defective_parts);
+		qcom_socinfo->info.ndefective_parts_array_offset =
+			__le32_to_cpu(soc_info->ndefective_parts_array_offset);
 
 		debugfs_create_u32("num_clusters", 0444, qcom_socinfo->dbg_root,
 				   &qcom_socinfo->info.num_clusters);
@@ -550,19 +555,19 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo,
 				   &qcom_socinfo->info.ndefective_parts_array_offset);
 		fallthrough;
 	case SOCINFO_VERSION(0, 13):
-		qcom_socinfo->info.nproduct_id = __le32_to_cpu(info->nproduct_id);
+		qcom_socinfo->info.nproduct_id = __le32_to_cpu(soc_info->nproduct_id);
 
 		debugfs_create_u32("nproduct_id", 0444, qcom_socinfo->dbg_root,
 				   &qcom_socinfo->info.nproduct_id);
-		DEBUGFS_ADD(info, chip_id);
+		DEBUGFS_ADD(soc_info, chip_id);
 		fallthrough;
 	case SOCINFO_VERSION(0, 12):
 		qcom_socinfo->info.chip_family =
-			__le32_to_cpu(info->chip_family);
+			__le32_to_cpu(soc_info->chip_family);
 		qcom_socinfo->info.raw_device_family =
-			__le32_to_cpu(info->raw_device_family);
+			__le32_to_cpu(soc_info->raw_device_family);
 		qcom_socinfo->info.raw_device_num =
-			__le32_to_cpu(info->raw_device_num);
+			__le32_to_cpu(soc_info->raw_device_num);
 
 		debugfs_create_x32("chip_family", 0444, qcom_socinfo->dbg_root,
 				   &qcom_socinfo->info.chip_family);
@@ -574,26 +579,26 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo,
 				   &qcom_socinfo->info.raw_device_num);
 		fallthrough;
 	case SOCINFO_VERSION(0, 11):
-		num_pmics = le32_to_cpu(info->num_pmics);
-		pmic_array_offset = le32_to_cpu(info->pmic_array_offset);
+		num_pmics = le32_to_cpu(soc_info->num_pmics);
+		pmic_array_offset = le32_to_cpu(soc_info->pmic_array_offset);
 		if (pmic_array_offset + 2 * num_pmics * sizeof(u32) <= info_size)
-			DEBUGFS_ADD(info, pmic_model_array);
+			DEBUGFS_ADD(soc_info, pmic_model_array);
 		fallthrough;
 	case SOCINFO_VERSION(0, 10):
 	case SOCINFO_VERSION(0, 9):
-		qcom_socinfo->info.foundry_id = __le32_to_cpu(info->foundry_id);
+		qcom_socinfo->info.foundry_id = __le32_to_cpu(soc_info->foundry_id);
 
 		debugfs_create_u32("foundry_id", 0444, qcom_socinfo->dbg_root,
 				   &qcom_socinfo->info.foundry_id);
 		fallthrough;
 	case SOCINFO_VERSION(0, 8):
 	case SOCINFO_VERSION(0, 7):
-		DEBUGFS_ADD(info, pmic_model);
-		DEBUGFS_ADD(info, pmic_die_rev);
+		DEBUGFS_ADD(soc_info, pmic_model);
+		DEBUGFS_ADD(soc_info, pmic_die_rev);
 		fallthrough;
 	case SOCINFO_VERSION(0, 6):
 		qcom_socinfo->info.hw_plat_subtype =
-			__le32_to_cpu(info->hw_plat_subtype);
+			__le32_to_cpu(soc_info->hw_plat_subtype);
 
 		debugfs_create_u32("hardware_platform_subtype", 0444,
 				   qcom_socinfo->dbg_root,
@@ -601,34 +606,34 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo,
 		fallthrough;
 	case SOCINFO_VERSION(0, 5):
 		qcom_socinfo->info.accessory_chip =
-			__le32_to_cpu(info->accessory_chip);
+			__le32_to_cpu(soc_info->accessory_chip);
 
 		debugfs_create_u32("accessory_chip", 0444,
 				   qcom_socinfo->dbg_root,
 				   &qcom_socinfo->info.accessory_chip);
 		fallthrough;
 	case SOCINFO_VERSION(0, 4):
-		qcom_socinfo->info.plat_ver = __le32_to_cpu(info->plat_ver);
+		qcom_socinfo->info.plat_ver = __le32_to_cpu(soc_info->plat_ver);
 
 		debugfs_create_u32("platform_version", 0444,
 				   qcom_socinfo->dbg_root,
 				   &qcom_socinfo->info.plat_ver);
 		fallthrough;
 	case SOCINFO_VERSION(0, 3):
-		qcom_socinfo->info.hw_plat = __le32_to_cpu(info->hw_plat);
+		qcom_socinfo->info.hw_plat = __le32_to_cpu(soc_info->hw_plat);
 
 		debugfs_create_u32("hardware_platform", 0444,
 				   qcom_socinfo->dbg_root,
 				   &qcom_socinfo->info.hw_plat);
 		fallthrough;
 	case SOCINFO_VERSION(0, 2):
-		qcom_socinfo->info.raw_ver  = __le32_to_cpu(info->raw_ver);
+		qcom_socinfo->info.raw_ver  = __le32_to_cpu(soc_info->raw_ver);
 
 		debugfs_create_u32("raw_version", 0444, qcom_socinfo->dbg_root,
 				   &qcom_socinfo->info.raw_ver);
 		fallthrough;
 	case SOCINFO_VERSION(0, 1):
-		DEBUGFS_ADD(info, build_id);
+		DEBUGFS_ADD(soc_info, build_id);
 		break;
 	}
 
@@ -656,7 +661,7 @@ static void socinfo_debugfs_exit(struct qcom_socinfo *qcom_socinfo)
 }
 #else
 static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo,
-				 struct socinfo *info, size_t info_size)
+				 size_t info_size)
 {
 }
 static void socinfo_debugfs_exit(struct qcom_socinfo *qcom_socinfo) {  }
@@ -665,14 +670,13 @@ static void socinfo_debugfs_exit(struct qcom_socinfo *qcom_socinfo) {  }
 static int qcom_socinfo_probe(struct platform_device *pdev)
 {
 	struct qcom_socinfo *qs;
-	struct socinfo *info;
 	size_t item_size;
 
-	info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
+	soc_info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
 			      &item_size);
-	if (IS_ERR(info)) {
+	if (IS_ERR(soc_info)) {
 		dev_err(&pdev->dev, "Couldn't find socinfo\n");
-		return PTR_ERR(info);
+		return PTR_ERR(soc_info);
 	}
 
 	qs = devm_kzalloc(&pdev->dev, sizeof(*qs), GFP_KERNEL);
@@ -681,25 +685,25 @@ static int qcom_socinfo_probe(struct platform_device *pdev)
 
 	qs->attr.family = "Snapdragon";
 	qs->attr.machine = socinfo_machine(&pdev->dev,
-					   le32_to_cpu(info->id));
+					   le32_to_cpu(soc_info->id));
 	qs->attr.soc_id = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%u",
-					 le32_to_cpu(info->id));
+					 le32_to_cpu(soc_info->id));
 	qs->attr.revision = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%u.%u",
-					   SOCINFO_MAJOR(le32_to_cpu(info->ver)),
-					   SOCINFO_MINOR(le32_to_cpu(info->ver)));
+					   SOCINFO_MAJOR(le32_to_cpu(soc_info->ver)),
+					   SOCINFO_MINOR(le32_to_cpu(soc_info->ver)));
 	if (offsetof(struct socinfo, serial_num) <= item_size)
 		qs->attr.serial_number = devm_kasprintf(&pdev->dev, GFP_KERNEL,
 							"%u",
-							le32_to_cpu(info->serial_num));
+							le32_to_cpu(soc_info->serial_num));
 
 	qs->soc_dev = soc_device_register(&qs->attr);
 	if (IS_ERR(qs->soc_dev))
 		return PTR_ERR(qs->soc_dev);
 
-	socinfo_debugfs_init(qs, info, item_size);
+	socinfo_debugfs_init(qs, item_size);
 
 	/* Feed the soc specific unique data into entropy pool */
-	add_device_randomness(info, item_size);
+	add_device_randomness(soc_info, item_size);
 
 	platform_set_drvdata(pdev, qs);
 
-- 
2.17.1

[PATCH 2/2] soc: qcom: socinfo: Add sysfs attributes for fields in v2-v6

[Naman Jain]

Add support in sysfs custom attributes for fields in socinfo version
v2-v6. This is to support SoC based operations in userland scripts
and test scripts. Also, add name mappings for hw-platform type to
make the sysfs information more descriptive.

Signed-off-by: Naman Jain <quic_namajain@quicinc.com>
---
 drivers/soc/qcom/socinfo.c | 181 +++++++++++++++++++++++++++++++++++++
 1 file changed, 181 insertions(+)

diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
index 251c0fd94962..ff92064c2246 100644
--- a/drivers/soc/qcom/socinfo.c
+++ b/drivers/soc/qcom/socinfo.c
@@ -41,6 +41,52 @@
  */
 #define SMEM_HW_SW_BUILD_ID            137
 
+enum {
+	HW_PLATFORM_UNKNOWN = 0,
+	HW_PLATFORM_SURF = 1,
+	HW_PLATFORM_FFA = 2,
+	HW_PLATFORM_FLUID = 3,
+	HW_PLATFORM_SVLTE_FFA = 4,
+	HW_PLATFORM_SVLTE_SURF = 5,
+	HW_PLATFORM_MTP_MDM = 7,
+	HW_PLATFORM_MTP = 8,
+	HW_PLATFORM_LIQUID = 9,
+	HW_PLATFORM_DRAGON = 10,
+	HW_PLATFORM_QRD = 11,
+	HW_PLATFORM_HRD = 13,
+	HW_PLATFORM_DTV = 14,
+	HW_PLATFORM_RCM = 21,
+	HW_PLATFORM_STP = 23,
+	HW_PLATFORM_SBC = 24,
+	HW_PLATFORM_HDK = 31,
+	HW_PLATFORM_ATP = 33,
+	HW_PLATFORM_IDP = 34,
+	HW_PLATFORM_INVALID
+};
+
+static const char * const hw_platform[] = {
+	[HW_PLATFORM_UNKNOWN] = "Unknown",
+	[HW_PLATFORM_SURF] = "Surf",
+	[HW_PLATFORM_FFA] = "FFA",
+	[HW_PLATFORM_FLUID] = "Fluid",
+	[HW_PLATFORM_SVLTE_FFA] = "SVLTE_FFA",
+	[HW_PLATFORM_SVLTE_SURF] = "SLVTE_SURF",
+	[HW_PLATFORM_MTP_MDM] = "MDM_MTP_NO_DISPLAY",
+	[HW_PLATFORM_MTP] = "MTP",
+	[HW_PLATFORM_RCM] = "RCM",
+	[HW_PLATFORM_LIQUID] = "Liquid",
+	[HW_PLATFORM_DRAGON] = "Dragon",
+	[HW_PLATFORM_QRD] = "QRD",
+	[HW_PLATFORM_HRD] = "HRD",
+	[HW_PLATFORM_DTV] = "DTV",
+	[HW_PLATFORM_STP] = "STP",
+	[HW_PLATFORM_SBC] = "SBC",
+	[HW_PLATFORM_HDK] = "HDK",
+	[HW_PLATFORM_ATP] = "ATP",
+	[HW_PLATFORM_IDP] = "IDP",
+	[HW_PLATFORM_INVALID] = "Invalid",
+};
+
 #ifdef CONFIG_DEBUG_FS
 #define SMEM_IMAGE_VERSION_BLOCKS_COUNT        32
 #define SMEM_IMAGE_VERSION_SIZE                4096
@@ -368,6 +414,140 @@ static const struct soc_id soc_id[] = {
 	{ qcom_board_id(QRU1062) },
 };
 
+/* sysfs attributes */
+#define ATTR_DEFINE(param) \
+	static DEVICE_ATTR(param, 0644, qcom_get_##param, NULL)
+
+/* Version 2 */
+static ssize_t
+qcom_get_raw_id(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	return scnprintf(buf, PAGE_SIZE, "%u\n",
+			 le32_to_cpu(soc_info->raw_id));
+}
+ATTR_DEFINE(raw_id);
+
+static ssize_t
+qcom_get_raw_version(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	return scnprintf(buf, PAGE_SIZE, "%u\n",
+			 le32_to_cpu(soc_info->raw_ver));
+}
+ATTR_DEFINE(raw_version);
+
+/* Version 3 */
+static ssize_t
+qcom_get_hw_platform(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	uint32_t hw_plat = le32_to_cpu(soc_info->hw_plat);
+
+	hw_plat = (hw_plat >= HW_PLATFORM_INVALID) ? HW_PLATFORM_INVALID : hw_plat;
+	return scnprintf(buf, PAGE_SIZE, "%-.32s\n",
+			hw_platform[hw_plat]);
+}
+ATTR_DEFINE(hw_platform);
+
+/* Version 4 */
+static ssize_t
+qcom_get_platform_version(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	return scnprintf(buf, PAGE_SIZE, "%u\n",
+			 le32_to_cpu(soc_info->plat_ver));
+}
+ATTR_DEFINE(platform_version);
+
+/* Version 5 */
+static ssize_t
+qcom_get_accessory_chip(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	return scnprintf(buf, PAGE_SIZE, "%u\n",
+			le32_to_cpu(soc_info->accessory_chip));
+}
+ATTR_DEFINE(accessory_chip);
+
+/* Version 6 */
+static ssize_t
+qcom_get_platform_subtype_id(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	return scnprintf(buf, PAGE_SIZE, "%u\n",
+			 le32_to_cpu(soc_info->hw_plat_subtype));
+}
+ATTR_DEFINE(platform_subtype_id);
+
+static struct attribute *qcom_custom_socinfo_attrs[7];
+
+static const struct attribute_group custom_soc_attr_group = {
+	.attrs = qcom_custom_socinfo_attrs,
+};
+
+static void qcom_socinfo_populate_sysfs(struct qcom_socinfo *qcom_socinfo)
+{
+	int i = 0, socinfo_format = le32_to_cpu(soc_info->fmt);
+
+	/* Note: qcom_custom_socinfo_attrs[] size needs to be in sync with attributes added here. */
+	switch (socinfo_format) {
+	case SOCINFO_VERSION(0, 16):
+		fallthrough;
+	case SOCINFO_VERSION(0, 15):
+		fallthrough;
+	case SOCINFO_VERSION(0, 14):
+		fallthrough;
+	case SOCINFO_VERSION(0, 13):
+		fallthrough;
+	case SOCINFO_VERSION(0, 12):
+		fallthrough;
+	case SOCINFO_VERSION(0, 11):
+		fallthrough;
+	case SOCINFO_VERSION(0, 10):
+		fallthrough;
+	case SOCINFO_VERSION(0, 9):
+		fallthrough;
+	case SOCINFO_VERSION(0, 8):
+		fallthrough;
+	case SOCINFO_VERSION(0, 7):
+		fallthrough;
+	case SOCINFO_VERSION(0, 6):
+		qcom_custom_socinfo_attrs[i++] =
+			&dev_attr_platform_subtype_id.attr;
+		fallthrough;
+	case SOCINFO_VERSION(0, 5):
+		qcom_custom_socinfo_attrs[i++] = &dev_attr_accessory_chip.attr;
+		fallthrough;
+	case SOCINFO_VERSION(0, 4):
+		qcom_custom_socinfo_attrs[i++] = &dev_attr_platform_version.attr;
+		fallthrough;
+	case SOCINFO_VERSION(0, 3):
+		qcom_custom_socinfo_attrs[i++] = &dev_attr_hw_platform.attr;
+		fallthrough;
+	case SOCINFO_VERSION(0, 2):
+		qcom_custom_socinfo_attrs[i++] = &dev_attr_raw_id.attr;
+		qcom_custom_socinfo_attrs[i++] = &dev_attr_raw_version.attr;
+		fallthrough;
+	case SOCINFO_VERSION(0, 1):
+		break;
+	default:
+		pr_err("Unknown socinfo format: v%u.%u\n",
+				SOCINFO_MAJOR(socinfo_format),
+				SOCINFO_MINOR(socinfo_format));
+		break;
+	}
+
+	qcom_custom_socinfo_attrs[i] = NULL;
+	qcom_socinfo->attr.custom_attr_group = &custom_soc_attr_group;
+}
+
 static const char *socinfo_machine(struct device *dev, unsigned int id)
 {
 	int idx;
@@ -696,6 +876,7 @@ static int qcom_socinfo_probe(struct platform_device *pdev)
 							"%u",
 							le32_to_cpu(soc_info->serial_num));
 
+	qcom_socinfo_populate_sysfs(qs);
 	qs->soc_dev = soc_device_register(&qs->attr);
 	if (IS_ERR(qs->soc_dev))
 		return PTR_ERR(qs->soc_dev);
-- 
2.17.1

Re: [PATCH 1/2] soc: qcom: socinfo: Change socinfo variable name and scope

[Trilok Soni]

On 1/11/2023 12:21 AM, Naman Jain wrote:
> Change socinfo structure variable scope from function to file
> to make it easy to support custom attributes for sysfs. Also,
> change variable name to make it more descriptive.

Did you mean debugfs?

Can you one example of custom attribute in the commit text so that we
understand the motivation better?

> 
> Signed-off-by: Naman Jain <quic_namajain@quicinc.com>
> ---
>   drivers/soc/qcom/socinfo.c | 80 ++++++++++++++++++++------------------
>   1 file changed, 42 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> index 10efdbcfdf05..251c0fd94962 100644
> --- a/drivers/soc/qcom/socinfo.c
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -175,6 +175,7 @@ struct socinfo {
>   	__le32  npartnamemap_offset;
>   	__le32  nnum_partname_mapping;
>   };
> +static struct socinfo *soc_info;

Is there any better way to do it? Should not asume the just one object
and dynamically allocate it? Let's wait for Bjorn to check as well.

---Trilok Soni

Re: [PATCH 2/2] soc: qcom: socinfo: Add sysfs attributes for fields in v2-v6

[Dmitry Baryshkov]

On 11/01/2023 10:21, Naman Jain wrote:
> Add support in sysfs custom attributes for fields in socinfo version
> v2-v6. This is to support SoC based operations in userland scripts
> and test scripts. Also, add name mappings for hw-platform type to
> make the sysfs information more descriptive.

Please include a patch documenting your additions to 
Documentation/ABI/testing/sysfs-devices-soc. Please describe usecases 
for new attributes and their applicability to non-Qualcomm boards.

Note, that testing scripts can access debugfs entries without any issues.

> 
> Signed-off-by: Naman Jain <quic_namajain@quicinc.com>
> ---
>   drivers/soc/qcom/socinfo.c | 181 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 181 insertions(+)
> 
> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> index 251c0fd94962..ff92064c2246 100644
> --- a/drivers/soc/qcom/socinfo.c
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -41,6 +41,52 @@
>    */
>   #define SMEM_HW_SW_BUILD_ID            137
>   
> +enum {
> +	HW_PLATFORM_UNKNOWN = 0,
> +	HW_PLATFORM_SURF = 1,
> +	HW_PLATFORM_FFA = 2,
> +	HW_PLATFORM_FLUID = 3,
> +	HW_PLATFORM_SVLTE_FFA = 4,
> +	HW_PLATFORM_SVLTE_SURF = 5,
> +	HW_PLATFORM_MTP_MDM = 7,
> +	HW_PLATFORM_MTP = 8,
> +	HW_PLATFORM_LIQUID = 9,
> +	HW_PLATFORM_DRAGON = 10,
> +	HW_PLATFORM_QRD = 11,
> +	HW_PLATFORM_HRD = 13,
> +	HW_PLATFORM_DTV = 14,
> +	HW_PLATFORM_RCM = 21,
> +	HW_PLATFORM_STP = 23,
> +	HW_PLATFORM_SBC = 24,
> +	HW_PLATFORM_HDK = 31,
> +	HW_PLATFORM_ATP = 33,
> +	HW_PLATFORM_IDP = 34,
> +	HW_PLATFORM_INVALID
> +};
> +
> +static const char * const hw_platform[] = {
> +	[HW_PLATFORM_UNKNOWN] = "Unknown",
> +	[HW_PLATFORM_SURF] = "Surf",
> +	[HW_PLATFORM_FFA] = "FFA",
> +	[HW_PLATFORM_FLUID] = "Fluid",
> +	[HW_PLATFORM_SVLTE_FFA] = "SVLTE_FFA",
> +	[HW_PLATFORM_SVLTE_SURF] = "SLVTE_SURF",
> +	[HW_PLATFORM_MTP_MDM] = "MDM_MTP_NO_DISPLAY",
> +	[HW_PLATFORM_MTP] = "MTP",
> +	[HW_PLATFORM_RCM] = "RCM",
> +	[HW_PLATFORM_LIQUID] = "Liquid",
> +	[HW_PLATFORM_DRAGON] = "Dragon",
> +	[HW_PLATFORM_QRD] = "QRD",
> +	[HW_PLATFORM_HRD] = "HRD",
> +	[HW_PLATFORM_DTV] = "DTV",
> +	[HW_PLATFORM_STP] = "STP",
> +	[HW_PLATFORM_SBC] = "SBC",
> +	[HW_PLATFORM_HDK] = "HDK",
> +	[HW_PLATFORM_ATP] = "ATP",
> +	[HW_PLATFORM_IDP] = "IDP",
> +	[HW_PLATFORM_INVALID] = "Invalid",
> +};

This is not a property of the SoC. It is a property of the device. As 
such it should not be part of /sys/bus/soc devices.

You can find board description in /sys/firmware/devicetree/base/model

> +
>   #ifdef CONFIG_DEBUG_FS
>   #define SMEM_IMAGE_VERSION_BLOCKS_COUNT        32
>   #define SMEM_IMAGE_VERSION_SIZE                4096
> @@ -368,6 +414,140 @@ static const struct soc_id soc_id[] = {
>   	{ qcom_board_id(QRU1062) },
>   };
>   
> +/* sysfs attributes */
> +#define ATTR_DEFINE(param) \
> +	static DEVICE_ATTR(param, 0644, qcom_get_##param, NULL)
> +
> +/* Version 2 */
> +static ssize_t
> +qcom_get_raw_id(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	return scnprintf(buf, PAGE_SIZE, "%u\n",
> +			 le32_to_cpu(soc_info->raw_id));
> +}
> +ATTR_DEFINE(raw_id);
> +
> +static ssize_t
> +qcom_get_raw_version(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	return scnprintf(buf, PAGE_SIZE, "%u\n",
> +			 le32_to_cpu(soc_info->raw_ver));
> +}
> +ATTR_DEFINE(raw_version);

Why are they raw? can you unraw them?

Whose version and id are these attributes referring to?

> +
> +/* Version 3 */
> +static ssize_t
> +qcom_get_hw_platform(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	uint32_t hw_plat = le32_to_cpu(soc_info->hw_plat);
> +
> +	hw_plat = (hw_plat >= HW_PLATFORM_INVALID) ? HW_PLATFORM_INVALID : hw_plat;
> +	return scnprintf(buf, PAGE_SIZE, "%-.32s\n",
> +			hw_platform[hw_plat]);
> +}
> +ATTR_DEFINE(hw_platform);
> +
> +/* Version 4 */
> +static ssize_t
> +qcom_get_platform_version(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	return scnprintf(buf, PAGE_SIZE, "%u\n",
> +			 le32_to_cpu(soc_info->plat_ver));
> +}
> +ATTR_DEFINE(platform_version);
> +
> +/* Version 5 */
> +static ssize_t
> +qcom_get_accessory_chip(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	return scnprintf(buf, PAGE_SIZE, "%u\n",
> +			le32_to_cpu(soc_info->accessory_chip));
> +}
> +ATTR_DEFINE(accessory_chip);

If this an _accessory_ chip, there should be a separate soc device 
describing it, rather than stuffing information into the soc0.

> +
> +/* Version 6 */
> +static ssize_t
> +qcom_get_platform_subtype_id(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	return scnprintf(buf, PAGE_SIZE, "%u\n",
> +			 le32_to_cpu(soc_info->hw_plat_subtype));
> +}
> +ATTR_DEFINE(platform_subtype_id);

Again, this is the board property, not an SoC one.

> +
> +static struct attribute *qcom_custom_socinfo_attrs[7];
> +
> +static const struct attribute_group custom_soc_attr_group = {
> +	.attrs = qcom_custom_socinfo_attrs,
> +};
> +
> +static void qcom_socinfo_populate_sysfs(struct qcom_socinfo *qcom_socinfo)
> +{
> +	int i = 0, socinfo_format = le32_to_cpu(soc_info->fmt);
> +
> +	/* Note: qcom_custom_socinfo_attrs[] size needs to be in sync with attributes added here. */
> +	switch (socinfo_format) {
> +	case SOCINFO_VERSION(0, 16):
> +		fallthrough;
> +	case SOCINFO_VERSION(0, 15):
> +		fallthrough;
> +	case SOCINFO_VERSION(0, 14):
> +		fallthrough;
> +	case SOCINFO_VERSION(0, 13):
> +		fallthrough;
> +	case SOCINFO_VERSION(0, 12):
> +		fallthrough;
> +	case SOCINFO_VERSION(0, 11):
> +		fallthrough;
> +	case SOCINFO_VERSION(0, 10):
> +		fallthrough;
> +	case SOCINFO_VERSION(0, 9):
> +		fallthrough;
> +	case SOCINFO_VERSION(0, 8):
> +		fallthrough;
> +	case SOCINFO_VERSION(0, 7):
> +		fallthrough;
> +	case SOCINFO_VERSION(0, 6):
> +		qcom_custom_socinfo_attrs[i++] =
> +			&dev_attr_platform_subtype_id.attr;
> +		fallthrough;
> +	case SOCINFO_VERSION(0, 5):
> +		qcom_custom_socinfo_attrs[i++] = &dev_attr_accessory_chip.attr;
> +		fallthrough;
> +	case SOCINFO_VERSION(0, 4):
> +		qcom_custom_socinfo_attrs[i++] = &dev_attr_platform_version.attr;
> +		fallthrough;
> +	case SOCINFO_VERSION(0, 3):
> +		qcom_custom_socinfo_attrs[i++] = &dev_attr_hw_platform.attr;
> +		fallthrough;
> +	case SOCINFO_VERSION(0, 2):
> +		qcom_custom_socinfo_attrs[i++] = &dev_attr_raw_id.attr;
> +		qcom_custom_socinfo_attrs[i++] = &dev_attr_raw_version.attr;
> +		fallthrough;
> +	case SOCINFO_VERSION(0, 1):
> +		break;
> +	default:
> +		pr_err("Unknown socinfo format: v%u.%u\n",
> +				SOCINFO_MAJOR(socinfo_format),
> +				SOCINFO_MINOR(socinfo_format));
> +		break;
> +	}
> +
> +	qcom_custom_socinfo_attrs[i] = NULL;
> +	qcom_socinfo->attr.custom_attr_group = &custom_soc_attr_group;
> +}
> +
>   static const char *socinfo_machine(struct device *dev, unsigned int id)
>   {
>   	int idx;
> @@ -696,6 +876,7 @@ static int qcom_socinfo_probe(struct platform_device *pdev)
>   							"%u",
>   							le32_to_cpu(soc_info->serial_num));
>   
> +	qcom_socinfo_populate_sysfs(qs);
>   	qs->soc_dev = soc_device_register(&qs->attr);
>   	if (IS_ERR(qs->soc_dev))
>   		return PTR_ERR(qs->soc_dev);

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] soc: qcom: socinfo: Add sysfs attributes for fields in v2-v6

[Trilok Soni]

On 1/11/2023 3:19 PM, Dmitry Baryshkov wrote:
> On 11/01/2023 10:21, Naman Jain wrote:
>> Add support in sysfs custom attributes for fields in socinfo version
>> v2-v6. This is to support SoC based operations in userland scripts
>> and test scripts. Also, add name mappings for hw-platform type to
>> make the sysfs information more descriptive.
> 
> Please include a patch documenting your additions to 
> Documentation/ABI/testing/sysfs-devices-soc. Please describe usecases 
> for new attributes and their applicability to non-Qualcomm boards.
> 
> Note, that testing scripts can access debugfs entries without any issues.

The commit text mentions the "userland" scripts and it could mean the 
product OS like Android or Yocto having the applications using these 
/sysfs entries. Naman, please clarify if the vendor application layer in 
the Android is using these Entries to make decisions based on the 
platforms / soc information?

---Trilok Soni

Re: [PATCH 1/2] soc: qcom: socinfo: Change socinfo variable name and scope

[Naman Jain]

Hi Trilok,

Thanks for reviewing the patches.

On 1/12/2023 3:07 AM, Trilok Soni wrote:
> On 1/11/2023 12:21 AM, Naman Jain wrote:
>> Change socinfo structure variable scope from function to file
>> to make it easy to support custom attributes for sysfs. Also,
>> change variable name to make it more descriptive.
>
> Did you mean debugfs?


No, I meant sysfs only. debugfs support is generally added with every 
version update, in kernel. Since debugfs can't be used for these 
purposes in production devices, we are proposing to extend current sysfs 
interface.


>
> Can you one example of custom attribute in the commit text so that we
> understand the motivation better?
>

I'll add the examples in next patch. Thanks.


>>
>> Signed-off-by: Naman Jain <quic_namajain@quicinc.com>
>> ---
>>   drivers/soc/qcom/socinfo.c | 80 ++++++++++++++++++++------------------
>>   1 file changed, 42 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
>> index 10efdbcfdf05..251c0fd94962 100644
>> --- a/drivers/soc/qcom/socinfo.c
>> +++ b/drivers/soc/qcom/socinfo.c
>> @@ -175,6 +175,7 @@ struct socinfo {
>>       __le32  npartnamemap_offset;
>>       __le32  nnum_partname_mapping;
>>   };
>> +static struct socinfo *soc_info;
>
> Is there any better way to do it? Should not asume the just one object
> and dynamically allocate it? Let's wait for Bjorn to check as well.


So current sysfs attributes are added in probe function, where this 
"info" variable is defined and used. For additions to current sysfs 
interface, using a separate function, the need for having this variable 
with file scope came. Now, I can keep the variable name, same, as "info" 
and not change it to "soc_info" if the forum suggests that, just that we 
thought it feels more descriptive to change it to "soc_info", when we 
make it's scope to file. Also, in future, with this variable global, if 
we decide to support kernel clients by exporting these fields through 
APIs, we can easily make use of this and implement.


>
> ---Trilok Soni


Thanks,

Naman Jain

Re: [PATCH 2/2] soc: qcom: socinfo: Add sysfs attributes for fields in v2-v6

[Naman Jain]

Hi Trilok,

On 1/12/2023 5:28 AM, Trilok Soni wrote:
> On 1/11/2023 3:19 PM, Dmitry Baryshkov wrote:
>> On 11/01/2023 10:21, Naman Jain wrote:
>>> Add support in sysfs custom attributes for fields in socinfo version
>>> v2-v6. This is to support SoC based operations in userland scripts
>>> and test scripts. Also, add name mappings for hw-platform type to
>>> make the sysfs information more descriptive.
>>
>> Please include a patch documenting your additions to 
>> Documentation/ABI/testing/sysfs-devices-soc. Please describe usecases 
>> for new attributes and their applicability to non-Qualcomm boards.
>>
>> Note, that testing scripts can access debugfs entries without any 
>> issues.
>
> The commit text mentions the "userland" scripts and it could mean the 
> product OS like Android or Yocto having the applications using these 
> /sysfs entries. Naman, please clarify if the vendor application layer 
> in the Android is using these Entries to make decisions based on the 
> platforms / soc information?


That may have been a wrong choice of word here, if that is what it 
means. The use of these interfaces is in post boot shell scripts, and 
userspace services. As these are Qualcomm specific, I don't think we 
have any examples in Android. I have now mentioned the use-cases of 
sysfs interface, in my reply on Dmitry's email on PATCH 2/2. Have added 
you there to avoid duplication.


>
> ---Trilok Soni


Thanks,

Naman Jain

Re: [PATCH 2/2] soc: qcom: socinfo: Add sysfs attributes for fields in v2-v6

[Naman Jain]

Thanks Dmitry for reviewing the patches. Sorry, for replying late on 
your email, I wanted to collect all the information, before I do it.

On 1/12/2023 4:49 AM, Dmitry Baryshkov wrote:
> On 11/01/2023 10:21, Naman Jain wrote:
>> Add support in sysfs custom attributes for fields in socinfo version
>> v2-v6. This is to support SoC based operations in userland scripts
>> and test scripts. Also, add name mappings for hw-platform type to
>> make the sysfs information more descriptive.
>
> Please include a patch documenting your additions to 
> Documentation/ABI/testing/sysfs-devices-soc. Please describe usecases 
> for new attributes and their applicability to non-Qualcomm boards.
>

The fields added here, are applicable to Qualcomm boards only. I can 
include in the same file sysfs-devices-soc, mentioning the same that it 
is Qcom specific, or I can create a new file for this, 
sysfs-devices-soc-qcom, however you suggest. Mentioning the use cases, 
later in the mail.


> Note, that testing scripts can access debugfs entries without any issues.


Yes, that is right. Thanks.


>
>>
>> Signed-off-by: Naman Jain <quic_namajain@quicinc.com>
>> ---
>>   drivers/soc/qcom/socinfo.c | 181 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 181 insertions(+)
>>
>> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
>> index 251c0fd94962..ff92064c2246 100644
>> --- a/drivers/soc/qcom/socinfo.c
>> +++ b/drivers/soc/qcom/socinfo.c
>> @@ -41,6 +41,52 @@
>>    */
>>   #define SMEM_HW_SW_BUILD_ID            137
>>   +enum {
>> +    HW_PLATFORM_UNKNOWN = 0,
>> +    HW_PLATFORM_SURF = 1,
>> +    HW_PLATFORM_FFA = 2,
>> +    HW_PLATFORM_FLUID = 3,
>> +    HW_PLATFORM_SVLTE_FFA = 4,
>> +    HW_PLATFORM_SVLTE_SURF = 5,
>> +    HW_PLATFORM_MTP_MDM = 7,
>> +    HW_PLATFORM_MTP = 8,
>> +    HW_PLATFORM_LIQUID = 9,
>> +    HW_PLATFORM_DRAGON = 10,
>> +    HW_PLATFORM_QRD = 11,
>> +    HW_PLATFORM_HRD = 13,
>> +    HW_PLATFORM_DTV = 14,
>> +    HW_PLATFORM_RCM = 21,
>> +    HW_PLATFORM_STP = 23,
>> +    HW_PLATFORM_SBC = 24,
>> +    HW_PLATFORM_HDK = 31,
>> +    HW_PLATFORM_ATP = 33,
>> +    HW_PLATFORM_IDP = 34,
>> +    HW_PLATFORM_INVALID
>> +};
>> +
>> +static const char * const hw_platform[] = {
>> +    [HW_PLATFORM_UNKNOWN] = "Unknown",
>> +    [HW_PLATFORM_SURF] = "Surf",
>> +    [HW_PLATFORM_FFA] = "FFA",
>> +    [HW_PLATFORM_FLUID] = "Fluid",
>> +    [HW_PLATFORM_SVLTE_FFA] = "SVLTE_FFA",
>> +    [HW_PLATFORM_SVLTE_SURF] = "SLVTE_SURF",
>> +    [HW_PLATFORM_MTP_MDM] = "MDM_MTP_NO_DISPLAY",
>> +    [HW_PLATFORM_MTP] = "MTP",
>> +    [HW_PLATFORM_RCM] = "RCM",
>> +    [HW_PLATFORM_LIQUID] = "Liquid",
>> +    [HW_PLATFORM_DRAGON] = "Dragon",
>> +    [HW_PLATFORM_QRD] = "QRD",
>> +    [HW_PLATFORM_HRD] = "HRD",
>> +    [HW_PLATFORM_DTV] = "DTV",
>> +    [HW_PLATFORM_STP] = "STP",
>> +    [HW_PLATFORM_SBC] = "SBC",
>> +    [HW_PLATFORM_HDK] = "HDK",
>> +    [HW_PLATFORM_ATP] = "ATP",
>> +    [HW_PLATFORM_IDP] = "IDP",
>> +    [HW_PLATFORM_INVALID] = "Invalid",
>> +};
>
> This is not a property of the SoC. It is a property of the device. As 
> such it should not be part of /sys/bus/soc devices.


I understand your point. The Socinfo structure as such on Qualcomm SoC 
gives not just SoC related information but also many other info like 
serial number, platform subtype etc. Now in order to support the 
usecases below, we are proposing sysfs interface extension, as we can't 
use debugfs interface in production/end user devices due to debugfs 
access restrictions.

Use cases:

1. In post-boot shell scripts, for various chip specific operations, 
that are relevant to that particular chip/board only:

     a. Setting kernel parameters using sysfs interfaces etc.

     b. Enabling particular traces, logs

     c. Changing permissions to certain paths

     d. Start a userspace service, and pass custom parameters to it on 
the fly

     e. Set certain device properties using setprop

     f. Miscellaneous things like DCC (Data Capture and Compare Engine) etc.

2. In userspace services, that depend on SoC information, for its 
configuration. Eg: Audio, Connectivity services use these.

3. adb needs device serial number, sensors need SoC information to 
decide its configuration.


>
> You can find board description in /sys/firmware/devicetree/base/model


Thanks for pointing this out. This is giving useful information on the 
chip and hw_platform, but the problem is that we need other fields as 
well, which we may want to use. Hence the ask.

model = "Qualcomm Technologies, Inc. Kalama MTP";


>
>> +
>>   #ifdef CONFIG_DEBUG_FS
>>   #define SMEM_IMAGE_VERSION_BLOCKS_COUNT        32
>>   #define SMEM_IMAGE_VERSION_SIZE                4096
>> @@ -368,6 +414,140 @@ static const struct soc_id soc_id[] = {
>>       { qcom_board_id(QRU1062) },
>>   };
>>   +/* sysfs attributes */
>> +#define ATTR_DEFINE(param) \
>> +    static DEVICE_ATTR(param, 0644, qcom_get_##param, NULL)
>> +
>> +/* Version 2 */
>> +static ssize_t
>> +qcom_get_raw_id(struct device *dev,
>> +        struct device_attribute *attr,
>> +        char *buf)
>> +{
>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>> +             le32_to_cpu(soc_info->raw_id));
>> +}
>> +ATTR_DEFINE(raw_id);
>> +
>> +static ssize_t
>> +qcom_get_raw_version(struct device *dev,
>> +        struct device_attribute *attr,
>> +        char *buf)
>> +{
>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>> +             le32_to_cpu(soc_info->raw_ver));
>> +}
>> +ATTR_DEFINE(raw_version);
>
> Why are they raw? can you unraw them?
>
> Whose version and id are these attributes referring to?


So basically, when we call them raw, it essentially means that it is not 
parsed as such (different bits may be giving different information, and 
the whole value may mean nothing).

*version* refers to the chip version, which can be like v1, v2, v1.1 etc 
in real terms. Its raw value is used to map it to one of these versions. 
*id* is used as chip ID for QC SoCs for using JTAG. It is different than 
the soc_id that we have.


>
>> +
>> +/* Version 3 */
>> +static ssize_t
>> +qcom_get_hw_platform(struct device *dev,
>> +        struct device_attribute *attr,
>> +        char *buf)
>> +{
>> +    uint32_t hw_plat = le32_to_cpu(soc_info->hw_plat);
>> +
>> +    hw_plat = (hw_plat >= HW_PLATFORM_INVALID) ? HW_PLATFORM_INVALID 
>> : hw_plat;
>> +    return scnprintf(buf, PAGE_SIZE, "%-.32s\n",
>> +            hw_platform[hw_plat]);
>> +}
>> +ATTR_DEFINE(hw_platform);
>> +
>> +/* Version 4 */
>> +static ssize_t
>> +qcom_get_platform_version(struct device *dev,
>> +        struct device_attribute *attr,
>> +        char *buf)
>> +{
>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>> +             le32_to_cpu(soc_info->plat_ver));
>> +}
>> +ATTR_DEFINE(platform_version);
>> +
>> +/* Version 5 */
>> +static ssize_t
>> +qcom_get_accessory_chip(struct device *dev,
>> +        struct device_attribute *attr,
>> +        char *buf)
>> +{
>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>> +            le32_to_cpu(soc_info->accessory_chip));
>> +}
>> +ATTR_DEFINE(accessory_chip);
>
> If this an _accessory_ chip, there should be a separate soc device 
> describing it, rather than stuffing information into the soc0.
>

This is used as a boolean currently to tell us whether SoC has an 
accessory chip or not.


>> +
>> +/* Version 6 */
>> +static ssize_t
>> +qcom_get_platform_subtype_id(struct device *dev,
>> +        struct device_attribute *attr,
>> +        char *buf)
>> +{
>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>> +             le32_to_cpu(soc_info->hw_plat_subtype));
>> +}
>> +ATTR_DEFINE(platform_subtype_id);
>
> Again, this is the board property, not an SoC one.


Same justification as one of my previous comments.


>
>> +
>> +static struct attribute *qcom_custom_socinfo_attrs[7];
>> +
>> +static const struct attribute_group custom_soc_attr_group = {
>> +    .attrs = qcom_custom_socinfo_attrs,
>> +};
>> +
>> +static void qcom_socinfo_populate_sysfs(struct qcom_socinfo 
>> *qcom_socinfo)
>> +{
>> +    int i = 0, socinfo_format = le32_to_cpu(soc_info->fmt);
>> +
>> +    /* Note: qcom_custom_socinfo_attrs[] size needs to be in sync 
>> with attributes added here. */
>> +    switch (socinfo_format) {
>> +    case SOCINFO_VERSION(0, 16):
>> +        fallthrough;
>> +    case SOCINFO_VERSION(0, 15):
>> +        fallthrough;
>> +    case SOCINFO_VERSION(0, 14):
>> +        fallthrough;
>> +    case SOCINFO_VERSION(0, 13):
>> +        fallthrough;
>> +    case SOCINFO_VERSION(0, 12):
>> +        fallthrough;
>> +    case SOCINFO_VERSION(0, 11):
>> +        fallthrough;
>> +    case SOCINFO_VERSION(0, 10):
>> +        fallthrough;
>> +    case SOCINFO_VERSION(0, 9):
>> +        fallthrough;
>> +    case SOCINFO_VERSION(0, 8):
>> +        fallthrough;
>> +    case SOCINFO_VERSION(0, 7):
>> +        fallthrough;
>> +    case SOCINFO_VERSION(0, 6):
>> +        qcom_custom_socinfo_attrs[i++] =
>> +            &dev_attr_platform_subtype_id.attr;
>> +        fallthrough;
>> +    case SOCINFO_VERSION(0, 5):
>> +        qcom_custom_socinfo_attrs[i++] = &dev_attr_accessory_chip.attr;
>> +        fallthrough;
>> +    case SOCINFO_VERSION(0, 4):
>> +        qcom_custom_socinfo_attrs[i++] = 
>> &dev_attr_platform_version.attr;
>> +        fallthrough;
>> +    case SOCINFO_VERSION(0, 3):
>> +        qcom_custom_socinfo_attrs[i++] = &dev_attr_hw_platform.attr;
>> +        fallthrough;
>> +    case SOCINFO_VERSION(0, 2):
>> +        qcom_custom_socinfo_attrs[i++] = &dev_attr_raw_id.attr;
>> +        qcom_custom_socinfo_attrs[i++] = &dev_attr_raw_version.attr;
>> +        fallthrough;
>> +    case SOCINFO_VERSION(0, 1):
>> +        break;
>> +    default:
>> +        pr_err("Unknown socinfo format: v%u.%u\n",
>> +                SOCINFO_MAJOR(socinfo_format),
>> +                SOCINFO_MINOR(socinfo_format));
>> +        break;
>> +    }
>> +
>> +    qcom_custom_socinfo_attrs[i] = NULL;
>> +    qcom_socinfo->attr.custom_attr_group = &custom_soc_attr_group;
>> +}
>> +
>>   static const char *socinfo_machine(struct device *dev, unsigned int 
>> id)
>>   {
>>       int idx;
>> @@ -696,6 +876,7 @@ static int qcom_socinfo_probe(struct 
>> platform_device *pdev)
>>                               "%u",
>> le32_to_cpu(soc_info->serial_num));
>>   +    qcom_socinfo_populate_sysfs(qs);
>>       qs->soc_dev = soc_device_register(&qs->attr);
>>       if (IS_ERR(qs->soc_dev))
>>           return PTR_ERR(qs->soc_dev);
>

Thanks,

Naman Jain

Re: [PATCH 2/2] soc: qcom: socinfo: Add sysfs attributes for fields in v2-v6

[Dmitry Baryshkov]

On 19/01/2023 11:39, Naman Jain wrote:
> Thanks Dmitry for reviewing the patches. Sorry, for replying late on 
> your email, I wanted to collect all the information, before I do it.
> 
> On 1/12/2023 4:49 AM, Dmitry Baryshkov wrote:
>> On 11/01/2023 10:21, Naman Jain wrote:
>>> Add support in sysfs custom attributes for fields in socinfo version
>>> v2-v6. This is to support SoC based operations in userland scripts
>>> and test scripts. Also, add name mappings for hw-platform type to
>>> make the sysfs information more descriptive.
>>
>> Please include a patch documenting your additions to 
>> Documentation/ABI/testing/sysfs-devices-soc. Please describe usecases 
>> for new attributes and their applicability to non-Qualcomm boards.
>>
> 
> The fields added here, are applicable to Qualcomm boards only. I can 
> include in the same file sysfs-devices-soc, mentioning the same that it 
> is Qcom specific, or I can create a new file for this, 
> sysfs-devices-soc-qcom, however you suggest. Mentioning the use cases, 
> later in the mail.

So, you are extending the generic SoC interface with the vendor-specific 
interfaces. There must be a file describing them in a generic enough way 
that other vendors can apply for their boards too.

Note, that /sys/devices/soc applies to SoC level, not the board level. 
Generally I think that you should export your data through a more 
generic data path, e.g. /sys/firmware.

> 
> 
>> Note, that testing scripts can access debugfs entries without any issues.
> 
> 
> Yes, that is right. Thanks.
> 
> 
>>
>>>
>>> Signed-off-by: Naman Jain <quic_namajain@quicinc.com>
>>> ---
>>>   drivers/soc/qcom/socinfo.c | 181 +++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 181 insertions(+)
>>>
>>> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
>>> index 251c0fd94962..ff92064c2246 100644
>>> --- a/drivers/soc/qcom/socinfo.c
>>> +++ b/drivers/soc/qcom/socinfo.c
>>> @@ -41,6 +41,52 @@
>>>    */
>>>   #define SMEM_HW_SW_BUILD_ID            137
>>>   +enum {
>>> +    HW_PLATFORM_UNKNOWN = 0,
>>> +    HW_PLATFORM_SURF = 1,
>>> +    HW_PLATFORM_FFA = 2,
>>> +    HW_PLATFORM_FLUID = 3,
>>> +    HW_PLATFORM_SVLTE_FFA = 4,
>>> +    HW_PLATFORM_SVLTE_SURF = 5,
>>> +    HW_PLATFORM_MTP_MDM = 7,
>>> +    HW_PLATFORM_MTP = 8,
>>> +    HW_PLATFORM_LIQUID = 9,
>>> +    HW_PLATFORM_DRAGON = 10,
>>> +    HW_PLATFORM_QRD = 11,
>>> +    HW_PLATFORM_HRD = 13,
>>> +    HW_PLATFORM_DTV = 14,
>>> +    HW_PLATFORM_RCM = 21,
>>> +    HW_PLATFORM_STP = 23,
>>> +    HW_PLATFORM_SBC = 24,
>>> +    HW_PLATFORM_HDK = 31,
>>> +    HW_PLATFORM_ATP = 33,
>>> +    HW_PLATFORM_IDP = 34,
>>> +    HW_PLATFORM_INVALID
>>> +};
>>> +
>>> +static const char * const hw_platform[] = {
>>> +    [HW_PLATFORM_UNKNOWN] = "Unknown",
>>> +    [HW_PLATFORM_SURF] = "Surf",
>>> +    [HW_PLATFORM_FFA] = "FFA",
>>> +    [HW_PLATFORM_FLUID] = "Fluid",
>>> +    [HW_PLATFORM_SVLTE_FFA] = "SVLTE_FFA",
>>> +    [HW_PLATFORM_SVLTE_SURF] = "SLVTE_SURF",
>>> +    [HW_PLATFORM_MTP_MDM] = "MDM_MTP_NO_DISPLAY",
>>> +    [HW_PLATFORM_MTP] = "MTP",
>>> +    [HW_PLATFORM_RCM] = "RCM",
>>> +    [HW_PLATFORM_LIQUID] = "Liquid",
>>> +    [HW_PLATFORM_DRAGON] = "Dragon",
>>> +    [HW_PLATFORM_QRD] = "QRD",
>>> +    [HW_PLATFORM_HRD] = "HRD",
>>> +    [HW_PLATFORM_DTV] = "DTV",
>>> +    [HW_PLATFORM_STP] = "STP",
>>> +    [HW_PLATFORM_SBC] = "SBC",
>>> +    [HW_PLATFORM_HDK] = "HDK",
>>> +    [HW_PLATFORM_ATP] = "ATP",
>>> +    [HW_PLATFORM_IDP] = "IDP",
>>> +    [HW_PLATFORM_INVALID] = "Invalid",
>>> +};
>>
>> This is not a property of the SoC. It is a property of the device. As 
>> such it should not be part of /sys/bus/soc devices.
> 
> 
> I understand your point. The Socinfo structure as such on Qualcomm SoC 
> gives not just SoC related information but also many other info like 
> serial number, platform subtype etc. Now in order to support the 
> usecases below, we are proposing sysfs interface extension, as we can't 
> use debugfs interface in production/end user devices due to debugfs 
> access restrictions.

"The vendor does it in this way" doesn't give you a right to repurpose 
the ABI.

> 
> Use cases:
> 
> 1. In post-boot shell scripts, for various chip specific operations, 
> that are relevant to that particular chip/board only:
> 
>      a. Setting kernel parameters using sysfs interfaces etc.

If the parameter is common to all devices of some kind, it should be set 
by the driver using the data in the DTS. See, how this is managed for 
PHY tunings. You can not expect for the userspace to function in any 
particular way. The whole userspace might be a single /bin/bash 
executing commands and/or scripts. And still the device should function 
_properly_.

> 
>      b. Enabling particular traces, logs

This should not depend on the device type. If you have something 
hw-specific, check the particular device instance rather than checking 
the board kind.

> 
>      c. Changing permissions to certain paths

Excuse me, what paths? Permissions have nothing to do with the board kind.

> 
>      d. Start a userspace service, and pass custom parameters to it on 
> the fly

I think this also depends on the hardware availability rather than the 
board properties.

> 
>      e. Set certain device properties using setprop

Android specifics. Please formulate this in a generic way.

> 
>      f. Miscellaneous things like DCC (Data Capture and Compare Engine) 
> etc.

Please expand this, you can not expect one to know what is DCC and how 
it is used.

> 
> 2. In userspace services, that depend on SoC information, for its 
> configuration. Eg: Audio, Connectivity services use these.

This is handled using the device ids, models, etc.. Please see, how this 
is handled by other software (hint: ALSA UCM, pulseaudio) instead of 
inventing something vendor-specific.

> 
> 3. adb needs device serial number, sensors need SoC information to 
> decide its configuration.

Already available via /proc/cmdline thanks for your bootloader.

> 
> 
>>
>> You can find board description in /sys/firmware/devicetree/base/model
> 
> 
> Thanks for pointing this out. This is giving useful information on the 
> chip and hw_platform, but the problem is that we need other fields as 
> well, which we may want to use. Hence the ask.
> 
> model = "Qualcomm Technologies, Inc. Kalama MTP";

Generally I think that Qualcomm's socinfo is a kind of firmware 
interface, so you can probably extend /sys/firmware to provide this kind 
of information.

> 
> 
>>
>>> +
>>>   #ifdef CONFIG_DEBUG_FS
>>>   #define SMEM_IMAGE_VERSION_BLOCKS_COUNT        32
>>>   #define SMEM_IMAGE_VERSION_SIZE                4096
>>> @@ -368,6 +414,140 @@ static const struct soc_id soc_id[] = {
>>>       { qcom_board_id(QRU1062) },
>>>   };
>>>   +/* sysfs attributes */
>>> +#define ATTR_DEFINE(param) \
>>> +    static DEVICE_ATTR(param, 0644, qcom_get_##param, NULL)
>>> +
>>> +/* Version 2 */
>>> +static ssize_t
>>> +qcom_get_raw_id(struct device *dev,
>>> +        struct device_attribute *attr,
>>> +        char *buf)
>>> +{
>>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>>> +             le32_to_cpu(soc_info->raw_id));
>>> +}
>>> +ATTR_DEFINE(raw_id);
>>> +
>>> +static ssize_t
>>> +qcom_get_raw_version(struct device *dev,
>>> +        struct device_attribute *attr,
>>> +        char *buf)
>>> +{
>>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>>> +             le32_to_cpu(soc_info->raw_ver));
>>> +}
>>> +ATTR_DEFINE(raw_version);
>>
>> Why are they raw? can you unraw them?
>>
>> Whose version and id are these attributes referring to?
> 
> 
> So basically, when we call them raw, it essentially means that it is not 
> parsed as such (different bits may be giving different information, and 
> the whole value may mean nothing).
> 
> *version* refers to the chip version, which can be like v1, v2, v1.1 etc 
> in real terms. Its raw value is used to map it to one of these versions. 
> *id* is used as chip ID for QC SoCs for using JTAG. It is different than 
> the soc_id that we have.

Unraw the values.

> 
> 
>>
>>> +
>>> +/* Version 3 */
>>> +static ssize_t
>>> +qcom_get_hw_platform(struct device *dev,
>>> +        struct device_attribute *attr,
>>> +        char *buf)
>>> +{
>>> +    uint32_t hw_plat = le32_to_cpu(soc_info->hw_plat);
>>> +
>>> +    hw_plat = (hw_plat >= HW_PLATFORM_INVALID) ? HW_PLATFORM_INVALID 
>>> : hw_plat;
>>> +    return scnprintf(buf, PAGE_SIZE, "%-.32s\n",
>>> +            hw_platform[hw_plat]);
>>> +}
>>> +ATTR_DEFINE(hw_platform);
>>> +
>>> +/* Version 4 */
>>> +static ssize_t
>>> +qcom_get_platform_version(struct device *dev,
>>> +        struct device_attribute *attr,
>>> +        char *buf)
>>> +{
>>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>>> +             le32_to_cpu(soc_info->plat_ver));
>>> +}
>>> +ATTR_DEFINE(platform_version);
>>> +
>>> +/* Version 5 */
>>> +static ssize_t
>>> +qcom_get_accessory_chip(struct device *dev,
>>> +        struct device_attribute *attr,
>>> +        char *buf)
>>> +{
>>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>>> +            le32_to_cpu(soc_info->accessory_chip));
>>> +}
>>> +ATTR_DEFINE(accessory_chip);
>>
>> If this an _accessory_ chip, there should be a separate soc device 
>> describing it, rather than stuffing information into the soc0.
>>
> 
> This is used as a boolean currently to tell us whether SoC has an 
> accessory chip or not.

SoC doesn't have accessory chip. It the board having the accessory (to 
the main SoC) or not.

Also, please do not use 'currently' for the sysfs files. They are ABI. 
And changing ABI is a painful process which might be not available at 
all. So once you export something through the sysfs, it is written in 
stone. Not 'currently, to be changed later'.

> 
> 
>>> +
>>> +/* Version 6 */
>>> +static ssize_t
>>> +qcom_get_platform_subtype_id(struct device *dev,
>>> +        struct device_attribute *attr,
>>> +        char *buf)
>>> +{
>>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>>> +             le32_to_cpu(soc_info->hw_plat_subtype));
>>> +}
>>> +ATTR_DEFINE(platform_subtype_id);
>>
>> Again, this is the board property, not an SoC one.
> 
> 
> Same justification as one of my previous comments.

Same comment. /sys/bus/soc exists to export information about, you 
guess, SoC. If you want to export information about the board, please 
find a better way.


-- 
With best wishes
Dmitry

Re: [PATCH 2/2] soc: qcom: socinfo: Add sysfs attributes for fields in v2-v6

[Naman Jain]

On 1/19/2023 4:29 PM, Dmitry Baryshkov wrote:
> On 19/01/2023 11:39, Naman Jain wrote:
>> Thanks Dmitry for reviewing the patches. Sorry, for replying late on 
>> your email, I wanted to collect all the information, before I do it.
>>
>> On 1/12/2023 4:49 AM, Dmitry Baryshkov wrote:
>>> On 11/01/2023 10:21, Naman Jain wrote:
>>>> Add support in sysfs custom attributes for fields in socinfo version
>>>> v2-v6. This is to support SoC based operations in userland scripts
>>>> and test scripts. Also, add name mappings for hw-platform type to
>>>> make the sysfs information more descriptive.
>>>
>>> Please include a patch documenting your additions to 
>>> Documentation/ABI/testing/sysfs-devices-soc. Please describe 
>>> usecases for new attributes and their applicability to non-Qualcomm 
>>> boards.
>>>
>>
>> The fields added here, are applicable to Qualcomm boards only. I can 
>> include in the same file sysfs-devices-soc, mentioning the same that 
>> it is Qcom specific, or I can create a new file for this, 
>> sysfs-devices-soc-qcom, however you suggest. Mentioning the use 
>> cases, later in the mail.
>
> So, you are extending the generic SoC interface with the 
> vendor-specific interfaces. There must be a file describing them in a 
> generic enough way that other vendors can apply for their boards too.
>
> Note, that /sys/devices/soc applies to SoC level, not the board level. 
> Generally I think that you should export your data through a more 
> generic data path, e.g. /sys/firmware.


Understood, will keep that in mind.


>
>>
>>
>>> Note, that testing scripts can access debugfs entries without any 
>>> issues.
>>
>>
>> Yes, that is right. Thanks.
>>
>>
>>>
>>>>
>>>> Signed-off-by: Naman Jain <quic_namajain@quicinc.com>
>>>> ---
>>>>   drivers/soc/qcom/socinfo.c | 181 
>>>> +++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 181 insertions(+)
>>>>
>>>> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
>>>> index 251c0fd94962..ff92064c2246 100644
>>>> --- a/drivers/soc/qcom/socinfo.c
>>>> +++ b/drivers/soc/qcom/socinfo.c
>>>> @@ -41,6 +41,52 @@
>>>>    */
>>>>   #define SMEM_HW_SW_BUILD_ID            137
>>>>   +enum {
>>>> +    HW_PLATFORM_UNKNOWN = 0,
>>>> +    HW_PLATFORM_SURF = 1,
>>>> +    HW_PLATFORM_FFA = 2,
>>>> +    HW_PLATFORM_FLUID = 3,
>>>> +    HW_PLATFORM_SVLTE_FFA = 4,
>>>> +    HW_PLATFORM_SVLTE_SURF = 5,
>>>> +    HW_PLATFORM_MTP_MDM = 7,
>>>> +    HW_PLATFORM_MTP = 8,
>>>> +    HW_PLATFORM_LIQUID = 9,
>>>> +    HW_PLATFORM_DRAGON = 10,
>>>> +    HW_PLATFORM_QRD = 11,
>>>> +    HW_PLATFORM_HRD = 13,
>>>> +    HW_PLATFORM_DTV = 14,
>>>> +    HW_PLATFORM_RCM = 21,
>>>> +    HW_PLATFORM_STP = 23,
>>>> +    HW_PLATFORM_SBC = 24,
>>>> +    HW_PLATFORM_HDK = 31,
>>>> +    HW_PLATFORM_ATP = 33,
>>>> +    HW_PLATFORM_IDP = 34,
>>>> +    HW_PLATFORM_INVALID
>>>> +};
>>>> +
>>>> +static const char * const hw_platform[] = {
>>>> +    [HW_PLATFORM_UNKNOWN] = "Unknown",
>>>> +    [HW_PLATFORM_SURF] = "Surf",
>>>> +    [HW_PLATFORM_FFA] = "FFA",
>>>> +    [HW_PLATFORM_FLUID] = "Fluid",
>>>> +    [HW_PLATFORM_SVLTE_FFA] = "SVLTE_FFA",
>>>> +    [HW_PLATFORM_SVLTE_SURF] = "SLVTE_SURF",
>>>> +    [HW_PLATFORM_MTP_MDM] = "MDM_MTP_NO_DISPLAY",
>>>> +    [HW_PLATFORM_MTP] = "MTP",
>>>> +    [HW_PLATFORM_RCM] = "RCM",
>>>> +    [HW_PLATFORM_LIQUID] = "Liquid",
>>>> +    [HW_PLATFORM_DRAGON] = "Dragon",
>>>> +    [HW_PLATFORM_QRD] = "QRD",
>>>> +    [HW_PLATFORM_HRD] = "HRD",
>>>> +    [HW_PLATFORM_DTV] = "DTV",
>>>> +    [HW_PLATFORM_STP] = "STP",
>>>> +    [HW_PLATFORM_SBC] = "SBC",
>>>> +    [HW_PLATFORM_HDK] = "HDK",
>>>> +    [HW_PLATFORM_ATP] = "ATP",
>>>> +    [HW_PLATFORM_IDP] = "IDP",
>>>> +    [HW_PLATFORM_INVALID] = "Invalid",
>>>> +};
>>>
>>> This is not a property of the SoC. It is a property of the device. 
>>> As such it should not be part of /sys/bus/soc devices.
>>
>>
>> I understand your point. The Socinfo structure as such on Qualcomm 
>> SoC gives not just SoC related information but also many other info 
>> like serial number, platform subtype etc. Now in order to support the 
>> usecases below, we are proposing sysfs interface extension, as we 
>> can't use debugfs interface in production/end user devices due to 
>> debugfs access restrictions.
>
> "The vendor does it in this way" doesn't give you a right to repurpose 
> the ABI.


Got it.


>
>>
>> Use cases:
>>
>> 1. In post-boot shell scripts, for various chip specific operations, 
>> that are relevant to that particular chip/board only:
>>
>>      a. Setting kernel parameters using sysfs interfaces etc.
>
> If the parameter is common to all devices of some kind, it should be 
> set by the driver using the data in the DTS. See, how this is managed 
> for PHY tunings. You can not expect for the userspace to function in 
> any particular way. The whole userspace might be a single /bin/bash 
> executing commands and/or scripts. And still the device should 
> function _properly_.


OK.


>
>>
>>      b. Enabling particular traces, logs
>
> This should not depend on the device type. If you have something 
> hw-specific, check the particular device instance rather than checking 
> the board kind.


Got it.


>
>>
>>      c. Changing permissions to certain paths
>
> Excuse me, what paths? Permissions have nothing to do with the board 
> kind.


I think, the solution to these type of use-cases, would fall under the 
umbrella of your previous comment " If you have something hw-specific, 
check the particular device instance rather than checking the board 
kind.". Thanks.


>
>>
>>      d. Start a userspace service, and pass custom parameters to it 
>> on the fly
>
> I think this also depends on the hardware availability rather than the 
> board properties.


OK.


>
>>
>>      e. Set certain device properties using setprop
>
> Android specifics. Please formulate this in a generic way.


Will do.


>
>>
>>      f. Miscellaneous things like DCC (Data Capture and Compare 
>> Engine) etc.
>
> Please expand this, you can not expect one to know what is DCC and how 
> it is used.
>
>>
>> 2. In userspace services, that depend on SoC information, for its 
>> configuration. Eg: Audio, Connectivity services use these.
>
> This is handled using the device ids, models, etc.. Please see, how 
> this is handled by other software (hint: ALSA UCM, pulseaudio) instead 
> of inventing something vendor-specific.


Noted.


>
>>
>> 3. adb needs device serial number, sensors need SoC information to 
>> decide its configuration.
>
> Already available via /proc/cmdline thanks for your bootloader.


Noted. Thanks


>
>>
>>
>>>
>>> You can find board description in /sys/firmware/devicetree/base/model
>>
>>
>> Thanks for pointing this out. This is giving useful information on 
>> the chip and hw_platform, but the problem is that we need other 
>> fields as well, which we may want to use. Hence the ask.
>>
>> model = "Qualcomm Technologies, Inc. Kalama MTP";
>
> Generally I think that Qualcomm's socinfo is a kind of firmware 
> interface, so you can probably extend /sys/firmware to provide this 
> kind of information.


OK, will check. Thanks.


>
>>
>>
>>>
>>>> +
>>>>   #ifdef CONFIG_DEBUG_FS
>>>>   #define SMEM_IMAGE_VERSION_BLOCKS_COUNT        32
>>>>   #define SMEM_IMAGE_VERSION_SIZE                4096
>>>> @@ -368,6 +414,140 @@ static const struct soc_id soc_id[] = {
>>>>       { qcom_board_id(QRU1062) },
>>>>   };
>>>>   +/* sysfs attributes */
>>>> +#define ATTR_DEFINE(param) \
>>>> +    static DEVICE_ATTR(param, 0644, qcom_get_##param, NULL)
>>>> +
>>>> +/* Version 2 */
>>>> +static ssize_t
>>>> +qcom_get_raw_id(struct device *dev,
>>>> +        struct device_attribute *attr,
>>>> +        char *buf)
>>>> +{
>>>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>>>> +             le32_to_cpu(soc_info->raw_id));
>>>> +}
>>>> +ATTR_DEFINE(raw_id);
>>>> +
>>>> +static ssize_t
>>>> +qcom_get_raw_version(struct device *dev,
>>>> +        struct device_attribute *attr,
>>>> +        char *buf)
>>>> +{
>>>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>>>> +             le32_to_cpu(soc_info->raw_ver));
>>>> +}
>>>> +ATTR_DEFINE(raw_version);
>>>
>>> Why are they raw? can you unraw them?
>>>
>>> Whose version and id are these attributes referring to?
>>
>>
>> So basically, when we call them raw, it essentially means that it is 
>> not parsed as such (different bits may be giving different 
>> information, and the whole value may mean nothing).
>>
>> *version* refers to the chip version, which can be like v1, v2, v1.1 
>> etc in real terms. Its raw value is used to map it to one of these 
>> versions. *id* is used as chip ID for QC SoCs for using JTAG. It is 
>> different than the soc_id that we have.
>
> Unraw the values.
>
>>
>>
>>>
>>>> +
>>>> +/* Version 3 */
>>>> +static ssize_t
>>>> +qcom_get_hw_platform(struct device *dev,
>>>> +        struct device_attribute *attr,
>>>> +        char *buf)
>>>> +{
>>>> +    uint32_t hw_plat = le32_to_cpu(soc_info->hw_plat);
>>>> +
>>>> +    hw_plat = (hw_plat >= HW_PLATFORM_INVALID) ? 
>>>> HW_PLATFORM_INVALID : hw_plat;
>>>> +    return scnprintf(buf, PAGE_SIZE, "%-.32s\n",
>>>> +            hw_platform[hw_plat]);
>>>> +}
>>>> +ATTR_DEFINE(hw_platform);
>>>> +
>>>> +/* Version 4 */
>>>> +static ssize_t
>>>> +qcom_get_platform_version(struct device *dev,
>>>> +        struct device_attribute *attr,
>>>> +        char *buf)
>>>> +{
>>>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>>>> +             le32_to_cpu(soc_info->plat_ver));
>>>> +}
>>>> +ATTR_DEFINE(platform_version);
>>>> +
>>>> +/* Version 5 */
>>>> +static ssize_t
>>>> +qcom_get_accessory_chip(struct device *dev,
>>>> +        struct device_attribute *attr,
>>>> +        char *buf)
>>>> +{
>>>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>>>> +            le32_to_cpu(soc_info->accessory_chip));
>>>> +}
>>>> +ATTR_DEFINE(accessory_chip);
>>>
>>> If this an _accessory_ chip, there should be a separate soc device 
>>> describing it, rather than stuffing information into the soc0.
>>>
>>
>> This is used as a boolean currently to tell us whether SoC has an 
>> accessory chip or not.
>
> SoC doesn't have accessory chip. It the board having the accessory (to 
> the main SoC) or not.
>
> Also, please do not use 'currently' for the sysfs files. They are ABI. 
> And changing ABI is a painful process which might be not available at 
> all. So once you export something through the sysfs, it is written in 
> stone. Not 'currently, to be changed later'.
>

My bad. That may have been just a word, that I use frequently. Totally 
got your point.


>>
>>
>>>> +
>>>> +/* Version 6 */
>>>> +static ssize_t
>>>> +qcom_get_platform_subtype_id(struct device *dev,
>>>> +        struct device_attribute *attr,
>>>> +        char *buf)
>>>> +{
>>>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>>>> +             le32_to_cpu(soc_info->hw_plat_subtype));
>>>> +}
>>>> +ATTR_DEFINE(platform_subtype_id);
>>>
>>> Again, this is the board property, not an SoC one.
>>
>>
>> Same justification as one of my previous comments.
>
> Same comment. /sys/bus/soc exists to export information about, you 
> guess, SoC. If you want to export information about the board, please 
> find a better way.
>
>

Thanks Dmitry for reviewing. Understood your points. Let us re-evaluate, 
what fields are coming under SoC, what are required and why, and we will 
start the discussion again with the new requirements, if any.


Regards,

Naman Jain