On 12/26/05, Pieter Hulshoff <[EMAIL PROTECTED]> wrote: > Hello all, > > Since the topics seem to have turned to hardware related matters, I decided to > see if I could contribute a bit. :) I've done high-speed ASIC/FPGA design for > the past 8 years now for a living, so I might be able to make a few useful > comments here and there. > > I'd like to start with the FIFO. Timothy and I already have a few emails about > this, but he said it would be good to put it on the mailinglist so everybody > can chip in. I'll post the code as it was sent to me below, and see if I can > put some questions/comments in between using -- (which incidently is VHDL > comment syntax, which is what I usually design in. My verilog knowledge is > still virtually nonexisting); just search for it to skip most of what you've > already seen. > > > // Async fifo > // Input and output can be in different clock domains > // Copyright 2005, Timothy Miller > // LGPL license > > -- Why is empty on the output bus? Is another block going to use this or will > -- they only consider valid_out in accordance with > -- http://opengraphics.gitk.com/fifo_handshake.gif? I can think of valid > -- reasons for wanting to use this, but it's not in the interface spec.
Good catch on that. I had actually first added it to the basic_fifo, which I'd designed earlier. That one only allows one clock domain, and I wanted it there so I could connect up two fifos and see when they were, together, only half full (that is, the first one is empty). This way, I could tell when it was appropriate to do a burst if 16 PCI cycles of DMA to get more words from a DMA command or image buffer. This signal should probably either be completely removed from the async_fifo or moved into the in_clock domain. > module async_fifo( > reset, > > in_clock, > data_in, > enq, > full, > > out_clock, > data_out, > valid_out, > deq, > empty > ); > > parameter fifo_width = 32; > // fifo depth is hard-wired to 16 > > input in_clock, out_clock, reset; > > input [fifo_width-1:0] data_in; > input enq; > output full; > reg full; > > output [fifo_width-1:0] data_out; > reg [fifo_width-1:0] data_out; > output valid_out; > reg valid_out; > input deq; > output empty; > reg empty; > > > -- I'd assume that head would be the write pointer, and tail the read pointer, I use an analogy with a bank queue. People enter the line at the tail and are removed from it at the head. > -- but since you use tail for writing data, I guess my first assumption is > -- wrong. Is head the read and tail the write pointer? Yes. > -- Also: I'd advise using useful nametags, like _1d, _2d for once delayed, > -- twice delayed, etc., _c2c for crossing clock areas, and _meta for meta > -- stability FFs. This will also help you locate these FFs for timing analysis > -- later. Good advise. A lot of my experience with chip design is self-taught, so while I had read some about design technique and methodology, I missed variable naming entirely. > reg [fifo_width-1:0] fifo_data [0:15]; > reg [3:0] head0, tail0, head4, head3; > reg [3:0] head1, tail1, tail2, tail4, tail5; > > wire [3:0] tail3 = next_gray_code(tail2); > > > // Fix metastability of head0 being clocked into in_clock domain. > always @(posedge in_clock) head3 <= head0; > always @(posedge in_clock) head4 <= head4; > -- I'm not quite sure what you're trying to do here. Is this the correct code > -- or am I misunderstanding something here? You're assigning head4 to head4? That is a typo. This is the correct code: always @(posedge in_clock) head4 <= head3; > > // accept input > wire next_full = tail2 == head4; > wire is_full = tail1 == head4; > always @(posedge in_clock or posedge reset) begin > -- This is an asynchronous reset; something which in my opinion should be > -- banned from all designs. It's caused more problems than it's solved if you > -- ask me. Is it still a problem if the reset is held active for a long time? That's always how I've done it. I've made sure that the reset was active for at least a few cycles in the slowest clock domain. Can you get glitches if you do sync reset on a single reset signal? Or should we ensure that there's a valid reset for each individual clock domain? > if (reset) begin > tail0 <= 0; > tail1 <= 1; > tail2 <= 3; > full <= 0; > end else begin > if (!full && enq) begin > // We can only enqueue when not full > -- I'm sure I'm missing something again. You're writing with the tail pointer, > -- but tail is also stored multiple times? Why? Seems to me like all you need > -- is the current write pointer, and the next write pointer. > fifo_data[tail0] <= data_in; > tail0 <= tail1; > tail1 <= tail2; > tail2 <= tail3; //next_gray_code(tail2); tail0 is where you're writing. tail1 is what you compare against head4 to determine if the fifo is full. tail2 is what you compare against head4 to determine is you're going to be full on the next cycle. tail3 is just a combinatorial 'increment' of tail2. Remember, tail==head means empty. tail+1==head means full. > > // We have to compute if it's full on next cycle > full <= next_full; > > // It might not really be full if we're dequeueing, > // But we don't really care. Setting full early > // will not result in data loss. > end else begin > full <= is_full; > end > end > end > > > // Fix metastability of tail0 being clocked into out_clock domain. > // As a side-effect, eliminate race condition between tail0 and > // memory data. > always @(posedge out_clock) tail4 <= tail0; > always @(posedge out_clock) tail5 <= tail4; > -- This makes more sense. tail0 is from the in_clock domain, and is twice > -- clocked up for meta stability. tail5 will be used in the comparison. > > wire [3:0] head2 = next_gray_code(head1); > > // provide output > wire next_empty = head1 == tail5; > wire is_empty = head0 == tail5; > always @(posedge out_clock or posedge reset) begin > if (reset) begin > valid_out <= 0; > data_out <= 0; > head0 <= 0; > head1 <= 1; > end else begin > // If no valid out or we're dequeueing, we want to grab > // the next data. If we're empty, we don't get valid_out, > // so we don't care if data_out is garbage. > if (!valid_out || deq) begin > data_out <= fifo_data[head0]; > end > > if (!valid_out || deq) begin > // Yes, can have valid_out and empty at the same time. > // empty just means there are 16 entries free. > empty <= next_empty; > end else begin > empty <= is_empty; > end > // In either case, empty could be 'wrong' if an enq happens at > // the same time. Fortunately, data_out functions as a 17th > // entry, and of course, you should never design anything that > // assumes 'empty' means you can blindly euqueue 16 entries > // (ie. without paying attention to 'full'). > > if (!is_empty) begin > if (!valid_out || deq) begin > head0 <= head1; > head1 <= head2; //next_gray_code(head1); > end > valid_out <= 1; > end else begin > if (deq) valid_out <= 0; > end > end > end > > > > // This should turn into four LUT RAMs, which is the best you can get > // from an FPGA for this function. > function [3:0] next_gray_code; > input [3:0] I; > begin > case (I) > 0: next_gray_code = 1; > 1: next_gray_code = 3; > 3: next_gray_code = 2; > 2: next_gray_code = 6; > 6: next_gray_code = 7; > 7: next_gray_code = 5; > 5: next_gray_code = 4; > 4: next_gray_code = 12; > 12: next_gray_code = 13; > 13: next_gray_code = 15; > 15: next_gray_code = 14; > 14: next_gray_code = 10; > 10: next_gray_code = 11; > 11: next_gray_code = 9; > 9: next_gray_code = 8; > 8: next_gray_code = 0; > endcase > end > endfunction > > endmodule > _______________________________________________ > Open-graphics mailing list > [email protected] > http://lists.duskglow.com/mailman/listinfo/open-graphics > List service provided by Duskglow Consulting, LLC (www.duskglow.com) > _______________________________________________ Open-graphics mailing list [email protected] http://lists.duskglow.com/mailman/listinfo/open-graphics List service provided by Duskglow Consulting, LLC (www.duskglow.com)
