[U-Boot] ENV_IS_EMBEDDED vs CONFIG_ENV_IS_EMBEDDED
is there a reason to keep ENV_IS_EMBEDDED around ? i see a few places that use CONFIG_ENV_IS_EMBEDDED, but it seems to be largely useless at the moment as the define doesn't really do anything without ENV_IS_EMBEDDED.
in other words, i'd like to just do a sed on the tree to convert all ENV_IS_EMBEDDED instances to CONFIG_ENV_IS_EMBEDDED. -mike
Dear Mike Frysinger,
In message 200906142240.59607.vapier@gentoo.org you wrote:
is there a reason to keep ENV_IS_EMBEDDED around ? i see a few places that use CONFIG_ENV_IS_EMBEDDED, but it seems to be largely useless at the moment as the define doesn't really do anything without ENV_IS_EMBEDDED.
I'm not sure where CONFIG_ENV_IS_EMBEDDED (resp. CFG_ENV_IS_EMBEDDED) coming from. I guess from some comments ("short-cut compile-time test") that it was intended to enable using something like
COBJS-$(CONFIG_ENV_IS_EMBEDDED) += env_embedded.o
Unfortunately it was not even documented in the README so we can only guess what it is supposed to mean :-( From original the software design point of view, CONFIG_ENV_IS_EMBEDDED is redundant at best, and usally just bogus, as the actual location of the environment sectors (as determined by CONFIG_ENV_OFFSET or CONFIG_ENV_ADDR combined with CONFIG_ENV_SECT_SIZE, plus eventually the definition of the corresponding "*_REDUND" settings) automatically determine if the environment is embedded or not - there is no need for such a variable.
in other words, i'd like to just do a sed on the tree to convert all ENV_IS_EMBEDDED instances to CONFIG_ENV_IS_EMBEDDED.
That whould IMHO be wrong. We should rather check if there was a way to get rid of CONFIG_ENV_IS_EMBEDDED (which is probably hard because of it's use in the Makefile).
Best regards,
Wolfgang Denk
On Monday 15 June 2009 05:02:42 Wolfgang Denk wrote:
Mike Frysinger wrote:
is there a reason to keep ENV_IS_EMBEDDED around ? i see a few places that use CONFIG_ENV_IS_EMBEDDED, but it seems to be largely useless at the moment as the define doesn't really do anything without ENV_IS_EMBEDDED.
I'm not sure where CONFIG_ENV_IS_EMBEDDED (resp. CFG_ENV_IS_EMBEDDED) coming from. I guess from some comments ("short-cut compile-time test") that it was intended to enable using something like
COBJS-$(CONFIG_ENV_IS_EMBEDDED) += env_embedded.o
Unfortunately it was not even documented in the README so we can only guess what it is supposed to mean :-( From original the software design point of view, CONFIG_ENV_IS_EMBEDDED is redundant at best, and usally just bogus, as the actual location of the environment sectors (as determined by CONFIG_ENV_OFFSET or CONFIG_ENV_ADDR combined with CONFIG_ENV_SECT_SIZE, plus eventually the definition of the corresponding "*_REDUND" settings) automatically determine if the environment is embedded or not - there is no need for such a variable.
i would actually prefer CONFIG_ENV_IS_EMBEDDED. otherwise we have to write the makefiles to account for all CONFIG_ENV_IS_IN_XXX variants. you can see the common/Makefile and tools/Makefile already suffer from this, and the recent patch i had to post to add missing config lines for some env sources.
so it would make life easier if these things bubbled up into just one config value -- easier for existing maintenance and easier for people adding new environment sources (like the newly added "mgdisk", whatever that is).
in other words, i'd like to just do a sed on the tree to convert all ENV_IS_EMBEDDED instances to CONFIG_ENV_IS_EMBEDDED.
That whould IMHO be wrong. We should rather check if there was a way to get rid of CONFIG_ENV_IS_EMBEDDED (which is probably hard because of it's use in the Makefile).
well, is there a downside from moving from ENV_IS_EMBEDDED to CONFIG_ENV_IS_EMBEDDED ? we could at least make sure it is kept in sync in common headers by doing something like: #if defined(CONFIG_ENV_IS_EMBEDDED) && !defined(ENV_IS_EMBEDDED) # error you should not define CONFIG_ENV_IS_EMBEDDED yourself #endif #ifdef ENV_IS_EMBEDDED # define CONFIG_ENV_IS_EMBEDDED #endif -mike
Dear Mike Frysinger,
In message 200906190548.37976.vapier@gentoo.org you wrote:
well, is there a downside from moving from ENV_IS_EMBEDDED to CONFIG_ENV_IS_EMBEDDED ? we could at least make sure it is kept in sync in
Yes. It's IMHO silly to have to configure something manually (and keep it in sync with potential changes of some other defines) when it is as well possible to have that same value computed automatically at build time.
Best regards,
Wolfgang Denk
On Friday 10 July 2009 18:17:26 Wolfgang Denk wrote:
Mike Frysinger wrote:
well, is there a downside from moving from ENV_IS_EMBEDDED to CONFIG_ENV_IS_EMBEDDED ? we could at least make sure it is kept in sync in
Yes. It's IMHO silly to have to configure something manually (and keep it in sync with potential changes of some other defines) when it is as well possible to have that same value computed automatically at build time.
so i should convert ENV_IS_EMBEDDED to CONFIG_ENV_IS_EMBEDDED or i should add to common code logic to force CONFIG_ENV_IS_EMBEDDED when ENV_IS_EMBEDDED is defined by the board ? -mike
Dear Mike Frysinger,
In message 200907102250.57311.vapier@gentoo.org you wrote:
Yes. It's IMHO silly to have to configure something manually (and keep it in sync with potential changes of some other defines) when it is as well possible to have that same value computed automatically at build time.
so i should convert ENV_IS_EMBEDDED to CONFIG_ENV_IS_EMBEDDED or i should add to common code logic to force CONFIG_ENV_IS_EMBEDDED when ENV_IS_EMBEDDED is defined by the board ?
If it's possible, generating CONFIG_ENV_IS_EMBEDDED when ENV_IS_EMBEDDED is defined would be much better - however, my understanding is that CONFIG_ENV_IS_EMBEDDED is needed by the make proces,,i. e. before actually running any commands, while ENV_IS_EMBEDDED gets computed by the C preprocessor, i. e. too late for make decisions (unless you copy the part of code that computes ENV_IS_EMBEDDED into some special script / file and run it through the C preprocessor - which is not exactly a leaner design either, it seems).
Best regards,
Wolfgang Denk
On Saturday 11 July 2009 03:47:28 Wolfgang Denk wrote:
Mike Frysinger wrote:
Yes. It's IMHO silly to have to configure something manually (and keep it in sync with potential changes of some other defines) when it is as well possible to have that same value computed automatically at build time.
so i should convert ENV_IS_EMBEDDED to CONFIG_ENV_IS_EMBEDDED or i should add to common code logic to force CONFIG_ENV_IS_EMBEDDED when ENV_IS_EMBEDDED is defined by the board ?
If it's possible, generating CONFIG_ENV_IS_EMBEDDED when ENV_IS_EMBEDDED is defined would be much better - however, my understanding is that CONFIG_ENV_IS_EMBEDDED is needed by the make proces,,i. e. before actually running any commands, while ENV_IS_EMBEDDED gets computed by the C preprocessor, i. e. too late for make decisions (unless you copy the part of code that computes ENV_IS_EMBEDDED into some special script / file and run it through the C preprocessor - which is not exactly a leaner design either, it seems).
well, automating for NOR flash and preventing future bleed is easy to do. but there doesnt seem to be a way for automating for nand and onenand types, so board people would still need to opt-in manually for their boards. how does the attached patch look ? i cant test any of these non-Blackfin boards, but the logic should be straight forward ...
i guess part of the problem is that the meaning of ENV_IS_EMBEDDED has changed over time. going by environment.h, it would seem that the original point was that the environment is embedded in the runtime address map (between monitor_base and monitor_base+monitor_len), but it has since changed to mean it is embedded in the u-boot blob as stored in flash. but for eeprom, nand, nvram, and onenand, it means the environment is in a sector that is in the middle of the u-boot image stored in flash.
i dont know how you want to document this define, but if you want to revert to the original meaning (embedded at runtime), then i dont think it'll be hard for the other types to convert. after all, this should just be a linker script issue (punch a hole in the map for the environment). -mike
Dear Mike Frysinger,
In message 200907241634.33639.vapier@gentoo.org you wrote:
From 8ea84aa414e204c3259861da80d11ff7cc9fded7 Mon Sep 17 00:00:00 2001 From: Mike Frysinger vapier@gentoo.org Date: Fri, 24 Jul 2009 15:40:52 -0400 Subject: [PATCH] unify {CONFIG_,}ENV_IS_EMBEDDED
Some boards have fallen out of sync by defining CONFIG_ENV_IS_EMBEDDED manually. While it is useful to have this available to the build system, let's do it automatically rather than forcing people to opt into it.
Signed-off-by: Mike Frysinger vapier@gentoo.org
include/configs/EB+MCF-EV123.h | 1 - include/configs/M52277EVB.h | 1 - include/configs/M5235EVB.h | 1 - include/configs/M5272C3.h | 1 - include/configs/M5275EVB.h | 1 - include/configs/M5329EVB.h | 1 - include/configs/M5373EVB.h | 1 - include/configs/M54451EVB.h | 1 - include/configs/M54455EVB.h | 1 - include/configs/M5475EVB.h | 1 - include/configs/M5485EVB.h | 1 - include/configs/OXC.h | 1 - include/configs/cobra5272.h | 1 - include/configs/pcu_e.h | 1 - include/environment.h | 33 +++++++++++++++++++++++++++------ 15 files changed, 27 insertions(+), 20 deletions(-)
Applied to "next" branch, thanks.
Best regards,
Wolfgang Denk
Commit 16fc32f introduced an error in include/environment.h, which makes u-boot fail to compile due to a missing #endif. This also fixes a merge conflict remaining in include/configs/EB+MCF-EV123.h which was caused by the same commit
Signed-off-by: Albin Tonnerre albin.tonnerre@free-electrons.com --- include/configs/EB+MCF-EV123.h | 7 ------- include/environment.h | 1 + 2 files changed, 1 insertions(+), 7 deletions(-)
diff --git a/include/configs/EB+MCF-EV123.h b/include/configs/EB+MCF-EV123.h index 1333a17..a0b27a8 100644 --- a/include/configs/EB+MCF-EV123.h +++ b/include/configs/EB+MCF-EV123.h @@ -63,13 +63,6 @@ #define CONFIG_ENV_ADDR 0xF003C000 /* End of 256K */ #define CONFIG_ENV_SECT_SIZE 0x4000 #define CONFIG_ENV_IS_IN_FLASH 1 -<<<<<<< HEAD:include/configs/EB+MCF-EV123.h -======= -/* -#define CONFIG_ENV_ADDR_REDUND 0xF0018000 -#define CONFIG_ENV_SECT_SIZE_REDUND 0x4000 -*/ ->>>>>>> unify {CONFIG_,}ENV_IS_EMBEDDED:include/configs/EB+MCF-EV123.h #else #define CONFIG_ENV_ADDR 0xFFE04000 #define CONFIG_ENV_SECT_SIZE 0x2000 diff --git a/include/environment.h b/include/environment.h index 6188457..b9924fd 100644 --- a/include/environment.h +++ b/include/environment.h @@ -100,6 +100,7 @@ !defined(CONFIG_ENV_IS_IN_NAND) && \ !defined(CONFIG_ENV_IS_IN_ONENAND) # error "CONFIG_ENV_IS_EMBEDDED not supported for your flash type" +# endif #endif
/*
On Tuesday 25 August 2009 04:46:07 Albin Tonnerre wrote:
Commit 16fc32f introduced an error in include/environment.h, which makes u-boot fail to compile due to a missing #endif. This also fixes a merge conflict remaining in include/configs/EB+MCF-EV123.h which was caused by the same commit
this is only an issue for the "next" branch ...
my local git tree has the right number of #endif's, so i guess i must have sent out the wrong patch
guess this needs to get squashed into the relevant change rather than added as a new commit. thanks for the update. -mike
On Tue, Aug 25, 2009 at 06:16:24AM -0400, Mike Frysinger wrote :
On Tuesday 25 August 2009 04:46:07 Albin Tonnerre wrote:
Commit 16fc32f introduced an error in include/environment.h, which makes u-boot fail to compile due to a missing #endif. This also fixes a merge conflict remaining in include/configs/EB+MCF-EV123.h which was caused by the same commit
this is only an issue for the "next" branch ...
my local git tree has the right number of #endif's, so i guess i must have sent out the wrong patch
guess this needs to get squashed into the relevant change rather than added as a new commit. thanks for the update.
How exactly would you do that? The commit actually got pushed, and AFAIK next doesn't get rebased
Regards,
Dear Albin Tonnerre,
In message 20090825102110.GE10327@pc-ras4041.res.insa you wrote:
guess this needs to get squashed into the relevant change rather than added as a new commit. thanks for the update.
How exactly would you do that? The commit actually got pushed, and AFAIK next doesn't get rebased
It gets rebased when there is a need to do so. This seems to be such a case.
Best regards,
Wolfgang Denk
Dear Albin Tonnerre,
In message 1251189967-11702-1-git-send-email-albin.tonnerre@free-electrons.com you wrote:
Commit 16fc32f introduced an error in include/environment.h, which makes u-boot fail to compile due to a missing #endif. This also fixes a merge conflict remaining in include/configs/EB+MCF-EV123.h which was caused by the same commit
Signed-off-by: Albin Tonnerre albin.tonnerre@free-electrons.com
include/configs/EB+MCF-EV123.h | 7 ------- include/environment.h | 1 + 2 files changed, 1 insertions(+), 7 deletions(-)
Applied to "next" branch (and rebased "next" to clean up the mess).
Thanks for catching / fixing this.
Best regards,
Wolfgang Denk
participants (3)
-
Albin Tonnerre -
Mike Frysinger -
Wolfgang Denk