Combinatorial loop in ahb lite signals

[jlmahowa-amd]

After the June 19th merge into main (below commit) there is a combinatorial loop commit 67563371b8208a8e6720fa6c33456c37914d858d (HEAD -> main, origin/main, origin/HEAD) Merge: d334c84 133ac2c Author: bharatpillilli bharat.pillilli@gmail.com Date: Mon Jun 19 18:36:16 2023 -0700 Merge pull request #119 from chipsalliance/dev-integrate Merge dev-integrate to main

Loop is between u_sb_lsu_ahb_mux and ahb_lite_bus_i however the renaming of signals in the FPGA makes narrowing this down further a challenge. u_sb_lsu_ahb_mux/field_storage_reg[internal_iccm_lock][lock][value]_0 -> hresp_error_r_reg_0 -> ahb_lite_bus_i -> pending_hsel[16] -> responder_inst.hready0_in -> u_sb_lsu_ahb_mux

Warning thrown by Vivado: [DRC LUTLP-1] Combinatorial Loop Alert: 6 LUT cells form a combinatorial loop. This can create a race condition. Timing analysis may not be accurate. The preferred resolution is to modify the design to remove combinatorial logic loops. If the loop is known and understood, this DRC can be bypassed by acknowledging the condition and setting the following XDC constraint on any one of the nets in the loop: 'set_property ALLOW_COMBINATORIAL_LOOPS TRUE [get_nets <myHier/myNet>]'. One net in the loop is caliptra_fpga_project_bd_i/caliptra_package_top_0/inst/cptra_wrapper/caliptra_top_dut/ahb_lite_bus_i/u_ahb_lite_address_decoder/E[0]. Please evaluate your design. The cells in the loop are: caliptra_fpga_project_bd_i/caliptra_package_top_0/inst/cptra_wrapper/caliptra_top_dut/u_sb_lsu_ahb_mux/dout[0]_i_1__1048, caliptra_fpga_project_bd_i/caliptra_package_top_0/inst/cptra_wrapper/caliptra_top_dut/soc_ifc_top1/i_soc_ifc_reg/hinitiator_ready_default_i_5, caliptra_fpga_project_bd_i/caliptra_package_top_0/inst/cptra_wrapper/caliptra_top_dut/ahb_lite_bus_i/u_ahb_lite_address_decoder/pending_hsel[16]_i_1, caliptra_fpga_project_bd_i/caliptra_package_top_0/inst/cptra_wrapper/caliptra_top_dut/u_sb_lsu_ahb_mux/pending_hsel[16]_i_4, caliptra_fpga_project_bd_i/caliptra_package_top_0/inst/cptra_wrapper/caliptra_top_dut/soc_ifc_top1/i_soc_ifc_reg/pending_hsel[16]_i_7, and caliptra_fpga_project_bd_i/caliptra_package_top_0/inst/cptra_wrapper/caliptra_top_dut/u_sb_lsu_ahb_mux/pending_hsel[16]_i_8.

[jlmahowa-amd]

@bharatpillilli I have not been able to narrow this down much. Does your toolchain flag any issue related to this?

From the RTL the only thing I have found so far that has me questioning probably wouldn't have any impact. The instantiation of u_sb_lsu_ahb_mux: // Responder .hsel_o (initiator_inst.hsel ), [LM: hsel not part of Initiator_Interface_Ports] .haddr_o (initiator_inst.haddr ), .hwdata_o (initiator_inst.hwdata), .hwrite_o (initiator_inst.hwrite), .htrans_o (initiator_inst.htrans), .hsize_o (initiator_inst.hsize ), .hready_o (initiator_inst.hready), [LM: hready not part of Initiator_Interface_Ports] .hresp_i (initiator_inst.hresp ), .hreadyout_i (initiator_inst.hreadyout), .hrdata_i (initiator_inst.hrdata)

[bharatpillilli]

Is this on the latest main branch? tagging @Nitsirks @calebofearth as FYI

[bharatpillilli]

@kgugala @tmichalak - quick chat with Mike sounded like this showed up after the jtag sb path change that went in - can you please look into this?

[bharatpillilli]

@jlmahowa-amd - In addition to what you have observed, Mike ran a quick DC on the RTL after the jtag sb fix and we think there is also a potential timing path with that jtag fix.

Tagging @nstewart-amd to see if there were any recent timing runs to show this bug/issue.

[nstewart-amd]

Bharat - We haven't pulled down the code revision that seems to expose this issue. Will advise in our next iteration.

[Nitsirks]

The source of the timing loop is the 2:1 mux logic. We coded this to interface with the ROM, a single entity with no stall path. It was optimized to allow address phase and data phase overlap.

When put in front of the address decoder, there is now a timing loop. The ready signal was being used to determine the grant of a given phase, which could change which address is getting presented. The address is used to determine which ready signal is returning from the address decoder, creating a timing loop.

I've added a parameter which removes the optimization for this instance of the 2:1 mux, so that the timing loop is gone. I'm working on some assertions based on this parameter to ensure any assumptions about the optimized or non optimized path are holding.

[tmichalak]

@jlmahowa-amd can you point us to the testcase/target you're running with vivado? @Nitsirks, sounds like your changes will fix the problem, correct? Or should we also do some investigation on our end?

[nstewart-amd]

This should show up in LINT runs. We shouldn't need ASIC or FPGA netlist synthesis to identify combo loops.

[jlmahowa-amd]

@tmichalak no testcase, just building the RTL. @Nitsirks do you have an ETA on that fix merging into main?

[bharatpillilli]

Mike is out on vacation. I expect Tomasz from ant micro to fix the problem as it arose due to the JTAG SB fix. We can review the fix next week when Mike is back

[bharatpillilli]

@calebofearth corrected me that we have a pending PR with that combo loop path fix that Mike made but the PR cannot go through due to some other issues as the PR also has RDC etc fixes. We can wait for a week for @Nitsirks to come back.

[jlmahowa-amd]

On Verilator this combinatorial loop issue is flagged as an UNOPTFLAT warning. Unfortunately those warnings are suppressed by -Wno-UNOPTFLAT and so this can go un-noticed. Since there are other UNOPTFLAT warnings that aren't real combinatorial loops just seeing the warning isn't indicative of a problem.

[bharatpillilli]

https://github.com/chipsalliance/caliptra-rtl/pull/149/ sync to dev-msft should fix this