[PATCH v3] arch: arm: Kconfig: Enable BOOTSTD_FULL for Rockchip SoCs

[Simon Glass]

Hi Tom,

On Mon, 11 Dec 2023 at 11:19, Tom Rini <trini at konsulko.com> wrote:
>
> On Mon, Dec 11, 2023 at 10:52:12AM -0700, Simon Glass wrote:
> > Hi,
> >
> > On Sat, 9 Dec 2023 at 15:19, Shantur Rathore <i at shantur.com> wrote:
> > >
> > > On Sat, Dec 9, 2023 at 8:56 PM Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Fri, Dec 08, 2023 at 01:59:26PM +0000, Shantur Rathore wrote:
> > > > > Hi Peter,
> > > > >
> > > > > On Fri, Dec 8, 2023 at 12:59 PM Peter Robinson <pbrobinson at gmail.com> wrote:
> > > > > >
> > > > > > On Fri, Dec 8, 2023 at 12:52 PM Shantur Rathore <i at shantur.com> wrote:
> > > > > > >
> > > > > > > Hi Jagan,
> > > > > > >
> > > > > > > On Fri, Dec 8, 2023 at 11:13 AM Jagan Teki <jagan at amarulasolutions.com> wrote:
> > > > > > > >
> > > > > > > > On Sun, Nov 19, 2023 at 10:54 PM Shantur Rathore <i at shantur.com> wrote:
> > > > > > > > >
> > > > > > > > > Rockchip SoCs can support wide range of bootflows.
> > > > > > > > > Without full bootflow commands, it can be difficult to
> > > > > > > > > figure out issues if any, hence enable by default.
> > > > > > > > >
> > > > > > > > > Reviewed-by: Simon Glass <sjg at chromium.org>
> > > > > > > > >
> > > > > > > > > Signed-off-by: Shantur Rathore <i at shantur.com>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > (no changes since v1)
> > > > > > > > >
> > > > > > > > >  arch/arm/Kconfig | 1 +
> > > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > >
> > > > > > > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > > > > > > > index d812685c98..fca6ef6d7e 100644
> > > > > > > > > --- a/arch/arm/Kconfig
> > > > > > > > > +++ b/arch/arm/Kconfig
> > > > > > > > > @@ -1986,6 +1986,7 @@ config ARCH_ROCKCHIP
> > > > > > > > >         imply CMD_DM
> > > > > > > > >         imply DEBUG_UART_BOARD_INIT
> > > > > > > > >         imply BOOTSTD_DEFAULTS
> > > > > > > > > +       imply BOOTSTD_FULL if BOOTSTD_DEFAULTS
> > > > > > > >
> > > > > > > > Yes, but better to give this option to specific board vendors as
> > > > > > > > defaults are enough to boot 1st bootflow and what ever media's it has.
> > > > > > > >
> > > > > > >
> > > > > > > Yes, that's correct it is enough to boot but by default there is no option
> > > > > > > to choose what to boot from.
> > > > > > > This was discussed in an earlier version of this patch [0] where I was
> > > > > > > explicitly enabling only for RP64.
> > > > > >
> > > > > > What actual extra functionality does this provide, what is the impact
> > > > > > on the size of images? You've not provided reasonable justification
> > > > > > outside of very vague statements, it would be useful to know what the
> > > > > > added options actually solves.
> > > > >
> > > > > BOOTSTD_FULL enables all the options in bootflow commands. This is needed if
> > > > > - you want to choose between multiple available bootflows rather than
> > > > > just boot one default.
> > > > > - if you need to list the available bootflows that bootstd has found
> > > > > - if you need to select and boot any bootflow other than default.
> > > > > By default all other commands in U-boot come with options to show details
> > > > > For example, nvme info, nvme detail, usb info, usb tree but with
> > > > > bootstd no way to know anything.
> > > > >
> > > > > Image size - u-boot.itd without BOOTSTD_FULL - 1193984 bytes
> > > > > Image size - u-boot.itb with BOOTSTD_FULL - 1214976 bytes
> > > > > Difference - 20992 bytes
> > > > >
> > > > > According to binman for RK3399 u-boot can take upto 4M [1] so we have
> > > > > ample space.
> > > > >
> > > > > This was discussed in the previous patch in the link below [0]
> > > > >
> > > > > [0] - https://patchwork.ozlabs.org/project/uboot/patch/20231111001329.537704-1-i@shantur.com/
> > > > > [1] - https://github.com/shantur/u-boot/blob/master/arch/arm/dts/rk3399-u-boot.dtsi#L68
> > > >
> > > > If I'm recalling everything right, this also brings "bootflow" as a
> > > > command more in line with what could be done with distro_bootcmd in
> > > > terms of "cover every possible case and let the user override things"
> > > >
> > > Yes,
> > > That's correct.
> > >
> > > U_BOOT_LONGHELP(bootflow,
> > > #ifdef CONFIG_CMD_BOOTFLOW_FULL
> > > "scan [-abeGl] [bdev]  - scan for valid bootflows (-l list, -a all, -e
> > > errors, -b boot, -G no global)\n"
> > > "bootflow list [-e]             - list scanned bootflows (-e errors)\n"
> > > "bootflow select [<num>|<name>] - select a bootflow\n"
> > > "bootflow info [-ds]            - show info on current bootflow (-d
> > > dump bootflow)\n"
> > > "bootflow read                  - read all current-bootflow files\n"
> > > "bootflow boot                  - boot current bootflow\n"
> > > "bootflow menu [-t]             - show a menu of available bootflows\n"
> > > "bootflow cmdline [set|get|clear|delete|auto] <param> [<value>] -
> > > update cmdline"
> > > #else
> > > "scan - boot first available bootflow\n"
> > > #endif
> > > );
> >
> > I suggest we keep it off for now, as we did put in quite a bit of
> > effort to reduce code size. It would be a shame to throw it all away.
> >
> > That said, I can imagine this becoming a pain for people over time, as
> > the 'bootflow' command becomes the common way to interact with U-Boot.
> > I just wonder if it is too early to make the switch?
>
> I think in hind-sight too much stuff is omitted without BOOTSTD_FULL.
> The option itself then enables other stuff too by default, but some
> parts of the bootflow command itself should be visible even without FULL
> to make things easier on the user.

At the time the goal was to avoid growth compared to the distro
scripts. We could perhaps add some more things in with BOOTSTD_FULL
but still have it as an option?

Regards,
Simon