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