On 01.04.2010 00:48, Michael Karcher wrote:
This should also fix broken write for non-uniform sectored FWH-like chips. These are: Intel 28F001 Sharp LHF00L04 ST M50FW002 ST M50LPW116
Signed-off-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
82802ab.c | 85 +++++++++++++++++++++++++++++-------------- chipdrivers.h | 3 ++ flash.h | 2 +- flashchips.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ flashchips.h | 4 ++ 5 files changed, 175 insertions(+), 29 deletions(-)
diff --git a/82802ab.c b/82802ab.c index aa7e45e..27acaa3 100644 --- a/82802ab.c +++ b/82802ab.c @@ -48,6 +48,12 @@ int probe_82802ab(struct flashchip *flash) chipaddr bios = flash->virtual_memory; uint8_t id1, id2; uint8_t flashcontent1, flashcontent2;
- int id2_offset;
- if (flash->feature_bits & FEATURE_ADDR_SHIFTED)
Umm... I'm pretty sure that FEATURE_ADDR_SHIFTED has another meaning (the 0x555 etc shifted left by one bit). The id2_offset may be needed anyway, but I propose to use a variable "shifted" which is 0 for all non-shifted chips and 1 for all shifted chips. It would be used as left shift for every programmatic access to special addresses. Such a change would allow us to kill m29f400bt.c which has buggy write functions anyway.
Example for probing:
/* Issue JEDEC Product ID Entry command */ chip_writeb(0xAA, bios + (0x5555 & mask) << shifted); chip_writeb(0x55, bios + (0x2AAA & mask) << shifted); chip_writeb(0x90, bios + (0x5555 & mask) << shifted);
id2_offset = 2;else
id2_offset = 1;/* Reset to get a clean state */ chip_writeb(0xFF, bios);
@@ -58,7 +64,7 @@ int probe_82802ab(struct flashchip *flash) programmer_delay(10);
id1 = chip_readb(bios);
Arguably my "shifted" proposal suggests to use the following line for consistency:
id1 = chip_readb(bios + 0x00 << shifted);
- id2 = chip_readb(bios + 0x01);
- id2 = chip_readb(bios + id2_offset);
id2 = chip_readb(bios + 0x01 << shifted);
/* Leave ID mode */ chip_writeb(0xFF, bios); @@ -72,7 +78,7 @@ int probe_82802ab(struct flashchip *flash)
/* Read the product ID location again. We should now see normal flash contents. */ flashcontent1 = chip_readb(bios);
- flashcontent2 = chip_readb(bios + 0x01);
flashcontent2 = chip_readb(bios + id2_offset);
if (id1 == flashcontent1) msg_cdbg(", id1 is normal flash content");
@@ -177,41 +183,64 @@ void write_page_82802ab(chipaddr bios, uint8_t *src,
int write_82802ab(struct flashchip *flash, uint8_t *buf) {
- int i;
- int total_size = flash->total_size * 1024;
- int i, j, blocknum;
- int blockoffset = 0; int page_size = flash->page_size; chipaddr bios = flash->virtual_memory; uint8_t *tmpbuf = malloc(page_size);
- struct eraseblock * blocks;
- int (*erase) (struct flashchip *flash, unsigned int blockaddr,
unsigned int blocklen);- /* find first erase layout with a working erase function */
- for (i = 0; flash->block_erasers[i].block_erase == NULL &&
i < NUM_ERASEFUNCTIONS; i++);- if (i == NUM_ERASEFUNCTIONS)
- {
msg_perr("No working erase function found - can't write\n");return -1;- }
- msg_pdbg("Chosing block layout #%d\n", i);
- blocks = flash->block_erasers[i].eraseblocks;
- erase = flash->block_erasers[i].block_erase;
That's too clever to be risked for 0.9.2. I can't see obvious bugs, but this code triggers my "might break if included directly before a release" fear.
if (!tmpbuf) { msg_cerr("Could not allocate memory!\n"); exit(1); }
- msg_cinfo("Programming page: \n");
- for (i = 0; i < total_size / page_size; i++) {
msg_cinfo("\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");msg_cinfo("%04d at address: 0x%08x", i, i * page_size);/* Auto Skip Blocks, which already contain the desired data* Faster, because we only write, what has changed* More secure, because blocks, which are excluded* (with the exclude or layout feature)* or not erased and rewritten; their data is retained also in* sudden power off situations*/chip_readn(tmpbuf, bios + i * page_size, page_size);if (!memcmp((void *)(buf + i * page_size), tmpbuf, page_size)) {msg_cdbg("SKIPPED\n");continue;}/* erase block by block and write block by block; this is the most secure way */if (erase_block_82802ab(flash, i * page_size, page_size)) {msg_cerr("ERASE FAILED!\n");return -1;
- msg_cinfo("Programming block: \n");
- blocknum = 0;
- for (i = 0; blocks[i].size != 0; i++) {
This is guaranteed access random data if a chip has NUM_ERASEREGIONS erase regions for a particular erase function. Either check for i<NUM_ERASEREGIONS as well or use the chip size as upper limit. The chip size check is probably more difficult here.
for(j = 0; j < blocks[i].count; j++) {msg_cinfo("\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");msg_cinfo("%04d at address: 0x%08x", blocknum,blockoffset);/* Auto Skip Blocks, which already contain the* desired data:* Faster, because we only write, what has changed* More secure, because blocks, which are excluded* (with the exclude or layout feature)* or not erased and rewritten; their data is* retained also in sudden power off situations*/chip_readn(tmpbuf, bios + blockoffset, blocks[i].size);if (!memcmp((void *)(buf + i * page_size), tmpbuf,blocks[i].size)) {msg_cdbg("SKIPPED\n");} else {if (erase(flash, blockoffset, page_size)) {msg_cerr("ERASE FAILED!\n");return -1;}write_page_82802ab(bios, buf + blockoffset,bios + blockoffset, blocks[i].size);}blockoffset += blocks[i].size;blocknum++;
write_page_82802ab(bios, buf + i * page_size,bios + i * page_size, page_size);
To be honest, I think the current code in write_82802ab() is something that should be ripped out and destroyed. It is essentially special-casing partial writes for a family of chips instead of doing it correctly in the generic code. The conversion to partial writes will happen post 0.9.2 anyway, and then this code will be ripped out completely. Your changes, on the other hand, beat at least some sanity into the code, so it would be desirable to have them in generic code post-0.9.2. Please coordinate with David Hendricks who is working on partial flashing.
} msg_cinfo("DONE!\n"); free(tmpbuf); diff --git a/chipdrivers.h b/chipdrivers.h index 6d5cef0..816088a 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -53,6 +53,9 @@ int spi_nbyte_read(int addr, uint8_t *bytes, int len); int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize); int spi_aai_write(struct flashchip *flash, uint8_t *buf);
+/* i28f00x.c */ +int write_28f00x(struct flashchip *flash, uint8_t *buf);
I don't see that function anywhere. Did you forget to svn add? Or was this some aborted attempt?
/* 82802ab.c */ uint8_t wait_82802ab(chipaddr bios); int probe_82802ab(struct flashchip *flash); diff --git a/flashchips.c b/flashchips.c index 2dbc1e0..f838ab2 100644 --- a/flashchips.c +++ b/flashchips.c @@ -2385,6 +2385,116 @@ struct flashchip flashchips[] = {
{ .vendor = "Intel",
.name = "28F004BV/BE-B",.bustype = CHIP_BUSTYPE_PARALLEL,- },
- {
.vendor = "Intel",.name = "28F004BV/BE-T",.bustype = CHIP_BUSTYPE_PARALLEL,- },
- {
.vendor = "Intel",.name = "28F400BV/CV/CE-B",.bustype = CHIP_BUSTYPE_PARALLEL,.feature_bits = FEATURE_ADDR_SHIFTED,- },
- {
.vendor = "Intel",.name = "28F004BV/BE-T",
That name is identical to the one two chips above AFAICS.
.bustype = CHIP_BUSTYPE_PARALLEL,.feature_bits = FEATURE_ADDR_SHIFTED,
So the bottom two chips have the address left-shifted by 1 bit, but use full address size?
Suggestions: How about pointing the FEATURE_ADDR_SHIFTED chips to a to-be-fixed m29f400bt.c for now? It would reduce jedec.c changes a bit. How about using erase_flash() in write_82802ab() and dropping any rolling reflash mechanism until post 0.9.2? It should solve all non-uniform sector stuff. While that's a fear-reaching change as well, I believe it makes the code more reviewable, and does allow us to kill more duplicated code. As an added benefit, conversion to partial write will be easier.
Regards, Carl-Daniel