February 2024 - flashrom-gerrit

[M] Change in flashrom[main]: Makefile,meson.build: Add support for Sphinx versions prior to 4.x
by Anton Samsonov (Code Review) 12 Feb '24

12 Feb '24

Attention is currently required from: Anastasia Klimchuk, Anton Samsonov, Peter Marheine, Thomas Heijligen. Anton Samsonov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77778?usp=email ) Change subject: Makefile,meson.build: Add support for Sphinx versions prior to 4.x ...................................................................... Patch Set 7: (2 comments) File doc/meson.build: https://review.coreboot.org/c/flashrom/+/77778/comment/77834057_d267ad75 : PS6, Line 3: sphinx_wrapper = meson.current_source_dir() + '/sphinx-wrapper.sh' > `current_source_dir() / 'sphinx-wrapper.sh'` is a little nicer here. […] Done File doc/sphinx-wrapper.sh: https://review.coreboot.org/c/flashrom/+/77778/comment/b28792c1_c490dc01 : PS5, Line 14: > > I know this info is in commit message… […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/77778?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I9cd280551a1ba4d17edb2e857d56f80431b61e1b Gerrit-Change-Number: 77778 Gerrit-PatchSet: 7 Gerrit-Owner: Anton Samsonov <devel(a)zxlab.ru> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anton Samsonov <avscomputing(a)gmail.com> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Anton Samsonov <devel(a)zxlab.ru> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Mon, 12 Feb 2024 14:10:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anton Samsonov <avscomputing(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: Makefile,meson.build: Add support for Sphinx versions prior to 4.x
by Anton Samsonov (Code Review) 12 Feb '24

12 Feb '24

