1

I am trying to output one bit at a time via SPI from a know 2D array.

logic [7:0] fpga_status_queue [0:17],

My state machine is for some reason going to a weird state.

18'h

Here is my code:

module FPGA_STATUS_READ (
     input clk,
    input sclk, 
    input cs, // Clock Enable
    input rst_n,  // Asynchronous reset active low
    input fpgastatus_command,
    input logic [3:0] group_control,
    input logic [7:0] loopback,
    output logic [7:0] fpga_status_queue [0:17],
    output logic dout
);

    // `include "../../config_files/rtl_config.svh"

    assign fpga_status_queue[ 0] = 8'h1;//{group_control};
    assign fpga_status_queue[ 1] = 8'h1;//{loopback}; 
    assign fpga_status_queue[ 2] = 8'h1;//{synth_version[0]}; 
    assign fpga_status_queue[ 3] = 8'h1;//{synth_version[1]}; 
    assign fpga_status_queue[ 4] = 8'h1;//{synth_version[2]}; 
    assign fpga_status_queue[ 5] = 8'h1;//{grid_version[0]}; 
    assign fpga_status_queue[ 6] = 8'h1;//{grid_version[1]}; 
    assign fpga_status_queue[ 7] = 8'h1;//{grid_version[2]}; 
    assign fpga_status_queue[ 8] = 8'h1;//{pa_version[0]}; 
    assign fpga_status_queue[ 9] = 8'h1;//{pa_version[1]}; 
    assign fpga_status_queue[10] = 8'h1;//{pa_version[2]}; 
    assign fpga_status_queue[11] = 8'h1;//{hdl_version[0]}; 
    assign fpga_status_queue[12] = 8'h1;//{hdl_version[1]}; 
    assign fpga_status_queue[13] = 8'h1;//{hdl_version[2]};     
    assign fpga_status_queue[14] = '1;
    assign fpga_status_queue[15] = '1;
    assign fpga_status_queue[16] = '0;
    assign fpga_status_queue[17] = '0;

    logic bit_run_counter;
    logic bit_load_counter;
    logic [4:0] bit_current_value;
    logic bit_count_reached;

    logic word_run_counter;
    logic word_load_counter;
    logic [4:0] word_current_value;
    logic word_count_reached;

    typedef enum logic [2:0] {IDLE, CS_WAIT, MISO_OUT, BIT_CHANGE, WORD_CHANGE} status_states;
    status_states current_state, next_state;

    always_ff @(posedge clk or negedge rst_n) begin : step_forward
        if(!rst_n)
            current_state                        <= IDLE;
        else 
            current_state                        <= next_state;
    end : step_forward

    always_comb begin : set_next_state
        next_state = IDLE;
        case (current_state)
            IDLE       : next_state = fpgastatus_command ? CS_WAIT : IDLE;
            CS_WAIT    : next_state = ~cs ? MISO_OUT : CS_WAIT;
            MISO_OUT   : begin//next_state = sclk ? bit_count_reached ? word_count_reached ? IDLE: WORD_CHANGE : BIT_CHANGE: MISO_OUT;
                if (sclk && bit_count_reached && word_count_reached) 
                    next_state = IDLE;
                else if (sclk && bit_count_reached) 
                    next_state = WORD_CHANGE;
                else if (sclk)
                    next_state = BIT_CHANGE;
                else 
                    next_state = MISO_OUT;
            end
            BIT_CHANGE : next_state = MISO_OUT;
            WORD_CHANGE: next_state = MISO_OUT;
            default : next_state = IDLE;
        endcase
    end

    always_comb begin : cntr_logic
        bit_run_counter           = '0;
        bit_load_counter          = '0;
        word_run_counter          = '0;
        word_load_counter         = '0;
        dout                      = '0;
        unique case (current_state)
            IDLE       :begin 
                bit_load_counter  = '1;
                word_load_counter = '1;
            end 
            CS_WAIT    :begin 
                bit_load_counter  = '1;
                word_load_counter = '1;
            end 
            MISO_OUT   :begin 
                dout              = fpga_status_queue[word_current_value][bit_current_value];
            end
            BIT_CHANGE :begin 
                bit_run_counter   = '1;
            end 
            WORD_CHANGE:begin 
                word_run_counter  = '1;
                bit_load_counter  = '1;
            end 
            default : dout        = '0;
        endcase
    end

    up_down_counter #(
            .ABSOLUTE_DATA_WIDTH(4)
        ) inst_bit_counter (
            .clk           (clk),
            .run_counter   (bit_run_counter),
            .rst_n         (rst_n),
            .count_value   (5'h7),
            .load_counter  (bit_load_counter),
            .up_counter    ('0),
            .current_value (bit_current_value),
            .count_reached (bit_count_reached)
        );

    up_down_counter #(
            .ABSOLUTE_DATA_WIDTH(5)
        ) inst_word_counter (
            .clk           (clk),
            .run_counter   (word_run_counter),
            .rst_n         (rst_n),
            .count_value   (5'h11),
            .load_counter  (word_load_counter),
            .up_counter    ('0),
            .current_value (word_current_value),
            .count_reached (word_count_reached)
        );

endmodule

It should go to WORD_CHANGE but both WORD_CHANGE and MISO_OUT for the next state for current state.

Zoomed out image of waves zoomed in image of waves

Sabersimon
  • 50
  • 2
  • 8
  • 1
    how are the states defined? I guess, 18 represents WORD_CHANGE. What is its value? – Serge Jan 24 '19 at 11:55
  • Why use a `default` with a `unique case` statement? Seems to lose all the benefits of using `unique`. If you took out the `default`, the tools should warn you that your state `18'h` does not match any valid states. – Kevin Kruse Jan 24 '19 at 16:07
  • Thanks for the responses. The problem is displayed in the last pic. Where the state values are high on two states. This should be impossible. – Sabersimon Jan 24 '19 at 17:57
  • Are there any timing violations reported? Is there any false path constraints set on paths leading up to the signal tap capture buffer/memories? – Pulimon Jan 24 '19 at 19:10
  • have you simulated this? how did it go? Actually i do not understand, why `current_state` has state values as members. Do you have an explanation? – Serge Jan 24 '19 at 19:48
  • 4
    @Serge SignalTap in my experience always converts FSM states into one-hot encoding, so no matter how you define `current_state`, when added to SignalTap, it will always make 1 bit per state with the state name as the signal name (https://stackoverflow.com/questions/29308500/quartusii-synthesis-enumerated-type-to-state-signals-encoding). So Im not sure how it does get to `current_state` having two bits set unless something messed up in synthesis maybe (or timing)? Any warnings @Sabersimon? – Unn Jan 24 '19 at 20:41
  • @Unn I am checking the timing. I will get back to you. – Sabersimon Jan 24 '19 at 22:08
  • @Unn on a side note Quartus thinks that two signals are clocks and are not. How to I tell Quartus that they are not clocks. – Sabersimon Jan 24 '19 at 22:30
  • I'm not sure of a way to prevent that, are you using these signals in sensitivity lists? If so, that might be the reason they are being treated as clock signals and you should make sure your design is properly synchronous – Unn Jan 25 '19 at 18:21

1 Answers1

2

This is almost certainly a timing issue. I'm guessing that sclk is not synchronous to clk - probably it's connected directly to a device input pin.

The problem is this piece of code:

(...)
else if (sclk)
   next_state = BIT_CHANGE;
else 
   next_state = MISO_OUT;

Whenever sclk transitions from zero to one, logic will raise the next_state bit corresponding to BIT_CHANGE, and in parallel lower the next_state bit corresponding to MISO_OUT. As this happens, there can be a brief moment where both bits are set or no bits are set, depending on which logic is faster. If you are unlucky and have a raising clk at this exact moment, you will get into the situation you are observing, where the state machine appears to be in two states at the same time.

The solution is to synchronize sclk, cs, and any other signals that determine the next state to clk. Such synchronization is typically done by simply sending the signals through two flip-flops.

pc3e
  • 1,299
  • 11
  • 18