Thanks for the answer. I spent a bit of time working through the code, tweaking things in a copy as I read. I thought it might be helpful to leave “review comments” as I go, so I filled this out (there’s a link to what I ended up with at the end).
The design code:
- With my background in ASIC design, I started switching stuff in
design.sv to use asynchronous reset assertion, before realising this is probably targeting an FPGA. I’d probably recommend switching over anyway: it will still work on an FPGA and is possibly a bit more conventional. (always @(posedge clk or posedge rst))
- Rather than using a 32-bit counter for the clock division, you probably want something smaller. It also doesn’t need supplying from the testbench. Maybe something like this?
// The main clock is divided down to a uart clock with a posedge every clock_freq / baud_rate
// cycles.
localparam int unsigned CountMax = (clock_freq / baud_rate / 2) - 1;
localparam int unsigned CountWidth = $clog2(CountMax);
logic [CountWidth-1:0] count;
always_ff @(posedge clk or posedge rst) begin
if (rst) begin
count <= CountMax;
uclk <= 0;
end else begin
if (count == 0) begin
uclk <= ~uclk;
count <= CountMax;
end else begin
count <= count - 1;
end
end
end
- You don’t really want parameters for your FSM state names. If doing it by hand, you at least want
localparam (it shouldn’t be configurable!). But an enum would probably be better:
typedef enum logic [1:0] {
IDLE = 2'b00,
START = 2'b01,
DATA = 2'b10,
STOP = 2'b11
} fsm_state_e;
- Doing that also lets you use the enum type for your state variables (
fsm_state_e state, next_state;). And now you get enum names if you print them with %0p.
- The default case in your
next_state combinational block doesn’t actually do anything (because you’re setting next_state = 2’bx at the top). You might also decide that you don’t need the code at all (since you’re handling all the possible cases anyway).
- There’s also lots of logic in your flop block. I think things are rather easier to read if the FSM state update and consumption live together in one (combinatorial) block and registers/outputs get sampled separately. For the combined combinational block, I’d suggest this:
always_comb begin
next_state = state;
next_rx_data = rx_data;
next_rx_done = 1'b0;
case(state)
IDLE : begin
if (rx == 0) begin
next_state = DATA;
next_bit_idx = 7;
end
end
DATA : begin
next_shifter = {rx, shifter[7:1]};
next_bit_idx = bit_idx - 1;
if (bit_idx == 0) begin
next_state = STOP;
end
end
STOP : begin
next_state = IDLE;
next_rx_data = shifter;
next_rx_done = 1'b1;
end
endcase
end
- Then the register update is short:
always_ff @(posedge uclk or posedge rst) begin
if (rst) begin
state <= IDLE;
rx_data <= '0;
rx_done <= 1'b0;
end else begin
state <= next_state;
rx_data <= next_rx_data;
rx_done <= next_rx_done;
shifter <= next_shifter;
bit_idx <= next_bit_idx;
end
end
- Better to count down than up! The design currently counts bits coming in in a
bit_count variable and stops when it gets to 8. It’s probably cleaner to use something like bits_left and decrement down to zero. Potentially smaller logic for the comparison and it’s also a bit easier to understand if the packet length becomes variable in future. (Also, my proposed change can count down from 7, so it only needs 3 bits of state)
- I also think something’s a bit messed up in the state variables. You should make the block that updates other variables based on the state consume
state, rather than next_state. This will result is shorter combinatorial paths, and it’s a more conventional way to do it.
Squinting a little at the design, I think that the intended behaviour is that I can send bits abcdefgh with a sequence of rx like 11111110abcdefgh11111. I think the usual way to do this just needs three states (IDLE, DATA and STOP).
Some notes about the testbench:
- I’d probably factor out the
clk and rst driving to live on its own (rather than merging them into the initial block like you had). Something like this, maybe:
// Run a clock with a period of 10 and posedges on multiples of 10 (to make the arithmetic easier!)
// Assert rst for the first clock cycle.
initial begin
clk <= 1;
rst <= 1;
#10
rst <= 0;
end
always #5 clk = ~clk;
- Then I strongly recommend detailed comments to explain what the rest of the directed test is doing. I ended up with the following:
initial begin
@(posedge rst);
// The uart starts in IDLE. To send data, we send rx with value zero and then eight bits
// of data. To check we're not sending data by fluke at the start of time, assert rx=1 and
// consume one extra clock edge.
rx <= 1;
@(negedge rst);
@(posedge dut.uclk);
// Send a start pulse (rx=0)
rx <= 0;
@(posedge dut.uclk);
// Send 8 bits of data
for(int unsigned i=0; i<=7; i++)begin
rx <= data_rx[i];
@(posedge dut.uclk);
end
// Send any value for the dead cycle in the STOP FSM state.
rx <= 'x;
@(posedge dut.uclk);
// At this point, the FSM is going back to IDLE. Assert rx so that we don't start another
// packet, then tick a few times to make sure we're really stopped.
rx <= 1;
repeat (4) @(posedge dut.uclk);
$display("\nrx_data=%0d\n",rx_data);
$finish;
end
- The use of
$monitor is kind of cool, but it’s probably easier just to print on all relevant clock edges and also to include a timestamp (not so informative once I’d put sensible clock division in, but possibly still helpful). When getting things working, I added some internal variables from the dut, so the code ended up as:
always @(posedge dut.uclk) begin
if (!rst) begin
$display("%6t: rx=%0d, rx_data=%3d, rx_done=%0d, shifter=%8b, bit_idx=%0d, state=%0p",
$time, rx, rx_data, rx_done, dut.shifter, dut.bit_idx, dut.state);
end
end
Anyway, this is an incredibly long reply, sorry. (I started enjoying hacking on it!)
For a version that I think is working properly, see dEL3 on EDA playground.
To prove it did something sensible:
# run -all
# 5210: rx=1, rx_data= 0, rx_done=0, shifter=xxxxxxxx, bit_idx=x, state=IDLE
# 15610: rx=0, rx_data= 0, rx_done=0, shifter=xxxxxxxx, bit_idx=x, state=IDLE
# 26010: rx=0, rx_data= 0, rx_done=0, shifter=xxxxxxxx, bit_idx=7, state=DATA
# 36410: rx=1, rx_data= 0, rx_done=0, shifter=0xxxxxxx, bit_idx=6, state=DATA
# 46810: rx=1, rx_data= 0, rx_done=0, shifter=10xxxxxx, bit_idx=5, state=DATA
# 57210: rx=1, rx_data= 0, rx_done=0, shifter=110xxxxx, bit_idx=4, state=DATA
# 67610: rx=1, rx_data= 0, rx_done=0, shifter=1110xxxx, bit_idx=3, state=DATA
# 78010: rx=1, rx_data= 0, rx_done=0, shifter=11110xxx, bit_idx=2, state=DATA
# 88410: rx=1, rx_data= 0, rx_done=0, shifter=111110xx, bit_idx=1, state=DATA
# 98810: rx=0, rx_data= 0, rx_done=0, shifter=1111110x, bit_idx=0, state=DATA
# 109210: rx=x, rx_data= 0, rx_done=0, shifter=01111110, bit_idx=7, state=STOP
# 119610: rx=1, rx_data=126, rx_done=1, shifter=01111110, bit_idx=7, state=IDLE
# 130010: rx=1, rx_data=126, rx_done=0, shifter=01111110, bit_idx=7, state=IDLE
# 140410: rx=1, rx_data=126, rx_done=0, shifter=01111110, bit_idx=7, state=IDLE
# 150810: rx=1, rx_data=126, rx_done=0, shifter=01111110, bit_idx=7, state=IDLE
#
# rx_data=126
