Simple SystemVerilog AXI controller

Question

I have a more long-term project I'm using to learn FPGA/HDL, and this is first sub-component of it used for testing. I'm targeting Zynq device.

I'd like to create a component which creates an image. For it, I created a simple controller which exposes a simple AXI 4 Lite slave interface (conf) the configuration outputs to core logic (hsize, vsize) and control signals (stop and idle). For debugging purposes, I included 60Hz counter input both to logic (included as watermark) and controller (included in register).

Main code:

import axi_lite_pkg::*;

module videotest_config(
    axi_lite.slave      conf,
    
    input        [15:0] cnt,
    
    output logic [15:0] vsize,
    output logic [15:0] hsize,
    
    input               idle,
    ack_sign.master     stop,
    output logic        intr
    );

    typedef enum {
        WORD_CONFIG,
        WORD_STATE,
        WORD_HSIZE_VSIZE,
        WORD_MAX
    } WORDS;

    logic [31:0] awaddr;
    logic [31:0] wdata;
    logic        write_addr_done;
    logic        write_data_done;
    logic        write_done;
    resp         bresp;
    logic [31:0] araddr;
    logic        read_addr_done;
    logic        read_done;
    logic [31:0] rdata;
    resp         rresp;
    task reset();
        write_addr_done <= 0;
        write_data_done <= 0;
        write_done <= 0;
        read_addr_done <= 0;
        read_done <= 0;
        reset_config();
        reset_state();
        reset_hsize_vsize();
    endtask;
    initial
    begin
        reset();
    end
    always_ff @(posedge conf.clk or negedge conf.reset)
    begin
        if (conf.reset == 0) begin
            reset();
        end else begin
            handle_write();
            handle_read();
        end
    end
    always_ff @(posedge conf.clk) //latch
    begin
        if (!write_addr_done) begin
            awaddr <= conf.awaddr;
        end
        if (!write_data_done) begin
            wdata <= conf.wdata;
        end
        if (!read_addr_done) begin
            araddr <= conf.araddr;
        end
    end
    
    assign conf.awready = !write_addr_done;
    assign conf.wready = !write_data_done;
    assign conf.bvalid = write_done & !conf.reset;
    assign conf.bresp = bresp;
    assign conf.arready = !read_addr_done;
    assign conf.rvalid = read_done;
    assign conf.rdata = rdata;
    assign conf.rresp = rresp;
    
    function automatic logic [2 + $clog2(WORD_MAX) + 2 - 1:0] transform_addr(input logic [31:0] addr);
         // Append LSB as MSB to fallback to default case in switch when they are set
        return {addr[1:0], addr[2 + $clog2(WORD_MAX) - 1:2]};
    endfunction
    
    task automatic handle_write();
        logic _write_addr_done = write_addr_done || conf.awvalid;
        logic _write_data_done = write_data_done || conf.wvalid;
        if (!write_done) begin;
             write_addr_done <= _write_addr_done;
             write_data_done <= _write_data_done;
        end
        if (!write_done & _write_addr_done & _write_data_done) begin
             case (transform_addr(awaddr))
             WORD_CONFIG: write_config();
             WORD_STATE: write_state();
             WORD_HSIZE_VSIZE: write_hsize_vsize();
             default: begin
                 bresp <= SLVERR;
                 write_done <= 1;
             end
             endcase
         end
         if (write_done) begin
              write_addr_done <= conf.bready;
              write_data_done <= conf.bready;
              write_done <= conf.bready;
              if (conf.bready) begin
                   bresp <= RESP_X;
              end
         end
    endtask

    task automatic handle_read();
        logic _read_addr_done = read_addr_done || conf.arvalid;
        if (!read_done) begin
             read_addr_done <= _read_addr_done;
        end 
        if (!read_done & _read_addr_done) begin
            case (transform_addr(araddr))
            WORD_CONFIG: read_config();
            WORD_STATE: read_state();
            WORD_HSIZE_VSIZE: read_hsize_vsize();
            default: begin
                rresp <= SLVERR;
                rdata <= 'X;
                read_done <= 1;
            end
            endcase
        end
        if (read_done) begin
            read_done <= !conf.rready;
            read_addr_done <= !conf.rready;
            if (conf.rready) begin
                rresp <= RESP_X;
                rdata <= 'X;
            end
        end
    endtask
    
    // Word 0: Config
    //       0: Interrupt on idle
    //       1: Interrupt on stop
    //       2: Stop
    //   3..31: Reserved (must be 0)
    logic intr_on_idle;
    logic intr_on_stop;
    logic stop_reg;
    task automatic reset_config();
        intr_on_idle <= 0;
        intr_on_stop <= 0;
        stop_reg <= 1;
    endtask
    
    task automatic write_config();
        if (wdata[31:3] != 0) begin
            bresp <= SLVERR;
            write_done <= 1;
        end else begin
            intr_on_idle <= wdata[0];
            intr_on_stop <= wdata[1];
            stop_reg <= wdata[2];
            bresp <= OKAY;
            write_done <= (wdata[2] == stop.din_ack);
        end
    endtask
    assign stop.din = stop_reg;
    
    task automatic read_config();
        rdata[0] <= intr_on_idle;
        rdata[1] <= intr_on_stop;
        rdata[2] <= stop.din_ack;
        rdata[31:3] <= '0;
        rresp <= OKAY;
        read_done <= 1;
    endtask
    
    assign intr = (intr_on_idle & idle) | (intr_on_stop & idle & stop.din_ack);
    
    // Word 1: State [RO]
    //   0..15: Count
    //      16: Idle
    //  16..31: Reserved
    task automatic reset_state();
    endtask
    
    task automatic write_state();
        bresp <= SLVERR;
        write_done <= 1;
    endtask

    task automatic read_state();
        rresp <= OKAY;
        rdata[15: 0] <= cnt;
        rdata[16]    <= idle;
        rdata[31:17] <= 0;
        read_done <= 1;
    endtask
    
    // Word 2: HSize VSize
    //   0..15: HSize
    //  16..31: VSize
    task automatic reset_hsize_vsize();
        hsize <= 0;
        vsize <= 0;
    endtask
    
    task automatic write_hsize_vsize();
        bresp <= OKAY;
        hsize <= wdata[15: 0];
        vsize <= wdata[31:16];
        write_done <= 1;
    endtask
    
    task automatic read_hsize_vsize();
        rresp <= OKAY;
        rdata[15: 0] <= hsize;
        rdata[31:16] <= vsize;
        read_done <= 1;
    endtask
endmodule

AXI Lite definition:

package axi_lite_pkg;
typedef enum logic [1:0] {
    OKAY = 2'b00,
    EXOKAY = 2'b01,
    SLVERR = 2'b10,
    DECERROR = 2'b11,
    RESP_X = 'X
} resp;
endpackage : axi_lite_pkg

import axi_lite_pkg::*;

interface axi_lite(
    input clk,
    input reset
    );
    parameter ADDR_WIDTH = 32;
    parameter DATA_WIDTH = 32;
    localparam AW = ADDR_WIDTH - 1;
    localparam DW = DATA_WIDTH - 1;
    
    
    wire [AW:0] awaddr;
    wire        awvalid;
    wire        awready;
    wire [DW:0] wdata;
    wire        wvalid;
    wire        wready;
    resp        bresp;
    wire        bvalid;
    wire        bready;
    wire [AW:0] araddr;
    wire        arvalid;
    wire        arready;
    wire [DW:0] rdata;
    resp        rresp;
    wire        rvalid;
    wire        rready;
    modport master(
        input clk, input reset,
        output awaddr, output awvalid, input awready,
        output wdata, output wvalid, input wready,
        input bresp, input bvalid, output bready,
        output araddr, output arvalid, input arready,
        input rdata, input rresp, input rvalid, output rready);
    modport slave(
        input clk, input reset,
        input awaddr, input awvalid, output awready,
        input wdata, input wvalid, output wready,
        output bresp, output bvalid, input bready,
        input araddr, input arvalid, output arready,
        output rdata, output rresp, output rvalid, input rready);
endinterface

Finally ack_sign is supposed to allow sending signals to different clocking domains waiting until it will be received by it.

interface ack_sign;
    wire din;
    wire din_ack;
    modport master(output din, input din_ack);
    modport slave(input din, output din_ack);
endinterface

module multiflipflop_ack(
    ack_sign.slave din,
    wire dout,
    wire clk_in,
    wire clk_out
    );
    logic reg_in_1;
    logic reg_in_2;
    logic reg_out_1;
    logic reg_out_2;
    
    always_ff @(posedge clk_out)
    begin
        reg_in_1 <= din.din;
        reg_in_2 <= reg_in_1;
    end
    always_ff @(posedge clk_in)
    begin
        reg_out_1 <= reg_in_2;
        reg_out_2 <= reg_out_1;
    end
    assign dout = reg_in_2;
    assign din.din_ack = reg_out_2;
endmodule

So far I just moved to proper testing but at least something seems to be working in simulator. However, I was more of a software person, and I picked FPGA as a hobby - code was created by trial and error (I can debug on my own - comments regarding style/best practice would be more valuable).

toolic · Accepted Answer · 2024-10-18 16:39:48Z

