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.

Signed-off-by Imran Khan <[email protected]>

drivers/soc/qcom/Makefile +0 -0

 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 +0 -0

index 18ec52f..05ff935 100644

drivers/soc/qcom/smem.c +3 -1

+
 /**
   * struct smem_proc_comm - proc_comm communication struct (legacy)
   * @command:	current command to be executed

@@ -695,7 +701,8 @@ static int qcom_smem_probe(struct platform_device *pdev)
 {
 	struct smem_header *header;
 	struct qcom_smem *smem;
-	size_t array_size;
+	void *socinfo;
+	size_t array_size, size;
 	int num_regions;
 	int hwlock_id;
 	u32 version;

@@ -751,6 +758,15 @@ static int qcom_smem_probe(struct platform_device *pdev)
 
 	__smem = smem;

drivers/soc/qcom/smem.c +0 -0

 	return 0;
 }
 

drivers/soc/qcom/socinfo.c +0 -0

+/*
+ * SOC Info Routines

drivers/soc/qcom/socinfo.c +12 -0

+
+#include <linux/export.h>
+#include <linux/module.h>
+#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>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/random.h>

+#define SOCINFO_VERSION(maj, min) ((((maj) & 0xffff) << 16)|((min) & 0xffff))

+#define PLATFORM_SUBTYPE_INTERPOSERV3 2

+#define PLATFORM_SUBTYPE_SGLTE 6

drivers/soc/qcom/socinfo.c +57 -0

+
+/*
+ * Shared memory identifiers, used to acquire handles to respective memory
+ * region.
+ */
+#define SMEM_IMAGE_VERSION_TABLE	469
+
+/*
+ * Qcom SoC types
+ */
+enum qcom_cpu {
+	MSM_CPU_UNKNOWN = 0,
+	MSM_CPU_8960,
+	MSM_CPU_8960AB,
+	MSM_CPU_8064,
+	MSM_CPU_8974,
+	MSM_CPU_8974PRO_AA,
+	MSM_CPU_8974PRO_AB,
+	MSM_CPU_8974PRO_AC,
+	MSM_CPU_8916,
+	MSM_CPU_8084,
+	MSM_CPU_8996
+};
+
+/*
+ * Qcom SoC IDs
+ */
+enum qcom_cpu_id {
+	MSM_UNKNOWN_ID,
+	MSM_8960_ID = 87,
+	APQ_8064_ID = 109,
+	MSM_8660A_ID = 122,
+	MSM_8260A_ID,
+	APQ_8060A_ID,
+	MSM_8974_ID = 126,
+	MPQ_8064_ID = 130,
+	MSM_8960AB_ID = 138,
+	APQ_8060AB_ID,
+	MSM_8260AB_ID,
+	MSM_8660AB_ID,
+	APQ_8084_ID = 178,
+	APQ_8074_ID = 184,
+	MSM_8274_ID,
+	MSM_8674_ID,
+	MSM_8974PRO_ID = 194,
+	MSM_8916_ID = 206,
+	APQ_8074_AA_ID = 208,
+	APQ_8074_AB_ID,
+	APQ_8074PRO_ID,
+	MSM_8274_AA_ID,
+	MSM_8274_AB_ID,
+	MSM_8274PRO_ID,
+	MSM_8674_AA_ID,
+	MSM_8674_AB_ID,
+	MSM_8674PRO_ID,
+	MSM_8974_AA_ID,
+	MSM_8974_AB_ID,

drivers/soc/qcom/socinfo.c +25 -0

+	PMIC_MODEL_PM8058 = 13,
+	PMIC_MODEL_PM8028,
+	PMIC_MODEL_PM8901,
+	PMIC_MODEL_PM8027,
+	PMIC_MODEL_ISL_9519,
+	PMIC_MODEL_PM8921,
+	PMIC_MODEL_PM8018,
+	PMIC_MODEL_PM8015,
+	PMIC_MODEL_PM8014,
+	PMIC_MODEL_PM8821,
+	PMIC_MODEL_PM8038,
+	PMIC_MODEL_PM8922,
+	PMIC_MODEL_PM8917,
+	PMIC_MODEL_UNKNOWN = 0xFFFFFFFF
+};
+
+enum hw_platform_type {
+	HW_PLATFORM_UNKNOWN,
+	HW_PLATFORM_SURF,
+	HW_PLATFORM_FFA,
+	HW_PLATFORM_FLUID,
+	HW_PLATFORM_SVLTE_FFA,
+	HW_PLATFORM_SVLTE_SURF,
+	HW_PLATFORM_MTP_MDM = 7,
+	HW_PLATFORM_MTP,

drivers/soc/qcom/socinfo.c +4 -0

+	PLATFORM_SUBTYPE_QRD,
+	PLATFORM_SUBTYPE_SKUAA,
+	PLATFORM_SUBTYPE_SKUF,
+	PLATFORM_SUBTYPE_SKUAB,

drivers/soc/qcom/socinfo.c +37 -0

+static const char *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",
+};
+
+static const char *qrd_hw_platform_subtype[] = {
+	[PLATFORM_SUBTYPE_QRD] = "QRD",
+	[PLATFORM_SUBTYPE_SKUAA] = "SKUAA",
+	[PLATFORM_SUBTYPE_SKUF] = "SKUF",
+	[PLATFORM_SUBTYPE_SKUAB] = "SKUAB",
+	[PLATFORM_SUBTYPE_SKUG] = "SKUG",
+	[PLATFORM_SUBTYPE_QRD_INVALID] = "INVALID",
+};
+
+static const char *hw_platform_subtype[] = {
+	[PLATFORM_SUBTYPE_UNKNOWN] = "Unknown",
+	[PLATFORM_SUBTYPE_CHARM] = "charm",
+	[PLATFORM_SUBTYPE_STRANGE] = "strange",
+	[PLATFORM_SUBTYPE_STRANGE_2A] = "strange_2a",
+	[PLATFORM_SUBTYPE_INVALID] = "Invalid",
+};
+#endif
+

drivers/soc/qcom/socinfo.c +68 -0

+
+static struct qcom_soc_info cpu_of_id[] = {
+
+	[MSM_UNKNOWN_ID] = {MSM_CPU_UNKNOWN, "Unknown CPU"},
+
+	/* 8x60 IDs */
+	[MSM_8960_ID] = {MSM_CPU_8960, "MSM8960"},
+
+	/* 8x64 IDs */
+	[APQ_8064_ID] = {MSM_CPU_8064, "APQ8064"},
+	[MPQ_8064_ID] = {MSM_CPU_8064, "MPQ8064"},
+
+	/* 8x60A IDs */
+	[MSM_8660A_ID] = {MSM_CPU_8960, "MSM8660A"},
+	[MSM_8260A_ID] = {MSM_CPU_8960, "MSM8260A"},
+	[APQ_8060A_ID] = {MSM_CPU_8960, "APQ8060A"},
+
+	/* 8x74 IDs */
+	[MSM_8974_ID] = {MSM_CPU_8974, "MSM8974"},
+	[APQ_8074_ID] = {MSM_CPU_8974, "APQ8074"},
+	[MSM_8274_ID] = {MSM_CPU_8974, "MSM8274"},
+	[MSM_8674_ID] = {MSM_CPU_8974, "MSM8674"},
+
+	/* 8x74AA IDs */
+	[APQ_8074_AA_ID] = {MSM_CPU_8974PRO_AA, "APQ8074-AA"},
+	[MSM_8274_AA_ID] = {MSM_CPU_8974PRO_AA, "MSM8274-AA"},
+	[MSM_8674_AA_ID] = {MSM_CPU_8974PRO_AA, "MSM8674-AA"},
+	[MSM_8974_AA_ID] = {MSM_CPU_8974PRO_AA, "MSM8974-AA"},
+
+	/* 8x74AB IDs */
+	[APQ_8074_AB_ID] = {MSM_CPU_8974PRO_AB, "APQ8074-AB"},
+	[MSM_8274_AB_ID] = {MSM_CPU_8974PRO_AB, "MSM8274-AB"},
+	[MSM_8674_AB_ID] = {MSM_CPU_8974PRO_AB, "MSM8674-AB"},
+	[MSM_8974_AB_ID] = {MSM_CPU_8974PRO_AB, "MSM8974-AB"},
+
+	/* 8x74AC IDs */
+	[MSM_8974PRO_ID] = {MSM_CPU_8974PRO_AC, "MSM8974PRO"},
+	[APQ_8074PRO_ID] = {MSM_CPU_8974PRO_AC, "APQ8074PRO"},
+	[MSM_8274PRO_ID] = {MSM_CPU_8974PRO_AC, "MSM8274PRO"},
+	[MSM_8674PRO_ID] = {MSM_CPU_8974PRO_AC, "MSM8674PRO"},
+
+	/* 8x60AB IDs */
+	[MSM_8960AB_ID] = {MSM_CPU_8960AB, "MSM8960AB"},
+	[APQ_8060AB_ID] = {MSM_CPU_8960AB, "APQ8060AB"},
+	[MSM_8260AB_ID] = {MSM_CPU_8960AB, "MSM8260AB"},
+	[MSM_8660AB_ID] = {MSM_CPU_8960AB, "MSM8660AB"},
+
+	/* 8x84 IDs */
+	[APQ_8084_ID] = {MSM_CPU_8084, "APQ8084"},
+
+	/* 8x16 IDs */
+	[MSM_8916_ID] = {MSM_CPU_8916, "MSM8916"},
+	[APQ_8016_ID] = {MSM_CPU_8916, "APQ8016"},
+	[MSM_8216_ID] = {MSM_CPU_8916, "MSM8216"},
+	[MSM_8116_ID] = {MSM_CPU_8916, "MSM8116"},
+	[MSM_8616_ID] = {MSM_CPU_8916, "MSM8616"},
+
+	/* 8x96 IDs */
+	[MSM_8996_ID] = {MSM_CPU_8996, "MSM8996"},
+	[MSM_8996AU_ID] = {MSM_CPU_8996, "MSM8996AU"},
+	[APQ_8096AU_ID] = {MSM_CPU_8996, "APQ8096AU"},
+	[APQ8096_ID] = {MSM_CPU_8996, "APQ8096"},
+	[MSM_8996SG_ID] = {MSM_CPU_8996, "MSM8996SG"},
+	[APQ_8096SG_ID] = {MSM_CPU_8996, "APQ8096SG"},
+
+	/*
+	 * Uninitialized IDs are not known to run Linux.
+	 * MSM_CPU_UNKNOWN is set to 0 to ensure these IDs are

drivers/soc/qcom/socinfo.c +4 -0

+static int current_image;
+static u32 socinfo_get_id(void);
+
+/* socinfo: sysfs functions */

drivers/soc/qcom/socinfo.c +72 -0

+	if (size_in != size) {
 
 
+		dev_err(dev, "Wrong size for smem item\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	return ptr;
+}
+
+
+static u32 socinfo_get_version(void)
+{
+	return (socinfo) ? socinfo->v0_1.version : 0;
+}
+
+static char *socinfo_get_build_id(void)
+{
+	return (socinfo) ? socinfo->v0_1.build_id : NULL;
+}
+
+static u32 socinfo_get_raw_version(void)
+{
+	return socinfo ?
+		(socinfo_format >= SOCINFO_VERSION(0, 2) ?
+			socinfo->v0_2.raw_version : 0)
+		: 0;
+}
+
+static u32 socinfo_get_platform_type(void)
+{
+	return socinfo ?
+		(socinfo_format >= SOCINFO_VERSION(0, 3) ?
+			socinfo->v0_3.hw_platform : 0)
+		: 0;
+}
+
+static u32 socinfo_get_platform_version(void)
+{
+	return socinfo ?
+		(socinfo_format >= SOCINFO_VERSION(0, 4) ?
+			socinfo->v0_4.platform_version : 0)
+		: 0;
+}
+
+static u32 socinfo_get_platform_subtype(void)
+{
+	return socinfo ?
+		(socinfo_format >= SOCINFO_VERSION(0, 6) ?
+			socinfo->v0_6.hw_platform_subtype : 0)
+		: 0;
+}
+
+static u32 socinfo_get_serial_number(void)
+{
+	return socinfo ?
+		(socinfo_format >= SOCINFO_VERSION(0, 10) ?
+			socinfo->v0_10.serial_number : 0)
+		: 0;
+}
+
+static enum qcom_pmic_model socinfo_get_pmic_model(void)
+{
+	return socinfo ?
+		(socinfo_format >= SOCINFO_VERSION(0, 7) ?
+			socinfo->v0_7.pmic_model : PMIC_MODEL_UNKNOWN)
+		: PMIC_MODEL_UNKNOWN;
+}
+
+static u32 socinfo_get_pmic_die_revision(void)
+{
+	return socinfo ?
+		(socinfo_format >= SOCINFO_VERSION(0, 7) ?
+			socinfo->v0_7.pmic_die_revision : 0)

drivers/soc/qcom/socinfo.c +43 -0

+}
+
+static ssize_t
+qcom_get_build_id(struct device *dev,
+		   struct device_attribute *attr,
+		   char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%-.32s\n",
+			socinfo_get_build_id());
+}
+
+static ssize_t
+qcom_get_hw_platform(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	u32 hw_type;
+
+	hw_type = socinfo_get_platform_type();
+
+	return snprintf(buf, PAGE_SIZE, "%-.32s\n",
+			hw_platform[hw_type]);
+}
+
+static ssize_t
+qcom_get_platform_version(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%u\n",
+		socinfo_get_platform_version());
+}
+
+static ssize_t
+qcom_get_accessory_chip(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%u\n",
+		socinfo_get_accessory_chip());
+}
+
+static ssize_t

