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

[Sudeep Holla <sudeep.holla@arm.com>]

On Wed, Jun 17, 2020 at 02:34:41PM +0100, Steven Price wrote:
> On 17/06/2020 11:05, Sudeep Holla wrote:
> > 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 <etienne.carriere@st.com>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >   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/
> > 
> > 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.

[...]

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