Solve the constant crisis by adding memory map description to circuits #581

hydrolarus · 2024-07-25T12:58:31Z

This implements #481

There's a few noteworthy parts to this PR:

the description of memory maps and their validation (Protocols.MemoryMap/.Check)
the BitPackC type class
the ToFieldType type class
a generator for Rust wrappers based on the memory map firmware-support/memmap-generate

lmbollen

Thanks for all the hard work :) Here's a first short round of review. Could you first add some documentation to the functions you export and to help me better interpret the intentions of the code?

lmbollen · 2024-08-07T12:43:07Z

clash-protocols-memmap/.gitignore

@@ -0,0 +1,6 @@
+# SPDX-FileCopyrightText: 2022 Google LLC


We switched to year of creation copyrights :)

lmbollen · 2024-08-07T12:43:16Z

clash-protocols-memmap/app/Example.hs

@@ -0,0 +1,228 @@
+-- SPDX-FileCopyrightText: 2022-2024 Google LLC


We switched to year of creation copyrights :)

lmbollen · 2024-08-07T14:06:36Z

cabal.project

+  -- flags: -rusty-callisto
+


Was this added by accident?
For development purposes you can create a cabal.project.local

lmbollen · 2024-08-07T14:14:41Z

clash-protocols-memmap/src/Protocols/MemoryMap.hs

+--      be possible to generate an equivalent data structure in Rust or C.
+--
+
+data BackwardAnnotated (annotation :: Type) (a :: Type)


BackAnnotated?
BwdAnnotated?

It's a bit of a mouthful, yeah. We could also go with BwdAnn as the shortest option 😅 Probably somewhere in between is good?

"Back annotation" has a specific meaning in the hardware world: https://www.quora.com/What-is-meant-by-back-annotated-netlist-in-asic-design-flow. Just a heads up. I think BwdAnn is best (though I'm generally a bit meh on abbreviations like this).

lmbollen · 2024-08-07T14:27:13Z

clash-protocols-memmap/src/Protocols/MemoryMap.hs

+  , fieldSize :: Size
+    -- ^ Size of the register in bytes


Could we not derive this from the FieldType?

In theory, yes, but it would involve a bunch of duplicated work. This field is meant to be filled by the BitPackC, I initially added a sizeOf :: FieldType -> Size function but it was pretty complex as it has to encode the same rules as BitPackC does. If we're fine with this duplication then I could add a function like that.

lmbollen · 2024-08-07T14:27:40Z

firmware-binaries/examples/hello/Cargo.toml

@@ -1,4 +1,4 @@
-# SPDX-FileCopyrightText: 2022 Google LLC
+# SPDX-FileCopyrightText: 2024 Google LLC


no need for this anymore :)

lmbollen · 2024-08-07T14:28:54Z

firmware-support/bittide-sys/src/time.rs

+                counter: addr.add(2).cast::<u64>(),
+                frequency: addr.add(4).cast::<u64>(),


Is this related to alignment requirements?

Yes exactly, probably should have gone in a separate commit, it's been a bit messy 🥴

lmbollen · 2024-08-07T14:33:14Z

firmware-support/memmap-generate/src/parse.rs

