# Lab3 FIR code review

Appreciate Jiin Lai for reviewing my lab3 verilog code and some suggestions.

[TOC]

## 1. AXI-Lite misunderstanding

- In Lab3, since we only load in coefficients for one time, so I let `awready` and `wready` be constant high.
- It may be ok in lab3, but it will have some errors in furture design.


- Here is my improved design:


    assign tmp_rvalid = (arvalid | rvalid & ~rready); // set by ARVALID, reset by RREADY
    always @(posedge axis_clk) RVALID  <= (tmp_rvalid)?        1 : 0;
    always @(posedge axis_clk) ARREADY <= (arvalid)?           1 : 0;
    always @(posedge axis_clk) AWREADY <= (awvalid && wvalid)? 1 : 0;    
    always @(posedge axis_clk) WREADY  <= (awvalid && wvalid)? 1 : 0;

    assign awready = AWREADY;
    assign arready = ARREADY; 
    assign wready  = WREADY;
    assign rvalid  = RVALID;

### awready/wready

1. `awvalid` means that the write-in address can be used.
2. `wvalid` means that the write-in data can be used.
3. We need both address and data when we write data into BRAM.
- Therefore, for `awready` and `wready`, they can assert **after** `awvalid` and `wvalid` are sampled.
---

### arready/rvalid

1. `rvalid` may fall after `rready` is asserted (reset by rready).
2. Should be asserted after `arvalid` is sampled.
- Therefore, we can known that `rvalid` is set by `arvalid` and reset by `rready`.
- `arready` asserted after `arvalid` is sampled.
---

## 2. Xin/Yout 3T

- `sm_tvalid` asserted 3T later than `ss_tready`


### Explanation

1. We can run the HLS co-simulation of lab2 FIR, and we will get the waveform of FIR without operation pipelining.


2. After we pipeline our operation like following:


3. The waveform should be like:


4. The reason of 3T delay:


### Verilog code:


## 3. Handle ap_ctrl independently

### Before:


### After:

    always @* begin
        /*----- ap_idle -----*/
        if (ap_state == `AP_IDLE)
            ap_ctrl[2] = 1;
        else
            ap_ctrl[2] = 0;
    
        /*----- ap_start ----*/
        if (ap_state == `AP_IDLE && awaddr = 12'd0
        && wdata[0] == 1 && tlast_cnt != data_length)
            ap_ctrl[0] = 1;
        else
            ap_ctrl[0] = 0;
        
        /*----- ap_done -----*/
        if (sm_tvalid && sm_tlast) // since TLAST cannot be independently used without qualified by VALID.
            ap_ctrl[1] = 1;
        else if (ap_state == `AP_DONE)
            ap_ctrl[1] = 1;
        else
            ap_ctrl[1] = 0;
    end

---
## 4. Status signal v.s. Control signal

- The TVALID and TREADY handshake determines when information is passed across the interface
- Note: All information (TLAST, TKEEP, TSTROB … ) can not be independently used without qualified by TVALID
- **We should distinguish Status signal v.s. Control signal. Status signal must be qualified by Control signal before used.**

    always @* begin
        case (ss_state)
        `SS_IDLE:
        begin
            if (ss_tvalid && ss_tlast) begin
                next_ss_state = `SS_DONE;
                ss_idle       = 1;
            end
            else begin
                next_ss_state = `SS_IDLE;
                ss_idle       = 1;
            end
        end
        `SS_DONE:
        begin
            if (ss_tvalid) begin
                next_ss_state = `SS_IDLE;
                ss_idle       = 1;
            end
            else begin
                next_ss_state = `SS_DONE;
                ss_idle       = 0;
            end
        end
    end

## 5. Tolerate the latency of Xn, Yn