Dear Josh Karabin,

In message <720c2e565183a38bc9ecc6b880373534b1826a8b.1243367498.git.gkarabi n@vocollect.com> you wrote:

If only a single CONFIG_ENV constant is defined at build time, the location of the environment will correspond to that environment.

If multiple CONFIG_ENV constants are defined at build time, the environment will default to one of the locations, but the particular location is not defined. Mechanisms for selection will be provided in later patches.

What is the size impact of this patch?

Note that this patch will not go in until you at least explain how you plan to implement such a selection.

...

+static void env_object_ptr_init(void) +{

• if (env_object_ptr)

• return;
  

+#if defined(CONFIG_ENV_IS_IN_DATAFLASH)

• env_object_ptr = &env_object_dataflash;

+#elif defined(CONFIG_ENV_IS_IN_EEPROM)

• env_object_ptr = &env_object_eeprom;

+#elif defined(CONFIG_ENV_IS_IN_FLASH)

• env_object_ptr = &env_object_flash;

+#elif defined(CONFIG_ENV_IS_IN_MG_DISK)

• env_object_ptr = &env_object_mgdisk;

+#elif defined(CONFIG_ENV_IS_IN_NAND)

• env_object_ptr = &env_object_nand;

+#elif defined(CONFIG_ENV_IS_NOWHERE)

• env_object_ptr = &env_object_nowhere;

+#elif defined(CONFIG_ENV_IS_IN_NVRAM)

• env_object_ptr = &env_object_nvram;

+#elif defined(CONFIG_ENV_IS_IN_ONENAND)

• env_object_ptr = &env_object_onenand;

+#elif defined(CONFIG_ENV_IS_IN_SPI_FLASH)

• env_object_ptr = &env_object_sf;

+#else +#error "No environment object specified!" +#endif

This looks as if the user had no choice about what the default location would be? I don't like that.

--- a/common/env_eeprom.c +++ b/common/env_eeprom.c

...

• We are still running from ROM, so data use is limited
• Use a (moderately small) buffer on the stack

*/ -int env_init(void) +static int init_eeprom(void) { ulong crc, len, new; unsigned off; uchar buf[64];

• env_ptr = NULL;

Has this code been tested? See the comment above - we're still running from ROM, and the data segment is not writable!

diff --git a/common/env_flash.c b/common/env_flash.c index 00792cd..7233dcb 100644 --- a/common/env_flash.c +++ b/common/env_flash.c

...

-int env_init(void) +static int init_flash(void) { int crc1_ok = 0, crc2_ok = 0;

@@ -104,6 +100,12 @@ int env_init(void) ulong addr1 = (ulong)&(flash_addr->data); ulong addr2 = (ulong)&(flash_addr_new->data);

+#ifdef ENV_IS_EMBEDDED

• env_ptr = (env_t *)(&environment[0]);

+#else

• env_ptr = (env_t *)CONFIG_ENV_ADDR;

+#endif

See above. I don't think this would work. We don't have a writable data segment yet.

Be careful when moving static initializations into runtime code. The environment needs to be accessed long before the data segment becomes writable.

Did you really test this code?

Best regards,

Wolfgang Denk