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

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

[Thomas Chou]

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

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
+
+	/* 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;
 
 	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");
+#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;
 
 	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
+
+#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)
 	flash_protect (FLAG_PROTECT_SET,
 		       CONFIG_SYS_MONITOR_BASE,
 		       CONFIG_SYS_MONITOR_BASE + monitor_flash_len  - 1,
-- 
1.6.6.1

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

[Stefan Roese]

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

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

[Thomas Chou]

On 03/22/2010 07:05 PM, Stefan Roese wrote:
> 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.
>
>    
Hi Stefan,

Thank you for the tout links, because I was bitten too. I have updated 
the patch as you suggested. I will test it on boards and resend.

Best regards,
Thomas