1
\$\begingroup\$

I am trying to implement a parallel CRC in Verilog but having trouble getting it to work. This is a snippet of the code that I'm having trouble with.

  reg val;
  reg [15:0] hashValue;
  reg [3:0] data_in;
  always @(*) begin 
    if(reset) begin
        hashValue=16'h58a9; //initial hashvalue
        poly=16'h1021; //the polynomial 
    end
    for(i=0; i < 4; i=i+1) begin
        val=hashValue[15];
        hashValue={hashValue[14:0],data_in[0]};
        if(val) hashValue=hashValue ^ poly;
        data_in=data_in >> 1;
     end
   end

The crc block will take in a 4 bit input. The code is inside a combinational always @(*) block. The problem is when the two consecutive nibbles of same type come in through data_in there is no change in the hashValue. What am I doing wrong?

\$\endgroup\$
  • \$\begingroup\$ You set hashValue=16'h58a9 every single time data_in changes, so you always get the CRC of only the most recent nibble. \$\endgroup\$ – pjc50 Dec 3 '13 at 11:09
  • \$\begingroup\$ i tried setting the initial values on reset. still the issue persists. \$\endgroup\$ – user7994 Dec 3 '13 at 11:17
  • \$\begingroup\$ Have you tried debugging with Modelsim or $display? Your style is very un-verilog; normally you'd have "always @(posedge clock)" and use '<=' instead of '=', although if you change to '<=' you need to be aware that it doesn't take effect until the block finishes. \$\endgroup\$ – pjc50 Dec 3 '13 at 11:26
  • \$\begingroup\$ I have used always @(posedge clock) the for sequential parts of the code . I am implementing only the CRC portion in the combinational always @(*) section \$\endgroup\$ – user7994 Dec 3 '13 at 11:29
  • \$\begingroup\$ If it's supposed to be combinational, then it needs to be stateless - crucially, you can't have the output depending on some function of the previous state. \$\endgroup\$ – pjc50 Dec 3 '13 at 11:35
4
\$\begingroup\$

This is a good example of mismatch in results between simulation and synthesis.

If you will synthesize this code and run a test on FPGA - it will work (at least you'll not see constant hash value). However, in simulation it behaves differently:

  • always @(*) construct means "evaluate the following block of code any time any of the signals used on the right hand side of the assignment change". In your code these signals are: data_in, poly and hashValue.
  • If none of the above signals change (which is the case you're describing, right?) - the block will not be evaluated by simulator and the assignments won't be made.

Once again - synthesis tool will produce the correct logic for this code, therefore these kind of bugs are very dangerous.

There correct way to handle this is to define a clock signal and use a sequential always @(posedge clk) construct.

Furthermore, it seems that you have at least two combinatorial loops in your code. Even if they are intended - this is very bad practice. You want to avoid using any synthesizable comb loop.

However, by inspection of your code, it seems that circular reference here is just for convenience - maybe it will not synthesize into comb loop. In this case you have two options:

  1. Find an equivalent form which does not use circular reference
  2. Use Verilog function construct.

The first approach is the correct one - Verilog is not a programming language, and the statements you are using to describe logic must be maximally similar to the inferred logic. However, this approach may be tricky, since many algorithms are written in software forms. Therefore you might use the second approach, in which case the code will look like this (not tested):

always @(posedge clk)
begin
  hashValue[15:0] <= calcNewHashValue(hashValue[15:0], poly[15:0], data_in[3:0]);
end

Where calcNewHashValue is the function encapsulating the for loop from your code.

Once again: if synthesis tool warns you about comb loops you mustn't use this design. In this case either think of other algorithmic implementation, or spread the calculation on several clock cycles.


You may also want to read this paper in order to get a deeper understanding of Verilog simulators behavior.

Except for that, your coding style is very bug-prone. As a guideline, I suggest you will define a separate always block for each signal. In other words - just one signal is assigned over in always block. Make exceptions only where you can't handle it other way, and think carefully before each such decision.

\$\endgroup\$
  • \$\begingroup\$ i tried to modify the code and to adapt it for use in an always @(posedge clk) block. the problem with that is I need the for loop to be executed 4 times per clock cycle, which goes through only one iteration per clock cycle. is there any other method that I can correctly implement this? the above implementation works perfectly if not for the bug when I get the same two nibbles consecutively at the input. \$\endgroup\$ – user7994 Dec 3 '13 at 12:10
  • \$\begingroup\$ @user7994, I edited the answer with suggestions of solutions. \$\endgroup\$ – Vasiliy Dec 3 '13 at 13:42
  • \$\begingroup\$ @user7994 - Software parallel CRC schemes for four bit inputs do not use the four state clocking logic. Instead they use a lookup table. The lookup table is just sixteen 16-bit words. You may want to try implementing that as a ROM in your code so you can get the whole nibble computed into the CRC in a single clock cycle. \$\endgroup\$ – Michael Karas Dec 3 '13 at 13:59
  • \$\begingroup\$ As a side note to my above comment I routinely use the 16 entry lookup table in C code even when dealing with byte wide data. I just run the same calculation two times, first on the low nibble of the byte and then on the high nibble. I have found this method works well as a balance between embedded code operational speed versus the code storage space consumed by the lookup table of two hundred fifty six 16-bit entries required when processing a CRC eight bits at a time. \$\endgroup\$ – Michael Karas Dec 3 '13 at 14:06
0
\$\begingroup\$

This is how I managed to get it working. I made a function and called that in the main always @(posedge clock) block. Thank you very much @Vasiliy. That helped a lot. :)

always @ (posedge clk or posedge reset)
if (reset) begin
    crc_v <= #1 16'h58A9;
end 
else if(startcrc_flg) 
begin
    //crc_in <= #1 data_in;
    crc_v <= #1 next_crc(crc_v[15:0],POLY[15:0],crc_in[3:0]);
end else begin
    crc_v <= #1 crc_v;
    i <= #1 5;
end


function [15:0] next_crc;
input [15:0] crc_v;
input [15:0] POLY;
input [3:0] crc_in;



integer i;

begin

if (startcrc_flg) begin
for(i=0; i <4; i=i+1) 
begin
    crc_reg=crc_v[15];
    crc_v={crc_v[14:0],crc_in[0]};
    if(crc_reg) crc_v=crc_v ^ POLY;
    crc_in = crc_in >> 1;
end
end
next_crc = crc_v;


end
endfunction
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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