On Tue, Jan 14, 2025 at 05:58:04AM -0700, Simon Glass wrote:
Hi Tom,
On Mon, 13 Jan 2025 at 18:22, Tom Rini trini@konsulko.com wrote:
On Thu, Jan 09, 2025 at 05:29:56AM -0700, Simon Glass wrote:
Loading a FIT is useful for other VBE methods, such as ABrec, so start a new common file. Add functions for reading the version and nvdata. Also add a function to get the block device.
This is not a good commit message when you're also introducing functional changes (blk_dread -> blk_read).
Yes, I did not actually notice this size change at all, as mentioned, as I only built for a few boards (firefly rk3399/rk3288 and a few sandbox ones). The blk_dread() function is the old API for reading
You have much faster build machines available than I do, it would be good to run a world build before/after (it might take an hour on alexandra) before posting these big series.
from a block. It didn't occur to me that blk_dread() would bypass the cache.
Yes, how is that happening? That's what doesn't make sense.
Further, this functional change seems to introduce some other code being pulled in very unexpectedly, and that needs to be explained. For example, phycore_am62x_r5_usbdfu is another case and that has CONFIG_BLOCK_CACHE=y but CONFIG_SPL_BLOCK_CACHE=n, and yet the size growth is in full U-Boot.
This board currently uses blk_dread(), but not blk_read(), so the addition of a blk_read() call in non-SPL causes the BLK cache to be pulled in.
But, how? I mean, this sounds like some underlying bug that needs to be fixed. The platform sets CONFIG_BLK=y and CONFIG_BLOCK_CACHE=y and drivers/block/blk-uclass.c has: ulong blk_dread(struct blk_desc *desc, lbaint_t start, lbaint_t blkcnt, void *buffer) { return blk_read(desc->bdev, start, blkcnt, buffer); }
Or perhaps the answer / problem here is that I need to track down what's going wrong here instead of asking you to track this down.
Just moving code should not result in unexpected size change.
That's correct, so long as 'expected' size changes includes the normal function-call overhead and loss of intra-module optimisation.
Yes, but that's not what this series shows. There's (a) the above bug and (b) you might be allocating buffers twice now instead of once?
So, what to do here? I certainly don't want to use the old blk API.
Yes. I'm not 100% sure, especially after https://patchwork.ozlabs.org/project/uboot/list/?series=437816&state=* which I need to v2 what the non-SPL case is. And for new features, sure, saying SPL_BLK is required too is fine. Heck, it *may* even be the case that only TPL_BLK=n is required to be valid, all of that would require other investigation and removals.
But I can perhaps do a patch which changes VBE to use the new one, first, so the size growth is within that commit. I looked up BLOCK_CACHE and it is used by 90% of boards.
Yes, BLOCK_CACHE should be used by everyone at this point, it was one of those cases of "this grows the code but it's a universal speedup".