Hyperram #149

GregAC · 2024-08-09T14:09:03Z

No description provided.

Update code from upstream repository https://github.com/OVGN/OpenHBMC.git to revision fc1c154877d5ac5100d97368e6e656c96e0d6a70 Signed-off-by: Greg Chadwick <gac@lowrisc.org>

GregAC · 2024-08-09T14:14:37Z

HyperRAM is disabled by default but can be built by adding the --EnableHyperram flag to your fusesoc build, e.g.

fusesoc --cores-root=. run   --target=sim --tool=verilator --setup   --build lowrisc:sonata:system   --verilator_options="-j $NUM_CORES"   --make_options="-j $NUM_CORES" --EnableHyperram

For verilator or

fusesoc --cores-root=. run --target=synth --setup --build lowrisc:sonata:system --SRAMInitFile=$PWD/sw/cheri/build/tests/hyperram_test.vmem --EnableHyperram

For FPGA build (note this particular builds in the hyperram test for the SRAM contents)

With HyperRAM enabled you loose half of the SRAM. I think Ibex may lock-up if you try to access to the top 128k of SRAM as the crossbar memory map does not get adjusted in the HyperRAM configuration.

The plan is HyperRAM will be in the default system so this problem will go away. If we do end up with a HyperRAM and non HyperRAM configs with differing SRAM sizes we can set something up so the top 128k of SRAM gives a bus error in the HyperRAM config.

I think access latency is around 10-11 cycles in this setup. Some scope to improve there but I think I've done all the easy stuff (it was 17 cycles) and we'll be getting into the heart of the controller and how it captures data coming back from the HyperRAM. Worth doing at some point but not a priority for now.

I've got a block level testbench for the HyperRAM as I completely rewrote the top-level to have a native tilelink interface. I haven't shared that as it's a bit of a mess (in particular how you build it). I do want to add them to the repository though. It works using a BFM of the HyperRAM.

I've also got a full system simulation including HyperRAM BFM you can run under Xcelium but against this requires some clean-up before it's suitable to upstream.

GregAC · 2024-08-09T14:17:03Z

sw/cheri/tests/hyperram_test.cc

+const int RandTestBlockSize = 256;
+const int HyperramSize = (1024 * 1024) / 4;


Note this is intended as a soak test on the real board (i.e. leave it running and see if anything fails, hopefully good for flushing out reliability issues). It has an infinite loop that runs the same tests over and over.

You can run it on Verilator but I'd suggest adjusting these numbers so it runs far fewer tests per loop. E.g. change this to:

const int RandTestBlockSize = 16; const int HyperramSize = 16;

Once we have the baremetal testsuite in we can adapt this so we've got a soak mode and a terminating test mode.

GregAC · 2024-08-09T14:18:53Z

Oh and this allows execution from hyperram, though it will be slow until we sort out the icache.

marnovandermaas

Had a brief look at this and had one question. Not yet approving because I haven't had a chance to test this yet.

marnovandermaas · 2024-08-14T13:30:09Z

rtl/system/sram.sv

@@ -69,7 +69,7 @@ module sram #(
    .tl_o        (tl_a_o),

    // Control interface.
-    .en_ifetch_i (prim_mubi_pkg::MuBi4False),
+    .en_ifetch_i (prim_mubi_pkg::MuBi4True),


Why does this need to be true? The A-side of this SRAM model is connected to the LSU not the fetch stage.

Though the A side is connected to fetch when this SRAM is used in simulation as a hyperram stand in (where it receives transactions from both I and D side).

I could have just used the B side there but really I didn't see the point in disabling fetch in either port. This is a security feature used in earlgrey to lock-down SRAM execution. It isn't something we need in Sonata and it's surprising behaviour from an apparently generic 2 ported SRAM module to not accept fetches on one of the ports.

marnovandermaas · 2024-08-14T13:33:30Z

Also, there's a CI linting error:

