hold timing violation on module fed by 2 synchronous clocks

[connoisseur_de_mimi]

Hi,

I've created a verilog module for a SPI peripheral that is fed 2 clocks, both generated by the same xilinx clocking wizard block. The main clock is a 80MHz clock which feeds the internal logic and determines SPI clk. A secondary clock of 1 MHz is used as a sample clock (that is, triggering an action of the SPI peripheral). in the screenshot below the names are sys_clk and sample_clk, respectively

the module:

module DAC80501 (
    input clk,
    input update,
    input rst_n,
    //...
    );
	
    reg update_last = 1'b1;
    
    always @ (posedge clk or negedge rst_n) begin
        if (!rst_n) begin
            //...
        end
        else begin            
            case (state)  
                STATE_IDLE: begin                    
                    if (!update & update_last) begin
                        //...
                    end
                end
                //...
            endcase
        
            update_last <= update;
        end
    end
    
    DAC80501SPI DAC (din_q, update_dac, clk, ready_dac, sclk, mosi, cs);
endmodule

this results in a hold time violation which I can't figure out how to fix:

 

Edited January 16 by connoisseur_de_mimi

[artvvb]

Hi @connoisseur_de_mimi

Asynchronous resets are also generally not recommended by Xilinx: https://docs.xilinx.com/r/en-US/ug949-vivado-design-methodology/Synchronous-Reset-vs.-Asynchronous-Reset. It seems that hold violations could be a result of this: https://support.xilinx.com/s/question/0D52E00006hpk6eSAA/hold-time-violation-on-asynchronous-reset. If that's the cause, synchronizing rst_n to the clock and removing the negedge rst_n from the sensitivity list could potentially fix it.

You can also potentially find more information in the implementation reports. For example, report clock interaction could tell you if there are unsafe clock domain crossings:

Thanks,

Arthur

[connoisseur_de_mimi]

Hi @artvvb,

I have the same issue with synchronous reset, I actually introduced the async reset to try to fix the issue. Thanks for the links, I have gone back to synchronous resets.

