Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fpga] Add flow and constraints to dump the flash info array cell sites #25363

Closed
wants to merge 2 commits into from

Conversation

a-will
Copy link
Contributor

@a-will a-will commented Nov 23, 2024

Adjust MMI dump to use a regex for mem type. Some memories in the FPGA can consist of both RAMB36 and RAMB18 types, depending on the width of the array. Use a regex to allow more flexible memory type detection.

Add KEEP_HIERARCHY to the prim_ram_1p instances representing the flash info arrays. This helps keep intelligible hierarchical paths to the memories, so we can readily select the correct cells. Because Vivado was previously combining all info types of the same bank into the same BRAMs, this increases BRAM utilization a bit.

Add calls to dump an MMI file for each array represented by a (bank,type) tuple, and add them to the bitstream cache entry. Collect all MMI data into a single file, and update the manifest and bazel rules to use it. Instead of having separate files, each map has a separate Processor element, and the InstData is assigned the key that identifies the memory (instead of using "dummy").

@a-will
Copy link
Contributor Author

a-will commented Nov 23, 2024

CC @moidx

Still a WIP, and I haven't tested splicing any particular image. We might be able to create a more targeted Tcl proc that could aggregate arrays (e.g. concatenate the two banks for each type into a linearized address space).

Also, it looks like the bitstream cache handling will need some updating. There are two approaches I can think of:

  1. Specify required properties in a schema for each top.
  2. Ignore problems with old bitstream cache entries, and do a two-phase integration.
    1. Add the bitstream and manifest generation, and get entries in the bitstream cache that have the new MMI files.
    2. Add bazel rules that consume the new MMI files after the bitstream cache is populated with the first entry.

@matutem
Copy link
Contributor

matutem commented Nov 24, 2024

Assuming the non-initialized pages remain in their current state (which I would assume is all 1's), is it possible to just initialize a portion of memory? Apparently we only need to deal with a handful of pages.

@a-will
Copy link
Contributor Author

a-will commented Nov 24, 2024

Assuming the non-initialized pages remain in their current state (which I would assume is all 1's), is it possible to just initialize a portion of memory? Apparently we only need to deal with a handful of pages.

I'm not sure what you mean here. The BRAM gets completely initialized when the bitstream is programmed, so we are always choosing the values. Right now, that is all ones, yes, representing erased flash cells. There is no partial loading at configuration time (i.e. when the bitstream is programmed into the FPGA).

It's best to have the spliced image cover the entire memory, and program anything "unspecified" as all ones. Also, it's important to keep in mind that integrity is checked at the 64-bit datum granularity.

@a-will a-will force-pushed the flash-info-mmi branch 2 times, most recently from 6f66b14 to f0f8695 Compare November 27, 2024 23:29
@a-will
Copy link
Contributor Author

a-will commented Nov 27, 2024

Now I have collected all the memory maps into a single MMI file and adjusted all the cache handling and splicing scripts.

@a-will a-will force-pushed the flash-info-mmi branch 4 times, most recently from 27412a3 to 9f7c29e Compare November 28, 2024 06:30
Reduce BUILD file boilerplate by placing all memory map info in a single
file. Use a separate Processor element for each map, so the InstData
attribute can be used as a key to identify the memory to be updated.
This construction also permits multiple memories to be spliced in a
single call to updatemem, though that is not implemented in this commit.

Adjust MMI dump to use a regex for mem type and a dictionary for each
memory's parameters. For the mem type, some memories in the FPGA can
consist of both RAMB36 and RAMB18 types, depending on the width of the
array. The regex allows more flexible memory cell detection.

Rev up the bitstream manifest schema to v3 so the memories contained in
the MMI file are described. Add a function to collect MMI data from v2
cache entries and rewrite in the expected v3 format, with the MMI data
in a single file.

Finally, drop support for bitstream cache entries that do not contain a
manifest. The tools to create a manifest have been in the repo for
awhile, and as of this commit's creation, no such cache entries remain
in the public cloud storage bucket.

Signed-off-by: Alexander Williams <[email protected]>
For earlgrey, export the flash info memory maps to the MMI file. Add
knowledge of these maps existing to the bitstream cache entries. The
flash info arrays are identified by a key in this format:

    flash<FlashBank>_info<InfoType>

Add KEEP_HIERARCHY to the prim_ram_1p instances representing the flash
info arrays. This helps keep intelligible hierarchical paths to the
memories, so we can readily select the correct cells.

This lays groundwork for a future PR where we can splice these memories.

Signed-off-by: Alexander Williams <[email protected]>
@a-will a-will marked this pull request as ready for review November 30, 2024 16:49
@a-will a-will requested review from moidx and cfrantz and removed request for rswarbrick, cfrantz and mundaym November 30, 2024 16:49
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I ought to use these example files as test vectors, instead of making something else up inside the self-test Python script. But I'd punt that change for now ;)

Comment on lines +13 to +16
"memories": [
"otp",
"rom"
]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This information isn't currently used in the bazel rules (not part of bitstreams_workspace.py), but I figured I'd make this info readily available in case it's useful.

The same info could be obtained from the MMI file directly, so if reviewers prefer I remove it from the manifest, I certainly can.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think it is useful to know. In the future it would be interesting for us to consider creating a Bazel target that splices all memories in a single Bazel invocation.

@a-will a-will requested review from matutem and jwnrt December 2, 2024 22:03
Copy link
Contributor

@moidx moidx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @a-will, these changes are very useful! Some nice refactorings as well.

dict set memInfo otp fake_word_width 0
dict set memInfo otp addr_end_multiplier 16

# The flash banks have 76-bit wide words. 64 bits are data, and 12 bits are metadata.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will updatemem support 76-bit wide words per line? It seems like it should, but I am curious to know if you have spotted any quirks when looking at the synthesis results.

Copy link
Contributor Author

@a-will a-will Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should, but I don't currently have an image that lets me check. Perhaps next we should cobble together something to test! :)

It at least doesn't complain about the presence of those memories in the MMI file.

Comment on lines +13 to +16
"memories": [
"otp",
"rom"
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think it is useful to know. In the future it would be interesting for us to consider creating a Bazel target that splices all memories in a single Bazel invocation.

Copy link
Contributor

@matutem matutem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alex, this seems to improve the MMI flow in addition to adding support for flash image uploading. I would only request you to make sure the docs about this area are adequate to navigate it for folks that need to work with images. This may be tricky since you are very familiar with it all, but things may look different if you pretend you don't :)

LGTM

@a-will
Copy link
Contributor Author

a-will commented Dec 18, 2024

Superseded by #25533

@a-will a-will closed this Dec 18, 2024
@a-will a-will deleted the flash-info-mmi branch December 18, 2024 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants