[PATCH 0/2] base: soc: Add JEP106 manufacturer's identification code

[PATCH 0/2] base: soc: Add JEP106 manufacturer's identification code

[Sudeep Holla]

Hi,

While attempting to add custom sysfs attributes for SMCCC ARCH_SOC_ID
Arnd suggested to make it generic. Here is my first attempt.

The original thread/discussion can be found here[1]

Regards,
Sudeep

[1] https://lore.kernel.org/r/CAK8P3a3dV0B26XE3oFQGTFf8EWV0AHoLudNtpSSB_t+pCfkOkQ@mail.gmail.com/

Sudeep Holla (2):
  base: soc: Add JEDEC JEP106 manufacturer's identification code attribute
  firmware: smccc: Add ARCH_SOC_ID support

 Documentation/ABI/testing/sysfs-devices-soc |  31 ++++++
 drivers/base/soc.c                          |  12 +++
 drivers/firmware/smccc/Kconfig              |   9 ++
 drivers/firmware/smccc/Makefile             |   1 +
 drivers/firmware/smccc/soc_id.c             | 113 ++++++++++++++++++++
 include/linux/arm-smccc.h                   |   5 +
 include/linux/sys_soc.h                     |   1 +
 7 files changed, 172 insertions(+)
 create mode 100644 drivers/firmware/smccc/soc_id.c

-- 
2.17.1

[PATCH 1/2] base: soc: Add JEDEC JEP106 manufacturer's identification code attribute

[Sudeep Holla]

SMCCC v1.2 adds a new optional function SMCCC_ARCH_SOC_ID to obtain a
SiP defined SoC identification value. Indeed of making it custom
attribute, let us add the same as generic attribute to soc_device.

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.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 Documentation/ABI/testing/sysfs-devices-soc | 31 +++++++++++++++++++++
 drivers/base/soc.c                          | 12 ++++++++
 include/linux/sys_soc.h                     |  1 +
 3 files changed, 44 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-devices-soc b/Documentation/ABI/testing/sysfs-devices-soc
index ba3a3fac0ee1..fd44c9b1e09a 100644
--- a/Documentation/ABI/testing/sysfs-devices-soc
+++ b/Documentation/ABI/testing/sysfs-devices-soc
@@ -54,6 +54,37 @@ contact:	Lee Jones <lee.jones@linaro.org>
 		Read-only attribute supported ST-Ericsson's silicon. Contains the
 		the process by which the silicon chip was manufactured.
 
+What:		/sys/devices/socX/jep106_identification_code
+Date:		June 2020
+Contact:	Sudeep Holla <sudeep.holla@arm.com>
+Description:
+		Read-only attribute supported on many of ARM based silicon
+		with SMCCC v1.2+ compliant firmware. Contains the JEDEC
+		JEP106 manufacturer’s identification 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
+		as JEP106 identity code of 0x3B and a continuation code of
+		0x4 to represent the four continuation codes preceding the
+		identity code.
+
+		This property represents it in the shortened form:
+		8-bit continuation code followed by 8 bit identity code
+		without the parity bit.
+
 What:		/sys/bus/soc
 Date:		January 2012
 contact:	Lee Jones <lee.jones@linaro.org>