drivers/soc/qcom/socinfo.c +14 -0

+			hw_subtype = PLATFORM_SUBTYPE_QRD_INVALID;
+		}
+		return snprintf(buf, PAGE_SIZE, "%-.32s\n",
+					qrd_hw_platform_subtype[hw_subtype]);
+	} else {
+		if (hw_subtype >= PLATFORM_SUBTYPE_INVALID) {
+			pr_err("Invalid hardware platform subtype\n");
+			hw_subtype = PLATFORM_SUBTYPE_INVALID;
+		}
+		return snprintf(buf, PAGE_SIZE, "%-.32s\n",
+			hw_platform_subtype[hw_subtype]);
+	}
+}
+

drivers/soc/qcom/socinfo.c +42 -0

+}
+
+static ssize_t
+qcom_get_foundry_id(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%u\n",
+		socinfo_get_foundry_id());
+}
+
+static ssize_t
+qcom_get_serial_number(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%u\n",
+		socinfo_get_serial_number());
+}
+
+static ssize_t
+qcom_get_chip_family(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "0x%x\n",
+		socinfo_get_chip_family());
+}
+
+static ssize_t
+qcom_get_raw_device_family(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "0x%x\n",
+		socinfo_get_raw_device_family());
+}
+
+static ssize_t
+qcom_get_raw_device_number(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)

drivers/soc/qcom/socinfo.c +11 -0

+	return snprintf(buf, PAGE_SIZE, "%u\n",
+		socinfo_get_pmic_model());
+}
+
+static ssize_t
+qcom_get_pmic_die_revision(struct device *dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%u\n",
+			 socinfo_get_pmic_die_revision());

drivers/soc/qcom/socinfo.c +9 -0

+	if (IS_ERR(string_address)) {
+		pr_err("Failed to get image version base address");
+		return snprintf(buf, SMEM_IMAGE_VERSION_NAME_SIZE, "Unknown");
+	}
+	string_address += current_image * SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
+	return snprintf(buf, SMEM_IMAGE_VERSION_NAME_SIZE, "%-.75s\n",
+			string_address);
+}
+

drivers/soc/qcom/socinfo.c +10 -0

