[U-Boot] Remove global variable env_t *env_ptr ?

[U-Boot] Remove global variable env_t *env_ptr ?

[Joakim Tjernlund]

I am looking at adding support for runtime sizing of CONFIG_ENV_ADDR as
we need to replace out flash but we don't want to create a new u-boot binairy
just for this simple change.

While converting env_flash.c I noted the global variable
 env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR;
which cannot be runtime decided.
Looking at users of this variable I only find one in pmc405de.c(not sure what that board is doing)
and for what I can tell this variable is not correct for redundant env. either.

Anyhow, I am faced wit two choices, either remove the env_ptr or
convert it to a function call.

What do fellow u-booters think about env_ptr?

 Jocke

PS. Adding my work so far here:

[U-Boot] Remove global variable env_t *env_ptr ?

[Wolfgang Denk]

Dear Joakim,

In message <1491221969.4177.81.camel@infinera.com> you wrote:
> I am looking at adding support for runtime sizing of CONFIG_ENV_ADDR as
> we need to replace out flash but we don't want to create a new u-boot binairy
> just for this simple change.

I doubt this will work for configurations that use embedded
environment.

> While converting env_flash.c I noted the global variable
>  env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR;
> which cannot be runtime decided.
> Looking at users of this variable I only find one in pmc405de.c(not sure what that board is doing)
> and for what I can tell this variable is not correct for redundant env. either.

Did you look in the code only, or in all files?

> Anyhow, I am faced wit two choices, either remove the env_ptr or
> convert it to a function call.

Probably neither will work for all use cases.  You remember the good
old times when we had parallel NOR flash with a few smaller sectors
somewhere near the beginning or the end of the device?  It was
pretty usual to use these small sectors for the environment, and it
was the task of thelinker script to "wrap" the rest of the code
around these reserved sectors.  For this, the environment location
must be known not only in the code, but also in the linker script.

Without thorough checking , at least these files look suspicious to
me:

arch/powerpc/cpu/mpc5xx/u-boot.lds:  . = env_start;
arch/powerpc/cpu/mpc5xx/u-boot.lds:  .ppcenv :
arch/powerpc/cpu/mpc5xx/u-boot.lds:    common/env_embedded.o (.ppcenv)
arch/powerpc/cpu/mpc5xxx/u-boot-customlayout.lds:    . = DEFINED(env_offset) ? env_offset : .;
arch/powerpc/cpu/mpc5xxx/u-boot-customlayout.lds:    common/env_embedded.o              (.ppcenv*)
board/tqc/tqm8xx/u-boot.lds:    . = DEFINED(env_offset) ? env_offset : .;
board/tqc/tqm8xx/u-boot.lds:    common/env_embedded.o   (.ppcenv*)
board/freescale/mx31ads/u-boot.lds:       . = DEFINED(env_offset) ? env_offset : .;
board/freescale/mx31ads/u-boot.lds:       common/env_embedded.o(.text*)

Please have a look at these, and verify that the image layout does
not change for these with any such changes.

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: wd at denx.de
An expert is a person who avoids the small errors while  sweeping  on
to the grand fallacy.

[U-Boot] Remove global variable env_t *env_ptr ?

[Joakim Tjernlund]

On Mon, 2017-04-03 at 22:17 +0200, Wolfgang Denk wrote:
> Dear Joakim,

Dear Wolfgang,

> 
> In message <1491221969.4177.81.camel@infinera.com> you wrote:
> > I am looking at adding support for runtime sizing of CONFIG_ENV_ADDR as
> > we need to replace out flash but we don't want to create a new u-boot binairy
> > just for this simple change.
> 
> I doubt this will work for configurations that use embedded
> environment.

I see, I don't think so but will look closer.

> 
> > While converting env_flash.c I noted the global variable
> >  env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR;
> > which cannot be runtime decided.
> > Looking at users of this variable I only find one in pmc405de.c(not sure what that board is doing)
> > and for what I can tell this variable is not correct for redundant env. either.
> 
> Did you look in the code only, or in all files?

