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-sj...
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