Skip to contentSkip to conversation

non-blocking flash functions - is this possible/acceptable?

U-Boot New Wolfgang Wegner 5 participants 16 messages Oct 27, 2009 – Jan 25, 2010 Memory technology devices (MTD)

lore
Share & tools
lore

Wolfgang Wegner

author
#0 lore raw

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

#1 lore raw
Quoted 14 lines · 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()

Quoted 7 lines

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.)

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

[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

#2 lore raw
Quoted 5 lines · Jerry Van Baren
Quoted 4 lines · Wolfgang Wegner

Hi,

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

[snip]

Quoted 5 lines

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]

Quoted 2 lines
Quoted 2 lines

Regards,
Wolfgang

Thanks,
gvb

author
#3 lore raw

Hi Jerry,

Quoted 2 lines · Jerry Van Baren
Quoted 1 line · Jerry Van Baren

Wolfgang Wegner wrote:

[...]

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

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

[...]

Quoted 3 lines

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

#4 lore raw
Quoted 6 lines · Wolfgang Wegner

Hi Jerry,

Quoted 2 lines · Jerry Van Baren
Quoted 1 line · Jerry Van Baren

Wolfgang Wegner wrote:

[...]

Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

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

[...]

Quoted 2 lines

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.

Quoted 7 lines

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]

Quoted 2 lines

Regards,
Wolfgang

Thanks,
gvb

author
#5 lore raw
In reply to Jerry Van Baren
Quoted 5 lines · Jerry Van Baren
Quoted 1 line · Wolfgang Wegner

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.

:-)

Quoted 9 lines
Quoted 2 lines

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

#6 lore raw
In reply to Wolfgang Wegner

Dear Wolfgang Wegner,

Quoted 3 lines · In message <[email protected]> you wrote:

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

...

Quoted 6 lines

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).

#7 lore raw

RFC PATCH 1/1 Implementation of non-blocking flash write/erase/status check functions.

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 <[email protected]>
---
 drivers/mtd/cfi_flash.c |  440 ++++++++++++++++++++++++++++++++++++++---------
 include/flash.h         |    3 +
 2 files changed, 365 insertions(+), 78 deletions(-)
drivers/mtd/cfi_flash.c +362 -78
index 4e8f5bf..0f813b0 100644
@@ -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 */
include/flash.h +3 -0
index 8feca1b..7b6fecc 100644
@@ -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():
Signature · 1 line
1.5.6.5
author
#8 lore raw

RFC PATCH 1/1 Implementation of non-blocking flash write/erase/status check functions.

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

#9 lore raw

RFC PATCH 1/1 Implementation of non-blocking flash write/erase/status check functions.

In reply to Wolfgang Wegner

Dear Wolfgang Wegner,

Quoted 8 lines · In message <[email protected]> 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 <[email protected]>
---
 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

Signature · 7 lines
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
#10 lore raw

PATCH 1/1 non-blocking flash write/erase/status check functions

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 <[email protected]>
---
 drivers/mtd/cfi_flash.c |  268 +++++++++++++++++++++++++++++++++++------------
 include/flash.h         |    7 ++
 2 files changed, 207 insertions(+), 68 deletions(-)
drivers/mtd/cfi_flash.c +200 -68
index 4e8f5bf..e0c7f2e 100644
@@ -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 */
include/flash.h +7 -0
index 8feca1b..6b870d3 100644
@@ -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():
Signature · 1 line
1.5.6.5
author
#11 lore raw

RFC PATCH 1/1 Implementation of non-blocking flash write/erase/status check functions.

In reply to Wolfgang Denk

Dear Wolfgang Denk,

On Fri, Oct 30, 2009 at 07:22:17PM +0100, Wolfgang Denk wrote:
[...]

Quoted 4 lines
Quoted 1 line
 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

#12 lore raw

RFC PATCH v2 1/1 non-blocking flash write/erase/status check functions

In reply to Wolfgang Denk

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 <[email protected]>

---
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(-)
drivers/mtd/cfi_flash.c +201 -68
index c611f6b..74fa75f 100644
@@ -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 */
include/flash.h +9 -0
index 8feca1b..026497b 100644
@@ -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():
Signature · 1 line
1.5.6.5
author
#13 lore raw

RFC PATCH v2 1/1 non-blocking flash write/erase/status check functions

Hi,

any comments on this?

Quoted 4 lines · 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.

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

#14 lore raw

RFC PATCH v2 1/1 non-blocking flash write/erase/status check functions

Dear Wolfgang Wegner,

Quoted 11 lines · In message <[email protected]> you wrote:
Quoted 4 lines · 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.

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).

#15 lore raw

RFC PATCH v2 1/1 non-blocking flash write/erase/status check functions

Quoted 13 lines · On Friday 22 January 2010 13:03:40 Wolfgang Denk wrote:
Quoted 9 lines
Quoted 4 lines

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).

Quoted 6 lines

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