Peter Marheine has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/74923 )
Change subject: doc: add information about contributing patches ......................................................................
doc: add information about contributing patches
This is copied from the "Development Guidelines" page on the wiki, with some unimportant historical bits removed (description of old branch names that are no longer menaingful) and notes on adding flash chips moved to a new page. Sections are also reordered to (hopefully) more closely match the order in which prospective contributors will care about them.
Change-Id: I12c5fac99f1dc9c05e3dd4d5554e937a57f64d6f Signed-off-by: Peter Marheine pmarheine@chromium.org --- M doc/contact.rst A doc/dev_guide/contributing.rst M doc/dev_guide/index.rst A doc/dev_guide/new_chips.rst 4 files changed, 438 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/23/74923/1
diff --git a/doc/contact.rst b/doc/contact.rst index 3a29c24..30db30a 100644 --- a/doc/contact.rst +++ b/doc/contact.rst @@ -3,6 +3,8 @@ .. The extra = is needed to prevent git from throwing a `leftover conflict marker` error when commiting.
+.. _mailing list: + Mailing List ------------ Flashrom related mails are welcome on the flashrom mailing list at `flashrom@flashrom.org mailto:flashrom@flashrom.org`_. diff --git a/doc/dev_guide/contributing.rst b/doc/dev_guide/contributing.rst new file mode 100644 index 0000000..0a4e452 --- /dev/null +++ b/doc/dev_guide/contributing.rst @@ -0,0 +1,270 @@ +======================== +Contributing to Flashrom +======================== + +Patches to Flashrom are accepted in two ways: + +1. Via `Gerrit on review.coreboot.org https://review.coreboot.org/#/q/project:flashrom`_. + This is **strongly preferred**, especially for large patches. +2. Sent to the :ref:`mailing list`. Send patches in-line (not as attachments) + so reviewers can easily comment. However, reviewers eventually need to push + patches sent to the mailing list to Gerrit anyway. + +Our guidelines borrow heavily from `coreboot coding style +https://doc.coreboot.org/contributing/coding_style.html`_ and `Gerrit +guidelines https://doc.coreboot.org/contributing/gerrit_guidelines.html`_, +and most of them apply to flashrom as well. The really important part is about +the :ref:`sign-off procedure <sign-off>`. + +---- + +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. + +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 as +soon as possible, but we have quite high standards regarding code quality. + +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 :ref:`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. + +.. _coding style: + +Coding style +============ + +Flashrom generally follows Linux kernel style: +https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu... + +The notable exception is line length limit. Our guidelines are: + + * 80-columns soft limit for most code and comments. This is to encourage simple design and concise naming. + * 112-columns hard limit. Use this to reduce line breaks in cases where they + harm grep-ability or overall readability, such as print statements and + function signatures. Don't abuse this for long variable/function names or + deep nesting. + * Tables are the only exception to the hard limit and may be as long as needed + for practical purposes. + + +.. _commit message: + +Commit message +============== + +Commit messages shall have the following format: + +.. code-block:: + + <component>: Short description (up to 72 characters) + + This is a long description. Max width of each line in the description + is 72 characters. It is separated from the summary by a blank line. You + may skip the long description if the short description is sufficient, + for example "flashchips: Add FOO25Q128" to add FOO25Q128 chip support. + + You may have multiple paragraphs in the long description, but please + do not write a novel here. For non-trivial changes you must explain + what your patch does, why, and how it was tested. + + Finally, follow the sign-off procedure to add your sign-off! + + Signed-off-by: Your Name your@domain + +.. _sign-off: + +Sign-off procedure +------------------ + +We employ a similar sign-off procedure as the `Linux kernel developers +http://web.archive.org/web/20070306195036/http://osdlab.org/newsroom/press_releases/2004/2004_05_24_dco.html`_ +do. Add a note such as + +.. code-block:: + + Signed-off-by: Random J Developer random@developer.example.org + +to your email/patch if you agree with the Developer's Certificate of Origin 1.1 +printed below. Read `this post on the LKML +https://lkml.org/lkml/2004/5/23/10`_ for rationale (spoiler: SCO). + +You must use your known identity in the ``Signed-off-by`` line and in any +copyright notices you add. Anonymous patches lack provenance and cannot be +committed! + +Developer's Certificate of Origin 1.1 +------------------------------------- + + By making a contribution to this project, I certify that: + + (a) The contribution was created in whole or in part by me and I have + the right to submit it under the open source license indicated in the file; or + + (b) The contribution is based upon previous work that, to the best of my + knowledge, is covered under an appropriate open source license and I have the + right under that license to submit that work with modifications, whether created + in whole or in part by me, under the same open source license (unless I am + permitted to submit under a different license), as indicated in the file; or + + (c) The contribution was provided directly to me by some other person who + certified (a), (b) or (c) and I have not modified it; and + + (d) In the case of each of (a), (b), or (c), I understand and agree that + this project and the contribution are public and that a record of the contribution + (including all personal information I submit with it, including my sign-off) is + maintained indefinitely and may be redistributed consistent with this project or the + open source license indicated in the file. + +.. note:: + + The `Developer's Certificate of Origin 1.1 + http://web.archive.org/web/20070306195036/http://osdlab.org/newsroom/press_releases/2004/2004_05_24_dco.html`_ + is licensed under the terms of the `Creative Commons Attribution-ShareAlike + 2.5 License http://creativecommons.org/licenses/by-sa/2.5/`_. + +Reviews +======= + +All patches must go through Gerrit to be accepted. Though, if the author +prefers, the actual reviewing process can also take place on the mailing list. +GitHub pull requests might still be reviewed, but are discouraged; see :ref:`GitHub`. + +All contributions should receive at least a preliminary review within one week +of submission by some flashrom developer (if that doesn't happen in time, +please be patient). At minimum this should include a broad indication of +acceptance or rejection of... + + * the idea/rationale/motivation, + * the implementation + +respectively. + +In general, reviews should focus on the architectural changes and things that +affect flashrom as a whole. This includes (but is by no means limited to) +changes in APIs and types, safety, portability, extensibility, and +maintainability. The purpose of reviews is not to create perfect patches, but +to steer development in the right direction and produce consensus within the +community. The goal of each patch should be to improve the state of the project +- it does not need to fix all problems of the respective field perfectly. + +.. note:: + + New contributors may need more detailed advices and should be told about + minor issues like formatting problems more precisely. The result of a review + should either be an accepted patch or a guideline how the existing code + should be changed to be eventually accepted. + +Using Gerrit +============ + +The quickest and best way to get your patch reviewed and merged is by sending +it to review.coreboot.org. Conveniently, you can use your GitHub, GitLab or +Google account as an OAuth2 `login method +https://review.coreboot.org/login`_. + +Setting up an account +--------------------- + +1. Go to https://review.coreboot.org/login and sign in using the credentials of + your choice. +2. Edit your settings by clicking on the gear icon in the upper right corner. +3. Set your Gerrit username (this may be the different from the username of an + external account you log in with). +4. Add an e-mail address so that Gerrit can send notifications to you about + your patch. +5. Upload an SSH public key, or click the button to generate an HTTPS password. + +Pushing patches +--------------- + +1. Install Git hooks: :code:`make gitconfig` + +2. Add upstream as a remote: + * If using SSH: :code:`git remote add -f upstream ssh://<gerrit_username>@review.coreboot.org:29418/flashrom` + * If using HTTPS: :code:`git remote add -f upstream https://review.coreboot.org/flashrom%60 + +3. Check out a new local branch that tracks :code:`upstream/master`: :code:`git + checkout -b <branch_name> upstream/master` + +4. Make commits on your new branch. Ensure you follow the rules for :ref:`code + style <coding style>` when writing code, and :ref:`commit messages <commit + message>` when writing your commit message. + +5. Push to Gerrit: :code:`git push upstream HEAD:refs/for/master` + * If using HTTPS you will be prompted for the username and password you + set in the Gerrit UI. + * If successful, the Gerrit URL for your patch will be shown in the output. + +Branches +======== + +New development occurs on the :code:`master` branch. There is no quality +promise for code on :code:`master`; even though we will try to keep the +regression rate as low as possible, the main purpose of the branch is to merge +new commits and make them available to a broader audience for testing. + +Release branches +---------------- + +Branching for a new release can happen at any point in time when a commit +(branch point) on ``master`` seems to be in good shape and was reasonably +tested after previous invasive changes. Between the branch point and the +release, every fix pushed for ``master`` for a problem that also persists +on the release branch shall be backported. The same also applies after the +release for the latest release branch and, optionally, for any earlier release +branch that is still maintained for other reasons (e.g. part of a long term +distribution). + +Whenever a release branch has no further unmerged commits in queue and is not +awaiting backported fixes, a release candidate (RC) can be tagged on that +branch. This can also be the original branch point. The RC shall undergo +extensive build tests and be publicly advertised as ready for testing. Not less +than three days after the last RC that included code changes, a release can be +tagged if no regressions showed up. + +Release-branch names follow the pattern ``<major>.<minor>.x`` (e.g. 1.0.x). +The first release of a branch is tagged ``v<major>.<minor>``, without a +point-release number (e.g. v1.0). Every following release from the same branch +will have a point-release number starting with .1 (e.g. v1.0.1) and will only +add backported fixes to the release. + +Unless the branch point (i.e. last common commit of master and a release +branch) is an RC, it should be tagged as ``p<major>.<minor>`` (e.g. p1.0), to +keep track where we are on the master branch. + +Merging to branches +------------------- + +Merging to branches is limited to the "flashrom developers" group on Gerrit. +This means every patch reviewed somewhere else (e.g. mailing list or GitHub) +must finally be pushed to Gerrit. The following rules apply, some are already +enforced by Gerrit: + + * Every commit has to be reviewed and needs at least one +2 that was not given + by the commit's author. + * Except, if a commit is authored by more than one person, each author may +2 + the other author's changes. + * Merging should not take place within less than 24 hours after the review + started (i.e. the first message by a reviewer on Gerrit). + * Finally, before hitting Submit, one is reponsible to check that all comments + have been addressed, especially if there was a negative review (-1). + +.. _github: + +GitHub +====== + +The official Flashrom **mirror** on GitHub is https://github.com/flashrom/flashrom + +Importantly, the **GitHub repo is only a mirror**, so all changes must go through +Gerrit on review.coreboot.org in order to be merged: even small patches such as +adding support for a flash chip. For this reason, **reviewers do not look at pull +requests**. Even if pull requests were automatically transferred to Gerrit, +requirements such as :ref:`sign-off` still present a problem. + diff --git a/doc/dev_guide/index.rst b/doc/dev_guide/index.rst index 9c6c0b1..2fd020e 100644 --- a/doc/dev_guide/index.rst +++ b/doc/dev_guide/index.rst @@ -4,4 +4,6 @@ .. toctree:: :maxdepth: 1
- building_from_source \ No newline at end of file + building_from_source + contributing + new_chips diff --git a/doc/dev_guide/new_chips.rst b/doc/dev_guide/new_chips.rst new file mode 100644 index 0000000..c15e44e --- /dev/null +++ b/doc/dev_guide/new_chips.rst @@ -0,0 +1,146 @@ +================================= +Adding/reviewing a new flash chip +================================= + +If you want to add support for a new flash chip or review a change doing so, +these guidelines can help you do it smoothly. + +#. Get the datasheet of the exact type of chip. + +#. Open ``flashchips.c`` and ``flashchips.h``. + +#. .. todo:: + This needs to be explained together with the probing somewhere else in detail. + + First, find the best IDs in the datasheet 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 + [#significance]_ differences in the chips behavior you have found a so + called evil twin. 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. + + + Usually the chip IDs follow a simple scheme: They are all uppercase; first + the manufacturer name (like for the manufacturer IDs on top of each + paragraph in flashchips.h) followed by an underscore and then the chipname. + The latter should in general equal the ``.name``, with dots (and other + disallowed characters) replaced by underscores. Shared chip IDs typically + use the macro name that happened to be added first to flashrom (which is + also probably the first one manufactured) and which usually matches the + other chips of that series in ``flashchips.h``. + +#. 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) + +#. Add ``.vendor``, ``.name``, IDs selected as explained above and + ``.total_size``. + +#. ``.page_size`` is really hard. Please read this `long explanation + http://www.flashrom.org/pipermail/flashrom/2013-April/010817.html`_, or + ignore it for now and set it to 256. + +#. We encode various features of flash chips in a bitmask named + ``.feature_bits``. The various possibilities can be found in ``flash.h``. + +#. ``.tested`` is used to indicate if the code was tested to work with real + hardware, its possible values are defined in ``flash.h``. Without any tests + it should be set to ``TEST_UNTESTED``. + +#. ``.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. + +#. ``.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). + +#. ``.block_erasers`` stores an array of pairs of erase functions + (``.block_erase``) with their respective layout (``.eraseblocks``). + + * ``.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. + + * 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). + +#. ``.printlock`` is a `misnomer to some extent + http://www.flashrom.org/pipermail/flashrom/2011-May/006495.html`_. It is + misused not only to print (write) protected address ranges of the chip, but + also to pretty print the values of the status register(s) - especially true + for SPI chips. There are a lot of existing functions for that already and + you should reuse one if possible. Comparing the description of the status + register in the datasheet of an already supported chip with that of your + chip can help to determine if you can reuse a printlock function. + +#. ``.unlock`` is called before flashrom wants to modify the chip's contents to + disable possible write protections. It is tightly related to the .printlock + function as it tries to change some of the bits displayed by .printlock. + +#. ``.write`` and ``.read`` are function pointers with the obvious meaning. + Currently flashrom does only support a single function each. The one that is + best supported by existing programmers should be used for now, but others + should be noted in a comment if available. + +#. ``.voltage`` defines the upper and lower bounds of the supply voltage of the + chip. If there are multiple chip models with different allowed voltage + ranges, the `intersection + http://en.wikipedia.org/wiki/Intersection_(set_theory)`_ should be used + and an appropriate comment added. + +#. The write `granularity + http://www.flashrom.org/pipermail/flashrom/2013-April/010817.html`_ can be + expressed by the ``.gran`` field. If you think you need something else than + the default (``write_gran_256bytes``) then you should definitely ask one of + the regular flashrom hackers first. Possible values can be found in + ``flash.h``. + +#. ``.reg_bits`` stores information about what configuration bits the chip has + and where they are found. For example, ``.cmp = {STATUS2, 6, RW}`` indicates + that the chip has a complement bit (CMP) and it is the 6th bit of the 2nd + status register. See ``struct reg_bit_info`` in ``flash.h`` for details on + each of the structure's fields. + + Note that some chips have configuration bits that function like TB/SEC/CMP + but are called something else in the datasheet (e.g. BP3/BP4/...). These + bits should be assigned to a field according their function and the + datasheet name should be noted in a comment, for example: + + .. code-block:: + + .sec = {STATUS1, 6, RW}, /* Called BP4 in datasheet, acts like SEC */ + +#. ``.decode_range`` points to a function that determines what protection range + will be selected by particular configuration bit values. It is required for + write-protect operations on the chip. + +.. rubric:: Footnotes + +.. [#significance] + + 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. +
-
Peter Marheine (Code Review)