ERROR: Setup failed : Cannot find lint/hyperram.vlt in :
>      ./rtl/ip/hyperram
>      ./rtl/ip/hyperram

HU90m

This looks good and is working on my FPGA; I'm hyped!

I couldn't test the simulation build because of the missing lint/hyperram.vlt. Once the sim is building, I'm happy for this to go in. Don't worry about fixing my comments; none of them are big enough to block this going in.

HU90m · 2024-08-15T11:57:03Z

rtl/system/sonata_system.sv

  localparam int unsigned SRAMAddrWidth = $clog2(MemSize);
+  localparam int unsigned HRSize        = 1024 * 1024; // 1 MiB


nit: unnecessary abbreviation

Suggested change

localparam int unsigned HRSize = 1024 * 1024; // 1 MiB

localparam int unsigned HyperramSize = 1024 * 1024; // 1 MiB

Also applies to HRClkFreq

HU90m · 2024-08-15T12:08:41Z

rtl/ip/hyperram/rtl/hbmc_tl_top.sv

+
+
+      (* IODELAY_GROUP = C_IODELAY_GROUP_ID *)
+      IDELAYCTRL


For those who also hadn't come across this before: https://www.xilinx.com/products/intellectual-property/util_idelay_ctrl.html

HU90m · 2024-08-15T12:16:34Z

sw/cheri/tests/hyperram_test.cc

