[SOLVED] Design Synchronization question and Verilog Code Review

redsees · Nov 24, 2016

Hello World,

I finished my first project in Verilog, which is an implementation for Simplified Data Encryption Algorithm (DES) that uses 8-bit data blocks, 10-bit key, 2 8-bit subkeys, and 2 rounds Feistel Function.

I tested the code and it works great. I just wanted from Verilog experts to review my code and give any advice they might have to improve the application (in any aspect, space or time) as this is my first Verilog project, and I want to get the maximum learning from it. Thanks in advance.

Here I'm not using any clock for synchronizing the execution, my second question is: is using a clock in every digital design I make mandatory even if my design works well without any clocks? I know it's always better to use a clock signal in order to predict outputs in different time frames, but otherwise is it a must to use it?

Here is my 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
/*
    Simplified Data Encryption Standard (S-DES)
    8-bit 2-round block cipher encryption and decryption algorithm using 10-bit key.
*/
 
 
module SDES(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 = iip_out;
    
    // 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)
    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
    
endmodule

Notes:
- I'm using 0:N-1 bit ordering because this is the famous bit ordering used in cryptography.

vGoodtimes · Nov 24, 2016

Practical designs will have a clock. The design you have performs a fairly simple function. However, it is common to have an output register for the majority of modules, and comment when the outputs are not registered. The logic you wrote could also be done using a function, as it has no memory and only 1 output. However, most modules will have multiple outputs as well as memory elements.

There are several alternative constructs that you can use. For example, the S_Box can be implemented as a function or an array.

tempB/tempD should have better names.

The use of tasks is well-done. Tasks are less used in synthesizable code, so some developers will be unfamiliar with them. There may be some prejudice against them as they are associated with software design. This leads to developers avoiding them.

You should understand when tasks are useful in order to understand why your use was justified. Determine how much logic is actually inferred by each task.

redsees · Nov 25, 2016

Thanks vGoodtimes for your reply.

Is it possible to determine the amount of gates used in a specific portion of my design code?
For example, how can I know if storing S Boxes' values into an array is better than using switch case inside a task? Is it possible to know the number of gates used in each case and decide whether to go for the array solution or the task one?

Also, I'd like to receive more advices/reviews about the code, so I won't mark the thread as solved for now. If this is wrong, please let me know.

Thanks!

ThisIsNotSam · Nov 25, 2016

so, essentially, you wrote a piece of software but using verilog. it's bad.

vGoodtimes · Nov 25, 2016

ThisIsNotSam said:
so, essentially, you wrote a piece of software but using verilog. it's bad.

Nope? Nope. Nope! The logic in the tasks reduce to very simple logic. Why is this specific case bad?

@redsees: ok, so there is a HUGE prejudice against using task/function in synthesizable code because there is an association with SW design because

That said, you should understand why tasks are acceptable here, as they can be misused.

dpaul · Nov 25, 2016

I am prejudiced against using task (the only reason is because I have myself never used it and most legacy codes/cores do not use it). I remember using function once and it was synthesizable.

In my opinion a clock is necessary. Since there is no clock, your use of blocking statements is justified

But you can use tasks inside a clocked always block and your code is synthesizable. You can use tasks to replicate repetitive code without adding a lot of code lines. It should work without problems.

redsees · Nov 25, 2016

ThisIsNotSam said:
so, essentially, you wrote a piece of software but using verilog. it's bad.

Why is it bad? You didn't provide any further details about the difference between a "good" and a "bad" design, and that's what I'm specifically looking for.

vGoodtimes said:
@redsees: ok, so there is a HUGE prejudice against using task/function in synthesizable code because there is an association with SW design because

That said, you should understand why tasks are acceptable here, as they can be misused.

I still can't see why using tasks in a synthesizable code is considered a bad practice? Generally, why using something technically associated with Software Development is considered a bad practice in Digital Design as long as it's used correctly from a hardware design perspective?

Tasks are used for avoiding repetition in code, they are also synthesizable if used correctly, then what's wrong in using them?

dpaul · Nov 25, 2016

Tasks are used for avoiding repetition in code, they are also synthesizable if used correctly, then what's wrong in using them?

I don't think anyone has told you that it is "wrong in using them".
You must understand that bad coding and wrong coding is totally different.

Anyways, I have already voiced my opinion regarding tasks & functions and will stick with it.

FvM · Nov 25, 2016

Behavioral code in general and particularly constructs like tasks are an abstraction form hardware level. Using it without understanding which hardware will synthesized from it brings up a risk of writing ineffective HDL (slow or with excessive logic utilization).

I tested the code and it works great.

Presumed it's functionally correct (I didn't check), the purely combinational design works only with acceptable performance because it's so small. A real DES unit won't without a clock and some kind of sequential processing.

