[U-Boot,2/3] configs: Rename environment variable fit_loadaddr to addr_fit
diff mbox series

Message ID 20190812195955.3512-2-afd@ti.com
State Under Review
Delegated to: Tom Rini
Headers show
Series
  • [U-Boot,1/3] configs: Remove unneeded overlay_files environment variable
Related show

Commit Message

Andrew F. Davis Aug. 12, 2019, 7:59 p.m. UTC
This is the first part of a larger effort I would like to propose to
unify and simplify the default set of environment variables.

When many early environment variables were named there were fewer images
being loaded, usually just a kernel. At this time names like 'loadaddr'
would suffice. Now we have more images and many more commands that act on
them, often re-using the same variable for several different uses. The
contents of a variable are also not immediately known causing one to have
to look up a chain of variables to understand what a command is actually
doing. I suggest the following.

To start, all variables containing names should be prefixed with name_
and addresses with addr_. This is like how K2 already does things and
allows for simple universal commands like:

get_fdt_nfs=nfs ${addr_fdt} /boot/${name_fdt}

Which is very clear on what is intended here and would work across all
board that using the this naming convention.

We can do this one variable at a time, start here with addr_fit.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 include/configs/k2g_evm.h            |  2 +-
 include/configs/ti_armv7_common.h    |  4 ++--
 include/configs/ti_armv7_keystone2.h | 12 ++++++------
 3 files changed, 9 insertions(+), 9 deletions(-)

Comments

Andreas Dannenberg Aug. 16, 2019, 7:11 p.m. UTC | #1
Andrew,

On Mon, Aug 12, 2019 at 03:59:54PM -0400, Andrew F. Davis wrote:
> This is the first part of a larger effort I would like to propose to
> unify and simplify the default set of environment variables.
> 
> When many early environment variables were named there were fewer images
> being loaded, usually just a kernel. At this time names like 'loadaddr'
> would suffice. Now we have more images and many more commands that act on
> them, often re-using the same variable for several different uses. The
> contents of a variable are also not immediately known causing one to have
> to look up a chain of variables to understand what a command is actually
> doing. I suggest the following.
> 
> To start, all variables containing names should be prefixed with name_
> and addresses with addr_. This is like how K2 already does things and
> allows for simple universal commands like:
> 
> get_fdt_nfs=nfs ${addr_fdt} /boot/${name_fdt}
> 
> Which is very clear on what is intended here and would work across all
> board that using the this naming convention.
> 
> We can do this one variable at a time, start here with addr_fit.

I think this is a good initiative. Just looking at the TI stuff there
seems to be some good opportunity for cleanup and consolidation. One
concern is we need to ensure to not break stuff, which this series from
what I looked at and my limited testing should not (unless somebody does
some custom ENV-side loading tapping into parts of the environment as
part of some of their own ENV config they may have created locally but
there is no way really for us to check that).

Two quick suggestions:

1) The PATCH $SUBJECT lines should more narrowly define the scope in
which changes are made (e.g., "config: ti: *") to help make things
clearer when browsing the commit log,

2) A cover letter would be good to more organically entertain general
discussions around the approach.

For the series...

Acked-by: Andreas Dannenberg <dannenberg@ti.com>

