February 2024 - flashrom-gerrit

[S] Change in flashrom[main]: serprog: Add support for multiple SPI chip selects
by Anastasia Klimchuk (Code Review) 18 Feb '24

18 Feb '24

Attention is currently required from: Riku Viitanen. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80498?usp=email ) Change subject: serprog: Add support for multiple SPI chip selects ...................................................................... Patch Set 2: (3 comments) Patchset: PS2: Riku, thank you for your patch! I have two small comments. File serprog.c: https://review.coreboot.org/c/flashrom/+/80498/comment/3b6802cf_d96d22fa : PS2, Line 827: cs = extract_programmer_param_str(cfg, "cs"); Since you are adding new programmer param, you need to update manpage as well. Specifically, manpage is generated from the file `doc/classic_cli_manpage.rst`, look for the section called `serprog programmer` and you can add a paragraph about `cs` param. May be useful to mention where it is supported (from your code, there are error paths, seems like it's not supported everywhere). Thank you! https://review.coreboot.org/c/flashrom/+/80498/comment/bfbb8cc9_2cc8bc0d : PS2, Line 833: msg_perr("Error: Invalid chip select requested! " : "Only 0-255 are valid.\n"); Not critical comment, but: this can be one line (and same for another error message before). Our guidelines for line length are here: https://www.flashrom.org/dev_guide/development_guide.html#coding-style -- To view, visit https://review.coreboot.org/c/flashrom/+/80498?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: If8052bc6f5c314dcc493bc083bb8270723efaae7 Gerrit-Change-Number: 80498 Gerrit-PatchSet: 2 Gerrit-Owner: Riku Viitanen <riku.viitanen(a)protonmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Riku Viitanen <riku.viitanen(a)protonmail.com> Gerrit-Comment-Date: Sun, 18 Feb 2024 12:19:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: Makefile: Fix version string for non-Git builds
by Anastasia Klimchuk (Code Review) 17 Feb '24

17 Feb '24

Attention is currently required from: Anton Samsonov, Thomas Heijligen. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79152?usp=email ) Change subject: Makefile: Fix version string for non-Git builds ...................................................................... Patch Set 5: (1 comment) Patchset: PS5: Anton, do you remember about this patch, let's maybe finish it? I have one comment, and I am guessing it would be easy for you to answer. I don't want your work to be lost. Thank you! -- To view, visit https://review.coreboot.org/c/flashrom/+/79152?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: I8694e618878823a9e96b1f2bcfa63f6c71d3c2ed Gerrit-Change-Number: 79152 Gerrit-PatchSet: 5 Gerrit-Owner: Anton Samsonov <devel(a)zxlab.ru> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Anton Samsonov <contact-launchpad(a)zxlab.ru> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anton Samsonov <devel(a)zxlab.ru> Gerrit-Comment-Date: Sat, 17 Feb 2024 09:45:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

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

16 Feb '24

Attention is currently required from: Anton Samsonov, Anton Samsonov, Thomas Heijligen. Anastasia Klimchuk 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: Code-Review+2 (2 comments) Patchset: PS7: I downloaded patch locally, and it works as expected (which is, as before) for version 4.3.2 File doc/sphinx-wrapper.sh: https://review.coreboot.org/c/flashrom/+/77778/comment/04cda7a7_db156890 : PS5, Line 14: > Done That's a really good comment! -- 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: Anton Samsonov <devel(a)zxlab.ru> Gerrit-Comment-Date: Fri, 16 Feb 2024 07:14:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Anton Samsonov <avscomputing(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: serprog: clean up documentation
by Riku Viitanen (Code Review) 14 Feb '24

14 Feb '24

