| Message ID | 20190812195955.3512-2-afd@ti.com |
|---|---|
| State | Accepted |
| Commit | d2986a9bd8c06df52483e244f5c381e3cb899876 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,1/3] configs: Remove unneeded overlay_files environment variable | expand |
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 >
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!
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
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(-)