On 21.10.2010 02:00, Andrew Morgan wrote: > On 20/10/10 05:06, Carl-Daniel Hailfinger wrote: >> If you're going to test this, please be advised that I have not tested >> execution, only compilation. Testing erase (and checking with a separate >> readback that erase actually worked) and write (same test with separate >> readback) would be highly appreciated. Verbose logs are even more >> appreciated. > Write seems work, but erase on its own doesn't. Some blocks aren't > erased: (good = erased, bad = not erased) > > 0x000000-0x000fff, 0x001000-0x001fff, good > 0x002000-0x002fff, bad > 0x003000-0x003fff, 0x004000-0x004fff, 0x005000-0x005fff, good > 0x006000-0x006fff, bad > 0x007000-0x007fff, good > 0x008000-0x008fff, bad
I suspect that there are more bugs in the code than I'd like. Please try the patch below which has more debug output. Signed-off-by: Carl-Daniel Hailfinger <[email protected]> Index: flashrom-partial_write_rolling_erase_write/flashrom.c =================================================================== --- flashrom-partial_write_rolling_erase_write/flashrom.c (Revision 1215) +++ flashrom-partial_write_rolling_erase_write/flashrom.c (Arbeitskopie) @@ -793,6 +793,7 @@ * Check if the buffer @have can be programmed to the content of @want without * erasing. This is only possible if all chunks of size @gran are either kept * as-is or changed from an all-ones state to any other state. + * * The following write granularities (enum @gran) are known: * - 1 bit. Each bit can be cleared individually. * - 1 byte. A byte can be written once. Further writes to an already written @@ -803,10 +804,12 @@ * this function. * - 256 bytes. If less than 256 bytes are written, the contents of the * unwritten bytes are undefined. + * Warning: This function assumes that @have and @want point to naturally + * aligned regions. * * @have buffer with current content * @want buffer with desired content - * @len length of the verified area + * @len length of the checked area * @gran write granularity (enum, not count) * @return 0 if no erase is needed, 1 otherwise */ @@ -846,10 +849,102 @@ break; } break; + default: + msg_cerr("%s: Unsupported granularity! Please report a bug at " + "[email protected]\n", __func__); } return result; } +/** + * Check if the buffer @have needs to be programmed to get the content of @want. + * If yes, return 1 and fill in first_start with the start address of the + * write operation and first_len with the length of the first to-be-written + * chunk. If not, return 0 and leave first_start and first_len undefined. + * + * Warning: This function assumes that @have and @want point to naturally + * aligned regions. + * + * @have buffer with current content + * @want buffer with desired content + * @len length of the checked area + * @gran write granularity (enum, not count) + * @return 0 if no write is needed, 1 otherwise + * @first_start offset of the first byte which needs to be written + * @first_len length of the first contiguous area which needs to be written + * + * FIXME: This function needs a parameter which tells it about coalescing + * in relation to the max write length of the programmer and the max write + * length of the chip. + */ +static int get_next_write(uint8_t *have, uint8_t *want, int len, + int *first_start, int *first_len, + enum write_granularity gran) +{ + int result = 0; + int i, j, limit; + + /* The passed in variable might be uninitialized. */ + *first_len = 0; + switch (gran) { + case write_gran_1bit: + case write_gran_1byte: + for (i = 0; i < len; i++) { + if (have[i] != want[i]) { + if (!result) { + /* First location where have and want + * differ. + */ + result = 1; + *first_start = i; + } + } else { + if (result) { + /* First location where have and want + * do not differ anymore. + */ + *first_len = i - *first_start; + break; + } + } + } + /* Did the loop terminate without setting first_len? */ + if (result && ! *first_len) + *first_len = i - *first_start; + break; + case write_gran_256bytes: + for (j = 0; j < len / 256; j++) { + limit = min(256, len - j * 256); + /* Are 'have' and 'want' identical? */ + if (memcmp(have + j * 256, want + j * 256, limit)) { + if (!result) { + /* First location where have and want + * differ. + */ + result = 1; + *first_start = j * 256; + } + } else { + if (result) { + /* First location where have and want + * do not differ anymore. + */ + *first_len = j * 256 - *first_start; + break; + } + } + } + /* Did the loop terminate without setting first_len? */ + if (result && ! *first_len) + *first_len = min(j * 256 - *first_start, len); + break; + default: + msg_cerr("%s: Unsupported granularity! Please report a bug at " + "[email protected]\n", __func__); + } + return result; +} + /* This function generates various test patterns useful for testing controller * and chip communication as well as chip behaviour. * @@ -1203,7 +1298,8 @@ return ret; } -/* This function shares a lot of its structure with erase_flash(). +/* This function shares a lot of its structure with erase_and_write_flash() and + * walk_eraseregions(). * Even if an error is found, the function will keep going and check the rest. */ static int selfcheck_eraseblocks(struct flashchip *flash) @@ -1271,10 +1367,59 @@ return ret; } +static int erase_and_write_block_helper(struct flashchip *flash, + unsigned int start, unsigned int len, + uint8_t *oldcontents, + uint8_t *newcontents, + int (*erasefn) (struct flashchip *flash, + unsigned int addr, + unsigned int len)) +{ + int starthere = 0; + int lenhere = 0; + int ret = 0; + enum write_granularity gran = write_gran_256bytes; /* FIXME */ + + /* oldcontents and newcontents are opaque to walk_eraseregions, and + * need to be adjusted here to keep the impression of proper abstraction + */ + oldcontents += start; + newcontents += start; + /* FIXME: Assume 256 byte granularity for now to play it safe. */ + if (need_erase(oldcontents, newcontents, len, gran)) { + msg_cdbg(" E"); + ret = erasefn(flash, start, len); + if (ret) + return ret; + /* Erase was successful. Adjust oldcontents. */ + memset(oldcontents + start, 0xff, len); + } else + msg_cdbg(" e"); + while (get_next_write(oldcontents + starthere, + newcontents + starthere, + len - starthere, &starthere, + &lenhere, gran)) { + msg_cdbg("W"); + /* Needs the partial write function signature. */ + ret = flash->write(flash, newcontents + starthere, start + starthere, lenhere); + if (ret) + break; + starthere += lenhere; + } + return ret; +} + static int walk_eraseregions(struct flashchip *flash, int erasefunction, int (*do_something) (struct flashchip *flash, unsigned int addr, - unsigned int len)) + unsigned int len, + uint8_t *param1, + uint8_t *param2, + int (*erasefn) ( + struct flashchip *flash, + unsigned int addr, + unsigned int len)), + void *param1, void *param2) { int i, j; unsigned int start = 0; @@ -1286,21 +1431,22 @@ */ len = eraser.eraseblocks[i].size; for (j = 0; j < eraser.eraseblocks[i].count; j++) { - msg_cdbg("0x%06x-0x%06x, ", start, + msg_cdbg("0x%06x-0x%06x", start, start + len - 1); - if (do_something(flash, start, len)) + if (do_something(flash, start, len, param1, param2, eraser.block_erase)) return 1; + msg_cdbg(", "); start += len; } } return 0; } -int erase_flash(struct flashchip *flash) +int erase_and_write_flash(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents) { int k, ret = 0, found = 0; - msg_cinfo("Erasing flash chip... "); + msg_cinfo("Erasing and writing flash chip... "); for (k = 0; k < NUM_ERASEFUNCTIONS; k++) { struct block_eraser eraser = flash->block_erasers[k]; @@ -1324,7 +1470,7 @@ } found = 1; msg_cdbg("trying... "); - ret = walk_eraseregions(flash, k, eraser.block_erase); + ret = walk_eraseregions(flash, k, &erase_and_write_block_helper, oldcontents, newcontents); msg_cdbg("\n"); /* If everything is OK, don't try another erase function. */ if (!ret) @@ -1637,6 +1783,19 @@ programmer_shutdown(); return ret; } + + oldcontents = (uint8_t *) malloc(size); + /* Assume worst case: All bits are 0. */ + memset(oldcontents, 0x00, size); + newcontents = (uint8_t *) malloc(size); + /* Assume best case: All bits should be 1. */ + memset(newcontents, 0xff, size); + /* Side effect of the assumptions above: Default write action is erase + * because newcontents looks like a completely erased chip, and + * oldcontents being completely 0x00 means we have to erase everything + * before we can write. + */ + if (erase_it) { /* FIXME: Do we really want the scary warning if erase failed? * After all, after erase the chip is either blank or partially @@ -1644,15 +1803,13 @@ * so if the user wanted erase and reboots afterwards, the user * knows very well that booting won't work. */ - if (erase_flash(flash)) { + if (erase_and_write_flash(flash, oldcontents, newcontents)) { emergency_help_message(); programmer_shutdown(); return 1; } } - newcontents = (uint8_t *) calloc(size, sizeof(char)); - if (write_it || verify_it) { if (read_buf_from_file(newcontents, size, filename)) { programmer_shutdown(); @@ -1665,8 +1822,6 @@ #endif } - oldcontents = (uint8_t *) calloc(size, sizeof(char)); - /* Read the whole chip to be able to check whether regions need to be * erased and to give better diagnostics in case write fails. * The alternative would be to read only the regions which are to be @@ -1686,9 +1841,9 @@ // //////////////////////////////////////////////////////////// if (write_it) { - if (erase_flash(flash)) { - msg_cerr("Uh oh. Erase failed. Checking if anything " - "changed.\n"); + if (erase_and_write_flash(flash, oldcontents, newcontents)) { + msg_cerr("Uh oh. Erase/write failed. Checking if " + "anything changed.\n"); if (!flash->read(flash, newcontents, 0, size)) { if (!memcmp(oldcontents, newcontents, size)) { msg_cinfo("Good. It seems nothing was " @@ -1701,24 +1856,6 @@ emergency_help_message(); programmer_shutdown(); return 1; - } - msg_cinfo("Writing flash chip... "); - ret = flash->write(flash, newcontents, 0, size); - if (ret) { - msg_cerr("Uh oh. Write failed. Checking if anything " - "changed.\n"); - if (!flash->read(flash, newcontents, 0, size)) { - if (!memcmp(oldcontents, newcontents, size)) { - msg_cinfo("Good. It seems nothing was " - "changed.\n"); - nonfatal_help_message(); - programmer_shutdown(); - return 1; - } - } - emergency_help_message(); - programmer_shutdown(); - return 1; } else { msg_cinfo("COMPLETE.\n"); } -- http://www.hailfinger.org/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
