Fix OTP bitstream splicing #15163

dmcardle · 2022-09-27T21:05:46Z

A series of miracles occurred on Friday, enabling me to deduce how updatemem interprets its input and how Vivado lays out data in the BRAMs. With these changes, I can splice OTP values into a bitstream and observe the result in a GDB test! (Now we can disable ROM execution for the SRAM GDB test and drop the flaky tag!)

I think that OTP splicing never worked. To prevent regressions, I think I'll write an "OTP dumper" functest. We can instantiate the functest a few times with a variety of OTP-spliced bitstreams and assert that it prints the expected values.

Fixes #15162
Fixes #15174

a-will

Nice work! I put in some nits about generalizing scripts, but that probably doesn't have to be in this PR.

a-will · 2022-10-17T16:41:31Z

hw/bitstream/BUILD

@@ -102,6 +91,24 @@ filegroup(
    }),
 )

+[
+    filegroup(


Can you add a "manual" tag to these filegroups? That way, Vivado-dependent targets won't get picked up by wildcards.

Done. I added to the other filegroups as well. I think that this hasn't been an issue because these targets aren't tests.

hw/ip/rom_ctrl/util/gen_vivado_mem_image.py

a-will · 2022-10-17T16:57:12Z

hw/top_earlgrey/util/vivado_hook_write_bitstream_pre.tcl

        # The scrambled Boot ROM is actually 39 bits wide but the updatemem tool segfaults
        # for slice sizes not divisible by 8.
-        if {[expr {($slice_end - $slice_begin + 1) < 8}]} {
+        if {$filename eq "rom.mmi" && [expr {($slice_end - $slice_begin + 1) < 8}]} {


nit: This hard-coded file name key is awkward. Is there an argument we can add that would specifically point to a different desired behavior for this process? (i.e. something that doesn't require knowing the function of the memory but instead describes how the collection of BRAMs should be processed)

At the top level, we'll have the top-specific stuff hard-coded, but this script will be more readily reused if we don't automatically push those all the way to the leaf processes.

Honestly, I don't understand why we're messing with slice_end for rom.mmi. This filename check was just meant to preserve the existing behavior for the ROM.

It kind of looks like we're lying to updatemem and saying that the bitlanes cover 40 bits when they actually cover 39. I wonder if this was because you can't write 39 bits with 10 hex digits. Maybe this special case for ROM would not be necessary if we wrote four 39-bit words at a time (39 hex digits)?

OK, I've generalized this a bit and it's no longer dependent on the filename. These changes are contained in the new commit, [fpga] Refactor quirks in MMI file generator. PTAL!

rules/splice.bzl

I discovered a few problems with the OTP-specific MMI files we have been generating prior to this commit. (1) The file's bitlanes were overlapping. For instance, the first bitlane claimed to cover bits 0-3 (inclusive) and the second one claimed to cover bits 1-4. This is expressly not allowed by Vivado's updatemem manual, so I knew something was wrong. By poking around the Vivado GUI, I realized that OTP bitlanes should each cover a single bit. I'm not sure whether that logic is correct or necessary for the ROM case, so I left it alone. (2) The MMI file claimed that BRAMS are the type "RAMB32" when the OTP BRAMs are actually RAMB18E1. It's not as simple as just saying "RAMB18" in the MMI file. Ultimately, I got it working after bumping the MMI's minor version from 0 to 1 and configuring an environment variable before calling updatemem. (3) The bitlane "Placement" attributes were all wrong because we were trying to trim the "RAMB36_" prefix from strings that actually began with "RAMB18_". (4) The AddressSpace and and DataWidth tags had inaccurate values. Apparently there is a significant amount of padding between OTP words in the OTP BRAMs. In fact, each word is followed by 15 zeroes. Separately, I also modified the TCL script to dump the OTP BRAMs' INIT_XX strings to a text file. Ultimately, examining these INIT_XX strings helped me crack the way that Vivado lays out data in the BRAMs (more detail in a later commit). However, examining the INIT_XX strings in the Vivado GUI required too much clicking; there are 64 INIT_XX strings per OTP BRAM, and there are 22 BRAMs. This is purely a debugging aid, but it only takes a few seconds to generate. Signed-off-by: Dan McArdle <dmcardle@google.com>

dmcardle

Ready for another look :)

dmcardle · 2022-10-18T14:55:04Z

hw/top_earlgrey/util/vivado_hook_write_bitstream_pre.tcl

        # The scrambled Boot ROM is actually 39 bits wide but the updatemem tool segfaults
        # for slice sizes not divisible by 8.
-        if {[expr {($slice_end - $slice_begin + 1) < 8}]} {
+        if {$filename eq "rom.mmi" && [expr {($slice_end - $slice_begin + 1) < 8}]} {


OK, I've generalized this a bit and it's no longer dependent on the filename. These changes are contained in the new commit, [fpga] Refactor quirks in MMI file generator. PTAL!

hw/ip/rom_ctrl/util/gen_vivado_mem_image.py

I noticed that a few unit test targets outside of //sw/... were not running on CI. This commit ensures that CI runs the following additional targets: * //hw/ip/rom_ctrl/util:gen_vivado_mem_image_test (after lowRISC#15163 merges) * //rules/scripts:bitstreams_workspace_test * //util:generate_compilation_db_test Signed-off-by: Dan McArdle <dmcardle@google.com>

Rather than enabling quirks based on filename, this commit adds parameters to the `generate_mmi` proc. This also simplifies the interface a bit and improves documentation. Signed-off-by: Dan McArdle <dmcardle@google.com>

This script reads MEM files that can be read by Verilog's $readmemh and produces a MEM file that `updatemem` will understand. It turns out that the transformation for ROM data was not right for OTP data due to differences in the underlying hardware. This caught me by surprise because updatemem was exiting successfully. By comparing //hw/ip/otp_ctrl/data:img_rma, which is passed directly to Vivado, with the INIT_XX strings that Vivado produces, I was able to determine the transformation. This commit also includes a few features that I used to bootstrap my understanding: (1) a function that reads INIT_XX strings from OTP BRAMs and produces their memory content and (2) a mocked out updatemem "simulator" that takes hex strings as input and produces the same INIT_XX lines that the real updatemem would. I used a modified version of the sram_program GDB test[0] to dump relevant sections of OTP memory and verify that the OTP values I modified can actually be read from OTP memory. [0]: //sw/device/examples/sram_program:sram_program_fpga_cw310 Signed-off-by: Dan McArdle <dmcardle@google.com>

The new OTP image sets CREATOR_SW_CFG_ROM_EXEC_EN to zero. This commit also adds the get_otp_images() macro to //rules/otp.bzl, which aims to keep OTP splicing targets in //hw/bitstream and //hw/bitstream/vivdao in sync with OTP image definitions. Signed-off-by: Dan McArdle <dmcardle@google.com>

Now that execution is disabled, OpenOCD connection should not be flaky anymore! This should fix lowRISC#15174. Signed-off-by: Dan McArdle <dmcardle@google.com>

dmcardle · 2022-10-18T20:42:42Z

Whoops, looks like I introduced a division-by-zero error in the CW305 build because there are no OTP cells. https://dev.azure.com/lowrisc/opentitan/_build/results?buildId=97760&view=logs&j=4bc87d6f-5c28-5789-95a2-5c084ae7a0b2&t=054949bd-ad52-58ee-5229-51b7a2cd10e0&l=17991

INFO: [1-1] Dumping MMI to rom.mmi
INFO: [1-4] MMI dumped to /azp/agent/_work/1/a/build-out/hw/synth-vivado/rom.mmi
WARNING: [Vivado 12-180] No cells matched 'get_cells -hierarchical -filter { PRIMITIVE_TYPE =~ BMEM.bram.* && NAME =~ *u_otp_ctrl*}'.
INFO: [2-1] Dumping MMI to otp.mmi
divide by zero
sourcing script /azp/agent/_work/1/a/build-out/hw/synth-vivado/vivado_hook_write_bitstream_pre.tcl failed

Just pushed a fix — returning early from the TCL proc when there are no BRAMs.

alphan

Thanks @dmcardle! Since this is such a substantial improvement over the level of OTP splicing support we have right now, which is none :), and since this will unblock several tests, let's get this merged as soon as CI is green. We can improve this incrementally as needed. Also, IIRC @milesdai was working on OTP overlays. We can look into simplifying the new OTP definitions once this is merged.

dmcardle · 2022-10-19T17:33:57Z

CI passed!

I noticed that a few unit test targets outside of //sw/... were not running on CI. This commit ensures that CI runs the following additional targets: * //hw/ip/rom_ctrl/util:gen_vivado_mem_image_test (after lowRISC#15163 merges) * //rules/scripts:bitstreams_workspace_test * //util:generate_compilation_db_test Signed-off-by: Dan McArdle <dmcardle@google.com>

I noticed that a few unit test targets outside of //sw/... were not running on CI. This commit ensures that CI runs the following additional targets: * //hw/ip/rom_ctrl/util:gen_vivado_mem_image_test (after #15163 merges) * //rules/scripts:bitstreams_workspace_test * //util:generate_compilation_db_test Signed-off-by: Dan McArdle <dmcardle@google.com>

dmcardle force-pushed the dmcardle/fix-otp-splice branch 8 times, most recently from bd05906 to fbdc515 Compare October 17, 2022 15:45

dmcardle requested review from alphan, a-will and andreaskurth October 17, 2022 15:54

dmcardle changed the title ~~WIP changing OTP splicing logic. See lowRISC/opentitan#15162~~ Fix OTP bitstream splicing Oct 17, 2022

dmcardle added Component:FPGA FPGA related issues SW:Build System labels Oct 17, 2022

dmcardle marked this pull request as ready for review October 17, 2022 15:58

dmcardle requested review from cfrantz and drewmacrae as code owners October 17, 2022 15:58

dmcardle force-pushed the dmcardle/fix-otp-splice branch from fbdc515 to cdc96f9 Compare October 17, 2022 16:06

dmcardle requested a review from jon-flatley October 17, 2022 16:06

a-will reviewed Oct 17, 2022

View reviewed changes

dmcardle force-pushed the dmcardle/fix-otp-splice branch from cdc96f9 to c2e4c31 Compare October 17, 2022 18:16

dmcardle mentioned this pull request Oct 18, 2022

[fpga] Generalize gen_vivado_mem_image.py #15552

Open

4 tasks

dmcardle force-pushed the dmcardle/fix-otp-splice branch from c2e4c31 to e842d02 Compare October 18, 2022 17:39

dmcardle commented Oct 18, 2022

View reviewed changes

dmcardle mentioned this pull request Oct 18, 2022

[ci] Run more on-host tests in sw_tests #15566

Merged

dmcardle added 2 commits October 18, 2022 16:41

[fpga] Refactor quirks in MMI file generator

387b0fc

Rather than enabling quirks based on filename, this commit adds parameters to the `generate_mmi` proc. This also simplifies the interface a bit and improves documentation. Signed-off-by: Dan McArdle <dmcardle@google.com>

dmcardle added 2 commits October 18, 2022 16:41

[sw] Disable ROM execution for sram_program_fpga_cw310_test

8f27d78

Now that execution is disabled, OpenOCD connection should not be flaky anymore! This should fix lowRISC#15174. Signed-off-by: Dan McArdle <dmcardle@google.com>

dmcardle force-pushed the dmcardle/fix-otp-splice branch from e842d02 to 8f27d78 Compare October 18, 2022 20:41

alphan approved these changes Oct 19, 2022

View reviewed changes

dmcardle added the Status:Ready to merge PR is ready to be merged by a committer. label Oct 19, 2022

drewmacrae merged commit f800b33 into lowRISC:master Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix OTP bitstream splicing #15163

Fix OTP bitstream splicing #15163

dmcardle commented Sep 27, 2022 •

edited

a-will left a comment

a-will Oct 17, 2022

dmcardle Oct 17, 2022

a-will Oct 17, 2022

dmcardle Oct 17, 2022

dmcardle Oct 17, 2022

dmcardle Oct 18, 2022

dmcardle left a comment

dmcardle Oct 18, 2022

dmcardle commented Oct 18, 2022

alphan left a comment

dmcardle commented Oct 19, 2022

Fix OTP bitstream splicing #15163

Fix OTP bitstream splicing #15163

Conversation

dmcardle commented Sep 27, 2022 • edited

a-will left a comment

Choose a reason for hiding this comment

a-will Oct 17, 2022

Choose a reason for hiding this comment

dmcardle Oct 17, 2022

Choose a reason for hiding this comment

a-will Oct 17, 2022

Choose a reason for hiding this comment

dmcardle Oct 17, 2022

Choose a reason for hiding this comment

dmcardle Oct 17, 2022

Choose a reason for hiding this comment

dmcardle Oct 18, 2022

Choose a reason for hiding this comment

dmcardle left a comment

Choose a reason for hiding this comment

dmcardle Oct 18, 2022

Choose a reason for hiding this comment

dmcardle commented Oct 18, 2022

alphan left a comment

Choose a reason for hiding this comment

dmcardle commented Oct 19, 2022

dmcardle commented Sep 27, 2022 •

edited