[U-Boot-Users] [PATCH] cfi_flash: fix flash on Big Endian machines.

[U-Boot-Users] [PATCH] cfi_flash: fix flash on Big Endian machines.

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot-Users] [PATCH] cfi_flash: fix flash on Big Endian machines.

[Sebastian Siewior]

* 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
-------------- 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;
}

[U-Boot-Users] [PATCH] cfi_flash: fix flash on Big Endian machines.

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot-Users] [PATCH] cfi_flash: fix flash on Big Endian machines.

[Sebastian Siewior]

* 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

[U-Boot-Users] [PATCH] cfi_flash: fix flash on Big Endian machines.

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot-Users] [PATCH] cfi_flash: fix flash on BE machines with CFG_WRITE_SWAPPED_DATA

[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 = 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;
-- 
1.5.5.2

[U-Boot-Users] [PATCH] cfi_flash: fix flash on BE machines with CFG_WRITE_SWAPPED_DATA

[Wolfgang Denk]

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

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

[U-Boot-Users] [PATCH] cfi_flash: fix flash on BE machines with CFG_WRITE_SWAPPED_DATA

[Sebastian Siewior]

* 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
>> +		 */
>>  		val = *((uchar*)&cmd + sizeof(u32) - cword_offset - 1);
>
>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

[U-Boot-Users] [PATCH v2] cfi_flash: fix flash on BE machines with CFG_WRITE_SWAPPED_DATA

[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 <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);
-- 
1.5.5.2

[U-Boot-Users] [PATCH v2] cfi_flash: fix flash on BE machines with CFG_WRITE_SWAPPED_DATA

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot-Users] [PATCH v2] cfi_flash: fix flash on BE machineswith CFG_WRITE_SWAPPED_DATA

[Vasiliy Leoenenko]

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