Attention is currently required from: Anastasia Klimchuk, Anton Samsonov, Anton Samsonov, Thomas Heijligen. Hello Anastasia Klimchuk, Peter Marheine, Thomas Heijligen, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/77778?usp=email to look at the new patch set (#7). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: Makefile,meson.build: Add support for Sphinx versions prior to 4.x ...................................................................... Makefile,meson.build: Add support for Sphinx versions prior to 4.x As of version 3.x, `sphinx-build` outputs man pages to "8" directory instead of "man8" expected by Makefile and doc/meson.build. See: https://github.com/sphinx-doc/sphinx/issues/7996 https://github.com/sphinx-doc/sphinx/issues/9217 Current solution is to rename "8" to "man8" after documentation build. That enables successful build and installation, as well as dependency tracking at build-system level, but not on `sphinx-build` own level upon which `meson` build blindly relies. Change-Id: I9cd280551a1ba4d17edb2e857d56f80431b61e1b Signed-off-by: Anton Samsonov <devel(a)zxlab.ru> --- M Makefile M doc/meson.build A doc/sphinx-wrapper.sh 3 files changed, 51 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/77778/7 -- To view, visit https://review.coreboot.org/c/flashrom/+/77778?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I9cd280551a1ba4d17edb2e857d56f80431b61e1b Gerrit-Change-Number: 77778 Gerrit-PatchSet: 7 Gerrit-Owner: Anton Samsonov <devel(a)zxlab.ru> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anton Samsonov <avscomputing(a)gmail.com> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Anton Samsonov <avscomputing(a)gmail.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Anton Samsonov <devel(a)zxlab.ru> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[main]: doc: Add doc how to add unit tests
by Anastasia Klimchuk (Code Review) 11 Feb '24

11 Feb '24

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/80340?usp=email ) Change subject: doc: Add doc how to add unit tests ...................................................................... doc: Add doc how to add unit tests Change-Id: I73f6add194a531c4f88b822d92c31ec67c23d2dc Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/80340 Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- A doc/contrib_howtos/how_to_add_unit_test.rst M doc/contrib_howtos/index.rst M doc/dev_guide/building_from_source.rst 3 files changed, 103 insertions(+), 0 deletions(-) Approvals: Peter Marheine: Looks good to me, approved build bot (Jenkins): Verified diff --git a/doc/contrib_howtos/how_to_add_unit_test.rst b/doc/contrib_howtos/how_to_add_unit_test.rst new file mode 100644 index 0000000..cb2aa37 --- /dev/null +++ b/doc/contrib_howtos/how_to_add_unit_test.rst @@ -0,0 +1,101 @@ +====================== +How to add a unit test +====================== + +Unit tests help to maintain higher bar for code quality. A new unit test which adds coverage to the code is always useful, +no matter how small or large it is! Unit test is a valuable contribution, moreover it makes a good starter project, since +you don't need specific hardware (apart from you development host machine). + +For more details on how to run unit tests and measure coverage, check the dev guide: :ref:`unit tests`. + +To see the examples of existing unit tests, check the ``/tests`` directory in the source tree. If it helps, you can also look +at git history for examples of previous commits that add new tests. + +When is a good time to add a unit test? Any time is a good time. Test can go in its own separate patch, and also it can go +together in a patch which introduces a new code. + +Unit tests are using `cmocka <https://cmocka.org/>`_ as a mocking framework, but we also have flashrom mock framework +on the top of that. + +Mocking +========= + +Unit tests mock all interactions with hardware, interactions with filesystem, syscalls, 3rd party libraries calls +(e.g. libusb, libpci) etc. You can think of a flashrom unit test as a mini-emulator. The goal is to cover as much as possible +flashrom code, but you don't need to go outside of that. + +See the list of all current mocks (which are called wraps in cmocka terminology) in ``/tests/wraps.h``. These might be enough for +your new test, or you might need to add more wraps as a part of new test. + +New wrap needs to be added to ``/tests/wraps.h``, ``/tests/tests.c``, ``/tests/meson.build``. If it's fine for new wrap to +do nothing, log invokation and return success, all good. + +If a wrap need to behave in a specific way for a test, and the behaviour can be different from one test to another, you need to +extend the wrap into ``/tests/io_mock.h`` framework. + +Add corresponding member (a function pointer) to ``struct io_mock`` +and redirect calls from a wrap function in ``/tests/tests.c`` into a member of ``io_mock``. The exact implementation +of the member function needs to be defined in your new test. At the beginning of a test scenario, define function pointers that your +test needs in your own ``struct io_mock`` and then register by calling ``io_mock_register``. At the end of a test, clean up +by calling ``io_mock_register(NULL)``. + +Note that ``io_mock`` can support state (if needed). State is a piece of custom data which is carried around for the duration +of the test scenario and is available in all functions in ``io_mock``. + +Adding a new test to a framework +================================ + +To add new test you will either add a new test function in existing .c file, or add new .c file and new function(s) there. + +If you add new file, you need to add it into the list of test source files in ``/tests/meson.build``. + +Each new test function needs to be added into ``/tests/tests.h`` and ``/tests/tests.c``. Follow existing entries as examples +how to do it. + +Types of tests +============== + +Programmers tests +----------------- + +The concept of a unit test for flashrom programmer is based on a programmer lifecycle. The options supported by the flashrom +test framework are the following (but you are very welcome to try implement new ideas). + +The smallest possible lifecycle is called basic and it does initialisation -> shutdown of a programmer (nothing else). +Another option is probing lifecycle, which does initialisation -> probing a chip -> shutdown. +These two expect successful scenarious, the whole scenario is expected to run until the end with no errors and +success return codes. + +One more option is to test expected failure for programmer initialisation. This is useful to test known invalid +(and potentially dangerous) combination of programmer parameters, when such combination should be detected at init time. +If invalid combination of parameters is detected, initialisation expected to fail early and programmer must not continue, +and not send any opcodes to the chip. + +For more details, source code is in ``/tests/lifecycle.h`` and ``/tests/lifecycle.c``. + +If you want to add new test(s) for a programmer, first you look whether that programmer has any tests already, or none at all. +Test source file has the same name as a programmer itself, for example programmer ``dummyflasher.c`` has its dedicated tests in +``/tests/dummyflasher.c`` file. Either add your tests to an existing file, or create new file for a programmer that had no tests +so far. The latter is more challenging, but it is very useful and highly appreciated. + +For programmers tests, the test scenario most likely won't be long: most likely it is one of the options to run lifecycle with +given combination of programmer params as an input. Most time and effort is typically spent on mocking (see above), and this +type of tests will indeed look like a mini-emulator. + +Chip operations tests +--------------------- + +These tests are based on dummyflasher programmer and they are running operations of a chip: read, write, erase, verify. +The test defines mock chip(s) with given properties, and all the operations of the chip are redirected to mock functions. +Mock chip has its own mock memory (an array of bytes) and all operations are performed on this array of bytes. +As all the others, these tests are completely independent of hardware, and are focused on testing core flashrom logic +for read, write, erase, verify. + +Examples of chip operation tests are: ``tests/chip.c``, ``/tests/chip_wp.c`` (focused on write-protection logic), +``/tests/erase_func_algo.c`` (focused on erasing and writing logic, especially the choice of erase blocks for given +layout regions). + +Misc +---- + +All other tests. You choose a function that you want to test, call it with given arguments, assert the results are as expected. diff --git a/doc/contrib_howtos/index.rst b/doc/contrib_howtos/index.rst index 0a4fd0e..59326cd 100644 --- a/doc/contrib_howtos/index.rst +++ b/doc/contrib_howtos/index.rst @@ -6,3 +6,4 @@ how_to_add_new_chip how_to_mark_chip_tested + how_to_add_unit_test diff --git a/doc/dev_guide/building_from_source.rst b/doc/dev_guide/building_from_source.rst index 4e50753..c997832 100644 --- a/doc/dev_guide/building_from_source.rst +++ b/doc/dev_guide/building_from_source.rst @@ -224,6 +224,7 @@ meson configure [updated builtin options] [updated flashrom options] <builddir> +.. _unit tests: Unit Tests ---------- -- To view, visit https://review.coreboot.org/c/flashrom/+/80340?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I73f6add194a531c4f88b822d92c31ec67c23d2dc Gerrit-Change-Number: 80340 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Jes Klinke <jbk(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[main]: flashchips: Add Macronix MX25R1635F
by Anastasia Klimchuk (Code Review) 07 Feb '24

