Message-ID: <d9ab1896-5c60-ef08-3eac-85de5624ca89@codeaurora.org>
Date:   Thu, 15 Dec 2016 21:59:36 +0530
From:   Imran Khan <kimran@...eaurora.org>
To:     Stephen Boyd <sboyd@...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/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@...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.
> 

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

Powered by blists - more mailing lists