[U-Boot] [PATCH v2 0/5] env: Add support for environment files

[U-Boot] [PATCH v2 0/5] env: Add support for environment files

[Simon Glass]

At present U-Boot environment variables, and thus scripts, are defined
by CONFIG_EXTRA_ENV_SETTINGS. It is painful to add large amounts of text
to this file and dealing with quoting and newlines is harder than it
should be. It would be better if we could just type the script into a
text file and have it included by U-Boot.

This series adds a feature that brings in a .env file associated with the
board config, if present. To use it, create a file in a board/<vendor>/env
directory called <board>.env (or common.env if you want the same
environment for all boards for that vendor).

The environment variables should be of the form "var=value". Values can
extend to multiple lines. See the README under 'Environment Variables:'
for more information and an example.

After discussion on the mailing list the .emv file was moved from
include/configs to board/. See here:

   http://patchwork.ozlabs.org/patch/237120/

There was also talk of using the C preprocessor for these boards. I tried
this out and found it to be extremely useful. In fact without it, the
scripts probably cannot move from the config header file, since many scripts
are put together based on information from CONFIG variables.

Another discussion was compatibility with the environment commands
'env export -t' and 'env import -t'. This series permits these to be used
and the environment is exported and imported as expected. I have dropped
the ugly \0 approach in favour of a more flexible awk script for parsing
the environment file. The environment commands use \ at the end of a line
for continuation which works nicely with this feature. I have added a patch
to 'run' so that it runs the entire script, not just the first line. A nice
benefit is that there is no longer any need to put ';' at the end of every
line - in other words U-Boot scripts become proper scripts with multiple
lines instead of messy and fragile continuations.

As an example, I have converted most of the tegra environment over to this
new approach on an RFC basis.

Changes in v2:
- Add additional include to env_embedded to deal with its dirty trick
- Add dependency rule so that the environment is rebuilt when it changes
- Add information and updated example script to README
- Add new patch to adjust 'run' command to better support testing
- Add new patch to get 'env import/export' working on sandbox
- Add new patch to illustrate the impact on Tegra environment
- Add separate patch to enable C preprocessor for environment files
- Enable var+=value form to simplify composing variables in multiple steps
- Move .env file from include/configs to board/
- Use awk script to process environment since it is much easier on the brain

Simon Glass (5):
  sandbox: Support 'env import' and 'env export'
  Make 'run' use run_command_list() instead of run_command()
  Allow U-Boot scripts to be placed in a .env file
  env: Allow environment files to use the C preprocessor
  RFC: tegra: Convert to using environment files

 Makefile                            |  31 +++++++++-
 README                              |  49 +++++++++++++++
 board/nvidia/env/common.env         |  79 ++++++++++++++++++++++++
 common/cmd_nvedit.c                 |  31 ++++++----
 common/env_embedded.c               |   1 +
 common/main.c                       |   2 +-
 config.mk                           |   2 +
 include/configs/tegra-common-post.h | 120 +-----------------------------------
 include/env_default.h               |   2 +
 mkconfig                            |   6 ++
 tools/scripts/env2string.awk        |  49 +++++++++++++++
 11 files changed, 238 insertions(+), 134 deletions(-)
 create mode 100644 board/nvidia/env/common.env
 create mode 100644 tools/scripts/env2string.awk

-- 
1.8.3

[U-Boot] [PATCH v2 1/5] sandbox: Support 'env import' and 'env export'

[Simon Glass]

Adjust the code for these commands so that they work on sandbox.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Add new patch to get 'env import/export' working on sandbox

 common/cmd_nvedit.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 2478c95..e8493c2 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -49,6 +49,7 @@
 #include <watchdog.h>
 #include <linux/stddef.h>
 #include <asm/byteorder.h>
+#include <asm/io.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -864,7 +865,8 @@ static int do_env_export(cmd_tbl_t *cmdtp, int flag,
 			 int argc, char * const argv[])
 {
 	char	buf[32];
-	char	*addr, *cmd, *res;
+	ulong	addr;
+	char	*ptr, *cmd, *res;
 	size_t	size = 0;
 	ssize_t	len;
 	env_t	*envp;
@@ -909,10 +911,11 @@ NXTARG:		;
 	if (argc < 1)
 		return CMD_RET_USAGE;
 
-	addr = (char *)simple_strtoul(argv[0], NULL, 16);
+	addr = simple_strtoul(argv[0], NULL, 16);
+	ptr = map_sysmem(addr, size);
 
 	if (size)
-		memset(addr, '\0', size);
+		memset(ptr, '\0', size);
 
 	argc--;
 	argv++;
@@ -920,7 +923,7 @@ NXTARG:		;
 	if (sep) {		/* export as text file */
 		len = hexport_r(&env_htab, sep,
 				H_MATCH_KEY | H_MATCH_IDENT,
-				&addr, size, argc, argv);
+				&ptr, size, argc, argv);
 		if (len < 0) {
 			error("Cannot export environment: errno = %d\n", errno);
 			return 1;
@@ -931,12 +934,12 @@ NXTARG:		;
 		return 0;
 	}
 
-	envp = (env_t *)addr;
+	envp = (env_t *)ptr;
 
 	if (chk)		/* export as checksum protected block */
 		res = (char *)envp->data;
 	else			/* export as raw binary data */
-		res = addr;
+		res = ptr;
 
 	len = hexport_r(&env_htab, '\0',
 			H_MATCH_KEY | H_MATCH_IDENT,
@@ -978,7 +981,8 @@ sep_err:
 static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 			 int argc, char * const argv[])
 {
-	char	*cmd, *addr;
+	ulong	addr;
+	char	*cmd, *ptr;
 	char	sep = '\n';
 	int	chk = 0;
 	int	fmt = 0;
@@ -1022,12 +1026,13 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 	if (!fmt)
 		printf("## Warning: defaulting to text format\n");
 
-	addr = (char *)simple_strtoul(argv[0], NULL, 16);
+	addr = simple_strtoul(argv[0], NULL, 16);
+	ptr = map_sysmem(addr, 0);
 
 	if (argc == 2) {
 		size = simple_strtoul(argv[1], NULL, 16);
 	} else {
-		char *s = addr;
+		char *s = ptr;
 
 		size = 0;
 
@@ -1047,7 +1052,7 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 
 	if (chk) {
 		uint32_t crc;
-		env_t *ep = (env_t *)addr;
+		env_t *ep = (env_t *)ptr;
 
 		size -= offsetof(env_t, data);
 		memcpy(&crc, &ep->crc, sizeof(crc));
@@ -1056,11 +1061,11 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 			puts("## Error: bad CRC, import failed\n");
 			return 1;
 		}
-		addr = (char *)ep->data;
+		ptr = (char *)ep->data;
 	}
 
-	if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR,
-			0, NULL) == 0) {
+	if (himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR, 0,
+		      NULL) == 0) {
 		error("Environment import failed: errno = %d\n", errno);
 		return 1;
 	}
-- 
1.8.3

[U-Boot] [PATCH v2 2/5] Make 'run' use run_command_list() instead of run_command()

[Simon Glass]

In the case where an environment variable spans multiple lines, we should
use run_command_list() so that all lines are executed. This shold be
backwards compatible with existing behaviour for existing scripts.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Add new patch to adjust 'run' command to better support testing

 common/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/main.c b/common/main.c
index 56da214..3b7f2f7 100644
--- a/common/main.c
+++ b/common/main.c
@@ -1560,7 +1560,7 @@ int do_run (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 			return 1;
 		}
 
-		if (run_command(arg, flag) != 0)
+		if (run_command_list(arg, -1, flag) != 0)
 			return 1;
 	}
 	return 0;
-- 
1.8.3

[U-Boot] [PATCH v2 3/5] Allow U-Boot scripts to be placed in a .env file

[Simon Glass]

At present U-Boot environment variables, and thus scripts, are defined
by CONFIG_EXTRA_ENV_SETTINGS. It is painful to add large amounts of text
to this file and dealing with quoting and newlines is harder than it
should be. It would be better if we could just type the script into a
text file and have it included by U-Boot.

Add a feature that brings in a .env file associated with the board
config, if present. To use it, create a file in a board/<vendor>/env
directory called <board>.env (or common.env if you want the same
environment for all boards).

The environment variables should be of the form "var=value". Values can
extend to multiple lines. See the README under 'Environment Variables:'
for more information and an example.

Comments are not permitted in the environment with this commit.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Add additional include to env_embedded to deal with its dirty trick
- Add dependency rule so that the environment is rebuilt when it changes
- Add information and updated example script to README
- Move .env file from include/configs to board/
- Use awk script to process environment since it is much easier on the brain

 Makefile                     | 30 +++++++++++++++++++++++++++++-
 README                       | 34 ++++++++++++++++++++++++++++++++++
 common/env_embedded.c        |  1 +
 config.mk                    |  2 ++
 include/env_default.h        |  2 ++
 mkconfig                     |  6 ++++++
 tools/scripts/env2string.awk | 43 +++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 117 insertions(+), 1 deletion(-)
 create mode 100644 tools/scripts/env2string.awk

diff --git a/Makefile b/Makefile
index 693b3f2..9dae750 100644
--- a/Makefile
+++ b/Makefile
@@ -695,7 +695,7 @@ $(obj)include/autoconf.mk.dep: $(obj)include/config.h include/common.h
 	$(CC) -x c -DDO_DEPS_ONLY -M $(CFLAGS) $(CPPFLAGS) \
 		-MQ $(obj)include/autoconf.mk include/common.h > $@
 
-$(obj)include/autoconf.mk: $(obj)include/config.h
+$(obj)include/generated/autoconf.mk.base: $(obj)include/config.h
 	@$(XECHO) Generating $@ ; \
 	set -e ; \
 	: Extract the config macros ; \
@@ -703,6 +703,34 @@ $(obj)include/autoconf.mk: $(obj)include/config.h
 		sed -n -f tools/scripts/define2mk.sed > $@.tmp && \
 	mv $@.tmp $@
 
+# We expect '<board>.env' but failing that will use 'common.env'
+ENV_HEADER := $(obj)include/generated/environment.h
+ENV_DIR := $(if $(VENDOR),$(VENDOR)/env,$(BOARD)/env)
+ENV_FILE_BOARD := $(src)board/${ENV_DIR}/$(BOARD).env
+ENV_FILE_COMMON := $(src)board/${ENV_DIR}/common.env
+ENV_FILE := $(if $(wildcard $(ENV_FILE_BOARD)),$(ENV_FILE_BOARD),$(ENV_FILE_COMMON))
+
+# Regenerate the environment if it changes
+$(obj)include/generated/environment.in: $(obj)include/generated/autoconf.mk.base \
+		$(wildcard $(ENV_FILE))
+	if [ -f "$(ENV_FILE)" ]; then \
+		cat $(ENV_FILE) >$@ ; \
+	else \
+		echo -n >$@ ; \
+	fi
+
+$(obj)include/generated/environment.inc: $(obj)include/generated/environment.in
+	@$(XECHO) Generating $@ ; \
+	set -e ; \
+	: Process the environment file ; \
+	echo -n "CONFIG_EXTRA_ENV_TEXT=" >$@ ; \
+	echo -n "#define CONFIG_EXTRA_ENV_TEXT " >$(ENV_HEADER) ; \
+	awk -f tools/scripts/env2string.awk $< | \
+		tee -a $(ENV_HEADER) >>$@ ; \
+
+$(obj)include/autoconf.mk: $(obj)include/generated/environment.inc
+	cat $(obj)include/generated/autoconf.mk.base $< >$@
+
 $(obj)include/generated/generic-asm-offsets.h:	$(obj)include/autoconf.mk.dep \
 	$(obj)lib/asm-offsets.s
 	@$(XECHO) Generating $@
diff --git a/README b/README
index cd0336c..2c8a8c9 100644
--- a/README
+++ b/README
@@ -4363,6 +4363,40 @@ environment. As long as you don't save the environment you are
 working with an in-memory copy. In case the Flash area containing the
 environment is erased by accident, a default environment is provided.
 
+The default environment is created in include/env_default.h, and can be
+augmented by various CONFIG defines. See that file for details. In
+particular you can define CONFIG_EXTRA_ENV_SETTINGS in your board file
+to add environment variables (see 'CONFIG_EXTRA_ENV_SETTINGS' above
+for details).
+
+It is also possible to create an environment file with the name
+board/<vendor>/env/<board>.env for your board. If that file is not present
+then U-Boot will look for board/<vendor>/env/common.env so that you can
+have a common environment for all vendor boards.
+
+This is a plain text file where you can type your environment variables in
+the form 'var=value'. Blank lines and multi-line variables are supported.
+
+For example, for snapper9260 you would create a text file called
+board/bluewater/env/snapper9260.env containing the environment text.
+
+>>>
+bootcmd=
+	if [ -z ${tftpserverip} ]; then
+		echo "Use 'setenv tftpserverip a.b.c.d' to set IP address."
+	fi
+
+	usb start; setenv autoload n; bootp;
+	tftpboot ${tftpserverip}:
+	bootm
+failed=
+	echo boot failed - please check your image
+<<<
+
+The resulting environment can be exported and importing using the
+'env export/import -t' commands.
+
+
 Some configuration options can be set using Environment Variables.
 
 List of environment variables (most likely not complete):
diff --git a/common/env_embedded.c b/common/env_embedded.c
index 52bc687..c8e7ee9 100644
--- a/common/env_embedded.c
+++ b/common/env_embedded.c
@@ -24,6 +24,7 @@
 #ifndef __ASSEMBLY__
 #define	__ASSEMBLY__			/* Dirty trick to get only #defines */
 #endif
+#include <generated/environment.h>
 #define	__ASM_STUB_PROCESSOR_H__	/* don't include asm/processor. */
 #include <config.h>
 #undef	__ASSEMBLY__
diff --git a/config.mk b/config.mk
index ddf350e..26385da 100644
--- a/config.mk
+++ b/config.mk
@@ -181,8 +181,10 @@ sinclude $(TOPDIR)/$(CPUDIR)/$(SOC)/config.mk	# include  SoC	specific rules
 endif
 ifdef	VENDOR
 BOARDDIR = $(VENDOR)/$(BOARD)
+ENVDIR=${vendor}/env
 else
 BOARDDIR = $(BOARD)
+ENVDIR=${board}/env
 endif
 ifdef	BOARD
 sinclude $(TOPDIR)/board/$(BOARDDIR)/config.mk	# include board specific rules
diff --git a/include/env_default.h b/include/env_default.h
index 39c5b7c..a394df5 100644
--- a/include/env_default.h
+++ b/include/env_default.h
@@ -137,6 +137,8 @@ const uchar default_environment[] = {
 #ifdef	CONFIG_EXTRA_ENV_SETTINGS
 	CONFIG_EXTRA_ENV_SETTINGS
 #endif
+	/* This is created in the Makefile */
+	CONFIG_EXTRA_ENV_TEXT
 	"\0"
 #ifdef DEFAULT_ENV_INSTANCE_EMBEDDED
 	}
diff --git a/mkconfig b/mkconfig
index 73f852e..7c286a7 100755
--- a/mkconfig
+++ b/mkconfig
@@ -141,8 +141,10 @@ fi
 # Assign board directory to BOARDIR variable
 if [ -z "${vendor}" ] ; then
     BOARDDIR=${board}
+    ENVDIR=${board}/env
 else
     BOARDDIR=${vendor}/${board}
+    ENVDIR=${vendor}/env
 fi
 
 #
@@ -171,10 +173,14 @@ echo "#define CONFIG_SYS_BOARD \"${board}\"" >> config.h
 
 cat << EOF >> config.h
 #define CONFIG_BOARDDIR board/$BOARDDIR
+#define CONFIG_ENVDIR board/$ENVDIR
 #include <config_cmd_defaults.h>
 #include <config_defaults.h>
 #include <configs/${CONFIG_NAME}.h>
 #include <asm/config.h>
+#if !defined(DO_DEPS_ONLY) && !defined(__ASSEMBLY__)
+#include <generated/environment.h>
+#endif
 #include <config_fallbacks.h>
 #include <config_uncmd_spl.h>
 EOF
diff --git a/tools/scripts/env2string.awk b/tools/scripts/env2string.awk
new file mode 100644
index 0000000..2d167c0
--- /dev/null
+++ b/tools/scripts/env2string.awk
@@ -0,0 +1,43 @@
+#
+# Sed script to parse a text file containing an environment and convert it
+# to a C string which can be compiled into U-Boot.
+#
+
+# We begin and end with "
+BEGIN {
+	# env holds the env variable we are currently processing
+	env = "";
+	ORS = ""
+	print "\""
+}
+
+{
+	# Quote quotes
+	gsub("\"", "\\\"")
+
+	# Is this the start of a new environment variable?
+	if (match($0, "^([^ =][^ =]*)=(.*)", arr)) {
+		if (length(env) != 0) {
+			# Record the value of the variable now completed
+			vars[var] = env
+		}
+		var = arr[1]
+		env = arr[2]
+	} else {
+		# Change newline to \n
+		env = env "\\n" $0;
+	}
+}
+
+END {
+	# Record the value of the variable now completed
+	if (length(env) != 0) {
+		vars[var] = env
+	}
+
+	# Print out all the variables
+	for (var in vars) {
+		print var "=" vars[var] "\\0";
+	}
+	print "\""
+}
-- 
1.8.3

[U-Boot] [PATCH v2 4/5] env: Allow environment files to use the C preprocessor

[Simon Glass]

In many cases environment variables need access to the U-Boot CONFIG
variables to select different options. Enable this so that the environment
scripts can be as useful as the ones currently in the board config files.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Add separate patch to enable C preprocessor for environment files
- Enable var+=value form to simplify composing variables in multiple steps

 Makefile                     |  3 ++-
 README                       | 17 ++++++++++++++++-
 tools/scripts/env2string.awk |  6 ++++++
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 9dae750..89ac4c5 100644
--- a/Makefile
+++ b/Makefile
@@ -714,7 +714,8 @@ ENV_FILE := $(if $(wildcard $(ENV_FILE_BOARD)),$(ENV_FILE_BOARD),$(ENV_FILE_COMM
 $(obj)include/generated/environment.in: $(obj)include/generated/autoconf.mk.base \
 		$(wildcard $(ENV_FILE))
 	if [ -f "$(ENV_FILE)" ]; then \
-		cat $(ENV_FILE) >$@ ; \
+		$(CPP) -P $(CFLAGS) -x assembler-with-cpp -D__ASSEMBLY__ \
+			-include $(obj)include/config.h $(ENV_FILE) -o $@; \
 	else \
 		echo -n >$@ ; \
 	fi
diff --git a/README b/README
index 2c8a8c9..6711cd0 100644
--- a/README
+++ b/README
@@ -4376,12 +4376,25 @@ have a common environment for all vendor boards.
 
 This is a plain text file where you can type your environment variables in
 the form 'var=value'. Blank lines and multi-line variables are supported.
+To add additional text to a variable you can use var+=value. This text is
+merged into the variable during the make process and made available as a
+single value to U-Boot.
 
 For example, for snapper9260 you would create a text file called
 board/bluewater/env/snapper9260.env containing the environment text.
 
+This file can include C-style comments. Blank lines and multi-line
+variables are supported, and you can use normal C preprocessor directives
+and CONFIG defines from your board config also.
+
 >>>
+stdout=serial
+#ifdef CONFIG_LCD
+stdout+=,lcd
+#endif
 bootcmd=
+	/* U-Boot script for booting */
+
 	if [ -z ${tftpserverip} ]; then
 		echo "Use 'setenv tftpserverip a.b.c.d' to set IP address."
 	fi
@@ -4390,7 +4403,9 @@ bootcmd=
 	tftpboot ${tftpserverip}:
 	bootm
 failed=
-	echo boot failed - please check your image
+	/* Print a message when boot fails */
+	echo CONFIG_SYS_BOARD boot failed - please check your image
+	echo Load address is CONFIG_SYS_LOAD_ADDR
 <<<
 
 The resulting environment can be exported and importing using the
diff --git a/tools/scripts/env2string.awk b/tools/scripts/env2string.awk
index 2d167c0..d647cf3 100644
--- a/tools/scripts/env2string.awk
+++ b/tools/scripts/env2string.awk
@@ -23,6 +23,12 @@ BEGIN {
 		}
 		var = arr[1]
 		env = arr[2]
+
+		# Deal with +=
+		if (match(var, "(.*)[+]$", var_arr)) {
+			var = var_arr[1]
+			env = vars[var] env
+		}
 	} else {
 		# Change newline to \n
 		env = env "\\n" $0;
-- 
1.8.3

[U-Boot] [PATCH v2 5/5] RFC: tegra: Convert to using environment files

[Simon Glass]

This seems more intuitive that the current #define way of doing things.
The resulting code is shorter, avoids the quoting and line continuation
pain, and also improves the clumsy way that stdio variables are created:

 #ifdef CONFIG_VIDEO_TEGRA
 #define STDOUT_LCD ",lcd"
 #else
 #define STDOUT_LCD ""
 #endif

...
 #define TEGRA_DEVICE_SETTINGS \
	"stdout=serial" STDOUT_LCD "\0" \
	...

The MEM_LAYOUT_ENV_SETTINGS variable is left in the header files, since
it depends on the SOC type and we probably don't want to add .emv files
for each board at this stage.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Add new patch to illustrate the impact on Tegra environment

 board/nvidia/env/common.env         |  79 ++++++++++++++++++++++++
 include/configs/tegra-common-post.h | 120 +-----------------------------------
 2 files changed, 80 insertions(+), 119 deletions(-)
 create mode 100644 board/nvidia/env/common.env

diff --git a/board/nvidia/env/common.env b/board/nvidia/env/common.env
new file mode 100644
index 0000000..aca36a5
--- /dev/null
+++ b/board/nvidia/env/common.env
@@ -0,0 +1,79 @@
+#ifndef CONFIG_BOOTCOMMAND
+rootpart=1
+script_boot=
+	if load ${devtype} ${devnum}:${rootpart}
+			${scriptaddr} ${prefix}${script}; then
+		echo ${script} found! Executing ...
+		source ${scriptaddr}
+	fi
+
+scan_boot=
+	echo Scanning ${devtype} ${devnum}...
+	for prefix in ${boot_prefixes}; do
+		for script in ${boot_scripts}; do
+			run script_boot
+		done
+	done
+
+boot_targets=
+boot_prefixes=/ /boot/
+boot_scripts=boot.scr.uimg boot.scr
+
+#ifdef CONFIG_CMD_MMC
+mmc_boot=
+	setenv devtype mmc
+		if mmc dev ${devnum}; then
+			run scan_boot
+		fi
+bootcmd_mmc0=setenv devnum 0; run mmc_boot
+bootcmd_mmc1=setenv devnum 1; run mmc_booxt
+boot_targets+= mmc1 mmc0
+#endif
+
+#ifdef CONFIG_CMD_USB
+#define BOOTCMD_INIT_USB run usb_init
+usb_init=
+	if ${usb_need_init}; then
+		set usb_need_init false
+		usb start 0
+	fi
+usb_boot=
+	setenv devtype usb
+	BOOTCMD_INIT_USB
+	if usb dev ${devnum}; then
+		run scan_boot
+	fi
+bootcmd_usb0=setenv devnum 0; run usb_boot
+boot_targets+= usb0
+#else
+#define BOOTCMD_INIT_USB
+#endif
+
+#ifdef CONFIG_CMD_DHCP
+bootcmd_dhcp=
+	BOOTCMD_INIT_USB
+	if dhcp ${scriptaddr} boot.scr.uimg; then
+		source ${scriptaddr}
+	fi
+boot_targets+= dhcp
+#endif
+
+bootcmd=for target in ${boot_targets}; do run bootcmd_${target}; done
+#endif
+
+/* Decide on values for stdio */
+stdin=serial
+stdout=serial
+stderr=serial
+
+#ifdef CONFIG_TEGRA_KEYBOARD
+stdin+=,tegra-kbc
+#endif
+
+#ifdef CONFIG_USB_KEYBOARD
+stdin+=,usbkbd
+#endif
+
+#ifdef CONFIG_VIDEO_TEGRA
+stdout+=,lcd
+#endif
diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-common-post.h
index 6ed2fde..e14a0e1 100644
--- a/include/configs/tegra-common-post.h
+++ b/include/configs/tegra-common-post.h
@@ -24,131 +24,13 @@
 #ifndef __TEGRA_COMMON_POST_H
 #define __TEGRA_COMMON_POST_H
 
-#ifdef CONFIG_BOOTCOMMAND
-
-#define BOOTCMDS_COMMON ""
-
-#else
-
-#ifdef CONFIG_CMD_MMC
-#define BOOTCMDS_MMC \
-	"mmc_boot=" \
-		"setenv devtype mmc; " \
-		"if mmc dev ${devnum}; then " \
-			"run scan_boot; " \
-		"fi\0" \
-	"bootcmd_mmc0=setenv devnum 0; run mmc_boot;\0" \
-	"bootcmd_mmc1=setenv devnum 1; run mmc_boot;\0"
-#define BOOT_TARGETS_MMC "mmc1 mmc0"
-#else
-#define BOOTCMDS_MMC ""
-#define BOOT_TARGETS_MMC ""
-#endif
-
-#ifdef CONFIG_CMD_USB
-#define BOOTCMD_INIT_USB "run usb_init; "
-#define BOOTCMDS_USB \
-	"usb_init=" \
-		"if ${usb_need_init}; then " \
-			"set usb_need_init false; " \
-			"usb start 0; " \
-		"fi\0" \
-	\
-	"usb_boot=" \
-		"setenv devtype usb; " \
-		BOOTCMD_INIT_USB \
-		"if usb dev ${devnum}; then " \
-			"run scan_boot; " \
-		"fi\0" \
-	\
-	"bootcmd_usb0=setenv devnum 0; run usb_boot;\0"
-#define BOOT_TARGETS_USB "usb0"
-#else
-#define BOOTCMD_INIT_USB ""
-#define BOOTCMDS_USB ""
-#define BOOT_TARGETS_USB ""
-#endif
-
-#ifdef CONFIG_CMD_DHCP
-#define BOOTCMDS_DHCP \
-	"bootcmd_dhcp=" \
-		BOOTCMD_INIT_USB \
-		"if dhcp ${scriptaddr} boot.scr.uimg; then "\
-			"source ${scriptaddr}; " \
-		"fi\0"
-#define BOOT_TARGETS_DHCP "dhcp"
-#else
-#define BOOTCMDS_DHCP ""
-#define BOOT_TARGETS_DHCP ""
-#endif
-
-#define BOOTCMDS_COMMON \
-	"rootpart=1\0" \
-	\
-	"script_boot="                                                    \
-		"if load ${devtype} ${devnum}:${rootpart} "               \
-				"${scriptaddr} ${prefix}${script}; then " \
-			"echo ${script} found! Executing ...;"            \
-			"source ${scriptaddr};"                           \
-		"fi;\0"                                                   \
-	\
-	"scan_boot="                                                      \
-		"echo Scanning ${devtype} ${devnum}...; "                 \
-		"for prefix in ${boot_prefixes}; do "                     \
-			"for script in ${boot_scripts}; do "              \
-				"run script_boot; "                       \
-			"done; "                                          \
-		"done;\0"                                                 \
-	\
-	"boot_targets=" \
-		BOOT_TARGETS_MMC " " \
-		BOOT_TARGETS_USB " " \
-		BOOT_TARGETS_DHCP " " \
-		"\0" \
-	\
-	"boot_prefixes=/ /boot/\0" \
-	\
-	"boot_scripts=boot.scr.uimg boot.scr\0" \
-	\
-	BOOTCMDS_MMC \
-	BOOTCMDS_USB \
-	BOOTCMDS_DHCP
-
-#define CONFIG_BOOTCOMMAND \
-	"for target in ${boot_targets}; do run bootcmd_${target}; done"
-
-#endif
-
-#ifdef CONFIG_TEGRA_KEYBOARD
-#define STDIN_KBD_KBC ",tegra-kbc"
-#else
-#define STDIN_KBD_KBC ""
-#endif
-
 #ifdef CONFIG_USB_KEYBOARD
-#define STDIN_KBD_USB ",usbkbd"
 #define CONFIG_SYS_USB_EVENT_POLL
 #define CONFIG_PREBOOT			"usb start"
-#else
-#define STDIN_KBD_USB ""
 #endif
 
-#ifdef CONFIG_VIDEO_TEGRA
-#define STDOUT_LCD ",lcd"
-#else
-#define STDOUT_LCD ""
-#endif
-
-#define TEGRA_DEVICE_SETTINGS \
-	"stdin=serial" STDIN_KBD_KBC STDIN_KBD_USB "\0" \
-	"stdout=serial" STDOUT_LCD "\0" \
-	"stderr=serial" STDOUT_LCD "\0" \
-	""
-
 #define CONFIG_EXTRA_ENV_SETTINGS \
-	TEGRA_DEVICE_SETTINGS \
-	MEM_LAYOUT_ENV_SETTINGS \
-	BOOTCMDS_COMMON
+	MEM_LAYOUT_ENV_SETTINGS
 
 #if defined(CONFIG_TEGRA20_SFLASH) || defined(CONFIG_TEGRA20_SLINK) || defined(CONFIG_TEGRA114_SPI)
 #define CONFIG_FDT_SPI
-- 
1.8.3

[U-Boot] [PATCH v2 3/5] Allow U-Boot scripts to be placed in a .env file

[Stephen Warren]

On 06/24/2013 02:46 PM, Simon Glass wrote:
> At present U-Boot environment variables, and thus scripts, are defined
> by CONFIG_EXTRA_ENV_SETTINGS. It is painful to add large amounts of text
> to this file and dealing with quoting and newlines is harder than it
> should be. It would be better if we could just type the script into a
> text file and have it included by U-Boot.
> 
> Add a feature that brings in a .env file associated with the board
> config, if present. To use it, create a file in a board/<vendor>/env
> directory called <board>.env (or common.env if you want the same
> environment for all boards).

I'm not entirely sure how useful common.env is.

Consider that many different Tegra boards, from different vendors, all
share the same environment. For example boards/compulab/trimslice shares
the core environment with boards/nvidia/*. IIUC, common.env wouldn't
work in this case, since the vendor is different. To solve that, we
could easily symlink board/compulab/env/trimslice.env ->
board/nvidia/env/something.env. But in that case, why not always rely on
creating symlinks, instead of having the common.env fallback work too?
Also consider that compulab makes a ton of boards only some of which use
Tegra, so even a common.env in board/compulab/env wouldn't be that useful.

> diff --git a/Makefile b/Makefile

> +$(obj)include/generated/environment.inc: $(obj)include/generated/environment.in
> +	@$(XECHO) Generating $@ ; \
> +	set -e ; \
> +	: Process the environment file ; \
> +	echo -n "CONFIG_EXTRA_ENV_TEXT=" >$@ ; \
> +	echo -n "#define CONFIG_EXTRA_ENV_TEXT " >$(ENV_HEADER) ; \
> +	awk -f tools/scripts/env2string.awk $< | \
> +		tee -a $(ENV_HEADER) >>$@ ; \

That generates two different files. Doesn't this confuse make a bit,
since if make needs to create $(ENV_HEADER) (which isn't $@ for that
rule) it won't know how to? I've certainly seen problems due to similar
make setups in the past. Perhaps even though it's a bit extra build-time
work, the two files should be generated separately? Or, is there some
reason here that there won't be a problem?

> diff --git a/README b/README

> +For example, for snapper9260 you would create a text file called
> +board/bluewater/env/snapper9260.env containing the environment text.
> +
> +>>>
> +bootcmd=
> +	if [ -z ${tftpserverip} ]; then
> +		echo "Use 'setenv tftpserverip a.b.c.d' to set IP address."
> +	fi
> +
> +	usb start; setenv autoload n; bootp;
> +	tftpboot ${tftpserverip}:
> +	bootm
> +failed=
> +	echo boot failed - please check your image
> +<<<

Presumably the parser looks for something like /^[A-Z]=/i to know when
to "switch" to a new environment variable. Is some form of escaping
useful if you want to include that form of text in your environment
variable's value? If the parser is somehow smarter than that, it might
be worth explaining the syntax structure a little more above.

> diff --git a/tools/scripts/env2string.awk b/tools/scripts/env2string.awk

> +# We begin and end with "

I'm not sure what the " means at the end of that line.

[U-Boot] [PATCH v2 4/5] env: Allow environment files to use the C preprocessor

[Stephen Warren]

On 06/24/2013 02:46 PM, Simon Glass wrote:
> In many cases environment variables need access to the U-Boot CONFIG
> variables to select different options. Enable this so that the environment
> scripts can be as useful as the ones currently in the board config files.

The addition of += seems like a separate change to enabling
pre-processing, but I guess they're both simple enough and this is a new
features, perhaps it's not a big deal.

> diff --git a/Makefile b/Makefile

>  $(obj)include/generated/environment.in: $(obj)include/generated/autoconf.mk.base \
>  		$(wildcard $(ENV_FILE))
>  	if [ -f "$(ENV_FILE)" ]; then \
> -		cat $(ENV_FILE) >$@ ; \
> +		$(CPP) -P $(CFLAGS) -x assembler-with-cpp -D__ASSEMBLY__ \
> +			-include $(obj)include/config.h $(ENV_FILE) -o $@; \

I guess -undef doesn't make sense here, since config.h could well rely
on standard pre-defined macros.

Does it make sense to -D __UBOOT_CONFIG__ rather than, or in addition
to, -D __ASSEMBLY__ here, so headers can tell what they're being
included for? The series I sent for dtc+cpp did -D __DTS__ for similar
reasons.

> diff --git a/tools/scripts/env2string.awk b/tools/scripts/env2string.awk

> +		# Deal with +=
> +		if (match(var, "(.*)[+]$", var_arr)) {
> +			var = var_arr[1]
> +			env = vars[var] env
> +		}

Does this work if you write just:

foo+=bar

rather than:

foo=
foo+=bar

It might be worth allowing the former syntax, so you don't need to
explicitly assign empty values before a sequence of ifdef'd +=
operations. If the above blows up, at least a descriptive error message
might be useful.

[U-Boot] [PATCH v2 5/5] RFC: tegra: Convert to using environment files

[Stephen Warren]

On 06/24/2013 02:46 PM, Simon Glass wrote:
> This seems more intuitive that the current #define way of doing things.
> The resulting code is shorter, avoids the quoting and line continuation
> pain, and also improves the clumsy way that stdio variables are created:
> 
>  #ifdef CONFIG_VIDEO_TEGRA
>  #define STDOUT_LCD ",lcd"
>  #else
>  #define STDOUT_LCD ""
>  #endif
> 
> ...
>  #define TEGRA_DEVICE_SETTINGS \
> 	"stdout=serial" STDOUT_LCD "\0" \
> 	...
> 
> The MEM_LAYOUT_ENV_SETTINGS variable is left in the header files, since
> it depends on the SOC type and we probably don't want to add .emv files
> for each board at this stage.

Presumably e.g. seaboard.env could #include "tegra20.env" in order to
allow *.env to define MEM_LAYOUT_ENV_SETTINGS?

BTW, what's the #include -I path for these files? Perhaps it's worth
setting it up as board/$vendor/$board/env board/$vendor/env
arch/$arch/env to match the dtc+cpp patches I posted?

> diff --git a/board/nvidia/env/common.env b/board/nvidia/env/common.env

> +bootcmd_mmc0=setenv devnum 0; run mmc_boot
> +bootcmd_mmc1=setenv devnum 1; run mmc_booxt
> +boot_targets+= mmc1 mmc0

The first two lines there have no space after = and the last has space
after +=. Does that get stripped? Should it?

> diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-common-post.h

> -#ifdef CONFIG_BOOTCOMMAND
> -
> -#define BOOTCMDS_COMMON ""
> -
> -#else
...

Overall this change seems reasonable, but as I alluded to earlier,
removing this from tegra-common-post.h will break Tegra boards for
vendors other than NVIDIA. I guess that's part of why this patch is RFC.

[U-Boot] [PATCH v2 3/5] Allow U-Boot scripts to be placed in a .env file

[Simon Glass]

Hi Stephen,

On Wed, Jun 26, 2013 at 1:56 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/24/2013 02:46 PM, Simon Glass wrote:
>> At present U-Boot environment variables, and thus scripts, are defined
>> by CONFIG_EXTRA_ENV_SETTINGS. It is painful to add large amounts of text
>> to this file and dealing with quoting and newlines is harder than it
>> should be. It would be better if we could just type the script into a
>> text file and have it included by U-Boot.
>>
>> Add a feature that brings in a .env file associated with the board
>> config, if present. To use it, create a file in a board/<vendor>/env
>> directory called <board>.env (or common.env if you want the same
>> environment for all boards).

[well perhaps it is time to get back to this]

>
> I'm not entirely sure how useful common.env is.
>
> Consider that many different Tegra boards, from different vendors, all
> share the same environment. For example boards/compulab/trimslice shares
> the core environment with boards/nvidia/*. IIUC, common.env wouldn't
> work in this case, since the vendor is different. To solve that, we
> could easily symlink board/compulab/env/trimslice.env ->
> board/nvidia/env/something.env. But in that case, why not always rely on
> creating symlinks, instead of having the common.env fallback work too?
> Also consider that compulab makes a ton of boards only some of which use
> Tegra, so even a common.env in board/compulab/env wouldn't be that useful.

I find symlinks a pain in the source tree. I think they should be the
exception rather than the rule. This feature hear mirrors the current
board/<vendor>/common directory which is fairly widely used.

$ ls board/*/common -d |wc -l
21

I feel that Compulab can do their own common file if they want it. A
file common to all Tegra perhaps belongs in arch/arm/.. somewhere?

>
>> diff --git a/Makefile b/Makefile
>
>> +$(obj)include/generated/environment.inc: $(obj)include/generated/environment.in
>> +     @$(XECHO) Generating $@ ; \
>> +     set -e ; \
>> +     : Process the environment file ; \
>> +     echo -n "CONFIG_EXTRA_ENV_TEXT=" >$@ ; \
>> +     echo -n "#define CONFIG_EXTRA_ENV_TEXT " >$(ENV_HEADER) ; \
>> +     awk -f tools/scripts/env2string.awk $< | \
>> +             tee -a $(ENV_HEADER) >>$@ ; \
>
> That generates two different files. Doesn't this confuse make a bit,
> since if make needs to create $(ENV_HEADER) (which isn't $@ for that
> rule) it won't know how to? I've certainly seen problems due to similar
> make setups in the past. Perhaps even though it's a bit extra build-time
> work, the two files should be generated separately? Or, is there some
> reason here that there won't be a problem?

Yes I will separate it. I agree it's a bit icky in that if someone
manually deletes one file they may not get the other. The speed-up is
small.

>
>> diff --git a/README b/README
>
>> +For example, for snapper9260 you would create a text file called
>> +board/bluewater/env/snapper9260.env containing the environment text.
>> +
>> +>>>
>> +bootcmd=
>> +     if [ -z ${tftpserverip} ]; then
>> +             echo "Use 'setenv tftpserverip a.b.c.d' to set IP address."
>> +     fi
>> +
>> +     usb start; setenv autoload n; bootp;
>> +     tftpboot ${tftpserverip}:
>> +     bootm
>> +failed=
>> +     echo boot failed - please check your image
>> +<<<
>
> Presumably the parser looks for something like /^[A-Z]=/i to know when
> to "switch" to a new environment variable. Is some form of escaping
> useful if you want to include that form of text in your environment
> variable's value? If the parser is somehow smarter than that, it might
> be worth explaining the syntax structure a little more above.

The match string is a little more basic than that since there are few
restrictions on environment variable names as far as I know:

"^([^ =][^ =]*)=(.*)"

I'll add some more details to the commit message - but the idea is to
indent your environment to avoid problems.

>
>> diff --git a/tools/scripts/env2string.awk b/tools/scripts/env2string.awk
>
>> +# We begin and end with "
>
> I'm not sure what the " means at the end of that line.

I'll improve that comment.

Regards,
Simon

[U-Boot] [PATCH v2 4/5] env: Allow environment files to use the C preprocessor

[Simon Glass]

Hi Stephen,

On Wed, Jun 26, 2013 at 2:05 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/24/2013 02:46 PM, Simon Glass wrote:
>> In many cases environment variables need access to the U-Boot CONFIG
>> variables to select different options. Enable this so that the environment
>> scripts can be as useful as the ones currently in the board config files.
>
> The addition of += seems like a separate change to enabling
> pre-processing, but I guess they're both simple enough and this is a new
> features, perhaps it's not a big deal.

Yes it is separate but I was concerned about not creating too many
patches for the same feature.

>
>> diff --git a/Makefile b/Makefile
>
>>  $(obj)include/generated/environment.in: $(obj)include/generated/autoconf.mk.base \
>>               $(wildcard $(ENV_FILE))
>>       if [ -f "$(ENV_FILE)" ]; then \
>> -             cat $(ENV_FILE) >$@ ; \
>> +             $(CPP) -P $(CFLAGS) -x assembler-with-cpp -D__ASSEMBLY__ \
>> +                     -include $(obj)include/config.h $(ENV_FILE) -o $@; \
>
> I guess -undef doesn't make sense here, since config.h could well rely
> on standard pre-defined macros.

In some cases yes.

>
> Does it make sense to -D __UBOOT_CONFIG__ rather than, or in addition
> to, -D __ASSEMBLY__ here, so headers can tell what they're being
> included for? The series I sent for dtc+cpp did -D __DTS__ for similar
> reasons.

Will do.

>
>> diff --git a/tools/scripts/env2string.awk b/tools/scripts/env2string.awk
>
>> +             # Deal with +=
>> +             if (match(var, "(.*)[+]$", var_arr)) {
>> +                     var = var_arr[1]
>> +                     env = vars[var] env
>> +             }
>
> Does this work if you write just:
>
> foo+=bar
>
> rather than:
>
> foo=
> foo+=bar
>
> It might be worth allowing the former syntax, so you don't need to
> explicitly assign empty values before a sequence of ifdef'd +=
> operations. If the above blows up, at least a descriptive error message
> might be useful.

Yes that works for me. With awk variable appear when used which helps here.

Regards,
Simon

[U-Boot] [PATCH v2 5/5] RFC: tegra: Convert to using environment files

[Simon Glass]

Hi Stephen,

On Wed, Jun 26, 2013 at 2:16 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/24/2013 02:46 PM, Simon Glass wrote:
>> This seems more intuitive that the current #define way of doing things.
>> The resulting code is shorter, avoids the quoting and line continuation
>> pain, and also improves the clumsy way that stdio variables are created:
>>
>>  #ifdef CONFIG_VIDEO_TEGRA
>>  #define STDOUT_LCD ",lcd"
>>  #else
>>  #define STDOUT_LCD ""
>>  #endif
>>
>> ...
>>  #define TEGRA_DEVICE_SETTINGS \
>>       "stdout=serial" STDOUT_LCD "\0" \
>>       ...
>>
>> The MEM_LAYOUT_ENV_SETTINGS variable is left in the header files, since
>> it depends on the SOC type and we probably don't want to add .emv files
>> for each board at this stage.
>
> Presumably e.g. seaboard.env could #include "tegra20.env" in order to
> allow *.env to define MEM_LAYOUT_ENV_SETTINGS?

Hmmm maybe.

>
> BTW, what's the #include -I path for these files? Perhaps it's worth
> setting it up as board/$vendor/$board/env board/$vendor/env
> arch/$arch/env to match the dtc+cpp patches I posted?

We could do that. I can't think of a down-side, but we need to decide
on where these files should go and what each directory should contain.
I am thinking that perhaps arch/arm/env might make sense and
arch/arm/cpu/armv7/env also.

On the other hand, I'm really not keen to take this all to far on a
first pass, and gain experience with it before building in lots of
bells and whistles.

>
>> diff --git a/board/nvidia/env/common.env b/board/nvidia/env/common.env
>
>> +bootcmd_mmc0=setenv devnum 0; run mmc_boot
>> +bootcmd_mmc1=setenv devnum 1; run mmc_booxt
>> +boot_targets+= mmc1 mmc0
>
> The first two lines there have no space after = and the last has space
> after +=. Does that get stripped? Should it?

We do need that space. There is no stripping done here as it reduces
flexibility.

>
>> diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-common-post.h
>
>> -#ifdef CONFIG_BOOTCOMMAND
>> -
>> -#define BOOTCMDS_COMMON ""
>> -
>> -#else
> ...
>
> Overall this change seems reasonable, but as I alluded to earlier,
> removing this from tegra-common-post.h will break Tegra boards for
> vendors other than NVIDIA. I guess that's part of why this patch is RFC.

Yes, that patch would need a bit of testing plus coming up with a
solution to the problem you identified (see my comments above).

Regards,
Simon

[U-Boot] [PATCH v2 3/5] Allow U-Boot scripts to be placed in a .env file

[Stephen Warren]

On 10/20/2013 09:47 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Wed, Jun 26, 2013 at 1:56 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 06/24/2013 02:46 PM, Simon Glass wrote:
>>> At present U-Boot environment variables, and thus scripts, are defined
>>> by CONFIG_EXTRA_ENV_SETTINGS. It is painful to add large amounts of text
>>> to this file and dealing with quoting and newlines is harder than it
>>> should be. It would be better if we could just type the script into a
>>> text file and have it included by U-Boot.
>>>
>>> Add a feature that brings in a .env file associated with the board
>>> config, if present. To use it, create a file in a board/<vendor>/env
>>> directory called <board>.env (or common.env if you want the same
>>> environment for all boards).
> 
> [well perhaps it is time to get back to this]
> 
>> I'm not entirely sure how useful common.env is.
>>
>> Consider that many different Tegra boards, from different vendors, all
>> share the same environment. For example boards/compulab/trimslice shares
>> the core environment with boards/nvidia/*. IIUC, common.env wouldn't
>> work in this case, since the vendor is different. To solve that, we
>> could easily symlink board/compulab/env/trimslice.env ->
>> board/nvidia/env/something.env. But in that case, why not always rely on
>> creating symlinks, instead of having the common.env fallback work too?
>> Also consider that compulab makes a ton of boards only some of which use
>> Tegra, so even a common.env in board/compulab/env wouldn't be that useful.
> 
> I find symlinks a pain in the source tree. I think they should be the
> exception rather than the rule. This feature here mirrors the current
> board/<vendor>/common directory which is fairly widely used.

Well, we use includes rather than symlinks, e.g. put the following into
boards/compulab/env/trimslice.env:

#include <boards/nvidia/env/something.env>

> $ ls board/*/common -d |wc -l
> 21
> 
> I feel that Compulab can do their own common file if they want it

I'd rather as many board worked the same way as possible; that makes it
much easier for distros, and even developers. We shouldn't go out of our
way to make that hard.

> A file common to all Tegra perhaps belongs in arch/arm/.. somewhere?

That might be a better location, yes.