frontend/adaptation: clean up LiteDRAMNativePortUpConverter code

litedram/frontend/adaptation.py

@@ -139,6 +139,9 @@ class LiteDRAMNativePortUpConverter(Module):
     (when possible, ie when consecutive and bursting)
     - N writes from user are regrouped in a single one to the controller
     (when possible, ie when consecutive and bursting)
+    Incomplete writes/reads (i.e. with n < N) are handled automatically in the
+    middle of a burst, but last command has to use cmd.last=1 if the last burst
+    is not complete (not all N addresses have been used).
     """
     def __init__(self, port_from, port_to, reverse=False):
         assert port_from.clock_domain == port_to.clock_domain
@@ -173,6 +176,7 @@ class LiteDRAMNativePortUpConverter(Module):
         rw_collision     = Signal()
 
         self.comb += [
+            cmd_buffer.source.ready.eq(wdata_finished | rdata_finished),
             addr_changed.eq(cmd_addr[log2_int(ratio):] != port_from.cmd.addr[log2_int(ratio):]),
             # collision happens on write to read transition when address does not change
             rw_collision.eq(cmd_we & (port_from.cmd.valid & ~port_from.cmd.we) & ~addr_changed),
@@ -182,8 +186,8 @@ class LiteDRAMNativePortUpConverter(Module):
             #  * we received all the `ratio` commands
             #  * this is the last command in a sequence
             next_cmd.eq(addr_changed | (cmd_we != port_from.cmd.we) | (sel == 2**ratio - 1) | cmd_last),
-            # when the first command is received, send it immediatelly
             If(sel == 0,
+                # when the first command is received, send it immediatelly
                 If(port_from.cmd.valid & ~read_lock,
                     port_to.cmd.valid.eq(1),
                     port_to.cmd.we.eq(port_from.cmd.we),
@@ -192,7 +196,7 @@ class LiteDRAMNativePortUpConverter(Module):
                 )
             ).Else(
                 # we have already sent the initial command, now either continue sending cmd.ready
-                # to the master or send the current command if we have to go to next command
+                # to the master or send the current command if we have to go to next one faster
                 If(next_cmd,
                     cmd_buffer.sink.valid.eq(1),
                     cmd_buffer.sink.sel.eq(sel),
@@ -201,11 +205,10 @@ class LiteDRAMNativePortUpConverter(Module):
                     port_from.cmd.ready.eq(port_from.cmd.valid),
                 )
             ),
-            cmd_buffer.source.ready.eq(wdata_finished | rdata_finished)
         ]
 
         self.sync += [
-            # whenever a command gets accepted, update `sel` bitmask and store the command info
+            # whenever a command gets accepted, store it and update `sel` based on address
             If(port_from.cmd.valid & port_from.cmd.ready,
                 cmd_addr.eq(port_from.cmd.addr),
                 cmd_we.eq(port_from.cmd.we),
@@ -227,19 +230,17 @@ class LiteDRAMNativePortUpConverter(Module):
         # Read Datapath ----------------------------------------------------------------------------
 
         if mode == "read" or mode == "both":
-            # buffers output from port_to
+            # queue received data not to loose it when it comes too fast
-            rdata_fifo = stream.SyncFIFO(port_to.rdata.description, ratio)
+            rdata_fifo = stream.SyncFIFO(port_to.rdata.description, ratio - 1)
-            # connected to the buffered output
             rdata_converter = stream.StrideConverter(
                 port_to.rdata.description,
                 port_from.rdata.description,
                 reverse=reverse)
             self.submodules +=  rdata_fifo, rdata_converter
 
-            # bitmask shift register with single 1 bit and all other 0s
+            # shift register with a bitmask of current chunk
             rdata_chunk       = Signal(ratio, reset=1)
             rdata_chunk_valid = Signal()
-            # whenever the converter spits data chunk we shift the chunk bitmask
             self.sync += \
                 If(rdata_converter.source.valid &
                    rdata_converter.source.ready,
@@ -247,84 +248,75 @@ class LiteDRAMNativePortUpConverter(Module):
                 )
 
             self.comb += [
-                # port_to -> rdata_fifo -> rdata_converter
+                # port_to -> rdata_fifo -> rdata_converter -> port_from
                 port_to.rdata.connect(rdata_fifo.sink),
                 rdata_fifo.source.connect(rdata_converter.sink),
-                # chunk is valid if it's bit is in `sel` sent previously to the FIFO
                 rdata_chunk_valid.eq((cmd_buffer.source.sel & rdata_chunk) != 0),
-                # whenever `sel` from FIFO is valid
                 If(cmd_buffer.source.valid & ~cmd_buffer.source.we,
-                    # if that chunk is valid we send it to the user port and wait for ready from user
+                   # if that chunk is valid we send it to the user port and wait for ready
                     If(rdata_chunk_valid,
                         port_from.rdata.valid.eq(rdata_converter.source.valid),
                         port_from.rdata.data.eq(rdata_converter.source.data),
                         rdata_converter.source.ready.eq(port_from.rdata.ready)
-                    # if this was not requested by `sel` then we just ack it
+                    ).Else(  # if this chunk was not requested in `sel`, ignore it
-                    ).Else(
                         rdata_converter.source.ready.eq(1)
                     ),
-                    rdata_finished.eq(rdata_converter.source.valid & rdata_converter.source.ready & rdata_chunk[ratio - 1])
+                    rdata_finished.eq(rdata_converter.source.valid & rdata_converter.source.ready
+                                      & rdata_chunk[ratio - 1])
                 ),
             ]
 
         # Write Datapath ---------------------------------------------------------------------------
 
         if mode == "write" or mode == "both":
-            wdata_fifo    = stream.SyncFIFO(port_from.wdata.description, ratio)
+            # queue write data not to miss it when the lower chunks haven't been reqested
+            wdata_fifo    = stream.SyncFIFO(port_from.wdata.description, ratio - 1)
             wdata_converter = stream.StrideConverter(
                 port_from.wdata.description,
                 port_to.wdata.description,
                 reverse=reverse)
             self.submodules += wdata_converter, wdata_fifo
 
-            # bitmask shift register with single 1 bit and all other 0s
+            # shift register with a bitmask of current chunk
             wdata_chunk       = Signal(ratio, reset=1)
             wdata_chunk_valid = Signal()
-            # whenever the converter spits data chunk we shift the chunk bitmask
             self.sync += \
                 If(wdata_converter.sink.valid & wdata_converter.sink.ready,
                     wdata_chunk.eq(Cat(wdata_chunk[ratio-1], wdata_chunk[:ratio-1]))
                 )
 
-            # replicate sel so that each bit covers according part of we bitmask (1 sel bit may cover
+            # replicate `sel` bits to match the width of port_to.wdata.we
-            # multiple bytes)
             wdata_sel = Signal.like(port_to.wdata.we)
-            #  self.comb += wdata_sel.eq(
+            wdata_sel_parts = [
-            #      Cat([Replicate(cmd_buffer.source.sel[i], port_to.wdata.we.nbits // sel.nbits) for i in range(ratio)])
+                Replicate(cmd_buffer.source.sel[i], port_to.wdata.we.nbits // sel.nbits)
-            #  )
+                for i in range(ratio)
-
-            self.sync += [
-                If(cmd_buffer.source.valid & cmd_buffer.source.we & wdata_chunk[ratio - 1],
-                    wdata_sel.eq(Cat([Replicate(cmd_buffer.source.sel[i], port_to.wdata.we.nbits // sel.nbits)
-                                      for i in range(ratio)]))
-                )
             ]
+            self.sync += \
+                If(cmd_buffer.source.valid & cmd_buffer.source.we & wdata_chunk[ratio - 1],
+                    wdata_sel.eq(Cat(wdata_sel_parts))
+                )
 
             self.comb += [
+                # port_from -> wdata_fifo -> wdata_converter
                 port_from.wdata.connect(wdata_fifo.sink),
-
                 wdata_chunk_valid.eq((cmd_buffer.source.sel & wdata_chunk) != 0),
-
                 If(cmd_buffer.source.valid & cmd_buffer.source.we,
+                    # when the current chunk is valid, read it from wdata_fifo
                     If(wdata_chunk_valid,
                         wdata_converter.sink.valid.eq(wdata_fifo.source.valid),
                         wdata_converter.sink.data.eq(wdata_fifo.source.data),
                         wdata_converter.sink.we.eq(wdata_fifo.source.we),
                         wdata_fifo.source.ready.eq(1),
-                    ).Else(
+                    ).Else(  # if chunk is not valid, send any data and do not advance fifo
                         wdata_converter.sink.valid.eq(1),
-                        wdata_converter.sink.data.eq(0),
-                        wdata_converter.sink.we.eq(0),
-                        wdata_fifo.source.ready.eq(0),
                     ),
                 ),
-
                 port_to.wdata.valid.eq(wdata_converter.source.valid),
                 port_to.wdata.data.eq(wdata_converter.source.data),
                 port_to.wdata.we.eq(wdata_converter.source.we & wdata_sel),
                 wdata_converter.source.ready.eq(port_to.wdata.ready),
-                #  wdata_finished.eq(wdata_converter.source.valid & wdata_converter.source.ready),
+                wdata_finished.eq(wdata_converter.sink.valid & wdata_converter.sink.ready
-                wdata_finished.eq(wdata_converter.sink.valid & wdata_converter.sink.ready & wdata_chunk[ratio-1]),
+                                  & wdata_chunk[ratio-1]),
             ]
 
 # LiteDRAMNativePortConverter ----------------------------------------------------------------------

test/test_adaptation.py

@@ -77,13 +77,13 @@ class CDCDUT(ConverterDUT):
 
 
 class TestAdaptation(MemoryTestDataMixin, unittest.TestCase):
-    def test_converter_down_ratio_must_be_integer(self):
+    def test_down_converter_ratio_must_be_integer(self):
         with self.assertRaises(ValueError) as cm:
             dut = ConverterDUT(user_data_width=64, native_data_width=24, mem_depth=128)
             dut.finalize()
         self.assertIn("ratio must be an int", str(cm.exception).lower())
 
-    def test_converter_up_ratio_must_be_integer(self):
+    def test_up_converter_ratio_must_be_integer(self):
         with self.assertRaises(ValueError) as cm:
             dut = ConverterDUT(user_data_width=32, native_data_width=48, mem_depth=128)
             dut.finalize()
@@ -155,7 +155,7 @@ class TestAdaptation(MemoryTestDataMixin, unittest.TestCase):
         # Verify 32-bit to 256-bit up-conversion.
         self.converter_test(test_data="32bit_to_256bit", user_data_width=32, native_data_width=256)
 
-    def test_converter_up_read_latencies(self):
+    def test_up_converter_read_latencies(self):
         # Verify that up-conversion works with different port reader latencies
         cases = {
             "1to2": dict(test_data="8bit_to_16bit",   user_data_width=8,  native_data_width=16),
@@ -168,7 +168,7 @@ class TestAdaptation(MemoryTestDataMixin, unittest.TestCase):
                     with self.subTest(conversion=conversion):
                         self.converter_test(**kwargs, read_latency=latency)
 
-    def test_converter_down_read_latencies(self):
+    def test_down_converter_read_latencies(self):
         # Verify that down-conversion works with different port reader latencies
         cases = {
             "2to1": dict(test_data="64bit_to_32bit", user_data_width=64, native_data_width=32),
@@ -330,7 +330,7 @@ class TestAdaptation(MemoryTestDataMixin, unittest.TestCase):
                 self.converter_readback_test(dut, pattern=[], mem_expected=mem_expected,
                                              main_generator=main_generator)
 
-    def test_converter_up_not_aligned(self):
+    def test_up_converter_not_aligned(self):
         data = self.pattern_test_data["8bit_to_32bit_not_aligned"]
         dut  = ConverterDUT(user_data_width=8, native_data_width=32,
                             mem_depth=len(data["expected"]), separate_rw=False)