From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Schwingen Date: Tue, 13 Nov 2007 19:59:12 +0100 Subject: [U-Boot-Users] PATCH: support JEDEC flash roms in CFI-flash framework In-Reply-To: <4739C21C.1090107@semihalf.com> References: <20071112202311.GB29521@discworld.dascon.de> <4739C21C.1090107@semihalf.com> Message-ID: <20071113185911.GA21337@discworld.dascon.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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.