LUTRAM write occurs on rising edge regardless of IS_WCLK_INVERTED property

[hansemro]

Issue Description

Issue discovered in #20 (comment)

When IS_WCLK_INVERTED property is asserted, we expect the write to LUTRAM to occur on the falling edge. However, this does not happen as the placer and fasm writer ignores the property for LUTRAMs, and, therefore, treats as if all writes should occur on rising edge. This may lead to functional errors in designs that require writes on negative edge.

Steps to Reproduce and Test

To distinguish between a write occuring on a positive edge and negative edge of clock, I created a simple test with two LUTRAMs sensitive to different edges of clock. Output of posedge LUTRAM and negedge LUTRAM drives led_o[0] and led_o[1], respectively. A single BSCANE2 primitive will be used to manually drive WCLK+DI+WE pins of the LUTRAM via JTAG.

1. Clone primitive-tests repo fork with xc7-lutram-negedge-write-demo branch

git clone https://github.com/hansemro/primitive-tests.git -b xc7-lutram-negedge-write-demo

2. Navigate to primitive-tests/lutram-tests/negedge-write-test and build the test with the openXC7 toolchain:

Note

If targeting an FPGA board other than SQRL Acorn CLE215+, then provide an XDC file with
3 active-low LEDs mapped to led_o[2:0] and update BOARD/FAMILY/PART/JTAG_CABLE/XDC
target parameters in the Makefile.

$ cd primitive-tests/lutram-tests/negedge-write-test
$ nix develop github:openXC7/toolchain-nix
[nix(openXC7)] make

3. Check that CLKINV bit is not set anywhere inside top.fasm (a sign that the test will fail). Proceed with the following steps to test.

4. Load the bitstream onto the FPGA:

make program

5. With LUTRAM initialized to 0, write a 1 on positive edge with the following openocd command:

Note

Use an appropriate interface adapter script (usually located in /usr/share/openocd/scripts/interface/).

Digilent HS2 support is provided with digilent-hs2.cfg in case interface/ftdi/digilent-hs2.cfg does not work.

Note

setup.cfg defines a function set_lutram <we> <data> <clk> to manually set LUTRAM input pins.

To avoid setup/hold violations, we should not change clk and we/data together.

openocd -f interface/ADAPTER.cfg -f setup.cfg \
    -c "set_lutram 0 0 0" \
    -c "set_lutram 0 0 0" \
    -c "set_lutram 1 1 0" \
    -c "set_lutram 1 1 1" \
    -c "shutdown"

If the design works correctly, only the LED (led_o[0]) assigned to the posedge LUTRAM should be lit after a positive-edge write. If LEDs led_o[1:0] light up together, then the design was incorrectly implemented. (LED at led_o[2] should dim on positive-edge write since the outputs should differ).

Anyhow, to perform a write on the negative edge, we would run the following afterwards:

openocd -f interface/ADAPTER.cfg -f setup.cfg \
    -c "set_lutram 1 1 0"
    -c "shutdown"

6. (Optional) Compare positive-edge (and negative-edge) write behavior against bitstream built with Vivado bitstream:

make vivadoclean top.vivado.bit
make BITSTREAM=top.vivado.bit program
# initial positive-edge write
openocd -f interface/ADAPTER.cfg -f setup.cfg \
    -c "set_lutram 0 0 0" \
    -c "set_lutram 0 0 0" \
    -c "set_lutram 1 1 0" \
    -c "set_lutram 1 1 1" \
    -c "shutdown"
# negative-edge write
openocd -f interface/ADAPTER.cfg -f setup.cfg \
    -c "set_lutram 1 1 0"
    -c "shutdown"

Design implemented with Vivado should have the two LUTRAM perform writes at different edges of the clock.

Other Considerations

LUTRAM and FFs share clock from CLKINV routing BEL, so all synchronous elements within the SLICE site must have the same IS_*CLK_INVERTED property.

[hansfbaier]

Do you know how to disassemble a bitstream with bit2fasm?

[hansemro]

Do you know how to disassemble a bitstream with bit2fasm?

Yes, here:

bit2fasm --db-root ${PRJXRAY_DB_DIR}/${FAMILY}/ --part ${PART} top.bit > top.fasm

[hansfbaier]

Not sure if I am comparing the right two parts here, but the CLKINV/NOCLKINV bits probably make the difference

left is openXC7, right is vivado

[hansfbaier]

See write_luts_config and write_ffs_config in fasm.cc

[hansfbaier]

It might be a case for LUTRAMs is missing here:

[hansemro]

@hansfbaier Yes, LUTRAM is currently not being handled in the fasm writer. Since LUTRAMs and FFs share the CLKINV BEL, I am thinking that the CLKINV bit should not be set inside write_ffs_config but inside a new function that handles both LUTRAMs and FFs. This new function would also be able to do some final DRC to check that the clock inversion property value matches for all used elements within the SLICE (half tile).

[hansemro]

There is a rule in Arch::xc7_logic_tile_valid to check that the FFs have the same inversion property:

nextpnr-xilinx/xilinx/arch_place.cc

Lines 557 to 558 in 30ba8fb

	if (ff->ffInfo.is_clkinv != clkinv)
	return false;

We may want to define a new rule to compare LUTRAM's clkinv and FF's clkinv.

[lehaifeng000]

good, I would join this work next week

[hansemro]

@lehaifeng000 @hansfbaier

Sounds good. I am still in the process of figuring out how to fully test whether the CLB placement rules work to catch problematic physical implementations. I suppose this is a good time to ask if anyone can reliably place LUTRAMs and FFs in the same CLB slice since I am failing to do this task with openxc7 flow.

[hansfbaier]

@hansemro How do you try to place them? xdc file, LOC annotations or other means?

[lehaifeng000]

In the place phase, is there any rule to avoid ff with different clkinv inside a slice?

[hansemro]

@hansemro How do you try to place them? xdc file, LOC annotations or other means?

This may be an impossible task for the moment since we do not yet support manual BEL placement yet with LOC and BEL attributes. I was looking into setting CONSTR_{X,Y,Z} attributes in the post-pack json, but I am not entirely sure this is where we should be looking. Sorry for giving a rather loaded task; I don't know enough about nextpnr to give proper instructions.

[lehaifeng000]

@hansemro How do you try to place them? xdc file, LOC annotations or other means?
@hansfbaier @hansemro
After reviewing the code for "place", I believe a potential optimization could be implemented by modifying the "overused" condition within the "CutSpreader" function.

[hansfbaier]

@hansemro The primitives-fixes branch contains changes which seem to fix this issue.
Can you confirm that? What do your tests say?