memory: fix address translation of resources located in dense windows.

As a breaking change, MemoryMap.add_window(self, window, sparse=False)
will fail if `self.data_width // window.data_width` isn't a power-of-2
or is greater than `2 ** window.alignment`.

Author: jfng

amaranth_soc/memory.py

@@ -441,22 +441,29 @@ def add_window(self, window, *, addr=None, sparse=None):
         ----------
         :exc:`ValueError`
             If the memory map is frozen.
-        :exc:`TypeError`
-            If the added memory map is not a :class:`MemoryMap`.
         :exc:`ValueError`
             If the requested address and size, after alignment, would overlap with any resources or
             windows that have already been added, or would be out of bounds.
         :exc:`ValueError`
-            If the added memory map has a wider datapath than this memory map.
+            If ``window.data_width`` is wider than :attr:`data_width`.
+        :exc:`ValueError`
+            If the address translation mode is unspecified and ``window.data_width`` is different
+            than :attr:`data_width`.
+        :exc:`ValueError`
+            If dense address translation is used and :attr:`data_width` is not an integer multiple
+            of ``window.data_width``.
         :exc:`ValueError`
-            If dense address translation is used and the datapath width of this memory map is not
-            an integer multiple of the datapath of the added memory map.
+            If dense address translation is used and the ratio of :attr:`data_width` to
+            ``window.data_width`` is not a power of 2.
         :exc:`ValueError`
-            If the name of the added memory map conflicts with the name of other resources or
-            windows present in this memory map.
+            If dense address translation is used and the ratio of :attr:`data_width` to
+            ``window.data_width`` is lesser than 2 raised to the power of :attr:`alignment`.
         :exc:`ValueError`
