[U-Boot] [PATCH] Revert "env_eeprom: Assign default environment during board_init_f"
This reverts commit ed6a5d4f880ac248530dbf64683b2257dbe54b64.
The original patch uses ENV_IS_EMBEDDED to decide if the default environment should be used or the one actually read from EEPROM. The code in environment.h allows setting of ENV_IS_EMBEDDED only for a subset of flash types. EEPROM is not included in that list. So basically reading environment from I2C EEPROM is broken now. I propose to revert the patch.
Signed-off-by: Ludger Dreier ludger.dreier@keymile.com --- common/env_eeprom.c | 12 ++---------- 1 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/common/env_eeprom.c b/common/env_eeprom.c index 905d39a..490ac73 100644 --- a/common/env_eeprom.c +++ b/common/env_eeprom.c @@ -147,7 +147,6 @@ int saveenv(void) #ifdef CONFIG_ENV_OFFSET_REDUND int env_init(void) { -#ifdef ENV_IS_EMBEDDED ulong len, crc[2], crc_tmp; unsigned int off, off_env[2]; uchar buf[64], flags[2]; @@ -213,16 +212,12 @@ int env_init(void) gd->env_addr = off_env[1] + offsetof(env_t, data); else if (gd->env_valid == 1) gd->env_addr = off_env[0] + offsetof(env_t, data); -#else - gd->env_addr = (ulong)&default_environment[0]; - gd->env_valid = 1; -#endif + return 0; } #else int env_init(void) { -#ifdef ENV_IS_EMBEDDED ulong crc, len, new; unsigned off; uchar buf[64]; @@ -255,10 +250,7 @@ int env_init(void) gd->env_addr = 0; gd->env_valid = 0; } -#else - gd->env_addr = (ulong)&default_environment[0]; - gd->env_valid = 1; -#endif + return 0; } #endif
On Tue, Sep 15, 2015 at 11:35:35AM +0200, Ludger Dreier wrote:
This reverts commit ed6a5d4f880ac248530dbf64683b2257dbe54b64.
The original patch uses ENV_IS_EMBEDDED to decide if the default environment should be used or the one actually read from EEPROM. The code in environment.h allows setting of ENV_IS_EMBEDDED only for a subset of flash types. EEPROM is not included in that list. So basically reading environment from I2C EEPROM is broken now. I propose to revert the patch.
Signed-off-by: Ludger Dreier ludger.dreier@keymile.com
OK, so I went and looked at the original patch and thread again[1] and Michal or Siva, what case was this change fixing exactly because now that I re-read it all again, and look harder at the code, this doesn't make sense. You can't have embedded u-boot + EEPROM. Was there perhaps some sort of init ordering issue in a board which did have EEPROM used as the backing store for env? I could see a case where perhaps (and Ludger, can you test this as well please?) the problem is that for env_eeprom env_init() needs to just default to the built-in (like it's basically doing today) and env_relocate_spec() needs to do the read, check which is valid, etc, etc, dance which it is _not_ doing today.
Hi Tom,
Am 2015-09-15 18:54, schrieb Tom Rini:
Was there perhaps some sort of init ordering issue in a board which did have EEPROM used as the backing store for env? I could see a case where perhaps (and Ludger, can you test this as well please?) the problem is that for env_eeprom env_init() needs to just default to the built-in (like it's basically doing today) and env_relocate_spec() needs to do the read, check which is valid, etc, etc, dance which it is _not_ doing today.
as I understand your idea, the code from both versions of env_init (redundant env and non-redunant) which is framed by "#ifdef ENV_IS_EMBEDDED" may potentially be moved to, or called by env_relocate_spec. I think this could work. If this ok for you, I can come up with a tested proposal. But this would need some days.
Ludger
On Wed, Sep 16, 2015 at 01:46:34PM +0200, Ludger Dreier wrote:
Hi Tom,
Am 2015-09-15 18:54, schrieb Tom Rini:
Was there perhaps some sort of init ordering issue in a board which did have EEPROM used as the backing store for env? I could see a case where perhaps (and Ludger, can you test this as well please?) the problem is that for env_eeprom env_init() needs to just default to the built-in (like it's basically doing today) and env_relocate_spec() needs to do the read, check which is valid, etc, etc, dance which it is _not_ doing today.
as I understand your idea, the code from both versions of env_init (redundant env and non-redunant) which is framed by "#ifdef ENV_IS_EMBEDDED" may potentially be moved to, or called by env_relocate_spec. I think this could work. If this ok for you, I can come up with a tested proposal. But this would need some days.
Roughly speaking, yes. env_init() should just be the "use the default built-in env" code and env_relocate_spec() should do all of the hard work that env_init() used to do.
participants (2)
-
Ludger Dreier -
Tom Rini