cfi_flash: fix flash on Big Endian machines.

PATCH New Stefan Roese 4 participants 11 messages Jul 15, 2008 – Jul 17, 2008 1 message not in the archive Memory technology devices (MTD)

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Stefan Roese

author

2008-07-15 08:46 UTC #1 lore raw

PATCH 1/1 cfi_flash: fix flash on Big Endian machines.

Hi Sebastian,

Quoted 5 lines · Sebastian Siewior

This got broken by commits 93c56f212c
[cfi_flash: support of long cmd in U-boot.]
That command seems to be access in a little endian way so wrappers are required.

Please explain why this is the case? Little endian wrappers on big endian machines for FLASH access? This sounds wrong to me.

Quoted 3 lines

Long is the wrong type because it will behave differently on 64bit machnines in a way that is probably not expected. int should be enough.

Yes. I suggest that you move this long/int issue to a separate patch. It isn't related to this endian issue.

And to you main patch:

NAK. After applying this, CFI support on kilauea (PPC405EX with 1* Spansion S29GL512N, 16bit wide) breaks:

U-Boot 1.3.3-02016-g3ed7287 (Jul 15 2008 - 10:37:58)

CPU: AMCC PowerPC 405EX Rev. C at 533.333 MHz (PLB=177, OPB=88, EBC=88 MHz)
Security support
Bootstrap Option H - Boot ROM Location I2C (Addr 0x52) 16 kB I-Cache 16 kB D-Cache

Board: Kilauea - AMCC PPC405EX Evaluation Board
I2C:   ready
DTT1:  34 C
DRAM:  256 MB
FLASH: CFI: Unknown command set 0x0

## Unknown FLASH on Bank 1 - Size = 0x00000000 = 0 MB *** failed ***
### ERROR ### Please RESET the board ###

Please explain what exactly you need to fix, perhaps with an example. The current version breaks other boards.

/me fetches the BDI to reflash the kilauea...

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de
=====================================================================

Sebastian Siewior

2008-07-15 09:36 UTC #2 lore raw

PATCH 1/1 cfi_flash: fix flash on Big Endian machines.

Quoted 1 line · * Stefan Roese | 2008-07-15 10:46:33 [+0200]:

Hi Sebastian,

Hi Stefan,

Quoted 9 lines · Sebastian Siewior

Quoted 5 lines
This got broken by commits 93c56f212c
[cfi_flash: support of long cmd in U-boot.]
That command seems to be access in a little endian way so wrappers are required.
Please explain why this is the case? Little endian wrappers on big endian machines for FLASH access? This sounds wrong to me.

Look at this snippet from flash_make_cmd (my case: big endian & CFG_WRITE_SWAPPED_DATA):

|         for (i = info->portwidth; i > 0; i--){
|                 cword_offset = (info->portwidth-i)%info->chipwidth;
|                 cp_offset = info->portwidth - i;
|                 val = *((uchar*)&cmd + cword_offset);
|                 cp[cp_offset] = (cword_offset >= sizeof(uint)) ? 0x00 : val;
|        }
|

Now, 8bit access, 8bit bus: portwidth = 1, chipwidth = 1. I am only once in this for loop and I get the MSB byte of cmd which is 0x00 instead of the command which is never touched.

Quoted 6 lines

Quoted 3 lines
Long is the wrong type because it will behave differently on 64bit machnines in a way that is probably not expected. int should be enough.
Yes. I suggest that you move this long/int issue to a separate patch. It isn't related to this endian issue.

Right.

Quoted 4 lines

And to you main patch:
NAK. After applying this, CFI support on kilauea (PPC405EX with 1* Spansion S29GL512N, 16bit wide) breaks:

Is this big endian + CFG_WRITE_SWAPPED_DATA? Did it work before the patch?

Quoted 1 line

FLASH: CFI: Unknown command set 0x0

The command has not been sent but MSB which was 0x00.

Quoted 1 line

## Unknown FLASH on Bank 1 - Size = 0x00000000 = 0 MB

