On Fri, Jun 12, 2009 at 17:01, Carl-Daniel Hailfingerc-d.hailfinger.devel.2006@gmx.net wrote:
flashrom only checks for very few chips if the erase worked. And even when it checks if the erase worked, the result of that check is often ignored.
Fix the majority of these problems. More to come, but I'd like to get this reviewed first.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
I'll do first a review-like listing of my changes and then inline my new patch (and attach too).
Attached patch: Signed-off-by: Urja Rannikko urjaman@gmail.com
Also carl-daniel's patch with my additions is Acked-by me, but...
+/* start is an offset to the base address of the flash chip */ +int check_erased_range(struct flashchip *flash, int start, int len) +{
- int page_size = flash->page_size;
- uint8_t *cmpbuf = malloc(page_size);
malloc (len), not page_size
- if (!cmpbuf) {
- fprintf(stderr, "Could not allocate memory!\n");
- exit(1);
- }
- memset(cmpbuf, 0xff, len);
- return verify_range(flash, cmpbuf, start, len, "ERASE");
add variable to hold return value, free cmpbuf after calling verify_range
+}
+/* start is an offset to the base address of the flash chip */ +int verify_range(struct flashchip *flash, uint8_t *cmpbuf, int start, int len, char *message) +{
- int i, j, starthere, lenhere;
- chipaddr bios = flash->virtual_memory;
- int page_size = flash->page_size;
- uint8_t *readbuf = malloc(page_size);
- if (!readbuf) {
- fprintf(stderr, "Could not allocate memory!\n");
- exit(1);
- }
- if (start + len > flash->total_size * 1024) {
- fprintf(stderr, "Error: %s called with start 0x%x + len 0x%x >"
- " total_size 0x%x\n", __func__, start, len,
- flash->total_size * 1024);
free readbuf
- return -1;
- }
- if (!len)
- return 0;
free readbuf
- if (!message)
- message = "VERIFY";
- /* Warning: This loop has a very unusual condition and body.
- * The loop needs to go through each page with at least one affected
- * byte. The lowest page number is (start / page_size) since that
- * division rounds down. The highest page number we want is the page
- * where the last byte of the range lives. That last byte has the
- * address (start + len - 1), thus the highest page number is
- * (start + len - 1) / page_size. Since we want to include that last
- * page as well, the loop condition uses <=.
- */
- for (i = start / page_size; i <= (start + len - 1) / page_size; i++) {
- /* Byte position of the first byte in the range in this page. */
- starthere = max(start, i * page_size);
- /* Length of bytes in the range in this page. */
- lenhere = min(start + len, (i + 1) * page_size) - starthere;
- /* starthere is an offset to the base address of the chip. */
- chip_readn(readbuf, bios + starthere, lenhere);
- for (j = 0; j < lenhere; j++) {
- if (cmpbuf[starthere + j] != readbuf[j]) {
- fprintf(stderr, "%s FAILED at 0x%08x! "
- "Expected=0x%02x, Read=0x%02x\n",
- message, starthere + j,
- cmpbuf[starthere + j], readbuf[j]);
as above
- return -1;
- }
- }
- }
as above
- return 0;
+}
Also, changed the erase check in jedec.c to use check_erased_range.
My new patch inlined: Index: flash.h =================================================================== --- flash.h (revision 589) +++ flash.h (working copy) @@ -722,6 +722,9 @@ void map_flash_registers(struct flashchip *flash); int read_memmapped(struct flashchip *flash, uint8_t *buf); int min(int a, int b); +int max(int a, int b); +int check_erased_range(struct flashchip *flash, int start, int len); +int verify_range(struct flashchip *flash, uint8_t *cmpbuf, int start, int len, char *message); extern char *pcidev_bdf;
/* layout.c */ Index: en29f002a.c =================================================================== --- en29f002a.c (revision 589) +++ en29f002a.c (working copy) @@ -98,7 +98,10 @@
//chip_writeb(0xF0, bios); programmer_delay(10); - erase_chip_jedec(flash); + if (erase_chip_jedec(flash)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + }
printf("Programming page: "); for (i = 0; i < total_size; i++) { Index: jedec.c =================================================================== --- jedec.c (revision 589) +++ jedec.c (working copy) @@ -344,13 +344,11 @@
erase_chip_jedec(flash); // dumb check if erase was successful. - for (i = 0; i < total_size; i++) { - if (chip_readb(bios + i) != 0xff) { - printf("ERASE FAILED @%d, val %02x!\n", i, chip_readb(bios + i)); - return -1; - } + if (check_erased_range(flash, 0, total_size)) { + 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); Index: sharplhf00l04.c =================================================================== --- sharplhf00l04.c (revision 589) +++ sharplhf00l04.c (working copy) @@ -124,6 +124,10 @@ print_lhf00l04_status(status); printf("DONE BLOCK 0x%x\n", offset);
+ if (check_erased_range(flash, offset, flash->page_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } return 0; }
@@ -135,7 +139,10 @@ printf("total_size is %d; flash->page_size is %d\n", total_size, flash->page_size); for (i = 0; i < total_size; i += flash->page_size) - erase_lhf00l04_block(flash, i); + if (erase_lhf00l04_block(flash, i)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } printf("DONE ERASE\n");
return 0; @@ -161,9 +168,8 @@ int page_size = flash->page_size; chipaddr bios = flash->virtual_memory;
- erase_lhf00l04(flash); - if (chip_readb(bios) != 0xff) { - printf("ERASE FAILED!\n"); + if (erase_lhf00l04(flash)) { + fprintf(stderr, "ERASE FAILED!\n"); return -1; } printf("Programming page: "); Index: w39v040c.c =================================================================== --- w39v040c.c (revision 589) +++ w39v040c.c (working copy) @@ -60,16 +60,13 @@ { int i; unsigned int total_size = flash->total_size * 1024; - chipaddr bios = flash->virtual_memory;
- for (i = 0; i < total_size; i += flash->page_size) - erase_sector_jedec(flash->virtual_memory, i); - - for (i = 0; i < total_size; i++) - if (0xff != chip_readb(bios + i)) { - printf("ERASE FAILED at 0x%08x! Expected=0xff, Read=0x%02x\n", i, chip_readb(bios + i)); + for (i = 0; i < total_size; i += flash->page_size) { + if (erase_sector_jedec(flash->virtual_memory, i)) { + fprintf(stderr, "ERASE FAILED!\n"); return -1; } + }
return 0; } @@ -81,8 +78,10 @@ int page_size = flash->page_size; chipaddr bios = flash->virtual_memory;
- if (flash->erase(flash)) + if (flash->erase(flash)) { + fprintf(stderr, "ERASE FAILED!\n"); return -1; + }
printf("Programming page: "); for (i = 0; i < total_size / page_size; i++) { Index: stm50flw0x0x.c =================================================================== --- stm50flw0x0x.c (revision 589) +++ stm50flw0x0x.c (working copy) @@ -163,7 +163,6 @@ int erase_block_stm50flw0x0x(struct flashchip *flash, int offset) { chipaddr bios = flash->virtual_memory + offset; - int j;
// clear status register chip_writeb(0x50, bios); @@ -175,13 +174,10 @@
wait_stm50flw0x0x(flash->virtual_memory);
- for (j = 0; j < flash->page_size; j++) { - if (chip_readb(bios + j) != 0xFF) { - printf("Erase failed at 0x%x\n", offset + j); - return -1; - } + if (check_erased_range(flash, offset, flash->page_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; } - printf("DONE BLOCK 0x%x\n", offset);
return 0; @@ -230,24 +226,29 @@ */ int erase_stm50flw0x0x(struct flashchip *flash) { - int i, rc = 0; + int i; int total_size = flash->total_size * 1024; int page_size = flash->page_size; chipaddr bios = flash->virtual_memory;
printf("Erasing page:\n"); - for (i = 0; (i < total_size / page_size) && (rc == 0); i++) { + for (i = 0; i < total_size / page_size; i++) { 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("%04d at address: 0x%08x ", i, i * page_size); - rc = unlock_block_stm50flw0x0x(flash, i * page_size); - if (!rc) - rc = erase_block_stm50flw0x0x(flash, i * page_size); + if (unlock_block_stm50flw0x0x(flash, i * page_size)) { + fprintf(stderr, "UNLOCK FAILED!\n"); + return -1; + } + if (erase_block_stm50flw0x0x(flash, i * page_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } } printf("\n"); protect_stm50flw0x0x(bios);
- return rc; + return 0; }
int write_stm50flw0x0x(struct flashchip *flash, uint8_t * buf) Index: sst_fwhub.c =================================================================== --- sst_fwhub.c (revision 589) +++ sst_fwhub.c (working copy) @@ -104,7 +104,10 @@ return 1; }
- erase_block_jedec(flash->virtual_memory, offset); + if (erase_block_jedec(flash->virtual_memory, offset)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } toggle_ready_jedec(flash->virtual_memory);
return 0; @@ -114,15 +117,10 @@ { int i; unsigned int total_size = flash->total_size * 1024; - chipaddr bios = flash->virtual_memory;
- for (i = 0; i < total_size; i += flash->page_size) - erase_sst_fwhub_block(flash, i); - - // dumb check if erase was successful. - for (i = 0; i < total_size; i++) { - if (chip_readb(bios + i) != 0xff) { - printf("ERASE FAILED!\n"); + for (i = 0; i < total_size; i += flash->page_size) { + if (erase_sst_fwhub_block(flash, i)) { + fprintf(stderr, "ERASE FAILED!\n"); return -1; } } Index: am29f040b.c =================================================================== --- am29f040b.c (revision 589) +++ am29f040b.c (working copy) @@ -20,8 +20,11 @@
#include "flash.h"
-static int erase_sector_29f040b(chipaddr bios, unsigned long address) +static int erase_sector_29f040b(struct flashchip *flash, unsigned long address) { + int page_size = flash->page_size; + chipaddr bios = flash->virtual_memory; + chip_writeb(0xAA, bios + 0x555); chip_writeb(0x55, bios + 0x2AA); chip_writeb(0x80, bios + 0x555); @@ -34,6 +37,10 @@ /* wait for Toggle bit ready */ toggle_ready_jedec(bios + address);
+ if (check_erased_range(flash, address, page_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } return 0; }
@@ -86,6 +93,7 @@
int erase_29f040b(struct flashchip *flash) { + int total_size = flash->total_size * 1024; chipaddr bios = flash->virtual_memory;
chip_writeb(0xAA, bios + 0x555); @@ -98,6 +106,10 @@ programmer_delay(10); toggle_ready_jedec(bios);
+ if (check_erased_range(flash, 0, total_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } return 0; }
@@ -111,7 +123,10 @@ printf("Programming page "); for (i = 0; i < total_size / page_size; i++) { /* erase the page before programming */ - erase_sector_29f040b(bios, i * page_size); + if (erase_sector_29f040b(flash, i * page_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + }
/* write to the sector */ printf("%04d at address: ", i); Index: w39v080fa.c =================================================================== --- w39v080fa.c (revision 589) +++ w39v080fa.c (working copy) @@ -142,9 +142,10 @@ return 0; }
-static int erase_sector_winbond_fwhub(chipaddr bios, +static int erase_sector_winbond_fwhub(struct flashchip *flash, unsigned int sector) { + chipaddr bios = flash->virtual_memory; /* Remember: too much sleep can waste your day. */
printf("0x%08x\b\b\b\b\b\b\b\b\b\b", sector); @@ -161,30 +162,30 @@ /* wait for Toggle bit ready */ toggle_ready_jedec(bios);
+ if (check_erased_range(flash, sector, flash->page_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } return 0; }
int erase_winbond_fwhub(struct flashchip *flash) { int i, total_size = flash->total_size * 1024; - chipaddr bios = flash->virtual_memory;
unlock_winbond_fwhub(flash);
printf("Erasing: ");
- for (i = 0; i < total_size; i += flash->page_size) - erase_sector_winbond_fwhub(bios, i); - - printf("\n"); - - for (i = 0; i < total_size; i++) { - if (chip_readb(bios + i) != 0xff) { - fprintf(stderr, "Error: Flash chip erase failed at 0x%08x(0x%02x)\n", i, chip_readb(bios + i)); + for (i = 0; i < total_size; i += flash->page_size) { + if (erase_sector_winbond_fwhub(flash, i)) { + fprintf(stderr, "ERASE FAILED!\n"); return -1; } }
+ printf("\n"); + return 0; }
Index: flashrom.c =================================================================== --- flashrom.c (revision 589) +++ flashrom.c (working copy) @@ -205,6 +205,11 @@ return (a < b) ? a : b; }
+int max(int a, int b) +{ + return (a > b) ? a : b; +} + char *strcat_realloc(char *dest, const char *src) { dest = realloc(dest, strlen(dest) + strlen(src) + 1); @@ -245,6 +250,79 @@ return ret; }
+/* start is an offset to the base address of the flash chip */ +int check_erased_range(struct flashchip *flash, int start, int len) +{ + int rv; + uint8_t *cmpbuf = malloc(len); + + if (!cmpbuf) { + fprintf(stderr, "Could not allocate memory!\n"); + exit(1); + } + memset(cmpbuf, 0xff, len); + rv = verify_range(flash, cmpbuf, start, len, "ERASE"); + free(cmpbuf); + return rv; +} + +/* start is an offset to the base address of the flash chip */ +int verify_range(struct flashchip *flash, uint8_t *cmpbuf, int start, int len, char *message) +{ + int i, j, starthere, lenhere; + chipaddr bios = flash->virtual_memory; + int page_size = flash->page_size; + uint8_t *readbuf = malloc(page_size); + + if (!readbuf) { + fprintf(stderr, "Could not allocate memory!\n"); + exit(1); + } + if (start + len > flash->total_size * 1024) { + fprintf(stderr, "Error: %s called with start 0x%x + len 0x%x >" + " total_size 0x%x\n", __func__, start, len, + flash->total_size * 1024); + free(readbuf); + return -1; + } + if (!len) { + free(readbuf); + return 0; + } + if (!message) + message = "VERIFY"; + + /* Warning: This loop has a very unusual condition and body. + * The loop needs to go through each page with at least one affected + * byte. The lowest page number is (start / page_size) since that + * division rounds down. The highest page number we want is the page + * where the last byte of the range lives. That last byte has the + * address (start + len - 1), thus the highest page number is + * (start + len - 1) / page_size. Since we want to include that last + * page as well, the loop condition uses <=. + */ + for (i = start / page_size; i <= (start + len - 1) / page_size; i++) { + /* Byte position of the first byte in the range in this page. */ + starthere = max(start, i * page_size); + /* Length of bytes in the range in this page. */ + lenhere = min(start + len, (i + 1) * page_size) - starthere; + /* starthere is an offset to the base address of the chip. */ + chip_readn(readbuf, bios + starthere, lenhere); + for (j = 0; j < lenhere; j++) { + if (cmpbuf[starthere + j] != readbuf[j]) { + fprintf(stderr, "%s FAILED at 0x%08x! " + "Expected=0x%02x, Read=0x%02x\n", + message, starthere + j, + cmpbuf[starthere + j], readbuf[j]); + free(readbuf); + return -1; + } + } + } + free(readbuf); + return 0; +} + struct flashchip *probe_flash(struct flashchip *first_flash, int force) { struct flashchip *flash; Index: 82802ab.c =================================================================== --- 82802ab.c (revision 589) +++ 82802ab.c (working copy) @@ -110,7 +110,6 @@ { chipaddr bios = flash->virtual_memory + offset; chipaddr wrprotect = flash->virtual_registers + offset + 2; - int j; uint8_t status;
// clear status register @@ -129,11 +128,9 @@ // now let's see what the register is status = wait_82802ab(flash->virtual_memory); //print_82802ab_status(status); - for (j = 0; j < flash->page_size; j++) { - if (chip_readb(bios + j) != 0xFF) { - printf("BLOCK ERASE failed at 0x%x\n", offset); - return -1; - } + if (check_erased_range(flash, offset, flash->page_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; } printf("DONE BLOCK 0x%x\n", offset);
@@ -148,7 +145,10 @@ printf("total_size is %d; flash->page_size is %d\n", total_size, flash->page_size); for (i = 0; i < total_size; i += flash->page_size) - erase_82802ab_block(flash, i); + if (erase_82802ab_block(flash, i)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } printf("DONE ERASE\n");
return 0; @@ -199,7 +199,10 @@ }
/* erase block by block and write block by block; this is the most secure way */ - erase_82802ab_block(flash, i * page_size); + if (erase_82802ab_block(flash, i * page_size)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } write_page_82802ab(bios, buf + i * page_size, bios + i * page_size, page_size); }