Re: [PATCH v2] firmware: smccc: Add ARCH_SOC_ID support

On Wed, Jun 17, 2020 at 02:34:41PM +0100, Steven Price wrote:

On 17/06/2020 11:05, Sudeep Holla wrote:

quoted

SMCCC v1.2 adds a new optional function SMCCC_ARCH_SOC_ID to obtain a
SiP defined SoC identification value. Add support for the same.

Also using the SoC bus infrastructure, let us expose the platform
specific SoC atrributes under sysfs.

There are various ways in which it can be represented in shortened form
for efficiency and ease of parsing for userspace. The chosen form is
described in the ABI document.

Cc: Steven Price <steven.price@arm.com>
Cc: Etienne Carriere <redacted>
Signed-off-by: Sudeep Holla <redacted>
---
  Documentation/ABI/testing/sysfs-devices-soc |  30 ++++++
  drivers/firmware/smccc/Kconfig              |   9 ++
  drivers/firmware/smccc/Makefile             |   1 +
  drivers/firmware/smccc/soc_id.c             | 114 ++++++++++++++++++++
  include/linux/arm-smccc.h                   |   5 +
  5 files changed, 159 insertions(+)
  create mode 100644 drivers/firmware/smccc/soc_id.c

Changes from v1[1] -> v2:
	- Dropped new jep106_id added to SoC infrastructure
	- Dropped all the tags(acks/reviews) as there is change in the format
	- Updated the format for SoC id to ensure there will be no
	  conflict in the namespace

[1] https://lore.kernel.org/lkml/20200522124951.35776-1-sudeep.holla@arm.com/ (local)

diff --git a/Documentation/ABI/testing/sysfs-devices-soc b/Documentation/ABI/testing/sysfs-devices-soc
index ba3a3fac0ee1..50707f316ea9 100644
--- a/Documentation/ABI/testing/sysfs-devices-soc
+++ b/Documentation/ABI/testing/sysfs-devices-soc

@@ -26,6 +26,30 @@ contact:	Lee Jones <lee.jones@linaro.org>
  		Read-only attribute common to all SoCs. Contains SoC family name
  		(e.g. DB8500).
+		On many of ARM based silicon with SMCCC v1.2+ compliant firmware
+		this will contain the JEDEC JEP106 manufacturer’s identification
+		code. The format is "jep106:XXYY" where XX is identity code and
+		YY is continuation code.
+
+		This manufacturer’s identification code is defined by one
+		or more eight (8) bit fields, each consisting of seven (7)
+		data bits plus one (1) odd parity bit. It is a single field,
+		limiting the possible number of vendors to 126. To expand
+		the maximum number of identification codes, a continuation
+		scheme has been defined.
+
+		The specified mechanism is that an identity code of 0x7F
+		represents the "continuation code" and implies the presence
+		of an additional identity code field, and this mechanism
+		may be extended to multiple continuation codes followed
+		by the manufacturer's identity code.
+
+		For example, ARM has identity code 0x7F 0x7F 0x7F 0x7F 0x3B,
+		which is code 0x3B on the fifth 'page'. This can be shortened

NIT: s/can be/is/ - since the format always uses the short form.

Will fix it.

[...]

quoted

+
+	sprintf(soc_id_rev_str, "0x%08x", soc_id_rev);
+	sprintf(soc_id_jep106_id_str, "jep106:%02x%02x",
+		JEP106_BANK_CONT_CODE(soc_id_version),
+		JEP106_ID_CODE(soc_id_version));
+	sprintf(soc_id_str, "%s:%04x", soc_id_jep106_id_str,
+		IMP_DEF_SOC_ID(soc_id_version));

My maths might be wrong, but I think this is one byte too long:

soc_id_jep106_id_str can be 11 characters (without the NUL), then we have a
':' byte followed by 4 hex digits and a trailing NUL: 11 + 1 + 4 + 1 = 17,
but the char array is 16 bytes long.

Nope, you are right. I was changing the format multiple times without
bothering much about that size. Thanks for the catch.

With that fixed feel free to add my "Reviewed-by".

I will fix it up and thanks for the review.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel