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

[Arnd Bergmann <arnd@arndb.de>]

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