On Tuesday 01 November 2022 19:29:01 Tom Rini wrote: > 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 <p...@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.
Well, for N900 problem is still there with u-boot master branch. preboot variable is defined two times in default env array and U-Boot on serial console prints following error: DELETE CANDIDATE: "preboot" hdelete: DELETE key "preboot" DELETE ERROR ############################## After applying patch 1/2 this error disappear.