[PATCH v2 2/3] rockchip: rk3588: Implement checkboard() to print SoC variant

[Quentin Schulz]

Hi Jagan,

On 11/8/24 9:55 AM, Jagan Teki wrote:
> On Fri, 8 Nov 2024 at 09:10, Kever Yang <kever.yang at rock-chips.com> wrote:
>>
>> Hi Jonas,
>>
>> On 2024/11/7 22:29, Jonas Karlman wrote:
>>> Hi Quentin,
>>>
>>> On 2024-11-07 13:01, Quentin Schulz wrote:
>>>> Hi Jonas,
>>>>
>>>> On 11/2/24 9:37 PM, Jonas Karlman wrote:
>>>>> Implement checkboard() to print current SoC model used by a board,
>>>>> e.g. one of:
>>>>>
>>>>>      SoC:   RK3582 v1
>>>>>      SoC:   RK3588 v0
>>>>>      SoC:   RK3588 v1
>>>>>      SoC:   RK3588S v0
>>>>>      SoC:   RK3588S v1
>>>>>      SoC:   RK3588S2 v1
>>>>>
>>>>> when U-Boot proper is running.
>>>>>
>>>>>      U-Boot 2025.01-rc1 (Nov 02 2024 - 20:19:01 +0000)
>>>>>
>>>>>      Model: Generic RK3588S/RK3588
>>>>>      SoC:   RK3588S2 v1
>>>>>      DRAM:  8 GiB
>>>>>
>>>>> Information about the SoC model, variant and version is read from OTP.
>>>>>
>>>>> Also update rk3588s-u-boot.dtsi to include OTP in U-Boot pre-reloc phase,
>>>>> where checkboard() is called.
>>>>>
>>>>> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
>>>>> ---
>>>>> v2:
>>>>> - Update commit message
>>>>> - Update code comments
>>>>> - Drop generic-rk3588_defconfig change
>>>>> ---
>>>>>     arch/arm/dts/rk3588s-u-boot.dtsi       |  4 ++
>>>>>     arch/arm/mach-rockchip/rk3588/rk3588.c | 58 ++++++++++++++++++++++++++
>>>>>     2 files changed, 62 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/dts/rk3588s-u-boot.dtsi b/arch/arm/dts/rk3588s-u-boot.dtsi
>>>>> index 09d8b311cec5..8880d162b11c 100644
>>>>> --- a/arch/arm/dts/rk3588s-u-boot.dtsi
>>>>> +++ b/arch/arm/dts/rk3588s-u-boot.dtsi
>>>>> @@ -69,6 +69,10 @@
>>>>>      bootph-all;
>>>>>     };
>>>>>
>>>>> +&otp {
>>>>> +   bootph-some-ram;
>>>>> +};
>>>>> +
>>>>>     &pcfg_pull_down {
>>>>>      bootph-all;
>>>>>     };
>>>>> diff --git a/arch/arm/mach-rockchip/rk3588/rk3588.c b/arch/arm/mach-rockchip/rk3588/rk3588.c
>>>>> index e2dac2a5b806..f9da7a6f1477 100644
>>>>> --- a/arch/arm/mach-rockchip/rk3588/rk3588.c
>>>>> +++ b/arch/arm/mach-rockchip/rk3588/rk3588.c
>>>>> @@ -4,6 +4,8 @@
>>>>>      * Copyright (c) 2022 Edgeble AI Technologies Pvt. Ltd.
>>>>>      */
>>>>>
>>>>> +#include <dm.h>
>>>>> +#include <misc.h>
>>>>>     #include <spl.h>
>>>>>     #include <asm/armv8/mmu.h>
>>>>>     #include <asm/arch-rockchip/bootrom.h>
>>>>> @@ -178,3 +180,59 @@ int arch_cpu_init(void)
>>>>>      return 0;
>>>>>     }
>>>>>     #endif
>>>>> +
>>>>> +#define RK3588_OTP_CPU_CODE_OFFSET         0x02
>>>>> +#define RK3588_OTP_SPECIFICATION_OFFSET            0x06
>>>>> +#define RK3588_OTP_CPU_VERSION_OFFSET              0x1c
>>>>> +
>>>>> +int checkboard(void)
>>>>> +{
>>>>> +   u8 cpu_code[2], specification, package, cpu_version;
>>>>> +   struct udevice *dev;
>>>>> +   char suffix[3];
>>>>> +   int ret;
>>>>> +
>>>>> +   ret = uclass_get_device_by_driver(UCLASS_MISC,
>>>>> +                                     DM_DRIVER_GET(rockchip_otp), &dev);
>>>>> +   if (ret) {
>>>>> +           debug("%s: could not find otp device, ret=%d\n", __func__, ret);
>>>> We should probably start using log_debug instead?
>>> I guess we should?, not sure why there is two different and what the
>>> difference is. Have typically only ever used debug() or printf(), so I
>>> just went with what was used in the code copied from board.c.
>>>
>>>> I guess with #define LOG_CATEGORY LOGC_ARCH
>>>>
>>>> at the top?
>>> I can give it a try, what/how is the log category used for?
>>>
>>> My simple debug workflow is just to add a '#define LOG_DEBUG' at top of
>>> a file when I want some more debug info.
>>>
>>>>> +           return 0;
>>>>> +   }
>>>>> +
>>>>> +   /* cpu-code: SoC model, e.g. 0x35 0x82 or 0x35 0x88 */
>>>>> +   ret = misc_read(dev, RK3588_OTP_CPU_CODE_OFFSET, cpu_code, 2);
>>>> This will fail if CONFIG_MISC=n which is neither selected nor implied
>>>> for RK3588.
>>> Good catch, did not test that scenario, will fix in a v3.
>>>
>>>> I tested on multiple RK3588 Jaguar, and the commercial (RK3588) grade
>>>> ones all showed RK3588 v0.
>>>>
>>>> The one industrial grade (RK3588J) showed RK3588J v1.
>>> Thanks for confirming this works on a J-variant.
>>>
>>>> I'm really not sure what this version number is for. If even Kever
>>>> doesn't know what it means, I am not sure it makes sense to print it?
>>> For rk356x there is some special handling for cpu-version <> 0 in vendor
>>> code, and DT describe a cpu-version nvmem cell, so thought it could be
>>> useful debug information. However it may just cause more confusion since
>>> there is no clear information on what the different cpu-version mean.
>>>
>>> Will drop the cpu-version information in a v3.
>>
>> Yes, this cpu-version does not help users, I think it may be one of the
>> silicon version(
>>
>> The process may change in foundry), package version(change in package
>> factory),
> 
> Apart from package number chip versions like RK3588, RK3588J and
> RK3588M would be much beneficial when we boot multi-dtb from SPL and
> load the respective silicon dtb in u-boot proper. So, chip numbers,
> temperature (if possible) are useful like imx chips supported so far.
> 

I mean... can't we just derive the operating temperature from the 
variant? It's pretty public what the operating range is for each SoC and 
its variants, so not sure what exactly you're after?

I think we agreed on keeping the printing of the variant in there, so 
that should be enough to derive the operating range?

We would probably need another mechanism to expose the variant to the 
rest of the code so it can be used by the SPL to load a multi-dtb proper 
(or at the very least have proper select the appropriate DTB from a 
multi-dtb kernel fitimage).

Cheers,
Quentin