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

[Quentin Schulz]

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 at 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?

> 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?

> 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