Multiple conflicting drivers for reg assigned in only one always block

Question

I'm working on a simple video signal timing module in Verilog, as a learning project. I've understood from earlier study that each reg should be assigned from only one always block, so I arranged my system into two state machine blocks and then one block for populating the output registers, as shown below:

module video_timing
(
    input wire reset,
    input wire clk,

    output reg [15:0] x,
    output reg [15:0] y,
    output reg hsync,
    output reg vsync,
    output reg visible
);

    // State constants for our two timing state machines (one horizontal, one vertical)
    `define VIDEO_SYNC       2'd0
    `define VIDEO_BACKPORCH  2'd1
    `define VIDEO_ACTIVE     2'd2
    `define VIDEO_FRONTPORCH 2'd3

    // These settings are for 720p, assuming clk is running at 74.25 MHz
    `define VIDEO_H_SYNC_PIXELS   16'd40
    `define VIDEO_H_BP_PIXELS     16'd220
    `define VIDEO_H_ACTIVE_PIXELS 16'd1280
    `define VIDEO_H_FP_PIXELS     16'd110
    `define VIDEO_H_SYNC_ACTIVE   1'b1
    `define VIDEO_V_SYNC_LINES    16'd5
    `define VIDEO_V_BP_LINES      16'd20
    `define VIDEO_V_ACTIVE_LINES  16'd720
    `define VIDEO_V_FP_LINES      16'd5
    `define VIDEO_V_SYNC_ACTIVE   1'b1

    reg [1:0] state_h;
    reg [15:0] count_h; // 1-based so we will stop when count_h is the total pixels for the current state
    reg inc_v = 1'b0;
    reg [1:0] state_v;
    reg [15:0] count_v; // 1-based so we will stop when count_v is the total lines for the current state

    // Change outputs on clock.
    // (These update one clock step behind everything else below, but that's
    //  okay because the lengths of all the periods are still correct.)
    always @(posedge clk) begin
        if (reset == 1'b1) begin
            hsync   <= ~`VIDEO_H_SYNC_ACTIVE;
            vsync   <= ~`VIDEO_V_SYNC_ACTIVE;
            visible <= 1'b0;
            x       <= 16'd0;
            y       <= 16'd0;
        end else begin
            hsync   <= (state_h == `VIDEO_SYNC) ^ (~`VIDEO_H_SYNC_ACTIVE);
            vsync   <= (state_v == `VIDEO_SYNC) ^ (~`VIDEO_V_SYNC_ACTIVE);
            visible <= (state_h == `VIDEO_ACTIVE) && (state_v == `VIDEO_ACTIVE);
            x       <= count_h - 1;
            y       <= count_v - 1;
         end
    end

    // Horizontal state machine
    always @(posedge clk) begin
        if (reset == 1'b1) begin
            count_h <= 16'b1;
            state_h <= `VIDEO_FRONTPORCH;
        end else begin
            inc_v <= 0;
            count_h <= count_h + 16'd1;

            case (state_h)
                `VIDEO_SYNC: begin
                    if (count_h == `VIDEO_H_SYNC_PIXELS) begin
                        state_h <= `VIDEO_BACKPORCH;
                        count_h <= 16'b1;
                    end
                end
                `VIDEO_BACKPORCH: begin
                    if (count_h == `VIDEO_H_BP_PIXELS) begin
                        state_h <= `VIDEO_ACTIVE;
                        count_h <= 16'b1;
                    end
                end
                `VIDEO_ACTIVE: begin
                    if (count_h == `VIDEO_H_ACTIVE_PIXELS) begin
                        state_h <= `VIDEO_FRONTPORCH;
                        count_h <= 16'b1;
                    end
                end
                `VIDEO_FRONTPORCH: begin
                    if (count_h == `VIDEO_H_FP_PIXELS) begin
                        state_h <= `VIDEO_SYNC;
                        count_h <= 16'b1;
                        inc_v <= 1;
                    end
                end
            endcase
        end
    end

    // Vertical state machine
    always @(posedge clk) begin
        if (reset == 1'b1) begin
            count_v <= 16'b1;
            state_v <= `VIDEO_FRONTPORCH;
        end else begin
            if (inc_v) begin
                count_v <= count_v + 16'd1;
                case (state_v)
                    `VIDEO_SYNC: begin
                        if (count_v == `VIDEO_V_SYNC_LINES) begin
                            state_v <= `VIDEO_BACKPORCH;
                            count_v <= 16'b1;
                        end
                    end
                    `VIDEO_BACKPORCH: begin
                        if (count_v == `VIDEO_V_BP_LINES) begin
                            state_v <= `VIDEO_ACTIVE;
                            count_v <= 16'b1;
                        end
                    end
                    `VIDEO_ACTIVE: begin
                        if (count_v == `VIDEO_V_ACTIVE_LINES) begin
                            state_v <= `VIDEO_FRONTPORCH;
                            count_v <= 16'b1;
                        end
                    end
                    `VIDEO_FRONTPORCH: begin
                        if (count_v == `VIDEO_V_FP_LINES) begin
                            state_v <= `VIDEO_SYNC;
                            count_v <= 16'b1;
                        end
                    end
                endcase
            end
        end
    end

endmodule

When attempting to synthesize this using the IceStorm toolchain, yosys warns that there are multiple drivers for hsync:

Warning: multiple conflicting drivers for top.\timing.hsync:
    port Q[0] of cell $techmap\timing.$procdff$109 ($adff)
    port Q[0] of cell $techmap\timing.$procdff$110 ($adff)

nextpnr-ice40 then subsequently fails on the same problem:

ERROR: multiple drivers on net 'P1B4' ($auto$simplemap.cc:496:simplemap_adff$375.Q and $auto$simplemap.cc:496:simplemap_adff$376.Q)
ERROR: Loading design failed.

I've tried a few different permutations to try to resolve this, and made some observations:

• If I change the assignment in the case (reset) 0: block (in the first always) to just hsync <= 'VIDEO_H_SYNC_ACTIVE then the result is synthesizable, and does indeed assert hsync constantly when not in reset as I'd expect after making that change.
• If I remove the hsync <= (state_h == 'VIDEO_SYNC) ^ (~'VIDEO_H_SYNC_ACTIVE) line entirely but retain the hsync <= ~'VIDEO_H_SYNC_ACTIVE during reset then the result is also sythesizable, and hsync is constantly unasserted.

(I replaced backticks with ' above just because backtick is the Markdown metacharacter for inline verbatim text. I'm using backticks for the constants in my actual program.)

This has led me to conclude that the expression on the right hand side of hsync <= that is the problem. But I'm then confused as to why the very similar following vsync <= line doesn't produce a similar error:

hsync <= (state_h == `VIDEO_SYNC) ^ (~`VIDEO_H_SYNC_ACTIVE);
vsync <= (state_v == `VIDEO_SYNC) ^ (~`VIDEO_V_SYNC_ACTIVE);

The module behaves as I expected under simulation with iverilog.

I'm using Yosys 0.8 and nextpnr built from git sha1 a6a4349. I also tried upgrading to Yosys 0.9 and got the same result, but I suspect the fault is mine, not in the toolchain.

Answer 1

This pattern of asynchronous resets is not permitted by the IEEE synthesis standard (1364.1). Only an if statement can be used thus:

Answer 2

The other answers here were important stepping-stones:

• My reset implementation was originally asynchronous and written in a way that wasn't conforming to the Verilog synthesis standard.
• My conditional reset logic was written using switch rather than if (due to some confusing advice I saw elsewhere).

Unfortunately, the trail went dry there because after that this timing.v was synthesizing just fine. It turned out that the problem was actually in top.v instead:

    video_timing timing(
        .reset(reset_loc),
        .clk(vga_ck),
        .hsync(vga_hs),
        .vsync(vga_hs),
        .visible(vga_de)
    );

I accidentally assigned vga_hs to both hsync and vsync. 🤦🏻‍♂️

After changing the vsync line to refer to vga_vs instead, the design now synthesizes and behaves as expected.

Lesson learned: the problem isn't always where the compiler thinks it is!