| Message ID | 71CF8D7F32C5C24C9CD1D0E02D52498A770D0BE6@NTXXIAMBX02.xacn.micron.com |
|---|---|
| State | Deferred |
| Delegated to: | Stefan Roese |
| Headers | show |
Dear Qi Wang 王起 (qiwang), In message <71CF8D7F32C5C24C9CD1D0E02D52498A770D0BE6@NTXXIAMBX02.xacn.micron.com> you wrote: > Micron Nor flash don't support read operation after send write command. As below, I understand you attempt to copy data between two different areas on the same NOR device? This is not supported. > If the src address locate in NOR flash, flash_read operation will be failed. > So, read out the data to DRAM before send write command operation. > --- > drivers/mtd/cfi_flash.c | 70 +++++++++++++++++++++++++++++++---------------- > 1 file changed, 46 insertions(+), 24 deletions(-) Thanks, but NAK. If the source address range and the target address range are both in the same NOR flash device, please use a two step approach instead: first copy the data to RAM, then copy from RAM to NOR. No code changes are needed for this. Best regards, Wolfgang Denk
Hi Wolfgang, > > Micron Nor flash don't support read operation after send write command. > As below, > > I understand you attempt to copy data between two different areas on > the same NOR device? This is not supported. > > > If the src address locate in NOR flash, flash_read operation will be failed. > > So, read out the data to DRAM before send write command operation. > > --- > > drivers/mtd/cfi_flash.c | 70 +++++++++++++++++++++++++++++++---------- > >------ > > 1 file changed, 46 insertions(+), 24 deletions(-) > > Thanks, but NAK. > > If the source address range and the target address range are both in > the same NOR flash device, please use a two step approach instead: > first copy the data to RAM, then copy from RAM to NOR. > > No code changes are needed for this. Yes, I totally agree with you, copy from NOR flash to NOR flash is not support. But in the flash_write_cfibuffer() function, flash_read16(src) is used to read data from src address, I think this "flash_read" function give users much mislead, src address can be located in flash address range, not only can be in RAM. And currently, one of micron customers uses "cp.b" command to copy data from NOR flash to Nor flash, such as "cp.b src_addr dest_addr len", both src_addr and dest_addr are located in NOR flash address range. Some manufactory's Nor flash can work fine under this command, but micron nor flash cannot. As you said, copy from NOR flash to NOR flash is forbidden. In the cp command function, do_mem_cp(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]), you can see below code, it just judge if the dest address is located in NOR flash, but don't do any detect on src address. #ifndef CONFIG_SYS_NO_FLASH /* check if we are copying to Flash */ if ( (addr2info(dest) != NULL) #ifdef CONFIG_HAS_DATAFLASH && (!addr_dataflash(dest)) #endif ) { int rc; puts ("Copy to Flash... "); rc = flash_write ((char *)addr, dest, count*size); if (rc != 0) { flash_perror (rc); return (1); } puts ("done\n"); return 0; } #endif So my suggestion is, 1. don't use flash_read function in flash_write_cfibuffer() function, use readb(),readw(), readl() function instead. 2. add detection in do_mem_cp() function, check if src address is locate in NOR flash address range. So what's your opinions? if it is ok for you, I can work out patch and send to you. Thanks for your help.
Hi Wolfgang, > > Micron Nor flash don't support read operation after send write command. > As below, > > I understand you attempt to copy data between two different areas on > the same NOR device? This is not supported. > > > If the src address locate in NOR flash, flash_read operation will be failed. > > So, read out the data to DRAM before send write command operation. > > --- > > drivers/mtd/cfi_flash.c | 70 +++++++++++++++++++++++++++++++---------- > >------ > > 1 file changed, 46 insertions(+), 24 deletions(-) > > Thanks, but NAK. > > If the source address range and the target address range are both in > the same NOR flash device, please use a two step approach instead: > first copy the data to RAM, then copy from RAM to NOR. > > No code changes are needed for this. Yes, I totally agree with you, copy from NOR flash to NOR flash is not support. But in the flash_write_cfibuffer() function, flash_read16(src) is used to read data from src address, I think this "flash_read" function give users much mislead, src address can be located in flash address range, not only can be in RAM. And currently, one of micron customers uses "cp.b" command to copy data from NOR flash to Nor flash, such as "cp.b src_addr dest_addr len", both src_addr and dest_addr are located in NOR flash address range. Some manufactory's Nor flash can work fine under this command, but micron nor flash cannot. As you said, copy from NOR flash to NOR flash is forbidden. In the cp command function, do_mem_cp(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]), you can see below code, it just judge if the dest address is located in NOR flash, but don't do any detect on src address. #ifndef CONFIG_SYS_NO_FLASH /* check if we are copying to Flash */ if ( (addr2info(dest) != NULL) #ifdef CONFIG_HAS_DATAFLASH && (!addr_dataflash(dest)) #endif ) { int rc; puts ("Copy to Flash... "); rc = flash_write ((char *)addr, dest, count*size); if (rc != 0) { flash_perror (rc); return (1); } puts ("done\n"); return 0; } #endif So my suggestion is, 1. don't use flash_read function in flash_write_cfibuffer() function, use readb(),readw(), readl() function instead. 2. add detection in do_mem_cp() function, check if src address is locate in NOR flash address range. So what's your opinions? Thanks for your help.
Dear Qi Wang 王起 (qiwang), In message <71CF8D7F32C5C24C9CD1D0E02D52498A770D149E@NTXXIAMBX02.xacn.micron.com> you wrote: > > Yes, I totally agree with you, copy from NOR flash to NOR flash is not support. Fine. > But in the flash_write_cfibuffer() function, flash_read16(src) is used to read data > from src address, I think this "flash_read" function give users much mislead, src address > can be located in flash address range, not only can be in RAM. Well, we cannot prevent users from misinterpreting the code. > And currently, one of micron customers uses "cp.b" command to copy data from NOR flash > to Nor flash, such as "cp.b src_addr dest_addr len", both src_addr and dest_addr are > located in NOR flash address range. Some manufactory's Nor flash can work fine under > this command, but micron nor flash cannot. U-Boot is a boot loader, i. e. we have always to find a compromize between resource consumption and functionality. Just because a user tries to do something that is not supported does not mean we should make the code bigger and more complex - users will find all kinds of exotic ways to do things. In this specific case, there is a simple and efficient way to perform the operation attempted by the user without code changes, so it makes sense to just educate this user. > As you said, copy from NOR flash to NOR flash is forbidden. > In the cp command function, > do_mem_cp(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]), > you can see below code, it just judge if the dest address is located in NOR flash, but > don't do any detect on src address. Yes, and this is perfectly OK so. Consider the case we have more than one NOR flash device on a system. It is perfectly legal to copy data from one NOR flash chip into another one. > So my suggestion is, > 1. don't use flash_read function in flash_write_cfibuffer() function, use > readb(),readw(), readl() function instead. > 2. add detection in do_mem_cp() function, check if src address is locate in NOR flash address range. > > So what's your opinions? I see no need for any code changes. Everything is working as expected. I repeat: U-Boot is a boot loader, not a general purpose OS. This means, that users of the command line interface aresupposed to understand what they are doing. There is no such strict error checking and measures against all possible kinds of incorrect or illegal input as you might expect from user space applications or from the OS. I think the only thing that should be done is to improve the documentation and explicitly warn that copying of data within the same bank of NOR flash cannot work. Please feel free to edit [1] as needed. Thanks. [1] http://www.denx.de/wiki/view/DULG/UBootCmdGroupFlash#Section_5.9.3.1. Best regards, Wolfgang Denk
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index a389cd1..0f532c0 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -25,6 +25,8 @@ #include <environment.h> #include <mtd/cfi_flash.h> #include <watchdog.h> +#include <malloc.h> +#include <asm-generic/errno.h> /* * This file implements a Common Flash Interface (CFI) driver for @@ -855,6 +857,8 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp, int cnt; int retcode; void *src = cp; + void *src2; + void *src2_bak; void *dst = (void *)dest; void *dst2 = dst; int flag = 1; @@ -880,29 +884,45 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp, goto out_unmap; } + src2 = malloc(len); + if(!src2) + { + free(src2); + return -ENOMEM; + } + + src2_bak = src2; cnt = len >> shift; while ((cnt-- > 0) && (flag == 1)) { switch (info->portwidth) { case FLASH_CFI_8BIT: - flag = ((flash_read8(dst2) & flash_read8(src)) == - flash_read8(src)); + *(u8 *)src2 = flash_read8(src); + flag = ((flash_read8(dst2) & *(u8 *)src2) == + *(u8 *)src2); src += 1, dst2 += 1; + src2 +=1; break; case FLASH_CFI_16BIT: - flag = ((flash_read16(dst2) & flash_read16(src)) == - flash_read16(src)); + *(u16 *)src2 = flash_read16(src); + flag = ((flash_read16(dst2) & *(u16 *)src2) == + *(u16 *)src2); src += 2, dst2 += 2; + src2 += 2; break; case FLASH_CFI_32BIT: - flag = ((flash_read32(dst2) & flash_read32(src)) == - flash_read32(src)); + *(u32 *)src2 = flash_read32(src); + flag = ((flash_read32(dst2) & *(u32 *)src2) == + *(u32 *)src2); src += 4, dst2 += 4; + src2 += 4; break; case FLASH_CFI_64BIT: - flag = ((flash_read64(dst2) & flash_read64(src)) == - flash_read64(src)); + *(u64 *)src2 = flash_read64(src); + flag = ((flash_read64(dst2) & *(u64 *)src2) == + *(u64 *)src2); src += 8, dst2 += 8; + src2 += 8; break; } } @@ -912,6 +932,7 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp, } src = cp; + src2 = src2_bak; sector = find_sector (info, dest); switch (info->vendor) { @@ -934,20 +955,20 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp, while (cnt-- > 0) { switch (info->portwidth) { case FLASH_CFI_8BIT: - flash_write8(flash_read8(src), dst); - src += 1, dst += 1; + flash_write8(*(u8 *)src2, dst); + src2 += 1, dst += 1; break; case FLASH_CFI_16BIT: - flash_write16(flash_read16(src), dst); - src += 2, dst += 2; + flash_write16(*(u16 *)src2, dst); + src2 += 2, dst += 2; break; case FLASH_CFI_32BIT: - flash_write32(flash_read32(src), dst); - src += 4, dst += 4; + flash_write32(*(u32 *)src2, dst); + src2 += 4, dst += 4; break; case FLASH_CFI_64BIT: - flash_write64(flash_read64(src), dst); - src += 8, dst += 8; + flash_write64(*(u64 *)src2, dst); + src2 += 8, dst += 8; break; default: retcode = ERR_INVAL; @@ -977,26 +998,26 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp, switch (info->portwidth) { case FLASH_CFI_8BIT: while (cnt-- > 0) { - flash_write8(flash_read8(src), dst); - src += 1, dst += 1; + flash_write8(*(u8 *)src2, dst); + src2 += 1, dst += 1; } break; case FLASH_CFI_16BIT: while (cnt-- > 0) { - flash_write16(flash_read16(src), dst); - src += 2, dst += 2; + flash_write16(*(u16 *)src2, dst); + src2 += 2, dst += 2; } break; case FLASH_CFI_32BIT: while (cnt-- > 0) { - flash_write32(flash_read32(src), dst); - src += 4, dst += 4; + flash_write32(*(u32 *)src2, dst); + src2 += 4, dst += 4; } break; case FLASH_CFI_64BIT: while (cnt-- > 0) { - flash_write64(flash_read64(src), dst); - src += 8, dst += 8; + flash_write64(*(u64 *)src2, dst); + src2 += 8, dst += 8; } break; default: @@ -1023,6 +1044,7 @@ static int flash_write_cfibuffer (flash_info_t * info, ulong dest, uchar * cp, } out_unmap: + free(src2_bak); return retcode; } #endif /* CONFIG_SYS_FLASH_USE_BUFFER_WRITE */
Micron Nor flash don't support read operation after send write command. As below, flash_write_cmd(info, sector, offset, AMD_CMD_WRITE_TO_BUFFER); cnt = len >> shift; flash_write_cmd(info, sector, offset, cnt - 1); switch (info->portwidth) { case FLASH_CFI_8BIT: while (cnt-- > 0) { flash_write8(flash_read8(src), dst); src += 1, dst += 1; } break; If the src address locate in NOR flash, flash_read operation will be failed. So, read out the data to DRAM before send write command operation. --- drivers/mtd/cfi_flash.c | 70 +++++++++++++++++++++++++++++++---------------- 1 file changed, 46 insertions(+), 24 deletions(-)