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

[Pali Rohár]

On Sunday 09 October 2022 15:03:03 Pali Rohár wrote:
> 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.

Any comments here?