[U-Boot-Users] PATCH: support JEDEC flash roms in CFI-flash framework

[Michael Schwingen]

On Tue, Nov 13, 2007 at 04:26:20PM +0100, Bartlomiej Sieka wrote:
> 
> Your patch fixes an issue with AMD_ADDR_* definitions for CFI flashes,
> along with its primary intent (JEDEC support in CFI framework). I think
> it would be better to submit the fix to AMD_ADDR_* as a separate patch.

Um - no. I removed that when changing to the unlock_addr* variables in the
flash_info_t struct, for just that reason - I wanted the patch to be applied
soon, and I have no way to test the change on a large number of boards.

Unless I made some error in the conversion, the CFI code should behave
exactly the same with and without my patch. Only the Jedec code uses
different unlock_addr values.

> It's more logical this way, also, it might get committed sooner, as it
> likely fixes a problem with an existing board. I am willing to test such
> a patch on one of the troublesome boards.

Actually, there are two points where I think the current code is wrong, and
which I did not change, because those are probably best handled in separate
patches:
 - unlock_addr values when running on 8-bit CFI flashs (interface == 0)
 - the AMD erase code, where the unlock sequence is written to the sector
   base address instead of the chip base address.

I am not sure what the policy is regarding changes that might break
existing boards? Are those patches applied if enough people are sure they
should be safe?

> Perhaps it would be better to follow what is being done in the Linux
> driver, adjusted to U-Boot context. I.e., something along the lines of
> (untested):
> 
> info->unlock_addr1 = 0x555;
> info->unlock_adde2 = 0x2aa;
> 
> /* Modify the unlock address if we are in compatibility mode */
> if (	/* x16 in x8 mode */
> 	((info->chipwidth == FLASH_CFI_BY8) &&
> 		(info->interface == 2)) ||
> 	/* x32 in x16 mode */
> 	((info->chipwidth == FLASH_CFI_BY16) &&
> 		(info->interface == 4)))
> {
> 	info->unlock_addr1 = 0xaaa;
> 	info->unlock_addr2 = 0x555;
> }

Agreed. I had this in an earlier version of my patch, where I needed to
modify the AMD_ADDR_* macros, but removed it later in order to make minimal
changes to the existing CFI behaviour. I think this should be added on top
of my patch, unless everyone on this list agrees that it should go in
immediately.

> > @@ -52,6 +52,9 @@ typedef struct {
> >  	ushort	ext_addr;		/* extended query table address		*/
> >  	ushort	cfi_version;		/* cfi version				*/
> >  	ushort	cfi_offset;		/* offset for cfi query 		*/
> > +	ulong   unlock_addr1;		/* unlock address 1 for AMD flash roms  */
> > +	ulong   unlock_addr2;		/* unlock address 2 for AMD flash roms  */
> 
> Linux driver uses addr_unlock1 and addr_unlock2 for this purpose, maybe 
> it's a good idea to keep the variable names in sync with Linux?

Not sure - the CFI code does not look very similar to the Linux code, so I
see no big benefit in doing so, but as it is just a name, I can live with
both variants.

cu
Michael
-- 
Some people have no respect of age unless it is bottled.