> 
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> ---
>  include/configs/k2g_evm.h            |  2 +-
>  include/configs/ti_armv7_common.h    |  4 ++--
>  include/configs/ti_armv7_keystone2.h | 12 ++++++------
>  3 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/include/configs/k2g_evm.h b/include/configs/k2g_evm.h
> index 3ec5a5acf5..b39e956def 100644
> --- a/include/configs/k2g_evm.h
> +++ b/include/configs/k2g_evm.h
> @@ -69,7 +69,7 @@
>  	"run run_mon_hs; "						\
>  	"run init_${boot}; "						\
>  	"run get_fit_${boot}; "						\
> -	"bootm ${fit_loadaddr}#${name_fdt}"
> +	"bootm ${addr_fit}#${name_fdt}"
>  #endif
>  
>  /* NAND Configuration */
> diff --git a/include/configs/ti_armv7_common.h b/include/configs/ti_armv7_common.h
> index 828fb1b2a5..ece329fc7d 100644
> --- a/include/configs/ti_armv7_common.h
> +++ b/include/configs/ti_armv7_common.h
> @@ -52,9 +52,9 @@
>  
>  #define DEFAULT_FIT_TI_ARGS \
>  	"boot_fit=0\0" \
> -	"fit_loadaddr=0x90000000\0" \
> +	"addr_fit=0x90000000\0" \
>  	"fit_bootfile=fitImage\0" \
> -	"update_to_fit=setenv loadaddr ${fit_loadaddr}; setenv bootfile ${fit_bootfile}\0" \
> +	"update_to_fit=setenv loadaddr ${addr_fit}; setenv bootfile ${fit_bootfile}\0" \
>  	"loadfit=run args_mmc; bootm ${loadaddr}#${fdtfile};\0" \
>  
>  /*
> diff --git a/include/configs/ti_armv7_keystone2.h b/include/configs/ti_armv7_keystone2.h
> index b44b51bbd1..401dec4493 100644
> --- a/include/configs/ti_armv7_keystone2.h
> +++ b/include/configs/ti_armv7_keystone2.h
> @@ -240,11 +240,11 @@
>  	"get_mon_net=dhcp ${addr_mon} ${tftp_root}/${name_mon}\0"	\
>  	"get_mon_nfs=nfs ${addr_mon} ${nfs_root}/boot/${name_mon}\0"	\
>  	"get_mon_ubi=ubifsload ${addr_mon} ${bootdir}/${name_mon}\0"	\
> -	"get_fit_net=dhcp ${fit_loadaddr} ${tftp_root}"			\
> +	"get_fit_net=dhcp ${addr_fit} ${tftp_root}"			\
>  						"/${fit_bootfile}\0"	\
> -	"get_fit_nfs=nfs ${fit_loadaddr} ${nfs_root}/boot/${fit_bootfile}\0"\
> -	"get_fit_ubi=ubifsload ${fit_loadaddr} ${bootdir}/${fit_bootfile}\0"\
> -	"get_fit_mmc=load mmc ${bootpart} ${fit_loadaddr} "		\
> +	"get_fit_nfs=nfs ${addr_fit} ${nfs_root}/boot/${fit_bootfile}\0"\
> +	"get_fit_ubi=ubifsload ${addr_fit} ${bootdir}/${fit_bootfile}\0"\
> +	"get_fit_mmc=load mmc ${bootpart} ${addr_fit} "			\
>  					"${bootdir}/${fit_bootfile}\0"	\
>  	"get_uboot_net=dhcp ${loadaddr} ${tftp_root}/${name_uboot}\0"	\
>  	"get_uboot_nfs=nfs ${loadaddr} ${nfs_root}/boot/${name_uboot}\0" \
> @@ -261,7 +261,7 @@
>  	"get_fdt_ramfs=dhcp ${fdtaddr} ${tftp_root}/${name_fdt}\0"	\
>  	"get_kern_ramfs=dhcp ${loadaddr} ${tftp_root}/${name_kern}\0"	\
>  	"get_mon_ramfs=dhcp ${addr_mon} ${tftp_root}/${name_mon}\0"	\
> -	"get_fit_ramfs=dhcp ${fit_loadaddr} ${tftp_root}"		\
> +	"get_fit_ramfs=dhcp ${addr_fit} ${tftp_root}"			\
>  						"/${fit_bootfile}\0"	\
>  	"get_fs_ramfs=dhcp ${rdaddr} ${tftp_root}/${name_fs}\0"	\
>  	"get_ubi_net=dhcp ${addr_ubi} ${tftp_root}/${name_ubi}\0"	\
> @@ -290,7 +290,7 @@
>  	"run run_mon_hs; "						\
>  	"run init_${boot}; "						\
>  	"run get_fit_${boot}; "						\
> -	"bootm ${fit_loadaddr}#${name_fdt}"
> +	"bootm ${addr_fit}#${name_fdt}"
>  #endif
>  #endif
>  
> -- 
> 2.17.1
>
Tom Rini Aug. 21, 2019, 12:42 p.m. UTC | #2
On Mon, Aug 12, 2019 at 03:59:54PM -0400, Andrew F. Davis wrote:

> This is the first part of a larger effort I would like to propose to
> unify and simplify the default set of environment variables.
> 
> When many early environment variables were named there were fewer images
> being loaded, usually just a kernel. At this time names like 'loadaddr'
> would suffice. Now we have more images and many more commands that act on
> them, often re-using the same variable for several different uses. The
> contents of a variable are also not immediately known causing one to have
> to look up a chain of variables to understand what a command is actually
> doing. I suggest the following.
> 
> To start, all variables containing names should be prefixed with name_
> and addresses with addr_. This is like how K2 already does things and
> allows for simple universal commands like:
> 
> get_fdt_nfs=nfs ${addr_fdt} /boot/${name_fdt}
> 
> Which is very clear on what is intended here and would work across all
> board that using the this naming convention.
> 
> We can do this one variable at a time, start here with addr_fit.
> 
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> Acked-by: Andreas Dannenberg <dannenberg@ti.com>

Applied to u-boot/master, thanks!

Patch
diff mbox series

diff --git a/include/configs/k2g_evm.h b/include/configs/k2g_evm.h
index 3ec5a5acf5..b39e956def 100644
--- a/include/configs/k2g_evm.h
+++ b/include/configs/k2g_evm.h
@@ -69,7 +69,7 @@ 
 	"run run_mon_hs; "						\
 	"run init_${boot}; "						\
 	"run get_fit_${boot}; "						\
-	"bootm ${fit_loadaddr}#${name_fdt}"
+	"bootm ${addr_fit}#${name_fdt}"
 #endif
 
 /* NAND Configuration */