07 Feb '24

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69678?usp=email ) Change subject: flashchips: Add Macronix MX25R1635F ...................................................................... flashchips: Add Macronix MX25R1635F 16Mbit (2MiB) [x1/x2/x4] Wide Voltage Range (VCC 1.65V-3.6V). It is similar to the already-supported MX25R3235F, but the total size is halved. Tested probe, read, erase, write and verify with buspirate. Datasheet: https://www.mxic.com.tw/Lists/Datasheet/Attachments/8702/MX25R1635F,%20Wide… Change-Id: Idce301ed90d6742b56e928068d201e5c3a2e5aee Signed-off-by: Davide Gerhard <rainbow(a)irh.it> Reviewed-on: https://review.coreboot.org/c/flashrom/+/69678 Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M flashchips.c M include/flashchips.h 2 files changed, 40 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved diff --git a/flashchips.c b/flashchips.c index f580a36..204f34f 100644 --- a/flashchips.c +++ b/flashchips.c @@ -10050,6 +10050,45 @@ { .vendor = "Macronix", + .name = "MX25R1635F", + .bustype = BUS_SPI, + .manufacture_id = MACRONIX_ID, + .model_id = MACRONIX_MX25R1635F, + .total_size = 2048, + .page_size = 256, + /* OTP: 1024B total; enter 0xB1, exit 0xC1 */ + .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP, + .tested = TEST_OK_PREW, + .probe = PROBE_SPI_RDID, + .probe_timing = TIMING_ZERO, + .block_erasers = + { + { + .eraseblocks = { {4 * 1024, 512} }, + .block_erase = SPI_BLOCK_ERASE_20, + }, { + .eraseblocks = { {32 * 1024, 64} }, + .block_erase = SPI_BLOCK_ERASE_52, + }, { + .eraseblocks = { {64 * 1024, 32} }, + .block_erase = SPI_BLOCK_ERASE_D8, + }, { + .eraseblocks = { {2 * 1024 * 1024, 1} }, + .block_erase = SPI_BLOCK_ERASE_60, + }, { + .eraseblocks = { {2 * 1024 * 1024, 1} }, + .block_erase = SPI_BLOCK_ERASE_C7, + } + }, + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_BP3_SRWD, /* bit 6 is quad enable */ + .unlock = SPI_DISABLE_BLOCKPROTECT_BP3_SRWD, + .write = SPI_CHIP_WRITE256, + .read = SPI_CHIP_READ, /* Fast read (0x0B) and multi I/O supported */ + .voltage = {1650, 3600}, + }, + + { + .vendor = "Macronix", .name = "MX25R3235F", .bustype = BUS_SPI, .manufacture_id = MACRONIX_ID, diff --git a/include/flashchips.h b/include/flashchips.h index d676cda..d0a858b 100644 --- a/include/flashchips.h +++ b/include/flashchips.h @@ -537,6 +537,7 @@ #define MACRONIX_MX25L3255E 0x9e16 #define MACRONIX_MX77L25650F 0x7519 +#define MACRONIX_MX25R1635F 0x2815 #define MACRONIX_MX25R3235F 0x2816 #define MACRONIX_MX25R6435F 0x2817 -- To view, visit https://review.coreboot.org/c/flashrom/+/69678?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Idce301ed90d6742b56e928068d201e5c3a2e5aee Gerrit-Change-Number: 69678 Gerrit-PatchSet: 3 Gerrit-Owner: ra1nb0w <rainbow(a)irh.it> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in flashrom[main]: doc: Add doc how to add unit tests
by Peter Marheine (Code Review) 04 Feb '24