diff --git a/drivers/base/soc.c b/drivers/base/soc.c
index 4af11a423475..44dc757aadf4 100644
--- a/drivers/base/soc.c
+++ b/drivers/base/soc.c
@@ -36,6 +36,7 @@ static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
 static DEVICE_ATTR(serial_number, S_IRUGO, soc_info_get,  NULL);
 static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
 static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(jep106_identification_code, S_IRUGO, soc_info_get,  NULL);
 
 struct device *soc_device_to_device(struct soc_device *soc_dev)
 {
@@ -64,6 +65,9 @@ static umode_t soc_attribute_mode(struct kobject *kobj,
 	if ((attr == &dev_attr_soc_id.attr)
 	    && (soc_dev->attr->soc_id != NULL))
 		return attr->mode;
+	if ((attr == &dev_attr_jep106_identification_code.attr)
+	    && (soc_dev->attr->jep106_id != NULL))
+		return attr->mode;
 
 	/* Unknown or unfilled attribute. */
 	return 0;
@@ -85,6 +89,8 @@ static ssize_t soc_info_get(struct device *dev,
 		return sprintf(buf, "%s\n", soc_dev->attr->serial_number);
 	if (attr == &dev_attr_soc_id)
 		return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
+	if (attr == &dev_attr_jep106_identification_code)
+		return sprintf(buf, "%s\n", soc_dev->attr->jep106_id);
 
 	return -EINVAL;
 
@@ -96,6 +102,7 @@ static struct attribute *soc_attr[] = {
 	&dev_attr_serial_number.attr,
 	&dev_attr_soc_id.attr,
 	&dev_attr_revision.attr,
+	&dev_attr_jep106_identification_code.attr,
 	NULL,
 };
 
@@ -214,6 +221,11 @@ static int soc_device_match_attr(const struct soc_device_attribute *attr,
 	    (!attr->soc_id || !glob_match(match->soc_id, attr->soc_id)))
 		return 0;
 
+	if (match->jep106_id &&
+	    (!attr->jep106_id ||
+	     !glob_match(match->jep106_id, attr->jep106_id)))
+		return 0;
+
 	return 1;
 }
 
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
index d9b3cf0f410c..394fa70ae16f 100644
--- a/include/linux/sys_soc.h
+++ b/include/linux/sys_soc.h
@@ -14,6 +14,7 @@ struct soc_device_attribute {
 	const char *revision;
 	const char *serial_number;
 	const char *soc_id;
+	const char *jep106_id;
 	const void *data;
 	const struct attribute_group *custom_attr_group;
 };
-- 
2.17.1

[PATCH 2/2] firmware: smccc: Add ARCH_SOC_ID support

[Sudeep Holla]

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.

Reviewed-by: Steven Price <steven.price@arm.com>
Acked-by: Etienne Carriere <etienne.carriere@st.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/smccc/Kconfig  |   9 +++
 drivers/firmware/smccc/Makefile |   1 +
 drivers/firmware/smccc/soc_id.c | 113 ++++++++++++++++++++++++++++++++
 include/linux/arm-smccc.h       |   5 ++
 4 files changed, 128 insertions(+)
 create mode 100644 drivers/firmware/smccc/soc_id.c

diff --git a/drivers/firmware/smccc/Kconfig b/drivers/firmware/smccc/Kconfig
index 27b675d76235..15e7466179a6 100644
--- a/drivers/firmware/smccc/Kconfig
+++ b/drivers/firmware/smccc/Kconfig
@@ -14,3 +14,12 @@ config HAVE_ARM_SMCCC_DISCOVERY
 	 to add SMCCC discovery mechanism though the PSCI firmware
 	 implementation of PSCI_FEATURES(SMCCC_VERSION) which returns
 	 success on firmware compliant to SMCCC v1.1 and above.
+
+config ARM_SMCCC_SOC_ID
+	bool "SoC bus device for the ARM SMCCC SOC_ID"
+	depends on HAVE_ARM_SMCCC_DISCOVERY
+	default y
+	select SOC_BUS
+	help
+	  Include support for the SoC bus on the ARM SMCCC firmware based
+	  platforms providing some sysfs information about the SoC variant.
diff --git a/drivers/firmware/smccc/Makefile b/drivers/firmware/smccc/Makefile
index 6f369fe3f0b9..72ab84042832 100644
--- a/drivers/firmware/smccc/Makefile
+++ b/drivers/firmware/smccc/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 #
 obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)	+= smccc.o
+obj-$(CONFIG_ARM_SMCCC_SOC_ID)	+= soc_id.o
diff --git a/drivers/firmware/smccc/soc_id.c b/drivers/firmware/smccc/soc_id.c
new file mode 100644
index 000000000000..437175a589e2
--- /dev/null
+++ b/drivers/firmware/smccc/soc_id.c
@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2020 Arm Limited
+ */
+
+#define pr_fmt(fmt) "SMCCC: SOC_ID: " fmt
+
+#include <linux/arm-smccc.h>
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/sys_soc.h>
+
+#define SMCCC_SOC_ID_JEP106_BANK_IDX_MASK	GENMASK(30, 24)
+/*
+ * As per the SMC Calling Convention specification v1.2 (ARM DEN 0028C)
+ * Section 7.4 SMCCC_ARCH_SOC_ID bits[23:16] are JEP-106 identification
+ * code with parity bit for the SiP. We can drop the parity bit.
+ */
+#define SMCCC_SOC_ID_JEP106_ID_CODE_MASK	GENMASK(22, 16)
+#define SMCCC_SOC_ID_IMP_DEF_SOC_ID_MASK	GENMASK(15, 0)
+
+#define JEP106_BANK_CONT_CODE(x)	\
+	(u8)(FIELD_GET(SMCCC_SOC_ID_JEP106_BANK_IDX_MASK, (x)))
+#define JEP106_ID_CODE(x)	\
+	(u8)(FIELD_GET(SMCCC_SOC_ID_JEP106_ID_CODE_MASK, (x)))
+#define IMP_DEF_SOC_ID(x)	\
+	(u16)(FIELD_GET(SMCCC_SOC_ID_IMP_DEF_SOC_ID_MASK, (x)))
+
+static struct soc_device *soc_dev;
+static struct soc_device_attribute *soc_dev_attr;
+
+static int __init smccc_soc_init(void)
+{
+	struct arm_smccc_res res;
+	int soc_id_rev, soc_id_version;
+	static char soc_id_str[8], soc_id_rev_str[12];
+	static char soc_id_jep106_id_str[8];
+
+	if (arm_smccc_get_version() < ARM_SMCCC_VERSION_1_2)
+		return 0;
+
+	if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE) {
+		pr_err("%s: invalid SMCCC conduit\n", __func__);
+		return -EOPNOTSUPP;
+	}
+
+	arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
+			     ARM_SMCCC_ARCH_SOC_ID, &res);
+
+	if (res.a0 == SMCCC_RET_NOT_SUPPORTED) {
+		pr_info("ARCH_SOC_ID not implemented, skipping ....\n");
+		return 0;
+	}
+
+	if ((int)res.a0 < 0) {
+		pr_info("ARCH_FEATURES(ARCH_SOC_ID) returned error: %lx\n",
+			res.a0);
+		return -EINVAL;
+	}
+
+        arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 0, &res);
+        if ((int)res.a0 < 0) {
+                pr_err("ARCH_SOC_ID(0) returned error: %lx\n", res.a0);
+                return -EINVAL;
+        }
+
+	soc_id_version = res.a0;
+
+        arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 1, &res);
+        if ((int)res.a0 < 0) {
+                pr_err("ARCH_SOC_ID(1) returned error: %lx\n", res.a0);
+                return -EINVAL;
+        }
+
+	soc_id_rev = res.a0;
+
+	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
+	if (!soc_dev_attr)
+		return -ENOMEM;
+
+	sprintf(soc_id_str, "0x%04x", IMP_DEF_SOC_ID(soc_id_version));
+	sprintf(soc_id_rev_str, "0x%08x", soc_id_rev);
+	sprintf(soc_id_jep106_id_str, "0x%02x%02x",
+		JEP106_BANK_CONT_CODE(soc_id_version),
+		JEP106_ID_CODE(soc_id_version));
+
+	soc_dev_attr->soc_id = soc_id_str;
+	soc_dev_attr->revision = soc_id_rev_str;
+	soc_dev_attr->jep106_id = soc_id_jep106_id_str;
+
+	soc_dev = soc_device_register(soc_dev_attr);
+	if (IS_ERR(soc_dev)) {
+		kfree(soc_dev_attr);
+		return PTR_ERR(soc_dev);
+	}
+
+	pr_info("ID = %s Revision = %s\n", soc_dev_attr->soc_id,
+		soc_dev_attr->revision);
+
+	return 0;
+}
+module_init(smccc_soc_init);
+
+static void __exit smccc_soc_exit(void)
+{
+	if (soc_dev)
+		soc_device_unregister(soc_dev);
+	kfree(soc_dev_attr);
+}
+module_exit(smccc_soc_exit);
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 56d6a5c6e353..8254e11ea857 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -71,6 +71,11 @@
 			   ARM_SMCCC_SMC_32,				\
 			   0, 1)
 
