Hi All!
Now I'm using an FPGA board with 4 MiB of flash, was using one with 128 kiB. So now the time to read/write/verify the whole memory is enough to annoy me ;-) So I tried creating a layout like this:
<-- 00000000:00007e2b fpga 00007e2c:003fffff free <--
Note: yes only 32300 bytes used, the rest can be used for other purposes (0,77%).
And then I used "-i fpga". But it didn't help ... flashrom read the 4 MiB twice (one at the beggining and another for the verification). Looking at the code I found a variable named read_all_first with a comment about the lack of implementation. So I implemented the needed code to: 1) Add a command line option to make it 0 (avoiding the big reads). This option can be used only when at least one image is indicated with -i 2) Skip the big read when read_all_first is 0. Only the regions indicated by the user are actually fetched from the memory. 3) Same for the verification stage.
Additionally I added a command line option to show the progress, very usefull for big memories (and impatient users ;-).
Are these additions desired in the project? If yes: I want to discuss the implementation details. If no: I'll just keep them for my use.
Regards, SET
On 04/19/2016 07:48 PM, Salvador Eduardo Tropea wrote:
Hi All!
Now I'm using an FPGA board with 4 MiB of flash, was using one with 128 kiB. So now the time to read/write/verify the whole memory is enough to annoy me ;-) So I tried creating a layout like this:
<-- 00000000:00007e2b fpga 00007e2c:003fffff free <--
Note: yes only 32300 bytes used, the rest can be used for other purposes (0,77%).
And then I used "-i fpga". But it didn't help ... flashrom read the 4 MiB twice (one at the beggining and another for the verification). Looking at the code I found a variable named read_all_first with a comment about the lack of implementation. So I implemented the needed code to:
- Add a command line option to make it 0 (avoiding the big reads).
This option can be used only when at least one image is indicated with -i 2) Skip the big read when read_all_first is 0. Only the regions indicated by the user are actually fetched from the memory. 3) Same for the verification stage.
Just a quick question: How does this mix with range erasure? Most likely, I can't erase 0-7e2b, but I have to erase 0-8000, or even the whole chip. If the chip was not read before erasing, the contents of those areas get destroyed. There are use cases for "-i" with the intention to keep everything in the chip intact except for the selected address range. What you need is a way to differentiate between "must not change" and "I don't care" areas, but the current layout system provides no way to do that. Traditionally "-i" is interpreted as declaring everything outside as "must not change", but your description sound like your patch changes the interpretation to "I don't care".
Regards, Michael Karcher
Hi Michael!
My patch modifies the buil_new_image function. The function is split in two cases:
1) The default case is when read_all_first=1. In this case the behavior should exactly the same as without the patch. In this case the function that replaces build_new_image is called modify_new_image_from_old. This function should behave as the old buil_new_image invoked with old_image_valid = 1. So this function is some kind of filter that modifies the content of "newcontents" using information from "oldcontents". Nothing new, just clearer code because it explicitly use memcpy (we know that olcontents contains a copy of the actual memory). 2) When read_all_first=0 (command line --no-read-all or -A) the beahvior changes. If the user specifies -A, but doesn't specify at least one image with -i an error is reported indicating that at least one image must be specified. In this case the buil_new_image function is replaced by "read_cur_content". Here is the comment I put on the code:
/** * 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. Also note that the change in the behavior is selected using -A, so you should get the same old behavior by default. Of course, I could introduced some bug, I'm not perfect. But my intention was to change the behavior only when the user asked for it. IMHO the default should be read_all_first=0, but this will confuse old users. Here are the two functions that replace buil_new_image:
/** * Modify @newcontents so that it contains the data that should be on the chip eventually. In the case the user * wants to update only parts of it, copy the chunks to be preserved from @oldcontents to @newcontents. * This function is used only when @oldcontents contains the full memory information. */ void modify_new_image_from_old(struct flashctx *flash, const uint8_t *oldcontents, uint8_t *newcontents) { unsigned int start = 0; romentry_t *entry; unsigned int size = flash->chip->total_size * 1024;
/* If no regions were specified for inclusion, assume * that the user wants to write the complete new image. */ if (num_include_args == 0) return;
/* Non-included romentries are ignored. * The union of all included romentries is used from the new image. */ while (start < size) { entry = get_next_included_romentry(start); /* No more romentries for remaining region? */ if (!entry) { /* Copy the old content to skip it */ memcpy(newcontents + start, oldcontents + start, size); break; } /* For non-included region, copy from old content. */ if (entry->start > start) memcpy(newcontents + start, oldcontents + start, entry->start - start); /* Skip to location after current romentry. The user wants to preserve it. */ start = entry->end + 1; /* Catch overflow. */ if (!start) break; } return; }
/** * 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_size; 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. */ if (entry->start == start) { msg_gdbg2("Read a chunk 0x%06x-0x%06x.\n", start, entry->end); region_size = entry->end - start + 1; ret = flash->chip->read(flash, oldcontents + start, start, region_size); if (ret != 0) { msg_gerr("Failed to read chunk 0x%06x-0x%06x.\n", start, entry->end); break; } } /* Skip to location after current romentry. */ start = entry->end + 1; /* Catch overflow. */ if (!start) break; } return ret; }
Hope that now you have a better idea of what I'm talking about.
Regards, SET
El 19/04/16 a las 17:57, Michael Karcher escribió:
On 04/19/2016 07:48 PM, Salvador Eduardo Tropea wrote:
Hi All!
Now I'm using an FPGA board with 4 MiB of flash, was using one with 128 kiB. So now the time to read/write/verify the whole memory is enough to annoy me ;-) So I tried creating a layout like this:
<-- 00000000:00007e2b fpga 00007e2c:003fffff free <--
Note: yes only 32300 bytes used, the rest can be used for other purposes (0,77%).
And then I used "-i fpga". But it didn't help ... flashrom read the 4 MiB twice (one at the beggining and another for the verification). Looking at the code I found a variable named read_all_first with a comment about the lack of implementation. So I implemented the needed code to:
- Add a command line option to make it 0 (avoiding the big reads).
This option can be used only when at least one image is indicated with -i 2) Skip the big read when read_all_first is 0. Only the regions indicated by the user are actually fetched from the memory. 3) Same for the verification stage.
Just a quick question: How does this mix with range erasure? Most likely, I can't erase 0-7e2b, but I have to erase 0-8000, or even the whole chip. If the chip was not read before erasing, the contents of those areas get destroyed. There are use cases for "-i" with the intention to keep everything in the chip intact except for the selected address range. What you need is a way to differentiate between "must not change" and "I don't care" areas, but the current layout system provides no way to do that. Traditionally "-i" is interpreted as declaring everything outside as "must not change", but your description sound like your patch changes the interpretation to "I don't care".
Regards, Michael Karcher
flashrom mailing list flashrom@flashrom.org https://www.flashrom.org/mailman/listinfo/flashrom
Hello Salvador, Yes, this is a very useful feature - we've had it in the chromiumos branch for a while now :-)
I need to read your implementation. Ours is called "fast-verify" which will read and only verify portions of the flash specified with -i arguments.
It will also restore content surrounding regions (to address Michael's concern) by aligning the bytes read with the erase size. So if you wish to change a small amount of content within an eraseable block, the entire block will be read and erased. The region specified with -i will be written with new content, and the remaining bytes within the block will have the old content restored.
Here's our tree in case you'd like to take a look: https://chromium.googlesource.com/chromiumos/third_party/flashrom/
On Tue, Apr 19, 2016 at 10:48 AM, Salvador Eduardo Tropea < salvador@inti.gob.ar> wrote:
Hi All!
Now I'm using an FPGA board with 4 MiB of flash, was using one with 128 kiB. So now the time to read/write/verify the whole memory is enough to annoy me ;-) So I tried creating a layout like this:
<-- 00000000:00007e2b fpga 00007e2c:003fffff free <--
Note: yes only 32300 bytes used, the rest can be used for other purposes (0,77%).
And then I used "-i fpga". But it didn't help ... flashrom read the 4 MiB twice (one at the beggining and another for the verification). Looking at the code I found a variable named read_all_first with a comment about the lack of implementation. So I implemented the needed code to:
- Add a command line option to make it 0 (avoiding the big reads). This
option can be used only when at least one image is indicated with -i 2) Skip the big read when read_all_first is 0. Only the regions indicated by the user are actually fetched from the memory. 3) Same for the verification stage.
Additionally I added a command line option to show the progress, very usefull for big memories (and impatient users ;-).
Are these additions desired in the project? If yes: I want to discuss the implementation details. If no: I'll just keep them for my use.
Regards, SET
-- Ing. Salvador Eduardo Tropea http://utic.inti.gob.ar/ INTI - Micro y Nanoelectrónica (CMNB) http://www.inti.gob.ar/ Unidad Técnica Sistemas Inteligentes Av. General Paz 5445 Tel: (+54 11) 4724 6300 ext. 6919 San Martín - B1650KNA FAX: (+54 11) 4754 5194 Buenos Aires * Argentina
flashrom mailing list flashrom@flashrom.org https://www.flashrom.org/mailman/listinfo/flashrom
-
David Hendricks -
Michael Karcher -
Salvador Eduardo Tropea