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 <lafo...@openmoko.org> > + * (C) Copyright 2008 Harald Welte <lafo...@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 <wer...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot