How to combine a FSM with BRAM?

Question

My project is as follows: I want to save the pixel values coming via UART to BRAM first, then pass them through an image processing filter, and send them back via UART. Currently, I want this filter to be used not as an image processing filter but for shifting right. That is, I want it to divide the incoming pixel values by 2 and send them back. I managed to run this without using BRAM. However, when BRAM is involved, I'm not sure if my FSM (Finite State Machine) and BRAM work together properly.

I created a Simple Dual Port BRAM by reading the Vivado Design Suite User Guide Synthesis. The main difference was that a separate always block was used to write to and read from BRAM. So, I did the same.

module imfilter
    #(parameter D_BITS = 8, // RAM WIDTH
                N = 400) // RAM DEPTH
    (
    input logic i_clk,
    input logic reset,
    //(*DONT_TOUCH = "true"*)
    input logic [31:0] bleng, //byte length of array
    input logic [D_BITS - 1:0] i_data,
    input logic i_valid, // write en signal
    input logic i_rdy,   // read en signal
    output logic [D_BITS - 1:0] o_data,
    output logic o_send
    );
    
    localparam RAM_PERFORMANCE = "LOW_LATENCY"; // Select "HIGH_PERFORMANCE" or "LOW_LATENCY" 
    localparam INIT_FILE = "";

    typedef enum logic [1:0] {
        IDLE = 2'b00,
        SAVE2MEM = 2'b01,
        SEND = 2'b10
        //STOP = 2'b10
    } state_t;
    /*(*DONT_TOUCH = "true"*)*/ //(* ram_style = "block" *)
    logic [D_BITS - 1:0] img [0:N - 1];
    logic [D_BITS - 1:0] img_data, dout;
    logic [$clog2(N) - 1:0] addra = 0;  // write address  index
    logic [$clog2(N) - 1:0] addrb = 0;  // read address index
    //(*DONT_TOUCH = "true"*)
    state_t state_reg;
    
    

    always_ff @(posedge i_clk) begin      
        if (i_valid) begin
            img[addra] <= i_data; end
        if(i_rdy) begin
            dout <= img[addrb]; end
    end    

    always_ff @(posedge i_clk) begin
        if(reset) begin
            state_reg <= SAVE2MEM;
            o_send <= 0;;
            addra <= 0;
            addrb <= 0;
        end else begin    
        o_send <= 0;
        case(state_reg) 
            IDLE: begin
                if(bleng) begin
                    state_reg <= SAVE2MEM; end
            end
            SAVE2MEM: begin
                if(i_valid) begin
                   addra <= addra + 1;        
                   if(addra == bleng - 1) begin
                       addra <= 0;
                       state_reg <= SEND; end
                end        
            end
            SEND: begin
                if(i_rdy) begin
                    o_send <= 1;
                    addrb <= addrb + 1; 
                    if(addrb == bleng - 1) begin
                        state_reg <= IDLE;
                        addrb <= 0; end
                end else begin
                    o_send <= 0; end
            end

            default: begin
                state_reg <= IDLE;
            end
        endcase end
    end
    
    assign o_data = (i_rdy)? dout : {D_BITS{1'bz}};

    
endmodule

Now, here are my questions:

1. Do you think there is any issue with this code? Have I properly combined FSM with BRAM?
2. I don't want to directly send the value from BRAM to UART, but I want to subject it to a specific operation. Can I directly do this operation while assigning to the dout variable like dout <= img[addrb] >> 1? Or would it be more appropriate to assign it to dout and do this in the FSM?
3. I looked at Vivado's sample codes, but there was nothing that resets the output data when the output register is not used. That's why I decided to let it go to high impedance if the read operation is not enabled. Do you think this is correct? How to reset output data whenever reset button is pressed?

Answer 1

1. I recommend writing a testbench to verify that the code does what is needed every clock cycle. You discuss a downstream UART but do not show one. I would add the UART to the DUT and verify the RAM and UART models at the same time. If it does not do what's needed, then add the testbench to your post as part of a minimal-reproducible-example and explain the issue, or post a different question.

   From a code review perspective, I would not use initializers on variable declarations like this addra = 0;. See also rtl-pitfalls-stop-initializing-signals. The right thing to do is to reset addra using reset instead which you are already doing, so just I would just remove the = 0 part. Synthesis support depends on the tool, and simulation will report & error indicating multiple drivers because you have used always_ff which turns on additional checking.
   The errors produced by multiple drivers look like this: xmelab: *E,MULAXX (./design.sv,43|12): Multiple drivers to always_ff output variable addra detected. always_ff @(posedge i_clk) begin

   From a project design perspective, you could use a FIFO IP, so a state machine is not needed. It seems the RAM is just buffering data. Vivado and similar tools have built in IP generation capability which can be used to create a FIFO, and its simulation model. Run Vivado, then launch IP Catalog and search for FIFO. Its easier to use the IP that has already been debugged. In fact, I would consider going one step further and using a UART IP with built in FIFO's that has already been debugged, so that you are able to concentrate on the other aspects of your project. The IP Catalog or similar can generate a UART\w FIFO & corresponding testbench. There are also UARTs with FIFO available using web search like this one UART-FIFO

2. This statement dout <= img[addrb] >> 1 is valid Verilog for a right shift of 1. If that is the behavior you what then use it. Put it where it makes sense to you. I would probably keep it out of the SM; just my choice though. It seems like you have a lot of choices where to put this.

3. I don't see a reason to reset the memory output port. Valid data is produced after a known latency (probably one clk, would need to see the simulation :) ) after read_en is asserted. Capture the data read from the ram using the same read enable in the downstream RTL model (use the read_en as a data valid downstream). If you use this method the x values will not propagate into the downstream data flow, as qualified by valid. There are other things you could do to initialize like using a memory initialization file via $readmemh(), however its NOT necessary, unless you have a meaningful pattern (ex filter coefficients) to initialize the RAM with. There is also no reason to drive z in this case.