[PATCH v6] soc: qcom: Add SoC info driver

[PATCH v6] soc: qcom: Add SoC info driver

[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 <kimran@codeaurora.org>
---
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

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 461b387..f89d34d 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -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
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index fdd664e..438efec 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -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
diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 18ec52f..c598cac 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -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;
 }
 
diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
new file mode 100644
index 0000000..57e23dfc
--- /dev/null
+++ b/drivers/soc/qcom/socinfo.c
@@ -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
+ * GNU General Public License for more details.
+ *
+ */
+
+#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>
+#include <linux/soc/qcom/smem.h>
+
+#define PMIC_MODEL_UNKNOWN		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
+#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 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,
+	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,		\
+		(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",
+	[21] = "RCM",
+	[23] = "STP",
+	[24] = "SBC",
+};
+
+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",
+};
+
+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",
+	[23] = "PMIC model: PM8038",
+	[24] = "PMIC model: PM8922",
+	[25] = "PMIC model: PM8917",
+};
+
+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 {
+	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;
+};
+
+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;
+	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;
+
+/* max socinfo format version supported */
+#define MAX_SOCINFO_FORMAT 12
+
+static const char *const cpu_of_id[] = {
+
+	[0] = "Unknown CPU",
+
+	/* 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 */
+	[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
+	 * considered as unknown CPU.
+	 */
+};
+
+/* socinfo: sysfs functions */
+
+static ssize_t
+qcom_show_vendor(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	return sprintf(buf, "%s", "Qualcomm\n");
+}
+
+static ssize_t
+qcom_show_raw_version(struct device *dev,
+		     struct device_attribute *attr,
+		     char *buf)
+{
+	return sprintf(buf, "%u\n", socinfo->v0_2.raw_version);
+}
+
+static ssize_t
+qcom_show_build_id(struct device *dev,
+		   struct device_attribute *attr,
+		   char *buf)
+{
+	return scnprintf(buf, PAGE_SIZE, "%s\n", socinfo->v0_1.build_id);
+}
+
+static ssize_t
+qcom_show_hw_platform(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	return sprintf(buf, "%s\n", hw_platform[socinfo->v0_3.hw_platform]);
+}
+
+static ssize_t
+qcom_show_platform_version(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return sprintf(buf, "%u\n", socinfo->v0_4.platform_version);
+}
+
+static ssize_t
+qcom_show_accessory_chip(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return sprintf(buf, "%u\n", socinfo->v0_5.accessory_chip);
+}
+
+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;
+
+	if (socinfo->v0_3.hw_platform == HW_PLATFORM_QRD)
+		if (hw_subtype >= 0 &&
+				hw_subtype < PLATFORM_SUBTYPE_QRD_INVALID)
+			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;
+}
+
+static ssize_t
+qcom_show_platform_subtype_id(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	return sprintf(buf, "%u\n", socinfo->v0_6.hw_platform_subtype);
+}
+
+static ssize_t
+qcom_show_foundry_id(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	return sprintf(buf, "%u\n", socinfo->v0_9.foundry_id);
+}
+
+static ssize_t
+qcom_show_serial_number(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	return sprintf(buf, "%u\n", socinfo->v0_10.serial_number);
+}
+
+static ssize_t
+qcom_show_chip_family(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	return sprintf(buf, "0x%x\n", socinfo->v0_12.chip_family);
+}
+
+static ssize_t
+qcom_show_raw_device_family(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	return sprintf(buf, "0x%x\n", socinfo->v0_12.raw_device_family);
+}
+
+static ssize_t
+qcom_show_raw_device_number(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	return sprintf(buf, "0x%x\n", socinfo->v0_12.raw_device_number);
+}
+
+static ssize_t
+qcom_show_pmic_model(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	return sprintf(buf, "%s\n", pmic_model[socinfo->v0_7.pmic_model]);
+}
+
+static ssize_t
+qcom_show_pmic_die_revision(struct device *dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	return sprintf(buf, "%u\n", socinfo->v0_7.pmic_die_revision);
+}
+
+static ssize_t
+qcom_show_image_version(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct dev_ext_attribute *dev_attr;
+	int index;
+
+	dev_attr = container_of(attr, struct dev_ext_attribute, attr);
+	index = (unsigned long)(dev_attr->var);
+	return scnprintf(buf, PAGE_SIZE, "%s\n",
+			smem_image_version[index].name);
+}
+
+static ssize_t
+qcom_store_image_version(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf, size_t size)
+{
+	struct dev_ext_attribute *dev_attr;
+	int index;
+
+	dev_attr = container_of(attr, struct dev_ext_attribute, attr);
+	index = (unsigned long)(dev_attr->var);
+	return strlcpy(smem_image_version[index].name, buf,
+			SMEM_IMAGE_VERSION_NAME_SIZE);
+}
+
+static ssize_t
+qcom_show_image_variant(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct dev_ext_attribute *dev_attr;
+	int index;
+
+	dev_attr = container_of(attr, struct dev_ext_attribute, attr);
+	index = (unsigned long)(dev_attr->var);
+	return scnprintf(buf, PAGE_SIZE, "%s\n",
+			smem_image_version[index].variant);
+}
+
+static ssize_t
+qcom_store_image_variant(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf, size_t size)
+{
+	struct dev_ext_attribute *dev_attr;
+	int index;
+
+	dev_attr = container_of(attr, struct dev_ext_attribute, attr);
+	index = (unsigned long)(dev_attr->var);
+	return strlcpy(smem_image_version[index].variant, buf,
+			SMEM_IMAGE_VERSION_VARIANT_SIZE);
+}
+
+static ssize_t
+qcom_show_image_crm(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct dev_ext_attribute *dev_attr;
+	int index;
+
+	dev_attr = container_of(attr, struct dev_ext_attribute, attr);
+	index = (unsigned long)(dev_attr->var);
+	return scnprintf(buf, PAGE_SIZE, "%s\n",
+			smem_image_version[index].oem);
+}
+
+static ssize_t
+qcom_store_image_crm(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf, size_t size)
+{
+	struct dev_ext_attribute *dev_attr;
+	int index;
+
+	dev_attr = container_of(attr, struct dev_ext_attribute, attr);
+	index = (unsigned long)(dev_attr->var);
+	return strlcpy(smem_image_version[index].oem, buf,
+			SMEM_IMAGE_VERSION_OEM_SIZE);
+}
+
+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);
+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
+	*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)
+		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++)
+		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)
+{
+	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");
+		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)
+		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)) {
+		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");
+		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);
+		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);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6] soc: qcom: Add SoC info driver