code only, for the use of the env_ptr variable only as this is the variable I looking at.

> 
> > Anyhow, I am faced wit two choices, either remove the env_ptr or
> > convert it to a function call.
> 
> Probably neither will work for all use cases.  You remember the good
> old times when we had parallel NOR flash with a few smaller sectors
> somewhere near the beginning or the end of the device?  It was

Sure do, this is the reason I am having this problem. The new flashes do
not have such smaller sectors, they are all uniform :(

> pretty usual to use these small sectors for the environment, and it
> was the task of thelinker script to "wrap" the rest of the code
> around these reserved sectors.  For this, the environment location
> must be known not only in the code, but also in the linker script.

After a brief look I think we are good. Let me explain, I am only
making it possible to #define CONFIG_ENV_ADDR, CONFIG_ENV_SECT_SIZE etc.
as non constant data by moving the assignment of flash_addr etc. to runtime,
removing the static variable(less relocs to perform too :), I not forcing
anyone to do so and only for env_flash.c

The only variable that I can't do away with it the:
 env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR;
as it is global. Nothing really uses this global variable(except pmc405de.c which is EPROM),
not even the linker scripts below. They use #defines directly(CONFIG_ENV_OFFSET,
CONFIG_SYS_FLASH_BASE etc. except for env_embedded.c which uses other variables)

As nothing really uses env_ptr and a variable isn't really a good interface(compare 
with the errno variable) I propose that the env_ptr variable in code is removed but
lets start with removing it for env_flash.c first.

> 
> Without thorough checking , at least these files look suspicious to
> me:
> 
> arch/powerpc/cpu/mpc5xx/u-boot.lds:  . = env_start;
> arch/powerpc/cpu/mpc5xx/u-boot.lds:  .ppcenv :
> arch/powerpc/cpu/mpc5xx/u-boot.lds:    common/env_embedded.o (.ppcenv)
> arch/powerpc/cpu/mpc5xxx/u-boot-customlayout.lds:    . = DEFINED(env_offset) ? env_offset : .;
> arch/powerpc/cpu/mpc5xxx/u-boot-customlayout.lds:    common/env_embedded.o              (.ppcenv*)
> board/tqc/tqm8xx/u-boot.lds:    . = DEFINED(env_offset) ? env_offset : .;
> board/tqc/tqm8xx/u-boot.lds:    common/env_embedded.o   (.ppcenv*)
> board/freescale/mx31ads/u-boot.lds:       . = DEFINED(env_offset) ? env_offset : .;
> board/freescale/mx31ads/u-boot.lds:       common/env_embedded.o(.text*)
> 
> Please have a look at these, and verify that the image layout does
> not change for these with any such changes.
> 
> Best regards,
> 
> Wolfgang Denk
> 

[U-Boot] Remove global variable env_t *env_ptr ?

[Wolfgang Denk]

Dear Joakim,

In message <1491293863.4177.92.camel@infinera.com> you wrote:
> 
> After a brief look I think we are good. Let me explain, I am only
> making it possible to #define CONFIG_ENV_ADDR, CONFIG_ENV_SECT_SIZE etc.
> as non constant data by moving the assignment of flash_addr etc. to runtime,
> removing the static variable(less relocs to perform too :), I not forcing
> anyone to do so and only for env_flash.c
>
> The only variable that I can't do away with it the:
>  env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR;
> as it is global. Nothing really uses this global variable(except pmc405de.c which is EPROM),
> not even the linker scripts below. They use #defines directly(CONFIG_ENV_OFFSET,
> CONFIG_SYS_FLASH_BASE etc. except for env_embedded.c which uses other variables)

But CONFIG_ENV_ADDR and CONFIG_ENV_OFFSET are somewhat related,
aren't they?

