Remove global variable env_t *env_ptr ?

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.

Dear Joakim,

Quoted 3 lines · In message <[email protected]> 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.

Quoted 6 lines

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?

Quoted 3 lines

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

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.

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:
From 5d3791099fb6a2c503b83298ac1f6135331466dc Mon Sep 17 00:00:00 2001
From: David Gounaris <[email protected]>
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(-)

common/env_flash.c +0 -13

index 004e884..306ef42 100644
@@ -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 */

common/env_flash.c +26 -6

-
-
 #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);

common/env_flash.c +16 -11

@@ -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),

common/env_flash.c +0 -2

-			up_data);
-		debug("Data (start 0x%lX, len 0x%lX) saved at

0x%p\n",

common/env_flash.c +1 -3

-			(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);

common/env_flash.c +6 -15

@@ -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;

common/env_flash.c +22 -12

 	}

@@ -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);

common/env_flash.c +0 -1

-		debug("Data (start 0x%lx, len 0x%lx) saved at

0x%lx\n",

common/env_flash.c +0 -4

-			(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);

common/env_flash.c +4 -2

@@ -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;

Hi Joakim,

Quoted 3 lines

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

Quoted 15 lines

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

Use case is when CONFIG_ENV_SECT_SIZE and/or CONFIG_ENV_ADDR are non constants.

Dear Joakim,

Quoted 2 lines · In message <[email protected]> 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 :-)

Quoted 3 lines

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.

Dear Joakim,

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

Quoted 3 lines

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?