+  hyperram_cap_area.bounds()  = HYPERRAM_BOUNDS;
+
+	while (true)
+	{


nit: mixing tabs and spaces. Don't worry about fixing; I'll set up an auto formatter this week.

HU90m · 2024-08-15T12:20:32Z

sw/cheri/tests/hyperram_test.cc

+  uart.address() = UART_ADDRESS;
+  uart.bounds()  = UART_BOUNDS;
+
+  uart->init(921600);


nit: buad rate defined in sw/common/defs.h

Suggested change

uart->init(921600);

uart->init(BAUD_RATE);

HU90m · 2024-08-15T12:37:00Z

sw/cheri/tests/hyperram_test.cc

+
+  uart->init(921600);
+  set_console_mode(uart, CC_BOLD);
+  write_str(uart, "\r\n\r\nGet hyped for hyperram!\r\n");


HU90m · 2024-08-15T12:37:36Z

sw/cheri/tests/hyperram_test.cc

+  return failures;
+}
+
+int rand_cap_test(Capability<volatile uint32_t> hyperram_area,


nit: some brief comments above these functions would be nice.

GregAC · 2024-08-15T13:47:24Z

Thanks for the reviews so far.

Just done a quick update with the missing lint file and a missing addition to the sonata verilator lint.

rtl/bus/xbar_ifetch.sv

nbdd0121 · 2024-08-15T13:48:51Z

rtl/system/sonata_system.sv

+  tlul_pkg::tl_h2d_t tl_hyperram_us_h2d[2];
+  tlul_pkg::tl_d2h_t tl_hyperram_us_d2h[2];
+  tlul_pkg::tl_h2d_t tl_hyperram_ds_h2d;
+  tlul_pkg::tl_d2h_t tl_hyperram_ds_d2h;


What does us/ds stand for? ds is device-side I guess? what's us?

rtl/system/sonata_system.sv

sw/cheri/tests/hyperram_test.cc

nbdd0121 · 2024-08-15T14:12:00Z

I think one thing that the current test misses is interleaved/randomized write to the HyperRAM with different tag bits, and read back to see if they're expected. I think if the HyperRAM just always return 1 for capability then the current test wouldn't be able to capture that?

nbdd0121 · 2024-08-16T15:25:25Z

When synthesising this I get 3 critical warnings:

[Synth 8-2490] overwriting previous definition of module ws281x_drv ["sonata-system/build/lowrisc_sonata_system_0/src/lowrisc_sonata_system_0/rtl/fpga/ws281x_drv.sv":5]
[Vivado 12-4739] set_false_path:No valid object(s) found for '-from [get_pins -of [get_cells u_sonata_system/g_hyperram.u_hyperram/u_hbmc_tl_top/hbmc_ufifo_inst/u_fifo/*storage*/*]]'. ["sonata-system/build/lowrisc_sonata_system_0/src/lowrisc_sonata_system_0/data/pins_sonata.xdc":310]
[Vivado 12-4739] set_false_path:No valid object(s) found for '-from [get_pins -of [get_cells u_sonata_system/g_hyperram.u_hyperram/u_hbmc_tl_top/hbmc_dfifo_inst/u_fifo/*storage*/*]]'. ["sonata-system/build/lowrisc_sonata_system_0/src/lowrisc_sonata_system_0/data/pins_sonata.xdc":311]

GregAC · 2024-08-19T08:47:40Z

I think one thing that the current test misses is interleaved/randomized write to the HyperRAM with different tag bits, and read back to see if they're expected. I think if the HyperRAM just always return 1 for capability then the current test wouldn't be able to capture that?

I also have a block-level testbench that does full random read/write accesses (including different sizes, not just the 32-bit reads/writes the test here does). This uses a hyperram BFM and was seeing no failures.

It isn't included in this PR as it's a bit of a mess and isn't vital for the v1.0 release. However I do plan to make a PR for it soon.

So we can extend the C test but it's not intended to be a fully comprehensive test, more of a combination of a smoke (i.e. make sure the basics work) and a soak (i.e. keep testing things over and thing to check reliability and hopefully flush out CDC issues) test.

GregAC · 2024-08-19T10:11:10Z

Oh and the block-level testbench does also test capability tags (randomized and modelled along with the memory data)

GregAC · 2024-08-19T11:33:24Z

I believe I've addressed all the comments, thanks for the reviews.

Additionally as discussed (on Zulip I think) hyperram is now enabled by default, though we still have the awkward top half of SRAM missing in that configuration (because we have to define a 256k window for it to allow the no hyperram configuration).

Now to fix lint, I think I need to update the build so lint builds with the hyperram sim model (bit of a hack but verilator cannot build the full hyperram controller right now because it uses Xilinx IP, we can hopefully setup something so we can at least lint it with blackboxes for the Xilinx specific stuff but it's not an immediate priority).

Includes hyperram port for hyperram integration

It is disabled by default for now as it halves the available SRAM.

GregAC · 2024-08-19T12:12:53Z

When synthesising this I get 3 critical warnings:

* `[Synth 8-2490] overwriting previous definition of module ws281x_drv ["sonata-system/build/lowrisc_sonata_system_0/src/lowrisc_sonata_system_0/rtl/fpga/ws281x_drv.sv":5]`

* `[Vivado 12-4739] set_false_path:No valid object(s) found for '-from [get_pins -of [get_cells u_sonata_system/g_hyperram.u_hyperram/u_hbmc_tl_top/hbmc_ufifo_inst/u_fifo/*storage*/*]]'. ["sonata-system/build/lowrisc_sonata_system_0/src/lowrisc_sonata_system_0/data/pins_sonata.xdc":310]`

* `[Vivado 12-4739] set_false_path:No valid object(s) found for '-from [get_pins -of [get_cells u_sonata_system/g_hyperram.u_hyperram/u_hbmc_tl_top/hbmc_dfifo_inst/u_fifo/*storage*/*]]'. ["sonata-system/build/lowrisc_sonata_system_0/src/lowrisc_sonata_system_0/data/pins_sonata.xdc":311]`

,
I'm not too worried about the warnings on the false path thing, I've managed to sort out the timing errors caused by false paths which is good enough for now.

As explained in the XDC ideally we'd have a more general setup for these but I've had a lot of trouble getting it to work. It seems the environment and design for the XDC execution is not the same as the environment and design used when you execute commands in the Vivado interactive TCL shell. So stuff that works just fine doesn't work when included in an XDC.

GregAC added 2 commits August 9, 2024 15:01

Add OpenHBMC (HyperRAM controller) vendor file

0531f77

Update OpenHBMC to OVGN/OpenHBMC@fc1c154

8418fc4

Update code from upstream repository https://github.com/OVGN/OpenHBMC.git to revision fc1c154877d5ac5100d97368e6e656c96e0d6a70 Signed-off-by: Greg Chadwick <gac@lowrisc.org>

GregAC requested review from marnovandermaas, HU90m and alees24 August 9, 2024 14:09

GregAC commented Aug 9, 2024

View reviewed changes

marnovandermaas reviewed Aug 14, 2024

View reviewed changes

HU90m approved these changes Aug 15, 2024

View reviewed changes

GregAC force-pushed the hyperram branch from d82e814 to 78cd799 Compare August 15, 2024 13:46

nbdd0121 reviewed Aug 15, 2024

View reviewed changes

GregAC force-pushed the hyperram branch 2 times, most recently from 0af0e96 to 9d36aa2 Compare August 19, 2024 11:26

GregAC added 4 commits August 19, 2024 12:36

Add hyperram IP block

12f8930

Add new crossbar for Ibex instruction fetch

13554de

Includes hyperram port for hyperram integration

Integrate Hyperram with sonata_system

7e14c79

It is disabled by default for now as it halves the available SRAM.

Add hyperram test

8fb9211

GregAC force-pushed the hyperram branch from 9d36aa2 to 8fb9211 Compare August 19, 2024 11:36

GregAC merged commit ffc0ef8 into lowRISC:main Aug 19, 2024
2 checks passed

GregAC deleted the hyperram branch August 19, 2024 12:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hyperram #149

Hyperram #149

GregAC commented Aug 9, 2024

GregAC commented Aug 9, 2024

GregAC Aug 9, 2024

GregAC commented Aug 9, 2024

marnovandermaas left a comment

marnovandermaas Aug 14, 2024

GregAC Aug 15, 2024

marnovandermaas commented Aug 14, 2024

HU90m left a comment

HU90m Aug 15, 2024

HU90m Aug 15, 2024

HU90m Aug 15, 2024

HU90m Aug 15, 2024

HU90m Aug 15, 2024

HU90m Aug 15, 2024

GregAC commented Aug 15, 2024

nbdd0121 Aug 15, 2024

nbdd0121 commented Aug 15, 2024 •

edited

Loading

nbdd0121 commented Aug 16, 2024

GregAC commented Aug 19, 2024

GregAC commented Aug 19, 2024

GregAC commented Aug 19, 2024

GregAC commented Aug 19, 2024

		const int RandTestBlockSize = 256;
		const int HyperramSize = (1024 * 1024) / 4;

		localparam int unsigned SRAMAddrWidth = $clog2(MemSize);
		localparam int unsigned HRSize = 1024 * 1024; // 1 MiB

	localparam int unsigned HRSize = 1024 * 1024; // 1 MiB
	localparam int unsigned HyperramSize = 1024 * 1024; // 1 MiB

Hyperram #149

Hyperram #149

Conversation

GregAC commented Aug 9, 2024

GregAC commented Aug 9, 2024

Choose a reason for hiding this comment

GregAC commented Aug 9, 2024

marnovandermaas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marnovandermaas commented Aug 14, 2024

HU90m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GregAC commented Aug 15, 2024

Choose a reason for hiding this comment

nbdd0121 commented Aug 15, 2024 • edited Loading

nbdd0121 commented Aug 16, 2024

GregAC commented Aug 19, 2024

GregAC commented Aug 19, 2024

GregAC commented Aug 19, 2024

GregAC commented Aug 19, 2024

nbdd0121 commented Aug 15, 2024 •

edited

Loading