Date:   Fri, 20 Jan 2023 15:45:32 +0530
From:   Naman Jain <quic_namajain@...cinc.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>
CC:     <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <quic_pkondeti@...cinc.com>, Trilok Soni <quic_tsoni@...cinc.com>,
        "Shiraz Hashim" <quic_shashim@...cinc.com>,
        <quic_kaushalk@...cinc.com>
Subject: Re: [PATCH 2/2] soc: qcom: socinfo: Add sysfs attributes for fields
 in v2-v6


On 1/19/2023 4:29 PM, Dmitry Baryshkov wrote:
> On 19/01/2023 11:39, Naman Jain wrote:
>> Thanks Dmitry for reviewing the patches. Sorry, for replying late on 
>> your email, I wanted to collect all the information, before I do it.
>>
>> On 1/12/2023 4:49 AM, Dmitry Baryshkov wrote:
>>> On 11/01/2023 10:21, Naman Jain wrote:
>>>> Add support in sysfs custom attributes for fields in socinfo version
>>>> v2-v6. This is to support SoC based operations in userland scripts
>>>> and test scripts. Also, add name mappings for hw-platform type to
>>>> make the sysfs information more descriptive.
>>>
>>> Please include a patch documenting your additions to 
>>> Documentation/ABI/testing/sysfs-devices-soc. Please describe 
>>> usecases for new attributes and their applicability to non-Qualcomm 
>>> boards.
>>>
>>
>> The fields added here, are applicable to Qualcomm boards only. I can 
>> include in the same file sysfs-devices-soc, mentioning the same that 
>> it is Qcom specific, or I can create a new file for this, 
>> sysfs-devices-soc-qcom, however you suggest. Mentioning the use 
>> cases, later in the mail.
>
> So, you are extending the generic SoC interface with the 
> vendor-specific interfaces. There must be a file describing them in a 
> generic enough way that other vendors can apply for their boards too.
>
> Note, that /sys/devices/soc applies to SoC level, not the board level. 
> Generally I think that you should export your data through a more 
> generic data path, e.g. /sys/firmware.


Understood, will keep that in mind.


>
>>
>>
>>> Note, that testing scripts can access debugfs entries without any 
>>> issues.
>>
>>
>> Yes, that is right. Thanks.
>>
>>
>>>
>>>>
>>>> Signed-off-by: Naman Jain <quic_namajain@...cinc.com>
>>>> ---
>>>>   drivers/soc/qcom/socinfo.c | 181 
>>>> +++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 181 insertions(+)
>>>>
>>>> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
>>>> index 251c0fd94962..ff92064c2246 100644
>>>> --- a/drivers/soc/qcom/socinfo.c
>>>> +++ b/drivers/soc/qcom/socinfo.c
>>>> @@ -41,6 +41,52 @@
>>>>    */
>>>>   #define SMEM_HW_SW_BUILD_ID            137
>>>>   +enum {
>>>> +    HW_PLATFORM_UNKNOWN = 0,
>>>> +    HW_PLATFORM_SURF = 1,
>>>> +    HW_PLATFORM_FFA = 2,
>>>> +    HW_PLATFORM_FLUID = 3,
>>>> +    HW_PLATFORM_SVLTE_FFA = 4,
>>>> +    HW_PLATFORM_SVLTE_SURF = 5,
>>>> +    HW_PLATFORM_MTP_MDM = 7,
>>>> +    HW_PLATFORM_MTP = 8,
>>>> +    HW_PLATFORM_LIQUID = 9,
>>>> +    HW_PLATFORM_DRAGON = 10,
>>>> +    HW_PLATFORM_QRD = 11,
>>>> +    HW_PLATFORM_HRD = 13,
>>>> +    HW_PLATFORM_DTV = 14,
>>>> +    HW_PLATFORM_RCM = 21,
>>>> +    HW_PLATFORM_STP = 23,
>>>> +    HW_PLATFORM_SBC = 24,
>>>> +    HW_PLATFORM_HDK = 31,
>>>> +    HW_PLATFORM_ATP = 33,
>>>> +    HW_PLATFORM_IDP = 34,
>>>> +    HW_PLATFORM_INVALID
>>>> +};
>>>> +
>>>> +static const char * const hw_platform[] = {
>>>> +    [HW_PLATFORM_UNKNOWN] = "Unknown",
>>>> +    [HW_PLATFORM_SURF] = "Surf",
>>>> +    [HW_PLATFORM_FFA] = "FFA",
>>>> +    [HW_PLATFORM_FLUID] = "Fluid",
>>>> +    [HW_PLATFORM_SVLTE_FFA] = "SVLTE_FFA",
>>>> +    [HW_PLATFORM_SVLTE_SURF] = "SLVTE_SURF",
>>>> +    [HW_PLATFORM_MTP_MDM] = "MDM_MTP_NO_DISPLAY",
>>>> +    [HW_PLATFORM_MTP] = "MTP",
>>>> +    [HW_PLATFORM_RCM] = "RCM",
>>>> +    [HW_PLATFORM_LIQUID] = "Liquid",
>>>> +    [HW_PLATFORM_DRAGON] = "Dragon",
>>>> +    [HW_PLATFORM_QRD] = "QRD",
>>>> +    [HW_PLATFORM_HRD] = "HRD",
>>>> +    [HW_PLATFORM_DTV] = "DTV",
>>>> +    [HW_PLATFORM_STP] = "STP",
>>>> +    [HW_PLATFORM_SBC] = "SBC",
>>>> +    [HW_PLATFORM_HDK] = "HDK",
>>>> +    [HW_PLATFORM_ATP] = "ATP",
>>>> +    [HW_PLATFORM_IDP] = "IDP",
>>>> +    [HW_PLATFORM_INVALID] = "Invalid",
>>>> +};
>>>
>>> This is not a property of the SoC. It is a property of the device. 
>>> As such it should not be part of /sys/bus/soc devices.
>>
>>
>> I understand your point. The Socinfo structure as such on Qualcomm 
>> SoC gives not just SoC related information but also many other info 
>> like serial number, platform subtype etc. Now in order to support the 
>> usecases below, we are proposing sysfs interface extension, as we 
>> can't use debugfs interface in production/end user devices due to 
>> debugfs access restrictions.
>
> "The vendor does it in this way" doesn't give you a right to repurpose 
> the ABI.


