4
\$\begingroup\$

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

I'd like to create a component which create 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 signal to different clocking domain 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 so far I was more of a software person and I picked FPGA as a hobby - code was created by trial and error so in some places I'm not sure if it is how it is supposed to be (I can debug on my own - comments regarding style/best practice would be more valuable).

\$\endgroup\$
2
\$\begingroup\$

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 encurage is to move the parameter deceleration in the axi_lite interface to be like below. It is not necessary in this case, but would be if the interfaces 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, task, or signals. This is a low risk when it is just your code. However, the risk increases are 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, your 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 you 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)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.