Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200522124951.35776-1-sudeep.holla@arm.com> <20200522124951.35776-3-sudeep.holla@arm.com>
In-Reply-To: <20200522124951.35776-3-sudeep.holla@arm.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Fri, 22 May 2020 16:46:12 +0200
Message-ID: <CAK8P3a1t6BrB_Gti138VDRbmaiR_TjwR9d6qMstLBFDWxZ1kjQ@mail.gmail.com>
Subject: Re: [PATCH 2/2] firmware: smccc: Add ARCH_SOC_ID support
To:     Sudeep Holla <sudeep.holla@arm.com>
Cc:     Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Will Deacon <will@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        harb@amperecomputing.com,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Francois Ozog <francois.ozog@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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