[PATCH 00/10] bootm: Clean up arch-specific, pre-OS clean-up

[Peter Robinson]

Hi Simon,

> > > Each arch does something slightly different before booting the OS. Some
> > > archs even do different things depending on the CPU type.
> > >
> > > EFT_LOADER has its own special code, regardless of which arch is being
> > > booted.
> > >
> > > It is quite hard to know what actually happens in the final milliseconds
> > > before the OS boot.
> > >
> > > This series attempts to clean up U-Boot in this area.
> > >
> > > The basic approach is to create a new bootm_final() function which is
> > > called by all archs. It provides some flags for a couple of necessary
> > > variations but otherwise it is generic.
> > >
> > > RISC-V, x86 and ARM are converted over to use this new function. For
> > > consistency, EFI loader is converted as well.
> > >
> > > A noteable change is that EFI_LOADER now does bootstage processing
> > > before boot, if enabled, thus producing a report.
> >
> > Why is the change needed, I don't see it explained in the patches or
> > here and what the implications might be.
>
> At present EFI_LOADER ignores bootstage and so there is no timing
> report produced even if enabled. It was probably an oversight.

So please include that level of detail.

> > > There is also a patch to drop EFI_GRUB_ARM32_WORKAROUND, as discussed
> > > recently on the mailing list.
> >
> > That appears to be unrelated to this series, it should be a separate
> > standalone patch else it needs to be described here why it's a
> > dependency, and even then likely still posted separately as a
> > dependency of this series.
>
> In what way is it unrelated? In my view this is related as it is a
> pre-OS cleanup. What do you mean by dependency here?

It is unrelated because if you remove it from the series the series
would still generally work as expected and if I apply the patch on
it's own without the rest of the series it works as expected. It is
only related by a vague association hence it should be sent separately
and discussed on it's own merit.

> > > Future work could take this a little further:
> > > - Drop board_quiesce_devices() and rely on driver model for that
> > > - Similarly with udc_disconnect(),
> > > - cleanup_before_linux() could use more details as to what it is
> > > supposed to do, to reduce the number of arch-specific variations
> >
> > What do you mean by that statement.
>
> Well, the function does different things on differents archs and is
> mostly uncommented (apart from an ARM-specific note in a README).
> Ideally it would be commented with exactly what it should do, ideally
> at a high level so it is not just arch-specific.

Well I would expect different arches to potentially do different
things, but your response doesn't explain to me why doing the same
thing is the right thing to do, why not doing the different things
won't affect the arrch, I want to know why the details of change is
good not a hand wavy thing (you yourself have asked others to do
similar explanations in the past too).

> > > Note: This is the first 10 patches for an 18-part series from the
> > > Concept tree, as requested. The patches after that complete the cleanup
> > > on ARM and RISC-V.
> > >
> > >
> > > Simon Glass (10):
> > >   boot: Create a function for final pre-boot steps
> > >   efi: Make use of bootm_final()
> > >   bootm: Add a message when booting
> > >   bootstage: Add some missing dummy functions
> > >   bootm: Do bootstage processing in bootm_final()
> > >   x86: Call bootm_final()
> > >   riscv: Drop board_quiesce_devices()
> > >   riscv: Call bootm_final()
> > >   efi: Drop EFI_GRUB_ARM32_WORKAROUND
> > >   x86: Add cleanup_before_linux()
> > >
> > >  MAINTAINERS                           |  1 +
> > >  arch/riscv/include/asm/u-boot-riscv.h |  1 -
> > >  arch/riscv/lib/bootm.c                | 24 +------------------
> > >  arch/x86/cpu/cpu.c                    |  5 ++++
> > >  arch/x86/lib/bootm.c                  | 14 +----------
> > >  boot/Makefile                         |  2 +-
> > >  boot/bootm_final.c                    | 34 +++++++++++++++++++++++++++
> > >  configs/mt7623n_bpir2_defconfig       |  1 -
> > >  include/bootm.h                       | 17 ++++++++++++++
> > >  include/bootstage.h                   |  8 +++++++
> > >  lib/efi_loader/Kconfig                | 10 --------
> > >  lib/efi_loader/efi_boottime.c         | 30 +----------------------
> > >  test/boot/bootflow.c                  |  2 +-
> > >  13 files changed, 70 insertions(+), 79 deletions(-)
> > >  create mode 100644 boot/bootm_final.c
> > >
> > > --
> > > 2.43.0
> > >
> > > base-commit: 6cdd7597a2fbfc1572c1b0af23d3daf1cefa2de7
> > > branch: bootma-us