Messages in this thread | | | Date | Fri, 20 Jan 2023 15:45:32 +0530 | Subject | Re: [PATCH 2/2] soc: qcom: socinfo: Add sysfs attributes for fields in v2-v6 | From | Naman Jain <> |
| |
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@quicinc.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
| |