Hi Michael,
On Tuesday 13 November 2007, Michael Schwingen wrote:
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,
Me too. But we should definitely wait till after the 1.3.0 release, which is hopefully going to happen in the next few days (Stefan waves to Grant ;)). Then this patch should go in very quickly, at least from my point of view, since it will enable me to drop a lot of board specific flash drivers.
and I have no way to test the change on a large number of boards.
I do have some here. And I will test it on some platforms.
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?
Kind of. The subsystem maintainer (custodian) decides if and when a patch gets applied. This is of course after official review on the mailing list. As it seems I am the custodian for the CFI driver right now (Tolunay didn't have time anymore), so if nobody objects and the tests work fine, your patch will get pulled into the CFI custodian repo pretty soon. In the next merge window. We are trying to follow here the Linux model.
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.
I vote for an incremental patch here too.
@@ -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.
Please change it to the Linux version.
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@denx.de =====================================================================