Verilog UART Transmitter

Question

This is one of the first Verilog programs I have written. I have a Xilinx Artix-7 FPGA card. Right now I just have it transmitting an "X" every second. It works, and I can see the result in my serial terminal. It uses a UART over USB connection.

I'm just wondering if I can get some feedback on my code and if you see any problems.

module uart_top(input clk,
 input rx,
 output tx);
 reg [31:0] count = 0;
 wire ready; 
 uart_send sender("X", count == 100000000, clk, tx, ready); 
 
 always @(posedge clk)
 if(count == 100000000) count <= 0;
 else count <= count + 1;
 
endmodule
module uart_send #(parameter BAUD_RATE = 9600,
 parameter CLOCK_SPEED_MHZ = 100)
 (input [7:0] data_byte, 
 input start_send, 
 input clk, 
 output tx,
 output ready);
 
 parameter integer CYCLES_WAIT = CLOCK_SPEED_MHZ * 1e6 / BAUD_RATE;
 
 parameter IDLE = 0;
 parameter START_BIT = 1;
 parameter END_BIT = 2;
 parameter DATA_BIT = 3;
 
 reg [2:0] state = IDLE;
 reg [15:0] cycle_count = 0;
 reg [3:0] bit_index = 0;
 reg [7:0] data;
 
 assign tx = state == IDLE ? 1 :
 state == START_BIT ? 0 :
 state == END_BIT ? 1 :
 data[bit_index];
 
 assign ready = state == IDLE;
 
 always @(posedge clk) begin
 if(state != IDLE)
 data <= data_byte;
 if(cycle_count == CYCLES_WAIT) cycle_count <= 0;
 else cycle_count <= cycle_count + 1;
 
 if(state == IDLE && start_send) begin
 state <= START_BIT;
 cycle_count <= 0;
 end else if(state == START_BIT && cycle_count == CYCLES_WAIT) begin
 state <= DATA_BIT;
 bit_index <= 0;
 end else if(state == DATA_BIT && cycle_count == CYCLES_WAIT) begin
 if(bit_index == 7) state <= END_BIT;
 else bit_index <= bit_index + 1;
 end else if(state == END_BIT && cycle_count == CYCLES_WAIT) begin
 state <= IDLE;
 end
 end
 
endmodule

Answer 1

I would recommend your outputs be flops. It would be a cleaner output signal and easier to do timing analyses. It takes time (few nano seconds) to calculate the tx value after the clock with combinational logic. During that time the intermediate values are being transmitted and causing noise. This noise will can get worse from resistance an capacitance in your USB cable; lowers the signal quality. It seems your running at low speeds, so you wouldn't see a problem, but at higher speeds you may run into trouble.

Be aware that many synthesizers treat conditional operators (the ? : syntax) as explicit 2:1 muxes. So your current tx logic will always be a chain of 2:1 muxes even when a 4:1 is more suitable. A case statement within an always block tend to synthesize more optimally. I have a more detail answer for ? : vs case() on here on Electronics StackExchange.

Usually a FSM is written as a case statement; not if-else statements. The synthesizer doesn't really care. It is more of a common practice with RTL designers. It makes the FSM easier to identify and control how it synthesizes (full_case, parallel_case, onehot, encoding, etc.).

For a more optimzed design, your code should look something like the below. Or you can use two always blocks (my preference); one for calculating the next states for the flops, the other for simple flop assignment.

module uart_send #(parameter BAUD_RATE = 9600,
 parameter CLOCK_SPEED_MHZ = 100)
 (input [7:0] data_byte, 
 input start_send, 
 input clk, 
 output reg tx, // <-- reg outputs
 output reg ready); // <-- reg outputs
 /* ... local parameter and reg declarations here ... */
 always @(posedge clk) begin
 case(state)
 IDLE : begin
 /*your IDLE code here*/
 end
 START_BIT : begin
 /*your START_BIT code here*/
 end
 END_BIT : begin
 /*your END_BIT code here*/
 end
 DATA_BIT : begin
 /*your DATA_BIT code here*/
 end
 endcase
 end
endmodule

---

I will also suggest changing the uart_send instantiation form connect by port order to connect by name.

uart_send sender("X", count == 100000000, clk, tx, ready);

to (port order does not matter)

uart_send sender( .data_byte("X"), .start_send(count == 100000000),
 .clk(clk), .tx(tx), .ready(ready) );

or even cleaner (width matching)

wire [7:0] data_byte = "X";
wire start_send = (count == 100000000);
uart_send sender( .data_byte(data_byte), .start_send(start_send),
 .clk(clk), .tx(tx), .ready(ready) );

I believe you can abbreviate matching name and size to just .name (ex .clk is the same as .clk(clk)) but I do not remember if that if Verilog or an added feature in SystemVerilog. I not have my copy if the LRMs with me.

wire [7:0] data_byte = "X";
wire start_send = (count == 100000000);
uart_send sender( .data_byte, .start_send, .clk, .tx, .ready );

Answer 2

I agree with the suggestions in Greg's answer. Here are some further considerations, mostly for coding style.

Large numbers like 100000000 are hard to read. Use underscores to make them easier to comprehend: 100_000_000. Or, use scientific notation as you did with 1e6.

Since you used this constant multiple times, it would be better to declare it as a parameter:

parameter MAX_COUNT = 100_000_000;

It is more conventional to use a case statement than repeated ternary conditions for the following:

assign tx = state == IDLE ? 1 :
 state == START_BIT ? 0 :
 state == END_BIT ? 1 :
 data[bit_index];

Here is how it would look:

output reg tx,
//...
always @* begin
 case (state)
 IDLE : tx = 1;
 START_BIT: tx = 0;
 END_BIT : tx = 1;
 default : tx = data[bit_index];
 endcase
end

It is a little clearer what values are assigned to tx under what conditions. Since this changes the assignments to tx from continuous to procedural, tx must be declared as reg.

Use consistent style for if/else statements. I recommend always using begin/end; you used it in some places, but not others. For example, change:

 if(count == 100000000) count <= 0;
 else count <= count + 1;

to:

 if (count == 100000000) begin
 count <= 0;
 end else begin
 count <= count + 1;
 end

While it is more verbose, it makes the code easier to understand with the assignments aligned horizontally with each other.

A common Verilog pitfall is to cram too much code into a single always block as in your uart_send module. The code would be easier to understand with the 3 if statements separated into their own always blocks. There is no penalty for doing so.

Also, the indentation of the following code is misleading:

 if(state != IDLE)
 data <= data_byte;
 if(cycle_count == CYCLES_WAIT) cycle_count <= 0;
 else cycle_count <= cycle_count + 1;

It implies that the 2nd if is conditioned by the first if. However, they are really 2 separate statements:

 if (state != IDLE)
 data <= data_byte;
 if (cycle_count == CYCLES_WAIT) cycle_count <= 0;
 else cycle_count <= cycle_count + 1;

When there are multiple operators in a statement, I prefer to add parentheses for clarity. For example, change:

 assign ready = state == IDLE;

to:

 assign ready = (state == IDLE);

With multiple operators in an expression, adding extra parens also avoids potential precedence issues. For example, change:

 if(state == IDLE && start_send) begin

to:

 if ((state == IDLE) && start_send) begin