> As nothing really uses env_ptr and a variable isn't really a good interface(compare 
> with the errno variable) I propose that the env_ptr variable in code is removed but
> lets start with removing it for env_flash.c first.

But if you make it possible to change flash_addr at runtime, then
the assumptions of the linker and the actual placeent of the
environment sector in flash would no longer be in sync with the
code's view of thing.


Maybe you could be so kind and explain your use case?  To me it
looks as if you want to "switch" to a different location, i. e. to
alternative environment settings.  If this is what you want to do I
wonder if it's not easier (at least it does not require code
changes) to switch "profiles" by using "env import" ?

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: wd at denx.de
Just about every computer on the market today runs Unix,  except  the
Mac (and nobody cares about it).                   - Bill Joy 6/21/85

[U-Boot] Remove global variable env_t *env_ptr ?

[Lukasz Majewski]

Hi Joakim,

> I am looking at adding support for runtime sizing of CONFIG_ENV_ADDR
> as we need to replace out flash but we don't want to create a new
> u-boot binairy just for this simple change.

Please correct me if I did not understand your use case correctly.

Other boards have separate regions in flash to store ENV variables -
even redundancy is supported (from ./include/mccmon6.h)

/* Envs are stored in NOR flash */
#define CONFIG_ENV_IS_IN_FLASH
#define CONFIG_ENV_SECT_SIZE    (SZ_128K)
#define CONFIG_ENV_ADDR (CONFIG_SYS_FLASH_BASE + 0x40000)

#define CONFIG_SYS_REDUNDAND_ENVIRONMENT
#define CONFIG_ENV_ADDR_REDUND  (CONFIG_SYS_FLASH_BASE + 0x60000)
#define CONFIG_ENV_SIZE_REDUND  CONFIG_ENV_SIZE

You can extract the ENV variables by using following script:

scripts/get_default_envs.sh > default_envs.txt

and then create updated env image (with [*]) to be stored on flash.


Note:

[*] 
./tools/mkenvimage -s 131072 -o ${UBOOT_ENVS_DEFAULT} default_envs.txt

Best regards,
Łukasz Majewski