Riku Viitanen has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/80499?usp=email ) Change subject: serprog: clean up documentation ...................................................................... serprog: clean up documentation * serprog.h doesn't exist, at least not in this repo * in the doc, no other command has S_CMD_ prefix either Change-Id: Ic83e7fd80840f2db0b006935a964721da0388068 Signed-off-by: Riku Viitanen <riku.viitanen(a)protonmail.com> Reviewed-on: https://review.sourcearcade.org/c/flashprog/+/52 Reviewed-by: Nico Huber <nico.h(a)gmx.de> Tested-by: Nico Huber <nico.h(a)gmx.de> --- M Documentation/serprog-protocol.txt 1 file changed, 1 insertion(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/99/80499/1 diff --git a/Documentation/serprog-protocol.txt b/Documentation/serprog-protocol.txt index d58b8e9..b3d4fd0 100644 --- a/Documentation/serprog-protocol.txt +++ b/Documentation/serprog-protocol.txt @@ -84,7 +84,7 @@ lower than the one requested. If there is no lower frequency available the lowest possible should be used. The value chosen is sent back in the reply with an ACK. - 0x15 (S_CMD_S_PIN_STATE): + 0x15 (S_PIN_STATE): Sets the state of the pin drivers connected to the flash chip. Disabling them allows other devices (e.g. a mainboard's chipset) to access the chip. This way the serprog controller can remain attached to the flash chip even when the board is running. The user is responsible to @@ -102,5 +102,3 @@ S_CMD_O_DELAY, S_CMD_O_EXEC. In addition, support for these commands is recommended: S_CMD_Q_PGMNAME, S_CMD_Q_BUSTYPE, S_CMD_Q_CHIPSIZE (if parallel). - -See also serprog.h. -- To view, visit https://review.coreboot.org/c/flashrom/+/80499?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: Ic83e7fd80840f2db0b006935a964721da0388068 Gerrit-Change-Number: 80499 Gerrit-PatchSet: 1 Gerrit-Owner: Riku Viitanen <riku.viitanen(a)protonmail.com> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[main]: serprog: Add support for multiple SPI chip selects
by Riku Viitanen (Code Review) 14 Feb '24

14 Feb '24

Attention is currently required from: Riku Viitanen. Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/80498?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified-1 by build bot (Jenkins) Change subject: serprog: Add support for multiple SPI chip selects ...................................................................... serprog: Add support for multiple SPI chip selects Tested with an EliteBook 8560w, Pi Pico, and my serprog firmware: https://codeberg.org/Riku_V/pico-serprog/ As seen on Flashprog: https://review.sourcearcade.org/c/flashprog/+/51 Change-Id: If8052bc6f5c314dcc493bc083bb8270723efaae7 Signed-off-by: Riku Viitanen <riku.viitanen(a)protonmail.com> --- M Documentation/serprog-protocol.txt M serprog.c 2 files changed, 31 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/98/80498/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/80498?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: If8052bc6f5c314dcc493bc083bb8270723efaae7 Gerrit-Change-Number: 80498 Gerrit-PatchSet: 2 Gerrit-Owner: Riku Viitanen <riku.viitanen(a)protonmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Riku Viitanen <riku.viitanen(a)protonmail.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[main]: serprog: Add support for multiple SPI chip selects
by Riku Viitanen (Code Review) 14 Feb '24

14 Feb '24

