[U-Boot] non-blocking flash functions - is this possible/acceptable?

[U-Boot] non-blocking flash functions - is this possible/acceptable?

[Wolfgang Wegner]

Hi,

we have an update protocol that normally relies on data being
received while the previous block is written to flash.

We hacked our U-Boot to provide non-blocking variants for flash
access for the relevant functions, which are:

flash_status_check_nb()
flash_full_status_check_nb()
flash_erase_nb() (single-sector only)
flash_write_cfibuffer_nb()
write_buff_nb()

Apart from flash_status_check_nb() and flash_erase_nb() (the latter
being reduced to handle only one sector at a time), these are mainly
the same functions as the originals, but use
flash_[full_]status_check_nb()
instead, so there is much duplicate code.

Is such a use case generally acceptable in U-Boot, and if so, does
anybody have an idea how to implement those without all this duplicate
code?
Of course I can also implement this stuff in our board code, but it
seems a bit unlogical to break the flash handling apart and the bloat
would remain, just in a different place.

[I am bringing this topic up because I am trying to prepare patches for
sending to the list, and this one seems to me as a real show-stopper
right now.]

Regards,
Wolfgang

[U-Boot] non-blocking flash functions - is this possible/acceptable?

[Jerry Van Baren]

Wolfgang Wegner wrote:
> Hi,
> 
> we have an update protocol that normally relies on data being
> received while the previous block is written to flash.
> 
> We hacked our U-Boot to provide non-blocking variants for flash
> access for the relevant functions, which are:
> 
> flash_status_check_nb()
> flash_full_status_check_nb()
> flash_erase_nb() (single-sector only)
> flash_write_cfibuffer_nb()
> write_buff_nb()
> 
> Apart from flash_status_check_nb() and flash_erase_nb() (the latter
> being reduced to handle only one sector at a time), these are mainly
> the same functions as the originals, but use
> flash_[full_]status_check_nb()
> instead, so there is much duplicate code.
> 
> Is such a use case generally acceptable in U-Boot, and if so,

I'll defer to Wolfgang Denk, Stefan Roese, (Scott Wood?), and others for 
this half.

