New environment code

lib/errno.c line 1

Quoted 4 lines

+++ b/lib/errno.c

lib/errno.c new file +1 -0

@@ -0,0 +1 @@
+int errno = 0;

drop the "= 0" so that errno ends up in the bss ?

lib/errno.c line 1

Quoted 4 lines · On Saturday, July 17, 2010 15:45:44 Wolfgang Denk wrote:

lib/errno.c new file +1 -0

@@ -0,0 +1 @@
+int errno = 0;

drop the "= 0" so that errno ends up in the bss ?
-mike

Quoted 3 lines

drop the "= 0" so that errno ends up in the bss ?
-mike

Is this going to be a problem during early startup (pre-relocation) vs. normal running? Pre-relocation, we only have a temporary stack and RAM is not set up initially.

I have not looked at when RAM gets initialized vs. when env. variables get used. It is probably OK. Wolfgang?

Needed for hash table support; probably useful in a lot of other places as well.

Signed-off-by Wolfgang Denk <[email protected]>

---
 include/errno.h |    9 +++++++++
 lib/Makefile    |    1 +
 lib/errno.c     |    1 +
 3 files changed, 11 insertions(+), 0 deletions(-)
 create mode 100644 include/errno.h
 create mode 100644 lib/errno.c

Code adapted from uClibc-0.9.30.3

Signed-off-by Wolfgang Denk <[email protected]>

---
 include/common.h |    4 +++
 lib/Makefile     |    1 +
 lib/qsort.c      |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+), 0 deletions(-)
 create mode 100644 lib/qsort.c

This implementation is based on code from uClibc-0.9.30.3 but was modified and extended for use within U-Boot.

Major modifications and extensions:

* hsearch() [modified / extended]:
- While the standard version does not make any assumptions about the type of the stored data objects at all, this implementation works with NUL terminated strings only.
- Instead of storing just pointers to the original objects, we create local copies so the caller does not need to care about the data any more.
- The standard implementation does not provide a way to update an existing entry. This version will create a new entry or update an existing one when both "action == ENTER" and "item.data != NULL".
- hsearch_r(): Instead of returning 1 on success, we return the index into the internal hash table, which is also guaranteed to be positive. This allows us direct access to the found hash table slot for example for functions like hdelete().
* hdelete() [added]:
- The standard implementation of hsearch(3) does not provide any way to delete any entries from the hash table. We extend the code to do that.
* hexport() [added]:
- Export the data stored in the hash table in linearized form:
Entries are exported as "name=value" strings, separated by an arbitrary (non-NUL, of course) separator character. This allows to use this function both when formatting the U-Boot environment for external storage (using '\0' as separator), but also when using it for the "printenv" command to print all variables, simply by using as '\n" as separator. This can also be used for new features like exporting the environment data as text file, including the option for later re-import.
- The entries in the result list will be sorted by ascending key values.
* himport() [added]:
- Import linearized data into hash table. This is the inverse function to hexport(): it takes a linear list of "name=value" pairs and creates hash table entries from it.
- Entries without "value", i. e. consisting of only "name" or "name=", will cause this entry to be deleted from the hash table.
- The "flag" argument can be used to control the behaviour: when the H_NOCLEAR bit is set, then an existing hash table will kept, i. e. new data will be added to an existing hash table; otherwise, old data will be discarded and a new hash table will be created.
- The separator character for the "name=value" pairs can be selected, so we both support importing from externally stored environment data (separated by NUL characters) and from plain text files (entries separated by newline characters).
- To allow for nicely formatted text input, leading white space (sequences of SPACE and TAB chars) is ignored, and entries starting (after removal of any leading white space) with a '#' character are considered comments and ignored.
- NOTE: this means that a variable name cannot start with a '#' character.
- When using a non-NUL separator character, backslash is used as escape character in the value part, allowing for example fo multi-line values.
- In theory, arbitrary separator characters can be used, but only '\0' and '\n' have really been tested.

Signed-off-by Wolfgang Denk <[email protected]>

---
 include/search.h |  106 ++++++++
 lib/Makefile     |    1 +
 lib/hashtable.c  |  721 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 828 insertions(+), 0 deletions(-)
 create mode 100644 include/search.h
 create mode 100644 lib/hashtable.c

This implementation is based on code from uClibc-0.9.30.3 but was modified and extended for use within U-Boot.

unless i'm missing something, the non-reentrant versions operate on a single shared hash table. so while this works today because there is only one

consumer (the env code), wont this cause problems as soon as someone else tries to use the non-reentrant hash table code ? as such, wouldnt it make sense to punt all of the non-reentrant versions and thus force everyone to maintain their own "struct hsearch_data" instance ?

Quoted 3 lines · Mike Frysinger

unless i'm missing something, the non-reentrant versions operate on a single shared hash table. so while this works today because there is only one

Correct.

Quoted 4 lines

consumer (the env code), wont this cause problems as soon as someone else tries to use the non-reentrant hash table code ? as such, wouldnt it make sense to punt all of the non-reentrant versions and thus force everyone to maintain their own "struct hsearch_data" instance ?

This could be done, but I don't see an immediate need.

This (undocumented) concept was only in use for the MVSMR and davinci_schmoogie Sergey Kubushyn <[email protected]> boards. Drop it for now. If really needed, it should be reimplemented later in the context of the new environment command set.

Signed-off-by Wolfgang Denk <[email protected]>

Cc Andre Schwarz <[email protected]>

Cc Sergey Kubushyn <[email protected]>

---

common/cmd_nvedit.c                 |   13 -------------
common/exports.c                    |    3 ---
include/_exports.h                  |    1 -
include/common.h                    |    3 ---
include/configs/MVSMR.h             |    1 -
include/configs/davinci_schmoogie.h |    1 -
include/exports.h                   |    3 ---
7 files changed, 0 insertions(+), 25 deletions(-)

common/cmd_nvedit.c +0 -13

