6

I'm trying to apply timing analysis to a RISC-V MCU I have designed in SystemVerilog, in Vivado, for a Basys 3 board.

My design contains several generated clocks, which are made by dividing the system clock (100MHz) by a number. I use fabric to do this, I know it's a bad practice to do so, although I use BUFG, but I'm expecting to be able to finish with the timing analysis for this project even with this kind of divided clocks. One of this clocks feeds the CPU, and others go to an SPI peripheral. The SPI SCK is generated in fabric as well.

I'm using this reduced clock to feed the CPU because, since I haven't done the timing analysis yet, I don't know what is the max. frequency my CPU can reach, and therefore I can't know if it will work at 100 MHz (system clock). In any case, I'm doing all of this for the shake of my learning, therefore having (one or more) generated clocks serves me to learn about timing analysis, which is on what I'm focusing at the moment.

After synthesizing my design, the constraints wizard reports a lot of generated clocks that, as far as I understand, should not exist.

The design is too big to post it here, so I have reduced it to the minimal expression that still reports these non-sense clocks. The result is a design that does not make any sense from the functional point of view, but serves as example.

I have noticed that commenting out part of the code, which is pure combinational, make the spurious clocks to disappear.

I'm expecting clk_div/clk to be a generated clock.

Here is the design:

`define CLK_PWIDTH 32'd500

typedef enum logic [3:0]
{
    ALU_OP_ADD = 4'b0000,
    ALU_OP_SUB = 4'b0001,
    ALU_OP_NONE = 4'b0011
} alu_op_e;

typedef enum logic [6:0]
{
    OP_I_TYPE_L = 7'b0000011,
    OP_B_TYPE = 7'b1100011
} op_e;


/**
 * Clock divider.
 *
 * @param div_clk Divided clock signal
 * @param clk Clock
 * @param rst Reset
 *
 * @tparam POL Polarity (@param{clk} value when in reset state)
 * @tparam PWIDTH @param{div_clk} pulse width in @param{clk} pulses.
 *
 */
module clk_div #(parameter POL = 1'd0, parameter PWIDTH = 8'd4) (
    output  wire        div_clk,
    input   wire        clk,
    input   wire        rst
);
    reg [31:0] timer;
    reg div_clk_r;

    always @(posedge clk or posedge rst) begin
        if (rst) begin
            div_clk_r <= POL;
            timer <= 0;
        end else begin
            if (timer < (PWIDTH - 1)) begin
                timer <= timer + 1;
            end else begin
                timer <= 0;
                div_clk_r = ~div_clk_r;
            end
        end
    end

    BUFG bufg0(.O(div_clk), .I(div_clk_r));
endmodule

/**
 * D flip-flop
 *
 */
module dff #(parameter N = 32) (
    input  wire [N-1:0] d,
    input  wire         en,
    output reg  [N-1:0] q,
    input  wire         clk,
    input  wire         rst
);
    always @(posedge clk) begin
        if (rst)
            q <= {N{1'b0}};
        else if (en == 1'b1)
            q <= d;
    end
endmodule

module alu (
    input   logic   [31:0]  a,
    input   logic   [31:0]  b,
    input   alu_op_e        op,
    output  logic   [31:0]  res,
    output  wire    [3:0]   flags
);
    logic [31:0] b_op, nb;
    logic cin;

    assign nb = ~b;

    always_comb begin
        case (op)
        ALU_OP_ADD: {b_op, cin, res} = {b, 1'b0, s};
        ALU_OP_SUB: {b_op, cin, res} = {nb, 1'b1, s};
        default: res = 32'hffffffff;
        endcase
    end

    wire [31:0] s;
    wire co;

    assign {co, s} = a + b_op + cin;
endmodule

/**
 * Decodes the ALU control (op. to perform) based on the inputs
 *
 */
module alu_dec(
    input   wire    [6:0] op,
    input   wire    [2:0] func3,
    input   wire    [6:0] func7,
    output  logic   [3:0] alu_ctrl
);
    always_comb begin
        case (op)
        OP_I_TYPE_L:    alu_ctrl = ALU_OP_ADD;
        OP_B_TYPE:      alu_ctrl = ALU_OP_SUB;
        default:        alu_ctrl = ALU_OP_NONE;
        endcase
    end
endmodule

module riscv_all_instr_physical_fpga_test_top(
    input   wire        btnC,
    output  wire [15:0] LED,
    output  wire [7:0]  JA,
    input   wire        CLK100MHZ
);
    wire rst;
    assign rst = btnC;


    wire clk;

    clk_div #(.POL(1'd0), .PWIDTH(`CLK_PWIDTH)) cd(clk, CLK100MHZ, rst);



    wire [31:0] instr, m_addr, i;

    dff #(.N(32)) cff(i, 1'b1, instr, clk, rst);

    assign i = instr + 1;




    logic [31:0] pc, pc_next;

    dff pc_ff(pc_next, 1'b1, pc, clk, rst);

    assign pc_next = pc + 4;



    wire [6:0] op;
    wire [2:0] func3;
    wire [6:0] func7;
    alu_op_e alu_op;

    assign op = instr[6:0];
    assign func3 = instr[14:12];
    assign func7 = instr[31:25];

    alu_dec ad(op, func3, func7, alu_op);



    logic [31:0] alu_op_a;
    logic [31:0] alu_op_b;
    wire [3:0] alu_flags;

    assign alu_op_a = instr;
    assign alu_op_b = instr + 2;

    alu alu0(alu_op_a, alu_op_b, alu_op, m_addr, alu_flags);


    assign {mosi, miso, ss, sck} = m_addr[31:28];
    assign LED = m_addr[27:24];
    assign JA[3] = mosi;
    assign JA[2] = miso;
    assign JA[1] = ss;
    assign JA[0] = sck;
endmodule


If I synthesize this design, the constraints wizard reports the following:

enter image description here

However, if I comment out the line assign {co, s} = a + b_op + cin; in alu, I get this:

enter image description here

The same happens if I replace the block

    wire [6:0] op;
    wire [2:0] func3;
    wire [6:0] func7;
    alu_op_e alu_op;

    assign op = instr[6:0];
    assign func3 = instr[14:12];
    assign func7 = instr[31:25];

    alu_dec ad(op, func3, func7, alu_op);

by

assign alu_op = (i % 2 == 0 ? ALU_OP_ADD : ALU_OP_SUB);

Why this happens? How can modifying pure combinational logic 'produce' generated clocks?

EDIT

In the case of all the spurious clocks are reported, this is (the relevant part of) the schematics: enter image description here

Why is BUFG an input of alu0, if alu0 is pure combinational?

If, for instance, I comment out assign {co, s} = a + b_op + cin; to avoid the generated clocks, the relevant part of the schematics goes to this: enter image description here

EDIT 2

Just in case it helps, this is the schematics after the latch problem pointed by @toolic is solved:

enter image description here

As you can see, there is no longer a BUFG input at alu0.

Martel
  • 1,213
  • 6
  • 20
  • If you're doing what I think you're doing, you can't divide clocks that way. The clock is on dedicated clock distribution networks and not generic logic for a reason. – DKNguyen Oct 11 '22 at 13:35
  • 2
    It looks like you're making home-made clocks from DFF outputs to divide down a logic clock. Unless you know exactly what you're doing and there's some overwhelming reason to do it that way, it's a disastrous way to generate lower frequency clocks. Your answer skips what you're trying and do and why, straight into what circuit you've come up with. Please can you edit your answer and add text (not more diagrams) detailing what you're doing and why. – TonyM Oct 11 '22 at 13:42
  • @DKNguyen I'm aware it's not recommended to divide clocks in fabric, and that the alternative (as far as I understand) is using always the system clock and a clock_enable pulse input in each module, however notice that I use BUFG to avoid the high skew. In any case, – Martel Oct 11 '22 at 13:42
  • As long as you're aware. – DKNguyen Oct 11 '22 at 13:43
  • 1
    It looks like you have an inferred latch in the `alu` `case` statement, but I'm not sure if that causes your problem. – toolic Oct 11 '22 at 13:47
  • @toolic How can I have a inferred latch if the `case` contains a `default`? – Martel Oct 11 '22 at 13:48
  • 2
    You didn't assign to cin and b_op in the default. – toolic Oct 11 '22 at 13:51
  • Are there no free PLLs? If there is, use that instead of a counter to make clocks, it's a far better solution. – TonyM Oct 11 '22 at 13:51
  • 1
    @toolic I fixed it and it indeed solves the spurious clocks problems. I replaced the default statement by `default: {b_op, cin, res} = {32'hffffffff, 32'hffffffff, 32'hffffffff};`. Thank you very much. I will wait in case you want to publish it as an answer. – Martel Oct 11 '22 at 13:57
  • @TonyM I think the Basys 3 boards have at least 1 PLL, but I don't know how to use it. I designed this MCU before knowing that dividing clocks in fabric was such a a bad idea. – Martel Oct 11 '22 at 13:58
  • I added an answer. – toolic Oct 11 '22 at 14:01
  • 1
    Thanks very much for your honesty. I can only heartily recommend that you stop to learn how to use the use the Vivado GUI to generate a PLL with the clocks you want. They're a breeze really and if you know what you're doing, it takes a few minutes, and if you don't it might take an hour. But the time it can save you later on fighting circuit instabilities and metastability problems makes it spectacularly worthwhile. I wouldn't emphasise it so strongly otherwise :-) – TonyM Oct 11 '22 at 14:03

