Re: [v10, 2/2] Documentation/ABI: Add ABI information for QCOM socinfo driver

[Imran Khan]

Hi Rob,

On 3/6/2017 12:19 PM, Imran Khan wrote:
> On 2/22/2017 7:34 PM, Rob Herring wrote:
>> On Mon, Feb 20, 2017 at 10:17:15PM +0530, Khan, Imran wrote:
>>> The socinfo ABI document describes the information provided
>>> by socinfo driver and the corresponding attributes to access
>>> that information.
>>>
>>> Signed-off-by: Imran Khan <kimran@xxxxxxxxxxxxxx>
>>> ---
>>>  .../ABI/testing/sysfs-driver-qcom_socinfo          | 171 +++++++++++++++++++++
>>>  1 file changed, 171 insertions(+)
>>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-qcom_socinfo
>>
>> Sorry to comment late on this (blame LWM), but I think creating this ABI 
>> is a mistake. The biggest issue I have is this doesn't scale if every 
>> SoC does its own thing. We should have a common interface so for example 
>> userspace can retrieve the serial number from any SoC in the same way. 
>> Yes, we can have custom attributes, but there should be common base.
>>
> 
> Yeah, I agree about the scalability part. Could you please suggest some way to
> implement a common base for the custom attributes. Like for serial number I think
> we can put it in generic soc_device_attribute but for custom attributes like accessory_chip,
> hw_platform etc., how can we implement a common base. Can we have a private pointer within
> generic soc_device_attribute structure and this private pointer can point to custom attributes.
> Or if you have some other suggestion to implement this common interface, please let me know.

Could you please provide some feedback regarding this?

>>
>>> diff --git a/Documentation/ABI/testing/sysfs-driver-qcom_socinfo b/Documentation/ABI/testing/sysfs-driver-qcom_socinfo
>>> new file mode 100644
>>> index 0000000..cce611f
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-driver-qcom_socinfo
>>> @@ -0,0 +1,171 @@
>>> +What:		/sys/devices/soc0/accessory_chip
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		This file shows the id of the accessory chip.
>>> +
>>> +What:		/sys/devices/soc0/adsp_image_crm
>>> +What:		/sys/devices/soc0/adsp_image_variant
>>> +What:		/sys/devices/soc0/adsp_image_version
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		These files respectively show the crm version, variant and
>>> +		version of the ADSP image.
>>
>> Shouldn't this be part of the ADSP driver?
>>
> Yes, It can be but I wanted to keep the image information at a central location, 
> rather than pushing it back to each driver. For image information we basically
> read the same item from SMEM but use different offsets within it for different images,
> so the idea was to read this information ( get SMEM handler) just once, rather than
> doing it for each driver. 
> But if this idea does not look correct, I can remove it from socinfo driver. 
>  

Could you please provide some feedback regarding this?

>>> +
>>> +What:		/sys/devices/soc0/apps_image_crm
>>> +What:		/sys/devices/soc0/apps_image_variant
>>> +What:		/sys/devices/soc0/apps_image_version
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		These files respectively show the crm version, variant and
>>> +		version of the APPS(Linux kernel, rootfs) image.
>>
>> Assuming that the kernel and rootfs are the same image and updated 
>> together?
>>
> 
> Yes. The kernel and rootfs are same image and they are updated together.
> 
>>> +
>>> +What:		/sys/devices/soc0/boot_image_crm
>>> +What:		/sys/devices/soc0/boot_image_variant
>>> +What:		/sys/devices/soc0/boot_image_version
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		These files respectively show the crm version, variant and
>>> +		version of the Boot(bootloader) image.
>>> +
>>> +What:		/sys/devices/soc0/build_id
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		This file shows the unique id of current build being used on
>>> +		the system.
>>
>> Build of what? The kernel already has a build version.
>>
> This is not build id of the kernel. This is build ID of complete meta image.
> 
>>> +
>>> +What:		/sys/devices/soc0/cnss_image_crm
>>> +What:		/sys/devices/soc0/cnss_image_variant
>>> +What:		/sys/devices/soc0/cnss_image_version
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		These files respectively show the crm version, variant and
>>> +		version of the CNSS image.
>>> +
>>> +What:		/sys/devices/soc0/family
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		This file shows the family(e.g Snapdragon) of the SoC.
>>
>> Sounds like a standard attr.
>>
> Yeah. This is standard attribute. Will remove this from Documentation here.
> 
>>> +
>>> +What:		/sys/devices/soc0/foundry_id
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		This file shows the id of the foundry, where soc was
>>> +		manufactured.
>>
>> I don't see how userspace should care...
>>
> Yeah, usually user space would not care for such information. But sometimes we have 
> come across h/w issues that were seen only on set of chips from a particular
> foundry. Under such situations we use this information to confirm if a certain h/w
> issue is specific to a batch from a particular foundry or not.
> 
Could you please provide some feedback regarding this?

