TinyTapeout / tt-support-tools

tools used by project repos to test configuration, generate OpenLane run summaries and documentation
Apache License 2.0
14 stars 15 forks source link

No warning if additional clocks aren't configured #34

Closed MichaelBell closed 5 months ago

MichaelBell commented 5 months ago

It's not uncommon for people to think that they can clock registers from generated clocks or different input pins, without adding the required configuration to the SDC file.

Currently, no obvious warnings are flagged if you do this, but it is likely your design won't work due to hold violations (setup violations might also be an issue, but would generally not be design breaking as you can just clock more slowly). This is because STA doesn't understand the clocks you are using so doesn't know to check the paths between different registers in your addition clock domains.

I think this problem will be picked up by there being "unclocked" registers in the STA report. So it might be a good idea to grep for Warning: There are \d* unclocked register/latch pins. in multi_corner_sta.checks.rpt and flag a warning on the action summary if this is present.

urish commented 5 months ago

Thanks! Any example of such project?

MichaelBell commented 5 months ago

An example is https://github.com/KennethWilke/tt06-cdc-fifo/actions/runs/8564552127

KennethWilke commented 5 months ago

I'm not sure if this is a concern for my project. If it is, then I'll have a next topic of study! :smile:

The read_clock and write_clock inputs to my design seem pretty similar to something like an I2C SCL input with a maximum speed of "good luck, why don't you try to find out!". Would an I2C clock input have the same concern?

urish commented 5 months ago

The issue with hold violations is that slowing down the clock doesn't help (AFAIK, only varying the supply voltage or temperature can help after the fact).

Check out this great video from @mattvenn, it does a good job of explaining hold violations: https://www.youtube.com/watch?v=5PRuPVIjEcs

KennethWilke commented 5 months ago

Will take a look at that video, and I'd gladly take any suggested improvements to my design. I'm hoping to revise it 1-2 times if I can improve test coverage and/or reliability.

I expect my FIFO should have similar challenges as https://github.com/cmu-stuco-98154/f22-tt02-jrecta/tree/main from TT02 as they aim to implement roughly the same thing

urish commented 5 months ago

I think it might make sense to add this to our "--print-warnings" that we already have in tt-support-tools:

https://github.com/TinyTapeout/tt-support-tools/blob/1ebe1ef29b33a27406877eaf3f15fd1b79f594ed/project.py#L695-L710

WDYT @MichaelBell ?

MichaelBell commented 5 months ago

Yes, that looks like the place to add the check. I don't see any warnings in that multi_corner_sta.checks.rpt on a good run, so should probably just flag any warnings in that file, as is done for synthesis.

urish commented 5 months ago

So grep for "Warning: " ?

urish commented 5 months ago

I've tested the updated script on tt06-factory-test, but I'm getting a warning:

===========================================================================
check_setup -verbose -unconstrained_endpoints -multiple_clock -no_clock -no_input_delay -loops -generated_clocks
===========================================================================
Warning: There is 1 unconstrained endpoint.
  _085_/D

Here is the relevant excerpt from the GL netlist:

  sky130_fd_sc_hd__conb_1 _094_ (
    .HI(_034_)
  );

  sky130_fd_sc_hd__dfrtp_2 _085_ (
    .CLK(clk),
    .D(_034_),
    .Q(rst_n_i),
    .RESET_B(rst_n)
  );

and relevant RTL:

    reg rst_n_i;
    reg [7:0] cnt;

    always @(posedge clk or negedge rst_n)
        if (~rst_n)
            rst_n_i <= 1'b0;
        else
            rst_n_i <= 1'b1;

Do you see any potential issue here?

MichaelBell commented 5 months ago

Ah, I think think that's because of the async reset. That's pretty normal so shouldn't be flagged - maybe restrict to startswith "Warning:" and contains "clock"?

urish commented 5 months ago

Something along these lines?

        sta_checks = "runs/wokwi/reports/signoff/30-sta-rcx_nom/multi_corner_sta.checks.rpt"
        with open(os.path.join(self.local_dir, sta_checks)) as f:
            for line in f.readlines():
                if line.startswith("Warning:") and 'clock' in line:
                    warnings.append(line.strip())

Also, any idea where we would find this report in the outputs of OpenLane 2 run?

here's an example of such recent run: https://github.com/TinyTapeout/tt06-dffram-example/actions/runs/8591971841

MichaelBell commented 5 months ago

Looks good.

In OL2 it looks like this is it: runs\wokwi\52-openroad-stapostpnr\nom_tt_025C_1v80\checks.rpt

urish commented 5 months ago

Thanks! so now we have the warnings enabled. Also tested it:

image