Re: [U-Boot-Users] [PATCH] cfi_flash: fix flash on Big Endian machines.
Hi Sebastian,
On Monday 14 July 2008, Sebastian Siewior wrote:
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.
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@denx.de =====================================================================
* Stefan Roese | 2008-07-15 10:46:33 [+0200]:
Hi Sebastian,
Hi Stefan,
On Monday 14 July 2008, Sebastian Siewior wrote:
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.
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.
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?
FLASH: CFI: Unknown command set 0x0
The command has not been sent but MSB which was 0x00.
## Unknown FLASH on Bank 1 - Size = 0x00000000 = 0 MB
That's the message I got before my patch. There is probably still something wrong.
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
On Tuesday 15 July 2008, Sebastian Siewior wrote:
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.
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.
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.
FLASH: CFI: Unknown command set 0x0
The command has not been sent but MSB which was 0x00.
## Unknown FLASH on Bank 1 - Size = 0x00000000 = 0 MB
That's the message I got before my patch. There is probably still something wrong.
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.
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
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
* Stefan Roese | 2008-07-15 11:48:11 [+0200]:
OK, I'll await a separate patch for this.
Could be in your inbox allready :)
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.
FLASH: CFI: Unknown command set 0x0
The command has not been sent but MSB which was 0x00.
## Unknown FLASH on Bank 1 - Size = 0x00000000 = 0 MB
That's the message I got before my patch. There is probably still something wrong.
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...
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.
Stefan
Sebastian
On Tuesday 15 July 2008, Sebastian Siewior wrote:
- 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. :)
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.
Good.
<snip>
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
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
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 akorolev@infradead.org Cc: Vasiliy Leonenko vasiliy.leonenko@mail.ru Signed-off-by: Sebastian Siewior bigeasy@linutronix.de --- 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(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index 4340b1b..a85e0ff 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -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;
In message 20080715211240.GA16786@www.tglx.de 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?
@@ -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.
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*/
Hm... we are BE, because we are not LE.
Sounds kind of obvious to me?
[Nitpick: incorrect multiline comment style]
Best regards,
Wolfgang Denk
* Wolfgang Denk | 2008-07-16 00:05:20 [+0200]:
In message 20080715211240.GA16786@www.tglx.de 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?
@@ -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.
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.
- BE with CFG_WRITE_SWAPPED_DATA
That is mine box, works after this patch.
- 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.
- 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.
I don't think you handle all for of them correctly.
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*/Hm... we are BE, because we are not LE.
Sounds kind of obvious to me?
Technically we still could run on PDP :)
[Nitpick: incorrect multiline comment style]
Ack, but since this comment is obvious I get rid of it anyway.
Wolfgang Denk
Sebastian
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 akorolev@infradead.org Cc: Vasiliy Leonenko vasiliy.leonenko@mail.ru Signed-off-by: Sebastian Siewior bigeasy@linutronix.de --- 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(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index 4340b1b..12647ef 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -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);
On Wednesday 16 July 2008, Sebastian Siewior wrote:
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 akorolev@infradead.org Cc: Vasiliy Leonenko vasiliy.leonenko@mail.ru Signed-off-by: Sebastian Siewior bigeasy@linutronix.de
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
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
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
participants (4)
-
Sebastian Siewior -
Stefan Roese -
Vasiliy Leoenenko -
Wolfgang Denk