init branch develop by BIGWJZ · Pull Request #1 · datenlord/blue-dmac

BIGWJZ · 2024-07-04T02:13:52Z

types definition in src/pcieTypes.bsv & src/dmaTypes.bsv
a test case of AXI-Stream interface

myrfy001 · 2024-07-04T02:58:10Z

+
+endmodule
+
+module mkTbAxisWire(AxisFifo#(keepWidth, usrWidth) ifc);


can this module be replaced by mkConnection ?

It can be replaced. In fact, I planned to remove TestAxiStream because the AXIS interface already has a testbench in the blue-wrapper.

myrfy001 · 2024-07-05T14:53:05Z

+        inputFifo.deq;
+        let offset = getOffSet(request);
+        // firstChunkLen = offset % PCIE_TLP_BYTES
+        DmaMemAddr firstLen = zeroExtend(offset[valueOf(PCIE_TLP_BYTES_WIDTH)-1:0]);


you have to read the PG213 to find out how to get the Max Payload Size(MPS) and Max Read Request Size(MRRS), those value is not const so you should do some refacrtor.

MPS&MRRS are passed in the Configuration Status Interface Port . They should be parameters of my module.

myrfy001 · 2024-07-08T03:24:51Z

-        DmaMemAddr offset = zeroExtend(fromInteger(valueOf(BUS_BOUNDARY)) - pack(request.startAddr[BUS_BOUNDARY_WIDTH-1:0]));
-        return offset;
+        // 4096 - startAddr % 4096
+        Bit#(BUS_BOUNDARY_WIDTH) remainder = truncate(request.startAddr);


Bit#(BUS_BOUNDARY_WIDTH) has occured more than once, so use typedef to define a new type.
Duplicated code is a kind of bad smell in your codebase

myrfy001 · 2024-07-08T03:27:37Z

    function DmaMemAddr getOffset(DmaRequestFrame request);
-        DmaMemAddr offset = zeroExtend(fromInteger(valueOf(BUS_BOUNDARY)) - pack(request.startAddr[BUS_BOUNDARY_WIDTH-1:0]));
-        return offset;
+        // 4096 - startAddr % 4096


4096 is bus boundary, not tlp boundary. are you massing them up?

Here, the calculated offset is relative to BUS_BOUNDARY. The offset will be sliced to PCIE_TLP_BYES in the getFirstChunkLen rule.

Is there any need to split by BUS_BOUNDARY? If you split by PCIE_TLP_BYES , since PCIE_TLP_BYES <= BUS_BOUNDARY, any slice that aligned to PCIE_TLP_BYES won't cross BUS_BOUNDARY.

You are right. I have modfied.

myrfy001 · 2024-07-08T03:29:27Z

-                end
-        end else begin
+        if (isSplittingReg) begin   // !isFirst
+            if (totalLenRemainReg <= fromInteger(valueOf(PCIE_TLP_BYTES))) begin 


In a previous comment, I have mentioned that the TLP size is a variable, not a const. This is a very important point. You must fix this issue before moving on.

myrfy001 · 2024-07-08T03:30:48Z

+    endrule
+
+    rule testFinish ;
+        if (testCntReg == fromInteger(valueOf(TEST_NUM))) $finish();


Don't write in one line.

myrfy001

the TLP size is a variable, not a const

myrfy001 · 2024-07-09T02:08:03Z

+typedef TLog#(DEFAULT_TLP_SIZE)             DEFAULT_TLP_SIZE_WIDTH;
+typedef 3                                   PCIE_TLP_SIZE_SETTING_WIDTH;
+typedef Bit#(PCIE_TLP_SIZE_SETTING_WIDTH)   PcieTlpSizeSetting;      
+typedef enum {DMA_RX, DMA_TX}               TRXDirection deriving(Bits, Eq);


Do not write in one line.

myrfy001 · 2024-07-14T01:09:05Z

+endfunction
+
+// Concat two DataStream frames into one. StreamA.isLast must be True, otherwise the function will return a empty frame to end the stream.
+function Tuple3#(DataStream, DataStream, DataBytePtr) getConcatStream (DataStream streamA, DataStream streamB, DataBytePtr bytePtrA, DataBytePtr bytePtrB);


please do defensive coding. Add an assert here since you have requirements for input params.
It will save you from endless debugging.

see

https://github.com/datenlord/blue-rdma/blob/ffdcc150ae50cd6c94aee6f683d5f2133a0c662e/src/PrimUtils.bsv#L148

and

https://github.com/datenlord/blue-rdma/blob/ffdcc150ae50cd6c94aee6f683d5f2133a0c662e/src/PayloadConAndGen.bsv#L999

myrfy001 · 2024-07-14T01:10:40Z

 } DmaCsrFrame deriving(Bits, Bounded, Eq, FShow);

