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@xxxxxxxxxxxxxx>
>>
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
