Add Wishbone wrapper for the True Dual Port block ram, issue #472 #535

Losiek · 2024-05-30T10:29:25Z

This PR contains a thin wishbone wrapper around Xilinx true dual-port block RAM from `clash-cores`.

True dual-port memory block RAM provides two separate access points to the same memory array, allowing for simultaneous and independent read/write operations, which enhances data throughput and flexibility in various high-performance applications. More on the TDPBRAM: PG058

The Wishbone protocol is a simple and flexible bus interconnect standard for connecting various components within a System-on-Chip (SoC). It supports standard signaling, master-slave communication, and efficient data transfer. More information about the protocol can be found here: Wishbone B4

This wrapper only supports classic Wishbone operations without pipelining and burst transfers.

google-cla · 2024-05-30T10:29:30Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

martijnbastiaan · 2024-05-30T11:01:19Z

@Losiek You have to commit with your QBayLogic email for the CLA to pass

lmbollen

First round of review. ping me if you want to discuss the points :)

lmbollen · 2024-07-03T07:39:36Z

bittide/src/Bittide/DoubleBufferedRam.hs

@@ -9,10 +9,12 @@
 {-# LANGUAGE NamedFieldPuns #-}
 {-# LANGUAGE RecordWildCards #-}
 {-# LANGUAGE TypeApplications #-}
+{-# LANGUAGE NoMonomorphismRestriction #-}


I htink you shouldn't need this, do you get an error when removing this pragma

When I was developing this wrapper, I encountered type issues. Adding this pragma helped. However, after rerunning the tests without it, it seems that it is no longer needed.

lmbollen · 2024-07-03T07:48:01Z

bittide/src/Bittide/DoubleBufferedRam.hs

+        addrBitToIndex :: forall dom n . (KnownDomain dom, KnownNat n) =>
+            Signal dom (WishboneM2S n 4 (Bytes 4)) -> Signal dom (Index (2 ^ n))
+        addrBitToIndex wbM2S = bv2i . addr <$> wbM2S
+        writeDataWb :: forall dom . (KnownDomain dom) =>
+            Signal dom (WishboneM2S nAddrs 4 (Bytes 4)) -> Signal dom (BitVector 32)
+        writeDataWb wbM2S = writeData <$> wbM2S


I don't think the mental overhead of new functions and their types are justified here.
I think you can just write bv2i . addr <$> aM2S and bv2i . addr <$> bM2S instead.
Same goes for writeData <$> aM2S and writeData <$> bM2S.

In the beginning, that was helpful for me. Now I see that it's a little bit noisy, so I will change it.

lmbollen · 2024-07-03T07:51:02Z

bittide/src/Bittide/DoubleBufferedRam.hs

+        byteEna :: forall dom . (KnownDomain dom) =>
+            Signal dom (WishboneM2S nAddrs 4 (Bytes 4)) -> Signal dom (BitVector 4)
+        byteEna wbM2S = (\wb -> if writeEnable wb then busSelect wb else 0 :: BitVector 4) <$> wbM2S
+        enableWb :: forall dom . (KnownDomain dom) =>
+            Signal dom (WishboneM2S nAddrs 4 (Bytes 4)) -> Enable dom
+        enableWb wbM2S = toEnable $ (\wb -> busCycle wb && strobe wb) <$> wbM2S


I think you should be able to omit the types here without them becoming ambiguous.
Types are nice when transformations are ambiguous (typechecker complains) or when they are not immediately obvious from the implementation.

In this case I feel like they are pretty obvious because we know byte enables are BitVector n (size will get derived from wbM2S).

We can also immediately tell from the implementation that we transform a wbM2S to an Enable dom.

Agree, I will remove types.

lmbollen · 2024-07-03T08:09:02Z