2023-01-11 08:26:22

by Naman Jain

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

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


2023-01-11 08:53:22

by Naman Jain

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

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 <[email protected]>
---
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

2023-01-11 22:49:44

by Trilok Soni

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

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 <[email protected]>
> ---
> 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

2023-01-19 10:40:33

by Naman Jain

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

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 <[email protected]>
>> ---
>>   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