Received-SPF: pass (google.com: domain of linux-kernel+bounces-68186-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 16 Feb 2024 10:47:16 +0300
From: Dmitry Rokosov <ddrokosov@salutedevices.com>
To: Neil Armstrong <neil.armstrong@linaro.org>
CC: Viacheslav Bocharov <adeep@lexina.in>, Jerome Brunet
	<jbrunet@baylibre.com>, Kevin Hilman <khilman@baylibre.com>, Martin
 Blumenstingl <martin.blumenstingl@googlemail.com>,
	<linux-amlogic@lists.infradead.org>, <linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>
Subject: Re: [DMARC error][DKIM error] [PATCH 0/5] soc: amlogic: add new
 meson-gx-socinfo-sm driver
Message-ID: <20240216074716.a33hrvl5xnpm7aep@CAB-WSD-L081021>
References: <20231122125643.1717160-1-adeep@lexina.in>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20231122125643.1717160-1-adeep@lexina.in>
User-Agent: NeoMutt/20220415

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?

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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

-- 
Thank you,
Dmitry