>>> +
>>> +What:		/sys/devices/soc0/hw_platform
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		This file shows the type of hardware platform
>>> +		(e.g MTP, QRD etc) where SoC is being used.
>>
>> What's a platform?
>>
> We may use same soc on different type of platforms. For example for QCOM we have
> MTP (board with which a debug board can be connected), QRD (no debug connection available).
> Similarly other ODMs may have different kind of platforms based on same soc. 
> hw_paltform indicates numeric id for different kind of such platforms.
> 
>>> +
>>> +What:		/sys/devices/soc0/machine
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		This file shows the machine name as given in the DT.
>>
>> This is already exposed.
>>
> Yeah. Will remove it from this document.
> 
>>> +
>>> +What:		/sys/devices/soc0/mpss_image_crm
>>> +What:		/sys/devices/soc0/mpss_image_variant
>>> +What:		/sys/devices/soc0/mpss_image_version
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		These files respectively show the crm version, variant and
>>> +		version of the MPSS image.
>>
>> Part of the MPSS driver?
>>
> Agree. Kindly see my comment above for the ADSP image(adsp_image_xxx).
> 
>>> +
>>> +What:		/sys/devices/soc0/platform_subtype
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		This file shows the sub-type of hardware platform
>>> +		(SKUAA, SKUF etc.) where SoC is being used.
>>> +
>>> +What:		/sys/devices/soc0/platform_version
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		This file show the version of the hardware platform.
>>> +
>>> +What:		/sys/devices/soc0/pmic_die_revision
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		This file shows revision of PMIC die.
>>
>> Part of the PMIC driver?
>>
> Agree. Kindly see my comment above for the ADSP image(adsp_image_xxx).
> 
>>> +
>>> +What:		/sys/devices/soc0/pmic_model
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		This file shows name of PMIC model.
>>> +
>>> +What:		/sys/devices/soc0/qcom_odm
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		This file shows the ODM using the SoC.
>>
>> The vendor in the top-level compatible should provide this.
>>
> Yeah. Have removed this in the latest version of driver. Will remove it 
> from ABI document too.
> 
>>> +
>>> +What:		/sys/devices/soc0/raw_version
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		This file shows raw version of the SoC.
>>> +
>>> +What:		/sys/devices/soc0/revision
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		This file shows revision of the SoC.
>>
>> Why do you need both?
>>
> Revision is version of the soc like 2.0, 3.0, 3.1 etc.
> and raw_version is is raw chip version which is an strictly increasing 
> integer counter, increasing for each version of the chip.
> For example:
> Version         Raw_version
> 1.0              0
> 1.1              1
> 2.0              2
> 2.1              3
> 2.2              4
> 3.0              5
> 
>>> +
>>> +What:		/sys/devices/soc0/rpm_image_crm
>>> +What:		/sys/devices/soc0/rpm_image_variant
>>> +What:		/sys/devices/soc0/rpm_image_version
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		These files respectively show the crm version, variant and
>>> +		version of the RPM image.
>>
>> RPM driver?
>>
> Agree. Kindly see my comment above for the ADSP image(adsp_image_xxx).
> 
>>> +
>>> +What:		/sys/devices/soc0/serial_number
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		This file shows serial number of the SoC.
>>
>> Already have a standard property in DT.
>>
>>> +
>>> +What:		/sys/devices/soc0/soc_id
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		This file shows the unique numeric id of a Qualcomm SoC.
>>
>> unique per chip or per SoC model?
>>
> This is unique per SoC model. For example 8996 and 8996pro
> would have different soc_id.
>  
>>> +
>>> +What:		/sys/devices/soc0/tz_image_crm
>>> +What:		/sys/devices/soc0/tz_image_variant
>>> +What:		/sys/devices/soc0/tz_image_version
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		These files respectively show the crm version, variant and
>>> +		version of the TZ image.
>>
>> TZ driver?
>>
> Agree. Kindly see my comment above for the ADSP image(adsp_image_xxx).
> 
>>> +
>>> +What:		/sys/devices/soc0/vendor
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		This file shows the vendor of the SoC.
>>
>> Already in DT.
>>
> Okay. Will remove this field from the driver and from the 
> document.
>  
>>> +
>>> +What:		/sys/devices/soc0/video_image_crm
>>> +What:		/sys/devices/soc0/video_image_variant
>>> +What:		/sys/devices/soc0/video_image_version
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@xxxxxxxxxxxxxxx
>>> +Description:
>>> +		These files respectively show the crm version, variant and
>>> +		version of the video image.
>>
>> Video as in display or video codec? Should be part of the driver.
>>
> Agree. Kindly see my comment above for the ADSP image(adsp_image_xxx).
>> Rob
> 
> Thanks for the review comments.
> 
> Thanks and Regards,
> Imran
>> --
>> 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
>>
> 
> 
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