[PATCH 2/5] rockchip: rk3399-pinebook-pro: Fix SPL max size and SPI flash payload offset

[Quentin Schulz]

Hi Jonas,

On 6/29/23 00:11, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 2023-06-28 15:49, Quentin Schulz wrote:
>> Hi Jonas,
>>
>> On 6/27/23 21:10, Jonas Karlman wrote:
>>> TPL max size is limited to 184 KB, SPL is loaded to 0x0 and TF-A is
>>> loaded to 0x40000, this limit SPL max size to 256 KB. With BootRom only
>>> reading first 2 KB per 4 KB page of SPI flash, 880 KB may be needed for
>>> TPL+SPL in a worst-case scenario. (184 KB + 256 KB) x 2 = 880 KB
>>>
>>> Use 0xE0000 (896 KB) as the payload offset in SPI flash, this allows
>>> for a payload of 3168 KB before env offset start to overlap.
>>>
>>> Also add CONFIG_ROCKCHIP_SPI_IMAGE=y to build a bootable SPI flash
>>> image, u-boot-rockchip-spi.bin.
>>>
>>> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
>>> ---
>>>    arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi | 4 ----
>>>    configs/pinebook-pro-rk3399_defconfig        | 4 +++-
>>>    2 files changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi b/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
>>> index ea7a5a17ae0f..88a77cad8d43 100644
>>> --- a/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
>>> +++ b/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
>>> @@ -10,10 +10,6 @@
>>>    	chosen {
>>>    		u-boot,spl-boot-order = "same-as-spl", &sdhci, &spiflash, &sdmmc;
>>>    	};
>>> -
>>> -	config {
>>> -		u-boot,spl-payload-offset = <0x60000>; /* @ 384KB */
>>> -	};
>>
>> Just a nitpick/question (and for the whole series): Is there any
>> specific reason for going back to the Kconfig way
>> (CONFIG_SYS_SPI_U_BOOT_OFFS) instead of just modifying the offset in the
>> -u-boot DTSI?
> 
> The main reason is that SYS_SPI_U_BOOT_OFFS is used as payload offset by
> binman in rockchip-u-boot.dtsi, for building u-boot-rockchip-spi.bin.
> And also to avoid any confusion on what value is used.
> 
> rockchip-u-boot.dtsi:
>    offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;
> 

Fair point, converging towards the default for Rockchip platforms seems 
like a good idea, thanks for the explanation.

>>
>> On a different note, that also means that we're effectively breaking
>> current setups by moving the environment some other location. I cannot
>> talk for the maintainer(s) and user(s) but I thought it was worth
>> mentioning.
> 
> The environment should not be affected by these changes. All rk3399
> boards using ENV_IS_IN_SPI_FLASH all use ENV_OFFSET=0x3F8000. u-boot.itb
> or u-boot.img can be up to 3168 KB before it affects environment.
> 
>    0x000000: idbloader.img (TPL+SPL)
>    0x0E0000: u-boot.itb or u-boot.img (was before at 0x060000)
>    0x3F8000: environment, same as before
> 
> I am not expecting any issues for users, as long as the updated
> instructions are followed when updating u-boot in SPI flash, i.e.
> writing u-boot-rockchip-spi.bin at start of SPI flash.
> 

Sorry, seems like I had a brain fart yesterday and mixed SPL payload (so 
U-Boot proper essentially) with U-Boot environment location. Thanks for 
correcting me.

Since those values are dependent on the SoC and not on the hardware 
design, shouldn't we rather hardcode those values for all RK3399 
devices? i.e. set CONFIG_SYS_SPI_U_BOOT_OFFS to 0xe0000 (it's currently 
0 which is probably a bad default) and have SPL_MAX_SIZE set to 0x40000? 
I'm asking now because I did the maths for RK3399 Puma and we got it 
wrong there too. We have TPL_MAX_SIZE set to 0x2e000 like all RK3399 
boards and SPL_MAX_SIZE to 0x2e000. So, (0x2e000 * 2) * 2 = 0xb8000. 
However we have u-boot,spl-payload-offset = <0x80000>; so clearly there 
are cases where this would be an issue.

In our case, changing the u-boot,spl-payload-offset isn't without 
backward compatibility issues because we want to be able to boot U-Boot 
proper from SPI but the TPL+SPL from eMMC for example (so the 
u-boot,spl-payload-offset from the DT in the eMMC needs to match the 
offset of U-Boot proper in the SPI). But since we are not aware of any 
user using the fallback mechanism this way rather than TPL+SPL from SPI 
and fallback to eMMC for U-Boot proper, we'd be fine changing it to 
something safer rather than limiting the size of SPL too much.

Basically, what I am asking is whether we should make those RK3399 
defaults instead of fixing them one by one in DTS/defconfigs? I also do 
not know how critical this currently is, does it absolutely need to be 
in this release (we're late in the rc process right now I believe?) or 
can we manage to put more thoughts into making this generic for all 
RK3399 (and maybe other SoCs from Rockchip)?

Otherwise, the maths sound good, so for the whole series:
Reviewed-by: Quentin Schulz <foss+u-boot at 0leil.net>

(thanks for the detailed commit log, really helps reviewing :) ).

Cheers,
Quentin

> Regards,
> Jonas
> 
>>
>>>    };
>>>    
>>>    &edp {
>>> diff --git a/configs/pinebook-pro-rk3399_defconfig b/configs/pinebook-pro-rk3399_defconfig
>>> index 58a8b91aa60f..1109ebb7387b 100644
>>> --- a/configs/pinebook-pro-rk3399_defconfig
>>> +++ b/configs/pinebook-pro-rk3399_defconfig
>>> @@ -12,6 +12,7 @@ CONFIG_ENV_OFFSET=0x3F8000
>>>    CONFIG_DEFAULT_DEVICE_TREE="rk3399-pinebook-pro"
>>>    CONFIG_DM_RESET=y
>>>    CONFIG_ROCKCHIP_RK3399=y
>>> +CONFIG_ROCKCHIP_SPI_IMAGE=y
>>>    CONFIG_TARGET_PINEBOOK_PRO_RK3399=y
>>>    CONFIG_SPL_STACK=0x400000
>>>    CONFIG_DEBUG_UART_BASE=0xFF1A0000
>>> @@ -26,7 +27,7 @@ CONFIG_USE_PREBOOT=y
>>>    CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-pinebook-pro.dtb"
>>>    CONFIG_DISPLAY_BOARDINFO_LATE=y
>>>    CONFIG_MISC_INIT_R=y
>>> -CONFIG_SPL_MAX_SIZE=0x2e000
>>> +CONFIG_SPL_MAX_SIZE=0x40000
>>
>> nitpick: We *could* have this in another commit after we move the
>> environment to another location later in the SPI flash.
>>
>> Cheers,
>> Quentin
>