04 Feb '24

Attention is currently required from: Alexander Goncharov, Anastasia Klimchuk, Jes Klinke, Sergii Dmytruk, Stefan Reinauer, Thomas Heijligen. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80340?usp=email ) Change subject: doc: Add doc how to add unit tests ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/80340?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I73f6add194a531c4f88b822d92c31ec67c23d2dc Gerrit-Change-Number: 80340 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Jes Klinke <jbk(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jes Klinke <jbk(a)chromium.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Sun, 04 Feb 2024 22:25:42 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: Makefile,meson.build: Add support for Sphinx versions prior to 4.x
by Peter Marheine (Code Review) 04 Feb '24

04 Feb '24

Attention is currently required from: Anastasia Klimchuk, Anton Samsonov, Anton Samsonov, Thomas Heijligen. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77778?usp=email ) Change subject: Makefile,meson.build: Add support for Sphinx versions prior to 4.x ...................................................................... Patch Set 6: (1 comment) File doc/meson.build: https://review.coreboot.org/c/flashrom/+/77778/comment/b49b18c0_16402256 : PS6, Line 3: sphinx_wrapper = meson.current_source_dir() + '/sphinx-wrapper.sh' `current_source_dir() / 'sphinx-wrapper.sh'` is a little nicer here. (`/` on strings is equivalent to `join_paths(x, y)`). -- To view, visit https://review.coreboot.org/c/flashrom/+/77778?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I9cd280551a1ba4d17edb2e857d56f80431b61e1b Gerrit-Change-Number: 77778 Gerrit-PatchSet: 6 Gerrit-Owner: Anton Samsonov <devel(a)zxlab.ru> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anton Samsonov <avscomputing(a)gmail.com> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Anton Samsonov <avscomputing(a)gmail.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Anton Samsonov <devel(a)zxlab.ru> Gerrit-Comment-Date: Sun, 04 Feb 2024 22:22:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: doc: Add doc how to add unit tests
by Anastasia Klimchuk (Code Review) 04 Feb '24

