-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add round-robin distributor #127
base: master
Are you sure you want to change the base?
Conversation
src/rr_distributor.sv
Outdated
valid_o = '0; | ||
ready_o = 1'b0; | ||
if (ready_i[rr_q]) begin | ||
valid_o[rr_q] = valid_i; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not guarantee that a valid_o
remains high once asserted until the handshake, correct? Many of our modules that work with valid/ready assume this property (borrowed from AXI handshakes), because it makes handling inputs much easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it? valid_o
will only be high if ready_i
is high (and valid_i
is high), meaning that whenever valid_o
is high the handshake completes, and isn't high otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are totally right, I don't remember why I asked this. 🙂
But I see another problem with this code: For the output handshake, valid_o
depends on ready_i
, but the rule in valid/ready handshaking is that ready
can depend on valid
, but not vice-versa (to prevent combinatorial loops).
Fortunately, the fix should be relatively simple:
always_comb begin
valid_o = '0;
valid_o[rr_q] = valid_i;
end
assign ready_o = ready_i[rr_q];
src/rr_distributor.sv
Outdated
|
||
always_comb begin : rr_next | ||
rr_d = rr_q; | ||
if (valid_i && one_ready) begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional that the counter increases in cycles when there is no handshake? Or should this condition be valid_i && ready_i[rr_q]
(or, equivalently after the change above, valid_i && ready_o
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thommythomaso is this intentional?
It being intentional would make sense to me, as in this case a valid will be served within a few cycles if any subordinate is available, even if the next in the rr distribution scheme is not ready yet (e.g. stalled for 100s of cycles). With the change discussed above, this may cause deassertion of the valid signal however, which would be against the ready/valid rules. However, not implementing the change above is also against these rules. I currently don't see a clean way to implement this feature while conforming to ready/vaild handshake rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. I suggest we clarify what "forwards requests to individual outputs in a round robin fashion" means in conjunction with valid
/ready
handshaking.
If that is not possible or does not make sense, an option could be to relax to req
/gnt
handshaking (where req
can be deasserted without gnt
, but req
must still not depend on gnt
).
e41f024
to
5d761f0
Compare
src/rr_distributor.sv
Outdated
/// Setting StrictRR enforces a strict round-robin distribution | ||
/// If set to 1'b1, the rr_distributor will distribute the requests to the next output | ||
/// in line, irrespective if this output is ready or not, irrespective if another | ||
/// output could accept the request. The transaction will wait until the next one in | ||
/// line accepts the handshake. | ||
/// If set to 1'b0, the rr_distributor will step through the outputs if one is ready | ||
/// but the current one is not. This can reduce wait time for the input. | ||
/// **THIS IS NOT COMPLIANT AS IT MAY DE-ASSERT VALID WITHOUT A PROPER HANDSHAKE** | ||
parameter bit StrictRR = 1'b0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification!
Now, is StrictRR = 1'b0
an actual use case? Also asking @thommythomaso?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior outlined by StrictRR = 1'b0
is currently used in the HERO dma implementation as far as I know, allowing for automatic distribution of dma transfers of different lengths. This way, if many requests are issued (> number of channels) and some are extremely long, the next transfer will be issued as soon as a dma is available, instead of waiting for the next channel in line to be available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, thanks for the explanation.
Wouldn't it make sense to reverse the handshake on the output interface? That is
// output stream
input logic [NumOut-1:0] req_data_valid_i,
output logic [NumOut-1:0] req_data_ready_o,
output data_t [NumOut-1:0] data_o,
output idx_t sel_o
Downstream modules could then request data through req_data_valid_i
as soon as they are available, and the rr_distributor
can select one of them and set its req_data_ready_o
.
- change `payload_t` to `data_t` - replace inline if with always comb block - add rr_distributor to src_files
27de944
to
3e433be
Compare
No description provided.