[PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Tom Rini]

On Tue, Jul 12, 2022 at 11:52:45PM +0200, Pali Rohár wrote:
> On Tuesday 12 July 2022 17:39:06 Tom Rini wrote:
> > On Tue, Jul 12, 2022 at 10:11:00AM +0200, Pali Rohár wrote:
> > > On Monday 11 July 2022 19:23:26 Tom Rini wrote:
> > > > On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote:
> > > > 
> > > > > CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list.
> > > > > Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does
> > > > > not do anything useful (it is infinite recursion). Config file for this
> > > > > board already contains default preboot= env variable with correct value,
> > > > > which has higher priority than CONFIG_PREBOOT and this is reason why
> > > > > nonsense CONFIG_PREBOOT is ignored.
> > > > > 
> > > > > Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file.
> > > > > 
> > > > > Signed-off-by: Pali Rohár <pali at kernel.org>
> > > > > ---
> > > > >  configs/nokia_rx51_defconfig | 1 -
> > > > >  1 file changed, 1 deletion(-)
> > > > > 
> > > > > diff --git a/configs/nokia_rx51_defconfig b/configs/nokia_rx51_defconfig
> > > > > index 309cf28269c1..46b794f168d9 100644
> > > > > --- a/configs/nokia_rx51_defconfig
> > > > > +++ b/configs/nokia_rx51_defconfig
> > > > > @@ -24,7 +24,6 @@ CONFIG_AUTOBOOT_MENU_SHOW=y
> > > > >  CONFIG_USE_BOOTCOMMAND=y
> > > > >  CONFIG_BOOTCOMMAND="run sdboot;run emmcboot;run attachboot;echo"
> > > > >  CONFIG_USE_PREBOOT=y
> > > > > -CONFIG_PREBOOT="run preboot"
> > > > >  # CONFIG_SYS_DEVICE_NULLDEV is not set
> > > > >  CONFIG_HUSH_PARSER=y
> > > > >  CONFIG_SYS_PROMPT="Nokia RX-51 # "
> > > > 
> > > > These changes are actually a bit puzzling.  There are other platforms
> > > > that set preboot in their default environment, rather than via
> > > > CONFIG_PREBOOT, and their final value ends up being the one set in
> > > > CONFIG_EXTRA_ENV_SETTINGS rather than the empty string that
> > > > CONFIG_PREBOOT is.  I assume you've confirmed that at run-time you end
> > > > up with preboot="run preboot" being set, and not preboot="long command"
> > > > ?
> > > 
> > > At nokia n900 runtime is always "preboot=long command" and not
> > > "preboot=run preboot". It also was before these changes.
> > 
> > OK, so then we really just need this patch, and not the other.
> 
> Note that this is observation and all automated tests depend on this
> behavior. So if this behavior somehow change (e.g. by changing
> implementation or hacking LTO to change order, or etc...) then tests

Yes, and n900 is not the only platform and preboot is not the only
environment variable that has this issue.

> start failing and you would see it in failed CI. I hope that n900 test
> is still running in CI and in case of issues somebody inform me about
> it...

I certainly hope it's still doing useful things in CI, but if it doesn't
fail when it should fail, I won't know.  On the other hand, if it does
fail then that means whatever broke has to be fixed.

> PATCH 1/2 avoids inclusion of "preboot=CONFIG_PREBOOT" into default
> environment when CONFIG_PREBOOT is not defined in defconfig file. And

Right, but it doesn't functionally change anything as preboot is set
right on this and other platforms all the same.  And we would want a
similar patch for other variables that have the same issue too.  PREBOOT
isn't special in this regard.

> PATCH 2/2 then unsets CONFIG_PREBOOT from nokia defconfig file.
> 
> > And long
> > term figure out how to better handle this along with the other
> > half-dozen places the user may want to configure the default environment
> > a bit more, on top of what's already provided.  There are a handful of
> > other environment variables that are handled the same (confusing) way
> > right now.
> > 
> > -- 
> > Tom
> 
> Apparently this problem appeared by converting config.h options into
> Kconfig. And continue to grow by conversion of new and new options.

I prefer to look at it as a problem shown by Kconfig that a number of
things are both "set CONFIG_FOO to enable it as a boolean" and "set
CONFIG_FOO to some non-empty value that will be used" rather than as two
options, one as a bool and one with the value.

> Really Kconfig is not ideal tool for generating environment variables.

It really isn't, that's why the plain text environment stuff is a good
step in the right direction.  What I'm not sure about how to best handle
is making it easy for a user, rather than developer, to override
environment values too.  What I mean he

> For all this stuff is needed some stronger tool/language than Kconfig,
> e.g. C preprocessor (like it was before in config.h) or any similar
> stronger macro language (e.g. m4) or script languages (shell / python).

Yes, so, take a look at 5c3f6a320678d64c9fa0c42755488822a033b567 as an
example of moving a board away from board config.h and to something
else.  What I'm not sure on is how to best handle preboot in this case
as I'm not quite liking "Enable CONFIG_PREBOOT and then edit
.../boardname.env to set preboot to the right value".

-- 
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/20220712/148ecbdb/attachment.sig>