+	return count;
+}
+
+static ssize_t
+qcom_get_image_variant(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	char *string_address;
+

drivers/soc/qcom/socinfo.c +59 -0

+}
+
+static ssize_t
+qcom_set_image_variant(struct device *dev,
+			struct device_attribute *attr,
+			const char *buf,
+			size_t count)
+{
+	char *store_address;
+
+	if (current_image != SMEM_IMAGE_VERSION_PARTITION_APPS)
+		return count;
+	store_address = socinfo_get_image_version_base_address(dev);
+	if (IS_ERR(store_address)) {
+		pr_err("Failed to get image version base address");
+		return count;
+	}
+	store_address += current_image * SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
+	store_address += SMEM_IMAGE_VERSION_VARIANT_OFFSET;
+	snprintf(store_address, SMEM_IMAGE_VERSION_VARIANT_SIZE, "%-.20s", buf);
+	return count;
+}
+
+static ssize_t
+qcom_get_image_crm_version(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	char *string_address;
+
+	string_address = socinfo_get_image_version_base_address(dev);
+	if (IS_ERR(string_address)) {
+		pr_err("Failed to get image version base address");
+		return snprintf(buf, SMEM_IMAGE_VERSION_OEM_SIZE, "Unknown");
+	}
+	string_address += current_image * SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
+	string_address += SMEM_IMAGE_VERSION_OEM_OFFSET;
+	return snprintf(buf, SMEM_IMAGE_VERSION_OEM_SIZE, "%-.32s\n",
+			string_address);
+}
+
+static ssize_t
+qcom_set_image_crm_version(struct device *dev,
+			struct device_attribute *attr,
+			const char *buf,
+			size_t count)
+{
+	char *store_address;
+
+	if (current_image != SMEM_IMAGE_VERSION_PARTITION_APPS)
+		return count;
+	store_address = socinfo_get_image_version_base_address(dev);
+	if (IS_ERR(store_address)) {
+		pr_err("Failed to get image version base address");
+		return count;
+	}
+	store_address += current_image * SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
+	store_address += SMEM_IMAGE_VERSION_OEM_OFFSET;
+	snprintf(store_address, SMEM_IMAGE_VERSION_OEM_SIZE, "%-.32s", buf);

drivers/soc/qcom/socinfo.c +6 -0

+}
+
+static ssize_t
+qcom_select_image(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{

drivers/soc/qcom/socinfo.c +24 -0

+	int pos = 0;
+	int image;
+	char *image_address;
+
+	image_address = socinfo_get_image_version_base_address(dev);
+	if (IS_ERR(image_address))
+		return snprintf(buf, PAGE_SIZE, "Unavailable\n");
+
+	*buf = '\0';
+	for (image = 0; image < SMEM_IMAGE_VERSION_BLOCKS_COUNT; image++) {
+		if (*image_address == '\0') {
+			image_address += SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
+			continue;
+		}
+
+		pos += snprintf(buf + pos, PAGE_SIZE - pos, "%d:\n",
+				image);
+		pos += snprintf(buf + pos, PAGE_SIZE - pos,
+				"\tCRM:\t\t%-.75s\n", image_address);
+		pos += snprintf(buf + pos, PAGE_SIZE - pos,
+				"\tVariant:\t%-.20s\n", image_address +
+				SMEM_IMAGE_VERSION_VARIANT_OFFSET);
+		pos += snprintf(buf + pos, PAGE_SIZE - pos,
+				"\tVersion:\t%-.32s\n\n",

drivers/soc/qcom/socinfo.c +68 -0

+
+static struct device_attribute qcom_soc_attr_vendor =
+	__ATTR(vendor, S_IRUGO, qcom_get_vendor,  NULL);
+
+static struct device_attribute qcom_soc_attr_build_id =
+	__ATTR(build_id, S_IRUGO, qcom_get_build_id, NULL);
+
+static struct device_attribute qcom_soc_attr_hw_platform =
+	__ATTR(hw_platform, S_IRUGO, qcom_get_hw_platform, NULL);
+
+
+static struct device_attribute qcom_soc_attr_platform_version =
+	__ATTR(platform_version, S_IRUGO,
+			qcom_get_platform_version, NULL);
+
+static struct device_attribute qcom_soc_attr_accessory_chip =
+	__ATTR(accessory_chip, S_IRUGO,
+			qcom_get_accessory_chip, NULL);
+
+static struct device_attribute qcom_soc_attr_platform_subtype =
+	__ATTR(platform_subtype, S_IRUGO,
+			qcom_get_platform_subtype, NULL);
+
+static struct device_attribute qcom_soc_attr_platform_subtype_id =
+	__ATTR(platform_subtype_id, S_IRUGO,
+			qcom_get_platform_subtype_id, NULL);
+
+static struct device_attribute qcom_soc_attr_foundry_id =
+	__ATTR(foundry_id, S_IRUGO,
+			qcom_get_foundry_id, NULL);
+
+static struct device_attribute qcom_soc_attr_serial_number =
+	__ATTR(serial_number, S_IRUGO,
+			qcom_get_serial_number, NULL);
+
+static struct device_attribute qcom_soc_attr_chip_family =
+	__ATTR(chip_family, S_IRUGO,
+			qcom_get_chip_family, NULL);
+
+static struct device_attribute qcom_soc_attr_raw_device_family =
+	__ATTR(raw_device_family, S_IRUGO,
+			qcom_get_raw_device_family, NULL);
+
+static struct device_attribute qcom_soc_attr_raw_device_number =
+	__ATTR(raw_device_number, S_IRUGO,
+			qcom_get_raw_device_number, NULL);
+
+static struct device_attribute qcom_soc_attr_pmic_model =
+	__ATTR(pmic_model, S_IRUGO,
+			qcom_get_pmic_model, NULL);
+
+static struct device_attribute qcom_soc_attr_pmic_die_revision =
+	__ATTR(pmic_die_revision, S_IRUGO,
+			qcom_get_pmic_die_revision, NULL);
+
+static struct device_attribute image_version =
+	__ATTR(image_version, S_IRUGO | S_IWUSR,
+			qcom_get_image_version, qcom_set_image_version);
+
+static struct device_attribute image_variant =
+	__ATTR(image_variant, S_IRUGO | S_IWUSR,
+			qcom_get_image_variant, qcom_set_image_variant);
+
+static struct device_attribute image_crm_version =
+	__ATTR(image_crm_version, S_IRUGO | S_IWUSR,
+			qcom_get_image_crm_version, qcom_set_image_crm_version);
+
+static struct device_attribute select_image =

drivers/soc/qcom/socinfo.c +51 -0

+	device_create_file(qcom_soc_device, &image_version);
+	device_create_file(qcom_soc_device, &image_variant);
+	device_create_file(qcom_soc_device, &image_crm_version);
+	device_create_file(qcom_soc_device, &select_image);
+	device_create_file(qcom_soc_device, &images);
+
+	switch (socinfo_format) {
+	case SOCINFO_VERSION(0, 12):
+		device_create_file(qcom_soc_device,
+					&qcom_soc_attr_chip_family);
+		device_create_file(qcom_soc_device,
+					&qcom_soc_attr_raw_device_family);
+		device_create_file(qcom_soc_device,
+					&qcom_soc_attr_raw_device_number);
+	case SOCINFO_VERSION(0, 11):
+	case SOCINFO_VERSION(0, 10):
+		 device_create_file(qcom_soc_device,
+					&qcom_soc_attr_serial_number);
+	case SOCINFO_VERSION(0, 9):
+		 device_create_file(qcom_soc_device,
+					&qcom_soc_attr_foundry_id);
+	case SOCINFO_VERSION(0, 8):
+	case SOCINFO_VERSION(0, 7):
+		device_create_file(qcom_soc_device,
+					&qcom_soc_attr_pmic_model);
+		device_create_file(qcom_soc_device,
+					&qcom_soc_attr_pmic_die_revision);
+	case SOCINFO_VERSION(0, 6):
+		device_create_file(qcom_soc_device,
+					&qcom_soc_attr_platform_subtype);
+		device_create_file(qcom_soc_device,
+					&qcom_soc_attr_platform_subtype_id);
+	case SOCINFO_VERSION(0, 5):
+		device_create_file(qcom_soc_device,
+					&qcom_soc_attr_accessory_chip);
+	case SOCINFO_VERSION(0, 4):
+		device_create_file(qcom_soc_device,
+					&qcom_soc_attr_platform_version);
+	case SOCINFO_VERSION(0, 3):
+		device_create_file(qcom_soc_device,
+					&qcom_soc_attr_hw_platform);
+	case SOCINFO_VERSION(0, 2):
+		device_create_file(qcom_soc_device,
+					&qcom_soc_attr_raw_version);
+	case SOCINFO_VERSION(0, 1):
+		device_create_file(qcom_soc_device,
+					&qcom_soc_attr_build_id);
+		break;
+	default:
+		pr_err("Unknown socinfo format: v%u.%u\n",
+				SOCINFO_VERSION_MAJOR(socinfo_format),

drivers/soc/qcom/socinfo.c +19 -0

+		return PTR_ERR(soc_dev);
+	}
+
+	qcom_soc_device = soc_device_to_device(soc_dev);
+	socinfo_populate_sysfs_files(qcom_soc_device);
+	return 0;
+}
+#else
+static int socinfo_init_sysfs(void)
+{
+	return 0;
+}
+#endif
+
+static u32 socinfo_get_id(void)
+{
+	return (socinfo) ? socinfo->v0_1.id : 0;
+}
+

drivers/soc/qcom/socinfo.c +138 -0

+	u32 f_maj = SOCINFO_VERSION_MAJOR(socinfo_format);
+	u32 f_min = SOCINFO_VERSION_MINOR(socinfo_format);
+	u32 v_maj = SOCINFO_VERSION_MAJOR(socinfo->v0_1.version);
+	u32 v_min = SOCINFO_VERSION_MINOR(socinfo->v0_1.version);
+
+	switch (socinfo_format) {
+	case SOCINFO_VERSION(0, 1):
+		pr_info("v%u.%u, id=%u, ver=%u.%u\n",
+			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min);
+		break;
+	case SOCINFO_VERSION(0, 2):
+		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u\n",
+			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
+			socinfo->v0_2.raw_version);
+		break;
+	case SOCINFO_VERSION(0, 3):
+		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u\n",
+			f_maj, f_min, socinfo->v0_1.id,
+			v_maj, v_min, socinfo->v0_2.raw_version,
+			socinfo->v0_3.hw_platform);
+		break;
+	case SOCINFO_VERSION(0, 4):
+		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u\n",
+			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
+			socinfo->v0_2.raw_version,
+			socinfo->v0_3.hw_platform,
+			socinfo->v0_4.platform_version);
+		break;
+	case SOCINFO_VERSION(0, 5):
+		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u accessory_chip=%u\n",
+			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
+			socinfo->v0_2.raw_version,
+			socinfo->v0_3.hw_platform,
+			socinfo->v0_4.platform_version,
+			socinfo->v0_5.accessory_chip);
+		break;
+	case SOCINFO_VERSION(0, 6):
+		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u accessory_chip=%u, hw_plat_subtype=%u\n",
+			f_maj, f_min, socinfo->v0_1.id,
+			v_maj, v_min, socinfo->v0_2.raw_version,
+			socinfo->v0_3.hw_platform,
+			socinfo->v0_4.platform_version,
+			socinfo->v0_5.accessory_chip,
+			socinfo->v0_6.hw_platform_subtype);
+		break;
+	case SOCINFO_VERSION(0, 7):
+	case SOCINFO_VERSION(0, 8):
+		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u accessory_chip=%u, hw_plat_subtype=%u, pmic_model=%u, pmic_die_revision=%u\n",
+			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
+			socinfo->v0_2.raw_version,
+			socinfo->v0_3.hw_platform,
+			socinfo->v0_4.platform_version,
+			socinfo->v0_5.accessory_chip,
+			socinfo->v0_6.hw_platform_subtype,
+			socinfo->v0_7.pmic_model,
+			socinfo->v0_7.pmic_die_revision);
+		break;
+	case SOCINFO_VERSION(0, 9):
+		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u accessory_chip=%u, hw_plat_subtype=%u, pmic_model=%u, pmic_die_revision=%u foundry_id=%u\n",
+			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
+			socinfo->v0_2.raw_version,
+			socinfo->v0_3.hw_platform,
+			socinfo->v0_4.platform_version,
+			socinfo->v0_5.accessory_chip,
+			socinfo->v0_6.hw_platform_subtype,
+			socinfo->v0_7.pmic_model,
+			socinfo->v0_7.pmic_die_revision,
+			socinfo->v0_9.foundry_id);
+		break;
+	case SOCINFO_VERSION(0, 10):
+		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u accessory_chip=%u, hw_plat_subtype=%u, pmic_model=%u, pmic_die_revision=%u, foundry_id=%u, serial_number=%u\n",
+			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
+			socinfo->v0_2.raw_version,
+			socinfo->v0_3.hw_platform,
+			socinfo->v0_4.platform_version,
+			socinfo->v0_5.accessory_chip,
+			socinfo->v0_6.hw_platform_subtype,
+			socinfo->v0_7.pmic_model,
+			socinfo->v0_7.pmic_die_revision,
+			socinfo->v0_9.foundry_id,
+			socinfo->v0_10.serial_number);
+		break;
+	case SOCINFO_VERSION(0, 11):
+		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u, accessory_chip=%u, hw_plat_subtype=%u, pmic_model=%u, pmic_die_revision=%u, foundry_id=%u, serial_number=%u num_pmics=%u\n",
+			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
+			socinfo->v0_2.raw_version,
+			socinfo->v0_3.hw_platform,
+			socinfo->v0_4.platform_version,
+			socinfo->v0_5.accessory_chip,
+			socinfo->v0_6.hw_platform_subtype,
+			socinfo->v0_7.pmic_model,
+			socinfo->v0_7.pmic_die_revision,
+			socinfo->v0_9.foundry_id,
+			socinfo->v0_10.serial_number,
+			socinfo->v0_11.num_pmics);
+		break;
+	case SOCINFO_VERSION(0, 12):
+		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u accessory_chip=%u, hw_plat_subtype=%u, pmic_model=%u, pmic_die_revision=%u, foundry_id=%u, serial_number=%u, num_pmics=%u, chip_family=0x%x, raw_device_family=0x%x, raw_device_number=0x%x\n",
+			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
+			socinfo->v0_2.raw_version,
+			socinfo->v0_3.hw_platform,
+			socinfo->v0_4.platform_version,
+			socinfo->v0_5.accessory_chip,
+			socinfo->v0_6.hw_platform_subtype,
+			socinfo->v0_7.pmic_model,
+			socinfo->v0_7.pmic_die_revision,
+			socinfo->v0_9.foundry_id,
+			socinfo->v0_10.serial_number,
+			socinfo->v0_11.num_pmics,
+			socinfo->v0_12.chip_family,
+			socinfo->v0_12.raw_device_family,
+			socinfo->v0_12.raw_device_number);
+		break;
+
+	default:
+		pr_err("Unknown format found: v%u.%u\n", f_maj, f_min);
+		break;
+	}
+}
+
+static void socinfo_select_format(void)
+{
+	u32 f_maj = SOCINFO_VERSION_MAJOR(socinfo->v0_1.format);
+	u32 f_min = SOCINFO_VERSION_MINOR(socinfo->v0_1.format);
+
+	if (f_maj != 0) {
+		pr_err("Unsupported format v%u.%u. Falling back to dummy values.\n",
+			f_maj, f_min);
+		socinfo = setup_dummy_socinfo();
+	}
+
+	if (socinfo->v0_1.format > MAX_SOCINFO_FORMAT) {
+		pr_warn("Unsupported format v%u.%u. Falling back to v%u.%u.\n",
+			f_maj, f_min, SOCINFO_VERSION_MAJOR(MAX_SOCINFO_FORMAT),
+			SOCINFO_VERSION_MINOR(MAX_SOCINFO_FORMAT));
+		socinfo_format = MAX_SOCINFO_FORMAT;
+	} else {
+		socinfo_format = socinfo->v0_1.format;

drivers/soc/qcom/socinfo.c +15 -0

+
+	WARN(socinfo_get_id() >= ARRAY_SIZE(cpu_of_id),
+		"New IDs added! ID => CPU mapping needs an update.\n");
+
+	socinfo_print();
+
+	socinfo_init_sysfs();
+
+	/* Feed the soc specific unique data into entropy pool */
+	add_device_randomness(info, size);
+
+	return 0;
+}
+EXPORT_SYMBOL(qcom_socinfo_init);
+

include/linux/soc/qcom/smem.h +0 -0

index 785e196..62e9476 100644

Quoted 6 lines

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

[..]

drivers/soc/qcom/Makefile obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o · line 5

Quoted 10 lines

drivers/soc/qcom/Makefile +1 -1

index fdd664e..b4fb8f8 100644
@@ -2,7 +2,7 @@ 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) +=	smem.o socinfo.o