That's the message I got before my patch. There is probably still something wrong.

Quoted 2 lines

Please explain what exactly you need to fix, perhaps with an example. The current version breaks other boards.

I attached a file which can be used for testing. On your little endian machine you should have __LITTLE_ENDIAN defined and your result should be 0xf0 in the case i = 0. Right now, I get 0x00 on my BE machine while LE case is working fine.

Sebastian
-------------- next part --------------
#include <stdio.h>
#include <string.h>

int portwidth = 1;

int chipwidth = 1;

static void flash_make_cmd(unsigned int cmd, void *cmdbuf)

{
	int i;
	int cword_offset;
	int cp_offset;
	unsigned char val;
	unsigned char *cp = (unsigned char *) cmdbuf;

	for (i = portwidth; i > 0; i--){
		cword_offset = (portwidth-i)%chipwidth;
#if defined(__LITTLE_ENDIAN) || defined(CFG_WRITE_SWAPPED_DATA)
		cp_offset = portwidth - i;
		val = *((unsigned char*)&cmd + cword_offset);
#else
		cp_offset = i - 1;
		val = *((unsigned char*)&cmd + sizeof(unsigned long) - cword_offset - 1);
#endif
		cp[cp_offset] = (cword_offset >= sizeof(unsigned long)) ? 0x00 : val;
	}
}

int main(void) {

	int i;
	unsigned char buf[8];

	memset(buf, 0, 8);
	flash_make_cmd(0xf0, &buf);
	for (i = 0; i < 8; i++)
		printf("%d -> %02x\n",
				i, buf[i]);
	return 0;
}

Stefan Roese

author

2008-07-15 09:48 UTC #3 lore raw

PATCH 1/1 cfi_flash: fix flash on Big Endian machines.

Quoted 20 lines · Sebastian Siewior

Quoted 5 lines
Quoted 2 lines
That command seems to be access in a little endian way so wrappers are required.
Please explain why this is the case? Little endian wrappers on big endian machines for FLASH access? This sounds wrong to me.
Look at this snippet from flash_make_cmd (my case: big endian &
CFG_WRITE_SWAPPED_DATA):
|         for (i = info->portwidth; i > 0; i--){
|                 cword_offset = (info->portwidth-i)%info->chipwidth;
|                 cp_offset = info->portwidth - i;
|                 val = *((uchar*)&cmd + cword_offset);
|                 cp[cp_offset] = (cword_offset >= sizeof(uint)) ? 0x00 :
| val; }
Now, 8bit access, 8bit bus: portwidth = 1, chipwidth = 1. I am only once in this for loop and I get the MSB byte of cmd which is 0x00 instead of the command which is never touched.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Long is the wrong type because it will behave differently on 64bit machnines in a way that is probably not expected. int should be enough.
Yes. I suggest that you move this long/int issue to a separate patch. It isn't related to this endian issue.
Right.

OK, I'll await a separate patch for this.

Quoted 7 lines

Quoted 4 lines
And to you main patch:
NAK. After applying this, CFI support on kilauea (PPC405EX with 1* Spansion S29GL512N, 16bit wide) breaks:
Is this big endian + CFG_WRITE_SWAPPED_DATA? Did it work before the patch?

This is big endian and CFG_WRITE_SWAPPED_DATA *not* defined. You can check this yourself, the port is officially available. And yes, of course it worked before the patch.

Quoted 6 lines

Quoted 1 line
FLASH: CFI: Unknown command set 0x0
The command has not been sent but MSB which was 0x00.
Quoted 1 line
## Unknown FLASH on Bank 1 - Size = 0x00000000 = 0 MB

Quoted 8 lines

That's the message I got before my patch. There is probably still something wrong.
Quoted 2 lines
Please explain what exactly you need to fix, perhaps with an example. The current version breaks other boards.
I attached a file which can be used for testing. On your little endian machine you should have __LITTLE_ENDIAN

Little endian? Why should I have this defined on my PPC system. It's not.

Quoted 3 lines