Your comments suggest that you didn't yet understand the relevance of a synchronous topology for real FPGA designs, otherwise you would use it from the start, also for a tiny 8 bit encryption demonstration unit.

vGoodtimes · Nov 25, 2016

The main point is that you understand why tasks are applicable. But perhaps not too much of an understanding. Once you understand tasks, you realize that Verilog is not well suited for hardware design despite being used for HW design.

ThisIsNotSam · Nov 26, 2016

vGoodtimes said:
Nope? Nope. Nope! The logic in the tasks reduce to very simple logic. Why is this specific case bad?

I have been doing this for many many years. the use of tasks, combined with the lack of clock, tells me the OP has the same issues my students have/had. they want to code software at all costs. they throw some events in (always @) and think is good. "if it simulates, it works". well, no.

redsees · Nov 28, 2016

Thanks all!
I think I will have to read a few projects in Verilog and read some resources to learn how a good HW code is written.

Regards

ads-ee · Nov 28, 2016

redsees said:
read some resources to learn how a good HW code is written.

Then don't read (much of) anything online...the majority examples can be lumped into the "bad coding" group. Only a few of the sites actually have what I would consider good coding examples.

vGoodtimes said:
you realize that Verilog is not well suited for hardware design despite being used for HW design.

I'd really like to know why you think this is the case. One could easily make the same case for VHDL. IMO it makes more sense to say that using text to describe hardware beyond gate level and structural code is not really suited to hardware. Any level of abstraction moves away from hardware.

redsees · Nov 29, 2016

ads-ee said:
Then don't read (much of) anything online...the majority examples can be lumped into the "bad coding" group. Only a few of the sites actually have what I would consider good coding examples.

Can you recommend some/any please?

Regards

vGoodtimes · Nov 29, 2016

ads-ee said:
I'd really like to know why you think this is the case. One could easily make the same case for VHDL. IMO it makes more sense to say that using text to describe hardware beyond gate level and structural code is not really suited to hardware. Any level of abstraction moves away from hardware.

My reason is because both VHDL and Verilog are primarily designed as simulation languages that sometimes get synthesis support. Language features that can be synthesized do not get synthesis support for years or even decades. Design choices for the languages always seem to focus on simulation, and making a simulators job easier, than on synthesis and making a logic designer's job easier.

ads-ee · Nov 29, 2016

redsees said:
Can you recommend some/any please?

Regards

Doulos has a number of examples, but they all use some ugly not easy to maintain module port declarations, which I absolutely dislike. IMO 1 port per line (so you can add comments or insert new ports wherever you need to).
https://www.doulos.com/

ASIC World tends to use the antiquated Verilog module port declarations that have been around since 1987. Hey it's 2016 15 years past the introduction of C-style port declarations (which also require less repetition). I haven't seen a tool in the last 5+ years that doesn't support Verilog 2001 port declarations. Besides that the code is okay.
https://www.asic-world.com/examples/verilog/index.html

Welcome to EDAboard.com

[SOLVED] Design Synchronization question and Verilog Code Review

Junior Member level 1

Advanced Member level 4

Junior Member level 1

Advanced Member level 5

Advanced Member level 4

Advanced Member level 5

Junior Member level 1

Advanced Member level 5

Super Moderator

Advanced Member level 4

Advanced Member level 5

Junior Member level 1

Super Moderator

Junior Member level 1

Advanced Member level 4

Super Moderator

Similar threads

Part and Inventory Search

Welcome to EDABoard.com

Sponsor