Got it.


>
>>
>> Use cases:
>>
>> 1. In post-boot shell scripts, for various chip specific operations, 
>> that are relevant to that particular chip/board only:
>>
>>      a. Setting kernel parameters using sysfs interfaces etc.
>
> If the parameter is common to all devices of some kind, it should be 
> set by the driver using the data in the DTS. See, how this is managed 
> for PHY tunings. You can not expect for the userspace to function in 
> any particular way. The whole userspace might be a single /bin/bash 
> executing commands and/or scripts. And still the device should 
> function _properly_.


OK.


>
>>
>>      b. Enabling particular traces, logs
>
> This should not depend on the device type. If you have something 
> hw-specific, check the particular device instance rather than checking 
> the board kind.


Got it.


>
>>
>>      c. Changing permissions to certain paths
>
> Excuse me, what paths? Permissions have nothing to do with the board 
> kind.


I think, the solution to these type of use-cases, would fall under the 
umbrella of your previous comment " If you have something hw-specific, 
check the particular device instance rather than checking the board 
kind.". Thanks.


>
>>
>>      d. Start a userspace service, and pass custom parameters to it 
>> on the fly
>
> I think this also depends on the hardware availability rather than the 
> board properties.


OK.


>
>>
>>      e. Set certain device properties using setprop
>
> Android specifics. Please formulate this in a generic way.


Will do.


>
>>
>>      f. Miscellaneous things like DCC (Data Capture and Compare 
>> Engine) etc.
>
> Please expand this, you can not expect one to know what is DCC and how 
> it is used.
>
>>
>> 2. In userspace services, that depend on SoC information, for its 
>> configuration. Eg: Audio, Connectivity services use these.
>
> This is handled using the device ids, models, etc.. Please see, how 
> this is handled by other software (hint: ALSA UCM, pulseaudio) instead 
> of inventing something vendor-specific.


Noted.


>
>>
>> 3. adb needs device serial number, sensors need SoC information to 
>> decide its configuration.
>
> Already available via /proc/cmdline thanks for your bootloader.


Noted. Thanks


>
>>
>>
>>>
>>> You can find board description in /sys/firmware/devicetree/base/model
>>
>>
>> Thanks for pointing this out. This is giving useful information on 
>> the chip and hw_platform, but the problem is that we need other 
>> fields as well, which we may want to use. Hence the ask.
>>
>> model = "Qualcomm Technologies, Inc. Kalama MTP";
>
> Generally I think that Qualcomm's socinfo is a kind of firmware 
> interface, so you can probably extend /sys/firmware to provide this 
> kind of information.


OK, will check. Thanks.