-            If the added memory map has no name, and the name of one of its resources or windows
-            conflicts with the name of others present in this memory map.
+            If the requested name would conflict with the name of other resources or windows that
+            have already been added.
+        :exc:`ValueError`
+            If ``window`` is anonymous and the name of one of its resources or windows would
+            conflict with the name of any resources or windows that have already been added.
         """
         if not isinstance(window, MemoryMap):
             raise TypeError(f"Window must be a MemoryMap, not {window!r}")
@@ -494,6 +501,17 @@ def add_window(self, window, *, addr=None, sparse=None):
             ratio = self.data_width // window.data_width
         else:
             ratio = 1
+
+        if ratio & (ratio - 1) != 0:
+            raise ValueError(f"Dense addressing cannot be used because the ratio {ratio} of the "
+                             f"memory map data width {self.data_width} to the window data width "
+                             f"{window.data_width} is not a power-of-2")
+        if ratio > (1 << window.alignment):
+            raise ValueError(f"Dense addressing cannot be used because the ratio {ratio} of the "
+                             f"memory map data width {self.data_width} to the window data width "
+                             f"{window.data_width} is greater than the window alignment "
+                             f"{1 << window.alignment}")
+
         size = (1 << window.addr_width) // ratio
         # For resources, the alignment argument of add_resource() affects both address and size
         # of the resource; aligning only the address should be done using align_to(). For windows,
@@ -556,14 +574,18 @@ def window_patterns(self):
 
     @staticmethod
     def _translate(resource_info, window, window_range):
+        # When a resource is accessed through a dense window, its size and address on the wider
+        # bus must be integer multiples of the number of addresses on the narrower bus that are
+        # accessed for each transaction.
         assert (resource_info.end - resource_info.start) % window_range.step == 0
+        assert resource_info.start % window_range.step == 0
         # Accessing a resource through a dense and then a sparse window results in very strange
         # layouts that cannot be easily represented, so reject those.
         assert window_range.step == 1 or resource_info.width == window.data_width
 
         path  = resource_info.path if window.name is None else (window.name, *resource_info.path)
         size  = (resource_info.end - resource_info.start) // window_range.step
-        start = resource_info.start + window_range.start
+        start = (resource_info.start // window_range.step) + window_range.start
         width = resource_info.width * window_range.step
         return ResourceInfo(resource_info.resource, path, start, start + size, width)
 
@@ -642,7 +664,7 @@ def decode_address(self, address):
             return assignment
         elif id(assignment) in self._windows:
             _, addr_range = self._windows[id(assignment)]
-            return assignment.decode_address((address - addr_range.start) // addr_range.step)
+            return assignment.decode_address((address - addr_range.start) * addr_range.step)
         else:
             assert False # :nocov:
 

tests/test_memory.py

@@ -399,7 +399,7 @@ def test_add_window_sparse(self):
 
     def test_add_window_dense(self):
         memory_map = MemoryMap(addr_width=16, data_width=32)
-        self.assertEqual(memory_map.add_window(MemoryMap(addr_width=10, data_width=8),
+        self.assertEqual(memory_map.add_window(MemoryMap(addr_width=10, data_width=8, alignment=2),
                                                sparse=False),
                          (0, 0x100, 4))
 
@@ -430,13 +430,28 @@ def test_add_window_wrong_no_mode(self):
                 r"a window with data width 8 to a memory map with data width 16"):
             memory_map.add_window(MemoryMap(addr_width=10, data_width=8))
 
-    def test_add_window_wrong_ratio(self):
+    def test_add_window_wrong_ratio_multiple(self):
         memory_map = MemoryMap(addr_width=16, data_width=16)
         with self.assertRaisesRegex(ValueError,
                 r"Dense addressing cannot be used because the memory map data width "
                 r"16 is not an integer multiple of window data width 7"):
             memory_map.add_window(MemoryMap(addr_width=10, data_width=7), sparse=False)
 
+    def test_add_window_wrong_ratio_pow2(self):
+        memory_map = MemoryMap(addr_width=16, data_width=24)
+        with self.assertRaisesRegex(ValueError,
+                r"Dense addressing cannot be used because the ratio 3 of the memory map "
+                r"data width 24 to the window data width 8 is not a power-of-2"):
+            memory_map.add_window(MemoryMap(addr_width=10, data_width=8), sparse=False)
+
+    def test_add_window_wrong_ratio_alignment(self):
+        memory_map = MemoryMap(addr_width=16, data_width=16)
+        with self.assertRaisesRegex(ValueError,
+                r"Dense addressing cannot be used because the ratio 2 of the memory map "
+                r"data width 16 to the window data width 8 is greater than the window "
+                r"alignment 1"):
+            memory_map.add_window(MemoryMap(addr_width=10, data_width=8), sparse=False)
+
     def test_add_window_wrong_out_of_bounds(self):
         memory_map = MemoryMap(addr_width=16, data_width=8)
         with self.assertRaisesRegex(ValueError,
@@ -489,9 +504,9 @@ def test_add_window_wrong_name_conflict_subordinate(self):
 
     def test_iter_windows(self):
         memory_map = MemoryMap(addr_width=16, data_width=16)
-        window_1 = MemoryMap(addr_width=10, data_width=8)
+        window_1 = MemoryMap(addr_width=10, data_width=8, alignment=1)
         window_2 = MemoryMap(addr_width=12, data_width=16)
-        window_3 = MemoryMap(addr_width=10, data_width=8)
+        window_3 = MemoryMap(addr_width=10, data_width=8, alignment=1)
         memory_map.add_window(window_1, sparse=False)
         memory_map.add_window(window_2)
         memory_map.add_window(window_3, sparse=False, addr=0x400)
@@ -503,9 +518,9 @@ def test_iter_windows(self):
 
     def test_iter_window_patterns(self):
         memory_map = MemoryMap(addr_width=16, data_width=16)
-        window_1 = MemoryMap(addr_width=10, data_width=8)
+        window_1 = MemoryMap(addr_width=10, data_width=8, alignment=1)
         window_2 = MemoryMap(addr_width=12, data_width=16)
-        window_3 = MemoryMap(addr_width=10, data_width=8)
+        window_3 = MemoryMap(addr_width=10, data_width=8, alignment=1)
         memory_map.add_window(window_1, sparse=False)
         memory_map.add_window(window_2)
         memory_map.add_window(window_3, sparse=False, addr=0x400)
@@ -555,9 +570,11 @@ def setUp(self):
         self.res5 = _MockResource("res5")
         self.win2.add_resource(self.res5, name=("name5",), size=16)
         self.root.add_window(self.win2, sparse=True)
-        self.win3 = MemoryMap(addr_width=16, data_width=8, name="win3")
+        self.win3 = MemoryMap(addr_width=16, data_width=8, alignment=2, name="win3")
         self.res6 = _MockResource("res6")
+        self.res7 = _MockResource("res7")
         self.win3.add_resource(self.res6, name=("name6",), size=16)
+        self.win3.add_resource(self.res7, name=("name7",), size=1)
         self.root.add_window(self.win3, sparse=False)
 
     def test_iter_all_resources(self):
@@ -599,6 +616,12 @@ def test_iter_all_resources(self):
         self.assertEqual(res_info[5].end,   0x00040004)
         self.assertEqual(res_info[5].width, 32)
 
+        self.assertIs(res_info[6].resource, self.res7)
+        self.assertEqual(res_info[6].path,  ("win3", ("name7",)))
+        self.assertEqual(res_info[6].start, 0x00040004)
+        self.assertEqual(res_info[6].end,   0x00040005)
+        self.assertEqual(res_info[6].width, 32)
+
     def test_find_resource(self):
         for res_info in self.root.all_resources():
             other = self.root.find_resource(res_info.resource)

tests/test_wishbone_bus.py

@@ -326,7 +326,7 @@ def elaborate(self, platform):
         self.assertEqual(dut.add(loop_1.bus, addr=0x10000),
                          (0x10000, 0x10100, 1))
         loop_2 = AddressLoopback(addr_width=6, data_width=32, granularity=8)
-        loop_2.bus.memory_map = MemoryMap(addr_width=8, data_width=8)
+        loop_2.bus.memory_map = MemoryMap(addr_width=8, data_width=8, alignment=1)
         self.assertEqual(dut.add(loop_2.bus, addr=0x20000),
                          (0x20000, 0x20080, 2))
         loop_3 = AddressLoopback(addr_width=8, data_width=16, granularity=16)