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

Fri Jun 30 12:24:21 CEST 2023

Hi Quentin,

On 2023-06-29 10:38, Quentin Schulz wrote:
> 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)?
> 

You are correct, the updated SPL max size and SPI payload offset can
probably be used as defaults instead of just modifying a few boards
defconfig.

I left RK3399 Puma out of this series because current defconfig would
trigger a build error if the TPL+SPL included in u-boot-rockchip-spi.bin
would have grown beyond the 0x80000 offset, i.e. with current defconfig
TPL+SPL does not overlap the payload offset.

For rockpro64 this was not the case, and with the use of 0x60000 and
ROCKCHIP_SPI_IMAGE=y added, binman complained that there was an overlap
of a few KB and a build ended with an error. Adding CONFIG_LTO=y made
SPL size reduce enough to fit below the 0x60000 offset.

However, Peter reported that the addition of CONFIG_LTO=y triggered
a build issue, one that was not triggered in my setup or in CI, and I
said I would prepare a small fix series, see [1].

Patch 1 in this series revert the CONFIG_LTO=y change and adjusts
offsets to fix the binman build error, remaining board updates was to
accommodate the updated flashing instruction.

Personally I have no problem with reworking this to use new defaults
or if this gets delayed, I do not see this as something critical.

Unfortunately, I will not have much time to do such rework next few
weeks so a follow up series or someone else picking this up would fit
my schedule better :-)

[1] https://lore.kernel.org/u-boot/d1f82951-1a41-6178-75c7-3a84a46834b2@kwiboo.se/

Regards,
Jonas

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