diff mbox series

[04/16] arm: stm32mp: detect U-Boot version used to save environment

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

Commit Message

Patrick Delaunay March 31, 2020, 4:04 p.m. UTC
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(-)

Comments

Patrice CHOTARD April 1, 2020, 7:33 a.m. UTC | #1
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 \
Wolfgang Denk April 1, 2020, 11:26 a.m. UTC | #2
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
Patrick Delaunay April 7, 2020, 2:37 p.m. UTC | #3
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
Patrick Delaunay April 7, 2020, 2:54 p.m. UTC | #4
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
Wolfgang Denk April 7, 2020, 4:28 p.m. UTC | #5
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
Patrick Delaunay April 8, 2020, 9:38 a.m. UTC | #6
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 mbox series

Patch

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 \