Conversation
Lore0599
changed the title
Lore0599
force-pushed
the
lleone/feature-vc
branch
8 times, most recently
from
6e00863 to
a38af31
Compare
Lore0599
force-pushed
the
lleone/feature-vc
branch
2 times, most recently
from
4bd53ea to
62da4d9
Compare
fischeti
left a comment
fischeti
left a comment
There was a problem hiding this comment.
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.
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.
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.
I would argue that this is already done in the chimney in the packing:
Lines 897 to 899 in 439a2e9
There was a problem hiding this comment.
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.
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
Lore0599
force-pushed
the
lleone/feature-vc
branch
9 times, most recently
from
358a726 to
8fbd54b
Compare
Lore0599
force-pushed
the
lleone/feature-vc
branch
2 times, most recently
from
c33fcfc to
4c90eda
Compare
Lore0599
force-pushed
the
lleone/feature-vc
branch
from
4c90eda to
294324e
Compare
2 participants
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.