defined and your result should
be 0xf0 in the case i = 0. Right now, I get 0x00 on my BE machine while LE case is working fine.

I have to admit that I don't have the time to test anything right now. It seems that you have a problem with the #else path in flash_make_cmd(). Do you have access to a big endian platform which doesn't define CFG_WRITE_SWAPPED_DATA? If yes, you could test both cases.

Best regards,
Stefan

Sebastian Siewior

2008-07-15 09:57 UTC #4 lore raw

PATCH 1/1 cfi_flash: fix flash on Big Endian machines.

Quoted 1 line · * Stefan Roese | 2008-07-15 11:48:11 [+0200]:

OK, I'll await a separate patch for this.

Could be in your inbox allready :)

Quoted 11 lines

Quoted 7 lines
Quoted 4 lines
And to you main patch:
NAK. After applying this, CFI support on kilauea (PPC405EX with 1* Spansion S29GL512N, 16bit wide) breaks:
Is this big endian + CFG_WRITE_SWAPPED_DATA? Did it work before the patch?
This is big endian and CFG_WRITE_SWAPPED_DATA *not* defined. You can check this yourself, the port is officially available. And yes, of course it worked before the patch.

So I might have an idea where the bug is.

Quoted 9 lines

Quoted 9 lines
Quoted 1 line
FLASH: CFI: Unknown command set 0x0
The command has not been sent but MSB which was 0x00.
Quoted 1 line
## Unknown FLASH on Bank 1 - Size = 0x00000000 = 0 MB
That's the message I got before my patch. There is probably still something wrong.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Please explain what exactly you need to fix, perhaps with an example. The current version breaks other boards.
I attached a file which can be used for testing. On your little endian machine you should have __LITTLE_ENDIAN
Little endian? Why should I have this defined on my PPC system. It's not.

nevermid...

Quoted 8 lines

Quoted 3 lines
defined and your result should
be 0xf0 in the case i = 0. Right now, I get 0x00 on my BE machine while LE case is working fine.
I have to admit that I don't have the time to test anything right now. It seems that you have a problem with the #else path in flash_make_cmd(). Do you have access to a big endian platform which doesn't define CFG_WRITE_SWAPPED_DATA? If yes, you could test both cases.

No I don't, but I have an idea how I could test this assuming that it has work after the patch in question and didn't after mine.

Quoted 1 line

Stefan

Sebastian

Stefan Roese

author

2008-07-15 12:57 UTC #5 lore raw

PATCH 1/1 cfi_flash: fix flash on Big Endian machines.

Quoted 4 lines · Sebastian Siewior

Quoted 1 line · * Stefan Roese | 2008-07-15 11:48:11 [+0200]:
OK, I'll await a separate patch for this.
Could be in your inbox allready :)

Yes. :)

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
And to you main patch:
NAK. After applying this, CFI support on kilauea (PPC405EX with 1* Spansion S29GL512N, 16bit wide) breaks:

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Is this big endian + CFG_WRITE_SWAPPED_DATA? Did it work before the patch?
This is big endian and CFG_WRITE_SWAPPED_DATA *not* defined. You can check this yourself, the port is officially available. And yes, of course it worked before the patch.
So I might have an idea where the bug is.

Good.

<snip>

Quoted 11 lines

Quoted 8 lines
Quoted 3 lines
defined and your result should
be 0xf0 in the case i = 0. Right now, I get 0x00 on my BE machine while LE case is working fine.
I have to admit that I don't have the time to test anything right now. It seems that you have a problem with the #else path in flash_make_cmd(). Do you have access to a big endian platform which doesn't define CFG_WRITE_SWAPPED_DATA? If yes, you could test both cases.
No I don't, but I have an idea how I could test this assuming that it has work after the patch in question and didn't after mine.

Yes, it works on those kind of platforms (CFG_WRITE_SWAPPED_DATA not defined) on current top of git repository. And fails with your patch applied.

Best regards,
Stefan

Sebastian Siewior

2008-07-15 21:12 UTC #6 lore raw

