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 <[email protected]> >>>> --- >>>> 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/[email protected]/ Regards, Jonas > Otherwise, the maths sound good, so for the whole series: > Reviewed-by: Quentin Schulz <[email protected]> > > (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 >>