04 Feb '24

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/80340?usp=email ) Change subject: doc: Add doc how to add unit tests ...................................................................... doc: Add doc how to add unit tests Change-Id: I73f6add194a531c4f88b822d92c31ec67c23d2dc Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- A doc/contrib_howtos/how_to_add_unit_test.rst M doc/contrib_howtos/index.rst M doc/dev_guide/building_from_source.rst 3 files changed, 103 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/40/80340/1 diff --git a/doc/contrib_howtos/how_to_add_unit_test.rst b/doc/contrib_howtos/how_to_add_unit_test.rst new file mode 100644 index 0000000..cb2aa37 --- /dev/null +++ b/doc/contrib_howtos/how_to_add_unit_test.rst @@ -0,0 +1,101 @@ +====================== +How to add a unit test +====================== + +Unit tests help to maintain higher bar for code quality. A new unit test which adds coverage to the code is always useful, +no matter how small or large it is! Unit test is a valuable contribution, moreover it makes a good starter project, since +you don't need specific hardware (apart from you development host machine). + +For more details on how to run unit tests and measure coverage, check the dev guide: :ref:`unit tests`. + +To see the examples of existing unit tests, check the ``/tests`` directory in the source tree. If it helps, you can also look +at git history for examples of previous commits that add new tests. + +When is a good time to add a unit test? Any time is a good time. Test can go in its own separate patch, and also it can go +together in a patch which introduces a new code. + +Unit tests are using `cmocka <https://cmocka.org/>`_ as a mocking framework, but we also have flashrom mock framework +on the top of that. + +Mocking +========= + +Unit tests mock all interactions with hardware, interactions with filesystem, syscalls, 3rd party libraries calls +(e.g. libusb, libpci) etc. You can think of a flashrom unit test as a mini-emulator. The goal is to cover as much as possible +flashrom code, but you don't need to go outside of that. + +See the list of all current mocks (which are called wraps in cmocka terminology) in ``/tests/wraps.h``. These might be enough for +your new test, or you might need to add more wraps as a part of new test. + +New wrap needs to be added to ``/tests/wraps.h``, ``/tests/tests.c``, ``/tests/meson.build``. If it's fine for new wrap to +do nothing, log invokation and return success, all good. + +If a wrap need to behave in a specific way for a test, and the behaviour can be different from one test to another, you need to +extend the wrap into ``/tests/io_mock.h`` framework. + +Add corresponding member (a function pointer) to ``struct io_mock`` +and redirect calls from a wrap function in ``/tests/tests.c`` into a member of ``io_mock``. The exact implementation +of the member function needs to be defined in your new test. At the beginning of a test scenario, define function pointers that your +test needs in your own ``struct io_mock`` and then register by calling ``io_mock_register``. At the end of a test, clean up +by calling ``io_mock_register(NULL)``. + +Note that ``io_mock`` can support state (if needed). State is a piece of custom data which is carried around for the duration +of the test scenario and is available in all functions in ``io_mock``. + +Adding a new test to a framework +================================ + +To add new test you will either add a new test function in existing .c file, or add new .c file and new function(s) there. + +If you add new file, you need to add it into the list of test source files in ``/tests/meson.build``. + +Each new test function needs to be added into ``/tests/tests.h`` and ``/tests/tests.c``. Follow existing entries as examples +how to do it. + +Types of tests +============== + +Programmers tests +----------------- + +The concept of a unit test for flashrom programmer is based on a programmer lifecycle. The options supported by the flashrom +test framework are the following (but you are very welcome to try implement new ideas). + +The smallest possible lifecycle is called basic and it does initialisation -> shutdown of a programmer (nothing else). +Another option is probing lifecycle, which does initialisation -> probing a chip -> shutdown. +These two expect successful scenarious, the whole scenario is expected to run until the end with no errors and +success return codes. + +One more option is to test expected failure for programmer initialisation. This is useful to test known invalid +(and potentially dangerous) combination of programmer parameters, when such combination should be detected at init time. +If invalid combination of parameters is detected, initialisation expected to fail early and programmer must not continue, +and not send any opcodes to the chip. + +For more details, source code is in ``/tests/lifecycle.h`` and ``/tests/lifecycle.c``. + +If you want to add new test(s) for a programmer, first you look whether that programmer has any tests already, or none at all. +Test source file has the same name as a programmer itself, for example programmer ``dummyflasher.c`` has its dedicated tests in +``/tests/dummyflasher.c`` file. Either add your tests to an existing file, or create new file for a programmer that had no tests +so far. The latter is more challenging, but it is very useful and highly appreciated. + +For programmers tests, the test scenario most likely won't be long: most likely it is one of the options to run lifecycle with +given combination of programmer params as an input. Most time and effort is typically spent on mocking (see above), and this +type of tests will indeed look like a mini-emulator. + +Chip operations tests +--------------------- + +These tests are based on dummyflasher programmer and they are running operations of a chip: read, write, erase, verify. +The test defines mock chip(s) with given properties, and all the operations of the chip are redirected to mock functions. +Mock chip has its own mock memory (an array of bytes) and all operations are performed on this array of bytes. +As all the others, these tests are completely independent of hardware, and are focused on testing core flashrom logic +for read, write, erase, verify. + +Examples of chip operation tests are: ``tests/chip.c``, ``/tests/chip_wp.c`` (focused on write-protection logic), +``/tests/erase_func_algo.c`` (focused on erasing and writing logic, especially the choice of erase blocks for given +layout regions). + +Misc +---- + +All other tests. You choose a function that you want to test, call it with given arguments, assert the results are as expected. diff --git a/doc/contrib_howtos/index.rst b/doc/contrib_howtos/index.rst index 0a4fd0e..59326cd 100644 --- a/doc/contrib_howtos/index.rst +++ b/doc/contrib_howtos/index.rst @@ -6,3 +6,4 @@ how_to_add_new_chip how_to_mark_chip_tested + how_to_add_unit_test diff --git a/doc/dev_guide/building_from_source.rst b/doc/dev_guide/building_from_source.rst index 4e50753..c997832 100644 --- a/doc/dev_guide/building_from_source.rst +++ b/doc/dev_guide/building_from_source.rst @@ -224,6 +224,7 @@ meson configure [updated builtin options] [updated flashrom options] <builddir> +.. _unit tests: Unit Tests ---------- -- To view, visit https://review.coreboot.org/c/flashrom/+/80340?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I73f6add194a531c4f88b822d92c31ec67c23d2dc Gerrit-Change-Number: 80340 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[main]: flashrom_tester: Fix partial_lock_test on libflashrom
by Anastasia Klimchuk (Code Review) 03 Feb '24