[Bjorn Andersson]

On Mon 12 Dec 07:17 PST 2016, Imran Khan wrote:

> 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 <kimran@codeaurora.org>

Looks good, just some minor style things.

[..]
> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
[..]
> +
> +#define PMIC_MODEL_UNKNOWN		0

Unused define

> +#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().

> +#define PLATFORM_SUBTYPE_INVALID	4

Dito

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

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

> +
> +/*
> + * 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,		\
> +		(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);
}

[..]
> +
> +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.

[..]
> +
> +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;

> +	if (socinfo->v0_3.hw_platform == HW_PLATFORM_QRD)

Use {} around if statements that are more than a single line.

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

> +			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;
> +}
> +
[..]
> +
> +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)

Use {} around blocks larger than a single line.

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

> +
> +	for (idx = 0; idx < MAX_ATTR_COUNT; idx++)

{}

> +		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)
> +{
> +	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.

> +	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)
> +		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.

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

> +		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);
> +		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

Re: [PATCH v6] soc: qcom: Add SoC info driver

[Stephen Boyd]

On 12/12, Imran Khan wrote:
> 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?

> 
> Signed-off-by: Imran Khan <kimran@codeaurora.org>
> 
> 
>  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.

> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 18ec52f..c598cac 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -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?

> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> new file mode 100644
> index 0000000..57e23dfc
> --- /dev/null
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -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
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/export.h>
> +#include <linux/module.h>

This include isn't necessary.

> +#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?

> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/random.h>
> +#include <linux/soc/qcom/smem.h>
> +
> +#define PMIC_MODEL_UNKNOWN		0

Used?

> +#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
> +#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?

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

> +};
> +
> +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?

> +	{ __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",
> +	[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.

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

> +
> +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",
> +	[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?

> +};
> +
> +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 {
> +	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?

> +};
> +
> +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;
> +	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?

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

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

> +	 * considered as unknown CPU.
> +	 */
> +};
[...]
> +
> +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);
> +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?

> +	*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)
> +		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?

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

> +{
> +	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");
> +		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.

> +		dev_err(&pdev->dev, "Wrong socinfo format\n");
> +		return;
> +	}
> +	socinfo_format = socinfo->v0_1.format;
> +
> +	if (!socinfo->v0_1.id)
> +		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.

> +		dev_warn(&pdev->dev, "Image version table absent.\n");

Will this warn on "older" platforms? Perhaps make this a
dev_dbg() instead.

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

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

> +	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);
> +
> +	/* 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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6] soc: qcom: Add SoC info driver

[Imran Khan]

On 12/14/2016 5:56 AM, Stephen Boyd wrote:
> On 12/12, Imran Khan wrote:
>> 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.
 
>>
>> Signed-off-by: Imran Khan <kimran@codeaurora.org>
>>
>>
>>  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.

>> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
>> index 18ec52f..c598cac 100644
>> --- a/drivers/soc/qcom/smem.c
>> +++ b/drivers/soc/qcom/smem.c
>> @@ -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?
>

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.


>> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
>> new file mode 100644
>> index 0000000..57e23dfc
>> --- /dev/null
>> +++ b/drivers/soc/qcom/socinfo.c
>> @@ -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
>> + * 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.

>> +#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.

>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +#include <linux/random.h>
>> +#include <linux/soc/qcom/smem.h>
>> +
>> +#define PMIC_MODEL_UNKNOWN		0
> 
> Used?
> 
Not used. Will remove this.

>> +#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
>> +#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?
> 

Again redundant. Will remove this.

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

>> +};
>> +
>> +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?
> 
Yes. will do this.

>> +	{ __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",
>> +	[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. 
>> +
>> +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.
> 
>> +
>> +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",
>> +	[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.
 
>> +};
>> +
>> +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>

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

Sure. Will do this.

>> +};
>> +
>> +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;
>> +	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.
 
>> +
>> +/* 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.
>

Sure. soc_of_id would be more appropriate than cpu_of_id. 
Will make this change.
 
>> +
>> +	/* 8x60 IDs */
>> +	[87] = "MSM8960",
>> +
>> +	/* 8x64 IDs */

