current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 7 down vote favorite

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
share|improve this question
    
Style point: Use underscores in numeric literals to improve readability 100_000_000 – toolic Dec 10 '16 at 15:10
up vote 8 down vote accepted

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 );
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.