Thank you for the thorough review, Scott. On Wed, May 26, 2010 at 6:58 PM, Scott Wood <scottw...@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 <lafo...@openmoko.org> >> + * (C) Copyright 2008 Harald Welte <lafo...@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 <wer...@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 -- Ben Gardiner Nanometrics Inc. +1 (613) 592-6776 x239 http://www.nanometrics.ca _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot