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

[Wolfgang Denk]

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?

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