[i2c,dv] I2C chip-level vseq fixes for transfer-level TLM #23959
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The way that the i2c_agent drives the interface reactively currently has a small race condition. We watch the in-progress transfer field 'inp_xfer.state' to know when the transfer has progressed to a new state, but sometimes the previous driving hook returns on the same timestep as the state variable changes. This causes the agent to miss the state-change and hence miss it's hook for when the bus should be driven. I think this architecture could be iterated on in the future, but for now a solution is to schedule bus driving events into background processes and to not await them to finish before going back to wait on the next state change. These hooks should not overlap in practice. Since we do not change i2c_agent sequences on-the-fly during testcases, these background threads can all complete in their own time. Signed-off-by: Harry Callahan <[email protected]>
- Add comments - Track an existing issue - Add a print implementation that can be used by the derived sequences to log before they begin. Signed-off-by: Harry Callahan <[email protected]>
- Add comments and cleanup - Move i2c_agent timing configuration into a separate routine - Add field macros to print sequence variables - Log the sequence state using the new base class print routine before starting - the agent autoresponder sequence Signed-off-by: Harry Callahan <[email protected]>
This builds upon changes from a number of previous PRs and commits to collect and drive Agent Stimulus using the transfer-level item abstractions. This sequence previously made use of create_txn()/fetch_txn() routines inspired by the previous i2c_base_vseq code, which had a much looser model of what each sequence item represented. That model was re-worked in lowRISC#23911, and so this sequence also needs to collect and drive transaction data at this new level of abstraction. Firstly, this sequence got a cleanup pass, removing unnecessary code, adding comments and some minor refactors such as moving the i2c_agent timing parameter configuration into a separate routine. Secondly and more substantially, the code responsible for generating stimulus `send_i2c_host_rx_data()` has been replaced with a new but equivalent routine `drive_i2c_agent_stimulus()`, which now works at the transfer-level. The driven stimulus is still the same, comprising of: - An Agent-Controller READ transfer - An Agent-Controller WRITE transfer, writing the same data received in the read payload. Transfer read data is captured via a TLM fifo in the chip sequencer 'i2c_rd_fifos', which needs to be connected to the monitor's 'controller_mode_rd_item_port'. This is another required change based on previous monitor reworks. Note. the routine convert_i2c_txn_to_drv_q() has been copied from the block level base_vseq, which is responsible for converting transfer-level items into a quirky queue of items which is how we currently interface with the driver. This behaviour will eventually be replaced, at which point this duplicate code can be removed. However, that task is a future improvement. Signed-off-by: Harry Callahan <[email protected]>
…egression" This reverts commit 896deb8. Signed-off-by: Harry Callahan <[email protected]>
… systemtest in DV" This reverts commit 08113ab. Signed-off-by: Harry Callahan <[email protected]>
hcallahan-lowrisc
force-pushed
the
i2c_chip_vseq_fixup_for_transfer_tlm
branch
from
eaaf329 to
a075186
Compare
|
This is ready now, @marnovandermaas would you mind having another look? Thanks |
marnovandermaas
approved these changes
Jul 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aligns the chip-level I2C vseq's with the I2C DV architecture changes in #23911 to communicate at a transfer-level of abstraction. These vseq's, plus the power_virus_test, broke as a result of the original monitor changes, and were temporarily removed from the regression. This PR aligns all of this test code with the new TLM item format, fixing all 3 of the tests, and re-enables them in CI.
The changes to
chip_sw_i2c_tx_rx_vseq.svandchip_sw_i2c_host_tx_rx_vseq.svare pretty minor, and are better categorized as cleanups. The changes required to fix the device_tx_rx_vseq were more substantial, and make up the majority of changes in this PR. As with previous PRs, a significant amount of the changes are new comments to explain the test implementations and structures.More detailed notes can be seen in the commit messages.
Finally, the TLTs and the I2C component of the power_virus test are re-enabled in CI. They were originally disabled in #23911.