Re: [DMARC error][DKIM error] [PATCH 0/5] soc: amlogic: add new meson-gx-socinfo-sm driver

[Neil Armstrong]

On 16/02/2024 08:47, Dmitry Rokosov wrote:
> Hello Neil,
>
> May I put in my two cents on this patch series?
>
> There appears to be a misunderstanding regarding the terminology used.
> Allow me to clarify my perspective.
>
> The original Amlogic chipid has the following format:
>
>      4 bytes      12 bytes
>      +-------+-------------------+
>      |       |                   |
>      | CPUID | SOC SERIAL NUMBER |
>      |       |                   |
>      +-------+-------------------+
>      0                          15
>
>
> In the current uboot [1] and kernel [2] upstream, only the SOC SERIAL
> NUMBER bytes are read from efuse OTP storage (and it isn't swapped, as
> Amlogic reference code does [3]). We refer to these bytes as "serial".
>
> The original chipid value is utilized in several algorithms (for
> example, in the Amlogic boot protocols), making it crucial to have the
> ability to read the original chipid value as intended by the vendor.
>
> In my opinion, we should align our naming terminology as follows:
>      - "chipid" - Represents the complete Amlogic SoC ID, includes
>                   "cpuid" and "serial"
>      - "serial" - 12 byte unique SoC number, identifying particular die
>
> We strongly believe that this patch series is essential and are highly interested in seeing it applied to the upstream linux-amlogic, for the following reasons:
>      - We use chipid for device identification in our boards
>      - The Amlogic boot protocols utilize the full version of chipid
>        (ADNL, Optimus). As I mentioned in the IRC, we are preparing a
>        patch series for uboot incorporating them.
>      - in OPTEE: for generation of SSK (Secure Storage Key) [4]
>      - RPMB: for generation of RPMB key, further provisioned into RPMB
>        controller (thus particular SoC is bound to particular eMMC
>
> Therefore, I propose that we rename "soc_id" in the Viacheslav patch
> series to "chipid" and subsequently port this patch series to uboot.
>
> What are your thoughts on this matter?

I'm perfectly fine with that, but I don't like the shared functions, the only
shared stuff are the soc id tables, the shared functions is not important enough
to be shared.

Neil

> Links:
> [1] - https://elixir.bootlin.com/u-boot/v2024.01/source/arch/arm/mach-meson/sm.c#L84
> [2] - https://elixir.bootlin.com/linux/v6.7.4/source/drivers/firmware/meson/meson_sm.c#L268
> [3] - https://github.com/CoreELEC/u-boot/blob/3efc85a8370796bcec3bcadcdecec9aed973f4a9/arch/arm/mach-meson/g12a/bl31_apis.c#L398-L417
> [4] - https://github.com/OP-TEE/optee_os/blob/5df2a985b2ffd0b6f1107f12ca2a88203bf31328/core/tee/tee_fs_key_manager.c#L152
>
> On Wed, Nov 22, 2023 at 03:56:38PM +0300, Viacheslav Bocharov wrote:
> > The Amlogic Meson SoC Secure Monitor implements a call to retrieve an
> > unique SoC ID starting from the GX Family and all new families.
> > But GX-family chips (e.g. GXB, GXL and newer) supports also 128-bit
> > chip ID. 128-bit chip ID consists 32-bit SoC version and 96-bit OTP data.
> >
> > This is the second attempt to publish data from the Amlogic secure monitor
> > chipid call. After discussions with Neil Armstrong, it was decided to
> > publish the chipid call results through the soc driver. Since
> > soc_device_match cannot wait for the soc driver to load, and the secure
> > monitor calls in turn depend on the sm driver, it was necessary to create
> > a new driver rather than expand an existing one.
> >
> > In the patches, in addition to writing the driver:
> > - convert commonly used structures and functions of the meson-gx-socinfo
> > driver to a header file.
> > - transfer the chipid sm call constants to a header file (perhaps they
> > need renaming?).
> > - add secure-monitor references for amlogic,meson-gx-ao-secure sections
> > in dts files of the a1, axg, g12, gx families.
> >
> > Viacheslav Bocharov (5):
> >    soc: amlogic: meson-gx-socinfo: move common code to header file
> >    soc: amlogic: meson-gx-socinfo: move common code to exported function
> >    firmware: meson_sm: move common definitions to header file
> >    soc: amlogic: Add Amlogic secure-monitor SoC Information driver
> >    arm64: dts: meson: add dts links to secure-monitor for soc driver in
> >      a1, axg, gx, g12
> >
> >   arch/arm64/boot/dts/amlogic/meson-a1.dtsi     |   1 +
> >   arch/arm64/boot/dts/amlogic/meson-axg.dtsi    |   1 +
> >   .../boot/dts/amlogic/meson-g12-common.dtsi    |   1 +
> >   arch/arm64/boot/dts/amlogic/meson-gx.dtsi     |   1 +
> >   drivers/firmware/meson/meson_sm.c             |   4 -
> >   drivers/soc/amlogic/Kconfig                   |  10 +
> >   drivers/soc/amlogic/Makefile                  |   1 +
> >   .../soc/amlogic/meson-gx-socinfo-internal.h   | 102 ++++++++++
> >   drivers/soc/amlogic/meson-gx-socinfo-sm.c     | 178 ++++++++++++++++++
> >   drivers/soc/amlogic/meson-gx-socinfo.c        | 106 ++---------
> >   include/linux/firmware/meson/meson_sm.h       |   4 +
> >   11 files changed, 317 insertions(+), 92 deletions(-)
> >   create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-internal.h
> >   create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-sm.c
> >
> > --
> > 2.34.1
> >
> >
> > _______________________________________________
> > linux-amlogic mailing list
> > linux-amlogic@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-amlogic