All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
@ 2006-12-22  7:27 wei.zhang at freescale.com
  2006-12-22 10:00 ` Wolfgang Denk
  0 siblings, 1 reply; 55+ messages in thread
From: wei.zhang at freescale.com @ 2006-12-22  7:27 UTC (permalink / raw)
  To: u-boot

From: Zhang Wei <wei.zhang@freescale.com>

For the flash of port width more than 8bit, a completetly read must be performed. For example, 16bit port width flash must perform a ushort read. Otherwise, some flashes will get error data.

Signed-off-by: Zhang Wei <wei.zhang@freescale.com>
---
 drivers/cfi_flash.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/cfi_flash.c b/drivers/cfi_flash.c
index 9b10220..f02b047 100644
--- a/drivers/cfi_flash.c
+++ b/drivers/cfi_flash.c
@@ -250,9 +250,9 @@ #endif
  */
 inline uchar flash_read_uchar (flash_info_t * info, uint offset)
 {
-	uchar *cp;
+	uchar cp[FLASH_CFI_64BIT];
 
-	cp = flash_make_addr (info, 0, offset);
+	memcpy(cp, flash_make_addr (info, 0, offset), info->portwidth);
 #if defined(__LITTLE_ENDIAN)
 	return (cp[0]);
 #else
-- 
1.4.0

^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2006-12-22  7:27 [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug wei.zhang at freescale.com
@ 2006-12-22 10:00 ` Wolfgang Denk
  2006-12-22 11:02   ` Zhang Wei
  0 siblings, 1 reply; 55+ messages in thread
From: Wolfgang Denk @ 2006-12-22 10:00 UTC (permalink / raw)
  To: u-boot

In message <1166772458178-git-send-email-> you wrote:
> 
> For the flash of port width more than 8bit, a completetly read must
> be performed. For example, 16bit port width flash must perform a
> ushort read. Otherwise, some flashes will get error data.

Please explain under which circumstances you think this is necessary.

> Signed-off-by: Zhang Wei <wei.zhang@freescale.com>
> ---
>  drivers/cfi_flash.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cfi_flash.c b/drivers/cfi_flash.c
> index 9b10220..f02b047 100644
> --- a/drivers/cfi_flash.c
> +++ b/drivers/cfi_flash.c
> @@ -250,9 +250,9 @@ #endif
>   */
>  inline uchar flash_read_uchar (flash_info_t * info, uint offset)
>  {
> -	uchar *cp;
> +	uchar cp[FLASH_CFI_64BIT];
>  
> -	cp = flash_make_addr (info, 0, offset);
> +	memcpy(cp, flash_make_addr (info, 0, offset), info->portwidth);

This most probably does NOT perform any ushort reads at all, but does
a simple byte by byte copy, so your patch does not make much sense to
me.

Please explain in which sort of problems you see that are supposed to
be fixed by this modification.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The last thing one knows in constructing a work is what to put first.
- Blaise Pascal

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2006-12-22 10:00 ` Wolfgang Denk
@ 2006-12-22 11:02   ` Zhang Wei
  2006-12-22 14:11     ` Wolfgang Denk
  0 siblings, 1 reply; 55+ messages in thread
From: Zhang Wei @ 2006-12-22 11:02 UTC (permalink / raw)
  To: u-boot

Hi, Wolfgang,

This cfi-flash issue was found in u-boot 1.1.6 top git tree. On our 
board (MPC8641HPCn), sometimes the u-boot will get the wrong 
'num_erase_regions' (0xff) when it boots up. It causes the u-boot can 
not find and access the flash. If I remove the patch -- '[PATCH] CFI 
driver AMD Command Set Top boot geometry reversal, etc.' from Stefan 
Roese <sr@denx.de> in Nov. 13, 2006. All of the flash in u-boot are OK.

That patch adds support for reading JEDEC Manufacturer ID and Device ID 
causes. -- function flash_read_jedec_ids(). In our board, we have 
'Spinsion S29GL064M90TFIR6' flash with 16bit width port connection. 
After perform flash_read_jedec_ids(), the cfi query read will get an 
'0xff'. From this flash's specification, the read for flash commands in 
16bit port connection should perform 16bit read and ignore the high 8bit 
data. Although this issue maybe occurs in the special chip, perform the 
fully 16bit read is a safety operation.

The modification refers to the cfi flash drivers in Linux kernel (The 
cfi_read_query() function in include/linux/mtd/cfi.h file). It's more 
easy and clear than making the different type date read for different 
portwidth (such as ushort_read() for x16, ulong_read() for x32).

Thanks!

Best Regards,
Zhang Wei

