[PATCH 0/5] rockchip: Fix SPI flash SPL and payload overlap
This series fixes a build issue introduced in commit 5713135ecc75 ("rockchip: rockpro64: Build u-boot-rockchip-spi.bin"), reported by Peter Robinson.
Closer inspection into how SPI flash is used revealed that current payload offset, 0x60000, is not enough to accommodate a worst-case scenario and may overlap SPL if written to SPI flash.
This series fixes this by adjusting payload offset to 0xE0000, adjusting SPL max size to 0x40000, enables build of u-boot-rockchip-spi.bin for affected boards and update the SPI flashing documentation.
Series depend on [1] for a clean apply.
[1] https://patchwork.ozlabs.org/patch/1800173/
Jonas Karlman (5): rockchip: rk3399-rockpro64: Fix SPL max size and SPI flash payload offset rockchip: rk3399-pinebook-pro: Fix SPL max size and SPI flash payload offset rockchip: rk3399-pinephone-pro: Fix SPL max size and SPI flash payload offset rockchip: rk3399-roc-pc: Fix SPL max size and SPI flash payload offset doc: rockchip: Update SPI flashing instruction
arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi | 4 --- arch/arm/dts/rk3399-pinephone-pro-u-boot.dtsi | 4 --- arch/arm/dts/rk3399-roc-pc-u-boot.dtsi | 4 --- arch/arm/dts/rk3399-rockpro64-u-boot.dtsi | 4 --- configs/pinebook-pro-rk3399_defconfig | 4 ++- configs/pinephone-pro-rk3399_defconfig | 4 ++- configs/roc-pc-mezzanine-rk3399_defconfig | 4 ++- configs/roc-pc-rk3399_defconfig | 4 ++- configs/rockpro64-rk3399_defconfig | 5 ++-- doc/board/rockchip/rockchip.rst | 26 ++++--------------- 10 files changed, 19 insertions(+), 44 deletions(-)
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 remove CONFIG_LTO=y now that there is sufficient space for SPL in SPI flash, and to fix a build issue reported by Peter Robinson.
Fixes: 5713135ecc75 ("rockchip: rockpro64: Build u-boot-rockchip-spi.bin") Reported-by: Peter Robinson pbrobinson@gmail.com Signed-off-by: Jonas Karlman jonas@kwiboo.se --- arch/arm/dts/rk3399-rockpro64-u-boot.dtsi | 4 ---- configs/rockpro64-rk3399_defconfig | 5 ++--- 2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi index bd864d067018..732727d9b036 100644 --- a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi +++ b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi @@ -9,10 +9,6 @@ chosen { u-boot,spl-boot-order = "same-as-spl", &spi_flash, &sdmmc, &sdhci; }; - - config { - u-boot,spl-payload-offset = <0x60000>; /* @ 384KB */ - }; };
&sdhci { diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig index dc4392c7451c..2f1ab1c28864 100644 --- a/configs/rockpro64-rk3399_defconfig +++ b/configs/rockpro64-rk3399_defconfig @@ -21,7 +21,6 @@ CONFIG_SPL_SPI=y CONFIG_SYS_LOAD_ADDR=0x800800 CONFIG_PCI=y CONFIG_DEBUG_UART=y -CONFIG_LTO=y CONFIG_SPL_FIT_SIGNATURE=y CONFIG_LEGACY_IMAGE_FORMAT=y CONFIG_BOOTSTAGE=y @@ -30,7 +29,7 @@ CONFIG_USE_PREBOOT=y CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-rockpro64.dtb" CONFIG_DISPLAY_BOARDINFO_LATE=y CONFIG_MISC_INIT_R=y -CONFIG_SPL_MAX_SIZE=0x2e000 +CONFIG_SPL_MAX_SIZE=0x40000 CONFIG_SPL_PAD_TO=0x7f8000 CONFIG_SPL_HAS_BSS_LINKER_SECTION=y CONFIG_SPL_BSS_START_ADDR=0x400000 @@ -40,7 +39,7 @@ CONFIG_SPL_BSS_MAX_SIZE=0x2000 CONFIG_SPL_STACK_R=y CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000 CONFIG_SPL_SPI_LOAD=y -CONFIG_SYS_SPI_U_BOOT_OFFS=0x60000 +CONFIG_SYS_SPI_U_BOOT_OFFS=0xE0000 CONFIG_TPL=y CONFIG_CMD_BOOTZ=y CONFIG_CMD_GPT=y
Update documentation on how to write a bootable u-boot-rockchip-spi.bin image into SPI flash. This removes the reference to a hardcoded and now obsolete 0x60000 payload offset.
Also remove an obsolete reference to pad_cat.
Signed-off-by: Jonas Karlman jonas@kwiboo.se --- doc/board/rockchip/rockchip.rst | 26 +++++--------------------- 1 file changed, 5 insertions(+), 21 deletions(-)
diff --git a/doc/board/rockchip/rockchip.rst b/doc/board/rockchip/rockchip.rst index 99376fb54c90..5471bb928205 100644 --- a/doc/board/rockchip/rockchip.rst +++ b/doc/board/rockchip/rockchip.rst @@ -211,7 +211,7 @@ SD Card ^^^^^^^
All Rockchip platforms (except rk3128 which doesn't use SPL) are now -supporting a single boot image using binman and pad_cat. +supporting a single boot image using binman.
To write an image that boots from a SD card (assumed to be /dev/sda):
@@ -262,31 +262,15 @@ is u-boot-dtb.img SPI ^^^
-The SPI boot method requires the generation of idbloader.img with help of the mkimage tool. +Write u-boot-rockchip-spi.bin to offset 0 of SPI flash.
-SPL-alone SPI boot image: - -.. code-block:: bash - - ./tools/mkimage -n rk3399 -T rkspi -d spl/u-boot-spl.bin idbloader.img - -TPL+SPL SPI boot image: - -.. code-block:: bash - - ./tools/mkimage -n rk3399 -T rkspi -d tpl/u-boot-tpl.bin:spl/u-boot-spl.bin idbloader.img - -Copy SPI boot images into SD card and boot from SD: +Copy u-boot-rockchip-spi.bin into SD card and boot from SD:
.. code-block:: bash
sf probe - load mmc 1:1 $kernel_addr_r idbloader.img - sf erase 0 +$filesize - sf write $kernel_addr_r 0 ${filesize} - load mmc 1:1 ${kernel_addr_r} u-boot.itb - sf erase 0x60000 +$filesize - sf write $kernel_addr_r 0x60000 ${filesize} + load mmc 1:1 $kernel_addr_r u-boot-rockchip-spi.bin + sf update $fileaddr 0 $filesize
2. Package the image with Rockchip miniloader ---------------------------------------------
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@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 */ - }; };
&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 CONFIG_SPL_PAD_TO=0x7f8000 CONFIG_SPL_HAS_BSS_LINKER_SECTION=y CONFIG_SPL_BSS_START_ADDR=0x400000 @@ -36,6 +37,7 @@ CONFIG_SPL_BSS_MAX_SIZE=0x2000 CONFIG_SPL_STACK_R=y CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000 CONFIG_SPL_SPI_LOAD=y +CONFIG_SYS_SPI_U_BOOT_OFFS=0xE0000 CONFIG_TPL=y CONFIG_CMD_BOOTZ=y CONFIG_CMD_GPIO=y
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@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?
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.
};
&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
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@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>;
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.
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
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@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@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
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@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.s...
Regards, Jonas
Otherwise, the maths sound good, so for the whole series: Reviewed-by: Quentin Schulz foss+u-boot@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
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@kwiboo.se --- arch/arm/dts/rk3399-pinephone-pro-u-boot.dtsi | 4 ---- configs/pinephone-pro-rk3399_defconfig | 4 +++- 2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/arm/dts/rk3399-pinephone-pro-u-boot.dtsi b/arch/arm/dts/rk3399-pinephone-pro-u-boot.dtsi index 347243fe4793..cabf0a9dae89 100644 --- a/arch/arm/dts/rk3399-pinephone-pro-u-boot.dtsi +++ b/arch/arm/dts/rk3399-pinephone-pro-u-boot.dtsi @@ -10,10 +10,6 @@ chosen { u-boot,spl-boot-order = "same-as-spl", &sdhci, &sdmmc; }; - - config { - u-boot,spl-payload-offset = <0x60000>; /* @ 384KB */ - }; };
&rng { diff --git a/configs/pinephone-pro-rk3399_defconfig b/configs/pinephone-pro-rk3399_defconfig index e4a8beeb1abb..b09211f4eb6e 100644 --- a/configs/pinephone-pro-rk3399_defconfig +++ b/configs/pinephone-pro-rk3399_defconfig @@ -12,6 +12,7 @@ CONFIG_ENV_OFFSET=0x3F8000 CONFIG_DEFAULT_DEVICE_TREE="rk3399-pinephone-pro" CONFIG_DM_RESET=y CONFIG_ROCKCHIP_RK3399=y +CONFIG_ROCKCHIP_SPI_IMAGE=y CONFIG_TARGET_PINEPHONE_PRO_RK3399=y CONFIG_SPL_STACK=0x400000 CONFIG_DEBUG_UART_BASE=0xFF1A0000 @@ -25,7 +26,7 @@ CONFIG_USE_PREBOOT=y CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-pinephone-pro.dtb" CONFIG_DISPLAY_BOARDINFO_LATE=y CONFIG_MISC_INIT_R=y -CONFIG_SPL_MAX_SIZE=0x2e000 +CONFIG_SPL_MAX_SIZE=0x40000 CONFIG_SPL_PAD_TO=0x7f8000 CONFIG_SPL_HAS_BSS_LINKER_SECTION=y CONFIG_SPL_BSS_START_ADDR=0x400000 @@ -35,6 +36,7 @@ CONFIG_SPL_BSS_MAX_SIZE=0x2000 CONFIG_SPL_STACK_R=y CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000 CONFIG_SPL_SPI_LOAD=y +CONFIG_SYS_SPI_U_BOOT_OFFS=0xE0000 CONFIG_TPL=y CONFIG_CMD_BOOTZ=y CONFIG_CMD_GPIO=y
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@kwiboo.se --- arch/arm/dts/rk3399-roc-pc-u-boot.dtsi | 4 ---- configs/roc-pc-mezzanine-rk3399_defconfig | 4 +++- configs/roc-pc-rk3399_defconfig | 4 +++- 3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi b/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi index f85e7b62d9ad..c8f4418a7389 100644 --- a/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi +++ b/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi @@ -11,10 +11,6 @@ u-boot,spl-boot-order = "same-as-spl", &spi_flash, &sdhci, &sdmmc; };
- config { - u-boot,spl-payload-offset = <0x60000>; /* @ 384KB */ - }; - vcc_hub_en: vcc_hub_en-regulator { compatible = "regulator-fixed"; enable-active-high; diff --git a/configs/roc-pc-mezzanine-rk3399_defconfig b/configs/roc-pc-mezzanine-rk3399_defconfig index 9321292ca526..6cacff77d4ad 100644 --- a/configs/roc-pc-mezzanine-rk3399_defconfig +++ b/configs/roc-pc-mezzanine-rk3399_defconfig @@ -13,6 +13,7 @@ CONFIG_ENV_SECT_SIZE=0x1000 CONFIG_DEFAULT_DEVICE_TREE="rk3399-roc-pc-mezzanine" CONFIG_DM_RESET=y CONFIG_ROCKCHIP_RK3399=y +CONFIG_ROCKCHIP_SPI_IMAGE=y CONFIG_TARGET_ROC_PC_RK3399=y CONFIG_SPL_STACK=0x400000 CONFIG_DEBUG_UART_BASE=0xFF1A0000 @@ -26,7 +27,7 @@ CONFIG_DEBUG_UART=y CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-roc-pc-mezzanine.dtb" CONFIG_DISPLAY_BOARDINFO_LATE=y CONFIG_MISC_INIT_R=y -CONFIG_SPL_MAX_SIZE=0x2e000 +CONFIG_SPL_MAX_SIZE=0x40000 CONFIG_SPL_PAD_TO=0x7f8000 CONFIG_SPL_HAS_BSS_LINKER_SECTION=y CONFIG_SPL_BSS_START_ADDR=0x400000 @@ -37,6 +38,7 @@ CONFIG_SPL_STACK_R=y CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x20000 CONFIG_SPL_ENV_SUPPORT=y CONFIG_SPL_SPI_LOAD=y +CONFIG_SYS_SPI_U_BOOT_OFFS=0xE0000 CONFIG_TPL=y CONFIG_CMD_BOOTZ=y CONFIG_CMD_GPT=y diff --git a/configs/roc-pc-rk3399_defconfig b/configs/roc-pc-rk3399_defconfig index 824b4041c208..199081ac8414 100644 --- a/configs/roc-pc-rk3399_defconfig +++ b/configs/roc-pc-rk3399_defconfig @@ -14,6 +14,7 @@ CONFIG_ENV_SECT_SIZE=0x1000 CONFIG_DEFAULT_DEVICE_TREE="rk3399-roc-pc" CONFIG_DM_RESET=y CONFIG_ROCKCHIP_RK3399=y +CONFIG_ROCKCHIP_SPI_IMAGE=y CONFIG_TARGET_ROC_PC_RK3399=y CONFIG_SPL_STACK=0x400000 CONFIG_DEBUG_UART_BASE=0xFF1A0000 @@ -27,7 +28,7 @@ CONFIG_USE_PREBOOT=y CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-roc-pc.dtb" CONFIG_DISPLAY_BOARDINFO_LATE=y CONFIG_MISC_INIT_R=y -CONFIG_SPL_MAX_SIZE=0x2e000 +CONFIG_SPL_MAX_SIZE=0x40000 CONFIG_SPL_PAD_TO=0x7f8000 CONFIG_SPL_HAS_BSS_LINKER_SECTION=y CONFIG_SPL_BSS_START_ADDR=0x400000 @@ -38,6 +39,7 @@ CONFIG_SPL_STACK_R=y CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x20000 CONFIG_SPL_ENV_SUPPORT=y CONFIG_SPL_SPI_LOAD=y +CONFIG_SYS_SPI_U_BOOT_OFFS=0xE0000 CONFIG_TPL=y CONFIG_CMD_BOOTZ=y CONFIG_CMD_GPT=y
participants (2)
-
Jonas Karlman -
Quentin Schulz