+typedef enum {
+    DMA_RX, DMA_TX


myrfy001 · 2024-07-14T01:11:30Z


+typedef enum {
+    DMA_RX, DMA_TX
+} TRXDirection deriving(Bits, Eq);


suggest you deriving FShow whenever possible. It helps print debug info.

myrfy001 · 2024-07-14T01:16:22Z

+    return tuple3(concatStream, remainStream, remainBytePtr);
+endfunction
+
+function Action showDataStream (DataStream stream);


You should make DataStreamderiving FShow, and you can simply display it anyware.

And if you insist on using showDataStream , then you may use it many place in your codebase. Why put it in this file?

I did not notice Fshow before. I will replace showDataStream funciton with it.

myrfy001 · 2024-07-14T01:16:55Z

+    endaction;
+endfunction
+
+function Action checkDataStream (DataStream stream, String name);


The same as above, it's a util function, right?

myrfy001 · 2024-07-14T01:21:49Z

+    ByteEn concatByteEnA = streamA.byteEn;
+
+    // Fill the high bytes by streamB data
+    Data concatDataB = streamB.data << bitPtrA;


Please align them to make the code pretty. I know there are a lot of code to change, but if you make it a habbit, then you will write beautiful code in the future.

Data concatDataB = streamB.data << bitPtrA; ByteEn concatByteEnB = streamB.byteEn << bytePtrA; Data concatData = concatDataA | concatDataB; ByteEn concatByteEn = concatByteEnA | concatByteEnB;

myrfy001 · 2024-07-14T01:33:33Z

+    FIFOF#(DataStream) outputFifo <- mkFIFOF;
+
+    FIFOF#(StreamWithPtr) prepareFifoA <- mkFIFOF;
+    FIFOF#(StreamWithPtr) prepareFifoB <- mkFIFOF;


The above 5 lines of code looks good.

you may ask why we need to make code pretty. In fact, it's not a problem of pretty or not, but aligned code can make reviewer's life easy. When you will review a lot of code, you will know what it feels like. For better aligned code, you can glimpse the code and won't think much of it, if the code is badly aligned, you brain will take extra effort to "decode" the code you are reading, which will make the reviewer tired for a lot of code to review.

For example, we do not allow to write code inline, and force you to write each element in a new line in an enum or struct. This is because when you are reviewing, your eyes will first look top down, trying to find the boundary of a piece of code. For an if statement, if you write like:

if () begin // xxxxx end else begin // xxxxx end

you will quickly get the boundary and with a simple glimpse you know the if statement has a else branch. But if you write

if () begin // xxxxx end else begin // xxxxx end

your eyes have to move right a little to find that there is a else statement. This is extra work for your brain.

Understand. I will pay attention to the code format.

myrfy001 · 2024-09-24T01:42:56Z

+    // Dma To Adapter DataStreams
+    interface Vector#(DMA_PATH_NUM, FifoIn#(DataStream))     dmaDataFifoIn;
+    interface Vector#(DMA_PATH_NUM, FifoIn#(SideBandByteEn)) dmaSideBandFifoIn;
+    // Adapter To Dma StraddleStreams, which may contains 2 TLP


for different group of interface, add a line of space here will be better.

myrfy001 · 2024-09-24T01:44:09Z

+    Reg#(Bool) roundRobinReg <- mkReg(False);
+
+    function Bool hasStraddleSpace(DataStream sdStream);
+        return !unpack(sdStream.byteEn[valueOf(STRADDLE_THRESH_BYTE_WIDTH)]);


there are functions called msb and lsb to simplify this line of code.

myrfy001 · 2024-09-24T01:44:56Z

+    endfunction
+
+    function Bool isValidShiftStream(DataStream shiftStream);
+        Bool valid = !unpack(shiftStream.byteEn[0]) && unpack(shiftStream.byteEn[valueOf(STRADDLE_THRESH_BYTE_WIDTH)]);


The same as above.

myrfy001 · 2024-09-24T01:49:15Z

+
+function RawPcieAxiStreamMaster#(usrWidth) convertRawBusToRawPcieAxiStreamMaster(
+    RawBusMaster#(PcieAxiStream#(usrWidth)) rawBus
+);


