[RFC PATCH 1/7] lmb: Replace lmb_reserve() with lmb_reserve_flags()
Ilias Apalodimas
ilias.apalodimas at linaro.org
Tue Dec 10 19:00:12 CET 2024
Hi Simon,
On Tue, 10 Dec 2024 at 18:16, Simon Glass <sjg at chromium.org> wrote:
>
> Hi,
>
> On Tue, 10 Dec 2024 at 02:54, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > On Sun, 8 Dec 2024 at 16:22, Ilias Apalodimas
> > <ilias.apalodimas at 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 at 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%
>
> 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.
FWIW you need to apply the entire series, not just a single 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.
/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/f79dc1fbf796dd5ad290f6080608ee68d7652cfc.1730452668.git.michal.simek@amd.com/
More information about the U-Boot
mailing list