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: Mon, 6 Mar 2017 12:19:45 +0530	[thread overview]
Message-ID: <0bf24858-33b2-8d08-ddc3-df83a2bd696d@codeaurora.org> (raw)
In-Reply-To: <20170222140448.qo677wamcd44tgkw@rob-hp-laptop>

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

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


-- 
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-03-06  6:59 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 [this message]
2017-04-18 14:23       ` Imran Khan
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=0bf24858-33b2-8d08-ddc3-df83a2bd696d@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).