Am 15.09.2013 20:14 schrieb Stefan Tauner:
On Sun, 15 Sep 2013 04:15:44 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 12.09.2013 22:40 schrieb Stefan Tauner:
Add an optional sub-parameter to the -i parameter to allow building the image to be written from multiple files. This will also allow regions to be read from flash and written to separate image files in a later patch. Document the whole layout handling including the new features a bit better.
based on chromiumos' d0ea9ed71e7f86bb8e8db2ca7c32a96de25343d8 Signed-off-by: David Hendricks dhendrix@chromium.org Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
First review round, man page only. I want to get our terms right before the existing confusion gets worse.
Code review follows in a separate mail.
flashrom.8.tmpl | 47 +++++++++++++-------------- flashrom.c | 8 +++-- layout.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++------- 3 files changed, 116 insertions(+), 37 deletions(-)
diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index 5ede423..b1371e4 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -6,7 +6,7 @@ flashrom - detect, read, write, verify and erase flash chips \fB-p\fR <programmername>[:<parameters>] [\fB-E\fR|\fB-r\fR <file>|\fB-w\fR <file>|\fB-v\fR <file>] \ [\fB-c\fR <chipname>]
[\fB\-l\fR <file> [\fB\-i\fR <image>]] [\fB\-n\fR] [\fB\-f\fR]]
[\fB\-l\fR <file> [\fB\-i\fR <image>[:<file>]]...] [\fB\-n\fR] [\fB\-f\fR]]The naming conflict between "image" (region) and "image file" (the file we write to or read from the flash chip) is very unfortunate. Unfortunately, we also lack good free single letters for a better name. While --region (instead of --image) would be a really good choice for the long option name, -r (read) and -R (revision/version) are already used and -i is an admittedly difficult association for --region.
hm. I have always read the -i as include, but in cli_classic it is really --image. Let's change that to -i/--include <region>:<file>?
Agreed. The time since we supported --include (with a totally different meaning) is long enough to reuse that name.
The new syntax also has one catch: You can't specify a separate file for a region without having that region actively written/read. While this won't be a problem for ordinary users, it might pose a problem for vendors like Google who want to use flashrom inside their firmware update process. Such update processes (at least when I look at the processes of other vendors) often do specify all files to source an image from, but they desire to select the to-be-written parts of the flash separately. Not sure if there is any practical relevance to this observation, though. David?
My 2 cents: since we define the regions in a separate file I dont see a problem with this at all. What would be gained by having another "enable" flag or whatever?
[\fB\-V\fR[\fBV\fR[\fBV\fR]]] [\fB-o\fR <logfile>].SH DESCRIPTION .B flashrom @@ -103,44 +103,45 @@ size for the flash bus.
- Force write even if write is known bad.
.TP .B "-l, --layout <file>" -Read ROM layout from +Read layout entries from
Do we want to call them "ROM layout", "flash layout" or simply "layout"? My preferences are 1. "ROM layout" 2. "flash layout".
I dont like the word ROM in there at all. It is actually untrue in context of flashrom and the concept behind the whole layout thing (i.e. address ranges with attributes) are not specific to them at all. IMHO ROM layout implies some kind of chip-specific partitioning scheme. Please give some rationale of your preferences.
Think of "ROM" as generic name for an image file of the contents of some flash/ROM chip. Just google for "ROMs" and you'll see what I mean.
.BR <file> . .sp -flashrom supports ROM layouts. This allows you to flash certain parts of -the flash chip only. A ROM layout file contains multiple lines with the -following syntax: +A layout entry describes an address region of the flash chip and gives it a name. This allows to access certain +parts of the flash chip only. A layout file can contain multiple entries one per line with the following syntax:
I am not a native speaker, but the sentences above look odd. Suggested rewording :
flashrom supports reading/writing/erasing flash chips in whole (default) or in part (when a ROM layout is given).
That is actually not true. Giving the layout alone does not make flashrom access only parts. Arguably "supporting" does not necessarily imply that it *does* so, but I would rather not write it like that in the manual.
What about "[...] (when a ROM layout file is given and certain other conditions are met)."
Layouts are stored in layout files. A layout entry describes an address region/range in the virtual file representing the flash chip contents and assigns a name to that region. A layout file consists of one or more lines with the following syntax:
.sp -.B " startaddr:endaddr imagename" +.B " startaddr:endaddr regionname" .sp .BR "startaddr " "and " "endaddr " -are hexadecimal addresses within the ROM file and do not refer to any -physical address. Please note that using a 0x prefix for those hexadecimal +are hexadecimal addresses within the ROM file representing the flash ROM contents.
are hexadecimal addresses within the virtual file representing the flash chip contents
So... "virtual file representing the flash chip contents" seems to be the new wording for... that. We should try to come up with a shorter way to refer to that. (VFRTFCC hm... nope. :)
"ROM image"? "ROM file"? I do prefer "ROM image". Suggested explanation to be added somewhere in the man page: "For historical reasons, the contents read from or written to a flash chip are called ROM image. Addresses within a ROM image start at 0 regardless of the address where the flash chip is mapped in hardware."
.sp -If you only want to update the image named -.BR "normal " "in a ROM based on the layout above, run" -.sp -.B " flashrom -p prog --layout rom.layout --image normal -w some.rom" +.TP +.B "-i, --image <name>[:<file>]" +Work on the flash region +.B name +instead of the full address space if a layout file is given and parsed correctly. +Multiple such image parameters can be used to include the union of different regions.
s/include/work on/
ok
+The optional +.B file +parameter specifies the name of an image file that is used to map the contents of that very region only.
"image file" naming again.
+This is useful for example if you want to write only a part of the flash chip and leave everything else alone +without preparing an image of the complete chip manually:
Uhh... sounds denglish. Suggested rewording: This allows replacing part of a ROM file with contents from another file before writing the resulting contents to the flash chip, eliminating the separate step of manually assembling a combined ROM image.
You seem to imply that the -i parameters do only overwrite parts of the file given by -w. While this is true for the current implementation, David and I discussed about making the file parameter after -w optional.
Ouch! I think that is a really really bad idea. I can see reasons to avoid full image files, but it feels wrong to have undefined regions in the image. Especially if writing fails and recovery is needed.
Also, your sentence makes it sound as if the *file* is modified instead of the flash chip.
Yes, my bad. Let's finish the terminology discussion first.
+.BR "normal " "and " "fallback " "in a ROM based on the layout mentioned above, run" .sp .B " flashrom -p prog -l rom.layout -i normal -i fallback -w some.rom" .sp -Overlapping sections are not supported. -.TP -.B "-i, --image <imagename>" -Only flash region/image -.B <imagename> -from flash layout. +.RB "Overlapping sections are resolved in an implementation-dependent manner - do " "not " "rely on it."
I reserve the right to have flashrom error out on overlapping regions unless you can convince me such an error would be a bad idea. Suggested rewording: .RB "Overlapping region definitions are resolved in an implementation-dependent manner or may yield an error - do " "not " "rely on any observed flashrom behaviour."
Redundant. Bailing out is a valid implementation-dependent resolution strategy ;) But I can add that for clarification...
"resolved" implied success for me.
Please let us decide on the exact names for images, layouts, layout entries, their names etc. and their respective API parameters before discussing any complete sentences in the manual.
Agreed.
Regards, Carl-Daniel