[flashrom] [RFC] JEDEC refactor w/ conversion notes and file eliminations

Sean Nelson audiohacked at gmail.com
Fri Dec 25 22:48:09 CET 2009


On 12/25/09 11:23 AM, Carl-Daniel Hailfinger wrote:
> On 25.12.2009 07:03, Sean Nelson wrote:
>    
>> This patch demonstrates how we could refactor the jedec code. After
>> the refactor
>> we can delete these:
>>   * am29f040b
>>   * en29f002a ->  file_not_used
>>   * m29f002
>>   * mx29f002
>>   * pm29f002
>>   * sst49lf040
>>   * w49f002u
>>      
> Great.
> What does the file_not_used mean? Was it never hooked up at all? In that
> case, it would be cool if we could go through each of the functions and
> check if we have to add a corresponding flashchip entry for it.
>
>
>    
file not used is a file that is not hooked up anywhere in the source 
code.  What I could figure out is that the en29f002a functions are 
nearly exactly the same as the am29f040b. And the flashchip entries are 
already using the _29f040b functions.
>> These files can be deleted if we *could* find a way to integrate the
>> problems
>> into jedec.c:
>>   * w29ee011 ->  checks specifically for w29ee011
>>      
> I think that was due to the probe sequence being non-standard, and that
> probe sequence could mess up the internal state of another chip
> (apparently not resettable).
>
>
>    
As it stands, we still need/use w29ee011 for that very check.
>>   * w39v040c ->  checks for lock in probe: address 0xfff2
>>      
> Will be solved once I update my locking infrastructure patch which
> splits lock printing/disabling/enabling away from probe.
>
>
>    
>>   * pm49fl00x ->  uses chip protect code
>>      
> Will be solved by the locking infrastructure.
>
>
>    
>>   * m29f002 ->  block based writing
>>   * m29f400bt ->  block based writing
>>      
> I need to finish my eraseblock based writing patch which will not only
> remove all chip specific code, it will also allow us to perform partial
> writes.
> The good news is that I don't know any chips which need different write
> functions for different chip areas, so a simple writelen/writefunc
> function pair in struct flashchip will address all partial write needs.
> Can send a prototype patch if you want.
>
>
>    
>> I was looking through these two files and saw that they used variable
>> block
>> sizes for writing sectors, maybe we can do something simpler to
>> block_erasers:
>>   * m29f002
>>   * m29f400bt
>>      
> Yes, eraseblock based writing will solve this.
>
>
>    
>> There are a few files that performs another map_flash_registers() after
>> successful probe, I was wondering if we could add the re-mapping to
>> probe_jedec_common or if its safe to omit the function call:
>>   * pm49fl00x
>>   * stm50flw0x0x
>>   * w39v080fa
>>      
> Mapping flash registers or not is something that should end up outside
> probe, probably being controlled by a flag in a to-be-created bitfield
> in struct flashchip. For now, I think a pretty good heuristic is that
> LPC/FWH flahs has registers, and Parallel/SPI doesn't because Parallel
> doesn't have enough address lines for this and SPI has opcodes which
> take care of this register stuff.
>
>
>    
In struct flashchips, I've added "int remap" for this very reason.
>> List of chips that use a specific addressing for command codes:
>> 0x2AA based chips:
>>   * am29f040b
>>   * mx29f002
>>   * pm29f002
>>
>> 0xAAA based chips:
>>   * m29f002
>>   * m29f400bt
>>
>> 0x2AAA based chips:
>>   * pm49fl00x
>>   * sst49lf040
>>   * stm50flw0x0x
>>   * w29ee011
>>   * w39v040c
>>   * w39v080fa
>>   * w49f002u
>>
>> If you want I can send my full notes so you can see what each file's
>> functions
>> can be converted to, on request.
>>      
> The notes you sent are already awesome, but I'm realling thinking we
> should have all this on the list (or in the wiki) for future
> generations. We never know when someone else will come along and take
> over flashrom development, and that person should have all the
> information we had.
> IMHO first priority is getting your patch merged, though.
>
>    
>> I don't know if my methods for the address mask
>> is proper. If you can think of a better way, I'm all ears.
>>      
> I think the (0x5555&  mask) is a good idea (would have done exactly the
> same thing). Given that the shift stuff is only needed for a specific
> subset of chips and for those chips only in a subset of configurations
> (AFAIK it only affects chips with 16-bit access granularity which are
> strapped to give 8-bit output) I would really like to leave out the
> shift parameter for now. Without shift, the code is IMHO more readable
> and the conversion is easier to review. We still can hook up shifting
> later (and such a single-purpose patch will be a lot easier to review as
> well).
>
>    
Everything is now using mask-only.
> Once you commit, please use the long contents of your mail as changelog.
> It is very enlightening and I want to keep it around in case someone
> besides me digs through code history.
>
> On to the comments.
>
> IMHO the _common stuff should not be in any header. It is internal to
> jedec.c. To get this to work, we may have to reorder some functions
> inside jedec.c. If these declarations help you right now, we can still
> reorder those functions (and remove the declarations) in a followup patch.
>
>
>    
I've removed the common stuff from the header except for 
erase_sector_jedec_common since at the moment I'd like to keep the other 
files intact and its used in the other files.
> I don't see a replacement for write_jedec_1. Is there something I
> missed? Some chips need the byte program sequence before each
> single-byte write.
>    