>
>>
>>
>>>
>>>> +
>>>>   #ifdef CONFIG_DEBUG_FS
>>>>   #define SMEM_IMAGE_VERSION_BLOCKS_COUNT        32
>>>>   #define SMEM_IMAGE_VERSION_SIZE                4096
>>>> @@ -368,6 +414,140 @@ static const struct soc_id soc_id[] = {
>>>>       { qcom_board_id(QRU1062) },
>>>>   };
>>>>   +/* sysfs attributes */
>>>> +#define ATTR_DEFINE(param) \
>>>> +    static DEVICE_ATTR(param, 0644, qcom_get_##param, NULL)
>>>> +
>>>> +/* Version 2 */
>>>> +static ssize_t
>>>> +qcom_get_raw_id(struct device *dev,
>>>> +        struct device_attribute *attr,
>>>> +        char *buf)
>>>> +{
>>>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>>>> +             le32_to_cpu(soc_info->raw_id));
>>>> +}
>>>> +ATTR_DEFINE(raw_id);
>>>> +
>>>> +static ssize_t
>>>> +qcom_get_raw_version(struct device *dev,
>>>> +        struct device_attribute *attr,
>>>> +        char *buf)
>>>> +{
>>>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>>>> +             le32_to_cpu(soc_info->raw_ver));
>>>> +}
>>>> +ATTR_DEFINE(raw_version);
>>>
>>> Why are they raw? can you unraw them?
>>>
>>> Whose version and id are these attributes referring to?
>>
>>
>> So basically, when we call them raw, it essentially means that it is 
>> not parsed as such (different bits may be giving different 
>> information, and the whole value may mean nothing).
>>
>> *version* refers to the chip version, which can be like v1, v2, v1.1 
>> etc in real terms. Its raw value is used to map it to one of these 
>> versions. *id* is used as chip ID for QC SoCs for using JTAG. It is 
>> different than the soc_id that we have.
>
> Unraw the values.
>
>>
>>
>>>
>>>> +
>>>> +/* Version 3 */
>>>> +static ssize_t
>>>> +qcom_get_hw_platform(struct device *dev,
>>>> +        struct device_attribute *attr,
>>>> +        char *buf)
>>>> +{
>>>> +    uint32_t hw_plat = le32_to_cpu(soc_info->hw_plat);
>>>> +
>>>> +    hw_plat = (hw_plat >= HW_PLATFORM_INVALID) ? 
>>>> HW_PLATFORM_INVALID : hw_plat;
>>>> +    return scnprintf(buf, PAGE_SIZE, "%-.32s\n",
>>>> +            hw_platform[hw_plat]);
>>>> +}
>>>> +ATTR_DEFINE(hw_platform);
>>>> +
>>>> +/* Version 4 */
>>>> +static ssize_t
>>>> +qcom_get_platform_version(struct device *dev,
>>>> +        struct device_attribute *attr,
>>>> +        char *buf)
>>>> +{
>>>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>>>> +             le32_to_cpu(soc_info->plat_ver));
>>>> +}
>>>> +ATTR_DEFINE(platform_version);
>>>> +
>>>> +/* Version 5 */
>>>> +static ssize_t
>>>> +qcom_get_accessory_chip(struct device *dev,
>>>> +        struct device_attribute *attr,
>>>> +        char *buf)
>>>> +{
>>>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>>>> +            le32_to_cpu(soc_info->accessory_chip));
>>>> +}
>>>> +ATTR_DEFINE(accessory_chip);
>>>
>>> If this an _accessory_ chip, there should be a separate soc device 
>>> describing it, rather than stuffing information into the soc0.
>>>
>>
>> This is used as a boolean currently to tell us whether SoC has an 
>> accessory chip or not.
>
> SoC doesn't have accessory chip. It the board having the accessory (to 
> the main SoC) or not.
>
> Also, please do not use 'currently' for the sysfs files. They are ABI. 
> And changing ABI is a painful process which might be not available at 
> all. So once you export something through the sysfs, it is written in 
> stone. Not 'currently, to be changed later'.
>

My bad. That may have been just a word, that I use frequently. Totally 
got your point.


>>
>>
>>>> +
>>>> +/* Version 6 */
>>>> +static ssize_t
>>>> +qcom_get_platform_subtype_id(struct device *dev,
>>>> +        struct device_attribute *attr,
>>>> +        char *buf)
>>>> +{
>>>> +    return scnprintf(buf, PAGE_SIZE, "%u\n",
>>>> +             le32_to_cpu(soc_info->hw_plat_subtype));
>>>> +}
>>>> +ATTR_DEFINE(platform_subtype_id);
>>>
>>> Again, this is the board property, not an SoC one.
>>
>>
>> Same justification as one of my previous comments.
>
> Same comment. /sys/bus/soc exists to export information about, you 
> guess, SoC. If you want to export information about the board, please 
> find a better way.
>
>

Thanks Dmitry for reviewing. Understood your points. Let us re-evaluate, 
what fields are coming under SoC, what are required and why, and we will 
start the discussion again with the new requirements, if any.


Regards,

Naman Jain

Powered by blists - more mailing lists