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

From: Arnd Bergmann <arnd@arndb.de>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Francois Ozog <francois.ozog@linaro.org>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	harb@amperecomputing.com, Will Deacon <will@kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 2/2] firmware: smccc: Add ARCH_SOC_ID support
Date: Fri, 22 May 2020 16:46:12 +0200	[thread overview]
Message-ID: <CAK8P3a1t6BrB_Gti138VDRbmaiR_TjwR9d6qMstLBFDWxZ1kjQ@mail.gmail.com> (raw)
In-Reply-To: <20200522124951.35776-3-sudeep.holla@arm.com>

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

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