| Message ID | 1286707060-7033-1-git-send-email-vapier@gentoo.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On Sun, Oct 10, 2010 at 06:37:40AM -0400, Mike Frysinger wrote: > diff --git a/common/cmd_nand.c b/common/cmd_nand.c > index 3f1d077..e0be7e5 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_init(); > + > if (quiet_str) > quiet = simple_strtoul(quiet_str, NULL, 0) != 0; > Also do_nandboot(). > diff --git a/common/env_nand.c b/common/env_nand.c > index 4e8307a..3dffebd 100644 > --- a/common/env_nand.c > +++ b/common/env_nand.c > @@ -359,6 +359,8 @@ void env_relocate_spec(void) > return; > } > > + nand_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_init(); > #endif /* ! ENV_IS_EMBEDDED */ Do we really need to initialize NAND if the environment is embedded, or could it be delayed to when the environment is saved? -Scott
On Monday, October 11, 2010 16:29:41 Scott Wood wrote: > On Sun, Oct 10, 2010 at 06:37:40AM -0400, Mike Frysinger wrote: > > --- a/common/cmd_nand.c > > +++ b/common/cmd_nand.c > > @@ -309,6 +309,8 @@ > > > > if (argc < 2) > > goto usage; > > > > + nand_init(); > > + > > Also do_nandboot(). does it need to be before the mtdparts init stuff, or can it be in nand_load_image() ? > > --- a/common/env_nand.c > > +++ b/common/env_nand.c > > @@ -359,6 +359,8 @@ void env_relocate_spec(void) > > return; > > } > > > > + nand_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_init(); > > > > #endif /* ! ENV_IS_EMBEDDED */ > > Do we really need to initialize NAND if the environment is embedded, or > could it be delayed to when the environment is saved? the reason i picked env_relocate_spec() is because i'd have to push the init into the read/write/save code paths. and those may be executed multiple times while running. the expectation is that if you're putting the env into nand, it's going to get read, so you might as well initialize it. -mike
On Mon, 11 Oct 2010 17:02:00 -0400 Mike Frysinger <vapier@gentoo.org> wrote: > On Monday, October 11, 2010 16:29:41 Scott Wood wrote: > > On Sun, Oct 10, 2010 at 06:37:40AM -0400, Mike Frysinger wrote: > > > --- a/common/cmd_nand.c > > > +++ b/common/cmd_nand.c > > > @@ -309,6 +309,8 @@ > > > > > > if (argc < 2) > > > goto usage; > > > > > > + nand_init(); > > > + > > > > Also do_nandboot(). > > does it need to be before the mtdparts init stuff, or can it be in > nand_load_image() ? I think before -- the mtdparts init checks whether an mtd device actually exists. Though in that case, and also for things like mtdparts.spread, nand_init() should go in mtdparts_init(), and also nand_load_image() in case mtdparts aren't enabled. > > > --- a/common/env_nand.c > > > +++ b/common/env_nand.c > > > @@ -359,6 +359,8 @@ void env_relocate_spec(void) > > > return; > > > } > > > > > > + nand_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_init(); > > > > > > #endif /* ! ENV_IS_EMBEDDED */ > > > > Do we really need to initialize NAND if the environment is embedded, or > > could it be delayed to when the environment is saved? > > the reason i picked env_relocate_spec() is because i'd have to push the init > into the read/write/save code paths. and those may be executed multiple times > while running. the expectation is that if you're putting the env into nand, > it's going to get read, so you might as well initialize it. If it's embedded, then you've already read the env out of NAND by whatever loaded the U-Boot image. You'd only need to initialize NAND if you're going to write it back, which is probably not the common case. In fact, I'm not sure when I'd use embedded-env with NAND at all unless the environment's completely read-only and thus can share an erase block with the u-boot image. OTOH, I'm fine with leaving that as a future refinement for someone who is using embedded-env with NAND and cares about the boot time it adds. -Scott
On Monday, October 11, 2010 17:27:33 Scott Wood wrote: > On Mon, 11 Oct 2010 17:02:00 -0400 Mike Frysinger wrote: > > On Monday, October 11, 2010 16:29:41 Scott Wood wrote: > > > On Sun, Oct 10, 2010 at 06:37:40AM -0400, Mike Frysinger wrote: > > > > --- a/common/cmd_nand.c > > > > +++ b/common/cmd_nand.c > > > > @@ -309,6 +309,8 @@ > > > > > > > > if (argc < 2) > > > > > > > > goto usage; > > > > > > > > + nand_init(); > > > > + > > > > > > Also do_nandboot(). > > > > does it need to be before the mtdparts init stuff, or can it be in > > nand_load_image() ? > > I think before -- the mtdparts init checks whether an mtd device > actually exists. Though in that case, and also for things > like mtdparts.spread, nand_init() should go in mtdparts_init(), and also > nand_load_image() in case mtdparts aren't enabled. np > OTOH, I'm fine with leaving that as a future refinement for someone who > is using embedded-env with NAND and cares about the boot time it adds. i'd lean towards this too. i have no way of testing this behavior. -mike
diff --git a/common/cmd_nand.c b/common/cmd_nand.c index 3f1d077..e0be7e5 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_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..3dffebd 100644 --- a/common/env_nand.c +++ b/common/env_nand.c @@ -359,6 +359,8 @@ void env_relocate_spec(void) return; } + nand_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_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_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_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..4a63d5c 100644 --- a/drivers/mtd/nand/nand.c +++ b/drivers/mtd/nand/nand.c @@ -81,6 +81,13 @@ void nand_init(void) { int i; unsigned int size = 0; + static uint8_t initialized; + + if (initialized) + return; + initialized = 1; + puts("NAND: "); + 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;
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 allow nand_init() to be called multiple times, and push this call down to the major NAND entry points rather than requiring the arch board files to call it all the time. Signed-off-by: Mike Frysinger <vapier@gentoo.org> --- common/cmd_nand.c | 2 ++ common/env_nand.c | 8 ++++++++ drivers/mtd/nand/nand.c | 7 +++++++ 3 files changed, 17 insertions(+), 0 deletions(-)