[PATCH] armv8: fsl : fix bootcmd and mcinitcmd default value

[Bedel, Alban]

On Wed, 2021-06-30 at 12:44 +0000, Wasim Khan (OSS) wrote:
> 
> 
> > -----Original Message-----
> > From: Bedel, Alban <alban.bedel at aerq.com>
> > Sent: Wednesday, June 30, 2021 6:03 PM
> > To: Priyanka Jain <priyanka.jain at nxp.com>; Varun Sethi < 
> > V.Sethi at nxp.com>;
> > Wasim Khan <wasim.khan at nxp.com>; Wasim Khan (OSS)
> > <wasim.khan at oss.nxp.com>
> > Cc: u-boot at lists.denx.de
> > Subject: Re: [PATCH] armv8: fsl : fix bootcmd and mcinitcmd default
> > value
> > 
> > On Wed, 2021-06-30 at 11:12 +0000, Priyanka Jain wrote:
> > > 
> > > snip
> 
> > 
> > * After issuing `env default -a ; saveenv' the board didn't boot
> > anymore because `bootcmd` was then empty.
> 
> This is not the case with latest u-boot code. 'env default -a' set
> bootcmd to default one (run distro_bootcmd).
> So, we are safe here.
> 
> 
> > 
> > * If redundant env on eMMC was enabled `bootcmd` was then overwritten
> > at every boot, preventing me from using a custom `bootcmd` at all.
> > 
> 
> Priyanka, Alban 
> Can you help me to understand what does enabling 'redundant env' on
> eMMC mean and how to enable it ?

See env/Kconfig:

config SYS_REDUNDAND_ENVIRONMENT
        bool "Enable redundant environment support"
        depends on ENV_IS_IN_EEPROM || ENV_IS_IN_FLASH || ENV_IS_IN_MMC || \
                ENV_IS_IN_NAND || ENV_IS_IN_SPI_FLASH || ENV_IS_IN_UBI
        help
          Normally, the environemt is stored in a single location.  By
          selecting this option, you can then define where to hold a redundant
          copy of the environment data, so that there is a valid backup copy in
          case there is a power failure during a "saveenv" operation.

When this option is enabled the internals of the env change
significantly and the old code then always detected the env as being
the default, erasing any previously user set value at every boot.

But beside the fact that it didn't work properly with all
configurations, the old code didn't really detect if the env was the
default. When it worked, it detected the case where no valid env was
stored and u-boot was using its internal in-memory defaults. That's why
resetting the env to default would then break the boot.

In my patch I replaced this logic by looking if `bootcmd` has the
default value, which worked well as long as the default value was
"unset". But as we see this is not a viable solution in the long run.
My suggestion would be to have something like this:

   if (env_get_yesno("fsl_bootcmd_set") <= 0) {
      // Set the default bootcmd according to the boot device
      ...
      env_set("fsl_bootcmd_set", "y");
   }

That way it doesn't matter what the default value of `bootcmd` is and
boards also have the possibility to disable this code by setting
`fsl_bootcmd_set` to `y` in their default env.

I think it would also make sense to have some code that set the TF-A
boot device in the env, that might allow handling this in the boot
scripts directly instead of all this hard coded logic.

Alban