linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Imran Khan <kimran@codeaurora.org>
To: Rob Herring <robh@kernel.org>
Cc: bjorn.andersson@linaro.org, sboyd@codeaurora.org,
	agross@codeaurora.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-soc@vger.kernel.org
Subject: Re: [v10, 2/2] Documentation/ABI: Add ABI information for QCOM socinfo driver
Date: Tue, 18 Apr 2017 19:53:27 +0530	[thread overview]
Message-ID: <4b85e85b-8c2a-a226-d3b2-67fde0820ada@codeaurora.org> (raw)
In-Reply-To: <0bf24858-33b2-8d08-ddc3-df83a2bd696d@codeaurora.org>

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@codeaurora.org>
>>> ---
>>>  .../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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +Description:
>>> +		This file show the version of the hardware platform.
>>> +
>>> +What:		/sys/devices/soc0/pmic_die_revision
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@vger.kernel.org
>>> +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@vger.kernel.org
>>> +Description:
>>> +		This file shows name of PMIC model.
>>> +
>>> +What:		/sys/devices/soc0/qcom_odm
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@vger.kernel.org
>>> +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@vger.kernel.org
>>> +Description:
>>> +		This file shows raw version of the SoC.
>>> +
>>> +What:		/sys/devices/soc0/revision
>>> +Date:		January 2017
>>> +Contact:	linux-arm-msm@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>>> +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@vger.kernel.org
>> 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

  reply	other threads:[~2017-04-18 14:23 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-20 16:47 [PATCH v10 0/2] soc: qcom: Add SoC info driver Imran Khan
2017-02-20 16:47 ` [PATCH v10 1/2] " Imran Khan
2017-02-20 16:47 ` [PATCH v10 2/2] Documentation/ABI: Add ABI information for QCOM socinfo driver Imran Khan
2017-02-22 14:04   ` [v10, " Rob Herring
2017-03-06  6:49     ` Imran Khan
2017-04-18 14:23       ` Imran Khan [this message]
2017-04-24 16:00         ` Imran Khan
2017-04-24 16:27           ` Rob Herring
2017-04-24 23:05             ` Bjorn Andersson
2017-04-25  7:35               ` Imran Khan
2017-04-25 17:13               ` Rob Herring
2017-04-25 23:08                 ` Bjorn Andersson
2017-05-01 13:07                   ` Imran Khan
2017-05-01 22:47                     ` Bjorn Andersson
2017-05-03 12:51                       ` Imran Khan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4b85e85b-8c2a-a226-d3b2-67fde0820ada@codeaurora.org \
    --to=kimran@codeaurora.org \
    --cc=agross@codeaurora.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-soc@vger.kernel.org \
    --cc=robh@kernel.org \
    --cc=sboyd@codeaurora.org \
    --subject='Re: [v10, 2/2] Documentation/ABI: Add ABI information for QCOM socinfo driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).