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

[Pali Rohár]

On Wednesday 27 July 2022 15:08:05 Tom Rini wrote:
> On Wed, Jul 27, 2022 at 09:01:15PM +0200, Pali Rohár wrote:
> > On Wednesday 27 July 2022 14:58:20 Tom Rini wrote:
> > > On Wed, Jul 27, 2022 at 08:52:01PM +0200, Pali Rohár wrote:
> > > > On Wednesday 27 July 2022 14:48:23 Tom Rini wrote:
> > > > > On Wed, Jul 27, 2022 at 08:34:41PM +0200, Pali Rohár wrote:
> > > > > > On Monday 25 July 2022 17:21:00 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>
> > > > > > > 
> > > > > > > Applied to u-boot/master, thanks!
> > > > > > 
> > > > > > Patch 1/2 is missing.
> > > > > 
> > > > > Sorry if I was unclear enough in the rest of the emails, I'm not
> > > > > applying 1/2.  The empty string is fine enough (again, other variables
> > > > > have this issue).  Unless I missed something and there's a run-time
> > > > > failure that 1/2 resolves?
> > > > 
> > > > The issue is that preboot= variable is defined in ENV array two times.
> > > > Once in Kconfig and second time in regular environment define.
> > > > 
> > > > It is just coincidence that the definition from Kconfig is ignored
> > > > because currently it is put _after_ the regular preboot= definition. But
> > > > this is fragile and probably some unrelated change/patch could change
> > > > this behavior.
> > > > 
> > > > We really do not want two preboot= definitions in ENV array and pray
> > > > that the correct one would be used by U-Boot at runtime.
> > > 
> > > It's not coincidence, it's down to link order.  So it's not going to
> > > break anytime soon.  And it's not just preboot that's in this situation.
> > 
> > It is not linker order, but rather order in which are specified
> > preprocessor macros...
> 
> Ah right, sorry, I was thinking of something else I found when doing
> CONFIG_EXTRA_ENV_TEXT / CONFIG_EXTRA_ENV_SETTINGS stuff.
> 
> > And what else is in the same situation as preboot? I just do not see
> > other variable which is defined two times.
> 
> Right so it's all from include/env_default.h yes?  CONFIG_EXTRA_ENV_TEXT
> and then CONFIG_EXTRA_ENV_SETTINGS win.  And pretty much everything else
> in that file that has a CONFIG knob to it either has a different bit of
> Kconfig "fun" to avoid the situation PREBOOT has or is already in the
> same boat.
> 
> > > A more generic approach to deal with this would be appreciated.
> 
> Which is why I said this.  How we're doing this right now isn't great
> and isn't super user friendly.  I don't want to have to repeat the logic
> you do in 1/2 for all of the CONFIG options in env_default.h that aren't
> already converted to Kconfig.

But this is how it is expressed in Kconfig (for string options). For
example in kernel there was recently fixes for specifying -mcpu powerpc
gcc option via Kconfig CONFIG_TARGET_CPU string option and there is also
corresponding Kconfig CONFIG_TARGET_CPU_BOOL option which has same
meaning as my *_DEFINED option. If you do not want to do it then do not
use Kconfig for those options. Generic better approach is to move these
settings out of the Kconfig, but this is something which you do not want
to do it. It is pretty simple, if you are going to use tool which is not
ideal or designed for some purpose then you need to deal with it. I just
described one example which I saw recently used in kernel.

In my opinion, options which use, call or contains parts of u-boot
script should _not_ be in Kconfig; but rather in CONFIG_EXTRA_ENV_SETTINGS
because it refers directly to other "run" variables defined in that
CONFIG_EXTRA_ENV_SETTINGS.