Development Guidelines

Patch submission

The following guidelines are for coreboot, but most of them apply to flashrom as well: coreboot development guidelines. The really important part is about the Signed-off-by procedure.

We try to reuse as much code as possible and create new files only if absolutely needed, so if you find a function somewhere in the tree which already does what you want (even if it is for a totally different chip), please use it. Most chips work fine with probe_jedec() even if the command sequence seems to differ at first glance. See also Command set secrets.

The patch reviews may sound harsh, but please don't get discouraged. We try to merge simple patches after one or two iterations and complicated ones after a maximum of three iterations.

If you introduce new features (not flash chips, but stuff like partial programming, support for new external programmers, voltage handling, etc) please discuss your plans on the mailing list first. That way, we can avoid duplicated work and know about how flashrom internals need to be adjusted and you avoid frustration if there is some disagreement about the design.

For patches that modify convoluted tables like struct flashchip flashchips[] in flashchips.c it may make sense to increase the lines of context to include enough information directly in the patch for reviewers (for example to include the chip names when changing other parameters like .voltage). To do this with subversion use svn diff --diff-cmd diff -x -u5; or with git use git format-patch -U5 where 5 is an example for the number of lines of context you want.

Recurring Tasks

Adding/reviewing a new flash chip

1. Get the datasheet of the exact type of chip.
2. Open flashchips.c and flashchips.h.
3. First, find the best* IDs in the datasheet (*FIXME: this needs to be explained together with the probing somewhere else in detail) and check if the ID exists in flashchips.h already
   • If it does but is named after a different chip,

     then add a comment regarding the twin and continue by comparing the definition in flashchips.c with the datasheet of the twin/new chip as if you would add it but leave out the next step (see below). First you should change the .name to reflect the additional chip model (see other chips of naming examples). If you find significant* differences in the chips behevior you have found a so called evil twin (*judging the significance of a difference is quite hard and requires some understanding of flashrom behavior, examples of significant differences are: different sizes of blocks or different opcodes for operations). In that case copy the entry and continue to change that (don't forget to undo the previous changes before).

   • If it does and the name matches too,

     the chip is either already added or only the ID was added and you should use that define

   • If it does not,

     then you should add it conforming to the standards/comments in the file

4. If possible copy an existing, similar entry in the giant array in flashchips.c or start a new one at the right position (according to the comment on top of the array)
5. Add .vendor, .name, IDs selected as explained above and .total_size.
6. .page_size is really hard. Please read this long explanation, or ignore it for now and set it to 256.
7. We encode various features of flash chips in a bitmask named .feature_bits. The various possibilities can be found in flash.h.
8. .tested is used to indicate if the code was tested with real hardware, its possible values are defined in flash.h. Without any tests it should be set to TEST_UNTESTED.
9. .probe indicates which function is called to fetch IDs from the chip and to compare them with the ones in .manufacture_id and .model_id. This requires some knowledge or source reading. For most SPI flash chips probe_spi_rdid is the right one if the datasheets mentions 0x9f as an identification/probing opcode.
10. .probe_timing is only used for non-SPI chips. It indicates the delay after "enter/exit ID mode" commands in microseconds (see flash.h for special values).
11. .block_erasers stores an array of pairs of erase functions (.block_erase) with their respective layout (.eraseblocks).
    1. .block_erase is similar to the probing function. You should at least check that the opcode named in the function name is matching the respective opcode in the datasheet.
    2. Two forms of .eraseblocks can be distinguished: symmetric and asymmetric layouts. Symmetric means that all blocks that can be erased by an opcode are sized equal. In that case a single range can define the whole layout (e.g. {4 * 1024, 256} means 256 blocks of 4 kB each). Asymmetric layouts on the other hand contain differently sized blocks, ordered by their base addresses (e.g. {8 * 1024, 1}, {4 * 1024, 2}, {16 * 1024, 7} describes a layout that starts with a single 8 kB block, followed by two 4 kB blocks and 7 16 kB blocks at the end).

Committing

Those with commit rights to the (currently subversion-based) source repository should follow the following rules.

• Existence of an Acked-by
  • Except for compile fixes and marking boards/chips/programmers as OK, you need an ack from someone else.
  • If the patch is needed by someone who is not an experienced developer, you can get an ack from him/her if it is stuff like board enables, chip support, chipset support, ... (no refactoring, no architectural impact) after he/she has tested the patch.
  • However, if a patch refactors an interface or changes cosmetics of one of the big tables or changes the command line interface, you should get a review from someone who knows the code well.
• If you ack something and the sender has commit rights, let him commit unless he asks you to commit.
• The commit message should meet the same requirements that coreboot's commit message have.
• After committing please reply to the mailing thread that contains the patch with at least the revision in the body to notify anyone involved of the commit and document it for references in the future and in patchwork or similar facilities.
• Don't share your login(s)/password(s) (should be obvious, but sadly it is not for everyone :)