From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Wed, 28 May 2008 12:57:04 -0500
Subject: [U-Boot-Users] [PATCH]env_nand.c Added bad block management	for
	environment variables
In-Reply-To: <c13b1cfc0805270701s38288252k5de057bd96d8c856@mail.gmail.com>
References: <c13b1cfc0805270701s38288252k5de057bd96d8c856@mail.gmail.com>
Message-ID: <20080528175704.GA3090@loki.buserror.net>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Tue, May 27, 2008 at 10:01:14AM -0400, Stuart Wood wrote:
> Hi All, This is my first attempt at submitting a change so please be
> patient and kind. This change allows for the environment variables to be
> stored in a rand of nand flash. If the first block is bad then the
> environment is stored in the next one, and so on. It introduces
> CFG_ENV_RANGE to define the size of the area that way contain the
> environment data. This will allow the environment to be loaded over an
> area with bad blocks from the factory without a problem.

This will break any board that doesn't define CFG_ENV_RANGE... it should
default to CFG_ENV_SIZE if unset.

>  	if(gd->env_valid == 1) {
> -		puts ("Erasing redundant Nand...");
> -		if (nand_erase(&nand_info[0],
> -			       CFG_ENV_OFFSET_REDUND, CFG_ENV_SIZE))
> +		puts ("Erasing redundant Nand...\n");
> +		for (offset = CFG_ENV_OFFSET_REDUND; offset <
> +				CFG_ENV_OFFSET_REDUND + CFG_ENV_RANGE; )
> +		{
> +			if (nand_erase(&nand_info[0],
> +					offset, CFG_ENV_SIZE)) {
> +				offset += CFG_ENV_SIZE;
> +			} else {
> +				break;
> +			}
> +		}

Please factor this code out instead of duplicating it for both environment
instances.

It'd also be nicer as something like this:

offset = CFG_ENV_REDUND;
end = offset + CFG_ENV_RANGE;

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

> @@ -189,15 +215,30 @@ int saveenv(void)
>  int saveenv(void)
>  {
>  	size_t total;
> +	size_t offset;
>  	int ret = 0;
> 
> -	puts ("Erasing Nand...");
> -	if (nand_erase(&nand_info[0], CFG_ENV_OFFSET, CFG_ENV_SIZE))
> +	if (CFG_ENV_RANGE < CFG_ENV_SIZE)
>  		return 1;
> +	puts ("Erasing Nand...\n");
> +	for (offset = CFG_ENV_OFFSET; offset <
> +			CFG_ENV_OFFSET + CFG_ENV_RANGE; )
> +	{
> +		if (nand_erase(&nand_info[0],
> +				offset, CFG_ENV_SIZE)) {
> +			offset += CFG_ENV_SIZE;
> +		} else {
> +			break;
> +		}
> +	}
> +	if (offset >= CFG_ENV_OFFSET + CFG_ENV_RANGE) {
> +		puts ("Nand area is completely bad!\n");
> +		return 1;
> +	}

It'd be nice if we could reduce the duplication between the two versions of
saveenv() as well.

> +	for (offset = CFG_ENV_OFFSET; offset < CFG_ENV_OFFSET + CFG_ENV_RANGE ; ) {
> +		if (nand_block_isbad (&nand_info[0], offset)) {
> +			offset += CFG_ENV_SIZE;
> +		} else {
> +			nand_read(&nand_info[0], offset, &total,
> +				(u_char*) tmp_env1);
> +			break;
> +		}
> +	}
> +	for (offset = CFG_ENV_OFFSET_REDUND; offset <
> +			CFG_ENV_OFFSET_REDUND + CFG_ENV_RANGE ; ) {
> +		if (nand_block_isbad (&nand_info[0], offset)) {
> +			offset += CFG_ENV_SIZE;
> +		} else {
> +			nand_read(&nand_info[0], offset, &total,
> +				(u_char*) tmp_env2);
> +			break;
> +		}
> +	}

Same comments as saveenv().

-Scott