Simplify Calyx AXI wrapper -- xVALID signals and reg invokes
#1846
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
TBD if this actually implements AXI correctly. There are currently some hacks in place (marked with todos) to get this to compile, namely some splicing that doesn't consider what we actually want to splice (it just takes [31:0]) as opposed to dynamically considering actual bits we want. A few other things that should be cleaned up eventually Need to create a cocotb testbench to test correctness
Maybe this shouldn't be here, but for now (having deleted my working directory earlier) putting it here
Simply run make from the cocotb directory and axi-read-tests will be executed
We tie ARID low in our manager
Prefixes should not contain trailing "_"
Got rid of "assert_val" and "block_transfer" groups and instead perform these things inside "do_ar_transfer", this is required because we cant assert valid before we drive the data correctly, so needs to happen in parallel. Currently: This seems to write 16 times to same place, this is due to hardcoding of 16 in ar transfer, not sure why address doesn't increment this is tbd (and next TODO)
This is part of read channel control sequence
Also reduces data bus width to 32
nathanielnrn
commented
Jan 11, 2024
|
|
||
| is_arvalid.in = !(is_arvalid.out & ARREADY) & !arvalid_was_high.out ? 1'b1; | ||
| //assert ARVALID as long as this is the first time we are asserting it | ||
| is_arvalid.in = !arvalid_was_high.out ? 1'b1; |
There was a problem hiding this comment.
It seems to me like is_arvalid.out & ARREADY will always be true because is_arvalid.out is always 0 as we call deassert_val in the control sequence:
while {
// some stuff ...
do_ar_transfer;
deassert_val;
// more stuff ...
}
So we can simplify to just !arvalid_was_high.out as our guard
nathanielnrn
commented
Jan 11, 2024
| @@ -496,7 +495,7 @@ component m_awwrite_channel( | |||
| // this contains blocking logic previously in its own group | |||
| group do_aw_transfer { | |||
| //assert AWVALID | |||
| is_awvalid.in = !(is_awvalid.out & AWREADY) & !awvalid_was_high.out ? 1'b1; | |||
| is_awvalid.in = !awvalid_was_high.out ? 1'b1; | |||
There was a problem hiding this comment.
The same thing as is_arvalid here
nathanielnrn
force-pushed
the
axi-wrapper-opts
branch
from
7577988 to
31b6986
Compare
nathanielnrn
force-pushed
the
axi-wrapper-opts
branch
from
31b6986 to
4c1d9fb
Compare
nathanielnrn
changed the title
xVALID signals and reg invokes
nathanielnrn
commented
Jan 16, 2024
| @@ -673,7 +572,7 @@ component m_write_channel( | |||
|
|
|||
| //set to 1 after valid has been high even once | |||
| wvalid_was_high.in = 1'b1; | |||
| wvalid_was_high.write_en = !(wvalid.out & WREADY) & !wvalid_was_high.out ? 1'b1; | |||
| wvalid_was_high.write_en = !(wvalid.out & WREADY & !wvalid_was_high.out) ? 1'b1; | |||
There was a problem hiding this comment.
This partitions assignment to wvalid.write_en whereas before there was some uncovered component of the guard (see ~5 lines up)
|
This is pretty self contained so going to merge this into main. Happy to hear any comments about this after the fact. |
rachitnigam
added a commit
that referenced
this pull request
Feb 16, 2024
) * init commit of hardcoded axi wrapper for a 'main' kernel * add axi-reads-calix * hook up inputs to channels in the wrapper. tbd if this works * Working calyx verison of AR and R TBD if this actually implements AXI correctly. There are currently some hacks in place (marked with todos) to get this to compile, namely some splicing that doesn't consider what we actually want to splice (it just takes [31:0]) as opposed to dynamically considering actual bits we want. A few other things that should be cleaned up eventually Need to create a cocotb testbench to test correctness * Track output of compiled calyx read channel Maybe this shouldn't be here, but for now (having deleted my working directory earlier) putting it here * update gitignore to get rid of sim_build and other cocotb artifacts * Working make files for running cocotb tests Simply run make from the cocotb directory and axi-read-tests will be executed * Add xID signals for cocotb compatability We tie ARID low in our manager * Fix prefix issue on cocotb axi test bench Prefixes should not contain trailing "_" * commit to repro 'make WAVES=1' cocotb error from axi-reads-calyx.futil * axi-reads patch * sync debug * Add txn_len initialization to 16 in calyx program * AXI Read fixed to get to read channel start Got rid of "assert_val" and "block_transfer" groups and instead perform these things inside "do_ar_transfer", this is required because we cant assert valid before we drive the data correctly, so needs to happen in parallel. Currently: This seems to write 16 times to same place, this is due to hardcoding of 16 in ar transfer, not sure why address doesn't increment this is tbd (and next TODO) * Add integer byte conversion for tests on Calyx AXI testharness * WIP get reads to work. Add incr_curr_addr group This is part of read channel control sequence * remove .fst from tracking * Add more data to testbench to make waveform viewing easier * Reads seem to be terminating correctly at RLAST * AR transfers seem to work, valid is high for 1 cycle * Unreduced axi-reads-calyx.futil Also reduces data bus width to 32 * Cocotb testbench now passes * Formatted and passing axi-read-tests * Reduce and comment axi-reads-calyx.futil * remove axi-reads.v from being tracked * add a todo * add required ARPROT signal. This is hardcoded to be priviliged * rename directories to yxi/axi-calyx * initial commit of axi-writes-calyx, a copy of axi-reads-calyx * WIP axi writes * rename directories * WIP imlpementing writes * add testing for writes, note makefile is overwritten so now tests writes, not reads * passing axi writes and testing * init commit of hardcoded axi wrapper for a 'main' kernel * add axi-reads-calix * hook up inputs to channels in the wrapper. tbd if this works * Working calyx verison of AR and R TBD if this actually implements AXI correctly. There are currently some hacks in place (marked with todos) to get this to compile, namely some splicing that doesn't consider what we actually want to splice (it just takes [31:0]) as opposed to dynamically considering actual bits we want. A few other things that should be cleaned up eventually Need to create a cocotb testbench to test correctness * Track output of compiled calyx read channel Maybe this shouldn't be here, but for now (having deleted my working directory earlier) putting it here * Working make files for running cocotb tests Simply run make from the cocotb directory and axi-read-tests will be executed * Add xID signals for cocotb compatability We tie ARID low in our manager * Fix prefix issue on cocotb axi test bench Prefixes should not contain trailing "_" * commit to repro 'make WAVES=1' cocotb error from axi-reads-calyx.futil * axi-reads patch * sync debug * Add txn_len initialization to 16 in calyx program * AXI Read fixed to get to read channel start Got rid of "assert_val" and "block_transfer" groups and instead perform these things inside "do_ar_transfer", this is required because we cant assert valid before we drive the data correctly, so needs to happen in parallel. Currently: This seems to write 16 times to same place, this is due to hardcoding of 16 in ar transfer, not sure why address doesn't increment this is tbd (and next TODO) * Add integer byte conversion for tests on Calyx AXI testharness * WIP get reads to work. Add incr_curr_addr group This is part of read channel control sequence * remove .fst from tracking * Add more data to testbench to make waveform viewing easier * Reads seem to be terminating correctly at RLAST * AR transfers seem to work, valid is high for 1 cycle * Unreduced axi-reads-calyx.futil Also reduces data bus width to 32 * Cocotb testbench now passes * Formatted and passing axi-read-tests * Reduce and comment axi-reads-calyx.futil * remove axi-reads.v from being tracked * add a todo * add required ARPROT signal. This is hardcoded to be priviliged * rename directories to yxi/axi-calyx * initial commit of axi-writes-calyx, a copy of axi-reads-calyx * WIP axi writes * rename directories * WIP imlpementing writes * add testing for writes, note makefile is overwritten so now tests writes, not reads * passing axi writes and testing * Work on full AXI wrapper, reads and compute works * cleaned up combined futil and tests * delete axi-reads* which is subsumed by axi-combined * add axi-combined-tests.py * remove axi-writes as it is subsumed by axi-combined * formatting * Update yxi/axi-calyx/axi-combined-calyx.futil Co-authored-by: Adrian Sampson <[email protected]> * formatting * add sim.sh which goes from calyx to running tests * simplify valid.in signals * WIP: replace groups with reg invokes * add python file that enables waveform (vcd/fst) generation * formatting * simplify valid.in signals * WIP: replace groups with reg invokes * Replaces register-init groups with invokes * Formatting of invokes * Replace reg groups with invokes in main --------- Co-authored-by: Rachit Nigam <[email protected]> Co-authored-by: Adrian Sampson <[email protected]>
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.
Thought it would be useful to create a place to keep track of possible simplifications to the AXI wrapper we have, especially as I'm working through making the generator and parameterizing things. Things have already popped up. This can be a place to publicize possible optimizations and get feedback on them. (Maybe this should actually be a bunch of issues? Happy to move this as seems appropriate).EDIT: Think it makes sense to do these one at a time. This PR addresses 2 issues, although the first 1 is fairly small.
Will continue opening these up as work gets done
This PR addresses two issues:
Simplifies
arvalidandawvalidsignalsReplaces groups that only write to a single reg with
invokes. This simplifies how we generate things and will probably make thinking about optimizations easier to think about.