RE: [Uboot-stm32] [PATCH 04/16] arm: stm32mp: detect U-Boot version used to save environment
Dear Wolfgang, > From: Uboot-stm32 On > Behalf Of Wolfgang Denk > > Dear Patrick, > > In message <8607d1778bcd4035807908e4a3a90...@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
Re: [PATCH 04/16] arm: stm32mp: detect U-Boot version used to save environment
Dear Patrick, In message <8607d1778bcd4035807908e4a3a90...@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 -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de "Well I don't see why I have to make one man miserable when I can make so many men happy." - Ellyn Mustard, about marriage
RE: [PATCH 04/16] arm: stm32mp: detect U-Boot version used to save environment
Dear Wolfgang,
> From: Wolfgang Denk
> Sent: mercredi 1 avril 2020 13:26
>
> Dear Patrick Delaunay,
>
> In message <20200331160456.26254-1-patrick.delau...@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
RE: [PATCH 04/16] arm: stm32mp: detect U-Boot version used to save environment
Hi Patrice
> From: Patrice CHOTARD
> 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
> > ---
[]
> > 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=0xc430\0" \
> > "ramdisk_addr_r=0xc440\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
Re: [PATCH 04/16] arm: stm32mp: detect U-Boot version used to save environment
Dear Patrick Delaunay, In message <20200331160456.26254-1-patrick.delau...@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 -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Computers are not intelligent. They only think they are.
Re: [PATCH 04/16] arm: stm32mp: detect U-Boot version used to save environment
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
> ---
>
> 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=0xc430\0" \
> "ramdisk_addr_r=0xc440\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 \
[PATCH 04/16] arm: stm32mp: detect U-Boot version used to save environment
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
---
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=0xc430\0" \
"ramdisk_addr_r=0xc440\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 \
--
2.17.1
