Thank you for the thorough review, Scott.
On Wed, May 26, 2010 at 6:58 PM, Scott Wood scottwood@freescale.com wrote:
On Mon, May 17, 2010 at 05:04:30PM -0400, Ben Gardiner wrote:
diff --git a/common/cmd_dynenv.c b/common/cmd_dynenv.c new file mode 100644 index 0000000..5167875 --- /dev/null +++ b/common/cmd_dynenv.c @@ -0,0 +1,112 @@ +/*
- (C) Copyright 2006-2007 OpenMoko, Inc.
- Author: Harald Welte laforge@openmoko.org
- (C) Copyright 2008 Harald Welte laforge@openmoko.org
Is this correct and up-to-date?
The code was taken from Harald's patches he submitted to the mailing list. I didn't do much to them except to re-factor the 'get' functionality. Should I have added myself / my organization to the copyright list? I figured my changes were not enough to warrant the additional attribution of copyright.
+unsigned long env_offset;
This is a pretty generic name for something NAND-specific -- as is "dynenv".
Maybe this should be a nand subcommand? Putting it in cmd_nand.c would also eliminate the need to export arg_off_size().
I'm glad you brought this up. I was wondering if the command might be better suited under the nand command -- I agree that it would eliminate the export of arg_off_size and all the renames, which would be very nice. Would 'nand dynenv' do? How about 'nand env.oob' ?
- if(mtd->oobavail < CONFIG_ENV_OFFSET_SIZE){
Please put a space after "if" and before the opening brace (in fact, there's no need for the braces at all on this one-liner).
will do
- printf("Insufficient available OOB bytes: %d OOB bytes available but %d required for dynenv support\n",mtd->oobavail,8);
Keep lines under 80 columns (both in source code and in output), and put spaces after commas.
sorry about that, I should know better :)
- }
- oob_buf = malloc(mtd->oobsize);
- if(!oob_buf)
- return -ENOMEM;
Let the user know it didn't work?
You only really need 8 bytes, why not just put it on the stack? Likewise in get_dynenv().
good ideas
- ops.datbuf = NULL;
- ops.mode = MTD_OOB_AUTO;
- ops.ooboffs = 0;
- ops.ooblen = CONFIG_ENV_OFFSET_SIZE;
- ops.oobbuf = (void *) oob_buf;
- ret = mtd->read_oob(mtd, CONFIG_ENV_OFFSET_SIZE, &ops);
- oob_buf[0] = ENV_OOB_MARKER;
- if (!ret) {
- if(addr & ~oob_buf[1]) {
- printf("ERROR: erase OOB block 0 to "
- "write this value\n");
You cannot erase OOB without erasing the entire block, so this message is a little confusing.
Do you really expect to make use of the ability to set a new address without erasing, if it only clears bits?
No, I suppose not. The main use-case seems to be 'dynenv set' on a freshly erased nand when using u-boot for programming. OTOH I have noticed that (one of) the nand erase utility that comes with the TI OMAP L138 EVM erases the NAND w/o erasing the OOB; so it was useful during testing to know if I was able to write the correct value to the OOB. I could replace this with a read-back check to accomplish the same task -- this will also make 'set' commit the new offset 'live' (see below)
- ret = mtd->write_oob(mtd, CONFIG_ENV_OFFSET_SIZE, &ops);
- if (!ret)
- CONFIG_ENV_OFFSET = addr;
- else {
- printf("Error writing OOB block 0\n");
- goto fail;
- }
If you put braces on one side of the else, put them on both.
- free(oob_buf);
- } else
- goto usage;
Likewise.
will do
Is there anything you can do on "dynenv set" so that the user won't have to reboot after setting the dynenv to be able to saveenv into the new environment?
Good catch. Yes I think this could also be handled by replacing the OOB bit-check with a call and check of the 'dynenv get' command after set. In this way the global variable that has been swapped inplace of CONFIG_ENV_OFFSET will have the value of the newly set dynamic environment.
+U_BOOT_CMD(dynenv, 4, 1, do_dynenv,
- "dynenv - dynamically placed (NAND) environment",
- "set off - set enviromnent offset\n"
- "dynenv get - get environment offset");
\ No newline at end of file
Please put a newline at the end of the file.
sorry I really should have caught that one.
diff --git a/common/cmd_nand.h b/common/cmd_nand.h new file mode 100644 index 0000000..023ed4f --- /dev/null +++ b/common/cmd_nand.h @@ -0,0 +1,33 @@ +/*
- cmd_nand.h - Convenience functions
- (C) Copyright 2006-2007 OpenMoko, Inc.
- Author: Werner Almesberger werner@openmoko.org
Is this really the right copyright/authorship for this file?
see answer above; but this file will be removed in the next version since I will stick the dynenv command under the nand command.
- if(!ret) {
- if(oob_buf[0] == ENV_OOB_MARKER) {
- *result = oob_buf[1];
Should probably encode the environment location as a block number, rather than as a byte, for flashes larger than 4GiB (there are other places in the environment handling where this won't work, but let's not add more).
good idea
- }
- else {
} else {
#ifdef CONFIG_ENV_OFFSET_REDUND void env_relocate_spec (void) { @@ -357,12 +398,23 @@ void env_relocate_spec (void) #if !defined(ENV_IS_EMBEDDED) int ret;
+#if defined(CONFIG_ENV_OFFSET_OOB)
- ret = get_dynenv(&CONFIG_ENV_OFFSET);
Taking the address of a macro looks really weird. This will only work if the macro is defined as env_offset, so why not just use env_offset?
Yeah... it's pretty weird, I'll subs env_offset here. :)
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();
#endif /* ! ENV_IS_EMBEDDED */
Unrelated whitespace change, please leave out.
will do
#endif /* CONFIG_ENV_OFFSET_REDUND */ diff --git a/include/environment.h b/include/environment.h index b9924fd..03b6c92 100644 --- a/include/environment.h +++ b/include/environment.h @@ -74,15 +74,24 @@ #endif /* CONFIG_ENV_IS_IN_FLASH */
#if defined(CONFIG_ENV_IS_IN_NAND) -# ifndef CONFIG_ENV_OFFSET -# error "Need to define CONFIG_ENV_OFFSET when using CONFIG_ENV_IS_IN_NAND" -# endif +# if defined(CONFIG_ENV_OFFSET_OOB) +# ifdef CONFIG_ENV_OFFSET_REDUND +# error "CONFIG_ENV_OFFSET_REDUND is not supported when CONFIG_ENV_OFFSET_OOB is set" +# endif +extern unsigned long env_offset; +# undef CONFIG_ENV_OFFSET +# define CONFIG_ENV_OFFSET env_offset
Don't undef CONFIG_ENV_OFFSET, we want the build to fail if the user tries to do it both ways (just like if CONFIG_ENV_OFFSET_REDUND is specified).
will do
+#define CONFIG_ENV_OFFSET_SIZE 8
Why is this configurable?
Can't think of a good reason. I'll just make a #define in a restricted scope to avoid magic number 8.
Best Regards, Ben Gardiner