<snip>

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

>> +	 * considered as unknown CPU.
>> +	 */
>> +};
> [...]
>> +
>> +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);
>> +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?
> 
Yes. Will do this.

>> +	*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)
>> +		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?
>

Sure. That would avoid this redundant macro.
 
>> +		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.

>> +{
>> +	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");
>> +		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.

>> +		dev_err(&pdev->dev, "Wrong socinfo format\n");
>> +		return;
>> +	}
>> +	socinfo_format = socinfo->v0_1.format;
>> +
>> +	if (!socinfo->v0_1.id)
>> +		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.
> 
>> +		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.

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

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

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?
 
>> +	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);
>> +
>> +	/* 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.
> 
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

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v6] soc: qcom: Add SoC info driver

[Stephen Boyd]

On 12/15, Imran Khan wrote:
> On 12/14/2016 5:56 AM, Stephen Boyd wrote:
> > On 12/12, Imran Khan wrote:
> >> 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.

Please include such information into the commit text.

> 
> >> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> >> index 18ec52f..c598cac 100644
> >> --- a/drivers/soc/qcom/smem.c
> >> +++ b/drivers/soc/qcom/smem.c
> >> @@ -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?
> >
> 
> 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.

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.

> 
> 
> >> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> >> new file mode 100644
> >> index 0000000..57e23dfc
> >> --- /dev/null
> >> +++ b/drivers/soc/qcom/socinfo.c
> >> +
> >> +/* 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",
> >> +	[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. 

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.

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

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.

> > 
> >> +
> >> +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",
> >> +	[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.

Sounds ok.

> 
> >> +		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.
> >
> 
> 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?

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.

>  
> >> +	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);
> >> +
> >> +	/* 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.
> > 
> 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?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6] soc: qcom: Add SoC info driver

[Imran Khan]

On 12/17/2016 6:56 AM, Stephen Boyd wrote:
> On 12/15, Imran Khan wrote:
>> On 12/14/2016 5:56 AM, Stephen Boyd wrote:
>>> On 12/12, Imran Khan wrote:
>>>> 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.
> 
> Please include such information into the commit text.
>

Sure. I will.
 
>>
>>>> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
>>>> index 18ec52f..c598cac 100644
>>>> --- a/drivers/soc/qcom/smem.c
>>>> +++ b/drivers/soc/qcom/smem.c
>>>> @@ -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?
>>>
>>
>> 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.
> 
> 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.
> 

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.

>>
>>
>>>> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
>>>> new file mode 100644
>>>> index 0000000..57e23dfc
>>>> --- /dev/null
>>>> +++ b/drivers/soc/qcom/socinfo.c
>>>> +
>>>> +/* 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",
>>>> +	[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. 
> 
> 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.

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

>>>
>>>> +
>>>> +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",
>>>> +	[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.
> 
> Sounds ok.
> 
>>
>>>> +		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.
>>>
>>
>> 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?
> 
> 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.
> 
>>  
>>>> +	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);
>>>> +
>>>> +	/* 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.
>>>
>> 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

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v6] soc: qcom: Add SoC info driver

[Stephen Boyd]

On 12/18, Imran Khan wrote:
> 
> 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.

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

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6] soc: qcom: Add SoC info driver

[Imran Khan]

On 12/21/2016 4:20 AM, Stephen Boyd wrote:
> On 12/18, Imran Khan wrote:
>>
>> 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.
> 
>>
>> 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.
> 
> 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

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v6] soc: qcom: Add SoC info driver

[Stephen Boyd]

On 12/21, Imran Khan wrote:
> On 12/21/2016 4:20 AM, Stephen Boyd wrote:
> > 
> > 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.

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?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6] soc: qcom: Add SoC info driver

[Imran Khan]

On 12/22/2016 6:01 AM, Stephen Boyd wrote:
> On 12/21, Imran Khan wrote:
>> On 12/21/2016 4:20 AM, Stephen Boyd wrote:
>>>
>>> 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.
> 
> 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.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v6] soc: qcom: Add SoC info driver

[Stephen Boyd]

On 12/23, Imran Khan wrote:
> On 12/22/2016 6:01 AM, Stephen Boyd wrote:
> > 
> > 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.
> 

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6] soc: qcom: Add SoC info driver

[Imran Khan]

On 12/29/2016 4:05 AM, Stephen Boyd wrote:
> On 12/23, Imran Khan wrote:
>> On 12/22/2016 6:01 AM, Stephen Boyd wrote:
>>>
>>> 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.
>>
> 
> 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.
> 
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


-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation