chore: testing exsting construct#447
chore: testing exsting construct#447KelvinChung2000 wants to merge 2 commits intoFPGA-Research:mainfrom
Conversation
KelvinChung2000
force-pushed
the
bel-testing
branch
from
9dee236 to
ee11c99
Compare
KelvinChung2000
force-pushed
the
bel-testing
branch
from
ee11c99 to
2ef818f
Compare
KelvinChung2000
force-pushed
the
bel-testing
branch
from
2ef818f to
4953c14
Compare
KelvinChung2000
force-pushed
the
bel-testing
branch
from
4953c14 to
48827b2
Compare
KelvinChung2000
force-pushed
the
bel-testing
branch
2 times, most recently
from
ad83e73 to
d7f1697
Compare
chore: minor test fix chore: add uart test chore: a large update chore: all test passing chore: update lock chore: partial fix chore: fix missing model pack fix: fixing test chore: more test update fix: update conftest
KelvinChung2000
force-pushed
the
bel-testing
branch
from
7736496 to
45ffbc8
Compare
IAmMarcelJung
left a comment
IAmMarcelJung
left a comment
There was a problem hiding this comment.
A first small review, basically only of the BRAM test. In general LGTM, just some questions.
| use ieee.std_logic_1164.all; | ||
| use ieee.numeric_std.all; | ||
|
|
||
| -- Generated from Verilog module sram_1rw1r_32_256_8_sky130 (/home/kelvin/FABulous_fork.worktrees/FABulous2.0-development/FABulous/fabric_files/FABulous_project_template_verilog/Fabric/models_pack.v:508) |
There was a problem hiding this comment.
Probably remove the absoulte path here
| # Test Case 2: Multiple addresses - smoke test for address decode logic | ||
| test_patterns = [ | ||
| (0x00, 0x11111111), | ||
| (0x01, 0x22222222), | ||
| (0x10, 0x33333333), | ||
| (0x20, 0x44444444), | ||
| (0xFF, 0x55555555), | ||
| ] | ||
|
|
||
| # Write all test patterns | ||
| for addr, data in test_patterns: | ||
| dut.wr_addr.value = addr | ||
| dut.wr_data.value = data | ||
| await RisingEdge(dut.clk) |
There was a problem hiding this comment.
Do we even need to test different values if we only do the smoke test and check if an int is returned?
| # Test Case 2: Try to write with alwaysWriteEnable = 0 and no dynamic enable | ||
| dut.C4.value = 0 # alwaysWriteEnable = 0 | ||
| dut.wr_data.value = new_data # Try to write different data | ||
| # Don't set the dynamic write enable bit (bit 20 in wr_data) | ||
| await RisingEdge(dut.clk) | ||
| await Timer(Decimal(10), units="ps") | ||
|
|
||
| # Read back - smoke test that read completes | ||
| dut.rd_addr.value = test_addr | ||
| await RisingEdge(dut.clk) | ||
| await Timer(Decimal(10), units="ps") | ||
| # Note: Cannot verify write was prevented due to SRAM timing variability |
There was a problem hiding this comment.
Does this even test anything if there is no assertion? And even if there was an assertion, a smoke test probably also would not test the behavior, since the first write would already write a valid value and therefore the read will always work?
There was a problem hiding this comment.
This is probably something I missed during my review. I will need to see what should be done
| await RisingEdge(dut.clk) | ||
|
|
||
| # Test address extension via data bits | ||
| # Default parameters: ReadAddressMSBFromDataLSB = 24, WriteAddressMSBFromDataLSB = 16 |
There was a problem hiding this comment.
| # Default parameters: ReadAddressMSBFromDataLSB = 24, WriteAddressMSBFromDataLSB = 16 | |
| # Default parameters: READ_ADDRESS_MSB_FROM_DATALSB = 24, WRITE_ADDRESS_MSB_FROM_DATALSB = 16 |
It helps to match the actual parameter name to know where this comes from.
| # Write with extended address bits | ||
| # Bits [17:16] of wr_data become bits [9:8] of write address | ||
| extended_write_data = base_data | (0x3 << 16) # Set bits [17:16] = 11 | ||
| dut.wr_addr.value = base_addr | ||
| dut.wr_data.value = extended_write_data | ||
| await RisingEdge(dut.clk) | ||
| await Timer(Decimal(10), units="ps") | ||
|
|
||
| # Read with extended address bits | ||
| # Bits [25:24] of wr_data become bits [9:8] of read address for next read | ||
| extended_read_data = base_data | (0x3 << 24) # Set bits [25:24] = 11 | ||
| dut.wr_data.value = extended_read_data # This affects read address mapping | ||
| dut.rd_addr.value = base_addr | ||
| await RisingEdge(dut.clk) | ||
| await Timer(Decimal(10), units="ps") | ||
|
|
||
| # The exact behavior depends on how the address extension is implemented | ||
| # This smoke test verifies the module accepts the extended addressing format | ||
| # without simulation errors |
There was a problem hiding this comment.
Does this even test anything if there is no assertion? Will this just fail if the module does not accept the extended addressing format?
|
I would not go with just getting it merged. This has been waiting so long that we are about to start mutating the Bel design, and they might become irrelevant very soon. This might need some thinking now... |
|
Okay but should it still be reviewed or should we also not do this for now. As you said it was pending quite long, so I at least wanted to start step by step, but if this will completely be changed anyway, there's no need to review it. |
|
I don't know what the right thing to do is, since they need to be there for complete coverage. Unless we decide we will never "retire" them, then we can add keep them, and add in the flexibility as we need to. |
2 participants
Spliting #399 written construct into this PR