Hi Tom,
On Tue, 14 Jan 2025 at 18:41, Tom Rini <tr...@konsulko.com> wrote:
>
> On Tue, Jan 14, 2025 at 06:18:26PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 14 Jan 2025 at 10:33, Tom Rini <tr...@konsulko.com> wrote:
> > >
> > > On Tue, Jan 14, 2025 at 10:58:01AM -0600, Tom Rini wrote:
> > > > 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 <tr...@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.
> >
> > OK. I used to do this, but with CI sometimes grabbing the machines it
> > ends up with a huge load and sometimes runs out of memory. I suppose I
> > can stop the runner before doing it and remember to restart it
> > afterwards. Or perhaps Alex has enough memory per thread? (96GB).
>
> Personally, I would make the script that does the world build for size
> test turn off the runner before starting and turn it back on when done
> otherwise build time will suffer greatly and you would have been better
> served doing it all on a slower class of machine.
>
> >
> > > >
> > > > > 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.
> >
> > OK, I see. It is actually one level deeper than I thought.
> >
> > PARTITIONS is not enabled, for example on phycore_am62x_r5_usbdfu
> > which means that blk_get_dev() returns NULL
> >
> > So long as all the code is in one module, blk_get_dev() returning NULL
> > causes the calls to vbe_simple_read_state() and simple_read_nvdata()
> > to be dropped, so their code is dropped too, so there is no
> > blk_read()/blk_dread() call. It is optimised away. It just happens to
> > be the only call in U-Boot
>
> Ah, that is all also true, thanks.
>
> > > > > > 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.
> > >
> > > So, digging in to this, what's going on is that the platforms I've
noted
> > > here (and a few others) don't actually enable any block devices.
That's
> > > why, today, with VBE on still, they discard all of the code still.
Your
> > > rework means that while there's still no block devices, we're now
> > > including the code. What to do? Both of:
> > > - These platforms *should* disable the assorted block device related
> > > library options they have enabled if / when possible (it might not
be
> > > a neat and easy un-tangle).
> >
> > As above, the issue seems to be PARTITIONS
>
> Yes, I think we both agree there's a number of useless functionality
> enabled on these handful of platforms. PARTITIONS explains how it gets
> optimized away and all of the useless block-related things like MMC
> core should also be dropped.
>
> > > - VBE needs to handle the case where there's not a block device
enabled
> > > and not fail the build. Part of this requires the Kconfig rework I
> > > linked to earlier (so that we can have BLK off) to be complete, but
> > > having the VBE code check for BLK being enabled either in Makefile
or
> > > C code should be the same regardless.
> >
> > I thought you NAK'd my patch to drop the 'depends on BLK' for bootstd?
>
> I know I NAK'd
>
https://patchwork.ozlabs.org/project/uboot/patch/20241113150938.1534931-2-...@chromium.org/
> as that should be instead done with
> https://patchwork.ozlabs.org/project/uboot/list/?series=440336 instead.
>
> With that series then yes, you should be able to have BOOTSTD not depend
> on BLK. But in a practical sense I worry that your changes in this
> series will break with BLK=n. My point above is to please make sure it
> still works in that case.
>
> > From what I can tell EFI_LOADER doesn't do any block access unless
> > PARTITIONS is enabled. I don't want that for VBE because the VBE data
> > is not actually in a partition (although perhaps it could be in
> > future).
>
> In a practical sense EFI_LOADER depends on BLK and I've fixed the
> dependency with my series and talked with Ilias and Heinrich about it.
> Making EFI_LOADER buildable without BLK is a mechanical effort, but
> making it usefully functional still might require a bit more work and
> they'll cross that bridge when they come to it.
>
> > Anyway, from what I can tell, the code-size increase is explainable
> > and perhaps we should just accept it.
>
> Other than the double buffer one, yes, I suppose it's not unreasonable
> to ask me to fixup the oddball platforms with extraneous PARTITIONS and
> other block type things enabled. Still need to get back to the buffer
> one as 200 bytes isn't normal for re-organizing things such that the
> compiler can't decide to inline something.
What is the double buffer one?
I have a v2 of this series which splits the first patch up a bit more. But
the end result is the same.
Regards,
Simon
