Arty A7-35 Dev Board, timing issues in design

[Kronkulus]

Hey folks,

VHDL/FPGA/Xilinx noob here, and I've been pulling my hair out over a design I've been working on. I have an Arty A7 dev board, the -35T variant. I wrote UART transmitter and receiver modules in VHDL and that worked fine, using a bufg instantiation to buffer the input clock, and then the clk_gen wizard to take that bufferd 100MHz down to 10MHz. It works when the entire design is using 10MHz, however I wanted to try and get it working at the 100MHz input just because.

What this design does is read the auxiliary ADC channels 1 and 9, and then spits out the readings over UART to my PC, where another piece of software I wrote takes that data from the com port and graphs it. With everything using a 10MHz clock, the design works. It waits for my PC to send ASCII 't' and then begins reading ADC values and transmitting them. However with putting the buffered 100MHz into everything I get timing errors in Vivado, and nothing works. In college we never really talked about clock management or anything like that, how do I begin to figure out where the source of the timing issue is? I've attached two screenshots showing what the implementation timing report is showing me.

I'm not exactly sure what Vivado is telling me but it looks bad. I tried double registering the serial_out and transmitting signals and that made no change to the timing errors. Any suggestions? I've also attached all my code minus the IP generated if anyone cares to dig deeper to try and help.

Thanks!

[miken]

The first thing to do is to open up the detailed reports on the failing paths. Double-click a failing line to bring up the report. You can also look in the right-click menu to generate a schematic representation. Struggle with it a bit and see if you can figure out what is wrong.

[Kronkulus]

So I went in and looked at the failing paths, serial_out and transmitting. The schematic had FF/Latches, I remember latches being bad. I also thought maybe the number's its dealing with are wrong, so I looked at the constraints file.

set_property IOSTANDARD LVCMOS33 [get_ports clk]
set_property PACKAGE_PIN E3 [get_ports clk]

set_property PACKAGE_PIN G13 [get_ports serial_out]
set_property IOSTANDARD LVCMOS33 [get_ports serial_out]
set_property DRIVE 16 [get_ports serial_out]

set_property PACKAGE_PIN B11 [get_ports serial_in]
set_property IOSTANDARD LVCMOS33 [get_ports serial_in]

set_property PACKAGE_PIN G4 [get_ports transmitting]
set_property IOSTANDARD LVCMOS33 [get_ports transmitting]

set_property PACKAGE_PIN F13 [get_ports vauxp9]
set_property PACKAGE_PIN F14 [get_ports vauxn9]
set_property PACKAGE_PIN C12 [get_ports vauxp1]
set_property PACKAGE_PIN B12 [get_ports vauxn1]
set_property IOSTANDARD LVCMOS33 [get_ports vauxn1]
set_property IOSTANDARD LVCMOS33 [get_ports vauxn9]
set_property IOSTANDARD LVCMOS33 [get_ports vauxp1]
set_property IOSTANDARD LVCMOS33 [get_ports vauxp9]

set_property PACKAGE_PIN D9 [get_ports reset_in]
set_property IOSTANDARD LVCMOS33 [get_ports reset_in]

create_clock -period 10.000 -name clk -waveform {0.000 5.000} [get_ports clk]

create_generated_clock -name clk_bufg -source [get_ports clk] -multiply_by 1 -add -master_clock clk [get_pins clk_IBUF_inst/O]
set_input_delay -clock [get_clocks clk_bufg] -min -add_delay 2.000 [get_ports reset_in]
set_input_delay -clock [get_clocks clk_bufg] -max -add_delay 4.000 [get_ports reset_in]
set_input_delay -clock [get_clocks clk_bufg] -min -add_delay 2.000 [get_ports serial_in]
set_input_delay -clock [get_clocks clk_bufg] -max -add_delay 4.000 [get_ports serial_in]
set_output_delay -clock [get_clocks clk_bufg] -min -add_delay 0.000 [get_ports serial_out]
set_output_delay -clock [get_clocks clk_bufg] -max -add_delay 2.000 [get_ports serial_out]
set_output_delay -clock [get_clocks clk_bufg] -min -add_delay 0.000 [get_ports transmitting]
set_output_delay -clock [get_clocks clk_bufg] -max -add_delay 2.000 [get_ports transmitting]

Not sure if these are valid constraints but it seems to still synthesize and implement. With these constraints the errors seem to be getting smaller, as in the slack is much closer to being positive now.

[rstofer]

Latches are commonly created when output signals from FSMs don't have a defined default value.  Here is a snippet of my code and you can see the number of default assignments.  These get overridden later on inside the FSM.

Code: [Select]

-- FSM
    process(State, BEN, IR,MemReady, Immediate, PSR, Interrupt)
    begin
        -- set default values for all signals
        GateBusSelect   <= GatePC;
        MIOenable       <= '0';
        LD_MAR          <= '0';
        LD_MDR          <= '0';
        LD_IR           <= '0';
        LD_BEN          <= '0';
        LD_PC           <= '0';
        LD_Reg          <= '0';
        LD_CC           <= '0';
        LD_Privilege    <= '0';
        LD_Priority     <= '0';
        LD_PSR          <= '0';
        LD_SavedUSP     <= '0';
        LD_SavedSSP     <= '0';
        LD_VectorAddr   <= '0';
        PCselect        <= PCplus1;
        MARSelect       <= MARzext;
        Addr1Select     <= Addr1PC;
        Addr2Select     <= Addr2Zero;
        SR2select       <= SR2OutSel;
        SR1addrSelect   <= SR1usesSR1;
        ALUop           <= ALUpassA;
        DRaddrSelect    <= DRusesDR;
        MemWr           <= '0';
        SPselect        <= SPminusOnesel;
        VectorMux       <= "00";
        Privilege       <= '0';
       
        case State is
            when  0     =>  -- Branch Instruction
                            if BEN = '1' then
                                NextState   <= 22;  -- branch is taken
                            else
                                NextState   <= 18;  -- branch not taken   
                            end if;                             
            when  1     =>  -- ADD Instruction
                            if Immediate = '1' then
                                SR2select <= Sext5sel;
                            else
                                SR2select <= SR2outSel;
                            end if;
                            ALUop           <= ALUadd;
                            GateBusSelect   <= GateALU;
                            LD_Reg          <= '1';
                            LD_CC           <= '1';
                            NextState       <= 18;                           


Look at ALUop in state 1 and then look above for the default value - ALUpassA.  In every state that doesn't involve ALUop, for example state 0, it will still have the default value.

Or, look at this snippet from VHDLwhiz.com:

Code: [Select]

    process(Clk) is
    begin
        if rising_edge(Clk) then
            if nRst = '0' then
                -- Reset values
                State   <= NorthNext;
                Counter <= 0;
                NorthRed    <= '1';
                NorthYellow <= '0';
                NorthGreen  <= '0';
                WestRed     <= '1';
                WestYellow  <= '0';
                WestGreen   <= '0';
 
            else
                -- Default values
                NorthRed    <= '0';
                NorthYellow <= '0';
                NorthGreen  <= '0';
                WestRed     <= '0';
                WestYellow  <= '0';
                WestGreen   <= '0';
 
                Counter <= Counter + 1;
 
                case State is
 
                    -- Red in all directions
                    when NorthNext =>
                        NorthRed <= '1';
                        WestRed  <= '1';
                        -- If 5 seconds have passed
                        if Counter = ClockFrequencyHz * 5 -1 then
                            Counter <= 0;
                            State   <= StartNorth;
                        end if;


https://vhdlwhiz.com/finite-state-machine/

All outputs from FSMs must be defined under every condition.  Even if the value is just a default.

[asmi]