Riku Viitanen has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/80498?usp=email ) Change subject: serprog: Add support for multiple SPI chip selects ...................................................................... serprog: Add support for multiple SPI chip selects Tested with an EN25F80, Pi Pico, and this serprog firmware: https://codeberg.org/Riku_V/pico-serprog/commits/branch/multi-cs Change-Id: If8052bc6f5c314dcc493bc083bb8270723efaae7 Signed-off-by: Riku Viitanen <riku.viitanen(a)protonmail.com> Reviewed-on: https://review.sourcearcade.org/c/flashprog/+/51 Reviewed-by: Nico Huber <nico.h(a)gmx.de> Tested-by: Nico Huber <nico.h(a)gmx.de> --- M Documentation/serprog-protocol.txt M serprog.c 2 files changed, 31 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/98/80498/1 diff --git a/Documentation/serprog-protocol.txt b/Documentation/serprog-protocol.txt index 6b7e7e3..d58b8e9 100644 --- a/Documentation/serprog-protocol.txt +++ b/Documentation/serprog-protocol.txt @@ -35,6 +35,7 @@ + slen bytes of data 0x14 Set SPI clock frequency in Hz 32-bit requested frequency ACK + 32-bit set frequency / NAK 0x15 Toggle flash chip pin drivers 8-bit (0 disable, else enable) ACK / NAK +0x16 Set SPI Chip Select 8-bit ACK / NAK 0x?? unimplemented command - invalid. @@ -89,6 +90,9 @@ remain attached to the flash chip even when the board is running. The user is responsible to NOT connect VCC and other permanently externally driven signals to the programmer as needed. If the value is 0, then the drivers should be disabled, otherwise they should be enabled. + 0x16 (S_SPI_CS): + Set which SPI Chip Select pin to use. This operation is immediate, + meaning it doesn't use the operation buffer. About mandatory commands: The only truly mandatory commands for any device are 0x00, 0x01, 0x02 and 0x10, but one can't really do anything with these commands. diff --git a/serprog.c b/serprog.c index b51f0cf..750538a 100644 --- a/serprog.c +++ b/serprog.c @@ -3,6 +3,7 @@ * * Copyright (C) 2009, 2011 Urja Rannikko <urjaman(a)gmail.com> * Copyright (C) 2009 Carl-Daniel Hailfinger + * Copyright (C) 2024 Riku Viitanen <riku.viitanen(a)protonmail.com> * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -63,6 +64,7 @@ #define S_CMD_O_SPIOP 0x13 /* Perform SPI operation. */ #define S_CMD_S_SPI_FREQ 0x14 /* Set SPI clock frequency */ #define S_CMD_S_PIN_STATE 0x15 /* Enable/disable output drivers */ +#define S_CMD_S_SPI_CS 0x16 /* Set SPI chip select to use */ #define MSGHEADER "serprog: " @@ -742,7 +744,7 @@ /* Check for the minimum operational set of commands. */ if (serprog_buses_supported & BUS_SPI) { uint8_t bt = BUS_SPI; - char *spispeed; + char *spispeed, *cs; if (sp_check_commandavail(S_CMD_O_SPIOP) == 0) { msg_perr("Error: SPI operation not supported while the " "bustype is SPI\n"); @@ -822,6 +824,30 @@ } } free(spispeed); + cs = extract_programmer_param("cs"); + if (cs) { + char *endptr = NULL; + errno = 0; + unsigned long cs_num = strtoul(cs, &endptr, 0); + if (!*cs || errno || *endptr || cs_num > 255) { + msg_perr("Error: Invalid chip select requested! " + "Only 0-255 are valid.\n"); + free(cs); + goto init_err_cleanup_exit; + } + free(cs); + if (!sp_check_commandavail(S_CMD_S_SPI_CS)) { + msg_perr("Error: Setting SPI chip select is not supported!\n"); + goto init_err_cleanup_exit; + } + msg_pdbg(MSGHEADER "Requested to use chip select %lu.\n", cs_num); + uint8_t cs_num8 = cs_num; + if (sp_docommand(S_CMD_S_SPI_CS, 1, &cs_num8, 0, NULL)) { + msg_perr("Error: Chip select %u not supported " + "by programmer!\n", cs_num8); + goto init_err_cleanup_exit; + } + } bt = serprog_buses_supported; if (sp_docommand(S_CMD_S_BUSTYPE, 1, &bt, 0, NULL)) goto init_err_cleanup_exit; -- To view, visit https://review.coreboot.org/c/flashrom/+/80498?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: If8052bc6f5c314dcc493bc083bb8270723efaae7 Gerrit-Change-Number: 80498 Gerrit-PatchSet: 1 Gerrit-Owner: Riku Viitanen <riku.viitanen(a)protonmail.com> Gerrit-MessageType: newchange

1 0

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

12 Feb '24

Attention is currently required from: Anastasia Klimchuk, 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 7: Code-Review+2 -- 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-Comment-Date: Mon, 12 Feb 2024 22:28:57 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes 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, 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

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit February 2024