AXI generator clean up
Closed this issue · 2 comments
nathanielnrn commented
A few small issues were brought up that should be addressed. I find it helpful to keep these all in one place.
- Reduce code duplication by refactoring where appropriate.
- Closed by #1867. As mentioned here, in our channels we create
base_addr
andcurr_addr
registers. The naming is a bit vague andbase_addr
should be changed to something that reflects that this goes to our byte-addressed AXI addresses, whilecurr_addr
should be changed to reflect that it indexes into our internal calyx memories based on the memories indexing. - Closed by #1867. Assert in
build
that ouryxi
is well formed. i.e, powers of 2, width is byte aligned, etc. - Closed by #1867. Talked about this synchronously with @sampsyo and have had a few more thoughts since then:
ReevaluatexVALID_was_high
signals based on our in sync discussion. In general the point of this is to denote we are not in the first "raising ofxVALID
" cycle and that a handshake has occurred. We need this because we want to ensure that in a single servicing of a transferxVALID
is only asserted once. Because we have no guarantees on when the servicing group will terminate, we look toxVALID_was_high
to ensure xVALID is only high once. When we discussed this in person we assumed this would be in the first cycle, but I think this should be generalized to denote when ahandshake_occurred
. More concretely if we are in the group that services a transfer I believe we want the following (being a bit sloppy with notation):
//we are not currently in a handshake and haven't been high before
xVALID.in = !(xVALID & xREADY) & !(handshake_occurred) ? 1'b1;
//negate the above. Key point is that `handshake_occurred` stays high.
xVALID.in = (xVALID & xREADY) | handshake_occurred ? 1'b0;
xVALID..write_en = 1'b1;
//left side of `|` is a handshake, useful for when xVALID is high and xREADY is low
//right hand side is preemptive handshake when xREADY is high and we are about to drive xVALID high. Saves a cycle?
// actually might not be that useful //guard for xVALID.in = 1'b1
handshake_occurred.in = (xVALID & xREADY) | (xREADY & (!(xVALID & xREADY) & !(handshake_occurred)))? 1'b1;
// okay to write to this as long as it is 0
handshake_occurred.write_en = !(handshake_occurred) ? 1'b1;
I think we can also make
handshake_occurred.in = (xVALID & xREADY) | (xREADY & (!(xVALID & xREADY) & !(handshake_occurred)))? 1'b1;
simpler and just do
handshake_occurred.in = (xVALID & xREADY) ? 1'b1
because even if there is a cycle delay by the time we are worried about xVALID
being set high again happening handshake_occurred
will be high in that cycle and prevent a toggling of xVALID
sampsyo commented
This sounds perfect. That new definition for handshake_occurred
crystalizes what's going on here: we want to keep track of the moment when the handshake happened, by that natural "valid && ready" definition, and use that to suppress future handshakes.
nathanielnrn commented
Closing this for now, leaving the refactoring item open as something that can be done with a later issue.