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

[Wasim Khan]

Hi Alban,

> -----Original Message-----
> From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Bedel, Alban
> Sent: Wednesday, June 30, 2021 7:08 PM
> To: Priyanka Jain <priyanka.jain at nxp.com>; Varun Sethi <V.Sethi 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 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


Thank you for explaining it. I could reproduce the issue in case we enable SYS_REDUNDAND_ENVIRONMENT.
Fixed it using another env variable as you suggested. Below are my test steps on lx2160ardb with xspi and SD boot.


######## XSPI BOOT #######

=> qixis_reset altbank
<bootup_logs_snip>

Loading Environment from SPIFlash... SF: Detected mt35xu512aba with page size 256 Bytes, erase size 128 KiB, total 64 MiB
*** Warning - bad CRC, using default environment	

<bootup_logs_snip>

=> pri bootcmd
bootcmd=sf probe 0:0; sf read 0x806c0000 0x6c0000 0x40000; env exists mcinitcmd && env exists secureboot && esbc_validate 0x806c0000; sf read 0x80d00000 0xd00000 0x100000; env exists mcinitcmd && fsl_mc lazyapply dpl 0x80d00000; run distro_bootcmd;run xspi_bootcmd; env exists secureboot && esbc_halt;

=> pri mcinitcmd
mcinitcmd=sf probe 0:0 && sf read 0x80640000 0x640000 0x80000 && env exists secureboot && esbc_validate 0x80640000 && esbc_validate 0x80680000; sf read 0x80a00000 0xa00000 0x300000 && sf read 0x80e00000 0xe00000 0x100000; fsl_mc start mc 0x80a00000 0x80e00000

=> pri fsl_bootcmd_mcinitcmd_set
fsl_bootcmd_mcinitcmd_set=y

=> setenv bootcmd run xspi_bootcmd
=> saveenv
Saving Environment to SPIFlash... Erasing SPI flash...Writing to SPI flash...done
Valid environment: 2
OK
=> pri bootcmd
bootcmd=run xspi_bootcmd
=> qixis_reset altbank
<bootup_logs_snip>
=> pri bootcmd
bootcmd=run xspi_bootcmd
=> env default -a;saveenv
## Resetting to default environment
Saving Environment to SPIFlash... Erasing SPI flash...Writing to SPI flash...done
Valid environment: 1
OK

=> qixis_reset altbank
<bootup_logs_snip>
=> pri bootcmd
bootcmd=sf probe 0:0; sf read 0x806c0000 0x6c0000 0x40000; env exists mcinitcmd && env exists secureboot && esbc_validate 0x806c0000; sf read 0x80d00000 0xd00000 0x100000; env exists mcinitcmd && fsl_mc lazyapply dpl 0x80d00000; run distro_bootcmd;run xspi_bootcmd; env exists secureboot && esbc_halt;
=> pri fsl_bootcmd_mcinitcmd_set
fsl_bootcmd_mcinitcmd_set=y







####### SD BOOT ########

Loading Environment from MMC... *** Warning - bad CRC, using default environment
<bootup_logs_snip>
=> pri bootcmd
bootcmd=env exists mcinitcmd && mmcinfo; mmc read 0x80d00000 0x6800 0x800; env exists mcinitcmd && env exists secureboot  && mmc read 0x806C0000 0x3600 0x20 && esbc_validate 0x806C0000;env exists mcinitcmd && fsl_mc lazyapply dpl 0x80d00000;run distro_bootcmd;run sd_bootcmd;env exists secureboot && esbc_halt;

=> pri fsl_bootcmd_mcinitcmd_set
fsl_bootcmd_mcinitcmd_set=y

=> setenv bootcmd run sd_bootcmd
=> saveenv
Saving Environment to MMC... Writing to redundant MMC(0)... OK

=> pri bootcmd
bootcmd=run sd_bootcmd
=> qixis_reset sd
<bootup_logs_snip>
=> pri bootcmd
bootcmd=run sd_bootcmd
=> qixis_reset sd
<bootup_logs_snip>
=> env default -a;saveenv
## Resetting to default environment
Saving Environment to MMC... Writing to MMC(0)... OK
=> pri bootcmd
bootcmd=env exists mcinitcmd && mmcinfo; mmc read 0x80d00000 0x6800 0x800; env exists mcinitcmd && env exists secureboot  && mmc read 0x806C0000 0x3600 0x20 && esbc_validate 0x806C0000;env exists mcinitcmd && fsl_mc lazyapply dpl 0x80d00000;run distro_bootcmd;run sd_bootcmd;env exists secureboot && esbc_halt;

=> pri fsl_bootcmd_mcinitcmd_set
fsl_bootcmd_mcinitcmd_set=y