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: 3976 bytes --]

On Mon, Jan 13, 2025 at 07:22:19PM -0600, Tom Rini wrote:
> 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?

This is the double buffer I was referring to.

-- 
Tom

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