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

[Stefan Roese <sr@denx.de>]

Hi Thomas,

On Friday 19 March 2010 23:57:47 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

Thanks. Please find some comments below.
 
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
>  drivers/mtd/cfi_flash.c |   94
> ++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 92
> insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index fdba297..4baa9dc 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -555,6 +555,50 @@ 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, cfiword_t cword, void
> *addr, +			     ulong tout, char *prompt)
> +{
> +	ulong start;
> +	int ready;
> +
> +#if CONFIG_SYS_HZ != 1000
> +	tout *= CONFIG_SYS_HZ/1000;
> +#endif

I realise that this is copied from flash_status_check(). There has been a 
discussion and a not yet finished patch for this part. Please take a look at 
this thread:

http://www.mail-archive.com/u-boot at lists.denx.de/msg19297.html

And the last version was posted here, with still a smaller change to be made:

http://www.mail-archive.com/u-boot at lists.denx.de/msg20515.html

It would be great if you could handle this in your patch as suggested in this 
email thread.

> +	/* Wait for command completion */
> +	start = get_timer(0);
> +	while (1) {
> +		switch (info->portwidth) {
> +		case FLASH_CFI_8BIT:
> +			ready = flash_read8(addr) == cword.c;
> +			break;
> +		case FLASH_CFI_16BIT:
> +			ready = flash_read16(addr) == cword.w;
> +			break;
> +		case FLASH_CFI_32BIT:
> +			ready = flash_read32(addr) == cword.l;
> +			break;
> +		case FLASH_CFI_64BIT:
> +			ready = flash_read64(addr) == cword.ll;
> +			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)addr, (ulong)flash_read8(addr));
> +			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 +793,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");
>  }
> 
> @@ -767,6 +818,7 @@ static int flash_write_cfibuffer (flash_info_t * info,
> ulong dest, uchar * cp, uint offset = 0;
>  	unsigned int shift;
>  	uchar write_cmd;
> +	cfiword_t cword;

Won't this result in a "unused variable" warning, when 
CONFIG_SYS_CFI_FLASH_STATUS_POLL is not defined?

>  	switch (info->portwidth) {
>  	case FLASH_CFI_8BIT:
> @@ -911,9 +963,35 @@ 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
> +		switch (info->portwidth) {
> +		case FLASH_CFI_8BIT:
> +			cword.c = flash_read8(src - 1);
> +			dst -= 1;
> +			break;
> +		case FLASH_CFI_16BIT:
> +			cword.w = flash_read16(src - 2);
> +			dst -= 2;
> +			break;
> +		case FLASH_CFI_32BIT:
> +			cword.l = flash_read32(src - 4);
> +			dst -= 4;
> +			break;
> +		case FLASH_CFI_64BIT:
> +			cword.ll = flash_read64(src - 8);
> +			dst -= 8;
> +			break;
> +		default:
> +			retcode = ERR_INVAL;
> +			goto out_unmap;
> +		}
> +		retcode = flash_status_poll(info, cword, dst,
> +				info->buffer_write_tout, "buffer write");

Hmmm. I don't like this extra switch statement here. Thinking more about it, 
wouldn't it make more sense to add this code into the new flash_status_poll() 
functions instead and don't pass cword at all?

> +#else
>  		retcode = flash_full_status_check (info, sector,
>  						   info->buffer_write_tout,
>  						   "buffer write");
> +#endif
>  		break;
> 
>  	default:
> @@ -935,6 +1013,8 @@ int flash_erase (flash_info_t * info, int s_first, int
> s_last) int rcode = 0;
>  	int prot;
>  	flash_sect_t sect;
> +	cfiword_t cword = (cfiword_t) 0xffffffffffffffffULL;
> +	ulong dest;

Again, doesn't this introduce compilation warnings?

> 
>  	if (info->flash_id != FLASH_MAN_CFI) {
>  		puts ("Can't erase unknown flash type - aborted\n");
> @@ -998,6 +1078,17 @@ int flash_erase (flash_info_t * info, int s_first,
> int s_last) break;
>  			}
> 
> +#ifdef CONFIG_SYS_CFI_FLASH_STATUS_POLL
> +			dest = (ulong)flash_map(info, sect, 0);
> +			flash_unmap(info, sect, 0, (void *)dest);
> +			if ((info->vendor == CFI_CMDSET_AMD_EXTENDED ||
> +			     info->vendor == CFI_CMDSET_AMD_STANDARD) &&
> +			    flash_status_poll(info, cword, (void *)dest,
> +					      info->erase_blk_tout, "erase"))
> +				rcode = 1;
> +			else
> +

Please remove this empty line.

> +#endif
>  			if (flash_full_status_check
>  			    (info, sect, info->erase_blk_tout, "erase")) {
>  				rcode = 1;
> @@ -1980,8 +2071,7 @@ unsigned long flash_init (void)
>  	}
> 
>  	/* Monitor protection ON by default */
> -#if (CONFIG_SYS_MONITOR_BASE >= CONFIG_SYS_FLASH_BASE) && \
> -	(!defined(CONFIG_MONITOR_IS_IN_RAM))
> +#if (CONFIG_SYS_MONITOR_BASE >= CONFIG_SYS_FLASH_BASE)

This seems wrong. You probably based your patch on an older driver version. 
Please rebase against the latest version.

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