+                "end_line": 435,
+                "file": "src/Protocols/MemoryMap.hs",
+                "module": "Protocols.MemoryMap",
+                "package": "clash-protocols-memmap-0.1-inplace",
+                "start_col": 15,
+                "start_line": 435
+              }
+            }
+          }
+        },
+        {
+          "relative_address": 2684354560,
+          "size": 536870911,
+          "tree": {
+            "device_instance": {
+              "absolute_address": 2684354560,
+              "device_name": "Timer",
+              "instance_name": "Timer",
+              "src_location": {
+                "end_col": 24,
+                "end_line": 435,
+                "file": "src/Protocols/MemoryMap.hs",
+                "module": "Protocols.MemoryMap",
+                "package": "clash-protocols-memmap-0.1-inplace",
+                "start_col": 15,
+                "start_line": 435
+              }
+            }
+          }
+        },
+        {
+          "relative_address": 3221225472,
+          "size": 536870911,
+          "tree": {
+            "device_instance": {
+              "absolute_address": 3221225472,
+              "device_name": "UART",
+              "instance_name": "UART",
+              "src_location": {
+                "end_col": 60,
+                "end_line": 45,
+                "file": "src/Bittide/Instances/MemoryMapLogic.hs",
+                "module": "Bittide.Instances.MemoryMapLogic",
+                "package": "bittide-instances-0.1-inplace",
+                "start_col": 50,
+                "start_line": 45
+              }
+            }
+          }
+        },
+        {
+          "relative_address": 3758096384,
+          "size": 536870911,
+          "tree": {
+            "device_instance": {
+              "absolute_address": 3758096384,
+              "device_name": "StatusReg",
+              "instance_name": "StatusReg",
+              "src_location": {
+                "end_col": 24,
+                "end_line": 435,
+                "file": "src/Protocols/MemoryMap.hs",
+                "module": "Protocols.MemoryMap",
+                "package": "clash-protocols-memmap-0.1-inplace",
+                "start_col": 15,
+                "start_line": 435
+              }
+            }
+          }
+        }
+      ],
+      "src_location": {
+        "end_col": 48,
+        "end_line": 233,
+        "file": "src/Bittide/ProcessingElement.hs",
+        "module": "Bittide.ProcessingElement",
+        "package": "bittide-0.1-inplace",
+        "start_col": 23,
+        "start_line": 233
+      }
+    }
+  }
+}
+
+"#;
+
+    #[test]
+    fn register_access() {
+        let src = "\"read_only\"";
+        let x: RegisterAccess = serde_json::from_str(src).unwrap();
+        assert_eq!(x, RegisterAccess::ReadOnly);
+    }
+
+    #[test]
+    fn test_deserialise_memmap() {
+        let memmap: MemoryMapDesc = serde_json::from_str(TEST_SRC).unwrap();
+
+        dbg!(&memmap);
+    }
+}


Could we use an actual generated description from a clash design instead of hardcoding this?

lmbollen · 2024-08-07T14:54:53Z

firmware-binaries/examples/hello/src/main.rs

+        UART: mut uart,
+        StatusReg: status,


Am I correct in understanding that DeviceInstances is used to auto generated components at compile time (e.g. UART based on the json description, and we automatically get generate the API (e.g. data based on the field and set_data to write?)

Yes exactly, DeviceInstances contains all the different accessible components specified in the memory map! Every device type gets a Rust wrapper and every register/field of a device gets the appropriate getter and setter.

martijnbastiaan

I think this looks good in general! Let's replace all error "shouldn't happen" with breadcrumbs. The more we introduce the more likely the unexpected is expected to happen.

martijnbastiaan · 2024-11-07T15:27:43Z

bittide-instances/src/Bittide/Instances/Hitl/VexRiscv.hs

+-- ╭────────┬───────┬───────┬────────────────────────────────────╮
+-- │ bin    │ hex   │ bus   │ description                        │
+-- ├────────┼───────┼───────┼────────────────────────────────────┤
+-- │ 0b000. │ 0x0   │       │                                    │
+-- │ 0b001. │ 0x2   │       │                                    │
+-- │ 0b010. │ 0x4   │ 1     │ Data memory                        │
+-- │ 0b011. │ 0x6   │       │                                    │
+-- │ 0b100. │ 0x8   │ 0     │ Instruction memory                 │
+-- │ 0b101. │ 0xA   │ 2     │ Time                               │
+-- │ 0b110. │ 0xC   │ 3     │ UART                               │
+-- │ 0b111. │ 0xE   │ 4     │ Test status register               │
+-- ╰────────┴───────┴───────┴────────────────────────────────────╯


I think we can delete this now :)

martijnbastiaan · 2024-11-07T15:30:21Z

bittide-instances/src/Bittide/Instances/Hitl/VexRiscv.hs