The clock interaction report shows the clocks are safely timed (according to https://docs.xilinx.com/r/2022.2-English/ug945-vivado-using-constraints-tutorial/Step-6-Clock-Interaction-Report )

The schematic is pretty straigt forward. This is one of the failing nets

sample_clk is passed to various places in the schematic. the only failing nets are those where sample_clk is connected to the D input of a register (like the circled ones below)

the design works in hardware (eclypse-z7 with custom pods), but I have yet to confirm that no sampling jitter is introduced by this (I can live with the register being updated one clock cycle later as long as it is consistent).

[connoisseur_de_mimi]

did a little more digging. I'm not sure if this is a red herring. I have another module that is very similar to the one shown above, but this module does not cause a hold time violation. Schematic view in the implementation shows that the non-violating module is synthesized to a LUT followed by a register, where the violating module (DAC80501_ch1) is synthesized to a register only:

(update and trigger are synonymous in this case)

 

the code that synthesizes to a register only:

always @ (posedge clk) begin
    if (!rst_n) begin
        //...
    end
    else begin            
        case (state)  
            STATE_IDLE: begin
                if (!update & update_last) begin
                    din_q <= {8'h08, din};
                    update_q <= 1'b0;
                    state <= STATE_WAIT_TRANSMISSION;
                end
                //...
            end
        end
    
    update_last <= update;
    ref_pwdwn_edge <= {ref_pwdwn_edge[0], ref_pwdwn};
    ready_dac_edge <= {ready_dac_edge[0], ready_dac};
end

 

the code that synthesizes to a LUT followed by a register:

always @ (posedge clk) begin
    if (!rst) begin
        //...
    end
    else begin    
        case (state)
            STATE_IDLE: begin
                if (!trigger & trigger_last) begin
                    //falling edge on trigger
                    convst_q <= 1'b0;
                    
                    state <= STATE_WAIT_IRQ;
                end
                //...
        endcase
                        
        irq_last <= irq;
        trigger_last <= trigger;        
        ready_last <= ready;
    end
end

 

[zygot]

First of all the post by artvvb is a good one and deserves your attention and spending some time doing a bit of research on HDL resets for Vivado synthesis.

I notice that clk_out5 hs a requested frequency of 10 MHz and the wizard gave you 1 MHz. I suggest resolving this first.

Resets, both synchronous and asynchronous varieties can be tricky. In general I suggest avoiding edge event resets in your logic.

If you want to use an asynchronous reset then you could try this:
always @ (posedge clk or rst_n) begin
if (!rst_n) begin
//...
end
else begin
You are probable better off creating a srst_n signal that is synchronous to the clk clock domain.
always @ (posedge clk) begin
if (!srst_n) begin
//...
end
else begin
Understand that, depending on the settings for synthesis, the tools might infer some functionality and replace you code with it's optimized version. This might not be what you want.

VHDL and Verilog are great. Unfortunately, they don't address a lot of what modern FPGA synthesis and simulation tools require. But you can write your code in a way that's consistent and unambiguous.

it's tempting to use a signal's edge event as an input to a process to do something; and you don't want that something to repeat unintentionally. The common way to do this is to create a one-clock wide strobe that occurs relative to some signal external to your process. Say that you have a signal that toggles at a 1 MHz rate; that is 500 ns low and 500ns high. You want to use rising edge of this signal as an 'event' to cause some action in your process. You can create a one clock wide strobe near the rising edge this way:
- create a delayed version of your input signal using clk
- create a strobe when s is high and s_r1 is low.
- use the strobe instead of s as an input to your process. Edited January 18 by zygot

[connoisseur_de_mimi]

Hi @zygot,

10 hours ago, zygot said:

First of all the post by artvvb is a good one and deserves your attention and spending some time doing a bit of research on HDL resets for Vivado synthesis.

I agree, I didn't know that async resets should be released synchronously, for example. I learned some things ^^

10 hours ago, zygot said:

I notice that clk_out5 hs a requested frequency of 10 MHz and the wizard gave you 1 MHz. I suggest resolving this first.

This is not an error. clk_out 5 is cascaded with clk_out7 to get down do 1 MHz (note the option is named after MMCM Port, not Clock Wizard Port).

primary clock is 100 MHz, so 100 * 48 / 5 = 960 MHz, div by 96 = 10 MHz, div by 10 = 1 MHz for clk_out5. I don't know why this is not displayed correctly.

10 hours ago, zygot said:

Understand that, depending on the settings for synthesis, the tools might infer some functionality and replace you code with it's optimized version. This might not be what you want.

VHDL and Verilog are great. Unfortunately, they don't address a lot of what modern FPGA synthesis and simulation tools require. But you can write your code in a way that's consistent and unambiguous.

Can you elaborate more on that or provide resources?

10 hours ago, zygot said:

it's tempting to use a signal's edge event as an input to a process to do something; and you don't want that something to repeat unintentionally. The common way to do this is to create a one-clock wide strobe that occurs relative to some signal external to your process. Say that you have a signal that toggles at a 1 MHz rate; that is 500 ns low and 500ns high. You want to use rising edge of this signal as an 'event' to cause some action in your process. You can create a one clock wide strobe near the rising edge this way:
- create a delayed version of your input signal using clk
- create a strobe when s is high and s_r1 is low.
- use the strobe instead of s as an input to your process.

This is what I am trying to do with

if (!trigger & trigger_last) begin

I'll try moving this out of the module and see if it fixes the issue

Edited January 19 by connoisseur_de_mimi

[zygot]

In earlier versions of the tools, ISE and early versions of Vivado, you could look at Settings/Synthesis, and Settings/Implementation to see a list of strategies and all of the settings for the tools preset strategies. In recent versions of Vivado, the list of setting is limited and replaced by an empty line with the nebulous name of Other....

For synthesis there will always be some optimization of how the tool understands your HDL text. It will also infer known structures like counters, state machines, etc, and replace your code with optimal instructions when it can infer them; as long as you enable these features.

The wonderful thing about VHDL and Verilog is that the designer has free reign to implement anything, using good, bad or indifferent coding. And that's the bad thing about the HDL flow; what you intend isn't necessarily what the tool infers from your code. To make matters worse, each vendors synthesis and implementation tool expects a coding style that isn't always in concert with another vendor's tools.

VHDL and Verilog just haven't keep up with modern FPGA architectures and features, so each vendor has their own way of completing the job. For instance there's no arest or sreset keyword in VHDL or Verilog; the subject just isn't addressed. HDL's don't know about single-ended or differential logic or buffers; Each tool has it's own way of specifying logic types. All vendors allow the designer to put some synthesis and implementation constraints into their HDL sources using PARAM or attributes, but there is no consistency between vendors tools or even an attmept at completeness for any vendor. it's hit or miss. I could go on and on, but hopefully you get the idea.

It's important to read you vendor's reference manuals. For Xilinx UG901, UG903, UG904, UG893 are required reading plus a lot of others. Don't download some random version. Use the Document Navigator for tool version specific versions of these documents.

Figuring out basic HDL stuctures and concepts is just the beginning of the HDL design flow. VHDL and Verilog have slightly different concepts to grasp, and there are subtleties that aren't at all obvious for a particular design expression. Basically, I view HDL source code as a kind of dance between the designer and the tools with the object of creating one functional behavior of the completed design netlist.

BTW, I like the idea of restructuring your code if you aren't happy with the results of the current version. It's part of the dance. For what you are trying to do, you might consider putting that functionality into you state machine. Letting events in process sensitivity lists, or simple boolean expressions do all of the heavy lifting is tempting but not always a good strategy. Sometimes you need to do more of the work for yourself.

Also, you are using a lot of the clocking resources. Relying on so many derived clocks spanning so wide of a frequency range is probably not the ideal strategy. You can put less burden on the tools by minimizing the number of clock domains and using logic structures like counters, strobes, etc to structure the timing of your design. As an added benefit this will force you to think about how signals in one of those clock domains might cause problems in a different clock domain. Edited January 19 by zygot

[connoisseur_de_mimi]

thanks for the explaination!

Quote

To make matters worse, each vendors synthesis and implementation tool expects a coding style that isn't always in concert with another vendor's tools.

seeing how different FPGAs of different vendors are, I am not surprised. But I did not expect things to be this dire

 

On 1/19/2024 at 4:03 PM, zygot said:

Also, you are using a lot of the clocking resources. Relying on so many derived clocks spanning so wide of a frequency range is probably not the ideal strategy.

that was necessary to enable clock cascading. its not possible to enable just clk_out_0, clk_out_5 and clk_out_7. to enable clk_out_n, all clk_out_[0..n-1] must be enabled.

I was able to fix the timing violation by replacing sample_clk with a binary counter. my system clock is an integer multiple of the sample clock, so this change was dead simple. implementation now meets timing.