[U-Boot] [PATCH 1/4] Support multiple CONFIG_ENV options in a single build.

[Wolfgang Denk]

Dear Josh Karabin,

In message <720c2e565183a38bc9ecc6b880373534b1826a8b.1243367498.git.gkarabi n at 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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Remember that Beethoven wrote his first symphony in C ...