Strange issue with synchronous signal update

[ozbenh]

Description
I apologize for not having a small repro-case nor even a good grasp on the issue here,
it's quite possibly me not understanding something properly but ...

In order to be able to test microwatt (VHDL) with Litedram (Verilog) in sim together, I
made a verilator model of litedram, which I wrapped with vhpidirect, and plugged it
in-place of the verilog when running GHDL.

Note: This is testing a case that is working fine on FPGA after vivado synthesis.

In this setup, llitedram is wrapped twice:

• litedram-wrapper.vhdl is the normal wrapper that imports the litedram verilog as a "component", instanciates it, and implement the conversion between microwatt wishbone bus and litedram "port".

• in sim, the verilog component is replaced by a vhdl entity in a separate file that uses VHPIDIRECT to call into the verilator model.

The problem happens In the first wrapper (that converts the wishbone bus from microwatt to the litedram port). In there is this rather simple process:

    -- DRAM user port State machine
    sm: process(system_clk)
    begin
	
	if rising_edge(system_clk) then
	    if system_reset = '1' then
		state <= CMD;
	    else
		case state is
		when CMD =>
                    if (user_port0_cmd_ready and user_port0_cmd_valid) = '1' then
			state <= MWRITE when wb_in.we = '1' else MREAD;
		    end if;
		when MWRITE =>
		    if user_port0_wdata_ready = '1' then
			state <= CMD;
		    end if;
		when MREAD =>
		    if user_port0_rdata_valid = '1' then
			state <= CMD;
		    end if;
                end case;
	    end if;
	end if;
    end process;

The various user_port0_ signals are either locally generated (user_port0_cmd_valid) or coming
from the litedram component.

The following wave file show what happens:

The key thing here is the behaviour of "state":

• The first transition from "cmd" to "mwrite" is as expected: user_port0_cmd_valid becomes 1,
  and on the next clock, the above state machine observes it and sets state to mwrite.

• HOWEVER, the transition then from mwrite back to cmd is abnormal, at least unless I misunderstand something here: We see user_port0_wdata_ready being set to 1 and on the same cycle state goes to cmd. It should have only be observed on the next cycle.

Now this probably requires diving a bit more on how user_port0_wdata_ready is generated. It's done in this piece of VHDL:

    poll: process(clk)
        variable wb_response  : std_ulogic_vector(35 downto 0);
        variable ur_response  : std_ulogic_vector(130 downto 0);
        variable do_wb        : boolean;
    begin
        -- Send/receive signals on rising edge of clock only
        if rising_edge(clk) then

            -- Minor optimisation: don't update WB state if
            -- cyc  is low unless it just transitioned.
            do_wb := wb_ctrl_cyc = '1' or wb_ctrl_cyc /= old_wb_cyc;

            if do_wb then
                litedram_set_wb(wb_ctrl_cti & wb_ctrl_bte &
                                wb_ctrl_sel & wb_ctrl_we &
                                wb_ctrl_stb & wb_ctrl_cyc &
                                wb_ctrl_adr & wb_ctrl_dat_w);
            end if;
            litedram_set_user(user_port_native_0_cmd_valid &
                              user_port_native_0_cmd_we &
                              user_port_native_0_wdata_valid &
                              user_port_native_0_rdata_ready &
                              user_port_native_0_cmd_addr &
                              user_port_native_0_wdata_we &
                              user_port_native_0_wdata_data);
            if do_wb then
                litedram_get_wb(wb_response);
                wb_ctrl_dat_r <= wb_response(31 downto 0);
                wb_ctrl_ack   <= wb_response(32);
                wb_ctrl_err   <= wb_response(33);
                idone         <= wb_response(34);
                ierr          <= wb_response(35);
            end if;
            old_wb_cyc <= wb_ctrl_cyc;
            litedram_get_user(ur_response);
            user_port_native_0_cmd_ready   <= ur_response(130);
            user_port_native_0_wdata_ready <= ur_response(129);
            user_port_native_0_rdata_valid <= ur_response(128);
            user_port_native_0_rdata_data  <= ur_response(127 downto 0);
            litedram_clock;
        end if;
    end process;