> 
> While converting env_flash.c I noted the global variable
>  env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR;
> which cannot be runtime decided.
> Looking at users of this variable I only find one in pmc405de.c(not
> sure what that board is doing) and for what I can tell this variable
> is not correct for redundant env. either.
> 
> Anyhow, I am faced wit two choices, either remove the env_ptr or
> convert it to a function call.
> 
> What do fellow u-booters think about env_ptr?
> 
>  Jocke
> 
> PS. Adding my work so far here:
> From 5d3791099fb6a2c503b83298ac1f6135331466dc Mon Sep 17 00:00:00 2001
> From: David Gounaris <david.gounaris@infinera.com>
> Date: Fri, 31 Mar 2017 11:31:40 +0200
> Subject: [PATCH 2/4] env_flash.c: Support dynamic sector size
> 
> This lay the ground work for supporting a non constant
> sector size environment data.
> ---
>  common/env_flash.c | 152
> +++++++++++++++++++++++++++++------------------------ 1 file changed,
> 83 insertions(+), 69 deletions(-)
> 
> diff --git a/common/env_flash.c b/common/env_flash.c
> index 004e884..306ef42 100644
> --- a/common/env_flash.c
> +++ b/common/env_flash.c
> @@ -36,31 +36,20 @@ char *env_name_spec = "Flash";
>  #ifdef ENV_IS_EMBEDDED
>  env_t *env_ptr = &environment;
>  
> -static env_t *flash_addr = (env_t *)CONFIG_ENV_ADDR;
> -
>  #else /* ! ENV_IS_EMBEDDED */
>  
>  env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR;
> -static env_t *flash_addr = (env_t *)CONFIG_ENV_ADDR;
>  #endif /* ENV_IS_EMBEDDED */
>  
> -#if defined(CMD_SAVEENV) || defined(CONFIG_ENV_ADDR_REDUND)
> -/* CONFIG_ENV_ADDR is supposed to be on sector boundary */
> -static ulong end_addr = CONFIG_ENV_ADDR + CONFIG_ENV_SECT_SIZE - 1;
> -#endif
> -
> -#ifdef CONFIG_ENV_ADDR_REDUND
> -static env_t *flash_addr_new = (env_t *)CONFIG_ENV_ADDR_REDUND;
> -
> -/* CONFIG_ENV_ADDR_REDUND is supposed to be on sector boundary */
> -static ulong end_addr_new = CONFIG_ENV_ADDR_REDUND +
> CONFIG_ENV_SECT_SIZE - 1; -#endif /* CONFIG_ENV_ADDR_REDUND */
> -
> -
>  #ifdef CONFIG_ENV_ADDR_REDUND
>  int env_init(void)
>  {
>  	int crc1_ok = 0, crc2_ok = 0;
> +	env_t *flash_addr;
> +	env_t *flash_addr_new;
> +
> +	flash_addr = (env_t *)CONFIG_ENV_ADDR;
> +	flash_addr_new = (env_t *)CONFIG_ENV_ADDR_REDUND;
>  
>  	uchar flag1 = flash_addr->flags;
>  	uchar flag2 = flash_addr_new->flags;
> @@ -109,9 +98,28 @@ int saveenv(void)
>  	char	*saved_data = NULL;
>  	char	flag = OBSOLETE_FLAG, new_flag = ACTIVE_FLAG;
>  	int	rc = 1;
> -#if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
> -	ulong	up_data = 0;
> -#endif
> +	ulong   up_data = 0;
> +	env_t *flash_addr;
> +	env_t *flash_addr_new;
> +	ulong end_addr;
> +	ulong end_addr_new;
> +
> +	flash_addr = (env_t *)CONFIG_ENV_ADDR;
> +	flash_addr_new = (env_t *)CONFIG_ENV_ADDR_REDUND;
> +	end_addr = CONFIG_ENV_ADDR + CONFIG_ENV_SECT_SIZE - 1;
> +	end_addr_new = CONFIG_ENV_ADDR_REDUND + CONFIG_ENV_SECT_SIZE
> - 1; +
> +	if (gd->env_addr != (ulong)&(flash_addr->data)) {
> +		/* Swap new and old ptrs */
> +		env_t *etmp = flash_addr;
> +		ulong ltmp = end_addr;
> +
> +		flash_addr = flash_addr_new;
> +		flash_addr_new = etmp;
> +
> +		end_addr = end_addr_new;
> +		end_addr_new = ltmp;
> +	}
>  
>  	debug("Protect off %08lX ... %08lX\n", (ulong)flash_addr,
> end_addr); 
> @@ -129,24 +137,25 @@ int saveenv(void)
>  		return rc;
>  	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%lX\n", up_data);
> -	if (up_data) {
> -		saved_data = malloc(up_data);
> -		if (saved_data == NULL) {
> -			printf("Unable to save the rest of sector
> (%ld)\n",
> -				up_data);
> -			goto done;
> +	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%lX\n", up_data);
> +		if (up_data) {
> +			saved_data = malloc(up_data);
> +			if (saved_data == NULL) {
> +				printf("Unable to save the rest of
> sector (%ld)\n",
> +				       up_data);
> +				goto done;
> +			}
> +			memcpy(saved_data,
> +			       (void *)((long)flash_addr_new +
> CONFIG_ENV_SIZE),
> +			       up_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);
>  		}
> -		memcpy(saved_data,
> -			(void *)((long)flash_addr_new +
> CONFIG_ENV_SIZE),
> -			up_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 ...", (ulong)flash_addr_new,
> end_addr_new); 
> @@ -167,7 +176,6 @@ int saveenv(void)
>  	if (rc)
>  		goto perror;
>  
> -#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", (long)flash_addr_new + CONFIG_ENV_SIZE, up_data);
> @@ -176,19 +184,8 @@ int saveenv(void)
>  				up_data))
>  			goto perror;
>  	}
> -#endif
> -	puts("done\n");
> -
> -	{
> -		env_t *etmp = flash_addr;
> -		ulong ltmp = end_addr;
> -
> -		flash_addr = flash_addr_new;
> -		flash_addr_new = etmp;
>  
> -		end_addr = end_addr_new;
> -		end_addr_new = ltmp;
> -	}
> +	puts("done\n");
>  
>  	rc = 0;
>  	goto done;
> @@ -209,8 +206,11 @@ done:
>  
>  int env_init(void)
>  {
> -	if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
> -		gd->env_addr	= (ulong)&(env_ptr->data);
> +	env_t *flash_addr;
> +
> +	flash_addr = (env_t *)CONFIG_ENV_ADDR;
> +	if (crc32(0, flash_addr->data, ENV_SIZE) == flash_addr->crc)
> {
> +		gd->env_addr	= (ulong)&(flash_addr->data);
>  		gd->env_valid	= 1;
>  		return 0;
>  	}
> @@ -226,26 +226,31 @@ int saveenv(void)
>  	env_t	env_new;
>  	int	rc = 1;
>  	char	*saved_data = NULL;
> -#if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
>  	ulong	up_data = 0;
> -
> -	up_data = end_addr + 1 - ((long)flash_addr +
> CONFIG_ENV_SIZE);
> -	debug("Data to save 0x%lx\n", up_data);
> -	if (up_data) {
> -		saved_data = malloc(up_data);
> -		if (saved_data == NULL) {
> -			printf("Unable to save the rest of sector
> (%ld)\n",
> -				up_data);
> -			goto done;
> +	env_t *flash_addr;
> +	ulong end_addr;
> +
> +	flash_addr = (env_t *)CONFIG_ENV_ADDR;
> +	end_addr = CONFIG_ENV_ADDR + CONFIG_ENV_SECT_SIZE - 1;
> +
> +	if(CONFIG_ENV_SECT_SIZE > 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) {
> +			saved_data = malloc(up_data);
> +			if (saved_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);
>  		}
> -		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 */
>  
>  	debug("Protect off %08lX ... %08lX\n", (ulong)flash_addr,
> end_addr); 
> @@ -265,7 +270,6 @@ int saveenv(void)
>  	if (rc != 0)
>  		goto perror;
>  
> -#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);
> @@ -274,7 +278,7 @@ int saveenv(void)
>  				up_data))
>  			goto perror;
>  	}
> -#endif
> +
>  	puts("done\n");
>  	rc = 0;
>  	goto done;
> @@ -294,6 +298,16 @@ done:
>  void env_relocate_spec(void)
>  {
>  #ifdef CONFIG_ENV_ADDR_REDUND
> +	env_t *flash_addr;
> +	env_t *flash_addr_new;
> +	ulong end_addr;
> +	ulong end_addr_new;
> +
> +	flash_addr = (env_t *)CONFIG_ENV_ADDR;
> +	flash_addr_new = (env_t *)CONFIG_ENV_ADDR_REDUND;
> +	end_addr = CONFIG_ENV_ADDR + CONFIG_ENV_SECT_SIZE - 1;
> +	end_addr_new = CONFIG_ENV_ADDR_REDUND + CONFIG_ENV_SECT_SIZE
> - 1; +
>  	if (gd->env_addr != (ulong)&(flash_addr->data)) {
>  		env_t *etmp = flash_addr;
>  		ulong ltmp = end_addr;




Best regards,

Lukasz Majewski

--

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: wd at denx.de

[U-Boot] Remove global variable env_t *env_ptr ?

[Joakim Tjernlund]

On Tue, 2017-04-04 at 10:55 +0200, Lukasz Majewski wrote:
> Hi Joakim,
> 
> > I am looking at adding support for runtime sizing of CONFIG_ENV_ADDR
> > as we need to replace out flash but we don't want to create a new
> > u-boot binairy just for this simple change.
> 
> Please correct me if I did not understand your use case correctly.
> 
> Other boards have separate regions in flash to store ENV variables -
> even redundancy is supported (from ./include/mccmon6.h)
> 
> /* Envs are stored in NOR flash */
> #define CONFIG_ENV_IS_IN_FLASH
> #define CONFIG_ENV_SECT_SIZE    (SZ_128K)
> #define CONFIG_ENV_ADDR (CONFIG_SYS_FLASH_BASE + 0x40000)
> 
> #define CONFIG_SYS_REDUNDAND_ENVIRONMENT
> #define CONFIG_ENV_ADDR_REDUND  (CONFIG_SYS_FLASH_BASE + 0x60000)
> #define CONFIG_ENV_SIZE_REDUND  CONFIG_ENV_SIZE

Use case is when CONFIG_ENV_SECT_SIZE and/or CONFIG_ENV_ADDR are non constants.
Then, in my case, these becomes:
#define CONFIG_ENV_SECT_SIZE    (get_env_sect_size())
#define CONFIG_ENV_ADDR    (get_env_address())

Jocke
> 
> You can extract the ENV variables by using following script:
> 
> scripts/get_default_envs.sh > default_envs.txt
> 
> and then create updated env image (with [*]) to be stored on flash.
> 
> 
> Note:
> 
> [*] 
> ./tools/mkenvimage -s 131072 -o ${UBOOT_ENVS_DEFAULT} default_envs.txt
> 
> Best regards,
> Łukasz Majewski
> 
> > 
> > While converting env_flash.c I noted the global variable
> >  env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR;
> > which cannot be runtime decided.
> > Looking at users of this variable I only find one in pmc405de.c(not
> > sure what that board is doing) and for what I can tell this variable
> > is not correct for redundant env. either.
> > 
> > Anyhow, I am faced wit two choices, either remove the env_ptr or
> > convert it to a function call.
> > 
> > What do fellow u-booters think about env_ptr?
> > 
> >  Jocke
> > 

[U-Boot] Remove global variable env_t *env_ptr ?

[Wolfgang Denk]

Dear Joakim,

In message <1491301459.28343.1.camel@infinera.com> you wrote:
>
> Use case is when CONFIG_ENV_SECT_SIZE and/or CONFIG_ENV_ADDR are non constants.

That is my exact question - when would this happen?  Flash sectors
do now wander around in memory or change size :-)

So you attempt to switch between different settings?

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: wd at denx.de
Shakespeare's Law of Prototyping: (Hamlet III, iv, 156-160)
        O, throw away the worser part of it,
        And live the purer with the other half.

[U-Boot] Remove global variable env_t *env_ptr ?

[Joakim Tjernlund]

On Tue, 2017-04-04 at 12:31 +0200, Wolfgang Denk wrote:
> Dear Joakim,
> 
> In message <1491301459.28343.1.camel@infinera.com> you wrote:
> > 
> > Use case is when CONFIG_ENV_SECT_SIZE and/or CONFIG_ENV_ADDR are non constants.
> 
> That is my exact question - when would this happen?  Flash sectors
> do now wander around in memory or change size :-)

