[i2c,dv] Testbench issues and potential improvements
@hcallahan-lowrisc is already working on this.
Since Jul 4, 2024.
- Dominant language
- SystemVerilog
- Stars
- 3.6k
- Forks
- 1.1k
- Avg merge
- 2d 22h
- Merged PRs (30d)
- 141
Description
### Description
This bucket issue is for tracking potential improvements to the I2C testbench at a block level. This may include more general sweeping comments that apply to the agent as well.
---
_Issues / PRs_
#14825 - Long standing issue capturing some of the i2c_agent deficiencies. Some suggestions/ideas in the code are tagged against this issue, so it's worth grepping for them when investigating.
#23911 - See unresolved conversations for potential improvements that came up as part of the review process.
---
_Code Specifics / Comments_
- [ ] Remove `i2c_scoreboard.sv` checking restrictions where DUT-Target comparisons use limited-scope checking routines that ignore most of the fields in the sequence item. Ideally, we could use the UVM standard routine `A.compare(B)` here, which is used already for the DUT-Controller comparison code. (TODO(#) added in #23919)
- [ ] Don't use discrete counters `(i2c_env_cfg.{sent_acq_cnt,rcvd_acq_cnt,},i2c_agent_cfg.rcvd_rd_byte,i2c_base_vseq.{sent_rd_byte,sent_txn_cnt,}` etc.) to determine how much stimulus to generate in terms of CSR reads and writes, when to end polling/interrupt-driven testbench routines, etc. This is error prone and hard work to keep everything tied up, and the scoreboard knows when all the expected transactions have been received anyway. (TODO(#) added in #23911 )
- [ ] `i2c_env_cfg.lastbyte`, used in `i2c_host_fifo_fmt_empty_vseq`, should be refactored. Data shouldn't need to passed through the cfg like this.
- [ ] `i2c_env_cfg.read_addr_q[$]` is used to check if each Agent-Controller read transfer driven will match one of the configured DUT addresses, and signals either "VALID" or "INVALID" to the vseq. This requires the i2c_agent to know what the currently-configured DUT addresses are, which is unpleasant. This should be refactored and removed.
- [ ] `i2c_base_vseq.sv` uses the system function `$dist_normal()` to achieve a similar randomization to the previous method, which was refactored as part of #23911 . However, the default Verible linting rules forbid use of certain "system-functions". Consider coming back and re-writing this to do the randomization differently, as the exact random distribution really isn't critical here.
- [ ] The i2c_agent cfg contains a variable`cfg.got_stop` which is used by the monitor/seqr to synchronize at the end of each i2c_transaction (i.e. when a STOP condition has been received). Given the changes in #23911 to better model and capture i2c transfers / transactions in TLM items, it should be determinable from the items alone when a transaction has completed. Cfg fields used as flags / semaphores like this are bad practice in general. Remove it.
- [ ] The chip-level i2c test vseq's (chip_sw_i2c_tx_rx_vseq.sv) use a statically-configured set of timing parameters in the I2C Agent. This should be derived from the testbench clock frequency rather than pre-defined. This limits any reuseability.
- [ ] The i2c_agent base sequence `i2c_base_seq.sv` has a codepath for Agent-Controller operation, but I don't think this is ever used. Agent stimulus for controller mode operation is always done using one of the derived sequences `i2c_target_base_seq.sv`.
---
_Scoreboard / Reference Model_
- [ ] Some directed test vseq's disable the scoreboard when it is unable to model the behaviours those sequences are stimulating. This is undesirable, and limits testing to very narrowly-scoped directed test cases. We should aim to minimize the number of places this occurs, preferably by better modelling DUT functionality in the predictor. However, we may decide that this is acceptable in some cases, and for a V3-level signoff or similar we should provide justifications why this sort of testing is sufficient.
- i2c_target_nack_txstretch_vseq.sv
- i2c_host_override_vseq.sv
- i2c_host_mode_toggle_vseq.sv
- i2c_target_glitch_vseq.sv
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.