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