From a style/best practice your code looks pretty good. The only glaring error is that your module multiflipflop_ack is missing port directions.

module multiflipflop_ack(
    ack_sign.slave  din,
    output wire     dout,   // Was 'wire', could also be 'output logic'
    input           clk_in, // Was 'wire'
    input           clk_out // Was 'wire'
    );

Another change I would encourage is to move the parameter declaration in the axi_lite interface to be like below. It is not necessary in this case, but would be if the interface's port list is parameterized.

interface axi_lite #(
    parameter ADDR_WIDTH = 32,
    parameter DATA_WIDTH = 32
    ) (
    input  clk,
    input  reset
    );
    localparam AW = ADDR_WIDTH - 1;
    localparam DW = DATA_WIDTH - 1;

Importing the axi_lite_pkg to the global space is legal, but it is not considered a good practice. There is the risk of confusion if there are other imported packages that happen to have the same names for their own typedefs, functions, tasks or signals. This is a low risk when it is just your code. However, the risk increases as you work with team members or start including external IP. I would recommend moving the line import axi_lite_pkg::*; inside the modules that need it or use the static scope operator (::) when declaring signals (ex: axi_lite_pkg::resp).

I'll also suggest renaming the user defined type resp to resp_e. With your favorite search engine search for: SystemVerilog Coding Guidelines. It is a common best practice to name simple typedefs with an _t suffix. typedef enums are often suffixed with _e.

I'm guessing you are not planning on synthesizing videotest_config. As a behavioral model, you're fine. My only suggestion is to change some of the tasks to function void to guard against accidental time delay. If it is going to be synthesized, then you may want to reconsider your coding style. I'm not going to get into how it should be coded for synthesis. I will provide links to papers from Verilog/SystemVerilog experts that I continue to find insightful. (FYI, My profile page also includes links to verification resources)

toolic · Accepted Answer · 2024-10-18 16:40:03Z

In addition to the good points made in this answer, consider the following suggestions.

Layout

I recommend adding blank lines between blocks of code such as task definitions, always blocks, etc. The following is difficult to read and understand:

logic [31:0] rdata;
resp         rresp;
task reset();
    write_addr_done <= 0;
    write_data_done <= 0;
    write_done <= 0;
    read_addr_done <= 0;
    read_done <= 0;
    reset_config();
    reset_state();
    reset_hsize_vsize();
endtask;
initial
begin
    reset();
end
always_ff @(posedge conf.clk or negedge conf.reset)
begin
    if (conf.reset == 0) begin
        reset();
    end else begin
        handle_write();
        handle_read();
    end
end
always_ff @(posedge conf.clk) //latch

This is clearer:

logic [31:0] rdata;
resp         rresp;

task reset();
    write_addr_done <= 0;
    write_data_done <= 0;
    write_done <= 0;
    read_addr_done <= 0;
    read_done <= 0;
    reset_config();
    reset_state();
    reset_hsize_vsize();
endtask;

initial
begin
    reset();
end

always_ff @(posedge conf.clk or negedge conf.reset)
begin
    if (conf.reset == 0) begin
        reset();
    end else begin
        handle_write();
        handle_read();
    end
end

always_ff @(posedge conf.clk) //latch

I prefer to place the begin keyword on the same line as the initial and always keywords, similar to what you do for your if statements. This saves on valuable vertical space. For example:

initial begin
    reset();
end

always_ff @(posedge conf.clk or negedge conf.reset) begin
    if (conf.reset == 0) begin
        reset();
    end else begin
        handle_write();
        handle_read();
    end
end

Indentation

It is common to indent the lines inside a case statement:

        case (transform_addr(araddr))
            WORD_CONFIG: read_config();
            WORD_STATE: read_state();
            WORD_HSIZE_VSIZE: read_hsize_vsize();
            default: begin
                rresp <= SLVERR;
                rdata <= 'X;
                read_done <= 1;
            end
        endcase

Comments

The following comment is misleading:

always_ff @(posedge conf.clk) //latch

A "latch" is a different type of device from a flip-flop. Remove the comment or add more text to the comment to describe specifically what is meant.

Unused code

The following task does nothing, and it can be removed along with its calling statement:

task automatic reset_state();
endtask

Don't clutter the code with too many placeholders which may never be used.

Documentation

Add block comments to the top of each file summarizing the purpose of the code.

Stack Exchange Network

Simple SystemVerilog AXI controller

2 Answers 2

Layout

Indentation

Comments

Unused code

Documentation

You must log in to answer this question.

Hot Network Questions

Simple SystemVerilog AXI controller

2 Answers 2

Layout

Indentation

Comments

Unused code

Documentation

You must log in to answer this question.

Related

Hot Network Questions