Skip to contentSkip to conversation

Revert "env_eeprom: Assign default environment during board_init_f"

PATCH New Ludger Dreier 2 participants 4 messages Sep 15, 2015 – Sep 16, 2015

lore
Share & tools
lore

Ludger Dreier

author
#0 lore raw

PATCH 1/1 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 <[email protected]>
---
 common/env_eeprom.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)
common/env_eeprom.c +2 -10
index 905d39a..490ac73 100644
@@ -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
Signature · 1 line
1.7.0.5
#1 lore raw

PATCH 1/1 Revert "env_eeprom: Assign default environment during board_init_f"

Quoted 10 lines · Ludger Dreier

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 <[email protected]>

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.

Signature · 11 lines
Tom

[1]:
http://u-boot.denx.narkive.com/sMDi8ur0/patch-env-eeprom-assign-default-environment-during-board-init-f
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150915/ddc496d9/attachment.sig>
author
#2 lore raw

PATCH 1/1 Revert "env_eeprom: Assign default environment during board_init_f"

Hi Tom,

Am 2015-09-15 18:54, schrieb Tom Rini:

Quoted 8 lines

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

#3 lore raw

PATCH 1/1 Revert "env_eeprom: Assign default environment during board_init_f"

Quoted 11 lines · Ludger Dreier

Hi Tom,

Am 2015-09-15 18:54, schrieb Tom Rini:

Quoted 7 lines

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.

Quoted 5 lines

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.

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150916/82dd63de/attachment.sig>