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
These files can be deleted if we *could* find a way to integrate the problems into jedec.c: * w29ee011 -> checks specifically for w29ee011 * w39v040c -> checks for lock in probe: address 0xfff2 * pm49fl00x -> uses chip protect code * m29f002 -> block based writing * m29f400bt -> block based writing
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
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
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. 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. Yes, I did work on this all Christmas Eve.
Am Donnerstag, den 24.12.2009, 22:03 -0800 schrieb Sean Nelson:
I cross-checked data sheets for chips where flashrom uses a reduced number of address bits (non-reduced being 0x5555/0x2AAA).
Regards, Michael Karcher
List of chips that use a specific addressing for command codes: 0x2AA based chips:
(that means 11 address bits, A0..A10 bits used)
- am29f040b
- mx29f002
- pm29f002
11 bits maybe needed: ===================== AMD Am29F010B: http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/am29... Datasheet does not tell anything about ignored address lines in commands.
PMC PM29F002B/PM29F002T: http://www.datasheetarchive.com/pdf-datasheets/Datasheets-26/DSA-508503.pdf Datasheet does not tell anything about ignored address lines.
AMIC A29002B/AM29002T: [Listed here because 0xAAA would have A11 set which is not marked as don't care. Currently, it uses write_jedec_1 which will fail if the datasheet is correct about A11 being used.] http://www.amictechnology.com/pdf/A29002.pdf Page 11, Note 4 "Address bits A17 - A12 are don't cares for unlock and command cycles, unless SA or PA required."
Macronix MX29LV040: [Currently uses write_jedec_1 which will fail if the higher address lines are not ignored.] http://www.datasheetcatalog.org/datasheet/macronix/MX29LV040TI-70.pdf Datasheet does not tell anything about ignored address lines.
more bits OK according to data sheet: ===================================== AMD Am29F016D: http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/2144... Page 21, Note 4 "Address bits A20-A11 are don't cares for unlock and command cycles, unless SA or PA required", SA and PA meaning "sector address" and "programmed address"
AMD Am29F040B: http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/2144... Page 14, Note 4 "Address bits A18-A11 are don't cares for unlock and command cycles, unless SA or PA required"
AMD Am29F080B: http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/2150... Page 18, Note 8 "Unless otherwise noted, address bits A19-A11 are don't care."
AMD Am29LV040B: http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/2135... Page 17, Note 4 "Address bits A18-A11 are don't cares for unlock and command cycles."
AMD Am29LV081B: http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/2152... Page 17, Note 4 "All address bits are don't cares for unlock and command cycles, except where SA or PA required."
AMIC A29040B: http://www.amictechnology.com/pdf/A29040B.pdf Page 14, Note 4 "Address bits A18-A11 are don't cares for unlock and command cycles, unless SA or PA required"
Macronix MX29F001T/B http://www.datasheetcatalog.org/datasheet/macronix/MX29F001TQC-90.pdf Page 5, Note 3 "The system should generate the following address patterns: 555H or 2AAH to Address A0-A10. Address bit A11-A17 = X = Don't care for all address commands except for Programm Address (PA) and Sector Address (SA). Write Sequence may be initiated with A11-A17 in either state."
Macronix MX29F002T/B http://pdf.dzsc.net.cn/20090603/200903281341143638.pdf Page 5, Note 3 "The system should generate the following address patterns: 555H or 2AAH to Address A0-A10. Address bit A11-A17 = X = Don't care for all address commands except for Programm Address (PA) and Sector Address (SA). Write Sequence may be initiated with A11-A17 in either state."
ST M29F040B: http://www.datasheetcatalog.org/datasheet/SGSThomsonMicroelectronics/mXyxzsz... Page 7: "The command interface only uses the address bits A0-A10 to verify the commands, the upper address bits are Don't Care."
Am29F010A/B: http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/am29... Datasheet does not tell anything about ignored address lines in commands. PM29F0002L/PM29F0002B - Chip name contains a type, it should be PM29F002L/PM29F002B http://www.alldatasheet.co.kr/datasheet-pdf/pdf_kor/118671/PMC/PM29F002B-55J... Datasheet does not tell anything about ignored address lines.
0xAAA based chips:
(i.e. Address bits up to A0-A11 used)
- m29f002
more bits OK according to data sheet: ===================================== ST M29F002B/M29F002NT/M29F002T: http://www.datasheetcatalog.org/datasheet/stmicroelectronics/5166.pdf Page 8, Note 7: "For Coded cycles address inputs A12-A17 are don't care."
- m29f400bt
this Chip is not standard 0xAAA based at all. Please carefully read the comment in the file.
On 12/25/09 9:09 AM, Michael Karcher wrote:
- m29f400bt
this Chip is not standard 0xAAA based at all. Please carefully read the comment in the file.
my full notes for m29f400bt: @m29f400bt { # 0x555 0xAAA block_based_write block_erase_chip_m29f400bt => erase_chip_block_jedec_aaa block_erase_m29f400bt => erase_sector_jedec_aaa erase_m29f400bt => erase_chip_jedec_aaa probe_m29f400bt => probe_jedec_aaa_did_02 write_coreboot_m29f400bt # same as previous write_m29f400bt # variable page writing write_page_m29f400bt }
I did a direct compare of the functions in jedec.c and m29f400bt.c and found the above to be equivalent. We can totally eliminate the file if we create a block_writers system.
Am Freitag, den 25.12.2009, 18:09 +0100 schrieb Michael Karcher:
11 bits maybe needed:
[...]
AMIC A29002B/AM29002T: [Listed here because 0xAAA would have A11 set which is not marked as don't care. Currently, it uses write_jedec_1 which will fail if the datasheet is correct about A11 being used.] http://www.amictechnology.com/pdf/A29002.pdf Page 11, Note 4 "Address bits A17 - A12 are don't cares for unlock and command cycles, unless SA or PA required."
As carldani told in IRC, there is a success report with the A29002B. This test used a 11-bit erase function (0x555/0x2AA) and a 15 bit probe and a 15 bit write function (0x5555/0x2AAA). As it worked, this chip is for sure 11-bits-needed, higher-bits-don't care.
Macronix MX29LV040: [Currently uses write_jedec_1 which will fail if the higher address lines are not ignored.] http://www.datasheetcatalog.org/datasheet/macronix/MX29LV040TI-70.pdf Datasheet does not tell anything about ignored address lines.
This chip also uses probe_29f002 (15 bits)/erase_29f002 (11 bits). Currently it is declared as probe/read successful; erase/write never was marked successful in svn since the chip got introduced in r366. As the data sheet specifies the 11-bit addresses and the 15-bit probe worked, it is very probable that all address bits starting at A11 are (as usual for 11 bit chips) don't care.
Regards, Michael Karcher
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.
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).
- 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.
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).
Yes, I did work on this all Christmas Eve.
Wow, thank you so much! I hope your family doesn't hate flashrom now. Please with them a Merry Christmas and submit my apologies to them.
I do have a few comments, and I hope you won't give up in despair after reading them. If you feel burnt out or frustrated with this, tell me and I'll do the changes myself and submit them for your consideration.
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.
diff --git a/chipdrivers.h b/chipdrivers.h index adcb46d..f722133 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -76,18 +76,34 @@ int write_en29f002a(struct flashchip *flash, uint8_t *buf); uint8_t oddparity(uint8_t val); void toggle_ready_jedec(chipaddr dst); void data_polling_jedec(chipaddr dst, uint8_t data); -void start_program_jedec(chipaddr bios); -int write_byte_program_jedec(chipaddr bios, uint8_t *src,
chipaddr dst);+void start_program_jedec_common(chipaddr bios, unsigned int mask,
unsigned int shift);+int write_byte_program_jedec(chipaddr bios, uint8_t *src, chipaddr dst); +int probe_jedec_common(struct flashchip *flash, unsigned int entry_cmd,
unsigned int mid_cmd, unsigned int did_cmd,unsigned int mask, unsigned int shift, int reset);+int erase_chip_jedec_common(struct flashchip *flash, unsigned int mask,
unsigned int shift);+int write_jedec_common(struct flashchip *flash, uint8_t *buf,
unsigned int mask, unsigned int shift);+int erase_sector_jedec_common(struct flashchip *flash, unsigned int page,
unsigned int pagesize, unsigned int mask,unsigned int shift);+int erase_block_jedec_common(struct flashchip *flash, unsigned int page,
unsigned int blocksize, unsigned int mask,unsigned int shift);+int erase_chip_block_jedec(struct flashchip *flash, unsigned int page,
unsigned int blocksize);+int write_sector_jedec_common(struct flashchip *flash, uint8_t *src, chipaddr dst,
unsigned int page_size, unsigned int mask,unsigned int shift);
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.
int probe_jedec(struct flashchip *flash); +int erase_sector_jedec(struct flashchip *flash, unsigned int page,
unsigned int pagesize);int erase_chip_jedec(struct flashchip *flash); +int erase_block_jedec(struct flashchip *flash, unsigned int page,
unsigned int blocksize);
The erase_sector_jedec and erase_block_jedec changes here are whitespace only AFAICS, right?
int write_jedec(struct flashchip *flash, uint8_t *buf); -int write_jedec_1(struct flashchip *flash, uint8_t *buf);
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.
-int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int pagesize); -int erase_block_jedec(struct flashchip *flash, unsigned int page, unsigned int blocksize); -int erase_chip_block_jedec(struct flashchip *flash, unsigned int page, unsigned int blocksize); -int write_sector_jedec(chipaddr bios, uint8_t *src,
chipaddr dst, unsigned int page_size);/* m29f002.c */ int erase_m29f002(struct flashchip *flash); diff --git a/flashchips.c b/flashchips.c index 59f9139..af9039d 100644 --- a/flashchips.c +++ b/flashchips.c @@ -106,7 +106,7 @@ struct flashchip flashchips[] = { .block_erase = erase_chip_block_jedec, }, },
.write = write_jedec_1,
.write = write_jedec,@@ -137,7 +137,7 @@ struct flashchip flashchips[] = { .block_erase = erase_chip_block_jedec, }, },
.write = write_jedec_1,
.write = write_jedec,@@ -1182,7 +1182,7 @@ struct flashchip flashchips[] = { .block_erase = erase_chip_29f002, }, },
.write = write_jedec_1,
.write = write_jedec,@@ -1213,7 +1213,7 @@ struct flashchip flashchips[] = { .block_erase = erase_chip_29f002, }, },
.write = write_jedec_1,
.write = write_jedec,@@ -1574,7 +1574,7 @@ struct flashchip flashchips[] = { .block_erase = erase_chip_block_jedec, }, },
.write = write_jedec_1,
.write = write_jedec,@@ -1605,7 +1605,7 @@ struct flashchip flashchips[] = { .block_erase = erase_chip_block_jedec, }, },
.write = write_jedec_1,
.write = write_jedec,@@ -2112,7 +2112,7 @@ struct flashchip flashchips[] = { .probe = probe_29f002, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (mx29f002.c) */ .erase = erase_29f002,
.write = write_jedec_1,
.write = write_jedec,@@ -2128,7 +2128,7 @@ struct flashchip flashchips[] = { .probe = probe_29f002, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (mx29f002.c) */ .erase = erase_29f002,
.write = write_jedec_1,
.write = write_jedec,@@ -2159,7 +2159,7 @@ struct flashchip flashchips[] = { .block_erase = erase_chip_29f002, }, },
.write = write_jedec_1,
.write = write_jedec,@@ -2190,7 +2190,7 @@ struct flashchip flashchips[] = { .block_erase = erase_chip_29f002, }, },
.write = write_jedec_1,
.write = write_jedec,@@ -2206,7 +2206,7 @@ struct flashchip flashchips[] = { .probe = probe_29f002, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (mx29f002.c) */ .erase = erase_29f002,
.write = write_jedec_1,
.write = write_jedec,
Please see my write_jedec_1 comment from above.
diff --git a/jedec.c b/jedec.c index d6cad41..86d3ac8 100644 --- a/jedec.c +++ b/jedec.c @@ -87,14 +87,16 @@ void data_polling_jedec(chipaddr dst, uint8_t data) printf_debug("%s: excessive loops, i=0x%x\n", __func__, i); }
-void start_program_jedec(chipaddr bios) +void start_program_jedec_common(chipaddr bios, unsigned int mask, unsigned int shift)
Can you replace chipaddr bios with struct flashchip *flash in the function signature?
{
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0xA0, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask) );
- chip_writeb(0x55, bios + ((0x2AAA & mask)>>shift) );
- chip_writeb(0xA0, bios + (0x5555 & mask) );
}
-int probe_jedec(struct flashchip *flash) +int probe_jedec_common(struct flashchip *flash, unsigned int entry_cmd,
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.
unsigned int mid_cmd, unsigned int did_cmd,
mid_addr, did_addr please.
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.
{ chipaddr bios = flash->virtual_memory; uint8_t id1, id2; @@ -118,19 +120,19 @@ int probe_jedec(struct flashchip *flash) }
/* Issue JEDEC Product ID Entry command */
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask) ); if (probe_timing_enter) programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + ((0x2AAA & mask)>>shift) );
Inconsistent with the code above. Either mask+shift for all addresses or mask only (I prefer the latter).
if (probe_timing_enter) programmer_delay(10);
- chip_writeb(0x90, bios + 0x5555);
chip_writeb(0x90, bios + (0x5555 & mask) ); if (probe_timing_enter) programmer_delay(probe_timing_enter);
/* Read product ID */
- id1 = chip_readb(bios);
- id2 = chip_readb(bios + 0x01);
- id1 = chip_readb(bios + mid_cmd);
- id2 = chip_readb(bios + did_cmd); largeid1 = id1; largeid2 = id2;
@@ -147,13 +149,16 @@ int probe_jedec(struct flashchip *flash) }
/* Issue JEDEC Product ID Exit command */
- chip_writeb(0xAA, bios + 0x5555);
- if (probe_timing_exit)
programmer_delay(10);- chip_writeb(0x55, bios + 0x2AAA);
- if (probe_timing_exit)
programmer_delay(10);- chip_writeb(0xF0, bios + 0x5555);
- if (!reset)
Hmm. Can you rename reset to long_reset? Both variants (AA 55 F0 and F0) are reset sequences.
- {
chip_writeb(0xAA, bios + (0x5555 & mask) );if (probe_timing_exit)programmer_delay(10);chip_writeb(0x55, bios + ((0x2AAA & mask)>>shift) );if (probe_timing_exit)programmer_delay(10);- }
- chip_writeb(0xF0, bios + (0x5555 & mask) ); if (probe_timing_exit) programmer_delay(probe_timing_exit);
@@ -184,24 +189,28 @@ int probe_jedec(struct flashchip *flash) if (largeid1 == flash->manufacture_id && largeid2 == flash->model_id) return 1;
- //map_flash_registers(flash);
Either make it conditional on the flash bus (as suggested above) or add some flag to a bitfield in struct flashchip.
- return 0;
}
-int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int pagesize) +int erase_sector_jedec_common(struct flashchip *flash, unsigned int page,
unsigned int pagesize, unsigned int mask,unsigned int shift)
Kill shift.
{ chipaddr bios = flash->virtual_memory;
/* Issue the Sector Erase command */
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask) ); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + ((0x2AAA & mask)>>shift) ); programmer_delay(10);
- chip_writeb(0x80, bios + 0x5555);
- chip_writeb(0x80, bios + (0x5555 & mask) ); programmer_delay(10);
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask) ); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + ((0x2AAA & mask)>>shift) ); programmer_delay(10); chip_writeb(0x30, bios + page); programmer_delay(10);
@@ -216,21 +225,23 @@ int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int return 0; }
-int erase_block_jedec(struct flashchip *flash, unsigned int block, unsigned int blocksize) +int erase_block_jedec_common(struct flashchip *flash, unsigned int block,
unsigned int blocksize, unsigned int mask,unsigned int shift)
Kill shift.
{ chipaddr bios = flash->virtual_memory;
/* Issue the Sector Erase command */
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask) ); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + ((0x2AAA & mask)>>shift) ); programmer_delay(10);
- chip_writeb(0x80, bios + 0x5555);
- chip_writeb(0x80, bios + (0x5555 & mask) ); programmer_delay(10);
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask) ); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + ((0x2AAA & mask)>>shift) ); programmer_delay(10); chip_writeb(0x50, bios + block); programmer_delay(10);
@@ -245,35 +256,25 @@ int erase_block_jedec(struct flashchip *flash, unsigned int block, unsigned int return 0; }
-/* erase chip with block_erase() prototype */ -int erase_chip_block_jedec(struct flashchip *flash, unsigned int addr, unsigned int blocksize) -{
- if ((addr != 0) || (blocksize != flash->total_size * 1024)) {
fprintf(stderr, "%s called with incorrect arguments\n",__func__);return -1;- }
- return erase_chip_jedec(flash);
-}
-int erase_chip_jedec(struct flashchip *flash) +int erase_chip_jedec_common(struct flashchip *flash, unsigned int mask,
unsigned int shift)
Kill shift.
{ int total_size = flash->total_size * 1024; chipaddr bios = flash->virtual_memory;
/* Issue the JEDEC Chip Erase command */
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask) ); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + ((0x2AAA & mask)>>shift) ); programmer_delay(10);
- chip_writeb(0x80, bios + 0x5555);
- chip_writeb(0x80, bios + (0x5555 & mask) ); programmer_delay(10);
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask) ); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + ((0x2AAA & mask)>>shift) ); programmer_delay(10);
- chip_writeb(0x10, bios + 0x5555);
chip_writeb(0x10, bios + (0x5555 & mask) ); programmer_delay(10);
toggle_ready_jedec_slow(bios);
@@ -285,19 +286,29 @@ int erase_chip_jedec(struct flashchip *flash) return 0; }
-int write_page_write_jedec(struct flashchip *flash, uint8_t *src,
int start, int page_size)+/* erase chip with block_erase() prototype */ +int erase_chip_block_jedec(struct flashchip *flash, unsigned int addr,
unsigned int blocksize){
- int i, tried = 0, failed;
- uint8_t *s = src;
- chipaddr bios = flash->virtual_memory;
- chipaddr dst = bios + start;
- chipaddr d = dst;
- if ((addr != 0) || (blocksize != flash->total_size * 1024)) {
fprintf(stderr, "%s called with incorrect arguments\n",__func__);return -1;- }
- return erase_chip_jedec_common(flash, 0x0000, 0);
mask needs to be 0xffff.
+}
-retry:
- /* Issue JEDEC Data Unprotect comand */
- start_program_jedec(bios);
+int write_helper_byte(uint8_t *src, chipaddr dst) +{
- /* transfer data from source to destination */
- if (*src != 0xFF)
chip_writeb(*src, dst);- return 0;
+}
+int write_helper_page(uint8_t *src, chipaddr dst, int page_size)
Isn't write_helper_{page, byte} taking the refactoring a bit too far?
+{
- int i = 0; /* transfer data from source to destination */ for (i = 0; i < page_size; i++) { /* If the data is 0xFF, don't program it */
@@ -306,77 +317,53 @@ retry: dst++; src++; }
- toggle_ready_jedec(dst - 1);
- dst = d;
- src = s;
- failed = verify_range(flash, src, start, page_size, NULL);
- if (failed && tried++ < MAX_REFLASH_TRIES) {
fprintf(stderr, "retrying.\n");goto retry;- }
- if (failed) {
fprintf(stderr, " page 0x%lx failed!\n",(d - bios) / page_size);- }
- return failed;
- return 0;
}
-int write_byte_program_jedec(chipaddr bios, uint8_t *src,
chipaddr dst)+int write_sector_jedec_common(struct flashchip *flash, uint8_t *src,
chipaddr dst, unsigned int page_size, unsigned int mask,unsigned int shift){
- int tried = 0, failed = 0;
- /* If the data is 0xFF, don't program it and don't complain. */
- if (*src == 0xFF) {
return 0;- }
- int tried = 0, failed;
- chipaddr bios = flash->virtual_memory;
- chipaddr v = bios;
retry:
- /* Issue JEDEC Byte Program command */
- start_program_jedec(bios);
- /* transfer data from source to destination */
- chip_writeb(*src, dst);
- toggle_ready_jedec(bios);
- /* Issue JEDEC Data Unprotect comand */
- start_program_jedec_common(bios, mask, shift);
- if (chip_readb(dst) != *src && tried++ < MAX_REFLASH_TRIES) {
goto retry;
- if (page_size > 1)
- {
failed = write_helper_page(src, dst, page_size);v = dst - 1;- }
- else
- {
failed = write_helper_byte(src, dst);
Can't you use write_helper_page(src, dst, page_size) here as well? Assuming page_size!=0, page_size will be 1 in this else branch, so it should be essentially the same.
}
- if (tried >= MAX_REFLASH_TRIES)
failed = 1;- return failed;
-}
-int write_sector_jedec(chipaddr bios, uint8_t *src,
chipaddr dst, unsigned int page_size)-{
- int i, failed = 0;
- chipaddr olddst;
- toggle_ready_jedec(v);
- //failed = verify_range(flash, src, start, page_size, NULL);
Why is verify_range commented out here?
- olddst = dst;
- for (i = 0; i < page_size; i++) {
if (write_byte_program_jedec(bios, src, dst))failed = 1;dst++, src++;
- if (failed && tried++ < MAX_REFLASH_TRIES) {
fprintf(stderr, "retrying.\n");goto retry;- }
- if (failed) {
fprintf(stderr, " page 0x%lx failed!\n",(dst - bios) / page_size);
- if (failed)
fprintf(stderr, " writing sector at 0x%lx failed!\n", olddst);- return failed;
}
-int write_jedec(struct flashchip *flash, uint8_t *buf) +int write_jedec_common(struct flashchip *flash, uint8_t *buf, unsigned int mask,
unsigned int shift)
Kill shift.
{ int i, failed = 0; int total_size = flash->total_size * 1024; int page_size = flash->page_size;
- if (erase_chip_jedec(flash)) {
- if (erase_chip_jedec_common(flash, mask, shift)) { fprintf(stderr,"ERASE FAILED!\n"); return -1; }
@@ -384,8 +371,8 @@ int write_jedec(struct flashchip *flash, uint8_t *buf) printf("Programming page: "); for (i = 0; i < total_size / page_size; i++) { printf("%04d at address: 0x%08x", i, i * page_size);
if (write_page_write_jedec(flash, buf + i * page_size,i * page_size, page_size))
if (write_sector_jedec_common(flash, buf + i * page_size,i * page_size, page_size, mask, shift)) failed = 1;@@ -394,29 +381,68 @@ int write_jedec(struct flashchip *flash, uint8_t *buf) return failed; }
-int write_jedec_1(struct flashchip *flash, uint8_t * buf) +void start_program_jedec(chipaddr bios)
Please use struct flashchip *flash instead of chipaddr bios in the function signature. Thanks.
{
- int i;
- chipaddr bios = flash->virtual_memory;
- chipaddr dst = bios;
- start_program_jedec_common(bios, 0x0000, 0);
mask should be 0xffff (default value).
+}
- programmer_delay(10);
- if (erase_flash(flash)) {
fprintf(stderr, "ERASE FAILED!\n");return -1;- }
+void start_program_jedec_2aa(chipaddr bios)
struct flashchip *flash instead of chipaddr bios please.
+{
- start_program_jedec_common(bios, 0xf000, 2);
mask should be 0x7ff (for 0x555) or 0x3ff (for 0x155). 0x2aa alone doesn't make it 100% clear.
+}
- 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.
+}
write_sector_jedec(bios, buf + i * 1024, dst + i * 1024, 1024);+int probe_jedec(struct flashchip *flash) +{
- return probe_jedec_common(flash, 0x90, 0x00, 0x01, 0x0000, 0, 0);
mask should be 0xffff (default value).
+}
if ((i & 0x3) == 0)printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");- }
+int probe_jedec_2aa_reset(struct flashchip *flash) +{
- return probe_jedec_common(flash, 0x90, 0x00, 0x01, 0xf000, 2, 1);
mask should be 0x3ff or 0x7ff depending on whether the other address is 0x155 or 0x555.
+}
- printf("\n");
- return 0;
+int probe_jedec_aaa_did_02(struct flashchip *flash) +{
- return probe_jedec_common(flash, 0x90, 0x00, 0x02, 0xf000, 0, 0);
mask.
+}
+int probe_jedec_60(struct flashchip *flash) +{
- return probe_jedec_common(flash, 0x60, 0x00, 0x01, 0x0000, 0, 0);
mask.
+}
+int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int size) +{
- return erase_sector_jedec_common(flash, page, size, 0x0000, 0);
mask.
+}
+int erase_sector_jedec_2aa(struct flashchip *flash, unsigned int page, unsigned int size) +{
- return erase_sector_jedec_common(flash, page, size, 0xf000, 2);
mask.
+}
+int erase_sector_jedec_aaa(struct flashchip *flash, unsigned int page, unsigned int size) +{
- return erase_sector_jedec_common(flash, page, size, 0xf000, 0);
mask.
+}
+int erase_block_jedec(struct flashchip *flash, unsigned int page, unsigned int size) +{
- return erase_block_jedec_common(flash, page, size, 0x0000, 0);
mask.
+}
+int erase_chip_jedec(struct flashchip *flash) +{
- return erase_chip_jedec_common(flash, 0x0000, 0);
mask.
}
+int write_jedec(struct flashchip *flash, uint8_t *buf) +{
- return write_jedec_common(flash, buf, 0x0000, 0);
mask.
+}
diff --git a/pm29f002.c b/pm29f002.c index bf78d13..8f1d010 100644 --- a/pm29f002.c +++ b/pm29f002.c @@ -21,7 +21,7 @@ #include "flash.h"
/* if write_sector_jedec is used,
- this is write_jedec_1 */
- this is write_jedec */
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.
int write_pm29f002(struct flashchip *flash, uint8_t *buf) { int i, total_size = flash->total_size * 1024; diff --git a/pm49fl00x.c b/pm49fl00x.c index 27a1163..a016928 100644 --- a/pm49fl00x.c +++ b/pm49fl00x.c @@ -101,8 +101,8 @@ int write_49fl00x(struct flashchip *flash, uint8_t *buf)
/* write to the sector */ printf("%04d at address: 0x%08x", i, i * page_size);
write_sector_jedec(bios, buf + i * page_size,bios + i * page_size, page_size);
write_sector_jedec_common(flash, buf + i * page_size,bios + i * page_size, page_size,0,0);
Hmmm. mask?
printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"); fflush(stdout);} diff --git a/sst49lf040.c b/sst49lf040.c index ab1c918..821347b 100644 --- a/sst49lf040.c +++ b/sst49lf040.c @@ -59,8 +59,8 @@ int write_49lf040(struct flashchip *flash, uint8_t *buf) if (i % 10 == 0) printf("%04d at address: 0x%08x ", i, i * page_size);
write_sector_jedec(bios, buf + i * page_size,bios + i * page_size, page_size);
write_sector_jedec_common(flash, buf + i * page_size,bios + i * page_size, page_size, 0, 0);
mask?
if (i % 10 == 0) printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");diff --git a/sst_fwhub.c b/sst_fwhub.c index f09aa54..079dec5 100644 --- a/sst_fwhub.c +++ b/sst_fwhub.c @@ -157,8 +157,8 @@ int write_sst_fwhub(struct flashchip *flash, uint8_t *buf) page_size); if (rc) return 1;
write_sector_jedec(bios, buf + i * page_size,bios + i * page_size, page_size);
write_sector_jedec_common(flash, buf + i * page_size,bios + i * page_size, page_size, 0, 0);
mask?
} printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");} diff --git a/w39v040c.c b/w39v040c.c index 722ae29..b4b72ca 100644 --- a/w39v040c.c +++ b/w39v040c.c @@ -80,8 +80,8 @@ int write_w39v040c(struct flashchip *flash, uint8_t *buf) printf("Programming page: "); for (i = 0; i < total_size / page_size; i++) { printf("%04d at address: 0x%08x", i, i * page_size);
write_sector_jedec(bios, buf + i * page_size,bios + i * page_size, page_size);
write_sector_jedec_common(flash, buf + i * page_size,bios + i * page_size, page_size, 0, 0);
mask?
printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");} printf("\n"); diff --git a/w39v080fa.c b/w39v080fa.c index 580657f..ec06d1b 100644 --- a/w39v080fa.c +++ b/w39v080fa.c @@ -182,7 +182,7 @@ int write_winbond_fwhub(struct flashchip *flash, uint8_t *buf) printf("Programming: "); for (i = 0; i < total_size; i += flash->page_size) { printf("0x%08x\b\b\b\b\b\b\b\b\b\b", i);
write_sector_jedec(bios, buf + i, bios + i, flash->page_size);
write_sector_jedec_common(flash, buf + i, bios + i, flash->page_size, 0, 0);
mask?
} printf("\n");
diff --git a/w49f002u.c b/w49f002u.c index d12bc72..28eaa99 100644 --- a/w49f002u.c +++ b/w49f002u.c @@ -36,8 +36,8 @@ int write_49f002(struct flashchip *flash, uint8_t *buf) for (i = 0; i < total_size / page_size; i++) { printf("%04d at address: 0x%08x ", i, i * page_size); /* Byte-wise writing of 'page_size' bytes. */
write_sector_jedec(bios, buf + i * page_size,bios + i * page_size, page_size);
write_sector_jedec_common(flash, buf + i * page_size,bios + i * page_size, page_size, 0, 0);
mask?
printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"); fflush(stdout);}
One word about coding style: I admit it is inconsistent, but having the function signature on one line (regardless of the 80 colun limit) is sometimes pretty useful (e.g. when grepping for functions).
Shift can not always replace mask here. Think 0xAAA/0x555 vs. 0x2AA/0x555. You can't find a shift value that will convert one pair into the other.
Regards, Carl-Daniel
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.
On 25.12.2009 22:48, Sean Nelson wrote:
On 12/25/09 11:23 AM, Carl-Daniel Hailfinger wrote:
On 25.12.2009 07:03, Sean Nelson wrote:
- en29f002a -> file_not_used
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.
Ah OK. I think sometime in the past we forgot to create flashchips.c entries for the chips en29f002a.c was created for.
As it stands, we still need/use w29ee011 for that very check.
The good news is that we can narrow down the check. The chip that dies is LPC, but w29ee011 is Parallel, so if buses_supported doesn't include LPC, we can probe without worries.
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.
I'd rather have int feature_bits; in struct flashchip and set a registermap bit like this: #define FEATURE_REGISTERMAP (1 << 0) .feature_bits = FEATURE_REGISTERMAP,
Everything is now using mask-only.
Very nice.
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.
Thanks.
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
Hm yes. For now, I'd like to keep it as write_jedec_1 if possible.
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.
Will take a look.
Blanket answer to the rest of your replies: OK.
diff --git a/chipdrivers.h b/chipdrivers.h index adcb46d..3ccc478 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -78,20 +78,21 @@ void toggle_ready_jedec(chipaddr dst); void data_polling_jedec(chipaddr dst, uint8_t data); void start_program_jedec(chipaddr bios); int write_byte_program_jedec(chipaddr bios, uint8_t *src, chipaddr dst); int probe_jedec(struct flashchip *flash); int erase_chip_jedec(struct flashchip *flash); int write_jedec(struct flashchip *flash, uint8_t *buf); -int write_jedec_1(struct flashchip *flash, uint8_t *buf); int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int pagesize); int erase_block_jedec(struct flashchip *flash, unsigned int page, unsigned int blocksize); int erase_chip_block_jedec(struct flashchip *flash, unsigned int page, unsigned int blocksize); -int write_sector_jedec(chipaddr bios, uint8_t *src,
chipaddr dst, unsigned int page_size);+int write_sector_jedec_common(struct flashchip *flash, uint8_t *src,
chipaddr dst, unsigned int page_size,unsigned int mask);/* m29f002.c */ int erase_m29f002(struct flashchip *flash); int write_m29f002t(struct flashchip *flash, uint8_t *buf); int write_m29f002b(struct flashchip *flash, uint8_t *buf);
/* m29f400bt.c */ diff --git a/flash.h b/flash.h index feac98e..cb3bc77 100644 --- a/flash.h +++ b/flash.h @@ -156,14 +156,16 @@ struct flashchip { * Identification code. */ uint32_t manufacture_id; uint32_t model_id;
int total_size; int page_size;
- int remap;
See my bitfield comments above.
- int byte_based_write;
Can we please either postpone this and keep jedec_write_1 or at least rename the field to write_granularity (in bytes) or somesuch. The latter will require setting a boatload of chips to write_granularity=256, and the former will require more stuff inside jedec.c. Bah.
/* * Indicate if flashrom has been tested with this flash chip and if * everything worked correctly. */ uint32_t tested;
diff --git a/flashchips.c b/flashchips.c index 59f9139..af9039d 100644 --- a/flashchips.c +++ b/flashchips.c @@ -102,15 +102,15 @@ struct flashchip flashchips[] = { }, .block_erase = erase_sector_jedec, }, { .eraseblocks = { {256 * 1024, 1} }, .block_erase = erase_chip_block_jedec, }, },
.write = write_jedec_1,
.write = write_jedec,.read = read_memmapped, },
{ .vendor = "AMD", .name = "Am29F002(N)BT", .bustype = CHIP_BUSTYPE_PARALLEL,
@@ -133,15 +133,15 @@ struct flashchip flashchips[] = { }, .block_erase = erase_sector_jedec, }, { .eraseblocks = { {256 * 1024, 1} }, .block_erase = erase_chip_block_jedec, }, },
.write = write_jedec_1,
.write = write_jedec,.read = read_memmapped, },
{ .vendor = "AMD", .name = "Am29F016D", .bustype = CHIP_BUSTYPE_PARALLEL,
@@ -1178,15 +1178,15 @@ struct flashchip flashchips[] = { }, .block_erase = erase_sector_29f002, }, { .eraseblocks = { {256 * 1024, 1} }, .block_erase = erase_chip_29f002, }, },
.write = write_jedec_1,
.write = write_jedec,.read = read_memmapped, },
{ .vendor = "AMIC", .name = "A29002T", .bustype = CHIP_BUSTYPE_PARALLEL,
@@ -1209,15 +1209,15 @@ struct flashchip flashchips[] = { }, .block_erase = erase_sector_29f002, }, { .eraseblocks = { {256 * 1024, 1} }, .block_erase = erase_chip_29f002, }, },
.write = write_jedec_1,
.write = write_jedec,.read = read_memmapped, },
{ .vendor = "AMIC", .name = "A29040B", .bustype = CHIP_BUSTYPE_PARALLEL,
@@ -1570,15 +1570,15 @@ struct flashchip flashchips[] = { }, .block_erase = erase_sector_jedec, }, { .eraseblocks = { {256 * 1024, 1} }, .block_erase = erase_chip_block_jedec, }, },
.write = write_jedec_1,
.write = write_jedec,.read = read_memmapped, },
{ .vendor = "EON", .name = "EN29F002(A)(N)T", .bustype = CHIP_BUSTYPE_PARALLEL,
@@ -1601,15 +1601,15 @@ struct flashchip flashchips[] = { }, .block_erase = erase_sector_jedec, }, { .eraseblocks = { {256 * 1024, 1} }, .block_erase = erase_chip_block_jedec, }, },
.write = write_jedec_1,
.write = write_jedec,.read = read_memmapped, },
{ .vendor = "Fujitsu", .name = "MBM29F004BC", .bustype = CHIP_BUSTYPE_PARALLEL,
@@ -2108,15 +2108,15 @@ struct flashchip flashchips[] = { .model_id = MX_29F001B, .total_size = 128, .page_size = 32 * 1024, .tested = TEST_OK_PRE, .probe = probe_29f002, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (mx29f002.c) */ .erase = erase_29f002,
.write = write_jedec_1,
.write = write_jedec,.read = read_memmapped, },
{ .vendor = "Macronix", .name = "MX29F001T", .bustype = CHIP_BUSTYPE_PARALLEL,
@@ -2124,15 +2124,15 @@ struct flashchip flashchips[] = { .model_id = MX_29F001T, .total_size = 128, .page_size = 32 * 1024, .tested = TEST_OK_PRE, .probe = probe_29f002, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (mx29f002.c) */ .erase = erase_29f002,
.write = write_jedec_1,
.write = write_jedec,.read = read_memmapped, },
{ .vendor = "Macronix", .name = "MX29F002B", .bustype = CHIP_BUSTYPE_PARALLEL,
@@ -2155,15 +2155,15 @@ struct flashchip flashchips[] = { }, .block_erase = erase_sector_29f002, /* This erase function has 64k blocksize for eLiteFlash */ }, { .eraseblocks = { {256 * 1024, 1} }, .block_erase = erase_chip_29f002, }, },
.write = write_jedec_1,
.write = write_jedec,.read = read_memmapped, },
{ .vendor = "Macronix", .name = "MX29F002T", .bustype = CHIP_BUSTYPE_PARALLEL,
@@ -2186,15 +2186,15 @@ struct flashchip flashchips[] = { }, .block_erase = erase_sector_29f002, /* This erase function has 64k blocksize for eLiteFlash */ }, { .eraseblocks = { {256 * 1024, 1} }, .block_erase = erase_chip_29f002, }, },
.write = write_jedec_1,
.write = write_jedec,.read = read_memmapped, },
{ .vendor = "Macronix", .name = "MX29LV040", .bustype = CHIP_BUSTYPE_PARALLEL,
@@ -2202,15 +2202,15 @@ struct flashchip flashchips[] = { .model_id = MX_29LV040, .total_size = 512, .page_size = 64 * 1024, .tested = TEST_OK_PR, .probe = probe_29f002, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (mx29f002.c) */ .erase = erase_29f002,
.write = write_jedec_1,
.write = write_jedec,.read = read_memmapped, },
{ .vendor = "Numonyx", .name = "M25PE10", .bustype = CHIP_BUSTYPE_SPI,
diff --git a/jedec.c b/jedec.c index d6cad41..302bee0 100644 --- a/jedec.c +++ b/jedec.c @@ -83,22 +83,25 @@ void data_polling_jedec(chipaddr dst, uint8_t data) break; } } if (i > 0x100000) printf_debug("%s: excessive loops, i=0x%x\n", __func__, i); }
-void start_program_jedec(chipaddr bios) +void start_program_jedec_common(struct flashchip *flash, unsigned int mask) {
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0xA0, bios + 0x5555);
- chipaddr bios = flash->virtual_memory;
- chip_writeb(0xAA, bios + (0x5555 & mask));
- chip_writeb(0x55, bios + (0x2AAA & mask));
- chip_writeb(0xA0, bios + (0x5555 & mask));
Exactly as I wanted it.
}
-int probe_jedec(struct flashchip *flash) +int probe_jedec_common(struct flashchip *flash,
unsigned int mid_addr, unsigned int did_addr,unsigned int mask, int long_reset){ chipaddr bios = flash->virtual_memory; uint8_t id1, id2; uint32_t largeid1, largeid2; uint32_t flashcontent1, flashcontent2; int probe_timing_enter, probe_timing_exit;
@@ -114,27 +117,27 @@ int probe_jedec(struct flashchip *flash) } else { printf("Chip has negative value in probe_timing, failing " "without chip access\n"); return 0; }
/* Issue JEDEC Product ID Entry command */
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask)); if (probe_timing_enter) programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + (0x2AAA & mask)); if (probe_timing_enter) programmer_delay(10);
- chip_writeb(0x90, bios + 0x5555);
chip_writeb(0x90, bios + (0x5555 & mask)); if (probe_timing_enter) programmer_delay(probe_timing_enter);
/* Read product ID */
- id1 = chip_readb(bios);
- id2 = chip_readb(bios + 0x01);
- id1 = chip_readb(bios + mid_addr);
- id2 = chip_readb(bios + did_addr);
Well, it seems I missed this in the last review. We should always read the first part of the ID at 0x00/0x01 and I thought did_addr/mid_addr specified the followup locations used for largeid1/largeid2.
largeid1 = id1; largeid2 = id2;
/* Check if it is a continuation ID, this should be a while loop. */ if (id1 == 0x7F) { largeid1 <<= 8; id1 = chip_readb(bios + 0x100); @@ -143,21 +146,24 @@ int probe_jedec(struct flashchip *flash) if (id2 == 0x7F) { largeid2 <<= 8; id2 = chip_readb(bios + 0x101); largeid2 |= id2; }
/* Issue JEDEC Product ID Exit command */
- chip_writeb(0xAA, bios + 0x5555);
- if (probe_timing_exit)
programmer_delay(10);- chip_writeb(0x55, bios + 0x2AAA);
- if (probe_timing_exit)
programmer_delay(10);- chip_writeb(0xF0, bios + 0x5555);
if (long_reset)
{
chip_writeb(0xAA, bios + (0x5555 & mask));if (probe_timing_exit)programmer_delay(10);chip_writeb(0x55, bios + (0x2AAA & mask));if (probe_timing_exit)programmer_delay(10);}
chip_writeb(0xF0, bios + (0x5555 & mask)); if (probe_timing_exit) programmer_delay(probe_timing_exit);
printf_debug("%s: id1 0x%02x, id2 0x%02x", __func__, largeid1, largeid2); if (!oddparity(id1)) printf_debug(", id1 parity violation");
@@ -180,243 +186,213 @@ int probe_jedec(struct flashchip *flash) if (largeid2 == flashcontent2) printf_debug(", id2 is normal flash content");
printf_debug("\n"); if (largeid1 == flash->manufacture_id && largeid2 == flash->model_id) return 1;
- if (flash->remap)
map_flash_registers(flash);
I'll be honest. If we really have remap (or registermap) as a value in struct flashchip, we could move the register mapping to generic probe code in flashrom.c.
- return 0;
}
-int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int pagesize) +int erase_sector_jedec_common(struct flashchip *flash, unsigned int page,
unsigned int pagesize, unsigned int mask){ chipaddr bios = flash->virtual_memory;
/* Issue the Sector Erase command */
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + (0x2AAA & mask)); programmer_delay(10);
- chip_writeb(0x80, bios + 0x5555);
- chip_writeb(0x80, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
chip_writeb(0x55, bios + (0x2AAA & mask)); programmer_delay(10); chip_writeb(0x30, bios + page); programmer_delay(10);
/* wait for Toggle bit ready */ toggle_ready_jedec_slow(bios);
if (check_erased_range(flash, page, pagesize)) { fprintf(stderr,"ERASE FAILED!\n"); return -1; } return 0;
}
-int erase_block_jedec(struct flashchip *flash, unsigned int block, unsigned int blocksize) +int erase_block_jedec_common(struct flashchip *flash, unsigned int block,
unsigned int blocksize, unsigned int mask){ chipaddr bios = flash->virtual_memory;
/* Issue the Sector Erase command */
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + (0x2AAA & mask)); programmer_delay(10);
- chip_writeb(0x80, bios + 0x5555);
- chip_writeb(0x80, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
chip_writeb(0x55, bios + (0x2AAA & mask)); programmer_delay(10); chip_writeb(0x50, bios + block); programmer_delay(10);
/* wait for Toggle bit ready */ toggle_ready_jedec_slow(bios);
if (check_erased_range(flash, block, blocksize)) { fprintf(stderr,"ERASE FAILED!\n"); return -1; } return 0;
}
-/* erase chip with block_erase() prototype */ -int erase_chip_block_jedec(struct flashchip *flash, unsigned int addr, unsigned int blocksize) -{
- if ((addr != 0) || (blocksize != flash->total_size * 1024)) {
fprintf(stderr, "%s called with incorrect arguments\n",__func__);return -1;- }
- return erase_chip_jedec(flash);
-}
-int erase_chip_jedec(struct flashchip *flash) +int erase_chip_jedec_common(struct flashchip *flash, unsigned int mask) { int total_size = flash->total_size * 1024; chipaddr bios = flash->virtual_memory;
/* Issue the JEDEC Chip Erase command */
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + (0x2AAA & mask)); programmer_delay(10);
- chip_writeb(0x80, bios + 0x5555);
- chip_writeb(0x80, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + (0x2AAA & mask)); programmer_delay(10);
- chip_writeb(0x10, bios + 0x5555);
chip_writeb(0x10, bios + (0x5555 & mask)); programmer_delay(10);
toggle_ready_jedec_slow(bios);
if (check_erased_range(flash, 0, total_size)) { fprintf(stderr,"ERASE FAILED!\n"); return -1; } return 0;
}
-int write_page_write_jedec(struct flashchip *flash, uint8_t *src,
int start, int page_size)+int write_sector_jedec_common(struct flashchip *flash, uint8_t *src,
chipaddr dst, unsigned int page_size,unsigned int mask)
Not sure about the changed prototype from "int start" to "chipaddr dst". Usage of chipaddr can go horribly wrong if confused with start. I am guilty of a few blunders in that area myself (did find most of them before submitting, though).
{ int i, tried = 0, failed;
- uint8_t *s = src; chipaddr bios = flash->virtual_memory;
- chipaddr dst = bios + start;
- chipaddr d = dst;
- chipaddr v = bios;
- unsigned int start = dst - bios;
retry: /* Issue JEDEC Data Unprotect comand */
While you're at it, call it JEDEC Start Program command. That makes it clear what we're doing here.
- start_program_jedec(bios);
start_program_jedec_common(flash, mask);
/* transfer data from source to destination */
- for (i = 0; i < page_size; i++) {
/* If the data is 0xFF, don't program it */
- if (flash->byte_based_write)
I don't like the if/else branch. Either use my write_granularity suggestion (and that variable can then be used as direct replacement of page_size) or at least set a local variable (e.g. named write_granularity or if you want write_page_size or whatever) to the value used as loop terminator (1 is a valid number of loops).
- {
for (i = 0; i < page_size; i++) {/* If the data is 0xFF, don't program it */if (*src != 0xFF)chip_writeb(*src, dst);dst++;src++;}v = dst - 1;- }
- else
- { if (*src != 0xFF) chip_writeb(*src, dst);
dst++;src++;}
toggle_ready_jedec(dst - 1);
dst = d;
src = s;
toggle_ready_jedec(v); failed = verify_range(flash, src, start, page_size, NULL);
if (failed && tried++ < MAX_REFLASH_TRIES) { fprintf(stderr, "retrying.\n"); goto retry; } if (failed) { fprintf(stderr, " page 0x%lx failed!\n",
(d - bios) / page_size);- }
- return failed;
-}
-int write_byte_program_jedec(chipaddr bios, uint8_t *src,
chipaddr dst)-{
- int tried = 0, failed = 0;
- /* If the data is 0xFF, don't program it and don't complain. */
- if (*src == 0xFF) {
return 0;- }
-retry:
- /* Issue JEDEC Byte Program command */
- start_program_jedec(bios);
- /* transfer data from source to destination */
- chip_writeb(*src, dst);
- toggle_ready_jedec(bios);
- if (chip_readb(dst) != *src && tried++ < MAX_REFLASH_TRIES) {
goto retry;- }
- if (tried >= MAX_REFLASH_TRIES)
failed = 1;- return failed;
-}
-int write_sector_jedec(chipaddr bios, uint8_t *src,
chipaddr dst, unsigned int page_size)-{
- int i, failed = 0;
- chipaddr olddst;
- olddst = dst;
- for (i = 0; i < page_size; i++) {
if (write_byte_program_jedec(bios, src, dst))failed = 1;dst++, src++;
(dst - bios) / page_size);
- if (failed)
fprintf(stderr, " writing sector at 0x%lx failed!\n", olddst);- return failed;
}
-int write_jedec(struct flashchip *flash, uint8_t *buf) +int write_jedec_common(struct flashchip *flash, uint8_t *buf, unsigned int mask) { int i, failed = 0; int total_size = flash->total_size * 1024; int page_size = flash->page_size;
- if (erase_chip_jedec(flash)) {
if (erase_chip_jedec_common(flash, mask)) { fprintf(stderr,"ERASE FAILED!\n"); return -1; }
printf("Programming page: "); for (i = 0; i < total_size / page_size; i++) { printf("%04d at address: 0x%08x", i, i * page_size);
if (write_page_write_jedec(flash, buf + i * page_size,i * page_size, page_size))
if (write_sector_jedec_common(flash, buf + i * page_size,i * page_size, page_size, mask))
Hm. Using page_size here seems very out of place. Not your fault, though. It should be revisited once we kill page_size.
failed = 1; printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");} printf("\n");
return failed; }
-int write_jedec_1(struct flashchip *flash, uint8_t * buf) -{
int i;
chipaddr bios = flash->virtual_memory;
chipaddr dst = bios;
programmer_delay(10);
if (erase_flash(flash)) {
fprintf(stderr, "ERASE FAILED!\n");+/* erase chip with block_erase() prototype */ +int erase_chip_block_jedec(struct flashchip *flash, unsigned int addr,
unsigned int blocksize)+{
- if ((addr != 0) || (blocksize != flash->total_size * 1024)) {
fprintf(stderr, "%s called with incorrect arguments\n",__func__);- return erase_chip_jedec_common(flash, 0xffff);
+}
- 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(struct flashchip *flash) +{
- start_program_jedec_common(flash, 0xffff);
+}
Is this still used?
write_sector_jedec(bios, buf + i * 1024, dst + i * 1024, 1024);+int probe_jedec(struct flashchip *flash) +{
- return probe_jedec_common(flash, 0x00, 0x01, 0xffff, 1);
#define SHORT_RESET 0 #define LONG_RESET 1 and use LONG_RESET here. It would make some stuff more readable.
Or we go the full way to abstraction and add a second bit to the .feature_bits in struct flashchip. #define FEATURE_RESET_LONG (1 << 1) #define FEATURE_RESET_SHORT (0 << 1) #define FEATURE_RESET_EITHER FEATURE_RESET_SHORT .feature_bits = FEATURE_LONGRESET,
+}
if ((i & 0x3) == 0)printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");- }
+int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int size) +{
- return erase_sector_jedec_common(flash, page, size, 0xffff);
+} +int erase_block_jedec(struct flashchip *flash, unsigned int page, unsigned int size) +{
- return erase_block_jedec_common(flash, page, size, 0xffff);
+}
- printf("\n");
- return 0;
+int erase_chip_jedec(struct flashchip *flash) +{
- return erase_chip_jedec_common(flash, 0xffff);
}
+int write_jedec(struct flashchip *flash, uint8_t *buf) +{
- return write_jedec_common(flash, buf, 0xffff);
+}
diff --git a/pm29f002.c b/pm29f002.c index bf78d13..8f1d010 100644 --- a/pm29f002.c +++ b/pm29f002.c @@ -17,15 +17,15 @@
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*/
#include "flash.h"
/* if write_sector_jedec is used,
- this is write_jedec_1 */
- this is write_jedec */
My usual write_jedec_1 complaint.
int write_pm29f002(struct flashchip *flash, uint8_t *buf) { int i, total_size = flash->total_size * 1024; chipaddr bios = flash->virtual_memory; chipaddr dst = bios;
if (erase_flash(flash)) { diff --git a/pm49fl00x.c b/pm49fl00x.c index 27a1163..424b0ed 100644 --- a/pm49fl00x.c +++ b/pm49fl00x.c @@ -97,16 +97,16 @@ int write_49fl00x(struct flashchip *flash, uint8_t *buf) if (erase_block_jedec(flash, i * page_size, page_size)) { fprintf(stderr, "ERASE FAILED!\n"); return -1; }
/* write to the sector */ printf("%04d at address: 0x%08x", i, i * page_size);
write_sector_jedec(bios, buf + i * page_size,bios + i * page_size, page_size);
write_sector_jedec_common(flash, buf + i * page_size,bios + i * page_size, page_size, 0xffff);printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"); fflush(stdout); } printf("\n");
/* protected */ write_lockbits_49fl00x(flash->virtual_registers, total_size, 1,
diff --git a/sst49lf040.c b/sst49lf040.c index ab1c918..c91a139 100644 --- a/sst49lf040.c +++ b/sst49lf040.c @@ -55,16 +55,16 @@ int write_49lf040(struct flashchip *flash, uint8_t *buf) return -1; }
/* write to the sector */ if (i % 10 == 0) printf("%04d at address: 0x%08x ", i, i * page_size);
write_sector_jedec(bios, buf + i * page_size,bios + i * page_size, page_size);
write_sector_jedec_common(flash, buf + i * page_size,bios + i * page_size, page_size, 0xffff);if (i % 10 == 0) printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"); fflush(stdout); } printf("\n");
diff --git a/sst_fwhub.c b/sst_fwhub.c index f09aa54..4a976e6 100644 --- a/sst_fwhub.c +++ b/sst_fwhub.c @@ -153,16 +153,16 @@ int write_sst_fwhub(struct flashchip *flash, uint8_t *buf) flash->read(flash, readbuf, i * page_size, page_size); if (memcmp((void *)(buf + i * page_size), (void *)(readbuf), page_size)) { rc = erase_sst_fwhub_block(flash, i * page_size, page_size); if (rc) return 1;
write_sector_jedec(bios, buf + i * page_size,bios + i * page_size, page_size);
write_sector_jedec_common(flash, buf + i * page_size,bios + i * page_size, page_size, 0xffff);} printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"); } printf("\n");
return 0;
} diff --git a/w39v040c.c b/w39v040c.c index 722ae29..66ab115 100644 --- a/w39v040c.c +++ b/w39v040c.c @@ -76,15 +76,15 @@ int write_w39v040c(struct flashchip *flash, uint8_t *buf) fprintf(stderr, "ERASE FAILED!\n"); return -1; }
printf("Programming page: "); for (i = 0; i < total_size / page_size; i++) { printf("%04d at address: 0x%08x", i, i * page_size);
write_sector_jedec(bios, buf + i * page_size,bios + i * page_size, page_size);
write_sector_jedec_common(flash, buf + i * page_size,bios + i * page_size, page_size, 0xffff);printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"); } printf("\n");
return 0;
} diff --git a/w39v080fa.c b/w39v080fa.c index 580657f..311e55b 100644 --- a/w39v080fa.c +++ b/w39v080fa.c @@ -178,13 +178,13 @@ int write_winbond_fwhub(struct flashchip *flash, uint8_t *buf)
if (erase_winbond_fwhub(flash)) return -1;
printf("Programming: "); for (i = 0; i < total_size; i += flash->page_size) { printf("0x%08x\b\b\b\b\b\b\b\b\b\b", i);
write_sector_jedec(bios, buf + i, bios + i, flash->page_size);
write_sector_jedec_common(flash, buf + i, bios + i, flash->page_size, 0xffff);} printf("\n");
return 0;
} diff --git a/w49f002u.c b/w49f002u.c index d12bc72..87ce000 100644 --- a/w49f002u.c +++ b/w49f002u.c @@ -32,16 +32,16 @@ int write_49f002(struct flashchip *flash, uint8_t *buf) return -1; }
printf("Programming page: "); for (i = 0; i < total_size / page_size; i++) { printf("%04d at address: 0x%08x ", i, i * page_size); /* Byte-wise writing of 'page_size' bytes. */
write_sector_jedec(bios, buf + i * page_size,bios + i * page_size, page_size);
write_sector_jedec_common(flash, buf + i * page_size,bios + i * page_size, page_size, 0xffff);printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"); fflush(stdout); } printf("\n");
return 0;
}
I have to admit I didn't check the conversion of the chip drivers. They look sane on a quick glance, but I'll have to verify calling conventions (int start vs. chipaddr dst) closely before giving a final Ack.
To be honest, I will kill all progress printing first thing after this is committed. Better no progress printing than the current slow (and sometimes buggy) stuff.
Regards, Carl-Daniel
Am Freitag, den 25.12.2009, 23:28 +0100 schrieb Carl-Daniel Hailfinger:
- int byte_based_write;
Can we please either postpone this and keep jedec_write_1 or at least rename the field to write_granularity (in bytes) or somesuch. The latter will require setting a boatload of chips to write_granularity=256, and the former will require more stuff inside jedec.c. Bah.
We definitely can not commit a patch that replaces write_jedec_1 by write_jedec in flashchips.c without also changing page_size (after verifying that it is not used as erase block size) or move to a write_block_size (or write_granularity) setting that is correctly set for all chips that use it. Stuff like:
diff --git a/flashchips.c b/flashchips.c index 59f9139..af9039d 100644 --- a/flashchips.c +++ b/flashchips.c @@ -102,15 +102,15 @@ struct flashchip flashchips[] = { }, .block_erase = erase_sector_jedec, }, { .eraseblocks = { {256 * 1024, 1} }, .block_erase = erase_chip_block_jedec, }, },
.write = write_jedec_1,
.write = write_jedec,.read = read_memmapped, },
{ .vendor = "AMD", .name = "Am29F002(N)BT", .bustype = CHIP_BUSTYPE_PARALLEL,
will definitely break many very common chips.
Regards, Michael Karcher
The chips that are suppose to use en29f002a already use 29f040b. Ok, I've added the feature bit field and FEATURE_REGISTERMAP. I've restored all the old write code and modified as little as I can, we should work on the design some more before anyone changes the write functions.
Well, it seems I missed this in the last review. We should always read the first part of the ID at 0x00/0x01 and I thought did_addr/mid_addr specified the followup locations used for largeid1/largeid2.
One chip expect to read the id using 0x00 and 0x02 on the first run... probe_m29f400bt => probe_jedec_aaa_did_02
I have to admit I didn't check the conversion of the chip drivers. They look sane on a quick glance, but I'll have to verify calling conventions (int start vs. chipaddr dst) closely before giving a final Ack.
To be honest, I will kill all progress printing first thing after this is committed. Better no progress printing than the current slow (and sometimes buggy) stuff.
Regards, Carl-Daniel
Sound great! Thanks for the review, help, and everything. Thanks to Michael Karcher for the review and the double check on Don't Cares in the datasheets.
Also added my Copyrights lines to jedec.c and flashchips.c.
On 12/25/2009 6:56 PM, Sean Nelson wrote:
The chips that are suppose to use en29f002a already use 29f040b. Ok, I've added the feature bit field and FEATURE_REGISTERMAP. I've restored all the old write code and modified as little as I can, we should work on the design some more before anyone changes the write functions.
Well, it seems I missed this in the last review. We should always read the first part of the ID at 0x00/0x01 and I thought did_addr/mid_addr specified the followup locations used for largeid1/largeid2.
One chip expect to read the id using 0x00 and 0x02 on the first run... probe_m29f400bt => probe_jedec_aaa_did_02
I have to admit I didn't check the conversion of the chip drivers. They look sane on a quick glance, but I'll have to verify calling conventions (int start vs. chipaddr dst) closely before giving a final Ack.
To be honest, I will kill all progress printing first thing after this is committed. Better no progress printing than the current slow (and sometimes buggy) stuff.
Regards, Carl-Daniel
Sound great! Thanks for the review, help, and everything. Thanks to Michael Karcher for the review and the double check on Don't Cares in the datasheets.
Also added my Copyrights lines to jedec.c and flashchips.c.
Ping?
On 26.12.2009 03:56, Sean Nelson wrote:
The chips that are suppose to use en29f002a already use 29f040b. Ok, I've added the feature bit field and FEATURE_REGISTERMAP. I've restored all the old write code and modified as little as I can, we should work on the design some more before anyone changes the write functions.
Nice.
Well, it seems I missed this in the last review. We should always read the first part of the ID at 0x00/0x01 and I thought did_addr/mid_addr specified the followup locations used for largeid1/largeid2.
One chip expect to read the id using 0x00 and 0x02 on the first run... probe_m29f400bt => probe_jedec_aaa_did_02
Hm. The M29F400BT/M29F400BB datasheet says deviceid should be read at address 0x01 (and not 0x02 as the code does). Then again, that chip can be switched between 16bit and 8bit mode and in 8bit mode all addresses are shifted by one. I'd like to leave it as special case and maybe find out who owned that chip when flashrom support was written for it. The MBM29F400TC/MBM29F400BC datasheet says deviceid should be read at address 0x01 in 16bit mode and 0x02 in 8bit mode. However, it uses shifted command addresses in 8bit mode as well.
The more I think of it, the happier I'd be if the mid/did changes were dropped for now. Sorry.
I think those chips will make good use of the shifting stuff once we introduce it.
diff --git a/chipdrivers.h b/chipdrivers.h index adcb46d..d8d9bbe 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -72,26 +72,28 @@ int probe_en29f002a(struct flashchip *flash); int erase_en29f002a(struct flashchip *flash); int write_en29f002a(struct flashchip *flash, uint8_t *buf);
/* jedec.c */ uint8_t oddparity(uint8_t val); void toggle_ready_jedec(chipaddr dst); void data_polling_jedec(chipaddr dst, uint8_t data); -void start_program_jedec(chipaddr bios); +void start_program_jedec(struct flashchip *flash); int write_byte_program_jedec(chipaddr bios, uint8_t *src, chipaddr dst); int probe_jedec(struct flashchip *flash); int erase_chip_jedec(struct flashchip *flash); int write_jedec(struct flashchip *flash, uint8_t *buf); int write_jedec_1(struct flashchip *flash, uint8_t *buf); int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int pagesize); int erase_block_jedec(struct flashchip *flash, unsigned int page, unsigned int blocksize); int erase_chip_block_jedec(struct flashchip *flash, unsigned int page, unsigned int blocksize); -int write_sector_jedec(chipaddr bios, uint8_t *src,
chipaddr dst, unsigned int page_size);+int write_sector_jedec_common(struct flashchip *flash, uint8_t *src,
chipaddr dst, unsigned int page_size,unsigned int mask);
This one may be more readable as one line. We violate the 80-col limit in flash.h anyway.
Extraneous newline.
/* m29f002.c */ int erase_m29f002(struct flashchip *flash); int write_m29f002t(struct flashchip *flash, uint8_t *buf); int write_m29f002b(struct flashchip *flash, uint8_t *buf);
/* m29f400bt.c */ diff --git a/flash.h b/flash.h index feac98e..4d1d6b6 100644 --- a/flash.h +++ b/flash.h @@ -140,14 +140,17 @@ enum chipbustype { #define NUM_ERASEREGIONS 5
/*
- How many different erase functions do we have per chip?
*/ #define NUM_ERASEFUNCTIONS 5
+#define FEATURE_REGISTERMAP (1 << 0) +#define FEATURE_BYTEWRITES (1 << 1)
struct flashchip { const char *vendor; const char *name;
enum chipbustype bustype;
/* @@ -156,14 +159,15 @@ struct flashchip { * Identification code. */ uint32_t manufacture_id; uint32_t model_id;
int total_size; int page_size;
int feature_bits;
/*
- Indicate if flashrom has been tested with this flash chip and if
- everything worked correctly.
*/ uint32_t tested;
diff --git a/flashchips.c b/flashchips.c index 59f9139..50a1226 100644 --- a/flashchips.c +++ b/flashchips.c @@ -1,14 +1,15 @@ /*
- This file is part of the flashrom project.
- Copyright (C) 2000 Silicon Integrated System Corporation
- Copyright (C) 2004 Tyan Corp
- Copyright (C) 2005-2008 coresystems GmbH stepan@openbios.org
- Copyright (C) 2006-2009 Carl-Daniel Hailfinger
- Copyright (C) 2009 Sean Nelson audiohacked@gmail.com
Maybe mention this copyright line in the changelog ("Add copyright line to flashchips.c because of my recent work in there"). Or you postpone this and include it in the followup patch which will change flashchips.c.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 2 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
diff --git a/jedec.c b/jedec.c index d6cad41..d1a8fe0 100644 --- a/jedec.c +++ b/jedec.c @@ -1,14 +1,15 @@ /*
- This file is part of the flashrom project.
- Copyright (C) 2000 Silicon Integrated System Corporation
- Copyright (C) 2006 Giampiero Giancipoli gianci@email.it
- Copyright (C) 2006 coresystems GmbH info@coresystems.de
- Copyright (C) 2007 Carl-Daniel Hailfinger
- Copyright (C) 2009 Sean Nelson audiohacked@gmail.com
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 2 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
@@ -83,22 +84,25 @@ void data_polling_jedec(chipaddr dst, uint8_t data) break; } } if (i > 0x100000) printf_debug("%s: excessive loops, i=0x%x\n", __func__, i); }
-void start_program_jedec(chipaddr bios) +void start_program_jedec_common(struct flashchip *flash, unsigned int mask) {
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0xA0, bios + 0x5555);
- chipaddr bios = flash->virtual_memory;
- chip_writeb(0xAA, bios + (0x5555 & mask));
- chip_writeb(0x55, bios + (0x2AAA & mask));
- chip_writeb(0xA0, bios + (0x5555 & mask));
}
-int probe_jedec(struct flashchip *flash) +int probe_jedec_common(struct flashchip *flash,
unsigned int mid_addr, unsigned int did_addr,unsigned int mask, int long_reset){ chipaddr bios = flash->virtual_memory; uint8_t id1, id2; uint32_t largeid1, largeid2; uint32_t flashcontent1, flashcontent2; int probe_timing_enter, probe_timing_exit;
@@ -114,27 +118,27 @@ int probe_jedec(struct flashchip *flash) } else { printf("Chip has negative value in probe_timing, failing " "without chip access\n"); return 0; }
/* Issue JEDEC Product ID Entry command */
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask)); if (probe_timing_enter) programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + (0x2AAA & mask)); if (probe_timing_enter) programmer_delay(10);
- chip_writeb(0x90, bios + 0x5555);
chip_writeb(0x90, bios + (0x5555 & mask)); if (probe_timing_enter) programmer_delay(probe_timing_enter);
/* Read product ID */
- id1 = chip_readb(bios);
- id2 = chip_readb(bios + 0x01);
- id1 = chip_readb(bios + mid_addr);
- id2 = chip_readb(bios + did_addr);
I still believe we should read address 0x0 and 0x01 above. The largeid code below should be what uses mid_addr and did_addr. And for the special case where 8bit/16bit chips need special probe addresses, we have to handle shifting for all addresses or it will fail anyway. Can you simply drop the mid/did stuff for now, both in the code and as parameters?
largeid1 = id1; largeid2 = id2;
/* Check if it is a continuation ID, this should be a while loop. */ if (id1 == 0x7F) { largeid1 <<= 8; id1 = chip_readb(bios + 0x100); @@ -143,21 +147,24 @@ int probe_jedec(struct flashchip *flash) if (id2 == 0x7F) { largeid2 <<= 8; id2 = chip_readb(bios + 0x101); largeid2 |= id2; }
/* Issue JEDEC Product ID Exit command */
- chip_writeb(0xAA, bios + 0x5555);
- if (probe_timing_exit)
programmer_delay(10);- chip_writeb(0x55, bios + 0x2AAA);
- if (probe_timing_exit)
programmer_delay(10);- chip_writeb(0xF0, bios + 0x5555);
if (long_reset)
{
chip_writeb(0xAA, bios + (0x5555 & mask));if (probe_timing_exit)programmer_delay(10);chip_writeb(0x55, bios + (0x2AAA & mask));if (probe_timing_exit)programmer_delay(10);}
chip_writeb(0xF0, bios + (0x5555 & mask)); if (probe_timing_exit) programmer_delay(probe_timing_exit);
printf_debug("%s: id1 0x%02x, id2 0x%02x", __func__, largeid1, largeid2); if (!oddparity(id1)) printf_debug(", id1 parity violation");
@@ -180,167 +187,123 @@ int probe_jedec(struct flashchip *flash) if (largeid2 == flashcontent2) printf_debug(", id2 is normal flash content");
printf_debug("\n"); if (largeid1 == flash->manufacture_id && largeid2 == flash->model_id) return 1;
- if (flash->feature_bits & FEATURE_REGISTERMAP)
map_flash_registers(flash);
Nice. We'll have to change flashchips.c for all chips which need that feature. Since you're not switching over chips yet, this is perfect.
- return 0;
}
-int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int pagesize) +int erase_sector_jedec_common(struct flashchip *flash, unsigned int page,
unsigned int pagesize, unsigned int mask){ chipaddr bios = flash->virtual_memory;
/* Issue the Sector Erase command */
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + (0x2AAA & mask)); programmer_delay(10);
- chip_writeb(0x80, bios + 0x5555);
- chip_writeb(0x80, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
chip_writeb(0x55, bios + (0x2AAA & mask)); programmer_delay(10); chip_writeb(0x30, bios + page); programmer_delay(10);
/* wait for Toggle bit ready */ toggle_ready_jedec_slow(bios);
if (check_erased_range(flash, page, pagesize)) { fprintf(stderr,"ERASE FAILED!\n"); return -1; } return 0;
}
-int erase_block_jedec(struct flashchip *flash, unsigned int block, unsigned int blocksize) +int erase_block_jedec_common(struct flashchip *flash, unsigned int block,
unsigned int blocksize, unsigned int mask){ chipaddr bios = flash->virtual_memory;
/* Issue the Sector Erase command */
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + (0x2AAA & mask)); programmer_delay(10);
- chip_writeb(0x80, bios + 0x5555);
- chip_writeb(0x80, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
chip_writeb(0x55, bios + (0x2AAA & mask)); programmer_delay(10); chip_writeb(0x50, bios + block); programmer_delay(10);
/* wait for Toggle bit ready */ toggle_ready_jedec_slow(bios);
if (check_erased_range(flash, block, blocksize)) { fprintf(stderr,"ERASE FAILED!\n"); return -1; } return 0;
}
-/* erase chip with block_erase() prototype */ -int erase_chip_block_jedec(struct flashchip *flash, unsigned int addr, unsigned int blocksize) -{
- if ((addr != 0) || (blocksize != flash->total_size * 1024)) {
fprintf(stderr, "%s called with incorrect arguments\n",__func__);return -1;- }
- return erase_chip_jedec(flash);
-}
-int erase_chip_jedec(struct flashchip *flash) +int erase_chip_jedec_common(struct flashchip *flash, unsigned int mask) { int total_size = flash->total_size * 1024; chipaddr bios = flash->virtual_memory;
/* Issue the JEDEC Chip Erase command */
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + (0x2AAA & mask)); programmer_delay(10);
- chip_writeb(0x80, bios + 0x5555);
- chip_writeb(0x80, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0xAA, bios + 0x5555);
- chip_writeb(0xAA, bios + (0x5555 & mask)); programmer_delay(10);
- chip_writeb(0x55, bios + 0x2AAA);
- chip_writeb(0x55, bios + (0x2AAA & mask)); programmer_delay(10);
- chip_writeb(0x10, bios + 0x5555);
chip_writeb(0x10, bios + (0x5555 & mask)); programmer_delay(10);
toggle_ready_jedec_slow(bios);
if (check_erased_range(flash, 0, total_size)) { fprintf(stderr,"ERASE FAILED!\n"); return -1; } return 0;
}
-int write_page_write_jedec(struct flashchip *flash, uint8_t *src,
int start, int page_size)+int write_byte_program_jedec_common(struct flashchip *flash, uint8_t *src,
chipaddr dst, unsigned int mask){
- int i, tried = 0, failed;
- uint8_t *s = src; chipaddr bios = flash->virtual_memory;
- chipaddr dst = bios + start;
- chipaddr d = dst;
-retry:
- /* Issue JEDEC Data Unprotect comand */
- start_program_jedec(bios);
- /* transfer data from source to destination */
- for (i = 0; i < page_size; i++) {
/* If the data is 0xFF, don't program it */if (*src != 0xFF)chip_writeb(*src, dst);dst++;src++;- }
- toggle_ready_jedec(dst - 1);
- dst = d;
- src = s;
- failed = verify_range(flash, src, start, page_size, NULL);
- if (failed && tried++ < MAX_REFLASH_TRIES) {
fprintf(stderr, "retrying.\n");goto retry;- }
- if (failed) {
fprintf(stderr, " page 0x%lx failed!\n",(d - bios) / page_size);- }
- return failed;
-}
-int write_byte_program_jedec(chipaddr bios, uint8_t *src,
chipaddr dst)-{ int tried = 0, failed = 0;
/* If the data is 0xFF, don't program it and don't complain. */ if (*src == 0xFF) { return 0; }
retry: /* Issue JEDEC Byte Program command */
- start_program_jedec(bios);
start_program_jedec_common(flash, mask);
/* transfer data from source to destination */ chip_writeb(*src, dst); toggle_ready_jedec(bios);
if (chip_readb(dst) != *src && tried++ < MAX_REFLASH_TRIES) { goto retry;
@@ -348,48 +311,88 @@ retry:
if (tried >= MAX_REFLASH_TRIES) failed = 1;
return failed; }
-int write_sector_jedec(chipaddr bios, uint8_t *src,
chipaddr dst, unsigned int page_size)+int write_sector_jedec_common(struct flashchip *flash, uint8_t *src,
chipaddr dst, unsigned int page_size, unsigned int mask){ int i, failed = 0; chipaddr olddst;
olddst = dst; for (i = 0; i < page_size; i++) {
if (write_byte_program_jedec(bios, src, dst))
if (write_byte_program_jedec_common(flash, src, dst, mask)) failed = 1;dst++, src++; } if (failed) fprintf(stderr, " writing sector at 0x%lx failed!\n", olddst);
return failed;
}
+int write_page_write_jedec_common(struct flashchip *flash, uint8_t *src,
int start, int page_size, unsigned int mask)
AFAICS you just moved this function down a few lines (and added mask). Is that right? If yes, fine.
+{
- int i, tried = 0, failed;
- uint8_t *s = src;
- chipaddr bios = flash->virtual_memory;
- chipaddr dst = bios + start;
- chipaddr d = dst;
+retry:
- /* Issue JEDEC Start Program comand */
- start_program_jedec_common(flash, mask);
- /* transfer data from source to destination */
- for (i = 0; i < page_size; i++) {
/* If the data is 0xFF, don't program it */if (*src != 0xFF)chip_writeb(*src, dst);dst++;src++;- }
- toggle_ready_jedec(dst - 1);
- dst = d;
- src = s;
- failed = verify_range(flash, src, start, page_size, NULL);
- if (failed && tried++ < MAX_REFLASH_TRIES) {
fprintf(stderr, "retrying.\n");goto retry;- }
- if (failed) {
fprintf(stderr, " page 0x%lx failed!\n",(d - bios) / page_size);- }
- return failed;
+}
int write_jedec(struct flashchip *flash, uint8_t *buf) { int i, failed = 0; int total_size = flash->total_size * 1024; int page_size = flash->page_size;
if (erase_chip_jedec(flash)) { fprintf(stderr,"ERASE FAILED!\n"); return -1; }
printf("Programming page: "); for (i = 0; i < total_size / page_size; i++) { printf("%04d at address: 0x%08x", i, i * page_size);
if (write_page_write_jedec(flash, buf + i * page_size,i * page_size, page_size))
if (write_page_write_jedec_common(flash, buf + i * page_size,i * page_size, page_size, 0xffff))
Hm. Maybe replace 0xffff with MASK_FULL or some other #define? To be honest, this comment of mine is very low priority and I am not even sure the #define is a good idea.
failed = 1; printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");} printf("\n");
return failed; } @@ -407,16 +410,53 @@ int write_jedec_1(struct flashchip *flash, uint8_t * buf) }
printf("Programming page: "); for (i = 0; i < flash->total_size; i++) { if ((i & 0x3) == 0) printf("address: 0x%08lx", (unsigned long)i * 1024);
write_sector_jedec(bios, buf + i * 1024, dst + i * 1024, 1024);
write_sector_jedec_common(flash, buf + i * 1024, dst + i * 1024, 1024, 0xffff);
Same here.
if ((i & 0x3) == 0) printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");}
printf("\n"); return 0; }
+/* erase chip with block_erase() prototype */ +int erase_chip_block_jedec(struct flashchip *flash, unsigned int addr,
unsigned int blocksize)+{
- if ((addr != 0) || (blocksize != flash->total_size * 1024)) {
fprintf(stderr, "%s called with incorrect arguments\n",__func__);return -1;- }
- return erase_chip_jedec_common(flash, 0xffff);
+}
+void start_program_jedec(struct flashchip *flash)
Is this function used? If not, drop.
+{
- start_program_jedec_common(flash, 0xffff);
+}
+int probe_jedec(struct flashchip *flash) +{
- return probe_jedec_common(flash, 0x00, 0x01, 0xffff, 1);
+}
+int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int size) +{
- return erase_sector_jedec_common(flash, page, size, 0xffff);
+}
+int erase_block_jedec(struct flashchip *flash, unsigned int page, unsigned int size) +{
- return erase_block_jedec_common(flash, page, size, 0xffff);
+}
+int erase_chip_jedec(struct flashchip *flash) +{
- return erase_chip_jedec_common(flash, 0xffff);
+} diff --git a/pm49fl00x.c b/pm49fl00x.c index 27a1163..424b0ed 100644 --- a/pm49fl00x.c +++ b/pm49fl00x.c @@ -97,16 +97,16 @@ int write_49fl00x(struct flashchip *flash, uint8_t *buf) if (erase_block_jedec(flash, i * page_size, page_size)) { fprintf(stderr, "ERASE FAILED!\n"); return -1; }
/* write to the sector */ printf("%04d at address: 0x%08x", i, i * page_size);
write_sector_jedec(bios, buf + i * page_size,bios + i * page_size, page_size);
write_sector_jedec_common(flash, buf + i * page_size,bios + i * page_size, page_size, 0xffff);
This function needs to die (or at least it should be a wrapper for a generic block walker). Not today, though. Your change is fine as is.
printf("\n");
/* protected */ write_lockbits_49fl00x(flash->virtual_registers, total_size, 1, diff --git a/sst49lf040.c b/sst49lf040.c index ab1c918..c91a139 100644 --- a/sst49lf040.c +++ b/sst49lf040.c @@ -55,16 +55,16 @@ int write_49lf040(struct flashchip *flash, uint8_t *buf) return -1; }
/* write to the sector */ if (i % 10 == 0) printf("%04d at address: 0x%08x ", i, i * page_size);
write_sector_jedec(bios, buf + i * page_size,bios + i * page_size, page_size);
write_sector_jedec_common(flash, buf + i * page_size,bios + i * page_size, page_size, 0xffff);
Same here.
if (i % 10 == 0) printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"); fflush(stdout);} printf("\n");
diff --git a/sst_fwhub.c b/sst_fwhub.c index f09aa54..4a976e6 100644 --- a/sst_fwhub.c +++ b/sst_fwhub.c @@ -153,16 +153,16 @@ int write_sst_fwhub(struct flashchip *flash, uint8_t *buf) flash->read(flash, readbuf, i * page_size, page_size); if (memcmp((void *)(buf + i * page_size), (void *)(readbuf), page_size)) { rc = erase_sst_fwhub_block(flash, i * page_size, page_size); if (rc) return 1;
write_sector_jedec(bios, buf + i * page_size,bios + i * page_size, page_size);
write_sector_jedec_common(flash, buf + i * page_size,bios + i * page_size, page_size, 0xffff);
And here...
} printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");} printf("\n");
return 0; } diff --git a/w39v040c.c b/w39v040c.c index 722ae29..66ab115 100644 --- a/w39v040c.c +++ b/w39v040c.c @@ -76,15 +76,15 @@ int write_w39v040c(struct flashchip *flash, uint8_t *buf) fprintf(stderr, "ERASE FAILED!\n"); return -1; }
printf("Programming page: "); for (i = 0; i < total_size / page_size; i++) { printf("%04d at address: 0x%08x", i, i * page_size);
write_sector_jedec(bios, buf + i * page_size,bios + i * page_size, page_size);
write_sector_jedec_common(flash, buf + i * page_size,bios + i * page_size, page_size, 0xffff);
And here...
printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");} printf("\n");
return 0; } diff --git a/w39v080fa.c b/w39v080fa.c index 580657f..311e55b 100644 --- a/w39v080fa.c +++ b/w39v080fa.c @@ -178,13 +178,13 @@ int write_winbond_fwhub(struct flashchip *flash, uint8_t *buf)
if (erase_winbond_fwhub(flash)) return -1;
printf("Programming: "); for (i = 0; i < total_size; i += flash->page_size) { printf("0x%08x\b\b\b\b\b\b\b\b\b\b", i);
write_sector_jedec(bios, buf + i, bios + i, flash->page_size);
write_sector_jedec_common(flash, buf + i, bios + i, flash->page_size, 0xffff);
And here...
} printf("\n");
return 0; } diff --git a/w49f002u.c b/w49f002u.c index d12bc72..87ce000 100644 --- a/w49f002u.c +++ b/w49f002u.c @@ -32,16 +32,16 @@ int write_49f002(struct flashchip *flash, uint8_t *buf) return -1; }
printf("Programming page: "); for (i = 0; i < total_size / page_size; i++) { printf("%04d at address: 0x%08x ", i, i * page_size); /* Byte-wise writing of 'page_size' bytes. */
write_sector_jedec(bios, buf + i * page_size,bios + i * page_size, page_size);
write_sector_jedec_common(flash, buf + i * page_size,bios + i * page_size, page_size, 0xffff);
And here...
printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"); fflush(stdout);} printf("\n");
return 0; }
The current version is a huge improvement over earlier iterations and a lot easier to review. I am very confident we can commit the next iteration of this patch.
Regards, Carl-Daniel
The patch converts jedec functions into mask-based generics which can be used for many chip provided the only changes are the the addresses are converted from 0x5555/0x2AAA to 0x555/0x2AA or similar. These functions won't work for the 0xAAA chips listed above, but most of listed chips should work with these changes. The other changes I suggested will come in later patches, and the writing functions are untouched since we'll move to a block writers system. The patch mostly changes jedec.c, but a few other files are changed because they use the jedec functions within their own functions.
The patch also adds a copyright line to flashchips.c because of my recent work in converting AMD and Atmel chips to use struct erase_block.
Signed-off-by: Sean Nelson audiohacked@gmail.com
On 04.01.2010 07:39, Sean Nelson wrote:
The patch converts jedec functions into mask-based generics which can be used for many chip provided the only changes are the the addresses are converted from 0x5555/0x2AAA to 0x555/0x2AA or similar.
These functions won't work for the 0xAAA chips listed above, but most of listed chips should work with these changes. The other changes I suggested will come in later patches, and the writing functions are untouched since we'll move to a block writers system.
Maybe snip the two sentences above from the changelog. (There are no "0xAAA chips listed above".)
The patch mostly changes jedec.c, but a few other files are changed because they use the jedec functions within their own functions.
The patch also adds a copyright line to flashchips.c because of my recent work in converting AMD and Atmel chips to use struct erase_block.
Signed-off-by: Sean Nelson audiohacked@gmail.com
Very nice. Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Regards, Carl-Daniel
The next step would be the conversion of the probe functions in flashchips.c.
I think we may have overcomplicated the design for chips with smaller address masks. Can't we add two feature bits for address masks like #define ADDR_FULL (0x0 << 2) #define ADDR_555_AAA (0x1 << 2) #define ADDR_555_2AA (0x2 << 2) #define ADDR_whatever (0x3 << 2) with ADDR_FULL being the default value? That way, we need only one probe_jedec function which checks the feature bits and sets the mask based on the ADDR_foo defines. In case we want to handle those odd 16bit/8bit chips as well, we can add another feature bit (ADDR_LEFTSHIFT_ONE) for them.
About the register mapping bits: To be honest, I think we can't move the register mapping settings blindly from the old code, they are incorrect in more than one place. AFAICS it is not possible with current mainboard hardware to do any register mapping stuff on Parallel/SPI chips. Parallel chips don't have enough address lines, and SPI chips have a command interface anyway and the higher address bits are defined as don't care. OTOH, I think every LPC/FWH chip out there uses register mapping.
Regards, Carl-Daniel
On 1/4/2010 6:05 AM, Carl-Daniel Hailfinger wrote:
On 04.01.2010 07:39, Sean Nelson wrote:
The patch converts jedec functions into mask-based generics which can be used for many chip provided the only changes are the the addresses are converted from 0x5555/0x2AAA to 0x555/0x2AA or similar.
These functions won't work for the 0xAAA chips listed above, but most of listed chips should work with these changes. The other changes I suggested will come in later patches, and the writing functions are untouched since we'll move to a block writers system.
Maybe snip the two sentences above from the changelog. (There are no "0xAAA chips listed above".)
The patch mostly changes jedec.c, but a few other files are changed because they use the jedec functions within their own functions.
The patch also adds a copyright line to flashchips.c because of my recent work in converting AMD and Atmel chips to use struct erase_block.
Signed-off-by: Sean Nelsonaudiohacked@gmail.com
Very nice. Acked-by: Carl-Daniel Hailfingerc-d.hailfinger.devel.2006@gmx.net
Regards, Carl-Daniel
Thanks, committed in r828.
~Sean Nelson
-
Carl-Daniel Hailfinger -
Michael Karcher -
Sean Nelson