Clean up AXI generator channels #1867
Conversation
Renamed some registers, simplified/corrected(?) the was_high signals Assert a few properties we expect from our yxi input. Still TODO: refactor out shared channels/code
Yep, makes sense! I guess the milestone that will enable this is when we have the Python-generated code passing the same test as the original code (and not necessarily any other tests)… at which point we can very confidently delete the latter. |
| @@ -78,24 +78,21 @@ component m_arread_channel( | |||
| // this contains blocking logic previously in its own group | |||
| group do_ar_transfer { | |||
| //assert ARVALID as long as this is the first time we are asserting it | |||
| is_arvalid.in = !arvalid_was_high.out ? 1'b1; | |||
| is_arvalid.in = !ar_handshake_occurred.out ? 1'b1; | |||
There was a problem hiding this comment.
This now seems quite easy to understand, with the new name in mind!
| @@ -446,21 +443,21 @@ component m_awwrite_channel( | |||
| // this contains blocking logic previously in its own group | |||
| group do_aw_transfer { | |||
| //assert AWVALID | |||
| is_awvalid.in = !awvalid_was_high.out ? 1'b1; | |||
| is_awvalid.in = !(is_awvalid.out & AWREADY) & !aw_handshake_occurred.out ? 1'b1; | |||
There was a problem hiding this comment.
I think the way to read this is:
This channel becomes valid whenever the remote side tells us it's ready. But we only do that when we were not previously valid, and we only do the handshake once during the execution of this group.
I guess one small question is: do we need !is_awvalid.out as part of this condition? That is, it seems like the condition AWREADY & !aw_handshake_occurred.out might suffice, because it will "suppress validity" on every cycle after we are valid for a single cycle.
There was a problem hiding this comment.
I think this is nice in that it helps point to the idea that .in is partitioned by our guards. In other words, it hopefully makes the application of De Morgan's more obvious. Another thing is that this will make is_awvalid.in depend on AWREADY in the abstract sense, which goes against the spirit of the recommendation we have discussed previously.
Also, I believe there is a subtle dependency on is_awvalid.out
is_awvalid.out |
AWREADY |
aw_handshake_occurred.out |
Desired input |
|---|---|---|---|
| 0 | 0 | 0 | 1 |
| 0 | 0 | 1 | 0 |
| 0 | 1 | 0 | 1 |
| 0 | 1 | 1 | 0 |
| 1 | 0 | 0 | 1 |
| 1 | 0 | 1 | 0 |
| 1 | 1 | 0 | 0 |
| 1 | 1 | 1 | 0 |
If the above is correct(?) we care about is_awvalid.out when {AWREADY, aw_handshake_occurred.out} is 10. Thinking about this case, is_awvalid.out can probably help us save a cycle, by making AWVALID low immediately after the handshake occurs, as opposed to waiting 2 cycles for aw_handshake_occurred.out to propagate.
There was a problem hiding this comment.
Aha, I'm pretty sure that makes sense! Thanks for clarifying. The crux for me here was that AWREADY gives us the signal to stop the handshake one cycle earlier than !is_awvalid.out.
| // but for now we will be explicit and reduce this in generation maybe. Not sure | ||
| // it even matters. | ||
| // This makes AWVALID go low after a single cycle. Without it it stays high for 2. | ||
| is_awvalid.in = is_awvalid.out & AWREADY & awvalid_was_high.out ? 1'b0; | ||
| is_awvalid.in = (is_awvalid.out & AWREADY) | aw_handshake_occurred.out ? 1'b0; |
There was a problem hiding this comment.
The condition seems like the "De Morgan's negative" of the other condition for is_awvalid.in. Perhaps, in the generator, we want a utility to emit this pattern:
lhs = cond ? 1;
lhs = !cond ? 0;
…with a single call to, like, assign_logic(lhs, cond) or something like that. Could make things a little more readable at the generator level.
There was a problem hiding this comment.
Note to self: Maybe something like partition_assignment(lhs, cond)?
| wvalid.in_ = (~(wvalid.out & WREADY) & ~w_handshake_occurred.out) @ 1 | ||
| wvalid.in_ = ((wvalid.out & WREADY) | w_handshake_occurred.out) @ 0 |
There was a problem hiding this comment.
Here's a use case for that assign_logic utility I was pondering above…
|
Merging this as is. Will look out for more use-cases that would benefit from the |
* Clean up AXI generator channels Renamed some registers, simplified/corrected(?) the was_high signals Assert a few properties we expect from our yxi input. Still TODO: refactor out shared channels/code * Rename `*_addr` registers in hardcoded axi
See #1865.
This PR touches both the hardcoded calyx implementation as well as the generator.
Doing this PR has made me realize we should start making efforts to get rid of the hardcoded implementation as things will pretty easily drift out of sync otherwise.
base_addrandcurr_addrtocurr_addr_axiandcurr_addr_interal_memrespectivelywas_highsignals, see the issue for more details. This is the thing I'm most unsure of w.r.t the correctness of the implementation. This ended up deviating a bit from the talk @sampsyo and I had. If anyone takes a look at this part I'd be happy to hear any and all feedback/thoughts.After this there is still a need to refactor out shared channels in the generator.