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

[Tom Rini]

On Tue, Nov 01, 2022 at 11:58:48PM +0100, Pali Rohár wrote:
> 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?

Yes, what I thought I had said before either above, or in the other
thread. The definition of the preboot variable belongs in the text based
environment file(s) and not in Kconfig. We no longer have boards
defining the value in BOTH Kconfig and their board.h files, which was
the problem.

-- 
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/20221101/0327db4e/attachment.sig>