+      DeviceDefinition
+        { deviceName = Name{name = "StatusReg", description = ""}
+        , registers = [(statusRegName, defLoc, statusRegDesc)]
+        , defLocation = defLoc


Would it be possible to hide this away in a smart constructor deviceDefinition?

martijnbastiaan · 2024-11-07T15:33:25Z

clash-protocols-memmap/src/BitPackC.hs

+{- | Typeclass that can be implemented (or derived) to enable C-FFI safe
+representation of Haskell types.
+
+This can be derived automatically as long as 'Generic' is implemented for the
+type.
+Currently there is a high likelihood that more complex types need an unusual
+amount of constraints on the instance. Ideally this won't be necessary when
+@ghc-typelits-extra@ gets extended to automatically discharge those
+constraints.


Could you add a few words on why this is more difficult than just BitPack? I.e., you need alignment --- what does that mean.

martijnbastiaan · 2024-11-07T15:41:59Z

clash-protocols-memmap/src/BitPackC.hs

+  ( BitPackC a
+  , BitPackC b
+  , BitPackC c
+  , 1 <= Max (AlignmentC a) (AlignmentC b)
+  , 1 <= Max (AlignmentC b) (AlignmentC c)
+  , 1 <= Max (AlignmentC a) (Max (AlignmentC b) (AlignmentC c))
+  , Mod (ByteSizeC a) (AlignmentC b) <= AlignmentC b
+  , Mod (ByteSizeC a) (AlignmentC b) <= ByteSizeC a
+  , Mod (ByteSizeC b) (AlignmentC c) <= AlignmentC c
+  , Mod (ByteSizeC b) (AlignmentC c) <= ByteSizeC b
+  , Mod (ByteSizeC a) (Max (AlignmentC b) (AlignmentC c))
+      <= Max (AlignmentC b) (AlignmentC c)
+  , Mod (ByteSizeC a) (Max (AlignmentC b) (AlignmentC c))
+      <= ByteSizeC a


martijnbastiaan · 2024-11-07T15:43:15Z

clash-protocols-memmap/src/Protocols/MemoryMap.hs

+type Size = Integer
+
+{- | A protocol agnostic wrapper for memory mapped protocols. It provides a way
+for memory mapped subordinates to propagate their memory map to the top level,


Suggested change

for memory mapped subordinates to propagate their memory map to the top level,

for memory mapped devices to propagate their memory map to the top level,

You call them devices elsewhere right?

martijnbastiaan · 2024-11-13T10:51:52Z

clash-protocols-memmap/src/Protocols/MemoryMap/Check/AbsAddress.hs

+validateAbsAddresses ::
+  ComponentPath ->


Suggested change

validateAbsAddresses ::

ComponentPath ->

validateAbsAddresses ::

HasCallStack =>

ComponentPath ->

Given that you're using error here.

martijnbastiaan · 2024-11-13T10:52:52Z

clash-protocols-memmap/src/Protocols/MemoryMap/Check/Overlap.hs

+        }
+      | interconnectSize > availableSize
+      ]
+    componentErrors = flip map comps2 $ \(path', (addr', size', comp)) -> walk path' (start + addr', start + addr' + size') comp


Suggested change

componentErrors = flip map comps2 $ \(path', (addr', size', comp)) -> walk path' (start + addr', start + addr' + size') comp

componentErrors = flip map comps2 $ \(path', (addr', size', comp)) ->

walk path' (start + addr', start + addr' + size') comp

martijnbastiaan · 2024-11-13T10:54:09Z

clash-protocols-memmap/src/Protocols/MemoryMap/FieldType.hs

+  toFieldType :: FieldType
+
+  args :: Vec (Generics a) FieldType
+  args = repeat undefined


Suggested change

args = repeat undefined

args = repeat (error "something something")

martijnbastiaan · 2024-11-13T10:54:55Z

clash-protocols-memmap/src/Protocols/MemoryMap/FieldType.hs

+    (Generic (WithVars a), GToFieldType (Rep (WithVars a))) => FieldType
+  toFieldType = case (gToFieldType @(Rep (WithVars a)), toList (args @a)) of
+    (ft, []) -> TypeReference ft []
+    (TypeReference _ft _args', _args'') -> error "shouldn't happen!??"


Suggested change

(TypeReference _ft _args', _args'') -> error "shouldn't happen!??"

(TypeReference _ft _args', _args'') -> error "toFieldType: TypeReference: unexpected constructor"

martijnbastiaan · 2024-11-13T10:56:45Z

firmware-support/memmap-generate/src/generators/mod.rs

+    // TODO handle things like CamelCase/snake_case conversion?
+    let s = match n.as_ref() {
+        "(,)" => "Pair",
+        "(,,)" => "Thruple",


Suggested change

"(,,)" => "Thruple",

"(,,)" => "Thruple",

Haha, haven't seen that one before! Triple?

hydrolarus force-pushed the memory-map branch from 6cfcdb8 to 33cd0ed Compare July 29, 2024 11:35

martijnbastiaan mentioned this pull request Aug 7, 2024

Add support for device tree specification embeddings #315

Closed

1 task

lmbollen requested changes Aug 7, 2024

View reviewed changes

hydrolarus force-pushed the memory-map branch from 1635553 to 20c96e5 Compare August 12, 2024 10:54

hydrolarus force-pushed the memory-map branch from 20c96e5 to 84d3e88 Compare August 22, 2024 09:37

hydrolarus added 5 commits August 26, 2024 11:15

add clash-protocols-memmap

17abeb4

add memory mapped version of first components

eae6799

Clean up

fb3077c

dynamically create memory.x based on memory map

065862f

add circuit versions of scatter- and gatherUnit

c3bc778

hydrolarus force-pushed the memory-map branch from 84d3e88 to c3bc778 Compare August 30, 2024 14:46

hydrolarus requested a review from rslawson October 30, 2024 15:05

martijnbastiaan approved these changes Nov 13, 2024

View reviewed changes

martijnbastiaan added the 3 Review/merge third label Nov 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solve the constant crisis by adding memory map description to circuits #581

Solve the constant crisis by adding memory map description to circuits #581

hydrolarus commented Jul 25, 2024

lmbollen left a comment

lmbollen Aug 7, 2024

lmbollen Aug 7, 2024

lmbollen Aug 7, 2024

lmbollen Aug 7, 2024

hydrolarus Aug 7, 2024

martijnbastiaan Nov 6, 2024

lmbollen Aug 7, 2024

hydrolarus Aug 7, 2024

lmbollen Aug 7, 2024

lmbollen Aug 7, 2024

hydrolarus Aug 7, 2024

lmbollen Aug 7, 2024

lmbollen Aug 7, 2024

hydrolarus Aug 7, 2024

martijnbastiaan left a comment

martijnbastiaan Nov 7, 2024

martijnbastiaan Nov 7, 2024

martijnbastiaan Nov 7, 2024

martijnbastiaan Nov 7, 2024

martijnbastiaan Nov 7, 2024

martijnbastiaan Nov 13, 2024

martijnbastiaan Nov 13, 2024

martijnbastiaan Nov 13, 2024

martijnbastiaan Nov 13, 2024

martijnbastiaan Nov 13, 2024

		@@ -0,0 +1,228 @@
		-- SPDX-FileCopyrightText: 2022-2024 Google LLC

		@@ -1,4 +1,4 @@
		# SPDX-FileCopyrightText: 2022 Google LLC
		# SPDX-FileCopyrightText: 2024 Google LLC

		counter: addr.add(2).cast::<u64>(),
		frequency: addr.add(4).cast::<u64>(),

	for memory mapped subordinates to propagate their memory map to the top level,
	for memory mapped devices to propagate their memory map to the top level,

	componentErrors = flip map comps2 $ \(path', (addr', size', comp)) -> walk path' (start + addr', start + addr' + size') comp
	componentErrors = flip map comps2 $ \(path', (addr', size', comp)) ->
	walk path' (start + addr', start + addr' + size') comp

	args = repeat undefined
	args = repeat (error "something something")

	(TypeReference _ft _args', _args'') -> error "shouldn't happen!??"
	(TypeReference _ft _args', _args'') -> error "toFieldType: TypeReference: unexpected constructor"

Solve the constant crisis by adding memory map description to circuits #581

Are you sure you want to change the base?

Solve the constant crisis by adding memory map description to circuits #581

Conversation

hydrolarus commented Jul 25, 2024

lmbollen 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

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

Choose a reason for hiding this comment

martijnbastiaan 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

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