Here, the litedram_* calls are VHPIDIRECT calls. However the whole thing happens in a if rising_edge(clk) block, thus user_port_native_0_wdata_ready should have been assigned "normally" as part of a synchronous process, and thus not observed by the first state machine until the next cycle.

Note: I've moved thing around in the above snipped, clock before, clock after, clock in the middle, etc.. with no changes.

Am I missing something here ?

As a result of this, the wishbone acks aren't generated and the core hangs. They are generated as by this:

    wb_out.ack <= user_port0_wdata_ready when state = MWRITE else
		  user_port0_rdata_valid when state = MREAD else '0';

This all seem to work fine when syntesized on FPGA.

Context
Please, provide the following information:

• OS: Ubuntu 18.04
  $ ghdl --version
  GHDL 1.0-dev (v0.37.0-579-g7285f745) [Dunoon edition]
  Compiled with GNAT Version: 7.5.0
  llvm code generator
  Written by Tristan Gingold.

[ozbenh]

Note: I have tried fairly hard to create a small repro-case without luck. I've created a little test bench, importing a components calling into C with roughly the same state machine etc... but in all cases, we see as expected the transition back to "cmd" on the cycle after the wdata_ready pulse.

Note also that if I change the state machine to use two processes as follow, then the problem goes away:

    -- DRAM user port State machine
    sm0: process(system_clk)
    begin
        if rising_edge(system_clk) then
            state <= nstate;
	end if;
    end process;

    sm: process(all)
    begin
        nstate <= state;
        if system_reset = '1' then
            nstate <= CMD;
        else
            case state is
            when CMD =>
                if (user_port0_cmd_ready and user_port0_cmd_valid) = '1' then
                    nstate <= MWRITE when wb_in.we = '1' else MREAD;
                end if;
            when MWRITE =>
                if user_port0_wdata_ready = '1' then
                    nstate <= CMD;
                end if;
		when MREAD =>
                if user_port0_rdata_valid = '1' then
                    nstate <= CMD;
                end if;
            end case;
        end if;
    end process;

Though I still can't quite figure out what's wrong with the single-process variant

[ozbenh]

If you want to reproduce, you can git clone my repository and use the "sim-litedram" branch

git clone git://git.ozlabs.org/home/benh/microwatt.git

You can then do

make core_dram_tb
ln -s hello_world/hello_world.bin main_ram.bin

To build it. You'll need verilator installed.

I then run it with the following command and wait until it hangs at "memtest_bus..."

./core_dram_tb  --wave=foo.ghw --read-wave-opt=foo.wopt --ieee-asserts=disable

With this in foo.wopt:

$ version 1.1

