Skip to content

Instantly share code, notes, and snippets.

@The6P4C The6P4C/top.v

Created Oct 1, 2019
Embed
What would you like to do?
a dodgy uart, without the r
`default_nettype none
module top(input CLK, output PIN_24);
wire tx_buf_empty;
reg tx_data_ready;
reg [7:0] tx_data;
uart_tx #(
.BAUD_DIVISOR(16000000 / 115200)
) uart_tx_inst (
.i_clk(CLK),
.o_tx(PIN_24),
.o_tx_buf_empty(tx_buf_empty),
.i_tx_data_ready(tx_data_ready),
.i_tx_data(tx_data)
);
reg [3:0] ctr = 0;
always @(*) begin
if (tx_buf_empty) begin
if (ctr == 10) begin
tx_data <= " ";
end else begin
tx_data <= "0" + ctr;
end
tx_data_ready <= 1;
end else begin
tx_data <= 'h00;
tx_data_ready <= 0;
end
end
always @(posedge CLK) begin
if (tx_buf_empty) begin
if (ctr == 10) begin
ctr <= 0;
end else begin
ctr <= ctr + 1;
end
end
end
endmodule
`default_nettype none
module uart_tx
# (
parameter BAUD_DIVISOR = 0,
parameter N_START_BITS = 1,
parameter N_DATA_BITS = 8,
parameter N_STOP_BITS = 1
) (
input i_clk,
output o_tx,
output o_tx_buf_empty,
input i_tx_data_ready,
input [N_DATA_BITS-1:0] i_tx_data
);
localparam SHREG_WIDTH = N_START_BITS + N_DATA_BITS + N_STOP_BITS;
localparam START_BITS = {N_START_BITS{1'b0}};
localparam STOP_BITS = {N_STOP_BITS{1'b1}};
localparam STATE_IDLE = 0;
localparam STATE_TX = 1;
reg state = STATE_IDLE;
reg next_state;
reg tx;
assign o_tx = tx;
reg tx_buf_empty = 1;
reg [N_DATA_BITS-1:0] tx_buf;
assign o_tx_buf_empty = tx_buf_empty;
reg [SHREG_WIDTH-1:0] shreg;
reg [3:0] num_bits_sent;
reg [10:0] ctr;
always @(*) begin
if (state == STATE_IDLE) begin
tx <= 1'b1;
if (!tx_buf_empty) begin
next_state <= STATE_TX;
end else begin
next_state <= STATE_IDLE;
end
end else if (state == STATE_TX) begin
tx <= shreg[0];
if ((num_bits_sent == SHREG_WIDTH - 1) & (ctr == BAUD_DIVISOR)) begin
next_state <= STATE_IDLE;
end else begin
next_state <= STATE_TX;
end
end
end
always @(posedge i_clk) begin
if (i_tx_data_ready & tx_buf_empty) begin
tx_buf_empty <= 0;
tx_buf <= i_tx_data;
end
if (state == STATE_IDLE) begin
if (!tx_buf_empty) begin
tx_buf_empty <= 1;
shreg <= {STOP_BITS, tx_buf, START_BITS};
num_bits_sent <= 0;
ctr <= 0;
end
end else if (state == STATE_TX) begin
if (ctr == BAUD_DIVISOR) begin
shreg <= {1'b0, shreg[SHREG_WIDTH-1:1]};
num_bits_sent <= num_bits_sent + 1;
ctr <= 0;
end else begin
ctr <= ctr + 1;
end
end
state <= next_state;
end
endmodule
@Wren6991

This comment has been minimized.

Copy link

Wren6991 commented Oct 1, 2019

Damn can you really not do inline comments on a gist? I will try to format this in a sensible way. All of these are for uart_tx.v

Line 14: input i_tx_data_ready,

  • Minor nit: lots of people will find this naming confusing. A flow handshake like this has customarily has one or more of the following signals:
    • *_valid, signalling to the consumer that the producer has data available
    • *_ready, signalling to the producer that the consumer will accept data presented on this cycle

With transfer taking place when both are high. So the naming here is just a little unconventional, and could cause confusion.

Line 21-22:

	localparam STATE_IDLE = 0;
	localparam STATE_TX = 1;
  • Nit: this is really a boolean. Instead consider giving state a more descriptive name

Line 23: reg state = STATE_IDLE;

  • Initialised registers can lead to portability issues. Some FPGAs only support initialising to 0, and ASIC tools won't know what to do with this. Consider a (synchronous) reset. This goes for all the initialised registers in the design

Line 40: tx <= 1'b1;

  • Nonblocking assignments are not generally used in always @ (*) blocks. They are nowhere near as dangerous as blocking assignments in clocked blocks, but can increase simulation time for large designs. Consider using blocking assignments here

Line 49: if ((num_bits_sent == SHREG_WIDTH - 1) & (ctr == BAUD_DIVISOR)) begin

  • This will never become true for BAUD_DIVISOR >= 2 ** 11. Consider sizing ctr with e.g. $clog2(BAUD_DIVISOR + 1)
  • Similar issue between num_bits_sent and SHREG_WIDTH
  • Nit: people often mix up precedence of & and == (although your code is fine). Consider && here

Line 67: shreg <= {STOP_BITS, tx_buf, START_BITS};

  • As far as I can tell, data is only loaded into the shift register when transitioning from the IDLE state to the TX state
  • This means you enter the IDLE state for one cycle after each byte.
  • This loses a tiny (probably negligible) amount of TX performance. More importantly...
  • Consider whether the transition to IDLE is strictly necessary, and whether the design could be made cleaner or more orthogonal without it

Line 72: if (ctr == BAUD_DIVISOR) begin

  • There are multiple instances of this comparison ctr == BAUD_DIVISOR, one of them in the block where ctr is updated. It requires a little more context to read
  • Done this way, all 11 bits of ctr are an input to the logic used in these two blocks. You may want to consider generating a single, registered clk_en signal from the free-running counter, and using this single bit to enable the downstream logic
@The6P4C

This comment has been minimized.

Copy link
Owner Author

The6P4C commented Oct 2, 2019

Thank you so much!!! Really appreciate it and will try and incorporate later :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.