No, but they happens when you are forced to update you HW with new type of flashes
with different layout so you have to move where the environment is stored.
Sure, this can be fixed by having two different u-boot images but that will
in our case be just painful to carry around an extra u-boot img, then in field
devise a method to chose the right img. for what looks like the same HW but the flash.

What I have done is not that earth shattering, basically just move from:
  static env_t *flash_addr = (env_t *)CONFIG_ENV_ADDR;
to
 int env_init(void)
 {
   ...
   env_t *flash_addr = (env_t *)CONFIG_ENV_ADDR;

This allows CONFIG_ENV_ADDR to be a function but it does not have to, you also
lose a relocation entry as a static variable is removed.

 Jocke

[U-Boot] Remove global variable env_t *env_ptr ?

[Wolfgang Denk]

Dear Joakim,

In message <1491302640.30240.1.camel@infinera.com> you wrote:
>
> > That is my exact question - when would this happen?  Flash sectors
> > do now wander around in memory or change size :-)
>
> No, but they happens when you are forced to update you HW with new type of flashes
> with different layout so you have to move where the environment is stored.
> Sure, this can be fixed by having two different u-boot images but that will
> in our case be just painful to carry around an extra u-boot img, then in field
> devise a method to chose the right img. for what looks like the same HW but the flash.