PATCH 1/1 cfi_flash: fix flash on BE machines with CFG_WRITE_SWAPPED_DATA

This got broken by commits 93c56f212c
[cfi_flash: support of long cmd in U-boot.]

That command needs to be in little endian format on BE machines with CFG_WRITE_SWAPPED_DATA. Without this patch, the command 0xf0 gets saved on stack as 0x00 00 00 f0 and 0x00 gets written into the cmdbuf in case portwidth = chipwidth = 8

Cc Alexey Korolev <[email protected]>

Cc Vasiliy Leonenko <[email protected]>

Signed-off-by Sebastian Siewior <[email protected]>

---
Stefan, this is on top of my previous patch. I don't touch the BE + !CFG_WRITE_SWAPPED_DATA because it worked before. What I'm not 100% sure is whether LE + CFG_WRITE_SWAPPED_DATA still works....

 drivers/mtd/cfi_flash.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

drivers/mtd/cfi_flash.c +6 -1

index 4340b1b..a85e0ff 100644
@@ -306,16 +306,21 @@ static void flash_make_cmd(flash_info_t *info, u32 cmd, void *cmdbuf)
 	int i;
 	int cword_offset;
 	int cp_offset;
+	int cmd_le;
 	uchar val;
 	uchar *cp = (uchar *) cmdbuf;
 