This should be something like:

obj-$(CONFIG_QCOM_SMEM) += qcom_smem.o
qcom_smem-y = smem.o
qcom_smem-y = socinfo.o

Otherwise this is treated as two different modules.

Quoted 8 lines

drivers/soc/qcom/smem.c +0 -0

index 18ec52f..05ff935 100644
@@ -85,6 +85,12 @@
 /* Max number of processors/hosts in a system */
 #define SMEM_HOST_COUNT		9
 

drivers/soc/qcom/smem.c line 92

Quoted 5 lines

drivers/soc/qcom/smem.c +5 -0

+/*
+ * Shared memory identifiers, used to acquire handles to respective memory
+ * region.
+ */
+#define SMEM_HW_SW_BUILD_ID		137

Move to socinfo.c

drivers/soc/qcom/smem.c static int qcom_smem_probe(struct platform_device *pdev) · line 769

Quoted 13 lines

drivers/soc/qcom/smem.c +9 -0

@@ -751,6 +758,15 @@ static int qcom_smem_probe(struct platform_device *pdev)
 
 	__smem = smem;
 
+	socinfo = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
+			&size);
+	if (IS_ERR(socinfo)) {
+		dev_warn(&pdev->dev,
+			"Can't find SMEM_HW_SW_BUILD_ID; falling back on dummy values.\n");
+		socinfo = setup_dummy_socinfo();
+	}
+	qcom_socinfo_init(socinfo, size);
+

Please just replace this with an unconditional qcom_socinfo_init() call and make it acquire the smem item and initialize itself.

And rather than setting up a dummy socinfo I think you should either not register the soc_device or make it up of the information we know. I see no point in exposing the value "Unknown" in various forms - better to just hide the attributes.

Quoted 4 lines

drivers/soc/qcom/smem.c +0 -0

 	return 0;
 }
 

drivers/soc/qcom/socinfo.c +0 -0

[..]

Quoted 2 lines

+/*
+ * SOC Info Routines

This comment doesn't add any value.

drivers/soc/qcom/socinfo.c

Quoted 4 lines

+ *
+ */

drivers/soc/qcom/socinfo.c +2 -0

+
+#define pr_fmt(fmt) "%s: " fmt, __func__

Now that this lives inside the smem driver you can pass the smem device
* and replace all pr_ with dev_ equivalents.

drivers/soc/qcom/socinfo.c line 42

Quoted 7 lines

+ * 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
+ */

drivers/soc/qcom/socinfo.c +2 -0

+#define SOCINFO_VERSION_MAJOR(ver) (((ver) & 0xffff0000) >> 16)
+#define SOCINFO_VERSION_MINOR(ver) ((ver) & 0x0000ffff)

These actually represents the socinfo->version, so this is fine.

Quoted 1 line

+#define SOCINFO_VERSION(maj, min) ((((maj) & 0xffff) << 16)|((min) & 0xffff))

But this is socinfo->format, which always has major == 0. So there's no need for this complexity until you actually bump the major. Just use the (minor) "format" directly and drop this.

drivers/soc/qcom/socinfo.c

Quoted 2 lines

drivers/soc/qcom/socinfo.c +2 -0

+
+#define PLATFORM_SUBTYPE_MDM	1

Unused.

Quoted 1 line

+#define PLATFORM_SUBTYPE_INTERPOSERV3 2

Unused.

Quoted 1 line

+#define PLATFORM_SUBTYPE_SGLTE 6

Unused.

drivers/soc/qcom/socinfo.c line 52

Quoted 5 lines

drivers/soc/qcom/socinfo.c +5 -0

+
+#define SMEM_SOCINFO_BUILD_ID_LENGTH 32
+#define SMEM_IMAGE_VERSION_BLOCKS_COUNT 32
+#define SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE 128
+#define SMEM_IMAGE_VERSION_SIZE 4096

You only need two of the count, block size and total size...

drivers/soc/qcom/socinfo.c line 57

Quoted 5 lines

drivers/soc/qcom/socinfo.c +5 -0

+#define SMEM_IMAGE_VERSION_NAME_SIZE 75
+#define SMEM_IMAGE_VERSION_VARIANT_SIZE 20
+#define SMEM_IMAGE_VERSION_VARIANT_OFFSET 75
+#define SMEM_IMAGE_VERSION_OEM_SIZE 32
+#define SMEM_IMAGE_VERSION_OEM_OFFSET 96

Please throw these into a:

struct image_version {
	char name[75];
	char variant[20];
	char pad;
	char oem[32];
};

This allows you to describe the version SMEM item just as an array of SMEM_IMAGE_VERSION_BLOCKS_COUNT image_version objects - i.e. the compiler will take care of doing the math for you.

drivers/soc/qcom/socinfo.c line 59

Quoted 2 lines

drivers/soc/qcom/socinfo.c +2 -0

+#define SMEM_IMAGE_VERSION_PARTITION_APPS 10
+#define SMEM_ITEM_SIZE_ALIGN 8

4096 is already aligned, so you don't need this.

Quoted 7 lines

drivers/soc/qcom/socinfo.c +7 -0

+	MSM_8274_AB_ID,
+	MSM_8274PRO_ID,
+	MSM_8674_AA_ID,
+	MSM_8674_AB_ID,
+	MSM_8674PRO_ID,
+	MSM_8974_AA_ID,
+	MSM_8974_AB_ID,

drivers/soc/qcom/socinfo.c line 126

Quoted 10 lines

drivers/soc/qcom/socinfo.c +10 -0

+	MSM_8996_ID = 246,
+	APQ_8016_ID,
+	MSM_8216_ID,
+	MSM_8116_ID,
+	MSM_8616_ID,
+	APQ8096_ID = 291,
+	MSM_8996SG_ID = 305,
+	MSM_8996AU_ID = 310,
+	APQ_8096AU_ID,
+	APQ_8096SG_ID

These values are only ever used one time, so I think it would be more readable if you just insert their numeric values into the cpu_of_id directly.

Compare

	[APQ_8096SG_ID] = {MSM_CPU_8996, "APQ8096SG"},

with:

	[312] = { MSM_CPU_8996, "APQ8096SG" },

They mean the same to the compiler, but saves us all from having to add and follow the extra indirection when maintaining this.

drivers/soc/qcom/socinfo.c line 134

Quoted 8 lines

drivers/soc/qcom/socinfo.c +8 -0

+};
+
+struct qcom_soc_info {
+	enum qcom_cpu generic_soc_type;
+	char *soc_id_string;
+};
+
+enum qcom_pmic_model {

None of the values in this enum are used.

drivers/soc/qcom/socinfo.c line 169

Quoted 6 lines

drivers/soc/qcom/socinfo.c +6 -0

+	HW_PLATFORM_HRD	= 13,
+	HW_PLATFORM_DTV,
+	HW_PLATFORM_RCM	= 21,
+	HW_PLATFORM_STP = 23,
+	HW_PLATFORM_SBC,
+	HW_PLATFORM_INVALID

Just inline these values into hw_platform, makes it easier to read.

drivers/soc/qcom/socinfo.c line 174

Quoted 5 lines

drivers/soc/qcom/socinfo.c +5 -0

+};
+
+enum accessory_chip_type {
+	ACCESSORY_CHIP_UNKNOWN = 0,
+	ACCESSORY_CHIP_CHARM = 58

Unused

drivers/soc/qcom/socinfo.c line 177

Quoted 3 lines

drivers/soc/qcom/socinfo.c +3 -0

+};
+
+enum qrd_platform_subtype {

Please inline values into qrd_hw_platform_subtype.

drivers/soc/qcom/socinfo.c line 191

Quoted 6 lines

drivers/soc/qcom/socinfo.c +6 -0

+enum platform_subtype {
+	PLATFORM_SUBTYPE_UNKNOWN,
+	PLATFORM_SUBTYPE_CHARM,
+	PLATFORM_SUBTYPE_STRANGE,
+	PLATFORM_SUBTYPE_STRANGE_2A,
+	PLATFORM_SUBTYPE_INVALID

Please inline these into hw_platform_subtype.

drivers/soc/qcom/socinfo.c line 194

Quoted 3 lines

drivers/soc/qcom/socinfo.c +3 -0

+};
+
+#ifdef CONFIG_SOC_BUS

Just add "select SOC_BUS" to the SMEM Kconfig

Quoted 18 lines

drivers/soc/qcom/socinfo.c +18 -0

+static const char *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",
+};

