Hi Tom, On Fri, 10 Jan 2025 at 09:17, Tom Rini <tr...@konsulko.com> wrote: > > On Fri, Jan 10, 2025 at 06:39:11AM -0700, Simon Glass wrote: > > Hi Tom, > > > > On Thu, 9 Jan 2025 at 11:08, Tom Rini <tr...@konsulko.com> wrote: > > > > > > On Thu, Jan 09, 2025 at 08:14:53AM -0700, Simon Glass wrote: > > > > Hi Tom, > > > > > > > > On Thu, 9 Jan 2025 at 08:10, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > > > On Thu, Jan 09, 2025 at 05:36:03AM -0700, Simon Glass wrote: > > > > > > Hi Tom, > > > > > > > > > > > > On Wed, 8 Jan 2025 at 11:25, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > > > > > > > On Mon, Jan 06, 2025 at 07:32:13AM -0700, Simon Glass wrote: > > > > > > > > > > > > > > > This function uses separate arguments for data and size. Use > > > > > > > > the new > > > > > > > > abuf instead, so that they are paired and in one place. In some > > > > > > > > cases it > > > > > > > > also saves an argument, thus potentially reducing code size. > > > > > > > > > > > > > > This is one of the commits that globally increases size in both > > > > > > > full > > > > > > > U-Boot and SPL/etc. > > > > > > > > > > > > > > Is all of the "abuf" changes just a "tidy up" that increases the > > > > > > > code a > > > > > > > bit? > > > > > > > > > > > > Yes, a tidy-up which I hope will help overall. I have been thinking > > > > > > for a while of how to avoid having addr/size and ptr/size passed > > > > > > everywhere. For now abuf seems to provide some sort of solution. > > > > > > > > > > > > I see this: > > > > > > > > > > > > 18: boot: Update fit_image_get_emb_data to use abuf > > > > > > aarch64: (for 1/1 boards) all +4.0 bss -24.0 spl/u-boot-spl:all > > > > > > +16.0 spl/u-boot-spl:text +16.0 text +28.0 > > > > > > > > > > > > so growth on firefly-rk3399 but not with rk3288. I am not sure if > > > > > > the > > > > > > growth will tail off as there are more users, though. We might even > > > > > > be > > > > > > able to be more clever with static inlines. > > > > > > > > > > Yeah, lets not do this now then and worry about some "clean up" later > > > > > when we can show that it does, or does not, improve size. > > > > > > > > Oh. > > > > > > > > > And there's > > > > > something wrong with your numbers: > > > > > 01: Fix neighbor discovery ethernet address saving > > > > > aarch64: w+ firefly-rk3399 > > > > > +(firefly-rk3399) Image 'simple-bin' is missing external blobs and is > > > > > non-functional: atf-bl31 > > > > > +(firefly-rk3399) > > > > > +(firefly-rk3399) /binman/simple-bin/fit/images/@atf-SEQ/atf-bl31 > > > > > (atf-bl31): > > > > > +(firefly-rk3399) See the documentation for your board. You may > > > > > need to build ARM Trusted > > > > > +(firefly-rk3399) Firmware and build with BL31=/path/to/bl31.bin > > > > > +(firefly-rk3399) Image 'simple-bin' is missing optional external > > > > > blobs but is still functional: tee-os > > > > > +(firefly-rk3399) /binman/simple-bin/fit/images/@tee-SEQ/tee-os > > > > > (tee-os): > > > > > +(firefly-rk3399) See the documentation for your board. You may > > > > > need to build Open Portable > > > > > +(firefly-rk3399) Trusted Execution Environment (OP-TEE) and build > > > > > with TEE=/path/to/tee.bin > > > > > +(firefly-rk3399) Some images are invalid > > > > > 37: dm: core: Provide ofnode_find_subnode_unit() > > > > > aarch64: (for 1/1 boards) all +324.0 bss +32.0 spl/u-boot-spl:all > > > > > +16.0 spl/u-boot-spl:text +16.0 text +292.0 > > > > > firefly-rk3399 : all +324 bss +32 spl/u-boot-spl:all +16 > > > > > spl/u-boot-spl:text +16 text +292 > > > > > u-boot: add: 6/-1, grow: 4/-4 bytes: 516/-224 (292) > > > > > function old > > > > > new delta > > > > > ofnode_name_eq_unit - > > > > > 160 +160 > > > > > ofnode_find_subnode_unit - > > > > > 116 +116 > > > > > fit_image_get_data 80 > > > > > 176 +96 > > > > > fit_image_get_emb_data - > > > > > 84 +84 > > > > > ofnode_write_prop 224 > > > > > 236 +12 > > > > > ofnode_add_subnode 232 > > > > > 244 +12 > > > > > abuf_init_const - > > > > > 12 +12 > > > > > abuf_init - > > > > > 12 +12 > > > > > abuf_addr - > > > > > 8 +8 > > > > > fit_image_print 780 > > > > > 784 +4 > > > > > image_locate_script 696 > > > > > 692 -4 > > > > > fit_image_load 1584 > > > > > 1580 -4 > > > > > fit_image_verify 176 > > > > > 164 -12 > > > > > ofnode_find_subnode 140 > > > > > 116 -24 > > > > > fit_image_get_data_and_size 180 > > > > > - -180 > > > > > spl-u-boot-spl: add: 3/-1, grow: 0/-1 bytes: 108/-92 > > > > > (16) > > > > > function old > > > > > new delta > > > > > fit_image_get_emb_data - > > > > > 84 +84 > > > > > abuf_init_const - > > > > > 12 +12 > > > > > abuf_init - > > > > > 12 +12 > > > > > load_simple_fit 580 > > > > > 568 -12 > > > > > fit_image_get_data 80 > > > > > - -80 > > > > > > > > Yes, that's the whole series, so not related to this change. > > > > > > Yes, that's the whole series including this change, so it's related to > > > this change. > > > > Right, but it is due to ofnode_find_subnode(), etc. > > > > > > > > > I elected to have two versions of ofnode_find_subnode() to avoid the > > > > size growth in the previous version. But the cost is larger size > > > > growth when OF_LIVE is used. > > > > > > > > Without OF_LIVE, the size growth is tiny. > > > > > > And even worse in SPL, somehow. But you want more OF_LIVE users, not > > > less, yes? > > > > Well, OF_LIVE is always quite a bit larger, at least at the moment. It > > has both Linux's of_access stuff and libfdt. It's not OF_LIVE I am > > bothered about, but I do want people using ofnode. Unfortunately > > people still send patches which use libfdt directly. > > > > > > > > > So...what to do? > > > > > > Well, if you drop the abuf changes for now, SPL won't change at all for > > > most platforms and that'll be an improvement. > > > > Yes, I'll look at that. This is one of many examples where I have a > > problem and realise that we need a nicer way of dealing with it, then > > implement it in the series, but then the series loses focus. So then I > > take it out again, then forget about it until next time, but I never > > actually make the change. > > I hate to start to derail this, but refactor for "nicer code" is very > much subjective. Especially when it also grows the code (and it's not > clear that wider usage would result in shrinkage). So yes, this really > needs to be put aside and also part of why I keep asking for one thing > at a time.
Yes I very much agree with this. > > > > And I'm going to keep complaining about size growth here because a > > > non-trivial subset of users just wants things to boot quickly and be > > > small. > > > > Yes, you won't get any complaints from me on that. I did propose some > > automated checking a few years back, but it never went anywhere. > > It be great if buildman size comparison had some way to csv the output. > That's what's missing imo from being able to have some automation or > even just nicer tooling. What sort of tooling could we have? I would like something in CI which reports code-size changes in a useful way, perhaps failing if the delta is too large for more than x boards. WDYT? Regards, Simon