+#define ARM_SMCCC_ARCH_SOC_ID						\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   0, 2)
+
 #define ARM_SMCCC_ARCH_WORKAROUND_1					\
 	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
 			   ARM_SMCCC_SMC_32,				\
-- 
2.17.1

Re: [PATCH 2/2] firmware: smccc: Add ARCH_SOC_ID support

[Arnd Bergmann]

On Fri, May 22, 2020 at 2:50 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> +
> +       soc_id_rev = res.a0;
> +
> +       soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> +       if (!soc_dev_attr)
> +               return -ENOMEM;
> +
> +       sprintf(soc_id_str, "0x%04x", IMP_DEF_SOC_ID(soc_id_version));
> +       sprintf(soc_id_rev_str, "0x%08x", soc_id_rev);
> +       sprintf(soc_id_jep106_id_str, "0x%02x%02x",
> +               JEP106_BANK_CONT_CODE(soc_id_version),
> +               JEP106_ID_CODE(soc_id_version));
> +
> +       soc_dev_attr->soc_id = soc_id_str;
> +       soc_dev_attr->revision = soc_id_rev_str;
> +       soc_dev_attr->jep106_id = soc_id_jep106_id_str;

Ok, let me try to understand how this maps the 64-bit ID into the
six strings in user space:

For a chip that identifies as