drivers/soc/qcom/socinfo.c line 396

Quoted 7 lines

drivers/soc/qcom/socinfo.c +7 -0

+
+static u32 socinfo_format;
+
+static struct socinfo_v0_1 dummy_socinfo = {
+	.format = SOCINFO_VERSION(0, 1),
+	.version = 1,
+};

Drop the dummy, if you don't know the "build_id" then lets just not expose a build id of "Unknown".

drivers/soc/qcom/socinfo.c

Quoted 2 lines

drivers/soc/qcom/socinfo.c +2 -0

+
+#ifdef CONFIG_SOC_BUS

Select SOC_BUS

drivers/soc/qcom/socinfo.c line 412

Quoted 4 lines

drivers/soc/qcom/socinfo.c +4 -0

+static u32 socinfo_get_accessory_chip(void)
+{
+	return socinfo ?
+		(socinfo_format >= SOCINFO_VERSION(0, 5) ?

The numerous indirections used makes it non-obvious, but the associated attribute will only be created if you have a socinfo and it's version >= 0.5.

So with some manual constant propagation this function does nothing more than and as such serves no purpose being broken out from qcom_get_accessory_chip() - just inline it.

	return socinfo->v0_5.accessory_chip;

drivers/soc/qcom/socinfo.c line 421

Quoted 9 lines

drivers/soc/qcom/socinfo.c +9 -0

+			socinfo->v0_5.accessory_chip : 0)
+		: 0;
+}
+
+static u32 socinfo_get_foundry_id(void)
+{
+	return socinfo ?
+		(socinfo_format >= SOCINFO_VERSION(0, 9) ?
+			socinfo->v0_9.foundry_id : 0)

Dito

drivers/soc/qcom/socinfo.c line 430

Quoted 9 lines

drivers/soc/qcom/socinfo.c +9 -0

+		: 0;
+}
+
+static u32 socinfo_get_chip_family(void)
+{
+	return socinfo ?
+		(socinfo_format >= SOCINFO_VERSION(0, 12) ?
+			socinfo->v0_12.chip_family : 0)
+		: 0;

Dito

drivers/soc/qcom/socinfo.c line 438

Quoted 8 lines

drivers/soc/qcom/socinfo.c +8 -0

+}
+
+static u32 socinfo_get_raw_device_family(void)
+{
+	return socinfo ?
+		(socinfo_format >= SOCINFO_VERSION(0, 12) ?
+			socinfo->v0_12.raw_device_family : 0)
+		: 0;

Dito

drivers/soc/qcom/socinfo.c line 446

Quoted 8 lines

drivers/soc/qcom/socinfo.c +8 -0

+}
+
+static u32 socinfo_get_raw_device_number(void)
+{
+	return socinfo ?
+		(socinfo_format >= SOCINFO_VERSION(0, 12) ?
+			socinfo->v0_12.raw_device_number : 0)
+		: 0;

Dito

drivers/soc/qcom/socinfo.c line 459

Quoted 13 lines

drivers/soc/qcom/socinfo.c +13 -0

+}
+
+static char *socinfo_get_image_version_base_address(struct device *dev)
+{
+	size_t size, size_in;
+	void *ptr;
+
+	ptr = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_IMAGE_VERSION_TABLE,
+			&size);
+	if (IS_ERR(ptr))
+		return ptr;
+
+	size_in = ALIGN(SMEM_IMAGE_VERSION_SIZE, SMEM_ITEM_SIZE_ALIGN);

4096 is an even power of 8, you don't need to align it.

drivers/soc/qcom/socinfo.c line 460

Quoted 1 line

drivers/soc/qcom/socinfo.c +1 -0

+	if (size_in != size) {

Quoted 8 lines

drivers/soc/qcom/socinfo.c +8 -0

+		dev_err(dev, "Wrong size for smem item\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	return ptr;
+}
+
+

Quoted 4 lines

drivers/soc/qcom/socinfo.c +4 -0

+
+
+static ssize_t
+qcom_get_vendor(struct device *dev,

drivers/soc/qcom/socinfo.c line 541

Quoted 4 lines

drivers/soc/qcom/socinfo.c +4 -0

+		struct device_attribute *attr,
+		char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "Qualcomm\n");

Per Documentation/filesystem/sysfs.txt, never use snprintf. Use scnprintf() if you're unsure about the size of your data and sprintf() otherwise.

Quoted 5 lines

drivers/soc/qcom/socinfo.c +5 -0

+}
+
+static ssize_t
+qcom_get_raw_version(struct device *dev,
+		     struct device_attribute *attr,

drivers/soc/qcom/socinfo.c line 550

Quoted 4 lines

drivers/soc/qcom/socinfo.c +4 -0

+		     char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%u\n",
+		socinfo_get_raw_version());

There's no need to line wrap these and that %u won't ever exceed PAGE_SIZE, so feel free to use sprintf() if you feel the 80 char limit is too close.

Quoted 11 lines

drivers/soc/qcom/socinfo.c +11 -0

+}
+
+static ssize_t
+qcom_get_build_id(struct device *dev,
+		   struct device_attribute *attr,
+		   char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%-.32s\n",
+			socinfo_get_build_id());
+}
+

Quoted 7 lines

drivers/soc/qcom/socinfo.c +7 -0

+static ssize_t
+qcom_get_platform_subtype(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	u32 hw_subtype;
+

drivers/soc/qcom/socinfo.c line 603

Quoted 4 lines

drivers/soc/qcom/socinfo.c +4 -0

+	hw_subtype = socinfo_get_platform_subtype();
+	if (socinfo_get_platform_type() == HW_PLATFORM_QRD) {
+		if (hw_subtype >= PLATFORM_SUBTYPE_QRD_INVALID) {
+			pr_err("Invalid hardware platform sub type for qrd found\n");

This print doesn't help my user space application much, figure this condition out during initialization and skip creating platform_subtype if you don't know.

Quoted 19 lines

drivers/soc/qcom/socinfo.c +19 -0

+			hw_subtype = PLATFORM_SUBTYPE_QRD_INVALID;
+		}
+		return snprintf(buf, PAGE_SIZE, "%-.32s\n",
+					qrd_hw_platform_subtype[hw_subtype]);
+	} else {
+		if (hw_subtype >= PLATFORM_SUBTYPE_INVALID) {
+			pr_err("Invalid hardware platform subtype\n");
+			hw_subtype = PLATFORM_SUBTYPE_INVALID;
+		}
+		return snprintf(buf, PAGE_SIZE, "%-.32s\n",
+			hw_platform_subtype[hw_subtype]);
+	}
+}
+
+static ssize_t
+qcom_get_platform_subtype_id(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{

drivers/soc/qcom/socinfo.c line 627

Quoted 5 lines

drivers/soc/qcom/socinfo.c +5 -0

+	u32 hw_subtype;
+
+	hw_subtype = socinfo_get_platform_subtype();
+	return snprintf(buf, PAGE_SIZE, "%u\n",
+		hw_subtype);

Drop the local variable and just put socinfo->v0_6.hw_platform_subtype here.

Quoted 11 lines

drivers/soc/qcom/socinfo.c +11 -0

+}
+
+static ssize_t
+qcom_get_foundry_id(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%u\n",
+		socinfo_get_foundry_id());
+}
+

drivers/soc/qcom/socinfo.c line 679

Quoted 6 lines

drivers/soc/qcom/socinfo.c +6 -0

