Hi Michael:
I see your point, and see it can be confusing if the interpretation is changed. What about reading the whole erase regions affected by each interval? (like David points out) I mean this:
/** * Look for the starting address of the erase region that contains @addr. * The size of this region is returned in @sz */ int find_page_start(unsigned int addr, struct eraseblock *blocks, unsigned int *sz) { unsigned int page_start, i, size, count, group_size; /* Look for the beggining of the erase region containing start address */ page_start = 0; for (i = 0; i < NUM_ERASEREGIONS; i++) { size = blocks[i].size; count = blocks[i].count; group_size = size * count; if (page_start + group_size > addr) { /* The address is inside one of these erase regions */ *sz = size; return page_start + ((addr - page_start) / size) * size; } else {/* Not here */ page_start += group_size; } } msg_gerr("ERROR: address out of memory!?\n"); exit(1); return -1; }
/** * Read all the erase regions containing the @start-@end interval. * Returns the last address fetched in @real_end_p */ int read_pages(struct flashctx *flash, uint8_t *oldcontents, unsigned int start, unsigned int end, unsigned int *real_end_p) { int k; unsigned int real_start, real_end, size;
k = find_erase_function(flash); if (k == -1) { msg_gerr("ERROR: No erase function available.\n"); return -1; } real_start = find_page_start(start, flash->chip->block_erasers[k].eraseblocks, &size); msg_gdbg("Start of range 0x%X rounded to page boundary: 0x%X\n", start, real_start); real_end = find_page_start(end, flash->chip->block_erasers[k].eraseblocks, &size); real_end += size - 1; msg_gdbg("End of range 0x%X rounded to page boundary: 0x%X\n", end, real_end); *real_end_p = real_end; return flash->chip->read(flash, oldcontents + real_start, real_start, real_end - real_start + 1); }
/** * Fill the @oldcontents buffer with data from the memory. The regions not included by the user are filled * with data from @newcontents instead of the actual memory content. It forces to skip them. * This function is used only when @oldcontents is empty. * Is also mandatory that at least one region was specified by the user. */ int read_cur_content(struct flashctx *flash, uint8_t *oldcontents, const uint8_t *newcontents) { unsigned int start = 0; romentry_t *entry; unsigned int size = flash->chip->total_size * 1024; unsigned int region_end; int ret = 0;
/* We will read from memory only the regions indicated by the user. * The rest will contain the same as the newcontents */ memcpy(oldcontents, newcontents, size);
/* Catch a missuse */ if (num_include_args == 0) { msg_perr("Internal error! build_old_image() invoked, but no regions specified.\n"); return 1; }
/* Non-included romentries are ignored. * The union of all included romentries is fetched from memory. */ while (start < size) { entry = get_next_included_romentry(start); /* No more romentries for remaining region? */ if (!entry) break; /* For included regions, read memory content content. */ msg_gdbg("Read a chunk 0x%06x-0x%06x.\n", entry->start, entry->end); ret = read_pages(flash, oldcontents, entry->start, entry->end, ®ion_end); if (ret != 0) { msg_gerr("Failed to read chunk 0x%06x-0x%06x.\n", entry->start, entry->end); break; } start = region_end + 1; /* Catch overflow. */ if (!start) break; } return ret; }
The find_erase_function() is in flashrom.c and is like this:
/* Look for the first valid erase function. Returns -1 if none. */ int find_erase_function(struct flashctx *flash) { int k;
msg_cdbg("\nLooking for an erase function.\n"); for (k = 0; k < NUM_ERASEFUNCTIONS; k++) { if (k != 0) msg_cdbg("Looking for another erase function.\n"); msg_cdbg("Trying erase function %i... ", k); if (!check_block_eraser(flash, k, 1)) { msg_cdbg("OK\n"); return k; } msg_cdbg("INVALID\n"); } msg_cdbg("No usable erase functions left.\n"); return -1; }
In the case of the range I'm using I get: (with -VV)
... Reading old flash chip contents... Read a chunk 0x000000-0x007e2b.
Looking for an erase function. Trying erase function 0... OK Start of range 0x0 rounded to page boundary: 0x0 End of range 0x7E2B rounded to page boundary: 0x7FFF ...
In this way we know the content of the blocks that we'll erase. The values look ok, since the first eraser have 4k pages. Note: this code uses the first usable erase function, erase_and_write_flash will try the same eraser. If this eraser fails we will need more data, but in this case erase_and_write_flash reads all the memory again.
Regards, SET
El 20/04/16 a las 15:23, Michael Karcher escribió:
On 04/20/2016 12:07 PM, Salvador Eduardo Tropea wrote:
/**
- Fill the @oldcontents buffer with data from the memory. The regions
not included by the user are filled
- with data from @newcontents instead of the actual memory content.
It forces to skip them.
- This function is used only when @oldcontents is empty.
- Is also mandatory that at least one region was specified by the user.
*/
My intention was to use the same (nasty) tricks that the code already uses. By forcing areas to match the rest of the code avoids touching it.
My concern is still valid. The claim "It forces to skip them" appears wrong. If the area of the new and the old image match, the code tries to optimize by not touching the area, but there is no guarantee that the area is indeed untouched. If the old contents of the chip has been read, flashrom "hides" the fact that it touched an area that did not need to be touched by reflashing the old contents after erasing. This hiding breaks down if flashrom doesn't know the real contents.
Here is an example of what I mean: Assume a flash chip containing four nibbles (used instead of bytes for presentation purposes; nibbles are represented in hex digits); the chip contains two independently erasable areas of two nibbles each, and I use a layout file like this (the erased state is the nibble value F).
0:2 please_flash 3:3 please_keep
Assume the flash chip contains 0127, and the new image contains 1230. Without -A, and selecting the image "please_flash" the following steps are taken:
- The file is loaded, newcontents is "1230".
- ROM contents are read. oldcontents is "0127"
- build_new_image/modify_new_image_from_old copies the last nibble from
oldcontents to newcontents, because it is in "please_keep", not in "please_flash". newcontents is now "1237" 4) The first two nibbles are erased, because the old value "01" does not match "12". Now they are "FF" 5) The first two nibbles are programmed to "12" 6) The second *two* nibbles are erased because "27" does not match "37". Now they are "FF" 7) The second two nibbles are programmed to "37". The flash chip now contains 1237.
With -A, the following steps are taken:
- The file is loaded, newcontents is "1230"
- read_cur_content loads the first three nibbles from the flash memory,
and takes the last one from newcontents. oldcontents is "0120". 3) The first two nibbles are erased, because the old value "01" does not match "12". Now they are "FF" 4) The first two nibbles are programmed to "12" 5) The second *two* nibbles are erased because "20" does not match "30". Now they are "FF" 6) The second two nibbles are programmed to "30". The flash chip now contains 1230.
As you see, the last nibble is modified to "0" instead of being kept at "7". There might be situations in which it doesn't matter that a nibble outside of the requested area is changed, but it definitely is different from how we handled layout files and partial flashing up to now. Note especially how copying the last nibble (the zero with -A, the 7 without -A) from newcontents to oldcontents did not force the nibble to be untouched in either case, but in the first case flashrom knew what to write so the 7 appears to be unmodified while in fact it was erased and rewritten.
Regards, Michael Karcher