JEP106_BANK_CONT_CODE = 12
JEP106_ID_CODE = 34
IMP_DEF_SOC_ID = 5678
soc_id_rev = 9abcdef0

the normal sysfs attributes contain these strings:

machine = ""
family = ""
revision = "0x9abcdef0
serial_number = ""
soc_id = "0x5678"

and the new attribute is

jep106_identification_code = "0x1234"

This still looks like a rather poorly designed interface to me, with a
number of downsides:

- Nothing in those strings identifies the numbers as using jep106
  numbers rather than some something else that might use strings
  with hexadecimal numbers.

- I think we should have something unique in "family" just because
  existing scripts can use that as the primary indentifier

- It seems odd that there is no way to read the serial number through
  the same interface and publish it the usual way. Francois Ozog
  recently asked for a generic way to find out a serial number for
  inventory management, and this would be the obvious place to have it.
  It can of course be added later when the next revision of the spec
  is there, it just seems like a surprising omission.

How about making the contents:

machine = "" /* could be a future addition, but board specific */
family = "jep106:1234"
revision = "0x9abcdef0
serial_number = "0xfedcba987654321" /* to be implemented later */
soc_id = "jep106:1234:5678" /* duplicates family but makes it unique*/

That would work without any new properties, dropping the other patch,
and be easier to use for identification from user space.

    Arnd

Re: [PATCH 2/2] firmware: smccc: Add ARCH_SOC_ID support

[Sudeep Holla]

(+ Jose (SMCCC Spec author))

On Fri, May 22, 2020 at 04:46:12PM +0200, Arnd Bergmann wrote:
> On Fri, May 22, 2020 at 2:50 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > +
> > +       soc_id_rev = res.a0;
> > +
> > +       soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> > +       if (!soc_dev_attr)
> > +               return -ENOMEM;
> > +
> > +       sprintf(soc_id_str, "0x%04x", IMP_DEF_SOC_ID(soc_id_version));
> > +       sprintf(soc_id_rev_str, "0x%08x", soc_id_rev);
> > +       sprintf(soc_id_jep106_id_str, "0x%02x%02x",
> > +               JEP106_BANK_CONT_CODE(soc_id_version),
> > +               JEP106_ID_CODE(soc_id_version));
> > +
> > +       soc_dev_attr->soc_id = soc_id_str;
> > +       soc_dev_attr->revision = soc_id_rev_str;
> > +       soc_dev_attr->jep106_id = soc_id_jep106_id_str;
>
> Ok, let me try to understand how this maps the 64-bit ID into the
> six strings in user space:
>
> For a chip that identifies as
>
> JEP106_BANK_CONT_CODE = 12
> JEP106_ID_CODE = 34
> IMP_DEF_SOC_ID = 5678
> soc_id_rev = 9abcdef0
>
> the normal sysfs attributes contain these strings:
>
> machine = ""
> family = ""
> revision = "0x9abcdef0
> serial_number = ""
> soc_id = "0x5678"
>
> and the new attribute is
>
> jep106_identification_code = "0x1234"
>
> This still looks like a rather poorly designed interface to me, with a
> number of downsides:
>
> - Nothing in those strings identifies the numbers as using jep106
>   numbers rather than some something else that might use strings
>   with hexadecimal numbers.
>

Not sure if I understand your concerns completely here.

Anyways I wanted to clarify that the jep106 encoding is applicable only
for manufacturer's id and not for SoC ID or revision. Not sure if that
changes anything about your concerns.

> - I think we should have something unique in "family" just because
>   existing scripts can use that as the primary indentifier
>

I agree with your idea of combining attributes, not sure exactly which
ones yet.

> - It seems odd that there is no way to read the serial number through
>   the same interface and publish it the usual way.

Valid concern and I will pass this to interface authors.

>   Francois Ozog
>   recently asked for a generic way to find out a serial number for
>   inventory management, and this would be the obvious place to have it.

Agreed, but not sure what author(s) have to say. I have cc-ed one of them.

>   It can of course be added later when the next revision of the spec
>   is there, it just seems like a surprising omission.
>

Yes, definitely. Good to get feedback.

> How about making the contents:
>
> machine = "" /* could be a future addition, but board specific */
> family = "jep106:1234"

But this just indicates manufacturer id and nothing related to SoC family.
If it is jep106:043b, all it indicates is Arm Ltd and assigning it to
family doesn't sound right to me.

