Attention is currently required from: Edward O'Callaghan, Jes Klinke, Nikolai Artemiev, Thomas Heijligen.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77999?usp=email )
Change subject: raiden: Support target index with generic REQ_ENABLE ......................................................................
Patch Set 4: Code-Review+1
(2 comments)
Patchset:
PS2:
I do not see any mention of existing values `target=AP` or `target=EC` in `raiden_debug_spi. […]
Yes, I looked at the test now (tests/raiden_debug_spi.c), this parameter is not set. Which means it goes by default flow which is, from the code... request_enable = RAIDEN_DEBUG_SPI_REQ_ENABLE. I am not sure whether this is realistic (or even valid) scenario, but because unit tests do not talk to actual hardware, only emulate it, this can explain why unit test passes.
For some more content, unit tests are relatively recent in comparison with the most of code base (started in 2021 vs ~2000), so there is of course lots of room for improvement and adding more coverage.
How to run tests: instructions here https://flashrom.org/dev_guide/building_from_source.html#unit-tests If you don't have meson build dir yet, you need to scroll up in the doc. tldr
mkdir builddir meson setup builddir meson compile -C builddir meson test -C builddir
Also you can run `ninja test` inside builddir once you setup.
For adding new test: You can copy `raiden_debug_basic_lifecycle_test_success` and add target param into `raiden_debug_param` string. Perhaps all the mocks will apply without any change (I would try)! You need to add your new test function into tests/tests.c and tests/tests.h
If new test run, that's amazing! If it fails, or does not run, you can still push new patchset and tell what the error is. It is possible that maybe some of the mock functions (you can see them in `raiden_debug_io` need to be copied->adjusted) but first lets see whether test runs.
I am plannig to write a doc "How to add unit test", I will use this my comment as a starting point :)
File doc/classic_cli_manpage.rst:
https://review.coreboot.org/c/flashrom/+/77999/comment/2bd2c13a_7aa14914 : PS4, Line 873: There is a lot of information, I looked at your commit message and I really like the paragraph that you wrote. Can you copy it here? (and keep it in commit message of course).
The paragraph is easy to read, and it explains stuff, also importantly it gives examples of devices.
Let's inject this on the line 873 ?
Some devices such as the GSC knows how it is wired to AP and EC flash
chips, and can be told which specific device to talk to. Other devices such as Servo Micro and HyperDebug are generic, and do not know how they are wired, the caller is responsible for first configure the appropriate MUXes or buffers, and then tell the debugger which port to use (Servo Micro has just one SPI port, HyperDebug is the first that has multiple). The Raiden protocol allows both the cases of USB devices knowing their wiring and not.
-
Anastasia Klimchuk (Code Review)