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