[flashrom] [PATCH 2/4] layout: Add -i <image>[:<file>] support.

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Sun Sep 15 04:15:44 CEST 2013


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 at chromium.org>
> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
>
> Signed-off-by: Stefan Tauner <stefan.tauner at 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.

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?


>           [\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".


>  .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). 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

> +Please note that using a 0x prefix for those hexadecimal
>  numbers is not necessary, but you can't specify decimal/octal numbers.
> -.BR "imagename " "is an arbitrary name for the region/image from"
> -.BR " startaddr " "to " "endaddr " "(both addresses included)."
> +.BR "regionname " "is an arbitrary name for the region from "
> +.BR "startaddr " "to " "endaddr " "(both addresses included)."
>  .sp
> -Example:
> +Example content of file rom.layout:
>  .sp
>    00000000:00008fff gfxrom
>    00009000:0003ffff normal
>    00040000:0007ffff fallback

We should use 0x prefixes in the example. That helps readability.


>  .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/


> +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.


>  .sp
> -To update only the images named
> -.BR "normal " "and " "fallback" ", run:"
> +If you only want to update the image named

Plural from "images" was removed by accident.


> +.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."


>  .TP
>  .B "\-L, \-\-list\-supported"
>  List the flash chips, chipsets, mainboards, and external programmers


Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/





More information about the flashrom mailing list