+
+static ssize_t
+qcom_get_pmic_model(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{

I would prefer if this printed the human readable name of the PMIC rather than a number that I can look up in the source code...

Quoted 16 lines

drivers/soc/qcom/socinfo.c +16 -0

+	return snprintf(buf, PAGE_SIZE, "%u\n",
+		socinfo_get_pmic_model());
+}
+
+static ssize_t
+qcom_get_pmic_die_revision(struct device *dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%u\n",
+			 socinfo_get_pmic_die_revision());
+}
+
+static ssize_t
+qcom_get_image_version(struct device *dev,
+			struct device_attribute *attr,

drivers/soc/qcom/socinfo.c line 700

Quoted 5 lines

drivers/soc/qcom/socinfo.c +5 -0

+			char *buf)
+{
+	char *string_address;
+
+	string_address = socinfo_get_image_version_base_address(dev);

Make this call once during initialization and don't expose the version attributes if you can't find the item.

Quoted 15 lines

drivers/soc/qcom/socinfo.c +15 -0

+	if (IS_ERR(string_address)) {
+		pr_err("Failed to get image version base address");
+		return snprintf(buf, SMEM_IMAGE_VERSION_NAME_SIZE, "Unknown");
+	}
+	string_address += current_image * SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
+	return snprintf(buf, SMEM_IMAGE_VERSION_NAME_SIZE, "%-.75s\n",
+			string_address);
+}
+
+static ssize_t
+qcom_set_image_version(struct device *dev,
+			struct device_attribute *attr,
+			const char *buf,
+			size_t count)
+{

drivers/soc/qcom/socinfo.c line 719

Quoted 4 lines

drivers/soc/qcom/socinfo.c +4 -0

+	char *store_address;
+
+	if (current_image != SMEM_IMAGE_VERSION_PARTITION_APPS)
+		return count;

If you just make this 0644 for apps and 0444 then this won't happen, but in cases like this you shouldn't just lie to user space about the success.

drivers/soc/qcom/socinfo.c line 723

Quoted 4 lines

drivers/soc/qcom/socinfo.c +4 -0

+	store_address = socinfo_get_image_version_base_address(dev);
+	if (IS_ERR(store_address)) {
+		pr_err("Failed to get image version base address");
+		return count;

Dito

drivers/soc/qcom/socinfo.c line 726

Quoted 3 lines

drivers/soc/qcom/socinfo.c +3 -0

+	}
+	store_address += current_image * SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
+	snprintf(store_address, SMEM_IMAGE_VERSION_NAME_SIZE, "%-.75s", buf);

I don't see the point in using a string formatter here, or why to limit the output to 75 chars by the format string - and the string is limited to 74 chars and then a \0.

Just replace it with:

	strlcpy(store_address, buf, SMEM_IMAGE_VERSION_NAME_SIZE);

Quoted 10 lines

drivers/soc/qcom/socinfo.c +10 -0

+	return count;
+}
+
+static ssize_t
+qcom_get_image_variant(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	char *string_address;
+

drivers/soc/qcom/socinfo.c line 746

Quoted 10 lines

drivers/soc/qcom/socinfo.c +10 -0

+	string_address = socinfo_get_image_version_base_address(dev);
+	if (IS_ERR(string_address)) {
+		pr_err("Failed to get image version base address");
+		return snprintf(buf, SMEM_IMAGE_VERSION_VARIANT_SIZE,
+		"Unknown");
+	}
+	string_address += current_image * SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
+	string_address += SMEM_IMAGE_VERSION_VARIANT_OFFSET;
+	return snprintf(buf, SMEM_IMAGE_VERSION_VARIANT_SIZE, "%-.20s\n",
+			string_address);

The output of this is the sysfs buffer, so PAGE_SIZE is the size you're looking for. Is this string not terminated or why the limiting format string?

And use scnprintf()

Quoted 23 lines

drivers/soc/qcom/socinfo.c +23 -0

+}
+
+static ssize_t
+qcom_set_image_variant(struct device *dev,
+			struct device_attribute *attr,
+			const char *buf,
+			size_t count)
+{
+	char *store_address;
+
+	if (current_image != SMEM_IMAGE_VERSION_PARTITION_APPS)
+		return count;
+	store_address = socinfo_get_image_version_base_address(dev);
+	if (IS_ERR(store_address)) {
+		pr_err("Failed to get image version base address");
+		return count;
+	}
+	store_address += current_image * SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
+	store_address += SMEM_IMAGE_VERSION_VARIANT_OFFSET;
+	snprintf(store_address, SMEM_IMAGE_VERSION_VARIANT_SIZE, "%-.20s", buf);
+	return count;
+}
+

drivers/soc/qcom/socinfo.c line 815

Quoted 8 lines

drivers/soc/qcom/socinfo.c +8 -0

+
+static ssize_t
+qcom_get_image_number(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+			current_image);

This is not a property of your soc...

Quoted 6 lines

drivers/soc/qcom/socinfo.c +6 -0

+}
+
+static ssize_t
+qcom_select_image(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{

drivers/soc/qcom/socinfo.c line 831

Quoted 10 lines

drivers/soc/qcom/socinfo.c +10 -0

+	int ret, digit;
+
+	ret = kstrtoint(buf, 10, &digit);
+	if (ret)
+		return ret;
+	if (digit >= 0 && digit < SMEM_IMAGE_VERSION_BLOCKS_COUNT)
+		current_image = digit;
+	else
+		current_image = 0;
+	return count;

...it's part of your version paging mechanism, which is not ok to put in sysfs.

Create individual attribute files for each version entry, make the ones representing apps read/write and the others read only.

drivers/soc/qcom/socinfo.c line 837

Quoted 6 lines

drivers/soc/qcom/socinfo.c +6 -0

+}
+
+static ssize_t
+qcom_get_images(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{

Isn't this just a combined version of the above versions? sysfs attributes should contain one entry each, no tables.

Quoted 28 lines

drivers/soc/qcom/socinfo.c +28 -0

+	int pos = 0;
+	int image;
+	char *image_address;
+
+	image_address = socinfo_get_image_version_base_address(dev);
+	if (IS_ERR(image_address))
+		return snprintf(buf, PAGE_SIZE, "Unavailable\n");
+
+	*buf = '\0';
+	for (image = 0; image < SMEM_IMAGE_VERSION_BLOCKS_COUNT; image++) {
+		if (*image_address == '\0') {
+			image_address += SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
+			continue;
+		}
+
+		pos += snprintf(buf + pos, PAGE_SIZE - pos, "%d:\n",
+				image);
+		pos += snprintf(buf + pos, PAGE_SIZE - pos,
+				"\tCRM:\t\t%-.75s\n", image_address);
+		pos += snprintf(buf + pos, PAGE_SIZE - pos,
+				"\tVariant:\t%-.20s\n", image_address +
+				SMEM_IMAGE_VERSION_VARIANT_OFFSET);
+		pos += snprintf(buf + pos, PAGE_SIZE - pos,
+				"\tVersion:\t%-.32s\n\n",
+				image_address + SMEM_IMAGE_VERSION_OEM_OFFSET);
+
+		image_address += SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
+	}

drivers/soc/qcom/socinfo.c line 871

Quoted 6 lines

drivers/soc/qcom/socinfo.c +6 -0

+
+	return pos;
+}
+
+static struct device_attribute qcom_soc_attr_raw_version =
+	__ATTR(raw_version, S_IRUGO, qcom_get_raw_version,  NULL);

As checkpatch tell you, replace S_IRUGO with 0444

Quoted 7 lines

drivers/soc/qcom/socinfo.c +7 -0

+
+static struct device_attribute qcom_soc_attr_vendor =
+	__ATTR(vendor, S_IRUGO, qcom_get_vendor,  NULL);
+
+static struct device_attribute qcom_soc_attr_build_id =
+	__ATTR(build_id, S_IRUGO, qcom_get_build_id, NULL);
+

Quoted 6 lines

drivers/soc/qcom/socinfo.c +6 -0

+static struct device_attribute select_image =
+	__ATTR(select_image, S_IRUGO | S_IWUSR,
+			qcom_get_image_number, qcom_select_image);
+
+static struct device_attribute images =
+	__ATTR(images, S_IRUGO, qcom_get_images, NULL);

drivers/soc/qcom/socinfo.c line 949

Quoted 5 lines

drivers/soc/qcom/socinfo.c +5 -0

+
+
+static void socinfo_populate_sysfs_files(struct device *qcom_soc_device)
+{
+	device_create_file(qcom_soc_device, &qcom_soc_attr_vendor);

If you're keeping vendor it should be added to the generic soc_device_attribute struct.

Quoted 10 lines

drivers/soc/qcom/socinfo.c +10 -0

+	device_create_file(qcom_soc_device, &image_version);
+	device_create_file(qcom_soc_device, &image_variant);
+	device_create_file(qcom_soc_device, &image_crm_version);
+	device_create_file(qcom_soc_device, &select_image);
+	device_create_file(qcom_soc_device, &images);
+
+	switch (socinfo_format) {
+	case SOCINFO_VERSION(0, 12):
+		device_create_file(qcom_soc_device,
+					&qcom_soc_attr_chip_family);

Quoted 15 lines

drivers/soc/qcom/socinfo.c +15 -0

+	case SOCINFO_VERSION(0, 2):
+		device_create_file(qcom_soc_device,
+					&qcom_soc_attr_raw_version);
+	case SOCINFO_VERSION(0, 1):
+		device_create_file(qcom_soc_device,
+					&qcom_soc_attr_build_id);
+		break;
+	default:
+		pr_err("Unknown socinfo format: v%u.%u\n",
+				SOCINFO_VERSION_MAJOR(socinfo_format),
+				SOCINFO_VERSION_MINOR(socinfo_format));
+		break;
+	}
+}
+

drivers/soc/qcom/socinfo.c line 1010

Quoted 5 lines

drivers/soc/qcom/socinfo.c +5 -0

+static void socinfo_populate(struct soc_device_attribute *soc_dev_attr)
+{
+	u32 soc_version = socinfo_get_version();
+
+	soc_dev_attr->soc_id   = kasprintf(GFP_KERNEL, "%d", socinfo_get_id());

I believe soc_id is supposed to be a human readable name; e.g. "MSM8996" not "246".

drivers/soc/qcom/socinfo.c line 1012

Quoted 2 lines

drivers/soc/qcom/socinfo.c +2 -0

+	soc_dev_attr->family  =  "Snapdragon";
+	soc_dev_attr->machine  = socinfo_get_id_string();

I think you're supposed to read the /model string from DT and put in "machine".

drivers/soc/qcom/socinfo.c line 1015

Quoted 3 lines

drivers/soc/qcom/socinfo.c +3 -0

+	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%u.%u",
+			SOCINFO_VERSION_MAJOR(soc_version),
+			SOCINFO_VERSION_MINOR(soc_version));

"revision" is supposed to state the revision of the SoC, not of the socinfo data.

Also, skip the indentation of the assignments in this section (it's not consistent anyways).

drivers/soc/qcom/socinfo.c line 1016

Quoted 1 line

drivers/soc/qcom/socinfo.c +1 -0

+	return;

No need to "return" here.

Quoted 6 lines

drivers/soc/qcom/socinfo.c +6 -0

+
+}
+
+static int socinfo_init_sysfs(void)
+{
+	struct device *qcom_soc_device;

drivers/soc/qcom/socinfo.c line 1028

Quoted 6 lines

drivers/soc/qcom/socinfo.c +6 -0

+	struct soc_device *soc_dev;
+	struct soc_device_attribute *soc_dev_attr;
+
+	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
+	if (!soc_dev_attr) {
+		pr_err("Soc Device alloc failed!\n");

No need to print error message on kzalloc failures.

drivers/soc/qcom/socinfo.c line 1032

Quoted 4 lines

drivers/soc/qcom/socinfo.c +4 -0

+		return -ENOMEM;
+	}
+
+	socinfo_populate(soc_dev_attr);

Just inline socinfo_populate() here.

drivers/soc/qcom/socinfo.c line 1036

Quoted 4 lines

drivers/soc/qcom/socinfo.c +4 -0

+	soc_dev = soc_device_register(soc_dev_attr);
+	if (IS_ERR(soc_dev)) {
+		kfree(soc_dev_attr);
+		pr_err("Soc device register failed\n");

I believe soc_device_register() will print something useful in the various error paths, so you shouldn't need to add another print here.

Quoted 8 lines

drivers/soc/qcom/socinfo.c +8 -0

+		return PTR_ERR(soc_dev);
+	}
+
+	qcom_soc_device = soc_device_to_device(soc_dev);
+	socinfo_populate_sysfs_files(qcom_soc_device);
+	return 0;
+}
+#else

drivers/soc/qcom/socinfo.c line 1065

Quoted 2 lines

drivers/soc/qcom/socinfo.c +2 -0

+static void socinfo_print(void)
+{

Does this function serve a purpose?

We already have an overwhelming amount of information (noise) being thrown at us during boot.

Quoted 15 lines

drivers/soc/qcom/socinfo.c +15 -0

+	u32 f_maj = SOCINFO_VERSION_MAJOR(socinfo_format);
+	u32 f_min = SOCINFO_VERSION_MINOR(socinfo_format);
+	u32 v_maj = SOCINFO_VERSION_MAJOR(socinfo->v0_1.version);
+	u32 v_min = SOCINFO_VERSION_MINOR(socinfo->v0_1.version);
+
+	switch (socinfo_format) {
+	case SOCINFO_VERSION(0, 1):
+		pr_info("v%u.%u, id=%u, ver=%u.%u\n",
+			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min);
+		break;
+	case SOCINFO_VERSION(0, 2):
+		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u\n",
+			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
+			socinfo->v0_2.raw_version);
+		break;

Quoted 42 lines

drivers/soc/qcom/socinfo.c +42 -0

+	case SOCINFO_VERSION(0, 12):
+		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u accessory_chip=%u, hw_plat_subtype=%u, pmic_model=%u, pmic_die_revision=%u, foundry_id=%u, serial_number=%u, num_pmics=%u, chip_family=0x%x, raw_device_family=0x%x, raw_device_number=0x%x\n",
+			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
+			socinfo->v0_2.raw_version,
+			socinfo->v0_3.hw_platform,
+			socinfo->v0_4.platform_version,
+			socinfo->v0_5.accessory_chip,
+			socinfo->v0_6.hw_platform_subtype,
+			socinfo->v0_7.pmic_model,
+			socinfo->v0_7.pmic_die_revision,
+			socinfo->v0_9.foundry_id,
+			socinfo->v0_10.serial_number,
+			socinfo->v0_11.num_pmics,
+			socinfo->v0_12.chip_family,
+			socinfo->v0_12.raw_device_family,
+			socinfo->v0_12.raw_device_number);
+		break;
+
+	default:
+		pr_err("Unknown format found: v%u.%u\n", f_maj, f_min);
+		break;
+	}
+}
+
+static void socinfo_select_format(void)
+{
+	u32 f_maj = SOCINFO_VERSION_MAJOR(socinfo->v0_1.format);
+	u32 f_min = SOCINFO_VERSION_MINOR(socinfo->v0_1.format);
+
+	if (f_maj != 0) {
+		pr_err("Unsupported format v%u.%u. Falling back to dummy values.\n",
+			f_maj, f_min);
+		socinfo = setup_dummy_socinfo();
+	}
+
+	if (socinfo->v0_1.format > MAX_SOCINFO_FORMAT) {
+		pr_warn("Unsupported format v%u.%u. Falling back to v%u.%u.\n",
+			f_maj, f_min, SOCINFO_VERSION_MAJOR(MAX_SOCINFO_FORMAT),
+			SOCINFO_VERSION_MINOR(MAX_SOCINFO_FORMAT));
+		socinfo_format = MAX_SOCINFO_FORMAT;
+	} else {
+		socinfo_format = socinfo->v0_1.format;

drivers/soc/qcom/socinfo.c line 1213

Quoted 10 lines

drivers/soc/qcom/socinfo.c +10 -0

+	}
+}
+
+int qcom_socinfo_init(void *info, size_t size)
+{
+	socinfo = info;
+
+	socinfo_select_format();
+
+	WARN(!socinfo_get_id(), "Unknown SOC ID!\n");

No need to WARN() about this.

I bet this is not really ever happening outside early development at Qualcomm, so I would go with a print and just skip the entire soc_device initialization when this happens.

Quoted 20 lines

drivers/soc/qcom/socinfo.c +15 -0

+
+	WARN(socinfo_get_id() >= ARRAY_SIZE(cpu_of_id),
+		"New IDs added! ID => CPU mapping needs an update.\n");
+
+	socinfo_print();
+
+	socinfo_init_sysfs();
+
+	/* Feed the soc specific unique data into entropy pool */
+	add_device_randomness(info, size);
+
+	return 0;
+}
+EXPORT_SYMBOL(qcom_socinfo_init);
+

