-
Notifications
You must be signed in to change notification settings - Fork 52
hw: Wide read and write decoupling #153
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
base: develop
Are you sure you want to change the base?
Conversation
71a7468 to
79cb84c
Compare
56c4ab1 to
c0aaa25
Compare
c0aaa25 to
3801749
Compare
6e00863 to
a38af31
Compare
b676aa9 to
4bd53ea
Compare
4bd53ea to
62da4d9
Compare
fischeti
left a comment
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 efforts in cleaning this up! I still need to go through the chimney in more detail, but I already have a round of comments. But I think we should be able to merge it eventually.
On general comment I have is that the parametrization of the decoupling feature is not really consistent between chimney and router. Or at least it is not entirely clear to me how to make them compatible. It would maybe be better to create a new enum:
typedef enum logic[1:0] {
None;
Vc;
Phys;
} wide_rw_decouple_e;and use this throughout the project to calculate the number of physical/virtual channels.
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.
Not really required, but since this wrapper is a nice playground for backend trials, it could be nice to also include the network interface in it for the local ports.
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 totally agree, but since this will require to make some PnR trials I would move this feature to a different PR
hw/floo_nw_router.sv
Outdated
| for (genvar i = 0; i < NumInputs; i++) begin: gen_credit_tied_zero_req | ||
| assign floo_req_o[i].credit = '0; | ||
| end | ||
| // Narrow rsp links never rely on credit based flow | ||
| for (genvar i = 0; i < NumOutputs; i++) begin: gen_credit_tied_zero_rsp | ||
| assign floo_rsp_o[i].credit = '0; | ||
| end |
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 would argue that this is already done in the chimney in the packing:
Lines 897 to 899 in 439a2e9
| always_comb begin | |
| floo_narrow_aw = '0; | |
| floo_narrow_aw.hdr.rob_req = narrow_aw_rob_req_out; |
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 signal floo_narrow_aw enters the module (i_req_wormhole_arbiter)[https://github.com/pulp-platform/FlooNoC/blob/439a2e9b6a37e57225bf042df7d36fa052f9db83/hw/floo_nw_chimney.sv#L1110-L1116].
This arbiter drives only valid, ready, and the data signals, leaving the output credit signal undriven.
For clarity, I can move the tie-off of credit to zero directly into the chimney.
hw/floo_router.sv
Outdated
| if (VcImplementation == floo_pkg::VcCreditBased) begin: gen_credit_support | ||
| assign credit_o[in][v] = credit_gnt_q[in][v]; | ||
| assign credit_gnt_d[in][v] = in_valid[in][v] & in_ready[in][v]; | ||
| `FF(credit_gnt_q[in][v], credit_gnt_d[in][v], 1'b0); | ||
| end else begin: gen_no_credit | ||
| assign credit_o[in][v] = 1'b1; |
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.
Actually, we could just reuse the ready signal to return the credit, no? This would make the typedefs a bit easier. Because afaik, the ready is not used in credit-based flow control, and the credit is in the end just a delayed ready
da84226 to
95b95c7
Compare
95b95c7 to
7135480
Compare
f6e6987 to
c2aeef3
Compare
5fd75de to
b0a5ba5
Compare
2dc586c to
358a726
Compare
358a726 to
8fbd54b
Compare
This PR introduces mechanisms to decouple the AXI read and write wide channels:
HW Changes:
floo_vc_arbiter:floo_nw_routeris configured withNumWidePhysChannels = 2andNumWideVirtChannels = 2, the wide router behaves as two separate routers: one for read streams and one for write streams.floo_vc_arbitercan now be implemented using a credit-based approach. This requires properInputFifoparameterization to sustain full NoC throughput.floo_nw_chimney: The chimney has been modified to support 2 inputs/outputs wide links. This is needed to decouple read and write streams.