| Message ID | 20210616121913.537255-1-wasim.khan@oss.nxp.com |
|---|---|
| State | Superseded |
| Delegated to: | Priyanka Jain |
| Headers | show |
| Series | armv8: fsl : fix bootcmd and mcinitcmd default value | expand |
On Wed, 2021-06-16 at 14:19 +0200, Wasim Khan wrote: > From: Wasim Khan <wasim.khan@nxp.com> > > NXP platforms expect bootcmd and mcinitcmd to be updated > as per boot source. > > commit cbf77d201870f2d12227e2d95718a416b16ec98b breaks this > behaviour. > Revert commit cbf77d201870f2d12227e2d95718a416b16ec98b As I already explained in the prior exchanges we had, I'm fully convinced that reverting this patch is not the solution to your problem. Please see the log of my patch for a full explanation, but basically the old code not only rely on the a broken assumption, it also fails to implement it correctly. The current code set `bootcmd` and `mcinicmd` only if they are not set which a simple and sane behaviour. When I submitted my patch these variables were not set in the default so I suspect that another patch now set these in the default env. Please look for the root cause of the problem instead of re-enabling known broken code. Alban
Hi Alban, > -----Original Message----- > From: Bedel, Alban <alban.bedel@aerq.com> > Sent: Wednesday, June 23, 2021 6:38 PM > To: Priyanka Jain <priyanka.jain@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; > Wasim Khan (OSS) <wasim.khan@oss.nxp.com> > Cc: u-boot@lists.denx.de; Wasim Khan <wasim.khan@nxp.com> > Subject: Re: [PATCH] armv8: fsl : fix bootcmd and mcinitcmd default value > > On Wed, 2021-06-16 at 14:19 +0200, Wasim Khan wrote: > > From: Wasim Khan <wasim.khan@nxp.com> > > > > NXP platforms expect bootcmd and mcinitcmd to be updated as per boot > > source. > > > > commit cbf77d201870f2d12227e2d95718a416b16ec98b breaks this behaviour. > > Revert commit cbf77d201870f2d12227e2d95718a416b16ec98b > > As I already explained in the prior exchanges we had, I'm fully convinced that > reverting this patch is not the solution to your problem. Please see the log of my > patch for a full explanation, but basically the old code not only rely on the a > broken assumption, it also fails to implement it correctly. > > The current code set `bootcmd` and `mcinicmd` only if they are not set which a > simple and sane behaviour. As I have explained earlier that the bootcmd is always set with default value as " run distro_bootcmd". So fsl_setenv_bootcmd() never gets executed which is causing the issue. > When I submitted my patch these variables were not > set in the default so I suspect that another patch now set these in the default > env. I hard reset my tree to your commit and I still see the issue. Please refer to below logs. I don’t see any other patch causing this issue. Would let @Priyanka Jain to share her comments. U-Boot 2021.01-rc3-00115-gcbf77d2018 (Jun 25 2021 - 10:51:56 +0200) SoC: LX2160ACE Rev2.0 (0x87360020) ... ... MMC: FSL_SDHC: 0, FSL_SDHC: 1 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 EEPROM: NXID v1 ... ... => pri bootcmd bootcmd=run distro_bootcmd Regards, Wasim > > Alban
>-----Original Message----- >From: Wasim Khan <wasim.khan@nxp.com> >Sent: Friday, June 25, 2021 2:40 PM >To: Bedel, Alban <alban.bedel@aerq.com>; Priyanka Jain ><priyanka.jain@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; Wasim Khan (OSS) ><wasim.khan@oss.nxp.com> >Cc: u-boot@lists.denx.de >Subject: RE: [PATCH] armv8: fsl : fix bootcmd and mcinitcmd default value > >Hi Alban, > >> -----Original Message----- >> From: Bedel, Alban <alban.bedel@aerq.com> >> Sent: Wednesday, June 23, 2021 6:38 PM >> To: Priyanka Jain <priyanka.jain@nxp.com>; Varun Sethi >> <V.Sethi@nxp.com>; Wasim Khan (OSS) <wasim.khan@oss.nxp.com> >> Cc: u-boot@lists.denx.de; Wasim Khan <wasim.khan@nxp.com> >> Subject: Re: [PATCH] armv8: fsl : fix bootcmd and mcinitcmd default >> value >> >> On Wed, 2021-06-16 at 14:19 +0200, Wasim Khan wrote: >> > From: Wasim Khan <wasim.khan@nxp.com> >> > >> > NXP platforms expect bootcmd and mcinitcmd to be updated as per boot >> > source. >> > >> > commit cbf77d201870f2d12227e2d95718a416b16ec98b breaks this >behaviour. >> > Revert commit cbf77d201870f2d12227e2d95718a416b16ec98b >> >> As I already explained in the prior exchanges we had, I'm fully >> convinced that reverting this patch is not the solution to your >> problem. Please see the log of my patch for a full explanation, but >> basically the old code not only rely on the a broken assumption, it also fails to >implement it correctly. >> >> The current code set `bootcmd` and `mcinicmd` only if they are not set >> which a simple and sane behaviour. > > >As I have explained earlier that the bootcmd is always set with default value as " >run distro_bootcmd". >So fsl_setenv_bootcmd() never gets executed which is causing the issue. > > >> When I submitted my patch these variables were not set in the default >> so I suspect that another patch now set these in the default env. > >I hard reset my tree to your commit and I still see the issue. >Please refer to below logs. I don’t see any other patch causing this issue. Would >let @Priyanka Jain to share her comments. > > >U-Boot 2021.01-rc3-00115-gcbf77d2018 (Jun 25 2021 - 10:51:56 +0200) >SoC: LX2160ACE Rev2.0 (0x87360020) >... >... >MMC: FSL_SDHC: 0, FSL_SDHC: 1 >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 > >EEPROM: NXID v1 >... >... >=> pri bootcmd >bootcmd=run distro_bootcmd > >Regards, >Wasim > >> >> Alban > Alban, Wasim, Lets try to fix both issues. One being faced by Alban and the one faced by Wasim. Alban , Can you please provide summary of the issues faced by you. Regards Priyanka
On Wed, 2021-06-30 at 11:12 +0000, Priyanka Jain wrote: > > > > -----Original Message----- > > From: Wasim Khan <wasim.khan@nxp.com> > > Sent: Friday, June 25, 2021 2:40 PM > > To: Bedel, Alban <alban.bedel@aerq.com>; Priyanka Jain > > <priyanka.jain@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; Wasim Khan > > (OSS) > > <wasim.khan@oss.nxp.com> > > Cc: u-boot@lists.denx.de > > Subject: RE: [PATCH] armv8: fsl : fix bootcmd and mcinitcmd default > > value > > > > Hi Alban, > > > > > -----Original Message----- > > > From: Bedel, Alban <alban.bedel@aerq.com> > > > Sent: Wednesday, June 23, 2021 6:38 PM > > > To: Priyanka Jain <priyanka.jain@nxp.com>; Varun Sethi > > > <V.Sethi@nxp.com>; Wasim Khan (OSS) <wasim.khan@oss.nxp.com> > > > Cc: u-boot@lists.denx.de; Wasim Khan <wasim.khan@nxp.com> > > > Subject: Re: [PATCH] armv8: fsl : fix bootcmd and mcinitcmd > > > default > > > value > > > > > > On Wed, 2021-06-16 at 14:19 +0200, Wasim Khan wrote: > > > > From: Wasim Khan <wasim.khan@nxp.com> > > > > > > > > NXP platforms expect bootcmd and mcinitcmd to be updated as per > > > > boot > > > > source. > > > > > > > > commit cbf77d201870f2d12227e2d95718a416b16ec98b breaks this > > behaviour. > > > > Revert commit cbf77d201870f2d12227e2d95718a416b16ec98b > > > > > > As I already explained in the prior exchanges we had, I'm fully > > > convinced that reverting this patch is not the solution to your > > > problem. Please see the log of my patch for a full explanation, > > > but > > > basically the old code not only rely on the a broken assumption, > > > it also fails to > > implement it correctly. > > > > > > The current code set `bootcmd` and `mcinicmd` only if they are > > > not set > > > which a simple and sane behaviour. > > > > > > As I have explained earlier that the bootcmd is always set with > > default value as " > > run distro_bootcmd". > > So fsl_setenv_bootcmd() never gets executed which is causing the > > issue. > > > > > > > When I submitted my patch these variables were not set in the > > > default > > > so I suspect that another patch now set these in the default env. > > > > I hard reset my tree to your commit and I still see the issue. > > Please refer to below logs. I don’t see any other patch causing > > this issue. Would > > let @Priyanka Jain to share her comments. > > > > > > U-Boot 2021.01-rc3-00115-gcbf77d2018 (Jun 25 2021 - 10:51:56 +0200) > > SoC: LX2160ACE Rev2.0 (0x87360020) > > ... > > ... > > MMC: FSL_SDHC: 0, FSL_SDHC: 1 > > 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 > > > > EEPROM: NXID v1 > > ... > > ... > > => pri bootcmd > > bootcmd=run distro_bootcmd > > > > Regards, > > Wasim > > > > > > > > Alban > > > > Alban, Wasim, > > Lets try to fix both issues. One being faced by Alban and the one > faced by Wasim. > Alban , > Can you please provide summary of the issues faced by you. The issue I faced are well describe in my original patch, but I'll sum them up again here: * After issuing `env default -a ; saveenv' the board didn't boot anymore because `bootcmd` was then empty. * If redundant env on eMMC was enabled `bootcmd` was then overwritten at every boot, preventing me from using a custom `bootcmd` at all. A typical u-boot build is configured at build time for a specific boot device, but with TF-A there is a single build for all boot devices and u-boot then have configure the default boot device on the first boot. This is where the code we are discussing here come into play. Back when I submitted my patch the default env didn't define `bootcmd` in my build, so I took that as the way to detect that `bootcmd` need to be initialised. We could instead just use another env variable to mark if `bootcmd` has been initialised or not. Alban
> -----Original Message----- > From: Bedel, Alban <alban.bedel@aerq.com> > Sent: Wednesday, June 30, 2021 6:03 PM > To: Priyanka Jain <priyanka.jain@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; > Wasim Khan <wasim.khan@nxp.com>; Wasim Khan (OSS) > <wasim.khan@oss.nxp.com> > Cc: u-boot@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 ? > snip
On Wed, 2021-06-30 at 12:44 +0000, Wasim Khan (OSS) wrote: > > > > -----Original Message----- > > From: Bedel, Alban <alban.bedel@aerq.com> > > Sent: Wednesday, June 30, 2021 6:03 PM > > To: Priyanka Jain <priyanka.jain@nxp.com>; Varun Sethi < > > V.Sethi@nxp.com>; > > Wasim Khan <wasim.khan@nxp.com>; Wasim Khan (OSS) > > <wasim.khan@oss.nxp.com> > > Cc: u-boot@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
Hi Alban, > -----Original Message----- > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Bedel, Alban > Sent: Wednesday, June 30, 2021 7:08 PM > To: Priyanka Jain <priyanka.jain@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; > Wasim Khan (OSS) <wasim.khan@oss.nxp.com> > Cc: u-boot@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@aerq.com> > > > Sent: Wednesday, June 30, 2021 6:03 PM > > > To: Priyanka Jain <priyanka.jain@nxp.com>; Varun Sethi < > > > V.Sethi@nxp.com>; Wasim Khan <wasim.khan@nxp.com>; Wasim Khan (OSS) > > > <wasim.khan@oss.nxp.com> > > > Cc: u-boot@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
diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c b/arch/arm/cpu/armv8/fsl-layerscape/soc.c index 7553b5bce2..ad209bde33 100644 --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c @@ -33,10 +33,13 @@ #include <fsl_validate.h> #endif #include <fsl_immap.h> +#ifdef CONFIG_TFABOOT +#include <env_internal.h> +#endif #include <dm.h> #include <dm/device_compat.h> #include <linux/err.h> -#ifdef CONFIG_GIC_V3_ITS +#if defined(CONFIG_TFABOOT) || defined(CONFIG_GIC_V3_ITS) DECLARE_GLOBAL_DATA_PTR; #endif @@ -953,12 +956,28 @@ int board_late_init(void) #endif #ifdef CONFIG_TFABOOT /* - * Set bootcmd and mcinitcmd if they don't exist in the environment. + * check if gd->env_addr is default_environment; then setenv bootcmd + * and mcinitcmd. + */ +#ifdef CONFIG_SYS_RELOC_GD_ENV_ADDR + if (gd->env_addr == (ulong)&default_environment[0]) { +#else + if (gd->env_addr + gd->reloc_off == (ulong)&default_environment[0]) { +#endif + fsl_setenv_bootcmd(); + fsl_setenv_mcinitcmd(); + } + + /* + * If the boot mode is secure, default environment is not present then + * setenv command needs to be run by default */ - if (!env_get("bootcmd")) +#ifdef CONFIG_CHAIN_OF_TRUST + if ((fsl_check_boot_mode_secure() == 1)) { fsl_setenv_bootcmd(); - if (!env_get("mcinitcmd")) fsl_setenv_mcinitcmd(); + } +#endif #endif #ifdef CONFIG_QSPI_AHB_INIT qspi_ahb_init();