I had requests for both of the above during the design of interface but
I was told vendors were happy with the interface. I will let the authors
speak about that.

> revision = "0x9abcdef0
> serial_number = "0xfedcba987654321" /* to be implemented later */

Sure.

> soc_id = "jep106:1234:5678" /* duplicates family but makes it unique*/

Not sure again.
>
> That would work without any new properties, dropping the other patch,
> and be easier to use for identification from user space.
>

OK, I agree on ease part. But for me, we don't have any property in the
list to indicate the vendor/manufacturer's name. I don't see issue adding
one, name can be fixed as jep106_identification_code is too specific.

How about manufacturer with the value in the format "jep106:1234" if
it is not normal string but jep106 encoding.

--
Regards,
Sudeep

Re: [PATCH 2/2] firmware: smccc: Add ARCH_SOC_ID support

[Sudeep Holla]

On Fri, May 22, 2020 at 05:54:22PM +0100, Sudeep Holla wrote:
> (+ Jose (SMCCC Spec author))
> 
> On Fri, May 22, 2020 at 04:46:12PM +0200, Arnd Bergmann wrote:
> > On Fri, May 22, 2020 at 2:50 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > +
> > > +       soc_id_rev = res.a0;
> > > +
> > > +       soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> > > +       if (!soc_dev_attr)
> > > +               return -ENOMEM;
> > > +
> > > +       sprintf(soc_id_str, "0x%04x", IMP_DEF_SOC_ID(soc_id_version));
> > > +       sprintf(soc_id_rev_str, "0x%08x", soc_id_rev);
> > > +       sprintf(soc_id_jep106_id_str, "0x%02x%02x",
> > > +               JEP106_BANK_CONT_CODE(soc_id_version),
> > > +               JEP106_ID_CODE(soc_id_version));
> > > +
> > > +       soc_dev_attr->soc_id = soc_id_str;
> > > +       soc_dev_attr->revision = soc_id_rev_str;
> > > +       soc_dev_attr->jep106_id = soc_id_jep106_id_str;
> >
> > Ok, let me try to understand how this maps the 64-bit ID into the
> > six strings in user space:
> >
> > For a chip that identifies as
> >
> > JEP106_BANK_CONT_CODE = 12
> > JEP106_ID_CODE = 34
> > IMP_DEF_SOC_ID = 5678
> > soc_id_rev = 9abcdef0
> >
> > the normal sysfs attributes contain these strings:
> >
> > machine = ""
> > family = ""
> > revision = "0x9abcdef0
> > serial_number = ""
> > soc_id = "0x5678"
> >
> > and the new attribute is
> >
> > jep106_identification_code = "0x1234"
> >
> > This still looks like a rather poorly designed interface to me, with a
> > number of downsides:
> >
> > - Nothing in those strings identifies the numbers as using jep106
> >   numbers rather than some something else that might use strings
> >   with hexadecimal numbers.
> >
> 
> Not sure if I understand your concerns completely here.
> 
> Anyways I wanted to clarify that the jep106 encoding is applicable only
> for manufacturer's id and not for SoC ID or revision. Not sure if that
> changes anything about your concerns.
> 
> > - I think we should have something unique in "family" just because
> >   existing scripts can use that as the primary indentifier
> >
> 
> I agree with your idea of combining attributes, not sure exactly which
> ones yet.
> 
> > - It seems odd that there is no way to read the serial number through
> >   the same interface and publish it the usual way.
> 
> Valid concern and I will pass this to interface authors.
> 
> >   Francois Ozog
> >   recently asked for a generic way to find out a serial number for
> >   inventory management, and this would be the obvious place to have it.
> 
> Agreed, but not sure what author(s) have to say. I have cc-ed one of them.
> 
> >   It can of course be added later when the next revision of the spec
> >   is there, it just seems like a surprising omission.
> >
> 
> Yes, definitely. Good to get feedback.
> 
> > How about making the contents:
> >
> > machine = "" /* could be a future addition, but board specific */
[...]
> > serial_number = "0xfedcba987654321" /* to be implemented later */

OK more thoughts and few points I missed earlier. The SoC infrastructure
is more wider than the SoC itself IMO. The machine and serial_number
are beyond the SoC and this SMCCC SOC_ID confined itself to SoC for
simple reason that it is part of SoC vendor firmware which could be
used across various machines/boards/platforms.

