Attention is currently required from: Stefan Reinauer.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78143?usp=email )
Change subject: Review access change
......................................................................
Patch Set 1: Code-Review+2 Verified+1
--
To view, visit https://review.coreboot.org/c/flashrom/+/78143?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: refs/meta/config
Gerrit-Change-Id: I7d357c4cdffe9667060bb6d58cc2e2ac4b913cde
Gerrit-Change-Number: 78143
Gerrit-PatchSet: 1
Gerrit-Owner: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Comment-Date: Sun, 24 Sep 2023 21:37:25 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Aarya, Edward O'Callaghan, Nikolai Artemiev, Simon Buhrow, Thomas Heijligen.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67535?usp=email )
Change subject: tests: Unit tests for erase function selection algo
......................................................................
Patch Set 14:
(3 comments)
File tests/erase_func_algo.c:
https://review.coreboot.org/c/flashrom/+/67535/comment/d9f30ef6_29ab7dad :
PS3, Line 238: void **state
> I will check cmocka docs how to do it, keep this unresolved for now.
I found something in cmocka docs, around this page https://api.cmocka.org/group__cmocka__exec.html#gaaccacc105038e49462888a3ed…
They way to set this state is to implement setup function, but also it needs to be passed to cmocka_unit_test_X creation function which we have in tests.c. But that means setup needs to be visible in tests.c, which means introducing headers for setup function? Isn't it leaking internal test details to tests' main function?
Another thing is that I wanted to move g_state to its separate file at some point later (probably not in the first version of the test) so that it can grow large. I don't mind adding twice more meaningful test cases to the list, but this better be in the separate file.
With all that, do you think we can keep it as is for now?
https://review.coreboot.org/c/flashrom/+/67535/comment/069093f7_cbb4f4fe :
PS3, Line 276: void **state
> ditto.
other comment has the question
File tests/erase_func_algo.c:
https://review.coreboot.org/c/flashrom/+/67535/comment/551874f9_bb4375ee :
PS5, Line 183: )
> We can reduce global state by passing the test case as an argument here, i.e. […]
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/67535?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I8f3fdefb76e71f6f8dc295d9dead5f38642aace7
Gerrit-Change-Number: 67535
Gerrit-PatchSet: 14
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Simon Buhrow
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Simon Buhrow
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Fri, 22 Sep 2023 13:17:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Aarya, Edward O'Callaghan, Nikolai Artemiev, Simon Buhrow, Thomas Heijligen.
Hello Aarya, Edward O'Callaghan, Simon Buhrow, Thomas Heijligen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/67535?usp=email
to look at the new patch set (#14).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: tests: Unit tests for erase function selection algo
......................................................................
tests: Unit tests for erase function selection algo
The test checks that algorithm for erase functions selection
works and there are no regressions.
Specifically, test contains an array of test cases. Each case
initialises a given initial state of the memory for the mock chip,
and layout regions on the chip, and then performs erase and write
operations.
At the end of operation, test asserts the following:
- the state of mock chip memory is as expected, i.e. properly erased
or written
- erase blocks are invoked in expected order and expected number
of them
- chip operation (erase or write) returned 0.
Change-Id: I8f3fdefb76e71f6f8dc295d9dead5f38642aace7
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
A tests/erase_func_algo.c
M tests/meson.build
M tests/tests.c
M tests/tests.h
4 files changed, 683 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/35/67535/14
--
To view, visit https://review.coreboot.org/c/flashrom/+/67535?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I8f3fdefb76e71f6f8dc295d9dead5f38642aace7
Gerrit-Change-Number: 67535
Gerrit-PatchSet: 14
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Simon Buhrow
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Simon Buhrow
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Name of user not set #1005084, Nikolai Artemiev, Stefan Reinauer.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78027?usp=email )
Change subject: flashchips: Add support for MXIC MX25U25643G
......................................................................
Patch Set 2: Code-Review-2
(1 comment)
Patchset:
PS2:
xianzheng, is this a script that pushes the same patch every 24 hours?
Could you please stop the script from running? thank you!
--
To view, visit https://review.coreboot.org/c/flashrom/+/78027?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id6c4254a02800f04c091009d19a628435fc20fce
Gerrit-Change-Number: 78027
Gerrit-PatchSet: 2
Gerrit-Owner: Name of user not set #1005084
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: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Name of user not set #1005084
Gerrit-Comment-Date: Thu, 21 Sep 2023 04:09:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nikolai Artemiev, Sergii Dmytruk, Stefan Reinauer.
Vasily Galkin has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77531?usp=email )
Change subject: flashchips: add WP features for W25X* analogous to tested W25X20
......................................................................
Patch Set 4:
(1 comment)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/77531/comment/d435a9e9_c5526017 :
PS3, Line 19730: .decode_range = DECODE_RANGE_SPI25,
> This chip seems to have unusual ranges, does DECODE_RANGE_SPI25 decode them correctly?
I can't be 100% sure since only the W25X20 from this series was phisycally tested, but according to the doc this chip has range compatible with DECODE_RANGE_SPI25.
https://www.winbond.com/resource-files/W25X05CL_G%2008012019.pdf
It says that while it has 2 non-volatile block protection bits in status register - any non-zero value just protects entire 64KiB.
Actually this logic is similar to 128KiB chips with 2 block protection bits - there is 4 possible values, but obly 3 possible states, so 0b10 and 0b11 are treated identically. And for 64KiB W25X05 all 0b10, 0b11 and 0b01 are the same.
This seems to be handled fine in https://review.coreboot.org/plugins/gitiles/flashrom/+/ebda447ad9df56dae7b9…
where the encoded WP lwngth is adjusted not to be greater then chip_len.
For the clarity I added some comments to the registers declaration in patch set 4.
And a "basic" test with temporarily adjusted JEDEC_RDID in dummyflasher was perfromed:
`flashrom --wp-list` correctly reports that there is only sngle range is supported:
```
Found Winbond flash chip "W25X05" (64 kB, SPI) on dummy.
===
This flash part has status UNTESTED for operations: WP
The test status of this chip ...
...
Available protection ranges:
start=0x00000000 length=0x00000000 (none)
start=0x00000000 length=0x00010000 (all)
```
--
To view, visit https://review.coreboot.org/c/flashrom/+/77531?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie69660a6f69e3cac31c5565792f401e69d43f8b8
Gerrit-Change-Number: 77531
Gerrit-PatchSet: 4
Gerrit-Owner: Vasily Galkin
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Thu, 21 Sep 2023 01:08:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: comment