03 Feb '24

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/79304?usp=email ) Change subject: flashrom_tester: Fix partial_lock_test on libflashrom ...................................................................... flashrom_tester: Fix partial_lock_test on libflashrom partial_lock_test (Lock_top_quad, Lock_bottom_quad, Lock_bottom_half, and Lock_top_half) tries to: 1. Disable HWWP 2. Lock partial 3. Enable HWWP 4. Try to write the locked partial and expect a failure ... The 4th step only works on flashrom binary since the binary will set FLASHROM_FLAG_VERIFY_AFTER_WRITE=1 by default and it will error out while verifying. But libflashrom does not set any flag beforehand, so it has FLASHROM_FLAG_VERIFY_AFTER_WRITE=0 and thus it will think the write command works normally and raise no error. This causes the issue that flashrom_tester with libflashrom has been failed until today. To solve this issue, there are two solutions: 1. Take care of the default flags in libflashrom 2. Always pass --noverify to flashrom binary and verify it afterwards. To make both methods more consistent, I fix it with approach 1. BUG=b:304439294 BRANCH=none TEST=flashrom_tester internal --libflashrom Lock_top_quad Change-Id: I7a8ac0c0984fef3cd9e73ed8d8097ddf429e54b2 Signed-off-by: roccochen(a)chromium.com <roccochen(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/79304 Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M bindings/rust/libflashrom/src/lib.rs M util/flashrom_tester/Cargo.toml M util/flashrom_tester/flashrom/src/cmd.rs M util/flashrom_tester/flashrom/src/flashromlib.rs M util/flashrom_tester/flashrom/src/lib.rs M util/flashrom_tester/src/tester.rs 6 files changed, 125 insertions(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved diff --git a/bindings/rust/libflashrom/src/lib.rs b/bindings/rust/libflashrom/src/lib.rs index 3f3fbc0..c3d4ebd 100644 --- a/bindings/rust/libflashrom/src/lib.rs +++ b/bindings/rust/libflashrom/src/lib.rs @@ -473,6 +473,87 @@ } } +/// A translation of the flashrom_flag type +/// +/// Keep this list in sync with libflashrom.h +#[derive(Clone, Debug, Eq, Hash, PartialEq)] +pub enum FlashromFlag { + FlashromFlagForce, + FlashromFlagForceBoardmismatch, + FlashromFlagVerifyAfterWrite, + FlashromFlagVerifyWholeChip, + FlashromFlagSkipUnreadableRegions, + FlashromFlagSkipUnwritableRegions, +} + +impl From<FlashromFlag> for libflashrom_sys::flashrom_flag { + fn from(e: FlashromFlag) -> Self { + match e { + FlashromFlag::FlashromFlagForce => libflashrom_sys::flashrom_flag::FLASHROM_FLAG_FORCE, + FlashromFlag::FlashromFlagForceBoardmismatch => { + libflashrom_sys::flashrom_flag::FLASHROM_FLAG_FORCE_BOARDMISMATCH + } + FlashromFlag::FlashromFlagVerifyAfterWrite => { + libflashrom_sys::flashrom_flag::FLASHROM_FLAG_VERIFY_AFTER_WRITE + } + FlashromFlag::FlashromFlagVerifyWholeChip => { + libflashrom_sys::flashrom_flag::FLASHROM_FLAG_VERIFY_WHOLE_CHIP + } + FlashromFlag::FlashromFlagSkipUnreadableRegions => { + libflashrom_sys::flashrom_flag::FLASHROM_FLAG_SKIP_UNREADABLE_REGIONS + } + FlashromFlag::FlashromFlagSkipUnwritableRegions => { + libflashrom_sys::flashrom_flag::FLASHROM_FLAG_SKIP_UNWRITABLE_REGIONS + } + e => panic!("Unexpected FlashromFlag: {:?}", e), + } + } +} + +/// Various flags of the flashrom context +/// +/// Keep the struct in sync with the flags in flash.h +#[derive(Debug)] +pub struct FlashromFlags { + pub force: bool, + pub force_boardmismatch: bool, + pub verify_after_write: bool, + pub verify_whole_chip: bool, + pub skip_unreadable_regions: bool, + pub skip_unwritable_regions: bool, +} + +/// Keep the default values in sync with cli_classic.c +impl Default for FlashromFlags { + fn default() -> Self { + Self { + force: false, + force_boardmismatch: false, + verify_after_write: true, + verify_whole_chip: true, + // These flags are introduced to address issues related to CSME locking parts. Setting + // them to false as default values + skip_unreadable_regions: false, + skip_unwritable_regions: false, + } + } +} + +impl fmt::Display for FlashromFlags { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "FlashromFlags {{ force: {}, force_boardmismatch: {}, verify_after_write: {}, verify_whole_chip: {}, skip_unreadable_regions: {}, skip_unwritable_regions: {} }}", + self.force, + self.force_boardmismatch, + self.verify_after_write, + self.verify_whole_chip, + self.skip_unreadable_regions, + self.skip_unwritable_regions, + ) + } +} + impl Chip { /// Probe for a chip /// @@ -705,6 +786,11 @@ }) } } + + /// Set a flag in the given flash context + pub fn flag_set(&mut self, flag: FlashromFlag, value: bool) -> () { + unsafe { libflashrom_sys::flashrom_flag_set(self.ctx.as_mut(), flag.into(), value) } + } } impl Drop for Chip { diff --git a/util/flashrom_tester/Cargo.toml b/util/flashrom_tester/Cargo.toml index e6ed9c0..a76a5b4 100644 --- a/util/flashrom_tester/Cargo.toml +++ b/util/flashrom_tester/Cargo.toml @@ -22,6 +22,7 @@ clap = { version = "2.33", default-features = false, optional = true } flashrom = { path = "flashrom/" } libc = "0.2" +libflashrom = { path = "../../bindings/rust/libflashrom" } log = { version = "0.4", features = ["std"] } rand = "0.6.4" serde_json = "1" diff --git a/util/flashrom_tester/flashrom/src/cmd.rs b/util/flashrom_tester/flashrom/src/cmd.rs index 9e66fe2..00c92cb 100644 --- a/util/flashrom_tester/flashrom/src/cmd.rs +++ b/util/flashrom_tester/flashrom/src/cmd.rs @@ -35,6 +35,8 @@ use crate::{FlashChip, FlashromError}; +use libflashrom::FlashromFlags; + use std::{ ffi::{OsStr, OsString}, path::Path, @@ -295,6 +297,12 @@ fn can_control_hw_wp(&self) -> bool { self.fc.can_control_hw_wp() } + + fn set_flags(&self, flags: &FlashromFlags) -> () { + // The flashrom CLI sets its own default flags, + // and we currently have no need for custom flags, + // so this set_flags function is intentionally a no-op. + } } fn flashrom_decode_opts(opts: FlashromOpt) -> Vec<OsString> { diff --git a/util/flashrom_tester/flashrom/src/flashromlib.rs b/util/flashrom_tester/flashrom/src/flashromlib.rs index 5e1747b..fb74188 100644 --- a/util/flashrom_tester/flashrom/src/flashromlib.rs +++ b/util/flashrom_tester/flashrom/src/flashromlib.rs @@ -32,7 +32,7 @@ // Software Foundation. // -use libflashrom::{Chip, Programmer}; +use libflashrom::{Chip, FlashromFlag, FlashromFlags, Programmer}; use std::{cell::RefCell, convert::TryFrom, fs, path::Path}; @@ -181,4 +181,25 @@ fn can_control_hw_wp(&self) -> bool { self.fc.can_control_hw_wp() } + + fn set_flags(&self, flags: &FlashromFlags) -> () { + self.flashrom + .borrow_mut() + .flag_set(FlashromFlag::FlashromFlagForce, flags.force); + self.flashrom + .borrow_mut() + .flag_set(FlashromFlag::FlashromFlagForceBoardmismatch, flags.force_boardmismatch); + self.flashrom + .borrow_mut() + .flag_set(FlashromFlag::FlashromFlagVerifyAfterWrite, flags.verify_after_write); + self.flashrom + .borrow_mut() + .flag_set(FlashromFlag::FlashromFlagVerifyWholeChip, flags.verify_whole_chip); + self.flashrom + .borrow_mut() + .flag_set(FlashromFlag::FlashromFlagSkipUnreadableRegions, flags.skip_unreadable_regions); + self.flashrom + .borrow_mut() + .flag_set(FlashromFlag::FlashromFlagSkipUnwritableRegions, flags.skip_unwritable_regions); + } } diff --git a/util/flashrom_tester/flashrom/src/lib.rs b/util/flashrom_tester/flashrom/src/lib.rs index e2c9149..e5ba9a8 100644 --- a/util/flashrom_tester/flashrom/src/lib.rs +++ b/util/flashrom_tester/flashrom/src/lib.rs @@ -45,7 +45,7 @@ pub use flashromlib::FlashromLib; pub use libflashrom::{ - flashrom_log_level, FLASHROM_MSG_DEBUG, FLASHROM_MSG_DEBUG2, FLASHROM_MSG_ERROR, + flashrom_log_level, FlashromFlags, FLASHROM_MSG_DEBUG, FLASHROM_MSG_DEBUG2, FLASHROM_MSG_ERROR, FLASHROM_MSG_INFO, FLASHROM_MSG_SPEW, FLASHROM_MSG_WARN, }; @@ -162,4 +162,7 @@ /// Return true if the hardware write protect of this flash can be controlled. fn can_control_hw_wp(&self) -> bool; + + /// Set flags used by the flashrom cli. + fn set_flags(&self, flags: &FlashromFlags) -> (); } diff --git a/util/flashrom_tester/src/tester.rs b/util/flashrom_tester/src/tester.rs index 4629c2e..c4098f2 100644 --- a/util/flashrom_tester/src/tester.rs +++ b/util/flashrom_tester/src/tester.rs @@ -38,6 +38,7 @@ use super::utils::{self, LayoutSizes}; use flashrom::FlashromError; use flashrom::{FlashChip, Flashrom}; +use libflashrom::FlashromFlags; use serde_json::json; use std::fs::File; use std::io::Write; @@ -83,6 +84,9 @@ random_data: "/tmp/random_content.bin".into(), layout_file: create_layout_file(rom_sz, Path::new("/tmp/"), print_layout), }; + let flags = FlashromFlags::default(); + info!("Set flags: {}", flags); + out.cmd.set_flags(&flags); info!("Stashing golden image for verification/recovery on completion"); out.cmd.read_into_file(&out.original_flash_contents)?; -- To view, visit https://review.coreboot.org/c/flashrom/+/79304?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I7a8ac0c0984fef3cd9e73ed8d8097ddf429e54b2 Gerrit-Change-Number: 79304 Gerrit-PatchSet: 5 Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Wu <david_wu(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Evan Benn <evanbenn(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hsuan Ting Chen <roccochen(a)chromium.org> Gerrit-CC: Nikolai Artemiev <nartemiev(a)chromium.org> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[main]: flashchips: Add Macronix MX25R1635F
by Anastasia Klimchuk (Code Review) 03 Feb '24

