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@xxxxxxxxxxx> ---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, logsThis 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 pathsExcuse 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 flyI think this also depends on the hardware availability rather than the board properties.
OK.
e. Set certain device properties using setpropAndroid 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/modelThanks 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
