[PATCH 02/15] vbe: Split out reading a FIT into a common file

[Simon Glass]

Hi Tom,

On Mon, 13 Jan 2025 at 13:44, Tom Rini <trini at konsulko.com> wrote:
>
> On Mon, Jan 13, 2025 at 01:03:52PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Sat, 11 Jan 2025 at 15:54, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Thu, Jan 09, 2025 at 05:29:57AM -0700, Simon Glass wrote:
> > >
> > > > Loading a FIT is useful for other VBE methods, such as ABrec. Create a
> > > > new function to handling reading it.
> > > >
> > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > >
> > > This causes a bunch of growth:
> > >             a3y17lte       : all +1328 text +1328
> > >                u-boot: add: 8/0, grow: 1/0 bytes: 1328/0 (1328)
> > >                  function                                   old     new   delta
> > >                  blkcache_fill                                -     332    +332
> > >                  blkcache_read                                -     240    +240
> > >                  blk_read                                     -     188    +188
> > >                  vbe_read_nvdata                              -     156    +156
> > >                  vbe_read_version                             -     140    +140
> > >                  vbe_get_blk                                  -     100    +100
> > >                  simple_read_nvdata                           -      96     +96
> > >                  crc8                                         -      72     +72
> > >                  vbe_simple_read_state                      108     112      +4
> > >
> > > Which is unexpected for just moving code around that's not newly used.
> >
> > I hadn't noticed that on the boards I was trying, so thank you for spotting it.
> >
> > This is because it now uses blk_read() instead of blk_dread(), so if
>
> That's not what this patch does? There's no caller before or after in
> this patch of "blk_dread". Just moving functions around should not
> increase size on platforms that weren't using the existing
> functionality.

Firstly, are we looking at the same patch? Here is the one I am looking at:

https://patchwork.ozlabs.org/project/uboot/patch/20250109123010.4005298-2-sjg@chromium.org/

It has blk_dread() in the old code and blk_read() in the new.

> Why is vbe_simple_read_state changing at all here, when
> it's not being touched?

Again, I'm not sure we are looking at the same code. In my version, it
does get changed, since it now calls vbe_get_blk() rather than finding
the block device itself. This is to avoid duplicating that code in
simple and abrec.

>
> > BLOCK_CACHE is enabled, it will use the block cache. We could disable
> > BLOCK_CACHE on those boards perhaps? It is a speed optimisation so
> > shouldn't be used by boards which care about code size.
> >
> > > And even when it's just a move it's still growing:
> > >             xilinx_zynqmp_virt: all +128 bss -72 text +200
> > >                u-boot: add: 4/0, grow: 0/-1 bytes: 540/-340 (200)
> > >                  function                                   old     new   delta
> > >                  vbe_read_nvdata                              -     156    +156
> > >                  vbe_get_blk                                  -     148    +148
> > >                  vbe_read_version                             -     140    +140
> > >                  simple_read_nvdata                           -      96     +96
> > >                  vbe_simple_read_state                      452     112    -340
> >
> > Unfortunately this one is hard to fix. As you know, whenever you take
> > code from a single module and put it into another, the compiler cannot
> > optimise away the function-call overhead. I'll note that there is no
> > increase when LTO is used, e.g. with xilinx_versal_net_mini_qspi
> >
> > So let me know what you think.
>
> You likely need to re-think your refactor a bit then. If it's in part G
> or H that we have more than one caller of any of these functions, that's
> perhaps where it's time to refactor and expose them?

Yes of course I can move things around. I would need to drop the FIT
loader as well, so this series would become quite small. Shall I do
that for the next version?

But just so that I understand...in the next series, when abrec is
added, and I have these same patches, will you accept this size
increase?

Regards,
Simon