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

[prim_ram_1p] Ram tiling for sram_ctrl and prim_ram_1p #25625

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Razer6
Copy link
Member

@Razer6 Razer6 commented Dec 12, 2024

No description provided.

@Razer6 Razer6 force-pushed the ram-cfg-tile branch 4 times, most recently from 474c469 to e0ea37d Compare December 12, 2024 14:23
@Razer6 Razer6 requested a review from msfschaffner as a code owner December 12, 2024 14:23
@Razer6 Razer6 requested a review from andreaskurth December 12, 2024 14:24
@@ -158,6 +162,8 @@ module top_earlgrey #(
output lc_ctrl_pkg::lc_tx_t ast_lc_dft_en_o,
input ast_pkg::ast_obs_ctrl_t obs_ctrl_i,
input prim_ram_1p_pkg::ram_1p_cfg_t ram_1p_cfg_i,
input prim_ram_1p_pkg::ram_1p_cfg_t [SramCtrlMainNumRamInst-1:0] sram_ctrl_main_cfg_i,
input prim_ram_1p_pkg::ram_1p_cfg_t [SramCtrlMainNumRamInst-1:0] sram_ctrl_ret_aon_cfg_i,
Copy link
Member Author

Choose a reason for hiding this comment

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

That seems to be a bug in the generator.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #25665

@a-will
Copy link
Contributor

a-will commented Dec 12, 2024

This makes for interesting questions for prims in general:

  • Are we able to concisely describe all the properties needed to instantiate specific primitives that might fan out from the simple generic prim abstraction?
    • This particular PR has a parameter for tiling arrays of a provided depth to reach the full depth of the generic prim.
    • Another PR seeks to specify the tile instance widths.
    • Would another user possibly want to have instance-level control of aspect ratios (which map to different primitives from the tech lib / memory compiler)?
  • If yes, then we can carry on adding these one-by-one.
  • If no, does it make sense to provide a top-specific, prim library-specific package that carries the configurations that can be looked up?
    • For instance level control, we could have a single ID parameter that can be exposed at the top level, and that could be used by prim libraries to define arbitrary configuration functions, which are defined for specific tops and consumed in the prim library modules.

I'm not sure which route is going to be better, but it might be worth thinking about and discussing.

@Razer6
Copy link
Member Author

Razer6 commented Dec 12, 2024

This is a good question. One reason for this parameter to be an instance level rather than a global package was in general to implement tiling one level higher even. I do not see a reason why tiling should be implemented in the tech prim rather than in the generic primitive. This would make backdoor loading much easier as it supports tiling as a first class citizen, e.g., when there is no tiling, there is only one loop iteration of the tiling loop.

@andreaskurth
Copy link
Contributor

Thx @Razer6 and @a-will for proposing this change and raising relevant questions. Also looping in @matutem -- it would be great to get your perspective also on DV aspects.

Other questions that come to my mind:

  • This PR propagates the new InstDepth parameter to the prim_ram_1p_scr instance in sram_ctrl, but what about other instances, also of prim_ram_1p_adv (e.g., in usbdev or in i2c)?
  • How do we deal with DFT I/Os, which may depend on the tiling? Do we generally pass 2D arrays (for 2D tiling) of arrays/structs of signals through the design to all (possibly) tiled instances? Or should a prim library package define a generic DFT struct that integrators can override according to their needs?

@Razer6
Copy link
Member Author

Razer6 commented Dec 13, 2024

  • This PR propagates the new InstDepth parameter to the prim_ram_1p_scr instance in sram_ctrl, but what about other instances, also of prim_ram_1p_adv (e.g., in usbdev or in i2c)?

That's totally doable, though I wanted to start with sram_ctrl's memory first, as tiling might be used there first (bigger memory).

  • How do we deal with DFT I/Os, which may depend on the tiling? Do we generally pass 2D arrays (for 2D tiling) of arrays/structs of signals through the design to all (possibly) tiled instances? Or should a prim library package define a generic DFT struct that integrators can override according to their needs?

In general, I am raising the question of whether 2D tiling should be implemented in prim_ram_1p_adv. That way, the backdoor loading could be much more constant across different integrators. In general, I am a fan of passing arrays rather than a common type, which would need to be the common denominator supporting all possible tiling configs of the system, which leaves a lot of unused signals...

@Razer6
Copy link
Member Author

Razer6 commented Dec 14, 2024

