soc: qcom: Add SoC info driver
Subsystems (2)
PATCH v6 1/1 soc: qcom: Add SoC info driver
The SoC info driver provides information such as Chip ID, Chip family, serial number and other such details about Qualcomm SoCs.
---
v5 --> v6:
- use dev_ext_attribute to represent SMEM image items
- Avoid redundant function calls
- Avoid use of unnecessary global variables
- Remove redundant enums
- Avoid redundant checking of socinfo being NULL or not
- Avoid redundant checking of socinfo format version
- Rename show/store function of attributes as _show/_store rather than _get/_set
- Use an array to represent socinfo attributes and create attribute files in a loop depending on the minimum socinfo format version for which these attributes are supported
- if obtained socinfo format version is greater than the maximum known version return an error rather than falling back to maximum known version
v4 --> v5:
- Removed redundant function socinfo_print
v3 --> v4:
- Corrected makefile so that smem and socinfo are treated as one module
- Moved the code snippet to get socinfo smem item into socinfo.c
- Removed redundant use of socinfo major version as it is always zero
- Removed unused enums
- Removed redundant indirections
- Use image_version object to store information about each entry in the smem image table
- Replaced usage of snprintf with sprintf and scnprintf
- Get the address of image version table at the beginning and setup image version attributes only if image version table is available
- Do the same for platform_subtype
- Make different type of image version objects read only or readable/ writable depending on their types. For example apps image attributes can be modified via sysfs but the same can't be done for modem image
- Show PMIC model in a human readable form rather than a numeric number
- Avoid using table in single sysfs entry
- Removed checkpatch warnings about S_IRUGO
v2 --> v3:
- Add support to toss soc information data into entropy pool
- Since socinfo is rolled into smem driver, compile the relevant portion of socinfo driver with smem driver
v1 --> v2:
- Removed inclusion of system_misc.h
- merged socinfo.h into socinfo.c
- made platform type and subtype arrays static
- replaced uint32_t with u32
- made functions static to avoid exposing vendor specific interface
- Replaced usage of IS_ERR_OR_NULL with IS_ERR.
- Remove raw-id attribute usage as human readable soc-id will suffice here
- Avoid using a separate platform driver for socinfo by rolling it into smem driver itself. The sysfs setup is being done in a separate file (socinfo.c)
- Replaced macro BUILD_ID_LENGTH with SMEM_SOCINFO_BUILD_ID_LENGTH.
- For failure cases where socinfo can not be read use a single dummy socinfo with error values.
- Removed usage of early_machine_is_xxx.
drivers/soc/qcom/Kconfig | 1 + drivers/soc/qcom/Makefile | 3 +- drivers/soc/qcom/smem.c | 5 + drivers/soc/qcom/socinfo.c | 671 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 679 insertions(+), 1 deletion(-) create mode 100644 drivers/soc/qcom/socinfo.c
drivers/soc/qcom/Kconfig +1 -0
@@ -24,6 +24,7 @@ config QCOM_SMEM tristate "Qualcomm Shared Memory Manager (SMEM)" depends on ARCH_QCOM depends on HWSPINLOCK + select SOC_BUS help Say y here to enable support for the Qualcomm Shared Memory Manager. The driver provides an interface to items in a heap shared among all
drivers/soc/qcom/Makefile +2 -1
@@ -2,7 +2,8 @@ obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o obj-$(CONFIG_QCOM_PM) += spm.o obj-$(CONFIG_QCOM_SMD) += smd.o obj-$(CONFIG_QCOM_SMD_RPM) += smd-rpm.o -obj-$(CONFIG_QCOM_SMEM) += smem.o +obj-$(CONFIG_QCOM_SMEM) += qcom_smem.o +qcom_smem-y := smem.o socinfo.o obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o obj-$(CONFIG_QCOM_SMP2P) += smp2p.o obj-$(CONFIG_QCOM_SMSM) += smsm.o
drivers/soc/qcom/smem.c +5 -0
@@ -85,6 +85,9 @@/* Max number of processors/hosts in a system */ #define SMEM_HOST_COUNT 9 + +extern void qcom_socinfo_init (struct platform_device *pdev); +/** * struct smem_proc_comm - proc_comm communication struct (legacy) * @command: current command to be executed
@@ -751,6 +754,8 @@ static int qcom_smem_probe(struct platform_device *pdev) __smem = smem; +qcom_socinfo_init (pdev); +return 0 ; }
drivers/soc/qcom/socinfo.c new file +671 -0
671 lines not on this page · raw message
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 5 lines · Imran Khan
The SoC info driver provides information such as Chip ID, Chip family, serial number and other such details about Qualcomm SoCs.
Signed-off-by Imran Khan <[email protected]>
Looks good, just some minor style things.
[..]
Quoted 1 line
drivers/soc/qcom/socinfo.c +0
-0
[..]
drivers/soc/qcom/socinfo.c
Quoted 2 lines
drivers/soc/qcom/socinfo.c +2
-0+ +#define PMIC_MODEL_UNKNOWN 0
Unused define
drivers/soc/qcom/socinfo.c line 30
Quoted 2 lines
drivers/soc/qcom/socinfo.c +2
-0+#define HW_PLATFORM_QRD 11 +#define PLATFORM_SUBTYPE_QRD_INVALID 6
Better replace this with ARRAY_SIZE(qrd_hw_platform_subtype) in qcom_show_platform_subtype().
Quoted 1 line
+#define
PLATFORM_SUBTYPE_INVALID4
Dito
Quoted 8 lines
drivers/soc/qcom/socinfo.c +8
-0+/* +* SOC version type with major number in the upper 16 bits and minor +* number in the lower 16 bits. For example: +* 1.0 -> 0x00010000 +* 2.3 -> 0x00020003 +*/ +#define SOC_VERSION_MAJOR (ver) (((ver) & 0xffff0000) >> 16) +#define SOC_VERSION_MINOR (ver) ((ver) & 0x0000ffff)
drivers/soc/qcom/socinfo.c line 49
Quoted 10 lines
drivers/soc/qcom/socinfo.c +10
-0+#define SOCINFO_VERSION_MAJOR SOC_VERSION_MAJOR +#define SOCINFO_VERSION_MINOR SOC_VERSION_MINOR + +#define SMEM_SOCINFO_BUILD_ID_LENGTH 32 +#define SMEM_IMAGE_VERSION_BLOCKS_COUNT 32 +#define SMEM_IMAGE_VERSION_SIZE 4096 +#define SMEM_IMAGE_VERSION_NAME_SIZE 75 +#define SMEM_IMAGE_VERSION_VARIANT_SIZE 20 +#define SMEM_IMAGE_VERSION_OEM_SIZE 32 +#define SMEM_IMAGE_VERSION_PARTITION_APPS 10
SMEM_IMAGE_VERSION_PARTITION_APPS is unused.
drivers/soc/qcom/socinfo.c line 58
Quoted 9 lines
drivers/soc/qcom/socinfo.c +9
-0+ +/* +* SMEM item ids, used to acquire handles to respective +* SMEM region. +*/ +#define SMEM_IMAGE_VERSION_TABLE 469 +#define SMEM_HW_SW_BUILD_ID 137 + +#define MAX_ATTR_COUNT 15
Replace this with ARRAY_SIZE(qcom_socinfo_attrs).
Quoted 36 lines
drivers/soc/qcom/socinfo.c +36
-0+ +/* +* SMEM Image table indices +*/ +enum smem_image_table_index { +SMEM_IMAGE_TABLE_BOOT_INDEX =0 , +SMEM_IMAGE_TABLE_TZ_INDEX , +SMEM_IMAGE_TABLE_RPM_INDEX =3 , +SMEM_IMAGE_TABLE_APPS_INDEX =10 , +SMEM_IMAGE_TABLE_MPSS_INDEX , +SMEM_IMAGE_TABLE_ADSP_INDEX , +SMEM_IMAGE_TABLE_CNSS_INDEX , +SMEM_IMAGE_TABLE_VIDEO_INDEX +}; + +struct qcom_socinfo_attr { +struct device_attribute attr; +int min_version; +}; + +#define QCOM_SOCINFO_ATTR (_name, _show, _min_version) \ + { __ATTR(_name, 0444, _show, NULL), _min_version } + +#define SMEM_IMG_ATTR_ENTRY (_name, _mode, _show, _store, _index) \ + struct dev_ext_attribute dev_attr_##_name = \ + { __ATTR(_name, _mode, _show, _store), (void *)_index } + +#define QCOM_SMEM_IMG_ITEM (_name, _mode, _index) \ + SMEM_IMG_ATTR_ENTRY(_name##_image_version, _mode, \ + qcom_show_image_version, qcom_store_image_version, \ + (unsigned long)_index); \ + SMEM_IMG_ATTR_ENTRY(_name##_image_variant, _mode, \ + qcom_show_image_variant, qcom_store_image_variant, \ + (unsigned long)_index); \ + SMEM_IMG_ATTR_ENTRY(_name##_image_crm, _mode, \ + qcom_show_image_crm, qcom_store_image_crm, \
drivers/soc/qcom/socinfo.c line 104
Quoted 10 lines
drivers/soc/qcom/socinfo.c +10
-0+ (unsigned long)_index); \ +static struct attribute *_name##_image_attrs[] = { \ + &dev_attr_##_name##_image_version.attr.attr, \ + &dev_attr_##_name##_image_variant.attr.attr, \ + &dev_attr_##_name##_image_crm.attr.attr, \ +NULL , \ +}; \ +static struct attribute_group _name##_image_attr_group = { \ + .attrs = _name##_image_attrs, \ +}
Rather than reusing dev_ext_attribute directly, you can roll your own struct. The following would save you a bunch of type casts:
struct smem_image_attribute {
struct device_attribute version;
struct device_attribute variant;
struct device_attribute crm;
int index;
};#define QCOM_SMEM_IMAGE_ITEM(_name, _mode, _index) \
static struct smem_image_attribute _name##_image_attrs = { \
__ATTR(_name##_image_version, _mode, \
qcom_show_image_version, qcom_store_image_version), \
__ATTR(_name##_image_variant, _mode, \
qcom_show_image_variant, qcom_store_image_variant), \
__ATTR(_name##_image_crm, _mode, \
qcom_show_image_crm, qcom_store_image_crm), \
_index \
}; \
static struct attribute_group _name##_image_attr_group = { \
.attrs = (struct attribute*[]) { \
&_name##_image_attrs.version.attr, \
&_name##_image_attrs.variant.attr, \
&_name##_image_attrs.crm.attr, \
NULL \
} \
}Making the show functions something like:
qcom_show_image_version()
{
struct smem_image_attribute *smem_attr;
smem_attr = container_of(attr, struct smem_image_attribute, version);
return scnprintf(buf, PAGE_SIZE, "%s\n",
smem_image_version[smem_attr->index].name);
}[..]
drivers/soc/qcom/socinfo.c line 249
Quoted 3 lines
drivers/soc/qcom/socinfo.c +3
-0+ +static struct smem_image_version *smem_image_version; +static u32 socinfo_format;
I still think you should fold socinfo_populate_sysfs_files() into qcom_socinfo_init(), and then this will turn into a local variable instead.
[..]
drivers/soc/qcom/socinfo.c line 381
Quoted 8 lines
drivers/soc/qcom/socinfo.c +8
-0+ +static ssize_t +qcom_show_platform_subtype (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +u32 hw_subtype = socinfo->v0_6.hw_platform_subtype; +
To simplify the inner conditionals you can move the common check here:
if (hw_subtype < 0)
return -EINVAL;
drivers/soc/qcom/socinfo.c line 382
Quoted 1 line
drivers/soc/qcom/socinfo.c +1
-0+if (socinfo->v0_3.hw_platform ==HW_PLATFORM_QRD )
Use {} around if statements that are more than a single line.
drivers/soc/qcom/socinfo.c line 384
Quoted 2 lines
drivers/soc/qcom/socinfo.c +2
-0+if (hw_subtype >=0 && + hw_subtype <PLATFORM_SUBTYPE_QRD_INVALID )
Just to follow common style, handle the error case first, like:
if (hw_subtype >= ARRAY_SIZE(qrd_hw_platform_subtype)) return -EINVAL; return sprintf(buf, "%s\n", qrd_hw_platform_subtype[hw_subtype]);
And if you shorten "hw_subtype" to just "subtype" you can get that in under 80 characters without loosing any clarity.
Quoted 13 lines
drivers/soc/qcom/socinfo.c +13
-0+return sprintf (buf,"%s\n" , + qrd_hw_platform_subtype[hw_subtype]); + else + return -EINVAL; + else + if (hw_subtype >= 0 && + hw_subtype < PLATFORM_SUBTYPE_INVALID) +return sprintf (buf,"%s\n" , + hw_platform_subtype[hw_subtype]); + else + return -EINVAL; +} +
[..]
Quoted 4 lines
drivers/soc/qcom/socinfo.c +4
-0+ +static void socinfo_populate_sysfs_files (struct device *dev) +{ +int idx;
drivers/soc/qcom/socinfo.c line 595
Quoted 10 lines
drivers/soc/qcom/socinfo.c +10
-0+int err; + +/* +* Expose SMEM_IMAGE_TABLE to sysfs only when we have IMAGE_TABLE +* available in SMEM. As IMAGE_TABLE and SOCINFO are two separate +* items within SMEM, we expose the remaining soc information(i.e +* only the SOCINFO item available in SMEM) to sysfs even in the +* absence of an IMAGE_TABLE. +*/ +if (smem_image_version)
Use {} around blocks larger than a single line.
drivers/soc/qcom/socinfo.c line 599
Quoted 4 lines
drivers/soc/qcom/socinfo.c +4
-0+for (idx =0 ; idx <SMEM_IMAGE_VERSION_BLOCKS_COUNT ; idx++) +if (smem_image_table[idx]) + err =sysfs_create_group (&dev->kobj, + smem_image_table[idx]);
Broken lines should be indented to line up with the parenthesis on the line above.
And as you don't care about the error here, just drop the "err" variable.
drivers/soc/qcom/socinfo.c line 601
Quoted 2 lines
drivers/soc/qcom/socinfo.c +2
-0+ +for (idx =0 ; idx <MAX_ATTR_COUNT ; idx++)
{}
Quoted 5 lines
drivers/soc/qcom/socinfo.c +5
-0+if (qcom_socinfo_attrs[idx].min_version <= socinfo_format) +device_create_file (dev, &qcom_socinfo_attrs[idx].attr); +} + +void qcom_socinfo_init (struct platform_device *pdev)
drivers/soc/qcom/socinfo.c line 616
Quoted 10 lines
drivers/soc/qcom/socinfo.c +10
-0+{ +struct soc_device_attribute *attr; +struct device *qcom_soc_device; +struct soc_device *soc_dev; +size_t img_tbl_size; +u32 soc_version; +size_t size; + + socinfo =qcom_smem_get (QCOM_SMEM_HOST_ANY ,SMEM_HW_SW_BUILD_ID , + &size);
As this value needs to be remembered to the end of the function name it less generic, e.g. item_size.
Quoted 14 lines
drivers/soc/qcom/socinfo.c +14
-0+if (IS_ERR (socinfo)) { +dev_err (&pdev->dev,"Coudn't find socinfo.\n" ); +return ; + } + +if ((SOCINFO_VERSION_MAJOR (socinfo->v0_1.format) !=0 ) || + (SOCINFO_VERSION_MINOR (socinfo->v0_1.format) <0 ) || + (socinfo->v0_1.format >MAX_SOCINFO_FORMAT )) { +dev_err (&pdev->dev,"Wrong socinfo format\n" ); +return ; + } + socinfo_format = socinfo->v0_1.format; + +if (!socinfo->v0_1.id)
drivers/soc/qcom/socinfo.c line 640
Quoted 10 lines
drivers/soc/qcom/socinfo.c +10
-0+dev_err (&pdev->dev,"socinfo: Unknown SoC ID!\n" ); + +WARN (socinfo->v0_1.id >=ARRAY_SIZE (cpu_of_id), +"New IDs added! ID => CPU mapping needs an update.\n" ); + + smem_image_version =qcom_smem_get (QCOM_SMEM_HOST_ANY , +SMEM_IMAGE_VERSION_TABLE , + &img_tbl_size); +if (IS_ERR (smem_image_version) || + (img_tbl_size !=SMEM_IMAGE_VERSION_SIZE )) {
This size is on the other hand very short lived, so the name doesn't matter as much - name it "size" and you don't have to line wrap the conditional.
drivers/soc/qcom/socinfo.c line 647
Quoted 7 lines
drivers/soc/qcom/socinfo.c +7
-0+dev_warn (&pdev->dev,"Image version table absent.\n" ); + smem_image_version =NULL ; + } + + attr =kzalloc (sizeof (*attr),GFP_KERNEL ); +if (!attr) { +dev_err (&pdev->dev,"Unable to allocate attributes.\n" );
kzalloc() will print an error message if it fails, so need for you to print another one.
Quoted 14 lines
drivers/soc/qcom/socinfo.c +14
-0+return ; + } + soc_version = socinfo->v0_1.version; + + attr->soc_id =kasprintf (GFP_KERNEL ,"%d" , socinfo->v0_1.id); + attr->family ="Snapdragon" ; + attr->machine = cpu_of_id[socinfo->v0_1.id]; + attr->revision =kasprintf (GFP_KERNEL ,"%u.%u" , +SOC_VERSION_MAJOR (soc_version), +SOC_VERSION_MINOR (soc_version)); + + soc_dev =soc_device_register (attr); +if (IS_ERR (soc_dev)) { +kfree (attr);
drivers/soc/qcom/socinfo.c line 671
Quoted 10 lines
drivers/soc/qcom/socinfo.c +10
-0+return ; + } + + qcom_soc_device =soc_device_to_device (soc_dev); +socinfo_populate_sysfs_files (qcom_soc_device); + +/* Feed the soc specific unique data into entropy pool */ +add_device_randomness (socinfo, size); +} +EXPORT_SYMBOL (qcom_socinfo_init);
Regards,
Bjorn
Quoted 3 lines · Imran Khan
The SoC info driver provides information such as Chip ID, Chip family, serial number and other such details about Qualcomm SoCs.
Yes but why do we care?
Quoted 8 lines
Signed-off-by Imran Khan <[email protected]>drivers/soc/qcom/Kconfig | 1 + drivers/soc/qcom/Makefile | 3 +- drivers/soc/qcom/smem.c | 5 + drivers/soc/qcom/socinfo.c | 671 +++++++++++++++++++++++++++++++++++++++++++++
There should be a Documentation/ABI/ file here as well to document all the sysfs files added in this patch.
drivers/soc/qcom/smem.c line 89
Quoted 10 lines
drivers/soc/qcom/smem.c +2
-0@@ -85,6 +85,9 @@/* Max number of processors/hosts in a system */ #define SMEM_HOST_COUNT 9 + +extern void qcom_socinfo_init (struct platform_device *pdev);
We can't put this into a header file?
Quoted 12 lines
drivers/soc/qcom/socinfo.c new file +6
-0@@ -0,0 +1,671 @@ +/* +* Copyright (c) 2009-2016, The Linux Foundation. All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 and +* only version 2 as published by the Free Software Foundation.
drivers/soc/qcom/socinfo.c line 16
Quoted 10 lines
drivers/soc/qcom/socinfo.c new file +10
-0+* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +*/ + +#include <linux/export.h> +#include <linux/module.h>
This include isn't necessary.
drivers/soc/qcom/socinfo.c line 22
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+#include <linux/err.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/sys_soc.h> +#include <linux/slab.h> +#include <linux/stat.h>
Is this include used?
drivers/soc/qcom/socinfo.c line 28
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+#include <linux/string.h> +#include <linux/types.h> +#include <linux/random.h> +#include <linux/soc/qcom/smem.h> + +#define PMIC_MODEL_UNKNOWN 0
Used?
Quoted 11 lines
drivers/soc/qcom/socinfo.c new file +11
-0+#define HW_PLATFORM_QRD 11 +#define PLATFORM_SUBTYPE_QRD_INVALID 6 +#define PLATFORM_SUBTYPE_INVALID 4 +/* +* SOC version type with major number in the upper 16 bits and minor +* number in the lower 16 bits. For example: +* 1.0 -> 0x00010000 +* 2.3 -> 0x00020003 +*/ +#define SOC_VERSION_MAJOR (ver) (((ver) & 0xffff0000) >> 16) +#define SOC_VERSION_MINOR (ver) ((ver) & 0x0000ffff)
drivers/soc/qcom/socinfo.c line 49
Quoted 10 lines
drivers/soc/qcom/socinfo.c new file +10
-0+#define SOCINFO_VERSION_MAJOR SOC_VERSION_MAJOR +#define SOCINFO_VERSION_MINOR SOC_VERSION_MINOR + +#define SMEM_SOCINFO_BUILD_ID_LENGTH 32 +#define SMEM_IMAGE_VERSION_BLOCKS_COUNT 32 +#define SMEM_IMAGE_VERSION_SIZE 4096 +#define SMEM_IMAGE_VERSION_NAME_SIZE 75 +#define SMEM_IMAGE_VERSION_VARIANT_SIZE 20 +#define SMEM_IMAGE_VERSION_OEM_SIZE 32 +#define SMEM_IMAGE_VERSION_PARTITION_APPS 10
Used?
Quoted 12 lines
drivers/soc/qcom/socinfo.c new file +12
-0+ +/* +* SMEM item ids, used to acquire handles to respective +* SMEM region. +*/ +#define SMEM_IMAGE_VERSION_TABLE 469 +#define SMEM_HW_SW_BUILD_ID 137 + +#define MAX_ATTR_COUNT 15 + +/* +* SMEM Image table indices
drivers/soc/qcom/socinfo.c line 71
Quoted 10 lines
drivers/soc/qcom/socinfo.c new file +10
-0+*/ +enum smem_image_table_index { +SMEM_IMAGE_TABLE_BOOT_INDEX =0 , +SMEM_IMAGE_TABLE_TZ_INDEX , +SMEM_IMAGE_TABLE_RPM_INDEX =3 , +SMEM_IMAGE_TABLE_APPS_INDEX =10 , +SMEM_IMAGE_TABLE_MPSS_INDEX , +SMEM_IMAGE_TABLE_ADSP_INDEX , +SMEM_IMAGE_TABLE_CNSS_INDEX , +SMEM_IMAGE_TABLE_VIDEO_INDEX
Please just use #defines. The enum isn't buying us anything besides confusion about what the numbers are.
drivers/soc/qcom/socinfo.c line 83
Quoted 12 lines
drivers/soc/qcom/socinfo.c new file +12
-0+}; + +struct qcom_socinfo_attr { +struct device_attribute attr; +int min_version; +}; + +#define QCOM_SOCINFO_ATTR (_name, _show, _min_version) \ + { __ATTR(_name, 0444, _show, NULL), _min_version } + +#define SMEM_IMG_ATTR_ENTRY (_name, _mode, _show, _store, _index) \ + struct dev_ext_attribute dev_attr_##_name = \
add static. and const?
Quoted 31 lines
drivers/soc/qcom/socinfo.c new file +31
-0+ { __ATTR(_name, _mode, _show, _store), (void *)_index } + +#define QCOM_SMEM_IMG_ITEM (_name, _mode, _index) \ + SMEM_IMG_ATTR_ENTRY(_name##_image_version, _mode, \ + qcom_show_image_version, qcom_store_image_version, \ + (unsigned long)_index); \ + SMEM_IMG_ATTR_ENTRY(_name##_image_variant, _mode, \ + qcom_show_image_variant, qcom_store_image_variant, \ + (unsigned long)_index); \ + SMEM_IMG_ATTR_ENTRY(_name##_image_crm, _mode, \ + qcom_show_image_crm, qcom_store_image_crm, \ + (unsigned long)_index); \ +static struct attribute *_name##_image_attrs[] = { \ + &dev_attr_##_name##_image_version.attr.attr, \ + &dev_attr_##_name##_image_variant.attr.attr, \ + &dev_attr_##_name##_image_crm.attr.attr, \ + NULL, \ +}; \ +static struct attribute_group _name##_image_attr_group = { \ + .attrs = _name##_image_attrs, \ +} + +/* Hardware platform types */ +static const char *const hw_platform[] = { + [0 ] ="Unknown" , + [1 ] ="Surf" , + [2 ] ="FFA" , + [3 ] ="Fluid" , + [4 ] ="SVLTE_FFA" , + [5 ] ="SLVTE_SURF" , + [7 ] ="MDM_MTP_NO_DISPLAY" ,
drivers/soc/qcom/socinfo.c line 124
Quoted 10 lines
drivers/soc/qcom/socinfo.c new file +10
-0+ [8] = "MTP", + [9] = "Liquid", + [10] = "Dragon", + [11] = "QRD", + [13] = "HRD", + [14] = "DTV", + [21] = "RCM", + [23] = "STP", + [24] = "SBC", +};
These only make sense on qcom platforms. Other boards can reuse the numbers here and have their own names for the platform field, so it doesn't seem like a good idea to do this in the kernel.
drivers/soc/qcom/socinfo.c line 137
Quoted 13 lines
drivers/soc/qcom/socinfo.c new file +13
-0+ +static const char *const qrd_hw_platform_subtype[] = { + [0 ] ="QRD" , + [1 ] ="SKUAA" , + [2 ] ="SKUF" , + [3 ] ="SKUAB" , + [5 ] ="SKUG" , + [6 ] ="INVALID" , +}; + +static const char *const hw_platform_subtype[] = { +"Unknown" ,"charm" ,"strange" ,"strange_2a" ,"Invalid" , +};
Same point here. Seems impossible to maintain this so please get rid of it and just output raw numbers if anything.
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+ +static const char *const pmic_model[] = { + [0 ] ="Unknown PMIC model" , + [13 ] ="PMIC model: PM8058" , + [14 ] ="PMIC model: PM8028" , + [15 ] ="PMIC model: PM8901" ,
drivers/soc/qcom/socinfo.c line 153
Quoted 10 lines
drivers/soc/qcom/socinfo.c new file +10
-0+ [16] = "PMIC model: PM8027", + [17] = "PMIC model: ISL9519", + [18] = "PMIC model: PM8921", + [19] = "PMIC model: PM8018", + [20] = "PMIC model: PM8015", + [21] = "PMIC model: PM8014", + [22] = "PMIC model: PM8821", + [23] = "PMIC model: PM8038", + [24] = "PMIC model: PM8922", + [25] = "PMIC model: PM8917",
Why do we have "PMIC model:" in sysfs? Shouldn't that be evident from the file name and shouldn't we strive for something more machine readable? And do we expose all the different models in sysfs or just the first one?
Quoted 67 lines
drivers/soc/qcom/socinfo.c new file +67
-0+}; + +struct smem_image_version { +char name[SMEM_IMAGE_VERSION_NAME_SIZE ]; +char variant[SMEM_IMAGE_VERSION_VARIANT_SIZE ]; +char pad; +char oem[SMEM_IMAGE_VERSION_OEM_SIZE ]; +}; + +/* Used to parse shared memory. Must match the modem. */ +struct socinfo_v0_1 { +u32 format; +u32 id; +u32 version; +char build_id[SMEM_SOCINFO_BUILD_ID_LENGTH ]; +}; + +struct socinfo_v0_2 { +struct socinfo_v0_1 v0_1; +u32 raw_version; +}; + +struct socinfo_v0_3 { +struct socinfo_v0_2 v0_2; +u32 hw_platform; +}; + +struct socinfo_v0_4 { +struct socinfo_v0_3 v0_3; +u32 platform_version; +}; + +struct socinfo_v0_5 { +struct socinfo_v0_4 v0_4; +u32 accessory_chip; +}; + +struct socinfo_v0_6 { +struct socinfo_v0_5 v0_5; +u32 hw_platform_subtype; +}; + +struct socinfo_v0_7 { +struct socinfo_v0_6 v0_6; +u32 pmic_model; +u32 pmic_die_revision; +}; + +struct socinfo_v0_8 { +struct socinfo_v0_7 v0_7; +u32 pmic_model_1; +u32 pmic_die_revision_1; +u32 pmic_model_2; +u32 pmic_die_revision_2; +}; + +struct socinfo_v0_9 { +struct socinfo_v0_8 v0_8; +u32 foundry_id; +}; + +struct socinfo_v0_10 { +struct socinfo_v0_9 v0_9; +u32 serial_number; +}; + +struct socinfo_v0_11 {
drivers/soc/qcom/socinfo.c line 230
Quoted 10 lines
drivers/soc/qcom/socinfo.c new file +10
-0+struct socinfo_v0_10 v0_10; +u32 num_pmics; +u32 pmic_array_offset; +}; + +struct socinfo_v0_12 { +struct socinfo_v0_11 v0_11; +u32 chip_family; +u32 raw_device_family; +u32 raw_device_number;
__le32 for all these u32 fields? And the appropriate endian accessors for them all as well?
Quoted 9 lines
drivers/soc/qcom/socinfo.c new file +9
-0+}; + +static union { + struct socinfo_v0_1 v0_1; + struct socinfo_v0_2 v0_2; + struct socinfo_v0_3 v0_3; + struct socinfo_v0_4 v0_4; + struct socinfo_v0_5 v0_5; + struct socinfo_v0_6 v0_6;
drivers/soc/qcom/socinfo.c line 249
Quoted 10 lines
drivers/soc/qcom/socinfo.c new file +10
-0+struct socinfo_v0_7 v0_7; +struct socinfo_v0_8 v0_8; +struct socinfo_v0_9 v0_9; +struct socinfo_v0_10 v0_10; +struct socinfo_v0_11 v0_11; +struct socinfo_v0_12 v0_12; +} *socinfo; + +static struct smem_image_version *smem_image_version; +static u32 socinfo_format;
Why are any of these things static and in the global namespace?
drivers/soc/qcom/socinfo.c line 256
Quoted 7 lines
drivers/soc/qcom/socinfo.c new file +7
-0+ +/* max socinfo format version supported */ +#define MAX_SOCINFO_FORMAT 12 + +static const char *const cpu_of_id[] = { + + [0 ] ="Unknown CPU" ,
An SoC is not a CPU? Probably should rename this array soc_of_id or machine_of_id instead.
Quoted 54 lines
drivers/soc/qcom/socinfo.c new file +54
-0+ + /* 8x60 IDs */ + [87] = "MSM8960", + +/* 8x64 IDs */ + [109] = "APQ8064", + [130] = "MPQ8064", + +/* 8x60A IDs */ + [122] = "MSM8660A", + [123] = "MSM8260A", + [124] = "APQ8060A", + +/* 8x74 IDs */ + [126] = "MSM8974", + [184] = "APQ8074", + [185] = "MSM8274", + [186] = "MSM8674", + +/* 8x74AA IDs */ + [208] = "APQ8074-AA", + [211] = "MSM8274-AA", + [214] = "MSM8674-AA", + [217] = "MSM8974-AA", + +/* 8x74AB IDs */ + [209] = "APQ8074-AB", + [212] = "MSM8274-AB", + [215] = "MSM8674-AB", + [218] = "MSM8974-AB", + +/* 8x74AC IDs */ + [194] = "MSM8974PRO", + [210] = "APQ8074PRO", + [213] = "MSM8274PRO", + [216] = "MSM8674PRO", + +/* 8x60AB IDs */ + [138] = "MSM8960AB", + [139] = "APQ8060AB", + [140] = "MSM8260AB", + [141] = "MSM8660AB", + +/* 8x84 IDs */ + [178] = "APQ8084", + +/* 8x16 IDs */ + [206] = "MSM8916", + [247] = "APQ8016", + [248] = "MSM8216", + [249] = "MSM8116", + [250] = "MSM8616", + +/* 8x96 IDs */
drivers/soc/qcom/socinfo.c line 320
Quoted 10 lines
drivers/soc/qcom/socinfo.c new file +10
-0+ [246] = "MSM8996", + [310] = "MSM8996AU", + [311] = "APQ8096AU", + [291] = "APQ8096", + [305] = "MSM8996SG", + [312] = "APQ8096SG", + +/* +* Uninitialized IDs are not known to run Linux. +* MSM_CPU_UNKNOWN is set to 0 to ensure these IDs are
What is this define? Please don't confuse CPUs with SoCs.
Quoted 3 lines
drivers/soc/qcom/socinfo.c new file +3
-0+* considered as unknown CPU. +*/ +};
[...]
Quoted 20 lines
drivers/soc/qcom/socinfo.c new file +20
-0+ +static const struct qcom_socinfo_attr qcom_socinfo_attrs[] = { +QCOM_SOCINFO_ATTR (chip_family, qcom_show_chip_family,12 ), +QCOM_SOCINFO_ATTR (raw_device_family, qcom_show_raw_device_family,12 ), +QCOM_SOCINFO_ATTR (raw_device_number, qcom_show_raw_device_number,12 ), +QCOM_SOCINFO_ATTR (serial_number, qcom_show_serial_number,10 ), +QCOM_SOCINFO_ATTR (foundry_id, qcom_show_foundry_id,9 ), +QCOM_SOCINFO_ATTR (pmic_model, qcom_show_pmic_model,7 ), +QCOM_SOCINFO_ATTR (pmic_die_revision, qcom_show_pmic_die_revision,7 ), +QCOM_SOCINFO_ATTR (platform_subtype, qcom_show_platform_subtype,6 ), +QCOM_SOCINFO_ATTR (platform_subtype_id, + qcom_show_platform_subtype_id,6 ), +QCOM_SOCINFO_ATTR (accessory_chip, qcom_show_accessory_chip,5 ), +QCOM_SOCINFO_ATTR (platform_version, qcom_show_platform_version,4 ), +QCOM_SOCINFO_ATTR (hw_platform, qcom_show_hw_platform,3 ), +QCOM_SOCINFO_ATTR (raw_version, qcom_show_raw_version,2 ), +QCOM_SOCINFO_ATTR (build_id, qcom_show_build_id,1 ), +QCOM_SOCINFO_ATTR (vendor, qcom_show_vendor,0 ), +}; +
drivers/soc/qcom/socinfo.c line 571
Quoted 10 lines
drivers/soc/qcom/socinfo.c new file +10
-0+QCOM_SMEM_IMG_ITEM (boot,0444 ,SMEM_IMAGE_TABLE_BOOT_INDEX ); +QCOM_SMEM_IMG_ITEM (tz,0444 ,SMEM_IMAGE_TABLE_TZ_INDEX ); +QCOM_SMEM_IMG_ITEM (rpm,0444 ,SMEM_IMAGE_TABLE_RPM_INDEX ); +QCOM_SMEM_IMG_ITEM (apps,0644 ,SMEM_IMAGE_TABLE_APPS_INDEX ); +QCOM_SMEM_IMG_ITEM (mpss,0444 ,SMEM_IMAGE_TABLE_MPSS_INDEX ); +QCOM_SMEM_IMG_ITEM (adsp,0444 ,SMEM_IMAGE_TABLE_ADSP_INDEX ); +QCOM_SMEM_IMG_ITEM (cnss,0444 ,SMEM_IMAGE_TABLE_CNSS_INDEX ); +QCOM_SMEM_IMG_ITEM (video,0444 ,SMEM_IMAGE_TABLE_VIDEO_INDEX ); + +static struct attribute_group
const?
Quoted 20 lines
drivers/soc/qcom/socinfo.c new file +20
-0+ *smem_image_table[SMEM_IMAGE_VERSION_BLOCKS_COUNT] = { + [SMEM_IMAGE_TABLE_BOOT_INDEX] = &boot_image_attr_group, + [SMEM_IMAGE_TABLE_TZ_INDEX] = &tz_image_attr_group, + [SMEM_IMAGE_TABLE_RPM_INDEX] = &rpm_image_attr_group, + [SMEM_IMAGE_TABLE_APPS_INDEX] = &apps_image_attr_group, + [SMEM_IMAGE_TABLE_MPSS_INDEX] = &mpss_image_attr_group, + [SMEM_IMAGE_TABLE_ADSP_INDEX] = &adsp_image_attr_group, + [SMEM_IMAGE_TABLE_CNSS_INDEX] = &cnss_image_attr_group, + [SMEM_IMAGE_TABLE_VIDEO_INDEX] = &video_image_attr_group, +}; + +static void socinfo_populate_sysfs_files (struct device *dev) +{ +int idx; +int err; + +/* +* Expose SMEM_IMAGE_TABLE to sysfs only when we have IMAGE_TABLE +* available in SMEM. As IMAGE_TABLE and SOCINFO are two separate +* items within SMEM, we expose the remaining soc information(i.e
drivers/soc/qcom/socinfo.c line 601
Quoted 10 lines
drivers/soc/qcom/socinfo.c new file +10
-0+* only the SOCINFO item available in SMEM) to sysfs even in the +* absence of an IMAGE_TABLE. +*/ +if (smem_image_version) +for (idx =0 ; idx <SMEM_IMAGE_VERSION_BLOCKS_COUNT ; idx++) +if (smem_image_table[idx]) + err =sysfs_create_group (&dev->kobj, + smem_image_table[idx]); + +for (idx =0 ; idx <MAX_ATTR_COUNT ; idx++)
Use ARRAY_SIZE instead?
drivers/soc/qcom/socinfo.c line 606
Quoted 5 lines
drivers/soc/qcom/socinfo.c new file +5
-0+if (qcom_socinfo_attrs[idx].min_version <= socinfo_format) +device_create_file (dev, &qcom_socinfo_attrs[idx].attr); +} + +void qcom_socinfo_init (struct platform_device *pdev)
Maybe this should just take a device argument as we don't use the platform part at all.
Quoted 8 lines
drivers/soc/qcom/socinfo.c new file +8
-0+{ +struct soc_device_attribute *attr; +struct device *qcom_soc_device; +struct soc_device *soc_dev; +size_t img_tbl_size; +u32 soc_version; +size_t size; +
drivers/soc/qcom/socinfo.c line 624
Quoted 10 lines
drivers/soc/qcom/socinfo.c new file +10
-0+ socinfo =qcom_smem_get (QCOM_SMEM_HOST_ANY ,SMEM_HW_SW_BUILD_ID , + &size); +if (IS_ERR (socinfo)) { +dev_err (&pdev->dev,"Coudn't find socinfo.\n" ); +return ; + } + +if ((SOCINFO_VERSION_MAJOR (socinfo->v0_1.format) !=0 ) || + (SOCINFO_VERSION_MINOR (socinfo->v0_1.format) <0 ) || + (socinfo->v0_1.format >MAX_SOCINFO_FORMAT )) {
Please drop all extraneous parenthesis here.
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+dev_err (&pdev->dev,"Wrong socinfo format\n" ); +return ; + } + socinfo_format = socinfo->v0_1.format; + +if (!socinfo->v0_1.id)
drivers/soc/qcom/socinfo.c line 640
Quoted 10 lines
drivers/soc/qcom/socinfo.c new file +10
-0+dev_err (&pdev->dev,"socinfo: Unknown SoC ID!\n" ); + +WARN (socinfo->v0_1.id >=ARRAY_SIZE (cpu_of_id), +"New IDs added! ID => CPU mapping needs an update.\n" ); + + smem_image_version =qcom_smem_get (QCOM_SMEM_HOST_ANY , +SMEM_IMAGE_VERSION_TABLE , + &img_tbl_size); +if (IS_ERR (smem_image_version) || + (img_tbl_size !=SMEM_IMAGE_VERSION_SIZE )) {
More extraneous parenthesis.
drivers/soc/qcom/socinfo.c line 641
Quoted 1 line
drivers/soc/qcom/socinfo.c new file +1
-0+dev_warn (&pdev->dev,"Image version table absent.\n" );
Will this warn on "older" platforms? Perhaps make this a dev_dbg() instead.
drivers/soc/qcom/socinfo.c line 647
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+ smem_image_version =NULL ; + } + + attr =kzalloc (sizeof (*attr),GFP_KERNEL ); +if (!attr) { +dev_err (&pdev->dev,"Unable to allocate attributes.\n" );
Please drop the full stop on all error messages.
drivers/soc/qcom/socinfo.c line 654
Quoted 7 lines
drivers/soc/qcom/socinfo.c new file +7
-0+return ; + } + soc_version = socinfo->v0_1.version; + + attr->soc_id =kasprintf (GFP_KERNEL ,"%d" , socinfo->v0_1.id); + attr->family ="Snapdragon" ; + attr->machine = cpu_of_id[socinfo->v0_1.id];
The id is used twice. According to the ABI documentation soc_id is a serial number. The part number is not the same as a serial number so assigning soc_id doesn't seem correct. Probably we should only assign soc_id to be v10 serial_number.
Quoted 5 lines
drivers/soc/qcom/socinfo.c new file +5
-0+ attr->revision =kasprintf (GFP_KERNEL ,"%u.%u" , +SOC_VERSION_MAJOR (soc_version), +SOC_VERSION_MINOR (soc_version)); + + soc_dev =soc_device_register (attr);
drivers/soc/qcom/socinfo.c line 669
Quoted 10 lines
drivers/soc/qcom/socinfo.c new file +10
-0+if (IS_ERR (soc_dev)) { +kfree (attr); +return ; + } + + qcom_soc_device =soc_device_to_device (soc_dev); +socinfo_populate_sysfs_files (qcom_soc_device); + +/* Feed the soc specific unique data into entropy pool */ +add_device_randomness (socinfo, size);
And thus adding mostly the same random bits for every SoC that's the same as a million other ones doesn't seem like a good choice for device randomness. Probably only the v10 serial_number should be added to the random pool.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 7 lines · Stephen Boyd
Quoted 3 lines · Imran Khan
The SoC info driver provides information such as Chip ID, Chip family, serial number and other such details about Qualcomm SoCs.
Yes but why do we care?
We intend to expose some standard SoC attributes (like soc-id) of QCOM SoCs to user space, so that if needed the user space utilities (like antutu) can query such information using sysfs interface. A further example can be a user space script (say Android's init.rc) which reads soc-id and does some gpio settings based on the soc-id.
Quoted 4 lines
Quoted 4 lines
Signed-off-by Imran Khan <[email protected]>
Quoted 8 lines
Quoted 4 lines
drivers/soc/qcom/Kconfig | 1 + drivers/soc/qcom/Makefile | 3 +- drivers/soc/qcom/smem.c | 5 + drivers/soc/qcom/socinfo.c | 671 +++++++++++++++++++++++++++++++++++++++++++++There should be a
Documentation/ABI/file here as well to document all the sysfs files added in this patch.
Sure. I will add this file in the subsequent patch set.
Quoted 8 lines
Quoted 8 lines
drivers/soc/qcom/smem.c +0
-0@@ -85,6 +85,9 @@/* Max number of processors/hosts in a system */ #define SMEM_HOST_COUNT 9
Quoted 5 lines
drivers/soc/qcom/smem.c line 89
Quoted 2 lines
drivers/soc/qcom/smem.c +2
-0+ +extern void qcom_socinfo_init (struct platform_device *pdev);We can't put this into a header file?
We can. In fact earlier I had put it in smem.h but since smem.h is public API for smem I removed it from there. As it was only a single function I used this extern declaration. Please let me know if it is acceptable. If it is not acceptable I will create a socinfo.h and will put this declaration there.
Quoted 16 lines
Quoted 16 lines
drivers/soc/qcom/socinfo.c new file +10
-0@@ -0,0 +1,671 @@ +/* +* Copyright (c) 2009-2016, The Linux Foundation. All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 and +* only version 2 as published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
Quoted 9 lines
drivers/soc/qcom/socinfo.c line 16
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+* GNU General Public License for more details. +* +*/ + +#include <linux/export.h> +#include <linux/module.h> This include isn't necessary.
True. Will remove this in the next patch set.
Quoted 9 lines
drivers/soc/qcom/socinfo.c line 22
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+#include <linux/err.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/sys_soc.h> +#include <linux/slab.h> +#include <linux/stat.h> Is this include used?
Will check and remove accordingly.
Quoted 9 lines
drivers/soc/qcom/socinfo.c line 28
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+#include <linux/string.h> +#include <linux/types.h> +#include <linux/random.h> +#include <linux/soc/qcom/smem.h> + +#define PMIC_MODEL_UNKNOWN 0Used?
Not used. Will remove this.
Quoted 15 lines
Quoted 15 lines
drivers/soc/qcom/socinfo.c new file +15
-0+#define HW_PLATFORM_QRD 11 +#define PLATFORM_SUBTYPE_QRD_INVALID 6 +#define PLATFORM_SUBTYPE_INVALID 4 +/* +* SOC version type with major number in the upper 16 bits and minor +* number in the lower 16 bits. For example: +* 1.0 -> 0x00010000 +* 2.3 -> 0x00020003 +*/ +#define SOC_VERSION_MAJOR (ver) (((ver) & 0xffff0000) >> 16) +#define SOC_VERSION_MINOR (ver) ((ver) & 0x0000ffff) +#define SOCINFO_VERSION_MAJOR SOC_VERSION_MAJOR +#define SOCINFO_VERSION_MINOR SOC_VERSION_MINOR + +#define SMEM_SOCINFO_BUILD_ID_LENGTH 32
Quoted 9 lines
drivers/soc/qcom/socinfo.c line 49
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+#define SMEM_IMAGE_VERSION_BLOCKS_COUNT 32 +#define SMEM_IMAGE_VERSION_SIZE 4096 +#define SMEM_IMAGE_VERSION_NAME_SIZE 75 +#define SMEM_IMAGE_VERSION_VARIANT_SIZE 20 +#define SMEM_IMAGE_VERSION_OEM_SIZE 32 +#define SMEM_IMAGE_VERSION_PARTITION_APPS 10Used?
Again redundant. Will remove this.
Quoted 17 lines
Quoted 17 lines
drivers/soc/qcom/socinfo.c new file +17
-0+ +/* +* SMEM item ids, used to acquire handles to respective +* SMEM region. +*/ +#define SMEM_IMAGE_VERSION_TABLE 469 +#define SMEM_HW_SW_BUILD_ID 137 + +#define MAX_ATTR_COUNT 15 + +/* +* SMEM Image table indices +*/ +enum smem_image_table_index { +SMEM_IMAGE_TABLE_BOOT_INDEX =0 , +SMEM_IMAGE_TABLE_TZ_INDEX , +SMEM_IMAGE_TABLE_RPM_INDEX =3 ,
Quoted 9 lines
drivers/soc/qcom/socinfo.c line 71
Quoted 5 lines
drivers/soc/qcom/socinfo.c new file +5
-0+SMEM_IMAGE_TABLE_APPS_INDEX =10 , +SMEM_IMAGE_TABLE_MPSS_INDEX , +SMEM_IMAGE_TABLE_ADSP_INDEX , +SMEM_IMAGE_TABLE_CNSS_INDEX , +SMEM_IMAGE_TABLE_VIDEO_INDEX Please just use #defines. The enum isn't buying us anything besides confusion about what the numbers are.
Okay. Will replace the enum with #define as it will be easier to understand.
Quoted 6 lines
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+}; + +struct qcom_socinfo_attr { +struct device_attribute attr; +int min_version; +};
Quoted 9 lines
drivers/soc/qcom/socinfo.c line 83
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+ +#define QCOM_SOCINFO_ATTR (_name, _show, _min_version) \ + { __ATTR(_name, 0444, _show, NULL), _min_version } + +#define SMEM_IMG_ATTR_ENTRY (_name, _mode, _show, _store, _index) \ + struct dev_ext_attribute dev_attr_##_name = \add static. and const?
Yes. will do this.
Quoted 37 lines
Quoted 37 lines
drivers/soc/qcom/socinfo.c new file +37
-0+ { __ATTR(_name, _mode, _show, _store), (void *)_index } + +#define QCOM_SMEM_IMG_ITEM (_name, _mode, _index) \ + SMEM_IMG_ATTR_ENTRY(_name##_image_version, _mode, \ + qcom_show_image_version, qcom_store_image_version, \ + (unsigned long)_index); \ + SMEM_IMG_ATTR_ENTRY(_name##_image_variant, _mode, \ + qcom_show_image_variant, qcom_store_image_variant, \ + (unsigned long)_index); \ + SMEM_IMG_ATTR_ENTRY(_name##_image_crm, _mode, \ + qcom_show_image_crm, qcom_store_image_crm, \ + (unsigned long)_index); \ +static struct attribute *_name##_image_attrs[] = { \ + &dev_attr_##_name##_image_version.attr.attr, \ + &dev_attr_##_name##_image_variant.attr.attr, \ + &dev_attr_##_name##_image_crm.attr.attr, \ + NULL, \ +}; \ +static struct attribute_group _name##_image_attr_group = { \ + .attrs = _name##_image_attrs, \ +} + +/* Hardware platform types */ +static const char *const hw_platform[] = { + [0 ] ="Unknown" , + [1 ] ="Surf" , + [2 ] ="FFA" , + [3 ] ="Fluid" , + [4 ] ="SVLTE_FFA" , + [5 ] ="SLVTE_SURF" , + [7 ] ="MDM_MTP_NO_DISPLAY" , + [8 ] ="MTP" , + [9 ] ="Liquid" , + [10 ] ="Dragon" , + [11 ] ="QRD" , + [13 ] ="HRD" , + [14 ] ="DTV" ,
Quoted 9 lines
drivers/soc/qcom/socinfo.c line 124
Quoted 4 lines
drivers/soc/qcom/socinfo.c new file +4
-0+ [21] = "RCM", + [23] = "STP", + [24] = "SBC", +};These only make sense on qcom platforms. Other boards can reuse the numbers here and have their own names for the platform field, so it doesn't seem like a good idea to do this in the kernel.
Sorry could not understand this. Do you mean that we should only expose the information that we can via generic soc_device_attribute. This object is being used for hw_platform attribute which we are explicitly creating in sysfs, so it should not conflict with other's implementation.
Or do you mean to just show the numbers and avoid the strings. I am using strings as I think in any case they will be more informative and also there are not many types/subtypes in any case. May be we can keep only those types/subtypes that are more frequent.
I may be completely wrong in understanding the comment here so could you kindly elaborate this a bit.
Quoted 30 lines
drivers/soc/qcom/socinfo.c line 137
Quoted 13 lines
drivers/soc/qcom/socinfo.c new file +13
-0+ +static const char *const qrd_hw_platform_subtype[] = { + [0 ] ="QRD" , + [1 ] ="SKUAA" , + [2 ] ="SKUF" , + [3 ] ="SKUAB" , + [5 ] ="SKUG" , + [6 ] ="INVALID" , +}; + +static const char *const hw_platform_subtype[] = { +"Unknown" ,"charm" ,"strange" ,"strange_2a" ,"Invalid" , +};Same point here. Seems impossible to maintain this so please get rid of it and just output raw numbers if anything.
Quoted 13 lines
drivers/soc/qcom/socinfo.c new file +13
-0+ +static const char *const pmic_model[] = { + [0 ] ="Unknown PMIC model" , + [13 ] ="PMIC model: PM8058" , + [14 ] ="PMIC model: PM8028" , + [15 ] ="PMIC model: PM8901" , + [16 ] ="PMIC model: PM8027" , + [17 ] ="PMIC model: ISL9519" , + [18 ] ="PMIC model: PM8921" , + [19 ] ="PMIC model: PM8018" , + [20 ] ="PMIC model: PM8015" , + [21 ] ="PMIC model: PM8014" , + [22 ] ="PMIC model: PM8821" ,
Quoted 9 lines
drivers/soc/qcom/socinfo.c line 153
Quoted 3 lines
drivers/soc/qcom/socinfo.c new file +3
-0+ [23] = "PMIC model: PM8038", + [24] = "PMIC model: PM8922", + [25] = "PMIC model: PM8917",Why do we have "PMIC model:" in sysfs? Shouldn't that be evident from the file name and shouldn't we strive for something more machine readable? And do we expose all the different models in sysfs or just the first one?
We are exposing just the first PMIC model and yes "PMIC model:" is redundant here. Will remove this in the next patch set. The SMEM content just gives the numeric PMIC id so here we can either dump that id or a string. As of now I am intending to dump string.
Please let me know if that looks okay.
Quoted 8 lines
Quoted 8 lines
drivers/soc/qcom/socinfo.c new file +8
-0+}; + +struct smem_image_version { +char name[SMEM_IMAGE_VERSION_NAME_SIZE ]; +char variant[SMEM_IMAGE_VERSION_VARIANT_SIZE ]; +char pad; +char oem[SMEM_IMAGE_VERSION_OEM_SIZE ]; +};
<snip>
Quoted 10 lines
drivers/soc/qcom/socinfo.c line 230
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+ +struct socinfo_v0_12 { +struct socinfo_v0_11 v0_11; +u32 chip_family; +u32 raw_device_family; +u32 raw_device_number;
__le32for all these u32 fields? And the appropriate endian accessors for them all as well?
Sure. Will do this.
Quoted 13 lines
Quoted 13 lines
drivers/soc/qcom/socinfo.c new file +13
-0+}; + +static union { + struct socinfo_v0_1 v0_1; + struct socinfo_v0_2 v0_2; + struct socinfo_v0_3 v0_3; + struct socinfo_v0_4 v0_4; + struct socinfo_v0_5 v0_5; + struct socinfo_v0_6 v0_6; + struct socinfo_v0_7 v0_7; + struct socinfo_v0_8 v0_8; + struct socinfo_v0_9 v0_9; + struct socinfo_v0_10 v0_10;
Quoted 9 lines
drivers/soc/qcom/socinfo.c line 249
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+struct socinfo_v0_11 v0_11; +struct socinfo_v0_12 v0_12; +} *socinfo; + +static struct smem_image_version *smem_image_version; +static u32 socinfo_format;Why are any of these things static and in the global namespace?
These are being used across multiple sysfs handlers.
Quoted 11 lines
drivers/soc/qcom/socinfo.c line 256
Quoted 7 lines
drivers/soc/qcom/socinfo.c new file +7
-0+ +/* max socinfo format version supported */ +#define MAX_SOCINFO_FORMAT 12 + +static const char *const cpu_of_id[] = { + + [0 ] ="Unknown CPU" ,An SoC is not a CPU? Probably should rename this array
soc_of_idormachine_of_idinstead.
Sure. soc_of_id would be more appropriate than cpu_of_id. Will make this change.
Quoted 5 lines
Quoted 5 lines
drivers/soc/qcom/socinfo.c new file +5
-0+ + /* 8x60 IDs */ + [87] = "MSM8960", + +/* 8x64 IDs */
<snip>
Quoted 11 lines
drivers/soc/qcom/socinfo.c line 320
Quoted 8 lines
drivers/soc/qcom/socinfo.c new file +7
-0+ /* 8x74AB IDs */ + [209] = "APQ8074-AB", + [212] = "MSM8274-AB", + [215] = "MSM8674-AB", +/* +* Uninitialized IDs are not known to run Linux. +* MSM_CPU_UNKNOWN is set to 0 to ensure these IDs are What is this define? Please don't confuse CPUs with SoCs.
Sure. Will correct the comment and the macro.
Quoted 28 lines
Quoted 3 lines
drivers/soc/qcom/socinfo.c new file +3
-0+* considered as unknown CPU. +*/ +};[...]
Quoted 24 lines
drivers/soc/qcom/socinfo.c new file +24
-0+ +static const struct qcom_socinfo_attr qcom_socinfo_attrs[] = { +QCOM_SOCINFO_ATTR (chip_family, qcom_show_chip_family,12 ), +QCOM_SOCINFO_ATTR (raw_device_family, qcom_show_raw_device_family,12 ), +QCOM_SOCINFO_ATTR (raw_device_number, qcom_show_raw_device_number,12 ), +QCOM_SOCINFO_ATTR (serial_number, qcom_show_serial_number,10 ), +QCOM_SOCINFO_ATTR (foundry_id, qcom_show_foundry_id,9 ), +QCOM_SOCINFO_ATTR (pmic_model, qcom_show_pmic_model,7 ), +QCOM_SOCINFO_ATTR (pmic_die_revision, qcom_show_pmic_die_revision,7 ), +QCOM_SOCINFO_ATTR (platform_subtype, qcom_show_platform_subtype,6 ), +QCOM_SOCINFO_ATTR (platform_subtype_id, + qcom_show_platform_subtype_id,6 ), +QCOM_SOCINFO_ATTR (accessory_chip, qcom_show_accessory_chip,5 ), +QCOM_SOCINFO_ATTR (platform_version, qcom_show_platform_version,4 ), +QCOM_SOCINFO_ATTR (hw_platform, qcom_show_hw_platform,3 ), +QCOM_SOCINFO_ATTR (raw_version, qcom_show_raw_version,2 ), +QCOM_SOCINFO_ATTR (build_id, qcom_show_build_id,1 ), +QCOM_SOCINFO_ATTR (vendor, qcom_show_vendor,0 ), +}; + +QCOM_SMEM_IMG_ITEM (boot,0444 ,SMEM_IMAGE_TABLE_BOOT_INDEX ); +QCOM_SMEM_IMG_ITEM (tz,0444 ,SMEM_IMAGE_TABLE_TZ_INDEX ); +QCOM_SMEM_IMG_ITEM (rpm,0444 ,SMEM_IMAGE_TABLE_RPM_INDEX ); +QCOM_SMEM_IMG_ITEM (apps,0644 ,SMEM_IMAGE_TABLE_APPS_INDEX );
Quoted 9 lines
drivers/soc/qcom/socinfo.c line 571
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+QCOM_SMEM_IMG_ITEM (mpss,0444 ,SMEM_IMAGE_TABLE_MPSS_INDEX ); +QCOM_SMEM_IMG_ITEM (adsp,0444 ,SMEM_IMAGE_TABLE_ADSP_INDEX ); +QCOM_SMEM_IMG_ITEM (cnss,0444 ,SMEM_IMAGE_TABLE_CNSS_INDEX ); +QCOM_SMEM_IMG_ITEM (video,0444 ,SMEM_IMAGE_TABLE_VIDEO_INDEX ); + +static struct attribute_groupconst?
Yes. Will do this.
Quoted 24 lines
Quoted 24 lines
drivers/soc/qcom/socinfo.c new file +24
-0+ *smem_image_table[SMEM_IMAGE_VERSION_BLOCKS_COUNT] = { + [SMEM_IMAGE_TABLE_BOOT_INDEX] = &boot_image_attr_group, + [SMEM_IMAGE_TABLE_TZ_INDEX] = &tz_image_attr_group, + [SMEM_IMAGE_TABLE_RPM_INDEX] = &rpm_image_attr_group, + [SMEM_IMAGE_TABLE_APPS_INDEX] = &apps_image_attr_group, + [SMEM_IMAGE_TABLE_MPSS_INDEX] = &mpss_image_attr_group, + [SMEM_IMAGE_TABLE_ADSP_INDEX] = &adsp_image_attr_group, + [SMEM_IMAGE_TABLE_CNSS_INDEX] = &cnss_image_attr_group, + [SMEM_IMAGE_TABLE_VIDEO_INDEX] = &video_image_attr_group, +}; + +static void socinfo_populate_sysfs_files (struct device *dev) +{ +int idx; +int err; + +/* +* Expose SMEM_IMAGE_TABLE to sysfs only when we have IMAGE_TABLE +* available in SMEM. As IMAGE_TABLE and SOCINFO are two separate +* items within SMEM, we expose the remaining soc information(i.e +* only the SOCINFO item available in SMEM) to sysfs even in the +* absence of an IMAGE_TABLE. +*/ +if (smem_image_version)
Quoted 9 lines
drivers/soc/qcom/socinfo.c line 601
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+for (idx =0 ; idx <SMEM_IMAGE_VERSION_BLOCKS_COUNT ; idx++) +if (smem_image_table[idx]) + err =sysfs_create_group (&dev->kobj, + smem_image_table[idx]); + +for (idx =0 ; idx <MAX_ATTR_COUNT ; idx++)Use
ARRAY_SIZEinstead?
Sure. That would avoid this redundant macro.
Quoted 9 lines
drivers/soc/qcom/socinfo.c line 606
Quoted 5 lines
drivers/soc/qcom/socinfo.c new file +5
-0+if (qcom_socinfo_attrs[idx].min_version <= socinfo_format) +device_create_file (dev, &qcom_socinfo_attrs[idx].attr); +} + +void qcom_socinfo_init (struct platform_device *pdev)Maybe this should just take a device argument as we don't use the platform part at all.
Indeed, device would be better argument here. Will make this change.
Quoted 12 lines
Quoted 12 lines
drivers/soc/qcom/socinfo.c new file +12
-0+{ +struct soc_device_attribute *attr; +struct device *qcom_soc_device; +struct soc_device *soc_dev; +size_t img_tbl_size; +u32 soc_version; +size_t size; + + socinfo =qcom_smem_get (QCOM_SMEM_HOST_ANY ,SMEM_HW_SW_BUILD_ID , + &size); +if (IS_ERR (socinfo)) { +dev_err (&pdev->dev,"Coudn't find socinfo.\n" );
Quoted 9 lines
drivers/soc/qcom/socinfo.c line 624
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+return ; + } + +if ((SOCINFO_VERSION_MAJOR (socinfo->v0_1.format) !=0 ) || + (SOCINFO_VERSION_MINOR (socinfo->v0_1.format) <0 ) || + (socinfo->v0_1.format >MAX_SOCINFO_FORMAT )) {Please drop all extraneous parenthesis here.
Sure I will.
Quoted 19 lines
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+dev_err (&pdev->dev,"Wrong socinfo format\n" ); +return ; + } + socinfo_format = socinfo->v0_1.format; + +if (!socinfo->v0_1.id)drivers/soc/qcom/socinfo.c line 640
Quoted 10 lines
drivers/soc/qcom/socinfo.c new file +10
-0+dev_err (&pdev->dev,"socinfo: Unknown SoC ID!\n" ); + +WARN (socinfo->v0_1.id >=ARRAY_SIZE (cpu_of_id), +"New IDs added! ID => CPU mapping needs an update.\n" ); + + smem_image_version =qcom_smem_get (QCOM_SMEM_HOST_ANY , +SMEM_IMAGE_VERSION_TABLE , + &img_tbl_size); +if (IS_ERR (smem_image_version) || + (img_tbl_size !=SMEM_IMAGE_VERSION_SIZE )) {More extraneous parenthesis.
Quoted 5 lines
drivers/soc/qcom/socinfo.c line 641
Quoted 1 line
drivers/soc/qcom/socinfo.c new file +1
-0+dev_warn (&pdev->dev,"Image version table absent.\n" );Will this warn on "older" platforms? Perhaps make this a
dev_dbg()instead.
Okay. I am fine with dev_dbg too.
Quoted 9 lines
drivers/soc/qcom/socinfo.c line 647
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +6
-0+ smem_image_version =NULL ; + } + + attr =kzalloc (sizeof (*attr),GFP_KERNEL ); +if (!attr) { +dev_err (&pdev->dev,"Unable to allocate attributes.\n" );Please drop the full stop on all error messages.
Okay.
Quoted 4 lines
Quoted 4 lines
drivers/soc/qcom/socinfo.c new file +4
-0+return ; + } + soc_version = socinfo->v0_1.version; +
Quoted 9 lines
drivers/soc/qcom/socinfo.c line 654
Quoted 3 lines
drivers/soc/qcom/socinfo.c new file +3
-0+ attr->soc_id =kasprintf (GFP_KERNEL ,"%d" , socinfo->v0_1.id); + attr->family ="Snapdragon" ; + attr->machine = cpu_of_id[socinfo->v0_1.id];The id is used twice. According to the ABI documentation
soc_idis a serial number. The part number is not the same as a serial number so assigningsoc_iddoesn't seem correct. Probably we should only assignsoc_idto be v10serial_number.
This part has been a point of confusion for me since long. Ealier I had discussed this point in threads pertaining to other patch sets but did not get any confirmation. I see that quite a few similar drivers have avoided the "attr->machine" field altogether:
http://lxr.free-electrons.com/source/arch/arm/mach-tegra/tegra.c
http://lxr.free-electrons.com/source/arch/arm/mach-zynq/common.c
So not sure if we should do the same or have a string in machine and
a numeric id in soc_id.
I am afraid that since v10 is relatively newer version of socinfo format, some older socs may not be able to provide serial_number although they might be having a valid soc-id.
Could you please provide your feedback in this regard?
Quoted 12 lines
Quoted 12 lines
drivers/soc/qcom/socinfo.c new file +12
-0+ attr->revision =kasprintf (GFP_KERNEL ,"%u.%u" , +SOC_VERSION_MAJOR (soc_version), +SOC_VERSION_MINOR (soc_version)); + + soc_dev =soc_device_register (attr); +if (IS_ERR (soc_dev)) { +kfree (attr); +return ; + } + + qcom_soc_device =soc_device_to_device (soc_dev); +socinfo_populate_sysfs_files (qcom_soc_device);
Quoted 9 lines
drivers/soc/qcom/socinfo.c line 669
Quoted 3 lines
drivers/soc/qcom/socinfo.c new file +3
-0+ +/* Feed the soc specific unique data into entropy pool */ +add_device_randomness (socinfo, size);And thus adding mostly the same random bits for every SoC that's the same as a million other ones doesn't seem like a good choice for device randomness. Probably only the v10
serial_numbershould be added to the random pool.
Yes a lot of fields in socinfo object tend to have same value. How about putting soc-id and v10 serial number (if we can conclude that these 2 should be different) along with build-id.
Thanks and Regards,
Imran
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
Quoted 6 lines · Imran Khan
Quoted 5 lines · Stephen Boyd
Quoted 3 lines · Imran Khan
The SoC info driver provides information such as Chip ID, Chip family, serial number and other such details about Qualcomm SoCs.
Quoted 8 lines
Quoted 2 lines
Yes but why do we care?
We intend to expose some standard SoC attributes (like soc-id) of QCOM SoCs to user space, so that if needed the user space utilities (like antutu) can query such information using sysfs interface. A further example can be a user space script (say Android's init.rc) which reads soc-id and does some gpio settings based on the soc-id.
Please include such information into the commit text.
Quoted 15 lines
Quoted 9 lines
Quoted 9 lines
drivers/soc/qcom/smem.c +0
-0@@ -85,6 +85,9 @@/* Max number of processors/hosts in a system */ #define SMEM_HOST_COUNT 9Quoted 5 lines
drivers/soc/qcom/smem.c line 89
Quoted 2 lines
drivers/soc/qcom/smem.c +2
-0+ +extern void qcom_socinfo_init (struct platform_device *pdev);We can't put this into a header file?
Quoted 5 lines
We can. In fact earlier I had put it in
smem.hbut sincesmem.his public API for smem I removed it from there. As it was only a single function I used this extern declaration. Please let me know if it is acceptable. If it is not acceptable I will create asocinfo.hand will put this declaration there.
I'm not sure we care about public vs. non-public APIs intermingled in the same file. Did anyone ask for it to be moved out of the header file? smem.h seems like a fine place to put.
Quoted 43 lines
Quoted 23 lines
Quoted 23 lines
drivers/soc/qcom/socinfo.c new file +16
-0+ +/* Hardware platform types */ +static const char *const hw_platform[] = { + [0 ] ="Unknown" , + [1 ] ="Surf" , + [2 ] ="FFA" , + [3 ] ="Fluid" , + [4 ] ="SVLTE_FFA" , + [5 ] ="SLVTE_SURF" , + [7 ] ="MDM_MTP_NO_DISPLAY" , + [8 ] ="MTP" , + [9 ] ="Liquid" , + [10 ] ="Dragon" , + [11 ] ="QRD" , + [13 ] ="HRD" , + [14 ] ="DTV" ,Quoted 9 lines
drivers/soc/qcom/socinfo.c line 124
Quoted 4 lines
drivers/soc/qcom/socinfo.c new file +4
-0+ [21] = "RCM", + [23] = "STP", + [24] = "SBC", +};These only make sense on qcom platforms. Other boards can reuse the numbers here and have their own names for the platform field, so it doesn't seem like a good idea to do this in the kernel.
Sorry could not understand this. Do you mean that we should only expose the information that we can via generic
soc_device_attribute. This object is being used forhw_platformattribute which we are explicitly creating in sysfs, so it should not conflict with other's implementation.
Or do you mean to just show the numbers and avoid the strings. I am using strings as I think in any case they will be more informative and also there are not many types/subtypes in any case. May be we can keep only those types/subtypes that are more frequent.
Quoted 2 lines
I may be completely wrong in understanding the comment here so could you kindly elaborate this a bit.
I mean that the number 8 for example could mean MTP on qcom platforms but to an ODM that isn't qcom (i.e. some phone manufacturer) the number 8 could mean "wonderbolt 345". We really have no way to control this number as it's completely arbitrary what it means.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/soc/qcom/socinfo.c new file +8
-0+ +static const char *const qrd_hw_platform_subtype[] = { + [0 ] ="QRD" , + [1 ] ="SKUAA" , + [2 ] ="SKUF" , + [3 ] ="SKUAB" , + [5 ] ="SKUG" , + [6 ] ="INVALID" ,
Quoted 8 lines
Quoted 8 lines
drivers/soc/qcom/socinfo.c line 137
Quoted 5 lines
drivers/soc/qcom/socinfo.c new file +5
-0+}; + +static const char *const hw_platform_subtype[] = { +"Unknown" ,"charm" ,"strange" ,"strange_2a" ,"Invalid" , +};Same point here. Seems impossible to maintain this so please get rid of it and just output raw numbers if anything.
And here the subtype becomes extremely complicated to parse. charm, strange, etc. are really old platform subtypes that I don't believe are used anymore but have stayed around in the code for unknown reasons. The subtype field is very large and is purely qcom specific.
Quoted 24 lines
Quoted 14 lines
Quoted 14 lines
drivers/soc/qcom/socinfo.c new file +13
-0+ +static const char *const pmic_model[] = { + [0 ] ="Unknown PMIC model" , + [13 ] ="PMIC model: PM8058" , + [14 ] ="PMIC model: PM8028" , + [15 ] ="PMIC model: PM8901" , + [16 ] ="PMIC model: PM8027" , + [17 ] ="PMIC model: ISL9519" , + [18 ] ="PMIC model: PM8921" , + [19 ] ="PMIC model: PM8018" , + [20 ] ="PMIC model: PM8015" , + [21 ] ="PMIC model: PM8014" , + [22 ] ="PMIC model: PM8821" ,Quoted 9 lines
drivers/soc/qcom/socinfo.c line 153
Quoted 3 lines
drivers/soc/qcom/socinfo.c new file +3
-0+ [23] = "PMIC model: PM8038", + [24] = "PMIC model: PM8922", + [25] = "PMIC model: PM8917",Why do we have "PMIC model:" in sysfs? Shouldn't that be evident from the file name and shouldn't we strive for something more machine readable? And do we expose all the different models in sysfs or just the first one?
Quoted 6 lines
We are exposing just the first PMIC model and yes "PMIC model:" is redundant here. Will remove this in the next patch set. The SMEM content just gives the numeric PMIC id so here we can either dump that id or a string. As of now I am intending to dump string.
Please let me know if that looks okay.
Sounds ok.
Quoted 27 lines
Quoted 14 lines
drivers/soc/qcom/socinfo.c line 654
Quoted 8 lines
drivers/soc/qcom/socinfo.c new file +7
-0+return ; + } + soc_version = socinfo->v0_1.version; + + attr->soc_id =kasprintf (GFP_KERNEL ,"%d" , socinfo->v0_1.id); + attr->family ="Snapdragon" ; + attr->machine = cpu_of_id[socinfo->v0_1.id];The id is used twice. According to the ABI documentation
soc_idis a serial number. The part number is not the same as a serial number so assigningsoc_iddoesn't seem correct. Probably we should only assignsoc_idto be v10serial_number.This part has been a point of confusion for me since long. Ealier I had discussed this point in threads pertaining to other patch sets but did not get any confirmation. I see that quite a few similar drivers have avoided the "
attr->machine" field altogether:
http://lxr.free-electrons.com/source/arch/arm/mach-tegra/tegra.c
http://lxr.free-electrons.com/source/arch/arm/mach-zynq/common.c
So not sure if we should do the same or have a string in machine and
a numeric id insoc_id.
I am afraid that since v10 is relatively newer version of socinfo format, some older socs may not be able to provideserial_numberalthough they might be having a valid soc-id.
Quoted 1 line
Could you please provide your feedback in this regard?
Hm.. perhaps serial number is confusing me. Those code examples seem to show soc_id as the part number (e.g. MSM8996 os MSM8974) in raw numeric form. So what you have here is ok then, just the documentation is very confusing.
Quoted 22 lines
Quoted 13 lines
Quoted 13 lines
drivers/soc/qcom/socinfo.c new file +12
-0+ attr->revision =kasprintf (GFP_KERNEL ,"%u.%u" , +SOC_VERSION_MAJOR (soc_version), +SOC_VERSION_MINOR (soc_version)); + + soc_dev =soc_device_register (attr); +if (IS_ERR (soc_dev)) { +kfree (attr); +return ; + } + + qcom_soc_device =soc_device_to_device (soc_dev); +socinfo_populate_sysfs_files (qcom_soc_device);Quoted 9 lines
drivers/soc/qcom/socinfo.c line 669
Quoted 3 lines
drivers/soc/qcom/socinfo.c new file +3
-0+ +/* Feed the soc specific unique data into entropy pool */ +add_device_randomness (socinfo, size);And thus adding mostly the same random bits for every SoC that's the same as a million other ones doesn't seem like a good choice for device randomness. Probably only the v10
serial_numbershould be added to the random pool.
Quoted 3 lines
Yes a lot of fields in socinfo object tend to have same value. How about putting soc-id and v10 serial number (if we can conclude that these 2 should be different) along with build-id.
soc-id is not really unique per-device. How about only adding the serial number if we have v10 format?
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 10 lines · Stephen Boyd
Quoted 9 lines · Imran Khan
Quoted 7 lines · Stephen Boyd
Quoted 3 lines · Imran Khan
The SoC info driver provides information such as Chip ID, Chip family, serial number and other such details about Qualcomm SoCs.
Yes but why do we care?
Quoted 8 lines
Quoted 5 lines
We intend to expose some standard SoC attributes (like soc-id) of QCOM SoCs to user space, so that if needed the user space utilities (like antutu) can query such information using sysfs interface. A further example can be a user space script (say Android's init.rc) which reads soc-id and does some gpio settings based on the soc-id.
Please include such information into the commit text.
Sure. I will.
Quoted 21 lines
Quoted 15 lines
Quoted 9 lines
Quoted 9 lines
drivers/soc/qcom/smem.c +0
-0@@ -85,6 +85,9 @@/* Max number of processors/hosts in a system */ #define SMEM_HOST_COUNT 9Quoted 5 lines
drivers/soc/qcom/smem.c line 89
Quoted 2 lines
drivers/soc/qcom/smem.c +2
-0+ +extern void qcom_socinfo_init (struct platform_device *pdev);We can't put this into a header file?
Quoted 5 lines
We can. In fact earlier I had put it in
smem.hbut sincesmem.his public API for smem I removed it from there. As it was only a single function I used this extern declaration. Please let me know if it is acceptable. If it is not acceptable I will create asocinfo.hand will put this declaration there.
Quoted 4 lines
I'm not sure we care about public vs. non-public APIs intermingled in the same file. Did anyone ask for it to be moved out of the header file?
smem.hseems like a fine place to put.
I had discussed this with Bjorn and it was recommended to keep it out of smem.h. If needed I can move it back there.
Quoted 43 lines
Quoted 43 lines
Quoted 23 lines
Quoted 23 lines
drivers/soc/qcom/socinfo.c new file +16
-0+ +/* Hardware platform types */ +static const char *const hw_platform[] = { + [0 ] ="Unknown" , + [1 ] ="Surf" , + [2 ] ="FFA" , + [3 ] ="Fluid" , + [4 ] ="SVLTE_FFA" , + [5 ] ="SLVTE_SURF" , + [7 ] ="MDM_MTP_NO_DISPLAY" , + [8 ] ="MTP" , + [9 ] ="Liquid" , + [10 ] ="Dragon" , + [11 ] ="QRD" , + [13 ] ="HRD" , + [14 ] ="DTV" ,Quoted 9 lines
drivers/soc/qcom/socinfo.c line 124
Quoted 4 lines
drivers/soc/qcom/socinfo.c new file +4
-0+ [21] = "RCM", + [23] = "STP", + [24] = "SBC", +};These only make sense on qcom platforms. Other boards can reuse the numbers here and have their own names for the platform field, so it doesn't seem like a good idea to do this in the kernel.
Sorry could not understand this. Do you mean that we should only expose the information that we can via generic
soc_device_attribute. This object is being used forhw_platformattribute which we are explicitly creating in sysfs, so it should not conflict with other's implementation.
Or do you mean to just show the numbers and avoid the strings. I am using strings as I think in any case they will be more informative and also there are not many types/subtypes in any case. May be we can keep only those types/subtypes that are more frequent.
Quoted 9 lines
Quoted 2 lines
I may be completely wrong in understanding the comment here so could you kindly elaborate this a bit.
I mean that the number 8 for example could mean MTP on qcom platforms but to an ODM that isn't qcom (i.e. some phone manufacturer) the number 8 could mean "wonderbolt 345". We really have no way to control this number as it's completely arbitrary what it means.
Yes. The numbers used here can have different meaning for different ODMs. But these attributes (hw_patform type/subtype etc.) are outside the generic soc_device_attribute so I think the interpretation of these numbers can very well be ODM specific. We can try to keep only those types here that are relevant for newer platforms but we intend to keep these attributes nonetheless.
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/soc/qcom/socinfo.c new file +8
-0+ +static const char *const qrd_hw_platform_subtype[] = { + [0 ] ="QRD" , + [1 ] ="SKUAA" , + [2 ] ="SKUF" , + [3 ] ="SKUAB" , + [5 ] ="SKUG" , + [6 ] ="INVALID" ,Quoted 8 lines
Quoted 8 lines
drivers/soc/qcom/socinfo.c line 137
Quoted 5 lines
drivers/soc/qcom/socinfo.c new file +5
-0+}; + +static const char *const hw_platform_subtype[] = { +"Unknown" ,"charm" ,"strange" ,"strange_2a" ,"Invalid" , +};Same point here. Seems impossible to maintain this so please get rid of it and just output raw numbers if anything.
Quoted 6 lines
And here the subtype becomes extremely complicated to parse. charm, strange, etc. are really old platform subtypes that I don't believe are used anymore but have stayed around in the code for unknown reasons. The subtype field is very large and is purely qcom specific.
Again we can keep here only relevant subtypes.
Quoted 89 lines
Quoted 24 lines
Quoted 14 lines
Quoted 14 lines
drivers/soc/qcom/socinfo.c new file +13
-0+ +static const char *const pmic_model[] = { + [0 ] ="Unknown PMIC model" , + [13 ] ="PMIC model: PM8058" , + [14 ] ="PMIC model: PM8028" , + [15 ] ="PMIC model: PM8901" , + [16 ] ="PMIC model: PM8027" , + [17 ] ="PMIC model: ISL9519" , + [18 ] ="PMIC model: PM8921" , + [19 ] ="PMIC model: PM8018" , + [20 ] ="PMIC model: PM8015" , + [21 ] ="PMIC model: PM8014" , + [22 ] ="PMIC model: PM8821" ,Quoted 9 lines
drivers/soc/qcom/socinfo.c line 153
Quoted 3 lines
drivers/soc/qcom/socinfo.c new file +3
-0+ [23] = "PMIC model: PM8038", + [24] = "PMIC model: PM8922", + [25] = "PMIC model: PM8917",Why do we have "PMIC model:" in sysfs? Shouldn't that be evident from the file name and shouldn't we strive for something more machine readable? And do we expose all the different models in sysfs or just the first one?
Quoted 6 lines
We are exposing just the first PMIC model and yes "PMIC model:" is redundant here. Will remove this in the next patch set. The SMEM content just gives the numeric PMIC id so here we can either dump that id or a string. As of now I am intending to dump string.
Please let me know if that looks okay.Sounds ok.
Quoted 27 lines
Quoted 14 lines
drivers/soc/qcom/socinfo.c line 654
Quoted 8 lines
drivers/soc/qcom/socinfo.c new file +7
-0+return ; + } + soc_version = socinfo->v0_1.version; + + attr->soc_id =kasprintf (GFP_KERNEL ,"%d" , socinfo->v0_1.id); + attr->family ="Snapdragon" ; + attr->machine = cpu_of_id[socinfo->v0_1.id];The id is used twice. According to the ABI documentation
soc_idis a serial number. The part number is not the same as a serial number so assigningsoc_iddoesn't seem correct. Probably we should only assignsoc_idto be v10serial_number.This part has been a point of confusion for me since long. Ealier I had discussed this point in threads pertaining to other patch sets but did not get any confirmation. I see that quite a few similar drivers have avoided the "
attr->machine" field altogether:
http://lxr.free-electrons.com/source/arch/arm/mach-tegra/tegra.c
http://lxr.free-electrons.com/source/arch/arm/mach-zynq/common.c
So not sure if we should do the same or have a string in machine and
a numeric id insoc_id.
I am afraid that since v10 is relatively newer version of socinfo format, some older socs may not be able to provideserial_numberalthough they might be having a valid soc-id.Quoted 1 line
Could you please provide your feedback in this regard?
Hm.. perhaps serial number is confusing me. Those code examples seem to show
soc_idas the part number (e.g. MSM8996 os MSM8974) in raw numeric form. So what you have here is ok then, just the documentation is very confusing.Quoted 22 lines
Quoted 22 lines
Quoted 6 lines
drivers/soc/qcom/socinfo.c new file +5
-0+ attr->revision =kasprintf (GFP_KERNEL ,"%u.%u" , +SOC_VERSION_MAJOR (soc_version), +SOC_VERSION_MINOR (soc_version)); + + soc_dev =soc_device_register (attr);drivers/soc/qcom/socinfo.c line 669
Quoted 10 lines
drivers/soc/qcom/socinfo.c new file +10
-0+if (IS_ERR (soc_dev)) { +kfree (attr); +return ; + } + + qcom_soc_device =soc_device_to_device (soc_dev); +socinfo_populate_sysfs_files (qcom_soc_device); + +/* Feed the soc specific unique data into entropy pool */ +add_device_randomness (socinfo, size);And thus adding mostly the same random bits for every SoC that's the same as a million other ones doesn't seem like a good choice for device randomness. Probably only the v10
serial_numbershould be added to the random pool.
Quoted 7 lines
Quoted 3 lines
Yes a lot of fields in socinfo object tend to have same value. How about putting soc-id and v10 serial number (if we can conclude that these 2 should be different) along with build-id.
soc-id is not really unique per-device. How about only adding the serial number if we have v10 format?
Okay. Sounds fine to me.
Thanks and Regards,
Imran
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
Quoted 3 lines · Imran Khan
I had discussed this with Bjorn and it was recommended to keep it out of
smem.h. If needed I can move it back there.
Ok no worries from me then if this has already been discussed.
Quoted 7 lines
Yes. The numbers used here can have different meaning for different ODMs. But these attributes (
hw_patformtype/subtype etc.) are outside the genericsoc_device_attributeso I think the interpretation of these numbers can very well be ODM specific. We can try to keep only those types here that are relevant for newer platforms but we intend to keep these attributes nonetheless.
I'll wait to see what the next patch version has. We will probably need to have some way to know which ODM the kernel is running on, so we can interpret the platform type/subtype fields properly. That part seems to be lacking from this patch right now. We assume it's always qcom as the ODM, which isn't true.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 15 lines · Stephen Boyd
Quoted 3 lines · Imran Khan
I had discussed this with Bjorn and it was recommended to keep it out of
smem.h. If needed I can move it back there.Ok no worries from me then if this has already been discussed.
Quoted 7 lines
Yes. The numbers used here can have different meaning for different ODMs. But these attributes (
hw_patformtype/subtype etc.) are outside the genericsoc_device_attributeso I think the interpretation of these numbers can very well be ODM specific. We can try to keep only those types here that are relevant for newer platforms but we intend to keep these attributes nonetheless.
Quoted 6 lines
I'll wait to see what the next patch version has. We will probably need to have some way to know which ODM the kernel is running on, so we can interpret the platform type/subtype fields properly. That part seems to be lacking from this patch right now. We assume it's always qcom as the ODM, which isn't true.
Now I get this point. So far we don't have any mechanism in the driver that gives ODM information. As far as generic soc_device_attribute's vendor field is concerned we use Qualcomm since this will be true for SoC.
For hardware type and sub-types the various relevant values in SMEM are numeric values and indeed it would be very difficult to estimate how some other ODM will use the same number.
So for the h/w types and sub-types can we keep the numeric values rather than showing strings as attribute values. We can leave the interpretation of these values to ODM specific code.
Will wait for your feedback so that I can take care of it accordingly in the next patch set.
Regards,
Imran
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
Quoted 8 lines · Imran Khan
Quoted 7 lines · Stephen Boyd
I'll wait to see what the next patch version has. We will probably need to have some way to know which ODM the kernel is running on, so we can interpret the platform type/subtype fields properly. That part seems to be lacking from this patch right now. We assume it's always qcom as the ODM, which isn't true.
Quoted 9 lines
Now I get this point. So far we don't have any mechanism in the driver that gives ODM information. As far as generic soc_device_attribute's vendor field is concerned we use Qualcomm since this will be true for SoC.
For hardware type and sub-types the various relevant values in SMEM are numeric values and indeed it would be very difficult to estimate how some other ODM will use the same number.
So for the h/w types and sub-types can we keep the numeric values rather than showing strings as attribute values. We can leave the interpretation of these values to ODM specific code.
Raw numbers sounds fine, but how do we know what ODM it is to understand how to parse the numbers appropriately? Perhaps the smem DT entry needs to have a property indicating the ODM that has configured these numbers, and then we can have an ODM sysfs node that we use to expose that string property to userspace?
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 19 lines · Stephen Boyd
Quoted 8 lines · Imran Khan
Quoted 7 lines · Stephen Boyd
I'll wait to see what the next patch version has. We will probably need to have some way to know which ODM the kernel is running on, so we can interpret the platform type/subtype fields properly. That part seems to be lacking from this patch right now. We assume it's always qcom as the ODM, which isn't true.
Quoted 9 lines
Now I get this point. So far we don't have any mechanism in the driver that gives ODM information. As far as generic soc_device_attribute's vendor field is concerned we use Qualcomm since this will be true for SoC.
For hardware type and sub-types the various relevant values in SMEM are numeric values and indeed it would be very difficult to estimate how some other ODM will use the same number.
So for the h/w types and sub-types can we keep the numeric values rather than showing strings as attribute values. We can leave the interpretation of these values to ODM specific code.
Quoted 6 lines
Raw numbers sounds fine, but how do we know what ODM it is to understand how to parse the numbers appropriately? Perhaps the smem DT entry needs to have a property indicating the ODM that has configured these numbers, and then we can have an ODM sysfs node that we use to expose that string property to userspace?
Okay smem DT entry can be used to provide ODM information but even after having this feature, I am not sure if we can provide a code in the driver that will act for all ODMs because we don't know how other ODMs will interpret platform types and subtypes numbers.
Or do you mean here that we should keep string values corresponding to different platform type and subtype numbers in the smem DT entry itself. We will use socinfo from smem to get the raw number and then translate that raw number to a string, using the mapping given in DT itself.
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
Quoted 8 lines · Imran Khan
Quoted 7 lines · Stephen Boyd
Raw numbers sounds fine, but how do we know what ODM it is to understand how to parse the numbers appropriately? Perhaps the smem DT entry needs to have a property indicating the ODM that has configured these numbers, and then we can have an ODM sysfs node that we use to expose that string property to userspace?
Quoted 9 lines
Okay smem DT entry can be used to provide ODM information but even after having this feature, I am not sure if we can provide a code in the driver that will act for all ODMs because we don't know how other ODMs will interpret platform types and subtypes numbers.
Or do you mean here that we should keep string values corresponding to different platform type and subtype numbers in the smem DT entry itself. We will use socinfo from smem to get the raw number and then translate that raw number to a string, using the mapping given in DT itself.
I mean in DT
smem {
compatible = "qcom,smem";
qcom,odm = "odm_name";
}And then in the driver code we look for the qcom,odm property and make a sysfs attribute called odm or something that exposes the string "odm_name" to userspace. Then we have some userspace database of odm string and platform type/subtype numbers that we can use to figure out what those numbers mean.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 26 lines · Stephen Boyd
Quoted 8 lines · Imran Khan
Quoted 7 lines · Stephen Boyd
Raw numbers sounds fine, but how do we know what ODM it is to understand how to parse the numbers appropriately? Perhaps the smem DT entry needs to have a property indicating the ODM that has configured these numbers, and then we can have an ODM sysfs node that we use to expose that string property to userspace?
Quoted 9 lines
Okay smem DT entry can be used to provide ODM information but even after having this feature, I am not sure if we can provide a code in the driver that will act for all ODMs because we don't know how other ODMs will interpret platform types and subtypes numbers.
Or do you mean here that we should keep string values corresponding to different platform type and subtype numbers in the smem DT entry itself. We will use socinfo from smem to get the raw number and then translate that raw number to a string, using the mapping given in DT itself.I mean in DT
smem { compatible = "qcom,smem"; qcom,odm = "odm_name"; }
Quoted 6 lines
And then in the driver code we look for the qcom,odm property and make a sysfs attribute called odm or something that exposes the string "
odm_name" to userspace. Then we have some userspace database of odm string and platform type/subtype numbers that we can use to figure out what those numbers mean.
Okay. This approach is fine for me. In the mean time I had tried one alternative approach which I wanted to share. So in the smem DT entry we have something like:
smem {
compatible = "qcom,smem";
..............................
..............................
smem,plat-type = "Unknown", "Surf", "FFA", "Fluid",
"SVLTE_FFA", "SVLTE_SURF", "Unknown",
"MDM_MTP_NO_DISPLAY", "MTP", "Liquid",
"Dragon", "QRD", "Unknown","HRD", "DTV";
smem,qrd-plat-subtype = "QRD", "SKUAA", "SKUF", "SKUAB",
"SKUG";
smem,plat-subtype = "Unknown", "charm", "strange",
"strange_2a";
};And back in the qcom_soc_init, we read these lists as per the value(index) obtained from smem:
if(socinfo->v0_1.format >= 6) {
/* Get platform type and subtype here */
type = socinfo->v0_6.hw_platform_subtype;
if(type >= 0 && plat_type) {
if (socinfo->v0_3.hw_platform == HW_PLATFORM_QRD) {
type_max = of_property_count_strings(
device->of_node,
"smem,qrd-plat-subtype");
if(type < type_max) {
of_property_read_string_index(
device->of_node,
"smem,qrd-plat-subtype",
type, &plat_type->sub_type);
}
} else {
type_max = of_property_count_strings(
device->of_node,
"smem,plat-subtype");
if(type < type_max) {
of_property_read_string_index(
device->of_node,
"smem,plat-subtype",
type, &plat_type->sub_type);
}
}
}
}
if (socinfo->v0_1.format >= 3) {
/* Get only platform type */
type = socinfo->v0_3.hw_platform;
type_max = of_property_count_strings(
device->of_node, "smem,plat-type");
if((type >= 0 && type < type_max) && plat_type) {
of_property_read_string_index(device->of_node,
"smem,plat-type", type,
&plat_type->type);
}
}Could you please also provide your feedback about this approach? Just wanted to share this before going ahead with final implementation.
Regards,
Imran
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation