Message-ID: <20161214002617.GS5423@codeaurora.org>
Date:   Tue, 13 Dec 2016 16:26:17 -0800
From:   Stephen Boyd <sboyd@...eaurora.org>
To:     Imran Khan <kimran@...eaurora.org>
Cc:     andy.gross@...aro.org, lee.jones@...aro.org,
        David Brown <david.brown@...aro.org>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@...r.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-soc@...r.kernel.org>
Subject: Re: [PATCH v6] soc: qcom: Add SoC info driver

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

Powered by blists - more mailing lists