My 2c: Overlapping data transfer with flash erase/write operations can 
be beneficial as it can reduce the programming time substantially. 
(Erase is less beneficial than write since erases don't happen as often 
and take a relatively long time, so the overlap optimization savings is 
a smaller percentage of the total erase time - Amdahl's Law.)

> anybody have an idea how to implement those without all this duplicate
> code?

Move the code to the non-blocking functions (which you have already 
done), and then implement the blocking versions as wrappers that simply 
call the non-blocking write followed by a loop calling 
flash_status_check_nb() until the write is complete.  For erase, you 
would need a loop to do the multi-sector erase as multiple non-blocking 
+ wait operations.

> Of course I can also implement this stuff in our board code, but it
> seems a bit unlogical to break the flash handling apart and the bloat
> would remain, just in a different place.

Yes, that would be horrible.

> [I am bringing this topic up because I am trying to prepare patches for
> sending to the list, and this one seems to me as a real show-stopper
> right now.]
> 
> Regards,
> Wolfgang

Best regards,
gvb

[U-Boot] non-blocking flash functions - is this possible/acceptable?

[Jerry Van Baren]

Jerry Van Baren wrote:
> Wolfgang Wegner wrote:
>> Hi,
>>
>> we have an update protocol that normally relies on data being
>> received while the previous block is written to flash.

[snip]

> My 2c: Overlapping data transfer with flash erase/write operations can 
> be beneficial as it can reduce the programming time substantially. 
> (Erase is less beneficial than write since erases don't happen as often 
> and take a relatively long time, so the overlap optimization savings is 
> a smaller percentage of the total erase time - Amdahl's Law.)

By the way, what sort of benefit do you see?  What is your load time 
with and without the non-blocking changes?

[snip]

>> Regards,
>> Wolfgang

Thanks,
gvb

[U-Boot] non-blocking flash functions - is this possible/acceptable?

[Wolfgang Wegner]

Hi Jerry,

On Tue, Oct 27, 2009 at 10:03:49AM -0400, Jerry Van Baren wrote:
> Jerry Van Baren wrote:
> >Wolfgang Wegner wrote:
[...]
> >>we have an update protocol that normally relies on data being
> >>received while the previous block is written to flash.
[...]
> 
> By the way, what sort of benefit do you see?  What is your load time 
> with and without the non-blocking changes?

I am not sure if I understand what you mean, or if we are talking
about different things.

Our update protocol starts the update and immediately starts sending
data over the (relatively slow) serial line. During the data is
transferred, the first flash block is erased (first operation "in
background"), and after the data for the complete flash block
arrived, this data is written to flash and the next block is erased
(again an operation "in background"), while the data transfer over
the serial line continues.

Of course, doing the whole erase & flash procedure after receiving
the complete bunch of data would solve this problem, but this update
protocol comes from time where there simply was not enough RAM to
store all the data before flashing, and we did not want to change
everything for this new device - and still, even erasing for and
programming the whole ~4MBytes of data would take some time which
can be hidden when erasing/programming each flash block during the
serial transfer, which is the real bottleneck of the whole procedure.

Regards,
Wolfgang

[U-Boot] non-blocking flash functions - is this possible/acceptable?

[Wolfgang Wegner]

On Tue, Oct 27, 2009 at 09:21:25AM -0400, Jerry Van Baren wrote:
> Wolfgang Wegner wrote:
> >Is such a use case generally acceptable in U-Boot, and if so,
> 
> I'll defer to Wolfgang Denk, Stefan Roese, (Scott Wood?), and others for 
> this half.

:-)

> >anybody have an idea how to implement those without all this duplicate
> >code?
> 
> Move the code to the non-blocking functions (which you have already 
> done), and then implement the blocking versions as wrappers that simply 
> call the non-blocking write followed by a loop calling 
> flash_status_check_nb() until the write is complete.  For erase, you 
> would need a loop to do the multi-sector erase as multiple non-blocking 
> + wait operations.

I will try this and see if it looks clean after implementing.

Thanks for the hint.

Regards,
Wolfgang

[U-Boot] non-blocking flash functions - is this possible/acceptable?

[Jerry Van Baren]

Wolfgang Wegner wrote:
> Hi Jerry,
> 
> On Tue, Oct 27, 2009 at 10:03:49AM -0400, Jerry Van Baren wrote:
>> Jerry Van Baren wrote:
>>> Wolfgang Wegner wrote:
> [...]
>>>> we have an update protocol that normally relies on data being
>>>> received while the previous block is written to flash.
> [...]
>> By the way, what sort of benefit do you see?  What is your load time 
>> with and without the non-blocking changes?
> 
> I am not sure if I understand what you mean, or if we are talking
> about different things.

Yes, you are addressing my question.  I was probing for your use case 
and results.

> Our update protocol starts the update and immediately starts sending
> data over the (relatively slow) serial line. During the data is
> transferred, the first flash block is erased (first operation "in
> background"), and after the data for the complete flash block
> arrived, this data is written to flash and the next block is erased
> (again an operation "in background"), while the data transfer over
> the serial line continues.

I was thinking in terms of TFTP - quite fast.  Your device is 
transferring the data it over the serial link - very slow.  This means 
you data transfer is slow even relative to a flash erase operation, so 
this gives a substantial speed improvement.

[snip]

> Regards,
> Wolfgang

Thanks,
gvb

[U-Boot] non-blocking flash functions - is this possible/acceptable?

[Wolfgang Denk]

Dear Wolfgang Wegner,

In message <20091027125146.GA3216@leila.ping.de> you wrote:
> 
> We hacked our U-Boot to provide non-blocking variants for flash
> access for the relevant functions, which are:
...
> Is such a use case generally acceptable in U-Boot, and if so, does
> anybody have an idea how to implement those without all this duplicate
> code?
> Of course I can also implement this stuff in our board code, but it
> seems a bit unlogical to break the flash handling apart and the bloat
> would remain, just in a different place.

I understand your use case, and why it makes sense to you (or at least
seems to make sense).

[U-Boot] [RFC PATCH] Implementation of non-blocking flash write/erase/status check functions.

[Wolfgang Wegner]

write_buff_nb() introduces quite an amount of duplicate code compared to
write_buff(), but I did not find an elegant solution to partition them.

Signed-off-by: Wolfgang Wegner <w.wegner@astro-kom.de>
---
 drivers/mtd/cfi_flash.c |  440 ++++++++++++++++++++++++++++++++++++++---------
 include/flash.h         |    3 +
 2 files changed, 365 insertions(+), 78 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 4e8f5bf..0f813b0 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -681,17 +681,13 @@ static int flash_status_check (flash_info_t * info, flash_sect_t sector,
 }
 
 /*-----------------------------------------------------------------------
- * Wait for XSR.7 to be set, if it times out print an error, otherwise
- * do a full status check.
+ * check retcode of flash_full_status_check[_nb]
  *
  * This routine sets the flash to read-array mode.
  */
-static int flash_full_status_check (flash_info_t * info, flash_sect_t sector,
-				    ulong tout, char *prompt)
+static int flash_full_status_retcode_check (flash_info_t * info, flash_sect_t sector,
+					    char *prompt, int retcode)
 {
-	int retcode;
-
-	retcode = flash_status_check (info, sector, tout, prompt);
 	switch (info->vendor) {
 	case CFI_CMDSET_INTEL_PROG_REGIONS:
 	case CFI_CMDSET_INTEL_EXTENDED:
@@ -728,6 +724,21 @@ static int flash_full_status_check (flash_info_t * info, flash_sect_t sector,
 }
 
 /*-----------------------------------------------------------------------
+ * Wait for XSR.7 to be set, if it times out print an error, otherwise
+ * do a full status check.
+ *
+ * This routine sets the flash to read-array mode.
+ */
+static int flash_full_status_check (flash_info_t * info, flash_sect_t sector,
+				    ulong tout, char *prompt)
+{
+	int retcode;
+
+	retcode = flash_status_check (info, sector, tout, prompt);
+	return flash_full_status_retcode_check (info, sector, prompt, retcode);
+}
+
+/*-----------------------------------------------------------------------
  */
 static void flash_add_byte (flash_info_t * info, cfiword_t * cword, uchar c)
 {
@@ -796,12 +807,11 @@ static flash_sect_t find_sector (flash_info_t * info, ulong addr)
 
 /*-----------------------------------------------------------------------
  */
-static int flash_write_cfiword (flash_info_t * info, ulong dest,
-				cfiword_t cword)
+static int flash_write_cfiword_stub (flash_info_t * info, ulong dest,
+				     cfiword_t cword, flash_sect_t *sect)
 {
 	void *dstaddr = (void *)dest;
 	int flag;
-	flash_sect_t sect = 0;
 	char sect_found = 0;
 
 	/* Check if Flash is (sufficiently) erased */
@@ -837,14 +847,14 @@ static int flash_write_cfiword (flash_info_t * info, ulong dest,
 		break;
 	case CFI_CMDSET_AMD_EXTENDED:
 	case CFI_CMDSET_AMD_STANDARD:
-		sect = find_sector(info, dest);
-		flash_unlock_seq (info, sect);
-		flash_write_cmd (info, sect, info->addr_unlock1, AMD_CMD_WRITE);
+		*sect = find_sector(info, dest);
+		flash_unlock_seq (info, *sect);
+		flash_write_cmd (info, *sect, info->addr_unlock1, AMD_CMD_WRITE);
 		sect_found = 1;
 		break;
 #ifdef CONFIG_FLASH_CFI_LEGACY
 	case CFI_CMDSET_AMD_LEGACY:
-		sect = find_sector(info, dest);
+		*sect = find_sector(info, dest);
 		flash_unlock_seq (info, 0);
 		flash_write_cmd (info, 0, info->addr_unlock1, AMD_CMD_WRITE);
 		sect_found = 1;
@@ -872,19 +882,31 @@ static int flash_write_cfiword (flash_info_t * info, ulong dest,
 		enable_interrupts ();
 
 	if (!sect_found)
-		sect = find_sector (info, dest);
+		*sect = find_sector (info, dest);
+
+	return 0;
+}
+
+static int flash_write_cfiword (flash_info_t * info, ulong dest,
+				cfiword_t cword)
+{
+	int retcode;
+	flash_sect_t sect = 0;
+
+	retcode = flash_write_cfiword_stub (info, dest, cword, &sect);
+	if (retcode)
+		return retcode;
 
 	return flash_full_status_check (info, sect, info->write_tout, "write");
 }
 
 #ifdef CONFIG_SYS_FLASH_USE_BUFFER_WRITE
 
-static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
-				  int len)
+static int flash_write_cfibuffer_stub (flash_info_t * info, ulong dest, uchar * cp,
+				       int len, flash_sect_t *sector)
 {
-	flash_sect_t sector;
 	int cnt;
-	int retcode;
+	int retcode = 0;
 	void *src = cp;
 	void *dst = (void *)dest;
 	void *dst2 = dst;
@@ -943,7 +965,7 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
 	}
 
 	src = cp;
-	sector = find_sector (info, dest);
+	*sector = find_sector (info, dest);
 
 	switch (info->vendor) {
 	case CFI_CMDSET_INTEL_PROG_REGIONS:
@@ -951,17 +973,17 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
 	case CFI_CMDSET_INTEL_EXTENDED:
 		write_cmd = (info->vendor == CFI_CMDSET_INTEL_PROG_REGIONS) ?
 					FLASH_CMD_WRITE_BUFFER_PROG : FLASH_CMD_WRITE_TO_BUFFER;
-		flash_write_cmd (info, sector, 0, FLASH_CMD_CLEAR_STATUS);
-		flash_write_cmd (info, sector, 0, FLASH_CMD_READ_STATUS);
-		flash_write_cmd (info, sector, 0, write_cmd);
-		retcode = flash_status_check (info, sector,
+		flash_write_cmd (info, *sector, 0, FLASH_CMD_CLEAR_STATUS);
+		flash_write_cmd (info, *sector, 0, FLASH_CMD_READ_STATUS);
+		flash_write_cmd (info, *sector, 0, write_cmd);
+		retcode = flash_status_check (info, *sector,
 					      info->buffer_write_tout,
 					      "write to buffer");
 		if (retcode == ERR_OK) {
 			/* reduce the number of loops by the width of
 			 * the port */
 			cnt = len >> shift;
-			flash_write_cmd (info, sector, 0, cnt - 1);
+			flash_write_cmd (info, *sector, 0, cnt - 1);
 			while (cnt-- > 0) {
 				switch (info->portwidth) {
 				case FLASH_CFI_8BIT:
@@ -985,11 +1007,8 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
 					goto out_unmap;
 				}
 			}
-			flash_write_cmd (info, sector, 0,
+			flash_write_cmd (info, *sector, 0,
 					 FLASH_CMD_WRITE_BUFFER_CONFIRM);
-			retcode = flash_full_status_check (
-				info, sector, info->buffer_write_tout,
-				"buffer write");
 		}
 
 		break;
@@ -999,11 +1018,11 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
 		flash_unlock_seq(info,0);
 
 #ifdef CONFIG_FLASH_SPANSION_S29WS_N
-		offset = ((unsigned long)dst - info->start[sector]) >> shift;
+		offset = ((unsigned long)dst - info->start[*sector]) >> shift;
 #endif
-		flash_write_cmd(info, sector, offset, AMD_CMD_WRITE_TO_BUFFER);
+		flash_write_cmd(info, *sector, offset, AMD_CMD_WRITE_TO_BUFFER);
 		cnt = len >> shift;
-		flash_write_cmd(info, sector, offset, cnt - 1);
+		flash_write_cmd(info, *sector, offset, cnt - 1);
 
 		switch (info->portwidth) {
 		case FLASH_CFI_8BIT:
@@ -1035,10 +1054,7 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
 			goto out_unmap;
 		}
 
-		flash_write_cmd (info, sector, 0, AMD_CMD_WRITE_BUFFER_CONFIRM);
-		retcode = flash_full_status_check (info, sector,
-						   info->buffer_write_tout,
-						   "buffer write");
+		flash_write_cmd (info, *sector, 0, AMD_CMD_WRITE_BUFFER_CONFIRM);
 		break;
 
 	default:
@@ -1050,16 +1066,92 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
 out_unmap:
 	return retcode;
 }
+
+static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
+				  int len)
+{
+	flash_sect_t sector;
+	int retcode;
+
+	retcode = flash_write_cfibuffer_stub(info, dest, cp, len, &sector);
+	if (retcode)
+		return retcode;
+	return flash_full_status_check (info, sector,
+					info->buffer_write_tout,
+					"buffer write");
+}
 #endif /* CONFIG_SYS_FLASH_USE_BUFFER_WRITE */
 
 
 /*-----------------------------------------------------------------------
+ * internal sector erase without final status check
+ */
+int flash_sector_erase (flash_info_t * info, flash_sect_t sect)
+{
+	int rcode = 0;
+
+	if (info->flash_id != FLASH_MAN_CFI) {
+		puts ("Can't erase unknown flash type - aborted\n");
+		return 1;
+	}
+	if (sect < 0) {
+		puts ("- no sector to erase\n");
+		return 1;
+	}
+
+	if (info->protect[sect] == 0) { /* not protected */
+		switch (info->vendor) {
+		case CFI_CMDSET_INTEL_PROG_REGIONS:
+		case CFI_CMDSET_INTEL_STANDARD:
+		case CFI_CMDSET_INTEL_EXTENDED:
+			flash_write_cmd (info, sect, 0,
+					 FLASH_CMD_CLEAR_STATUS);
+			flash_write_cmd (info, sect, 0,
+					 FLASH_CMD_BLOCK_ERASE);
+			flash_write_cmd (info, sect, 0,
+					 FLASH_CMD_ERASE_CONFIRM);
+			break;
+		case CFI_CMDSET_AMD_STANDARD:
+		case CFI_CMDSET_AMD_EXTENDED:
+			flash_unlock_seq (info, sect);
+			flash_write_cmd (info, sect,
+					info->addr_unlock1,
+					AMD_CMD_ERASE_START);
+			flash_unlock_seq (info, sect);
+			flash_write_cmd (info, sect, 0,
+					 AMD_CMD_ERASE_SECTOR);
+			break;
+#ifdef CONFIG_FLASH_CFI_LEGACY
+		case CFI_CMDSET_AMD_LEGACY:
+			flash_unlock_seq (info, 0);
+			flash_write_cmd (info, 0, info->addr_unlock1,
+					AMD_CMD_ERASE_START);
+			flash_unlock_seq (info, 0);
+			flash_write_cmd (info, sect, 0,
+					AMD_CMD_ERASE_SECTOR);
+			break;
+#endif
+		default:
+			debug ("Unkown flash vendor %d\n",
+			       info->vendor);
+			break;
+		}
+	}
+	else {
+		printf ("- Error: protected sector %d can not be erased!\n", (int)sect);
+		rcode = ERR_PROTECTED;
+	}
+	return rcode;
+}
+
+/*-----------------------------------------------------------------------
  */
 int flash_erase (flash_info_t * info, int s_first, int s_last)
 {
 	int rcode = 0;
 	int prot;
 	flash_sect_t sect;
+	int tmp_rcode;
 
 	if (info->flash_id != FLASH_MAN_CFI) {
 		puts ("Can't erase unknown flash type - aborted\n");
@@ -1083,51 +1175,18 @@ int flash_erase (flash_info_t * info, int s_first, int s_last)
 		putc ('\n');
 	}
 
-
 	for (sect = s_first; sect <= s_last; sect++) {
 		if (info->protect[sect] == 0) { /* not protected */
-			switch (info->vendor) {
-			case CFI_CMDSET_INTEL_PROG_REGIONS:
-			case CFI_CMDSET_INTEL_STANDARD:
-			case CFI_CMDSET_INTEL_EXTENDED:
-				flash_write_cmd (info, sect, 0,
-						 FLASH_CMD_CLEAR_STATUS);
-				flash_write_cmd (info, sect, 0,
-						 FLASH_CMD_BLOCK_ERASE);
-				flash_write_cmd (info, sect, 0,
-						 FLASH_CMD_ERASE_CONFIRM);
-				break;
-			case CFI_CMDSET_AMD_STANDARD:
-			case CFI_CMDSET_AMD_EXTENDED:
-				flash_unlock_seq (info, sect);
-				flash_write_cmd (info, sect,
-						info->addr_unlock1,
-						AMD_CMD_ERASE_START);
-				flash_unlock_seq (info, sect);
-				flash_write_cmd (info, sect, 0,
-						 AMD_CMD_ERASE_SECTOR);
-				break;
-#ifdef CONFIG_FLASH_CFI_LEGACY
-			case CFI_CMDSET_AMD_LEGACY:
-				flash_unlock_seq (info, 0);
-				flash_write_cmd (info, 0, info->addr_unlock1,
-						AMD_CMD_ERASE_START);
-				flash_unlock_seq (info, 0);
-				flash_write_cmd (info, sect, 0,
-						AMD_CMD_ERASE_SECTOR);
-				break;
-#endif
-			default:
-				debug ("Unkown flash vendor %d\n",
-				       info->vendor);
-				break;
+			tmp_rcode = flash_sector_erase (info, sect);
+			if (0 == tmp_rcode) {
+				if (flash_full_status_check
+				    (info, sect, info->erase_blk_tout, "erase")) {
+					rcode = 1;
+				} else if (flash_verbose)
+					putc ('.');
 			}
-
-			if (flash_full_status_check
-			    (info, sect, info->erase_blk_tout, "erase")) {
-				rcode = 1;
-			} else if (flash_verbose)
-				putc ('.');
+			else
+				rcode = tmp_rcode;
 		}
 	}
 
@@ -2135,3 +2194,228 @@ unsigned long flash_init (void)
 
 	return (size);
 }
+
+#if defined(CONFIG_SYS_FLASH_CFI_NONBLOCK)
+/*-----------------------------------------------------------------------
+ *  check if XSR.7 is set. When tout is nonzero, start timer, else check
+ *  if time out value is reached.
+ *  This routine does not set the flash to read-array mode.
+ */
+static int flash_status_check_nb (flash_info_t * info, flash_sect_t sector,
+			       ulong tout, char *prompt)
+{
+	static ulong start;
+	static ulong stout;
+
+#if CONFIG_SYS_HZ != 1000
+	tout *= CONFIG_SYS_HZ/1000;
+#endif
+
+	if (tout){
+		stout = tout;
+		start = get_timer (0);
+	}
+
+	/* Check for command completion */
+
+	if (flash_is_busy (info, sector))
+	{
+		if (get_timer (start) > stout) {
+			printf ("Flash %s timeout at address %lx data %lx\n",
+				prompt, info->start[sector],
+				flash_read_long (info, sector, 0));
+			flash_write_cmd (info, sector, 0, info->cmd_reset);
+			return ERR_TIMOUT;
+		}
+		udelay (1);		/* also triggers watchdog */
+		return ERR_BUSY;
+	}
+	return ERR_OK;
+}
+
+/*-----------------------------------------------------------------------
+ * Wait for XSR.7 to be set, if it times out print an error, otherwise
+ * do a full status check.
+ * This routine sets the flash to read-array mode.
+ */
+int flash_full_status_check_nb (flash_info_t * info, flash_sect_t sector,
+				    ulong tout, char *prompt)
+{
+	int retcode;
+
+	retcode = flash_status_check_nb (info, sector, tout, prompt);
+	if (retcode == ERR_BUSY)
+		return retcode;
+	return flash_full_status_retcode_check (info, sector, prompt, retcode);
+}
+
+int flash_erase_nb (flash_info_t * info, int sector)
+{
+	int rcode = 0;
+	flash_sect_t sect;
+
+	sect = sector;
+
+	rcode = flash_sector_erase (info, sect);
+	if (rcode == 0)
+		rcode = flash_full_status_check_nb(info, sect, info->erase_blk_tout, "erase");
+	return rcode;
+}
+
+static int flash_write_cfiword_nb (flash_info_t * info, ulong dest,
+				   cfiword_t cword)
+{
+	int retcode;
+	flash_sect_t sect = 0;
+
+	retcode = flash_write_cfiword_stub (info, dest, cword, &sect);
+	if (retcode)
+		return retcode;
+
+	return flash_full_status_check_nb (info, sect, info->write_tout, "write");
+}
+
+#ifdef CONFIG_SYS_FLASH_USE_BUFFER_WRITE
+static int flash_write_cfibuffer_nb (flash_info_t * info, ulong dest, uchar * cp,
+				  int len)
+{
+	int retcode;
+	flash_sect_t sector;
+
+	retcode = flash_write_cfibuffer_stub (info, dest, cp, len, &sector);
+	if(retcode)
+		return retcode;
+	sector = find_sector (info, dest);;
+	return flash_full_status_check_nb (info, sector, info->buffer_write_tout,
+					   "buffer write");
+}
+#endif /* CONFIG_SYS_FLASH_USE_BUFFER_WRITE */
+
+/*-----------------------------------------------------------------------
+ * Copy memory to flash, returns:
+ * 0 - OK
+ * 1 - write timeout
+ * 2 - Flash not erased
+ */
+int write_buff_nb (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
+{
+	ulong wp;
+	uchar *p;
+	int aln;
+	cfiword_t cword;
+	int i, rc;
+#ifdef CONFIG_SYS_FLASH_USE_BUFFER_WRITE
+	int buffered_size;
+#endif
+#ifdef CONFIG_FLASH_SHOW_PROGRESS
+	int digit = CONFIG_FLASH_SHOW_PROGRESS;
+	int scale = 0;
+	int dots  = 0;
+
+	/*
+	 * Suppress if there are fewer than CONFIG_FLASH_SHOW_PROGRESS writes.
+	 */
+	if (cnt >= CONFIG_FLASH_SHOW_PROGRESS) {
+		scale = (int)((cnt + CONFIG_FLASH_SHOW_PROGRESS - 1) /
+			CONFIG_FLASH_SHOW_PROGRESS);
+	}
+#endif
+
+	/* get lower aligned address */
+	wp = (addr & ~(info->portwidth - 1));
+
+	/* handle unaligned start */
+	if ((aln = addr - wp) != 0) {
+		cword.l = 0;
+		p = (uchar *)wp;
+		for (i = 0; i < aln; ++i)
+			flash_add_byte (info, &cword, flash_read8(p + i));
+
+		for (; (i < info->portwidth) && (cnt > 0); i++) {
+			flash_add_byte (info, &cword, *src++);
+			cnt--;
+		}
+		for (; (cnt == 0) && (i < info->portwidth); ++i)
+			flash_add_byte (info, &cword, flash_read8(p + i));
+
+		rc = flash_write_cfiword_nb (info, wp, cword);
+		if (rc != 0)
+			return rc;
+
+		wp += i;
+		FLASH_SHOW_PROGRESS(scale, dots, digit, i);
+	}
+
+	/* handle the aligned part */
+#ifdef CONFIG_SYS_FLASH_USE_BUFFER_WRITE
+	buffered_size = (info->portwidth / info->chipwidth);
+	buffered_size *= info->buffer_size;
+	while (cnt >= info->portwidth) {
+		/* prohibit buffer write when buffer_size is 1 */
+		if (info->buffer_size == 1) {
+			cword.l = 0;
+			for (i = 0; i < info->portwidth; i++)
+				flash_add_byte (info, &cword, *src++);
+			if ((rc = flash_write_cfiword_nb (info, wp, cword)) != 0)
+				return rc;
+			wp += info->portwidth;
+			cnt -= info->portwidth;
+			continue;
+		}
+
+		/* write buffer until next buffered_size aligned boundary */
+		i = buffered_size - (wp % buffered_size);
+		if (i > cnt)
+			i = cnt;
+		rc = flash_write_cfibuffer_nb (info, wp, src, i);
+		if ((rc != ERR_OK) && (rc != ERR_BUSY))
+			return rc;
+		i -= i & (info->portwidth - 1);
+		wp += i;
+		src += i;
+		cnt -= i;
+		if ((rc) != ERR_BUSY)
+			return rc;
+
+		if (cnt >= info->portwidth) {
+			rc = flash_full_status_check (info, find_sector (info, wp),
+						      info->buffer_write_tout,
+			  			      "buffer write");
+			if (rc != ERR_OK)
+				return rc;
+		}
+		FLASH_SHOW_PROGRESS(scale, dots, digit, i);
+	}
+#else
+	while (cnt >= info->portwidth) {
+		cword.l = 0;
+		for (i = 0; i < info->portwidth; i++) {
+			flash_add_byte (info, &cword, *src++);
+		}
+		if ((rc = flash_write_cfiword_nb (info, wp, cword)) != 0)
+			return rc;
+		wp += info->portwidth;
+		cnt -= info->portwidth;
+		FLASH_SHOW_PROGRESS(scale, dots, digit, info->portwidth);
+	}
+#endif /* CONFIG_SYS_FLASH_USE_BUFFER_WRITE */
+
+	if (cnt == 0) {
+		return (0);
+	}
+
+	/*
+	 * handle unaligned tail bytes
+	 */
+	cword.l = 0;
+	p = (uchar *)wp;
+	for (i = 0; (i < info->portwidth) && (cnt > 0); ++i) {
+		flash_add_byte (info, &cword, *src++);
+		--cnt;
+	}
+	for (; i < info->portwidth; ++i)
+		flash_add_byte (info, &cword, flash_read8(p + i));
+
+	return flash_write_cfiword_nb (info, wp, cword);
+}
+#endif /* CONFIG_SYS_FLASH_CFI_NONBLOCK */
diff --git a/include/flash.h b/include/flash.h
index 8feca1b..7b6fecc 100644
--- a/include/flash.h
+++ b/include/flash.h
@@ -138,6 +138,9 @@ extern flash_info_t *flash_get_info(ulong base);
 #define ERR_UNKNOWN_FLASH_VENDOR	32
 #define ERR_UNKNOWN_FLASH_TYPE		64
 #define ERR_PROG_ERROR			128
+#if defined(CONFIG_SYS_FLASH_CFI_NONBLOCK)
+#define ERR_BUSY			256
+#endif
 
 /*-----------------------------------------------------------------------
  * Protection Flags for flash_protect():
-- 
1.5.6.5

[U-Boot] [RFC PATCH] Implementation of non-blocking flash write/erase/status check functions.

[Wolfgang Wegner]

Hi list,

sorry, there should have been more description on top, but somehow I
messed up the commit log message and did not realize it until now. :-(

 Implementation of non-blocking flash write/erase/status check functions.
 Enable with CONFIG_SYS_FLASH_CFI_NONBLOCK
 These can be useful to erase flash/write data during a serial data
 transfer for software updates.

In reference to the comments of Wolfgang Denk:
The update protocol is surely not perfect (although I probably have
other things in mind I do not like about it than you), but the update
is normally done from the application. When this update fails due to
power loss or whatever reason, the bootloader shall be able to update
the software on its own. The advantage of this rather crude approach
is that it needs less flash space and there is no check necessary to
see which image in flash is the most current one.

Regards,
Wolfgang

[U-Boot] [RFC PATCH] Implementation of non-blocking flash write/erase/status check functions.

[Wolfgang Denk]

Dear Wolfgang Wegner,

In message <1256914121-26044-1-git-send-email-w.wegner@astro-kom.de> you wrote:
> write_buff_nb() introduces quite an amount of duplicate code compared to
> write_buff(), but I did not find an elegant solution to partition them.
> 
> Signed-off-by: Wolfgang Wegner <w.wegner@astro-kom.de>
> ---
>  drivers/mtd/cfi_flash.c |  440 ++++++++++++++++++++++++++++++++++++++---------
>  include/flash.h         |    3 +
>  2 files changed, 365 insertions(+), 78 deletions(-)

This summary alone is a pretty clear message to me. This is indeed a
lot of added, and even worse, duplicated code.

I see currently no reason to change my mind: I don;t want to see this
code in mainline. It breaks with the design principles, and makes the
common code more difficult  to  maintain  with  no  benefit  for  the
majority of the users.

Sorry.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
But it's real. And if it's real it can be affected ... we may not  be
able  to break it, but, I'll bet you credits to Navy Beans we can put
a dent in it.
	-- deSalle, "Catspaw", stardate 3018.2

[U-Boot] [PATCH] non-blocking flash write/erase/status check functions

[Wolfgang Wegner]

More tightly integrated non-blocking variants of some CFI flash access
functions. Enable with CONFIG_SYS_FLASH_CFI_NONBLOCK
These can be useful to erase flash or write complete sectors of flash
during a serial data transfer for software updates.

This re-worked patch addresses the code duplication, which is now avoided.

Signed-off-by: Wolfgang Wegner <w.wegner@astro-kom.de>
---
 drivers/mtd/cfi_flash.c |  268 +++++++++++++++++++++++++++++++++++------------
 include/flash.h         |    7 ++
 2 files changed, 207 insertions(+), 68 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 4e8f5bf..e0c7f2e 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -653,22 +653,30 @@ static int flash_is_busy (flash_info_t * info, flash_sect_t sect)
 }
 
 /*-----------------------------------------------------------------------
- *  wait for XSR.7 to be set. Time out with an error if it does not.
+ *  check/wait for XSR.7 is set. When tout is nonzero, start timer, else check
+ *  if time out value is reached.
  *  This routine does not set the flash to read-array mode.
  */
-static int flash_status_check (flash_info_t * info, flash_sect_t sector,
-			       ulong tout, char *prompt)
+static int flash_status_check_int (flash_info_t * info, flash_sect_t sector,
+				   ulong tout, char *prompt, int nonblock)
 {
-	ulong start;
+	static ulong start;
+	static ulong stout;
 
 #if CONFIG_SYS_HZ != 1000
 	tout *= CONFIG_SYS_HZ/1000;
 #endif
 
-	/* Wait for command completion */
-	start = get_timer (0);
-	while (flash_is_busy (info, sector)) {
-		if (get_timer (start) > tout) {
+	if (tout || (nonblock == 0)){
+		stout = tout;
+		start = get_timer (0);
+	}
+
+	/* Check for command completion */
+
+	while (flash_is_busy (info, sector))
+	{
+		if (get_timer (start) > stout) {
 			printf ("Flash %s timeout at address %lx data %lx\n",
 				prompt, info->start[sector],
 				flash_read_long (info, sector, 0));
@@ -676,22 +684,38 @@ static int flash_status_check (flash_info_t * info, flash_sect_t sector,
 			return ERR_TIMOUT;
 		}
 		udelay (1);		/* also triggers watchdog */
+		if(nonblock)
+			return ERR_BUSY;
 	}
 	return ERR_OK;
 }
 
 /*-----------------------------------------------------------------------
- * Wait for XSR.7 to be set, if it times out print an error, otherwise
- * do a full status check.
+ *  wait for XSR.7 to be set. Time out with an error if it does not.
+ *  This routine does not set the flash to read-array mode.
+ */
+static int flash_status_check (flash_info_t * info, flash_sect_t sector,
+			       ulong tout, char *prompt)
+{
+	return flash_status_check_int (info, sector, tout, prompt, 0);
+}
+
+/*-----------------------------------------------------------------------
+ * Check for XSR.7 to be set, either waiting for it (0 == nonblock) or
+ * returning FLASH_BUSY (1 == nonblock). If timeout is reached, print an
+ * error;
  *
- * This routine sets the flash to read-array mode.
+ * This routine sets the flash to read-array mode if blocking mode is
+ * enabled or if successful in non-blocking mode.
  */
-static int flash_full_status_check (flash_info_t * info, flash_sect_t sector,
-				    ulong tout, char *prompt)
+static int flash_full_status_check_int (flash_info_t * info, flash_sect_t sector,
+					ulong tout, char *prompt, int nonblock)
 {
 	int retcode;
 
-	retcode = flash_status_check (info, sector, tout, prompt);
+	retcode = flash_status_check_int (info, sector, tout, prompt, nonblock);
+	if(retcode == ERR_BUSY)
+		return retcode;
 	switch (info->vendor) {
 	case CFI_CMDSET_INTEL_PROG_REGIONS:
 	case CFI_CMDSET_INTEL_EXTENDED:
@@ -728,6 +752,18 @@ static int flash_full_status_check (flash_info_t * info, flash_sect_t sector,
 }
 
 /*-----------------------------------------------------------------------
+ * Wait for XSR.7 to be set, if it times out print an error, otherwise
+ * do a full status check.
+ *
+ * This routine sets the flash to read-array mode.
+ */
+static int flash_full_status_check (flash_info_t * info, flash_sect_t sector,
+				    ulong tout, char *prompt)
+{
+	return flash_full_status_check_int (info, sector, tout, prompt, 0);
+}
+
+/*-----------------------------------------------------------------------
  */
 static void flash_add_byte (flash_info_t * info, cfiword_t * cword, uchar c)
 {
@@ -796,8 +832,8 @@ static flash_sect_t find_sector (flash_info_t * info, ulong addr)
 
 /*-----------------------------------------------------------------------
  */
-static int flash_write_cfiword (flash_info_t * info, ulong dest,
-				cfiword_t cword)
+static int flash_write_cfiword_int (flash_info_t * info, ulong dest,
+				    cfiword_t cword, int nonblock)
 {
 	void *dstaddr = (void *)dest;
 	int flag;
@@ -874,13 +910,19 @@ static int flash_write_cfiword (flash_info_t * info, ulong dest,
 	if (!sect_found)
 		sect = find_sector (info, dest);
 
-	return flash_full_status_check (info, sect, info->write_tout, "write");
+	return flash_full_status_check_int (info, sect, info->write_tout, "write", nonblock);
+}
+
+static int flash_write_cfiword (flash_info_t * info, ulong dest,
+				cfiword_t cword)
+{
+	return flash_write_cfiword_int (info, dest, cword, 0);
 }
 
 #ifdef CONFIG_SYS_FLASH_USE_BUFFER_WRITE
 
-static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
-				  int len)
+static int flash_write_cfibuffer_int (flash_info_t * info, ulong dest, uchar * cp,
+				      int len, int nonblock)
 {
 	flash_sect_t sector;
 	int cnt;
@@ -987,9 +1029,9 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
 			}
 			flash_write_cmd (info, sector, 0,
 					 FLASH_CMD_WRITE_BUFFER_CONFIRM);
-			retcode = flash_full_status_check (
+			retcode = flash_full_status_check_int (
 				info, sector, info->buffer_write_tout,
-				"buffer write");
+				"buffer write", nonblock);
 		}
 
 		break;
@@ -1036,9 +1078,9 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
 		}
 
 		flash_write_cmd (info, sector, 0, AMD_CMD_WRITE_BUFFER_CONFIRM);
-		retcode = flash_full_status_check (info, sector,
-						   info->buffer_write_tout,
-						   "buffer write");
+		retcode = flash_full_status_check_int (info, sector,
+						       info->buffer_write_tout,
+						       "buffer write", nonblock);
 		break;
 
 	default:
@@ -1050,10 +1092,77 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
 out_unmap:
 	return retcode;
 }
+
+static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
+				  int len)
+{
+	return flash_write_cfibuffer_int(info, dest, cp, len, 0);
+}
 #endif /* CONFIG_SYS_FLASH_USE_BUFFER_WRITE */
 
 
 /*-----------------------------------------------------------------------
+ * internal sector erase without final status check
+ */
+int flash_sector_erase (flash_info_t * info, flash_sect_t sect)
+{
+	int rcode = 0;
+
+	if (info->flash_id != FLASH_MAN_CFI) {
+		puts ("Can't erase unknown flash type - aborted\n");
+		return 1;
+	}
+	if (sect < 0) {
+		puts ("- no sector to erase\n");
+		return 1;
+	}
+
+	if (info->protect[sect] == 0) { /* not protected */
+		switch (info->vendor) {
+		case CFI_CMDSET_INTEL_PROG_REGIONS:
+		case CFI_CMDSET_INTEL_STANDARD:
+		case CFI_CMDSET_INTEL_EXTENDED:
+			flash_write_cmd (info, sect, 0,
+					 FLASH_CMD_CLEAR_STATUS);
+			flash_write_cmd (info, sect, 0,
+					 FLASH_CMD_BLOCK_ERASE);
+			flash_write_cmd (info, sect, 0,
+					 FLASH_CMD_ERASE_CONFIRM);
+			break;
+		case CFI_CMDSET_AMD_STANDARD:
+		case CFI_CMDSET_AMD_EXTENDED:
+			flash_unlock_seq (info, sect);
+			flash_write_cmd (info, sect,
+					info->addr_unlock1,
+					AMD_CMD_ERASE_START);
+			flash_unlock_seq (info, sect);
+			flash_write_cmd (info, sect, 0,
+					 AMD_CMD_ERASE_SECTOR);
+			break;
+#ifdef CONFIG_FLASH_CFI_LEGACY
+		case CFI_CMDSET_AMD_LEGACY:
+			flash_unlock_seq (info, 0);
+			flash_write_cmd (info, 0, info->addr_unlock1,
+					AMD_CMD_ERASE_START);
+			flash_unlock_seq (info, 0);
+			flash_write_cmd (info, sect, 0,
+					AMD_CMD_ERASE_SECTOR);
+			break;
+#endif
+		default:
+			debug ("Unkown flash vendor %d\n",
+			       info->vendor);
+			break;
+		}
+	}
+	else {
+		printf ("- Error: protected sector %d can not be erased!\n", (int)sect);
+		rcode = ERR_PROTECTED;
+	}
+	return rcode;
+}
+
+/*-----------------------------------------------------------------------
  */
 int flash_erase (flash_info_t * info, int s_first, int s_last)
 {
@@ -1083,46 +1192,9 @@ int flash_erase (flash_info_t * info, int s_first, int s_last)
 		putc ('\n');
 	}
 
-
 	for (sect = s_first; sect <= s_last; sect++) {
 		if (info->protect[sect] == 0) { /* not protected */
-			switch (info->vendor) {
-			case CFI_CMDSET_INTEL_PROG_REGIONS:
-			case CFI_CMDSET_INTEL_STANDARD:
-			case CFI_CMDSET_INTEL_EXTENDED:
-				flash_write_cmd (info, sect, 0,
-						 FLASH_CMD_CLEAR_STATUS);
-				flash_write_cmd (info, sect, 0,
-						 FLASH_CMD_BLOCK_ERASE);
-				flash_write_cmd (info, sect, 0,
-						 FLASH_CMD_ERASE_CONFIRM);
-				break;
-			case CFI_CMDSET_AMD_STANDARD:
-			case CFI_CMDSET_AMD_EXTENDED:
-				flash_unlock_seq (info, sect);
-				flash_write_cmd (info, sect,
-						info->addr_unlock1,
-						AMD_CMD_ERASE_START);
-				flash_unlock_seq (info, sect);
-				flash_write_cmd (info, sect, 0,
-						 AMD_CMD_ERASE_SECTOR);
-				break;
-#ifdef CONFIG_FLASH_CFI_LEGACY
-			case CFI_CMDSET_AMD_LEGACY:
-				flash_unlock_seq (info, 0);
-				flash_write_cmd (info, 0, info->addr_unlock1,
-						AMD_CMD_ERASE_START);
-				flash_unlock_seq (info, 0);
-				flash_write_cmd (info, sect, 0,
-						AMD_CMD_ERASE_SECTOR);
-				break;
-#endif
-			default:
-				debug ("Unkown flash vendor %d\n",
-				       info->vendor);
-				break;
-			}
-
+			flash_sector_erase (info, sect);
 			if (flash_full_status_check
 			    (info, sect, info->erase_blk_tout, "erase")) {
 				rcode = 1;
@@ -1265,8 +1337,9 @@ void flash_print_info (flash_info_t * info)
  * 0 - OK
  * 1 - write timeout
  * 2 - Flash not erased
+ * 256 - Flash busy (when CONFIG_SYS_FLASH_CFI_NONBLOCK is set)
  */
-int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
+int write_buff_int (flash_info_t * info, uchar * src, ulong addr, ulong cnt, int nonblock)
 {
 	ulong wp;
 	uchar *p;
@@ -1307,7 +1380,7 @@ int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
 		for (; (cnt == 0) && (i < info->portwidth); ++i)
 			flash_add_byte (info, &cword, flash_read8(p + i));
 
-		rc = flash_write_cfiword (info, wp, cword);
+		rc = flash_write_cfiword_int (info, wp, cword, nonblock);
 		if (rc != 0)
 			return rc;
 
@@ -1325,7 +1398,7 @@ int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
 			cword.l = 0;
 			for (i = 0; i < info->portwidth; i++)
 				flash_add_byte (info, &cword, *src++);
-			if ((rc = flash_write_cfiword (info, wp, cword)) != 0)
+			if ((rc = flash_write_cfiword_int (info, wp, cword, nonblock)) != 0)
 				return rc;
 			wp += info->portwidth;
 			cnt -= info->portwidth;
@@ -1336,12 +1409,25 @@ int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
 		i = buffered_size - (wp % buffered_size);
 		if (i > cnt)
 			i = cnt;
-		if ((rc = flash_write_cfibuffer (info, wp, src, i)) != ERR_OK)
+		rc = flash_write_cfibuffer_int (info, wp, src, i, nonblock);
+		if ((rc != ERR_OK) && (!nonblock || (rc != ERR_BUSY)))
 			return rc;
 		i -= i & (info->portwidth - 1);
 		wp += i;
 		src += i;
 		cnt -= i;
+		if(nonblock) {
+			if ((rc) != ERR_BUSY)
+				return rc;
+
+			if (cnt >= info->portwidth) {
+				rc = flash_full_status_check_int (info, find_sector (info, wp),
+								  info->buffer_write_tout,
+			  					  "buffer write", nonblock);
+				if (rc != ERR_OK)
+					return rc;
+			}
+		}
 		FLASH_SHOW_PROGRESS(scale, dots, digit, i);
 	}
 #else
@@ -1350,7 +1436,7 @@ int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
 		for (i = 0; i < info->portwidth; i++) {
 			flash_add_byte (info, &cword, *src++);
 		}
-		if ((rc = flash_write_cfiword (info, wp, cword)) != 0)
+		if ((rc = flash_write_cfiword_int (info, wp, cword, nonblock)) != 0)
 			return rc;
 		wp += info->portwidth;
 		cnt -= info->portwidth;
@@ -1374,7 +1460,18 @@ int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
 	for (; i < info->portwidth; ++i)
 		flash_add_byte (info, &cword, flash_read8(p + i));
 
-	return flash_write_cfiword (info, wp, cword);
+	return flash_write_cfiword_int (info, wp, cword, nonblock);
+}
+
+/*-----------------------------------------------------------------------
+ * Copy memory to flash, returns:
+ * 0 - OK
+ * 1 - write timeout
+ * 2 - Flash not erased
+ */
+int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
+{
+	return write_buff_int(info, src, addr, cnt, 0);
 }
 
 /*-----------------------------------------------------------------------
@@ -2135,3 +2232,38 @@ unsigned long flash_init (void)
 
 	return (size);
 }
+
+#if defined(CONFIG_SYS_FLASH_CFI_NONBLOCK)
+/*-----------------------------------------------------------------------
+ * Wait for XSR.7 to be set, if it times out print an error, otherwise
+ * do a full status check.
+ * This routine sets the flash to read-array mode.
+ */
+int flash_full_status_check_nb (flash_info_t * info, flash_sect_t sector,
+				ulong tout, char *prompt)
+{
+	return flash_full_status_check_int (info, sector, tout, prompt, 1);
+}
+
+int flash_erase_nb (flash_info_t * info, int sector)
+{
+	int rcode;
+	flash_sect_t sect = sector;
+
+	if ((rcode = flash_sector_erase (info, sect)) == 0)
+		rcode = flash_full_status_check_int (info, sect, info->erase_blk_tout, "erase", 1);
+	return rcode;
+}
+
+/*-----------------------------------------------------------------------
+ * Copy memory to flash, returns:
+ * 0 - OK
+ * 1 - write timeout
+ * 2 - Flash not erased
+ * 256 - Flash busy
+ */
+int write_buff_nb (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
+{
+	return write_buff_int(info, src, addr, cnt, 1);
+}
+#endif /* CONFIG_SYS_FLASH_CFI_NONBLOCK */
diff --git a/include/flash.h b/include/flash.h
index 8feca1b..6b870d3 100644
--- a/include/flash.h
+++ b/include/flash.h
@@ -126,6 +126,12 @@ extern int jedec_flash_match(flash_info_t *info, ulong base);
 extern flash_info_t *flash_get_info(ulong base);
 #endif
 
+#if defined(CONFIG_SYS_FLASH_CFI_NONBLOCK)
+extern int flash_full_status_check_nb (flash_info_t * info, ulong sector,ulong tout, char *prompt);  
+extern int flash_erase_nb (flash_info_t * info, int sector);
+extern int write_buff_nb (flash_info_t * info, uchar * src, ulong addr, ulong cnt);
+#endif
+
 /*-----------------------------------------------------------------------
  * return codes from flash_write():
  */
@@ -138,6 +144,7 @@ extern flash_info_t *flash_get_info(ulong base);
 #define ERR_UNKNOWN_FLASH_VENDOR	32
 #define ERR_UNKNOWN_FLASH_TYPE		64
 #define ERR_PROG_ERROR			128
+#define ERR_BUSY			256
 
 /*-----------------------------------------------------------------------
  * Protection Flags for flash_protect():
-- 
1.5.6.5

[U-Boot] [RFC PATCH] Implementation of non-blocking flash write/erase/status check functions.

[Wolfgang Wegner]

Dear Wolfgang Denk,

On Fri, Oct 30, 2009 at 07:22:17PM +0100, Wolfgang Denk wrote:
[...]
> >  2 files changed, 365 insertions(+), 78 deletions(-)
> 
> This summary alone is a pretty clear message to me. This is indeed a
> lot of added, and even worse, duplicated code.

I tried to address this by re-working the code to completely avoid
duplicate code. This leads to a much tighter integration, I do not
know if this is wanted by the CFI/flash maintainers.

The patch is on its way - comments are highly welcome, as I do not
see a real conflict of the underlying idea to the U-Boot design
principles, I am definitely willing to address all implementation/
coding style issues in case this helps to change your mind. :-)

Best regards,
Wolfgang Wegner

[U-Boot] [PATCH RFC v2] non-blocking flash write/erase/status check functions

[Wolfgang Wegner]

More tightly integrated non-blocking variants of some CFI flash access
functions. Enable with CONFIG_SYS_FLASH_CFI_NONBLOCK
These can be useful to erase flash or write complete sectors of flash
during a serial data transfer for software updates.

Signed-off-by: Wolfgang Wegner <w.wegner@astro-kom.de>
---
Re-worked patch avoiding code duplication and fixing white-space as
well as line length errors I found while forwarding to next branch.

 drivers/mtd/cfi_flash.c |  269 +++++++++++++++++++++++++++++++++++------------
 include/flash.h         |    9 ++
 2 files changed, 210 insertions(+), 68 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index c611f6b..74fa75f 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -528,22 +528,30 @@ static int flash_is_busy (flash_info_t * info, flash_sect_t sect)
 }
 
 /*-----------------------------------------------------------------------
- *  wait for XSR.7 to be set. Time out with an error if it does not.
+ *  check/wait for XSR.7 is set. When tout is nonzero, start timer, else check
+ *  if time out value is reached.
  *  This routine does not set the flash to read-array mode.
  */
-static int flash_status_check (flash_info_t * info, flash_sect_t sector,
-			       ulong tout, char *prompt)
+static int flash_status_check_int (flash_info_t * info, flash_sect_t sector,
+				   ulong tout, char *prompt, int nonblock)
 {
-	ulong start;
+	static ulong start;
+	static ulong stout;
 
 #if CONFIG_SYS_HZ != 1000
 	tout *= CONFIG_SYS_HZ/1000;
 #endif
 
-	/* Wait for command completion */
-	start = get_timer (0);
-	while (flash_is_busy (info, sector)) {
-		if (get_timer (start) > tout) {
+	if (tout || (nonblock == 0)){
+		stout = tout;
+		start = get_timer (0);
+	}
+
+	/* Check for command completion */
+
+	while (flash_is_busy (info, sector))
+	{
+		if (get_timer (start) > stout) {
 			printf ("Flash %s timeout at address %lx data %lx\n",
 				prompt, info->start[sector],
 				flash_read_long (info, sector, 0));
@@ -551,22 +559,38 @@ static int flash_status_check (flash_info_t * info, flash_sect_t sector,
 			return ERR_TIMOUT;
 		}
 		udelay (1);		/* also triggers watchdog */
+		if(nonblock)
+			return ERR_BUSY;
 	}
 	return ERR_OK;
 }
 
 /*-----------------------------------------------------------------------
- * Wait for XSR.7 to be set, if it times out print an error, otherwise
- * do a full status check.
+ *  wait for XSR.7 to be set. Time out with an error if it does not.
+ *  This routine does not set the flash to read-array mode.
+ */
+static int flash_status_check (flash_info_t * info, flash_sect_t sector,
+			       ulong tout, char *prompt)
+{
+	return flash_status_check_int (info, sector, tout, prompt, 0);
+}
+
+/*-----------------------------------------------------------------------
+ * Check for XSR.7 to be set, either waiting for it (0 == nonblock) or
+ * returning FLASH_BUSY (1 == nonblock). If timeout is reached, print an
+ * error;
  *
- * This routine sets the flash to read-array mode.
+ * This routine sets the flash to read-array mode if blocking mode is
+ * enabled or if successful in non-blocking mode.
  */
-static int flash_full_status_check (flash_info_t * info, flash_sect_t sector,
-				    ulong tout, char *prompt)
+static int flash_full_status_check_int (flash_info_t * info, flash_sect_t sector,
+					ulong tout, char *prompt, int nonblock)
 {
 	int retcode;
 
-	retcode = flash_status_check (info, sector, tout, prompt);
+	retcode = flash_status_check_int (info, sector, tout, prompt, nonblock);
+	if(retcode == ERR_BUSY)
+		return retcode;
 	switch (info->vendor) {
 	case CFI_CMDSET_INTEL_PROG_REGIONS:
 	case CFI_CMDSET_INTEL_EXTENDED:
@@ -603,6 +627,18 @@ static int flash_full_status_check (flash_info_t * info, flash_sect_t sector,
 }
 
 /*-----------------------------------------------------------------------
+ * Wait for XSR.7 to be set, if it times out print an error, otherwise
+ * do a full status check.
+ *
+ * This routine sets the flash to read-array mode.
+ */
+static int flash_full_status_check (flash_info_t * info, flash_sect_t sector,
+				    ulong tout, char *prompt)
+{
+	return flash_full_status_check_int (info, sector, tout, prompt, 0);
+}
+
+/*-----------------------------------------------------------------------
  */
 static void flash_add_byte (flash_info_t * info, cfiword_t * cword, uchar c)
 {
@@ -671,8 +707,8 @@ static flash_sect_t find_sector (flash_info_t * info, ulong addr)
 
 /*-----------------------------------------------------------------------
  */
-static int flash_write_cfiword (flash_info_t * info, ulong dest,
-				cfiword_t cword)
+static int flash_write_cfiword_int (flash_info_t * info, ulong dest,
+				    cfiword_t cword, int nonblock)
 {
 	void *dstaddr = (void *)dest;
 	int flag;
@@ -749,13 +785,19 @@ static int flash_write_cfiword (flash_info_t * info, ulong dest,
 	if (!sect_found)
 		sect = find_sector (info, dest);
 
-	return flash_full_status_check (info, sect, info->write_tout, "write");
+	return flash_full_status_check_int (info, sect, info->write_tout, "write", nonblock);
+}
+
+static int flash_write_cfiword (flash_info_t * info, ulong dest,
+				cfiword_t cword)
+{
+	return flash_write_cfiword_int (info, dest, cword, 0);
 }
 
 #ifdef CONFIG_SYS_FLASH_USE_BUFFER_WRITE
 
-static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
-				  int len)
+static int flash_write_cfibuffer_int (flash_info_t * info, ulong dest, uchar * cp,
+				      int len, int nonblock)
 {
 	flash_sect_t sector;
 	int cnt;
@@ -862,9 +904,9 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
 			}
 			flash_write_cmd (info, sector, 0,
 					 FLASH_CMD_WRITE_BUFFER_CONFIRM);
-			retcode = flash_full_status_check (
+			retcode = flash_full_status_check_int (
 				info, sector, info->buffer_write_tout,
-				"buffer write");
+				"buffer write", nonblock);
 		}
 
 		break;
@@ -911,9 +953,9 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
 		}
 
 		flash_write_cmd (info, sector, 0, AMD_CMD_WRITE_BUFFER_CONFIRM);
-		retcode = flash_full_status_check (info, sector,
-						   info->buffer_write_tout,
-						   "buffer write");
+		retcode = flash_full_status_check_int (info, sector,
+						       info->buffer_write_tout,
+						       "buffer write", nonblock);
 		break;
 
 	default:
@@ -925,10 +967,77 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
 out_unmap:
 	return retcode;
 }
+
+static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp,
+				  int len)
+{
+	return flash_write_cfibuffer_int(info, dest, cp, len, 0);
+}
 #endif /* CONFIG_SYS_FLASH_USE_BUFFER_WRITE */
 
 
 /*-----------------------------------------------------------------------
+ * internal sector erase without final status check
+ */
+int flash_sector_erase (flash_info_t * info, flash_sect_t sect)
+{
+	int rcode = 0;
+
+	if (info->flash_id != FLASH_MAN_CFI) {
+		puts ("Can't erase unknown flash type - aborted\n");
+		return 1;
+	}
+	if (sect < 0) {
+		puts ("- no sector to erase\n");
+		return 1;
+	}
+
+	if (info->protect[sect] == 0) { /* not protected */
+		switch (info->vendor) {
+		case CFI_CMDSET_INTEL_PROG_REGIONS:
+		case CFI_CMDSET_INTEL_STANDARD:
+		case CFI_CMDSET_INTEL_EXTENDED:
+			flash_write_cmd (info, sect, 0,
+					 FLASH_CMD_CLEAR_STATUS);
+			flash_write_cmd (info, sect, 0,
+					 FLASH_CMD_BLOCK_ERASE);
+			flash_write_cmd (info, sect, 0,
+					 FLASH_CMD_ERASE_CONFIRM);
+			break;
+		case CFI_CMDSET_AMD_STANDARD:
+		case CFI_CMDSET_AMD_EXTENDED:
+			flash_unlock_seq (info, sect);
+			flash_write_cmd (info, sect,
+					info->addr_unlock1,
+					AMD_CMD_ERASE_START);
+			flash_unlock_seq (info, sect);
+			flash_write_cmd (info, sect, 0,
+					 AMD_CMD_ERASE_SECTOR);
+			break;
+#ifdef CONFIG_FLASH_CFI_LEGACY
+		case CFI_CMDSET_AMD_LEGACY:
+			flash_unlock_seq (info, 0);
+			flash_write_cmd (info, 0, info->addr_unlock1,
+					AMD_CMD_ERASE_START);
+			flash_unlock_seq (info, 0);
+			flash_write_cmd (info, sect, 0,
+					AMD_CMD_ERASE_SECTOR);
+			break;
+#endif
+		default:
+			debug ("Unkown flash vendor %d\n",
+			       info->vendor);
+			break;
+		}
+	}
+	else {
+		printf ("- Error: protected sector %d can not be erased!\n", (int)sect);
+		rcode = ERR_PROTECTED;
+	}
+	return rcode;
+}
+
+/*-----------------------------------------------------------------------
  */
 int flash_erase (flash_info_t * info, int s_first, int s_last)
 {
@@ -958,46 +1067,9 @@ int flash_erase (flash_info_t * info, int s_first, int s_last)
 		putc ('\n');
 	}
 
-
 	for (sect = s_first; sect <= s_last; sect++) {
 		if (info->protect[sect] == 0) { /* not protected */
-			switch (info->vendor) {
-			case CFI_CMDSET_INTEL_PROG_REGIONS:
-			case CFI_CMDSET_INTEL_STANDARD:
-			case CFI_CMDSET_INTEL_EXTENDED:
-				flash_write_cmd (info, sect, 0,
-						 FLASH_CMD_CLEAR_STATUS);
-				flash_write_cmd (info, sect, 0,
-						 FLASH_CMD_BLOCK_ERASE);
-				flash_write_cmd (info, sect, 0,
-						 FLASH_CMD_ERASE_CONFIRM);
-				break;
-			case CFI_CMDSET_AMD_STANDARD:
-			case CFI_CMDSET_AMD_EXTENDED:
-				flash_unlock_seq (info, sect);
-				flash_write_cmd (info, sect,
-						info->addr_unlock1,
-						AMD_CMD_ERASE_START);
-				flash_unlock_seq (info, sect);
-				flash_write_cmd (info, sect, 0,
-						 AMD_CMD_ERASE_SECTOR);
-				break;
-#ifdef CONFIG_FLASH_CFI_LEGACY
-			case CFI_CMDSET_AMD_LEGACY:
-				flash_unlock_seq (info, 0);
-				flash_write_cmd (info, 0, info->addr_unlock1,
-						AMD_CMD_ERASE_START);
-				flash_unlock_seq (info, 0);
-				flash_write_cmd (info, sect, 0,
-						AMD_CMD_ERASE_SECTOR);
-				break;
-#endif
-			default:
-				debug ("Unkown flash vendor %d\n",
-				       info->vendor);
-				break;
-			}
-
+			flash_sector_erase (info, sect);
 			if (flash_full_status_check
 			    (info, sect, info->erase_blk_tout, "erase")) {
 				rcode = 1;
@@ -1140,8 +1212,9 @@ void flash_print_info (flash_info_t * info)
  * 0 - OK
  * 1 - write timeout
  * 2 - Flash not erased
+ * 256 - Flash busy (when CONFIG_SYS_FLASH_CFI_NONBLOCK is set)
  */
-int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
+int write_buff_int (flash_info_t * info, uchar * src, ulong addr, ulong cnt, int nonblock)
 {
 	ulong wp;
 	uchar *p;
@@ -1182,7 +1255,7 @@ int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
 		for (; (cnt == 0) && (i < info->portwidth); ++i)
 			flash_add_byte (info, &cword, flash_read8(p + i));
 
-		rc = flash_write_cfiword (info, wp, cword);
+		rc = flash_write_cfiword_int (info, wp, cword, nonblock);
 		if (rc != 0)
 			return rc;
 
@@ -1200,7 +1273,7 @@ int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
 			cword.l = 0;
 			for (i = 0; i < info->portwidth; i++)
 				flash_add_byte (info, &cword, *src++);
-			if ((rc = flash_write_cfiword (info, wp, cword)) != 0)
+			if ((rc = flash_write_cfiword_int (info, wp, cword, nonblock)) != 0)
 				return rc;
 			wp += info->portwidth;
 			cnt -= info->portwidth;
@@ -1211,12 +1284,25 @@ int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
 		i = buffered_size - (wp % buffered_size);
 		if (i > cnt)
 			i = cnt;
-		if ((rc = flash_write_cfibuffer (info, wp, src, i)) != ERR_OK)
+		rc = flash_write_cfibuffer_int (info, wp, src, i, nonblock);
+		if ((rc != ERR_OK) && (!nonblock || (rc != ERR_BUSY)))
 			return rc;
 		i -= i & (info->portwidth - 1);
 		wp += i;
 		src += i;
 		cnt -= i;
+		if(nonblock) {
+			if ((rc) != ERR_BUSY)
+				return rc;
+
+			if (cnt >= info->portwidth) {
+				rc = flash_full_status_check_int (info, find_sector (info, wp),
+								  info->buffer_write_tout,
+								  "buffer write", nonblock);
+				if (rc != ERR_OK)
+					return rc;
+			}
+		}
 		FLASH_SHOW_PROGRESS(scale, dots, digit, i);
 	}
 #else
@@ -1225,7 +1311,7 @@ int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
 		for (i = 0; i < info->portwidth; i++) {
 			flash_add_byte (info, &cword, *src++);
 		}
-		if ((rc = flash_write_cfiword (info, wp, cword)) != 0)
+		if ((rc = flash_write_cfiword_int (info, wp, cword, nonblock)) != 0)
 			return rc;
 		wp += info->portwidth;
 		cnt -= info->portwidth;
@@ -1249,7 +1335,18 @@ int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
 	for (; i < info->portwidth; ++i)
 		flash_add_byte (info, &cword, flash_read8(p + i));
 
-	return flash_write_cfiword (info, wp, cword);
+	return flash_write_cfiword_int (info, wp, cword, nonblock);
+}
+
+/*-----------------------------------------------------------------------
+ * Copy memory to flash, returns:
+ * 0 - OK
+ * 1 - write timeout
+ * 2 - Flash not erased
+ */
+int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
+{
+	return write_buff_int(info, src, addr, cnt, 0);
 }
 
 /*-----------------------------------------------------------------------
@@ -2020,3 +2117,39 @@ unsigned long flash_init (void)
 
 	return (size);
 }
+
+#if defined(CONFIG_SYS_FLASH_CFI_NONBLOCK)
+/*-----------------------------------------------------------------------
+ * Wait for XSR.7 to be set, if it times out print an error, otherwise
+ * do a full status check.
+ * This routine sets the flash to read-array mode.
+ */
+int flash_full_status_check_nb (flash_info_t * info, flash_sect_t sector,
+				ulong tout, char *prompt)
+{
+	return flash_full_status_check_int (info, sector, tout, prompt, 1);
+}
+
+int flash_erase_nb (flash_info_t * info, int sector)
+{
+	int rcode;
+	flash_sect_t sect = sector;
+
+	if ((rcode = flash_sector_erase (info, sect)) == 0)
+		rcode = flash_full_status_check_int (info, sect,
+					info->erase_blk_tout, "erase", 1);
+	return rcode;
+}
+
+/*-----------------------------------------------------------------------
+ * Copy memory to flash, returns:
+ * 0 - OK
+ * 1 - write timeout
+ * 2 - Flash not erased
+ * 256 - Flash busy
+ */
+int write_buff_nb (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
+{
+	return write_buff_int(info, src, addr, cnt, 1);
+}
+#endif /* CONFIG_SYS_FLASH_CFI_NONBLOCK */
diff --git a/include/flash.h b/include/flash.h
index 8feca1b..026497b 100644
--- a/include/flash.h
+++ b/include/flash.h
@@ -126,6 +126,14 @@ extern int jedec_flash_match(flash_info_t *info, ulong base);
 extern flash_info_t *flash_get_info(ulong base);
 #endif
 
+#if defined(CONFIG_SYS_FLASH_CFI_NONBLOCK)
+extern int flash_full_status_check_nb (flash_info_t * info, ulong sector,
+					ulong tout, char *prompt);
+extern int flash_erase_nb (flash_info_t * info, int sector);
+extern int write_buff_nb (flash_info_t * info, uchar * src, ulong addr,
+				ulong cnt);
+#endif
+
 /*-----------------------------------------------------------------------
  * return codes from flash_write():
  */
@@ -138,6 +146,7 @@ extern flash_info_t *flash_get_info(ulong base);
 #define ERR_UNKNOWN_FLASH_VENDOR	32
 #define ERR_UNKNOWN_FLASH_TYPE		64
 #define ERR_PROG_ERROR			128
+#define ERR_BUSY			256
 
 /*-----------------------------------------------------------------------
  * Protection Flags for flash_protect():
-- 
1.5.6.5

[U-Boot] [PATCH RFC v2] non-blocking flash write/erase/status check functions

[Wolfgang Wegner]

Hi,

any comments on this?

On Wed, Dec 09, 2009 at 05:00:11PM +0100, Wolfgang Wegner wrote:
> More tightly integrated non-blocking variants of some CFI flash access
> functions. Enable with CONFIG_SYS_FLASH_CFI_NONBLOCK
> These can be useful to erase flash or write complete sectors of flash
> during a serial data transfer for software updates.

The first version had a strong NACK by Wolfgang Denk because of
code duplication. Does this version have a chance for inclusion,
or are there other things I have to fix/change to get it to this
stage?

Regards,
Wolfgang

[U-Boot] [PATCH RFC v2] non-blocking flash write/erase/status check functions

[Wolfgang Denk]

Dear Wolfgang Wegner,

In message <20100122100332.GK23389@leila.ping.de> you wrote:
> 
> On Wed, Dec 09, 2009 at 05:00:11PM +0100, Wolfgang Wegner wrote:
> > More tightly integrated non-blocking variants of some CFI flash access
> > functions. Enable with CONFIG_SYS_FLASH_CFI_NONBLOCK
> > These can be useful to erase flash or write complete sectors of flash
> > during a serial data transfer for software updates.
> 
> The first version had a strong NACK by Wolfgang Denk because of
> code duplication. Does this version have a chance for inclusion,
> or are there other things I have to fix/change to get it to this
> stage?

The patch is still pretty intrusive - the resulting code is much
harder to read, to understand and to debug (in both configurations).

[U-Boot] [PATCH RFC v2] non-blocking flash write/erase/status check functions

[Stefan Roese]

On Friday 22 January 2010 13:03:40 Wolfgang Denk wrote:
> > > More tightly integrated non-blocking variants of some CFI flash access
> > > functions. Enable with CONFIG_SYS_FLASH_CFI_NONBLOCK
> > > These can be useful to erase flash or write complete sectors of flash
> > > during a serial data transfer for software updates.
> >
> > The first version had a strong NACK by Wolfgang Denk because of
> > code duplication. Does this version have a chance for inclusion,
> > or are there other things I have to fix/change to get it to this
> > stage?
> 
> The patch is still pretty intrusive - the resulting code is much
> harder to read, to understand and to debug (in both configurations).
> 
> From my point of view the disadvantages compared to the advantages
> (and the potential number of users of this new feature) don't justify
> to apply this patch.
> 
> 
> Stefan, what do you think?

I have the same feeling. Even though Wolfgang Wegner has put bigger effort 
into making this less intrusive, it still makes the CFI driver even more 
complex and less readable.

So I tend to not pull this patch (Sorry Wolfgang).

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