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

[hw,prim,ram_1/2p] Add DFT response channel #25654

Merged
merged 5 commits into from
Dec 24, 2024

Conversation

Razer6
Copy link
Member

@Razer6 Razer6 commented Dec 14, 2024

The DFT logic of the underlying RAM macros might have a response channel. This PR adds a new package definition for the prim_ram_1p and prim_ram_2p type of RAM and routes the signal up to the top-level. For the generic implementation of prim_ram_1p, this DFT signal only contains a logic signal that is tied to 0.

In Earlgrey, the DFT outputs are currently left open. In Darjeeling, they are connected to the top-level interface.

Regarding Ibex:
Ibex also uses RAM instances. There we have a chicken-egg problem. We first need to introduce the definition to OT, then update Ibex, and then re-vendor and add the port to rv_core_ibex. At Ibex level, multi-tiled caches are not yet feeding their output signals to the top level.

Edit: Actually, I think there is an interlocked dependency, which cannot be solved alas having one repo CI broken for a short time?

@Razer6 Razer6 force-pushed the dft-output-channel branch 5 times, most recently from 99fb686 to 33d3543 Compare December 14, 2024 16:54
@Razer6 Razer6 requested a review from a team as a code owner December 14, 2024 16:54
@Razer6 Razer6 requested review from rswarbrick and removed request for a team December 14, 2024 16:54
@Razer6 Razer6 force-pushed the dft-output-channel branch 11 times, most recently from 4295be8 to 764cfcb Compare December 15, 2024 15:01
@Razer6 Razer6 changed the title [hw,prim,ram_1p] Add DFT response channel [hw,prim,ram_1/2p] Add DFT response channel Dec 15, 2024
@Razer6 Razer6 added the CI:Rerun Rerun failed CI jobs label Dec 15, 2024
@github-actions github-actions bot removed the CI:Rerun Rerun failed CI jobs label Dec 15, 2024
@Razer6 Razer6 force-pushed the dft-output-channel branch from 764cfcb to 3dd7974 Compare December 15, 2024 18:06
@a-will
Copy link
Contributor

a-will commented Dec 15, 2024

Hehe, here's the ports variant! This PR raises the question (again?), "How should we handle tech lib-specific ports that aren't part of the generic prim abstraction?". #25533 and #25625 raise the question , "How should we handle tech lib-specific parameters that aren't part of the generic prim abstraction?"

