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

2021-09-20 Thread Wolfgang Denk
Dear Simon,

In message 
<20210919125937.v4.3.If789ba3e2667c46c03eda3386ca84a863baeda55@changeid> you 
wrote:
>
...
> +It is also possible to create an environment file with the name
> +`board//env/.env` for your board. If that file is not present
> +then U-Boot will look for `oard//env/common.env` so that you can

a/oard/board/

> +have a common environment for all vendor boards.

Actually it would be nice to look for `board//env/common.env`
first, and then for `board//env/.env` - and if both
exost, they should be concatenated, so a vendor can keep all common
definitions in the common.env, and have only board specific
definitions in the .env files - otherwise he would have to
repeat everything in the board files, and the common file makes
little sense.

> +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.
> +The conversion script looks for a line that starts with a letter or number
> +and has an equals sign immediately afterwards. Spaces before the = are not
> +permitted. It is a good idea to indent your scripts so that only the 'var='
> +appears at the start of a line.

This is not correct.  Variable names can be more complex:

=> setenv _foo 1
=> setenv ,bar 2
=> setenv /baz 3

etc.

> +For example, for snapper9260 you would create a text file called
> +`board/bluewater/env/snapper9260.env` containing the environment text.
> +
> +Example::
> +
> +stdout=serial
> +#ifdef CONFIG_LCD
> +stdout+=,lcd

Is that a new feature?  I didn't see it documented anywhere?

> +#endif
> +bootcmd=
> +/* U-Boot script for booting */
> +
> +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=
> +/* Print a message when boot fails */
> +echo CONFIG_SYS_BOARD boot failed - please check your image
> +echo Load address is CONFIG_SYS_LOAD_ADDR

You _must_ define a clear syntax for the file, including indentation
rules.  Otherwise there is plenty chance for incorrect arsing.


> + # Is this the start of a new environment variable?
> + if (match($0, "^([^ =][^ =]*)=(.*)", arr)) {

This is not what you document above.

> + if (length(env) != 0) {
> + # Record the value of the variable now completed
> + vars[var] = env
> + }

What in case of length == 0 ?


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
Imitation is the sincerest form of plagarism.


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

2021-09-19 Thread 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//env
directory called .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 
---

Changes in v4:
- Move this from being part of configuring U-Boot to part of building it
- Don't put the environment in autoconf.mk as it is not needed
- Add documentation in rST format instead of README
- Drop mention of import/export
- Update awk script to ignore blank lines, as generated by clang

Changes in v3:
- Adjust Makefile to generate the .inc and .h files in separate fules
- Add more detail in the README about the format of .env files
- Improve the comment about " in the awk script
- Correctly terminate environment files with \n

Changes in v2:
- Move .env file from include/configs to board/
- Use awk script to process environment since it is much easier on the brain
- Add information and updated example script to README
- Add dependency rule so that the environment is rebuilt when it changes

 Makefile  | 34 ++-
 config.mk |  2 ++
 doc/usage/environment.rst | 41 
 env/embedded.c|  1 +
 include/env_default.h |  8 +++
 scripts/env2string.awk| 49 +++
 6 files changed, 134 insertions(+), 1 deletion(-)
 create mode 100644 scripts/env2string.awk

diff --git a/Makefile b/Makefile
index 3014788e14e..9e44af2ccb1 100644
--- a/Makefile
+++ b/Makefile
@@ -513,6 +513,7 @@ version_h := include/generated/version_autogenerated.h
 timestamp_h := include/generated/timestamp_autogenerated.h
 defaultenv_h := include/generated/defaultenv_autogenerated.h
 dt_h := include/generated/dt.h
+env_h := include/generated/environment.h
 
 no-dot-config-targets := clean clobber mrproper distclean \
 help %docs check% coccicheck \
@@ -1802,6 +1803,37 @@ quiet_cmd_sym ?= SYM $@
 u-boot.sym: u-boot FORCE
$(call if_changed,sym)
 
+# We expect '.env' but failing that will use 'common.env'
+ENV_DIR := $(if $(VENDOR),$(VENDOR)/env,$(BOARD)/env)
+ENV_FILE_BOARD := $(srctree)/board/${ENV_DIR}/$(BOARD).env
+ENV_FILE_COMMON := $(srctree)/board/${ENV_DIR}/common.env
+ENV_FILE := $(if $(wildcard 
$(ENV_FILE_BOARD)),$(ENV_FILE_BOARD),$(ENV_FILE_COMMON))
+
+# Run the environment text file through the preprocessor
+quiet_cmd_gen_envp = ENVP$@
+  cmd_gen_envp = \
+   if [ -f "$(ENV_FILE)" ]; then \
+   cat $(ENV_FILE) >$@; \
+   else \
+   echo -n >$@ ; \
+   fi
+
+# Regenerate the environment if it changes
+# We use 'wildcard' since the file is not required to exist (at present), in
+# which case we don't want this dependency, but instead should create an empty
+# file
+include/generated/environment.in: \
+   $(if $(wildcard $(ENV_FILE)),$(wildcard $(ENV_FILE)),FORCE)
+   $(call cmd,gen_envp)
+
+quiet_cmd_gen_envt = ENVT$@
+  cmd_gen_envt = \
+   echo -n "\#define CONFIG_EXTRA_ENV_TEXT " >$@; \
+   awk -f $(srctree)/scripts/env2string.awk $< >>$@
+
+$(env_h): include/generated/environment.in
+   $(call cmd,gen_envt)
+
 # The actual objects are generated when descending,
 # make sure no implicit rule kicks in
 $(sort $(u-boot-init) $(u-boot-main)): $(u-boot-dirs) ;
@@ -1857,7 +1889,7 @@ endif
 # prepare2 creates a makefile if using a separate output directory
 prepare2: prepare3 outputmakefile cfg
 
-prepare1: prepare2 $(version_h) $(timestamp_h) $(dt_h) \
+prepare1: prepare2 $(version_h) $(timestamp_h) $(dt_h) $(env_h) \
include/config/auto.conf
 ifeq ($(wildcard $(LDSCRIPT)),)
@echo >&2 "  Could not find linker script."
diff --git a/config.mk b/config.mk
index 7bb1fd4ed1b..2595aed218b 100644
--- a/config.mk
+++ b/config.mk
@@ -50,8 +50,10 @@ endif
 ifneq ($(BOARD),)
 ifdef  VENDOR
 BOARDDIR = $(VENDOR)/$(BOARD)
+ENVDIR=${vendor}/env
 else
 BOARDDIR = $(BOARD)
+ENVDIR=${board}/env
 endif
 endif
 ifdef  BOARD
diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
index be785a8f717..81130d17f85 100644
--- a/doc/usage/environment.rst
+++ b/doc/usage/environment.rst
@@ -14,6 +14,47 @@ 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