set_input_delay -clock [get_clocks clk_bufg] -min -add_delay 2.000 [get_ports reset_in]
set_input_delay -clock [get_clocks clk_bufg] -max -add_delay 4.000 [get_ports reset_in]
set_input_delay -clock [get_clocks clk_bufg] -min -add_delay 2.000 [get_ports serial_in]
set_input_delay -clock [get_clocks clk_bufg] -max -add_delay 4.000 [get_ports serial_in]
set_output_delay -clock [get_clocks clk_bufg] -min -add_delay 0.000 [get_ports serial_out]
set_output_delay -clock [get_clocks clk_bufg] -max -add_delay 2.000 [get_ports serial_out]
set_output_delay -clock [get_clocks clk_bufg] -min -add_delay 0.000 [get_ports transmitting]
set_output_delay -clock [get_clocks clk_bufg] -max -add_delay 2.000 [get_ports transmitting]

I'm not a VHDL person, but as far as I can tell all these constraints make no sense. First off, your modules seem to have an asyncronous reset, and so an input constraint on it makes no sense. Next, you seem to claim implementing UART, well "A" in UART stands for "asyncronous" and therefore it can come in at any time. And besides, syncronizing output signals to internal clock would only make sense if you have another external device that expects output singals to be syncronous to that clock. But again, UART is asyncronous by definition. So unless I got something completely wrong (which can happen - we're all humans and can make mistakes), you need to get rid of all input and output delays and leave only clock constraint. Also Vivado is smart enough to direct a clock signal into a clock buffer, so no need to explicitly specify that - though you can if you want to, but I think it just adds more noise into already noisy VHDL code, same for IBUF/OBUF's - synthesizer will add them automatically. You only need to explicitly instantiate these buffers if you want to use some kind of advanced functionality of those buffers which can not be inferred easily from the code.

[hamster_nz]

Hi!

This looks a little odd...  in UART_TX you have a clocked process that updates the 'state' from 'next_state', but the process that updates next_state is itself clocked - in the two process paradigm it's usually async.

Code: [Select]

   state_machine : process (clk)
   begin
        if rising_edge(clk) then
            state <= next_state;
        end if;
    end process state_machine;
   
    tx_main : process (clk)
    begin   
        if rising_edge(clk) then
            case state is
                when idle =>
                    serial_out <= '1';
                    transmitting <= '0';
                    finished <= '0';
                    clk_count <= 0;
                    bit_count <= 0;
                   
                    if data_valid = '1' then
                        byte <= data_in;
                        next_state <= start_bit;
                    end if;


Also, in UART_RX, it can get trapped into "start_bit" state by a short glitch on the incoming line, causing bad stuff.

Code: [Select]

            case state is
                when idle =>
                    clk_count <= 0;
                    bit_count <= 0;
                    rx_dv <= '0';
                    if registerTwo = '0' then
                        next_state <= start_bit;
                    end if;

                when start_bit =>
                    if clk_count < (g_CLKS_PER_BIT / 2) - 1 then
                        clk_count <= clk_count + 1;
                    elsif registerTwo = '0' then
                        clk_count <= 0;
                        next_state <= data_bits;
                    end if;

Also because in that module your assignment of 'next_state' to state is in a separate clocked process, you may/will be stuck in a process for one cycle longer than you think it should be, causing weirdness to happen.

[hamster_nz]

I had some spare time, so made your source so it seems to simulate properly (read the XADC, respond to serial input, send the serial data).

The test bench (tb_main.vhd) sends the trigger character 0x74 every 10 ms at 115200 baud..

I didn't have your XADC IP block, so made one that was similar with the XADC primative. It is most likely all configured wrong - you should replace it with your IP block, or review all the configuration attributes.

The "design.txt" file is the XADC simulation data file, that is referenced in this attribute in my_adc.vhd:

Code: [Select]

      SIM_MONITOR_FILE => "C:/Users/Hamster/Desktop/design.txt"  -- Analog simulation data file name

Without this the XADC model doesn't work in simulation, and doesn't strobe the EOC or EOS signals. You will need to update the path should you run the simulation.

I also fixed up the reset to async assert, sync release - the preferred for FPGAs.

I didn't properly fix the synchronizer on the serial input - it's still a little bit wrong (but works)

Please feel free to ask questions!