include/linux/soc/qcom/smem.h +0 -0

index 785e196..62e9476 100644

include/linux/soc/qcom/smem.h line 10

Quoted 5 lines

include/linux/soc/qcom/smem.h +1 -1

@@ -7,5 +7,6 @@
 void *qcom_smem_get(unsigned host, unsigned item, size_t *size);
 
 int qcom_smem_get_free_space(unsigned host);
-
+int qcom_socinfo_init(void *info, size_t size);

soc/qcom/smem.h is the public API for smem, this function is not part of that API so I don't like having it defined here.

Normally we would create an internal include file in drivers/soc/qcom, but as it's only a single function I would suggest just declaring it external inside smem.c

include/linux/soc/qcom/smem.h line 11

Quoted 1 line

include/linux/soc/qcom/smem.h +1 -0

+void *setup_dummy_socinfo(void);

And drop this.

include/linux/soc/qcom/smem.h line 12

Quoted 1 line

include/linux/soc/qcom/smem.h +0 -0

 #endif

Regards,
Bjorn

On 11/3/2016 12:00 AM, Bjorn Andersson wrote:
Thanks Bjorn for your review comments. I have taken into account most of these in the subsequent patch set. For a few of the comments I want to discuss further, so please see the same inline and let me know your feedback about the same.

Quoted 1 line · Imran Khan

Quoted 15 lines

drivers/soc/qcom/socinfo.c line 57

Quoted 5 lines

drivers/soc/qcom/socinfo.c new file +5 -0

+#define SMEM_IMAGE_VERSION_NAME_SIZE 75
+#define SMEM_IMAGE_VERSION_VARIANT_SIZE 20
+#define SMEM_IMAGE_VERSION_VARIANT_OFFSET 75
+#define SMEM_IMAGE_VERSION_OEM_SIZE 32
+#define SMEM_IMAGE_VERSION_OEM_OFFSET 96

Please throw these into a:

struct image_version {
	char name[75];
	char variant[20];
	char pad;
	char oem[32];
};

Quoted 4 lines

This allows you to describe the version SMEM item just as an array of SMEM_IMAGE_VERSION_BLOCKS_COUNT image_version objects - i.e. the compiler will take care of doing the math for you.

Okay done. Although I have kept the macros to describe length of various members of image_version object.

Quoted 7 lines

drivers/soc/qcom/socinfo.c line 949

Quoted 3 lines

drivers/soc/qcom/socinfo.c new file +3 -0

