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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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