platforms: asap7: Fixed set/reset signals for DFFASRHQNx1_ASAP7_75t_R

[jbylicki]

The definitions for DFFASRHQNx1_ASAP7_75t_R appear to be invalid. When they get a reset signal, they will have each bit of their initial state invalid. This Pull Request aims to correct this problem.

Steps to reproduce:

• Prerequisites:

  • Verilator: minimum required version 5.036 (tested on commit cfbcfd913c9c). I've confirmed the issue on an industry-standard, proprietary simulator and it displays the same behavior.
  • env varialbe $ORFS: path to base directory of OpenROAD-flow-scripts
  • an assumption is made that all required files are within $PWD

• Files used for the design:

  • countdown.sv, countdown_tb.sv, config.mk and constraint.sdc are included to this PR as a .zip archive. config.mk requires paths to be updated based on individual configuration.

• Steps:

  • Synthesize the design using ORFS: make -C $ORFS/flow DESIGN_CONFIG=$(pwd)/config.mk clean_all synth
  • Run verilation - it will create an additional directory $PWD/verilator:

  verilator --timing --binary --trace -Wno-fatal -Mdir ./verilator -j 12 --build-jobs 12 --prefix countdown \
    $ORFS/flow/results/asap7/countdown/base/1_synth.v \
    countdown_tb.sv \
    $ORFS/flow/platforms/asap7/verilog/stdcell/asap7sc7p5t_AO_RVT_TT_201020.v \
    $ORFS/flow/platforms/asap7/verilog/stdcell/asap7sc7p5t_INVBUF_RVT_TT_201020.v \
    $ORFS/flow/platforms/asap7/verilog/stdcell/asap7sc7p5t_SEQ_RVT_TT_220101.v \
    $ORFS/flow/platforms/asap7/verilog/stdcell/asap7sc7p5t_SIMPLE_RVT_TT_201020.v \
    $ORFS/flow/platforms/asap7/verilog/stdcell/empty.v \
    $ORFS/flow/platforms/asap7/work_around_yosys/asap7sc7p5t_OA_RVT_TT_201020.v
  

  • Simulate the verilated design: ./verilator/countdown

• Output pre-change:

- V e r i l a t i o n   R e p o r t: Verilator 5.036 2025-04-27 rev v5.036
- Verilator: Built from 2.364 MB sources in 227 modules, into 0.079 MB in 11 C++ files needing 0.000 MB
- Verilator: Walltime 1.849 s (elab=0.012, cvt=0.008, bld=1.729); cpu 0.123 s on 12 threads; alloced 55.938 MB
[0] val = 111
[500] val = 010
[15000] val = 001
[25000] val = 000
- countdown_tb.sv:18: Verilog $finish
- S i m u l a t i o n   R e p o r t: Verilator 5.036 2025-04-27

• Output post-change:

  - V e r i l a t i o n   R e p o r t: Verilator 5.036 2025-04-27 rev v5.036
  - Verilator: Built from 2.364 MB sources in 227 modules, into 0.079 MB in 11 C++ files needing 0.000 MB
  - Verilator: Walltime 0.275 s (elab=0.013, cvt=0.008, bld=0.156); cpu 0.123 s on 12 threads; alloced 55.930 MB
  [0] val = 111
  [500] val = 101
  [15000] val = 100
  [25000] val = 011
  [35000] val = 010
  [45000] val = 001
  [55000] val = 000
  - countdown_tb.sv:18: Verilog $finish
  - S i m u l a t i o n   R e p o r t: Verilator 5.036 2025-04-27
  

Please note that at time interval 500, when reset is asserted, the original state's val bits are flipped, as countdown.sv defines the reset state to be 3'b101

Relevant files:
asap7-pr-files.zip

[QuantamHD]

@maliberty this is an important fix. Mind taking a look when you can

[maliberty]

Please make a similar PR for https://github.com/The-OpenROAD-Project/asap7sc7p5t_28

[povik]

@jbylicki @QuantamHD How do we know the Liberty model is to blame? Looking at Yosys code I think this is likely a bug in interpretation of the model when a flop has both async set/clear signals and an inverted data input.

[jbylicki]

We analyzed the generated code emitted after synthesis and:

• In the case where the bit should be reset to 1, RESETN was connected to rst via a buffer, while SETN was tied to a 1, effectively setting the flip-flop output to 0 upon a reset,
• Otherwise, the SETN and RESETN signals were reversed, effectively setting the flip-flop output to 1 upon a reset.

Otherwise, the synthesis correctly mapped the implemented logic. Only RESETN and SETN were flipped. If you look at the library changes from this PR, you can see that the clear input , which is supposed to set the output of the gate to 0, was connected to the SETN signal instead of RESETN and preset, which sets the gate's output to 1, was connected to RESETN.

[povik]

