[U-Boot] non-blocking flash functions - is this possible/acceptable?
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
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
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
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
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
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
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).
From your description I feel little enthusiasm for such code to go
mainline.
First, U-Boot is by design single-threaded. There are no such things as "background tasks" running, and adding these seems little attractive to me. If you need such complicated things or protocols you should be using an OS instead.
Second, I recommend not to implement such an update scheme. It suffers from the problem that you first have to erase valid data (without backup), before you know if you will get a replacement for these. Normally I recommend never to start erasing the flash before the download of the new image has been completed, and the image has been checked for consistency. This makes reliable update procedures much easier. Agin, quite often it makes sense to implement this not in U-Boot, but in an OS instead.
Third, your decription makes it clear that the use of this new feature depends on a proprietary download protocol, which nobody else uses (and probably nobody ever will use), i. e. you are the only user of this feature.
On the other hand, our rul for accepting code is that it should be useful at least to some, as long as it doesn't hurt others. So before making a definitive statement I think we should be able to have a look at the code first.
Best regards,
Wolfgang Denk
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, §); + 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, §or); + 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, §); + 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, §or); + 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():
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
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
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():
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
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():
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
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).
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?
Best regards,
Wolfgang Denk
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@denx.de
participants (5)
-
Jerry Van Baren -
Stefan Roese -
Wolfgang Denk -
Wolfgang Wegner -
wolfgang＠leila.ping.de