From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758967AbcLQB01 (ORCPT ); Fri, 16 Dec 2016 20:26:27 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:46502 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758288AbcLQB0R (ORCPT ); Fri, 16 Dec 2016 20:26:17 -0500 DMARC-Filter: OpenDMARC Filter v1.3.1 smtp.codeaurora.org DBCB06166C 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=sboyd@codeaurora.org Date: Fri, 16 Dec 2016 17:26:15 -0800 From: Stephen Boyd To: Imran Khan Cc: andy.gross@linaro.org, lee.jones@linaro.org, David Brown , open list , "open list:ARM/QUALCOMM SUPPORT" , "open list:ARM/QUALCOMM SUPPORT" Subject: Re: [PATCH v6] soc: qcom: Add SoC info driver Message-ID: <20161217012615.GV5423@codeaurora.org> References: <1481555889-29380-1-git-send-email-kimran@codeaurora.org> <20161214002617.GS5423@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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