One way of solving and replacing write_jedec_1 is to add a 
byte_based_write field to struct flashchips
> Can you replace chipaddr bios with struct flashchip *flash in the
> function signature?
>
>    
Done.
> I think entry_cmd is taking the refactoring a bit too far. If the
> sequence is non-standard, it shoudln't be called foo_jedec.
>
>    
I also agree. Fixed.
>    
>> +			unsigned int mid_cmd, unsigned int did_cmd,
>>
>>      
> mid_addr, did_addr please.
>
>
>    
Fixed.

>> +			unsigned int mask, unsigned int shift, int reset)
>>
>>      
> I am unconvinced that shift is the right approach right now, I'd like to
> operate mask-only if possible.
>
>    
Done. Using masks: (0x5555 & mask) and (0x2AAA & mask). To get 0x2AA, we 
use mask 0x7FF; same to get 0x555.

> Hmm. Can you rename reset to long_reset? Both variants (AA 55 F0 and F0)
> are reset sequences.
>
>
>    
Done. Also using : "if (long_reset)"

> Either make it conditional on the flash bus (as suggested above) or add
> some flag to a bitfield in struct flashchip.
>
>    
Conditional on "flash->remap".

> Isn't write_helper_{page, byte} taking the refactoring a bit too far?
>
>    
Agreed, Removed.
> Why is verify_range commented out here?
>
>    
I couldn't figure out how to get "int start" back for it, I think I 
fixed it. See the patch.
>> +}
>>
>> -	printf("Programming page: ");
>> -	for (i = 0; i<  flash->total_size; i++) {
>> -		if ((i&  0x3) == 0)
>> -			printf("address: 0x%08lx", (unsigned long)i * 1024);
>> +void start_program_jedec_aaa(chipaddr bios)
>>
>>      
> struct flashchip *flash instead of chipaddr bios please.
>
>    
>> +{
>> +	start_program_jedec_common(bios, 0xf000, 0);
>>
>>      
> mask should be 0xfff or 0x1fff.
>
>    
For now I'm ignoring the _aaa functions since its used for m29lf400bt
>
> I am unconvinced unless you also fix up page_size (and to fix page_size,
> you have to convert all chips to struct eraseblock because some chips
> use page_size as eraseblock size.
>
>    
After this patch is committed, I'll continue coverting all chips to 
struct eraseblock.

Since the chips that used the _aaa functions were byte based sequences 
I'm going to ignore them and leave the associated file alone.


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: jedec3.diff
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20091225/011ad29c/attachment.ksh>


More information about the flashrom mailing list