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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4608 bytes --]

On Mon, Jan 13, 2025 at 05:13:46PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 13 Jan 2025 at 13:44, Tom Rini <trini@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@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@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/

You're right, I replied to the wrong patch here, sorry for the
confusion. I'll move some of my comments in reply to the correct patch
now.

[snip]
> > > > 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

Yes, but 200 bytes isn't just function call overhead. Some of that might
be from going from one ALLOC_CACHE_ALIGN_BUFFER(u8, buf,
MMC_MAX_BLOCK_LEN) to two?

> > > 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?

It sounds like you're talking about re-ordering patches and I'm asking
you to re-work your re-work of the code so that it doesn't grow things
without explanation, and minimizes growth. Maybe that's code changes,
maybe that's better commit messages, likely it's a combination of both.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]