03 Feb '24

Attention is currently required from: Nikolai Artemiev, ra1nb0w. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69678?usp=email ) Change subject: flashchips: Add Macronix MX25R1635F ...................................................................... Patch Set 2: Code-Review+2 (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/69678/comment/61f8df32_1b14e9ba : PS1, Line 13: Tested > Can you mention which operation you tested, did you run verify at the end? […] Done Patchset: PS1: > Davide, I am so sorry that your patch got missed :\ I hope you are still here! Do you have a link to […] Done Patchset: PS2: Thank you for your contribution! File flashchips.c: https://review.coreboot.org/c/flashrom/+/69678/comment/da52685e_4300739e : PS1, Line 9306: .printlock = spi_prettyprint_status_register_bp3_srwd, /* bit 6 is quad enable */ : .unlock = spi_disable_blockprotect_bp3_srwd, > These two are now enums are well (not function pointers anymore). […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/69678?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Idce301ed90d6742b56e928068d201e5c3a2e5aee Gerrit-Change-Number: 69678 Gerrit-PatchSet: 2 Gerrit-Owner: ra1nb0w <rainbow(a)irh.it> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: ra1nb0w <rainbow(a)irh.it> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sat, 03 Feb 2024 07:42:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[L] Change in flashrom[main]: doc: Add docs how to add and update test status of flashchips
by Anastasia Klimchuk (Code Review) 03 Feb '24

03 Feb '24

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/80205?usp=email ) Change subject: doc: Add docs how to add and update test status of flashchips ...................................................................... doc: Add docs how to add and update test status of flashchips For reference, link to doc about adding new chip on the old website: https://wiki.flashrom.org/Development_Guidelines#Adding/reviewing_a_new_fla… Docs on how to update test status of flashchips never existed before. Change-Id: I8dde689f2a0430ae10d3fa68bee727c5a9d70aec Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/80205 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Reviewed-by: Peter Marheine <pmarheine(a)chromium.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, 269 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Sergii Dmytruk: Looks good to me, approved Peter Marheine: Looks good to me, but someone else must approve 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..b046ac3 --- /dev/null +++ b/doc/contrib_howtos/how_to_add_new_chip.rst @@ -0,0 +1,214 @@ +===================== +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 alphabetic 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 https://review.coreboot.org/c/flashrom/+/80205?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I8dde689f2a0430ae10d3fa68bee727c5a9d70aec Gerrit-Change-Number: 80205 Gerrit-PatchSet: 5 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit February 2024