Wolfgang Denk wrote:
> In message <1166772458178-git-send-email-> you wrote:
>   
>> For the flash of port width more than 8bit, a completetly read must
>> be performed. For example, 16bit port width flash must perform a
>> ushort read. Otherwise, some flashes will get error data.
>>     
>
> Please explain under which circumstances you think this is necessary.
>
>   
>> Signed-off-by: Zhang Wei <wei.zhang@freescale.com>
>> ---
>>  drivers/cfi_flash.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cfi_flash.c b/drivers/cfi_flash.c
>> index 9b10220..f02b047 100644
>> --- a/drivers/cfi_flash.c
>> +++ b/drivers/cfi_flash.c
>> @@ -250,9 +250,9 @@ #endif
>>   */
>>  inline uchar flash_read_uchar (flash_info_t * info, uint offset)
>>  {
>> -	uchar *cp;
>> +	uchar cp[FLASH_CFI_64BIT];
>>  
>> -	cp = flash_make_addr (info, 0, offset);
>> +	memcpy(cp, flash_make_addr (info, 0, offset), info->portwidth);
>>     
>
> This most probably does NOT perform any ushort reads at all, but does
> a simple byte by byte copy, so your patch does not make much sense to
> me.
>
> Please explain in which sort of problems you see that are supposed to
> be fixed by this modification.
>
> Best regards,
>
> Wolfgang Denk
>
>   

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2006-12-22 11:02   ` Zhang Wei
@ 2006-12-22 14:11     ` Wolfgang Denk
  2006-12-22 16:34       ` [U-Boot-Users] 答复: " Zhang Wei-r63237
  2006-12-22 17:07       ` Chris Fester
  0 siblings, 2 replies; 55+ messages in thread
From: Wolfgang Denk @ 2006-12-22 14:11 UTC (permalink / raw)
  To: u-boot

In message <458BBB5D.1030005@freescale.com> you wrote:
> 
> After perform flash_read_jedec_ids(), the cfi query read will get an 
> '0xff'. From this flash's specification, the read for flash commands in 
> 16bit port connection should perform 16bit read and ignore the high 8bit 
> data. Although this issue maybe occurs in the special chip, perform the 
> fully 16bit read is a safety operation.

But your patch does NOT perform a 16 bit read. It calls memcpy(); the
default implementation of memcpy [see lib_generic/string.c] does this:

	char *tmp = (char *) dest, *s = (char *) src;

	while (count--)
		*tmp++ = *s++;

i. e. it performs a character copy, too, so it makes no difference
compared to the original code.

> The modification refers to the cfi flash drivers in Linux kernel (The 
> cfi_read_query() function in include/linux/mtd/cfi.h file). It's more 
> easy and clear than making the different type date read for different 
> portwidth (such as ushort_read() for x16, ulong_read() for x32).

I understand your intentions, but your patch does  not  do  what  you
think it does, so if it really fixes the problem on your system there
must be another cause or effect.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Emotions are alien to me.  I'm a scientist.
	-- Spock, "This Side of Paradise", stardate 3417.3

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] 答复: [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug. 
  2006-12-22 14:11     ` Wolfgang Denk
@ 2006-12-22 16:34       ` Zhang Wei-r63237
  2006-12-22 17:17         ` Wolfgang Denk
  2006-12-22 17:07       ` Chris Fester
  1 sibling, 1 reply; 55+ messages in thread
From: Zhang Wei-r63237 @ 2006-12-22 16:34 UTC (permalink / raw)
  To: u-boot

Hi, Wolfgang,

Yes, the memcpy() is just a byte copy. But a x16 read can be emulated by two x8 read. And in fact, the flash_read_ushort(), flash_read_long() in cfi_flash.c are using the same implementation.

In addition, the original code only reads 8bit, not the full 16bit. My patch ensures the full 16bit data are read completely.

Thanks!

Best Regards,
Zhang Wei


-----Original Message-----
From: Wolfgang Denk [mailto:wd at denx.de]
Sent: 2006-12-22 (???) 22:11
To: Zhang Wei-r63237
Cc: u-boot-users at lists.sourceforge.net
Subject: Re: [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug. 
 
In message <458BBB5D.1030005@freescale.com> you wrote:
> 
> After perform flash_read_jedec_ids(), the cfi query read will get an 
> '0xff'. From this flash's specification, the read for flash commands in 
> 16bit port connection should perform 16bit read and ignore the high 8bit 
> data. Although this issue maybe occurs in the special chip, perform the 
> fully 16bit read is a safety operation.

But your patch does NOT perform a 16 bit read. It calls memcpy(); the
default implementation of memcpy [see lib_generic/string.c] does this:

	char *tmp = (char *) dest, *s = (char *) src;

	while (count--)
		*tmp++ = *s++;

i. e. it performs a character copy, too, so it makes no difference
compared to the original code.

> The modification refers to the cfi flash drivers in Linux kernel (The 
> cfi_read_query() function in include/linux/mtd/cfi.h file). It's more 
> easy and clear than making the different type date read for different 
> portwidth (such as ushort_read() for x16, ulong_read() for x32).

I understand your intentions, but your patch does  not  do  what  you
think it does, so if it really fixes the problem on your system there
must be another cause or effect.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Emotions are alien to me.  I'm a scientist.
	-- Spock, "This Side of Paradise", stardate 3417.3

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.denx.de/pipermail/u-boot/attachments/20061223/e5b62754/attachment.htm 

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2006-12-22 14:11     ` Wolfgang Denk
  2006-12-22 16:34       ` [U-Boot-Users] 答复: " Zhang Wei-r63237
@ 2006-12-22 17:07       ` Chris Fester
  2006-12-22 17:24         ` Wolfgang Denk
  1 sibling, 1 reply; 55+ messages in thread
From: Chris Fester @ 2006-12-22 17:07 UTC (permalink / raw)
  To: u-boot

I'm interested in how this discussion pans out.  I have encountered this
problem (with flinfo) with my MPC8641HPCN.  The patch presented by Zhang
Wei (and other freescale employees) does seem to allieviate the problem
when I compile u-boot with ELDK 4.0.

However, some scenarios here worry me:
-- with the patch, compiled with ELDK 4.0 is peachy.
-- without the patch, compiled with ELDK 4.0 FAILS.
-- without the patch, compiled with ELDK 3.1.1 is peachy.

At the time of my testing (early this week) I was using a git tree about
2 weeks old from jdl.com, which is what the freescale folks seem to be
using at the moment.  It worries me that changing a compiler version
could affect this bug, but I guess I've seen stranger things.

Could this be a subtle timing problem with the processor?  Could it be a
timing problem with the flash?  Am I nuts?  :P

Merry Christmas!
Chris

On Fri, 2006-12-22 at 15:11 +0100, Wolfgang Denk wrote:
> In message <458BBB5D.1030005@freescale.com> you wrote:
> > 
> > After perform flash_read_jedec_ids(), the cfi query read will get an 
> > '0xff'. From this flash's specification, the read for flash commands in 
> > 16bit port connection should perform 16bit read and ignore the high 8bit 
> > data. Although this issue maybe occurs in the special chip, perform the 
> > fully 16bit read is a safety operation.
> 
> But your patch does NOT perform a 16 bit read. It calls memcpy(); the
> default implementation of memcpy [see lib_generic/string.c] does this:
> 
> 	char *tmp = (char *) dest, *s = (char *) src;
> 
> 	while (count--)
> 		*tmp++ = *s++;
> 
> i. e. it performs a character copy, too, so it makes no difference
> compared to the original code.
> 
> > The modification refers to the cfi flash drivers in Linux kernel (The 
> > cfi_read_query() function in include/linux/mtd/cfi.h file). It's more 
> > easy and clear than making the different type date read for different 
> > portwidth (such as ushort_read() for x16, ulong_read() for x32).
> 
> I understand your intentions, but your patch does  not  do  what  you
> think it does, so if it really fixes the problem on your system there
> must be another cause or effect.
> 
> Best regards,
> 
> Wolfgang Denk
> 

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] 答复: [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug. 
  2006-12-22 16:34       ` [U-Boot-Users] 答复: " Zhang Wei-r63237
@ 2006-12-22 17:17         ` Wolfgang Denk
  2006-12-25  7:03           ` Zhang Wei
  0 siblings, 1 reply; 55+ messages in thread
From: Wolfgang Denk @ 2006-12-22 17:17 UTC (permalink / raw)
  To: u-boot

In message <2176B872C0407E44887F07CCAA869293832458@zch01exm21.fsl.freescale.net> you wrote:
> 
> Yes, the memcpy() is just a byte copy. But a x16 read can be emulated by
> two x8 read. And in fact, the flash_read_ushort(), flash_read_long() in
> cfi_flash.c are using the same implementation.

But these are supposed to read more than one byte.

> In addition, the original code only reads 8bit, not the full 16bit. My
> patch ensures the full 16bit data are read completely.

I still don't understand why flash_read_uchar() should read more than
one byte? If you need a 16 bit read operation I would expect  you  to
use flash_read_ushort() instead.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Save energy:  Drive a smaller shell.

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2006-12-22 17:07       ` Chris Fester
@ 2006-12-22 17:24         ` Wolfgang Denk
  2006-12-22 17:42           ` Chris Fester
  0 siblings, 1 reply; 55+ messages in thread
From: Wolfgang Denk @ 2006-12-22 17:24 UTC (permalink / raw)
  To: u-boot

In message <1166807257.26147.25.camel@kaboom.lisle.iphase.com> you wrote:
> 
> However, some scenarios here worry me:
> -- with the patch, compiled with ELDK 4.0 is peachy.
> -- without the patch, compiled with ELDK 4.0 FAILS.
> -- without the patch, compiled with ELDK 3.1.1 is peachy.

Now *this* is interesting information.

Did you compare the generated code?

> On Fri, 2006-12-22 at 15:11 +0100, Wolfgang Denk wrote:

If you want to make me a Xmas present, then please stop top posting /
full quoting (the same goes to Zhang Wei); please read
http://www.netmeister.org/news/learn2quote.html



Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"If it ain't broke, don't fix it."                       - Bert Lantz

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2006-12-22 17:24         ` Wolfgang Denk
@ 2006-12-22 17:42           ` Chris Fester
  2006-12-22 21:33             ` Wolfgang Denk
  0 siblings, 1 reply; 55+ messages in thread
From: Chris Fester @ 2006-12-22 17:42 UTC (permalink / raw)
  To: u-boot

On Fri, 2006-12-22 at 18:24 +0100, Wolfgang Denk wrote:
> Did you compare the generated code?

Nope, but that's a good idea.  I'll compile and objdump the various
scenarios next year.

Merry Christmas from a very bad quoting net citizen.  :P
Chris

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2006-12-22 17:42           ` Chris Fester
@ 2006-12-22 21:33             ` Wolfgang Denk
  0 siblings, 0 replies; 55+ messages in thread
From: Wolfgang Denk @ 2006-12-22 21:33 UTC (permalink / raw)
  To: u-boot

Hello,

in message <1166809358.26147.32.camel@kaboom.lisle.iphase.com> you wrote:
>
> > Did you compare the generated code?
> 
> Nope, but that's a good idea.  I'll compile and objdump the various
> scenarios next year.

I just did this - this is always pretty  instructive  in  such  situ-
ations.  GCC-4.x  optimizes much better, but will also unremorsefully
punish even small sins like a forgotten "volatile".

   text    data     bss     dec     hex filename
 202340   12104   32056  246500   3c2e4 /tmp/ELDK-3.1.1/u-boot
 185751   12016   32048  229815   381b7 /tmp/ELDK-4.0/u-boot
 ------

That's nearly 10% difference in code size...

But here the difference is not in this function  -  both  compile  to
exactly  the  same  code.  Obviously  some other functions in the CFI
driver are different, though.

> Merry Christmas from a very bad quoting net citizen.  :P

Thanks, and Merry Christmas, too.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It is easier to change the specification to fit the program than vice
versa.

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2006-12-22 17:17         ` Wolfgang Denk
@ 2006-12-25  7:03           ` Zhang Wei
  2006-12-25 23:24             ` Wolfgang Denk
  0 siblings, 1 reply; 55+ messages in thread
From: Zhang Wei @ 2006-12-25  7:03 UTC (permalink / raw)
  To: u-boot

Hi, Wolfgang,

Merry X'mas!

I want to explain two key points. I also take the x16 connection flash 
example.

1. Reading 16bit data in flash_read_uchar() will not cause an error.
    We can get an example from the CFI specification. We run the CFI 
Query-unique ACSII string "QRY" command on x16 device with x16 mode 
connection. From the word address 0x10, 0x11, 0x12, we could get "Q", 
"R", "Y". If using byte address, we should get them like below: byte 
address 0x20 got "Q", 0x21 got 0x0, 0x22 got "R", 0x23 got 0x0, 0x24 got 
"Y", 0x25 got 0x0. In the u-boot, The word address offset '0x10' in 
flash_read_uchar() will be translated to byte address '0x20' by 
flash_make_addr(). I get the high 8-bit in flash_read_uchar(), which is 
just a NULL and discarded.

2. Reading 16bit data in flash_read_uchar() is a safety operation, which 
will avoid the potential problem.
    In the Linux kernel, the cfi driver using the similar 
implementation. I think reading the full width bit data from the flash 
port may clear the flash data buffer. I've tested to add 'volatile' 
keyword to the flash_get_size() and flash_read_jedec_ids() functions' 
declaration. The flash can not work also. From Chris' work,  the  
compiler's optimizing causes some flash to get the error.

Thanks!

Best Regards,
Zhang Wei

Wolfgang Denk wrote:
> In message <2176B872C0407E44887F07CCAA869293832458@zch01exm21.fsl.freescale.net> you wrote:
>   
>> Yes, the memcpy() is just a byte copy. But a x16 read can be emulated by
>> two x8 read. And in fact, the flash_read_ushort(), flash_read_long() in
>> cfi_flash.c are using the same implementation.
>>     
>
> But these are supposed to read more than one byte.
>
>   
>> In addition, the original code only reads 8bit, not the full 16bit. My
>> patch ensures the full 16bit data are read completely.
>>     
>
> I still don't understand why flash_read_uchar() should read more than
> one byte? If you need a 16 bit read operation I would expect  you  to
> use flash_read_ushort() instead.
>
> Best regards,
>
> Wolfgang Denk
>
>   

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2006-12-25  7:03           ` Zhang Wei
@ 2006-12-25 23:24             ` Wolfgang Denk
  2006-12-26  6:04               ` Zhang Wei
  2007-01-04  8:36               ` Zhang Wei-r63237
  0 siblings, 2 replies; 55+ messages in thread
From: Wolfgang Denk @ 2006-12-25 23:24 UTC (permalink / raw)
  To: u-boot

Dear Zhang Wei,

in message <458F77D0.10203@freescale.com> you wrote:
> 
> Merry X'mas!

Thanks, and the same to you.

> I want to explain two key points. I also take the x16 connection flash 
> example.

OK.

> 1. Reading 16bit data in flash_read_uchar() will not cause an error.

OK.

> 2. Reading 16bit data in flash_read_uchar() is a safety operation, which 
> will avoid the potential problem.

Now this is what I want to understand. What exactly is the "potential
problem"?

>     In the Linux kernel, the cfi driver using the similar 

Can you please point out which specific part of the Linux MTD code you
mean? And which version of the code?

> implementation. I think reading the full width bit data from the flash 
> port may clear the flash data buffer. I've tested to add 'volatile' 

I have to admit that I don't really understand this. I would not be
surprised that some statement like this can be found in some chip
errata, but I would like to know this for certain first.

> keyword to the flash_get_size() and flash_read_jedec_ids() functions' 
> declaration. The flash can not work also. From Chris' work,  the  
> compiler's optimizing causes some flash to get the error.

For me this is an indication that the problem is  actually  somewhere
else,  and while your modification might actually fix the symptoms, I
doubt that it is the correct fix - or the correct  problem.  If  your
explanation was right, this should not depend on compiler versions.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Any sufficiently advanced  technology  is  indistinguishable  from  a
rigged demo.                              - Andy Finkel, computer guy

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2006-12-25 23:24             ` Wolfgang Denk
@ 2006-12-26  6:04               ` Zhang Wei
  2007-01-04  2:07                 ` Zhang Wei-r63237
  2007-01-04  8:36               ` Zhang Wei-r63237
  1 sibling, 1 reply; 55+ messages in thread
From: Zhang Wei @ 2006-12-26  6:04 UTC (permalink / raw)
  To: u-boot

Hi, Wolfgang,

Wolfgang Denk wrote:
> Now this is what I want to understand. What exactly is the "potential
> problem"?
>   
That's the issue in the flash 'Spinsion S29GL064M90TFIR6' with x16 
connection. After running flash_read_jedec_ids(), any follow CFI query 
command will get the data with high 8bit = 0xff, but the low 8bit is 
valid. And if we only read low 8bit, we'll get the 0xff too. In 
addition, the second follow CFI query command has no that issue. So, I 
read the full 16bit date and only take the valid low 8bit.
> Can you please point out which specific part of the Linux MTD code you
> mean? And which version of the code?
>   
The reference codes is in Linux Kernel 2.6.19.

drivers/mtd/chips/cfi_probe.c: cfi_chip_setup() calls:
    int num_erase_regions = cfi_read_query(map, base + (0x10 + 
28)*ofs_factor);
include/linux/mtd/cfi.h: cfi_read_query() calls:
    map_word val = map_read(map, addr);
include/linux/mtd/map.h defines:
    #define map_read(map, ofs) inline_map_read(map, ofs)
include/linux/mtd/map.h: function inline_map_read() body:
static inline map_word inline_map_read(struct map_info *map, unsigned 
long ofs)
{
    map_word r;

    if (map_bankwidth_is_1(map))
        r.x[0] = __raw_readb(map->virt + ofs);
    else if (map_bankwidth_is_2(map))
        r.x[0] = __raw_readw(map->virt + ofs);
    else if (map_bankwidth_is_4(map))
        r.x[0] = __raw_readl(map->virt + ofs);
#if BITS_PER_LONG >= 64
    else if (map_bankwidth_is_8(map))
        r.x[0] = __raw_readq(map->virt + ofs);
#endif
    else if (map_bankwidth_is_large(map))
        memcpy_fromio(r.x, map->virt+ofs, map->bankwidth);

    return r;
}
And the 'map_word' definition in include/linux/mtd/map.h is below:
typedef union {
    unsigned long x[MAX_MAP_LONGS];
} map_word;

> I have to admit that I don't really understand this. I would not be
> surprised that some statement like this can be found in some chip
> errata, but I would like to know this for certain first.
>   
I only find one clue from 'Spinsion S29GL064M90TFIR6' specification, 
which is the comment 'Data bits DQ15?DQ8 are don?t care. ' for 'RESET' 
command. And the comment has not found in some other AMD, Spinsion chips 
specifications.
> For me this is an indication that the problem is  actually  somewhere
> else,  and while your modification might actually fix the symptoms, I
> doubt that it is the correct fix - or the correct  problem.  If  your
> explanation was right, this should not depend on compiler versions.
>   

This is a real weird issue. The compiler is also a factor. Chris's 
different compilers get the different results. In fact, the same gcc 
with different size codes will also get different results.

Thanks!

Best Regards,
Zhang Wei

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2006-12-26  6:04               ` Zhang Wei
@ 2007-01-04  2:07                 ` Zhang Wei-r63237
  2007-01-04  8:17                   ` Wolfgang Denk
  0 siblings, 1 reply; 55+ messages in thread
From: Zhang Wei-r63237 @ 2007-01-04  2:07 UTC (permalink / raw)
  To: u-boot

Hi, Wolfgang,

Any feedback about this mail?

Thanks!

Zhang Wei 

> -----Original Message-----
> From: u-boot-users-bounces at lists.sourceforge.net 
> [mailto:u-boot-users-bounces at lists.sourceforge.net] On Behalf 
> Of Zhang Wei
> Sent: Tuesday, December 26, 2006 2:05 PM
> To: Wolfgang Denk
> Cc: u-boot-users at lists.sourceforge.net
> Subject: Re: [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
> 
> Hi, Wolfgang,
> 
> Wolfgang Denk wrote:
> > Now this is what I want to understand. What exactly is the 
> "potential
> > problem"?
> >   
> That's the issue in the flash 'Spinsion S29GL064M90TFIR6' with x16 
> connection. After running flash_read_jedec_ids(), any follow 
> CFI query 
> command will get the data with high 8bit = 0xff, but the low 8bit is 
> valid. And if we only read low 8bit, we'll get the 0xff too. In 
> addition, the second follow CFI query command has no that 
> issue. So, I 
> read the full 16bit date and only take the valid low 8bit.
> > Can you please point out which specific part of the Linux 
> MTD code you
> > mean? And which version of the code?
> >   
> The reference codes is in Linux Kernel 2.6.19.
> 
> drivers/mtd/chips/cfi_probe.c: cfi_chip_setup() calls:
>     int num_erase_regions = cfi_read_query(map, base + (0x10 + 
> 28)*ofs_factor);
> include/linux/mtd/cfi.h: cfi_read_query() calls:
>     map_word val = map_read(map, addr);
> include/linux/mtd/map.h defines:
>     #define map_read(map, ofs) inline_map_read(map, ofs)
> include/linux/mtd/map.h: function inline_map_read() body:
> static inline map_word inline_map_read(struct map_info *map, unsigned 
> long ofs)
> {
>     map_word r;
> 
>     if (map_bankwidth_is_1(map))
>         r.x[0] = __raw_readb(map->virt + ofs);
>     else if (map_bankwidth_is_2(map))
>         r.x[0] = __raw_readw(map->virt + ofs);
>     else if (map_bankwidth_is_4(map))
>         r.x[0] = __raw_readl(map->virt + ofs);
> #if BITS_PER_LONG >= 64
>     else if (map_bankwidth_is_8(map))
>         r.x[0] = __raw_readq(map->virt + ofs);
> #endif
>     else if (map_bankwidth_is_large(map))
>         memcpy_fromio(r.x, map->virt+ofs, map->bankwidth);
> 
>     return r;
> }
> And the 'map_word' definition in include/linux/mtd/map.h is below:
> typedef union {
>     unsigned long x[MAX_MAP_LONGS];
> } map_word;
> 
> > I have to admit that I don't really understand this. I would not be
> > surprised that some statement like this can be found in some chip
> > errata, but I would like to know this for certain first.
> >   
> I only find one clue from 'Spinsion S29GL064M90TFIR6' specification, 
> which is the comment 'Data bits DQ15?DQ8 are don?t care. ' 
> for 'RESET' 
> command. And the comment has not found in some other AMD, 
> Spinsion chips 
> specifications.
> > For me this is an indication that the problem is  actually  
> somewhere
> > else,  and while your modification might actually fix the 
> symptoms, I
> > doubt that it is the correct fix - or the correct  problem. 
>  If  your
> > explanation was right, this should not depend on compiler versions.
> >   
> 
> This is a real weird issue. The compiler is also a factor. Chris's 
> different compilers get the different results. In fact, the same gcc 
> with different size codes will also get different results.
> 
> Thanks!
> 
> Best Regards,
> Zhang Wei
> 
> 
> --------------------------------------------------------------
> -----------
> Take Surveys. Earn Cash. Influence the Future of IT
> Join SourceForge.net's Techsay panel and you'll get the 
> chance to share your
> opinions on IT & business topics through brief surveys - and earn cash
> http://www.techsay.com/default.php?page=join.php&p=sourceforge
&CID=DEVDEV
> _______________________________________________
> U-Boot-Users mailing list
> U-Boot-Users at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/u-boot-users
> 

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-04  2:07                 ` Zhang Wei-r63237
@ 2007-01-04  8:17                   ` Wolfgang Denk
  0 siblings, 0 replies; 55+ messages in thread
From: Wolfgang Denk @ 2007-01-04  8:17 UTC (permalink / raw)
  To: u-boot

In message <46B96294322F7D458F9648B60E15112C03EE48@zch01exm26.fsl.freescale.net> you wrote:
> SGksIFdvbGZnYW5nLA0KDQpBbnkgZmVlZGJhY2sgYWJvdXQgdGhpcyBtYWlsPw0KDQpUaGFua3Mh
> DQoNClpoYW5nIFdlaSANCg0KPiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiB1
> LWJvb3QtdXNlcnMtYm91bmNlc0BsaXN0cy5zb3VyY2Vmb3JnZS5uZXQgDQo+IFttYWlsdG86dS1i
> b290LXVzZXJzLWJvdW5jZXNAbGlzdHMuc291cmNlZm9yZ2UubmV0XSBPbiBCZWhhbGYgDQo+IE9m
> IFpoYW5nIFdlaQ0KPiBTZW50OiBUdWVzZGF5LCBEZWNlbWJlciAyNiwgMjAwNiAyOjA1IFBNDQo+
> IFRvOiBXb2xmZ2FuZyBEZW5rDQo+IENjOiB1LWJvb3QtdXNlcnNAbGlzdHMuc291cmNlZm9yZ2Uu
> bmV0DQo+IFN1YmplY3Q6IFJlOiBbVS1Cb290LVVzZXJzXSBbUEFUQ0hdIEZpeGVkIGNmaSBmbGFz
> aCByZWFkIHVjaGFyIGJ1Zy4NCj4gDQo+IEhpLCBXb2xmZ2FuZywNCj4gDQo+IFdvbGZnYW5nIERl
> bmsgd3JvdGU6DQo+ID4gTm93IHRoaXMgaXMgd2hhdCBJIHdhbnQgdG8gdW5kZXJzdGFuZC4gV2hh
> dCBleGFjdGx5IGlzIHRoZSANCj4gInBvdGVudGlhbA0KPiA+IHByb2JsZW0iPw0KPiA+ICAgDQo+
> IFRoYXQncyB0aGUgaXNzdWUgaW4gdGhlIGZsYXNoICdTcGluc2lvbiBTMjlHTDA2NE05MFRGSVI2
> JyB3aXRoIHgxNiANCj4gY29ubmVjdGlvbi4gQWZ0ZXIgcnVubmluZyBmbGFzaF9yZWFkX2plZGVj
> X2lkcygpLCBhbnkgZm9sbG93IA0KPiBDRkkgcXVlcnkgDQo+IGNvbW1hbmQgd2lsbCBnZXQgdGhl
> IGRhdGEgd2l0aCBoaWdoIDhiaXQgPSAweGZmLCBidXQgdGhlIGxvdyA4Yml0IGlzIA0KPiB2YWxp
> ZC4gQW5kIGlmIHdlIG9ubHkgcmVhZCBsb3cgOGJpdCwgd2UnbGwgZ2V0IHRoZSAweGZmIHRvby4g
> SW4gDQo+IGFkZGl0aW9uLCB0aGUgc2Vjb25kIGZvbGxvdyBDRkkgcXVlcnkgY29tbWFuZCBoYXMg
> bm8gdGhhdCANCj4gaXNzdWUuIFNvLCBJIA0KPiByZWFkIHRoZSBmdWxsIDE2Yml0IGRhdGUgYW5k
> IG9ubHkgdGFrZSB0aGUgdmFsaWQgbG93IDhiaXQuDQo+ID4gQ2FuIHlvdSBwbGVhc2UgcG9pbnQg
> b3V0IHdoaWNoIHNwZWNpZmljIHBhcnQgb2YgdGhlIExpbnV4IA0KPiBNVEQgY29kZSB5b3UNCj4g
> PiBtZWFuPyBBbmQgd2hpY2ggdmVyc2lvbiBvZiB0aGUgY29kZT8NCj4gPiAgIA0KPiBUaGUgcmVm
> ZXJlbmNlIGNvZGVzIGlzIGluIExpbnV4IEtlcm5lbCAyLjYuMTkuDQo+IA0KPiBkcml2ZXJzL210
> ZC9jaGlwcy9jZmlfcHJvYmUuYzogY2ZpX2NoaXBfc2V0dXAoKSBjYWxsczoNCj4gICAgIGludCBu
> dW1fZXJhc2VfcmVnaW9ucyA9IGNmaV9yZWFkX3F1ZXJ5KG1hcCwgYmFzZSArICgweDEwICsgDQo+
> IDI4KSpvZnNfZmFjdG9yKTsNCj4gaW5jbHVkZS9saW51eC9tdGQvY2ZpLmg6IGNmaV9yZWFkX3F1
> ZXJ5KCkgY2FsbHM6DQo+ICAgICBtYXBfd29yZCB2YWwgPSBtYXBfcmVhZChtYXAsIGFkZHIpOw0K
> PiBpbmNsdWRlL2xpbnV4L210ZC9tYXAuaCBkZWZpbmVzOg0KPiAgICAgI2RlZmluZSBtYXBfcmVh
> ZChtYXAsIG9mcykgaW5saW5lX21hcF9yZWFkKG1hcCwgb2ZzKQ0KPiBpbmNsdWRlL2xpbnV4L210
> ZC9tYXAuaDogZnVuY3Rpb24gaW5saW5lX21hcF9yZWFkKCkgYm9keToNCj4gc3RhdGljIGlubGlu
> ZSBtYXBfd29yZCBpbmxpbmVfbWFwX3JlYWQoc3RydWN0IG1hcF9pbmZvICptYXAsIHVuc2lnbmVk
> IA0KPiBsb25nIG9mcykNCj4gew0KPiAgICAgbWFwX3dvcmQgcjsNCj4gDQo+ICAgICBpZiAobWFw
> X2Jhbmt3aWR0aF9pc18xKG1hcCkpDQo+ICAgICAgICAgci54WzBdID0gX19yYXdfcmVhZGIobWFw
> LT52aXJ0ICsgb2ZzKTsNCj4gICAgIGVsc2UgaWYgKG1hcF9iYW5rd2lkdGhfaXNfMihtYXApKQ0K
> PiAgICAgICAgIHIueFswXSA9IF9fcmF3X3JlYWR3KG1hcC0+dmlydCArIG9mcyk7DQo+ICAgICBl
> bHNlIGlmIChtYXBfYmFua3dpZHRoX2lzXzQobWFwKSkNCj4gICAgICAgICByLnhbMF0gPSBfX3Jh
> d19yZWFkbChtYXAtPnZpcnQgKyBvZnMpOw0KPiAjaWYgQklUU19QRVJfTE9ORyA+PSA2NA0KPiAg
> ICAgZWxzZSBpZiAobWFwX2Jhbmt3aWR0aF9pc184KG1hcCkpDQo+ICAgICAgICAgci54WzBdID0g
> X19yYXdfcmVhZHEobWFwLT52aXJ0ICsgb2ZzKTsNCj4gI2VuZGlmDQo+ICAgICBlbHNlIGlmICht
> YXBfYmFua3dpZHRoX2lzX2xhcmdlKG1hcCkpDQo+ICAgICAgICAgbWVtY3B5X2Zyb21pbyhyLngs
> IG1hcC0+dmlydCtvZnMsIG1hcC0+YmFua3dpZHRoKTsNCj4gDQo+ICAgICByZXR1cm4gcjsNCj4g
> fQ0KPiBBbmQgdGhlICdtYXBfd29yZCcgZGVmaW5pdGlvbiBpbiBpbmNsdWRlL2xpbnV4L210ZC9t
> YXAuaCBpcyBiZWxvdzoNCj4gdHlwZWRlZiB1bmlvbiB7DQo+ICAgICB1bnNpZ25lZCBsb25nIHhb
> TUFYX01BUF9MT05HU107DQo+IH0gbWFwX3dvcmQ7DQo+IA0KPiA+IEkgaGF2ZSB0byBhZG1pdCB0
> aGF0IEkgZG9uJ3QgcmVhbGx5IHVuZGVyc3RhbmQgdGhpcy4gSSB3b3VsZCBub3QgYmUNCj4gPiBz
> dXJwcmlzZWQgdGhhdCBzb21lIHN0YXRlbWVudCBsaWtlIHRoaXMgY2FuIGJlIGZvdW5kIGluIHNv
> bWUgY2hpcA0KPiA+IGVycmF0YSwgYnV0IEkgd291bGQgbGlrZSB0byBrbm93IHRoaXMgZm9yIGNl
> cnRhaW4gZmlyc3QuDQo+ID4gICANCj4gSSBvbmx5IGZpbmQgb25lIGNsdWUgZnJvbSAnU3BpbnNp
> b24gUzI5R0wwNjRNOTBURklSNicgc3BlY2lmaWNhdGlvbiwgDQo+IHdoaWNoIGlzIHRoZSBjb21t
> ZW50ICdEYXRhIGJpdHMgRFExNeKAk0RROCBhcmUgZG9u4oCZdCBjYXJlLiAnIA0KPiBmb3IgJ1JF
> U0VUJyANCj4gY29tbWFuZC4gQW5kIHRoZSBjb21tZW50IGhhcyBub3QgZm91bmQgaW4gc29tZSBv
> dGhlciBBTUQsIA0KPiBTcGluc2lvbiBjaGlwcyANCj4gc3BlY2lmaWNhdGlvbnMuDQo+ID4gRm9y
> IG1lIHRoaXMgaXMgYW4gaW5kaWNhdGlvbiB0aGF0IHRoZSBwcm9ibGVtIGlzICBhY3R1YWxseSAg
> DQo+IHNvbWV3aGVyZQ0KPiA+IGVsc2UsICBhbmQgd2hpbGUgeW91ciBtb2RpZmljYXRpb24gbWln
> aHQgYWN0dWFsbHkgZml4IHRoZSANCj4gc3ltcHRvbXMsIEkNCj4gPiBkb3VidCB0aGF0IGl0IGlz
> IHRoZSBjb3JyZWN0IGZpeCAtIG9yIHRoZSBjb3JyZWN0ICBwcm9ibGVtLiANCj4gIElmICB5b3Vy
> DQo+ID4gZXhwbGFuYXRpb24gd2FzIHJpZ2h0LCB0aGlzIHNob3VsZCBub3QgZGVwZW5kIG9uIGNv
> bXBpbGVyIHZlcnNpb25zLg0KPiA+ICAgDQo+IA0KPiBUaGlzIGlzIGEgcmVhbCB3ZWlyZCBpc3N1
> ZS4gVGhlIGNvbXBpbGVyIGlzIGFsc28gYSBmYWN0b3IuIENocmlzJ3MgDQo+IGRpZmZlcmVudCBj
> b21waWxlcnMgZ2V0IHRoZSBkaWZmZXJlbnQgcmVzdWx0cy4gSW4gZmFjdCwgdGhlIHNhbWUgZ2Nj
> IA0KPiB3aXRoIGRpZmZlcmVudCBzaXplIGNvZGVzIHdpbGwgYWxzbyBnZXQgZGlmZmVyZW50IHJl
> c3VsdHMuDQo+IA0KPiBUaGFua3MhDQo+IA0KPiBCZXN0IFJlZ2FyZHMsDQo+IFpoYW5nIFdlaQ0K
> PiANCj4gDQo+IC0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
> LS0tLS0tLS0tLS0tLS0tDQo+IC0tLS0tLS0tLS0tDQo+IFRha2UgU3VydmV5cy4gRWFybiBDYXNo
> LiBJbmZsdWVuY2UgdGhlIEZ1dHVyZSBvZiBJVA0KPiBKb2luIFNvdXJjZUZvcmdlLm5ldCdzIFRl
> Y2hzYXkgcGFuZWwgYW5kIHlvdSdsbCBnZXQgdGhlIA0KPiBjaGFuY2UgdG8gc2hhcmUgeW91cg0K
> PiBvcGluaW9ucyBvbiBJVCAmIGJ1c2luZXNzIHRvcGljcyB0aHJvdWdoIGJyaWVmIHN1cnZleXMg
> LSBhbmQgZWFybiBjYXNoDQo+IGh0dHA6Ly93d3cudGVjaHNheS5jb20vZGVmYXVsdC5waHA/cGFn
> ZT1qb2luLnBocCZwPXNvdXJjZWZvcmdlDQomQ0lEPURFVkRFVg0KPiBfX19fX19fX19fX19fX19f
> X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXw0KPiBVLUJvb3QtVXNlcnMgbWFpbGluZyBs
> aXN0DQo+IFUtQm9vdC1Vc2Vyc0BsaXN0cy5zb3VyY2Vmb3JnZS5uZXQNCj4gaHR0cHM6Ly9saXN0
> cy5zb3VyY2Vmb3JnZS5uZXQvbGlzdHMvbGlzdGluZm8vdS1ib290LXVzZXJzDQo+IA0K
> 

Please do not send base 64 encoded messages.

Please do not send HTML messages.

Please send plain text only.

Message unreadable, ignored. Sorry.

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2006-12-25 23:24             ` Wolfgang Denk
  2006-12-26  6:04               ` Zhang Wei
@ 2007-01-04  8:36               ` Zhang Wei-r63237
  2007-01-04  9:23                 ` Wolfgang Denk
  1 sibling, 1 reply; 55+ messages in thread
From: Zhang Wei-r63237 @ 2007-01-04  8:36 UTC (permalink / raw)
  To: u-boot

Hi, Wolfgang,

Sorry about the wrong encoding mail.

The below is the previous email:

Wolfgang Denk wrote:
> Now this is what I want to understand. What exactly is the "potential
> problem"?
   
That's the issue in the flash 'Spinsion S29GL064M90TFIR6' with x16 
connection. After running flash_read_jedec_ids(), any follow CFI query 
command will get the data with high 8bit = 0xff, but the low 8bit is 
valid. And if we only read low 8bit, we'll get the 0xff too. In 
addition, the second follow CFI query command has no that issue. So, I 
read the full 16bit date and only take the valid low 8bit.

> Can you please point out which specific part of the Linux MTD code you
> mean? And which version of the code?
   
The reference codes is in Linux Kernel 2.6.19.

drivers/mtd/chips/cfi_probe.c: cfi_chip_setup() calls:
    int num_erase_regions = cfi_read_query(map, base + (0x10 + 
28)*ofs_factor);
include/linux/mtd/cfi.h: cfi_read_query() calls:
    map_word val = map_read(map, addr);
include/linux/mtd/map.h defines:
    #define map_read(map, ofs) inline_map_read(map, ofs)
include/linux/mtd/map.h: function inline_map_read() body:
static inline map_word inline_map_read(struct map_info *map, unsigned 
long ofs)
{
    map_word r;

    if (map_bankwidth_is_1(map))
        r.x[0] = __raw_readb(map->virt + ofs);
    else if (map_bankwidth_is_2(map))
        r.x[0] = __raw_readw(map->virt + ofs);
    else if (map_bankwidth_is_4(map))
        r.x[0] = __raw_readl(map->virt + ofs);
#if BITS_PER_LONG >= 64
    else if (map_bankwidth_is_8(map))
        r.x[0] = __raw_readq(map->virt + ofs);
#endif
    else if (map_bankwidth_is_large(map))
        memcpy_fromio(r.x, map->virt+ofs, map->bankwidth);

    return r;
}
And the 'map_word' definition in include/linux/mtd/map.h is below:
typedef union {
    unsigned long x[MAX_MAP_LONGS];
} map_word;

> I have to admit that I don't really understand this. I would not be
> surprised that some statement like this can be found in some chip
> errata, but I would like to know this for certain first.
   
I only find one clue from 'Spinsion S29GL064M90TFIR6' specification, 
which is the comment 'Data bits DQ15-DQ8 are don't care. ' for 'RESET' 
command. And the comment has not found in some other AMD, Spinsion chips

specifications.

> For me this is an indication that the problem is  actually  somewhere
> else,  and while your modification might actually fix the symptoms, I
> doubt that it is the correct fix - or the correct  problem.  If  your
> explanation was right, this should not depend on compiler versions.
   

This is a real weird issue. The compiler is also a factor. Chris's 
different compilers get the different results. In fact, the same gcc 
with different size codes will also get different results.

Thanks!

Best Regards,
Zhang Wei

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-04  8:36               ` Zhang Wei-r63237
@ 2007-01-04  9:23                 ` Wolfgang Denk
  2007-01-04 11:19                   ` Stefan Roese
  2007-01-05 13:27                   ` Stefan Roese
  0 siblings, 2 replies; 55+ messages in thread
From: Wolfgang Denk @ 2007-01-04  9:23 UTC (permalink / raw)
  To: u-boot

In message <46B96294322F7D458F9648B60E15112C03EEE0@zch01exm26.fsl.freescale.net> you wrote:
> 
> The below is the previous email:
> 
> Wolfgang Denk wrote:
> > Now this is what I want to understand. What exactly is the "potential
> > problem"?
>    
> That's the issue in the flash 'Spinsion S29GL064M90TFIR6' with x16 
> connection. After running flash_read_jedec_ids(), any follow CFI query 
> command will get the data with high 8bit = 0xff, but the low 8bit is 
> valid. And if we only read low 8bit, we'll get the 0xff too. In 
> addition, the second follow CFI query command has no that issue. So, I 
> read the full 16bit date and only take the valid low 8bit.

etc. etc.

I didn't see any new facts in your current posting. My  position  has
not  changed  either:  I don't see how your character-wise copy using
memcpy() would be different from accessing the flash through a  uchar
pointer;  also  I  still  think  that if the compiler version changes
behaviour then we don't really understand what's going on here.

Maybe Tolunay or Stefan can comment now that both are back from their
Xmas breaks; they both know the CFI driver much better than me.

I tend to reject your proposed patch until we really understand the
problem. To me, the patch seems to be wrong.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Unix is like a toll road on which you have to stop every 50  feet  to
pay another nickel. But hey! You only feel 5 cents poorer each time.
                 - Larry Wall in <1992Aug13.192357.15731@netlabs.com>

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-04  9:23                 ` Wolfgang Denk
@ 2007-01-04 11:19                   ` Stefan Roese
  2007-01-05 13:27                   ` Stefan Roese
  1 sibling, 0 replies; 55+ messages in thread
From: Stefan Roese @ 2007-01-04 11:19 UTC (permalink / raw)
  To: u-boot

On Thursday 04 January 2007 10:23, Wolfgang Denk wrote:
> Maybe Tolunay or Stefan can comment now that both are back from their
> Xmas breaks; they both know the CFI driver much better than me.

I'll try to look into this issue at the weekend.

Best regards,
Stefan

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-04  9:23                 ` Wolfgang Denk
  2007-01-04 11:19                   ` Stefan Roese
@ 2007-01-05 13:27                   ` Stefan Roese
  2007-01-07 10:12                     ` Tolunay Orkun
  2007-01-08  2:41                     ` [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug Zhang Wei-r63237
  1 sibling, 2 replies; 55+ messages in thread
From: Stefan Roese @ 2007-01-05 13:27 UTC (permalink / raw)
  To: u-boot

On Thursday 04 January 2007 10:23, Wolfgang Denk wrote:
> > Wolfgang Denk wrote:
> > > Now this is what I want to understand. What exactly is the "potential
> > > problem"?
> >
> > That's the issue in the flash 'Spinsion S29GL064M90TFIR6' with x16
> > connection. After running flash_read_jedec_ids(), any follow CFI query
> > command will get the data with high 8bit = 0xff, but the low 8bit is
> > valid. And if we only read low 8bit, we'll get the 0xff too. In
> > addition, the second follow CFI query command has no that issue. So, I
> > read the full 16bit date and only take the valid low 8bit.
>
> etc. etc.
>
> I didn't see any new facts in your current posting. My  position  has
> not  changed  either:  I don't see how your character-wise copy using
> memcpy() would be different from accessing the flash through a  uchar
> pointer;  also  I  still  think  that if the compiler version changes
> behaviour then we don't really understand what's going on here.
>
> Maybe Tolunay or Stefan can comment now that both are back from their
> Xmas breaks; they both know the CFI driver much better than me.

What I noticed after looking at the flash access functions like 
flash_read_uchar() is that no access macros and even no volatile pointer 
access is used to read from the flash. This looks like a potential problem, 
that can show when using different compiler versions.

Please find attached a small patch that adds fixes this potential problem for 
the 3 functions flash_read_uchar/ushort/long. Please give it a try and let me 
know if this changed the behavior somehow.

Thanks.

Best regards,
Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cfi_flash_volatile.patch
Type: text/x-diff
Size: 908 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20070105/bb1f4dcd/attachment.patch 

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-05 13:27                   ` Stefan Roese
@ 2007-01-07 10:12                     ` Tolunay Orkun
  2007-01-07 10:40                       ` Wolfgang Denk
  2007-01-08  2:41                     ` [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug Zhang Wei-r63237
  1 sibling, 1 reply; 55+ messages in thread
From: Tolunay Orkun @ 2007-01-07 10:12 UTC (permalink / raw)
  To: u-boot

Stefan Roese wrote:
> On Thursday 04 January 2007 10:23, Wolfgang Denk wrote:
>>> Wolfgang Denk wrote:
>>>> Now this is what I want to understand. What exactly is the "potential
>>>> problem"?
>>> That's the issue in the flash 'Spinsion S29GL064M90TFIR6' with x16
>>> connection. After running flash_read_jedec_ids(), any follow CFI query
>>> command will get the data with high 8bit = 0xff, but the low 8bit is
>>> valid. And if we only read low 8bit, we'll get the 0xff too. In
>>> addition, the second follow CFI query command has no that issue. So, I
>>> read the full 16bit date and only take the valid low 8bit.
>> etc. etc.
>>
>> I didn't see any new facts in your current posting. My  position  has
>> not  changed  either:  I don't see how your character-wise copy using
>> memcpy() would be different from accessing the flash through a  uchar
>> pointer;  also  I  still  think  that if the compiler version changes
>> behaviour then we don't really understand what's going on here.
>>
>> Maybe Tolunay or Stefan can comment now that both are back from their
>> Xmas breaks; they both know the CFI driver much better than me.
> 
> What I noticed after looking at the flash access functions like 
> flash_read_uchar() is that no access macros and even no volatile pointer 
> access is used to read from the flash. This looks like a potential problem, 
> that can show when using different compiler versions.
> 
> Please find attached a small patch that adds fixes this potential problem for 
> the 3 functions flash_read_uchar/ushort/long. Please give it a try and let me 
> know if this changed the behavior somehow.

I think this is a good idea given GCC 4.x is agressive in optimizations. 
I do not think converting these to use memcpy() is a good idea. I am 
with Wolfgang on this.

I think we should commit Stefan's patch even if it might or might not 
solve the problem Zhang is experiencing.

Best regards,
Tolunay

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-07 10:12                     ` Tolunay Orkun
@ 2007-01-07 10:40                       ` Wolfgang Denk
  2007-01-13  7:11                         ` Tolunay Orkun
  0 siblings, 1 reply; 55+ messages in thread
From: Wolfgang Denk @ 2007-01-07 10:40 UTC (permalink / raw)
  To: u-boot

Dear Tolunay,

in message <45A0C777.1000508@orkun.us> you wrote:
>
> > Please find attached a small patch that adds fixes this potential problem for 
> > the 3 functions flash_read_uchar/ushort/long. Please give it a try and let me 
> > know if this changed the behavior somehow.
> 
> I think this is a good idea given GCC 4.x is agressive in optimizations. 

I already discussed this internally with Stefan. I *don't* think it's
a good idea. I really hate to change bits and pieces of code  without
really understanding why we are doing this.

In the current situation we  are  accessing  flash  memory  which  is
supposed  to  be  in  read  mode, i. e. it behaves like normal system
memory. It should be no problem even if the flash memory content  was
cached. So why would "volatile" improve anything?

As long as I don't see (for example in the generated assembler  code)
how  a problem might exist, and how the suggested patch fixes exactly
this problem, I'd like to continue researching this problem.

> I do not think converting these to use memcpy() is a good idea. I am 
> with Wolfgang on this.

Actually I might have been wrong in my assessment here, when I stated
that memcpy() performs a character-wise copy, too.  The  simple  code
from  lib_generic/string.c  is  used  only  if  __HAVE_ARCH_MEMCPY is
undefined,   and   especially   on   PPC   we   define    this    (in
include/asm-ppc/string.h).  So  we  might  use  an optimized versions
where it *does* make a difference.

Checking this idea I see that we are actually using the memcpy() code
from lib_ppc/ppcstring.S which *does* implement some optimiation, but
I then I don't think this is efffecting us here.

> I think we should commit Stefan's patch even if it might or might not 
> solve the problem Zhang is experiencing.

Please explain why you think adding volatile's here would be  a  good
thing, andin which way it improves the code.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

"What happened to the crewman?"     "The M-5 computer  needed  a  new
power source, the crewman merely got in the way."
	-- Kirk and Dr. Richard Daystrom, "The Ultimate Computer",
	   stardate 4731.3.

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-05 13:27                   ` Stefan Roese
  2007-01-07 10:12                     ` Tolunay Orkun
@ 2007-01-08  2:41                     ` Zhang Wei-r63237
  1 sibling, 0 replies; 55+ messages in thread
From: Zhang Wei-r63237 @ 2007-01-08  2:41 UTC (permalink / raw)
  To: u-boot

Hi, Stefan,

Thanks! 
But it's a pity they are no useful.

> 
> What I noticed after looking at the flash access functions like 
> flash_read_uchar() is that no access macros and even no 
> volatile pointer 
> access is used to read from the flash. This looks like a 
> potential problem, 
> that can show when using different compiler versions.
> 
> Please find attached a small patch that adds fixes this 
> potential problem for 
> the 3 functions flash_read_uchar/ushort/long. Please give it 
> a try and let me 
> know if this changed the behavior somehow.

Agree with you!
It's a potential problem. Is my patch fit for it? :D

Best Regards,
Zhang Wei

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-07 10:40                       ` Wolfgang Denk
@ 2007-01-13  7:11                         ` Tolunay Orkun
  2007-01-13 17:53                           ` Håvard Skinnemoen
  2007-01-25  4:38                           ` Zhang Wei-r63237
  0 siblings, 2 replies; 55+ messages in thread
From: Tolunay Orkun @ 2007-01-13  7:11 UTC (permalink / raw)
  To: u-boot

Wolfgang Denk wrote:
> Dear Tolunay,
> 
> in message <45A0C777.1000508@orkun.us> you wrote:
>>> Please find attached a small patch that adds fixes this potential problem for 
>>> the 3 functions flash_read_uchar/ushort/long. Please give it a try and let me 
>>> know if this changed the behavior somehow.
>> I think this is a good idea given GCC 4.x is agressive in optimizations. 
> 
> I already discussed this internally with Stefan. I *don't* think it's
> a good idea. I really hate to change bits and pieces of code  without
> really understanding why we are doing this.
> 
> In the current situation we  are  accessing  flash  memory  which  is
> supposed  to  be  in  read  mode, i. e. it behaves like normal system
> memory. It should be no problem even if the flash memory content  was
> cached. So why would "volatile" improve anything?

These functions are used to read data from flash tables. I agree most 
tables do not change but I think (need to verify) we also use these 
functions to read the status registers to determine if programming is OK 
etc. We might have been OK since we probably access these registers once 
in a function context. When the function returns the compiler 
optimization context is gone so no state data is used. So, it might not 
be necessary.

Perhaps, more important issue would be if these areas were cached. In 
that case, every time we change the flash from read mode to special 
query modes, we should probably invalidate the cache. In PowerPC 405 
platform our flash is uncached so this is not necessary. I do not know 
if any platform caches its flash areas.

> As long as I don't see (for example in the generated assembler  code)
> how  a problem might exist, and how the suggested patch fixes exactly
> this problem, I'd like to continue researching this problem.
> 
>> I do not think converting these to use memcpy() is a good idea. I am 
>> with Wolfgang on this.
> 
> Actually I might have been wrong in my assessment here, when I stated
> that memcpy() performs a character-wise copy, too.  The  simple  code
> from  lib_generic/string.c  is  used  only  if  __HAVE_ARCH_MEMCPY is
> undefined,   and   especially   on   PPC   we   define    this    (in
> include/asm-ppc/string.h).  So  we  might  use  an optimized versions
> where it *does* make a difference.

Memcopy might be OK for flash tables but I am not sure if we use the 
same function to access status registers. I would rather access flash 
using bus wide accesses instead of byte by byte. Maybe it is safe but I 
do not know how it behaves in various platforms and bus interface units 
of various processors. I would take the safe route.

Best regards,
Tolunay

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-13  7:11                         ` Tolunay Orkun
@ 2007-01-13 17:53                           ` Håvard Skinnemoen
  2007-01-16  6:52                             ` Stefan Roese
  2007-01-25  4:38                           ` Zhang Wei-r63237
  1 sibling, 1 reply; 55+ messages in thread
From: Håvard Skinnemoen @ 2007-01-13 17:53 UTC (permalink / raw)
  To: u-boot

On 1/13/07, Tolunay Orkun <listmember@orkun.us> wrote:
> Perhaps, more important issue would be if these areas were cached. In
> that case, every time we change the flash from read mode to special
> query modes, we should probably invalidate the cache. In PowerPC 405
> platform our flash is uncached so this is not necessary. I do not know
> if any platform caches its flash areas.

AVR32 does cache its flash area by default, but it can be worked
around by specifying an uncached virtual mapping (i.e. the physical
address ORed with 0xa0000000, similar to MIPS and SH) as the flash
address instead of the physical address.

However, doing cached flash accesses would probably improve the
performance of things like fsload, so how about using something like
the following interface when uncached access is needed?

void *flash_map(unsigned long paddr, size_t len);
void flash_unmap(void *vaddr, size_t len);

On platforms where the flash is always uncached, flash_map would
simply return paddr cast as a void *, and flash_unmap would do
nothing. Other platforms may invalidate the range and return a virtual
address which will bypass the cache (or simply disable the dcache and
return the physical address.)

> > Actually I might have been wrong in my assessment here, when I stated
> > that memcpy() performs a character-wise copy, too.  The  simple  code
> > from  lib_generic/string.c  is  used  only  if  __HAVE_ARCH_MEMCPY is
> > undefined,   and   especially   on   PPC   we   define    this    (in
> > include/asm-ppc/string.h).  So  we  might  use  an optimized versions
> > where it *does* make a difference.
>
> Memcopy might be OK for flash tables but I am not sure if we use the
> same function to access status registers. I would rather access flash
> using bus wide accesses instead of byte by byte. Maybe it is safe but I
> do not know how it behaves in various platforms and bus interface units
> of various processors. I would take the safe route.

I agree. Please don't make any assumptions about how memcpy() accesses
memory -- it might do all kinds of crazy stuff in the name of
optimization, including prefetching. And it may introduce _really_
subtle bugs where adding a field to a struct may cause things to break
because it causes its size to cross some implementation threshold (for
example on avr32 we might switch to 32-byte load/store-multiple
instructions if the size is >= 32 bytes and the start address is
properly aligned. Also, I believe gcc uses some threshold to determine
whether to emit __builtin_memcpy in-line or emit a call to the
out-of-line memcpy implementation, which may cause entirely different
things to happen from the flash chip's point of view.)

Best regards,
Haavard

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-13 17:53                           ` Håvard Skinnemoen
@ 2007-01-16  6:52                             ` Stefan Roese
  0 siblings, 0 replies; 55+ messages in thread
From: Stefan Roese @ 2007-01-16  6:52 UTC (permalink / raw)
  To: u-boot

Hi Haavard,

On Saturday 13 January 2007 18:53, H?vard Skinnemoen wrote:
> On 1/13/07, Tolunay Orkun <listmember@orkun.us> wrote:
> > Perhaps, more important issue would be if these areas were cached. In
> > that case, every time we change the flash from read mode to special
> > query modes, we should probably invalidate the cache. In PowerPC 405
> > platform our flash is uncached so this is not necessary. I do not know
> > if any platform caches its flash areas.
>
> AVR32 does cache its flash area by default, but it can be worked
> around by specifying an uncached virtual mapping (i.e. the physical
> address ORed with 0xa0000000, similar to MIPS and SH) as the flash
> address instead of the physical address.
>
> However, doing cached flash accesses would probably improve the
> performance of things like fsload, so how about using something like
> the following interface when uncached access is needed?
>
> void *flash_map(unsigned long paddr, size_t len);
> void flash_unmap(void *vaddr, size_t len);
>
> On platforms where the flash is always uncached, flash_map would
> simply return paddr cast as a void *, and flash_unmap would do
> nothing. Other platforms may invalidate the range and return a virtual
> address which will bypass the cache (or simply disable the dcache and
> return the physical address.)

I tend to like this idea of these mapping functions. This would give us more 
flexibility. On some 4xx boards for example, we initialize the flash cached 
after powerup and switch to uncached access after relocation. We wouldn't 
need this change in the TLB's with these mapping functions anymore.

And you are right of course. If implementing such an extension, we should 
first make sure that those mapping functions will do nothing on most 
platform, so we stay compatible and don't break any existing designs.

If thinking longer about this, I think it would be helpful to make these 
mapping functions even more generic. Meaning not flash focused, but generic 
mapping functions with a parameter like ioremap in Linux. This way we could 
use these function to also map a region as cached or uncached memory if 
needed.

Best regards,
Stefan

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-13  7:11                         ` Tolunay Orkun
  2007-01-13 17:53                           ` Håvard Skinnemoen
@ 2007-01-25  4:38                           ` Zhang Wei-r63237
  2007-01-25 16:10                             ` Timur Tabi
  1 sibling, 1 reply; 55+ messages in thread
From: Zhang Wei-r63237 @ 2007-01-25  4:38 UTC (permalink / raw)
  To: u-boot

Hi, Wolfgang,

It's so pity that the flash got wrong 'num_erase_regions' issue is still
in u-boot 1.2.0.

The byte by byte accessing is not preferred. But the flash_read_ushort()
and flash_read_long() in drivers/cfi_flash.c are both implemented by
byte accessing. How about it?

Can the instruction '
	retval = (addr[0] << 16) | (addr[(info->portwidth)] << 24) |
		(addr[(2 * info->portwidth)]) | (addr[(3 *
info->portwidth)] << 8);'
In flash_read_long() be replaced by memcpy() function?

Thanks!
Zhang Wei

> 
> Memcopy might be OK for flash tables but I am not sure if we use the 
> same function to access status registers. I would rather access flash 
> using bus wide accesses instead of byte by byte. Maybe it is 
> safe but I 
> do not know how it behaves in various platforms and bus 
> interface units 
> of various processors. I would take the safe route.
> 
> Best regards,
> Tolunay
> 
> 

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-25  4:38                           ` Zhang Wei-r63237
@ 2007-01-25 16:10                             ` Timur Tabi
  2007-01-25 20:33                               ` Wolfgang Denk
  0 siblings, 1 reply; 55+ messages in thread
From: Timur Tabi @ 2007-01-25 16:10 UTC (permalink / raw)
  To: u-boot

Zhang Wei-r63237 wrote:
> Hi, Wolfgang,
> 
> It's so pity that the flash got wrong 'num_erase_regions' issue is still
> in u-boot 1.2.0.
> 
> The byte by byte accessing is not preferred. But the flash_read_ushort()
> and flash_read_long() in drivers/cfi_flash.c are both implemented by
> byte accessing. How about it?
> 
> Can the instruction '
> 	retval = (addr[0] << 16) | (addr[(info->portwidth)] << 24) |
> 		(addr[(2 * info->portwidth)]) | (addr[(3 *
> info->portwidth)] << 8);'
> In flash_read_long() be replaced by memcpy() function?

I really don't think we should be using mempcy().  You're not supposed 
to care about the internal implementation of mempcy(), so if we start 
using it for specific-sized reads, then we start caring about how it's 
implemented.

If you want to read 32 bits at one time, then just do a 32-bit read!

	retval = * (ulong *) addr;

Looking at the code, I don't understand why it's so complicated.  If 
portwidth is 2, then retval above will be a conglomeration of addr[0], 
addr[2], addr[4], and addr[6].  Why isn't it just reading from 
[0][1][2][3]??

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-25 16:10                             ` Timur Tabi
@ 2007-01-25 20:33                               ` Wolfgang Denk
  2007-01-26  2:13                                 ` Zhang Wei-r63237
  0 siblings, 1 reply; 55+ messages in thread
From: Wolfgang Denk @ 2007-01-25 20:33 UTC (permalink / raw)
  To: u-boot

In message <45B8D66A.6080209@freescale.com> you wrote:
>
> > Can the instruction '
> > 	retval = (addr[0] << 16) | (addr[(info->portwidth)] << 24) |
> > 		(addr[(2 * info->portwidth)]) | (addr[(3 *
> > info->portwidth)] << 8);'
> > In flash_read_long() be replaced by memcpy() function?

INHO: no, it cannot.

> If you want to read 32 bits at one time, then just do a 32-bit read!
> 
> 	retval = * (ulong *) addr;

You assume that we want to read four contiguous bytes, which may,  or
may not, be the case.

> Looking at the code, I don't understand why it's so complicated.  If 
> portwidth is 2, then retval above will be a conglomeration of addr[0], 
> addr[2], addr[4], and addr[6].  Why isn't it just reading from 
> [0][1][2][3]??

Because the data we are interested in  is  only  available  in  every
other byte?

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The Gates in my computer are AND, OR and NOT; they are not Bill.

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-25 20:33                               ` Wolfgang Denk
@ 2007-01-26  2:13                                 ` Zhang Wei-r63237
  2007-01-29 11:29                                   ` Tolunay Orkun
  0 siblings, 1 reply; 55+ messages in thread
From: Zhang Wei-r63237 @ 2007-01-26  2:13 UTC (permalink / raw)
  To: u-boot

> Because the data we are interested in  is  only  available  in  every
> other byte?
> 
Does it means if we need other byte data we can access them byte by byte? If we needn't we should not access them?

Thanks!
Zhang Wei

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-26  2:13                                 ` Zhang Wei-r63237
@ 2007-01-29 11:29                                   ` Tolunay Orkun
  2007-01-30  3:36                                     ` Wang Haiying-r54964
  0 siblings, 1 reply; 55+ messages in thread
From: Tolunay Orkun @ 2007-01-29 11:29 UTC (permalink / raw)
  To: u-boot

Zhang Wei-r63237 wrote:
>> Because the data we are interested in  is  only  available  in  every
>> other byte?
>>
> Does it means if we need other byte data we can access them byte by byte? If we needn't we should not access them?
> 
> Thanks!
> Zhang Wei

The current code works because most CPU Bus Interface Unit will execute 
buswide cycles and discard the unused byte(s).

I understand that you would like to convert this code to use memcpy() 
and pick various bytes from local ram instead of directly from flash. 
Right? What exactly you are experiencing when you use current code. 
Could you refresh my memory?

I do not like memcpy() as it is a black box. It could be implemented at 
low level using string instructions, it could do various things for 
different cpu architectures and various sizes so it could lead to very 
hard to find issues.

My proposal for you to try: Do not depend on bus interface unit doing 
buswide reads, explicitly read buswide chunks after doing the bus-wide 
read into a temporary variable, pick the usable bytes from that 
temporary variable. This should eliminate the bus interface unit 
dependency of the CPU and possible board designer interfacing gimmicks.

Let us know how it goes (can you post your patch too so I can verify it).

Tolunay

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-29 11:29                                   ` Tolunay Orkun
@ 2007-01-30  3:36                                     ` Wang Haiying-r54964
  2007-01-31  9:01                                       ` Tolunay Orkun
  0 siblings, 1 reply; 55+ messages in thread
From: Wang Haiying-r54964 @ 2007-01-30  3:36 UTC (permalink / raw)
  To: u-boot

 
>The current code works because most CPU Bus Interface Unit 
>will execute 
>buswide cycles and discard the unused byte(s).
>
>I understand that you would like to convert this code to use memcpy() 
>and pick various bytes from local ram instead of directly from flash. 
>Right? What exactly you are experiencing when you use current code. 
>Could you refresh my memory?

The problem we experienced here is that the flash(am29lv641d) could not
get the correct size during boot up(the error message like "FLASH:
....255 erase regions found, only 4 used...0kB" is printed out), could
not be read correctly by flinfo, could not erased or written etc.
It is because the num_erase_regions got a wrong value by
flash_read_uchar(info, FLASH_OFFSET_NUM_ERASE_REGIONS)in line 1204.
It happened only after the latest update of cfi_flash driver after
u-boot 1.1.6 released. This update added a flash_read_jedec_ids before
reading num_erase_resions. We suspect that reset command in
flash_read_jedec_ids()may cause the problem. After the last reset in
this function, a flash_write_cmd() follows it, then the flash_read_uchar
is executed directly after. This flash_read_uchar returns a wrong
number(124, 255 for num_erase_regions etc.). If this flash_read_uchar
here reads some other flash info other than
FLASH_OFFSET_NUM_ERASE_REGIONS by uchar, it also returns wrong value. 

>I do not like memcpy() as it is a black box. It could be 
>implemented at 
>low level using string instructions, it could do various things for 
>different cpu architectures and various sizes so it could lead to very 
>hard to find issues.
I agree.

>My proposal for you to try: Do not depend on bus interface unit doing 
>buswide reads, explicitly read buswide chunks after doing the bus-wide 
>read into a temporary variable, pick the usable bytes from that 
>temporary variable. This should eliminate the bus interface unit 
>dependency of the CPU and possible board designer interfacing gimmicks.
Thanks a lot. :)

>Let us know how it goes (can you post your patch too so I can 
>verify it).
Zhang Wei is on vacation this week, he had generated a new patch to
implement it with the similar idea last week. He will post his new patch
after he is back.

Thanks.

Haiying

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-30  3:36                                     ` Wang Haiying-r54964
@ 2007-01-31  9:01                                       ` Tolunay Orkun
  2007-01-31 19:25                                         ` Haiying Wang
  0 siblings, 1 reply; 55+ messages in thread
From: Tolunay Orkun @ 2007-01-31  9:01 UTC (permalink / raw)
  To: u-boot

Wang Haiying-r54964 wrote:
>  
>> The current code works because most CPU Bus Interface Unit 
>> will execute 
>> buswide cycles and discard the unused byte(s).
>>
>> I understand that you would like to convert this code to use memcpy() 
>> and pick various bytes from local ram instead of directly from flash. 
>> Right? What exactly you are experiencing when you use current code. 
>> Could you refresh my memory?
> 
> The problem we experienced here is that the flash(am29lv641d) could not
> get the correct size during boot up(the error message like "FLASH:
> ....255 erase regions found, only 4 used...0kB" is printed out), could
> not be read correctly by flinfo, could not erased or written etc.
> It is because the num_erase_regions got a wrong value by
> flash_read_uchar(info, FLASH_OFFSET_NUM_ERASE_REGIONS)in line 1204.
> It happened only after the latest update of cfi_flash driver after
> u-boot 1.1.6 released. This update added a flash_read_jedec_ids before
> reading num_erase_resions. We suspect that reset command in
> flash_read_jedec_ids()may cause the problem. After the last reset in

Could you add some printfs and print the following variables right after 
the flash_detect_cfi() call in flash_get_size() function.

printf("chipwidth = %d\n", info->chipwidth);
printf("portwidth = %d\n", info->portwidth);
printf("interface = %d\n", info->interface);
printf("vendor    = %d\n", info->vendor);
printf("cfi_offset= %d\n", info->cfi_offset);

And the following printf right after flash_read_jedec_ids() in 
flash_get_size();

printf("manufacturer_id = %d\n", info->manufacturer_id);
printf("device_id       = %d\n", info->device_id);
printf("device_id2      = %d\n", info->device_id2);

Please collect the output and post to the list and cc to me.

Also try replacing following line in flash_detect_cfi() function:

flash_write_cmd (info, 0, 0, info->cmd_reset);

with the following two lines:

flash_write_cmd(info, 0, 0, FLASH_CMD_RESET);
flash_write_cmd(info, 0, 0, AMD_CMD_RESET);

I would like to see if this change makes any difference as well.

 From your description, I have the feeling that you did not have 
debugged the problem fully.

If you are suspecting that the flash is not in array read mode after 
flash_read_jedec_ids() you can easily dump the first 256 bytes of flash 
right after [write a simple hexdump routine].

> this function, a flash_write_cmd() follows it, then the flash_read_uchar

After this command, the flash should be in cfi_read mode. Hexdump the 
first 256 bytes again. You should be able to spot easily if you landed 
in cfi query mode or not. These are crucial before you put the blame on 
flash_read_uchar().

> is executed directly after. This flash_read_uchar returns a wrong
> number(124, 255 for num_erase_regions etc.). If this flash_read_uchar
> here reads some other flash info other than
> FLASH_OFFSET_NUM_ERASE_REGIONS by uchar, it also returns wrong value. 

Please do the checks I've provided above and collect the logs and send 
it to me. If flash does not switch to cfi query mode properly the read 
will fetch whatever content that was available in read mode. If the 
flash was completely blank (all 1s) you will get 255 which is matching 
with the symptoms you are facing.

>> My proposal for you to try: Do not depend on bus interface unit doing 
>> buswide reads, explicitly read buswide chunks after doing the bus-wide 
>> read into a temporary variable, pick the usable bytes from that 
>> temporary variable. This should eliminate the bus interface unit 
>> dependency of the CPU and possible board designer interfacing gimmicks.
> Thanks a lot. :)

Please do the checks before you even bother with such a patch. The 
problem could be elsewhere.

> 
>> Let us know how it goes (can you post your patch too so I can 
>> verify it).
> Zhang Wei is on vacation this week, he had generated a new patch to
> implement it with the similar idea last week. He will post his new patch
> after he is back.
> 
> Thanks.
> 
> Haiying

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-31  9:01                                       ` Tolunay Orkun
@ 2007-01-31 19:25                                         ` Haiying Wang
  2007-02-01  5:26                                           ` Tolunay Orkun
  0 siblings, 1 reply; 55+ messages in thread
From: Haiying Wang @ 2007-01-31 19:25 UTC (permalink / raw)
  To: u-boot

> Could you add some printfs and print the following variables right after 
> the flash_detect_cfi() call in flash_get_size() function.
> 
> printf("chipwidth = %d\n", info->chipwidth);
> printf("portwidth = %d\n", info->portwidth);
> printf("interface = %d\n", info->interface);
> printf("vendor    = %d\n", info->vendor);
> printf("cfi_offset= %d\n", info->cfi_offset);
> 
> And the following printf right after flash_read_jedec_ids() in 
> flash_get_size();
> 
> printf("manufacturer_id = %d\n", info->manufacturer_id);
> printf("device_id       = %d\n", info->device_id);
> printf("device_id2      = %d\n", info->device_id2);
> 
> Please collect the output and post to the list and cc to me.
Here comes the first log after adding the printf above:
------------
U-Boot 1.2.0 (Jan 31 2007 - 13:12:04)

Freescale PowerPC
CPU:
    Core: E600, Version: 0.2, (0x80040202)
    System: 8641D, Version: 2.0, (0x80900120)
    Clocks: CPU:1000 MHz, MPX: 400 MHz, DDR: 200 MHz, LBC:  50 MHz
    L2: Enabled
Board: MPC8641HPCN
PCI-EXPRESS1: Disabled
I2C:   ready
DRAM:      DDR: 256 MB
FLASH: ## Unknown FLASH on Bank 1 - Size = 0x00000000 = 0 MB
chipwidth = 2
portwidth = 2
interface = 1
vendor    = 0
cfi_offset= 85
manufacturer_id = 1
device_id       = 126
device_id2      = 4865
124 erase regions found, only 4 used
 0 kB
Using default environment

In:    serial
Out:   serial
Err:   serial
Net:   eTSEC1, eTSEC2, eTSEC3, eTSEC4
Hit any key to stop autoboot:  0
--------------
> Also try replacing following line in flash_detect_cfi() function:
> 
> flash_write_cmd (info, 0, 0, info->cmd_reset);
> 
> with the following two lines:
> 
> flash_write_cmd(info, 0, 0, FLASH_CMD_RESET);
> flash_write_cmd(info, 0, 0, AMD_CMD_RESET);
> 
> I would like to see if this change makes any difference as well.
Then the second log file after replacing the original flash_write_cmd()
in flash_detect_cfi:
--------------
U-Boot 1.2.0 (Jan 31 2007 - 13:43:50)

Freescale PowerPC
CPU:
    Core: E600, Version: 0.2, (0x80040202)
    System: 8641D, Version: 2.0, (0x80900120)
    Clocks: CPU:1000 MHz, MPX: 400 MHz, DDR: 200 MHz, LBC:  50 MHz
    L2: Enabled
Board: MPC8641HPCN
PCI-EXPRESS1: Disabled
I2C:   ready
DRAM:      DDR: 256 MB
FLASH: ## Unknown FLASH on Bank 1 - Size = 0x00000000 = 0 MB
chipwidth = 2
portwidth = 2
interface = 1
vendor    = 0
cfi_offset= 85
manufacturer_id = 1
device_id       = 126
device_id2      = 4865
124 erase regions found, only 4 used
 0 kB
Using default environment

In:    serial
Out:   serial
Err:   serial
Net:   eTSEC1, eTSEC2, eTSEC3, eTSEC4
Hit any key to stop autoboot:  0
=>
--------------
Seems no difference than the first log.

> If you are suspecting that the flash is not in array read mode after 
> flash_read_jedec_ids() you can easily dump the first 256 bytes of flash 
> right after [write a simple hexdump routine].
> 
> > this function, a flash_write_cmd() follows it, then the flash_read_uchar
> 
> After this command, the flash should be in cfi_read mode. Hexdump the 
> first 256 bytes again. You should be able to spot easily if you landed 
> in cfi query mode or not. These are crucial before you put the blame on 
> flash_read_uchar().
Then I dump the first 256bytes of flash, first after
flash_read_jedec_ids(), then after flash_write_cmd(), see log:

-------
U-Boot 1.2.0 (Jan 31 2007 - 13:46:59)

Freescale PowerPC
CPU:
    Core: E600, Version: 0.2, (0x80040202)
    System: 8641D, Version: 2.0, (0x80900120)
    Clocks: CPU:1000 MHz, MPX: 400 MHz, DDR: 200 MHz, LBC:  50 MHz
    L2: Enabled
Board: MPC8641HPCN
PCI-EXPRESS1: Disabled
I2C:   ready
DRAM:      DDR: 256 MB
FLASH: ## Unknown FLASH on Bank 1 - Size = 0x00000000 = 0 MB
chipwidth = 2
portwidth = 2
interface = 1
vendor    = 0
cfi_offset= 85

Dump the first 256 byte of flash
27 05 19 56 26 6c 3d b3 44 e4 ae 15 00 2b 05 33
00 00 00 00 00 00 00 00 35 b6 3f 82 05 07 03 01
75 62 6f 6f 74 20 65 78 74 32 20 72 61 6d 64 69
73 6b 20 72 6f 6f 74 66 73 00 00 00 00 00 00 00
1f 8b 08 08 14 ae e4 44 00 03 72 6f 6f 74 66 73
2e 65 78 74 32 00 ec 9d 09 7c 15 d5 bd c7 cf dc
dc dc 24 17 ac 97 a5 42 2b 68 dc 65 11 90 35 09
01 c2 22 6a 45 45 dc 00 17 92 90 00 d1 6c 26 81
62 ab 08 56 ad 5a 17 5a ad 5b 7d 16 7d f6 55 5b
b5 58 ab e0 52 1b 97 56 6c 51 41 01 51 2c 82 62
b5 28 02 ae d4 2e be ef ff ce 19 e6 04 6e ca e4
35 e3 f4 d5 f3 e7 f3 e5 cc ff ce b9 f3 9b 73 ce
ff 2c 73 73 ef 8c 52 d6 ac 59 fb b2 5a 2a 4b a9
1e 87 29 f5 91 a3 d4 7d b9 4a fd 95 d7 1c 63 ff
c2 2e 2e 97 9b 2f aa ae ea e4 ad 8e b2 66 cd da
ff 6f a3 fb ab 38 64 a7 bd 79 6a df 5d f6 5f 47
manufacturer_id = 1
device_id       = 126
device_id2      = 4865

Dump the first 256 byte of flash
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 51 00 52 00 59 00 02 00 00 00 40 00 00 00 00
00 00 00 00 00 00 00 27 00 36 00 00 00 00 00 07
00 07 00 0a 00 00 00 01 00 05 00 04 00 00 00 17
00 01 00 00 00 05 00 00 00 01 00 7f 00 00 00 00
00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 50 00 52 00 49 00 31 00 33 00 08 00 02 00 04
00 01 00 04 00 00 00 00 00 01 00 b5 00 c5 00 05
00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01
 8 MB
Using default environment

In:    serial
Out:   serial
Err:   serial
Net:   eTSEC1, eTSEC2, eTSEC3, eTSEC4
Hit any key to stop autoboot:  0
=>
-------
This time, the flash got the correct size and can be accessed. I think
the second dump()function I added made it work, as it worked when I
inserted a udelay after flash_write_cmd(). 

> > is executed directly after. This flash_read_uchar returns a wrong
> > number(124, 255 for num_erase_regions etc.). If this flash_read_uchar
> > here reads some other flash info other than
> > FLASH_OFFSET_NUM_ERASE_REGIONS by uchar, it also returns wrong value. 
> 
> Please do the checks I've provided above and collect the logs and send 
> it to me. If flash does not switch to cfi query mode properly the read 
> will fetch whatever content that was available in read mode. If the 
> flash was completely blank (all 1s) you will get 255 which is matching 
> with the symptoms you are facing.
It looks that flash does switch to cfi query mode. But from the first
two logs I still got 124(0xfc) for num_erase_regions (sometimes I got
0xff)

Please shed some lights. Thanks a lot!

Haiying

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-01-31 19:25                                         ` Haiying Wang
@ 2007-02-01  5:26                                           ` Tolunay Orkun
  2007-02-01 22:06                                             ` Haiying Wang
                                                               ` (3 more replies)
  0 siblings, 4 replies; 55+ messages in thread
From: Tolunay Orkun @ 2007-02-01  5:26 UTC (permalink / raw)
  To: u-boot

Haiying Wang wrote:
> ------------
> U-Boot 1.2.0 (Jan 31 2007 - 13:12:04)
> 
> Freescale PowerPC
> CPU:
>     Core: E600, Version: 0.2, (0x80040202)
>     System: 8641D, Version: 2.0, (0x80900120)
>     Clocks: CPU:1000 MHz, MPX: 400 MHz, DDR: 200 MHz, LBC:  50 MHz
>     L2: Enabled

Pretty fast CPU you've got here :)

> Board: MPC8641HPCN
> PCI-EXPRESS1: Disabled
> I2C:   ready
> DRAM:      DDR: 256 MB
> FLASH: ## Unknown FLASH on Bank 1 - Size = 0x00000000 = 0 MB

It looks like you inserted the debug prints in wrong place. The output 
below should have been after the debug stuff. Anyway...

> chipwidth = 2
> portwidth = 2
> interface = 1
> vendor    = 0
> cfi_offset= 85
> manufacturer_id = 1
> device_id       = 126
> device_id2      = 4865

Except for "vendor" everything looks OK here. I should have asked you to 
print vendor right before flash_read_jedec_ids() call. It was not yet 
initialized when printed. It must be correctly set as well since 
flash_read_jedec_ids() uses it would not have returned correct data 
otherwise.

You have a 16-bit flash accessible on 16-bit bus. The flash manufacturer 
is "AMD/Spansion" or compatible and device id is 7E1301 which is correct 
for your part:

http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/25538a1.pdf

Please note that flash_read_jedec_ids() is used for manufacturer id and 
device_ids and this function in turn is using flash_read_uchar() which 
worked just fine for you.

> 124 erase regions found, only 4 used

This is not correct. This is why you are blaming the flash_read_uchar() 
function. But since it worked correctly before the failure to read the 
number of erase regions is probably not due to the function.

>> Also try replacing following line in flash_detect_cfi() function:
>>
>> flash_write_cmd (info, 0, 0, info->cmd_reset);
>>
>> with the following two lines:
>>
>> flash_write_cmd(info, 0, 0, FLASH_CMD_RESET);
>> flash_write_cmd(info, 0, 0, AMD_CMD_RESET);

You demonstrated that this did not affect you because the output was the 
same. The problem with this line was that info->cmd_reset was not yet 
initialized when flash_detect_cfi() was called. This is a minor bug 
since the flash was not in query mode yet. I think we can safely remove 
this call as well or replace it both amd anf intel reset commands back 
to back. But anyway, back to your issue.

>> If you are suspecting that the flash is not in array read mode after 
>> flash_read_jedec_ids() you can easily dump the first 256 bytes of flash 
>> right after [write a simple hexdump routine].
>>
>>> this function, a flash_write_cmd() follows it, then the flash_read_uchar
>> After this command, the flash should be in cfi_read mode. Hexdump the 
>> first 256 bytes again. You should be able to spot easily if you landed 
>> in cfi query mode or not. These are crucial before you put the blame on 
>> flash_read_uchar().
> Then I dump the first 256bytes of flash, first after
> flash_read_jedec_ids(), then after flash_write_cmd(), see log:
> 
> -------
> U-Boot 1.2.0 (Jan 31 2007 - 13:46:59)
> 
> Freescale PowerPC
> CPU:
>     Core: E600, Version: 0.2, (0x80040202)
>     System: 8641D, Version: 2.0, (0x80900120)
>     Clocks: CPU:1000 MHz, MPX: 400 MHz, DDR: 200 MHz, LBC:  50 MHz
>     L2: Enabled
> Board: MPC8641HPCN
> PCI-EXPRESS1: Disabled
> I2C:   ready
> DRAM:      DDR: 256 MB
> FLASH: ## Unknown FLASH on Bank 1 - Size = 0x00000000 = 0 MB
> chipwidth = 2
> portwidth = 2
> interface = 1
> vendor    = 0
> cfi_offset= 85
> 
> Dump the first 256 byte of flash
> 27 05 19 56 26 6c 3d b3 44 e4 ae 15 00 2b 05 33
> 00 00 00 00 00 00 00 00 35 b6 3f 82 05 07 03 01
> 75 62 6f 6f 74 20 65 78 74 32 20 72 61 6d 64 69
> 73 6b 20 72 6f 6f 74 66 73 00 00 00 00 00 00 00
> 1f 8b 08 08 14 ae e4 44 00 03 72 6f 6f 74 66 73
> 2e 65 78 74 32 00 ec 9d 09 7c 15 d5 bd c7 cf dc
> dc dc 24 17 ac 97 a5 42 2b 68 dc 65 11 90 35 09
> 01 c2 22 6a 45 45 dc 00 17 92 90 00 d1 6c 26 81
> 62 ab 08 56 ad 5a 17 5a ad 5b 7d 16 7d f6 55 5b
> b5 58 ab e0 52 1b 97 56 6c 51 41 01 51 2c 82 62
> b5 28 02 ae d4 2e be ef ff ce 19 e6 04 6e ca e4
> 35 e3 f4 d5 f3 e7 f3 e5 cc ff ce b9 f3 9b 73 ce
> ff 2c 73 73 ef 8c 52 d6 ac 59 fb b2 5a 2a 4b a9
> 1e 87 29 f5 91 a3 d4 7d b9 4a fd 95 d7 1c 63 ff
> c2 2e 2e 97 9b 2f aa ae ea e4 ad 8e b2 66 cd da
> ff 6f a3 fb ab 38 64 a7 bd 79 6a df 5d f6 5f 47
> manufacturer_id = 1
> device_id       = 126
> device_id2      = 4865
> 
> Dump the first 256 byte of flash
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 51 00 52 00 59 00 02 00 00 00 40 00 00 00 00
> 00 00 00 00 00 00 00 27 00 36 00 00 00 00 00 07
> 00 07 00 0a 00 00 00 01 00 05 00 04 00 00 00 17
> 00 01 00 00 00 05 00 00 00 01 00 7f 00 00 00 00
> 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 50 00 52 00 49 00 31 00 33 00 08 00 02 00 04
> 00 01 00 04 00 00 00 00 00 01 00 b5 00 c5 00 05
> 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01

Guess what this looks like a valid CFI table now. I can definitely see Q 
= 0x51, R = 0x52, Y = 0x59 etc. in the correct places.

>  8 MB

It looks like the rest of the commands worked as well :) All without 
changing flash_read_uchar() functionality!

> Using default environment
> 
> In:    serial
> Out:   serial
> Err:   serial
> Net:   eTSEC1, eTSEC2, eTSEC3, eTSEC4
> Hit any key to stop autoboot:  0
> =>
> -------
> This time, the flash got the correct size and can be accessed. I think
> the second dump()function I added made it work, as it worked when I
> inserted a udelay after flash_write_cmd(). 

At this point, all data indicates that flash_read_uchar() is just fine.

I am suspecting two things:

1) Your CPU might be trying to re-order writes and reads to optimize 
performance. Since the command write and data read are from different 
addresses it could do this but the write command should really finish 
before we access the table data. So, we might need to add powerpc "sync" 
instructions in flash_write_cmd() function. Apparently CONFIG_BLACKFIN 
has a similar need as well. I would add a generic "sync" for the whole 
PowerPC family but there is not a conveninent CONFIG_POWERPC macro as 
far as I can see. Anyway, try adding:

