Date:   Sun, 18 Dec 2016 23:42:30 +0530
From:   Imran Khan <kimran@...eaurora.org>
To:     Stephen Boyd <sboyd@...eaurora.org>
Cc:     andy.gross@...aro.org, lee.jones@...aro.org,
        David Brown <david.brown@...aro.org>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@...r.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-soc@...r.kernel.org>
Subject: Re: [PATCH v6] soc: qcom: Add SoC info driver

On 12/17/2016 6:56 AM, Stephen Boyd wrote:
> 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.
>

Sure. I will.
 
>>
>>>> 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.
> 

I had discussed this with Bjorn and it was recommended to keep it out of 
smem.h. If needed I can move it back 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
>>>> +
>>>> +/* 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.
> 

Yes. The numbers used here can have different meaning for different ODMs.
But these attributes (hw_patform type/subtype etc.) are outside the
generic soc_device_attribute so I think the interpretation of these numbers
can very well be ODM specific. We can try to keep only those types here that
are relevant for newer platforms but we intend to keep these attributes
nonetheless.

>>>> +
>>>> +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.
> 

Again we can keep here only relevant subtypes.

>>>
>>>> +
>>>> +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?
> 

Okay. Sounds fine to me.

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

Powered by blists - more mailing lists