I think you're saying that you expect RESETN to set the output to 0 when activated. When the output is named QN (Q negated) the library designers could have meant for RESETN to set QN to 1.

[maliberty]

The more certain method is to look at the CDL netlist and determine the functionality from the transistors.

[povik]

CDL notwithstanding we may have introduced a mismatch between the Liberty and Verilog models. Looking at DFFASRHQNx1_ASAP7_75t_R:

module DFFASRHQNx1_ASAP7_75t_R (QN, D, RESETN, SETN, CLK);
	// ...
	not (int_fwire_s, delayed_RESETN);
	altos_dff_sr_0 (int_fwire_IQN, notifier, delayed_CLK, int_fwire_d, int_fwire_s, int_fwire_r, xcr_0);
	buf (QN, int_fwire_IQN);
	// ...
endmodule

When RESETN is low, if delayed_RESETN is also low (that part is hard to parse), int_fwire_s is high, and by the table inside altos_dff_sr_0 that seems to set QN to high.

primitive altos_dff_sr_0 (q, v, clk, d, s, r, xcr);
	output q;
	reg q;
	input v, clk, d, s, r, xcr;

	table
	//	v,  clk, d, s, r : q' : q;

		*  ?   ?   ?   ?   ? : ? : x;
		?  ?   ?   ?   1   ? : ? : 0;
		?  ?   ?   1   0   ? : ? : 1;
		?  b   ? (1?)  0   ? : 1 : -;
		?  x   1 (1?)  0   ? : 1 : -;
		?  ?   ? (10)  0   ? : ? : -;
		?  ?   ? (x0)  0   ? : ? : -;
		?  ?   ? (0x)  0   ? : 1 : -;
		?  b   ?  0   (1?) ? : 0 : -;
		?  x   0  0   (1?) ? : 0 : -;
		?  ?   ?  0   (10) ? : ? : -;
		?  ?   ?  0   (x0) ? : ? : -;
		?  ?   ?  0   (0x) ? : 0 : -;
		? (x1) 0  0    ?   0 : ? : 0;
		? (x1) 1  ?    0   0 : ? : 1;
		? (x1) 0  0    ?   1 : 0 : 0;
		? (x1) 1  ?    0   1 : 1 : 1;
		? (x1) ?  ?    0   x : ? : -;
		? (x1) ?  0    ?   x : ? : -;
		? (1x) 0  0    ?   ? : 0 : -;
		? (1x) 1  ?    0   ? : 1 : -;
		? (x0) 0  0    ?   ? : ? : -;
		? (x0) 1  ?    0   ? : ? : -;
		? (x0) ?  0    0   x : ? : -;
		? (0x) 0  0    ?   ? : 0 : -;
		? (0x) 1  ?    0   ? : 1 : -;
		? (01) 0  0    ?   ? : ? : 0;
		? (01) 1  ?    0   ? : ? : 1;
		? (10) ?  0    ?   ? : ? : -;
		? (10) ?  ?    0   ? : ? : -;
		?  b   *  0    ?   ? : ? : -;
		?  b   *  ?    0   ? : ? : -;
		?  ?   ?  ?    ?   * : ? : -;
	endtable
endprimitive

[maliberty]

I traced out the CDL and I see: (incorrect see below)

[maliberty]

I see RESETn is an active-low reset and SETn is an active-low preset. I see it as non-inverting from D to Q so

    pin (QN) {
      direction : output;
      function : "IQN";
...
    ff (IQN,IQNN) {
      clocked_on : "CLK";
      next_state : "!D";
    }

does not seem correct. I think QN should have function be IQNN. The name itself seems strange for a non-inverting gate.

[povik]

The truth table from the datasheet:

[maliberty]

Unless I've made some mistake in examining the CDL that doesn't seem to be true.

[maliberty]

The names make more sense if that is the functionality....

[maliberty]

I did have a bug in my drawing. I got the schematic from Prof. Clark and the final inverter comes off SH not SS (which I verified is in the CDL). It appears the FF is inverting as specified.

[maliberty]

I believe the changes to the Liberty here are correct. I haven't traced the Verilog simulation models. @povik do they need updating too?

[povik]

I believe the Liberty changes are incorrect and the schematic agrees with the truth table above (active SETN sets QN to zero and active RESETN sets QN to one)

[maliberty]

@povik I agree this is wrong. I'll revert unless @QuantamHD or @jbylicki want to argue their side.

[QuantamHD]

It just needs to be correct. I have no opinion.

[povik]

I think the actual issue is inside Yosys near this line https://github.com/YosysHQ/yosys/blob/e0747b71b7308d1a1ef72506a932908ea06329b6/passes/techmap/dfflibmap.cc#L317 The code is confused whether cell_next_pol signifies input inversion or output inversion (the two differ when async set/reset is involved)

[maliberty]

@povik does it make sense to file a yosys issue then?

[povik]

Yes, the details above should be helpful to whoever fixes it