Re: [PATCH 2/3] rockchip: Require an external TPL binary when TPL is missing

[Kever Yang <kever.yang@rock-chips.com>]

On 2023/2/9 00:06, Quentin Schulz wrote:
> Hi Kever,
>
> On 2/8/23 16:41, Kever Yang wrote:
>> Hi Quentin,
>>
>> On 2023/2/6 19:26, Quentin Schulz wrote:
>>> Hi Jonas,
>>>
>>> On 2/5/23 21:21, Jonas Karlman wrote:
>>>> Rockchip SoCs typically use U-Boot TPL to initialize DRAM, then jumps
>>>> back to boot-rom to load the next stage of the boot flow, U-Boot SPL.
>>>>
>>>> For RK356x there is currently no support to initialize DRAM using 
>>>> U-Boot
>>>> TPL and instead an external TPL binary must be used to generate a
>>>> working u-boot-rockchip.bin image.
>>>>
>>>> Use the new external-tpl entry unless CONFIG_TPL=y to indicate that an
>>>> external TPL binary must be provided to generate a working firmware.
>>>>
>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>> ---
>>>>   Makefile                          |  1 +
>>>>   arch/arm/dts/rockchip-u-boot.dtsi | 16 ++++++++++++----
>>>>   tools/binman/missing-blob-help    |  5 +++++
>>>>   3 files changed, 18 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Makefile b/Makefile
>>>> index 7eaf45496c1c..7e9272be937f 100644
>>>> --- a/Makefile
>>>> +++ b/Makefile
>>>> @@ -1332,6 +1332,7 @@ cmd_binman = $(srctree)/tools/binman/binman 
>>>> $(if $(BINMAN_DEBUG),-D) \
>>>>           -a opensbi-path=${OPENSBI} \
>>>>           -a default-dt=$(default_dt) \
>>>>           -a scp-path=$(SCP) \
>>>> +        -a external-tpl-path=$(EXTERNAL_TPL) \
>>>>           -a spl-bss-pad=$(if $(CONFIG_SPL_SEPARATE_BSS),,1) \
>>>>           -a tpl-bss-pad=$(if $(CONFIG_TPL_SEPARATE_BSS),,1) \
>>>>           -a spl-dtb=$(CONFIG_SPL_OF_REAL) \
>>>> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi 
>>>> b/arch/arm/dts/rockchip-u-boot.dtsi
>>>> index 6c662a72d4f9..bc3bc9bc3e37 100644
>>>> --- a/arch/arm/dts/rockchip-u-boot.dtsi
>>>> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
>>>> @@ -20,12 +20,16 @@
>>>>           mkimage {
>>>>               filename = "idbloader.img";
>>>>               args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
>>>> -#ifdef CONFIG_TPL
>>>>               multiple-data-files;
>>>>   +#ifdef CONFIG_TPL
>>>>               u-boot-tpl {
>>>> -            };
>>>> +#else
>>>> +            external-tpl {
>>>> +                filename = "ddr.bin";
>>>> +                missing-msg = "external-tpl-rockchip";
>>>>   #endif
>>>> +            };
>>>
>>> NACK. This forces the use of a TPL (either built by U-Boot or 
>>> external) which is not always the case. There are still boards 
>>> without a TPL which work perfectly fine (at least I would hope so :) ).
>>>
>>> Basically there are three possible cases:
>>> SPL
>>> TPL+SPL
>>> TPL(external blob)+SPL
>>
>> I'm afraid all the boards support by mainline U-Boot is using 
>> "TPL(U-Boot or external)+SPL+U-Boot" mode, and this mode also always 
>> used by rockchip vendor branch.
>>
>
> That seems to be incorrect.
>
> for conf in $(git grep -l ARCH_ROCKCHIP configs); do
>     make $(basename $conf) > /dev/null 2>&1
>     if ! grep -q CONFIG_TPL=y .config; then
>         echo $conf does not enable TPL;
>     fi
> done
>
> returns:
> configs/chromebit_mickey_defconfig does not enable TPL
> configs/chromebook_bob_defconfig does not enable TPL
> configs/chromebook_jerry_defconfig does not enable TPL
> configs/chromebook_kevin_defconfig does not enable TPL
> configs/chromebook_minnie_defconfig does not enable TPL
> configs/chromebook_speedy_defconfig does not enable TPL
> configs/elgin-rv1108_defconfig does not enable TPL
> configs/evb-rk3036_defconfig does not enable TPL
> configs/evb-rk3128_defconfig does not enable TPL
> configs/evb-rk3308_defconfig does not enable TPL
> configs/evb-rk3568_defconfig does not enable TPL
> configs/evb-rv1108_defconfig does not enable TPL
> configs/ficus-rk3399_defconfig does not enable TPL
> configs/geekbox_defconfig does not enable TPL
> configs/kylin-rk3036_defconfig does not enable TPL
> configs/miqi-rk3288_defconfig does not enable TPL
> configs/phycore-rk3288_defconfig does not enable TPL
> configs/popmetal-rk3288_defconfig does not enable TPL
> configs/roc-cc-rk3308_defconfig does not enable TPL
> configs/rock2_defconfig does not enable TPL
> configs/rock_defconfig does not enable TPL
> configs/sheep-rk3368_defconfig does not enable TPL
>
> is there something I'm missing?

