[U-Boot-Users] [PATCH]env_nand.c Added bad block management for environment variables

[Scott Wood]

Stuart Wood wrote:
> Scott,
> 
> Here is a new version of my patch to env_nand.c. Thanks for the good comments.
> Fixed a problem with the new code that allowed it to read a
> environment area even
> if it contained a bad block after the good environment data.

Please put comments such as these that don't belong in the commit 
message below a --- line, with the commit message and a signed-off-by 
above it.

> diff --git a/common/env_nand.c b/common/env_nand.c
> index 49742f5..3ee42e0 100644
> --- a/common/env_nand.c
> +++ b/common/env_nand.c
> @@ -1,4 +1,7 @@
>  /*
> + * (C) Copywrite 2008
> + * Stuart Wood, Lab X Technologies <stuart.wood at labxtechnologies.com>
> + *
>   * (C) Copyright 2004

It's spelled "Copyright" (as in the "right" to "copy").
Just like the one below it. :-)

>   * Jian Zhang, Texas Instruments, jzhang at ti.com.
> 
> @@ -53,6 +56,10 @@
>  #error CONFIG_INFERNO not supported yet
>  #endif
> 
> +#ifndef CFG_ENV_RANGE
> +#define CFG_ENV_RANGE	CFG_ENV_SIZE
> +#endif
> +
>  int nand_legacy_rw (struct nand_chip* nand, int cmd,
>  	    size_t start, size_t len,
>  	    size_t * retlen, u_char * buf);
> @@ -152,30 +159,57 @@ int env_init(void)
>   * nand_dev_desc + 0. This is also the behaviour using the new NAND code.
>   */
>  #ifdef CFG_ENV_OFFSET_REDUND
> +size_t erase_env(size_t offset)
> +{

erase_env() should not be within this ifdef.

> +	size_t end;
> +
> +	end = offset + CFG_ENV_RANGE;
> +
> +	while (offset < end && nand_erase(&nand_info[0],offset, CFG_ENV_SIZE))

Spaces after commas.

> +		offset += CFG_ENV_SIZE;
> +
> +	if (offset >= end)
> +		return 0;
> +
> +	return offset;
> +}

What if the offset is zero?  Should return -1 or something similar on error.

> @@ -208,10 +250,26 @@ int saveenv(void)
>  #endif /* CMD_SAVEENV */
> 
>  #ifdef CFG_ENV_OFFSET_REDUND
> +int check_env_size (size_t offset)
> +{

What about the non-redundant version of env_relocate_spec?
Also, this isn't checking the size, so it's not really an appropriate name.

> +	size_t end;
> +	int ret_val = 0;
> +	end = offset + CFG_ENV_SIZE;
> +
> +	for (; offset < end; offset += nand_info[0].erasesize) {
> +		if (nand_block_isbad(&nand_info[0],offset))
> +			ret_val = 1;
> +	}
> +
> +	return ret_val;

size_t end = offset + CFG_ENV_SIZE;

while (offset < end)
	if (nand_block_isbad(&nand_info[0], offset))
		return 1;

return 0;

> @@ -220,10 +278,27 @@ void env_relocate_spec (void)
>  	tmp_env1 = (env_t *) malloc(CFG_ENV_SIZE);
>  	tmp_env2 = (env_t *) malloc(CFG_ENV_SIZE);
> 
> -	nand_read(&nand_info[0], CFG_ENV_OFFSET, &total,
> -		  (u_char*) tmp_env1);
> -	nand_read(&nand_info[0], CFG_ENV_OFFSET_REDUND, &total,
> -		  (u_char*) tmp_env2);
> +	offset = CFG_ENV_OFFSET;
> +	end = offset + CFG_ENV_RANGE;
> +
> +	while (offset < end && check_env_size(offset)) {
> +		offset += CFG_ENV_SIZE;
> +	}
> +	if (offset >= end)
> +		puts("No Valid Environment Area Found\n");
> +	else
> +		nand_read(&nand_info[0], offset, &total, (u_char*) tmp_env1);

If the environment can span multiple blocks, we should be able to skip 
blocks internally rather than requiring contiguous good blocks.

-Scott