pay attention to align

…er many bytes

…re test pattern

BIGWJZ added 2 commits July 3, 2024 21:04

Initialize base types

f3d3de9

init branch develop

03eb410

BIGWJZ force-pushed the main branch from f3d3de9 to 545f8a7 Compare July 4, 2024 02:30

myrfy001 reviewed Jul 4, 2024

View reviewed changes

Inplement PCIe Interface Types

765d837

BIGWJZ closed this Jul 4, 2024

Inplement PCIe Interface Types

f259b91

BIGWJZ reopened this Jul 4, 2024

myrfy001 reviewed Jul 4, 2024

View reviewed changes

Comment thread src/PcieTypes.bsv Outdated

Comment thread src/PcieTypes.bsv Outdated

update mkChunkComputer

a4a8d12

myrfy001 reviewed Jul 5, 2024

View reviewed changes

Add mkChunkComputeTb and test pass

2cc532c

myrfy001 reviewed Jul 8, 2024

View reviewed changes

myrfy001 requested changes Jul 8, 2024

View reviewed changes

BIGWJZ added 2 commits July 8, 2024 22:48

Dynamic TLP Max Payload Size

89dfb11

Dynamic TLP Max Payload Size

85baea7

myrfy001 reviewed Jul 9, 2024

View reviewed changes

BIGWJZ added 8 commits July 9, 2024 16:55

Test various MPS settings and verify timing

00a6386

Add StreamUtils::StreamConcat

5e6b009

a simple test:

75ebadb

a simple test

1b76335

Test Pass

ebc614f

Update StreamUtils

d20503f

Add StreamSplit

9c0ac45

Test StreamUtils Pass

4f92212

myrfy001 reviewed Jul 14, 2024

View reviewed changes

BIGWJZ added 4 commits July 15, 2024 02:59

Update all modules according to the review

44e0f8a

Update StreaUtils

3267090

update PcieTypes

9fb805d

Add CompleterRequest

0c9018d

myrfy001 reviewed Sep 24, 2024

View reviewed changes

BIGWJZ and others added 29 commits October 8, 2024 20:16

Add Simple Mode

7af1a43

solve h2c r&w

f9e46fa

add cocotb bar tb

875df73

pass simple mode tb

d4d5a4c

Add read-write loop tb

15ef729

optimize for fully-pipeline

10666a5

Add blue-rdma style interface

35c503d

Add blue-rdma style interface

cb50bdc

avoid same name file in blue-rdma

e146e69

Fix User Bar Problems

cb2073c

Fix C2HPipe.reshapeMRRS and cocotb.loop

30f7649

Fix TlpOutMux

0c5ac0d

Add DummyCsr of Bypass Mode

3e9e8f6

feat: rewrite cplt buffer

9361b16

fixbug: Read stall

6edfb09

fix: can test on hardware with vu13p, but still will hang when transf…

e161014

…er many bytes

fix: inflight counter width error

b951c0c

fix: make reshape fully pipelined and fix test blocking bug

8eb787d

update test script

79eca48

update dma test hardware module

55f76a1

fix 2 bugs in stream processing

be7ab50

add cocotb correctness test

1071fc2

fixbug: chunk will exceed MPS (TODO: make cocotb check TLP oversize)

6eb35db

feat: add support for tlp attributes, and make test module support mo…

abf8784

…re test pattern

improve double channel write only test

b9007c2

refactor: add prefix for filenames

79e114e

feat: make some input FIFO to LFIFO

ee999d2

make more fifo to LFIFO

1eab114

optmize code, merge two module to save latency and resource usage

e103aa0


		endmodule

		module mkTbAxisWire(AxisFifo#(keepWidth, usrWidth) ifc);

Conversation

BIGWJZ commented Jul 4, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BIGWJZ Jul 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myrfy001 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BIGWJZ Jul 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

BIGWJZ Jul 14, 2024 •

edited

Loading

myrfy001 left a comment •

edited

Loading

BIGWJZ Jul 14, 2024 •

edited

Loading