[U-Boot] [PATCH] cfi flash: add status polling method for amd flash

[Stefan Roese <sr@denx.de>]

Hi Thomas,

On Tuesday 23 March 2010 04:24:08 Thomas Chou wrote:
> This patch adds status polling method to offer an alternative to
> data toggle method for amd flash chips.
> 
> This patch is needed for nios2 cfi flash interface, where the bus
> controller performs 4 bytes read cycles for a single byte read
> instruction. The data toggle method can not detect chip busy
> status correctly. So we have to poll DQ7, which will be inverted
> when the chip is busy.
> 
> This feature is enabled with the config def,
> CONFIG_SYS_CFI_FLASH_STATUS_POLL
> 
> Include patch, "drivers/mtd/cfi_flash: precision and underflow
> problem in tout calculation", submitted by
>   Alessandro Rubini <rub...@gnudd.com>
>   Renato Andreola <renato.andre...@imagos.it>

Thanks for taking care of this. But we shouldn't really integrate this patch 
into your flash_status_poll() patch. Please re-send this part as a separate 
patch (some subject as original patch with S-o-b line of original author 
please). Thanks again for this.

Please find still more enhancement suggestions below.
 
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
>  drivers/mtd/cfi_flash.c |   78
> ++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 77
> insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index fdba297..db22255 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -537,7 +537,10 @@ static int flash_status_check (flash_info_t * info,
> flash_sect_t sector, ulong start;
> 
>  #if CONFIG_SYS_HZ != 1000
> -	tout *= CONFIG_SYS_HZ/1000;
> +	if ((ulong)CONFIG_SYS_HZ > 100000)
> +		tout *= (ulong)CONFIG_SYS_HZ / 1000;  /* for a big HZ, avoid 
overflow */
> +	else
> +		tout = DIV_ROUND_UP (tout * (ulong)CONFIG_SYS_HZ, 1000);
>  #endif
> 
>  	/* Wait for command completion */
> @@ -555,6 +558,53 @@ static int flash_status_check (flash_info_t * info,
> flash_sect_t sector, return ERR_OK;
>  }
> 
> +#ifdef CONFIG_SYS_CFI_FLASH_STATUS_POLL
> +static int flash_status_poll (flash_info_t *info, void *src, void *dst,
> +			     ulong tout, char *prompt)
> +{
> +	ulong start;
> +	int ready;
> +
> +#if CONFIG_SYS_HZ != 1000
> +	if ((ulong)CONFIG_SYS_HZ > 100000)
> +		tout *= (ulong)CONFIG_SYS_HZ / 1000;  /* for a big HZ, avoid 
overflow */
> +	else
> +		tout = DIV_ROUND_UP (tout * (ulong)CONFIG_SYS_HZ, 1000);
> +#endif
> +
> +	/* Wait for command completion */
> +	start = get_timer (0);
> +	while (1) {
> +		switch (info->portwidth) {
> +		case FLASH_CFI_8BIT:
> +			ready = flash_read8(dst) == flash_read8(src);
> +			break;
> +		case FLASH_CFI_16BIT:
> +			ready = flash_read16(dst) == flash_read16(src);
> +			break;
> +		case FLASH_CFI_32BIT:
> +			ready = flash_read32(dst) == flash_read32(src);
> +			break;
> +		case FLASH_CFI_64BIT:
> +			ready = flash_read64(dst) == flash_read64(src);
> +			break;
> +		default:
> +			ready = 0;
> +			break;
> +		}
> +		if (ready)
> +			break;
> +		if (get_timer (start) > tout) {
> +			printf ("Flash %s timeout at address %lx data %lx\n",
> +			       prompt, (ulong)dst, (ulong)flash_read8(dst));
> +			return ERR_TIMOUT;
> +		}
> +		udelay (1);		/* also triggers watchdog */
> +	}
> +	return ERR_OK;
> +}
> +#endif
> +
>  /*-----------------------------------------------------------------------
>   * Wait for XSR.7 to be set, if it times out print an error, otherwise
>   * do a full status check.
> @@ -749,6 +799,13 @@ static int flash_write_cfiword (flash_info_t * info,
> ulong dest, if (!sect_found)
>  		sect = find_sector (info, dest);
> 
> +#ifdef CONFIG_SYS_CFI_FLASH_STATUS_POLL
> +	if (info->vendor == CFI_CMDSET_AMD_EXTENDED ||
> +	    info->vendor == CFI_CMDSET_AMD_STANDARD)
> +		return flash_status_poll (info, &cword, dstaddr,
> +					 info->write_tout, "write");
> +	else
> +#endif
>  	return flash_full_status_check (info, sect, info->write_tout, 
"write");

I don't like this ugly "incorrect" indentation of the line below the "#endif" 
above. Perhaps you could implement this in another way. By moving the #ifdef 
and the vendor == amd check into a separate function:

static int use_flash_status_poll(flash_info_t *info)
{
#ifdef CONFIG_SYS_CFI_FLASH_STATUS_POLL
	if (info->vendor == CFI_CMDSET_AMD_EXTENDED ||
	    info->vendor == CFI_CMDSET_AMD_STANDARD)
		return 1;
#endif
	return 0;
}

And then use this code in flash_write_cfiword():

	if (use_flash_status_poll(info)) {
		return flash_status_poll(info, &cword, dstaddr,
					 info->write_tout, "write");
	} else {
		return flash_full_status_check(info, sect,
					       info->write_tout, "write");
	}

What do you think? Looks nicer to me. And we only need to implement this 
vendor == AMD check once (see below).

>  }
> 
> @@ -911,9 +968,14 @@ static int flash_write_cfibuffer (flash_info_t * info,
> ulong dest, uchar * cp, }
> 
>  		flash_write_cmd (info, sector, 0, 
AMD_CMD_WRITE_BUFFER_CONFIRM);
> +#ifdef CONFIG_SYS_CFI_FLASH_STATUS_POLL
> +		retcode = flash_status_poll (info, src - (1 << shift),
> +			dst - (1 << shift), info->buffer_write_tout, "buffer 
write");
> +#else
>  		retcode = flash_full_status_check (info, sector,
>  						   info->buffer_write_tout,
>  						   "buffer write");
> +#endif
>  		break;
> 
>  	default:
> @@ -935,6 +997,10 @@ int flash_erase (flash_info_t * info, int s_first, int
> s_last) int rcode = 0;
>  	int prot;
>  	flash_sect_t sect;
> +#ifdef CONFIG_SYS_CFI_FLASH_STATUS_POLL
> +	cfiword_t cword = (cfiword_t)0xffffffffffffffffULL;
> +	void *dest;
> +#endif
> 
>  	if (info->flash_id != FLASH_MAN_CFI) {
>  		puts ("Can't erase unknown flash type - aborted\n");
> @@ -998,6 +1064,16 @@ int flash_erase (flash_info_t * info, int s_first,
> int s_last) break;
>  			}
> 
> +#ifdef CONFIG_SYS_CFI_FLASH_STATUS_POLL
> +			dest = flash_map (info, sect, 0);
> +			flash_unmap (info, sect, 0, dest);
> +			if ((info->vendor == CFI_CMDSET_AMD_EXTENDED ||
> +			     info->vendor == CFI_CMDSET_AMD_STANDARD) &&
> +			    flash_status_poll (info, &cword, dest,
> +					      info->erase_blk_tout, "erase"))
> +				rcode = 1;
> +			else
> +#endif

Again, you could use the newly created function here.

Please give it a try and let me know if this works.

Thanks.

Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de