February 2024 - flashrom-gerrit

[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

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

02 Feb '24

Attention is currently required from: ra1nb0w. Hello Anastasia Klimchuk, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/69678?usp=email to look at the new patch set (#2). 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> --- M flashchips.c M include/flashchips.h 2 files changed, 40 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/69678/2 -- 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: 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-MessageType: newpatchset

1 0

[M] Change in flashrom[main]: flashchips: Remove Macronix MX25U25635F from chip list
by Anastasia Klimchuk (Code Review) 02 Feb '24

02 Feb '24

Attention is currently required from: Daniel, Nikolai Artemiev, Stefan Reinauer. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79633?usp=email ) Change subject: flashchips: Remove Macronix MX25U25635F from chip list ...................................................................... Patch Set 1: (1 comment) File flashchips.c: https://review.coreboot.org/c/flashrom/+/79633/comment/39a8999d_cac946ee : PS1, Line 10310: .wps = {SECURITY, 7, OTP}, /* This bit is set by WPSEL command */ > Hi, if WPSEL command is issued for MX25U25635F (older chip), it will be ignored. In general, what Nikolai is saying is the right approach, and also it's a future-proof approach. I understand this situation happens for the first time? (the situation being two chips of the same model ID, and the customer only wants the newest one). But realistically, it is likely to happen again. And next time two chips may have more differences than just one .wps reg bit. As an upstream project, we maintain all the chips (and older too) and removing the chip definition is something extremely rare (to the extent that I have never seen this happening). There are lots of users, and you never know which chips people are using. Specifically to your customer situation, if the customer has a fork of flashrom, they can introduce a local list of chips to ignore, and add old models there. This way they don't need to change automatic test flow, and also it's a future-proof solution as any more old chips like that can be added to the list. In terms of how hard it is to implement, Chrome OS flashrom is also open source, and they implemented this solution, so the code can be used as an inspiration. If you are interested, I can try to find links to relevant code. I don't know how your communication with customer is set up, but if it is fine, you are welcome to share with them the link to this patch (so that they can see the discussion), and you can even add them to this patch (if there is an engineer from customer side who can create a Gerrit account). What do you think about it? -- To view, visit https://review.coreboot.org/c/flashrom/+/79633?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: Ief3fd7641bed817066692c4abffff6d3b0df16b9 Gerrit-Change-Number: 79633 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Daniel Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Fri, 02 Feb 2024 09:27:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Daniel Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit February 2024