index 13325bc..8c86f15 100644
@@ -247,12 +247,7 @@ int _do_setenv (int flag, int argc, char * const argv[])
 		 * ver is readonly.
 		 */
 		if (
-#ifdef CONFIG_HAS_UID
-		/* Allow serial# forced overwrite with 0xdeaf4add flag */
-		    ((strcmp (name, "serial#") == 0) && (flag != 0xdeaf4add)) ||
-#else
 		    (strcmp (name, "serial#") == 0) ||
-#endif
 		    ((strcmp (name, "ethaddr") == 0)
 #if defined(CONFIG_OVERWRITE_ETHADDR_ONCE) && defined(CONFIG_ETHADDR)
 		     && (strcmp ((char *)env_get_addr(oldval),MK_STR(CONFIG_ETHADDR)) != 0)

@@ -397,14 +392,6 @@ int setenv (char *varname, char *varvalue)
 		return _do_setenv (0, 3, argv);
 }
 
-#ifdef CONFIG_HAS_UID
-void forceenv (char *varname, char *varvalue)
-{
-	char * const argv[4] = { "forceenv", varname, varvalue, NULL };
-	_do_setenv (0xdeaf4add, 3, argv);
-}
-#endif
-
 int do_setenv (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	if (argc < 2) {

common/exports.c +0 -3

index cefe6f6..bde52a6 100644
@@ -35,9 +35,6 @@ unsigned long get_version(void)
 # define spi_release_bus   dummy
 # define spi_xfer          dummy
 #endif
-#ifndef CONFIG_HAS_UID
-# define forceenv          dummy
-#endif
 
 void jumptable_init(void)
 {

include/_exports.h +0 -1

index f3df568..d89b65b 100644
@@ -18,7 +18,6 @@ EXPORT_FUNC(vprintf)
 EXPORT_FUNC(do_reset)
 EXPORT_FUNC(getenv)
 EXPORT_FUNC(setenv)
-EXPORT_FUNC(forceenv)
 EXPORT_FUNC(simple_strtoul)
 EXPORT_FUNC(simple_strtol)
 EXPORT_FUNC(strcmp)

include/common.h +0 -3

index e4b4ec0..5eef9c7 100644
@@ -262,9 +262,6 @@ int	saveenv	     (void);
 int inline setenv   (char *, char *);
 #else
 int	setenv	     (char *, char *);
-#ifdef CONFIG_HAS_UID
-void	forceenv     (char *, char *);
-#endif
 #endif /* CONFIG_PPC */
 #ifdef CONFIG_ARM
 # include <asm/mach-types.h>

include/configs/MVSMR.h +0 -1

index 6492068..000c4c6 100644
@@ -185,7 +185,6 @@
 */
 #define CONFIG_ENV_IS_IN_FLASH
 #undef	CONFIG_SYS_FLASH_PROTECTION
-#define CONFIG_HAS_UID
 #define	CONFIG_OVERWRITE_ETHADDR_ONCE
 
 #define CONFIG_ENV_OFFSET	0x8000

include/configs/davinci_schmoogie.h +0 -1

index 875dda4..04cdc21 100644
@@ -99,7 +99,6 @@
 /*=====================*/
 #define CONFIG_RTC_DS1307		/* RTC chip on SCHMOOGIE */
 #define CONFIG_SYS_I2C_RTC_ADDR	0x6f	/* RTC chip I2C address */
-#define CONFIG_HAS_UID
 #define CONFIG_UID_DS28CM00		/* Unique ID on SCHMOOGIE */
 #define CONFIG_SYS_UID_ADDR		0x50	/* UID chip I2C address */
 /*==============================*/

include/exports.h +0 -3

index 1d79a31..7404a7c 100644
@@ -26,9 +26,6 @@ int setenv (char *varname, char *varvalue);
 long simple_strtol(const char *cp,char **endp,unsigned int base);
 int strcmp(const char * cs,const char * ct);
 int ustrtoul(const char *cp, char **endp, unsigned int base);
-#ifdef CONFIG_HAS_UID
-void forceenv (char *varname, char *varvalue);
-#endif
 #if defined(CONFIG_CMD_I2C)
 int i2c_write (uchar, uint, int , uchar* , int);
 int i2c_read (uchar, uint, int , uchar* , int);

Signature · 1 line

1.7.1.1

This (undocumented) concept was only in use for the MVSMR and davinci_schmoogie Sergey Kubushyn <[email protected]> boards. Drop it for now. If really needed, it should be reimplemented later in the context of the new environment command set.

Signed-off-by Wolfgang Denk <[email protected]>

Cc Andre Schwarz <[email protected]>

Cc Sergey Kubushyn <[email protected]>

---
 common/cmd_nvedit.c                 |   13 -------------
 common/exports.c                    |    3 ---
 include/_exports.h                  |    1 -
 include/common.h                    |    3 ---
 include/configs/MVSMR.h             |    1 -
 include/configs/davinci_schmoogie.h |    1 -
 include/exports.h                   |    3 ---
 7 files changed, 0 insertions(+), 25 deletions(-)

- It would be nice if we could add wildcard support for environment variables; this is needed for variable name auto-completion, but it would also be nice to be able to say "printenv ip*" or "printenv *addr*"

int var_complete(int argc, char * const argv[], char last_char, int maxv, char *cmdv[]) {
+#if 0 /* need to reimplement */

Quoted 5 lines

- It would be nice if we could add wildcard support for environment variables; this is needed for variable name auto-completion, but it would also be nice to be able to say "printenv ip*" or "printenv *addr*"

you were right - a grepenv/findenv/'env search' substring implementation on top of this looks to be at least as expensive as a full export operation :/

Quoted 3 lines

int var_complete(int argc, char * const argv[], char last_char, int maxv, char *cmdv[]) {
+#if 0 /* need to reimplement */

ouch - this is u-boot's most useful feature :)

It would be good to know boot time overhead the initial import function makes, esp. in terms of number of boot-time accesses to the environment...

Dear Kim Phillips,

Quoted 6 lines · In message <[email protected]> you wrote:

Quoted 5 lines

- It would be nice if we could add wildcard support for environment variables; this is needed for variable name auto-completion, but it would also be nice to be able to say "printenv ip*" or "printenv *addr*"

Quoted 3 lines

you were right - a grepenv/findenv/'env search' substring implementation on top of this looks to be at least as expensive as a full export operation :/

Expensive in terms of what? Code size? Probably not. It all boils down to running strstr() over all entries...

As you can see, there are stages where the new code is a bit slower (20...40 milliseconds, which is close to the resolution of the measurement), but in this specific test we win some 280 milliseconds; if you exclude the 5 seconds boot delay that's some 2.7%. I don't think it's worth mentioning, but at least it's not worse than the old code.

I haven't tested many boards, especially not for timing. If you have additional input it will be welcome.

I would, however u-boot-testing.git/hashtable currently fails to boot on an 8572:

U-Boot 2010.06-02552-g9895ea5 (Jul 19 2010 - 18:39:19)

CPU0:  8572E, Version: 1.1, (0x80e80011)
Core:  E500, Version: 3.0, (0x80210030)
Clock Configuration:
       CPU0:1499.985 MHz, CPU1:1499.985 MHz, 
       CCB:599.994 MHz,
       DDR:399.996 MHz (799.992 MT/s data rate) (Asynchronous), LBC:37.500 MHz
L1:    D-cache 32 kB enabled
       I-cache 32 kB enabled
Board: MPC8572DS Sys ID: 0x14, Sys Ver: 0x30, FPGA Ver: 0x10, vBank: 0
I2C:   ready
DRAM:  Initializing....

I presume it's related to the issue brought up in this thread?:

http://lists.denx.de/pipermail/u-boot/2010-July/074151.html

What we see here is in the pre-relocation phase, where we are still parsing the original, in-flash copy of the stringified environment. What is strange is that this seems to have worked when accessing "baudrate" (either because the environment was found, or because the default environment was used), but is stopping here.

Quoted 4 lines

I think the problem is that before relocation we have to use getenv_r(), which is done for example to read the baudrate, see arch/powerpc/lib/board.c: init_baudrate()

How hard is it to detect this case and generate a stackdump if someone calls getenv() before relocation is complete? That would help to spot places where people are breaking the rules...

However, I cannot see any gentenv() use before relocation for your board, so the problem there is still unclear. I think I need your help to debug this...

Quoted 4 lines

However, I cannot see any gentenv() use before relocation for your board, so the problem there is still unclear. I think I need your help to debug this...

looks like arch/powerpc/cpu/mpc8xxx/ddr/options.c gets memory interleaving configuration from the environment, using getenv().

Dear Kim,

Quoted 5 lines · In message <[email protected]> you wrote:

Quoted 4 lines

However, I cannot see any gentenv() use before relocation for your board, so the problem there is still unclear. I think I need your help to debug this...

Quoted 2 lines

looks like arch/powerpc/cpu/mpc8xxx/ddr/options.c gets memory interleaving configuration from the environment, using getenv().

Ah! Can you please try the following patch - it's not intended to be a fix, just to verify my hypothesis. Thanks in advance.

Quoted 3 lines

Ah! Can you please try the following patch - it's not intended to be a fix, just to verify my hypothesis. Thanks in advance.

indeed, your patch plus the following makes the 8572 boot properly again.

Could you please also test the code from the current (rebased) version in the "hashtable" branch of the git://git.denx.de/u-boot-testing.git
repo?

I have adresses the problem of early environment accesses, and at least on the boards I've tested so far it appears to be working.

I would, however u-boot-testing.git/hashtable currently fails to boot on an 8572:

I presume it's related to the issue brought up in this thread?:

http://lists.denx.de/pipermail/u-boot/2010-July/074151.html

common/cmd_nvedit.c +0 -0

index 8c86f15..976a30b 100644

+/*
+ * env export [-t | -b | -c] addr [size]
+ * -t: export as text format; if size is given, data will be
+ * padded with '\0' bytes; if not, one terminating '\0'
+ * will be added (which is included in the "filesize"
+ * setting so you can for exmple copy this to flash and
+ * keep the termination).
+ * -b: export as binary format (name=value pairs separated by
+ * '\0', list end marked by double "\0\0")
+ * -c: export as checksum protected environment format as
+ * used for example by "saveenv" command
+ * addr: memory address where environment gets stored
+ * size: size of output buffer
+ *

common/cmd_nvedit.c +43 -1

+		char *arg = *argv;
+		while (*++arg) {
+			switch (*arg) {
+			case 'b':		/* raw binary format */
+				if (fmt++)
+					goto sep_err;
+				sep = '\0';
+				break;
+			case 'c':		/* external checksum format */
+				if (fmt++)
+					goto sep_err;
+				sep = '\0';
+				chk = 1;
+				break;
+			case 't':		/* text format */
+				if (fmt++)
+					goto sep_err;
+				sep = '\n';
+				break;
+			default:
+				cmd_usage(cmdtp);
+				return 1;
+			}
+		}
+	}
 
-/**************************************************/
+	if (argc < 1) {
+		cmd_usage(cmdtp);
+		return 1;
+	}
+
+	addr = (char *)simple_strtoul(argv[0], NULL, 16);
+
+	if (argc == 2) {
+		size = simple_strtoul(argv[1], NULL, 16);
+		memset(addr, '\0', size);
+	} else {
+		size = 0;
+	}
+
+	if (sep) {		/* export as text file */
+		len = hexport(sep, &addr, size);
+		if (len < 0) {
+			error("Cannot export environment: errno = %d\n",

common/cmd_nvedit.c +16 -0

+	}
+
+	len = hexport('\0', &res, ENV_SIZE);
+	if (len < 0) {
+		error("Cannot export environment: errno = %d\n",
+			errno);
+		return 1;
+	}
+
+	if (chk) {
+		envp->crc   = crc32(0, envp->data, ENV_SIZE);
+#ifdef CONFIG_ENV_ADDR_REDUND
+		envp->flags = ACTIVE_FLAG;
+#endif
+	}
+	sprintf(buf, "%zX", len + offsetof(env_t,data));

I could think of some situations where the new env command is helpful. But more during development than for production systems.

Switching between environment profiles would be cool. And a "env default -f" behavior that keeps MAC addresses and serial# is also on my wishlist.

I did some testing on our PMC440 with environment in EEPROM. Please see some comments below.

common/cmd_nvedit.c int envmatch (uchar *s1, int i2) · line 597

Quoted 7 lines

common/cmd_nvedit.c +7 -0

+static int do_env_export(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	char	buf[32];
+	char	*addr, *cmd, *res;
+	size_t	size;
+	ssize_t	len;
+	env_t	*envp = (env_t *)addr;

addr is uninitialized. declaration is enough here.

common/cmd_nvedit.c int envmatch (uchar *s1, int i2) · line 658

Quoted 1 line

common/cmd_nvedit.c +1 -0

+	if (chk) {		/* export as checksum protected block */

Add:

		envp = (env_t *)addr;

common/cmd_nvedit.c int envmatch (uchar *s1, int i2) · line 661

Quoted 3 lines

common/cmd_nvedit.c +3 -0

+		res = (char *)&envp->data;
+	} else {		/* export as raw binary data */
+		res = (char *)&addr;

Should'n this be

		res = addr;

Fixes for non-building board (AR405, CANBT, PMC440) will come up shortly.

Dear Matthias Fuchs,

Quoted 3 lines · In message <[email protected]> you wrote:

I could think of some situations where the new env command is helpful. But more during development than for production systems.

It depends. "Reset to factory defaults" is a not so uncommon request. And acceleration of scripts is not so uncommon either.

Quoted 2 lines

Switching between environment profiles would be cool. And a "env default -f" behavior that keeps MAC addresses and serial# is also on my wishlist.

Actually neither MAC addresses nor serial# are part of the default environment.

Quoted 3 lines

I did some testing on our PMC440 with environment in EEPROM. Please see some comments below.

Thanks.

Quoted 8 lines

common/cmd_nvedit.c int envmatch (uchar *s1, int i2) · line 597

Quoted 7 lines

common/cmd_nvedit.c +7 -0

+static int do_env_export(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	char	buf[32];
+	char	*addr, *cmd, *res;
+	size_t	size;
+	ssize_t	len;
+	env_t	*envp = (env_t *)addr;

addr is uninitialized. declaration is enough here.

Will check this.

...

Quoted 3 lines

common/cmd_nvedit.c int envmatch (uchar *s1, int i2) · line 658

Quoted 1 line

common/cmd_nvedit.c +1 -0

+	if (chk) {		/* export as checksum protected block */

Add:

		envp = (env_t *)addr;

Quoted 5 lines

common/cmd_nvedit.c int envmatch (uchar *s1, int i2) · line 661

Quoted 3 lines

common/cmd_nvedit.c +3 -0

+		res = (char *)&envp->data;
+	} else {		/* export as raw binary data */
+		res = (char *)&addr;

Should'n this be

		res = addr;

No. We need the address of the pointer variable, so the function can store the result pointer there.

Quoted 3 lines

Actually neither MAC addresses nor serial# are part of the default environment.

Right. But, I'd like to have that described feature in any case. Let's call it different, but the functionality would be helpful. What about a further option like '-p' for keep _p_rotected.

Quoted 2 lines

No. We need the address of the pointer variable, so the function can store the result pointer there.

Nak. You added &-operator when calling: hexport('\0', &res, ENV_SIZE); That's one to much. You can also remove the &-operator from this:

		res = (char *)&envp->data;

So the code will look like this:

Motivation:

* Old environment code used a pessimizing implementation:
- variable lookup used linear search => slow
- changed/added variables were added at the end, i. e. most frequently used variables had the slowest access times => slow
- each setenv() would calculate the CRC32 checksum over the whole environment block => slow
* "redundant" envrionment was locked down to two copies
* No easy way to implement features like "reset to factory defaults", or to select one out of several pre-defined (previously saved) sets of environment settings ("profiles")
* No easy way to import or export environment settings

======================================================================

API Changes:

- Variable names starting with '#' are no longer allowed

I didn't find any such variable names being used; it is highly recommended to follow standard conventions and start variable names with an alphanumeric character

- "printenv" will now print a backslash at the end of all but the last lines of a multi-line variable value.

Multi-line variables have never been formally defined, allthough there is no reason not to use them. Now we define rules how to deal with them, allowing for import and export.

- Function forceenv() and the related code in saveenv() was removed. At the moment this is causing build problems for the only user of this code (schmoogie - which has no entry in MAINTAINERS); may be fixed later by implementing the "env set -f" feature.

Inconsistencies:

- "printenv" will '\\'-escape the '\n' in multi-line variables, while "printenv var" will not do that.

======================================================================

Advantages:

- "printenv" output much better readable (sorted)
- faster!
- extendable (additional variable properties can be added)
- new, powerful features like "factory reset" or easy switching between several different environment settings ("profiles")

Disadvantages:

- Image size grows by typically 5...7 KiB (might shrink a bit again on systems with redundant environment with a following patch series)

======================================================================

Implemented:

- env command with subcommands:

- env print [arg ...]

same as "printenv": print environment

- env set [-f] name [arg ...]

same as "setenv": set (and delete) environment variables

["-f" - force setting even for read-only variables - not implemented yet.]

- end delete [-f] name

not implemented yet

["-f" - force delete even for read-only variables]

- env save

same as "saveenv": save environment

- env export [-t | -b | -c] addr [size]

export internal representation (hash table) in formats usable for persistent storage or processing:

-t: export as text format; if size is given, data will be
padded with '\0' bytes; if not, one terminating '\0' will be added (which is included in the "filesize" setting so you can for exmple copy this to flash and keep the termination).
-b: export as binary format (name=value pairs separated by
'\0', list end marked by double "\0\0")
-c: export as checksum protected environment format as
used for example by "saveenv" command
addr: memory address where environment gets stored
size: size of output buffer

With "-c" and size is NOT given, then the export command will format the data as currently used for the persistent storage, i. e. it will use CONFIG_ENV_SECT_SIZE as output block size and prepend a valid CRC32 checksum and, in case of resundant environment, a "current" redundancy flag. If size is given, this value will be used instead of CONFIG_ENV_SECT_SIZE; again, CRC32 checksum and redundancy flag will be inserted.

With "-b" and "-t", always only the real data (including a terminating '\0' byte) will be written; here the optional size argument will be used to make sure not to overflow the user provided buffer; the command will abort if the size is not sufficient. Any remainign space will be '\0' padded.

On successful return, the variable "filesize" will be set. Note that filesize includes the trailing/terminating '\0' byte(s).

Usage szenario: create a text snapshot/backup of the current settings:

=> env export -t 100000
=> era ${backup_addr} +${filesize} => cp.b 100000 ${backup_addr} ${filesize}

Re-import this snapshot, deleting all other settings:

=> env import -d -t ${backup_addr}

- env import [-d] [-t | -b | -c] addr [size]

import external format (text or binary) into hash table, optionally deleting existing values:

-d: delete existing environment before importing;
otherwise overwrite / append to existion definitions
-t: assume text format; either "size" must be given or the
text data must be '\0' terminated
-b: assume binary format ('\0' separated, "\0\0" terminated)
-c: assume checksum protected environment format
addr: memory address to read from
size: length of input data; if missing, proper '\0'
termination is mandatory

- env default -f

reset default environment: drop all environment settings and load default environment

- env ask name [message] [size]

same as "askenv": ask for environment variable

- env edit name

same as "editenv": edit environment variable

- env run

same as "run": run commands in an environment variable

======================================================================

TODO:

- drop default env as implemented now; provide a text file based initialization instead (eventually using several text files to incrementally build it from common blocks) and a tool to convert it into a binary blob / object file.

- It would be nice if we could add wildcard support for environment variables; this is needed for variable name auto-completion, but it would also be nice to be able to say "printenv ip*" or "printenv *addr*"

- Some boards don't link any more due to the grown code size:
AR405, CANBT, DU405, PMC440, canyonlands, sc3, sequoia, socrates.

=> cc: Matthias Fuchs <[email protected]>,
Stefan Roese <[email protected]>,
Heiko Schocher <[email protected]>

- Dropping forceenv() causes build problems on schmoogie

=> cc: Sergey Kubushyn <[email protected]>

- Build tested on PPC and ARM only; runtime tested with NOR and NAND flash only => needs testing!!

Signed-off-by Wolfgang Denk <[email protected]>

Cc Matthias Fuchs <[email protected]>,

Cc Stefan Roese <[email protected]>,

Cc Heiko Schocher <[email protected]>

Cc Sergey Kubushyn <[email protected]>

---
 README                 |    8 +
 board/zeus/zeus.c      |   25 +--
 common/cmd_nvedit.c    |  655 +++++++++++++++++++++++++++++++++---------------
 common/command.c       |    3 +-
 common/dlmalloc.c      |   25 +-
 common/env_common.c    |  125 +++++-----
 common/env_dataflash.c |  114 ++++++----
 common/env_eeprom.c    |   85 ++++---
 common/env_flash.c     |  245 ++++++++++---------
 common/env_mgdisk.c    |   33 +--
 common/env_nand.c      |  188 ++++++++------

 common/env_nowhere.c   |   11 +-
 common/env_nvram.c     |   46 +++-
 common/env_onenand.c   |   65 +++---
 common/env_sf.c        |  132 ++++++-----
 common/exports.c       |    1 -
 include/environment.h  |    5 +-
 lib/hashtable.c        |   32 ++-
 18 files changed, 1094 insertions(+), 704 deletions(-)

common/env_flash.c +49 -55

 #ifdef CMD_SAVEENV
 int saveenv(void)
 {
-	char *saved_data = NULL;
-	int rc = 1;
-	char flag = OBSOLETE_FLAG, new_flag = ACTIVE_FLAG;
+	env_t	env_new;
+	ssize_t	len;
+	char	*saved_data = NULL;
+	char	*res;
+	int	rc = 1;
+	char	flag = OBSOLETE_FLAG, new_flag = ACTIVE_FLAG;
 #if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
-	ulong up_data = 0;
+	ulong	up_data = 0;
 #endif
 
-	debug ("Protect off %08lX ... %08lX\n",
+	debug("Protect off %08lX ... %08lX\n",
 		(ulong)flash_addr, end_addr);
 
-	if (flash_sect_protect (0, (ulong)flash_addr, end_addr)) {
-		goto Done;
+	if (flash_sect_protect(0, (ulong)flash_addr, end_addr)) {
+		goto done;
 	}
 
-	debug ("Protect off %08lX ... %08lX\n",
+	debug("Protect off %08lX ... %08lX\n",
 		(ulong)flash_addr_new, end_addr_new);
 
-	if (flash_sect_protect (0, (ulong)flash_addr_new, end_addr_new)) {
-		goto Done;
+	if (flash_sect_protect(0, (ulong)flash_addr_new, end_addr_new)) {
+		goto done;
+	}
+
+	res = (char *)&env_new.data;
+	len = hexport('\0', &res, ENV_SIZE);
+	if (len < 0) {
+		error("Cannot export environment: errno = %d\n", errno);
+		goto done;
 	}
+	env_new.crc   = crc32(0, env_new.data, ENV_SIZE);
+	env_new.flags = new_flag;
 
 #if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
 	up_data = (end_addr_new + 1 - ((long)flash_addr_new + CONFIG_ENV_SIZE));
-	debug ("Data to save 0x%x\n", up_data);
+	debug("Data to save 0x%lX\n", up_data);
 	if (up_data) {
 		if ((saved_data = malloc(up_data)) == NULL) {
 			printf("Unable to save the rest of sector (%ld)\n",
 				up_data);
-			goto Done;
+			goto done;
 		}
 		memcpy(saved_data,
 			(void *)((long)flash_addr_new + CONFIG_ENV_SIZE), up_data);
-		debug ("Data (start 0x%x, len 0x%x) saved at 0x%x\n",
-			   (long)flash_addr_new + CONFIG_ENV_SIZE,
-				up_data, saved_data);
+		debug("Data (start 0x%lX, len 0x%lX) saved at 0x%p\n",
+			(long)flash_addr_new + CONFIG_ENV_SIZE,
+			up_data, saved_data);
 	}
 #endif
-	puts ("Erasing Flash...");
-	debug (" %08lX ... %08lX ...",
+	puts("Erasing Flash...");
+	debug(" %08lX ... %08lX ...",
 		(ulong)flash_addr_new, end_addr_new);
 
-	if (flash_sect_erase ((ulong)flash_addr_new, end_addr_new)) {
-		goto Done;
+	if (flash_sect_erase((ulong)flash_addr_new, end_addr_new)) {
+		goto done;
 	}
 
-	puts ("Writing to Flash... ");
-	debug (" %08lX ... %08lX ...",
+	puts("Writing to Flash... ");
+	debug(" %08lX ... %08lX ...",
 		(ulong)&(flash_addr_new->data),
 		sizeof(env_ptr->data)+(ulong)&(flash_addr_new->data));
-	if ((rc = flash_write((char *)env_ptr->data,
-			(ulong)&(flash_addr_new->data),
-			sizeof(env_ptr->data))) ||
-	    (rc = flash_write((char *)&(env_ptr->crc),
-			(ulong)&(flash_addr_new->crc),
-			sizeof(env_ptr->crc))) ||
+	if ((rc = flash_write((char *)&env_new,
+			(ulong)flash_addr_new,
+			sizeof(env_new))) ||
 	    (rc = flash_write(&flag,
 			(ulong)&(flash_addr->flags),
-			sizeof(flash_addr->flags))) ||
-	    (rc = flash_write(&new_flag,
-			(ulong)&(flash_addr_new->flags),
-			sizeof(flash_addr_new->flags))))
-	{
-		flash_perror (rc);
-		goto Done;
+			sizeof(flash_addr->flags))) ) {
+		flash_perror(rc);
+		goto done;
 	}
-	puts ("done\n");
 
 #if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
 	if (up_data) { /* restore the rest of sector */
-		debug ("Restoring the rest of data to 0x%x len 0x%x\n",
-			   (long)flash_addr_new + CONFIG_ENV_SIZE, up_data);
+		debug("Restoring the rest of data to 0x%lX len 0x%lX\n",
+			(long)flash_addr_new + CONFIG_ENV_SIZE, up_data);
 		if (flash_write(saved_data,
 				(long)flash_addr_new + CONFIG_ENV_SIZE,
 				up_data)) {
 			flash_perror(rc);
-			goto Done;
+			goto done;
 		}
 	}
 #endif
+	puts("done\n");
+
 	{
 		env_t * etmp = flash_addr;
 		ulong ltmp = end_addr;

@@ -220,13 +230,12 @@ int saveenv(void)
 	}
 
 	rc = 0;
-Done:
-
+done:
 	if (saved_data)
-		free (saved_data);
+		free(saved_data);
 	/* try to re-protect */
-	(void) flash_sect_protect (1, (ulong)flash_addr, end_addr);
-	(void) flash_sect_protect (1, (ulong)flash_addr_new, end_addr_new);
+	(void) flash_sect_protect(1, (ulong)flash_addr, end_addr);
+	(void) flash_sect_protect(1, (ulong)flash_addr_new, end_addr_new);
 
 	return rc;
 }

@@ -244,83 +253,93 @@ int  env_init(void)
 
 	gd->env_addr  = (ulong)&default_environment[0];
 	gd->env_valid = 0;
-	return (0);
+	return 0;
 }
 
 #ifdef CMD_SAVEENV
 
 int saveenv(void)
 {
-	int	len, rc;
-	ulong	end_addr;
-	ulong	flash_sect_addr;
-#if defined(CONFIG_ENV_SECT_SIZE) && (CONFIG_ENV_SECT_SIZE >
 CONFIG_ENV_SIZE) -	ulong	flash_offset;
-	uchar	env_buffer[CONFIG_ENV_SECT_SIZE];
-#else
-	uchar *env_buffer = (uchar *)env_ptr;
-#endif	/* CONFIG_ENV_SECT_SIZE */
-	int rcode = 0;
-
-#if defined(CONFIG_ENV_SECT_SIZE) && (CONFIG_ENV_SECT_SIZE >

CONFIG_ENV_SIZE) -

common/env_flash.c +53 -33

-	flash_offset    = ((ulong)flash_addr) & (CONFIG_ENV_SECT_SIZE-1);
-	flash_sect_addr = ((ulong)flash_addr) & ~(CONFIG_ENV_SECT_SIZE-1);
-
-	debug ( "copy old content: "
-		"sect_addr: %08lX  env_addr: %08lX  offset: %08lX\n",
-		flash_sect_addr, (ulong)flash_addr, flash_offset);
-
-	/* copy old contents to temporary buffer */
-	memcpy (env_buffer, (void *)flash_sect_addr, CONFIG_ENV_SECT_SIZE);
-
-	/* copy current environment to temporary buffer */
-	memcpy ((uchar *)((unsigned long)env_buffer + flash_offset),
-		env_ptr,
-		CONFIG_ENV_SIZE);
+	env_t	env_new;
+	ssize_t	len;
+	int	rc = 1;
+	char	*res;
+	char	*saved_data = NULL;
+#if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
+	ulong	up_data = 0;
 
-	len	 = CONFIG_ENV_SECT_SIZE;
-#else
-	flash_sect_addr = (ulong)flash_addr;
-	len	 = CONFIG_ENV_SIZE;
+	up_data = (end_addr + 1 - ((long)flash_addr + CONFIG_ENV_SIZE));
+	debug("Data to save 0x%lx\n", up_data);
+	if (up_data) {
+		if ((saved_data = malloc(up_data)) == NULL) {
+			printf("Unable to save the rest of sector (%ld)\n",
+				up_data);
+			goto done;
+		}
+		memcpy(saved_data,
+			(void *)((long)flash_addr + CONFIG_ENV_SIZE), up_data);
+		debug("Data (start 0x%lx, len 0x%lx) saved at 0x%lx\n",
+			(ulong)flash_addr + CONFIG_ENV_SIZE,
+			up_data,
+			(ulong)saved_data);
+	}
 #endif	/* CONFIG_ENV_SECT_SIZE */
 
-	end_addr = flash_sect_addr + len - 1;
+	debug("Protect off %08lX ... %08lX\n",
+		(ulong)flash_addr, end_addr);
 
-	debug ("Protect off %08lX ... %08lX\n",
-		(ulong)flash_sect_addr, end_addr);
+	if (flash_sect_protect(0, (long)flash_addr, end_addr))
+		goto done;
 
-	if (flash_sect_protect (0, flash_sect_addr, end_addr))
-		return 1;
+	res = (char *)&env_new.data;
+	len = hexport('\0', &res, ENV_SIZE);
+	if (len < 0) {
+		error("Cannot export environment: errno = %d\n", errno);
+		goto done;
+	}
+	env_new.crc = crc32(0, env_new.data, ENV_SIZE);
 
-	puts ("Erasing Flash...");
-	if (flash_sect_erase (flash_sect_addr, end_addr))
-		return 1;
+	puts("Erasing Flash...");
+	if (flash_sect_erase((long)flash_addr, end_addr))
+		goto done;
 
-	puts ("Writing to Flash... ");
-	rc = flash_write((char *)env_buffer, flash_sect_addr, len);
+	puts("Writing to Flash... ");
+	rc = flash_write((char *)&env_new, (long)flash_addr, CONFIG_ENV_SIZE);
 	if (rc != 0) {
-		flash_perror (rc);
-		rcode = 1;
-	} else {
-		puts ("done\n");
+		flash_perror(rc);
+		goto done;
 	}
-
+#if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
+	if (up_data) {	/* restore the rest of sector */
+		debug("Restoring the rest of data to 0x%lx len 0x%lx\n",
+			(ulong)flash_addr + CONFIG_ENV_SIZE, up_data);
+		if (flash_write(saved_data,
+				(long)flash_addr + CONFIG_ENV_SIZE,
+				up_data)) {
+			flash_perror(rc);
+			goto done;
+		}
+	}
+#endif
+	puts("done\n");

Quoted 51 lines · On Saturday, July 17, 2010 15:45:48 Wolfgang Denk wrote:

common/env_flash.c +0 -4

 int saveenv(void)
 {
 
-	int	len, rc;
-	ulong	end_addr;
-	ulong	flash_sect_addr;
-#if defined(CONFIG_ENV_SECT_SIZE) && (CONFIG_ENV_SECT_SIZE >

CONFIG_ENV_SIZE) - ulong flash_offset;

common/env_flash.c +0 -7

-	uchar	env_buffer[CONFIG_ENV_SECT_SIZE];
-#else
-	uchar *env_buffer = (uchar *)env_ptr;
-#endif	/* CONFIG_ENV_SECT_SIZE */
-	int rcode = 0;
-
-#if defined(CONFIG_ENV_SECT_SIZE) && (CONFIG_ENV_SECT_SIZE >

CONFIG_ENV_SIZE) -

common/env_flash.c +14 -18

-	flash_offset    = ((ulong)flash_addr) & (CONFIG_ENV_SECT_SIZE-1);
-	flash_sect_addr = ((ulong)flash_addr) & ~(CONFIG_ENV_SECT_SIZE-1);
-
-	debug ( "copy old content: "
-		"sect_addr: %08lX  env_addr: %08lX  offset: %08lX\n",
-		flash_sect_addr, (ulong)flash_addr, flash_offset);
-
-	/* copy old contents to temporary buffer */
-	memcpy (env_buffer, (void *)flash_sect_addr, CONFIG_ENV_SECT_SIZE);
-
-	/* copy current environment to temporary buffer */
-	memcpy ((uchar *)((unsigned long)env_buffer + flash_offset),
-		env_ptr,
-		CONFIG_ENV_SIZE);
+	env_t	env_new;
+	ssize_t	len;
+	int	rc = 1;
+	char	*res;
+	char	*saved_data = NULL;
+#if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
+	ulong	up_data = 0;
 
-	len	 = CONFIG_ENV_SECT_SIZE;
-#else
-	flash_sect_addr = (ulong)flash_addr;
-	len	 = CONFIG_ENV_SIZE;
+	up_data = (end_addr + 1 - ((long)flash_addr + CONFIG_ENV_SIZE));
+	debug("Data to save 0x%lx\n", up_data);
+	if (up_data) {
+		if ((saved_data = malloc(up_data)) == NULL) {
+			printf("Unable to save the rest of sector (%ld)\n",
+				up_data);
+			goto done;

common/env_flash.c int env_init(void)

Quoted 10 lines

common/env_flash.c +8 -0

+		}
+		memcpy(saved_data,
+			(void *)((long)flash_addr + CONFIG_ENV_SIZE), up_data);
+		debug("Data (start 0x%lx, len 0x%lx) saved at 0x%lx\n",
+			(ulong)flash_addr + CONFIG_ENV_SIZE,
+			up_data,
+			(ulong)saved_data);
+	}
 
 #endif	/* CONFIG_ENV_SECT_SIZE */

it would seem that somewhere nestled in this rewrite, support for embedded env was broken (CONFIG_ENV_SIZE < CONFIG_ENV_SECT_SIZE). on a bf548-ezkit for example, i see this behavior:

(v2010.09 / before this commit):
bfin> save
Saving Environment to Flash...
. done
Un-Protected 1 sectors
Erasing Flash...
. done
Erased 1 sectors
Writing to Flash... done
. done
Protected 1 sectors

(after this commit / v2010.12):
bfin> save
Saving Environment to Flash...
Error: start address not on sector boundary
Error: start address not on sector boundary

not sure if anyone has noticed/fixed this yet and would save me from the trouble of finding/fixing the problem ...

seems the new saveenv code in the non-redund case has been rewritten completely and doesn't support the same feature set as it used to.

so the difference is that the new code no longer supports envs which do not start on a sector boundary. it does this so that it doesn't memcpy() as much data out of the flash at the expense of doing more CPU bound math operations (mostly bit twiddling).

so the question is whether the old behavior should be restored. if it not,

the documentation should be tweaked to note these requirements and some simple CPP checks added to environment.h so that this issue turns into a build failure. after all, it's easy to detect an env offset that isnt at the start of the sector:

	#if (CONFIG_ENV_OFFSET & (CONFIG_ENV_SECT_SIZE - 1))
	# error env offset not sector aligned
	#endif

The following patch series adds some utilities (qsort and hash table based database functions) and uses these to reimplement the code used for the internal handling of environment variables.

The following patch series adds some utilities (qsort and hash table based database functions) and uses these to reimplement the code used for the internal handling of environment variables.

Motivation:

* Old environment code used a pessimizing implementation:
- variable lookup used linear search => slow
- changed/added variables were added at the end, i. e. most frequently used variables had the slowest access times => slow
- each setenv() would calculate the CRC32 checksum over the whole environment block => slow

Disadvantages:

- Image size grows by typically 5...7 KiB (might shrink a bit again on systems with redundant environment with a following patch series)

Quoted 5 lines · Wolfgang Denk

Disadvantages:

- Image size grows by typically 5...7 KiB (might shrink a bit again on

systems with redundant environment with a following patch series)

Looking at the patches I can't really see where that large increase in image size comes from;
but if its really true, it seems to be a very high price to pay for those who get along with the environment as it is.

i wonder if we could #ifdef the sorting/hashing. i tend to agree with

Reinhard that it is very common for boards to be deployed with a small env, so i wonder if the normal runtime experience is actually faster, or if the difference is about the same as system noise: boot u-boot, load env with ~20 entries, read a few env vars, boot OS.

Quoted 4 lines · Mike Frysinger

Reinhard that it is very common for boards to be deployed with a small env, so i wonder if the normal runtime experience is actually faster, or if the difference is about the same as system noise: boot u-boot, load env with ~20 entries, read a few env vars, boot OS.

Yes, this is one common use case.

But then, I see also systems that are running pretty complex shell scripts to implement features like reliable software updates etc., and there we see an improvment.

Also, speed is just one aspect - for me the capability of having several "profiles" is much, much more important.

how many users are out there that have such huge environments that speed is of any concern?

The "grepenv" idea however seems to indicate some might have huge environments...

I see the advantages offered, but

Quoted 4 lines

Disadvantages:

- Image size grows by typically 5...7 KiB (might shrink a bit again on systems with redundant environment with a following patch series)

Looking at the patches I can't really see where that large increase in image size comes from;

but if its really true, it seems to be a very high price to pay for those who get along with the
environment as it is.

I would at least recommend that all new UI commands are optional; as basics "setenv", "printenv", "saveenv" should do as until now. "export" and "import" must be adding
a lot to the code.

The following patch series adds some utilities (qsort and hash table based database functions) and uses these to reimplement the code used for the internal handling of environment variables.

Motivation:

* Old environment code used a pessimizing implementation:
- variable lookup used linear search => slow
- changed/added variables were added at the end, i. e. most frequently used variables had the slowest access times => slow
- each setenv() would calculate the CRC32 checksum over the whole environment block => slow

Just to give a datapoint for speed:

measured on TQM5200 (MPC5200 at 400 MHz, environment size 16 kB):

OLD implementation:

	=> sete ttt 'date;for i in 0 1 2 3 4 5 6 7 8 9 ; do for j in 0 1 2 3 4 5 6 7 8 9 ; do for k in 0 1 2 3 4 5 6 7 8 9 ; do run tt ; done ; done ; done;date'
	=> sete tt 'sete var1 xxx;sete var2 ${var1};sete var1 ${var2};sete var1;sete var2'
	=> run tt
	=> run ttt
	Date: 1910-07-17 (unknown day)    Time: 22:02:31
	Date: 1910-07-17 (unknown day)    Time: 22:03:27

New implementation:

	=> run ttt
	Date: 1910-07-17 (unknown day)    Time: 20:45:51
	Date: 1910-07-17 (unknown day)    Time: 20:45:53

But, how does the timing look in the current implementation when the CRC calculation
is done only in the saveenv command?

The following patch series adds some utilities (qsort and hash table based database functions) and uses these to reimplement the code used for the internal handling of environment variables.

Quoted 4 lines

The following patch series adds some utilities (qsort and hash table based database functions) and uses these to reimplement the code used for the internal handling of environment variables.

seems to break autocompletion CONFIG_AUTO_COMPLETE :(

TODO:

- drop default env as implemented now; provide a text file based initialization instead (eventually using several text files to incrementally build it from common blocks) and a tool to convert it into a binary blob / object file.

- It would be nice if we could add wildcard support for environment variables; this is needed for variable name auto-completion, but it would also be nice to be able to say "printenv ip*" or "printenv *addr*"

- Some boards don't link any more due to the grown code size:
AR405, CANBT, DU405, PMC440, canyonlands, sc3, sequoia, socrates.

=> cc: Matthias Fuchs <[email protected]>,
Stefan Roese <[email protected]>,
Heiko Schocher <[email protected]>

- Dropping forceenv() causes build problems on schmoogie

=> cc: Sergey Kubushyn <[email protected]>

- Build tested on PPC and ARM only; runtime tested with NOR and NAND flash only => needs testing!!

seems that some todo items are missing, or not well implied. for example, it seems that gd->env_addr needs to be punted, and these functions get a "_f" appended to their name since they only work on default_environment[] now:

	env_get_addr()
	env_get_char()
	env_get_char_memory()
	envmatch()

Quoted 8 lines · Mike Frysinger

seems that some todo items are missing, or not well implied. for example, it seems that gd->env_addr needs to be punted, and these functions get a "_f" appended to their name since they only work on default_environment[] now:

	env_get_addr()
	env_get_char()
	env_get_char_memory()
	envmatch()

I'm not sure I understand what you mean.

These functions not NOT only operate on the default_environment[]; they are also used to access the normal environment before relocation.