[U-Boot] [PATCH] cfi_flash: Micron Nor flash don't support read operation after send

[U-Boot] [PATCH] cfi_flash: Micron Nor flash don't support read operation after send

[Qi Wang 王起 (qiwang)]

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

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 */
-- 
1.7.9.5


Best Regards, 
Qi Wang
EBU APAC SE
Tel: 86-021-38997158
Mobile: 86-15201958202
Email: qiwang at micron.com
Address: No 601 Fasai Rd, Pudong, Shanghai, China, 200131

[U-Boot] [PATCH] cfi_flash: Micron Nor flash don't support read operation after send

[Wolfgang Denk]

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

-- 
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
I wish I had a bronze torc for every user who didn't read the manual.
                             - Terry Pratchett, _The Light Fantastic_

[U-Boot] [PATCH] cfi_flash: Micron Nor flash don't support read operation after send

[Qi Wang 王起 (qiwang)]

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.

[U-Boot] [PATCH] cfi_flash: Micron Nor flash don't support read operation after send

[Qi Wang 王起 (qiwang)]

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.

[U-Boot] [PATCH] cfi_flash: Micron Nor flash don't support read operation after send

[Wolfgang Denk]

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

-- 
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
A consultant is a person who borrows your watch, tells you what  time
it is, pockets the watch, and sends you a bill for it.