Most of them use simple SPL+U-Boot(for 32bit processer) or have external 
TPL.

We can keep an option to support SPL only case for now.

>
>> For "SPL+U-Boot" use case, it only happen many years ago in some of 
>> rk3288 board and rk3399 board,
>>
>> the SPL will need to support both sdram driver and storage(SPI, 
>> SDCard, eMMC) driver and without back to BootRom,
>>
>> and other function may add to SPL, but the sram size limit is always 
>> there, so all the rk3288 and rk3399 board have migrate to use 
>> "TPL+SPL+U-Boot.itb" mode.
>>
>
> It seems not all have migrated.
>
>> The "TPL+SPL+U-Boot.itb" is much clear and easy to maintine for all 
>> the boards and will be the only accept mode for new board support in 
>> the future on rockchip platform:
>>
>> - TPL for dram init;
>>
>> - SPL for storage init and load next stage firmware, decode FIT image 
>> with ATF/OPTEE support, secure boot support and etc;
>>
>> - U-Boot.itb including ATF and U-Boot proper, maybe also OPTEE.
>>
>> This model can very easy to do the debug with replace the binary from 
>> rockchip  vendor tree during board bringup.
>>
>
> That's fine by me, but we currently have Rockchip boards in mainline 
> which do NOT have TPL enabled, so we need to handle those properly, in 
> binman since it's now the only way to generate the images.
>
>> The SPL+U-Boot mode can only happen for legacy board with only need 
>> U-Boot raw image instead of itb which including trust support,
>>
>
> What makes SPL+U-Boot not capable of using U-Boot.itb or support 
> secure-boot/trust?

It's possible to use SPL+U-Boot.itb, but it's not easy to maintain, 
because this model need to make

all the driver available(FDT support, SDMMC/EMMC driver, FIT decode and 
etc)in the SPL, the SPL

running in SRAM which has limit size, often cause size overflow issue.

Since  BACK_TO_BOOTROM feature is available for all the rockchip SoCs, 
we can us TPL+SPL instead

for much flexible solution.


Thanks,
- Kever
>
>> this kind of board can get the image with only one mkimage command, I 
>> don't think it will need binman support because it only have one image.
>>
>
> I suggest we leave it as it is today, handling TPL+SPL+U-Boot AND 
> SPL+U-Boot, or we "upgrade" current boards which do not have TPL 
> support enabled yet so we only need to have TPL+SPL+U-Boot to support 
> and maintenance is easier in the long term (for the latter case, you 
> can then enforce it by having ARCH_ROCKCHIP imply TPL for example).
>
> Cheers,
> Quentin