diff --git a/include/configs/ti_armv7_common.h b/include/configs/ti_armv7_common.h
index 828fb1b2a5..ece329fc7d 100644
--- a/include/configs/ti_armv7_common.h
+++ b/include/configs/ti_armv7_common.h
@@ -52,9 +52,9 @@ 
 
 #define DEFAULT_FIT_TI_ARGS \
 	"boot_fit=0\0" \
-	"fit_loadaddr=0x90000000\0" \
+	"addr_fit=0x90000000\0" \
 	"fit_bootfile=fitImage\0" \
-	"update_to_fit=setenv loadaddr ${fit_loadaddr}; setenv bootfile ${fit_bootfile}\0" \
+	"update_to_fit=setenv loadaddr ${addr_fit}; setenv bootfile ${fit_bootfile}\0" \
 	"loadfit=run args_mmc; bootm ${loadaddr}#${fdtfile};\0" \
 
 /*
diff --git a/include/configs/ti_armv7_keystone2.h b/include/configs/ti_armv7_keystone2.h
index b44b51bbd1..401dec4493 100644
--- a/include/configs/ti_armv7_keystone2.h
+++ b/include/configs/ti_armv7_keystone2.h
@@ -240,11 +240,11 @@ 
 	"get_mon_net=dhcp ${addr_mon} ${tftp_root}/${name_mon}\0"	\
 	"get_mon_nfs=nfs ${addr_mon} ${nfs_root}/boot/${name_mon}\0"	\
 	"get_mon_ubi=ubifsload ${addr_mon} ${bootdir}/${name_mon}\0"	\
-	"get_fit_net=dhcp ${fit_loadaddr} ${tftp_root}"			\
+	"get_fit_net=dhcp ${addr_fit} ${tftp_root}"			\
 						"/${fit_bootfile}\0"	\
-	"get_fit_nfs=nfs ${fit_loadaddr} ${nfs_root}/boot/${fit_bootfile}\0"\
-	"get_fit_ubi=ubifsload ${fit_loadaddr} ${bootdir}/${fit_bootfile}\0"\
-	"get_fit_mmc=load mmc ${bootpart} ${fit_loadaddr} "		\
+	"get_fit_nfs=nfs ${addr_fit} ${nfs_root}/boot/${fit_bootfile}\0"\
+	"get_fit_ubi=ubifsload ${addr_fit} ${bootdir}/${fit_bootfile}\0"\
+	"get_fit_mmc=load mmc ${bootpart} ${addr_fit} "			\
 					"${bootdir}/${fit_bootfile}\0"	\
 	"get_uboot_net=dhcp ${loadaddr} ${tftp_root}/${name_uboot}\0"	\
 	"get_uboot_nfs=nfs ${loadaddr} ${nfs_root}/boot/${name_uboot}\0" \
@@ -261,7 +261,7 @@ 
 	"get_fdt_ramfs=dhcp ${fdtaddr} ${tftp_root}/${name_fdt}\0"	\
 	"get_kern_ramfs=dhcp ${loadaddr} ${tftp_root}/${name_kern}\0"	\
 	"get_mon_ramfs=dhcp ${addr_mon} ${tftp_root}/${name_mon}\0"	\
-	"get_fit_ramfs=dhcp ${fit_loadaddr} ${tftp_root}"		\
+	"get_fit_ramfs=dhcp ${addr_fit} ${tftp_root}"			\
 						"/${fit_bootfile}\0"	\
 	"get_fs_ramfs=dhcp ${rdaddr} ${tftp_root}/${name_fs}\0"	\
 	"get_ubi_net=dhcp ${addr_ubi} ${tftp_root}/${name_ubi}\0"	\
@@ -290,7 +290,7 @@ 
 	"run run_mon_hs; "						\
 	"run init_${boot}; "						\
 	"run get_fit_${boot}; "						\
-	"bootm ${fit_loadaddr}#${name_fdt}"
+	"bootm ${addr_fit}#${name_fdt}"
 #endif
 #endif