1 Answers1

7

The following code in alu infers a latch:

always_comb begin
    case (op)
    ALU_OP_ADD: {b_op, cin, res} = {b, 1'b0, s};
    ALU_OP_SUB: {b_op, cin, res} = {nb, 1'b1, s};
    default: res = 32'hffffffff;
    endcase
end

All signals must be assigned values in all branches of the case statement. However, b_op and cin are not assigned values in the default line.

One way to avoid the latch is to change the default line to something like:

default: {b_op, cin, res} = {32'hffffffff, 1'b, 32'hffffffff};

From IEEE Std 1800-2017, section 9.2.2.2 Combinational logic always_comb procedure:

Software tools should perform additional checks to warn if the behavior within an always_comb procedure does not represent combinational logic, such as if latched behavior can be inferred.

Your Vivado software should warn you that it inferred a latch.

toolic
  • 5,637
  • 5
  • 20
  • 33
  • 1
    Does this happen because a latch will necessarily need a clock signal, and Vivado is creating the latch and feeding it with the clock signal that is supposed to fit better? – Martel Oct 11 '22 at 14:07
  • 1
    @Martel: Perhaps Vivado generates a warning message somewhere in its synthesis logs which notifies you of inferred latches. If not, look through the Vivado synthesis documentation to see how latches are implemented. – toolic Oct 11 '22 at 14:32