[PATCH v2 17/33] boot: Update fit_image_get_emb_data to use abuf
Tom Rini
trini at konsulko.com
Fri Jan 10 17:17:32 CET 2025
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 <trini at 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 <trini at 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 <trini at 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.
> > 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.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20250110/e0e37620/attachment.sig>
More information about the U-Boot
mailing list