I wonder now that rather than adding this as a SoC sysfs driver on it's
own, does it make sense to keep it as library which existing drivers
can use. Or we need to standardise the machine level interface that may
like outside SoC firmware and use both to have a generic SoC sysfs driver.

Sorry, I am just writing as I am thinking and may be talking garbage.

-- 
Regards,
Sudeep

Re: [PATCH 2/2] firmware: smccc: Add ARCH_SOC_ID support

[Arnd Bergmann]

On Fri, May 22, 2020 at 6:54 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> (+ Jose (SMCCC Spec author))
>
> On Fri, May 22, 2020 at 04:46:12PM +0200, Arnd Bergmann wrote:
> > On Fri, May 22, 2020 at 2:50 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > +
> > > +       soc_id_rev = res.a0;
> > > +
> > > +       soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> > > +       if (!soc_dev_attr)
> > > +               return -ENOMEM;
> > > +
> > > +       sprintf(soc_id_str, "0x%04x", IMP_DEF_SOC_ID(soc_id_version));
> > > +       sprintf(soc_id_rev_str, "0x%08x", soc_id_rev);
> > > +       sprintf(soc_id_jep106_id_str, "0x%02x%02x",
> > > +               JEP106_BANK_CONT_CODE(soc_id_version),
> > > +               JEP106_ID_CODE(soc_id_version));
> > > +
> > > +       soc_dev_attr->soc_id = soc_id_str;
> > > +       soc_dev_attr->revision = soc_id_rev_str;
> > > +       soc_dev_attr->jep106_id = soc_id_jep106_id_str;
> >
> > Ok, let me try to understand how this maps the 64-bit ID into the
> > six strings in user space:
> >
> > For a chip that identifies as
> >
> > JEP106_BANK_CONT_CODE = 12
> > JEP106_ID_CODE = 34
> > IMP_DEF_SOC_ID = 5678
> > soc_id_rev = 9abcdef0
> >
> > the normal sysfs attributes contain these strings:
> >
> > machine = ""
> > family = ""
> > revision = "0x9abcdef0
> > serial_number = ""
> > soc_id = "0x5678"
> >
> > and the new attribute is
> >
> > jep106_identification_code = "0x1234"
> >
> > This still looks like a rather poorly designed interface to me, with a
> > number of downsides:
> >
> > - Nothing in those strings identifies the numbers as using jep106
> >   numbers rather than some something else that might use strings
> >   with hexadecimal numbers.
> >
>
> Not sure if I understand your concerns completely here.
>
> Anyways I wanted to clarify that the jep106 encoding is applicable only
> for manufacturer's id and not for SoC ID or revision. Not sure if that
> changes anything about your concerns.

The problem I see is that by looking at just the existing attributes,
you have no way of telling what namespace the strings are in,
and a script that tries pattern matching could confuse two
hexadecimal numbers from a different namespace, such as
pci vendor/device or usb vendor/device IDs that are similar
in spirit to the jep106 codes.

> > - I think we should have something unique in "family" just because
> >   existing scripts can use that as the primary indentifier

This is part of the same issue: If we put just "jep106 identified SoC"
as the "family", it would be something a driver could match against.

> > How about making the contents:
> >
> > machine = "" /* could be a future addition, but board specific */
> > family = "jep106:1234"
>
> But this just indicates manufacturer id and nothing related to SoC family.
> If it is jep106:043b, all it indicates is Arm Ltd and assigning it to
> family doesn't sound right to me.
>
> I had requests for both of the above during the design of interface but
> I was told vendors were happy with the interface. I will let the authors
> speak about that.

In most cases, the existing drivers put a hardcoded string into the
family, such as

"Samsung Exynos"
"Freescale i.MX"
"Amlogic Meson"

or slightly more specific

"R-Car Gen2"

Having a numeric identifier for the SoC manufacturer here is a
bit more coarse than that, but would be similar in spirit.

> > soc_id = "jep106:1234:5678" /* duplicates family but makes it unique*/
>
> Not sure again.

> > That would work without any new properties, dropping the other patch,
> > and be easier to use for identification from user space.
> >
>
> OK, I agree on ease part. But for me, we don't have any property in the
> list to indicate the vendor/manufacturer's name. I don't see issue adding
> one, name can be fixed as jep106_identification_code is too specific.
>
> How about manufacturer with the value in the format "jep106:1234" if
> it is not normal string but jep106 encoding.

I don't think we need a real name like "Arm" or "Samsung" here,
but just a number is not enough, it should at least be something
that can be assumed to never conflict with the name of a chip
by another scheme.

