Anastasia Klimchuk has uploaded this change for review.

View Change

doc: Add docs how to add and test flashchips

Change-Id: I8dde689f2a0430ae10d3fa68bee727c5a9d70aec
Signed-off-by: Anastasia Klimchuk <aklm@flashrom.org>
---
A doc/contrib_howtos/how_to_add_new_chip.rst
A doc/contrib_howtos/how_to_mark_chip_tested.rst
A doc/contrib_howtos/index.rst
M doc/index.rst
4 files changed, 268 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/05/80205/1

diff --git a/doc/contrib_howtos/how_to_add_new_chip.rst b/doc/contrib_howtos/how_to_add_new_chip.rst
new file mode 100644
index 0000000..af619db
--- /dev/null
+++ b/doc/contrib_howtos/how_to_add_new_chip.rst
@@ -0,0 +1,213 @@
+=====================
+How to add a new chip
+=====================
+
+To add a new chip definition you need to send a patch to flashrom. This can be your first patch to flashrom. However if it is
+your first patch please read carefully :doc:`/dev_guide/development_guide`, and set up dev environment locally.
+
+The expectation is that you have tested successfully at least some of the operations on the chip (not necessarily all of them).
+There is an exception: when datasheet covers more than one model of the same chip, but you only have one and tested that one.
+Typically, you would add all the models covered by the datasheet you have, but only mark tested the one you tested.
+
+To begin with, make sure you have a link to the publicly available datasheet for the chip you want to add. Publicly
+available datasheet is a requirement for adding a new chip.
+
+Open the datasheet, ``flashchips.c`` and ``include/flashchips.h``, and then
+
+Model ID and vendor ID
+======================
+
+Find model ID and vendor ID in the datasheet. This information often can be found in the sections like "Device identification",
+"Device ID", "Manufacturer ID", "Table of ID definitions" or something similar to that.
+These values come in combination with commands used for probing the chip (more on probing below). Most commonly used command for
+probing SPI chips is ``0x9f`` which corresponds to ``PROBE_SPI_RDID``.
+
+At this point you need to double-check if exact same chip is already defined. Search for the ``.name`` and ``.vendor`` of the chip
+you have in ``flashchips.c`` and if found, compare values of ``.manufacture_id`` and ``.model_id``. If all 4 values match, it
+seems like the chip already defined. In this case you can try using it. Possibly, only the testing status of the chip needs to be
+updated, in this case see :doc:`how_to_mark_chip_tested` for instructions.
+
+Vendor (manufacturer) ID
+------------------------
+
+Search for vendor ID in ``include/flashchips.h``, IDs are defined as macros. If it exists already, use existing macro in your
+chip definition. If it does not exist (which means your chip is the first ever for this vendor), define macro for this ID,
+add a comment. Look at existing IDs in ``include/flashchips.h`` as examples.
+
+Model ID
+--------
+
+Search for model ID in ``include/flashchips.h``. If there is no macro defined for it, add macro for model ID in the corresponding
+section for given vendor.
+
+Model IDs should follow the pattern. They are all uppercase; first the manufacturer name (used for the manufacturer IDs macros
+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 ``include/flashchips.h``.
+
+If you added a new model ID, use it in new chip definition, skip the rest of instructions about Model ID
+and continue with the next section "Properties".
+
+Model ID already exists
+^^^^^^^^^^^^^^^^^^^^^^^
+
+It is possible that model ID already exists in the header. Then find the corresponding definition in ``flashchips.c`` and inspect
+it carefully. Is it the same chip that you have or a different one?
+
+The question you need to figure out is: whether existing definition can work for your chip or not. You figure that out by
+reading the rest of instructions and comparing the values from the datasheet with existing definition.
+
+Existing definition matches, but it has a different chip name
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+It is possible all the values from the datasheet match existing definition, but it has a different chip name. This is fine,
+and in this case you need to add a comment to the macro ID definition which looks like ``Same as ...``.
+Look at existing examples in ``include/flashchips.h`` file.
+You should change the ``.name`` to reflect the additional chip model (see other chips of naming examples).
+You don't need to add new definition.
+
+Existing definition does not match
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+It is possible that, despite of the same model ID, existing vs new chips have significant different and they
+need different definitions. Judging the significance of a difference might be not trivial and might require some understanding
+of flashrom behavior, however the examples of significant differences are: different sizes of erase blocks,
+or different opcodes for operations (see more on operations below), or different configuration for write-protection (if this
+config is defined).
+
+In this case you need to do both:
+
+* Add a comment to existing model ID macro, ``Same as ...``
+* Add new chip definition, re-use existing ID macro
+
+Note that if you need to add new chip definition, it is fine to copy a similar one that already exist and correct the values.
+
+Make sure to keep the abc ordering of chip names, and place new definition into the section with other chips by the same vendor.
+
+Properties
+==========
+
+* From the datasheet, get ``.vendor``, ``.name``, ``.bustype``, ``.total_size``.
+* ``.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 <https://en.wikipedia.org/wiki/Intersection_(set_theory)>`_
+  should be used and an appropriate comment added.
+* ``.page_size`` is really hard.
+  Please read this `long explanation <https://mail.coreboot.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``.
+  Available options can be found in ``include/flash.h``, look for macros defined by the pattern ``#define FEATURE_XXX``.
+* ``.tested`` is used to indicate if the code was tested to work with real hardware, its possible values are defined
+  in ``include/flash.h``. Without any tests it should be set to ``TEST_UNTESTED``.
+  See also another doc :doc:`how_to_mark_chip_tested`.
+
+Operations
+==========
+
+Each operation is defined as a enum value from a corresponding enum.
+
+Probe
+-----
+
+``.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``. For most SPI flash chips ``PROBE_SPI_RDID`` is the most commonly used if the datasheets
+mentions **0x9f** as an identification/probing opcode.
+
+To see the full list of available probing functions, check definition of ``enum probe_func`` in ``include/flash.h``.
+You may need to inspect the source code of what a probing function is doing, check the mapping ``lookup_probe_func_ptr`` and
+search for the function code.
+
+``.probe_timing`` is only used for non-SPI chips. It indicates the delay after "enter/exit ID mode" commands in microseconds
+(see ``include/flash.h`` for special values).
+
+Read and write
+--------------
+
+``.read`` and ``.write`` indicate which functions are used for reading and writing on the chip. 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.
+
+To see the full list of available functions, check definitions of ``enum read_func`` and ``enum write_func`` in ``include/flash.h``.
+To inspect the source code, check the mappings ``lookup_write_func_ptr`` and ``lookup_read_func_ptr`` and search for
+the function code.
+
+The write granularity 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 ``include/flash.h``.
+
+Erase
+-----
+
+``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.
+
+To see the full list of available functions, check definition of ``enum block_erase_func`` in ``include/flash.h``.
+To inspect the source code, check the mappings ``lookup_erase_func_ptr`` and search for the function code.
+
+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).
+
+``.eraseblocks`` should be listed in order, from the smallest to the largest size.
+
+Printlock
+---------
+
+``.printlock`` is a `misnomer to some extent <https://mail.coreboot.org/pipermail/flashrom/2011-May/006495.html>`_.
+
+It is used 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.
+
+Check for definition of ``enum printlock_func`` and ``lookup_printlock_func_ptr`` for available options and source code.
+
+Unlock
+------
+
+``.unlock`` is called before flashrom wants to modify the chip's contents to disable possible write protections.
+It is related to the ``.printlock`` function as it tries to change some of the bits displayed by ``.printlock``.
+
+Check for definition of ``enum blockprotect_func`` and ``lookup_blockprotect_func_ptr`` for available options and source code.
+
+Write-protection
+================
+
+Write-protection support is optional, and if you haven't tested it on the chip, don't add it.
+If you, however, used and tested it, that would be great to add to chip definition.
+
+Registers bits
+--------------
+
+``.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 ``include/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:`.sec = {STATUS1, 6, RW}, /* Called BP4 in datasheet, acts like SEC */`
+
+Decode range
+------------
+
+``.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.
+
+Check for definition of ``enum decode_range_func`` and ``lookup_decode_range_func_ptr`` for available options and source code.
+
+Test your changes
+=================
+
+After making changes in the code, rebuild flashrom, run unit tests, and test the chip.
+
+Add testing information to commit message.
+
+When all of the above done, follow :doc:`/dev_guide/development_guide` to push a patch and go through review process.
+Dev guide has more details on the process.
diff --git a/doc/contrib_howtos/how_to_mark_chip_tested.rst b/doc/contrib_howtos/how_to_mark_chip_tested.rst
new file mode 100644
index 0000000..5f02ffd
--- /dev/null
+++ b/doc/contrib_howtos/how_to_mark_chip_tested.rst
@@ -0,0 +1,46 @@
+==========================
+How to mark chip as tested
+==========================
+
+flashrom has a massive amount of flashchips definitions, not all of them are fully tested. The reason for this is:
+in some situations chip definition can be introduced based on the values that the datasheet claims,
+but without testing on hardware. In this case chip definition is marked as not tested, **TEST_UNTESTED**. If later
+someone uses the chip for real, and it works, the chip definition can be (and should be) updated, and operations
+which run successfully should be marked as tested.
+
+To mark the chip as tested, someone (one of the maintainers, or one of the contributors) need to send a patch to update
+the list of flashchips. It can be you, and *yes it can be your first patch to flashrom!* If this is your first patch,
+make sure you read the :doc:`/dev_guide/development_guide` and set up dev environment locally. If this is not possible, you
+are welcome to send test results to the mailing list, see :ref:`mailing list`.
+
+Instructions below assume you went through Development guide, and set up environment and repository locally.
+
+To begin with, make sure you have full logs from all the operations that you have run successfully and want to
+mark as tested. **Providing full logs which indicate successful run is required to mark chip as tested.**
+
+Information about tested status of the chip is stored in ``flashchips.c``, specifically in the ``.tested`` member
+of ``struct flashchip``.
+
+Relevant definitions and available options are in ``include/flash.h``, specifically see the definition of:
+
+* ``enum test_state``
+* ``struct tested`` (inside ``struct flashchip``)
+* pre-defined macros for ``struct tested``, all of them follow the pattern ``#define TEST_xxx_yyy``
+
+Choose the correct value depending on the operations that you have tested successfully. And then:
+
+#. Open ``flashchips.c`` and find the definition of your chip.
+#. Check the tested status of the chip. If you tested *not* on the latest HEAD (perhaps on the latest released version,
+   or on flashrom built a month ago, etc), it is possible that tested status has been updated already. If the status
+   has not been updated,
+#. Update ``.tested`` value to the one which reflects current test status. Note that not all the operations have to be
+   tested at once: maybe you tested only probe and read, then mark just that.
+#. Make sure flashrom builds successfully, and all the unit tests pass.
+#. For commit title, use the string ``flashchips: Mark <chip name> as tested for <operations>``.
+#. Provide the logs in commit message, you can use flashrom paste service at `paste.flashrom.org <https://paste.flashrom.org>`_
+   or any other paste service. As a plan B, you can post message on the :ref:`mailing list`, attach the logs to the post,
+   and then add the link to the post to the commit message.
+#. Follow :doc:`/dev_guide/development_guide` and send your patch for review.
+#. Go through review process until your patch gets approved (see Development guide for more details on this).
+
+To see the examples of such patches, have a look at commit history of ``flashchips.c``.
diff --git a/doc/contrib_howtos/index.rst b/doc/contrib_howtos/index.rst
new file mode 100644
index 0000000..0a4fd0e
--- /dev/null
+++ b/doc/contrib_howtos/index.rst
@@ -0,0 +1,8 @@
+Contributors howtos
+===================
+
+.. toctree::
+    :maxdepth: 1
+
+    how_to_add_new_chip
+    how_to_mark_chip_tested
diff --git a/doc/index.rst b/doc/index.rst
index 8217056..2fdf778 100644
--- a/doc/index.rst
+++ b/doc/index.rst
@@ -11,6 +11,7 @@
 
   dev_guide/index
   user_docs/index
+  contrib_howtos/index
   classic_cli_manpage
   contact
   release_notes/index

To view, visit change 80205. To unsubscribe, or for help writing mail filters, visit settings.