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.