jep106:5678 (the IMP_DEF_SOC_ID field in my example) would
probably be sufficient to not conflict with a another soc_device
driver, but is quite likely to clash with an ID used by another
manufacturer.

jep106:1234 (the manufacturer ID) in turn seems too broad from
the soc_id field, as that would include every chip made by one
company.

     Arnd

Re: [PATCH 2/2] firmware: smccc: Add ARCH_SOC_ID support

[Sudeep Holla]

On Fri, May 22, 2020 at 08:41:59PM +0200, Arnd Bergmann wrote:
> On Fri, May 22, 2020 at 6:54 PM Sudeep Holla <sudeep.holla@arm.com> wrote:

[...]

> >
> > Not sure if I understand your concerns completely here.
> >
> > Anyways I wanted to clarify that the jep106 encoding is applicable only
> > for manufacturer's id and not for SoC ID or revision. Not sure if that
> > changes anything about your concerns.
>
> The problem I see is that by looking at just the existing attributes,
> you have no way of telling what namespace the strings are in,
> and a script that tries pattern matching could confuse two
> hexadecimal numbers from a different namespace, such as
> pci vendor/device or usb vendor/device IDs that are similar
> in spirit to the jep106 codes.
>

Ah OK, understood.

> > > - I think we should have something unique in "family" just because
> > >   existing scripts can use that as the primary indentifier
>
> This is part of the same issue: If we put just "jep106 identified SoC"
> as the "family", it would be something a driver could match against.
>

I understand that and that's the reason I was introducing new field as
manufacture but I now see there is no point in adding that.

[...]

> > But this just indicates manufacturer id and nothing related to SoC family.
> > If it is jep106:043b, all it indicates is Arm Ltd and assigning it to
> > family doesn't sound right to me.
> >
> > I had requests for both of the above during the design of interface but
> > I was told vendors were happy with the interface. I will let the authors
> > speak about that.
>
> In most cases, the existing drivers put a hardcoded string into the
> family, such as
>
> "Samsung Exynos"
> "Freescale i.MX"
> "Amlogic Meson"
>
> or slightly more specific
>
> "R-Car Gen2"
>

Hmmm....

> Having a numeric identifier for the SoC manufacturer here is a
> bit more coarse than that, but would be similar in spirit.
>

Agreed.


[..]

> >
> > OK, I agree on ease part. But for me, we don't have any property in the
> > list to indicate the vendor/manufacturer's name. I don't see issue adding
> > one, name can be fixed as jep106_identification_code is too specific.
> >
> > How about manufacturer with the value in the format "jep106:1234" if
> > it is not normal string but jep106 encoding.
>
> I don't think we need a real name like "Arm" or "Samsung" here,
> but just a number is not enough, it should at least be something
> that can be assumed to never conflict with the name of a chip
> by another scheme.
>

Fair enough.

> jep106:5678 (the IMP_DEF_SOC_ID field in my example) would
> probably be sufficient to not conflict with a another soc_device
> driver, but is quite likely to clash with an ID used by another
> manufacturer.
>

IIUC, you are fine with "jep106:1234:5678" where 1234 is jep106 manufacture
id code and 5678 is soc_id as it may avoid all the conflicts across
the manufacture namespaces.

> jep106:1234 (the manufacturer ID) in turn seems too broad from
> the soc_id field, as that would include every chip made by one
> company.
>

I understand, and IIUC you prefer this to be embedded with soc_id
especially to avoid namespace conflicts which makes sense.

Thanks for all the discussion and valuable inputs. I am now bit nervous
to add SoC info from SMCCC ARCH_SOC_ID to sysfs yet as we need more info
especially "machine" and "serial_number" for elsewhere(OEM firmware mostly
from DT or ACPI).


TBH, the mix might be bit of a mess as there are soc drivers that rely
on DT completely today. Anyways, this SOC_ID can be added as library that
can be used by a *generic* soc driver once we define a standard way to
fetch other information("machine" and "serial_number"). I am happy to
get suggestions on that front especially from you and Francois as you
have got some context already.

--
Regards,
Sudeep

Re: [PATCH 2/2] firmware: smccc: Add ARCH_SOC_ID support

[Arnd Bergmann]

On Sat, May 23, 2020 at 7:27 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> On Fri, May 22, 2020 at 08:41:59PM +0200, Arnd Bergmann wrote:
> > On Fri, May 22, 2020 at 6:54 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> > jep106:5678 (the IMP_DEF_SOC_ID field in my example) would
> > probably be sufficient to not conflict with a another soc_device
> > driver, but is quite likely to clash with an ID used by another
> > manufacturer.
> >
>
> IIUC, you are fine with "jep106:1234:5678" where 1234 is jep106 manufacture
> id code and 5678 is soc_id as it may avoid all the conflicts across
> the manufacture namespaces.