(Of course, #25625 also asks whether we should implement tiling above the generic prim abstraction layer, which would allow it to avoid answering the question about the layer below, but it is roped in because the other PR tiles memories via a totally different dimension.)

For parameters, we have a hodgepodge of stuff, especially parameters that are only useful to specific families of FPGAs. Do we just keep stuffing parameters at the generic abstraction layer and give them inert defaults, then possibly expose them all at the top level?

For ports, there is technically the option of connecting across hierarchies (unlike parameters, since defparam is on the way out), so the ports could be buried and not be part of the port list at the top_<topname> level. We could also choose to have a virtual core that implements the package defining library-specific ports at the generic abstraction layer, then wire those up to the top.

In any case, here it looks like we've got a prim_ram_1p_pkg that isn't a virtual core, so how do integrators tune prim_ram_1p_pkg::cfg_t for their specific technology? Does that struct's contents just happen to be enough for all the partners working on OT? (similar question for the response being introduced here)

@Razer6 Razer6 force-pushed the dft-output-channel branch from 3dd7974 to 11a0600 Compare December 16, 2024 05:45
@Razer6
Copy link
Member Author

Razer6 commented Dec 16, 2024

For ports, there is technically the option of connecting across hierarchies (unlike parameters, since defparam is on the way out), so the ports could be buried and not be part of the port list at the top_<topname> level. We could also choose to have a virtual core that implements the package defining library-specific ports at the generic abstraction layer, then wire those up to the top.

In any case, here it looks like we've got a prim_ram_1p_pkg that isn't a virtual core, so how do integrators tune prim_ram_1p_pkg::cfg_t for their specific technology? Does that struct's contents just happen to be enough for all the partners working on OT? (similar question for the response being introduced here)

While I agree that you might be able to connect those ports manually through tooling, I don't like it as it adds much more burden. Making this connection across different uncore environments, testbenches, etc. It's just another way of introducing bugs rather than creating it explicitly via the port definitions.

Hehe, a virtual core package definition is exactly what I aim for. That will be implemented as soon as the proper tooling for that is available. Partners can then overwrite the request and response structure to their needs and not need to mess with any workarounds.

For parameters, we have a hodgepodge of stuff, especially parameters that are only useful to specific families of FPGAs. Do we just keep stuffing parameters at the generic abstraction layer and give them inert defaults, then possibly expose them all at the top level?

I think we need to differentiate a little bit on what tiling means. Does tiling change the hierarchy, i.e., do we create different multiple instances of prim? Or does it affect the internal organization of a prim instance? For the former, I would argue that it can or maybe should be implemented as part of the non-tech prim. The second is something different. An ASIC ram primitive also might now be necessarily organized as Nx32 RAMs. But that is something I think that should be internal, as it is abstracted away from OT.

@Razer6 Razer6 force-pushed the dft-output-channel branch 2 times, most recently from 254dc26 to 00be38f Compare December 16, 2024 06:06
@Razer6 Razer6 force-pushed the dft-output-channel branch from 00be38f to c0bb7a4 Compare December 16, 2024 06:13
hw/ip/i2c/rtl/i2c_core.sv Outdated Show resolved Hide resolved
hw/ip/prim_generic/rtl/prim_generic_ram_1p.sv Outdated Show resolved Hide resolved
hw/vendor/lowrisc_ibex/rtl/ibex_top.sv Outdated Show resolved Hide resolved
@Razer6 Razer6 force-pushed the dft-output-channel branch 5 times, most recently from 0f26128 to cce4140 Compare December 23, 2024 08:48
@Razer6 Razer6 force-pushed the dft-output-channel branch from cce4140 to d8274c2 Compare December 23, 2024 09:02
CI uses *.vbl to build a common waiver for the whole design and
misses top-level waivers due to a different file extension

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

CHANGE AUTHORIZED: hw/ip/i2c/data/i2c.hjson
CHANGE AUTHORIZED: hw/ip/i2c/rtl/i2c.sv
CHANGE AUTHORIZED: hw/ip/i2c/rtl/i2c_core.sv
CHANGE AUTHORIZED: hw/ip/i2c/rtl/i2c_fifos.sv
CHANGE AUTHORIZED: hw/ip/otbn/data/otbn.hjson
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/rtl/prim_ram_1r1w_adv.sv
CHANGE AUTHORIZED: hw/ip/prim/rtl/prim_ram_1r1w_async_adv.sv
CHANGE AUTHORIZED: hw/ip/prim/rtl/prim_ram_2p_async_adv.sv
CHANGE AUTHORIZED: hw/ip/prim_generic/rtl/prim_generic_flash_bank.sv
CHANGE AUTHORIZED: hw/ip/prim_generic/rtl/prim_generic_otp.sv
CHANGE AUTHORIZED: hw/ip/prim_generic/rtl/prim_generic_ram_1p.sv
CHANGE AUTHORIZED: hw/ip/prim_generic/rtl/prim_generic_ram_1r1w.sv
CHANGE AUTHORIZED: hw/ip/prim_generic/rtl/prim_generic_ram_2p.sv
CHANGE AUTHORIZED: hw/ip/prim_generic/rtl/prim_ram_1p_pkg.sv
CHANGE AUTHORIZED: hw/ip/prim_generic/rtl/prim_ram_2p_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
CHANGE AUTHORIZED: hw/ip/rv_core_ibex/data/rv_core_ibex.hjson
CHANGE AUTHORIZED: hw/ip/rv_core_ibex/rtl/rv_core_ibex.sv
CHANGE AUTHORIZED: hw/ip/spi_device/data/spi_device.hjson
CHANGE AUTHORIZED: hw/ip/spi_device/rtl/spi_device.sv
CHANGE AUTHORIZED: hw/ip/spi_device/rtl/spid_dpram.sv
CHANGE AUTHORIZED: hw/ip/sram_ctrl/data/sram_ctrl.hjson
CHANGE AUTHORIZED: hw/ip/sram_ctrl/rtl/sram_ctrl.sv
CHANGE AUTHORIZED: hw/ip/usbdev/data/usbdev.hjson
CHANGE AUTHORIZED: hw/ip/usbdev/rtl/usbdev.sv
CHANGE AUTHORIZED: hw/top_earlgrey/data/top_earlgrey.hjson
CHANGE AUTHORIZED: hw/top_earlgrey/rtl/autogen/top_earlgrey.sv
CHANGE AUTHORIZED: hw/vendor/lowrisc_ibex/rtl/ibex_top.sv

This PR adds DFT response signals to prims/macros. No functional change.

@a-will
Copy link
Contributor

a-will commented Dec 23, 2024

CHANGE AUTHORIZED: hw/ip/i2c/data/i2c.hjson
CHANGE AUTHORIZED: hw/ip/i2c/rtl/i2c.sv
CHANGE AUTHORIZED: hw/ip/i2c/rtl/i2c_core.sv
CHANGE AUTHORIZED: hw/ip/i2c/rtl/i2c_fifos.sv
CHANGE AUTHORIZED: hw/ip/otbn/data/otbn.hjson
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/rtl/prim_ram_1r1w_adv.sv
CHANGE AUTHORIZED: hw/ip/prim/rtl/prim_ram_1r1w_async_adv.sv
CHANGE AUTHORIZED: hw/ip/prim/rtl/prim_ram_2p_async_adv.sv
CHANGE AUTHORIZED: hw/ip/prim_generic/rtl/prim_generic_flash_bank.sv
CHANGE AUTHORIZED: hw/ip/prim_generic/rtl/prim_generic_otp.sv
CHANGE AUTHORIZED: hw/ip/prim_generic/rtl/prim_generic_ram_1p.sv
CHANGE AUTHORIZED: hw/ip/prim_generic/rtl/prim_generic_ram_1r1w.sv
CHANGE AUTHORIZED: hw/ip/prim_generic/rtl/prim_generic_ram_2p.sv
CHANGE AUTHORIZED: hw/ip/prim_generic/rtl/prim_ram_1p_pkg.sv
CHANGE AUTHORIZED: hw/ip/prim_generic/rtl/prim_ram_2p_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
CHANGE AUTHORIZED: hw/ip/rv_core_ibex/data/rv_core_ibex.hjson
CHANGE AUTHORIZED: hw/ip/rv_core_ibex/rtl/rv_core_ibex.sv
CHANGE AUTHORIZED: hw/ip/spi_device/data/spi_device.hjson
CHANGE AUTHORIZED: hw/ip/spi_device/rtl/spi_device.sv
CHANGE AUTHORIZED: hw/ip/spi_device/rtl/spid_dpram.sv
CHANGE AUTHORIZED: hw/ip/sram_ctrl/data/sram_ctrl.hjson
CHANGE AUTHORIZED: hw/ip/sram_ctrl/rtl/sram_ctrl.sv
CHANGE AUTHORIZED: hw/ip/usbdev/data/usbdev.hjson
CHANGE AUTHORIZED: hw/ip/usbdev/rtl/usbdev.sv
CHANGE AUTHORIZED: hw/top_earlgrey/data/top_earlgrey.hjson
CHANGE AUTHORIZED: hw/top_earlgrey/rtl/autogen/top_earlgrey.sv
CHANGE AUTHORIZED: hw/vendor/lowrisc_ibex/rtl/ibex_top.sv

@andreaskurth andreaskurth merged commit a18b3dd into lowRISC:master Dec 24, 2024
38 checks passed
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