How to send a packet every n clock cycles in verilog?

Question

I am fairly new to Verilog and in general Digital Design. I am working on a project which has a state machine. The module, in a particular state, receives a read request packet from some other module and I have to decode the required read response data size and send back as many data bytes in the payload. Now I have to do this every n clock cycles.

I am finding it difficult to understand where should my count be updated and where should it be assigned the next count value? Here is the general structure of my code.

    always@(posedge clk_i or negedge resetn_i) begin
    if (resetn_i == 1'b0) begin
        reg_read_en         <= 1'b0;
        reg_write_en        <= 1'b0;
        count               <= 3'b111;
        next_count          <= 3'b111;  
        state               <= S_INITIAL;

    end else begin
        state               <= next_state;
        count               <= next_count;
    end

This is the sequential block.Now in my combinational block

always@(*) begin

    next_state  = state;
    next_count  = count;
    .......
    ........
    M_PASSIVE:begin                         
                        if(count == 3'b000)
                        begin
                           //Update some registers with new data values
                        end else begin
                        next_count = count - 3'b001;
                        end

I am unable to understand does this wait the required count no of cycles and then update the reg values or I am missing something here.

I would very much appreciate some clarity about this.

P.S. Pardon me if I am missing some fundamental aspects of behavioural design regarding difference between sequential and combinational blocks.

Here is a complete code of what I am trying to do. Essentially I want to read a packet, turn of reading of packets, wait 7 or 8 clock cycles or N clk cycles and then send a read response with the data in the registers which are assigned to output ports continuosly.

    always@(posedge clk_i or negedge resetn_i) begin
    if (resetn_i == 1'b0) begin
        reg_read_en         <= 1'b0;
        reg_write_en        <= 1'b0;
        count           <= 3'b111;
        state           <= S_INITIAL;
    end else begin
        state           <= next_state;
        count           <= next_count;
    end 
end 

always@(*) begin
    next_state  = state;
    next_count  = count;
    case(state)
        S_INITIAL: begin //not enabled
            if(enable_i == 1'b1) begin                  
                next_state = S_ENABLED;
            end
            reg_read_en         <= 1'b0;
            reg_write_en        <= 1'b0;
        end 

        S_ENABLED: begin
            if(enable_i == 1'b0) begin
                next_state = S_INITIAL;                 
            end

            case(mode_i)
                M_PASSIVE:begin
                    reg_read_en =   dnoc_packet_ready;
                    //pkt_type  =   dnoc_packet_pld_header[]
                    if(dnoc_packet_ready)
                    begin
                        reg_read_en = 1'b0;
                        if(count == 3'b000)
                        begin
                        reg_write_en    =   1'b1; 
                        read_resp_tx_data_size = 3'b001;     
                        read_resp_tx_qpe_dest  = 6'b000010;      
                        read_resp_tx_mod_dest   = 5'b00000;
                        read_resp_tx_c_routing  = 0;
                        read_resp_tx_pld_header = 17'b00100101000000000;
                        read_resp_tx_pld_address    = 32'h0001;   
                        read_resp_tx_pld_data   = 256;
                                                        //count = 3'b111; 
                        reg_write_en    =   1'b0;
                        end
                        next_count = count - 3'b001;
                    end
                end

If more information about the specific context is required, please ask. — frisco_1989, Feb 15 '18 at 11:44
Splitting the state machine into two like this is necessary in some cases, but it is overcomplicating things here, especially since you're new. Take a look at this answer I wrote, it includes an example of a full state machine with counter: https://electronics.stackexchange.com/questions/57035/n-bit-shift-register-serial-in-serial-out-in-vhdl/57042#57042 I highly recommend avoiding combinational always blocks (the ones with the (*) in the sensitivity list) until you get used to coding with just sequential blocks and get a feel for "this code -> this behaviour" — stanri, Feb 15 '18 at 14:16
@stanri Thank you very much for the reply. I will look through the answer you mentioned. I will write back here if I need more clarifications. — frisco_1989, Feb 15 '18 at 14:27
@stanri when you say a signal must be driven from only one proper process, do you mean reg or wire? — frisco_1989, Feb 15 '18 at 14:41
I realise the link I gave you was VHDL, but the principle still applies. `reg`s are used in sequential process blocks, so I'm talking about a `reg` in Verilog. (By the way, is SystemVerilog an option for you to learn? SystemVerilog does away with the reg/wire distinction, amongst other things) — stanri, Feb 15 '18 at 14:56
It is an option but sadly not for the project im working on. — frisco_1989, Feb 15 '18 at 15:06
I am also facing the dilemma of how can i keep the write enable high till count is reached to zero. — frisco_1989, Feb 15 '18 at 15:09
Why do you have reg_write_en twice in the if(dnoc_packet_ready) block? What are you trying to achieve? — stanri, Feb 15 '18 at 15:20
Do you want reg_write_enable to be high the entire time it's counting down (all the time from 7 down to 0) or just for the one clock cycle that it's at 0? — stanri, Feb 15 '18 at 15:24
Let us [continue this discussion in chat](http://chat.stackexchange.com/rooms/73210/discussion-between-frisco-1989-and-stanri). — frisco_1989, Feb 15 '18 at 17:16

Oldfart · Answer 1 · 2018-02-15T15:27:20.837

You are very close. The error is the 'next_count' is only updated in the combinatorial always @(*) block and 'count' is only update in the register/clocked part always@(posedge clk_i....

Thus remove next_count <= 3'b111;

I can't see the rest of your code so start with that.

You did not say how many cycle you have to wait: 7 or 8? I'll be honest. Most of the time I write code which gets me 99% of the way. If the data is stored after 7 or 8 cycles are details which are so easily solved that I often run the simulation first and then make a +1 or -1 correction.

Post edit after new code:
I can't debug your state machine for you, but I can make some remarks. You set reg_write_en and some lines lower later clear it again.

I assume this is supposed to really work in hardware not just in simulation. If so your code produces lots of latches. You will not be notified of those until you synthesis the code. For example these:

read_resp_tx_qpe_dest      
read_resp_tx_mod_dest  
read_resp_tx_c_routing 
reg_read_en
reg_write_en

In your combinatorial section always @(*) you have to assign a value to every variable in every state. If not you get latches. One way to do that is what you do with next_state and next_count: set a default value at the top. But then it is also good practice to document that it is a default value which will be changed later.

// Defaults:
next_state  = state;
next_count  = count;
reg_write_en = 1'b0; 
reg_read_en = 1'b0;

The first two have to stay the same if not touched.
The other two are examples of variables which are only set in one or two states and are zero everywhere else.
As for your 'read_resp_tx...' variables: I don't know what else is done with them so you have to decide how to handle them.

As to splitting your code in a register an combinatorial section: That is up to you. Whatever you are comfortable with. I tend to do it a lot unless it is a trivial state machine. I have seen code from a very big IP company which provides millions of CPU cores all over the world and they use it everywhere. I suspect it is obligatory in their coding style guide.

Thank you for the answer. I removed the `next_count <= 3'b111` from the sequential block. I have edited my question to include the full code of what I am trying to do. So you will be able to better understand the way regs are being updated. — frisco_1989, Feb 15 '18 at 13:42
I have found that the the register/combinitorial state machine split is useful if you want to detect and work off state transitions `if ((state != next_state) && (next_state = MY_STATE))`. This means that instead of detecting the *condition that triggers* the state change, you detect the *state change itself*, and this allows you to change the state-change conditions in one place only. Register-only state machines make it a lot harder to do this. In the register-only case, you need to register the state and do `state != state_z`, which is delayed by 1cc from the transition. — stanri, Feb 15 '18 at 15:45
But in the case of the OP, they don't need to work off state transitions and they're a beginner, so a register-only method makes a lot more sense (no inferred latches, etc) — stanri, Feb 15 '18 at 15:49

stanri · Accepted Answer · 2018-02-15T15:41:34.977

There are a couple of things to point out in your code.

1) you're driving the reg_read_en and reg_write_en signals in both always blocks.

always@(posedge clk_i or negedge resetn_i) begin
if (resetn_i == 1'b0) begin
    reg_read_en         <= 1'b0; <=
    reg_write_en        <= 1'b0; <=
    count               <= 3'b111;
    next_count          <= 3'b111;  
    state               <= S_INITIAL;

end else begin
    state               <= next_state;
    count               <= next_count;
end

and

always@(*) begin
next_state  = state;
next_count  = count;
case(state)
    S_INITIAL: begin //not enabled
        if(enable_i == 1'b1) begin                  
            next_state = S_ENABLED;
        end
        reg_read_en         <= 1'b0; <==
        reg_write_en        <= 1'b0; <==
    end

This isn't synthesizable and probably wouldn't be allowed by some simulation tools either.

2) You're inferring latches in your combinitorial blocks:

always@(*) begin
next_state  = state;
next_count  = count;
case(state)
    S_INITIAL: begin //not enabled
        if(enable_i == 1'b1) begin         <==         
            next_state = S_ENABLED;        <==
        end                                <==
        reg_read_en         <= 1'b0;
        reg_write_en        <= 1'b0;
    end

The marked if statement needs a corresponding else in every case. You need to cover all the different possible options.

This is why in my comment I suggested you use a single sequential block, it'll fix both of these issues, and make everything a lot easier to keep track of.

So here's an example of combining them:

always@(posedge clk_i or negedge resetn_i) begin
  if (resetn_i == 1'b0) begin
     reg_read_en              <= 1'b0;
     reg_write_en             <= 1'b0;
     count                    <= 3'b111;
     state                    <= S_INITIAL;

     // I was trained to give everything a reset value
     // But that's not really necessary and is more a matter of style
     // as long as it gets a value when it needs one.
     // including this here for completeness
     read_resp_tx_data_size   <= 3'b000;
     read_resp_tx_qpe_dest    <= 6'b000000;
     read_resp_tx_mod_dest    <= 5'b00000;
     read_resp_tx_c_routing   <= 0;
     read_resp_tx_pld_header  <= 17'b00000000000000000;
     read_resp_tx_pld_address <= 32'h0000;
     read_resp_tx_pld_data    <= 0;

  end
  else begin

     case(state)
       S_INITIAL: begin //not enabled
          if(enable_i == 1'b1) begin
             state <= S_ENABLED;

             // set count to the initial value here
             // this way, you re-set it every time you switch to enabled
             count      <= 3'b111;


             // If there's anything else you want to setup, here's a good place to do it...

          end
          else begin
             state <= S_INITIAL;

          end

          reg_read_en  <= 1'b0;
          reg_write_en <= 1'b0;

       end

       S_ENABLED: begin
          if(enable_i == 1'b0) begin
             state <= S_INITIAL;

             // here's where you would clear any signals that you want to clear before going back to the initial state


          end
          else begin
             state <= S_ENABLED;

          end

          case(mode_i)
            M_PASSIVE:begin
               reg_read_en <= dnoc_packet_ready;

               //pkt_type  =   dnoc_packet_pld_header[]
               if(dnoc_packet_ready) begin
                  reg_read_en <= 1'b0;

                  if(count == 3'b000) begin
                     reg_write_en             <= 1'b1;
                     read_resp_tx_data_size   <= 3'b001;
                     read_resp_tx_qpe_dest    <= 6'b000010;
                     read_resp_tx_mod_dest    <= 5'b00000;
                     read_resp_tx_c_routing   <= 0;
                     read_resp_tx_pld_header  <= 17'b00100101000000000;
                     read_resp_tx_pld_address <= 32'h0001;
                     read_resp_tx_pld_data    <= 256;

                     // you can reset count here to 7 if you want it to start again
                     count                    <= 3'b111;


                  end
                  else begin

                     // this will make reg_write_en valid only when count is 0
                     // and it will be disabled the rest of the time.
                     // if you want it valid all the time, you can set this to 1 and clear it elsewhere.
                     reg_write_en <= 1'b0;



                     // this stops the counter from underflowing
                     // otherwise, when it's 0 it will underflow to 111.
                     // Unless that's what you want, of course.
                     count        <= count - 3'b001;

                  end

               end
            end
            // other cases...
          endcase

       endcase
  end
end

There are inferred latches (such as all the`read_resp_tx_*`), but `next_state` is not one of them. Note the `next_state = state;` which provides a default value, thereby satisfying the conditions for the synthesizer to see it as a combinational logic. A latch is inferred where there is at least one path the variable will not be assigned withing the entire `always @*` block. — Greg, Feb 15 '18 at 17:11
Thank you @Greg for the elaborate answer. I will try this in my simulation and see how it works. I have another question. Lets say I keep this running and I am sending data every N clock cycles. If I change the tx_pld address and data in the if statement where the count falls through, would it reflect in a single clock cycle? Is that what posedge clk means, at a given clokc edge? — frisco_1989, Feb 15 '18 at 17:40
Thank you @Greg for the help. It works for me. More importantly thank you so much for the beautiful explanation of the concept of inferred latches and how to avoid them. — frisco_1989, Feb 16 '18 at 12:40
Just to add to the discussion, I would want a delay of one clock cycle when I enable my write enable signal to clear it. How can I do this in the combinational block? I did some research and this can be done by introducing a dummy state where I can jump to and then come back to my original state. Would this introduce a one clock cycle delay? Like in the M_PASSIVE case if i receive a packet, I want to write it back to my FIFO and so the write enable needs to high for exactly one clock cycle `S_ONECLK_DELAY: next_state <= S_ENABLED;`. But write enb is always high.. Am I missing something? — frisco_1989, Mar 13 '18 at 10:23

How to send a packet every n clock cycles in verilog?

2 Answers2