+static void socinfo_populate_sysfs_files(struct device *qcom_soc_device)
+{
+	device_create_file(qcom_soc_device, &qcom_soc_attr_vendor);

If you're keeping vendor it should be added to the generic soc_device_attribute struct.

Actually along with vendor, I also intend to keep serial number and foundry_id in the generic soc_device_attribute structure but as this part is still under discussion, so far I have kept these fields separate. Once these fileds gets accepted as part of generic soc_device_attribute, I can modify this driver. I hope this approach is okay, if you have some alternative suggestion please let me know.

Quoted 9 lines

drivers/soc/qcom/socinfo.c line 1010

Quoted 5 lines

drivers/soc/qcom/socinfo.c new file +5 -0

+static void socinfo_populate(struct soc_device_attribute *soc_dev_attr)
+{
+	u32 soc_version = socinfo_get_version();
+
+	soc_dev_attr->soc_id   = kasprintf(GFP_KERNEL, "%d", socinfo_get_id());

I believe soc_id is supposed to be a human readable name; e.g. "MSM8996" not "246".

I am not sure about this. I see other vendors also exposing soc_id as numeric value and machine is perhaps used for a human readable name. Please let me if I am getting something wrong here.

Quoted 1 line

drivers/soc/qcom/socinfo.c line 1011

Quoted 1 line

drivers/soc/qcom/socinfo.c new file +1 -0

+	soc_dev_attr->family  =  "Snapdragon";

Quoted 5 lines

drivers/soc/qcom/socinfo.c line 1012

Quoted 1 line

drivers/soc/qcom/socinfo.c new file +1 -0

+	soc_dev_attr->machine  = socinfo_get_id_string();

I think you're supposed to read the /model string from DT and put in "machine".

Yes. We can read the machine string from DT also, but here we want to use the relevant information latched into SMEM as munch as possible so I went for reading this from socinfo. Please let me know if you see see some problem with this approach.

Quoted 7 lines

drivers/soc/qcom/socinfo.c line 1015

Quoted 3 lines

drivers/soc/qcom/socinfo.c new file +3 -0

+	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%u.%u",
+			SOCINFO_VERSION_MAJOR(soc_version),
+			SOCINFO_VERSION_MINOR(soc_version));

"revision" is supposed to state the revision of the SoC, not of the socinfo data.

Yes. It is indeed stating the revision of SoC. I guess the name of the macro is misleading here. Have corrected that in subsequent patch.

Quoted 8 lines

Also, skip the indentation of the assignments in this section (it's not consistent anyways).

drivers/soc/qcom/socinfo.c line 1016

Quoted 1 line

drivers/soc/qcom/socinfo.c new file +1 -0

+	return;

No need to "return" here.

Okay done.

Quoted 6 lines

Quoted 6 lines

drivers/soc/qcom/socinfo.c new file +6 -0

+
+}
+
+static int socinfo_init_sysfs(void)
+{
+	struct device *qcom_soc_device;

Quoted 9 lines

drivers/soc/qcom/socinfo.c line 1028

Quoted 6 lines

drivers/soc/qcom/socinfo.c new file +6 -0

+	struct soc_device *soc_dev;
+	struct soc_device_attribute *soc_dev_attr;
+
+	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
+	if (!soc_dev_attr) {
+		pr_err("Soc Device alloc failed!\n");

No need to print error message on kzalloc failures.

Okay done.

Quoted 7 lines

drivers/soc/qcom/socinfo.c line 1032

Quoted 4 lines

drivers/soc/qcom/socinfo.c new file +4 -0

+		return -ENOMEM;
+	}
+
+	socinfo_populate(soc_dev_attr);

Just inline socinfo_populate() here.

Quoted 8 lines

drivers/soc/qcom/socinfo.c line 1036

Quoted 4 lines

drivers/soc/qcom/socinfo.c new file +4 -0

+	soc_dev = soc_device_register(soc_dev_attr);
+	if (IS_ERR(soc_dev)) {
+		kfree(soc_dev_attr);
+		pr_err("Soc device register failed\n");

I believe soc_device_register() will print something useful in the various error paths, so you shouldn't need to add another print here.

Okay done.

Quoted 8 lines

Quoted 8 lines

drivers/soc/qcom/socinfo.c new file +8 -0

+		return PTR_ERR(soc_dev);
+	}
+
+	qcom_soc_device = soc_device_to_device(soc_dev);
+	socinfo_populate_sysfs_files(qcom_soc_device);
+	return 0;
+}
+#else

<snip>

Quoted 8 lines

drivers/soc/qcom/socinfo.c line 1065

Quoted 2 lines

drivers/soc/qcom/socinfo.c new file +2 -0

+static void socinfo_print(void)
+{

Does this function serve a purpose?

We already have an overwhelming amount of information (noise) being thrown at us during boot.

As such this funtion does not serve any overly important purpose. It just prints socinfo related stuff in one line in boot logs which sometimes helps in identification of the platform while doing a debugging from dumps.
So I intend to keep it but if you see some issues please let me know, I can try to modify it.

Quoted 9 lines

drivers/soc/qcom/socinfo.c line 1074

Quoted 9 lines

drivers/soc/qcom/socinfo.c new file +9 -0

+	u32 f_maj = SOCINFO_VERSION_MAJOR(socinfo_format);
+	u32 f_min = SOCINFO_VERSION_MINOR(socinfo_format);
+	u32 v_maj = SOCINFO_VERSION_MAJOR(socinfo->v0_1.version);
+	u32 v_min = SOCINFO_VERSION_MINOR(socinfo->v0_1.version);
+
+	switch (socinfo_format) {
+	case SOCINFO_VERSION(0, 1):
+		pr_info("v%u.%u, id=%u, ver=%u.%u\n",
+			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min);

Thanks and Regards,

On Mon 07 Nov 06:35 PST 2016, Imran Khan wrote:

[..]

Quoted 10 lines

Quoted 9 lines

drivers/soc/qcom/socinfo.c line 1010

Quoted 5 lines

drivers/soc/qcom/socinfo.c new file +5 -0

+static void socinfo_populate(struct soc_device_attribute *soc_dev_attr)
+{
+	u32 soc_version = socinfo_get_version();
+
+	soc_dev_attr->soc_id   = kasprintf(GFP_KERNEL, "%d", socinfo_get_id());

I believe soc_id is supposed to be a human readable name; e.g. "MSM8996" not "246".

Quoted 4 lines

I am not sure about this. I see other vendors also exposing soc_id as numeric value and machine is perhaps used for a human readable name. Please let me if I am getting something wrong here.

I'm slightly confused to what these various properties are supposed to contain, according to Documentation/ABI/testing/sysfs-devices-soc soc_id should contain the SoC serial number, while most implementations does like you and put something telling which SoC it is.

246 is however not a useful number, as everyone reading it - be it human or computer - will have to carry the translation table to figure out what it actually says.

Quoted 1 line

Quoted 1 line

drivers/soc/qcom/socinfo.c line 1011

Quoted 1 line

drivers/soc/qcom/socinfo.c new file +1 -0

+	soc_dev_attr->family  =  "Snapdragon";

I think family should be e.g. "MSM8996" and then machine should be e.g. "MSM8996AU".

Quoted 7 lines

As such this funtion does not serve any overly important purpose. It just prints socinfo related stuff in one line in boot logs which sometimes helps in identification of the platform while doing a debugging from dumps.
So I intend to keep it but if you see some issues please let me know, I can try to modify it.

I presumed this was used for post mortem purposes. But how useful is this? Doesn't the dump file come with enough information to identify the platform you collected the dump on?

And when we enter download mode the SMEM items should be intact, so the raw information should be available already - verbatim.

The kernel log is for human consumption, first and foremost, so if you're printing anything it must make sense to humans.

Quoted 9 lines

Quoted 9 lines

drivers/soc/qcom/socinfo.c line 1074

Quoted 9 lines

drivers/soc/qcom/socinfo.c new file +9 -0

+	u32 f_maj = SOCINFO_VERSION_MAJOR(socinfo_format);
+	u32 f_min = SOCINFO_VERSION_MINOR(socinfo_format);
+	u32 v_maj = SOCINFO_VERSION_MAJOR(socinfo->v0_1.version);
+	u32 v_min = SOCINFO_VERSION_MINOR(socinfo->v0_1.version);
+
+	switch (socinfo_format) {
+	case SOCINFO_VERSION(0, 1):
+		pr_info("v%u.%u, id=%u, ver=%u.%u\n",
+			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min);

This is neither nicely formatted (info logs should not be based on __func__) nor useful to the average developer.

The reason why I'm objecting is that booting a CAF kernel gives me pages and pages of cryptic debug messages, warning and error messages. But due to the amount of logs no-one will actually read through the log and correct things - and when you're actually looking for an error the scroll-back buffer is too short.

So I object to this, based on the broken window theory.

[..]

Quoted 1 line

Thanks and Regards,

Thank you Imran, looking forward to the next version.

Regards,
Bjorn

Quoted 21 lines · Bjorn Andersson

On Mon 07 Nov 06:35 PST 2016, Imran Khan wrote:

[..]

Quoted 10 lines

Quoted 9 lines

drivers/soc/qcom/socinfo.c line 1010

Quoted 5 lines

drivers/soc/qcom/socinfo.c new file +5 -0

+static void socinfo_populate(struct soc_device_attribute *soc_dev_attr)
+{
+	u32 soc_version = socinfo_get_version();
+
+	soc_dev_attr->soc_id   = kasprintf(GFP_KERNEL, "%d", socinfo_get_id());

I believe soc_id is supposed to be a human readable name; e.g. "MSM8996" not "246".

Quoted 4 lines

I am not sure about this. I see other vendors also exposing soc_id as numeric value and machine is perhaps used for a human readable name. Please let me if I am getting something wrong here.

Quoted 9 lines

I'm slightly confused to what these various properties are supposed to contain, according to Documentation/ABI/testing/sysfs-devices-soc soc_id should contain the SoC serial number, while most implementations does like you and put something telling which SoC it is.

246 is however not a useful number, as everyone reading it - be it human or computer - will have to carry the translation table to figure out what it actually says.

Yeah. I agree on this point. I was just following the lead of other SoCs here. Just worried if having a string here breaks the convention. At least having a numeric number is more in line with the documentation which expects a serial number. May be here by serial number the documentation means numeric id itself. Can someone please provide some feedback?

Quoted 5 lines

Quoted 1 line

Quoted 1 line

drivers/soc/qcom/socinfo.c line 1011

Quoted 1 line

drivers/soc/qcom/socinfo.c new file +1 -0

+	soc_dev_attr->family  =  "Snapdragon";

I think family should be e.g. "MSM8996" and then machine should be e.g. "MSM8996AU".

I think here family should be Snapdragon.The following site also mentions the SoCs as Snapdragon family of processors.

https://www.qualcomm.com/products/snapdragon/processors/comparison

Could you please confirm if it's okay?

Quoted 32 lines · Imran Khan

Quoted 21 lines · Bjorn Andersson

On Mon 07 Nov 06:35 PST 2016, Imran Khan wrote:

[..]

Quoted 10 lines

Quoted 9 lines

drivers/soc/qcom/socinfo.c line 1010

Quoted 5 lines

drivers/soc/qcom/socinfo.c new file +5 -0

+static void socinfo_populate(struct soc_device_attribute *soc_dev_attr)
+{
+	u32 soc_version = socinfo_get_version();
+
+	soc_dev_attr->soc_id   = kasprintf(GFP_KERNEL, "%d", socinfo_get_id());

I believe soc_id is supposed to be a human readable name; e.g. "MSM8996" not "246".

Quoted 4 lines

I am not sure about this. I see other vendors also exposing soc_id as numeric value and machine is perhaps used for a human readable name. Please let me if I am getting something wrong here.

Quoted 9 lines

I'm slightly confused to what these various properties are supposed to contain, according to Documentation/ABI/testing/sysfs-devices-soc soc_id should contain the SoC serial number, while most implementations does like you and put something telling which SoC it is.

246 is however not a useful number, as everyone reading it - be it human or computer - will have to carry the translation table to figure out what it actually says.

Quoted 6 lines

Yeah. I agree on this point. I was just following the lead of other SoCs here. Just worried if having a string here breaks the convention. At least having a numeric number is more in line with the documentation which expects a serial number. May be here by serial number the documentation means numeric id itself. Can someone please provide some feedback?

Yeah, the more i look at this the more puzzled I become about what should go where.

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

drivers/soc/qcom/socinfo.c line 1011

Quoted 1 line

drivers/soc/qcom/socinfo.c new file +1 -0

+	soc_dev_attr->family  =  "Snapdragon";

I think family should be e.g. "MSM8996" and then machine should be e.g. "MSM8996AU".

Quoted 7 lines

I think here family should be Snapdragon.The following site also mentions the SoCs as Snapdragon family of processors.

https://www.qualcomm.com/products/snapdragon/processors/comparison

Could you please confirm if it's okay?

In our previous technical discussions regarding Qualcomm platforms the possible values for "family" would be U, A and B (maybe something new these days?).

But I don't think we gain anything from having the kernel tell us this.

So I'm fine with you reporting "Snapdragon" as family and I guess machine would then get e.g. "APQ8096". I don't know what to put in soc_id.

I think this would be sufficient for user space's needs.

Regards,
Bjorn