| Message ID | 20231216031446.2222362-17-sjg@chromium.org |
|---|---|
| State | Accepted |
| Commit | d37086a95f4b2c39f9ecfe602b792df8ab3bd8f9 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Complete decoupling of bootm logic from commands | expand |
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@chromium.org> > Suggested-by: Tom Rini <trini@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?
Hi Tom, On Sat, 16 Dec 2023 at 10:12, Tom Rini <trini@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@chromium.org> > > Suggested-by: Tom Rini <trini@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
On Sat, Dec 16, 2023 at 11:45:36AM -0700, Simon Glass wrote: > Hi Tom, > > On Sat, 16 Dec 2023 at 10:12, Tom Rini <trini@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@chromium.org> > > > Suggested-by: Tom Rini <trini@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. OK, lets put it on the TODO list for once the CMDLINE=n case is fully flushed out and code otherwise cleaned up and organized, it'll perhaps be clearer then if the answer is old functions, new function or neither function. Reviewed-by: Tom Rini <trini@konsulko.com>
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 *
Create a common function used by the three existing bootz/i/m_run() functions, to reduce duplicated code. Signed-off-by: Simon Glass <sjg@chromium.org> Suggested-by: Tom Rini <trini@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(-)