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

[Imran Khan]

On 4/25/2017 4:35 AM, Bjorn Andersson wrote:
> On Mon 24 Apr 09:27 PDT 2017, Rob Herring wrote:
> 
>> 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.
>>
> 
> But the pattern of amending the common attributes with vendor or
> platform-specific ones is how its done in other drivers, e.g.
> integrator.
> 
> Why should we for the Qualcomm case make up some other pattern?
> 
> Or am I misunderstanding your suggestion?
> 
>>>>>>> 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.
>>
> 
> Defining the struct in some shared header file and implementing the
> "parsing" throughout various drivers would probably work out fine.
> 
> What I do not fancy is where these "parsers" would be implemented and
> where the information would end up in sysfs.
> 
> "The ADSP driver" has to refer to the remoteproc driver for the adsp and
> it would be reasonable to have version information of the firmware
> available for a running piece of remoteproc. The same would work out for
> modem and wireless.
> 
> The v4l driver is responsible for controlling the venus core, so it
> could provide the version attribute - which would show up somewhere in
> the /sys/bus/platform hierarchy.
> 
> We have a mfd-like driver for communicating with the RPM, so perhaps we
> could shoehorn in an attribute there. But the sysfs path will be
> completely different, depending on platform and system configuration.
> 
> There is no driver representing the boot code.
> 
> 
> So, when Android goes belly up and we want to produce a bugreport that
> describes all the pieces of the system we will have to all over sysfs to
> figure out the versions of the firmware...
> 
>>>>
>>>>>>> +
>>>>>>> +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.
>>
> 
> This attribute is special in that it is populated from user space, so
> any tool running on apps should be able to fetch it directly anyways
> (e.g. from Android properties).
> 
> Imran, is there any users of this information outside the apps context?
> E.g. does your tools for analysing memory-dumps depend on this
> information being available in SMEM?
> 

Yes, apart from apps our memory dump analyzers and diag tools make use of this 
information

>>>>>
>>>>>>> +
>>>>>>> +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.
> 
> Not necessarily compiled together, but packaged up in something that
> gets a "release number" - which I presume is quite common for any type
> of embedded products.
> 
> E.g. we do give the Linaro releases version numbers such as "16.09".
> 
>> It doesn't seem like that is information the kernel should
>> provide.
>>
> 
> I used to say that we could just store this information in a file in
> /etc (or in build.prop), then I started doing post-build composition and
> realized that you really do want a new system-version if you change e.g.
> the version of the boot firmware - without having to regenerate the
> rootfs.
> 
> So while I agree that this is none of the kernel's business I'm not sure
> how you would get this information from SMEM to a user space process for
> e.g. bugreport generation purposes.
> 
>>>>>
>>>>>>> +
>>>>>>> +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>]"
>>
> 
> For userspace to be able to parse out this information from
> /proc/device-tree/compatible we need to improve that expression quite a
> bit! What's the foundry_id of <"acme,phone-a","qcom,platform-b-c">?
> 
>>>>
>>>>>>> +
>>>>>>> +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.
>>
> 
> I believe this is what OMAP calls "type", in a custom attribute on the
> side of the common ones.
> 
> But as far as I can see this information should be trivial to derive
> from the compatible of the board. Imran, what is this data used for?
> 

This data is used in several cases. For example MTP and QRD boards may use
different thermal calibration, different firmwares, different wcnss settings.
These are the some use cases, I can recall right now, there may be others too.

> Regards,
> Bjorn
> 


-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation