On Tue, Jun 02, 2020 at 06:06:17PM +0200, Marek Vasut wrote:
On 6/2/20 6:00 PM, Tom Rini wrote:
On Tue, Jun 02, 2020 at 05:55:25PM +0200, Marek Vasut wrote:
On 6/2/20 4:38 PM, Tom Rini wrote:
On Tue, Jun 02, 2020 at 02:47:12PM +0200, Marek Vasut wrote:
On 6/2/20 2:44 PM, Tom Rini wrote:
On Tue, Jun 02, 2020 at 02:05:39PM +0200, Rasmus Villemoes wrote: > On 02/06/2020 13.04, Marek Vasut wrote: >> On 6/2/20 8:42 AM, Rasmus Villemoes wrote: >>> On 29/05/2020 19.54, Marek Vasut wrote: >>>> +config ENV_APPEND >>>> + bool "Always append the environment with new data" >>>> + default n >>>> + help >>>> + If defined, the environment hash table is only ever appended with new >>>> + data, but the existing hash table can never be dropped and reloaded >>>> + with newly imported data. This may be used in combination with static >>>> + flags to e.g. to protect variables which must not be modified. >>>> + >>>> config ENV_ACCESS_IGNORE_FORCE >>>> bool "Block forced environment operations" >>>> default n >>>> diff --git a/env/env.c b/env/env.c >>>> index 024d36fdbe..967a9d36d7 100644 >>>> --- a/env/env.c >>>> +++ b/env/env.c >>>> @@ -204,7 +204,9 @@ int env_load(void) >>>> ret = drv->load(); >>>> if (!ret) { >>>> printf("OK\n"); >>>> +#if !CONFIG_IS_ENABLED(ENV_APPEND) >>>> return 0; >>>> +#endif >>> >>> Don't use CONFIG_IS_ENABLED() unless you actually introduce both >>> CONFIG_FOO and CONFIG_SPL_FOO. Otherwise the above >>> CONFIG_IS_ENABLED(ENV_APPEND) is guaranteed to evaluate to false in SPL. >>> Of course that only matters if environment support is enabled in SPL, >>> but some actually use that. >> >> We actually want to use CONFIG_IS_ENABLED() as much as possible to make >> these options future-proof, so that others won't have to chase down all >> kinds of #ifdef CONFIG stuff and fix it later on for SPL/TPL/etc. >> > > That makes no sense. You're introducing something whose help text > doesn't spell out that the option only applies to U-Boot proper, and is > completely ignored in SPL (since CONFIG_SPL_ENV_APPEND never exists). > The reason it's ignored in SPL is that you use the SPL-or-not-SPL-aware > CONFIG_IS_ENABLED() helper, and you say that's so that somebody in the > future can implement CONFIG_SPL_ENV_APPEND? > > If you intend for ENV_APPEND to be something that's either set or not > set for a given board, then the code needs to use the SPL-agnostic > IS_ENABLED(CONFIG_ENV_APPEND). If you intend it to be something that can > be set independently for the env support in SPL vs U-Boot proper, you > need to add both config options and, as you do, use CONFIG_IS_ENABLED.
How will this code behave if there is a mismatch between SPL and full U-Boot (disabled SPL, enabled full, as the patch stands today) ?
One will append the environment, the other will override it (if you have multiple envs enabled).
So it sounds like it wouldn't be valid to have this option differ between SPL and main U-Boot?
Consider the case where you have default env in SPL, and multiple envs in U-Boot proper.
Yes, today you can end up with cases where you build something that doesn't work as intended (likely something around falcon boot and/or boot count limit in env). Which is what I'm getting at here. Is there some cases where it would make any sense to enable this option in full U-Boot but disable it in SPL?
Yes, like my current use case, where I want to configure the SPL differently than U-Boot itself. SPL doesn't even have environment support enabled, but it might be needed later.
Sorry I wasn't clear enough. Does it make sense (when? how?) to have environment in SPL but mismatch this feature?
And also, I don't want to end up in the same problem we currently have e.g. with USB gadget, where I have to manually #ifdef CONFIG_SPL_BUILD #undef CONFIG_ options in the board config file.
Yes, don't do that, I've had to fix a few of those of late in catching converted but still in config header options.