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

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20231211/8e45f8f6/attachment.sig>