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

[Rob Herring]

On Mon, Apr 24, 2017 at 11:00 AM, Imran Khan <kimran@xxxxxxxxxxxxxx> wrote:
> On 4/18/2017 7:53 PM, Imran Khan wrote:
> Hi Rob,
> Could you please provide some feedback so that this discussion can move forward
> and ABI document can be finalized?
> Without the ABI document we are not able to get the corresponding driver
> finalized and get merged.
>
> Thanks again for your time,
> Imran
>> 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?

Splitting things between common and private seems like a good direction.

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

I don't think parsing things once will save you much.

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

Maybe for you, but generally those are separate pieces.

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

That's assuming everything is built together which generally isn't
true. It doesn't seem like that is information the kernel should
provide.

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

The qcom compatible string format already provides this. I don't think
we need 2 ABIs that are both vendor specific to expose this. Now if
there's other vendors wanting to expose the foundry, then a common
attr would make sense.

compatible = "qcom,<SoC>[-<soc_version>][-<foundry_id>]-<board>[/<subtype>][-<board_version>]"

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

As above, I believe /compatible already provides that information.

Rob