[U-Boot] [PATCH] NAND: Support dynamic location of enviromnent (CONFIG_ENV_OFFSET_OOB)

[Ben Gardiner]

Thank you for the thorough review, Scott.

On Wed, May 26, 2010 at 6:58 PM, Scott Wood <scottwood at 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 at openmoko.org>
>> + * (C) Copyright 2008 Harald Welte <laforge at 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 at 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