From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936472AbcLOQ3p (ORCPT ); Thu, 15 Dec 2016 11:29:45 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:46316 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750801AbcLOQ3n (ORCPT ); Thu, 15 Dec 2016 11:29:43 -0500 DMARC-Filter: OpenDMARC Filter v1.3.1 smtp.codeaurora.org 19F776147D Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=pass smtp.mailfrom=kimran@codeaurora.org Subject: Re: [PATCH v6] soc: qcom: Add SoC info driver To: Stephen Boyd References: <1481555889-29380-1-git-send-email-kimran@codeaurora.org> <20161214002617.GS5423@codeaurora.org> Cc: andy.gross@linaro.org, lee.jones@linaro.org, David Brown , open list , "open list:ARM/QUALCOMM SUPPORT" , "open list:ARM/QUALCOMM SUPPORT" From: Imran Khan Message-ID: Date: Thu, 15 Dec 2016 21:59:36 +0530 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.5.0 MIME-Version: 1.0 In-Reply-To: <20161214002617.GS5423@codeaurora.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> >> >> 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 >> +#include > > This include isn't necessary. > True. Will remove this in the next patch set. >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include > > Is this include used? > Will check and remove accordingly. >> +#include >> +#include >> +#include >> +#include >> + >> +#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]; >> +}; >> + >> +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 */ >> + /* 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