[bazel] Refactor FPGA test_cmd setup/cleanup boilerplate #28552
Conversation
sasdf
force-pushed
the
mgTc05c7768
branch
2 times, most recently
from
cd24921 to
d0c98d5
Compare
| testopt_clear_after_test = "True", | ||
| testopt_clear_before_test = "True", |
There was a problem hiding this comment.
There are many tests in the codebase that want to clear the bitstream both before and after. For example, most of the ownership and rescue tests have this property since they want to initialize with a modified owner config or modify the owner config during the test.
Are there any tests that don't have this property of wanting a clear both before and after?
There was a problem hiding this comment.
Some of the tests with changes_otp tag clear the bitstream after but not before.
rules/opentitan/fpga.bzl
Outdated
| # Construct the opentitantool test setup commands | ||
| test_setup_cmd = ['--exec="transport init"'] | ||
| if _get_bool(param, "testopt_clear_before_test"): | ||
| test_setup_cmd.append('--exec="fpga clear-bitstream"') | ||
| if "bitstream" in param: | ||
| test_setup_cmd.append('--exec="fpga load-bitstream {bitstream}"') | ||
| if _get_bool(param, "testopt_bootstrap") and "firmware" in param: | ||
| test_setup_cmd.append('--exec="bootstrap --leave-in-reset --clear-uart=true {firmware}"') | ||
| test_setup_cmd = "\n".join(test_setup_cmd) | ||
| test_setup_cmd = test_setup_cmd.format(**param) |
There was a problem hiding this comment.
Most of the custom test-harness tests manage their own bitstream and bootstrapping in the harness. How does this step interact with those tests? (ie: can we set flags appropriately to end up constructing an empty test_setup_cmd so that those tests continue to operate normally)
There was a problem hiding this comment.
I removed the load bitstream from the custom harness flags, and will also remove the --bootstrap flags when migrating the test_cmds.
Since the old tests will always re-initialize the FPGA, these setup cmd won't have any impact on the tests except a longer testing time.
| "--rcfile=", | ||
| "--logging=info", | ||
| "--interface={interface}", | ||
| "--drop-reset", |
There was a problem hiding this comment.
Should we really add a top-level flag to do this?
How about --exec='gpio remove RESET'
There was a problem hiding this comment.
This flag needs to work with both opentitantool and custom harness (i.e. test_utils) which doesn't understand the --exec flags.
sasdf
force-pushed
the
mgTc05c7768
branch
2 times, most recently
from
da18076 to
56f22d1
Compare
sasdf
force-pushed
the
mgTc05c7768
branch
4 times, most recently
from
e3fff45 to
7003e3b
Compare
Currently, an execution environment can derive from a base execution environment. However, some fields, like `param`, are dictionaries are completely replaced instead of partially overridable. This change introduces `_NESTED_FIELDS` to indicate fields that should merge their values when derived from a base, rather than just replacing them. Specifically, it enables `param` to be partially overridden, allowing a derived environment to add or modify specific parameters without losing all parameters from its base. Change-Id: Ie0922e2e90c80c92749907f491109b527879a278 Signed-off-by: Yi-Hsuan Deng <[email protected]>
This change extracts the FPGA test command setup and cleanup logic into a more modular system using `testopt` flags, which reduces the boilerplate needed to customize test commands. Change-Id: I008cf6ce4617ba5afe2adf81df855e8593d4995e Signed-off-by: Yi-Hsuan Deng <[email protected]>
Added the `drop-reset` command line option to `opentitantool` and `opentitanlib`, which allows de-asserting the `RESET` pin before executing any command. This is useful for devices held in a reset state, where `RESET` needs to be de-asserted to allow the host to communicate with the device. Change-Id: I7c9b3520132f635d9a12733e5e22fe658f6961a3 Signed-off-by: Yi-Hsuan Deng <[email protected]>
This change: * Cleans up FPGA parameter fields by utilizing the new partial overwrite semantics. * Sets up FPGA execution environments using `testopt` flags. * Modifies the default `test_cmd` for FPGA environments to be empty, providing a better default for custom harnesses that don't require flags. Change-Id: I645ac75eb2007d0d109bcab2fdc0cad042e7c8e1 Signed-off-by: Yi-Hsuan Deng <[email protected]>
This change applies the `testopt_bootstrap`, `testopt_clear_before_test` and `testopt_clear_after_test` refactoring to existing tests instead of using the test_cmd to bootstrap and clear the bitstream before/after the test. These options simplify test configuration and make it easier to manage how tests interact with the FPGA and bootstrap process. Change-Id: Ic05c77684e2d7577bcd328872a230d396035847d Signed-off-by: Yi-Hsuan Deng <[email protected]>
2 participants
This PR refactors FPGA
test_cmdby separating common setup (initialization, bitstream loading, bootstrapping) and cleanup logic from the core test command. In addition to the improved code maintainability, this patch enables more flexible test setups, such as for ROM coverage tests where extra steps are required between bitstream loading and bootstrapping, which can now be easily configured usingtestoptin the execution environment.Previously, modifying the test command required repeating these boilerplate steps. Now, these steps are removed from the
test_cmdbut can still be controlled via a set oftestoptflags infpga_params.bool testopt_clear_before_test: (Default=False) Call clear-bitstream before loading bitstream.bool testopt_bootstrap: (Default=True) Bootstrap firmware before test.bool testopt_clear_after_test: (Default=False) Call clear-bitstream after test or failure.This PR also updates the semantic of several things:
paramfield in exec env rules now partially overrides theparamfrom base exec env.We can have a follow-up PRs that also refactors other environments (qemu, silicon, ...) to align with FPGA.
Two tests failed with unread UART log overflow after the refactoring:
We workaround this issue by initializing the console at the beginning of the test.