Continue to Site

Welcome to EDAboard.com

Welcome to our site! EDAboard.com is an international Electronics Discussion Forum focused on EDA software, circuits, schematics, books, theory, papers, asic, pld, 8051, DSP, Network, RF, Analog Design, PCB, Service Manuals... and a whole lot more! To participate you need to register. Registration is free. Click here to register now.

[SOLVED] Verilog Design Question

Status
Not open for further replies.

redsees

Junior Member level 1
Joined
Jan 30, 2016
Messages
18
Helped
0
Reputation
0
Reaction score
0
Trophy points
1
Activity points
313
Hello World,

A while ago, I posted a thread here asking about something in my Simplified DES Encryption Algorithm Verilog design.
Everyone looked at the code said that it was a hardware design written by a software programmer, and that was I using a bad practices in my code.
That was my software-like Verilog code:

Code Verilog - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
/*
    Simplified Data Encryption Standard (S-DES)
    8-bit 2-round block cipher encryption and decryption algorithm using 10-bit key.
*/
 
 
module SDES(input en, input[0:9] key, input[0:7] plaintext, input encrypt, output[0:7] ciphertext);
    reg[0:7] tempB, tempD, IP_out, sw_out, iip_out, feistel_out;
    
    assign ciphertext = (en)?(iip_out):(8'b0);
    
    // Generate Key1 and Key2
    task GenerateKeys;
        input[0:9] Key;
        output[0:7] Key1, Key2;
        
        reg[0:9] p10_out, tempA, tempC;
 
        begin
            // P10 Permutations
            p10_out = {Key[2], Key[4], Key[1], Key[6], Key[3], Key[9], Key[0], Key[8], Key[7], Key[5]};
        
            // Left Rotation by 1 bit
            tempA = {p10_out[1:4], p10_out[0], p10_out[6:9], p10_out[5]};
        
            // P8 Permutations
            Key1  = {tempA[5], tempA[2], tempA[6], tempA[3], tempA[7], tempA[4], tempA[9], tempA[8]};
        
            // Left Rotation by 3 bits
            tempC = {p10_out[3:4], p10_out[0:2], p10_out[8:9], p10_out[5:7]};
        
            // P8 Permutations
            Key2 = {tempC[5], tempC[2], tempC[6], tempC[3], tempC[7], tempC[4], tempC[9], tempC[8]};
        end
        
    endtask
    
    // Feistel Function
    task Feistel;
    input[0:7] inp_block, key;
    output[0:7] out_block;
    
    reg[0:3] first_chunk, second_chunk, xor_fout, xor_f1, xor_f2, p4_in, p4_out;
    reg[0:7] EP_out, xor_out;
    reg[0:1] s0_out, s1_out;
    
    begin
        first_chunk  = inp_block[0:3];
        second_chunk = inp_block[4:7];
        
        EP_out = {second_chunk[3], second_chunk[0], second_chunk[1], second_chunk[2], second_chunk[1], second_chunk[2], second_chunk[3], second_chunk[0]};
        
        xor_out = EP_out ^ key;
    
        xor_f1 = xor_out[0:3];
        xor_f2 = xor_out[4:7];
        
        S0_Box(xor_f1, s0_out);
        S1_Box(xor_f2, s1_out);
        
        p4_in = {s0_out, s1_out};
 
        p4_out = {p4_in[1],p4_in[3],p4_in[2],p4_in[0]};
            
        xor_fout = p4_out ^ first_chunk;
        
        out_block = {xor_fout, second_chunk};
    end
    
    endtask
    
    // S0 Box
    task S0_Box;
    input[0:3] inp_bits;
    output[0:1] out_bits;
    
    begin
        case(inp_bits)
            4'b0000: out_bits = 2'b01;
            4'b0001: out_bits = 2'b11;
            4'b0010: out_bits = 2'b00;
            4'b0011: out_bits = 2'b10;
            4'b0100: out_bits = 2'b11;
            4'b0101: out_bits = 2'b01;
            4'b0110: out_bits = 2'b10;
            4'b0111: out_bits = 2'b00;
            4'b1000: out_bits = 2'b00;
            4'b1001: out_bits = 2'b11;
            4'b1010: out_bits = 2'b10;
            4'b1011: out_bits = 2'b01;
            4'b1100: out_bits = 2'b01;
            4'b1101: out_bits = 2'b11;
            4'b1110: out_bits = 2'b11;
            4'b1111: out_bits = 2'b10;
        endcase
    end
    
    endtask
 
    // S1 Box
    task S1_Box;
    input[0:3] inp_bits;
    output[0:1] out_bits;
    
    begin
        case(inp_bits)
            4'b0000: out_bits = 2'b00;
            4'b0001: out_bits = 2'b10;
            4'b0010: out_bits = 2'b01;
            4'b0011: out_bits = 2'b00;
            4'b0100: out_bits = 2'b10;
            4'b0101: out_bits = 2'b01;
            4'b0110: out_bits = 2'b11;
            4'b0111: out_bits = 2'b11;
            4'b1000: out_bits = 2'b11;
            4'b1001: out_bits = 2'b10;
            4'b1010: out_bits = 2'b00;
            4'b1011: out_bits = 2'b01;
            4'b1100: out_bits = 2'b01;
            4'b1101: out_bits = 2'b00;
            4'b1110: out_bits = 2'b00;
            4'b1111: out_bits = 2'b11;
        endcase
    end
    
    endtask
    
    //always@(plaintext or key or encrypt)
    always@(en)
    begin
        if(en)
        begin
        
        // Generate Key1 and Key2
        GenerateKeys(key, tempB, tempD);
                    
        // Initial Permutation
        IP_out = {plaintext[1], plaintext[5], plaintext[2], plaintext[0], plaintext[3], plaintext[7], plaintext[4], plaintext[6]};
        
        // First Round
        if(encrypt)
            Feistel(IP_out, tempB, feistel_out);
        else
            Feistel(IP_out, tempD, feistel_out);
        
        // Swapping 
        sw_out = {feistel_out[4:7], feistel_out[0:3]};
        
        // Second Round
        if(encrypt)
            Feistel(sw_out, tempD, feistel_out);
        else
            Feistel(sw_out, tempB, feistel_out);
        
        // Inverse Initial Permutation
        iip_out = {feistel_out[3], feistel_out[0], feistel_out[2], feistel_out[4], feistel_out[6], feistel_out[1], feistel_out[7], feistel_out[5]};
        end
    end
    
endmodule



After getting a few advices from some of the experts in this forum. I decided to redesign the whole algorithm taking everything you said into consideration.
I ended up with the following code.


Code Verilog - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
/*
    Simplified Data Encryption Standard (S-DES)
    8-bit 2-round block cipher symmetric-key algorithm using 10-bit key.
    Encryption/Decryption routine takes total of 7 clock cycles
*/
 
 
module SDES(    en,             // Enable Bit
                CLK,            // Clock signal
                Key,            // 10-bit Key
                plaintext,      // 8-bit data block
                encrypt,        // Encryption flag, 1 for encryption, 0 for decryption
                ciphertext      // 8-bit data block
                );
                
    input           en;
    input           encrypt;
    input           CLK;
    input   [0:7]   plaintext;
    input   [0:9]   Key;
    output  [0:7]   ciphertext;
    
    // Internal signals
    reg     [0:7]   sw_out, iip_out, xor_f1, xor_f2, p4_in, s0_out, s1_out;
    reg     [0:2]   cnt = 3'b000;
    reg     [0:7]   Key1, Key2;
    
    assign ciphertext = (en)?(iip_out):(8'b0);
    
    always@(cnt)
    begin
        if(cnt == 3'b110)
            cnt = 3'b000;
    end
    
    always@(posedge CLK)
    begin
        Key1 = {Key[0], Key[6], Key[8], Key[3], Key[7], Key[2], Key[9], Key[5]};
        Key2 = {Key[7], Key[2], Key[5], Key[4], Key[9], Key[1], Key[8], Key[0]};
    end
    
    // Feistel Function Round One
    always@(posedge CLK)
    begin
        
        if(cnt < 3'b011)
        begin
            
            if(encrypt)
            begin
                xor_f1  <= { plaintext[6] ^ Key1[0], 
                             plaintext[3] ^ Key1[1], 
                             plaintext[7] ^ Key1[2], 
                             plaintext[4] ^ Key1[3] };
                             
                xor_f2  <= { plaintext[7] ^ Key1[4], 
                             plaintext[4] ^ Key1[5], 
                             plaintext[6] ^ Key1[6], 
                             plaintext[3] ^ Key1[7] };
            end
            else
            begin
                xor_f1  <= { plaintext[6] ^ Key2[0], 
                             plaintext[3] ^ Key2[1], 
                             plaintext[7] ^ Key2[2], 
                             plaintext[4] ^ Key2[3] };
                             
                xor_f2  <= { plaintext[7] ^ Key2[4], 
                             plaintext[4] ^ Key2[5], 
                             plaintext[6] ^ Key2[6], 
                             plaintext[3] ^ Key2[7] };      
            end
            
            p4_in  <= {s0_out, s1_out};
            
            cnt <= cnt + 1'b1;
            
        end 
    end
    
    // Feistel Function Round Two   
    always@(posedge CLK)
    begin
    
        if(cnt == 3'b011)
        begin
        
            if(encrypt)
            begin
                xor_f1 <= { p4_in[0] ^ plaintext[0] ^ Key2[0], 
                            p4_in[1] ^ plaintext[1] ^ Key2[1], 
                            p4_in[3] ^ plaintext[5] ^ Key2[2], 
                            p4_in[2] ^ plaintext[2] ^ Key2[3] };
                            
                xor_f2 <= { p4_in[3] ^ plaintext[5] ^ Key2[4], 
                            p4_in[2] ^ plaintext[2] ^ Key2[5], 
                            p4_in[0] ^ plaintext[0] ^ Key2[6], 
                            p4_in[1] ^ plaintext[1] ^ Key2[7] };
            end
            else
            begin
                xor_f1 <= { p4_in[0] ^ plaintext[0] ^ Key1[0], 
                            p4_in[1] ^ plaintext[1] ^ Key1[1], 
                            p4_in[3] ^ plaintext[5] ^ Key1[2], 
                            p4_in[2] ^ plaintext[2] ^ Key1[3] };
                            
                xor_f2 <= { p4_in[3] ^ plaintext[5] ^ Key1[4], 
                            p4_in[2] ^ plaintext[2] ^ Key1[5], 
                            p4_in[0] ^ plaintext[0] ^ Key1[6], 
                            p4_in[1] ^ plaintext[1] ^ Key1[7] };
            end
                    
            p4_in <= {s0_out, s1_out};
            
            iip_out <= {    p4_in[0] ^ plaintext[6], 
                            p4_in[1] ^ plaintext[3], 
                            p4_in[2] ^ plaintext[4], 
                            p4_in[1] ^ plaintext[1], 
                            p4_in[2] ^ plaintext[2], 
                            p4_in[3] ^ plaintext[7], 
                            p4_in[0] ^ plaintext[0], 
                            p4_in[3] ^ plaintext[5] };
                            
            cnt <= cnt + 1'b1;
        end
    end
        
    // S0 Box
    always@(xor_f1)
    begin
        case(xor_f1)
            4'b0000: s0_out = 2'b01;
            4'b0001: s0_out = 2'b11;
            4'b0010: s0_out = 2'b00;
            4'b0011: s0_out = 2'b10;
            4'b0100: s0_out = 2'b11;
            4'b0101: s0_out = 2'b01;
            4'b0110: s0_out = 2'b10;
            4'b0111: s0_out = 2'b00;
            4'b1000: s0_out = 2'b00;
            4'b1001: s0_out = 2'b11;
            4'b1010: s0_out = 2'b10;
            4'b1011: s0_out = 2'b01;
            4'b1100: s0_out = 2'b01;
            4'b1101: s0_out = 2'b11;
            4'b1110: s0_out = 2'b11;
            4'b1111: s0_out = 2'b10;
        endcase
    end
 
    // S1 Box
    always@(xor_f2)
    begin
        case(xor_f2)
            4'b0000: s1_out = 2'b00;
            4'b0001: s1_out = 2'b10;
            4'b0010: s1_out = 2'b01;
            4'b0011: s1_out = 2'b00;
            4'b0100: s1_out = 2'b10;
            4'b0101: s1_out = 2'b01;
            4'b0110: s1_out = 2'b11;
            4'b0111: s1_out = 2'b11;
            4'b1000: s1_out = 2'b11;
            4'b1001: s1_out = 2'b10;
            4'b1010: s1_out = 2'b00;
            4'b1011: s1_out = 2'b01;
            4'b1100: s1_out = 2'b01;
            4'b1101: s1_out = 2'b00;
            4'b1110: s1_out = 2'b00;
            4'b1111: s1_out = 2'b11;
        endcase
    end
    
endmodule



Of course, I'm not asking for a complete code review or any tracing. My question is what do you think about the new design? Is it still a hardware design written by a software programmer or it is a bit better?
Also, if anyone has any additional advices about the design, I'll be happy to read it.

Much Regards
 

Woo-hoo, it actually looks like code meant for hardware :)

I have a few observations:
1. The reg [0:7] definitions: the majority of coders use [7:0] unless the usage of [0:7] matches some algorithmic definition and makes it easier to follow (matches the algorithm definition). I'm not saying this is wrong in this case, I'm assuming this was done intentionally to match the bit direction of this algorithm.

2. There appears to be a lot of bit width issues:
Code:
// your definitions for the following don't match up:
    reg     [0:7]   sw_out, iip_out, xor_f1, xor_f2, [COLOR="#FF0000"]p4_in[/COLOR], [COLOR="#FF0000"]s0_out[/COLOR], [COLOR="#FF0000"]s1_out[/COLOR];

// used later like this:
            p4_in  <= {s0_out, s1_out}; // concatenates two 8-bit into a 16-bit and assigns it to an 8-bit.

// the result is equivalent to.
            p4_in[0:3] <= s1_out[0:3]; // as you only use the upper nibble of p4_in in your code.

// You are also assigning the s0_out and s1_out, both of which are 8-bit with 2-bit values...
        case(xor_f1)
            4'b0000: s0_out = 2'b01;
            4'b0001: s0_out = 2'b11;

// there are also bit width issues with xor_f1 and xor_f2

I never define multiple signals on the same line. My rule is only one definition per line.
Code:
// I think this is what you really want for the widths
//    reg     [0:7]   sw_out; // not even used, should be deleted.
    reg     [0:7]   iip_out;
    reg     [0:3]   xor_f1;
    reg     [0:3]   xor_f2;
    reg     [0:3]   p4_in;
    reg     [0:1]   s0_out;
    reg     [0:1]   s1_out;

Also just noticed a problem with the cnt generation, you have the assignments distributed between three different always blocks, this will probably result in problems with multiple drivers when trying to synthesize the code. All assignments for a reg should only be done in a single always block to avoid this issue. This is also done with the xor_f# and the p4_in assignments. Put each assignment in its own always block. I'm not sure your code is synthesizable right now due to this issue. I also see an issue where you are treating cnt as combinational and synchronous always @(cnt) implies cnt is a combinational block of code, but then you have it assigned in a synchronous always @(posedge clk) block. You can't assign it both ways if you want synthesizable code. Let me repeat write all of the conditions for cnt in a single always block and do the same for each signal xor_f1, xor_f2, p4_in, and iip_out. The two S box combinational always blocks are fine as is.

Note: I only think it will have a problem as I never code this way so can't be absolutely sure it will result in a problem, but furthermore it detracts from maintainability as now the code for the cnt is distributed across all of your code and requires much more study to determine what count does. Not a good practice in any case. Another rule, If you can't glance at a piece of code and determine it's function within seconds the code is already badly written IMO. This is why I avoid making giant multi-signal (5+) if statements that run on for 100's of lines it makes this quick glance at the code and know what it does next to impossible. I suppose some Genius on edaboard will claim otherwise. :roll:
 
  • Like
Reactions: redsees

    redsees

    Points: 2
    Helpful Answer Positive Rating
Code:
/*
    Simplified Data Encryption Standard (S-DES)
    8-bit 2-round block cipher symmetric-key algorithm using 10-bit key.
    Encryption/Decryption routine takes total of 7 clock cycles
*/
 
 
// vG:  This should use Verilog-2001 style ports.
// vG:  CLK should be clk, Key should be key.
// vG:  there probably should be a key_en and a plaintext_en
// vG:  there probably should be a ciphertext_valid output (or ciphertext_vld)
module SDES(    en,             // Enable Bit
                CLK,            // Clock signal
                Key,            // 10-bit Key
                plaintext,      // 8-bit data block
                encrypt,        // Encryption flag, 1 for encryption, 0 for decryption
                ciphertext      // 8-bit data block
                );
                
    input           en;
    input           encrypt;
    input           CLK;
    input   [0:7]   plaintext;
    input   [0:9]   Key;
    output  [0:7]   ciphertext;
    
    // Internal signals
    reg     [0:7]   sw_out, iip_out, xor_f1, xor_f2, p4_in, s0_out, s1_out;
    reg     [0:2]   cnt = 3'b000;
    reg     [0:7]   Key1, Key2;
    
    // vG:  This is inconvenient at best.
    // vG:  This places a requirement on the interface to know when the output is valid.
    assign ciphertext = (en)?(iip_out):(8'b0);
    
    // vG: as mentioned, you have cnt assigned in multiple different always blocks.
    always@(cnt)
    begin
        if(cnt == 3'b110)
            cnt = 3'b000;
    end
    
    // vG:  This should probably be "always @(Key)" or use non-blocking assignments.
    always@(posedge CLK)
    begin
        Key1 = {Key[0], Key[6], Key[8], Key[3], Key[7], Key[2], Key[9], Key[5]};
        Key2 = {Key[7], Key[2], Key[5], Key[4], Key[9], Key[1], Key[8], Key[0]};
    end
    
    // Feistel Function Round One
    always@(posedge CLK)
    begin
        
        if(cnt < 3'b011)
        begin
            // vG:  These actually could be tasks.  That part is fine.  But they don't have to be.
            // vG:  That part is up to you.
            if(encrypt)
            begin
                xor_f1  <= { plaintext[6] ^ Key1[0], 
                             plaintext[3] ^ Key1[1], 
                             plaintext[7] ^ Key1[2], 
                             plaintext[4] ^ Key1[3] };
                             
                xor_f2  <= { plaintext[7] ^ Key1[4], 
                             plaintext[4] ^ Key1[5], 
                             plaintext[6] ^ Key1[6], 
                             plaintext[3] ^ Key1[7] };
            end
            else
            begin
                xor_f1  <= { plaintext[6] ^ Key2[0], 
                             plaintext[3] ^ Key2[1], 
                             plaintext[7] ^ Key2[2], 
                             plaintext[4] ^ Key2[3] };
                             
                xor_f2  <= { plaintext[7] ^ Key2[4], 
                             plaintext[4] ^ Key2[5], 
                             plaintext[6] ^ Key2[6], 
                             plaintext[3] ^ Key2[7] };      
            end
            
            p4_in  <= {s0_out, s1_out};
            
            // vG: cnt is confusing.
            cnt <= cnt + 1'b1;
            
        end 
    end
    
    // vG: you don't need a second always block techically.
    // Feistel Function Round Two   
    always@(posedge CLK)
    begin
    
        if(cnt == 3'b011)
        begin
        
            // vG:  same commentary about functions/tasks.
            // vG:  also that xor_f1, xor_f2 is defined here a second time.
            // vG:  This could be an "else" for the above always block for xor_f1/xor_f2.
            if(encrypt)
            begin
                xor_f1 <= { p4_in[0] ^ plaintext[0] ^ Key2[0], 
                            p4_in[1] ^ plaintext[1] ^ Key2[1], 
                            p4_in[3] ^ plaintext[5] ^ Key2[2], 
                            p4_in[2] ^ plaintext[2] ^ Key2[3] };
                            
                xor_f2 <= { p4_in[3] ^ plaintext[5] ^ Key2[4], 
                            p4_in[2] ^ plaintext[2] ^ Key2[5], 
                            p4_in[0] ^ plaintext[0] ^ Key2[6], 
                            p4_in[1] ^ plaintext[1] ^ Key2[7] };
            end
            else
            begin
                xor_f1 <= { p4_in[0] ^ plaintext[0] ^ Key1[0], 
                            p4_in[1] ^ plaintext[1] ^ Key1[1], 
                            p4_in[3] ^ plaintext[5] ^ Key1[2], 
                            p4_in[2] ^ plaintext[2] ^ Key1[3] };
                            
                xor_f2 <= { p4_in[3] ^ plaintext[5] ^ Key1[4], 
                            p4_in[2] ^ plaintext[2] ^ Key1[5], 
                            p4_in[0] ^ plaintext[0] ^ Key1[6], 
                            p4_in[1] ^ plaintext[1] ^ Key1[7] };
            end
                    
            p4_in <= {s0_out, s1_out};
            
            // vG: probably wrong, not sure what the intended cycle delays are.
            iip_out <= {    p4_in[0] ^ plaintext[6], 
                            p4_in[1] ^ plaintext[3], 
                            p4_in[2] ^ plaintext[4], 
                            p4_in[1] ^ plaintext[1], 
                            p4_in[2] ^ plaintext[2], 
                            p4_in[3] ^ plaintext[7], 
                            p4_in[0] ^ plaintext[0], 
                            p4_in[3] ^ plaintext[5] };
                            
            // vG:  cnt is confusing as it is defined in multiple always blocks.
            cnt <= cnt + 1'b1;
        end
    end
        
    // vG:  These probably make more sense as lookup tables or functions.
    // vG:  They are defined a fair distance from where they are used.
    // vG:  also, the case statements are missing a "default" case for the non-0/1 cases.
    // S0 Box
    always@(xor_f1)
    begin
        case(xor_f1)
            4'b0000: s0_out = 2'b01;
            4'b0001: s0_out = 2'b11;
            4'b0010: s0_out = 2'b00;
            4'b0011: s0_out = 2'b10;
            4'b0100: s0_out = 2'b11;
            4'b0101: s0_out = 2'b01;
            4'b0110: s0_out = 2'b10;
            4'b0111: s0_out = 2'b00;
            4'b1000: s0_out = 2'b00;
            4'b1001: s0_out = 2'b11;
            4'b1010: s0_out = 2'b10;
            4'b1011: s0_out = 2'b01;
            4'b1100: s0_out = 2'b01;
            4'b1101: s0_out = 2'b11;
            4'b1110: s0_out = 2'b11;
            4'b1111: s0_out = 2'b10;
        endcase
    end
 
    // S1 Box
    always@(xor_f2)
    begin
        case(xor_f2)
            4'b0000: s1_out = 2'b00;
            4'b0001: s1_out = 2'b10;
            4'b0010: s1_out = 2'b01;
            4'b0011: s1_out = 2'b00;
            4'b0100: s1_out = 2'b10;
            4'b0101: s1_out = 2'b01;
            4'b0110: s1_out = 2'b11;
            4'b0111: s1_out = 2'b11;
            4'b1000: s1_out = 2'b11;
            4'b1001: s1_out = 2'b10;
            4'b1010: s1_out = 2'b00;
            4'b1011: s1_out = 2'b01;
            4'b1100: s1_out = 2'b01;
            4'b1101: s1_out = 2'b00;
            4'b1110: s1_out = 2'b00;
            4'b1111: s1_out = 2'b11;
        endcase
    end
    
endmodule

Overall, it "looks" more like HW design because you removed the task/function constructs that are associated with "SW design". Your original uses of tasks/functions were fine. The issue still is the incorrect use of always blocks and the core concepts of clock-synchronous designs.
 
  • Like
Reactions: redsees

    redsees

    Points: 2
    Helpful Answer Positive Rating
Thanks for your replies vG and ads.
So usage of tasks/functions within a clock-synchronous design is fine?

I think I repaired the other mistakes and everything now is clear to me. Thanks again!
 

So usage of tasks/functions within a clock-synchronous design is fine?
IMO no they should be used sparingly if used at all.

I agree with both dpaul and ThisIsNotSam on your other thread, don't use tasks in synthesizable code. If for no other reason than it is not what the majority of the industry uses. I've only seen functions used and sparingly at that, I've only seen a task used once in ~20 years of doing this and it could have been easily replaced with a function. In those few instances I've seen only one of them was done for clarity. The other cases were done, I think, to show how clever they were at coding in Veilog. I actually had to examine that code for a significant time to see any reason for using the task/function, there wasn't...and the name of the thing wasn't even clear.

To me the most important thing about any code is the clarity of the code. If another engineer can come into a project and pick up your code and understand it quickly and make any required changes and/of fixes then you did your job correctly, if they have to figure out what you were doing because you are doing something completely unfamiliar to the majority of engineers then you are either being a "show-off" (using obscure language features) or trying to make yourself indispensable (nobody else can work on my code, so they have to keep me around, I'd get rid of that person ASAP).
 
  • Like
Reactions: redsees

    redsees

    Points: 2
    Helpful Answer Positive Rating
Thanks for your intense explaination ads.
What if I wanted to use two sequential statements that depend on each other, and that both are used in an LUT table to get some specific values from it in a synchronous design? That point always gets me confused in HW design.

As an example, in the first case of my code (when tasks were used), I was able to get the values of s0_out and s1_out twice in my code, and they must be sequential (one must happen before the other), that was done by providing xor_f1 and xor_f2 to a task that returns s0_out and s1_out values, that could be used twice or more even using the same variable into the LUT will get me what I want.
The problem comes when we try to implement that logic in a synchronous design, in the second code, I need to first get values of s0_out and s1_out using xor_f1 and xor_f2, and after a while, I will need again to get s0_out and s1_out values using xor_f1 and xor_f2. When that's done according to a clock tick, this is done in one clock cycle, and from your comments on this and the previous thread (and other google resources) evaluating different RHS values into same LHS variables in a synchronous design will be unpredictable, i.e which will be evaluated first/last. That's why I'm always confused how can I synchronize getting values from an LUT using same variable in the LUT's sensitivity list in synchronous designs.
 

the tasks are fine in this case. The S-Boxes can be implemented as a constant lookup table (using an init block), as a combinatorial always block, as a task, or as a function. The lookup-table or function are a little better as they allow them to be re-used. task doesn't offer any benefit over function in this case.

The main reason to avoid tasks is that there is a HW-elitism among some developers. The next reason to avoid them would be that you don't understand what benefit they would provide. If you don't see the benefit of a task over another construct, you shouldn't be using a task.


It's also sad that tasks are considered and obscure language feature to the point you would need to keep around the one guy who understands them. It takes all of five minutes to learn how tasks work, at least to the point of being able to understand code.

edit -- also, tasks in verilog and even vhdl are fairly limited in use just because of the language restrictions. Hopefully SystemVerilog will improve adoption of tasks/functions.
 
  • Like
Reactions: redsees

    redsees

    Points: 2
    Helpful Answer Positive Rating
Everything makes sense now. Thanks all.
 

Status
Not open for further replies.

Part and Inventory Search

Welcome to EDABoard.com

Sponsor

Back
Top