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?
+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().
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).
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.
}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().
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?
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.
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?
+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.
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?
+#ifndef CMD_NAND_H +#define CMD_NAND_H
+#include <nand.h>
+/* common/cmd_nand.c */ +int nand_arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off,
- ulong *size, int net);
Just put it in nand.h.
- 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).
}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?
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.
#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).
+#define CONFIG_ENV_OFFSET_SIZE 8
Why is this configurable?
-Scott