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

Scott Wood
2010-05-26T22:58:08Z

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?

> +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@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 <werner@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