I think the tiling information must be propagated via the parameter information as its dimensions are instance-dependent. Different instances have different dimensions, yielding different wide DFT signals. This cannot be determined at instance level via a a function.

local: "false"
expose: "true"
},
{ name: "NumRamInst",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why using this parameter if it can be derived from others? Seems redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically no need for that. Unfortunately, the topgen tooling cannot yet deal with the computed number of RAM instances. I'll file an issue on that, for a later improvement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could this just be a localparam in the individual module holding the RAMs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately no. This parameter is used as a dimension for the DFT signals, which depends on the number of RAM instances.

@matutem
Copy link
Contributor

matutem commented Dec 18, 2024

I think @a-will comment about other configs is regarding FPGA tiling? Also @Razer6 case is similar to the foundry concerns. In any case I think both cases are important and orthogonal, as they suggest tiling along rows or columns. As for the DV concerns, they probably mostly concern mem_bkdr_util, scrambling, and the such. My understanding is that those utils need to access individual arrays, so we may need to create multiple arrays in mem_bkdr per high level RAM using the configuration; that way the interface exposed by mem_bkdr for each high-level memory would be simpler.

I assume row tiling will use tile 0 for the lowest addresses. Column tiling has to decide on endian-ness... fun :)

The FPGA flows don't use mem_bkdr_util, and we currently splice the memory contents into the bitstream, yet I assume we need to scramble and generate ECC for them. Also, perhaps backdoor RAM access is possible for other FPGA vendors?

Can we try to zoom into config in terms of rows and columns and then tackle mem_bkdr in one shot. WDYT?

@a-will
Copy link
Contributor

a-will commented Dec 18, 2024

I think @a-will comment about other configs is regarding FPGA tiling? Also @Razer6 case is similar to the foundry concerns. In any case I think both cases are important and orthogonal, as they suggest tiling along rows or columns.

We have aligned here. We would seek to model tiling with parameters along both dimensions. This affects the number of RAM macro instances, which affects the size of the tech lib-specific ports in the port list. Thus, we need the parameter to flow from the top level, so it's available to the entire hierarchy for those ports. 🙂

I assume row tiling will use tile 0 for the lowest addresses. Column tiling has to decide on endian-ness... fun :)

No, no endianness ambiguity, please. The bit indices and the macro instance indices must always grow in the same direction. Endianness only affects how an image represented as a sequence of bytes gets laid out, and that is an orthogonal issue to tiling.

The FPGA flows don't use mem_bkdr_util, and we currently splice the memory contents into the bitstream, yet I assume we need to scramble and generate ECC for them. Also, perhaps backdoor RAM access is possible for other FPGA vendors?

Backdoor access is generally only available if you build the logic yourself, hehe. At run time, that can get complicated, but it's interesting.

For splicing specifically, yes, ECC and scrambling and such must be accommodated in the image construction. We have the utilities for this for flash_ctrl already, it turns out.

@Razer6 Razer6 force-pushed the ram-cfg-tile branch 4 times, most recently from f146b84 to 9cf4b18 Compare December 21, 2024 08:59
@Razer6 Razer6 force-pushed the ram-cfg-tile branch 17 times, most recently from 44473e6 to 625e839 Compare December 23, 2024 11:11
Signed-off-by: Robert Schilling <[email protected]>

Signed-off-by: Robert Schilling <[email protected]>
@andreaskurth
Copy link
Contributor

CHANGE AUTHORIZED: hw/ip/otbn/rtl/otbn.sv
CHANGE AUTHORIZED: hw/ip/prim/rtl/prim_ram_1p_adv.sv
CHANGE AUTHORIZED: hw/ip/prim/rtl/prim_ram_1p_scr.sv
CHANGE AUTHORIZED: hw/ip/prim_generic/rtl/prim_generic_ram_1p.sv
CHANGE AUTHORIZED: hw/ip/sram_ctrl/data/sram_ctrl.hjson
CHANGE AUTHORIZED: hw/ip/sram_ctrl/rtl/sram_ctrl.sv
CHANGE AUTHORIZED: hw/top_earlgrey/data/top_earlgrey.hjson
CHANGE AUTHORIZED: hw/top_earlgrey/rtl/autogen/chip_earlgrey_asic.sv
CHANGE AUTHORIZED: hw/top_earlgrey/rtl/autogen/top_earlgrey.sv

This PR adds depth tiling parameters for prim_ram_1p. No functional change, and for Earlgrey the instances remain effectively untiled.

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.

4 participants