[U-Boot] [RFC PATCH 1/2] nand: allow delayed initialization
Many people like the current nand_init() behavior where it is always initialized during boot and the flash size shown, but there are cases where we are willing to forgo this niceness for speed/functionality. So rather than change the default, introduce a delayed config option people may enable. This way the nand is only poked when someone tries to actually use it.
Signed-off-by: Mike Frysinger vapier@gentoo.org --- note: i havent documented this in the README yet as i want to get feedback on the approach first
common/cmd_nand.c | 2 ++ common/env_nand.c | 8 ++++++++ drivers/mtd/nand/nand.c | 9 +++++++++ include/nand.h | 5 +++++ 4 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/common/cmd_nand.c b/common/cmd_nand.c index f611fd7..ed3ca54 100644 --- a/common/cmd_nand.c +++ b/common/cmd_nand.c @@ -222,6 +222,8 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) if (argc < 2) goto usage;
+ nand_delayed_init(); + if (quiet_str) quiet = simple_strtoul(quiet_str, NULL, 0) != 0;
diff --git a/common/env_nand.c b/common/env_nand.c index 99f661e..e8be67b 100644 --- a/common/env_nand.c +++ b/common/env_nand.c @@ -307,6 +307,8 @@ void env_relocate_spec (void) return use_default(); }
+ nand_delayed_init(); + if (readenv(CONFIG_ENV_OFFSET, (u_char *) tmp_env1)) puts("No Valid Environment Area Found\n"); if (readenv(CONFIG_ENV_OFFSET_REDUND, (u_char *) tmp_env2)) @@ -347,6 +349,8 @@ void env_relocate_spec (void) free(tmp_env1); }
+#else + nand_delayed_init(); #endif /* ! ENV_IS_EMBEDDED */ } #else /* ! CONFIG_ENV_OFFSET_REDUND */ @@ -359,12 +363,16 @@ void env_relocate_spec (void) #if !defined(ENV_IS_EMBEDDED) int ret;
+ nand_delayed_init(); + ret = readenv(CONFIG_ENV_OFFSET, (u_char *) env_ptr); if (ret) return use_default();
if (crc32(0, env_ptr->data, ENV_SIZE) != env_ptr->crc) return use_default(); +#else + nand_delayed_init(); #endif /* ! ENV_IS_EMBEDDED */ } #endif /* CONFIG_ENV_OFFSET_REDUND */ diff --git a/drivers/mtd/nand/nand.c b/drivers/mtd/nand/nand.c index 47d6872..42ec40a 100644 --- a/drivers/mtd/nand/nand.c +++ b/drivers/mtd/nand/nand.c @@ -81,6 +81,15 @@ void nand_init(void) { int i; unsigned int size = 0; + +#ifdef CONFIG_SYS_NAND_DELAYED_INIT + static uint8_t initialized; + if (initialized) + return; + initialized = 1; + printf("Delayed NAND init: "); +#endif + for (i = 0; i < CONFIG_SYS_MAX_NAND_DEVICE; i++) { nand_init_chip(&nand_info[i], &nand_chip[i], base_address[i]); size += nand_info[i].size / 1024; diff --git a/include/nand.h b/include/nand.h index 2a81597..939b8e7 100644 --- a/include/nand.h +++ b/include/nand.h @@ -25,6 +25,11 @@ #define _NAND_H_
extern void nand_init(void); +#ifdef CONFIG_SYS_NAND_DELAYED_INIT +# define nand_delayed_init() nand_init() +#else +# define nand_delayed_init() do { } while (0) +#endif
#include <linux/mtd/compat.h> #include <linux/mtd/mtd.h>
Signed-off-by: Mike Frysinger vapier@gentoo.org --- arch/blackfin/lib/board.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/blackfin/lib/board.c b/arch/blackfin/lib/board.c index 7649f9a..02a5ee9 100644 --- a/arch/blackfin/lib/board.c +++ b/arch/blackfin/lib/board.c @@ -346,7 +346,7 @@ void board_init_r(gd_t * id, ulong dest_addr) bd->bi_flashoffset = 0; #endif
-#ifdef CONFIG_CMD_NAND +#if defined(CONFIG_CMD_NAND) && !defined(CONFIG_SYS_NAND_DELAYED_INIT) puts("NAND: "); nand_init(); /* go init the NAND */ #endif
Many people like the current nand_init() behavior where it is always initialized during boot and the flash size shown, but there are cases where we are willing to forgo this niceness for speed/functionality. So rather than change the default, introduce a delayed config option people may enable. This way the nand is only poked when someone tries to actually use it.
Signed-off-by: Mike Frysinger vapier@gentoo.org --- v2 - update to current mainline
common/cmd_nand.c | 2 ++ common/env_nand.c | 8 ++++++++ drivers/mtd/nand/nand.c | 9 +++++++++ include/nand.h | 5 +++++ 4 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/common/cmd_nand.c b/common/cmd_nand.c index 3f1d077..5409382 100644 --- a/common/cmd_nand.c +++ b/common/cmd_nand.c @@ -309,6 +309,8 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) goto usage;
+ nand_delayed_init(); + if (quiet_str) quiet = simple_strtoul(quiet_str, NULL, 0) != 0;
diff --git a/common/env_nand.c b/common/env_nand.c index 4e8307a..3d80510 100644 --- a/common/env_nand.c +++ b/common/env_nand.c @@ -359,6 +359,8 @@ void env_relocate_spec(void) return; }
+ nand_delayed_init(); + if (readenv(CONFIG_ENV_OFFSET, (u_char *) tmp_env1)) puts("No Valid Environment Area found\n");
@@ -404,6 +406,8 @@ void env_relocate_spec(void) free(tmp_env1); free(tmp_env2);
+#else + nand_delayed_init(); #endif /* ! ENV_IS_EMBEDDED */ } #else /* ! CONFIG_ENV_OFFSET_REDUND */ @@ -418,6 +422,8 @@ void env_relocate_spec (void) int ret; char buf[CONFIG_ENV_SIZE];
+ nand_delayed_init(); + #if defined(CONFIG_ENV_OFFSET_OOB) ret = get_nand_env_oob(&nand_info[0], &nand_env_oob_offset); /* @@ -439,6 +445,8 @@ void env_relocate_spec (void) }
env_import(buf, 1); +#else + nand_delayed_init(); #endif /* ! ENV_IS_EMBEDDED */ } #endif /* CONFIG_ENV_OFFSET_REDUND */ diff --git a/drivers/mtd/nand/nand.c b/drivers/mtd/nand/nand.c index 47d6872..42ec40a 100644 --- a/drivers/mtd/nand/nand.c +++ b/drivers/mtd/nand/nand.c @@ -81,6 +81,15 @@ void nand_init(void) { int i; unsigned int size = 0; + +#ifdef CONFIG_SYS_NAND_DELAYED_INIT + static uint8_t initialized; + if (initialized) + return; + initialized = 1; + printf("Delayed NAND init: "); +#endif + for (i = 0; i < CONFIG_SYS_MAX_NAND_DEVICE; i++) { nand_init_chip(&nand_info[i], &nand_chip[i], base_address[i]); size += nand_info[i].size / 1024; diff --git a/include/nand.h b/include/nand.h index 8bdf419..74ef73f 100644 --- a/include/nand.h +++ b/include/nand.h @@ -25,6 +25,11 @@ #define _NAND_H_
extern void nand_init(void); +#ifdef CONFIG_SYS_NAND_DELAYED_INIT +# define nand_delayed_init() nand_init() +#else +# define nand_delayed_init() do { } while (0) +#endif
#include <linux/mtd/compat.h> #include <linux/mtd/mtd.h>
Dear Mike Frysinger,
In message 1286048840-1901-1-git-send-email-vapier@gentoo.org you wrote:
Many people like the current nand_init() behavior where it is always initialized during boot and the flash size shown, but there are cases where we are willing to forgo this niceness for speed/functionality. So rather than change the default, introduce a delayed config option people may enable. This way the nand is only poked when someone tries to actually use it.
Signed-off-by: Mike Frysinger vapier@gentoo.org
...
extern void nand_init(void); +#ifdef CONFIG_SYS_NAND_DELAYED_INIT +# define nand_delayed_init() nand_init() +#else +# define nand_delayed_init() do { } while (0) +#endif
Would it not be esier to rename your nand_delayed_init() into nand_init(), and add a "#ifndef CONFIG_SYS_NAND_DELAYED_INIT" around the current call to nand_init()?
Question: is there a risk of problems with boards that have the environment in NAND?
Best regards,
Wolfgang Denk
On Sunday, October 03, 2010 14:27:13 Wolfgang Denk wrote:
Mike Frysinger wrote:
Many people like the current nand_init() behavior where it is always initialized during boot and the flash size shown, but there are cases where we are willing to forgo this niceness for speed/functionality. So rather than change the default, introduce a delayed config option people may enable. This way the nand is only poked when someone tries to actually use it.
extern void nand_init(void);
+#ifdef CONFIG_SYS_NAND_DELAYED_INIT +# define nand_delayed_init() nand_init() +#else +# define nand_delayed_init() do { } while (0) +#endif
Would it not be esier to rename your nand_delayed_init() into nand_init(), and add a "#ifndef CONFIG_SYS_NAND_DELAYED_INIT" around the current call to nand_init()?
nand_init() cant handle being called multiple times. and i need to add more nand_init() points that only apply to when things are delayed. so when delayed init is not enabled (the default), there is no change in compiled code size.
Question: is there a risk of problems with boards that have the environment in NAND?
that's why my patch adds delayed init points to the major nand env entry points. my understanding is that these must be called before the env read/write funcs may be called. -mike
Dear Mike Frysinger,
In message 201010031632.47732.vapier@gentoo.org you wrote:
Would it not be esier to rename your nand_delayed_init() into nand_init(), and add a "#ifndef CONFIG_SYS_NAND_DELAYED_INIT" around the current call to nand_init()?
nand_init() cant handle being called multiple times. and i need to add more nand_init() points that only apply to when things are delayed. so when delayed init is not enabled (the default), there is no change in compiled code size.
Well, you have this in your new nand_init() code:
+ static uint8_t initialized; + if (initialized) + return; + initialized = 1;
Why cannot we call nand_init() multiple times, then?
Question: is there a risk of problems with boards that have the environment in NAND?
that's why my patch adds delayed init points to the major nand env entry points. my understanding is that these must be called before the env read/write funcs may be called.
OK, just wanted to make sure.
Best regards,
Wolfgang Denk
On Sunday, October 03, 2010 17:40:32 Wolfgang Denk wrote:
Mike Frysinger wrote:
Would it not be esier to rename your nand_delayed_init() into nand_init(), and add a "#ifndef CONFIG_SYS_NAND_DELAYED_INIT" around the current call to nand_init()?
nand_init() cant handle being called multiple times. and i need to add more nand_init() points that only apply to when things are delayed. so when delayed init is not enabled (the default), there is no change in compiled code size.
Well, you have this in your new nand_init() code:
- static uint8_t initialized;
- if (initialized)
return;- initialized = 1;
Why cannot we call nand_init() multiple times, then?
because of the whole of my statement. i did not want to affect code size if this option was disabled.
in a preious patch, i had the env code doing: #ifdef CONFIG_SYS_NAND_DELAYED_INIT nand_init(); #endif
but i thought it made more sense to rework it so the #ifdef existed in one place (the header) and not in the source. -mike
Dear Mike Frysinger,
In message 201010031819.35739.vapier@gentoo.org you wrote:
Why cannot we call nand_init() multiple times, then?
because of the whole of my statement. i did not want to affect code size if this option was disabled.
in a preious patch, i had the env code doing: #ifdef CONFIG_SYS_NAND_DELAYED_INIT nand_init(); #endif
but i thought it made more sense to rework it so the #ifdef existed in one> place (the header) and not in the source.
I'm not sure where we are with this.
Do you plan to post an update?
Best regards,
Wolfgang Denk
On Wednesday, October 06, 2010 16:40:47 Wolfgang Denk wrote:
I'm not sure where we are with this.
Do you plan to post an update?
there isnt a clear indication of where to take this. seems like we want to do this, and we want it as the default moving forward, but we want all existing boards to be unchanged. so only reasonable way would be to invert the logic, add a define for the arch lib/board.c files, and then add that define to all existing boards.
call the new define CONFIG_NAND_EARLY_INIT ? -mike
Dear Mike Frysinger,
In message 201010071300.28379.vapier@gentoo.org you wrote:
Do you plan to post an update?
there isnt a clear indication of where to take this. seems like we want to do this, and we want it as the default moving forward, but we want all existing boards to be unchanged. so only reasonable way would be to invert the logic, add a define for the arch lib/board.c files, and then add that define to all existing boards.
I don't think we want to modify 550+ Board configurations and re-test on that many boards...
I think we should rather enable the new feature by some #define, and recommend to enable this on new boards.
call the new define CONFIG_NAND_EARLY_INIT ?
Fine with me.
Best regards,
Wolfgang Denk
On Thursday, October 07, 2010 15:35:44 Wolfgang Denk wrote:
Mike Frysinger wrote:
Do you plan to post an update?
there isnt a clear indication of where to take this. seems like we want to do this, and we want it as the default moving forward, but we want all existing boards to be unchanged. so only reasonable way would be to invert the logic, add a define for the arch lib/board.c files, and then add that define to all existing boards.
I don't think we want to modify 550+ Board configurations and re-test on that many boards...
it would be ~100 boards. board_init() is only called when CONFIG_CMD_NAND is defined. so it should be as simple as: sed -i \ '/define[[:space:]]*CONFIG_CMD_NAND/i#define CONFIG_NAND_EARLY_INIT' \ include/configs/*
I think we should rather enable the new feature by some #define, and recommend to enable this on new boards.
problem with recommendations is that people dont notice them -mike
On Thursday, October 07, 2010 17:26:55 Mike Frysinger wrote:
On Thursday, October 07, 2010 15:35:44 Wolfgang Denk wrote:
Mike Frysinger wrote:
Do you plan to post an update?
there isnt a clear indication of where to take this. seems like we want to do this, and we want it as the default moving forward, but we want all existing boards to be unchanged. so only reasonable way would be to invert the logic, add a define for the arch lib/board.c files, and then add that define to all existing boards.
I don't think we want to modify 550+ Board configurations and re-test on that many boards...
it would be ~100 boards. board_init() is only called when CONFIG_CMD_NAND is defined. so it should be as simple as: sed -i \ '/define[[:space:]]*CONFIG_CMD_NAND/i#define CONFIG_NAND_EARLY_INIT' \ include/configs/*
I think we should rather enable the new feature by some #define, and recommend to enable this on new boards.
problem with recommendations is that people dont notice them
hmm, what about this scheme: - add NAND_MAYBE_EARLY_INIT to include/config_defaults.h - have nand_init() emit a #warning if NAND_MAYBE_EARLY_INIT is defined but NAND_EARLY_INIT is not - board porters add either "#define NAND_EARLY_INIT" or "#undef NAND_MAYBE_EARLY_INIT" to their board config - after a release or two, we set "#define NAND_EARLY_INIT" to any boards where their maintainers did not step up and drop "NAND_MAYBE_EARLY_INIT" totally
this way, existing behavior is retained, board porters have an incentive to choose the desired behavior themselves (kill the #warning), and we have confidence that we didnt break (most) people. -mike
On Thu, Oct 7, 2010 at 10:00 PM, Mike Frysinger wrote:
On Thursday, October 07, 2010 17:26:55 Mike Frysinger wrote:
On Thursday, October 07, 2010 15:35:44 Wolfgang Denk wrote:
Mike Frysinger wrote:
Do you plan to post an update?
there isnt a clear indication of where to take this. seems like we want to do this, and we want it as the default moving forward, but we want all existing boards to be unchanged. so only reasonable way would be to invert the logic, add a define for the arch lib/board.c files, and then add that define to all existing boards.
I don't think we want to modify 550+ Board configurations and re-test on that many boards...
it would be ~100 boards. board_init() is only called when CONFIG_CMD_NAND is defined. so it should be as simple as: sed -i \ '/define[[:space:]]*CONFIG_CMD_NAND/i#define CONFIG_NAND_EARLY_INIT' \ include/configs/*
I think we should rather enable the new feature by some #define, and recommend to enable this on new boards.
problem with recommendations is that people dont notice them
hmm, what about this scheme: - add NAND_MAYBE_EARLY_INIT to include/config_defaults.h - have nand_init() emit a #warning if NAND_MAYBE_EARLY_INIT is defined but NAND_EARLY_INIT is not - board porters add either "#define NAND_EARLY_INIT" or "#undef NAND_MAYBE_EARLY_INIT" to their board config - after a release or two, we set "#define NAND_EARLY_INIT" to any boards where their maintainers did not step up and drop "NAND_MAYBE_EARLY_INIT" totally
this way, existing behavior is retained, board porters have an incentive to choose the desired behavior themselves (kill the #warning), and we have confidence that we didnt break (most) people.
btw, i'm waiting for you to OK this path before i code it up and post new patches ;) -mike
Dear Mike Frysinger,
In message AANLkTikYwZ+G6+LDsfe0rhzmETcCatGMgQj=gK8zgebi@mail.gmail.com you wrote:
this way, existing behavior is retained, board porters have an incentive to choose the desired behavior themselves (kill the #warning), and we have confidence that we didnt break (most) people.
btw, i'm waiting for you to OK this path before i code it up and post new patches ;)
Yes, this is OK with me. [Please add an entry to doc/feature-removal-schedule.txt as well.]
Best regards,
Wolfgang Denk
On Sat, 2 Oct 2010 15:47:20 -0400 Mike Frysinger vapier@gentoo.org wrote:
+#ifdef CONFIG_SYS_NAND_DELAYED_INIT
Seems like this should be CONFIG_ rather than CONFIG_SYS_.
I'm wondering if we should just make this the default behavior. We already deferred the bad block scanning, perhaps this could replace that.
-Scott
On Monday, October 04, 2010 13:36:14 Scott Wood wrote:
I'm wondering if we should just make this the default behavior. We already deferred the bad block scanning, perhaps this could replace that.
i would like that, but Wolfgang indicated in the past that the default behavior of detecting the nand flash & printing out its size was desirable. perhaps that stance is flexible ... -mike
On Tue, 5 Oct 2010 04:08:12 -0400 Mike Frysinger vapier@gentoo.org wrote:
On Monday, October 04, 2010 13:36:14 Scott Wood wrote:
I'm wondering if we should just make this the default behavior. We already deferred the bad block scanning, perhaps this could replace that.
i would like that, but Wolfgang indicated in the past that the default behavior of detecting the nand flash & printing out its size was desirable. perhaps that stance is flexible ...
So much for "U-Boot doesn't initialize hardware that it doesn't use." :-)
-Scott
Dear Scott Wood,
In message 20101005113126.215b5df6@udp111988uds.am.freescale.net you wrote:
On Tue, 5 Oct 2010 04:08:12 -0400 Mike Frysinger vapier@gentoo.org wrote:
On Monday, October 04, 2010 13:36:14 Scott Wood wrote:
I'm wondering if we should just make this the default behavior. We already deferred the bad block scanning, perhaps this could replace that.
i would like that, but Wolfgang indicated in the past that the default behavior of detecting the nand flash & printing out its size was desirable. perhaps that stance is flexible ...
So much for "U-Boot doesn't initialize hardware that it doesn't use." :-)
U-Boot has a > 10 year long history. In that time we learned a lesson or two, and some things would today not be accepted any more that once seemed to be pretty innocent or even a clever thing.
I think the delayed initialization of NAND (or should we rather call it "on demand initialization" ?) is indeed a good thing, and a definite improvement.
If possible, I would like to see this the default setting for all new board to be added. Existing boards should have the option to select this behaviour, too.
What I do not want to do (if it can be avoided) is change the default behaviour of existing boards, i. e. I would like to leave this decision to the respective board maintainers (in the active opt-in sense).
Best regards,
Wolfgang Denk
On Tue, 5 Oct 2010 20:27:43 +0200 Wolfgang Denk wd@denx.de wrote:
If possible, I would like to see this the default setting for all new board to be added. Existing boards should have the option to select this behaviour, too.
What I do not want to do (if it can be avoided) is change the default behaviour of existing boards, i. e. I would like to leave this decision to the respective board maintainers (in the active opt-in sense).
Configurability and retaining existing behavior is good, but too much of it can leave the code a mess, especially when you get semi-redundant alternatives such as delaying everything or delaying just the bad block scan. Plus things like optional informative probing might be better triggered by a script rather than an ifdef.
I don't care that much either way, though -- if you want to keep this one as an ifdef, it's fine with me.
-Scott
participants (3)
-
Mike Frysinger -
Scott Wood -
Wolfgang Denk