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