| Message ID | 1491221969.4177.81.camel@infinera.com |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
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
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 >
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@denx.de
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 > >
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
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
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
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
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
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;
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 <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(-)