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

[Scott Wood]

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?

> +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 at 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 at 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