I think either jep106:5678 or jep106:1234:5678 (or some variation with
different field separators if you prefer) would be fine here.

> > jep106:1234 (the manufacturer ID) in turn seems too broad from
> > the soc_id field, as that would include every chip made by one
> > company.
> >
>
> I understand, and IIUC you prefer this to be embedded with soc_id
> especially to avoid namespace conflicts which makes sense.
>
> Thanks for all the discussion and valuable inputs. I am now bit nervous
> to add SoC info from SMCCC ARCH_SOC_ID to sysfs yet as we need more info
> especially "machine" and "serial_number" for elsewhere(OEM firmware mostly
> from DT or ACPI).

I probably wouldn't mix those in with the same driver. If machine and
serial_number have no smccc interface, then they should be left out,
but there could be a separate soc_device that gets that information
by other means, usually using one of the existing hardware id register
drivers.

> TBH, the mix might be bit of a mess as there are soc drivers that rely
> on DT completely today. Anyways, this SOC_ID can be added as library that
> can be used by a *generic* soc driver once we define a standard way to
> fetch other information("machine" and "serial_number"). I am happy to
> get suggestions on that front especially from you and Francois as you
> have got some context already.

Well, I suppose we could have the entire data from the smccc interface
in a central place somewhere, such as (to stay with my example)
"1234:5678:9abcdef0" in an attribute of any soc_device driver that
adds a call to a library function for the jep106 ID, or possibly in
/sys/firmware or even a field added to /proc/cpuinfo.

       Arnd

RE: [PATCH 2/2] firmware: smccc: Add ARCH_SOC_ID support

[Jose Marinho]

> On Sat, May 23, 2020 at 7:27 PM Sudeep Holla <sudeep.holla@arm.com>
> wrote:
> > On Fri, May 22, 2020 at 08:41:59PM +0200, Arnd Bergmann wrote:
> > > On Fri, May 22, 2020 at 6:54 PM Sudeep Holla <sudeep.holla@arm.com>
> wrote:
> >
> > > jep106:5678 (the IMP_DEF_SOC_ID field in my example) would probably
> > > be sufficient to not conflict with a another soc_device driver, but
> > > is quite likely to clash with an ID used by another manufacturer.
> > >
> >
> > IIUC, you are fine with "jep106:1234:5678" where 1234 is jep106
> manufacture
> > id code and 5678 is soc_id as it may avoid all the conflicts across
> > the manufacture namespaces.
> 
> I think either jep106:5678 or jep106:1234:5678 (or some variation with
> different field separators if you prefer) would be fine here.
> 
> > > jep106:1234 (the manufacturer ID) in turn seems too broad from
> > > the soc_id field, as that would include every chip made by one
> > > company.
> > >
> >
> > I understand, and IIUC you prefer this to be embedded with soc_id
> > especially to avoid namespace conflicts which makes sense.
> >
> > Thanks for all the discussion and valuable inputs. I am now bit nervous
> > to add SoC info from SMCCC ARCH_SOC_ID to sysfs yet as we need more
> info
> > especially "machine" and "serial_number" for elsewhere(OEM firmware
> mostly
> > from DT or ACPI).
> 
> I probably wouldn't mix those in with the same driver. If machine and
> serial_number have no smccc interface, then they should be left out,
> but there could be a separate soc_device that gets that information
> by other means, usually using one of the existing hardware id register
> drivers.
> 
> > TBH, the mix might be bit of a mess as there are soc drivers that rely
> > on DT completely today. Anyways, this SOC_ID can be added as library that
> > can be used by a *generic* soc driver once we define a standard way to
> > fetch other information("machine" and "serial_number"). I am happy to
> > get suggestions on that front especially from you and Francois as you
> > have got some context already.
> 
> Well, I suppose we could have the entire data from the smccc interface
> in a central place somewhere, such as (to stay with my example)
> "1234:5678:9abcdef0" in an attribute of any soc_device driver that
> adds a call to a library function for the jep106 ID, or possibly in
> /sys/firmware or even a field added to /proc/cpuinfo.

I think this is a great way to expose the SoC ID info. It's important to have the SoC ID as a whole in a sysfs (or somewhere where it's easy to obtain and parse from user-space).
The information provided by SoC ID should be listed in this form jep106:1234:5678, that is jep106:<manufacturer ID>:<SoC ID> .

Regards,

Jose