+	cmd_le = cpu_to_le32(cmd);
 	for (i = info->portwidth; i > 0; i--){
 		cword_offset = (info->portwidth-i)%info->chipwidth;
 #if defined(__LITTLE_ENDIAN) || defined(CFG_WRITE_SWAPPED_DATA)
 		cp_offset = info->portwidth - i;
-		val = *((uchar*)&cmd + cword_offset);
+		val = *((uchar*)&cmd_le + cword_offset);
 #else
 		cp_offset = i - 1;
+		/* we rely here on the fact, that cmd _has_ to be in BE
+		 * order because we are not __LITTLE_ENDIAN
+		 */
 		val = *((uchar*)&cmd + sizeof(u32) - cword_offset - 1);
 #endif
 		cp[cp_offset] = (cword_offset >= sizeof(u32)) ? 0x00 : val;

Signature · 1 line

1.5.5.2

Wolfgang Denk

2008-07-15 22:05 UTC #7 lore raw

PATCH 1/1 cfi_flash: fix flash on BE machines with CFG_WRITE_SWAPPED_DATA

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

That command needs to be in little endian format on BE machines with CFG_WRITE_SWAPPED_DATA. Without this patch, the command 0xf0 gets saved on stack as 0x00 00 00 f0 and 0x00 gets written into

On stack?

drivers/mtd/cfi_flash.c static void flash_make_cmd(flash_info_t *info, u32 cmd, void *cmdbuf) · line 313

Quoted 9 lines

drivers/mtd/cfi_flash.c +2 -0

@@ -306,16 +306,21 @@ static void flash_make_cmd(flash_info_t *info, u32 cmd, void *cmdbuf)
 	int i;
 	int cword_offset;
 	int cp_offset;
+	int cmd_le;
 	uchar val;
 	uchar *cp = (uchar *) cmdbuf;
 
+	cmd_le = cpu_to_le32(cmd);

This bloats code size for cases where it is not needed.

You seem to be focussing one one specific system configuration only, but there are 4 possible situations that need to be handled:

- BE without CFG_WRITE_SWAPPED_DATA
- BE with CFG_WRITE_SWAPPED_DATA
- LE without CFG_WRITE_SWAPPED_DATA
- LE with CFG_WRITE_SWAPPED_DATA

I don't think you handle all for of them correctly.

drivers/mtd/cfi_flash.c static void flash_make_cmd(flash_info_t *info, u32 cmd, void *cmdbuf) · line 324

Quoted 12 lines

drivers/mtd/cfi_flash.c +4 -1

 	for (i = info->portwidth; i > 0; i--){
 		cword_offset = (info->portwidth-i)%info->chipwidth;
 #if defined(__LITTLE_ENDIAN) || defined(CFG_WRITE_SWAPPED_DATA)
 		cp_offset = info->portwidth - i;
-		val = *((uchar*)&cmd + cword_offset);
+		val = *((uchar*)&cmd_le + cword_offset);
 #else
 		cp_offset = i - 1;
+		/* we rely here on the fact, that cmd _has_ to be in BE
+		 * order because we are not __LITTLE_ENDIAN
+		 */
 		val = *((uchar*)&cmd + sizeof(u32) - cword_offset - 1);

Hm... we are BE, because we are not LE.

Sounds kind of obvious to me?

[Nitpick: incorrect multiline comment style]

Best regards,

Wolfgang Denk

Signature · 5 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"To IBM, 'open' means there is a modicum  of  interoperability  among
some of their equipment."                            - Harv Masterson

Sebastian Siewior

2008-07-16 08:04 UTC #8 lore raw

PATCH 1/1 cfi_flash: fix flash on BE machines with CFG_WRITE_SWAPPED_DATA

Quoted 7 lines · * Wolfgang Denk | 2008-07-16 00:05:20 [+0200]:

Quoted 4 lines · In message <[email protected]> you wrote:
That command needs to be in little endian format on BE machines with CFG_WRITE_SWAPPED_DATA. Without this patch, the command 0xf0 gets saved on stack as 0x00 00 00 f0 and 0x00 gets written into
On stack?

Yes. We refer to the command via &cmd and therefore it will be saved on the stack. Did I miss phrase myself or is this about something else?

Quoted 11 lines

drivers/mtd/cfi_flash.c static void flash_make_cmd(flash_info_t *info, u32 cmd, void *cmdbuf) · line 313
Quoted 9 lines
drivers/mtd/cfi_flash.c +2 -0
@@ -306,16 +306,21 @@ static void flash_make_cmd(flash_info_t *info, u32 cmd, void *cmdbuf)
 	int i;
 	int cword_offset;
 	int cp_offset;
+	int cmd_le;
 	uchar val;
 	uchar *cp = (uchar *) cmdbuf;
 
+	cmd_le = cpu_to_le32(cmd);
This bloats code size for cases where it is not needed.

I assumed this will be optimized away in the non needed-case and I tried not use that much of ifdefs.

Quoted 4 lines

You seem to be focussing one one specific system configuration only, but there are 4 possible situations that need to be handled:
- BE without CFG_WRITE_SWAPPED_DATA

That is Stefan's box, code worked before, remained untouched should still work.

Quoted 1 line

- BE with CFG_WRITE_SWAPPED_DATA

That is mine box, works after this patch.

Quoted 1 line

- LE without CFG_WRITE_SWAPPED_DATA

In that case, cpu_to_le32 is a null-macro, the code is the same like before my patch, should work.

Quoted 1 line

- LE with CFG_WRITE_SWAPPED_DATA

That one could be broken. Hmmm. Okay, I take the old code (before expanding char -> long) take a look how the commands are expanded, compare with what I have now and let you know.

Quoted 17 lines

I don't think you handle all for of them correctly.

drivers/mtd/cfi_flash.c static void flash_make_cmd(flash_info_t *info, u32 cmd, void *cmdbuf) · line 324

Quoted 12 lines

drivers/mtd/cfi_flash.c +4 -1

 	for (i = info->portwidth; i > 0; i--){
 		cword_offset = (info->portwidth-i)%info->chipwidth;
 #if defined(__LITTLE_ENDIAN) || defined(CFG_WRITE_SWAPPED_DATA)
 		cp_offset = info->portwidth - i;
-		val = *((uchar*)&cmd + cword_offset);
+		val = *((uchar*)&cmd_le + cword_offset);
 #else
 		cp_offset = i - 1;
+		/* we rely here on the fact, that cmd _has_ to be in BE
+		 * order because we are not __LITTLE_ENDIAN
+		 */
 		val = *((uchar*)&cmd + sizeof(u32) - cword_offset - 1);

Hm... we are BE, because we are not LE.

Quoted 1 line

Sounds kind of obvious to me?

Technically we still could run on PDP :)

Quoted 1 line

[Nitpick: incorrect multiline comment style]

Ack, but since this comment is obvious I get rid of it anyway.

Quoted 1 line

Wolfgang Denk

Sebastian

Sebastian Siewior

2008-07-16 18:04 UTC #9 lore raw

PATCH v2 1/1 cfi_flash: fix flash on BE machines with CFG_WRITE_SWAPPED_DATA

In reply to Wolfgang Denk

This got broken by commits 93c56f212c
[cfi_flash: support of long cmd in U-boot.]

Cc Alexey Korolev <[email protected]>

Cc Vasiliy Leonenko <[email protected]>

Signed-off-by Sebastian Siewior <[email protected]>

---
I verified the results with
http://download.breakpoint.cc/cfi-uboout-test.c
and I get equal results with old / new method in four categories
- BE & swapped data
- BE & data now swapped
- LE & swapped data
- LE & data now swapped
on x86 & powerpc

 drivers/mtd/cfi_flash.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

drivers/mtd/cfi_flash.c +4 -1

index 4340b1b..12647ef 100644
@@ -306,6 +306,9 @@ static void flash_make_cmd(flash_info_t *info, u32 cmd, void *cmdbuf)
 	int i;
 	int cword_offset;
 	int cp_offset;
+#if defined(__LITTLE_ENDIAN) || defined(CFG_WRITE_SWAPPED_DATA)
+	u32 cmd_le = cpu_to_le32(cmd);
+#endif
 	uchar val;
 	uchar *cp = (uchar *) cmdbuf;

@@ -313,7 +316,7 @@ static void flash_make_cmd(flash_info_t *info, u32 cmd, void *cmdbuf)
 		cword_offset = (info->portwidth-i)%info->chipwidth;
 #if defined(__LITTLE_ENDIAN) || defined(CFG_WRITE_SWAPPED_DATA)
 		cp_offset = info->portwidth - i;
-		val = *((uchar*)&cmd + cword_offset);
+		val = *((uchar*)&cmd_le + cword_offset);
 #else
 		cp_offset = i - 1;
 		val = *((uchar*)&cmd + sizeof(u32) - cword_offset - 1);

Signature · 1 line

1.5.5.2

Stefan Roese

author

2008-07-17 09:46 UTC #10 lore raw

PATCH v2 1/1 cfi_flash: fix flash on BE machines with CFG_WRITE_SWAPPED_DATA

Quoted 11 lines · Sebastian Siewior

This got broken by commits 93c56f212c
[cfi_flash: support of long cmd in U-boot.]
That command needs to be in little endian format on BE machines with CFG_WRITE_SWAPPED_DATA. Without this patch, the command 0xf0 gets saved on stack as 0x00 00 00 f0 and 0x00 gets written into the cmdbuf in case portwidth = chipwidth = 8bit.
Cc Alexey Korolev <[email protected]>
Cc Vasiliy Leonenko <[email protected]>
Signed-off-by Sebastian Siewior <[email protected]>

Quoted 9 lines

---
I verified the results with
http://download.breakpoint.cc/cfi-uboout-test.c
and I get equal results with old / new method in four categories
- BE & swapped data
- BE & data now swapped
- LE & swapped data
- LE & data now swapped
on x86 & powerpc

Now this looks better and actually works again on my BE platform.

Applied to u-boot-cfi-flash. Thanks.

Best regards,
Stefan

Vasiliy Leoenenko

2008-07-17 10:41 UTC #11 lore raw

PATCH v2 1/1 cfi_flash: fix flash on BE machineswith CFG_WRITE_SWAPPED_DATA

Hi,

I checked last patch which was sent by Sebastian in this thread with u-boot 1.3.4-rc1 (it includes another patch made by Sebastian: "cfi_flash: make the command u32 only"). I used Mainstone II platform (arm-pxa270) with Intel M18 flash (16x16). It works fine.

Best regards,
Vasiliy