[PATCH v3 22/22] bootm: Create a new boot_run() function to handle booting

[Simon Glass]

Hi Tom,

On Sat, 16 Dec 2023 at 10:12, Tom Rini <trini at konsulko.com> wrote:
>
> On Fri, Dec 15, 2023 at 08:14:26PM -0700, Simon Glass wrote:
> > Create a common function used by the three existing bootz/i/m_run()
> > functions, to reduce duplicated code.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > Suggested-by: Tom Rini <trini at konsulko.com>
> > ---
> >
> > Changes in v3:
> > - Add new boot_run() function
> >
> >  boot/bootm.c    | 40 ++++++++++++++--------------------------
> >  include/bootm.h | 18 ++++++++++++++++++
> >  2 files changed, 32 insertions(+), 26 deletions(-)
> >
> > diff --git a/boot/bootm.c b/boot/bootm.c
> > index 53236136f489..6a4cebcf7a08 100644
> > --- a/boot/bootm.c
> > +++ b/boot/bootm.c
> > @@ -1123,47 +1123,35 @@ err:
> >       return ret;
> >  }
> >
> > -int bootm_run(struct bootm_info *bmi)
> > +int boot_run(struct bootm_info *bmi, const char *cmd, int extra_states)
> >  {
> >       int states;
> >
> > -     bmi->cmd_name = "bootm";
> > -     states = BOOTM_STATE_START | BOOTM_STATE_FINDOS | BOOTM_STATE_PRE_LOAD |
> > -             BOOTM_STATE_FINDOTHER | BOOTM_STATE_LOADOS |
> > -             BOOTM_STATE_OS_PREP | BOOTM_STATE_OS_FAKE_GO |
> > -             BOOTM_STATE_OS_GO | BOOTM_STATE_MEASURE;
> > +     bmi->cmd_name = cmd;
> > +     states = BOOTM_STATE_MEASURE | BOOTM_STATE_OS_PREP |
> > +             BOOTM_STATE_OS_FAKE_GO | BOOTM_STATE_OS_GO;
> >       if (IS_ENABLED(CONFIG_SYS_BOOT_RAMDISK_HIGH))
> >               states |= BOOTM_STATE_RAMDISK;
> > -     if (IS_ENABLED(CONFIG_PPC) || IS_ENABLED(CONFIG_MIPS))
> > -             states |= BOOTM_STATE_OS_CMDLINE;
> > +     states |= extra_states;
> >
> >       return bootm_run_states(bmi, states);
> >  }
> >
> > -int bootz_run(struct bootm_info *bmi)
> > +int bootm_run(struct bootm_info *bmi)
> >  {
> > -     int states;
> > -
> > -     bmi->cmd_name = "bootz";
> > -     states = BOOTM_STATE_MEASURE | BOOTM_STATE_OS_PREP |
> > -             BOOTM_STATE_OS_FAKE_GO | BOOTM_STATE_OS_GO;
> > -     if (IS_ENABLED(CONFIG_SYS_BOOT_RAMDISK_HIGH))
> > -             states |= BOOTM_STATE_RAMDISK;
> > +     return boot_run(bmi, "bootm", BOOTM_STATE_START | BOOTM_STATE_FINDOS |
> > +                     BOOTM_STATE_PRE_LOAD | BOOTM_STATE_FINDOTHER |
> > +                     BOOTM_STATE_LOADOS);
> > +}
> >
> > -     return bootm_run_states(bmi, states);
> > +int bootz_run(struct bootm_info *bmi)
> > +{
> > +     return boot_run(bmi, "bootz", 0);
> >  }
> >
> >  int booti_run(struct bootm_info *bmi)
> >  {
> > -     int states;
> > -
> > -     bmi->cmd_name = "booti";
> > -     states = BOOTM_STATE_MEASURE | BOOTM_STATE_OS_PREP |
> > -             BOOTM_STATE_OS_FAKE_GO | BOOTM_STATE_OS_GO;
> > -     if (IS_ENABLED(CONFIG_SYS_BOOT_RAMDISK_HIGH))
> > -             states |= BOOTM_STATE_RAMDISK;
> > -
> > -     return bootm_run_states(bmi, states);
> > +     return boot_run(bmi, "booti", 0);
> >  }
> >
> >  int bootm_boot_start(ulong addr, const char *cmdline)
> > diff --git a/include/bootm.h b/include/bootm.h
> > index eba35b33b4e5..9e0f8d60de0a 100644
> > --- a/include/bootm.h
> > +++ b/include/bootm.h
> > @@ -150,6 +150,24 @@ int bootm_measure(struct bootm_headers *images);
> >   */
> >  int bootm_run_states(struct bootm_info *bmi, int states);
> >
> > +/**
> > + * boot_run() - Run the entire bootm/booti/bootz process
> > + *
> > + * This runs through the boot process from start to finish, with a base set of
> > + * states, along with the extra ones supplied.
> > + *
> > + * This uses bootm_run_states().
> > + *
> > + * Note that it is normally easier to use bootm_run(), etc. since they handle
> > + * the extra states correctly.
> > + *
> > + * @bmi: bootm information
> > + * @cmd: command being run, NULL if none
> > + * @extra_states: Mask of extra states to use for the boot
> > + * Return: 0 if ok, something else on error
> > + */
> > +int boot_run(struct bootm_info *bmi, const char *cmd, int extra_states);
> > +
> >  /**
> >   * bootm_run() - Run the entire bootm process
> >   *
>
> Overall good, thanks. My question is, should we mark the others as
> static, or is this new helper static and not to be called externally?

The others are used externally.

For bootm_run() I toyed with the idea of it being static, but then
wondered if we might use that programmatically instead of the
bootz/i/m(_run) versions. It is quite nice that the differences are
now pretty minor between them.

I don't mind either way, though.

Regards,
Simon