[U-Boot] [PATCH v4 0/4] configs: rk3288: Tinker Board SPL file must fit into 32 KiB
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific limit.
A common Makefile function is used for this test and the test against CONFIG_BOARD_SIZE_LIMIT.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
v4: use a common function for all size checks in the Makefiles
Heinrich Schuchardt (4): Makefile: reusable function for BOARD_SIZE_CHECK imx: move BOARD_SIZE_CHECK to main Makefile configs: define CONFIG_SPL_SIZE_LIMIT configs: rk3288: Tinker Board SPL file must fit into 32 KiB
Kconfig | 8 ++++++++ Makefile | 33 +++++++++++++++++++++++---------- arch/arm/mach-imx/Makefile | 16 ---------------- configs/tinker-rk3288_defconfig | 1 + 4 files changed, 32 insertions(+), 26 deletions(-)
-- 2.20.1
Carve out function size_check from macro BOARD_SIZE_CHECK. This will allow us to reuse the function for other file size checks.
Depending on the value of CONFIG_BOARD_SIZE_LIMIT an error like the following is thrown:
u-boot-dtb.img exceeds file size limit: limit: 409516 bytes actual: 444346 bytes excess: 34830 bytes make: *** [Makefile:1212: u-boot-dtb.img] Error 1
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v4 new patch --- Makefile | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/Makefile b/Makefile index c1af9307b3..9878595a82 100644 --- a/Makefile +++ b/Makefile @@ -330,6 +330,19 @@ endif # KBUILD_MODULES := 1 #endif
+define size_check + actual=$$( wc -c $1 | awk '{print $$1}'); \ + limit=$$( printf "%d" $2 ); \ + if test $$actual -gt $$limit; then \ + echo "$1 exceeds file size limit:" >&2; \ + echo " limit: $$limit bytes" >&2; \ + echo " actual: $$actual bytes" >&2; \ + echo " excess: $$((actual - limit)) bytes" >&2; \ + exit 1; \ + fi +endef +export size_check + export KBUILD_MODULES KBUILD_BUILTIN export KBUILD_CHECKSRC KBUILD_SRC KBUILD_EXTMOD
@@ -771,16 +784,7 @@ LDPPFLAGS += \ #########################################################################
ifneq ($(CONFIG_BOARD_SIZE_LIMIT),) -BOARD_SIZE_CHECK = \ - @actual=`wc -c $@ | awk '{print $$1}'`; \ - limit=`printf "%d" $(CONFIG_BOARD_SIZE_LIMIT)`; \ - if test $$actual -gt $$limit; then \ - echo "$@ exceeds file size limit:" >&2 ; \ - echo " limit: $$limit bytes" >&2 ; \ - echo " actual: $$actual bytes" >&2 ; \ - echo " excess: $$((actual - limit)) bytes" >&2; \ - exit 1; \ - fi +BOARD_SIZE_CHECK= @ $(call size_check,$@,$(CONFIG_BOARD_SIZE_LIMIT)) else BOARD_SIZE_CHECK = endif -- 2.20.1
On Tue, Apr 02, 2019 at 07:19:04PM +0200, Heinrich Schuchardt wrote:
Carve out function size_check from macro BOARD_SIZE_CHECK. This will allow us to reuse the function for other file size checks.
Depending on the value of CONFIG_BOARD_SIZE_LIMIT an error like the following is thrown:
u-boot-dtb.img exceeds file size limit: limit: 409516 bytes actual: 444346 bytes excess: 34830 bytes make: *** [Makefile:1212: u-boot-dtb.img] Error 1
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
Applied to u-boot/master, thanks!
We currently have duplicate definitions for BOARD_SIZE_CHECK in Makefile and arch/arm/mach-imx/Makefile.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
Depending on the value of CONFIG_BOARD_SIZE_LIMIT an error like an error like the following is thrown:
u-boot-dtb.imx exceeds file size limit: limit: 503696 bytes actual: 509720 bytes excess: 6024 bytes make: *** [Makefile:1051: u-boot-dtb.imx] Error 1
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v4 new patch --- Makefile | 1 + arch/arm/mach-imx/Makefile | 16 ---------------- 2 files changed, 1 insertion(+), 16 deletions(-)
diff --git a/Makefile b/Makefile index 9878595a82..6398117e64 100644 --- a/Makefile +++ b/Makefile @@ -1042,6 +1042,7 @@ endif
%.imx: %.bin $(Q)$(MAKE) $(build)=arch/arm/mach-imx $@ + $(BOARD_SIZE_CHECK)
%.vyb: %.imx $(Q)$(MAKE) $(build)=arch/arm/cpu/armv7/vf610 $@ diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile index c3ed62aed6..7985afb154 100644 --- a/arch/arm/mach-imx/Makefile +++ b/arch/arm/mach-imx/Makefile @@ -61,21 +61,6 @@ obj-$(CONFIG_CMD_HDMIDETECT) += cmd_hdmidet.o obj-$(CONFIG_CMD_DEKBLOB) += cmd_dek.o endif
-ifneq ($(CONFIG_BOARD_SIZE_LIMIT),) -BOARD_SIZE_CHECK = \ - @actual=`wc -c $@ | awk '{print $$1}'`; \ - limit=`printf "%d" $(CONFIG_BOARD_SIZE_LIMIT)`; \ - if test $$actual -gt $$limit; then \ - echo "$@ exceeds file size limit:" >&2 ; \ - echo " limit: $$limit bytes" >&2 ; \ - echo " actual: $$actual bytes" >&2 ; \ - echo " excess: $$((actual - limit)) bytes" >&2; \ - exit 1; \ - fi -else -BOARD_SIZE_CHECK = -endif - PLUGIN = board/$(BOARDDIR)/plugin
ifeq ($(CONFIG_USE_IMXIMG_PLUGIN),y) @@ -124,7 +109,6 @@ u-boot.imx: MKIMAGEOUTPUT = u-boot.imx.log
u-boot.imx: u-boot.bin u-boot.cfgout $(PLUGIN).bin FORCE $(call if_changed,mkimage) - $(BOARD_SIZE_CHECK)
ifeq ($(CONFIG_OF_SEPARATE),y) MKIMAGEFLAGS_u-boot-dtb.imx = -n $(filter-out $(PLUGIN).bin $< $(PHONY),$^) \ -- 2.20.1
On 4/2/19 7:19 PM, Heinrich Schuchardt wrote:
We currently have duplicate definitions for BOARD_SIZE_CHECK in Makefile and arch/arm/mach-imx/Makefile.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
Depending on the value of CONFIG_BOARD_SIZE_LIMIT an error like an error like the following is thrown:
u-boot-dtb.imx exceeds file size limit: limit: 503696 bytes actual: 509720 bytes excess: 6024 bytes make: *** [Makefile:1051: u-boot-dtb.imx] Error 1
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
Hello Stefano, hello Fabio,
there have been some comments to 0/4 indicating that the first patch of the series should be reworked.
But I think this one is worth merging on it own. Could you, please, review it and if ok add it to your IMX repository.
Best regards
Heinrich
v4 new patch
Makefile | 1 + arch/arm/mach-imx/Makefile | 16 ---------------- 2 files changed, 1 insertion(+), 16 deletions(-)
diff --git a/Makefile b/Makefile index 9878595a82..6398117e64 100644 --- a/Makefile +++ b/Makefile @@ -1042,6 +1042,7 @@ endif
%.imx: %.bin $(Q)$(MAKE) $(build)=arch/arm/mach-imx $@
- $(BOARD_SIZE_CHECK)
%.vyb: %.imx $(Q)$(MAKE) $(build)=arch/arm/cpu/armv7/vf610 $@ diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile index c3ed62aed6..7985afb154 100644 --- a/arch/arm/mach-imx/Makefile +++ b/arch/arm/mach-imx/Makefile @@ -61,21 +61,6 @@ obj-$(CONFIG_CMD_HDMIDETECT) += cmd_hdmidet.o obj-$(CONFIG_CMD_DEKBLOB) += cmd_dek.o endif
-ifneq ($(CONFIG_BOARD_SIZE_LIMIT),) -BOARD_SIZE_CHECK = \
@actual=`wc -c $@ | awk '{print $$1}'`; \limit=`printf "%d" $(CONFIG_BOARD_SIZE_LIMIT)`; \if test $$actual -gt $$limit; then \echo "$@ exceeds file size limit:" >&2 ; \echo " limit: $$limit bytes" >&2 ; \echo " actual: $$actual bytes" >&2 ; \echo " excess: $$((actual - limit)) bytes" >&2; \exit 1; \fi-else -BOARD_SIZE_CHECK = -endif
PLUGIN = board/$(BOARDDIR)/plugin
ifeq ($(CONFIG_USE_IMXIMG_PLUGIN),y) @@ -124,7 +109,6 @@ u-boot.imx: MKIMAGEOUTPUT = u-boot.imx.log
u-boot.imx: u-boot.bin u-boot.cfgout $(PLUGIN).bin FORCE $(call if_changed,mkimage)
- $(BOARD_SIZE_CHECK)
ifeq ($(CONFIG_OF_SEPARATE),y) MKIMAGEFLAGS_u-boot-dtb.imx = -n $(filter-out $(PLUGIN).bin $< $(PHONY),$^) \ -- 2.20.1
Hi Heinrich,
On Tue, Apr 2, 2019 at 2:19 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
We currently have duplicate definitions for BOARD_SIZE_CHECK in Makefile and arch/arm/mach-imx/Makefile.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
Depending on the value of CONFIG_BOARD_SIZE_LIMIT an error like an error like the following is thrown:
u-boot-dtb.imx exceeds file size limit: limit: 503696 bytes actual: 509720 bytes excess: 6024 bytes make: *** [Makefile:1051: u-boot-dtb.imx] Error 1
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
Yes, it makes sense. No need for a imx specific size check:
Reviewed-by: Fabio Estevam festevam@gmail.com
On 4/5/19 3:05 PM, Fabio Estevam wrote:
Hi Heinrich,
On Tue, Apr 2, 2019 at 2:19 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
We currently have duplicate definitions for BOARD_SIZE_CHECK in Makefile and arch/arm/mach-imx/Makefile.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
Depending on the value of CONFIG_BOARD_SIZE_LIMIT an error like an error like the following is thrown:
u-boot-dtb.imx exceeds file size limit: limit: 503696 bytes actual: 509720 bytes excess: 6024 bytes make: *** [Makefile:1051: u-boot-dtb.imx] Error 1
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
Yes, it makes sense. No need for a imx specific size check:
Reviewed-by: Fabio Estevam festevam@gmail.com
Thanks for reviewing.
@Stefano: I have assigned this single patch to you in patchwork to please pick-up for the imx tree.
Best regards
Heinrich
On Tue, Apr 02, 2019 at 07:19:05PM +0200, Heinrich Schuchardt wrote:
We currently have duplicate definitions for BOARD_SIZE_CHECK in Makefile and arch/arm/mach-imx/Makefile.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
Depending on the value of CONFIG_BOARD_SIZE_LIMIT an error like an error like the following is thrown:
u-boot-dtb.imx exceeds file size limit: limit: 503696 bytes actual: 509720 bytes excess: 6024 bytes make: *** [Makefile:1051: u-boot-dtb.imx] Error 1
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Fabio Estevam festevam@gmail.com
Applied to u-boot/master, thanks!
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific maximum size for the SPL file.
Use Makefile function size_check() to implement the test.
Depending on the size of CONFIG_SPL_SIZE_LIMIT an error like the following is thrown:
spl/u-boot-spl.bin exceeds file size limit: limit: 30720 bytes actual: 33426 bytes excess: 2706 bytes make: *** [Makefile:1663: spl/u-boot-spl.bin] Error 1
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v4 use function size_check() split off change in tinker-rk3288_defconfig --- Kconfig | 8 ++++++++ Makefile | 8 ++++++++ 2 files changed, 16 insertions(+)
diff --git a/Kconfig b/Kconfig index 305b265ed7..69212bf58f 100644 --- a/Kconfig +++ b/Kconfig @@ -172,6 +172,14 @@ config TPL_SYS_MALLOC_F_LEN particular needs this to operate, so that it can allocate the initial serial device and any others that are needed.
+config SPL_SIZE_LIMIT + int "Maximum size of SPL image" + depends on SPL + default 0 + help + Specifies the maximum length of the U-Boot SPL image. + If this value is zero, it is ignored. + menuconfig EXPERT bool "Configure standard U-Boot features (expert users)" default y diff --git a/Makefile b/Makefile index 6398117e64..073ef2b387 100644 --- a/Makefile +++ b/Makefile @@ -789,6 +789,12 @@ else BOARD_SIZE_CHECK = endif
+ifneq ($(CONFIG_SPL_SIZE_LIMIT),0) +SPL_SIZE_CHECK = @$(call size_check,$@,$(CONFIG_SPL_SIZE_LIMIT)) +else +SPL_SIZE_CHECK = +endif + # Statically apply RELA-style relocations (currently arm64 only) # This is useful for arm64 where static relocation needs to be performed on # the raw binary, but certain simulators only accept an ELF file (but don't @@ -1654,6 +1660,8 @@ u-boot.lds: $(LDSCRIPT) prepare FORCE
spl/u-boot-spl.bin: spl/u-boot-spl @: + $(SPL_SIZE_CHECK) + spl/u-boot-spl: tools prepare \ $(if $(CONFIG_OF_SEPARATE)$(CONFIG_OF_EMBED)$(CONFIG_SPL_OF_PLATDATA),dts/dt.dtb) \ $(if $(CONFIG_OF_SEPARATE)$(CONFIG_OF_EMBED)$(CONFIG_TPL_OF_PLATDATA),dts/dt.dtb) -- 2.20.1
On Tue, Apr 02, 2019 at 07:19:06PM +0200, Heinrich Schuchardt wrote:
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific maximum size for the SPL file.
Use Makefile function size_check() to implement the test.
Depending on the size of CONFIG_SPL_SIZE_LIMIT an error like the following is thrown:
spl/u-boot-spl.bin exceeds file size limit: limit: 30720 bytes actual: 33426 bytes excess: 2706 bytes make: *** [Makefile:1663: spl/u-boot-spl.bin] Error 1
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
Applied to u-boot/master, thanks!
Hi Tom and Heinrich,
On Fri, Jun 7, 2019 at 7:06 PM Tom Rini trini@konsulko.com wrote:
On Tue, Apr 02, 2019 at 07:19:06PM +0200, Heinrich Schuchardt wrote:
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific maximum size for the SPL file.
Use Makefile function size_check() to implement the test.
Depending on the size of CONFIG_SPL_SIZE_LIMIT an error like the following is thrown:
spl/u-boot-spl.bin exceeds file size limit: limit: 30720 bytes actual: 33426 bytes excess: 2706 bytes make: *** [Makefile:1663: spl/u-boot-spl.bin] Error 1
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
Applied to u-boot/master, thanks!
Just tested this on mx6sabresd_defconfig, where the SPL size limit is 68 * 1024, so I passed:
CONFIG_SPL_SIZE_LIMIT=69632
but SPL builds without error and the generated size is 76800.
Any ideas why CONFIG_SPL_SIZE_LIMIT did not catch the SPL size overflow for mx6sabresd_defconfig?
Thanks
On Fri, Jun 07, 2019 at 07:13:47PM -0300, Fabio Estevam wrote:
Hi Tom and Heinrich,
On Fri, Jun 7, 2019 at 7:06 PM Tom Rini trini@konsulko.com wrote:
On Tue, Apr 02, 2019 at 07:19:06PM +0200, Heinrich Schuchardt wrote:
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific maximum size for the SPL file.
Use Makefile function size_check() to implement the test.
Depending on the size of CONFIG_SPL_SIZE_LIMIT an error like the following is thrown:
spl/u-boot-spl.bin exceeds file size limit: limit: 30720 bytes actual: 33426 bytes excess: 2706 bytes make: *** [Makefile:1663: spl/u-boot-spl.bin] Error 1
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
Applied to u-boot/master, thanks!
Just tested this on mx6sabresd_defconfig, where the SPL size limit is 68 * 1024, so I passed:
CONFIG_SPL_SIZE_LIMIT=69632
but SPL builds without error and the generated size is 76800.
Any ideas why CONFIG_SPL_SIZE_LIMIT did not catch the SPL size overflow for mx6sabresd_defconfig?
So, I hit this too. SPL_SIZE_LIMIT is hex, not int. Or rather, Simon's patch changes it from int to hex, which, yeah, that's wrong. Thanks for the report, patch coming up!
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- The error for the TinkerBoard is resolved by: configs: tinker-rk3288 disable CONFIG_SPL_I2C_SUPPORT https://lists.denx.de/pipermail/u-boot/2019-February/360367.html
v4 split of change in tinker-rk3288_defconfig --- configs/tinker-rk3288_defconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/configs/tinker-rk3288_defconfig b/configs/tinker-rk3288_defconfig index 85ef9dabbd..585fbf505c 100644 --- a/configs/tinker-rk3288_defconfig +++ b/configs/tinker-rk3288_defconfig @@ -3,6 +3,7 @@ CONFIG_ARCH_ROCKCHIP=y CONFIG_SYS_TEXT_BASE=0x00000000 CONFIG_SYS_MALLOC_F_LEN=0x2000 CONFIG_ROCKCHIP_RK3288=y +CONFIG_SPL_SIZE_LIMIT=30720 CONFIG_SPL_ROCKCHIP_BACK_TO_BROM=y CONFIG_TARGET_TINKER_RK3288=y CONFIG_DEBUG_UART_BASE=0xff690000 -- 2.20.1
On Tue, Apr 02, 2019 at 07:19:07PM +0200, Heinrich Schuchardt wrote:
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
Applied to u-boot/master, thanks!
On Tue, Apr 02, 2019 at 07:19:03PM +0200, Heinrich Schuchardt wrote:
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific limit.
A common Makefile function is used for this test and the test against CONFIG_BOARD_SIZE_LIMIT.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
I'm sorry you weren't Cc'd on Simon's thread where we're trying to improve the size check stuff to be generic enough to use everywhere. We can't generically use a shell script as we need to know some processed values too. I don't know if Simon got to the point of writing a C based helper to use or not.
On 4/2/19 7:29 PM, Tom Rini wrote:
On Tue, Apr 02, 2019 at 07:19:03PM +0200, Heinrich Schuchardt wrote:
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific limit.
A common Makefile function is used for this test and the test against CONFIG_BOARD_SIZE_LIMIT.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
I'm sorry you weren't Cc'd on Simon's thread where we're trying to improve the size check stuff to be generic enough to use everywhere. We can't generically use a shell script as we need to know some processed values too. I don't know if Simon got to the point of writing a C based helper to use or not.
Hello Tom,
could you, please, provide a link to the thread.
Is the test prior to my patch incorrect? Or do you want to imply that after my patch we get different results?
Best regards
Heinrich
On Tue, Apr 02, 2019 at 07:33:42PM +0200, Heinrich Schuchardt wrote:
On 4/2/19 7:29 PM, Tom Rini wrote:
On Tue, Apr 02, 2019 at 07:19:03PM +0200, Heinrich Schuchardt wrote:
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific limit.
A common Makefile function is used for this test and the test against CONFIG_BOARD_SIZE_LIMIT.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
I'm sorry you weren't Cc'd on Simon's thread where we're trying to improve the size check stuff to be generic enough to use everywhere. We can't generically use a shell script as we need to know some processed values too. I don't know if Simon got to the point of writing a C based helper to use or not.
Hello Tom,
could you, please, provide a link to the thread.
https://patchwork.ozlabs.org/patch/1050465/ (and no, I didn't apply it, I forgot to delete my email before I sent out that batch of applieds).
Is the test prior to my patch incorrect? Or do you want to imply that after my patch we get different results?
The problem is that I don't want to make wider use of the shell checking notion. It's not as complete as we'd like and can't be used on as many platforms as need it either, due to needing cpp to actually determine values in some cases.
On 4/2/19 7:47 PM, Tom Rini wrote:
On Tue, Apr 02, 2019 at 07:33:42PM +0200, Heinrich Schuchardt wrote:
On 4/2/19 7:29 PM, Tom Rini wrote:
On Tue, Apr 02, 2019 at 07:19:03PM +0200, Heinrich Schuchardt wrote:
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific limit.
A common Makefile function is used for this test and the test against CONFIG_BOARD_SIZE_LIMIT.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
I'm sorry you weren't Cc'd on Simon's thread where we're trying to improve the size check stuff to be generic enough to use everywhere. We can't generically use a shell script as we need to know some processed values too. I don't know if Simon got to the point of writing a C based helper to use or not.
Hello Tom,
could you, please, provide a link to the thread.
https://patchwork.ozlabs.org/patch/1050465/ (and no, I didn't apply it, I forgot to delete my email before I sent out that batch of applieds).
Is the test prior to my patch incorrect? Or do you want to imply that after my patch we get different results?
The problem is that I don't want to make wider use of the shell checking notion. It's not as complete as we'd like and can't be used on as many platforms as need it either, due to needing cpp to actually determine values in some cases.
Yes, the patch by Simon G. is similiar to my patch series. What his patch does not address is the duplicate coding for checking *.imx files. My idea has been to put all tests into the main Makefile to avoid problems with variable substitutions.
It is unclear to me what you mean by: "It's not as complete as we'd like" and where you see a need for "cpp to actually determine values".
I would not be able to provide a better patch without understanding your view on the requirements.
Best regards
Heinrich
On 02.04.19 19:33, Heinrich Schuchardt wrote:
On 4/2/19 7:29 PM, Tom Rini wrote:
On Tue, Apr 02, 2019 at 07:19:03PM +0200, Heinrich Schuchardt wrote:
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific limit.
A common Makefile function is used for this test and the test against CONFIG_BOARD_SIZE_LIMIT.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
I'm sorry you weren't Cc'd on Simon's thread where we're trying to improve the size check stuff to be generic enough to use everywhere. We can't generically use a shell script as we need to know some processed values too. I don't know if Simon got to the point of writing a C based helper to use or not.
Right, I should have included you, Heinrich, sorry.
No, unfortunately, I haven't found the time to work on the C based approach, yet.
To sum it up, Heinrich, the issue was that for socfpga gen5, we're approaching the 64KiB total limit for SPL. That includes all regions we can check via CONFIG_SPL_MAX_FOOTPRINT (e.g. text, data, bss) plus devicetree, early malloc, global data and initial stack.
My approach would have been much like your check (which by the way is better than mine in that the check stays in main Makefile), but the size limit needs to include those items I've mentioned above. So while I could add a Kconfig based total size, I would need to subtract gd, malloc area and reserved stack size to get the SPL_SIZE_LIMIT value.
The problem I see here is that this is a different size limit to the one you want to check. I need "SRAM_SIZE - x - y - z" while you need "LOAD_LIMIT", which is constant.
Maybe we could start with your limit and then add bool config options like "SPL_SIZE_LIMIT_SUBTRACT_GD", "SPL_SIZE_LIMIT_SUBTRACT_MALLOC_F" and "SPL_SIZE_LIMIT_SUBTRACT_STACK_SIZE" that could then be used by a C program to help the size check.
Regards, Simon
Hello Tom,
could you, please, provide a link to the thread.
Is the test prior to my patch incorrect? Or do you want to imply that after my patch we get different results?
Best regards
Heinrich
Hi Heinrich,
On 04/03/2019 01:19 AM, Heinrich Schuchardt wrote:
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
32KB is the limit of SPL size, not SPL image size, does not include 2KB header.
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific limit.
There is already CONFIG_SPL_MAX_SIZE for the SPL size limit, isn't it? I don't understand why we need new variable.
Thanks, - Kever
A common Makefile function is used for this test and the test against CONFIG_BOARD_SIZE_LIMIT.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
v4: use a common function for all size checks in the Makefiles
Heinrich Schuchardt (4): Makefile: reusable function for BOARD_SIZE_CHECK imx: move BOARD_SIZE_CHECK to main Makefile configs: define CONFIG_SPL_SIZE_LIMIT configs: rk3288: Tinker Board SPL file must fit into 32 KiB
Kconfig | 8 ++++++++ Makefile | 33 +++++++++++++++++++++++---------- arch/arm/mach-imx/Makefile | 16 ---------------- configs/tinker-rk3288_defconfig | 1 + 4 files changed, 32 insertions(+), 26 deletions(-)
-- 2.20.1
On 4/3/19 2:59 AM, Kever Yang wrote:
Hi Heinrich,
On 04/03/2019 01:19 AM, Heinrich Schuchardt wrote:
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
32KB is the limit of SPL size, not SPL image size, does not include 2KB header.
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific limit.
There is already CONFIG_SPL_MAX_SIZE for the SPL size limit, isn't it? I don't understand why we need new variable.
SPL_MAX_SIZE is used to set CONFIG_SPL_PAD_TO if CONFIG_SPL_PAD_TO is not defined. If CONFIG_SPL_PAD_TO is defined it specifies the maximum value of CONFIG_SPL_PAD_TO. So in case CONFIG_SPL_PAD_TO is not defined SPL_MAX_SIZE does not specify a maximum size but a minimum one.
It is unclear to me why Benoît in 6113d3f27ca ("Makefile: Change CONFIG_SPL_PAD_TO to image offset") chose this design. Entangling the variables in this way does not provide a clean design.
The check
#if defined(IMAGE_MAX_SIZE) ASSERT(__image_copy_end - __image_copy_start < (IMAGE_MAX_SIZE), \ "SPL image too big"); #endif
in arch/arm/cpu/u-boot-spl.lds neither considers the size of the device tree nor of the BSS section. So at least for Rockchip SOCs it is checking a measure that is not directly related to the image size created by `mkimage -T rksd`.
Best regards
Heinrich
Thanks,
- Kever
A common Makefile function is used for this test and the test against CONFIG_BOARD_SIZE_LIMIT.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
v4: use a common function for all size checks in the Makefiles
Heinrich Schuchardt (4): Makefile: reusable function for BOARD_SIZE_CHECK imx: move BOARD_SIZE_CHECK to main Makefile configs: define CONFIG_SPL_SIZE_LIMIT configs: rk3288: Tinker Board SPL file must fit into 32 KiB
Kconfig | 8 ++++++++ Makefile | 33 +++++++++++++++++++++++---------- arch/arm/mach-imx/Makefile | 16 ---------------- configs/tinker-rk3288_defconfig | 1 + 4 files changed, 32 insertions(+), 26 deletions(-)
-- 2.20.1
Hi Heinrich,
On Wed, Apr 3, 2019 at 8:20 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 4/3/19 2:59 AM, Kever Yang wrote:
Hi Heinrich,
On 04/03/2019 01:19 AM, Heinrich Schuchardt wrote:
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
32KB is the limit of SPL size, not SPL image size, does not include 2KB header.
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific limit.
There is already CONFIG_SPL_MAX_SIZE for the SPL size limit, isn't it? I don't understand why we need new variable.
SPL_MAX_SIZE is used to set CONFIG_SPL_PAD_TO if CONFIG_SPL_PAD_TO is not defined. If CONFIG_SPL_PAD_TO is defined it specifies the maximum value of CONFIG_SPL_PAD_TO.
If CONFIG_SPL_PAD_TO is defined and non-zero, CONFIG_SPL_MAX_SIZE specifies the _minimum_ value of CONFIG_SPL_PAD_TO.
So in case CONFIG_SPL_PAD_TO is not defined SPL_MAX_SIZE does not specify a maximum size but a minimum one.
CONFIG_SPL_MAX_SIZE specifies a minimum value for CONFIG_SPL_PAD_TO, but a maximum size for the SPL.
It is unclear to me why Benoît in 6113d3f27ca ("Makefile: Change CONFIG_SPL_PAD_TO to image offset") chose this design. Entangling the variables in this way does not provide a clean design.
Everything is explained in this thread: https://lists.denx.de/pipermail/u-boot/2013-February/146851.html
These two variables coexisted before this change. They have different purposes, but they are related, so they cannot take any values independently of each other.
The purpose of CONFIG_SPL_MAX_SIZE is not to define default or minimum values for CONFIG_SPL_PAD_TO, but only what its name says, i.e. to define the maximum SPL size.
After the SPL, there may be a gap, then a payload appended to the image. The purpose of CONFIG_SPL_PAD_TO is to define the image offset of the payload, if any. CONFIG_SPL_PAD_TO can be 0 if the payload can always be appended to the SPL without any gap.
Therefore, the SPL must fit inside CONFIG_SPL_MAX_SIZE, and CONFIG_SPL_MAX_SIZE must fit inside CONFIG_SPL_PAD_TO if the latter is non-zero.
[...]
Best regards, Benoît
On Wed, Apr 3, 2019 at 8:20 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 4/3/19 2:59 AM, Kever Yang wrote:
Hi Heinrich,
On 04/03/2019 01:19 AM, Heinrich Schuchardt wrote:
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
32KB is the limit of SPL size, not SPL image size, does not include 2KB header.
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific limit.
There is already CONFIG_SPL_MAX_SIZE for the SPL size limit, isn't it? I don't understand why we need new variable.
SPL_MAX_SIZE is used to set CONFIG_SPL_PAD_TO if CONFIG_SPL_PAD_TO is not defined. If CONFIG_SPL_PAD_TO is defined it specifies the maximum value of CONFIG_SPL_PAD_TO. So in case CONFIG_SPL_PAD_TO is not defined SPL_MAX_SIZE does not specify a maximum size but a minimum one.
It is unclear to me why Benoît in 6113d3f27ca ("Makefile: Change CONFIG_SPL_PAD_TO to image offset") chose this design. Entangling the variables in this way does not provide a clean design.
The check
#if defined(IMAGE_MAX_SIZE) ASSERT(__image_copy_end - __image_copy_start < (IMAGE_MAX_SIZE), \ "SPL image too big"); #endif
in arch/arm/cpu/u-boot-spl.lds neither considers the size of the device tree nor of the BSS section. So at least for Rockchip SOCs it is checking a measure that is not directly related to the image size created by `mkimage -T rksd`.
Best regards
Heinrich
Thanks,
- Kever
A common Makefile function is used for this test and the test against CONFIG_BOARD_SIZE_LIMIT.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
v4: use a common function for all size checks in the Makefiles
Heinrich Schuchardt (4): Makefile: reusable function for BOARD_SIZE_CHECK imx: move BOARD_SIZE_CHECK to main Makefile configs: define CONFIG_SPL_SIZE_LIMIT configs: rk3288: Tinker Board SPL file must fit into 32 KiB
Kconfig | 8 ++++++++ Makefile | 33 +++++++++++++++++++++++---------- arch/arm/mach-imx/Makefile | 16 ---------------- configs/tinker-rk3288_defconfig | 1 + 4 files changed, 32 insertions(+), 26 deletions(-)
-- 2.20.1
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Heinrich,
On 02.04.19 19:19, Heinrich Schuchardt wrote:
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific limit.
A common Makefile function is used for this test and the test against CONFIG_BOARD_SIZE_LIMIT.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
Has anything from this series been applied? I now have a working patch that applies on top of this and adds a full SPL SRAM size check (including HEAP, GD and stack; via a host tool) which works for socfpga (as an example of a platform where SPL binary is loaded to limited SRAM).
Actually, my patch would replace your patch 3/4 but build on 1/4 (2/4 and 4/4 are arch-specific).
How should we proceed here? I could send a series including your 1/4, or I could send a series completely building on this series, at the downside of more or less reverting your 2/4.
Regards, Simon
v4: use a common function for all size checks in the Makefiles
Heinrich Schuchardt (4): Makefile: reusable function for BOARD_SIZE_CHECK imx: move BOARD_SIZE_CHECK to main Makefile configs: define CONFIG_SPL_SIZE_LIMIT configs: rk3288: Tinker Board SPL file must fit into 32 KiB
Kconfig | 8 ++++++++ Makefile | 33 +++++++++++++++++++++++---------- arch/arm/mach-imx/Makefile | 16 ---------------- configs/tinker-rk3288_defconfig | 1 + 4 files changed, 32 insertions(+), 26 deletions(-)
-- 2.20.1
On Thu, Apr 18, 2019 at 10:12:36PM +0200, Simon Goldschmidt wrote:
Heinrich,
On 02.04.19 19:19, Heinrich Schuchardt wrote:
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific limit.
A common Makefile function is used for this test and the test against CONFIG_BOARD_SIZE_LIMIT.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
Has anything from this series been applied? I now have a working patch that applies on top of this and adds a full SPL SRAM size check (including HEAP, GD and stack; via a host tool) which works for socfpga (as an example of a platform where SPL binary is loaded to limited SRAM).
Actually, my patch would replace your patch 3/4 but build on 1/4 (2/4 and 4/4 are arch-specific).
How should we proceed here? I could send a series including your 1/4, or I could send a series completely building on this series, at the downside of more or less reverting your 2/4.
What I would like to see is i.MX (and rockchip) converted to use your new test as well (also, yay! Thanks for following up on that!) and we drop the existing check here.
Am 22.04.2019 um 16:36 schrieb Tom Rini:
On Thu, Apr 18, 2019 at 10:12:36PM +0200, Simon Goldschmidt wrote:
Heinrich,
On 02.04.19 19:19, Heinrich Schuchardt wrote:
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific limit.
A common Makefile function is used for this test and the test against CONFIG_BOARD_SIZE_LIMIT.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
Has anything from this series been applied? I now have a working patch that applies on top of this and adds a full SPL SRAM size check (including HEAP, GD and stack; via a host tool) which works for socfpga (as an example of a platform where SPL binary is loaded to limited SRAM).
Actually, my patch would replace your patch 3/4 but build on 1/4 (2/4 and 4/4 are arch-specific).
How should we proceed here? I could send a series including your 1/4, or I could send a series completely building on this series, at the downside of more or less reverting your 2/4.
What I would like to see is i.MX (and rockchip) converted to use your new test as well (also, yay! Thanks for following up on that!) and we drop the existing check here.
Ok, so while I cannot really help on i.MX and rockchip, why don't you accept this series from Heinrich and I'll send my patch as followup? Then we can discuss this with i.MX and rockchip maintainers.
Regards, Simon
On Mon, Apr 22, 2019 at 08:40:52PM +0200, Simon Goldschmidt wrote:
Am 22.04.2019 um 16:36 schrieb Tom Rini:
On Thu, Apr 18, 2019 at 10:12:36PM +0200, Simon Goldschmidt wrote:
Heinrich,
On 02.04.19 19:19, Heinrich Schuchardt wrote:
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific limit.
A common Makefile function is used for this test and the test against CONFIG_BOARD_SIZE_LIMIT.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
Has anything from this series been applied? I now have a working patch that applies on top of this and adds a full SPL SRAM size check (including HEAP, GD and stack; via a host tool) which works for socfpga (as an example of a platform where SPL binary is loaded to limited SRAM).
Actually, my patch would replace your patch 3/4 but build on 1/4 (2/4 and 4/4 are arch-specific).
How should we proceed here? I could send a series including your 1/4, or I could send a series completely building on this series, at the downside of more or less reverting your 2/4.
What I would like to see is i.MX (and rockchip) converted to use your new test as well (also, yay! Thanks for following up on that!) and we drop the existing check here.
Ok, so while I cannot really help on i.MX and rockchip, why don't you accept this series from Heinrich and I'll send my patch as followup? Then we can discuss this with i.MX and rockchip maintainers.
Can you post your series now, and we can move from there? Thanks!
On 22.04.19 21:29, Tom Rini wrote:
On Mon, Apr 22, 2019 at 08:40:52PM +0200, Simon Goldschmidt wrote:
Am 22.04.2019 um 16:36 schrieb Tom Rini:
On Thu, Apr 18, 2019 at 10:12:36PM +0200, Simon Goldschmidt wrote:
Heinrich,
On 02.04.19 19:19, Heinrich Schuchardt wrote:
The SPL image for the Tinker Board has to fit into 32 KiB. This includes up to 2 KiB for the file header.
A new configuration variable CONFIG_SPL_SIZE_LIMIT is introduced to define the board specific limit.
A common Makefile function is used for this test and the test against CONFIG_BOARD_SIZE_LIMIT.
Move the board size check from arch/arm/mach-imx/Makefile to Makefile.
Has anything from this series been applied? I now have a working patch that applies on top of this and adds a full SPL SRAM size check (including HEAP, GD and stack; via a host tool) which works for socfpga (as an example of a platform where SPL binary is loaded to limited SRAM).
Actually, my patch would replace your patch 3/4 but build on 1/4 (2/4 and 4/4 are arch-specific).
How should we proceed here? I could send a series including your 1/4, or I could send a series completely building on this series, at the downside of more or less reverting your 2/4.
What I would like to see is i.MX (and rockchip) converted to use your new test as well (also, yay! Thanks for following up on that!) and we drop the existing check here.
Ok, so while I cannot really help on i.MX and rockchip, why don't you accept this series from Heinrich and I'll send my patch as followup? Then we can discuss this with i.MX and rockchip maintainers.
Can you post your series now, and we can move from there? Thanks!
Did that: https://patchwork.ozlabs.org/patch/1088865/ [Depends on this series from Heinrich]
Regards, Simon
participants (6)
-
Benoît Thébaudeau -
Fabio Estevam -
Heinrich Schuchardt -
Kever Yang -
Simon Goldschmidt -
Tom Rini