[U-Boot] [PATCH] Revert "env_eeprom: Assign default environment during board_init_f"

[U-Boot] [PATCH] Revert "env_eeprom: Assign default environment during board_init_f"

[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 <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
-- 
1.7.0.5

[U-Boot] [PATCH] Revert "env_eeprom: Assign default environment during board_init_f"

[Tom Rini]

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.

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

[U-Boot] [PATCH] Revert "env_eeprom: Assign default environment during board_init_f"

[Ludger Dreier]

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

[U-Boot] [PATCH] Revert "env_eeprom: Assign default environment during board_init_f"

[Tom Rini]

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.

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