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 prim features and tool flow to dump the flash info array cell sites #25533

Merged
merged 3 commits into from
Dec 16, 2024

Conversation

a-will
Copy link
Contributor

@a-will a-will commented Dec 7, 2024

Collect MMI data into one file.

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.

Add specialized ram_1p prim for Xilinx FPGAs

Add a specialized ram_1p prim for the prim_xilinx library. This prim adds some prim_xilinx-specific configuration ability to enable selecting particular layouts for embedded memories. The intention is to have the top-level specify the configuration via a special prim_xilinx_pkg, which is a virtual core. The prim instance will do a lookup to decide how to split the memories.

This feature is intended for use with memories that are too wide to fit in updatemem's capabilities (where splicing is required). The flash info pages are one such type, since updatemem can only handle up to 64-bit wide items, and the info page array is 76 bits wide. Synthesis chooses the most efficient layout of the memories, but the layout is awkward for handling splices.

Force earlgrey's flash info data and metadata into separate arrays for the FPGA.

Dump flash info maps to the MMI file.

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>_<data|intg>`

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

This was tested by hand by manually splicing the uart_smoketest binary into flash bank 0, info partition 0 and running a slightly modified //sw/device/tests:flash_ctrl_info_access_lc_prod_personalized_fpga_cw310_sival (to prevent overwriting the data). Without the integrity bits in, the flash control read would get operation errors. With the integrity bits also spliced in, the reads would succeed, and we'd get comparison failures showing data from the MEM file.

This PR likely will supersede #25363

Note: The API for the prim_xilinx_pkg is known to be a terrible interface, but it's left this way to avoid alternatives that might change RTL for the ASIC's tree in earlgrey_1.0.0. An example of a more refined approach that changes that RTL might be to expose a top-level parameter for every major memory, so an ID-based lookup could be used to control prim library-specific configuration. That would yield instance-level control as needed.

Partially addresses #21225 and #23038

@a-will a-will force-pushed the flash-info-mmi-splitmem branch from 067f883 to 054b79c Compare December 7, 2024 01:56
@a-will
Copy link
Contributor Author

a-will commented Dec 7, 2024

If you were following along #25363 before, only the last commit is new.

@a-will
Copy link
Contributor Author

a-will commented Dec 7, 2024

The SV was bugged anyway, but something seems a little off with our dvsim tool flow. prim_xilinx_ultrascale RTL files shouldn't be part of the file list for any of its top modules' build graphs.

Edit: Never mind. I realized with the way primgen works, it needs to bring in those files. This will be improved with our work with virtual cores to replace primgen!

@a-will a-will force-pushed the flash-info-mmi-splitmem branch 5 times, most recently from b7e19f4 to 7b87af7 Compare December 7, 2024 06:34
@a-will a-will marked this pull request as ready for review December 7, 2024 06:34
@a-will a-will requested review from moidx, cfrantz, andreaskurth and HU90m and removed request for rswarbrick, cfrantz and mundaym December 7, 2024 06:34
@a-will
Copy link
Contributor Author

a-will commented Dec 7, 2024

An alternative would've been to leave the memory as-is and require the bitstream cache to explicitly represent partitioning of memories (in practice, of splice-capable memories that are wider than 64 bits), then have tools that break images into the partitions and call updatemem on each of the partitioned MEM files.

If that is preferred for reviewers but the approach in this PR isn't too objectionable, I would ask that we target that for the next round and let this one into the repo. We can handle a change to the bitstream cache schema, as is done here, and there is work that continues to be blocked while we lack some way to fix up the flash info page state.

@a-will
Copy link
Contributor Author

a-will commented Dec 8, 2024

The failing CW340 CI flows appear to be an ongoing infrastructure failure, not a result of this PR.

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.

Thanks @a-will, great job simplifying the memory mapping infrastructure for updatemem.

@andreaskurth Can you or someone on your end take a look at the FPGA prim changes included in this PR?

Thanks

@moidx
Copy link
Contributor

moidx commented Dec 10, 2024

CHANGE AUTHORIZED: hw/ip/prim_xilinx/rtl/prim_xilinx_pkg.sv
CHANGE AUTHORIZED: hw/ip/prim_xilinx/rtl/prim_xilinx_ram_1p.sv
CHANGE AUTHORIZED: hw/ip/prim_xilinx_ultrascale/rtl/prim_xilinx_ultrascale_ram_1p.sv

Copy link
Contributor

@andreaskurth andreaskurth left a comment

Choose a reason for hiding this comment

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

This looks great to me overall, thanks @a-will! One question and a few suggestions from my side

doc/contributing/fpga/debugging_with_ila.md Outdated Show resolved Hide resolved
Comment on lines +8 to +9
virtual:
- lowrisc:prim_xilinx:prim_xilinx_pkg
Copy link
Contributor

Choose a reason for hiding this comment

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

This requires FuseSoC v2.4 to work properly, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we use virtual cores with my fusesoc fix already. I added the satsolver instructions to our fork at the same time as upstream.

If we use upstream fusesoc, then 2.4 is the first release that includes my commits.

hw/top_earlgrey/util/vivado_hook_write_bitstream_pre.tcl Outdated Show resolved Hide resolved
hw/top_earlgrey/util/vivado_hook_write_bitstream_pre.tcl Outdated Show resolved Hide resolved
Comment on lines 9 to 10
set_property KEEP_HIERARCHY TRUE [get_cells "top_earlgrey/u_flash_ctrl/u_eflash/u_flash/gen_generic.u_impl_generic/gen_prim_flash_banks[0].u_prim_flash_bank/gen_info_types[0].u_info_mem"]
set_property KEEP_HIERARCHY TRUE [get_cells "top_earlgrey/u_flash_ctrl/u_eflash/u_flash/gen_generic.u_impl_generic/gen_prim_flash_banks[0].u_prim_flash_bank/gen_info_types[1].u_info_mem"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Another opportunity to deduplicate (probably not even using eval)?

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 actually gets added and removed again because of the changing ideas. If you'd like, I can go back in the history and get rid of its appearance altogether.

@andreaskurth
Copy link
Contributor

CHANGE AUTHORIZED: hw/ip/prim_xilinx/rtl/prim_xilinx_pkg.sv
CHANGE AUTHORIZED: hw/ip/prim_xilinx/rtl/prim_xilinx_ram_1p.sv
CHANGE AUTHORIZED: hw/ip/prim_xilinx_ultrascale/rtl/prim_xilinx_ultrascale_ram_1p.sv

These are new Xilinx-specific prims, which should be functionally equivalent to the existing implementation from an I/O perspective.

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]>
Add a specialized ram_1p prim for the prim_xilinx library. This prim
adds some prim_xilinx-specific parameters to enable selecting particular
layouts for embedded memories. The intention is to have the top-level
override the parameters with hierarchical assignment.

This feature is intended for use with memories that are too wide to fit
in updatemem's capabilities (where splicing is required). The flash info
pages are one such type, since updatemem can only handle up to 64-bit
wide items, and the info page array is 76 bits wide. Synthesis chooses
the most efficient layout of the memories, but the layout is awkward for
handling splices.

Force earlgrey's flash info data and metadata into separate arrays for
the FPGA.

Signed-off-by: Alexander Williams <[email protected]>
@a-will a-will force-pushed the flash-info-mmi-splitmem branch from 7b87af7 to 9776162 Compare December 14, 2024 06:37
@a-will
Copy link
Contributor Author

a-will commented Dec 14, 2024

The AscentLint job is failing, but the report is all green.

@jwnrt jwnrt merged commit 56f2e95 into lowRISC:master Dec 16, 2024
38 checks passed
@a-will a-will deleted the flash-info-mmi-splitmem branch December 16, 2024 15:17
@a-will a-will added the CherryPick:earlgrey_1.0.0 This PR should be cherry-picked to earlgrey_1.0.0 label Dec 16, 2024
Copy link

Backport failed for earlgrey_1.0.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin earlgrey_1.0.0
git worktree add -d .worktree/backport-25533-to-earlgrey_1.0.0 origin/earlgrey_1.0.0
cd .worktree/backport-25533-to-earlgrey_1.0.0
git switch --create backport-25533-to-earlgrey_1.0.0
git cherry-pick -x 9e1ef80bca3d6648b1091efd1e5e7b3776a220f2 bf595b6f694043433ea71064735cc8088cf6b3b5 9776162eb91b785d2dabc01021dda00e015edd96

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CherryPick:earlgrey_1.0.0 This PR should be cherry-picked to earlgrey_1.0.0 Manually CherryPick This PR should be manually cherry picked.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants