[RFC PATCH 1/7] lmb: Replace lmb_reserve() with lmb_reserve_flags()
Simon Glass
sjg at chromium.org
Tue Dec 10 19:58:26 CET 2024
Hi Ilias,
On Tue, 10 Dec 2024 at 11:00, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> 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%
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.
>
> /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/
Regards,
SImon
More information about the U-Boot
mailing list