Thanks Simon,
On Tue, 10 Dec 2024 at 20:58, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Tue, 10 Dec 2024 at 11:00, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Tue, 10 Dec 2024 at 18:16, Simon Glass sjg@chromium.org wrote:
Hi,
On Tue, 10 Dec 2024 at 02:54, Sughosh Ganu sughosh.ganu@linaro.org wrote:
On Sun, 8 Dec 2024 at 16:22, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
lmb_reserve() is just calling lmb_reserve_flags() with LMB_NONE. There's not much we gain from this abstraction, so let's remove the former and make the code a bit easier to follow.
The code size increase is minimal - e.g for sandbox which includes all of the LMB tests
add/remove: 0/1 grow/shrink: 12/0 up/down: 46/-4 (42) Function old new delta lib_test_lmb_overlapping_reserve 1018 1030 +12 version_string 70 76 +6 test_get_unreserved_size 1032 1038 +6 test_alloc_addr 2933 2939 +6 test_multi_alloc.constprop 3034 3036 +2 test_bigblock 911 913 +2 load_serial 946 948 +2 lib_test_lmb_flags 2101 2103 +2 do_bootz 526 528 +2 do_bootm_linux 2067 2069 +2 bootm_run_states 5275 5277 +2 boot_relocate_fdt 599 601 +2 lmb_reserve 4 - -4 Total: Before=2492742, After=2492784, chg +0.00%
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Fwiw, I had attempted this earlier on similar lines -- have a single API, which would take the flag as a parameter [1], but was asked not to take this approach as part of the review [2].
You missed that sandbox has LTO enabled. With that disabled (like many boards):
sandbox: (for 1/1 boards) all -32.0 text -32.0
We just had a patch and discussion to remove a single function call to save 8 bytes[3]
For firefly-rk3399 this adds 36 bytes:
aarch64: (for 1/1 boards) all +68.0 bss +48.0 text +20.0
Did you include DM tests in that config? Because applying the series says otherwise
for firefly-rk3399_defconfig add/remove: 0/5 grow/shrink: 14/0 up/down: 480/-544 (-64) Function old new delta lmb_alloc_base_flags 76 324 +248 lmb_alloc_addr_flags 4 144 +140 version_string 50 70 +20 boot_relocate_fdt 488 508 +20 boot_ramdisk_high 268 284 +16 tftp_handler 1304 1308 +4 load_serial 512 516 +4 lmb_alloc 8 12 +4 image_setup_libfdt 380 384 +4 do_spi_flash 2164 2168 +4 do_load 732 736 +4 do_bootz 332 336 +4 do_booti 520 524 +4 bootm_run_states 2176 2180 +4 lmb_reserve 8 - -8 lmb_alloc_addr 8 - -8 lmb_alloc_base 80 - -80 _lmb_alloc_addr 144 - -144 _lmb_alloc_base 304 - -304 Total: Before=691587, After=691523, chg -0.01%
OK good
You could run buildman on all arm64 boards to check it, perhaps.
I did test on non-LTO platforms as well, building for all is kind of pointless no? It's either LTO or no LTO.
Right
FWIW you need to apply the entire series, not just a single patch.
Yes, understood, I was just replying to that one patch.
rpi_arm64_defconfig add/remove: 0/5 grow/shrink: 11/0 up/down: 452/-544 (-92) Function old new delta lmb_alloc_base_flags 76 324 +248 lmb_alloc_addr_flags 4 144 +140 boot_relocate_fdt 516 536 +20 boot_ramdisk_high 268 284 +16 tftp_handler 1552 1556 +4 load_serial 512 516 +4 lmb_alloc 8 12 +4 image_setup_libfdt 456 460 +4 do_load 728 732 +4 do_booti 520 524 +4 bootm_run_states 1824 1828 +4 lmb_reserve 8 - -8 lmb_alloc_addr 8 - -8 lmb_alloc_base 80 - -80 _lmb_alloc_addr 144 - -144 _lmb_alloc_base 304 - -304 Total: Before=542062, After=541970, chg -0.02%
qemu_arm64_lwip_defconfig add/remove: 0/5 grow/shrink: 11/0 up/down: 452/-544 (-92) Function old new delta lmb_alloc_base_flags 76 324 +248 lmb_alloc_addr_flags 4 144 +140 boot_relocate_fdt 488 508 +20 boot_ramdisk_high 268 284 +16 load_serial 548 552 +4 lmb_alloc 8 12 +4 image_setup_libfdt 368 372 +4 do_load 728 732 +4 do_bootz 332 336 +4 do_booti 520 524 +4 bootm_run_states 2176 2180 +4 lmb_reserve 8 - -8 lmb_alloc_addr 8 - -8 lmb_alloc_base 80 - -80 _lmb_alloc_addr 144 - -144 _lmb_alloc_base 304 - -304 Total: Before=1020122, After=1020030, chg -0.01%
The other thing is that there is actually only one place apart from tests where the flag is needed - in boot_fdt_add_mem_rsv_regions(). Boards don't use that flag. It is the flags themselves which are confusing.
I still think it's way easier to read without two levels of abstraction. Since the series decreases the size -- unless you include all DM Tests, I'd like to keep the flags as is for now, so I can send the next round which is the actual cleanup. We can re-introduce them if someone finds a 'size problem'. But since it's only when you enable DM I doubt anyone wants to have them working in SPL or production firmware.
OK, well I don't mind, if there is no growth. I wonder if you could do better overall without this patch, but perhaps not.
Probably yes, but we are not looking into anything more than a few bytes -- probably under 4 as well. TBH if we all agree that removing the non-flag variants is fine, I can name the functions without it.
/Ilias
/Ilias
Regards, Simon
-sughosh
[1] - https://lists.denx.de/pipermail/u-boot/2024-July/559790.html [2] - https://lists.denx.de/pipermail/u-boot/2024-August/561607.html
arch/powerpc/cpu/mpc85xx/mp.c | 2 +- arch/powerpc/lib/misc.c | 2 +- boot/bootm.c | 3 ++- boot/image-board.c | 2 +- boot/image-fdt.c | 5 +++-- cmd/booti.c | 2 +- cmd/bootz.c | 2 +- cmd/load.c | 2 +- include/lmb.h | 1 - lib/lmb.c | 5 ----- test/lib/lmb.c | 30 +++++++++++++++--------------- 11 files changed, 26 insertions(+), 30 deletions(-)
[3] https://patchwork.ozlabs.org/project/uboot/patch/f79dc1fbf796dd5ad290f608060...
Regards, SImon