# Signals in entities :
/core_dram_tb/clk
/core_dram_tb/rst
/core_dram_tb/dram/clk_in
/core_dram_tb/dram/rst
/core_dram_tb/dram/system_clk
/core_dram_tb/dram/system_reset
/core_dram_tb/dram/core_alt_reset
/core_dram_tb/dram/wb_in
/core_dram_tb/dram/wb_out
/core_dram_tb/dram/wb_ctrl_in
/core_dram_tb/dram/wb_ctrl_out
/core_dram_tb/dram/wb_ctrl_is_csr
/core_dram_tb/dram/wb_ctrl_is_init
/core_dram_tb/dram/serial_tx
/core_dram_tb/dram/serial_rx
/core_dram_tb/dram/init_done
/core_dram_tb/dram/init_error
/core_dram_tb/dram/user_port0_cmd_valid
/core_dram_tb/dram/user_port0_cmd_ready
/core_dram_tb/dram/user_port0_cmd_we
/core_dram_tb/dram/user_port0_cmd_addr
/core_dram_tb/dram/user_port0_wdata_valid
/core_dram_tb/dram/user_port0_wdata_ready
/core_dram_tb/dram/user_port0_wdata_we
/core_dram_tb/dram/user_port0_wdata_data
/core_dram_tb/dram/user_port0_rdata_valid
/core_dram_tb/dram/user_port0_rdata_ready
/core_dram_tb/dram/user_port0_rdata_data
/core_dram_tb/dram/ad3
/core_dram_tb/dram/wb_ctrl_adr
/core_dram_tb/dram/wb_ctrl_dat_w
/core_dram_tb/dram/wb_ctrl_dat_r
/core_dram_tb/dram/wb_ctrl_sel
/core_dram_tb/dram/wb_ctrl_cyc
/core_dram_tb/dram/wb_ctrl_stb
/core_dram_tb/dram/wb_ctrl_ack
/core_dram_tb/dram/wb_ctrl_we
/core_dram_tb/dram/wb_init_in
/core_dram_tb/dram/wb_init_out
/core_dram_tb/dram/state
/core_dram_tb/dram/litedram/clk
/core_dram_tb/dram/litedram/rst
/core_dram_tb/dram/litedram/init_done
/core_dram_tb/dram/litedram/init_error
/core_dram_tb/dram/litedram/user_clk
/core_dram_tb/dram/litedram/user_rst
/core_dram_tb/dram/litedram/wb_ctrl_adr
/core_dram_tb/dram/litedram/wb_ctrl_dat_w
/core_dram_tb/dram/litedram/wb_ctrl_dat_r
/core_dram_tb/dram/litedram/wb_ctrl_sel
/core_dram_tb/dram/litedram/wb_ctrl_cyc
/core_dram_tb/dram/litedram/wb_ctrl_stb
/core_dram_tb/dram/litedram/wb_ctrl_ack
/core_dram_tb/dram/litedram/wb_ctrl_we
/core_dram_tb/dram/litedram/wb_ctrl_cti
/core_dram_tb/dram/litedram/wb_ctrl_bte
/core_dram_tb/dram/litedram/wb_ctrl_err
/core_dram_tb/dram/litedram/user_port_native_0_cmd_valid
/core_dram_tb/dram/litedram/user_port_native_0_cmd_ready
/core_dram_tb/dram/litedram/user_port_native_0_cmd_we
/core_dram_tb/dram/litedram/user_port_native_0_cmd_addr
/core_dram_tb/dram/litedram/user_port_native_0_wdata_valid
/core_dram_tb/dram/litedram/user_port_native_0_wdata_ready
/core_dram_tb/dram/litedram/user_port_native_0_wdata_we
/core_dram_tb/dram/litedram/user_port_native_0_wdata_data
/core_dram_tb/dram/litedram/user_port_native_0_rdata_valid
/core_dram_tb/dram/litedram/user_port_native_0_rdata_ready
/core_dram_tb/dram/litedram/user_port_native_0_rdata_data
/core_dram_tb/dram/litedram/idone
/core_dram_tb/dram/litedram/ierr
/core_dram_tb/dram/litedram/old_wb_cyc

You can then look for the first 0->1 transition of user_port0_cmd_valid to see the problem

[tgingold]

The most obvious possibility is a delta between system_clk and clk. Are they really the same clock ? I will have a look at it later. Ideally after finishing the attribute work!

[tgingold]

I have just checked out your repo and indeed system_clk and clk are not in phase. In sim_litedram.vhdl the process should use user_clk instead of clk.

[ozbenh]

I'm curious here, mind educating me ? When using the sim model, sim_litedram.vhdl does:

user_clk <= clk

(system_clk is user_clk)

So how come they aren't in phase ? Isn't the above basically just a wire ? IE. they are the same clocks no ?

(and yes I verified your fix does work, but I'd love to understand why :)

[tgingold]

For synthesis, they are just the same wire. For simulation, no. user_clk <= clk; is the same as: process (clk) begin user_clk <= clk; end process; So when there is an event on clk, all processes that are sensitive to clk are scheduled to run. In your case, that includes the process for user_clk assignment and the process that accesses to the lite dram. Then this is the next delta. Then the signals are updated (and that includes the user_clk). And the processes that are sensitive are scheduled. This includes the state machine, which reads the new values from litedram that have been updated in the previous delta. Conclusion: never assign clocks. In verilog, you have non blocking assignments and blocking assignments. You could play with clocks using blocking assignments. But you have to be very careful about that. In an ideal HDL, you would be able to do immediate assignment only for clocks.

[ozbenh]

Ok. I see. Something to keep in mind then, thanks.