Message-ID: <475dd166-6590-4a76-b076-a878c784ae31@linaro.org>
Date: Mon, 19 Feb 2024 09:36:44 +0100
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Dmitry Rokosov <ddrokosov@...utedevices.com>
Cc: Viacheslav Bocharov <adeep@...ina.in>,
 Jerome Brunet <jbrunet@...libre.com>, Kevin Hilman <khilman@...libre.com>,
 Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
 linux-amlogic@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [DMARC error][DKIM error] [PATCH 0/5] soc: amlogic: add new
 meson-gx-socinfo-sm driver

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

Powered by blists - more mailing lists