asm("sync;");

statements in that function. Use Blackfin as an example.

2) Because you have a rather fast CPU it is possible that we are not 
allowing enough time after reset command is executed before array is in 
read mode. According to the datasheet of your flash part if external 
reset signal was asserted you could need up to 20usec before flash 
returns to read mode. I am not sure if this delay is needed for issued 
reset commands as well. We might need to add some delay after flash 
reset commands. Try adding udelay(100); right after flash reset command 
is sent. You can locate flash reset commands by searching 
flash_write_cmd() statements that passes FLASH_CMD_RESET, AMD_CMD_RESET 
or cfi->cmd_reset as the last argument.

> Please shed some lights. Thanks a lot!

I hope this helps.

Best regards,
Tolunay

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-02-01  5:26                                           ` Tolunay Orkun
@ 2007-02-01 22:06                                             ` Haiying Wang
  2007-02-01 22:52                                               ` Chris Fester
  2007-02-09 17:47                                             ` [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd Haiying Wang
                                                               ` (2 subsequent siblings)
  3 siblings, 1 reply; 55+ messages in thread
From: Haiying Wang @ 2007-02-01 22:06 UTC (permalink / raw)
  To: u-boot

> Except for "vendor" everything looks OK here. I should have asked you to 
> print vendor right before flash_read_jedec_ids() call. It was not yet 
> initialized when printed. It must be correctly set as well since 
> flash_read_jedec_ids() uses it would not have returned correct data 
> otherwise.
My fault, vendor is 2 when I put the printf to the correct place.:-)

> You have a 16-bit flash accessible on 16-bit bus. The flash manufacturer 
> is "AMD/Spansion" or compatible and device id is 7E1301 which is correct 
> for your part:
> 
> http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/25538a1.pdf
> 
> Please note that flash_read_jedec_ids() is used for manufacturer id and 
> device_ids and this function in turn is using flash_read_uchar() which 
> worked just fine for you.
Thanks.

> > 124 erase regions found, only 4 used
> 
> This is not correct. This is why you are blaming the flash_read_uchar() 
> function. But since it worked correctly before the failure to read the 
> number of erase regions is probably not due to the function.
I agree with you that the failure is not due to the function
(flash_read_uchar()). We thought before that it was better to modify
this function so that flash with different port-width could be accessed
safer.

> You demonstrated that this did not affect you because the output was the 
> same. The problem with this line was that info->cmd_reset was not yet 
> initialized when flash_detect_cfi() was called. This is a minor bug 
> since the flash was not in query mode yet. I think we can safely remove 
> this call as well or replace it both amd anf intel reset commands back 
> to back. But anyway, back to your issue.
Understood.:)

> It looks like the rest of the commands worked as well :) All without 
> changing flash_read_uchar() functionality!
Yes. But in our previous tests, without adding code in between
flash_write_cmd() and flash_read_uchar()(for num_erase_region), just
adding some code elsewhere, the flash worked sometimes and failed
sometimes. Another case is that, one of our customer pointed that on
their test, flash could work with ELDK 1.3.0 and failed at ELDK 1.4.0. I
just suspect that the different compiler may have difference on
optimization thus affected the instruction execution sequence.

> At this point, all data indicates that flash_read_uchar() is just fine.
> 
> I am suspecting two things:
> 
> 1) Your CPU might be trying to re-order writes and reads to optimize 
> performance. Since the command write and data read are from different 
> addresses it could do this but the write command should really finish 
> before we access the table data. So, we might need to add powerpc "sync" 
> instructions in flash_write_cmd() function. Apparently CONFIG_BLACKFIN 
> has a similar need as well. I would add a generic "sync" for the whole 
> PowerPC family but there is not a conveninent CONFIG_POWERPC macro as 
> far as I can see. Anyway, try adding:
> 
> asm("sync;");
> 
> statements in that function. Use Blackfin as an example.
> 
> 2) Because you have a rather fast CPU it is possible that we are not 
> allowing enough time after reset command is executed before array is in 
> read mode. According to the datasheet of your flash part if external 
> reset signal was asserted you could need up to 20usec before flash 
> returns to read mode. I am not sure if this delay is needed for issued 
> reset commands as well. We might need to add some delay after flash 
> reset commands. Try adding udelay(100); right after flash reset command 
> is sent. You can locate flash reset commands by searching 
> flash_write_cmd() statements that passes FLASH_CMD_RESET, AMD_CMD_RESET 
> or cfi->cmd_reset as the last argument.
Your suggestions are very valuable. I did think about the flash response
time for reset command, but did not figure out where and why was
reasonable.I am very appreciated for your kind help on this. We will try
both suggestions and do enough tests before sending out a new patch for
review.:-)
 
> I hope this helps.
Many thanks again.

Haiying

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
  2007-02-01 22:06                                             ` Haiying Wang
@ 2007-02-01 22:52                                               ` Chris Fester
  0 siblings, 0 replies; 55+ messages in thread
From: Chris Fester @ 2007-02-01 22:52 UTC (permalink / raw)
  To: u-boot

Haiying,

I may be able to test your code on my MPC8641hpcn here.  It's sounding
like the strategically placed sync's are the way to go, especially given
the differences I saw with compilers on this bug.  I'm sharing the box
with another developer at the moment, but I definitely have an interest
in getting this portion of the code rock solid.  Please contact me
off-list if you want help testing.

Thanks Tolunay, Haiying, and everyone else on this thread for working so
dilligently on this!  :)

Chris

On Thu, 2007-02-01 at 17:06 -0500, Haiying Wang wrote:
> Your suggestions are very valuable. I did think about the flash response
> time for reset command, but did not figure out where and why was
> reasonable.I am very appreciated for your kind help on this. We will try
> both suggestions and do enough tests before sending out a new patch for
> review.:-)
>  
> > I hope this helps.
> Many thanks again.
> 
> Haiying

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd
  2007-02-01  5:26                                           ` Tolunay Orkun
  2007-02-01 22:06                                             ` Haiying Wang
@ 2007-02-09 17:47                                             ` Haiying Wang
  2007-02-09 19:42                                               ` Wolfgang Denk
  2007-02-09 17:47                                             ` [U-Boot-Users] [PATCH 1/2] Add DO_SYNC at the end of flash_write_cmd Haiying Wang
  2007-02-09 17:47                                             ` [U-Boot-Users] [PATCH 2/2] Define DO_SYNC in each CPU's header file Haiying Wang
  3 siblings, 1 reply; 55+ messages in thread
From: Haiying Wang @ 2007-02-09 17:47 UTC (permalink / raw)
  To: u-boot

Because SYNC is already defined in include/ppc_asm.tmpl for some ppc
based CPUs to use, I use DO_SYNC instead of SYNC for this patch.

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH 1/2] Add DO_SYNC at the end of flash_write_cmd
  2007-02-01  5:26                                           ` Tolunay Orkun
  2007-02-01 22:06                                             ` Haiying Wang
  2007-02-09 17:47                                             ` [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd Haiying Wang
@ 2007-02-09 17:47                                             ` Haiying Wang
  2007-02-09 19:45                                               ` Wolfgang Denk
  2007-02-09 17:47                                             ` [U-Boot-Users] [PATCH 2/2] Define DO_SYNC in each CPU's header file Haiying Wang
  3 siblings, 1 reply; 55+ messages in thread
From: Haiying Wang @ 2007-02-09 17:47 UTC (permalink / raw)
  To: u-boot

Make sure the flash write command is fully finished and remove the unnecessary ssync operation which should be moved to it CPU header file

Signed-off-by: Haiying Wang <haiying.wang@freescale.com>
---
 drivers/cfi_flash.c |   15 +++------------
 1 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/cfi_flash.c b/drivers/cfi_flash.c
index 696f9a4..c686e53 100644
--- a/drivers/cfi_flash.c
+++ b/drivers/cfi_flash.c
@@ -931,27 +931,18 @@ static void flash_write_cmd (flash_info_t * info, flash_sect_t sect, uint offset
 		debug ("fwc addr %p cmd %x %x 8bit x %d bit\n", addr.cp, cmd,
 		       cword.c, info->chipwidth << CFI_FLASH_SHIFT_WIDTH);
 		*addr.cp = cword.c;
-#ifdef CONFIG_BLACKFIN
-		asm("ssync;");
-#endif
 		break;
 	case FLASH_CFI_16BIT:
 		debug ("fwc addr %p cmd %x %4.4x 16bit x %d bit\n", addr.wp,
 		       cmd, cword.w,
 		       info->chipwidth << CFI_FLASH_SHIFT_WIDTH);
 		*addr.wp = cword.w;
-#ifdef CONFIG_BLACKFIN
-		asm("ssync;");
-#endif
 		break;
 	case FLASH_CFI_32BIT:
 		debug ("fwc addr %p cmd %x %8.8lx 32bit x %d bit\n", addr.lp,
 		       cmd, cword.l,
 		       info->chipwidth << CFI_FLASH_SHIFT_WIDTH);
 		*addr.lp = cword.l;
-#ifdef CONFIG_BLACKFIN
-		asm("ssync;");
-#endif
 		break;
 	case FLASH_CFI_64BIT:
 #ifdef DEBUG
@@ -966,11 +957,11 @@ static void flash_write_cmd (flash_info_t * info, flash_sect_t sect, uint offset
 		}
 #endif
 		*addr.llp = cword.ll;
-#ifdef CONFIG_BLACKFIN
-		asm("ssync;");
-#endif
 		break;
 	}
+
+	/* Ensure all the instruction are finished */
+	DO_SYNC;
 }
 
 static void flash_unlock_seq (flash_info_t * info, flash_sect_t sect)
-- 
1.4.4.4

^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH 2/2] Define DO_SYNC in each CPU's header file
  2007-02-01  5:26                                           ` Tolunay Orkun
                                                               ` (2 preceding siblings ...)
  2007-02-09 17:47                                             ` [U-Boot-Users] [PATCH 1/2] Add DO_SYNC at the end of flash_write_cmd Haiying Wang
@ 2007-02-09 17:47                                             ` Haiying Wang
  2007-02-09 19:46                                               ` Wolfgang Denk
  3 siblings, 1 reply; 55+ messages in thread
From: Haiying Wang @ 2007-02-09 17:47 UTC (permalink / raw)
  To: u-boot

For the arches which need sync in flash_write_cmd, DO_SYNC will be defined. Otherwise, it is a dummy stub for now.

Signed-off-by: Haiying Wang <haiying.wang@freescale.com>
---
 include/asm-arm/processor.h      |    2 ++
 include/asm-avr32/processor.h    |    2 ++
 include/asm-blackfin/processor.h |    2 ++
 include/asm-i386/processor.h     |    3 +++
 include/asm-m68k/processor.h     |    2 ++
 include/asm-mips/processor.h     |    2 ++
 include/asm-nios2/processor.h    |    3 +++
 include/asm-ppc/processor.h      |    2 ++
 8 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/include/asm-arm/processor.h b/include/asm-arm/processor.h
index 445d449..5b61eee 100644
--- a/include/asm-arm/processor.h
+++ b/include/asm-arm/processor.h
@@ -11,6 +11,8 @@
 #ifndef __ASM_ARM_PROCESSOR_H
 #define __ASM_ARM_PROCESSOR_H
 
+#define DO_SYNC		/* dummy stub */
+
 /*
  * Default implementation of macro that returns current
  * instruction pointer ("program counter").
diff --git a/include/asm-avr32/processor.h b/include/asm-avr32/processor.h
index cc59dfa..94f3432 100644
--- a/include/asm-avr32/processor.h
+++ b/include/asm-avr32/processor.h
@@ -22,6 +22,8 @@
 #ifndef __ASM_AVR32_PROCESSOR_H
 #define __ASM_AVR32_PROCESSOR_H
 
+#define DO_SYNC		/* dummy stub */
+
 #ifndef __ASSEMBLY__
 
 #define current_text_addr() ({ void *pc; __asm__("mov %0,pc" : "=r"(pc)); pc; })
diff --git a/include/asm-blackfin/processor.h b/include/asm-blackfin/processor.h
index 19bd720..4cd0be5 100644
--- a/include/asm-blackfin/processor.h
+++ b/include/asm-blackfin/processor.h
@@ -30,6 +30,8 @@
 #ifndef __ASM_BLACKFIN_PROCESSOR_H
 #define __ASM_BLACKFIN_PROCESSOR_H
 
+#define DO_SYNC		asm("ssync;")
+
 /*
  * Default implementation of macro that returns current
  * instruction pointer ("program counter").
diff --git a/include/asm-i386/processor.h b/include/asm-i386/processor.h
index 5dedba8..64633d2 100644
--- a/include/asm-i386/processor.h
+++ b/include/asm-i386/processor.h
@@ -26,4 +26,7 @@
 /* Currently this header is unused in the i386 port
  * but some generic files #include <asm/processor.h>
  * so this file is a placeholder. */
+
+#define DO_SYNC		/* dummy stub */
+
 #endif
diff --git a/include/asm-m68k/processor.h b/include/asm-m68k/processor.h
index 3fafa6f..c49dab6 100644
--- a/include/asm-m68k/processor.h
+++ b/include/asm-m68k/processor.h
@@ -1,6 +1,8 @@
 #ifndef __ASM_M68K_PROCESSOR_H
 #define __ASM_M68K_PROCESSOR_H
 
+#define DO_SYNC		/* dummy stub */
+
 #include <asm/ptrace.h>
 #include <asm/types.h>
 
diff --git a/include/asm-mips/processor.h b/include/asm-mips/processor.h
index 6838aee..4ad29e2 100644
--- a/include/asm-mips/processor.h
+++ b/include/asm-mips/processor.h
@@ -11,6 +11,8 @@
 #ifndef _ASM_PROCESSOR_H
 #define _ASM_PROCESSOR_H
 
+#define DO_SYNC		/* dummy stub */
+
 #include <linux/config.h>
 
 #include <asm/isadep.h>
diff --git a/include/asm-nios2/processor.h b/include/asm-nios2/processor.h
index 68502a5..825e7b2 100644
--- a/include/asm-nios2/processor.h
+++ b/include/asm-nios2/processor.h
@@ -23,4 +23,7 @@
 
 #ifndef __ASM_NIOS2_PROCESSOR_H_
 #define __ASM_NIOS2_PROCESSOR_H_
+
+#define DO_SYNC		/* dummy stub */
+
 #endif /* __ASM_NIOS2_PROCESSOR_H_ */
diff --git a/include/asm-ppc/processor.h b/include/asm-ppc/processor.h
index f102600..7843061 100644
--- a/include/asm-ppc/processor.h
+++ b/include/asm-ppc/processor.h
@@ -1,6 +1,8 @@
 #ifndef __ASM_PPC_PROCESSOR_H
 #define __ASM_PPC_PROCESSOR_H
 
+#define DO_SYNC		asm("sync;")
+
 /*
  * Default implementation of macro that returns current
  * instruction pointer ("program counter").
-- 
1.4.4.4

^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd
  2007-02-09 17:47                                             ` [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd Haiying Wang
@ 2007-02-09 19:42                                               ` Wolfgang Denk
  2007-02-09 19:48                                                 ` Haiying Wang
  2007-02-09 19:59                                                 ` Haavard Skinnemoen
  0 siblings, 2 replies; 55+ messages in thread
From: Wolfgang Denk @ 2007-02-09 19:42 UTC (permalink / raw)
  To: u-boot

In message <1171043255.3932.20.camel@udp097531uds.am.freescale.net> you wrote:
> Because SYNC is already defined in include/ppc_asm.tmpl for some ppc
> based CPUs to use, I use DO_SYNC instead of SYNC for this patch.

AFAICT that's a general definition which is not specific to "some ppc
based CPUs" only. Rather than redefining it why  not  just  use  this
definition? It seems to match our purposes.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There are two ways to write error-free programs. Only the  third  one
works.

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH 1/2] Add DO_SYNC at the end of flash_write_cmd
  2007-02-09 17:47                                             ` [U-Boot-Users] [PATCH 1/2] Add DO_SYNC at the end of flash_write_cmd Haiying Wang
@ 2007-02-09 19:45                                               ` Wolfgang Denk
  0 siblings, 0 replies; 55+ messages in thread
From: Wolfgang Denk @ 2007-02-09 19:45 UTC (permalink / raw)
  To: u-boot

In message <1171043258.3932.21.camel@udp097531uds.am.freescale.net> you wrote:
> Make sure the flash write command is fully finished and remove the unnecessary ssync operation which should be moved to it CPU header file
...
> +	DO_SYNC;

Please use SYNC (instead of DO_SYNC) as discussed.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
On a clear disk you can seek forever.

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH 2/2] Define DO_SYNC in each CPU's header file
  2007-02-09 17:47                                             ` [U-Boot-Users] [PATCH 2/2] Define DO_SYNC in each CPU's header file Haiying Wang
@ 2007-02-09 19:46                                               ` Wolfgang Denk
  2007-02-10  7:17                                                 ` Tolunay Orkun
  0 siblings, 1 reply; 55+ messages in thread
From: Wolfgang Denk @ 2007-02-09 19:46 UTC (permalink / raw)
  To: u-boot

In message <1171043259.3932.23.camel@udp097531uds.am.freescale.net> you wrote:
> For the arches which need sync in flash_write_cmd, DO_SYNC will be defined. Otherwise, it is a dummy stub for now.
> 

Please don't break definition and use of this into separate  patches.
This is ONE change, and should be sumbitted as one commit only.

And use SYNC as discussed, please.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Imagination is more important than knowledge.      -- Albert Einstein

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd
  2007-02-09 19:42                                               ` Wolfgang Denk
@ 2007-02-09 19:48                                                 ` Haiying Wang
  2007-02-10  1:04                                                   ` Wolfgang Denk
  2007-02-09 19:59                                                 ` Haavard Skinnemoen
  1 sibling, 1 reply; 55+ messages in thread
From: Haiying Wang @ 2007-02-09 19:48 UTC (permalink / raw)
  To: u-boot

SYNC is defined as 
"	#define SYNC \
        sync; \
        isync
" in include/ppc_asm.tmpl,

and can not be used by .c file, am I right? :-). In fact it is used by
the start.S file for 85xx/83xx/8xx/4xx/5xxx/74xx_7xx.

We need to define SYNC as asm("sync;").

Haiying


On Fri, 2007-02-09 at 20:42 +0100, Wolfgang Denk wrote:
> In message <1171043255.3932.20.camel@udp097531uds.am.freescale.net> you wrote:
> > Because SYNC is already defined in include/ppc_asm.tmpl for some ppc
> > based CPUs to use, I use DO_SYNC instead of SYNC for this patch.
> 
> AFAICT that's a general definition which is not specific to "some ppc
> based CPUs" only. Rather than redefining it why  not  just  use  this
> definition? It seems to match our purposes.
> 
> Best regards,
> 
> Wolfgang Denk
> 

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd
  2007-02-09 19:42                                               ` Wolfgang Denk
  2007-02-09 19:48                                                 ` Haiying Wang
@ 2007-02-09 19:59                                                 ` Haavard Skinnemoen
  2007-02-10  1:02                                                   ` Wolfgang Denk
  1 sibling, 1 reply; 55+ messages in thread
From: Haavard Skinnemoen @ 2007-02-09 19:59 UTC (permalink / raw)
  To: u-boot

On 2/9/07, Wolfgang Denk <wd@denx.de> wrote:
> In message <1171043255.3932.20.camel@udp097531uds.am.freescale.net> you wrote:
> > Because SYNC is already defined in include/ppc_asm.tmpl for some ppc
> > based CPUs to use, I use DO_SYNC instead of SYNC for this patch.
>
> AFAICT that's a general definition which is not specific to "some ppc
> based CPUs" only. Rather than redefining it why  not  just  use  this
> definition? It seems to match our purposes.

Except that it's not written in C ;-)

Haavard

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd
  2007-02-09 19:59                                                 ` Haavard Skinnemoen
@ 2007-02-10  1:02                                                   ` Wolfgang Denk
  0 siblings, 0 replies; 55+ messages in thread
From: Wolfgang Denk @ 2007-02-10  1:02 UTC (permalink / raw)
  To: u-boot

In message <1defaf580702091159s2e2fc9e7wf962d4fff0ec985c@mail.gmail.com> you wrote:
>
> > > Because SYNC is already defined in include/ppc_asm.tmpl for some ppc
> > > based CPUs to use, I use DO_SYNC instead of SYNC for this patch.
> >
> > AFAICT that's a general definition which is not specific to "some ppc
> > based CPUs" only. Rather than redefining it why  not  just  use  this
> > definition? It seems to match our purposes.
> 
> Except that it's not written in C ;-)

You cannot write this in C anyway. C has no concept of such things.
You will always end up withpulling some "asm" tricks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
... not meant for the weak-at-heart: /^(?=.*one)(?=.*two)/
If you can follow that, you can use it.
          - Randal L. Schwartz in <ukpw67rhl3.fsf@julie.teleport.com>

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd
  2007-02-09 19:48                                                 ` Haiying Wang
@ 2007-02-10  1:04                                                   ` Wolfgang Denk
  2007-02-10  7:23                                                     ` Tolunay Orkun
  0 siblings, 1 reply; 55+ messages in thread
From: Wolfgang Denk @ 2007-02-10  1:04 UTC (permalink / raw)
  To: u-boot

In message <1171050483.3932.31.camel@udp097531uds.am.freescale.net> you wrote:
> SYNC is defined as 
> "	#define SYNC \
>         sync; \
>         isync
> " in include/ppc_asm.tmpl,
> 
> and can not be used by .c file, am I right? :-). In fact it is used by
> the start.S file for 85xx/83xx/8xx/4xx/5xxx/74xx_7xx.
> 
> We need to define SYNC as asm("sync;").

Or, to be sure, ""sync;isync"

Where is the problem? Which code includes include/ppc_asm.tmpl ?  Why
cannot  we  have  the  same  definition  once  for  C  and  again for
assembler?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You can do this in a number of ways.     IBM chose to do all of them.
Why do you find that funny?        -- D. Taylor, Computer Science 350

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH 2/2] Define DO_SYNC in each CPU's header file
  2007-02-09 19:46                                               ` Wolfgang Denk
@ 2007-02-10  7:17                                                 ` Tolunay Orkun
  0 siblings, 0 replies; 55+ messages in thread
From: Tolunay Orkun @ 2007-02-10  7:17 UTC (permalink / raw)
  To: u-boot

Wolfgang Denk wrote:
> In message <1171043259.3932.23.camel@udp097531uds.am.freescale.net> you wrote:
>> For the arches which need sync in flash_write_cmd, DO_SYNC will be defined. Otherwise, it is a dummy stub for now.
>>
> 
> Please don't break definition and use of this into separate  patches.

Actually he originally created a single patch. I asked him to come up 
with two patches particularly because one crosses multiple custodians 
and another only one.

Combined is OK for me. Actually, combined is probably easier now that we 
will be maintaining different trees.

> This is ONE change, and should be sumbitted as one commit only.
> 
> And use SYNC as discussed, please.

Yes.

> 
> Best regards,
> 
> Wolfgang Denk
> 

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd
  2007-02-10  1:04                                                   ` Wolfgang Denk
@ 2007-02-10  7:23                                                     ` Tolunay Orkun
  2007-02-10  7:40                                                       ` Stefan Roese
  0 siblings, 1 reply; 55+ messages in thread
From: Tolunay Orkun @ 2007-02-10  7:23 UTC (permalink / raw)
  To: u-boot

Wolfgang Denk wrote:
> In message <1171050483.3932.31.camel@udp097531uds.am.freescale.net> you wrote:
>> SYNC is defined as 
>> "	#define SYNC \
>>         sync; \
>>         isync
>> " in include/ppc_asm.tmpl,
>>
>> and can not be used by .c file, am I right? :-). In fact it is used by
>> the start.S file for 85xx/83xx/8xx/4xx/5xxx/74xx_7xx.
>>
>> We need to define SYNC as asm("sync;").
> 
> Or, to be sure, ""sync;isync"

OK.

> 
> Where is the problem? Which code includes include/ppc_asm.tmpl ?  Why
> cannot  we  have  the  same  definition  once  for  C  and  again for
> assembler?

I agree, I think we can define the equivalent one in a C header file

#define SYNC asm("sync; isync;")

I am not sure if the assembler one is ever included in the C code.

Tolunay

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd
  2007-02-10  7:23                                                     ` Tolunay Orkun
@ 2007-02-10  7:40                                                       ` Stefan Roese
  2007-02-10  7:57                                                         ` Tolunay Orkun
                                                                           ` (2 more replies)
  0 siblings, 3 replies; 55+ messages in thread
From: Stefan Roese @ 2007-02-10  7:40 UTC (permalink / raw)
  To: u-boot

On Saturday 10 February 2007 08:23, Tolunay Orkun wrote:
> >> We need to define SYNC as asm("sync;").
> >
> > Or, to be sure, ""sync;isync"
>
> OK.

I would not do this. Please let a "sync" instruction _not_ do a "isync" too. 
There will be times when you explicitly _don't_ what this.

> > Where is the problem? Which code includes include/ppc_asm.tmpl ?  Why
> > cannot  we  have  the  same  definition  once  for  C  and  again for
> > assembler?
>
> I agree, I think we can define the equivalent one in a C header file
>
> #define SYNC asm("sync; isync;")
>
> I am not sure if the assembler one is ever included in the C code.

Why not use

#define sync()  __asm__ __volatile__ ("sync" : : : "memory");

from include/asm-ppc/io.h? This seems to be exactly what we need.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
=====================================================================

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd
  2007-02-10  7:40                                                       ` Stefan Roese
@ 2007-02-10  7:57                                                         ` Tolunay Orkun
  2007-02-10  8:07                                                           ` Stefan Roese
  2007-02-10 21:54                                                         ` Wolfgang Denk
  2007-02-11  2:34                                                         ` Timur Tabi
  2 siblings, 1 reply; 55+ messages in thread
From: Tolunay Orkun @ 2007-02-10  7:57 UTC (permalink / raw)
  To: u-boot

Stefan Roese wrote:
> On Saturday 10 February 2007 08:23, Tolunay Orkun wrote:
>>>> We need to define SYNC as asm("sync;").
>>> Or, to be sure, ""sync;isync"
>> OK.
> 
> I would not do this. Please let a "sync" instruction _not_ do a "isync" too. 
 > There will be times when you explicitly _don't_ what this.

Could you give a specific example.

There is also "eieio" and "msync" to consider though these usually map 
to former two (or vice versa).

>>> Where is the problem? Which code includes include/ppc_asm.tmpl ?  Why
>>> cannot  we  have  the  same  definition  once  for  C  and  again for
>>> assembler?
>> I agree, I think we can define the equivalent one in a C header file
>>
>> #define SYNC asm("sync; isync;")
>>
>> I am not sure if the assembler one is ever included in the C code.
> 
> Why not use
> 
> #define sync()  __asm__ __volatile__ ("sync" : : : "memory");
> 
> from include/asm-ppc/io.h? This seems to be exactly what we need.
> 

I would rather prefer an uppercase SYNC since it is a macro but whatever 
style you guys choose is OK with me.

Please you and Wolfgang decide on this matter. I do not want to confuse 
Haiying with conflicting messages.

Tolunay

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd
  2007-02-10  7:57                                                         ` Tolunay Orkun
@ 2007-02-10  8:07                                                           ` Stefan Roese
  2007-02-10 21:55                                                             ` Wolfgang Denk
  0 siblings, 1 reply; 55+ messages in thread
From: Stefan Roese @ 2007-02-10  8:07 UTC (permalink / raw)
  To: u-boot

Hi Tolunay,

On Saturday 10 February 2007 08:57, Tolunay Orkun wrote:
> > I would not do this. Please let a "sync" instruction _not_ do a "isync"
> > too.
> >
>  > There will be times when you explicitly _don't_ what this.
>
> Could you give a specific example.

The "isync" will reload the TLB's on 440's for example and sometimes you 
really don't want this on a "normal" sync instruction.

> There is also "eieio" and "msync" to consider though these usually map
> to former two (or vice versa).

Yes. But I find the name "sync" more platform generic.

> > Why not use
> >
> > #define sync()  __asm__ __volatile__ ("sync" : : : "memory");
> >
> > from include/asm-ppc/io.h? This seems to be exactly what we need.
>
> I would rather prefer an uppercase SYNC since it is a macro but whatever
> style you guys choose is OK with me.

Could be that on other platforms this sync is not a define but a function. But 
I don't have a strong preference here. Wolfgang, you decide. ;-)

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
=====================================================================

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd
  2007-02-10  7:40                                                       ` Stefan Roese
  2007-02-10  7:57                                                         ` Tolunay Orkun
@ 2007-02-10 21:54                                                         ` Wolfgang Denk
  2007-02-11  2:34                                                         ` Timur Tabi
  2 siblings, 0 replies; 55+ messages in thread
From: Wolfgang Denk @ 2007-02-10 21:54 UTC (permalink / raw)
  To: u-boot

In message <200702100840.46587.sr@denx.de> you wrote:
>
> > > Or, to be sure, ""sync;isync"
> 
> I would not do this. Please let a "sync" instruction _not_ do a "isync" too. 
> There will be times when you explicitly _don't_ what this.

Note that the current assember #define SYNC does exactly what I quoted
above...

> Why not use
> 
> #define sync()  __asm__ __volatile__ ("sync" : : : "memory");
> 
> from include/asm-ppc/io.h? This seems to be exactly what we need.

Indeed. Let's use that!

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
One difference between a man and a machine is that a machine is quiet
when well oiled.

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd
  2007-02-10  8:07                                                           ` Stefan Roese
@ 2007-02-10 21:55                                                             ` Wolfgang Denk
  0 siblings, 0 replies; 55+ messages in thread
From: Wolfgang Denk @ 2007-02-10 21:55 UTC (permalink / raw)
  To: u-boot

In message <200702100907.37451.sr@denx.de> you wrote:
> 
> Could be that on other platforms this sync is not a define but a function. But 
> I don't have a strong preference here. Wolfgang, you decide. ;-)

I like sync(); better.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
All these theories, diverse as they are, have two things  in  common:
they explain the observed facts, and they are completeley and utterly
wrong.                       - Terry Pratchett, _The Light Fantastic_

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd
  2007-02-10  7:40                                                       ` Stefan Roese
  2007-02-10  7:57                                                         ` Tolunay Orkun
  2007-02-10 21:54                                                         ` Wolfgang Denk
@ 2007-02-11  2:34                                                         ` Timur Tabi
  2007-02-11 10:23                                                           ` Wolfgang Denk
  2 siblings, 1 reply; 55+ messages in thread
From: Timur Tabi @ 2007-02-11  2:34 UTC (permalink / raw)
  To: u-boot

Stefan Roese wrote:

> Why not use
> 
> #define sync()  __asm__ __volatile__ ("sync" : : : "memory");

I like this idea, but is that semicolon supposed to be there?  Shouldn't 
we make it an inline function instead of a macro?

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd
  2007-02-11  2:34                                                         ` Timur Tabi
@ 2007-02-11 10:23                                                           ` Wolfgang Denk
  0 siblings, 0 replies; 55+ messages in thread
From: Wolfgang Denk @ 2007-02-11 10:23 UTC (permalink / raw)
  To: u-boot

In message <45CE80A1.5050206@freescale.com> you wrote:
> 
> > #define sync()  __asm__ __volatile__ ("sync" : : : "memory");
> 
> I like this idea, but is that semicolon supposed to be there?  Shouldn't 
> we make it an inline function instead of a macro?

No, and yes, please.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Felson's Law:
	To steal ideas from one person is plagiarism; to steal from
	many is research.

^ permalink raw reply	[flat|nested] 55+ messages in thread

end of thread, other threads:[~2007-02-11 10:23 UTC | newest]

Thread overview: 55+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-12-22  7:27 [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug wei.zhang at freescale.com
2006-12-22 10:00 ` Wolfgang Denk
2006-12-22 11:02   ` Zhang Wei
2006-12-22 14:11     ` Wolfgang Denk
2006-12-22 16:34       ` [U-Boot-Users] 答复: " Zhang Wei-r63237
2006-12-22 17:17         ` Wolfgang Denk
2006-12-25  7:03           ` Zhang Wei
2006-12-25 23:24             ` Wolfgang Denk
2006-12-26  6:04               ` Zhang Wei
2007-01-04  2:07                 ` Zhang Wei-r63237
2007-01-04  8:17                   ` Wolfgang Denk
2007-01-04  8:36               ` Zhang Wei-r63237
2007-01-04  9:23                 ` Wolfgang Denk
2007-01-04 11:19                   ` Stefan Roese
2007-01-05 13:27                   ` Stefan Roese
2007-01-07 10:12                     ` Tolunay Orkun
2007-01-07 10:40                       ` Wolfgang Denk
2007-01-13  7:11                         ` Tolunay Orkun
2007-01-13 17:53                           ` Håvard Skinnemoen
2007-01-16  6:52                             ` Stefan Roese
2007-01-25  4:38                           ` Zhang Wei-r63237
2007-01-25 16:10                             ` Timur Tabi
2007-01-25 20:33                               ` Wolfgang Denk
2007-01-26  2:13                                 ` Zhang Wei-r63237
2007-01-29 11:29                                   ` Tolunay Orkun
2007-01-30  3:36                                     ` Wang Haiying-r54964
2007-01-31  9:01                                       ` Tolunay Orkun
2007-01-31 19:25                                         ` Haiying Wang
2007-02-01  5:26                                           ` Tolunay Orkun
2007-02-01 22:06                                             ` Haiying Wang
2007-02-01 22:52                                               ` Chris Fester
2007-02-09 17:47                                             ` [U-Boot-Users] [PATCH0/2] Re-do the patch for adding DO_SYNC in flash_write_cmd Haiying Wang
2007-02-09 19:42                                               ` Wolfgang Denk
2007-02-09 19:48                                                 ` Haiying Wang
2007-02-10  1:04                                                   ` Wolfgang Denk
2007-02-10  7:23                                                     ` Tolunay Orkun
2007-02-10  7:40                                                       ` Stefan Roese
2007-02-10  7:57                                                         ` Tolunay Orkun
2007-02-10  8:07                                                           ` Stefan Roese
2007-02-10 21:55                                                             ` Wolfgang Denk
2007-02-10 21:54                                                         ` Wolfgang Denk
2007-02-11  2:34                                                         ` Timur Tabi
2007-02-11 10:23                                                           ` Wolfgang Denk
2007-02-09 19:59                                                 ` Haavard Skinnemoen
2007-02-10  1:02                                                   ` Wolfgang Denk
2007-02-09 17:47                                             ` [U-Boot-Users] [PATCH 1/2] Add DO_SYNC at the end of flash_write_cmd Haiying Wang
2007-02-09 19:45                                               ` Wolfgang Denk
2007-02-09 17:47                                             ` [U-Boot-Users] [PATCH 2/2] Define DO_SYNC in each CPU's header file Haiying Wang
2007-02-09 19:46                                               ` Wolfgang Denk
2007-02-10  7:17                                                 ` Tolunay Orkun
2007-01-08  2:41                     ` [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug Zhang Wei-r63237
2006-12-22 17:07       ` Chris Fester
2006-12-22 17:24         ` Wolfgang Denk
2006-12-22 17:42           ` Chris Fester
2006-12-22 21:33             ` Wolfgang Denk

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.