I see. Well, this obviously means that you are _not_ using an
embedded environment, as otherwise the linker generated image would
be wrong for the "other" type of flash chips - which means, the
environment is somewhare outside, separate from the U-Boot image.

So you have old hardware, running old U-Boot, which does not support
the new flash.  For this, you need a new U-Boot, which then shall
support both the old and the new flash, right?  But why can you then
not simply come up with a new flash layout, which is compatoble with
the old and new flashes, so it can use a common configuration,
without code changes?

I'm aware that embedded environment is not used very often these
days any more, as is parallel NOR flash, but changes in this are
have caused trouble more than once in the past, so I am not
convinced that it's wise to touch such ancient, "stable" code for
an exotic feature that probably nobody else will ever need?


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: wd at denx.de
The explanation requiring the fewest assumptions is the  most  likely
to be correct.                                    -- William of Occam

[U-Boot] Remove global variable env_t *env_ptr ?

[Joakim Tjernlund]

On Tue, 2017-04-04 at 13:27 +0200, Wolfgang Denk wrote:
> Dear Joakim,
> 
> In message <1491302640.30240.1.camel@infinera.com> you wrote:
> > 
> > > That is my exact question - when would this happen?  Flash sectors
> > > do now wander around in memory or change size :-)
> > 
> > No, but they happens when you are forced to update you HW with new type of flashes
> > with different layout so you have to move where the environment is stored.
> > Sure, this can be fixed by having two different u-boot images but that will
> > in our case be just painful to carry around an extra u-boot img, then in field
> > devise a method to chose the right img. for what looks like the same HW but the flash.
> 
> I see. Well, this obviously means that you are _not_ using an
> embedded environment, as otherwise the linker generated image would
> be wrong for the "other" type of flash chips - which means, the
> environment is somewhare outside, separate from the U-Boot image.

Right. The env. is on separate sectors not in u-boot's own space.

> 
> So you have old hardware, running old U-Boot, which does not support
> the new flash.  For this, you need a new U-Boot, which then shall
> support both the old and the new flash, right?  But why can you then
> not simply come up with a new flash layout, which is compatoble with
> the old and new flashes, so it can use a common configuration,
> without code changes?

Because the old flash has its env. in "small" sectors which does
not exist on new flash so the layout on the new flash has both different env.
address as well as a bigger env. sector size. I cannot move the old
env. either as there is no free space for that(the rest of the flash is a JFFS2 FS)

> 
> I'm aware that embedded environment is not used very often these
> days any more, as is parallel NOR flash, but changes in this are
> have caused trouble more than once in the past, so I am not
> convinced that it's wise to touch such ancient, "stable" code for
> an exotic feature that probably nobody else will ever need?

Again, I don't think I am changing much(if anything) for your embedded case.
Please read the WIP patch I sent earlier and perhaps you can point me to
a potential problem area?

Not sure about "nobody else will ever need" as Micron are terminating the
Intel/cmdset0001 flashes(don't know if this applies to all Intel chips though)
and we have to swap to AMD/cmd0002, we are just early with this change. 

 Jocke