Am 15.09.2013 20:14 schrieb Stefan Tauner: > On Sun, 15 Sep 2013 04:15:44 +0200 > Carl-Daniel Hailfinger <[email protected]> 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 <[email protected]> >>> Signed-off-by: Stefan Tauner <[email protected]> >>> >>> Signed-off-by: Stefan Tauner <[email protected]> >> 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 -- http://www.hailfinger.org/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
