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

Ben Gardiner
2010-05-26T23:38:28Z

Thank you for the thorough review, Scott.

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