| Message ID | 20200331160456.26254-1-patrick.delaunay@st.com |
|---|---|
| State | Rejected |
| Delegated to: | Patrick Delaunay |
| Headers | show |
| Series | [01/16] arm: stm32mp: update dependency for STM32_ETZPC | expand |
Hi Patrick On 3/31/20 6:04 PM, Patrick Delaunay wrote: > Imply CONFIG_VERSION_VARIABLE for stm32mp1 target > and test U-Boot version ($env_ver) when the environment was > saved for the last time and to display warning trace. > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> > --- > > arch/arm/mach-stm32mp/Kconfig | 1 + > include/configs/stm32mp1.h | 11 ++++++++--- > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/mach-stm32mp/Kconfig b/arch/arm/mach-stm32mp/Kconfig > index 032facff31..a86288cb76 100644 > --- a/arch/arm/mach-stm32mp/Kconfig > +++ b/arch/arm/mach-stm32mp/Kconfig > @@ -67,6 +67,7 @@ config TARGET_ST_STM32MP15x > imply DISABLE_CONSOLE > imply PRE_CONSOLE_BUFFER > imply SILENT_CONSOLE > + imply VERSION_VARIABLE > help > target the STMicroelectronics board with SOC STM32MP15x > managed by board/st/stm32mp1: > diff --git a/include/configs/stm32mp1.h b/include/configs/stm32mp1.h > index 42717c167e..ae060fbc4b 100644 > --- a/include/configs/stm32mp1.h > +++ b/include/configs/stm32mp1.h > @@ -222,9 +222,14 @@ > "splashimage=0xc4300000\0" \ > "ramdisk_addr_r=0xc4400000\0" \ > "altbootcmd=run bootcmd\0" \ > - "env_default=1\0" \ > - "env_check=if test $env_default -eq 1;"\ > - " then env set env_default 0;env save;fi\0" \ > + "env_check=" \ > + "env exists env_ver || env set env_ver ${ver};" \ > + "if env info -p -d -q; then env save; fi;" \ Is option "-q" exist ? i can't find anything about it into source code > + "if test \"$env_ver\" != \"$ver\"; then" \ > + " echo \"*** Warning: old environment ${env_ver}\";" \ > + " echo '* set default: env default -a; env save; reset';" \ > + " echo '* update current: env set env_ver ${ver}; env save';" \ > + "fi;\0" \ > STM32MP_BOOTCMD \ > STM32MP_MTDPARTS \ > STM32MP_DFU_ALT_RAM \
Dear Patrick Delaunay, In message <20200331160456.26254-1-patrick.delaunay@st.com> you wrote: > Imply CONFIG_VERSION_VARIABLE for stm32mp1 target > and test U-Boot version ($env_ver) when the environment was > saved for the last time and to display warning trace. What is env_ver? Are you by chance reinventing the wheel? The U-Boot version is stored in the environment variable "ver"; there should be no need for something similar. Also. where is $env_ver coming from? It does not exist in mainline, nor in any of the 3 patches that preceed this patch # 4/16 ? Best regards, Wolfgang Denk
Hi Patrice > From: Patrice CHOTARD <patrice.chotard@st.com> > Sent: mercredi 1 avril 2020 09:34 > > Hi Patrick > > On 3/31/20 6:04 PM, Patrick Delaunay wrote: > > Imply CONFIG_VERSION_VARIABLE for stm32mp1 target and test U-Boot > > version ($env_ver) when the environment was saved for the last time > > and to display warning trace. > > > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> > > --- [....] > > diff --git a/include/configs/stm32mp1.h b/include/configs/stm32mp1.h > > index 42717c167e..ae060fbc4b 100644 > > --- a/include/configs/stm32mp1.h > > +++ b/include/configs/stm32mp1.h > > @@ -222,9 +222,14 @@ > > "splashimage=0xc4300000\0" \ > > "ramdisk_addr_r=0xc4400000\0" \ > > "altbootcmd=run bootcmd\0" \ > > - "env_default=1\0" \ > > - "env_check=if test $env_default -eq 1;"\ > > - " then env set env_default 0;env save;fi\0" \ > > + "env_check=" \ > > + "env exists env_ver || env set env_ver ${ver};" \ > > + "if env info -p -d -q; then env save; fi;" \ > > Is option "-q" exist ? i can't find anything about it into source code > > Introduced in [v3,1/7] cmd: env: add option for quiet output on env info http://patchwork.ozlabs.org/patch/1236816/ I forget to indicate dependency. PAtrick
Dear Wolfgang, > From: Wolfgang Denk <wd@denx.de> > Sent: mercredi 1 avril 2020 13:26 > > Dear Patrick Delaunay, > > In message <20200331160456.26254-1-patrick.delaunay@st.com> you wrote: > > Imply CONFIG_VERSION_VARIABLE for stm32mp1 target and test U-Boot > > version ($env_ver) when the environment was saved for the last time > > and to display warning trace. > > What is env_ver? Are you by chance reinventing the wheel? The script env_check is ; env exists env_ver || env set env_ver ${ver}; if env info -p -d -q; then env save; fi; if test \"$env_ver\" != \"$ver\"; then echo \"*** Warning: old environment ${env_ver}\"; echo '* set default: env default -a; env save; reset'; echo '* update current: env set env_ver ${ver}; env save'; fi; In the first line of the script: "env exists env_ver || env set env_ver ${ver}", so $env_ver = $ver, before the first env_save during the first boot (second line of the script) > The U-Boot version is stored in the environment variable "ver"; there should be no > need for something similar. > > > Also. where is $env_ver coming from? It does not exist in mainline, nor in any of > the 3 patches that preceed this patch # 4/16 ? env_ver is only defined and used in this script to detect that current U-Boot version ($ver) and the version of U-Boot for last env save ($env_var) are not aligned. I introduce this warning after debug of many issue around this kind of error, but perhaps more a debug feature. So if you found that it is a bad idea for upstream, I will drop this part and just to the new quiet option To simplify the test: env_check = " if env info -p -d -q; then env save; fi;" > Best regards, > > Wolfgang Denk Regards Patrick
Dear Patrick, In message <8607d1778bcd4035807908e4a3a90381@SFHDAG6NODE3.st.com> you wrote: > > To simplify the test: > > env_check = " if env info -p -d -q; then env save; fi;" All such automatical "env save" actions somewhere in the code give me the creeps. I've seen too often that they did things I nver intended to do or would have accepted if I had a chance to decide. Use extremely careful, please. From a user point of view, it's me who owns the environment, and nobody should mess with my data without me confirming it. Best regards, Wolfgang Denk
Dear Wolfgang, > From: Uboot-stm32 <uboot-stm32-bounces@st-md-mailman.stormreply.com> On > Behalf Of Wolfgang Denk > > Dear Patrick, > > In message <8607d1778bcd4035807908e4a3a90381@SFHDAG6NODE3.st.com> > you wrote: > > > > To simplify the test: > > > > env_check = " if env info -p -d -q; then env save; fi;" > > All such automatical "env save" actions somewhere in the code give me the > creeps. I've seen too often that they did things I nver intended to do or would > have accepted if I had a chance to decide. > > Use extremely careful, please. Sure, In this case, the command "env info -d" tests if the default environment is currently used, So the user have never updated and saved the environment. In this case and if the persistent storage is available (option -p), the script "env_check" save the environment. PS: I take the initial idea from ./include/configs/opos6uldev.h and ./include/configs/apf27.h > From a user point of view, it's me who owns the environment, and nobody should > mess with my data without me confirming it. As the save action is performed only when default environment is used, it is done before any user modification so I don't think that it is annoying for user. I also kept the call this feature only in the ST specific bootcmd_stm32mp to allow customization for users or other boards. (I prefer to don't add it in board_late_init() as it is done in board/intel/edison/edison.c) The purpose of the "env save" is just to avoid a "Warning" during the boot, until the first user action and "env save" command: Loading Environment from MMC... *** Warning - bad CRC, using default environment The content of environment before and after the save is identical: it is the default one. but if it is too aggressive I can kept it for downstream. Marek do you have a opinion: it is acceptable for DH SOM using STM32MP15x SOC? > Best regards, > > Wolfgang Denk > Best Regards Patrick Delaunay
diff --git a/arch/arm/mach-stm32mp/Kconfig b/arch/arm/mach-stm32mp/Kconfig index 032facff31..a86288cb76 100644 --- a/arch/arm/mach-stm32mp/Kconfig +++ b/arch/arm/mach-stm32mp/Kconfig @@ -67,6 +67,7 @@ config TARGET_ST_STM32MP15x imply DISABLE_CONSOLE imply PRE_CONSOLE_BUFFER imply SILENT_CONSOLE + imply VERSION_VARIABLE help target the STMicroelectronics board with SOC STM32MP15x managed by board/st/stm32mp1: diff --git a/include/configs/stm32mp1.h b/include/configs/stm32mp1.h index 42717c167e..ae060fbc4b 100644 --- a/include/configs/stm32mp1.h +++ b/include/configs/stm32mp1.h @@ -222,9 +222,14 @@ "splashimage=0xc4300000\0" \ "ramdisk_addr_r=0xc4400000\0" \ "altbootcmd=run bootcmd\0" \ - "env_default=1\0" \ - "env_check=if test $env_default -eq 1;"\ - " then env set env_default 0;env save;fi\0" \ + "env_check=" \ + "env exists env_ver || env set env_ver ${ver};" \ + "if env info -p -d -q; then env save; fi;" \ + "if test \"$env_ver\" != \"$ver\"; then" \ + " echo \"*** Warning: old environment ${env_ver}\";" \ + " echo '* set default: env default -a; env save; reset';" \ + " echo '* update current: env set env_ver ${ver}; env save';" \ + "fi;\0" \ STM32MP_BOOTCMD \ STM32MP_MTDPARTS \ STM32MP_DFU_ALT_RAM \
Imply CONFIG_VERSION_VARIABLE for stm32mp1 target and test U-Boot version ($env_ver) when the environment was saved for the last time and to display warning trace. Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> --- arch/arm/mach-stm32mp/Kconfig | 1 + include/configs/stm32mp1.h | 11 ++++++++--- 2 files changed, 9 insertions(+), 3 deletions(-)