Re: [PATCH v6] soc: qcom: Add SoC info driver

[Stephen Boyd]

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